Page MenuHomePhabricator

[DWARF] Add an api to get "interpreted" location lists
ClosedPublic

Authored by labath on Nov 18 2019, 6:40 AM.

Details

Summary

This patch adds DWARFDie::getLocations, which returns the location
expressions for a given attribute (typically DW_AT_location). It handles
both "inline" locations and references to the external location list
sections (currently only of the DW_FORM_sec_offset type). It is
implemented on top of DWARFUnit::findLoclistFromOffset, which is also
added in this patch. I tried to make their signatures similar to the
equivalent range list functionality.

The actual location list interpretation logic is in
DWARFLocationTable::visitAbsoluteLocationList. This part is not
equivalent to the range list code, but this deviation is motivated by a
desire to reuse the same location list parsing code within lldb.

The functionality is tested via a c++ unit test of the DWARFDie API.

Event Timeline

labath created this revision.Nov 18 2019, 6:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 18 2019, 6:40 AM
labath marked 4 inline comments as done.Nov 18 2019, 6:46 AM
labath added inline comments.
llvm/include/llvm/DebugInfo/DWARF/DWARFAddressRange.h
59 ↗(On Diff #229832)

Ideally I'd have both this and the operator< above also compare the section index, but I did not want to do that in the same patch.

llvm/include/llvm/DebugInfo/DWARF/DWARFLocationExpression.h
32–37

These, and other random operators the patch is adding, are to make sure one can make sense of the gtest output when the tests fail.

llvm/lib/ObjectYAML/DWARFEmitter.cpp
318

This doesn't take type units into account, but then the whole of DWARFEmitter does not handle type units AFAICT.

llvm/test/tools/llvm-dwarfdump/X86/statistics-dwo.test
83–84 ↗(On Diff #229832)

I haven't checked if these numbers are fully correct, but they should definitely be *more* correct then before, as the code previously did not handle split dwarf location lists at all.

dblaikie added inline comments.Nov 18 2019, 3:59 PM
llvm/include/llvm/DebugInfo/DWARF/DWARFAddressRange.h
52–61 ↗(On Diff #229832)

Welcome to/probably best to commit changes like this by themselves and separate from this commit - makes the review easier without these orthogonal fixes, etc.

llvm/include/llvm/DebugInfo/DWARF/DWARFLocationExpression.h
32–38

Maybe introducing these separately with unit tests of their own would be good?

llvm/include/llvm/Object/ObjectFile.h
158–159 ↗(On Diff #229832)

Separate+unit test, perhaps? (wouldn't bother sending this & the other for review - I imagine they'd be simple enough for review-after-commit)

llvm/lib/ObjectYAML/DWARFEmitter.cpp
318

Might be worth a comment or rephrasing to make the 8/7 values clear as to what they relate to/come from?

llvm/test/tools/llvm-dwarfdump/X86/statistics-dwo.test
83–84 ↗(On Diff #229832)

Might be worth a manual check of the clang binary to see that the numbers remain the same in the cases where it's expected & perhaps that the numbers are the same for split and non-split builds if this change is expected to fix a bug where they were divergent?

krisb added a subscriber: krisb.Nov 19 2019, 6:19 AM
labath updated this revision to Diff 230059.Nov 19 2019, 6:25 AM
labath marked 5 inline comments as done.
  • committed operator<<s separately
  • statistics stuff moved to a separate patch
  • add DWARFLocationExpressionsVector typedef for more range list symmetry
llvm/test/tools/llvm-dwarfdump/X86/statistics-dwo.test
83–84 ↗(On Diff #229832)

Closer examination of this code revealed more problems, so I am going to split the statistics stuff into a separate patch too.

labath edited the summary of this revision. (Show Details)Nov 19 2019, 7:53 AM
dblaikie accepted this revision.Nov 19 2019, 11:45 AM
dblaikie added inline comments.
llvm/unittests/DebugInfo/DWARF/DWARFDieTest.cpp
56–67

Maybe break this down a bit further by field? (0, 0, 0, 0 length \ 5 version, etc... including for the location expressions? Otherwise the strings of zeros especially (& random 2s?) are a bit opaque)

66

"intentionally missing end of list entry to test error handling" (maybe that's too verbose - but as it stands, just "no end of list" is a bit confusing to me - it's not clear why that's the case/being highlighted)

This revision is now accepted and ready to land.Nov 19 2019, 11:45 AM
This revision was automatically updated to reflect the committed changes.