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

[Jan Kratochvil via Phabricator via llvm-commits]

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