[DebugInfo] Add error handling to DWARFDebugAbbrev::getAbbreviationDeclarationSet

[bulbazord]

Summary

This gives us more meaningful information when
getAbbreviationDeclarationSet fails. Right now only
verifyAbbrevSection actually uses the error that it returns, but the
other call sites could be rewritten to take advantage of the returned error.

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp

167

Not sure why you've added the parentheses? Oh, I see they're there in the other return clauses in this method - but they're unnecessary there too, and should probably be removed to avoid confusion.

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp

157

Comment?

I'm not too familiar with the verifier, but it seems to me that if there's an error parsing the abbrevs, we should report it as a verification failure, including the error's message, rather than throwing away that message. Of course, that isn't part of this patch, hence a FIXME comment would probably be appropriate.

[bulbazord]

Address feedback from @jhenderson

[dblaikie]

llvm/lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp

162

no need for std::move here, I think? (implicit move from a local variable when you return local)

[bulbazord]

llvm/lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp

162

Last time I did that, some older compilers on some buildbots didn't like it. I added the std::move to be explicit.

[jhenderson]

LGTM, but please wait for @dblaikie too.

[fdeazeve]

LGTM!

llvm/lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp

162

The move is required because the return type is Expected so, without the move, we are calling the constructor of Expected with an Error &, and this ctor doesn't exist. We need an Error && for the right ctor to be found

llvm/test/tools/llvm-dwarfdump/X86/empty-CU.s

22

Out of curiosity, do you know what EOM stands for? End of <something>? End <something> Marker?

[dblaikie]

Fine by me

[bulbazord]

Thanks all!

llvm/test/tools/llvm-dwarfdump/X86/empty-CU.s

22

I don't know for sure, but I assume it means "End Of" Marker.

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugAbbrev.h

class DWARFDebugAbbrev {
 
   mutable DWARFAbbreviationDeclarationSetMap AbbrDeclSets;
   mutable DWARFAbbreviationDeclarationSetMap::const_iterator PrevAbbrOffsetPos;
   mutable std::optional<DataExtractor> Data;
 
 public:
   DWARFDebugAbbrev(DataExtractor Data);
 
-  const DWARFAbbreviationDeclarationSet *
+  Expected<const DWARFAbbreviationDeclarationSet *>
   getAbbreviationDeclarationSet(uint64_t CUAbbrOffset) const;
 
   void dump(raw_ostream &OS) const;
   void parse() const;
 
   DWARFAbbreviationDeclarationSetMap::const_iterator begin() const {
     parse();
     return AbbrDeclSets.begin();

llvm/lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp

void DWARFDebugAbbrev::dump(raw_ostream &OS) const {
   }
 
   for (const auto &I : AbbrDeclSets) {
     OS << format("Abbrev table for offset: 0x%8.8" PRIx64 "\n", I.first);
     I.second.dump(OS);
   }
 }
 
-const DWARFAbbreviationDeclarationSet*
+Expected<const DWARFAbbreviationDeclarationSet *>
 DWARFDebugAbbrev::getAbbreviationDeclarationSet(uint64_t CUAbbrOffset) const {
   const auto End = AbbrDeclSets.end();
   if (PrevAbbrOffsetPos != End && PrevAbbrOffsetPos->first == CUAbbrOffset) {
-    return &(PrevAbbrOffsetPos->second);
+    return &PrevAbbrOffsetPos->second;
   }
 
   const auto Pos = AbbrDeclSets.find(CUAbbrOffset);
   if (Pos != End) {
     PrevAbbrOffsetPos = Pos;
-    return &(Pos->second);
+    return &Pos->second;
   }
 
-  if (Data && CUAbbrOffset < Data->getData().size()) {
-    uint64_t Offset = CUAbbrOffset;
-    DWARFAbbreviationDeclarationSet AbbrDecls;
-    if (Error Err = AbbrDecls.extract(*Data, &Offset)) {
-      // FIXME: We should propagate the error upwards.
-      consumeError(std::move(Err));
-      return nullptr;
-    }
-    PrevAbbrOffsetPos =
-        AbbrDeclSets.insert(std::make_pair(CUAbbrOffset, std::move(AbbrDecls)))
-            .first;
-    return &PrevAbbrOffsetPos->second;
-  }
-
-  return nullptr;
-}
+  if (!Data || CUAbbrOffset >= Data->getData().size())
+    return make_error<llvm::object::GenericBinaryError>(
+        "the abbreviation offset into the .debug_abbrev section is not valid");
+
+  if (Error Err = AbbrDecls.extract(*Data, &Offset))
+    return std::move(Err);
+

llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp

assert(getDIEIndex(Die) ==
            "Bad unit die");
     return &DieArray.back();
   }
 
   return nullptr;
 }
 
 const DWARFAbbreviationDeclarationSet *DWARFUnit::getAbbreviations() const {
-  if (!Abbrevs)
-    Abbrevs = Abbrev->getAbbreviationDeclarationSet(getAbbreviationsOffset());
+  if (!Abbrevs) {
+    Expected<const DWARFAbbreviationDeclarationSet *> AbbrevsOrError =
+        Abbrev->getAbbreviationDeclarationSet(getAbbreviationsOffset());
+    if (!AbbrevsOrError) {
+      // FIXME: We should propagate this error upwards.
+      consumeError(AbbrevsOrError.takeError());
+      return nullptr;
+    }
+    Abbrevs = *AbbrevsOrError;
+  }
   return Abbrevs;
 }
 
 std::optional<object::SectionedAddress> DWARFUnit::getBaseAddress() {
   if (BaseAddr)
     return BaseAddr;
 
   DWARFDie UnitDie = (SU ? SU : this)->getUnitDIE();

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp

if (Version >= 5) {
     AbbrOffset = isUnitDWARF64 ? DebugInfoData.getU64(Offset) : DebugInfoData.getU32(Offset);
     ValidType = dwarf::isUnitType(UnitType);
   } else {
     UnitType = 0;
     AbbrOffset = isUnitDWARF64 ? DebugInfoData.getU64(Offset) : DebugInfoData.getU32(Offset);
     AddrSize = DebugInfoData.getU8(Offset);
   }
 
-  if (!DCtx.getDebugAbbrev()->getAbbreviationDeclarationSet(AbbrOffset))
+  Expected<const DWARFAbbreviationDeclarationSet *> AbbrevSetOrErr =
+      DCtx.getDebugAbbrev()->getAbbreviationDeclarationSet(AbbrOffset);
+  if (!AbbrevSetOrErr) {
     ValidAbbrevOffset = false;
+    // FIXME: A problematic debug_abbrev section is reported below in the form
+    // of a `note:`. We should propagate this error there (or elsewhere) to
+    // avoid losing the specific problem with the debug_abbrev section.
+    consumeError(AbbrevSetOrErr.takeError());
+  }
 
   ValidLength = DebugInfoData.isValidOffset(OffsetStart + Length + 3);
   ValidVersion = DWARFContext::isSupportedVersion(Version);
   ValidAddrSize = DWARFContext::isAddressSizeSupported(AddrSize);
   if (!ValidLength || !ValidVersion || !ValidAddrSize || !ValidAbbrevOffset ||
       !ValidType) {
     Success = false;
     error() << format("Units[%d] - start offset: 0x%08" PRIx64 " \n", UnitIndex,
unsigned DWARFVerifier::verifyDebugInfoCallSite(const DWARFDie &Die) {
 
   return 0;
 }
 
 unsigned DWARFVerifier::verifyAbbrevSection(const DWARFDebugAbbrev *Abbrev) {
   if (!Abbrev)
     return 0;
 
-  const DWARFAbbreviationDeclarationSet *AbbrDecls =
+  Expected<const DWARFAbbreviationDeclarationSet *> AbbrDeclsOrErr =
       Abbrev->getAbbreviationDeclarationSet(0);
-  // FIXME: If we failed to get a DWARFAbbreviationDeclarationSet, it's possible
-  // that there are errors. We need to propagate the error from
-  // getAbbreviationDeclarationSet.
-  if (!AbbrDecls)
-    return 0;
+  if (!AbbrDeclsOrErr) {
+    error() << toString(AbbrDeclsOrErr.takeError()) << "\n";
+    return 1;
+  }
 
+  const auto *AbbrDecls = *AbbrDeclsOrErr;
   unsigned NumErrors = 0;
   for (auto AbbrDecl : *AbbrDecls) {
     SmallDenseSet<uint16_t> AttributeSet;
     for (auto Attribute : AbbrDecl.attributes()) {
       auto Result = AttributeSet.insert(Attribute.Attr);
       if (!Result.second) {
         error() << "Abbreviation declaration contains multiple "
                 << AttributeString(Attribute.Attr) << " attributes.\n";

llvm/test/tools/llvm-dwarfdump/X86/empty-CU.s

 # RUN: llvm-mc %s -filetype obj -triple x86_64-apple-darwin -o - \
 # RUN: | not llvm-dwarfdump --verify --debug-info - \
 # RUN: | FileCheck %s
+# CHECK: error: unable to decode LEB128 at offset 0x00000005: malformed uleb128, extends past end
 # CHECK: error: Compilation unit without DIE.
 
         .section        __DWARF,__debug_info,regular,debug
 .long 8  # CU length
 .short 3 # Version
 .long 0  # Abbrev offset
 .byte 4  # AddrSize
 .byte 1  # Abbrev 1
 .long 7  # Unit lengthh...
 .short 3
 .long 0
 .byte 4
         .section        __DWARF,__debug_abbrev,regular,debug
 .byte 1    # Abbrev code
 .byte 0x11 # TAG_compile_unit
 .byte 0    # no children
-.byte 0    # no attributes
-.byte 0
+.byte 0    # EOM(1)
+.byte 0    # EOM(2)
+           # Intentionally missing EOM(3)