[DebugInfo] Report errors when DWARFUnitHeader::applyIndexEntry fails

[bulbazord]

Motivation: LLDB is able to report errors about these scenarios whereas LLVM's DWARF parser only gives a boolean success/fail. I want to migrate LLDB to using LLVM's DWARFUnitHeader class, but I don't want to lose some of the error reporting, so I'm adding it to the LLVM class first.

[llvmbot]

@llvm/pr-subscribers-debuginfo

Author: Alex Langford (bulbazord)

Changes

Motivation: LLDB is able to report errors about these scenarios whereas LLVM's DWARF parser only gives a boolean success/fail. I want to migrate LLDB to using LLVM's DWARFUnitHeader class, but I don't want to lose some of the error reporting, so I'm adding it to the LLVM class first.

---

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

2 Files Affected:

• (modified) llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h (+1-1)
• (modified) llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp (+23-7)

diff --git a/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h b/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
index f20e71781f46be..80c27aea893123 100644
--- a/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
+++ b/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
@@ -85,7 +85,7 @@ class DWARFUnitHeader {
                 uint64_t *offset_ptr, DWARFSectionKind SectionKind);
   // For units in DWARF Package File, remember the index entry and update
   // the abbreviation offset read by extract().
-  bool applyIndexEntry(const DWARFUnitIndex::Entry *Entry);
+  Error applyIndexEntry(const DWARFUnitIndex::Entry *Entry);
   uint64_t getOffset() const { return Offset; }
   const dwarf::FormParams &getFormParams() const { return FormParams; }
   uint16_t getVersion() const { return FormParams.Version; }
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp b/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
index 9f455fa7e96a7e..985566ad329f63 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
@@ -98,8 +98,12 @@ void DWARFUnitVector::addUnitsImpl(
         if (!IndexEntry)
           IndexEntry = Index.getFromOffset(Header.getOffset());
       }
-      if (IndexEntry && !Header.applyIndexEntry(IndexEntry))
-        return nullptr;
+      if (IndexEntry) {
+        if (Error ApplicationErr = Header.applyIndexEntry(IndexEntry)) {
+          Context.getWarningHandler()(std::move(ApplicationErr));
+          return nullptr;
+        }
+      }
       std::unique_ptr<DWARFUnit> U;
       if (Header.isTypeUnit())
         U = std::make_unique<DWARFTypeUnit>(Context, InfoSection, Header, DA,
@@ -334,21 +338,33 @@ Error DWARFUnitHeader::extract(DWARFContext &Context,
   return Error::success();
 }
 
-bool DWARFUnitHeader::applyIndexEntry(const DWARFUnitIndex::Entry *Entry) {
+Error DWARFUnitHeader::applyIndexEntry(const DWARFUnitIndex::Entry *Entry) {
   assert(Entry);
   assert(!IndexEntry);
   IndexEntry = Entry;
   if (AbbrOffset)
-    return false;
+    return createStringError(errc::invalid_argument,
+                             "DWARF package unit from offset 0x%8.8" PRIx64
+                             " has a non-zero abbreviation offset",
+                             Offset);
+
   auto *UnitContrib = IndexEntry->getContribution();
   if (!UnitContrib ||
       UnitContrib->getLength() != (getLength() + getUnitLengthFieldByteSize()))
-    return false;
+    return createStringError(errc::invalid_argument,
+                             "DWARF package unit at offset 0x%8.8" PRIx64
+                             "has an inconsistent index",
+                             Offset);
+
   auto *AbbrEntry = IndexEntry->getContribution(DW_SECT_ABBREV);
   if (!AbbrEntry)
-    return false;
+    return createStringError(errc::invalid_argument,
+                             "DWARF package unit at offset 0x%8.8 " PRIx64
+                             " mising abbreviation column",
+                             Offset);
+
   AbbrOffset = AbbrEntry->getOffset();
-  return true;
+  return Error::success();
 }
 
 Error DWARFUnit::extractRangeList(uint64_t RangeListOffset,

[bulbazord]

I realize there are no tests for this currently, I'm working through that but may need some help. :)

[bulbazord]

Okay, I've added some tests and made sure that they actually test all the relevant code paths. This is good to review now! 😄

[JDevlieghere]

Seems like this clause is covering two things that should be reported separately: the lack of a unit and the length of the contribution not matching the expected length. The latter could be part of the error message (e.g. expected x, actual y).

[bulbazord]

I can split them into separate cases and then test them separately (I think). I'll also update the error message :)

[felipepiovezan]

LGTM!
I find it surprising that, looking for the word "package" in the dwarf 5 spec yields the first result in section 7, which is usually reserved for the finer points of the data representation

[dwblaikie]

looks good to me

[JDevlieghere]

LGTM, thanks!