Page MenuHomePhabricator

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

Authored by friss on Sep 4 2014, 6:55 AM.



This is the merge of 2 patches:

Pass the DWARFUnit to DWARFDebugLine::getFileNameByIndex().

This way it can query the compilation dir when it is referenced (when a file references directory index 0, it refers to the compilation dir).


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

This patch pretty prints the contents of the DW_AT_decl_file, DW_AT_call_file, DW_AT_decl_line and DW_AT_call line attributes.
Drop the const on the passed DWARFUnit to be able to call getLineTableForUnit (the line table construction is lazy, thus the getter might modify the Unit).

Diff Detail


Event Timeline

friss updated this revision to Diff 13258.Sep 4 2014, 6:55 AM
friss retitled this revision from to [dwarfdump] Implement extraction of file information referenced in .debug_info..
friss updated this object.
friss edited the test plan for this revision. (Show Details)
friss added reviewers: dblaikie, echristo, aprantl.
friss added a subscriber: Unknown Object (MLST).
friss added inline comments.Sep 4 2014, 8:19 AM
125 ↗(On Diff #13258)

The more I look at it, the less I like it. I'll work on something a bit different.

friss updated this revision to Diff 13286.Sep 4 2014, 2:22 PM
  • [dwarfdump] Dump DW_AT_(decl|call)_line attribute values as decimal values.
  • Turn DWARFContext helpers getFileNameForUnit() and getFileLineInfoForCompileUnit() into full blowm DWARFDebugLine::LineTable methods.
  • [dwarfdump] Dump full filenames as DW_AT_(decl|call)_file attribute values
friss updated this revision to Diff 13287.Sep 4 2014, 2:29 PM

I really have troubles to get arcanist do what I want. I hope this update won't lose the fist 2 patches...

  • [dwarfdump] Dump full filenames as DW_AT_(decl|call)_file attribute values
friss added a comment.Sep 4 2014, 2:37 PM

Sorry for the spam. I'm still learning to use arcanist, and I must say at this point it's more a burden that a help... This latest revision is the result of a small series of 3 commits that should be easy enough to distinguish. They aren't really intermingled except for the test updates.
The first just prints the line numbers in decimal and updates the tests.
The second moves some static helpers of DWARFContext.cpp into equivalent methods of LineTable. There should be no functional change though.
The third one uses the newly exposed method to find and print full names for files referenced by DW_AT_(decl|call)_file.

dblaikie edited edge metadata.Sep 4 2014, 3:28 PM

Please go ahead & commit the decl/call_line portion of this (I'll take your
word for it that these seem like the only ones that should be printed as
decimal, the rest as hex - and if we see other cases we can figure it out
at that point) & then rebase this patch.

The realpath piece of this is particularly interesting - I don't know what
that API is, where it comes from (why it might not always be available),
etc. Also, not sure it makes sense to canonicalize the paths based on the
actual filesystem, rather than just rendering what's in the line table...
maybe some examples would help me understand better. (is that codepath
tested? How do we test it on machines that don't have realpath?)

friss updated this revision to Diff 13302.Sep 5 2014, 1:21 AM
friss edited edge metadata.

The line number portion is in. Rebase:

  • Turn DWARFContext helpers getFileNameForUnit() and getFileLineInfoForCompileUnit() into full blowm DWARFDebugLine::LineTable methods.
  • [dwarfdump] Dump full filenames as DW_AT_(decl|call)_file attribute values
dblaikie added inline comments.Sep 5 2014, 11:45 AM
458 ↗(On Diff #13302)

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)

94 ↗(On Diff #13302)

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)

98 ↗(On Diff #13302)

I probably wouldn't worry about the typedef for just one usage

99 ↗(On Diff #13302)

This variable could be rolled into an if condition down on line 103:

if (auto *LT = ... getLineTableForUnit(...))

if (LT->getFileNameByIndex(...))
  File = '"' + ...
104 ↗(On Diff #13302)

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)

106 ↗(On Diff #13302)

Just for fun you could use std::move on the RHS here:

File = '"' + std::move(File) + '"';

to possibly reuse the storage.

107 ↗(On Diff #13302)

this pointer will be dangling when used on line 111, outside the scope of the 'File' variable

674 ↗(On Diff #13302)

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)

186 ↗(On Diff #13302)

Moving these two functions may be better as a separate patch.

friss added inline comments.Sep 5 2014, 12:32 PM
458 ↗(On Diff #13302)

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.

94 ↗(On Diff #13302)

Yep, the comparison bellow is cheaper. I'll move that.

98 ↗(On Diff #13302)

I did that just for the 80 characters limit. I can of course remove it.

104 ↗(On Diff #13302)

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?

107 ↗(On Diff #13302)


674 ↗(On Diff #13302)

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.

186 ↗(On Diff #13302)

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.

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.

496 ↗(On Diff #13302)

Why did this become conditional+continue?

98 ↗(On Diff #13302)

I would assume clang-format would do something plausible with the long expression, but I could be wrong.

104 ↗(On Diff #13302)

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)

674 ↗(On Diff #13302)

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)

186 ↗(On Diff #13302)

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.

I'm sorry that you spent time again reviewing this, but I hadn't actually uploaded a new version. I opened a new revision with just the API move from the context to the LineTable (D5354) and I was going to update this one with just the file printing once the other is approved.

emaste added a subscriber: emaste.Sep 16 2014, 10:24 AM
friss updated this revision to Diff 13879.Sep 19 2014, 9:43 AM

Rebased on trunk and addressed old review comments.

dblaikie accepted this revision.Sep 19 2014, 10:20 AM
dblaikie edited edge metadata.

Looks good to me - thanks!

This revision is now accepted and ready to land.Sep 19 2014, 10:20 AM
samsonov accepted this revision.Sep 19 2014, 3:59 PM
samsonov added a reviewer: samsonov.
samsonov added a subscriber: samsonov.

Looks good modulo a couple of nits below.

93 ↗(On Diff #13879)

You can rewrite it as:

if (const auto *LT = ...) {
99 ↗(On Diff #13879)

std::move looks confusing here.

friss added inline comments.Sep 22 2014, 4:48 AM
99 ↗(On Diff #13879)

This was actually Dave's suggestion. I've backed it out from the patch I'm committing.

friss closed this revision.Sep 22 2014, 5:46 AM
friss updated this revision to Diff 13928.

Closed by commit rL218246 (authored by @friss).