This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH 1/2] MIPS: Compressed PLT/stubs support
- From: Richard Sandiford <rdsandiford at googlemail dot com>
- To: "Maciej W. Rozycki" <macro at codesourcery dot com>
- Cc: Catherine Moore <clm at codesourcery dot com>, <binutils at sourceware dot org>
- Date: Sat, 09 Mar 2013 07:43:40 +0000
- Subject: Re: [PATCH 1/2] MIPS: Compressed PLT/stubs support
- References: <alpine.DEB.1.10.1302072108300.6762@tp.orcam.me.uk> <87621mwt3l.fsf@talisman.default> <alpine.DEB.1.10.1303081151260.21335@tp.orcam.me.uk>
"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