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

[Rafael Ávila de Espíndola]

REPOSITORY
  rL LLVM

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp:29
@@ +28,3 @@
+struct MipsRelocationEntry {
+  MipsRelocationEntry(ELFRelocationEntry &R, unsigned MatchingLo):
+      Reloc(R), SortOffset(R.Offset), MatchingLoType(MatchingLo),
----------------
Please use the same names for arguments and fields so that you have
... : foo(foo), bar(bar) ... {

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp:33
@@ +32,3 @@
+
+  ELFRelocationEntry Reloc;
+  // SortOffset equals Reloc.Offset except for the *HI16 relocations, for which
----------------
Can this be a

const  ELFRelocationEntry &Reloc;

?

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp:39
@@ +38,3 @@
+  // For *HI16 relocations, a matching relocation type for Reloc.Type.
+  int64_t MatchingLoType;
+  // True when this is a *LO16 relocation preceded with a matching *HI16
----------------
Why do you need to store this?

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp:278
@@ +277,3 @@
+
+  // Don't check if symbol is local for non-*GOT16 relocations.
+  if (!(Reloc.Type == ELF::R_MIPS_GOT16
----------------
Why is that?

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp:279
@@ +278,3 @@
+  // Don't check if symbol is local for non-*GOT16 relocations.
+  if (!(Reloc.Type == ELF::R_MIPS_GOT16
+        || Reloc.Type == ELF::R_MICROMIPS_GOT16
----------------
Distributing the ! is probably more readable:

Reloc.Type != ... && Reloc.Type !=

also, put a 

unsigned Type = Roloc.Type;

at the start of the function so you don't have to type" Reloc." all the time.

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp:285
@@ +284,3 @@
+  const MCSymbolData &SD = Asm.getSymbolData(*Reloc.Symbol);
+  if (MCELF::GetBinding(SD) == ELF::STB_LOCAL) {
+    if (Reloc.Type == ELF::R_MIPS_GOT16) return ELF::R_MIPS_LO16;
----------------
Use an earlier return

 if (MCELF::GetBinding(SD) != ELF::STB_LOCAL)
    return ELF::R_MIPS_NONE;


================
Comment at: lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp:296
@@ +295,3 @@
+// second's type and both relocations are against the same symbol.
+static bool areMatchingHiAndLo(MipsRelocationEntry &first,
+                               MipsRelocationEntry &second) {
----------------
Start variable names with an upper case letter.

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp:309
@@ +308,3 @@
+  Hi.IsNextMatchingLo = true;
+  Hi.SortOffset = Lo.Reloc.Offset -1;
+}
----------------
Why -1?
This looks like a leftover from the sort function not considering a relocation own offset to break ties.

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp:335
@@ +334,3 @@
+// rule, we try to replicate binutils' behaviour, for no other reason than
+// consistency with what gnu as produces.
+//
----------------
This wording is a bit soft.

Assembly is user input, and if gnu as matches a %lo and a %hi differently from what we do we would be producing a .o with different semantics.

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp:344
@@ +343,3 @@
+// When there are more HIs matched with one LO, sort them in descending order by
+// offset.
+//
----------------
This description makes it sound like that given

lui    $2, %hi(func2)
lui    $2, %hi(func2)
addiu  $2, $2, %lo(func2)
addiu  $2, $2, %lo(func2)

the second %hi would be matched with the second %lo, but in a previous comment you said that gas produces


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

That is, the second %hi (offset 4) is matched with the first lo (offset 8).


================
Comment at: lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp:353
@@ +352,3 @@
+                                     std::vector<ELFRelocationEntry> &Relocs) {
+  if (Relocs.size() < 2) return;
+
----------------
Please git-clang-format the patch.

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp:366
@@ +365,3 @@
+    // Also find HIs and LOs that are already matched.
+    if (I > 0 && areMatchingHiAndLo(MipsRelocs[I], MipsRelocs[I-1]))
+      setMatch(MipsRelocs[I], MipsRelocs[I-1]);
----------------
Why is this first check for I and I-1 necessary? The main loop should handle it, no?

================
Comment at: test/MC/Mips/sort-relocation-table.s:6
@@ +5,3 @@
+# corresponding *LO16 relocation against the same symbol.
+
+# TODO: Add mips16 and micromips tests.
----------------
Can you add a comment about the gnu as target and command line that can be used to compare the result of this file?

http://reviews.llvm.org/D7414

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