[DebugInfo] Change return type of DWARFDebugAbbrev::parse

[bulbazord]

To make DWARFDebugAbbrev more amenable to error-handling, I would like to change the return type of DWARFDebugAbbrev::parse from void to Error. Users of DWARFDebugAbbrev can consume the error if they want to use all the valid DWARF that was parsed (without worrying about the malformed DWARF) or stop when the parse fails if the use case needs to be strict.

This also will bring the LLVM DWARFDebugAbbrev interface closer to LLDB's which opens up the opportunity for LLDB adopt the LLVM implementation with minimal changes.

[llvmbot]

@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-objectyaml

Changes

To make DWARFDebugAbbrev more amenable to error-handling, I would like to change the return type of DWARFDebugAbbrev::parse from void to Error. Users of DWARFDebugAbbrev can consume the error if they want to use all the valid DWARF that was parsed (without worrying about the malformed DWARF) or stop when the parse fails if the use case needs to be strict.

This also will bring the LLVM DWARFDebugAbbrev interface closer to LLDB's which opens up the opportunity for LLDB adopt the LLVM implementation with minimal changes.

---

Full diff: https://github.com/llvm/llvm-project/pull/67191.diff

5 Files Affected:

• (modified) llvm/include/llvm/DebugInfo/DWARF/DWARFDebugAbbrev.h (+1-1)
• (modified) llvm/lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp (+8-6)
• (modified) llvm/tools/obj2yaml/dwarf2yaml.cpp (+10-3)
• (modified) llvm/tools/obj2yaml/macho2yaml.cpp (+2-4)
• (modified) llvm/tools/obj2yaml/obj2yaml.h (+1-1)

diff --git a/llvm/include/llvm/DebugInfo/DWARF/DWARFDebugAbbrev.h b/llvm/include/llvm/DebugInfo/DWARF/DWARFDebugAbbrev.h
index 8e4aa3aa61e9ea5..6439827ef70f0f0 100644
--- a/llvm/include/llvm/DebugInfo/DWARF/DWARFDebugAbbrev.h
+++ b/llvm/include/llvm/DebugInfo/DWARF/DWARFDebugAbbrev.h
@@ -70,7 +70,7 @@ class DWARFDebugAbbrev {
   getAbbreviationDeclarationSet(uint64_t CUAbbrOffset) const;
 
   void dump(raw_ostream &OS) const;
-  void parse() const;
+  Error parse() const;
 
   DWARFAbbreviationDeclarationSetMap::const_iterator begin() const {
     assert(!Data && "Must call parse before iterating over DWARFDebugAbbrev");
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp b/llvm/lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp
index 3014e61f566a909..85959ecc5e17f14 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp
@@ -105,9 +105,9 @@ std::string DWARFAbbreviationDeclarationSet::getCodeRange() const {
 DWARFDebugAbbrev::DWARFDebugAbbrev(DataExtractor Data)
     : AbbrDeclSets(), PrevAbbrOffsetPos(AbbrDeclSets.end()), Data(Data) {}
 
-void DWARFDebugAbbrev::parse() const {
+Error DWARFDebugAbbrev::parse() const {
   if (!Data)
-    return;
+    return Error::success();
   uint64_t Offset = 0;
   auto I = AbbrDeclSets.begin();
   while (Data->isValidOffset(Offset)) {
@@ -116,17 +116,19 @@ void DWARFDebugAbbrev::parse() const {
     uint64_t CUAbbrOffset = Offset;
     DWARFAbbreviationDeclarationSet AbbrDecls;
     if (Error Err = AbbrDecls.extract(*Data, &Offset)) {
-      // FIXME: We should propagate the error upwards.
-      consumeError(std::move(Err));
-      break;
+      Data = std::nullopt;
+      return Err;
     }
     AbbrDeclSets.insert(I, std::make_pair(CUAbbrOffset, std::move(AbbrDecls)));
   }
   Data = std::nullopt;
+  return Error::success();
 }
 
 void DWARFDebugAbbrev::dump(raw_ostream &OS) const {
-  parse();
+  if (Error Err = parse())
+    // FIXME: We should propagate this error or otherwise display it.
+    llvm::consumeError(std::move(Err));
 
   if (AbbrDeclSets.empty()) {
     OS << "< EMPTY >\n";
diff --git a/llvm/tools/obj2yaml/dwarf2yaml.cpp b/llvm/tools/obj2yaml/dwarf2yaml.cpp
index 3d3798d5f18d784..f2c96968cc7deeb 100644
--- a/llvm/tools/obj2yaml/dwarf2yaml.cpp
+++ b/llvm/tools/obj2yaml/dwarf2yaml.cpp
@@ -22,11 +22,12 @@
 
 using namespace llvm;
 
-void dumpDebugAbbrev(DWARFContext &DCtx, DWARFYAML::Data &Y) {
+Error dumpDebugAbbrev(DWARFContext &DCtx, DWARFYAML::Data &Y) {
   auto AbbrevSetPtr = DCtx.getDebugAbbrev();
   if (AbbrevSetPtr) {
     uint64_t AbbrevTableID = 0;
-    AbbrevSetPtr->parse();
+    if (Error Err = AbbrevSetPtr->parse())
+      return Err;
     for (auto AbbrvDeclSet : *AbbrevSetPtr) {
       Y.DebugAbbrev.emplace_back();
       Y.DebugAbbrev.back().ID = AbbrevTableID++;
@@ -48,6 +49,7 @@ void dumpDebugAbbrev(DWARFContext &DCtx, DWARFYAML::Data &Y) {
       }
     }
   }
+  return Error::success();
 }
 
 Error dumpDebugAddr(DWARFContext &DCtx, DWARFYAML::Data &Y) {
@@ -220,7 +222,12 @@ void dumpDebugInfo(DWARFContext &DCtx, DWARFYAML::Data &Y) {
     if (NewUnit.Version >= 5)
       NewUnit.Type = (dwarf::UnitType)CU->getUnitType();
     const DWARFDebugAbbrev *DebugAbbrev = DCtx.getDebugAbbrev();
-    DebugAbbrev->parse();
+    // FIXME: Ideally we would propagate this error upwards, but that would
+    // prevent us from displaying any debug info at all. For now we just consume
+    // the error and display everything that was parsed successfully.
+    if (Error Err = DebugAbbrev->parse())
+      llvm::consumeError(std::move(Err));
+
     NewUnit.AbbrevTableID = std::distance(
         DebugAbbrev->begin(),
         llvm::find_if(
diff --git a/llvm/tools/obj2yaml/macho2yaml.cpp b/llvm/tools/obj2yaml/macho2yaml.cpp
index ed678ea28a71735..efd43a5f1285f74 100644
--- a/llvm/tools/obj2yaml/macho2yaml.cpp
+++ b/llvm/tools/obj2yaml/macho2yaml.cpp
@@ -139,10 +139,8 @@ MachODumper::constructSection(MachO::section_64 Sec, size_t SecIndex) {
 
 static Error dumpDebugSection(StringRef SecName, DWARFContext &DCtx,
                               DWARFYAML::Data &DWARF) {
-  if (SecName == "__debug_abbrev") {
-    dumpDebugAbbrev(DCtx, DWARF);
-    return Error::success();
-  }
+  if (SecName == "__debug_abbrev")
+    return dumpDebugAbbrev(DCtx, DWARF);
   if (SecName == "__debug_aranges")
     return dumpDebugARanges(DCtx, DWARF);
   if (SecName == "__debug_info") {
diff --git a/llvm/tools/obj2yaml/obj2yaml.h b/llvm/tools/obj2yaml/obj2yaml.h
index e0204c6c608f4b1..03d7db5317acde1 100644
--- a/llvm/tools/obj2yaml/obj2yaml.h
+++ b/llvm/tools/obj2yaml/obj2yaml.h
@@ -46,7 +46,7 @@ struct Data;
 }
 } // namespace llvm
 
-void dumpDebugAbbrev(llvm::DWARFContext &DCtx, llvm::DWARFYAML::Data &Y);
+llvm::Error dumpDebugAbbrev(llvm::DWARFContext &DCtx, llvm::DWARFYAML::Data &Y);
 llvm::Error dumpDebugAddr(llvm::DWARFContext &DCtx, llvm::DWARFYAML::Data &Y);
 llvm::Error dumpDebugARanges(llvm::DWARFContext &DCtx,
                              llvm::DWARFYAML::Data &Y);

[jh7370]

In principle, seems reasonable to me, though I worry slightly about the impact on existing users who won't expect this breaking-if-they-have-appropriate-options-selected change. How confident are you that you've caught all in-tree uses?

Also, does this change allow us to check the lower-level error in a test somewhere, e.g. in a unit test?

[bulbazord]

In principle, seems reasonable to me, though I worry slightly about the impact on existing users who won't expect this breaking-if-they-have-appropriate-options-selected change. How confident are you that you've caught all in-tree uses?

I would say I lean more towards confident than not. I've built clang, lld, lldb, bolt, and compiler-rt (all on an arm64-apple-macOS machine). The call-sites that I updated were found through a combination of commenting out the method in the header to introduce a compilation failure, IDE tooling, and tests. If there are places I did not catch, it is because of conditional compilation checks that my build did not meet. As always, if and when I land this I will pay attention to the CI machines.

I assume downstream users will adjust accordingly.

Also, does this change allow us to check the lower-level error in a test somewhere, e.g. in a unit test?

Added a unit test similar to the other ones I introduced.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[jh7370]

LGTM.