Re: GOLD: RFA: Add support for RX target

[Ian Lance Taylor <iant at google dot com>]

Nick Clifton <nickc@redhat.com> writes:

>>> +       // This is a copy of the relocate_section() function in target-reloc.h,
>>> +       // except with the additional calls to convert_host to fix up the
>>> +       // extracted information.
>>> +       // FIXME: There ought to be a better way to do this.
>>
>> Yes, there ought to be a better way to do this.  If your code is
>> correct, then it looks like the problem is that you have a little-endian
>> object but the reloc fields are written with big-endian data.  The
>> comment at the top of the file suggests that the symbol table is also
>> written with big-endian data, in which case I think this code is
>> insufficient.
>>
>> Anyhow, if the reloc fields are consistenly written big-endian, then the
>> right approach is not to call convert_host, but instead to change
>>
>>> +       typedef Reloc_types<elfcpp::SHT_RELA, 32, false>::Reloc Reltype;
>>
>> to
>>
>>> +       typedef Reloc_types<elfcpp::SHT_RELA, 32, true>::Reloc Reltype;
>>
>> That will cause the reloc code to fetch values big-endian rather than
>> little-endian.
>
> I found it very hard to get the reloc code work correctly for the RX
> but the current version does that.  I did think about splitting up
> relocate_section in target-reloc.h but I did not want to interfere
> with already working code just for the vagaries of this one target.
> (Plus I was not sure that I would not introduce new bugs this way.
> That and the fact that big-endian data/little-endian code is a rare
> case for the RX).
>
> So, if it is OK with you I would like to leave the code as it
> currently is.  It may be less efficient, but it does work.

My real concern is not efficiency, it's the copying of the code in
target-reloc.h.  That code has been tweaked several times and will
undoubtedly be tweaked again.  If it is copied into rx.cc, then the copy
will not be tweaked.  That's a bad road to walk down and I really don't
want to start.  I would much rather have you change the code in
target-reloc.h a little bit than have a copy of that code in rx.cc.

Also all the calls to convert_host but one are for fields fetched from a
elfcpp::Rela, and as far as I can tell all fields fetched from the Rela
are passed to convert_host.  That tells me that the calls to
convert_host are wrong, and that correct code would use a different
endianness for a Rela, and would be templatized on the right class.  I
know that the endianness handling in gold can be difficult to penetrate,
but I really think it is worth understanding.  It's basically a
heirarchy of type-checked macros.


Looking at your code, I don't understand this line in scan_relocs:

+ 	      shndx = elfcpp::Convert<16, true>::convert_host(shndx);

That is swapping a section index read from a local symbol.  If that
swapping is correct, then it seems to me that there must be a lot of
other code which will be reading the wrong local symbol index.  I don't
understand how that code could be working.

Ian