[DWARF] Added check that verifies that no abbreviation declaration has more than one attribute with the same name.

[sgravani]

Summary

This patch adds a verification check on the abbreviation declarations in the .debug_abbrev section.
The check makes sure that no abbreviation declaration has more than one attributes with the same name.

[friss]

lib/DebugInfo/DWARF/DWARFVerifier.cpp

97–105 ↗

(On Diff #107370)

While this is really simple, I feel like it's also really overkill to build a set for every DIE. Every DIE using the same abbreviation number will expose the same issue. Can we somehow prevent the duplicated work?

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

84 ↗

(On Diff #107370)

By adding this just to the abrrev table, I think you completely break the DIE tree decoding. Wouldn't it be better to add it to the debug_info section as well?

[aprantl]

include/llvm/DebugInfo/DWARF/DWARFVerifier.h

60 ↗

(On Diff #107370)

separate commit and perhaps use the opportunity to document verifyUnitContents? (no need for separate review)

lib/DebugInfo/DWARF/DWARFVerifier.cpp

97–105 ↗

(On Diff #107370)

Could we iterate over the abbreviations instead of the DIEs?
It also may be faster to use a SmallDenseSet over a std::set since the number of attributes will be typically very small.

[sgravani]

Moved verification check from DIEs to Abbreviation Declarations.

lib/DebugInfo/DWARF/DWARFVerifier.cpp

97–105 ↗

(On Diff #107370)

Done; verifying .debug_abbrev instead.

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

84 ↗

(On Diff #107370)

I'm not sure what you mean.. Can you explain?

[aprantl]

lib/DebugInfo/DWARF/DWARFContext.cpp

417 ↗

(On Diff #107420)

Shorter alterntive:
Success &= verifier.handleDebugAbbrev();

lib/DebugInfo/DWARF/DWARFVerifier.cpp

119 ↗

(On Diff #107420)

does a range-based for work here?
for (auto AbbrDecl : AbbrDecls) ...

[aprantl]

I think this looks fine now, but I'll let Fred do the final sign off.

[probinson]

Wouldn't you also want to verify .debug_abbrev.dwo?

[sgravani]

Minor code style changes to previous diff.

[sgravani]

In D35643#815473, @probinson wrote:

Wouldn't you also want to verify .debug_abbrev.dwo?

Ideally, yes (as well as other sections from the split dwarf5 proposal).
So far I've focused on verifications for/related to the .debug_info section.
Is it the case that we may need abbreviation declarations for .debug_info section from .debug_abbrev.dwo?

[friss]

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

84 ↗

(On Diff #107370)

Sorry, I thought this was an insertion, but now I see that you replaced an attribute with one with a equivalent form. This LGTM, thanks!

[dblaikie]

In D35643#815498, @sgravani wrote:

In D35643#815473, @probinson wrote:

Wouldn't you also want to verify .debug_abbrev.dwo?

Ideally, yes (as well as other sections from the split dwarf5 proposal).
So far I've focused on verifications for/related to the .debug_info section.

I'll +1 Paul's comment - seems worthwhile generalizing things across .dwo/non-.dwo where it's convenient/easy - otherwise we'll end up with a weirdly inconsistent verification coverage.

Is it the case that we may need abbreviation declarations for .debug_info section from .debug_abbrev.dwo?

No, .debug_abbrev.dwo would only apply to .debug_info.dwo - but pretty much any check you can do for .debug_info you should be able to do for .debug_info.dwo

[probinson]

I am pretty sure that any verification you want to do on .debug_abbrev would be equally applicable to .debug_abbrev.dwo; I suspect we could also verify that .debug_abbrev.dwo did not use certain forms, e.g. DW_FORM_addr, but that would be a bonus.

[sgravani]

In D35643#815593, @dblaikie wrote:

In D35643#815498, @sgravani wrote:

In D35643#815473, @probinson wrote:

Wouldn't you also want to verify .debug_abbrev.dwo?

Ideally, yes (as well as other sections from the split dwarf5 proposal).
So far I've focused on verifications for/related to the .debug_info section.

I'll +1 Paul's comment - seems worthwhile generalizing things across .dwo/non-.dwo where it's convenient/easy - otherwise we'll end up with a weirdly inconsistent verification coverage.

Is it the case that we may need abbreviation declarations for .debug_info section from .debug_abbrev.dwo?

No, .debug_abbrev.dwo would only apply to .debug_info.dwo - but pretty much any check you can do for .debug_info you should be able to do for .debug_info.dwo

I went ahead with the generalization. https://reviews.llvm.org/D35698

Thanks for the feedback!

[sgravani]

In D35643#816408, @probinson wrote:

I am pretty sure that any verification you want to do on .debug_abbrev would be equally applicable to .debug_abbrev.dwo; I suspect we could also verify that .debug_abbrev.dwo did not use certain forms, e.g. DW_FORM_addr, but that would be a bonus.

I'll add this check in a future patch! Thanks :)

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

class DWARFVerifier {
   /// This function currently checks for:
   /// - addresses within a sequence that decrease in value
   /// - invalid file indexes
   void verifyDebugLineRows();
 
 public:
   DWARFVerifier(raw_ostream &S, DWARFContext &D)
       : OS(S), DCtx(D) {}
+  /// Verify the information in the .debug_abbrev section.
+  ///
+  /// Currently, we check that no Abbreviation Declaration has more than one
+  /// attributes with the same name.
+  ///
+  /// \returns true if the .debug_abbrev verifies successfully, false otherwise.
+  bool handleDebugAbbrev();
   /// Verify the information in the .debug_info section.
   ///
   /// Any errors are reported to the stream that was this object was
   /// constructed with.
   ///
   /// \returns true if the .debug_info verifies successfully, false otherwise.
   bool handleDebugInfo();
 

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

DWARFDie DWARFContext::getDIEForOffset(uint32_t Offset) {
   if (auto *CU = CUs.getUnitForOffset(Offset))
     return CU->getDIEForOffset(Offset);
   return DWARFDie();
 }
 
 bool DWARFContext::verify(raw_ostream &OS, DIDumpType DumpType) {
   bool Success = true;
   DWARFVerifier verifier(OS, *this);
+  Success &= verifier.handleDebugAbbrev();
   if (DumpType == DIDT_All || DumpType == DIDT_Info) {
     if (!verifier.handleDebugInfo())
       Success = false;
   }
   if (DumpType == DIDT_All || DumpType == DIDT_Line) {
     if (!verifier.handleDebugLine())
       Success = false;
   }

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

for (unsigned I = 0; I < NumDies; ++I) {
     for (auto AttrValue : Die.attributes()) {
       NumUnitErrors += verifyDebugInfoAttribute(Die, AttrValue);
       NumUnitErrors += verifyDebugInfoForm(Die, AttrValue);
     }
   }
   return NumUnitErrors == 0;
 }
 
+bool DWARFVerifier::handleDebugAbbrev() {
+  OS << "Verifying .debug_abbrev...\n";
+
+  const DWARFObject &DObj = DCtx.getDWARFObj();
+  if (DObj.getAbbrevSection().empty()) {
+    OS << "Warning: .debug_abbrev is empty.\n";
+    return true;
+  }
+
+  unsigned NumErrors = 0;
+  const DWARFDebugAbbrev *Abbrev = DCtx.getDebugAbbrev();
+  if (Abbrev) {
+    const DWARFAbbreviationDeclarationSet *AbbrDecls =
+        Abbrev->getAbbreviationDeclarationSet(0);
+    for (auto AbbrDecl : *AbbrDecls) {
+      SmallDenseSet<uint16_t> AttributeSet;
+      for (auto Attribute : AbbrDecl.attributes()) {
+        auto Result = AttributeSet.insert(Attribute.Attr);
+        if (!Result.second) {
+          OS << format("Error: Abbreviation declaration with code %d ",
+                       AbbrDecl.getCode());
+          OS << "contains multiple " << AttributeString(Attribute.Attr)
+             << " attributes.\n";
+          ++NumErrors;
+        }
+      }
+    }
+  }
+  return NumErrors == 0;
+}
+
 bool DWARFVerifier::handleDebugInfo() {
   OS << "Verifying .debug_info Unit Header Chain...\n";
 
   const DWARFObject &DObj = DCtx.getDWARFObj();
   DWARFDataExtractor DebugInfoData(DObj, DObj.getInfoSection(),
                                    DCtx.isLittleEndian(), 0);
   uint32_t NumDebugInfoErrors = 0;
   uint32_t OffsetStart = 0, Offset = 0, UnitIdx = 0;

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

 # RUN: llvm-mc %s -filetype obj -triple x86_64-apple-darwin -o - \
 # RUN: | not llvm-dwarfdump -verify - \
 # RUN: | FileCheck %s
 
-# CHECK: Verifying .debug_info Unit Header Chain...
+# CHECK: Verifying .debug_abbrev...
+# CHECK-NEXT: Error: Abbreviation declaration with code 2 contains multiple DW_AT_low_pc attributes.
+# CHECK-NEXT: Verifying .debug_info Unit Header Chain...
 # CHECK-NEXT: error: DIE has invalid DW_AT_stmt_list encoding:{{[[:space:]]}}
 # CHECK-NEXT: 0x0000000c: DW_TAG_compile_unit [1] *
 # CHECK-NEXT: DW_AT_producer [DW_FORM_strp] ( .debug_str[0x00000000] = "clang version 5.0.0 (trunk 308185) (llvm/trunk 308186)")
 # CHECK-NEXT: DW_AT_language [DW_FORM_data2]  (DW_LANG_C99)
 # CHECK-NEXT: DW_AT_name [DW_FORM_strp] ( .debug_str[0x00000037] = "basic.c")
 # CHECK-NEXT: DW_AT_stmt_list [DW_FORM_strx4] ( indexed (00000000) string = )
 # CHECK-NEXT: DW_AT_comp_dir [DW_FORM_strp] ( .debug_str[0x0000003f] = "/Users/sgravani/Development/tests")
 # CHECK-NEXT: DW_AT_low_pc [DW_FORM_addr] (0x0000000000000000)
Lsection_abbrev:
   .byte 6                       ## DW_FORM_data4
   .byte 0                       ## EOM(1)
   .byte 0                       ## EOM(2)
   .byte 2                       ## Abbreviation Code
   .byte 46                      ## DW_TAG_subprogram
   .byte 1                       ## DW_CHILDREN_yes
   .byte 17                      ## DW_AT_low_pc
   .byte 1                       ## DW_FORM_addr
-  .byte 18                      ## DW_AT_high_pc
+  .byte  17                      ## DW_AT_low_pc -- Error: Die at offset 0x0000002b contains multiple DW_AT_low_pc attributes.
   .byte 6                       ## DW_FORM_data4
   .byte 64                      ## DW_AT_frame_base
   .byte 24                      ## DW_FORM_exprloc
   .byte 3                       ## DW_AT_name
   .byte 14                      ## DW_FORM_strp
   .byte 58                      ## DW_AT_decl_file
   .byte 11                      ## DW_FORM_data1
   .byte 59                      ## DW_AT_decl_line