Page MenuHomePhabricator

[lldb] 2/2: Fix DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC)
ClosedPublic

Authored by jankratochvil on Mar 9 2021, 1:03 PM.

Details

Summary

DW_AT_ranges can use DW_FORM_sec_offset (instead of DW_FORM_rnglistx). In such case DW_AT_rnglists_base does not need to be present.
DWARF-5 spec:

"If the offset_entry_count is zero, then DW_FORM_rnglistx cannot be used to access a range list; DW_FORM_sec_offset must be used instead. If the offset_entry_count is non-zero, then DW_FORM_rnglistx may be used to access a range list;"

This fix is for TestTypeCompletion.py category dwarf using GCC with DWARF-5.
TestTypeCompletion.py category dwo needs a pending GCC fix: -gdwarf-5 -gsplit-dwarf puts .debug_rnglists to main file, not .dwo file

The fix just provides GetRnglist() lazy getter for m_rnglist_table.
The testcase is easier to review by: diff -u lldb/test/Shell/SymbolFile/DWARF/DW_AT_low_pc-addrx.s lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s

Diff Detail

Event Timeline

jankratochvil created this revision.Mar 9 2021, 1:03 PM
jankratochvil requested review of this revision.Mar 9 2021, 1:03 PM
jankratochvil planned changes to this revision.Mar 9 2021, 2:09 PM

I'm not sure what are the "planned changes", but have you compared this implementation with the one in llvm. IIRC, I tried to implement it the same one as the one over there, but it seems the llvm one has changed since then. At a cursory glance it looks like it does not suffer from this problem, so it would be preferable if we could fix this problem, by making our implementation be more like the llvm one.

There is some discussion what is valid vs. invalid DWARF-5 and whether DWARF-5 needs some updates: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99490
The planned change is that with this version of the patch m_ranges_base defauled to 0 but my idea for the 'planned change' was it should default to an invalid value (permitting DW_FORM_sec_offset but not DW_FORM_rnglistx). But then I found LLVM DWOs default to llvm::DWARFListTableHeader::getHeaderSize(DWARF32) which is nowhere stated in DWARF-5. And the GCC DWOs are somehow overall broken.
So probably going to propose the LLVM behavior for DWARF-6. I will check the LLVM implementation, thanks for the hint.

Yeah, happy to discuss/clarify/help with anything in this front either on that thread, in this review, or elsewhere.

Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2021, 2:10 AM
jankratochvil added inline comments.Mar 12 2021, 2:10 AM
llvm/include/llvm/DebugInfo/DWARF/DWARFListTable.h
116
Harbormaster completed remote builds in B93447: Diff 330185.
dblaikie added inline comments.Mar 12 2021, 1:50 PM
llvm/include/llvm/DebugInfo/DWARF/DWARFListTable.h
116

Changes to LLVM should have tests in LLVM - probably best to split this out as a separate patch to LLVM only, if you can?

jankratochvil edited the summary of this revision. (Show Details)

The testcase assumes checked-in D98589.

jankratochvil marked an inline comment as done.Mar 13 2021, 2:01 PM
jankratochvil retitled this revision from [lldb] Fix DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC) to [lldb] 2/2: Fix DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC).
jankratochvil added a reviewer: clayborg.
jankratochvil removed a project: Restricted Project.
jankratochvil planned changes to this revision.Mar 31 2021, 9:31 AM
jankratochvil added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
522

if (GetVersion() < 5) return llvm::None; is no longer possible with the returned reference.

clayborg added inline comments.Apr 1 2021, 10:34 AM
lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
522

Do we actually need "m_rnglist_table_done" here? Can't we just check if m_rnglist_table has a value since it is an optional?

522–527

Do we need this check? We should probably be checking "m_ranges_base" in the DWARFUnit::GetRnglist() and report the error there?

523

Should we check m_ranges_base here to make sure it has a value?

524–527

Will this function call return an error if "m_ranges_base" doesn't have a value?

dblaikie added inline comments.Apr 1 2021, 5:28 PM
lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s
12

I'm not sure if %t is guaranteed to produce a file name with that format/name. So maybe using {{.*}} for the file name (and {{.*}}-rnglistx below) would be a bit more stable

33

Perhaps this could be more informative about what makes the range list index of 0 invalid? "index 0 out of range of range list table (with range list base 0xXXX) with offset entry count of XX (valid indexes 0-(XX-1))" Maybe that's too verbose/not worth worrying about since this'll only be relevant to DWARF producers trying to debug their DWARFv5, maybe no one will ever see this message in practice. Just a thought.

jankratochvil marked an inline comment as done.Apr 2 2021, 1:28 PM

Thanks for the review.

lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
522

Without m_rnglist_table_done during missing or invalid .debug_rnglists section we would be wasting time trying to parse it again and again printing Failed to extract range list table at offset each time we find another DW_FORM_rnglistx. An extra bool is ugly, we need states:

  • not yet parsed (no value)
  • parsed with error or missing (no value)
  • parsed and holding its value
522–527

No, answered above.

523

That would fail this new testcase. m_ranges_base being set is verified in DWARFUnit::GetRnglistOffset. But DWARFUnit::FindRnglistFromOffset may use default m_ranges_base (which is zero) which is called by DWARFUnit::FindRnglistFromOffset which is called by GetRangesOrReportError which is used for DW_AT_ranges which does not require DW_AT_rnglists_base (or some other kind of its specification) when DW_AT_ranges is using DW_FORM_sec_offset.
Also it would IMO break D24514 but there is missing a DWARF .s testcase and its .c testcase is no longer useful with current compilers. I haven't tried to code a new DWARF .s testcase for D24514.

524–527

No, its intentional default value 0 is interpreted as from the start of the .debug_ranges/.debug_rnglists section (after its header).
I understand the code is a bit intertwined supporting all the historical plus current formats.

jankratochvil marked an inline comment as done.
jankratochvil marked an inline comment as done.
jankratochvil added inline comments.
lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s
12

I agree, fixed.

33

I have written it to my TODO list as it is orthogonal to this patch.

jankratochvil marked an inline comment as done.Apr 10 2021, 8:21 AM

Sorry for the ping but is there anything left to clarify or fix?

No objections from me. Anyone else?

This revision was not accepted when it landed; it landed in state Needs Review.May 19 2021, 6:57 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.

No objections from me. Anyone else?

So I have checked it in, thanks for checking it.

jankratochvil marked an inline comment as done.
jankratochvil added inline comments.
lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s
33

This is now implemented by D102851.