[PATCH] [dwarfdump] Implement extraction of file information referenced in .debug_info.

[Frederic Riss]

================
Comment at: lib/DebugInfo/DWARFContext.cpp:458
@@ -498,2 +457,3 @@
   if (Spec.FLIKind != FileLineInfoKind::None) {
     const DWARFLineTable *LineTable = getLineTableForUnit(CU);
+    if (LineTable)
----------------
dblaikie wrote:
> you could roll this declaration into the if condition below (though I'm still trying to figure out if this is the changes below are the right API direction)
What I had in the first patch was basically a duplication of the getFileNameByIndex() helper. I needed to make the helper globally available, and I decided to put it in LineTable as it is a LineTable search function. I could have put it in the Context or the Unit also, I'm open to suggestions.

================
Comment at: lib/DebugInfo/DWARFDebugInfoEntry.cpp:94
@@ -90,3 +93,3 @@
   const char *Name = nullptr;
   if (Optional<uint64_t> Val = formValue.getAsUnsignedConstant())
     Name = AttributeValueString(attr, *Val);
----------------
dblaikie wrote:
> Presumably this could be an "else" to the new if you're introducing at 97 - since they are mutually exclusive (a decl_file/decl_line will never be decoded by AttributeValueString - so you might as well not call it & avoid all the switching, etc)
Yep, the comparison bellow is cheaper. I'll move that.

================
Comment at: lib/DebugInfo/DWARFDebugInfoEntry.cpp:98
@@ +97,3 @@
+  if (attr == DW_AT_decl_file || attr == DW_AT_call_file) {
+    typedef DILineInfoSpecifier::FileLineInfoKind FileLineInfoKind;
+    const DWARFDebugLine::LineTable *LT;
----------------
dblaikie wrote:
> I probably wouldn't worry about the typedef for just one usage
I did that just for the 80 characters limit. I can of course remove it.

================
Comment at: lib/DebugInfo/DWARFDebugInfoEntry.cpp:104
@@ +103,3 @@
+    if (LT &&
+        LT->getFileNameByIndex(formValue.getAsUnsignedConstant().getValue(),
+                               FileLineInfoKind::AbsoluteFilePath, File, u)) {
----------------
dblaikie wrote:
> Do you have test coverage for the situation where the file name is not found in the line table? (testing the fallback to the old behavior)
The programming is defensive, but the false path should never trigger. If the attribute references a line table entry, it'd better be there. Testing that would mean generating a invalid dwarf file, by striping the .debug_line section for example. Is that what you have in mind?

================
Comment at: lib/DebugInfo/DWARFDebugInfoEntry.cpp:107
@@ +106,3 @@
+      File = '"' + File + '"';
+      Name = File.c_str();
+    }
----------------
dblaikie wrote:
> this pointer will be dangling when used on line 111, outside the scope of the 'File' variable
Oops...

================
Comment at: lib/DebugInfo/DWARFDebugLine.cpp:674
@@ +673,3 @@
+
+  if (U && Kind == FileLineInfoKind::AbsoluteFilePath &&
+      sys::path::is_relative(FilePath.str())) {
----------------
dblaikie wrote:
> test coverage for this? Some absolute and relative paths? (hopefully you can do this with just some hardcoded dwarf (not sure is is_relative will properly return false for "/foo/bar" on a Windows machine, though)
Note that the only added logic in this function is really the realpath stuff. The other added code comes from the wrapper I moved out of DWARFContext.cpp and as such is not really new. I can try to craft some tests anyway.

================
Comment at: lib/DebugInfo/DWARFDebugLine.h:186
@@ -183,2 +185,3 @@
                             DILineInfoSpecifier::FileLineInfoKind Kind,
-                            std::string &Result) const;
+                            std::string &Result, DWARFUnit *U = nullptr) const;
+
----------------
dblaikie wrote:
> Moving these two functions may be better as a separate patch.
It is a separate patch. Arcanist/Phabricator presents it a merged revision, but the DebugInfoEntry.cpp modification is in a separate patch in my tree, and I intend to commit is as such.

http://reviews.llvm.org/D5192