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

[David Blaikie]

Last iteration updated here still has the issues mentioned earlier (the "oops" in DWARFDebugInfoEntryMinimal::dumpAttribute) & some other bits & pieces.

Should be easier to see with a new version, with feedback addressed.

The general API change - I don't have a strong opinion/idea about (perhaps because my brain is a bit stuck trying to understand the smaller details of the patch) but it sounds reasonable.

================
Comment at: lib/DebugInfo/DWARFContext.cpp:496
@@ -535,3 +495,3 @@
     DILineInfo Result;
-    getFileNameForUnit(CU, LineTable, Row.File, Spec.FLIKind,
-                       Result.FileName);
+    if (!LineTable->getFileNameByIndex(Row.File, Spec.FLIKind,
+                                       Result.FileName, CU))
----------------
Why did this become conditional+continue?

================
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;
----------------
friss wrote:
> 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.
I would assume clang-format would do something plausible with the long expression, but I could be wrong.

================
Comment at: lib/DebugInfo/DWARFDebugInfoEntry.cpp:104
@@ +103,3 @@
+    if (LT &&
+        LT->getFileNameByIndex(formValue.getAsUnsignedConstant().getValue(),
+                               FileLineInfoKind::AbsoluteFilePath, File, u)) {
----------------
friss wrote:
> 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?
That (crafting a debug_info section without a debug_line) is what came to mind, yet. This is a case where "as a bug investigation aid" seems to come to mind, and accounting for invalid/broken inputs and verifying good behavior there seems appropriate.

I mean I doubt we're robust enough to handle all forms of mangled input, but I think, eventually that may be a reasonable goal (especially if you guys intend to ship this as a user-facing tool, it might be worth fuzzing it, making it actually robust, etc)

================
Comment at: lib/DebugInfo/DWARFDebugLine.cpp:674
@@ +673,3 @@
+
+  if (U && Kind == FileLineInfoKind::AbsoluteFilePath &&
+      sys::path::is_relative(FilePath.str())) {
----------------
friss wrote:
> 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.
Fair enough - the simple code move might be easier to see as a separate patch & I wouldn't worry too much about existing untested code. That's up to you.

(oops, old comment I forgot to send)

================
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;
+
----------------
friss wrote:
> 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.
Somewhat hard to review like this, I think.

At least for some of this it's probably best to send "simple mechanical move" then "change in behavior" (it's difficult to scroll up & down trying to visually inspect the changes you made to the chunks of code you moved around). I'm trying to review things as quickly as possible so that many small patches doesn't slow you down - it should, in theory, speed up review because it's easy to eyeball for correctness.

http://reviews.llvm.org/D5192