[PATCH] D11186: [mips][mcjit] Calculate correct addend for R_MIPS_HI16 and R_MIPS_PCHI16 relocations.

[Daniel Sanders via llvm-commits]

dsanders added inline comments.

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp:1334
@@ +1333,3 @@
+      RelocationEntry RE(SectionID, Offset, RelType, Addend);
+      PendingRelocs.insert(PendingRelocs.begin(), std::make_pair(Value, RE));
+    } else if (RelType == ELF::R_MIPS_LO16 || RelType == ELF::R_MIPS_PCLO16) {
----------------
Won't this get expensive when there's lots of relocations? SmallVectors is an array internally so insert() has to move the elements after the insertion point (i.e. all elements) to make room for the new one.

Can we use push_back() here and a backwards search below?

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp:1337-1339
@@ +1336,5 @@
+      int64_t Addend = Value.Addend + SignExtend32<16>(Opcode & 0x0000ffff);
+      SmallVector<std::pair<RelocationValueRef, RelocationEntry>, 8>::iterator
+          I = PendingRelocs.begin();
+      while (I != PendingRelocs.end()) {
+        const RelocationValueRef &MatchingValue = I->first;
----------------
You could use a range-based for loop here at the moment. Unfortunately, fixing the comment above will prevent this. It should still use a for-loop instead of a while though.

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp:1342-1344
@@ +1341,5 @@
+        RelocationEntry &Reloc = I->second;
+        if (MatchingValue == Value &&
+            RelType == getMatchingLoRelocation(Reloc.RelType) &&
+            SectionID == Reloc.SectionID) {
+          Reloc.Addend += Addend;
----------------
This is almost certainly for a later patch but I'll mention it now. Does '.' work? For example:

  lui $2, $3, %hi(. + 4)
  addiu $2, $3, %lo(. + 4)

I'm thinking it won't work in the current code because '.' drops unique private labels each time it's used. There's also an interesting special case with '.' and relocs like R_MIPS_(HI|LO)16. The '.' refers to the address for the %hi and the distance between the %hi and %lo is added to the addend. This is quite useful but it surprised me when I tried it. I previously thought the '.' was the current position and the programmer had to manually account for the distance.

================
Comment at: test/ExecutionEngine/RuntimeDyld/Mips/ELF_O32_PIC_relocations.s:64
@@ -55,1 +63,2 @@
 	.size	bar, .-bar
+	.comm   var,9,1
----------------
Nit: indentation


Repository:
  rL LLVM

http://reviews.llvm.org/D11186