This is an archive of the discontinued LLVM Phabricator instance.

Turn local DWARFContext helpers getFileNameForUnit() and getFileLineInfoForCompileUnit() into full-blowm DWARFDebugLine::LineTable methods.
ClosedPublic

Authored by friss on Sep 15 2014, 6:30 AM.

Details

Summary

getFileNameForUnit() is basically a wrapper around LineTable::getFileNameByIndex().
Fold its additional functionality (adding the DWARFUnit compilation dir) into
LineTable::getFileNameByIndex().

getFileLineInfoForCompileUnit() is a wrapper around getFileNameForUnit(). As
a function to search the line information by address, it seems natural to put
it in the LineTable also.

Before this commit only the Context with its private helpers could do Linetable
lookups. This newly exposed feature will be used by the DIE dumping code to
get access to file information referenced in DIE attributes.

This commit has already been partly reviewed in D5192 and contained an
additional and a bit controversial 'realpath' call that is left out of this
patch. We can reinstate that realpath code later if it is desirable.

Diff Detail

Event Timeline

friss updated this revision to Diff 13706.Sep 15 2014, 6:30 AM
friss retitled this revision from to Turn local DWARFContext helpers getFileNameForUnit() and getFileLineInfoForCompileUnit() into full-blowm DWARFDebugLine::LineTable methods..
friss edited the test plan for this revision. (Show Details)
friss added reviewers: dblaikie, echristo, aprantl, samsonov.
friss updated this object.
friss added a subscriber: Unknown Object (MLST).
samsonov edited edge metadata.Sep 15 2014, 10:54 AM

Can you pass just a compilation dir instead of DWARFUnit* into a DWARFDebugLine methods? For now we don't need any other information from unit, and I'm not sure we will - line table should be more or less self-contained.

lib/DebugInfo/DWARFDebugLine.cpp
663

Looks like you can have a single SmallString<> to construct an absolute filepath.

693

It's not clear now why CU must be nonzero, even though getFileNameByIndex allows U to be nullptr.

lib/DebugInfo/DWARFDebugLine.h
184

It's somewhat confusing to have output parameter not the last parameter in the function. See my suggestion about passing compilation dir instead of DWARFUnit* - in this case you can just pass empty string if unit is unavailable.

friss added inline comments.Sep 15 2014, 11:38 AM
lib/DebugInfo/DWARFDebugLine.cpp
663

You mean removing the AbsolutePath variable in the is_relative code bellow right?

693

This is just how the original code is written, this patch is supposed to have no functional change. I agree with you that this looks strange, and maybe this check should be extracted and moved to the current callers.

lib/DebugInfo/DWARFDebugLine.h
184

Agreed. I mechanically add new parameters with a default value to the end to avoid having to modify the existing callers, but here it looks strange.
I'd rather pass a raw const char * to avoid creating unnecessary string objects though.

samsonov added inline comments.Sep 15 2014, 1:11 PM
lib/DebugInfo/DWARFDebugLine.cpp
663

Yes.

friss updated this revision to Diff 13754.Sep 16 2014, 9:40 AM
friss edited edge metadata.

As per review comments:

  • Remove DWARFUnit * parameters to LineTable methods. Pass just the compilation dir instead.
  • Remove useless temporaries in getFileNameByIndex
  • Change the order of getFileNameByIndex arguments to be more natural

With above changes, the name getFileLineInfoForCompilationUnit didn't make any sense anymore (no more CU argument). Rename the function to getFileLineInfoForAddress instead.

samsonov added inline comments.Sep 17 2014, 2:47 PM
lib/DebugInfo/DWARFDebugLine.cpp
11

And you don't need these headers.

13

and this config.h file.

666

You don't need this extra variable.

679

Just call sys::part::append(FilePath, CompDir) here.

699

you may use "const auto& " here.

lib/DebugInfo/DWARFDebugLine.h
23

Now you don't need these forward declarations.

177

Looks like we can make this a private method now (in a separate change, after this one goes in).

193

Add a brief comment for this function.

friss updated this revision to Diff 13822.Sep 18 2014, 2:15 AM
  • remove now useless includes and forward declarations.
  • add comment to new member function
  • code changes as per review comments
samsonov accepted this revision.Sep 18 2014, 10:25 AM
samsonov edited edge metadata.

LGTM

This revision is now accepted and ready to land.Sep 18 2014, 10:25 AM
friss closed this revision.Sep 19 2014, 8:21 AM
friss updated this revision to Diff 13873.

Closed by commit rL218125 (authored by @friss).