[DebugInfo] Return an error from DWARFUnitHeader::applyIndexEntry

[bulbazord]

Summary

I'm in the process of converting LLDB to use LLVM's DWARFUnitHeader
instead of its own implementation. One big difference between the two is
that LLDB actually provides an error when applying an index entry to a
DWARFHeaderUnit in a DWO context. LLVM's implementation would be more
useful if it did the same.

[bulbazord]

I realize this should have a test as well. I didn't find any existing tests that I could easily extend so if anybody has any suggestions for a way to construct a test, please let me know!

[aprantl]

llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp

358

Are these actually specific to DWP files or should the error messages be more generic?

[bulbazord]

llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp

358

These error messages are inspired by the equivalent ones in LLDB, but I'm not entirely sure. My understanding is that this code only executes in the context of DWO, so if it's specific to DWP I wouldn't be surprised. Maybe somebody else can chime in and give some better context.

[dblaikie]

I realize this should have a test as well. I didn't find any existing tests that I could easily extend so if anybody has any suggestions for a way to construct a test, please let me know!

Yeah, would be good to have test coverage. I'd start by adding an assert(false) to the function you're modifying (or maybe just to one of the error paths, though it's likely they're not so well tested/might not have coverage) - to identify some test cases that exercise this code,t hen maybe you can extend those tests or create new ones inspired by the existing ones?

llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h

public:
   /// Parse a unit header from \p debug_info starting at \p offset_ptr.
   /// Note that \p SectionKind is used as a hint to guess the unit type
   /// for DWARF formats prior to DWARFv5. In DWARFv5 the unit type is
   /// explicitly defined in the header and the hint is ignored.
   bool extract(DWARFContext &Context, const DWARFDataExtractor &debug_info,
                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; }
   dwarf::DwarfFormat getFormat() const { return FormParams.Format; }
   uint8_t getAddressByteSize() const { return FormParams.AddrSize; }
   uint8_t getRefAddrByteSize() const { return FormParams.getRefAddrByteSize(); }
   uint8_t getDwarfOffsetByteSize() const {
     return FormParams.getDwarfOffsetByteSize();

llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp

Parser = [=, &Context, &Obj, &Section, &SOS,
           if (Header.isTypeUnit())
             IndexEntry = Index.getFromHash(Header.getTypeHash());
           else if (auto DWOId = Header.getDWOId())
             IndexEntry = Index.getFromHash(*DWOId);
         }
         if (!IndexEntry)
           IndexEntry = Index.getFromOffset(Header.getOffset());
       }
-      if (IndexEntry && !Header.applyIndexEntry(IndexEntry))
+      if (IndexEntry) {
+        if (Error Err = Header.applyIndexEntry(IndexEntry)) {
+          Context.getWarningHandler()(std::move(Err));
           return nullptr;
+        }
+      }
       std::unique_ptr<DWARFUnit> U;
       if (Header.isTypeUnit())
         U = std::make_unique<DWARFTypeUnit>(Context, InfoSection, Header, DA,
                                              RS, LocSection, SS, SOS, AOS, LS,
                                              LE, IsDWO, *this);
       else
         U = std::make_unique<DWARFCompileUnit>(Context, InfoSection, Header,
                                                 DA, RS, LocSection, SS, SOS,
if (Error SizeErr = DWARFContext::checkAddressSizeSupported(
     return false;
   }
 
   // Keep track of the highest DWARF version we encounter across all units.
   Context.setMaxVersionIfGreater(getVersion());
   return true;
 }
 
-bool DWARFUnitHeader::applyIndexEntry(const DWARFUnitIndex::Entry *Entry) {
+Error DWARFUnitHeader::applyIndexEntry(const DWARFUnitIndex::Entry *Entry) {
   assert(Entry);
   assert(!IndexEntry);
-  IndexEntry = Entry;
   if (AbbrOffset)
-    return false;
-  auto *UnitContrib = IndexEntry->getContribution();
+    return createStringError(
+        errc::invalid_argument,
+        "DWARF package unit at offset 0x%8.8" PRIx64
+        " has a non-zero abbreviation offset prior to applying an index entry",
+        Offset);
+
+  auto *UnitContrib = Entry->getContribution();
   if (!UnitContrib ||
       UnitContrib->getLength() != (getLength() + getUnitLengthFieldByteSize()))
-    return false;
-  auto *AbbrEntry = IndexEntry->getContribution(DW_SECT_ABBREV);
+    return createStringError(errc::invalid_argument,
+                             "DWARF package unit at offset 0x%8.8" PRIx64
+                             " has an inconsistent unit index",
+                             Offset);
+
+  auto *AbbrEntry = Entry->getContribution(DW_SECT_ABBREV);
   if (!AbbrEntry)
-    return false;
+    return createStringError(errc::invalid_argument,
+                             "DWARF package unit at offset 0x%8.8" PRIx64
+                             " is missing an abbreviation column",
+                             Offset);
   AbbrOffset = AbbrEntry->getOffset();
-  return true;
+  IndexEntry = Entry;
+  return Error::success();
 }
 
 Error DWARFUnit::extractRangeList(uint64_t RangeListOffset,
                                   DWARFDebugRangeList &RangeList) const {
   // Require that compile unit is extracted.
   assert(!DieArray.empty());
   DWARFDataExtractor RangesData(Context.getDWARFObj(), *RangeSection,
                                 IsLittleEndian, getAddressByteSize());