[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–557

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–557

.....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–557

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

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

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::pair<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::pair<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 {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))
+             << " in accelerator table does not match Tag "
+             << dwarf::TagString(Die.getTag()) << " of DIE[" << HashDataIdx
+             << "].\n";
+          ++NumErrors;
         }
       }
       ++StringCount;
     }
   }
   return NumErrors;
 }
 

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

+# RUN: llvm-mc %s -filetype obj -triple x86_64-apple-darwin -o - \
+# RUN: | not llvm-dwarfdump -verify - \
+# RUN: | FileCheck %s
+
+# CHECK: Verifying .apple_types...
+# CHECK-NEXT: error: Tag DW_TAG_ptr_to_member_type in accelerator table does not match Tag DW_TAG_base_type of DIE[0].
+
+# This test is meant to verify that the -verify option 
+# in llvm-dwarfdump, correctly identifies an invalid DIE tag in .apple_types.
+
+  .section  __TEXT,__text,regular,pure_instructions
+  .macosx_version_min 10, 12
+  .file 1 "basic.c"
+  .comm _i,4,2                  ## @i
+  .section  __DWARF,__debug_str,regular,debug
+Linfo_string:
+  .asciz  "clang version 6.0.0 (trunk 309427) (llvm/trunk 309432)" ## string offset=0
+  .asciz  "basic.c"               ## string offset=55
+  .asciz  "/Users/sgravani/Development/tests" ## string offset=63
+  .asciz  "i"                     ## string offset=97
+  .asciz  "int"                   ## string offset=99
+  .section  __DWARF,__debug_abbrev,regular,debug
+Lsection_abbrev:
+  .byte 1                       ## Abbreviation Code
+  .byte 17                      ## DW_TAG_compile_unit
+  .byte 1                       ## DW_CHILDREN_yes
+  .byte 37                      ## DW_AT_producer
+  .byte 14                      ## DW_FORM_strp
+  .byte 19                      ## DW_AT_language
+  .byte 5                       ## DW_FORM_data2
+  .byte 3                       ## DW_AT_name
+  .byte 14                      ## DW_FORM_strp
+  .byte 16                      ## DW_AT_stmt_list
+  .byte 23                      ## DW_FORM_sec_offset
+  .byte 27                      ## DW_AT_comp_dir
+  .byte 14                      ## DW_FORM_strp
+  .byte 0                       ## EOM(1)
+  .byte 0                       ## EOM(2)
+  .byte 2                       ## Abbreviation Code
+  .byte 52                      ## DW_TAG_variable
+  .byte 0                       ## DW_CHILDREN_no
+  .byte 3                       ## DW_AT_name
+  .byte 14                      ## DW_FORM_strp
+  .byte 73                      ## DW_AT_type
+  .byte 19                      ## DW_FORM_ref4
+  .byte 63                      ## DW_AT_external
+  .byte 25                      ## DW_FORM_flag_present
+  .byte 58                      ## DW_AT_decl_file
+  .byte 11                      ## DW_FORM_data1
+  .byte 59                      ## DW_AT_decl_line
+  .byte 11                      ## DW_FORM_data1
+  .byte 2                       ## DW_AT_location
+  .byte 24                      ## DW_FORM_exprloc
+  .byte 0                       ## EOM(1)
+  .byte 0                       ## EOM(2)
+  .byte 3                       ## Abbreviation Code
+  .byte 36                      ## DW_TAG_base_type
+  .byte 0                       ## DW_CHILDREN_no
+  .byte 3                       ## DW_AT_name
+  .byte 14                      ## DW_FORM_strp
+  .byte 62                      ## DW_AT_encoding
+  .byte 11                      ## DW_FORM_data1
+  .byte 11                      ## DW_AT_byte_size
+  .byte 11                      ## DW_FORM_data1
+  .byte 0                       ## EOM(1)
+  .byte 0                       ## EOM(2)
+  .byte 0                       ## EOM(3)
+  .section  __DWARF,__debug_info,regular,debug
+Lsection_info:
+Lcu_begin0:
+  .long 56                      ## Length of Unit
+  .short  5                       ## DWARF version number
+  .byte 1                       ## DWARF Unit Type
+  .byte 8                       ## Address Size (in bytes)
+Lset0 = Lsection_abbrev-Lsection_abbrev ## Offset Into Abbrev. Section
+  .long Lset0
+  .byte 1                       ## Abbrev [1] 0xc:0x30 DW_TAG_compile_unit
+  .long 0                       ## DW_AT_producer
+  .short  12                      ## DW_AT_language
+  .long 55                      ## DW_AT_name
+Lset1 = Lline_table_start0-Lsection_line ## DW_AT_stmt_list
+  .long Lset1
+  .long 63                      ## DW_AT_comp_dir
+  .byte 2                       ## Abbrev [2] 0x1f:0x15 DW_TAG_variable
+  .long 97                      ## DW_AT_name
+  .long 52                      ## DW_AT_type
+                                        ## DW_AT_external
+  .byte 1                       ## DW_AT_decl_file
+  .byte 1                       ## DW_AT_decl_line
+  .byte 9                       ## DW_AT_location
+  .byte 3
+  .quad _i
+  .byte 3                       ## Abbrev [3] 0x34:0x7 DW_TAG_base_type
+  .long 99                      ## DW_AT_name
+  .byte 5                       ## DW_AT_encoding
+  .byte 4                       ## DW_AT_byte_size
+  .byte 0                       ## End Of Children Mark
+  .section  __DWARF,__apple_types,regular,debug
+Ltypes_begin:
+  .long 1212240712              ## Header Magic
+  .short  1                       ## Header Version
+  .short  0                       ## Header Hash Function
+  .long 1                       ## Header Bucket Count
+  .long 1                       ## Header Hash Count
+  .long 20                      ## Header Data Length
+  .long 0                       ## HeaderData Die Offset Base
+  .long 3                       ## HeaderData Atom Count
+  .short  1                       ## DW_ATOM_die_offset
+  .short  6                       ## DW_FORM_data4
+  .short  3                       ## DW_ATOM_die_tag
+  .short  5                       ## DW_FORM_data2
+  .short  4                       ## DW_ATOM_type_flags
+  .short  11                      ## DW_FORM_data1
+  .long 0                       ## Bucket 0
+  .long 193495088               ## Hash in Bucket 0
+  .long Ltypes0-Ltypes_begin    ## Offset in Bucket 0
+Ltypes0:
+  .long 99                      ## int
+  .long 1                       ## Num DIEs
+  .long 52
+  .short  31            ## error: Tag DW_TAG_ptr_to_member_type in accelerator table does not match Tag DW_TAG_base_type of DIE[0].
+  .byte 0
+  .long 0
+
+.subsections_via_symbols
+  .section  __DWARF,__debug_line,regular,debug
+Lsection_line:
+Lline_table_start0: