[PATCH] [Mips64] Add support for MCJIT for MIPS64r2 and MIPS64r6

[Daniel Sanders]

Thanks, that's much cleaner. Most of my comments on the latest patch are nits or notes on things that will need to be followed up on. There is one question among them though, as well as one more comment below.

We seem to have lost the ability to compose distinct relocs in this patch. This should be ok in the immediate term (because we do handle our common composition case of the 3-in-1 relocs) but it's something we'll need to follow up on.

Assuming it's safe to only check r_type for R_MIPS_CALL16, R_MIPS_GOT_PAGE, or R_MIPS_GOT_DISP : LGTM with the nits fixed.


REPOSITORY
  rL LLVM

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp:191
@@ -190,3 +190,3 @@
     : RuntimeDyldImpl(MemMgr, Resolver), GOTSectionID(0), CurrentGOTIndex(0) {}
-RuntimeDyldELF::~RuntimeDyldELF() {}
+RuntimeDyldELF::~RuntimeDyldELF() { SectionToGOTMap.clear(); }
 
----------------
Nit: Seems unnecessary. It's about to be destroyed anyway and we don't seem to be managing memory.

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp:512-513
@@ +511,4 @@
+  if (!StringRef(Triple::getArchTypePrefix(Arch)).equals("mips")) {
+    IsMipsO32ABI = false;
+    IsMipsN64ABI = false;
+    return;
----------------
For follow-up: It would be preferable to use a MipsABIInfo object. It's essentially an enum representing the ABI but you can ask it questions (e.g. Does the current ABI use REL or RELA relocations? etc.) using the member functions.

I don't think we should do this now though. I think it should wait until Architectures have their own subclasses. If we tried to do it now it would probably require Mips support to always be compiled into LLVM and that's undesirable.

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp:516-521
@@ +515,8 @@
+  }
+  unsigned AbiVariant;
+  Obj.getPlatformFlags(AbiVariant);
+  IsMipsO32ABI = AbiVariant & ELF::EF_MIPS_ABI_O32;
+  IsMipsN64ABI = Obj.getFileFormatName().equals("ELF64-mips");
+  if (AbiVariant & ELF::EF_MIPS_ABI2)
+    llvm_unreachable("Mips N32 ABI is not supported yet");
+}
----------------
For follow-up: This is a good example of knowledge that would be suitable for MipsABIInfo. There are multiple places that need to know how to map ELF e_flags to the ABI.

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp:536
@@ +535,3 @@
+  uint32_t RelType = r_type;
+  int64_t calculatedValue = evaluateMIPS64Relocation(Section, Offset, Value,
+                                                     RelType, Addend,
----------------
Nit: Variables should begin with a capital.

r_type/r_type2/r_type3 are arguably ok since they are trying to match member names of an ELF64 structure so there's no need to change those.

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp:559
@@ +558,3 @@
+                                         uint64_t SymOffset, SID SectionID) {
+  int64_t calculatedValue = 0;
+
----------------
Nit: Capitalization

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp:561
@@ +560,3 @@
+
+  DEBUG(dbgs() << "resolveMips64Relocation, LocalAddress: 0x"
+               << format("%llx", Section.Address + Offset)
----------------
Nit: Doesn't match the function name (the 'resolve' and the 'Mips')

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp:579-581
@@ +578,5 @@
+  case ELF::R_MIPS_32:
+  case ELF::R_MIPS_64:
+    calculatedValue = Value + Addend;
+    break;
+  case ELF::R_MIPS_26:
----------------
Optional nit: Move the return into the cases. E.g:
  case ELF::R_MIPS_64:
    return Value + Addend;

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp:1035
@@ -817,1 +1034,3 @@
+      resolveMIPS64Relocation(Section, Offset, Value, Type, Addend, SymOffset,
+                              SectionID);
     break;
----------------
Nit:
  else
    llvm_unreachable("Mips ABI not handled");

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp:1277-1280
@@ +1276,6 @@
+  } else if (IsMipsN64ABI) {
+    uint32_t r_type = RelType & 0xff;
+    RelocationEntry RE(SectionID, Offset, RelType, Value.Addend);
+    if (r_type == ELF::R_MIPS_CALL16 || r_type == ELF::R_MIPS_GOT_PAGE
+        || r_type == ELF::R_MIPS_GOT_DISP) {
+      StringMap<uint64_t>::iterator i = GOTSymbolOffsets.find(TargetName);
----------------
Is it definitely only r_type that matters? Is it possible to have these relocs in r_type2 or r_type3?

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp:1605
@@ -1362,2 +1604,3 @@
+      Result = sizeof(uint64_t);
     break;
   default:
----------------
Nit:
  else
    llvm_unreachable(...);

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.h:61
@@ +60,3 @@
+
+  void applyMIPS64Relocation(uint8_t *TargetPtr, int64_t calculatedValue,
+                             uint32_t Type);
----------------
Nit: calculatedValue should begin with a capital

http://reviews.llvm.org/D9667

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