[PATCH] MIPS gas: Fix broken relocation sorting
Wed Nov 8 07:41:00 GMT 2006
Richard Sandiford wrote:
> David Daney <email@example.com> writes:
>> Richard Sandiford wrote:
>>> David Daney <firstname.lastname@example.org> 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
> 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,
More information about the Binutils