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

[David Daney <ddaney at avtrex dot com>]

Richard Sandiford wrote:
> David Daney <ddaney@avtrex.com> writes:
>   
> > Richard Sandiford wrote:
> >     
> > > David Daney <ddaney@avtrex.com> writes:
> > >       
> > > > 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.
> >     
>
> I was thinking that if you move a GOT16 reloc in front of a LO16
> that didn't need it, you might orphan a HI16 reloc that does need it.
>   

The GOT16 is only moved if it is for *exactly* the same symbol as the 
LO16.  I cannot think of a situation where you would have a GOT16 and a 
HI16 for the same symbol, so it would not be possible to have some sort 
of conflict between the two.
> However, I suppose with the way the linker searches for relocations,
> that isn't really a problem; it will just skip over the GOT16.
> I'd still argue that the binary was wrong though. ;)
>
> To be honest, I'm slightly nervous about your patch.  It's the first
> use of section_symbol_p in the gas sources,
Not quite, I just copied the 'if' statement right out of 
adjust_reloc_syms() in write.c

>  and given that we already
> handle most section symbols correctly, it seems like a broad stroke.
>   

How do we know that we handle them correctly?  Are there testcases?  
Both my original patch and the revised version that I committed cause no 
regressions for mipsel-linux.  Perhaps I should have tested mips64-linux 
as well.
> The clinching factor in your testcase appears to be that the section
> is comdat.
>   
Clinching factor?  For what?

When jump tables are generated for case statements in c++ methods, they 
end up in comdat sections.  To do otherwise could cause redundant code 
to be emitted into the final object.

We must emit the relocations in the proper order.  The easiest ways to 
achieve that as far as I can see are:

1) Treat section symbols as local by  having pic_need_relax() return 
true as I did in the committed patch.

2) Remove the check for pic_need_relax() in mips_frob_file(), as I did 
in the first (uncommitted) version of the patch and which I think is 
safe as explained above.

3) In mips_fix_adjustable(), don't allow the conversion to section 
symbol if it would affect a relocation that could possibly need sorting 
or be paired with a relocation that needs sorting.

I tested all three of these options (although I didn't run the testsuite 
on #3) and they all fix the problem.  The third option seemed less than 
ideal as it prevents conversion to section symbols.

I am not particularly enamored of my patch.  If you or anyone else can 
come up with it, I would not be at all offended if my patch were 
reverted in favor of a better solution.  However I do like the test case 
so that should probably remain :-)

As you know, we *were* generating bad code for constructs that I think 
must be quite common in C++.  In any event something needs to be done.

In any event thanks for the comments,

David Daney