Page MenuHomePhabricator

DWARF: Fix address range support in mixed 4+5 scenario
ClosedPublic

Authored by labath on May 23 2019, 5:42 AM.

Details

Summary

debug_ranges got renamed to debug_rnglists in DWARF 5. Prior to this
patch lldb was just picking the first section it could find in the file,
and using that for all address ranges lookups. This is not correct in
case the file contains a mixture of compile units with various standard
versions (not a completely unlikely scenario).

In this patch I make lldb support reading from both sections
simulaneously, and decide the correct section to use based on the
version number of the compile unit. SymbolFileDWARF::DebugRanges is
split into GetDebugRanges and GetDebugRngLists (the first one is renamed
mainly so we can catch all incorrect usages).

I tried to structure the code similarly to how llvm handles this logic
(hence DWARFUnit::FindRnglistFromOffset/Index), but the implementations
are still relatively far from each other.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.May 23 2019, 5:42 AM
labath marked 2 inline comments as done.May 23 2019, 5:45 AM
labath added inline comments.
source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
706–713 ↗(On Diff #200937)

This is dead dumping code, which wasn't even correct for DWARF5.

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
3240–3260 ↗(On Diff #200937)

This is also effectively dead, as scope_ranges is not being initialized with anything. Furthermore, I can find no evidence of clang ever emitting the DW_AT_start_scope attribute.

Drive-by: For the "dead code" did you check whether gcc emits DW_AT_start_scope? LLDB should support more than just what Clang emits.

Drive-by: For the "dead code" did you check whether gcc emits DW_AT_start_scope? LLDB should support more than just what Clang emits.

I agree with that, but the fact that clang does not emit the attribute was not the reason why I called the code dead. It was the reason why I did not bother implementing it here. :)

The code is dead because it parses the ranges in the DW_AT_start_scope attribute, and then it just throws them away. What's missing is the part which converts this into the debug-format-agnostic form that the rest of lldb can use.

(and no, I didn't check whether gcc emits the attribute)

Nice patch. See inlined comments.

source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
435–448 ↗(On Diff #200937)

Seems like this code should be put into a method and used here.

961–973 ↗(On Diff #200937)

Seems like this code should be put into a method and used here.

source/Plugins/SymbolFile/DWARF/DWARFUnit.h
208–216 ↗(On Diff #200937)

Maybe just make one function here that takes a DWARFFormValue?:

llvm::Expected<DWARFRangeList> FindRnglistFromOffset(const DWARFFormValue &value) const;

Then it will clean up the code that calls it quite nicely.

labath updated this revision to Diff 201515.May 27 2019, 5:21 AM
  • reduce the code duplication by adding a helper function. I've elected to put the helper function into DWARFDebugInfoEntry, to keep the DWARFUnit part of this patch identical with the llvm DWARFUnit. I think the main source of duplication here is that DWARFDebugInfoEntry has two very similar methods (GetAttributeAddressRanges and GetDIENamesAndRanges), which both parse DW_AT_ranges. Once that is resolved, we will be able to reduce the duplication and get rid of the helper function.
clayborg accepted this revision.May 28 2019, 9:04 AM
This revision is now accepted and ready to land.May 28 2019, 9:04 AM
JDevlieghere added inline comments.May 28 2019, 9:11 AM
source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
372 ↗(On Diff #201515)

Can we add a test that ensures we actually display this error?

labath marked 2 inline comments as done.May 29 2019, 2:15 AM
labath added inline comments.
source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
372 ↗(On Diff #201515)

Done.

This revision was automatically updated to reflect the committed changes.
labath marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptMay 29 2019, 2:20 AM