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

[Keno Fischer]

I'll split up the patch for further in depth review, but I wanted to have a chance to have everybody see how everything plays together. In particular, I'd like @echristo to comment whether the general direction of this patch seems reasonable. If that's the case, I'll split the patch into three parts (The debug info changes, the LoadedObjectInfo integration with DwarfContext and the Darwin support for RelocVisitor).


REPOSITORY
  rL LLVM

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

================
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;
----------------
friss wrote:
> 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?
Yes, the JIT will load and relocate debug info in some cases. 

================
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;
+            }
+          }
+        }
 
----------------
friss wrote:
> 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).
Currently there's only the lookup by name. I'm not sure if the RuntimeDyld SectionIDs have any relation to the section order in the object file. @lhames?

================
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);
----------------
friss wrote:
> 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.
> 
Will do. I had just noticed it while testing this.

http://reviews.llvm.org/D6961

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