[DWARF] Generalized verification of .apple_names accelerator table to be applicable to any acceleration table. Added verification for .apple_types, .apple_namespaces and .apple_objc sections.

[sgravani]

Summary

This patch replaces .handleAppleNames() with .handleAccelTables().
This way, any accelerator tables will be verified (if it exists).

[aprantl]

include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h

52 ↗

(On Diff #108148)

I'm not sure this needs to be part of the interface — and if it is this should be a static constant or a constexpr instead of a data member.

include/llvm/DebugInfo/DWARF/DWARFVerifier.h

175 ↗

(On Diff #108148)

... existing *Apple-style* accelerator tables ...
DWARF v5 accelerator tables are not yet supported.

lib/DebugInfo/DWARF/DWARFContext.cpp

430 ↗

(On Diff #108148)

Could you just update all of the other cases in a separate commit?

lib/DebugInfo/DWARF/DWARFVerifier.cpp

478 ↗

(On Diff #108148)

"Section" << Sectionname << " is too small to fit a section header\n"

482 ↗

(On Diff #108148)

why initialize to 8?

484 ↗

(On Diff #108148)

"Section" << Sectionname << " is smaller than size described in section header\n"

569 ↗

(On Diff #108148)

if (!D.getAppleNamesSection().Data.empty())

572 ↗

(On Diff #108148)

...

583 ↗

(On Diff #108148)

add one :-)

[sgravani]

Addressed reviewer's comments.

include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h

52 ↗

(On Diff #108148)

Removed it. I can use getSizeHdr() instead.

lib/DebugInfo/DWARF/DWARFContext.cpp

430 ↗

(On Diff #108148)

Yes, I'll do that.

lib/DebugInfo/DWARF/DWARFVerifier.cpp

482 ↗

(On Diff #108148)

I thought I need to verify whether num_of_buckets > 0 (and that field would be at offset 8 from the beginning of the section).
However, after the first check (for the size of the fixed part of the header), if we just check the return value of extract(), it suffices
to perform the second check (for the size of the section).

I modified the code to perform only extract() for the second check.

[aprantl]

Two minor nitpicks, but otherwise LGTM, thanks!

include/llvm/DebugInfo/DWARF/DWARFVerifier.h

175 ↗

(On Diff #108148)

Would you mind removing the *s again? I meant to use them as <insert new text here>-marker.

Sorry :-)

lib/DebugInfo/DWARF/DWARFVerifier.cpp

483 ↗

(On Diff #108179)

remove commented out line

[aprantl]

lib/DebugInfo/DWARF/DWARFVerifier.cpp

567 ↗

(On Diff #108179)

One last thing: You should not need to hard-code the name here. Can you extract it from the Section datastructure instead?

[sgravani]

lib/DebugInfo/DWARF/DWARFVerifier.cpp

567 ↗

(On Diff #108179)

I'm not sure how to do this. Even dumpAccelSection() in DWARFContext.cpp is being called with the name of the section as an argument.

llvm/trunk/include/llvm/DebugInfo/DWARF/DWARFVerifier.h

 class raw_ostream;
 struct DWARFAttribute;
 class DWARFContext;
 class DWARFDie;
 class DWARFUnit;
 class DWARFAcceleratorTable;
 class DWARFDataExtractor;
 class DWARFDebugAbbrev;
+class DataExtractor;
+struct DWARFSection;
 
 /// A class that verifies DWARF debug information given a DWARF Context.
 class DWARFVerifier {
   raw_ostream &OS;
   DWARFContext &DCtx;
   /// A map that tracks all references (converted absolute references) so we
   /// can verify each reference points to a valid DIE and not an offset that
   /// lies between to valid DIEs.
   std::map<uint64_t, std::set<uint32_t>> ReferenceToDIEOffsets;
   uint32_t NumDebugLineErrors = 0;
-  uint32_t NumAppleNamesErrors = 0;
 
   /// Verifies the abbreviations section.
   ///
   /// This function currently checks that:
   /// --No abbreviation declaration has more than one attributes with the same
   /// name.
   ///
   /// \param Abbrev Pointer to the abbreviations section we are verifying
class DWARFVerifier {
 
   /// Verify that all of the rows in the line table are valid.
   ///
   /// This function currently checks for:
   /// - addresses within a sequence that decrease in value
   /// - invalid file indexes
   void verifyDebugLineRows();
 
+  /// Verify that an Apple-style accelerator table is valid.
+  ///
+  /// This function currently checks that:
+  /// - The fixed part of the header fits in the section
+  /// - The size of the section is as large as what the header describes
+  /// - There is at least one atom
+  /// - The form for each atom is valid
+  /// - The buckets have a valid index, or they are empty
+  /// - Each hashdata offset is valid
+  /// - Each DIE is valid
+  /// 
+  /// \param AccelSection pointer to the section containing the acceleration table
+  /// \param StrData pointer to the string section
+  /// \param SectionName the name of the table we're verifying
+  ///
+  /// \returns The number of errors occured during verification
+  unsigned verifyAccelTable(const DWARFSection *AccelSection,
+                            DataExtractor *StrData, const char *SectionName);
+
 public:
   DWARFVerifier(raw_ostream &S, DWARFContext &D)
       : OS(S), DCtx(D) {}
   /// Verify the information in any of the following sections, if available:
   /// .debug_abbrev, debug_abbrev.dwo
   ///
   /// Any errors are reported to the stream that was this object was
   /// constructed with.
public:
   /// Verify the information in the .debug_line section.
   ///
   /// Any errors are reported to the stream that was this object was
   /// constructed with.
   ///
   /// \returns true if the .debug_line verifies successfully, false otherwise.
   bool handleDebugLine();
 
-  /// Verify the information in the .apple_names accelerator table.
+  /// Verify the information in accelerator tables, if they exist.
   ///
   /// Any errors are reported to the stream that was this object was
   /// constructed with.
   ///
-  /// \returns true if the .apple_names verifies successfully, false otherwise.
-  bool handleAppleNames();
+  /// \returns true if the existing Apple-style accelerator tables verify
+  /// successfully, false otherwise.
+  bool handleAccelTables();
 };
 
 } // end namespace llvm
 
 #endif // LLVM_DEBUGINFO_DWARF_DWARFCONTEXT_H

llvm/trunk/lib/DebugInfo/DWARF/DWARFContext.cpp

bool DWARFContext::verify(raw_ostream &OS, DIDumpType DumpType) {
   if (DumpType == DIDT_All || DumpType == DIDT_Info) {
     if (!verifier.handleDebugInfo())
       Success = false;
   }
   if (DumpType == DIDT_All || DumpType == DIDT_Line) {
     if (!verifier.handleDebugLine())
       Success = false;
   }
-  if (DumpType == DIDT_All || DumpType == DIDT_AppleNames) {
-    if (!verifier.handleAppleNames())
-      Success = false;
-  }
+  Success &= verifier.handleAccelTables();
   return Success;
 }
 
 const DWARFUnitIndex &DWARFContext::getCUIndex() {
   if (CUIndex)
     return *CUIndex;
 
   DataExtractor CUIndexData(DObj->getCUIndexSection(), isLittleEndian(), 0);

llvm/trunk/lib/DebugInfo/DWARF/DWARFVerifier.cpp

 bool DWARFVerifier::handleDebugLine() {
   NumDebugLineErrors = 0;
   OS << "Verifying .debug_line...\n";
   verifyDebugLineStmtOffsets();
   verifyDebugLineRows();
   return NumDebugLineErrors == 0;
 }
 
-bool DWARFVerifier::handleAppleNames() {
-  NumAppleNamesErrors = 0;
-  const DWARFObject &D = DCtx.getDWARFObj();
-  DWARFDataExtractor AppleNamesSection(D, D.getAppleNamesSection(),
-                                       DCtx.isLittleEndian(), 0);
-  DataExtractor StrData(D.getStringSection(), DCtx.isLittleEndian(), 0);
-  DWARFAcceleratorTable AppleNames(AppleNamesSection, StrData);
-
-  if (!AppleNames.extract()) {
-    return true;
-  }
+unsigned DWARFVerifier::verifyAccelTable(const DWARFSection *AccelSection,
+                                         DataExtractor *StrData,
+                                         const char *SectionName) {
+  unsigned NumErrors = 0;
+  DWARFDataExtractor AccelSectionData(DCtx.getDWARFObj(), *AccelSection,
+  DWARFAcceleratorTable AccelTable(AccelSectionData, *StrData);
 
-  OS << "Verifying .apple_names...\n";
+  OS << "Verifying " << SectionName << "...\n";
+  // Verify that the fixed part of the header is not too short.
 
+  if (!AccelSectionData.isValidOffset(AccelTable.getSizeHdr())) {
+    OS << "\terror: Section is too small to fit a section header.\n";
+    return 1;
+  }
+  // Verify that the section is not too short.
+  if (!AccelTable.extract()) {
+    OS << "\terror: Section is smaller than size described in section header.\n";
+    return 1;
+  }
   // Verify that all buckets have a valid hash index or are empty.
-  uint32_t NumBuckets = AppleNames.getNumBuckets();
-  uint32_t NumHashes = AppleNames.getNumHashes();
+  uint32_t NumBuckets = AccelTable.getNumBuckets();
+  uint32_t NumHashes = AccelTable.getNumHashes();
 
   uint32_t BucketsOffset =
-      AppleNames.getSizeHdr() + AppleNames.getHeaderDataLength();
+      AccelTable.getSizeHdr() + AccelTable.getHeaderDataLength();
   uint32_t HashesBase = BucketsOffset + NumBuckets * 4;
   uint32_t OffsetsBase = HashesBase + NumHashes * 4;
-
   for (uint32_t BucketIdx = 0; BucketIdx < NumBuckets; ++BucketIdx) {
-    uint32_t HashIdx = AppleNamesSection.getU32(&BucketsOffset);
+    uint32_t HashIdx = AccelSectionData.getU32(&BucketsOffset);
     if (HashIdx >= NumHashes && HashIdx != UINT32_MAX) {
-      OS << format("error: Bucket[%d] has invalid hash index: %u\n", BucketIdx,
+      OS << format("\terror: Bucket[%d] has invalid hash index: %u.\n", BucketIdx,
                    HashIdx);
-      ++NumAppleNamesErrors;
+      ++NumErrors;
     }
   }
-
-  uint32_t NumAtoms = AppleNames.getAtomsDesc().size();
+  uint32_t NumAtoms = AccelTable.getAtomsDesc().size();
   if (NumAtoms == 0) {
-    OS << "error: no atoms; failed to read HashData\n";
-    ++NumAppleNamesErrors;
-    return false;
+    OS << "\terror: no atoms; failed to read HashData.\n";
+    return 1;
   }
-
-  if (!AppleNames.validateForms()) {
-    OS << "error: unsupported form; failed to read HashData\n";
-    ++NumAppleNamesErrors;
-    return false;
+  if (!AccelTable.validateForms()) {
+    OS << "\terror: unsupported form; failed to read HashData.\n";
+    return 1;
   }
 
   for (uint32_t HashIdx = 0; HashIdx < NumHashes; ++HashIdx) {
     uint32_t HashOffset = HashesBase + 4 * HashIdx;
     uint32_t DataOffset = OffsetsBase + 4 * HashIdx;
-    uint32_t Hash = AppleNamesSection.getU32(&HashOffset);
-    uint32_t HashDataOffset = AppleNamesSection.getU32(&DataOffset);
-    if (!AppleNamesSection.isValidOffsetForDataOfSize(HashDataOffset,
-                                                      sizeof(uint64_t))) {
-      OS << format("error: Hash[%d] has invalid HashData offset: 0x%08x\n",
+    uint32_t Hash = AccelSectionData.getU32(&HashOffset);
+    uint32_t HashDataOffset = AccelSectionData.getU32(&DataOffset);
+    if (!AccelSectionData.isValidOffsetForDataOfSize(HashDataOffset,
+      OS << format("\terror: Hash[%d] has invalid HashData offset: 0x%08x.\n",
                    HashIdx, HashDataOffset);
-      ++NumAppleNamesErrors;
+      ++NumErrors;
     }
 
     uint32_t StrpOffset;
     uint32_t StringOffset;
     uint32_t StringCount = 0;
     uint32_t DieOffset = dwarf::DW_INVALID_OFFSET;
 
-    while ((StrpOffset = AppleNamesSection.getU32(&HashDataOffset)) != 0) {
+    while ((StrpOffset = AccelSectionData.getU32(&HashDataOffset)) != 0) {
       const uint32_t NumHashDataObjects =
-          AppleNamesSection.getU32(&HashDataOffset);
+          AccelSectionData.getU32(&HashDataOffset);
       for (uint32_t HashDataIdx = 0; HashDataIdx < NumHashDataObjects;
            ++HashDataIdx) {
-        DieOffset = AppleNames.readAtoms(HashDataOffset);
+        DieOffset = AccelTable.readAtoms(HashDataOffset);
         if (!DCtx.getDIEForOffset(DieOffset)) {
           const uint32_t BucketIdx =
               NumBuckets ? (Hash % NumBuckets) : UINT32_MAX;
           StringOffset = StrpOffset;
-          const char *Name = StrData.getCStr(&StringOffset);
+          const char *Name = StrData->getCStr(&StringOffset);
           if (!Name)
             Name = "<NULL>";
 
           OS << format(
-              "error: .apple_names Bucket[%d] Hash[%d] = 0x%08x "
+              "\terror: %s Bucket[%d] Hash[%d] = 0x%08x "
               "Str[%u] = 0x%08x "
               "DIE[%d] = 0x%08x is not a valid DIE offset for \"%s\".\n",
-              BucketIdx, HashIdx, Hash, StringCount, StrpOffset, HashDataIdx,
-              DieOffset, Name);
+              SectionName, BucketIdx, HashIdx, Hash, StringCount, StrpOffset,
+              HashDataIdx, DieOffset, Name);
 
-          ++NumAppleNamesErrors;
+          ++NumErrors;
         }
       }
       ++StringCount;
     }
   }
-  return NumAppleNamesErrors == 0;
+  return NumErrors;
+}
+
+bool DWARFVerifier::handleAccelTables() {
+  const DWARFObject &D = DCtx.getDWARFObj();
+  DataExtractor StrData(D.getStringSection(), DCtx.isLittleEndian(), 0);
+  unsigned NumErrors = 0;
+  if (!D.getAppleNamesSection().Data.empty())
+    NumErrors +=
+        verifyAccelTable(&D.getAppleNamesSection(), &StrData, ".apple_names");
+  if (!D.getAppleTypesSection().Data.empty())
+    NumErrors +=
+        verifyAccelTable(&D.getAppleTypesSection(), &StrData, ".apple_types");
+  if (!D.getAppleNamespacesSection().Data.empty())
+    NumErrors += verifyAccelTable(&D.getAppleNamespacesSection(), &StrData,
+                                  ".apple_namespaces");
+  if (!D.getAppleObjCSection().Data.empty())
+    NumErrors +=
+        verifyAccelTable(&D.getAppleObjCSection(), &StrData, ".apple_objc");
+  return NumErrors == 0;
 }

llvm/trunk/test/DebugInfo/dwarfdump-accel.test

 RUN: llvm-dwarfdump %p/Inputs/dwarfdump-objc.x86_64.o | FileCheck %s
-RUN: llvm-dwarfdump -verify %p/Inputs/dwarfdump-objc.x86_64.o | FileCheck %s --check-prefix=VERIFY
+RUN: not llvm-dwarfdump -verify %p/Inputs/dwarfdump-objc.x86_64.o | FileCheck %s --check-prefix=VERIFY
 
 Gather some DIE indexes to verify the accelerator table contents.
 CHECK: .debug_info contents
 CHECK: [[TESTINTERFACE:0x[0-9a-f]*]]:{{.*}}DW_TAG_structure_type
 CHECK-NOT: DW_TAG
 CHECK:     DW_AT_name{{.*}}"TestInterface"
 CHECK: [[READONLY:0x[0-9a-f]*]]:{{.*}}DW_TAG_subprogram
 CHECK-NOT: DW_TAG

 CHECK: .apple_objc contents:
 CHECK:     Name{{.*}}"TestInterface"
 CHECK-NOT: Name
 CHECK:     {Atom[0]: [[READONLY]]}
 CHECK:     {Atom[0]: [[ASSIGN]]}
 CHECK:     {Atom[0]: [[SETASSIGN]]}
 
 Verify the debug info in the apple_names accelerator table.
-VERIFY: Verifying .apple_names
-VERIFY-NEXT: No errors.
+VERIFY: Verifying .apple_names...
+VERIFY-NEXT: Verifying .apple_types...
+VERIFY-NEXT: Verifying .apple_namespaces...
+VERIFY-NEXT: error: Section is smaller than size described in section header.
+VERIFY-NEXT: Verifying .apple_objc...
+VERIFY-NEXT: Errors detected.

llvm/trunk/test/tools/llvm-dwarfdump/X86/apple_names_verify_data.s

 # RUN: llvm-mc %s -filetype obj -triple x86_64-apple-darwin -o - \
 # RUN: | not llvm-dwarfdump -verify - \
 # RUN: | FileCheck %s
 
 # CHECK: Verifying .apple_names...
-# CHECK-NEXT: error: Bucket[0] has invalid hash index: 4294967294
-# CHECK-NEXT: error: Hash[0] has invalid HashData offset: 0x000000b4
-# CHECK-NEXT: error: .apple_names Bucket[1] Hash[1] = 0x0002b60f Str[0] = 0x0000005a DIE[0] = 0x00000001 is not a valid DIE offset for "j".
+# CHECK-NEXT: error: Bucket[0] has invalid hash index: 4294967294.
+# CHECK-NEXT: error: Hash[0] has invalid HashData offset: 0x000000b4.
+# CHECK-NEXT: error: .apple_names Bucket[1] Hash[1] = 0x0002b60f Str[0] = 0x0000005a DIE[0] = 0x00000001 is not a valid DIE offset for "j".
 
 # This test is meant to verify that the -verify option 
 # in llvm-dwarfdump, correctly identifies
 # an invalid hash index for bucket[0] in the .apple_names section, 
 # an invalid HashData offset for Hash[0], as well as
 # an invalid DIE offset in the .debug_info section.
 # We're reading an invalid DIE due to the incorrect interpretation of DW_FORM for the DIE.
 # Instead of DW_FORM_data4 the Atom[0].form is: DW_FORM_flag_present.
Lnames_begin:
   .short  0                       ## Header Hash Function
   .long 2                       ## Header Bucket Count
   .long 2                       ## Header Hash Count
   .long 12                      ## Header Data Length
   .long 0                       ## HeaderData Die Offset Base
   .long 1                       ## HeaderData Atom Count
   .short  1                       ## DW_ATOM_die_offset
   .short  25                       ## DW_FORM_data4 -- error: .apple_names Bucket[1] Hash[1] = 0x0002b60f Str[0] = 0x0000005a DIE[0] = 0x00000001 is not a valid DIE offset for "j".
-  .long -2                      ## Bucket 0 -- error: Bucket[0] has invalid hash index: 4294967294
+  .long -2                      ## Bucket 0 -- error: Bucket[0] has invalid hash index: 4294967294.
   .long 1                       ## Bucket 1
   .long 177678                  ## Hash in Bucket 0
   .long 177679                  ## Hash in Bucket 1
-  .long Lsection_line    ## Offset in Bucket 0 -- error: Hash[0] has invalid HashData offset: 0x000000b4
+  .long Lsection_line    ## Offset in Bucket 0 -- error: Hash[0] has invalid HashData offset: 0x000000b4.
   .long LNames1-Lnames_begin    ## Offset in Bucket 1
 LNames0:
   .long 84                      ## i
   .long 1                       ## Num DIEs
   .long 30
   .long 0
 LNames1:
   .long 90                      ## j
   .long 1                       ## Num DIEs
   .long 58
   .long 0
 
 .subsections_via_symbols
   .section  __DWARF,__debug_line,regular,debug
 Lsection_line:
 Lline_table_start0:

llvm/trunk/test/tools/llvm-dwarfdump/X86/apple_names_verify_form.s

 # RUN: llvm-mc %s -filetype obj -triple x86_64-apple-darwin -o - \
 # RUN: | not llvm-dwarfdump -verify - \
 # RUN: | FileCheck %s
 
 # CHECK: Verifying .apple_names...
-# CHECK-NEXT: error: unsupported form; failed to read HashData
+# CHECK-NEXT: error: unsupported form; failed to read HashData.
 
 # This test is meant to verify that the -verify option 
 # in llvm-dwarfdump, correctly identifies that Atom[0].form is unsupported.
 # As a result, the hashdata cannot be read.
 
   .section  __TEXT,__text,regular,pure_instructions
   .file 1 "basic.c"
   .comm _i,4,2                  ## @i
Lnames_begin:
   .short  1                       ## Header Version
   .short  0                       ## Header Hash Function
   .long 2                       ## Header Bucket Count
   .long 2                       ## Header Hash Count
   .long 12                      ## Header Data Length
   .long 0                       ## HeaderData Die Offset Base
   .long 1                       ## HeaderData Atom Count
   .short  1                       ## DW_ATOM_die_offset
-  .short  400                     ## DW_FORM_data4 -- error: unsupported form; failed to read HashData
+  .short  400                     ## DW_FORM_data4 -- error: unsupported form; failed to read HashData.
   .long 0                       ## Bucket 0
   .long 1                       ## Bucket 1
   .long 177678                  ## Hash in Bucket 0
   .long 177679                  ## Hash in Bucket 1
   .long LNames0-Lnames_begin    ## Offset in Bucket 0
   .long LNames1-Lnames_begin    ## Offset in Bucket 1
 LNames0:
   .long 84                      ## i

llvm/trunk/test/tools/llvm-dwarfdump/X86/apple_names_verify_num_atoms.s

 # RUN: llvm-mc %s -filetype obj -triple x86_64-apple-darwin -o - \
 # RUN: | not llvm-dwarfdump -verify - \
 # RUN: | FileCheck %s
 
 # CHECK: Verifying .apple_names...
-# CHECK-NEXT: error: no atoms; failed to read HashData
+# CHECK-NEXT: error: no atoms; failed to read HashData.
 
 # This test is meant to verify that the -verify option 
 # in llvm-dwarfdump, correctly identifies that there is not Atom.
 # As a result, the hashdata cannot be read.
 
   .section  __TEXT,__text,regular,pure_instructions
   .file 1 "basic.c"
   .comm _i,4,2                  ## @i

 Lnames_begin:
   .long 1212240712              ## Header Magic
   .short  1                       ## Header Version
   .short  0                       ## Header Hash Function
   .long 2                       ## Header Bucket Count
   .long 2                       ## Header Hash Count
   .long 12                      ## Header Data Length
   .long 0                       ## HeaderData Die Offset Base
-  .long 0                       ## HeaderData Atom Count -- error: no atoms; failed to read HashData
+  .long 0                       ## HeaderData Atom Count -- error: no atoms; failed to read HashData.
   .short  1                       ## DW_ATOM_die_offset
   .short  6                       ## DW_FORM_data4
   .long 0                       ## Bucket 0
   .long 1                       ## Bucket 1
   .long 177678                  ## Hash in Bucket 0
   .long 177679                  ## Hash in Bucket 1
   .long LNames0-Lnames_begin    ## Offset in Bucket 0
   .long LNames1-Lnames_begin    ## Offset in Bucket 1