[PATCH] D31649: [mips] Adds support for R_MIPS_26, HIGHER, HIGHEST relocations in RuntimeDyld

[Simon Dardis via Phabricator via llvm-commits]

sdardis added a comment.

Sorry for the delay in getting back to this. I've spotted something that I think is a problem. Highest & higher are processed as a pair, followed by processing Hi & Lo as a pair. I don't think this is correct, See my comment inline.



================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp:1315-1316
+
+        uint8_t *StubTargetAddr = createStubFunction(
+        Section.getAddressWithOffset(Section.getStubOffset()), AbiVariant);
+
----------------
clang-format this, the indentation for the argument is incorrect.


================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp:1335-1339
+          RelocationEntry REHighest(SectionID, StubTargetAddr - Section.getAddress(),
+                               ELF::R_MIPS_HIGHEST, Value.Addend);
+          RelocationEntry REHigher(SectionID,
+                               StubTargetAddr - Section.getAddress() + 4,
+                               ELF::R_MIPS_HIGHER, Value.Addend);
----------------
Indentation of the arguments and line length are wrong here.


================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp:1360
+        RelocationEntry RE(SectionID, Offset, RelType, Section.getStubOffset());
+        addRelocationForSection(RE, SectionID);        
+        Section.advanceStubOffset(getMaxStubSize());
----------------
Spurious whitespace after the ';'.


================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp:1394-1414
+      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 &&
+            RelType == getMatchingLoRelocation(Reloc.RelType) &&
+            SectionID == Reloc.SectionID) {
----------------
I don't think this is correct for HIGHER/HIGHEST/HI/LO. You're creating two relocation entries for a symbol/section. One containing the HIGHEST+HIGHER part of the addend and the other the HI+HO. Shouldn't R_MIPS_HIGHER get pushed back as well and add the RelocationEntry after computing the addend from all four relocations?


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldELFMips.cpp:148
+    return ((Value + Addend + 0x800080008000) >> 48) & 0xffff;
+
   case ELF::R_MIPS_CALL16:
----------------
Spurious blank line.


Repository:
  rL LLVM

https://reviews.llvm.org/D31649