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

[Daniel Sanders via llvm-commits]

dsanders accepted this revision.
dsanders added a comment.
This revision is now accepted and ready to land.

LGTM.

We'll have to revise this to follow the rules more closely as the corner cases start to appear but being able to correctly process the majority of HI16/LO16 pairs is a big step forwards compared to what we had before


================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp:1363-1365
@@ +1362,5 @@
+      int64_t Addend = Value.Addend + SignExtend32<16>(Opcode & 0x0000ffff);
+      for (auto I = PendingRelocs.begin(); I != PendingRelocs.end();) {
+        const RelocationValueRef &MatchingValue = I->first;
+        RelocationEntry &Reloc = I->second;
+        if (MatchingValue == Value &&
----------------
I notice you're still iterating forwards here even though you now call push_back above. I was going to say that you need to reverse the iteration direction but having re-read the (very confusing) rules again, I think iterating forwards is the right thing to do.

The reason for this change of opinion is that the HI16's would normally be sunk down to their matching LO16 which is often the first LO16 with the same symbol after the HI16 (there's some corner cases where there are multiple candidates). By iterating forwards, this code effectively does the same thing except the LO16 searches for the matching HI16.

> I think that I can't use a range-based for loop, because I'm removing elements from SmallVector.

For iterating forwards, I agree. If we iterated backwards, that would have been the reason instead.


================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp:1368-1370
@@ +1367,5 @@
+            RelType == getMatchingLoRelocation(Reloc.RelType) &&
+            SectionID == Reloc.SectionID) {
+          Reloc.Addend += Addend;
+          if (Value.SymbolName)
+            addRelocationForSymbol(Reloc, Value.SymbolName);
----------------
Not for this patch.

For later patches:
I think the 'MatchingValue == Value' is stricter than it should be. The addend in the %lo is allowed to differ from one in the %hi on the condition that the user knows they didn't cause a carry from the lo16 to the hi16.

There's also the issue of '.' not working properly but that's a consequence of us not transforming relocations to be section-relative instead of symbol-relative. Once that's fixed, '.' will also work.




Repository:
  rL LLVM

http://reviews.llvm.org/D11186