[DWARF] Added verification check for tags in accelerator tables. This patch verifies that the atom tag is actually the same with the tag of the DIE that we retrieve from the table.

[aprantl]

This needs a testcase.

include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h

68

I would use a std::pair<> or a simple struct here.

lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp

102

A dwarf::Tag is a uint_16 — do we also need to check that the value is within range, or is this impossible because we already verified the FORM?

108

return {DieOffset, DieTag};

should also work here

lib/DebugInfo/DWARF/DWARFVerifier.cpp

550

maybe continue; here and get rid of the else? not sure if this will be better :-)

554

Could we be more specific in the error message?
perhaps: "Tag in accelerator table does not match Tag of DIE"

[sgravani]

Addressed reviewer's comments.

lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp

102

I think we will catch such an error when we validate the forms

lib/DebugInfo/DWARF/DWARFVerifier.cpp

550

.....why do that? I'm not sure if this is better either :P

[aprantl]

lib/DebugInfo/DWARF/DWARFVerifier.cpp

534

I think something like this would be more readable:

unsigned Offset;
unsigned Tag;
std::tie(Offset, Tag) = AccelTable.readAtoms(HashDataOffset);

550

The LLVM coding style prefers early exits over nested ifs to improve readability. I'll leave this up to you.

test/tools/llvm-dwarfdump/X86/apple_types_verify_tag.s

121 ↗

(On Diff #108950)

Where's the CHECK?

[aprantl]

This LGTM once all pending comments are addressed.

[sgravani]

lib/DebugInfo/DWARF/DWARFVerifier.cpp

534

I followed the advice, thanks!

test/tools/llvm-dwarfdump/X86/apple_types_verify_tag.s

121 ↗

(On Diff #108950)

beginning of test:

1. CHECK: Verifying .apple_types...
2. CHECK-NEXT: error: Tag DW_TAG_ptr_to_member_type in accelerator table does not match Tag DW_TAG_base_type of DIE[0].

include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h

public:
   uint32_t getHeaderDataLength();
   ArrayRef<std::pair<HeaderData::AtomType, HeaderData::Form>> getAtomsDesc();
   bool validateForms();
 
   /// Return information related to the DWARF DIE we're looking for when
   /// performing a lookup by name.
   ///
   /// \param HashDataOffset an offset into the hash data table
-  /// \returns DIEOffset the offset into the .debug_info section for the DIE
-  /// related to the input hash data offset. Currently this function returns
-  /// only the DIEOffset but it can be modified to return more data regarding
-  /// the DIE
-  uint32_t readAtoms(uint32_t &HashDataOffset);
+  /// \returns <DieOffset, DieTag>
+  /// DieOffset is the offset into the .debug_info section for the DIE
+  /// related to the input hash data offset.
+  /// DieTag is the tag of the DIE
+  std::tuple<uint32_t, dwarf::Tag> readAtoms(uint32_t &HashDataOffset);
   void dump(raw_ostream &OS) const;
 };
 
 } // end namespace llvm
 
 #endif // LLVM_DEBUGINFO_DWARFACCELERATORTABLE_H

include/llvm/DebugInfo/DWARF/DWARFVerifier.h

class DWARFVerifier {
 
   /// 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 tag for each DIE in the table 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);
 

lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp

DWARFAcceleratorTable::getAtomsDesc() {
   return HdrData.Atoms;
 }
 
 bool DWARFAcceleratorTable::validateForms() {
   for (auto Atom : getAtomsDesc()) {
     DWARFFormValue FormValue(Atom.second);
     switch (Atom.first) {
     case dwarf::DW_ATOM_die_offset:
+    case dwarf::DW_ATOM_die_tag:
+    case dwarf::DW_ATOM_type_flags:
       if ((!FormValue.isFormClass(DWARFFormValue::FC_Constant) &&
            !FormValue.isFormClass(DWARFFormValue::FC_Flag)) ||
           FormValue.getForm() == dwarf::DW_FORM_sdata)
         return false;
     default:
       break;
     }
   }
   return true;
 }
 
-uint32_t DWARFAcceleratorTable::readAtoms(uint32_t &HashDataOffset) {
+std::tuple<uint32_t, dwarf::Tag>
+DWARFAcceleratorTable::readAtoms(uint32_t &HashDataOffset) {
   uint32_t DieOffset = dwarf::DW_INVALID_OFFSET;
+  dwarf::Tag DieTag = dwarf::DW_TAG_null;
 
   for (auto Atom : getAtomsDesc()) {
     DWARFFormValue FormValue(Atom.second);
     FormValue.extractValue(AccelSection, &HashDataOffset, NULL);
     switch (Atom.first) {
     case dwarf::DW_ATOM_die_offset:
       DieOffset = *FormValue.getAsUnsignedConstant();
       break;
+    case dwarf::DW_ATOM_die_tag:
+      DieTag = (dwarf::Tag)*FormValue.getAsUnsignedConstant();
+      break;
     default:
       break;
     }
   }
-  return DieOffset;
+  return std::make_tuple(DieOffset, DieTag);
 }
 
 LLVM_DUMP_METHOD void DWARFAcceleratorTable::dump(raw_ostream &OS) const {
   // Dump the header.
   OS << "Magic = " << format("0x%08x", Hdr.Magic) << '\n'
      << "Version = " << format("0x%04x", Hdr.Version) << '\n'
      << "Hash function = " << format("0x%08x", Hdr.HashFunction) << '\n'
      << "Bucket count = " << Hdr.NumBuckets << '\n'

lib/DebugInfo/DWARF/DWARFVerifier.cpp

if (!AccelSectionData.isValidOffsetForDataOfSize(HashDataOffset,
       OS << format("\terror: Hash[%d] has invalid HashData offset: 0x%08x.\n",
                    HashIdx, HashDataOffset);
       ++NumErrors;
     }
 
     uint32_t StrpOffset;
     uint32_t StringOffset;
     uint32_t StringCount = 0;
-    uint32_t DieOffset = dwarf::DW_INVALID_OFFSET;
-
     while ((StrpOffset = AccelSectionData.getU32(&HashDataOffset)) != 0) {
       const uint32_t NumHashDataObjects =
           AccelSectionData.getU32(&HashDataOffset);
       for (uint32_t HashDataIdx = 0; HashDataIdx < NumHashDataObjects;
            ++HashDataIdx) {
-        DieOffset = AccelTable.readAtoms(HashDataOffset);
-        if (!DCtx.getDIEForOffset(DieOffset)) {
+        auto Atoms = AccelTable.readAtoms(HashDataOffset);
+        auto Die = DCtx.getDIEForOffset(std::get<0>(Atoms));
+        if (!Die) {
           const uint32_t BucketIdx =
               NumBuckets ? (Hash % NumBuckets) : UINT32_MAX;
           StringOffset = StrpOffset;
           const char *Name = StrData->getCStr(&StringOffset);
           if (!Name)
             Name = "<NULL>";
 
           OS << format(
               "\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",
               SectionName, BucketIdx, HashIdx, Hash, StringCount, StrpOffset,
-              HashDataIdx, DieOffset, Name);
+              HashDataIdx, std::get<0>(Atoms), Name);
 
           ++NumErrors;
+        } else if ((std::get<1>(Atoms) != dwarf::DW_TAG_null) &&
+                   (Die.getTag() != std::get<1>(Atoms))) {
+          OS << "\terror: Tag " << dwarf::TagString(std::get<1>(Atoms))
+             << " is not a valid tag for DIE[" << HashDataIdx << "].\n";
         }
       }
       ++StringCount;
     }
   }
   return NumErrors;
 }