Re: [PATCH] MIPS gas: Fix broken relocation sorting

[David Daney <ddaney at avtrex dot com>]

Richard Sandiford wrote:
> David Daney <ddaney@avtrex.com> writes:
>
> > The problem here is that some explicit relocations were not getting 
> > sorted properly.  The %got() and %lo() were being separated by another 
> > %got()/%lo() pair.  This resulted in incorrect relocations being applied 
> > by the linker.
> >
> > At first I thought this was a GCC bug, but Ian Lance Taylor set me 
> > straight here.  For those interested, the GCC bug report is here:
> >
> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=29721
> >
> > The root of the problem is that the relocations are against a local 
> > symbol in another section which get converted to relocations against the 
> > global section symbol.  The relocation sorting was not done for 
> > relocations against global symbols.
>
>
> The section symbol is local, not global.  The bug appears to be
> that pic_need_relax doesn't realise this.
>
>
> > There are as far as I can see two easy ways to fix it:
> >
> > 1) In mips_fix_adjustable, don't allow the conversion if it would affect 
> > a relocation that could possibly need sorting.
> >
> > 2) In mips_from_file, do the sorting with relocations against global 
> > symbols also.
> >
> > I chose the second option, as it looked like the exclusion of global 
> > relocations from the sorting was probably just an optimization.  The 
> > first option would result in many %got() relocations that have no 
> > corresponding %lo() not being converted to be against the section.
>
>
> It isn't just an optimisation. 

The optimization I was referring to is that for R_MIPS_GOT16 against a 
global symbol, the relocation was never considered for moving.  If there 
is no corresponding R_MIPS_LO16 it will not be moved anyway.  So the 
optimization was the elimination of the search for the corresponding 
R_MIPS_LO16.  That is all my patch changes.  It changes nothing to do 
with whether it is a global or local symbol.

> This is one of the nasty warts of
> the 32-bit ABI: the meaning of an R_MIPS_GOT16 relocation depends
> on whether the symbol is STB_LOCAL.  If the symbol is STB_LOCAL,
> the relocation gives the address of the 64k page that contains
> the symbol, and you need to add the associated R_MIPS_LO16 to
> get the symbol's value.  If the symbol is not STB_LOCAL,
> R_MIPS_GOT16 gives the symbol's value up-front.
>
> Thus if we were indeed converting R_MIPS_GOT16 relocations against
> local symbols into R_MIPS_GOT16 relocations against global symbols,
> (1) would be the right fix.  But like I say, the bug appears at
> first glance to be in pic_need_relax.

The comments say that pic_need_relax must agree with adjust_reloc_syms 
in write.c.  This can be changed just for MIPS by doctoring 
pic_need_relax and mips_fix_adjustable.  Which is similar to my option 
#1 above but with the pic_need_relax change also.

Does this seem like the correct approach?

David Daney