Page MenuHomePhabricator

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

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



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.

Diff Detail

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.
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.


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.


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

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
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.


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

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)


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

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
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.

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)


"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.