[PATCH] D10687: [Mips] Add support for MCJIT for MIPS32r6

[Daniel Sanders]

dsanders accepted this revision.

This revision is now accepted and ready to land.

LGTM with R_MIPS_PC19_S2 corrected and R_MIPS_PCHI16's alignment mask added.

Fixing the addend for HI16/LO16/PCHI16/PCLO16 can be in a later patch.


================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp:522
@@ +521,3 @@
+    Insn &= 0xffff0000;
+    Insn |= ((Value - FinalAddress) >> 2) & 0xffff;
+    writeBytesUnaligned(Insn, TargetPtr, 4);
----------------
Are you sure about the '>> 2'? This isn't mentioned in the documentation I'm reading and it isn't present on the PCLO16. I suspect it's a copy/paste error from the *_S2 relocs below

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp:527
@@ +526,3 @@
+  case ELF::R_MIPS_PC19_S2: {
+    uint32_t FinalAddress = (Section.LoadAddress + Offset);
+    Insn &= 0xfff80000;
----------------
The documentation says '(sign_extend(A) + S - (P & ~0x3)) >> 2' so I believe there should be a '& ~0x3' on FinalAddress.

This only applies to R_MIPS_PC19_S2. R_MIPS_PC18_S3 is similar (with 0x7) but the other *_S2's don't do the same thing.

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp:547-560
@@ -515,4 +546,16 @@
+  }
+  case ELF::R_MIPS_PCHI16: {
     uint32_t FinalAddress = (Section.LoadAddress + Offset);
-    writeBytesUnaligned(Value + Addend - FinalAddress, (uint8_t *)TargetPtr, 4);
+    Insn &= 0xffff0000;
+    Insn |= ((Value - FinalAddress + 0x8000) >> 16) & 0xffff;
+    writeBytesUnaligned(Insn, TargetPtr, 4);
     break;
   }
+  case ELF::R_MIPS_PCLO16: {
+    uint32_t FinalAddress = (Section.LoadAddress + Offset);
+    Insn &= 0xffff0000;
+    Insn |= (Value - FinalAddress) & 0xffff;
+    writeBytesUnaligned(Insn, TargetPtr, 4);
+    break;
+  }
+  }
----------------
These two have the same quirk as R_MIPS_HI and R_MIPS_LO. Their addends are taken together as a 32-bit value. This can be fixed at the same time as R_MIPS_HI/R_MIPS_LO.

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp:1312-1315
@@ -1268,6 +1311,6 @@
     } else {
-      if (RelType == ELF::R_MIPS_HI16)
+      if (RelType == ELF::R_MIPS_HI16 || RelType == ELF::R_MIPS_PCHI16)
         Value.Addend += (Opcode & 0x0000ffff) << 16;
       else if (RelType == ELF::R_MIPS_LO16)
         Value.Addend += (Opcode & 0x0000ffff);
       else if (RelType == ELF::R_MIPS_32)
----------------
vradosavljevic wrote:
> dsanders wrote:
> > I'll need to look up the details of the Mips32r6 relocs to review this patch.
> > 
> > However, I believe I've noticed a significant bug in the nearby code that I ought to mention. The handling of R_MIPS_HI16 and R_MIPS_LO16 is incorrect. These relocs have the unusual property that their addend is 32-bit with Hi16 and LO16 each contributing 16-bits. This means that to handle a HI16, we must scan forwards to the corresponding LO16 to retrieve the other half of the addend. Similarly, LO16 must scan backwards to find the top half of the addend. We probably have bugs when the addend causes the result to cross a 16-bit boundary since the carry from the LO16 won't propagate into the HI16:
> >   foo = 65532
> >   R_MIPS_HI16(Symbol = foo, Addend = 0): Value is 65532 >> 16 = 0 (but should be 1)
> >   R_MIPS_LO16(Symbol = foo, Addend = 8): Value is 65540 & 0xffff = 4
> > 
> > @atanasyan: I think you've implemented HI16/LO16 in lld. Is my understanding of this correct?
> I can add FIXME in this patch, and a fix for this bug can be part of another patch. What are your thoughts?
That's ok.

The same applies to PCHI16 and PCLO16.


Repository:
  rL LLVM

http://reviews.llvm.org/D10687