This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 1/2] MIPS: Compressed PLT/stubs support


"Maciej W. Rozycki" <macro@codesourcery.com> writes:
> On Wed, 20 Feb 2013, Richard Sandiford wrote:
>
>> > @@ -9905,21 +10306,39 @@ _bfd_mips_elf_finish_dynamic_symbol (bfd
>> >  	 binary where pointer equality matters.  */
>> >        sym->st_shndx = SHN_UNDEF;
>> >        if (h->pointer_equality_needed)
>> > -	sym->st_other = STO_MIPS_PLT;
>> > +	{
>> > +	  if (ELF_ST_IS_MIPS16 (sym->st_other))
>> > +	    sym->st_other
>> > +	      = ELF_ST_SET_MIPS16 (ELF_ST_SET_MIPS_PLT (sym->st_other));
>> > +	  else
>> > +	    sym->st_other = ELF_ST_SET_MIPS_PLT (sym->st_other);
>> > +	}
>> 
>> Please update the definition of ELF_ST_SET_MIPS_PLT instead, so that
>> STO_MIPS16 gets preserved in the same way as STO_MICROMIPS.
>
>  Obviously this is a preexisting problem [...]

I disagree.  As things stand, all PLTs use the standard encoding.
If you redirect a MIPS16 symbol to a PLT, then it is no longer
a MIPS16 symbol.

Your 1/2 patch is changing that, which is fine, but it needs to
update the macro at the same time.  So...

> -#define ELF_ST_IS_MIPS_PLT(other) (((other) & STO_MIPS_FLAGS) == STO_MIPS_PLT)
> -#define ELF_ST_SET_MIPS_PLT(other) (((other) & ~STO_MIPS_FLAGS) | STO_MIPS_PLT)
> +#define ELF_ST_IS_MIPS_PLT(other)					\
> +  ((ELF_ST_IS_MIPS16 (other)						\
> +    ? ((other) & (~STO_MIPS16 & STO_MIPS_FLAGS))			\
> +    : ((other) & STO_MIPS_FLAGS)) == STO_MIPS_PLT)
> +#define ELF_ST_SET_MIPS_PLT(other)					\
> +  ((ELF_ST_IS_MIPS16 (other)						\
> +    ? ((other) & (STO_MIPS16 | ~STO_MIPS_FLAGS))			\
> +    : ((other) & ~STO_MIPS_FLAGS)) | STO_MIPS_PLT)

...this part looks OK if included in the main patch, but not as a separate
change.

I agree it's a bug that the current definition leaves 0xc0 set for
PLT symbols that were originally MIPS16.  It looks like that came
in with the microMIPS/STO_MIPS_ISA stuff.  But fixing that under the
current scheme -- i.e. clearing 0xc0 -- goes in the opposite direction
from your patch, so it wouldn't make a useful stepping stone.

Similarly for PICness: STO_MIPS16 and STO_MIPS_PIC are mutually exclusive
(because MIPS16 functions don't care about the incoming value of $25).
This part:

>  /* This value is used to mark PIC functions in an object that mixes
>     PIC and non-PIC.  Note that this bit overlaps with STO_MIPS16,
>     although MIPS16 symbols are never considered to be MIPS_PIC.  */
>  #define STO_MIPS_PIC		0x20
> -#define ELF_ST_IS_MIPS_PIC(other) (((other) & STO_MIPS_FLAGS) == STO_MIPS_PIC)
> -#define ELF_ST_SET_MIPS_PIC(other) (((other) & ~STO_MIPS_FLAGS) | STO_MIPS_PIC)
> +#define ELF_ST_IS_MIPS_PIC(other)					\
> +  (!ELF_ST_IS_MIPS16 (other)						\
> +   && ((other) & STO_MIPS_FLAGS) == STO_MIPS_PIC)
> +#define ELF_ST_SET_MIPS_PIC(other)					\
> +  (ELF_ST_IS_MIPS16 (other)						\
> +   ? (other)								\
> +   : (((other) & ~STO_MIPS_FLAGS) | STO_MIPS_PIC))

looks wrong because ELF_ST_SET_MIPS_PIC really ought to make the symbol
a MIPS_PIC symbol, rather than ignore the caller and leave the symbol as
something else.  E.g. this could happen if we want to redirect what was
originally a MIPS16 function to a PIC hard-float stub.

The same 0xc0 bug exists here; the pre-microMIPS version was:

#define ELF_ST_SET_MIPS_PIC(OTHER) \
  (STO_MIPS_PIC | ELF_ST_VISIBILITY (OTHER))

which I think was correct at the time.  I'd suggest:

#define ELF_ST_SET_MIPS_PIC(OTHER) \
  ((ELF_ST_IS_MICROMIPS (OTHER) ? STO_MICROMIPS : 0) \
   | ELF_ST_VISIBILITY (OTHER) \
   | STO_MIPS_PIC)

which is OK if you agree.  The change to ELF_ST_IS_MIPS_PIC looks
superfluous though.

Richard


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]