[llvm+lldb] Remove dead-code in DWARFListTableHeader::extract modifying DWARFDataExtractor

[jankratochvil]

Summary

@ikudrin has correctly noticed modifying a copy of DWARFDataExtractor makes no sense. Removed it as it currently has no effect. In fact one caller needs it as otherwise it assumes .debug_rnglists width from ELF width which in practice works but it is not correct. All callers of DWARFListTableHeader::extract:

• llvm/unittests/DebugInfo/DWARF/DWARFListTableTest.cpp - not using address parsing
• llvm/lib/DebugInfo/DWARF/DWARFContext.cpp dumpLoclistsSection - it uses: Data.setAddressSize(Header.getAddrSize());
• lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp SetLoclistsBase - it sets m_loclist_table_header which is never used for parsing addresses
• lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp ParseListTableHeader - called only by GetRnglistTable

Callers of GetRnglistTable:

• lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp GetRnglistOffset - not used for parsing addresses
• lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp FindRnglistFromOffset - calling findList which does not set the address size. So this part needed a fix in this patch.

[ikudrin]

llvm/unittests/DebugInfo/DWARF/DWARFListTableTest.cpp

128

This looks odd. DWARFListTableBase::findList() should not require the setting to be done in the calling code if it can apply it itself.

[jankratochvil]

llvm/unittests/DebugInfo/DWARF/DWARFListTableTest.cpp

128

I agree but as I have found it is not so simple. findList is being used also with uninitialized headers: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp DWARFUnit::findRnglistFromOffset
So I had to implement DWARFListTableBase::setAddrSize which is a bit ugly. One cannot find appropriate .debug_loclists header just for the requested offset without DW_AT_rnglists_base which may be missing, that is what D106466 is about. Or one could use DWARFListTableHeader::create from that patch but that looks to me as too much complicated.

In fact the codebase is already designed so that caller has to prepare proper DWARFDataExtractor - llvm::DWARFDebugLoclists and llvm::DWARFDebugLoc also rely on it, I tried to implement this callee approach for them as: https://www.jankratochvil.net/t/gccoffset1-2-2b.patch
But I am not going to submit it as one either has to provide address size to both DWARFDataExtractor and llvm::DWARFDebugLoc* constructors or otherwise DWARFDataExtractor would have unset address size (implemented in the patch) which is also a bit ugly.

So personally I would choose the previous patch but I am fine also with this patch.

[ikudrin]

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

285–289

if Header.Length is zero that means that there is no header discovered for the table. That can be handled accordingly and no need to fill header fields with artificial values based on a particular use.

[jankratochvil]

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

285–289

I agree, I still did not get the length==0 case, thanks for showing it.
(It would be nice to have a more simple class for that case.)

[ikudrin]

DWARFDebugInfo.TestRnglistsAddressSize, DWARFListTableHeader.AddressSize64Offset, and DWARFListTableHeader.AddressSize32Offset pass without applying the patch. Why adding them?

llvm/unittests/DebugInfo/DWARF/DWARFListTableTest.cpp

123

The rest of the test makes no sense if extractHeaderAndOffsets() returns an error.

129–130

If these checks fail, the execution of the test should be terminated.

[jankratochvil]

In D107470#2946689, @ikudrin wrote:

DWARFDebugInfo.TestRnglistsAddressSize, DWARFListTableHeader.AddressSize64Offset, and DWARFListTableHeader.AddressSize32Offset pass without applying the patch. Why adding them?

This code has shown it is prone to errors so future changes could break some other combination. Better code coverage by the testsuite is AFAIK always preferred.

lldb/unittests/SymbolFile/DWARF/DWARFUnitTest.cpp

)";
   ASSERT_TRUE((bool)t.GetDwarfUnit());
 
   DWARFUnit *unit = t.GetDwarfUnit();
   const DWARFDebugInfoEntry *die_first = unit->DIE().GetDIE();
   ASSERT_NE(die_first, nullptr);
   EXPECT_EQ(die_first->GetFirstChild(), nullptr);
   EXPECT_EQ(die_first->GetSibling(), nullptr);
 }
+
+TEST(DWARFUnitTest, Elf64RnglistsAddressSize64) {
+  // Test parsing .debug_rnglists with address size matching its ELF size.
+  const char *yamldata = R"(
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+  Entry:           0x401020
+Sections:
+  - Name:            .debug_info
+    Type:            SHT_PROGBITS
+    AddressAlign:    0x1
+    Content:         '09000000050001080000000001'
+  - Name:            .debug_abbrev
+    Type:            SHT_PROGBITS
+    AddressAlign:    0x1
+    Content:         '011100000000'
+  - Name:            .debug_rnglists
+    Type:            SHT_PROGBITS
+    AddressAlign:    0x1
+    Content:         1A0000000500080000000000060706050403020100878685848382818000
+)";
+
+  YAMLModuleTester t(yamldata);
+  ASSERT_TRUE((bool)t.GetDwarfUnit());
+
+  DWARFUnit *unit = t.GetDwarfUnit();
+  llvm::Expected<DWARFRangeList> Rnglist = unit->FindRnglistFromOffset(12);
+  ASSERT_TRUE(!!Rnglist);
+  ASSERT_EQ(Rnglist->GetSize(), 1U);
+  ASSERT_EQ(Rnglist->GetEntryRef(0).base, 0x0001020304050607U);
+  ASSERT_EQ(Rnglist->GetEntryRef(0).size, 0x8080808080808080U);
+}
+
+TEST(DWARFUnitTest, Elf64RnglistsAddressSize32) {
+  // Test parsing .debug_rnglists with address size different than its ELF size.
+  const char *yamldata = R"(
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+  Entry:           0x401020
+Sections:
+  - Name:            .debug_info
+    Type:            SHT_PROGBITS
+    AddressAlign:    0x1
+    Content:         '09000000050001040000000001'
+  - Name:            .debug_abbrev
+    Type:            SHT_PROGBITS
+    AddressAlign:    0x1
+    Content:         '011100000000'
+  - Name:            .debug_rnglists
+    Type:            SHT_PROGBITS
+    AddressAlign:    0x1
+    Content:         12000000050004000000000006030201008382818000
+)";
+
+  YAMLModuleTester t(yamldata);
+  ASSERT_TRUE((bool)t.GetDwarfUnit());
+
+  DWARFUnit *unit = t.GetDwarfUnit();
+  llvm::Expected<DWARFRangeList> Rnglist = unit->FindRnglistFromOffset(12);
+  ASSERT_TRUE(!!Rnglist);
+  ASSERT_EQ(Rnglist->GetSize(), 1U);
+  ASSERT_EQ(Rnglist->GetEntryRef(0).base, 0x00010203U);
+  ASSERT_EQ(Rnglist->GetEntryRef(0).size, 0x80808080U);
+}

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

public:
   }
   /// Extract the table header and the array of offsets.
   Error extractHeaderAndOffsets(DWARFDataExtractor Data, uint64_t *OffsetPtr) {
     return Header.extract(Data, OffsetPtr);
   }
   /// Extract an entire table, including all list entries.
   Error extract(DWARFDataExtractor Data, uint64_t *OffsetPtr);
   /// Look up a list based on a given offset. Extract it and enter it into the
-  /// list map if necessary.
+  /// list map if necessary. \a Data setAddressSize needs to be set iff
+  /// neither extractHeaderAndOffsets nor extract were called.
   Expected<DWARFListType> findList(DWARFDataExtractor Data,
                                    uint64_t Offset) const;
 
   uint64_t getHeaderOffset() const { return Header.getHeaderOffset(); }
   uint8_t getAddrSize() const { return Header.getAddrSize(); }
   uint32_t getOffsetEntryCount() const { return Header.getOffsetEntryCount(); }
   dwarf::DwarfFormat getFormat() const { return Header.getFormat(); }
 

 }
 
 template <typename DWARFListType>
 Expected<DWARFListType>
 DWARFListTableBase<DWARFListType>::findList(DWARFDataExtractor Data,
                                             uint64_t Offset) const {
   // Extract the list from the section and enter it into the list map.
   DWARFListType List;
-  if (Header.length())
+  if (Header.length()) {
     Data = DWARFDataExtractor(Data, getHeaderOffset() + Header.length());
+    Data.setAddressSize(getAddrSize());
+  }
   if (Error E =
           List.extract(Data, Header.length() ? getHeaderOffset() : 0, &Offset,
                        Header.getSectionName(), Header.getListTypeString()))
     return std::move(E);
   return List;
 }
 
 } // end namespace llvm
 
 #endif // LLVM_DEBUGINFO_DWARF_DWARFLISTTABLE_H

llvm/lib/DebugInfo/DWARF/DWARFListTable.cpp

return createStringError(errc::not_supported,
                        " has unsupported segment selector size %" PRIu8,
                        SectionName.data(), HeaderOffset, HeaderData.SegSize);
   if (End < HeaderOffset + getHeaderSize(Format) +
                 HeaderData.OffsetEntryCount * OffsetByteSize)
     return createStringError(errc::invalid_argument,
         "%s table at offset 0x%" PRIx64 " has more offset entries (%" PRIu32
         ") than there is space for",
         SectionName.data(), HeaderOffset, HeaderData.OffsetEntryCount);
-  Data.setAddressSize(HeaderData.AddrSize);
   *OffsetPtr += HeaderData.OffsetEntryCount * OffsetByteSize;
   return Error::success();
 }
 
 void DWARFListTableHeader::dump(DataExtractor Data, raw_ostream &OS,
                                 DIDumpOptions DumpOpts) const {
   if (DumpOpts.Verbose)
     OS << format("0x%8.8" PRIx64 ": ", HeaderOffset);

llvm/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp

DWARFCompileUnit CU(*Context, Sec, Header, /* DA = */ 0, /* RS = */ 0,
                         /* LocSection = */ 0, /* SS = */ StringRef(),
                         /* SOS = */ DummySec, /* AOS = */ 0,
                         /* LS = */ DummySec, /* LE = */ true,
                         /* isDWO= */ false, DummyUnitVector);
     ASSERT_EQ(0x1122334455667788ULL, CU.getLength());
   });
 }
 
+TEST(DWARFDebugInfo, TestRnglistsAddressSize) {
+  static const char DebugInfoSecRaw[] =
+      "\x09\x00\x00\x00"                             // Length
+      "\x05\x00"                                     // Version
+      "\x01"                                         // DW_UT_compile
+      "\x08"                                         // Address size
+      "\x00\x00\x00\x00"                             // Offset Into Abbrev. Sec.
+      "\x01";                                        // Abbreviation 1
+  static const char DebugAbbrevSecRaw[] = "\x01"     // Abbreviation Code
+                                          "\x11"     // DW_TAG_compile_unit
+                                          "\x00"     // DW_CHILDREN_no
+                                          "\x00\x00" // End of Abbreviation 1
+                                          "\x00";    // End of Abbreviations
+  static const char DebugRnglistsSecRaw[] =
+      "\x1A\x00\x00\x00"                 // Length
+      "\x05\x00"                         // Version
+      "\x08"                             // Address Size
+      "\x00"                             // Segment Selector Size
+      "\x00\x00\x00\x00"                 // Offset Entry Count
+      "\x06"                             // DW_RLE_start_end
+      "\x07\x06\x05\x04\x03\x02\x01\x00" // Start Address
+      "\x87\x86\x85\x84\x83\x82\x81\x80" // End Address
+      "\x00";                            // DW_RLE_end_of_list
+
+  StringMap<std::unique_ptr<MemoryBuffer>> Sections;
+  Sections.insert(std::make_pair(
+      "debug_info", MemoryBuffer::getMemBuffer(StringRef(
+                        DebugInfoSecRaw, sizeof(DebugInfoSecRaw) - 1))));
+  Sections.insert(std::make_pair(
+      "debug_abbrev", MemoryBuffer::getMemBuffer(StringRef(
+                          DebugAbbrevSecRaw, sizeof(DebugAbbrevSecRaw) - 1))));
+  Sections.insert(std::make_pair(
+      "debug_rnglists",
+      MemoryBuffer::getMemBuffer(
+          StringRef(DebugRnglistsSecRaw, sizeof(DebugRnglistsSecRaw) - 1))));
+  auto Context = DWARFContext::create(Sections, /* AddrSize = */ 8,
+                                      /* isLittleEndian = */ true);
+  const auto &Obj = Context->getDWARFObj();
+  Obj.forEachInfoSections([&](const DWARFSection &Sec) {
+    DWARFUnitHeader Header;
+    DWARFDataExtractor Data(Obj, Sec, /* IsLittleEndian = */ true,
+                            /* AddressSize = */ 4);
+    uint64_t Offset = 0;
+    EXPECT_TRUE(Header.extract(*Context, Data, &Offset, DW_SECT_INFO));
+    ASSERT_EQ(8, Header.getAddressByteSize());
+
+    // Check that the length can be correctly read in the unit class.
+    DWARFUnitVector DummyUnitVector;
+    DWARFSection DummySec;
+    DWARFDataExtractor AbbrevData(Obj.getAbbrevSection(),
+                                  /* IsLittleEndian = */ true,
+                                  /* AddressSize = */ 4);
+    DWARFDebugAbbrev Abbrev;
+    Abbrev.extract(AbbrevData);
+    Abbrev.parse();
+    DWARFCompileUnit CU(*Context, Sec, Header, /* DA = */ &Abbrev,
+                        /* RS = */ &Obj.getRnglistsSection(),
+                        /* LocSection = */ 0, /* SS = */ StringRef(),
+                        /* SOS = */ DummySec, /* AOS = */ 0,
+                        /* LS = */ DummySec, /* LE = */ true,
+                        /* isDWO= */ false, DummyUnitVector);
+    llvm::Expected<DWARFAddressRangesVector> Rnglist =
+        CU.findRnglistFromOffset(12);
+    ASSERT_TRUE(!!Rnglist);
+    ASSERT_EQ(Rnglist->size(), 1U);
+    ASSERT_EQ(Rnglist->at(0).LowPC, 0x0001020304050607U);
+    ASSERT_EQ(Rnglist->at(0).HighPC, 0x8081828384858687U);
+  });
+}
+
 } // end anonymous namespace

llvm/unittests/DebugInfo/DWARF/DWARFListTableTest.cpp

 //===- DWARFListTableTest.cpp ---------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/DebugInfo/DWARF/DWARFListTable.h"
+#include "llvm/DebugInfo/DWARF/DWARFDebugRnglists.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 
 namespace {
 
 TEST(DWARFListTableHeader, TruncatedLength) {
TEST(DWARFListTableHeader, OffsetEntryCount) {
   Optional<uint64_t> Offset0 = Header.getOffsetEntry(Extractor, 0);
   EXPECT_TRUE(!!Offset0);
   EXPECT_EQ(Offset0, uint64_t(4));
   Optional<uint64_t> Offset1 = Header.getOffsetEntry(Extractor, 1);
   EXPECT_FALSE(!!Offset1);
   EXPECT_EQ(Header.length(), sizeof(SecData) - 1);
 }
 
+TEST(DWARFListTableHeader, AddressSize64Header) {
+  static const char SecData[] = "\x1a\x00\x00\x00" // Length
+                                "\x05\x00"         // Version
+                                "\x08"             // Address size
+                                "\x00"             // Segment selector size
+                                "\x00\x00\x00\x00" // Offset entry count
+                                "\x06"             // DW_RLE_start_end
+                                "\x07\x06\x05\x04" // 64-bit starting address
+                                "\x03\x02\x01\x00" // 64-bit starting address
+                                "\x87\x86\x85\x84" // 64-bit ending address
+                                "\x83\x82\x81\x80" // 64-bit ending address
+                                "\x00";            // DW_RLE_end_of_list
+  DWARFDataExtractor Extractor(StringRef(SecData, sizeof(SecData) - 1),
+                               /*isLittleEndian=*/true,
+                               /*AddrSize=*/4);
+  DWARFListTableHeader Header(/*SectionName=*/".debug_rnglists",
+                              /*ListTypeString=*/"range");
+  uint64_t Offset = 0;
+  llvm::DWARFDebugRnglistTable Table;
+  llvm::Error E = Table.extractHeaderAndOffsets(Extractor, &Offset);
+  EXPECT_FALSE(!!E);
+  EXPECT_EQ(Offset, 12U);
+  EXPECT_EQ(Table.length(), sizeof(SecData) - 1);
+  EXPECT_EQ(Extractor.getAddressSize(), 4U);
+  EXPECT_EQ(Table.getAddrSize(), 8U);
+  Expected<DWARFDebugRnglist> List = Table.findList(Extractor, Offset);
+  EXPECT_TRUE(!!List);
+  EXPECT_EQ(List->getEntries().size(), 2U);
+  EXPECT_EQ(List->getEntries()[0].Offset, 12 + 0U);
+  EXPECT_EQ(List->getEntries()[0].EntryKind, dwarf::DW_RLE_start_end);
+  EXPECT_EQ(List->getEntries()[0].Value0, 0x0001020304050607U);
+  EXPECT_EQ(List->getEntries()[0].Value1, 0x8081828384858687U);
+  EXPECT_EQ(List->getEntries()[1].Offset, 12 + 17U);
+  EXPECT_EQ(List->getEntries()[1].EntryKind, dwarf::DW_RLE_end_of_list);
+}
+
+TEST(DWARFListTableHeader, AddressSize64Offset) {
+  static const char SecData[] = "\x1a\x00\x00\x00" // Length
+                                "\x05\x00"         // Version
+                                "\x08"             // Address size
+                                "\x00"             // Segment selector size
+                                "\x00\x00\x00\x00" // Offset entry count
+                                "\x06"             // DW_RLE_start_end
+                                "\x07\x06\x05\x04" // 64-bit starting address
+                                "\x03\x02\x01\x00" // 64-bit starting address
+                                "\x87\x86\x85\x84" // 64-bit ending address
+                                "\x83\x82\x81\x80" // 64-bit ending address
+                                "\x00";            // DW_RLE_end_of_list
+  DWARFDataExtractor Extractor(StringRef(SecData, sizeof(SecData) - 1),
+                               /*isLittleEndian=*/true,
+                               /*AddrSize=*/4);
+  DWARFListTableHeader Header(/*SectionName=*/".debug_rnglists",
+                              /*ListTypeString=*/"range");
+  llvm::DWARFDebugRnglistTable Table;
+  // Table.getAddrSize() is uninitialized.
+  EXPECT_EQ(Extractor.getAddressSize(), 4U);
+  Extractor.setAddressSize(8);
+  Expected<DWARFDebugRnglist> List = Table.findList(Extractor, 12);
+  EXPECT_TRUE(!!List);
+  EXPECT_EQ(List->getEntries().size(), 2U);
+  EXPECT_EQ(List->getEntries()[0].Offset, 12 + 0U);
+  EXPECT_EQ(List->getEntries()[0].EntryKind, dwarf::DW_RLE_start_end);
+  EXPECT_EQ(List->getEntries()[0].Value0, 0x0001020304050607U);
+  EXPECT_EQ(List->getEntries()[0].Value1, 0x8081828384858687U);
+  EXPECT_EQ(List->getEntries()[1].Offset, 12 + 17U);
+  EXPECT_EQ(List->getEntries()[1].EntryKind, dwarf::DW_RLE_end_of_list);
+}
+
+TEST(DWARFListTableHeader, AddressSize32Header) {
+  static const char SecData[] = "\x12\x00\x00\x00" // Length
+                                "\x05\x00"         // Version
+                                "\x04"             // Address size
+                                "\x00"             // Segment selector size
+                                "\x00\x00\x00\x00" // Offset entry count
+                                "\x06"             // DW_RLE_start_end
+                                "\x03\x02\x01\x00" // 32-bit starting address
+                                "\x83\x82\x81\x80" // 32-bit ending address
+                                "\x00";            // DW_RLE_end_of_list
+  DWARFDataExtractor Extractor(StringRef(SecData, sizeof(SecData) - 1),
+                               /*isLittleEndian=*/true,
+                               /*AddrSize=*/8);
+  DWARFListTableHeader Header(/*SectionName=*/".debug_rnglists",
+                              /*ListTypeString=*/"range");
+  uint64_t Offset = 0;
+  llvm::DWARFDebugRnglistTable Table;
+  llvm::Error E = Table.extractHeaderAndOffsets(Extractor, &Offset);
+  EXPECT_FALSE(!!E);
+  EXPECT_EQ(Offset, 12U);
+  EXPECT_EQ(Table.length(), sizeof(SecData) - 1);
+  EXPECT_EQ(Extractor.getAddressSize(), 8U);
+  EXPECT_EQ(Table.getAddrSize(), 4U);
+  Expected<DWARFDebugRnglist> List = Table.findList(Extractor, Offset);
+  EXPECT_TRUE(!!List);
+  EXPECT_EQ(List->getEntries().size(), 2U);
+  EXPECT_EQ(List->getEntries()[0].Offset, 12 + 0U);
+  EXPECT_EQ(List->getEntries()[0].EntryKind, dwarf::DW_RLE_start_end);
+  EXPECT_EQ(List->getEntries()[0].Value0, 0x00010203U);
+  EXPECT_EQ(List->getEntries()[0].Value1, 0x80818283U);
+  EXPECT_EQ(List->getEntries()[1].Offset, 12 + 9U);
+  EXPECT_EQ(List->getEntries()[1].EntryKind, dwarf::DW_RLE_end_of_list);
+}
+
+TEST(DWARFListTableHeader, AddressSize32Offset) {
+  static const char SecData[] = "\x12\x00\x00\x00" // Length
+                                "\x05\x00"         // Version
+                                "\x04"             // Address size
+                                "\x00"             // Segment selector size
+                                "\x00\x00\x00\x00" // Offset entry count
+                                "\x06"             // DW_RLE_start_end
+                                "\x03\x02\x01\x00" // 32-bit starting address
+                                "\x83\x82\x81\x80" // 32-bit ending address
+                                "\x00";            // DW_RLE_end_of_list
+  DWARFDataExtractor Extractor(StringRef(SecData, sizeof(SecData) - 1),
+                               /*isLittleEndian=*/true,
+                               /*AddrSize=*/8);
+  DWARFListTableHeader Header(/*SectionName=*/".debug_rnglists",
+                              /*ListTypeString=*/"range");
+  llvm::DWARFDebugRnglistTable Table;
+  // Table.getAddrSize() is uninitialized.
+  EXPECT_EQ(Extractor.getAddressSize(), 8U);
+  Extractor.setAddressSize(4);
+  Expected<DWARFDebugRnglist> List = Table.findList(Extractor, 12);
+  EXPECT_TRUE(!!List);
+  EXPECT_EQ(List->getEntries().size(), 2U);
+  EXPECT_EQ(List->getEntries()[0].Offset, 12 + 0U);
+  EXPECT_EQ(List->getEntries()[0].EntryKind, dwarf::DW_RLE_start_end);
+  EXPECT_EQ(List->getEntries()[0].Value0, 0x00010203U);
+  EXPECT_EQ(List->getEntries()[0].Value1, 0x80818283U);
+  EXPECT_EQ(List->getEntries()[1].Offset, 12 + 9U);
+  EXPECT_EQ(List->getEntries()[1].EntryKind, dwarf::DW_RLE_end_of_list);
+}
+
 } // end anonymous namespace