jankratochvil marked an inline comment as done.
jankratochvil added a comment.
Thanks for the review.
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:489
const llvm::Optional<llvm::DWARFDebugRnglistTable> &DWARFUnit::GetRnglist() {
+ if (GetVersion() >= 5 && !m_rnglist_table_done) {
+ m_rnglist_table_done = true;
----------------
clayborg wrote:
> jankratochvil wrote:
> > `if (GetVersion() < 5) return llvm::None;` is no longer possible with the
> > returned reference.
> 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?
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
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:490
+ if (GetVersion() >= 5 && !m_rnglist_table_done) {
+ m_rnglist_table_done = true;
+ if (auto table_or_error =
----------------
clayborg wrote:
> Should we check m_ranges_base here to make sure it has a value?
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.
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:491-494
+ if (auto table_or_error =
+ ParseListTableHeader<llvm::DWARFDebugRnglistTable>(
+ m_dwarf.GetDWARFContext().getOrLoadRngListsData().GetAsLLVM(),
+ m_ranges_base, DWARF32))
----------------
clayborg wrote:
> Will this function call return an error if "m_ranges_base" doesn't have a
> value?
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.
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:508-513
+ if (!m_ranges_base) {
+ GetSymbolFileDWARF().GetObjectFile()->GetModule()->ReportError(
+ "%8.8x: DW_FORM_rnglistx cannot be used without DW_AT_rnglists_base",
+ GetOffset());
+ return llvm::None;
+ }
----------------
clayborg wrote:
> Do we need this check? We should probably be checking "m_ranges_base" in the
> DWARFUnit::GetRnglist() and report the error there?
No, answered above.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98289/new/
https://reviews.llvm.org/D98289
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
