[DebugInfo] Support 64-bit DWARF for .debug_names.

[probinson]

Some parts of this could be extracted into a separate NFC (or mostly NFC) patch:

• The Dwarf.h part
• In DWARFAcceleratorTable.h, merging the Header and HeaderPOD structs, and removing the Padding field. (Adding the Format field would still be part of the functional patch.)
• In DWARFAcceleratorTable.cpp, eliminating the Padding field, and introducing CommonHeaderSize in DWARFDebugNames::Header::extract()

That would make the rest of the patch a little more focused. (Which overall looks pretty reasonable, although I'd prefer to wait for the next round before approving.)

llvm/lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp

380

Could this be constexpr ?

628

Is this line too long? (Did you run clang-format-diff?)

[ikudrin]

• Extract accompanying fixes into separate patches.
• Make expressions constexpr where possible.

[ikudrin]

llvm/lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp

380

Done. Thanks!

628

It is exactly 80 characters long. And yes, it is clang-format which made this formatting.

[ikudrin]

• Rebase, update.

DWARF64HeaderFixedPartSize cannot be constexpr because dwarf::getUnitLengthFieldByteSize() connot be constexpr because not all compilers allow llvm_unreachanble() to be used in a constexpr function.

[ikudrin]

Ping.

[dblaikie]

llvm/lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp

394–397

Perhaps this could be refactored to common the length test?

uint64_t StartingOffset = *Offset);
UnitLength = ...;
Format = UnitLength == dwarf::DW_LENGTH_DWARF64 ? dwarf::DWARF64 : dwarf::DWARF32;
if (!AS.isValidOffsetForDataOfSize(StartingOffset, dwarf::getUnitLengthFieldByteSize(Format) + CommonHeaderSize))
  return createStringError(...);
if (Format == dwarf::DWARF64) {
  UnitLength = AS.getU64(Offset);
} else if (...) {
  return createStringError(...);
}

I'm not super convinced it's so much better - but it does avoid the quirky "- 4" and such.

Alternatively we could skip all this and use the Error second parameter to the DataExtractor functions, and handle the error after the read attempts? More like the way DWARFUnitHeader::extract works, for example.

[ikudrin]

• Avoid the quirky "- 4" by using StartingOffset.

@dblaikie, what do you think about this variant?

[ikudrin]

llvm/lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp

394–397

I like the idea to check the remaining length before reading more than using the Error parameter. That makes the code more straightforward and allows producing more expressive error messages. So, for my taste, I would better rework DWARFUnitHeader::extract().

[ikudrin]

• Make the checking for the remaining length closer to what @dblaikie suggested.

David, which variant looks better to you?

[dblaikie]

Close enough. Whole thing'll hopefully be revisited/refactored with a constrained DWARFDataExtractor at some point soon (@labath - FYI)

llvm/include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h

 class DWARFDebugNames : public DWARFAcceleratorTable {
 public:
   class NameIndex;
   class NameIterator;
   class ValueIterator;
 
   /// DWARF v5 Name Index header.
   struct Header {
-    uint32_t UnitLength;
+    uint64_t UnitLength;
+    dwarf::DwarfFormat Format;
     uint16_t Version;
     uint32_t CompUnitCount;
     uint32_t LocalTypeUnitCount;
     uint32_t ForeignTypeUnitCount;
     uint32_t BucketCount;
     uint32_t NameCount;
     uint32_t AbbrevTableSize;
     uint32_t AugmentationStringSize;
public:
     /// Look up all entries in this Name Index matching \c Key.
     iterator_range<ValueIterator> equal_range(StringRef Key) const;
 
     NameIterator begin() const { return NameIterator(this, 1); }
     NameIterator end() const { return NameIterator(this, getNameCount() + 1); }
 
     Error extract();
     uint64_t getUnitOffset() const { return Base; }
-    uint64_t getNextUnitOffset() const { return Base + 4 + Hdr.UnitLength; }
+    uint64_t getNextUnitOffset() const {
+      return Base + dwarf::getUnitLengthFieldByteSize(Hdr.Format) +
+             Hdr.UnitLength;
+    }
     void dump(ScopedPrinter &W) const;
 
     friend class DWARFDebugNames;
   };
 
   class ValueIterator : public std::iterator<std::input_iterator_tag, Entry> {
 
     /// The Name Index we are currently iterating through. The implementation

llvm/lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp

void DWARFDebugNames::Header::dump(ScopedPrinter &W) const {
   W.printNumber("Bucket count", BucketCount);
   W.printNumber("Name count", NameCount);
   W.printHex("Abbreviations table size", AbbrevTableSize);
   W.startLine() << "Augmentation: '" << AugmentationString << "'\n";
 }
 
 Error DWARFDebugNames::Header::extract(const DWARFDataExtractor &AS,
                                              uint64_t *Offset) {
   // These fields are the same for 32-bit and 64-bit DWARF formats.
   constexpr unsigned CommonHeaderSize = 2 + // Version
                                         2 + // Padding
                                         4 + // CU count
                                         4 + // Local TU count
                                         4 + // Foreign TU count
                                         4 + // Bucket count
                                         4 + // Name count
                                         4 + // Abbreviations table size
                                         4;  // Augmentation string size
   static const unsigned DWARF32HeaderFixedPartSize =
       dwarf::getUnitLengthFieldByteSize(dwarf::DWARF32) + CommonHeaderSize;
+  static const unsigned DWARF64HeaderFixedPartSize =
+      dwarf::getUnitLengthFieldByteSize(dwarf::DWARF64) + CommonHeaderSize;
   // Check that we can read the fixed-size part.
   if (!AS.isValidOffsetForDataOfSize(*Offset, DWARF32HeaderFixedPartSize))
     return createStringError(errc::illegal_byte_sequence,
                              "Section too small: cannot read header.");
 
+  Format = dwarf::DWARF32;
   UnitLength = AS.getU32(Offset);
+  if (UnitLength == dwarf::DW_LENGTH_DWARF64) {
+    // Check again as the header is larger for DWARF64.
+    // Subtract 4 as we've already read one uword.
+    if (!AS.isValidOffsetForDataOfSize(*Offset, DWARF64HeaderFixedPartSize - 4))
+      return createStringError(
+          errc::illegal_byte_sequence,
+          "Section too small for DWARF64: cannot read header.");
+    Format = dwarf::DWARF64;
+    UnitLength = AS.getU64(Offset);
+  } else if (UnitLength >= dwarf::DW_LENGTH_lo_reserved) {
+    return createStringError(errc::illegal_byte_sequence,
+                             "Unsupported reserved unit length value");
+  }
   Version = AS.getU16(Offset);
   // Skip padding
   *Offset += 2;
   CompUnitCount = AS.getU32(Offset);
   LocalTypeUnitCount = AS.getU32(Offset);
   ForeignTypeUnitCount = AS.getU32(Offset);
   BucketCount = AS.getU32(Offset);
   NameCount = AS.getU32(Offset);

 }
 
 Error DWARFDebugNames::NameIndex::extract() {
   const DWARFDataExtractor &AS = Section.AccelSection;
   uint64_t Offset = Base;
   if (Error E = Hdr.extract(AS, &Offset))
     return E;
 
+  const unsigned SectionOffsetSize = dwarf::getDwarfOffsetByteSize(Hdr.Format);
   CUsBase = Offset;
-  Offset += Hdr.CompUnitCount * 4;
-  Offset += Hdr.LocalTypeUnitCount * 4;
+  Offset += Hdr.CompUnitCount * SectionOffsetSize;
+  Offset += Hdr.LocalTypeUnitCount * SectionOffsetSize;
   Offset += Hdr.ForeignTypeUnitCount * 8;
   BucketsBase = Offset;
   Offset += Hdr.BucketCount * 4;
   HashesBase = Offset;
   if (Hdr.BucketCount > 0)
     Offset += Hdr.NameCount * 4;
   StringOffsetsBase = Offset;
-  Offset += Hdr.NameCount * 4;
+  Offset += Hdr.NameCount * SectionOffsetSize;
   EntryOffsetsBase = Offset;
-  Offset += Hdr.NameCount * 4;
+  Offset += Hdr.NameCount * SectionOffsetSize;
 
   if (!AS.isValidOffsetForDataOfSize(Offset, Hdr.AbbrevTableSize))
     return createStringError(errc::illegal_byte_sequence,
                              "Section too small: cannot read abbreviations.");
 
   EntriesBase = Offset + Hdr.AbbrevTableSize;
 
   for (;;) {

 
 char DWARFDebugNames::SentinelError::ID;
 std::error_code DWARFDebugNames::SentinelError::convertToErrorCode() const {
   return inconvertibleErrorCode();
 }
 
 uint64_t DWARFDebugNames::NameIndex::getCUOffset(uint32_t CU) const {
   assert(CU < Hdr.CompUnitCount);
-  uint64_t Offset = CUsBase + 4 * CU;
-  return Section.AccelSection.getRelocatedValue(4, &Offset);
+  const unsigned SectionOffsetSize = dwarf::getDwarfOffsetByteSize(Hdr.Format);
+  uint64_t Offset = CUsBase + SectionOffsetSize * CU;
+  return Section.AccelSection.getRelocatedValue(SectionOffsetSize, &Offset);
 }
 
 uint64_t DWARFDebugNames::NameIndex::getLocalTUOffset(uint32_t TU) const {
   assert(TU < Hdr.LocalTypeUnitCount);
-  uint64_t Offset = CUsBase + 4 * (Hdr.CompUnitCount + TU);
-  return Section.AccelSection.getRelocatedValue(4, &Offset);
+  const unsigned SectionOffsetSize = dwarf::getDwarfOffsetByteSize(Hdr.Format);
+  uint64_t Offset = CUsBase + SectionOffsetSize * (Hdr.CompUnitCount + TU);
+  return Section.AccelSection.getRelocatedValue(SectionOffsetSize, &Offset);
 }
 
 uint64_t DWARFDebugNames::NameIndex::getForeignTUSignature(uint32_t TU) const {
   assert(TU < Hdr.ForeignTypeUnitCount);
+  const unsigned SectionOffsetSize = dwarf::getDwarfOffsetByteSize(Hdr.Format);
   uint64_t Offset =
-      CUsBase + 4 * (Hdr.CompUnitCount + Hdr.LocalTypeUnitCount) + 8 * TU;
+      CUsBase +
+      SectionOffsetSize * (Hdr.CompUnitCount + Hdr.LocalTypeUnitCount) + 8 * TU;
   return Section.AccelSection.getU64(&Offset);
 }
 
 Expected<DWARFDebugNames::Entry>
 DWARFDebugNames::NameIndex::getEntry(uint64_t *Offset) const {
   const DWARFDataExtractor &AS = Section.AccelSection;
   if (!AS.isValidOffset(*Offset))
     return createStringError(errc::illegal_byte_sequence,
                              "Incorrectly terminated entry list.");
 
   uint32_t AbbrevCode = AS.getULEB128(Offset);
   if (AbbrevCode == 0)
     return make_error<SentinelError>();
 
   const auto AbbrevIt = Abbrevs.find_as(AbbrevCode);
   if (AbbrevIt == Abbrevs.end())
     return createStringError(errc::invalid_argument, "Invalid abbreviation.");
 
   Entry E(*this, *AbbrevIt);
 
-  dwarf::FormParams FormParams = {Hdr.Version, 0, dwarf::DwarfFormat::DWARF32};
+  dwarf::FormParams FormParams = {Hdr.Version, 0, Hdr.Format};
   for (auto &Value : E.Values) {
     if (!Value.extractValue(AS, Offset, FormParams))
       return createStringError(errc::io_error,
                                "Error extracting index attribute values.");
   }
   return std::move(E);
 }
 
 DWARFDebugNames::NameTableEntry
 DWARFDebugNames::NameIndex::getNameTableEntry(uint32_t Index) const {
   assert(0 < Index && Index <= Hdr.NameCount);
-  uint64_t StringOffsetOffset = StringOffsetsBase + 4 * (Index - 1);
-  uint64_t EntryOffsetOffset = EntryOffsetsBase + 4 * (Index - 1);
+  const unsigned SectionOffsetSize = dwarf::getDwarfOffsetByteSize(Hdr.Format);
+  uint64_t StringOffsetOffset =
+      StringOffsetsBase + SectionOffsetSize * (Index - 1);
+  uint64_t EntryOffsetOffset =
+      EntryOffsetsBase + SectionOffsetSize * (Index - 1);
   const DWARFDataExtractor &AS = Section.AccelSection;
 
-  uint64_t StringOffset = AS.getRelocatedValue(4, &StringOffsetOffset);
-  uint64_t EntryOffset = AS.getU32(&EntryOffsetOffset);
+  uint64_t StringOffset =
+      AS.getRelocatedValue(SectionOffsetSize, &StringOffsetOffset);
+  uint64_t EntryOffset = AS.getUnsigned(&EntryOffsetOffset, SectionOffsetSize);
   EntryOffset += EntriesBase;
   return {Section.StringSection, Index, StringOffset, EntryOffset};
 }
 
 uint32_t
 DWARFDebugNames::NameIndex::getBucketArrayEntry(uint32_t Bucket) const {
   assert(Bucket < Hdr.BucketCount);
   uint64_t BucketOffset = BucketsBase + 4 * Bucket;

llvm/test/DebugInfo/X86/dwarfdump-debug-names.s

 .Lnames_abbrev_end1:
 .Lnames_entries1:
 .Lnames2:
   .byte 52                      # Abbrev code
   .long .Ldie1-.Lcu_begin1      # DW_IDX_die_offset
   .long 0                       # End of list: bar
   .p2align  2
 .Lnames_end1:
+
+  .long 0xffffffff                  # DWARF64 mark
+  .quad .Lnames_end2-.Lnames_start2 # Length
+.Lnames_start2:
+  .short  5                           # Version
+  .space  2                           # Padding
+  .long 1                           # CU count
+  .long 1                           # Local TU count
+  .long 1                           # Foreign TU count
+  .long 1                           # Bucket count
+  .long 1                           # Name count
+  .long .Lnames_abbrev_end2-.Lnames_abbrev_start2   # Abbreviations table size
+  .long 0                           # Augmentation string size
+  .quad 0xcc00cccccccc              # CU0 offset
+  .quad 0xaa00aaaaaaaa              # Local TU0 offset
+  .quad 0xffffff00ffffffff          # Foreign TU2 signature
+  .long 1                           # Bucket 0
+  .long 0xb887389                   # Hash in Bucket 0
+  .quad .Linfo_string0              # String in Bucket 0: foo
+  .quad .Lnames3-.Lnames_entries2   # Offset in Bucket 0
+.Lnames_abbrev_start2:
+  .byte 0x01                        # Abbrev code
+  .byte 0x24                        # DW_TAG_base_type
+  .byte 0x02                        # DW_IDX_type_unit
+  .byte 0x06                        # DW_FORM_data4
+  .byte 0x05                        # DW_IDX_type_hash
+  .byte 0x07                        # DW_FORM_data8
+  .byte 0x00                        # End of abbrev
+  .byte 0x00                        # End of abbrev
+  .byte 0x00                        # End of abbrev list
+.Lnames_abbrev_end2:
+.Lnames_entries2:
+.Lnames3:
+  .byte 0x01                        # Abbrev code
+  .long 1                           # DW_IDX_type_unit
+  .quad 0xff03ffffffff              # DW_IDX_type_hash
+  .byte   0x00                        # End of list: foo
+  .p2align  2
+.Lnames_end2:
+
 # CHECK: .debug_names contents:
 # CHECK-NEXT: Name Index @ 0x0 {
 # CHECK-NEXT:   Header {
 # CHECK-NEXT:     Length: 0x60
 # CHECK-NEXT:     Version: 5
 # CHECK-NEXT:     CU count: 1
 # CHECK-NEXT:     Local TU count: 0
 # CHECK-NEXT:     Foreign TU count: 0

 # CHECK-NEXT:       Entry @ 0xa3 {
 # CHECK-NEXT:         Abbrev: 0x34
 # CHECK-NEXT:         Tag: DW_TAG_variable
 # CHECK-NEXT:         DW_IDX_die_offset: 0x00000001
 # CHECK-NEXT:       }
 # CHECK-NEXT:     }
 # CHECK-NEXT:   ]
 # CHECK-NEXT: }
+# CHECK-NEXT: Name Index @ 0xac {
+# CHECK-NEXT:   Header {
+# CHECK-NEXT:     Length: 0x68
+# CHECK-NEXT:     Version: 5
+# CHECK-NEXT:     CU count: 1
+# CHECK-NEXT:     Local TU count: 1
+# CHECK-NEXT:     Foreign TU count: 1
+# CHECK-NEXT:     Bucket count: 1
+# CHECK-NEXT:     Name count: 1
+# CHECK-NEXT:     Abbreviations table size: 0x9
+# CHECK-NEXT:     Augmentation: ''
+# CHECK-NEXT:   }
+# CHECK-NEXT:   Compilation Unit offsets [
+# CHECK-NEXT:     CU[0]: 0xcc00cccccccc
+# CHECK-NEXT:   ]
+# CHECK-NEXT:   Local Type Unit offsets [
+# CHECK-NEXT:     LocalTU[0]: 0xaa00aaaaaaaa
+# CHECK-NEXT:   ]
+# CHECK-NEXT:   Foreign Type Unit signatures [
+# CHECK-NEXT:     ForeignTU[0]: 0xffffff00ffffffff
+# CHECK-NEXT:   ]
+# CHECK-NEXT:   Abbreviations [
+# CHECK-NEXT:     Abbreviation 0x1 {
+# CHECK-NEXT:       Tag: DW_TAG_base_type
+# CHECK-NEXT:       DW_IDX_type_unit: DW_FORM_data4
+# CHECK-NEXT:       DW_IDX_type_hash: DW_FORM_data8
+# CHECK-NEXT:     }
+# CHECK-NEXT:   ]
+# CHECK-NEXT:   Bucket 0 [
+# CHECK-NEXT:     Name 1 {
+# CHECK-NEXT:       Hash: 0xB887389
+# CHECK-NEXT:       String: 0x00000000 "foo"
+# CHECK-NEXT:       Entry @ 0x111 {
+# CHECK-NEXT:         Abbrev: 0x1
+# CHECK-NEXT:         Tag: DW_TAG_base_type
+# CHECK-NEXT:         DW_IDX_type_unit: 0x00000001
+# CHECK-NEXT:         DW_IDX_type_hash: 0x0000ff03ffffffff
+# CHECK-NEXT:       }
+# CHECK-NEXT:     }
+# CHECK-NEXT:   ]
+# CHECK-NEXT: }

llvm/unittests/DebugInfo/DWARF/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   ${LLVM_TARGETS_TO_BUILD}
   AsmPrinter
   DebugInfoDWARF
   MC
   Object
   ObjectYAML
   Support
   )
 
 add_llvm_unittest(DebugInfoDWARFTests
   DwarfGenerator.cpp
   DwarfUtils.cpp
+  DWARFAcceleratorTableTest.cpp
   DWARFDebugArangeSetTest.cpp
   DWARFDebugInfoTest.cpp
   DWARFDebugLineTest.cpp
   DWARFDieTest.cpp
   DWARFFormValueTest.cpp
   DWARFLocationExpressionTest.cpp
   )
 
 target_link_libraries(DebugInfoDWARFTests PRIVATE LLVMTestingSupport)

llvm/unittests/DebugInfo/DWARF/DWARFAcceleratorTableTest.cpp

+//===- DWARFAcceleratorTableTest.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/DWARFAcceleratorTable.h"
+#include "llvm/DebugInfo/DWARF/DWARFContext.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+namespace {
+
+void ExpectDebugNamesExtractError(StringRef NamesSecData, StringRef StrSecData,
+                                  const char *ErrorMessage) {
+  DWARFSection NamesDWARFSection;
+  NamesDWARFSection.Data = NamesSecData;
+  StringMap<std::unique_ptr<MemoryBuffer>> Sections;
+  auto Context = DWARFContext::create(Sections, /* AddrSize = */ 4,
+                                      /* isLittleEndian = */ true);
+  DWARFDataExtractor NamesExtractor(Context->getDWARFObj(), NamesDWARFSection,
+                                    /* isLittleEndian = */ true,
+                                    /* AddrSize = */ 4);
+  DataExtractor StrExtractor(StrSecData,
+                             /* isLittleEndian = */ true,
+                             /* AddrSize = */ 4);
+  DWARFDebugNames Table(NamesExtractor, StrExtractor);
+  Error E = Table.extract();
+  ASSERT_TRUE(E.operator bool());
+  EXPECT_STREQ(ErrorMessage, toString(std::move(E)).c_str());
+}
+
+TEST(DWARFDebugNames, ReservedUnitLength) {
+  static const char NamesSecData[64] =
+      "\xf0\xff\xff\xff"; // Reserved unit length value
+  ExpectDebugNamesExtractError(StringRef(NamesSecData, sizeof(NamesSecData)),
+                               StringRef(),
+                               "Unsupported reserved unit length value");
+}
+
+TEST(DWARFDebugNames, TooSmallForDWARF64) {
+  // DWARF64 header takes at least 44 bytes.
+  static const char NamesSecData[43] = "\xff\xff\xff\xff"; // DWARF64 mark
+  ExpectDebugNamesExtractError(
+      StringRef(NamesSecData, sizeof(NamesSecData)), StringRef(),
+      "Section too small for DWARF64: cannot read header.");
+}
+
+} // end anonymous namespace