[PATCH] Re-enable a hook in MCELFObjectTargetWriter to allow target-specific relocationtable sorting and use this hook for Mips.

[Vladimir Stefanovic]

A new version of the patch is uploaded.
________________________________________
From: Rafael Ávila de Espíndola [rafael.espindola at gmail.com]
Sent: Thursday, February 19, 2015 7:03 PM
To: Vladimir Stefanovic; Daniel Sanders; simon at atanasyan.com
Cc: Toma Tabacu; Petar Jovanovic; llvm-commits at cs.uiuc.edu
Subject: Re: [PATCH] Re-enable a hook in MCELFObjectTargetWriter to allow target-specific relocationtable sorting and use this hook for Mips.

> About the relocations that must match another:

>

> -mips32, mips64:

>  R_MIPS_HI16 and local R_MIPS_GOT16 must match R_MIPS_LO16 against the same

>  symbol.

>

> -micromips:

>  R_MICROMIPS_HI16 and local R_MICROMIPS_GOT16 must match R_MICROMIPS_LO16

>  against the same symbol.

>

> -mips16:

>  R_MIPS16_HI16 and local R_MIPS16_GOT16 must match R_MIPS16_LO16 against the same

>  symbol.


And it is not possible for a file to mix relocations from these sets? I.E, no file will ever have a R_MIPS16_LO16 and a R_MIPS_HI16? If that is not the case I think there is a bug in gold :-)

Well, you can have this in a .S file:

lui  $2,%hi(sym1)
.set mips16
li   $2,%lo(sym1)

Then relocations would be:
00000000  00000605 R_MIPS_HI16       00000000   sym1
00000004  00000669 R_MIPS16_LO16     00000000   sym1

so the first relocation wouldn't match the second.

> Now, since mips16 and micromips are a work in progress, I would rather skip

>  handling them for now (ie. return generic sortRelocs() and add a TODO for the

>  HI16/GOT16 exceptions). E.g., instead of R_MICROMIPS_LO16, llvm currently

>  generates R_MIPS_LO16, so I can't even add a micromips test that will pass at

>  the moment.


That is fine. An assert would be better than a comment to make sure it gets implemented :-)

> For the deterministic output - in the examples I ran, when entering sortRelocs()

>  relocs were sorted by offset in the ascending order. But I will add a call to

>  generic sortRelocs() at the beginning of the function, to make the output

>  deterministic for sure.


Yes, I honestly cannot remember where the non determinism was coming from, but I do remember the original function sorting only by Offest and we having issues.

> And, apart from HI16/GOT16, I would like to sort relocs by offset - like other

>  architectures and mips gcc do.


MC suffer a lot from trying to produce object files that look like what gas produces. I know this is mostly my fault, but if I can help us move to a point where every 'if' in the writer has a good justification that would be awesome.

> A quote from binutils source (binutils/bfd/elfxx-mips.c):

>

>   The ABI requires that the *LO16 immediately follow the *HI16.

>   However, as a GNU extension, we permit an arbitrary number of

>   *HI16s to be associated with a single *LO16.


Check. It seems to be what gold implements.

> The logic I used in the code below is the simplest I came up with to obey the

>  rule above: for every HI16 / local GOT16 relocation at the given offset, pair it

>  with the first found LO16 relocation against the same symbol, starting from

>  offset + 4 and ending at offset -4. (Wrap around reloc table size.)


That is something that needs to be explicitly documented, since it is the semantic of the of assembly file, which is potentially an user input.

> GCC does it differently; here is a comment about it from

>  binutils/gas/config/tc-mips.c:

>

>   When several %lo()s match a particular %got() or %hi(), we use the

>   following rules to distinguish them:

>

>     (1) %lo()s with smaller offsets are a better match than %lo()s with

>         higher offsets.

>

>     (2) %lo()s with no matching %got() or %hi() are better than those

>         that already have a matching %got() or %hi().

>

>     (3) later %lo()s are better than earlier %lo()s.

>   These rules are applied in order.

>

>

>

> Thus, for this example:

>

>   lui    $2, %hi(func2)

>   lui    $2, %hi(func2)

>   addiu  $2, $2, %lo(func2)

>   addiu  $2, $2, %lo(func2)

>

>

>

> the code below sorts the table like this:

>

>   Offset     Info    Type            Sym.Value  Sym. Name

>

> 00000000  00000605 R_MIPS_HI16       00000000   func2

>  00000004  00000605 R_MIPS_HI16       00000000   func2

>  00000008  00000606 R_MIPS_LO16       00000000   func2

>  0000000c  00000606 R_MIPS_LO16       00000000   func2

>

> and this is what gcc does:

>

>   Offset     Info    Type            Sym.Value  Sym. Name

>

> 00000004  00000605 R_MIPS_HI16       00000000   func2

>  00000008  00000606 R_MIPS_LO16       00000000   func2

>  00000000  00000605 R_MIPS_HI16       00000000   func2

>  0000000c  00000606 R_MIPS_LO16       00000000   func2

>

> So, at least for consistency reasons, maybe I should change this code to behave

>  like gcc. What do you think?


I agree. We should implement the same *semantics* as gas.

In the above comment, what does "match", "earlier" and "in order" mean?

'match' for a *HI16 relocation is a matching LO16 relocation, ie. the one
against the same symbol and with the appropriate *LO16 relocation type.
'earlier' means smaller offset.
'in order' is similar to our cmpRel() sorting function - apply the first rule,
and if the output isn't deterministic yet, apply the second one, etc.


REPOSITORY
  rL LLVM

http://reviews.llvm.org/D7414

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/