[PATCH] Make it easier to use DwarfContext with MCJIT and add RelocVisitor support for Darwin

[Frederic Riss]

Your patch seems to touch a few different and pretty independent parts on the compiler/tools. It looks like this could be split up quite a bit. Just commenting on the libDebugInfo code:


REPOSITORY
  rL LLVM

================
Comment at: include/llvm/Object/RelocVisitor.h:228-240
@@ -223,1 +227,15 @@
 
+  RelocToApply visitMachO(uint32_t RelocType, RelocationRef R, uint64_t Value) {
+    switch (ObjToVisit.getArch()) {
+    default: break;
+    case Triple::x86_64:
+      switch (RelocType) {
+        default: break;
+        case MachO::X86_64_RELOC_UNSIGNED:
+          return visitMACHO_X86_64_UNSIGNED(R, Value);
+      }
+    }
+    HasError = true;
+    return RelocToApply();
+  }
+
----------------
I'd suggest to submit that in a separate patch too. It can be tested quite simply by verifying that llvm-dwarfdump doesn't emit any relocation processing "errors" anymore on x86_64 macho object files.

================
Comment at: lib/DebugInfo/DWARFContext.cpp:557-559
@@ +556,5 @@
+
+    uint64_t LoadAddress = L ? L->getSectionLoadAddress(name) : 0;
+    if (LoadAddress != 0)
+    {
+      uint64_t Size = Section.getSize();
----------------
The { should be on the same line as the if. If you don't use the LoadAddress variable out of the if, you can move its declaration inside the condition.

================
Comment at: lib/DebugInfo/DWARFContext.cpp:561
@@ +560,3 @@
+      uint64_t Size = Section.getSize();
+      data = StringRef(reinterpret_cast<char *>(LoadAddress),Size);
+    } else {
----------------
Wouldn't it be cleaner to have a method inside the LoadedObjectInfo that directly returns a StringRef (or ArrayRef) to the section data?

================
Comment at: lib/DebugInfo/DWARFContext.cpp:633-637
@@ -623,1 +632,7 @@
     RelocatedSection->getName(RelSecName);
+
+    // Check if the JIT already did the relocations for us
+    // If so, we just use the relocated section
+    bool AlreadyRelocated = L && L->getSectionLoadAddress(RelSecName) != 0;
+    if (AlreadyRelocated)
+      continue;
----------------
No need for the variable, just put the condition in the if. We only care about debug sections here, can this ever be true for the debug sections? Will the JIT load them and relocate them?

================
Comment at: lib/DebugInfo/DWARFContext.cpp:680-701
@@ -661,2 +679,24 @@
           Sym->getAddress(SymAddr);
+          Sym->getSection(RSec);
+          if (RSec != Obj.section_end()) {
+            StringRef SecName;
+            RSec->getName(SecName);
+            SectionLoadAddress = L ? L->getSectionLoadAddress(SecName) : 0;
+            if (SectionLoadAddress != 0) {
+              uint64_t SecAddr = RSec->getAddress();
+              SymAddr += SectionLoadAddress - SecAddr;
+            }
+          }
+        } else {
+          RSec = Reloc.getSection();
+          if (RSec != Obj.section_end()) {
+            SymAddr = RSec->getAddress();
+            StringRef SecName;
+            RSec->getName(SecName);
+            SectionLoadAddress = L ? L->getSectionLoadAddress(SecName) : 0;
+            if (SectionLoadAddress != 0) {
+              SymAddr = SectionLoadAddress;
+            }
+          }
+        }
 
----------------
You introduce section lookups by name for every relocation. Isn't there a more efficient interface? Also, I think I would prefer the code for the LoadedObjectInfo case to be in its own if() rather than mixed with the standard relocation processing (So that it's more self-evident that it's only adding a new code path).

================
Comment at: lib/DebugInfo/DWARFDebugLine.cpp:608-610
@@ -607,6 +607,5 @@
       RowIter row_pos = std::upper_bound(first_row, last_row, row,
-                                         DWARFDebugLine::Row::orderByAddress);
+                                         DWARFDebugLine::Row::orderByAddressLeq);
       // The 'row_pos' iterator references the first row that is greater than
-      // our start address. Unless that's the first row, we want to start at
-      // the row before that.
+      // or equal to our start address.
       first_row_index = cur_seq.FirstRowIndex + (row_pos - first_row);
----------------
This part of the change should be submitted separately with appropriate test coverage. 

Not suggesting to do it for this patch, but looking more globally, I wonder if this function should really be returning results that span multiple sequences or if we shouldn't simply modify it to only look in the first sequence containing the start address. 
Or maybe we need to change DWARFContext::getLineInfoForAddressRange() to interpret the results differently. Right now, if your result spans multiple sequences, it'll return line mappings for the EndSequence rows which could be very misleading.

http://reviews.llvm.org/D6961

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