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

["Maciej W. Rozycki" <macro at codesourcery dot com>]

On Sat, 9 Mar 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.

 Back to these submissions after a while...  I think you're right after 
all, this is not an installed header, so the macros are only ever used 
internally by BFD.  I have folded it now into the main 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).

 Sure, otherwise bit #5 couldn't have been used for STO_MIPS_PIC.

> 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.

 Your view makes sense to me.

> 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.

 I agree, however I'd prefer STO_MIPS16 to be referred explicitly here.  
Using STO_MICROMIPS as a shortcut where what we really mean is STO_MIPS16 
is only going to confuse people I believe.  Also the macro would have to 
get tweaked further if 0x40 was defined as a valid STO_MIPS_ISA encoding 
in the future even if the meaning of STO_MIPS_PIC remained the same for 
that ISA.

 Therefore I propose the following change instead.  I have also dropped 
the change to ELF_ST_IS_MIPS_PIC, per your suggestion -- while coded in a 
bit obscure way as far as STO_MIPS16 is concerned the macro in its current 
form does do the right thing there.

 No regressions across the usual MIPS targets.  OK to apply?

2013-06-06  Maciej W. Rozycki  <macro@codesourcery.com>

        include/elf/
	* mips.h (ELF_ST_SET_MIPS_PIC): Clear any STO_MIPS16 setting.

  Maciej

binutils-include-mips-elf-st-set-pic.diff
Index: binutils-fsf-trunk-quilt/include/elf/mips.h
===================================================================
--- binutils-fsf-trunk-quilt.orig/include/elf/mips.h	2013-05-31 21:44:59.723851688 +0100
+++ binutils-fsf-trunk-quilt/include/elf/mips.h	2013-05-31 21:49:03.753223717 +0100
@@ -810,7 +810,10 @@ extern void bfd_mips_elf32_swap_reginfo_
    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_SET_MIPS_PIC(other)					\
+  ((ELF_ST_IS_MIPS16 (other)						\
+    ? ((other) & ~(STO_MIPS16 | STO_MIPS_FLAGS))			\
+    : ((other) & ~STO_MIPS_FLAGS)) | STO_MIPS_PIC)
 
 /* This value is used for a mips16 .text symbol.  */
 #define STO_MIPS16		0xf0