Page MenuHomePhabricator

Get function start line number from DWARF info
ClosedPublic

Authored by sque on Dec 19 2016, 5:06 PM.

Details

Summary

DWARF info contains info about the line number at which a function starts (DW_AT_decl_line).

This patch creates a function to look up the start line number for a function, and returns it in
DILineInfo when looking up debug info for a particular address.

Diff Detail

Repository
rL LLVM

Event Timeline

sque updated this revision to Diff 82045.Dec 19 2016, 5:06 PM
sque retitled this revision from to Get function start line number from DWARF info.
sque updated this object.
sque added a reviewer: echristo.
sque added a subscriber: llvm-commits.

What's use-case for this?

Also, could you add a test-case?

sque added a comment.Dec 19 2016, 5:28 PM

We have some changes within Google code that depend on this. echristo has already reviewed the internal changes.

I'm still not familiar with the way tests work for this kind of code. Can you point me to a prior commit to lib/DebugInfo/DWARF that has a test case, so I can use it as an example?

If the change is observable from llvm-dwarfdump, you can add a textual test using FileCheck. Otherwise (and it looks like this is the case here) you can add a unit test to unittests/unittests/DebugInfo/DWARF/*.

Process side note: Having "internal changes that depend on it" is typically not a good enough reason to add code to LLVM. Every line of code added to LLVM adds a maintenance burden for the community, so it is typically better to argue why a change benefits LLVM or other downstream users when submitting a patch for review :-)

aprantl added inline comments.Dec 19 2016, 5:53 PM
lib/DebugInfo/DWARF/DWARFContext.cpp
91 ↗(On Diff #82045)

Why do you need to demangle the name here?

105 ↗(On Diff #82045)

Shouldn't this be using accelerator tables if available?

This seems like it's doing way more work than necessary. AFAICT each of the callers is already having to figure out the InlinedChain, which hands you a DIE for the function. It might be the DIE you actually want, or it might have a DW_AT_specification or DW_AT_abstract_origin that is a reference to the DIE you actually want. I can't see any reason to extract a name and then try to do a (possibly ambiguous) name search through the entire debug-info section.
Am I missing something?

echristo edited edge metadata.Jan 3 2017, 7:06 PM

I'm not sure why you need this as part of the line number lookup. Can you explain a bit more how the code that uses this works and why you need this here?

Thanks!

lib/DebugInfo/DWARF/DWARFContext.cpp
91 ↗(On Diff #82045)

Agreed. Also, this needs to be llvm style.

105 ↗(On Diff #82045)

Possibly, but let's add those on after...

sque updated this revision to Diff 83440.Jan 6 2017, 2:57 PM
sque edited edge metadata.

Simplified code to get start line

sque added a comment.Jan 6 2017, 3:04 PM

PTAL. I still need to add a unit test.

aprantl added inline comments.
lib/DebugInfo/DWARF/DWARFContext.cpp
442 ↗(On Diff #83440)

We're trying to get rid of all DWARF APIs that use special failure values right now. Could this return an Optional<uint32_t> instead?

If "StartLine" is actually DW_AT_decl_line, then I would suggest calling it "DeclLine". See other inlined comments.

include/llvm/DebugInfo/DIContext.h
37 ↗(On Diff #83440)

If this actually is always the value of DW_AT_decl_line, why not name it accordingly? "DeclLine" maybe instead of "StartLine"? StartLine seems confusing.

lib/DebugInfo/DWARF/DWARFContext.cpp
442 ↗(On Diff #83440)

This should be added to DWARFDie as a member function:

Optional<uint32_t> DWARFDie::getDeclLine() const;

Other clients will want the DW_AT_decl_line and we shouldn't write the code more than once.

sque added a comment.Jan 9 2017, 11:35 AM

I've added a unit test: https://reviews.llvm.org/D28428

I'll wait until that patch is approved before updating this one, which will modify that patch.

include/llvm/DebugInfo/DIContext.h
37 ↗(On Diff #83440)

It depends on what this field is called in other formats. I only know that it is called "decl_line" in DWARF. Maybe this should be under the "DWARF-specific" section if we call it "DeclLine"

lib/DebugInfo/DWARF/DWARFContext.cpp
442 ↗(On Diff #83440)

Good idea.

sque updated this revision to Diff 83751.Jan 9 2017, 5:16 PM
sque marked 2 inline comments as done.

Add function to get decl line from DWARFDie

clayborg added inline comments.Jan 9 2017, 5:21 PM
lib/DebugInfo/DWARF/DWARFDie.cpp
347 ↗(On Diff #83751)

Remove the -1U here, it will return an optional for you. You can just do:

if (auto DeclLine = getAttributeValueAsUnsignedConstant(DW_AT_decl_line))
  return DeclLine;
353–356 ↗(On Diff #83751)

Recurse in case there is more than one specification or abstract origin:

if (auto DeclLine = SpecDie.getDeclLine())
  return DeclLine;
361–364 ↗(On Diff #83751)

Ditto

sque updated this revision to Diff 83756.Jan 9 2017, 5:37 PM
sque marked 3 inline comments as done.

Recursively get specification or abstract origin; omit use of failure value

DWARFDie stuff looks good. I will let others comment on the rest.

sque updated this revision to Diff 83862.Jan 10 2017, 1:54 PM

Reorganize get function name logic in getFunctionNameAndStartLineForAddress

sque updated this revision to Diff 83865.Jan 10 2017, 2:01 PM

Reorganize function name logic

dblaikie added inline comments.
lib/DebugInfo/DWARF/DWARFDie.cpp
347–349 ↗(On Diff #83865)

Roll the variable declaration into the use:

if (auto DeclLine = getAttributeAs...)
  return DeclLine;
352–354 ↗(On Diff #83865)

And here

357–359 ↗(On Diff #83865)

Similarly here:

if (auto AbsDie = ...)

if (auto DeclLine = ...)
  return DeclLine;

Also - what's the use case for this? (& test coverage would be good/necessary)

sque updated this revision to Diff 84743.Jan 17 2017, 2:08 PM
sque marked 3 inline comments as done.

Updated with dblaikie's suggestions (roll variable declaration into use)

sque added a comment.Jan 17 2017, 2:10 PM

Also - what's the use case for this? (& test coverage would be good/necessary)

Use case: Internal use case, will email you about it.

Unit test: No unit test exists for DWARFContext, but I added one here, awaiting review: https://reviews.llvm.org/D28428

(Both are already mentioned earlier in this thread)

sque updated this revision to Diff 84782.Jan 17 2017, 5:40 PM

Print start line in DIPrinter, used by LLVM Symbolizer

sque added a comment.Jan 17 2017, 5:41 PM
In D27962#649064, @sque wrote:

Print start line in DIPrinter, used by LLVM Symbolizer

Example output:

sque@sq:~/code/llvm-build $ bin/llvm-symbolizer -obj=bin/llvm-symbolizer
0x406f1f
bool error<llvm::DILineInfo>(llvm::Expected<llvm::DILineInfo>&)
/usr/local/google/home/sque/code/llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:89:0
Start line: 89

In D27962#649067, @sque wrote:
In D27962#649064, @sque wrote:

Print start line in DIPrinter, used by LLVM Symbolizer

Example output:

sque@sq:~/code/llvm-build $ bin/llvm-symbolizer -obj=bin/llvm-symbolizer
0x406f1f
bool error<llvm::DILineInfo>(llvm::Expected<llvm::DILineInfo>&)
/usr/local/google/home/sque/code/llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:89:0
Start line: 89

Hrm. But it already had the line above?

sque added a comment.Jan 17 2017, 5:51 PM

0x406f1f
bool error<llvm::DILineInfo>(llvm::Expected<llvm::DILineInfo>&)
/usr/local/google/home/sque/code/llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:89:0
Start line: 89

Hrm. But it already had the line above?

Bad example, that was the first line of the function. How about:

sque@sq:~/code/llvm-build $ bin/llvm-symbolizer -obj=bin/llvm-symbolizer
0x406f2d
bool error<llvm::DILineInfo>(llvm::Expected<llvm::DILineInfo>&)
/usr/local/google/home/sque/code/llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:90:0
Start line: 89

0x406f30
bool error<llvm::DILineInfo>(llvm::Expected<llvm::DILineInfo>&)
/usr/local/google/home/sque/code/llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:90:0
Start line: 89

0x406f40
bool error<llvm::DILineInfo>(llvm::Expected<llvm::DILineInfo>&)
/usr/local/google/home/sque/code/llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:91:0
Start line: 89

0x406f50
bool error<llvm::DILineInfo>(llvm::Expected<llvm::DILineInfo>&)
/usr/local/google/home/sque/code/llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:93:0
Start line: 89

sque updated this revision to Diff 86695.Feb 1 2017, 12:10 PM

Rebased to latest revision; Added regression test coverage

dblaikie accepted this revision.Feb 1 2017, 1:48 PM

Looks good - a few minor improvements/tidyups that'd be good.

lib/DebugInfo/DWARF/DWARFContext.cpp
567–568 ↗(On Diff #86695)

Roll the declaration into the if condition:

if (auto DeclLineResult = FunctionDIE.getDeclLine())

(up to you if you use auto - whatever you think's more readable)

lib/DebugInfo/DWARF/DWARFDie.cpp
295–307 ↗(On Diff #86695)

I think this whole function can now be implemented with this:

return toUnsigned(findRecursively(DW_AT_line));
test/tools/llvm-symbolizer/sym-verbose.test
28 ↗(On Diff #86695)

Might be good to describe this as "Function start line" to be more clear (& maybe put it above "Line" - so that line/col/discrim are together, since they're closely related.

This revision is now accepted and ready to land.Feb 1 2017, 1:48 PM
sque updated this revision to Diff 86803.Feb 2 2017, 6:50 AM
sque marked 2 inline comments as done.

Responded to dblaikie's comments

sque updated this revision to Diff 86804.Feb 2 2017, 6:52 AM
sque marked an inline comment as done.

Move printing of start line to before line/column/discrim

This revision was automatically updated to reflect the committed changes.