Page MenuHomePhabricator

[DWARF] Make llvm-dwarfdump display location lists in a .dwp file correctly. Fixes PR38990.
ClosedPublic

Authored by wolfgangp on Oct 11 2018, 11:18 AM.

Details

Summary

Considers the index when extracting location lists from a .dwp file. The location list sections are added when constructing units.

Majority of the patch by David Blaikie.

@dblaikie:
I've extracted the cleanup portion of your patch with the intent to apply it separately.

Diff Detail

Repository
rL LLVM

Event Timeline

wolfgangp created this revision.Oct 11 2018, 11:18 AM
dblaikie added inline comments.Oct 11 2018, 11:26 AM
lib/DebugInfo/DWARF/DWARFDie.cpp
117–120 ↗(On Diff #169244)

Any chance this could be done when the unit is constructed, rather than every time a loc list is dumped? (ie: could the unit either be constructed with a StringRef of the LocSection already offset to the index entry - or have the unit constructor take the whole LocSection, examine its index entry, and then store the sub-section adjusted/computed using the index entry)

lib/DebugInfo/DWARF/DWARFUnit.cpp
447 ↗(On Diff #169244)

Could be worth doing this isDWO->IsDWO rename separately, easy to commit & simplify this change a bit.

wolfgangp updated this revision to Diff 169343.Oct 11 2018, 5:03 PM
wolfgangp marked an inline comment as done.

Addressed review comments:

  • Keeping track of locsection data separately so we can adjust it when handling dwp files.
  • removing a cleanup that can be applied later.
lib/DebugInfo/DWARF/DWARFDie.cpp
117–120 ↗(On Diff #169244)

We still need a pointer or reference to the whole loc section, because there are relocations in it, so I'm keeping track of the data separately. The constructor then adjusts it if necessary.

The alternative is to use a StringRef as you say, and get the section from the context when needed. Not sure what's preferable, but using the section for construction seems slightly more consistent overall.

dblaikie added inline comments.Oct 11 2018, 5:28 PM
include/llvm/DebugInfo/DWARF/DWARFUnit.h
203 ↗(On Diff #169343)

Could this be a union, perhaps?

wolfgangp updated this revision to Diff 169436.Oct 12 2018, 9:44 AM

Made the location list section pointer and the stringref that describes its data a union rather than a pair.

include/llvm/DebugInfo/DWARF/DWARFUnit.h
203 ↗(On Diff #169343)

Yes, that's better. I guess we can't use std::variant yet?

dblaikie accepted this revision.Oct 15 2018, 12:31 PM
dblaikie added inline comments.
include/llvm/DebugInfo/DWARF/DWARFUnit.h
203 ↗(On Diff #169343)

I don't think so, unfortunately :/

This revision is now accepted and ready to land.Oct 15 2018, 12:31 PM
This revision was automatically updated to reflect the committed changes.
dblaikie added inline comments.Oct 22 2018, 11:51 AM
llvm/trunk/lib/DebugInfo/DWARF/DWARFUnit.cpp
183–188

Not sure if I mentioned this, but any chance we could generalize this solution for all contribution/unit index handling? (so unindexed sections are passed into the ctor here, but they're all converted to indexed offset sections from here on out?) For example the str_offsets section handling in extractDIEsIfNeeded could be moved up here, maybe?