Re: [patch] MIPS: Incorrect calculation for R_MIPS_LO16 relocs

["Maciej W. Rozycki" <macro at linux-mips dot org>]

On Tue, 29 Jun 2004, Richard Sandiford wrote:

> If we were only now adding support for mergeable sections and split REL
> relocations, I'm sure we wouldn't want to do it like this.  Something like
> Maciej's patch would be cleaner.
> 
> Unfortunately, there's a long history of not putting HI16s before LO16s,
> and if we patched the linker now, it would silently mishandle existing
> references to mergeable section symbols + offsets.  Patching gas seems
> much less invasive and keeps the existing ABI.

 Well, the existing cases are broken anyway -- depending on contents of a
section and offsets used, a set of object files may link properly or not.  
With my changes at least you have an opportunity to get an "access beyond
end of merged section" message at the final link, so you get the attention
something is wrong.

 Would your change fix linking existing object files?  I don't think so --
you're only changing gas. ;-)  And if building from sources, there's no
problem with ABI changes resulting from my patches and you get:

1. Correct binaries if building everything from scratch.

2. Warning messages for many cases (unfortunately not all, but information
is lost already) when a previously built object is malformed and may
produce a broken binary.  I think this is the case you refer to above,
writing: "[...] it would silently mishandle existing references to
mergeable section symbols + offsets."  It's neither a worse mishandling
than the current one -- the offset will be left intact as for an ordinary
section -- nor actually a silent one.

3. Warning messages for sources that use %hi and %lo operators in a
dangerous way.  I was quite surprised having discovered GCC is an offender
here.

Note that only objects generated by gcc 3.4 and newer are affected (the
culprit are explicit relocs) plus perhaps a little number of handcoded
assembly sources (if any at all).  And the change stops at the final link,
so there's no problem with shared object interaction which would be
painful, indeed.

 Thus I think it's still early enough to fix the BFD MIPS ABI instead of
trying to cover bugs with half-fixes and keeping this incompatibility to
the official MIPS ABI forever.  Given note #2 above I think it'll actually
always be early enough -- existing objects are broken anyway and don't
link reliably.

 For NewABI we may of course skip this reordering and the already existing
HI16 one as well.  I don't think there's an explicit NewABI document, but
"64-bit ELF Object File Specification, Version 2.4" by Jim Dehnert of MIPS
Technologies / Silicon Graphics Computer Systems is the closest
approximate and it's quite explicit about HI16/LO16 pairing being needed
for SHT_REL sections only and confirms my interpretation as implemented by
these patches is correct (table 32, page 47):

"An R_MIPS_HI16 must be followed immediately by an R_MIPS_LO16 relocation
record in a SHT_REL section.  The contents of the two fields to be
relocated are combined to form a full 32-bit addend AHL.  An R_MIPS_LO16
entry which does not immediately follow a R_MIPS_HI16 is combined with the
most recent one encountered, i.e. multiple R_MIPS_LO16 entries may be
associated with a single R_MIPS_HI16.  Use of these relocation types in a
SHT_REL section is discouraged and may be forbidden to avoid this
complication."

> I tested this on:
> 
>    mips64-elf  mips64el-elf  mips-elf  mipsel-elf  mips-ecoff
>    mips-linux-gnu  mipsel-linux-gnu mips64-linux-gnu  mips64el-linux-gnu
> 
> no regressions.  I've reattached Maciej's testcase with a couple of
> trivial changes to the *.d file.  (Specifically: there's no need to
> force little-endian code, since the testcase works with either endianness.
> There's also no need to use -melf32ltsmip, which not all configurations
> support, since the testcase works for both REL and RELA targets.  Of course,
> we're mostly interested in REL here, but the test should still pass for
> RELA as well.)

 Thanks for having a look at this and testing for a wide set of
configurations.  I don't insist on a particular endianness or other
settings, certainly.  I'll fix it in my patches.

  Maciej