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

[jankratochvil]

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

[labath]

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.

[jankratochvil]

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.

[jankratochvil]

Ah there is already a discussion from it: http://lists.dwarfstd.org/pipermail/dwarf-discuss-dwarfstd.org/2021-March/004765.html

[dblaikie]

In D98289#2618881, @jankratochvil wrote:

Ah there is already a discussion from it: http://lists.dwarfstd.org/pipermail/dwarf-discuss-dwarfstd.org/2021-March/004765.html

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

[jankratochvil]

llvm/include/llvm/DebugInfo/DWARF/DWARFListTable.h

116 ↗

(On Diff #330181)

A regression from: https://reviews.llvm.org/D86110#2621583

[dblaikie]

llvm/include/llvm/DebugInfo/DWARF/DWARFListTable.h

116 ↗

(On Diff #330181)

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]

The testcase assumes checked-in D98589.

[jankratochvil]

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

494

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

[clayborg]

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

494

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?

495

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

496–499

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

513–518

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

[dblaikie]

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]

Thanks for the review.

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

494

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

495

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.

496–499

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.

513–518

No, answered above.

[jankratochvil]

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]

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

[jankratochvil]

ping#2

[clayborg]

No objections from me. Anyone else?

[jankratochvil]

In D98289#2764829, @clayborg wrote:

No objections from me. Anyone else?

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

[jankratochvil]

lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s

33

This is now implemented by D102851.

lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h

protected:
   dw_addr_t m_ranges_base = 0;   ///< Value of DW_AT_rnglists_base.
 
   /// Value of DW_AT_stmt_list.
   dw_offset_t m_line_table_offset = DW_INVALID_OFFSET;
 
   dw_offset_t m_str_offsets_base = 0; // Value of DW_AT_str_offsets_base.
 
   llvm::Optional<llvm::DWARFDebugRnglistTable> m_rnglist_table;
+  bool m_rnglist_table_done = false;
   llvm::Optional<llvm::DWARFListTableHeader> m_loclist_table_header;
 
   const DIERef::Section m_section;
   bool m_is_dwo;
   /// Value of DW_AT_GNU_dwo_id (v4) or dwo_id from CU header (v5).
   uint64_t m_dwo_id;
 
 private:

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

if (const auto *contribution = entry->getContribution(llvm::DW_SECT_EXT_LOC))
       return DWARFDataExtractor(data, contribution->Offset,
                                 contribution->Length);
     return DWARFDataExtractor();
   }
   return data;
 }
 
 void DWARFUnit::SetRangesBase(dw_addr_t ranges_base) {
-  m_ranges_base = ranges_base;
+  lldbassert(!m_rnglist_table_done);
 
-  if (GetVersion() < 5)
-    return;
+  m_ranges_base = ranges_base;
+}
 
-  if (auto table_or_error = ParseListTableHeader<llvm::DWARFDebugRnglistTable>(
+const llvm::Optional<llvm::DWARFDebugRnglistTable> &DWARFUnit::GetRnglist() {
+  if (GetVersion() >= 5 && !m_rnglist_table_done) {
+    m_rnglist_table_done = true;
+    if (auto table_or_error =
+            ParseListTableHeader<llvm::DWARFDebugRnglistTable>(
                 m_dwarf.GetDWARFContext().getOrLoadRngListsData().GetAsLLVM(),
-          ranges_base, DWARF32))
+                m_ranges_base, DWARF32))
       m_rnglist_table = std::move(table_or_error.get());
     else
       GetSymbolFileDWARF().GetObjectFile()->GetModule()->ReportError(
           "Failed to extract range list table at offset 0x%" PRIx64 ": %s",
-        ranges_base, toString(table_or_error.takeError()).c_str());
+          m_ranges_base, toString(table_or_error.takeError()).c_str());
   }
-
-const llvm::Optional<llvm::DWARFDebugRnglistTable> &DWARFUnit::GetRnglist() {
   return m_rnglist_table;
 }
 
+// This function is called only for DW_FORM_rnglistx.
 llvm::Optional<uint64_t> DWARFUnit::GetRnglistOffset(uint32_t Index) {
   if (!GetRnglist())
     return llvm::None;
+  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;
+  }
   if (llvm::Optional<uint64_t> off = GetRnglist()->getOffsetEntry(
           m_dwarf.GetDWARFContext().getOrLoadRngListsData().GetAsLLVM(), Index))
     return *off + m_ranges_base;
   return llvm::None;
 }
 
 void DWARFUnit::SetStrOffsetsBase(dw_offset_t str_offsets_base) {
   m_str_offsets_base = str_offsets_base;

lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s

+# 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.
+
+# REQUIRES: x86
+
+# RUN: llvm-mc -triple=x86_64-pc-linux -filetype=obj %s > %t
+# RUN: %lldb %t -o "image lookup -v -s lookup_rnglists" \
+# RUN:   -o exit | FileCheck %s
+
+# Failure was the block range 1..2 was not printed plus:
+# error: DW_AT_range-DW_FORM_sec_offset.s.tmp {0x0000003f}: DIE has DW_AT_ranges(0xc) attribute, but range extraction failed (missing or invalid range list table), please file a bug and attach the file at the start of this error message
+
+# CHECK-LABEL: image lookup -v -s lookup_rnglists
+# CHECK:  Function: id = {0x00000029}, name = "rnglists", range = [0x0000000000000000-0x0000000000000003)
+# CHECK:    Blocks: id = {0x00000029}, range = [0x00000000-0x00000003)
+# CHECK-NEXT:       id = {0x0000003f}, range = [0x00000001-0x00000002)
+
+# RUN: llvm-mc -triple=x86_64-pc-linux -filetype=obj \
+# RUN:   --defsym RNGLISTX=0 %s > %t-rnglistx
+# RUN: %lldb %t-rnglistx -o "image lookup -v -s lookup_rnglists" \
+# RUN:   -o exit 2>&1 | FileCheck --check-prefix=RNGLISTX %s
+
+# RNGLISTX-LABEL: image lookup -v -s lookup_rnglists
+# RNGLISTX: error: {{.*}} 00000000: DW_FORM_rnglistx cannot be used without DW_AT_rnglists_base
+
+# RUN: llvm-mc -triple=x86_64-pc-linux -filetype=obj \
+# RUN:   --defsym RNGLISTX=0 --defsym RNGLISTBASE=0 %s > %t-rnglistbase
+# RUN: %lldb %t-rnglistbase -o "image lookup -v -s lookup_rnglists" \
+# RUN:   -o exit 2>&1 | FileCheck --check-prefix=RNGLISTBASE %s
+
+# RNGLISTBASE-LABEL: image lookup -v -s lookup_rnglists
+# RNGLISTBASE: error: {{.*}}-rnglistbase {0x00000043}: DIE has DW_AT_ranges(0x0) attribute, but range extraction failed (invalid range list table index 0), please file a bug and attach the file at the start of this error message
+
+        .text
+rnglists:
+        nop
+.Lblock1_begin:
+lookup_rnglists:
+        nop
+.Lblock1_end:
+        nop
+.Lrnglists_end:
+
+        .section        .debug_abbrev,"",@progbits
+        .byte   1                       # Abbreviation Code
+        .byte   17                      # DW_TAG_compile_unit
+        .byte   1                       # DW_CHILDREN_yes
+        .byte   37                      # DW_AT_producer
+        .byte   8                       # DW_FORM_string
+        .byte   17                      # DW_AT_low_pc
+        .byte   27                      # DW_FORM_addrx
+        .byte   18                      # DW_AT_high_pc
+        .byte   6                       # DW_FORM_data4
+        .byte   115                     # DW_AT_addr_base
+        .byte   23                      # DW_FORM_sec_offset
+.ifdef RNGLISTBASE
+        .byte   0x74                    # DW_AT_rnglists_base
+        .byte   23                      # DW_FORM_sec_offset
+.endif
+        .byte   0                       # EOM(1)
+        .byte   0                       # EOM(2)
+        .byte   2                       # Abbreviation Code
+        .byte   46                      # DW_TAG_subprogram
+        .byte   1                       # DW_CHILDREN_yes
+        .byte   17                      # DW_AT_low_pc
+        .byte   1                       # DW_FORM_addr
+        .byte   18                      # DW_AT_high_pc
+        .byte   6                       # DW_FORM_data4
+        .byte   3                       # DW_AT_name
+        .byte   8                       # DW_FORM_string
+        .byte   0                       # EOM(1)
+        .byte   0                       # EOM(2)
+        .byte   5                       # Abbreviation Code
+        .byte   11                      # DW_TAG_lexical_block
+        .byte   0                       # DW_CHILDREN_no
+        .byte   85                      # DW_AT_ranges
+.ifndef RNGLISTX
+        .byte   0x17                    # DW_FORM_sec_offset
+.else
+        .byte   0x23                    # DW_FORM_rnglistx
+.endif
+        .byte   0                       # EOM(1)
+        .byte   0                       # EOM(2)
+        .byte   0                       # EOM(3)
+
+        .section        .debug_info,"",@progbits
+.Lcu_begin0:
+        .long   .Ldebug_info_end0-.Ldebug_info_start0 # Length of Unit
+.Ldebug_info_start0:
+        .short  5                       # DWARF version number
+        .byte   1                       # DWARF Unit Type
+        .byte   8                       # Address Size (in bytes)
+        .long   .debug_abbrev           # Offset Into Abbrev. Section
+        .byte   1                       # Abbrev [1] 0xc:0x5f DW_TAG_compile_unit
+        .asciz  "Hand-written DWARF"    # DW_AT_producer
+        .byte   0                       # DW_AT_low_pc
+        .long   .Lrnglists_end-rnglists # DW_AT_high_pc
+        .long   .Laddr_table_base0      # DW_AT_addr_base
+.ifdef RNGLISTBASE
+        .long   .Ldebug_ranges0         # DW_AT_rnglists_base
+.endif
+        .byte   2                       # Abbrev [2] 0x2b:0x37 DW_TAG_subprogram
+        .quad   rnglists                # DW_AT_low_pc
+        .long   .Lrnglists_end-rnglists # DW_AT_high_pc
+        .asciz  "rnglists"              # DW_AT_name
+        .byte   5                       # Abbrev [5] 0x52:0xf DW_TAG_lexical_block
+.ifndef RNGLISTX
+        .long   .Ldebug_ranges0         # DW_AT_ranges DW_FORM_sec_offset
+.else
+        .uleb128 0                      # DW_AT_ranges DW_FORM_rnglistx
+.endif
+        .byte   0                       # End Of Children Mark
+        .byte   0                       # End Of Children Mark
+.Ldebug_info_end0:
+
+        .section        .debug_addr,"",@progbits
+        .long   .Ldebug_addr_end0-.Ldebug_addr_start0 # Length of contribution
+.Ldebug_addr_start0:
+        .short  5                       # DWARF version number
+        .byte   8                       # Address size
+        .byte   0                       # Segment selector size
+.Laddr_table_base0:
+        .quad   rnglists
+.Ldebug_addr_end0:
+
+        .section        .debug_rnglists,"",@progbits
+        .long   .Ldebug_rnglist_table_end0-.Ldebug_rnglist_table_start0 # Length
+.Ldebug_rnglist_table_start0:
+        .short  5                       # Version
+        .byte   8                       # Address size
+        .byte   0                       # Segment selector size
+        .long   0                       # Offset entry count
+.Ldebug_ranges0:
+        .byte   4                       # DW_RLE_offset_pair
+        .uleb128 .Lblock1_begin-rnglists  #   starting offset
+        .uleb128 .Lblock1_end-rnglists    #   ending offset
+        .byte   0                       # DW_RLE_end_of_list
+.Ldebug_rnglist_table_end0: