[ELF] - Add ability for DWARFContextInMemory to exit early when any error happen.

[grimar]

Summary

Change introduces error reporting policy for DWARFContextInMemory.
New callback provided by client is able to handle error on it's
side and return Halt or Continue.

That allows to either keep current behavior when parser prints all errors
but continues parsing object or implement something very different, like
stop parsing on a first error and report an error from client side.

[dblaikie]

I'd probably favor the API change of returning Expected<unique_ptr<DWARFContext>> - potentially with a backwards-compatible API that maintains the old API (& calls the new API, reports any Error and silently returns the DWARFContext). Though as you say, some callers want to continue on error - so this new API wouldn't express that.

Probably what might work better is if a callback for error handling was provided (a default one would be to maintain the existing behavior: print and continue. But other callers could say "print and stop". (eg: instead of "AllowsError", have llvm::function_ref<bool(Error E)> HandleError then do 'if (HandleError(std::move(E))) { return; }" or the like)

Why is the speed of lld important when the input is broken anyway? Printing all the errors may make it easier for a user to address them all without going through several slow link steps to get more errors (the same reason the compiler doesn't stop after the first error)? (eg: one corrupted compressed section, then one non-corrupt compressed section)

Also important to include test coverage - potentially in a unit test (though testing the IO would be tricky - but you could test if, given an input that had one broken, followed by a non-broken thing, then with the "stop on error" case returns no things and the "continue on error" returns the one non-broken thing.

[grimar]

In D34328#783474, @dblaikie wrote:

I'd probably favor the API change of returning Expected<unique_ptr<DWARFContext>> - potentially with a backwards-compatible API that maintains the old API (& calls the new API, reports any Error and silently returns the DWARFContext). Though as you say, some callers want to continue on error - so this new API wouldn't express that.

Yes and I not sure how much is good to provide 2 API that are doing the same. I think doing something universal is cleaner,
even if it will require more reasonable amount of changes around, probably.

Probably what might work better is if a callback for error handling was provided (a default one would be to maintain the existing behavior: print and continue. But other callers could say "print and stop". (eg: instead of "AllowsError", have llvm::function_ref<bool(Error E)> HandleError then do 'if (HandleError(std::move(E))) { return; }" or the like)

That sounds really good for me. That way if we still have HadError flag or alike, we can print all errors in a format that is specific for any tool on its side and then after object is parsed check the flag and report final error then, if it is required.

Or even better may be return not a bool, but enum, like PRINT_AND_STOP, JUST_PRINT, JUST_STOP, IGNORE_AND_CONTINUE.
Last one may be used to emit warnings untill we do not implement some relocations for example, like for .gdb_index case (while it seems it does not affect index),
and after that can switch to errors in one single change.

One more case where it is useful is when we have callback on LLD side that print errors, we can implement the same error reporting strategy we already use now: a limit of 20 errors, we can print them on our side and return IGNORE_AND_CONTINUE,
all time until reaching the limit - we just return JUST_STOP. Then check the HadError flag and stop link.

Why is the speed of lld important when the input is broken anyway? Printing all the errors may make it easier for a user to address them all without going through several slow link steps to get more errors (the same reason the compiler doesn't stop after the first error)? (eg: one corrupted compressed section, then one non-corrupt compressed section)

it makes sence to me. Main reason for this patch is to add way to fail link somehow still, if we can report more errors, we can do that.

Also important to include test coverage - potentially in a unit test (though testing the IO would be tricky - but you could test if, given an input that had one broken, followed by a non-broken thing, then with the "stop on error" case returns no things and the "continue on error" returns the one non-broken thing.

OK.

Thanks a lot for your ideas, I will think a bit more on above and update patch with something different.

[grimar]

• Addressed review comments.
• Added testcase.

[grimar]

• Fix comment formatting.

[dblaikie]

include/llvm/DebugInfo/DWARF/DWARFContext.h

357 ↗

(On Diff #103957)

Might be worth introducing an enum ("Halt/Continue") rather than using a bool, so it's more obvious/legible which result means what.

unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp

2150–2155 ↗

(On Diff #103957)

I thought one of the ideas of the yaml2obj stuff was to enable the writing of invalid inputs, etc - is that not the case? Should that functionality be added to the yaml2obj somehow rather than having big byte arrays?

[grimar]

• Addressed review comments.

include/llvm/DebugInfo/DWARF/DWARFContext.h

357 ↗

(On Diff #103957)

Done.

unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp

2150–2155 ↗

(On Diff #103957)

What I was trying to achieve here is to produce broken object::ObjectFile somehow to be able to test first constructor of DWARFContextInMemory with new functionality.
Extending yaml2obj to produce StringRef (if I correctly understood the suggestion) would require doing some librarification for it. Like splitting part of yaml2obj into separate library probably. And then test would be able to use such library.

It probably can be usefull (I am not sure how much other users we may have, looks can be usefull for testcases like this), but sounds as a large enough separate change. It is unclear for me if introducing a new library worth that atm.

Fortunately I found alternative way to achieve what I wanted initially in this testcase, without using additional arrays and dependency on yaml2obj. Solution partially consistent with other testcases in this file which aslo use dwarfgen::Generator but for different things.

Alternative solution I was thinking about - is to introduce --error-limit=X option to llvm-dwarfdump. It probably may be usefull itself and would allow to write testcase in a regular manner (separate test file + one/two broken input files and regular tools calls).

[dblaikie]

Looks great - thanks!

[grimar]

In D34328#792140, @dblaikie wrote:

Looks great - thanks!

Thanks for review ! I'll commit this at my tomorrow morning.
(want to be online for case if this breaks any slow BB or something else).

[chapuni]

llvm/trunk/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp

18

s/Codegen/CodeGen/

[grimar]

llvm/trunk/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp

18

Ah, thanks !

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

private:
   /// Return the compile unit that includes an offset (relative to .debug_info).
   DWARFCompileUnit *getCompileUnitForOffset(uint32_t Offset);
 
   /// Return the compile unit which contains instruction with provided
   /// address.
   DWARFCompileUnit *getCompileUnitForAddress(uint64_t Address);
 };
 
+/// Used as a return value for a error callback passed to DWARF context.
+/// Callback should return Halt if client application wants to stop
+/// object parsing, or should return Continue otherwise.
+enum class ErrorPolicy { Halt, Continue };
+
 /// DWARFContextInMemory is the simplest possible implementation of a
 /// DWARFContext. It assumes all content is available in memory and stores
 /// pointers to it.
 class DWARFContextInMemory : public DWARFContext {
   virtual void anchor();
 
   StringRef FileName;
   bool IsLittleEndian;
class DWARFContextInMemory : public DWARFContext {
 
   StringRef *MapSectionToMember(StringRef Name);
 
   /// If Sec is compressed section, decompresses and updates its contents
   /// provided by Data. Otherwise leaves it unchanged.
   Error maybeDecompress(const object::SectionRef &Sec, StringRef Name,
                         StringRef &Data);
 
+  /// Function used to handle default error reporting policy. Prints a error
+  /// message and returns Continue, so DWARF context ignores the error.
+  static ErrorPolicy defaultErrorHandler(Error E);
+
 public:
-  DWARFContextInMemory(const object::ObjectFile &Obj,
-    const LoadedObjectInfo *L = nullptr);
+  DWARFContextInMemory(
+      const object::ObjectFile &Obj, const LoadedObjectInfo *L = nullptr,
+      function_ref<ErrorPolicy(Error)> HandleError = defaultErrorHandler);
 
   DWARFContextInMemory(const StringMap<std::unique_ptr<MemoryBuffer>> &Sections,
                        uint8_t AddrSize,
                        bool isLittleEndian = sys::IsLittleEndianHost);
 
   StringRef getFileName() const override { return FileName; }
   bool isLittleEndian() const override { return IsLittleEndian; }
   uint8_t getAddressSize() const override { return AddressSize; }

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

static Expected<SymInfo> getSymbolInfo(const object::ObjectFile &Obj,
   if (Sym != Obj.symbol_end()) {
     bool New;
     std::tie(CacheIt, New) = Cache.insert({*Sym, {0, 0}});
     if (!New)
       return CacheIt->second;
 
     Expected<uint64_t> SymAddrOrErr = Sym->getAddress();
     if (!SymAddrOrErr)
-      return createError("error: failed to compute symbol address: ",
+      return createError("failed to compute symbol address: ",
                          SymAddrOrErr.takeError());
 
     // Also remember what section this symbol is in for later
     auto SectOrErr = Sym->getSection();
     if (!SectOrErr)
-      return createError("error: failed to get symbol section: ",
+      return createError("failed to get symbol section: ",
                          SectOrErr.takeError());
 
     RSec = *SectOrErr;
     Ret.Address = *SymAddrOrErr;
   } else if (auto *MObj = dyn_cast<MachOObjectFile>(&Obj)) {
     RSec = MObj->getRelocationSection(Reloc.getRawDataRefImpl());
     Ret.Address = RSec->getAddress();
   }
if (auto Err = Decompressor->resizeAndDecompress(Out))
     return Err;
 
   UncompressedSections.emplace_back(std::move(Out));
   Data = UncompressedSections.back();
 
   return Error::success();
 }
 
-DWARFContextInMemory::DWARFContextInMemory(const object::ObjectFile &Obj,
-                                           const LoadedObjectInfo *L)
+ErrorPolicy DWARFContextInMemory::defaultErrorHandler(Error E) {
+  errs() << "error: " + toString(std::move(E)) << '\n';
+  return ErrorPolicy::Continue;
+}
+
+DWARFContextInMemory::DWARFContextInMemory(
+    const object::ObjectFile &Obj, const LoadedObjectInfo *L,
+    function_ref<ErrorPolicy(Error)> HandleError)
     : FileName(Obj.getFileName()), IsLittleEndian(Obj.isLittleEndian()),
       AddressSize(Obj.getBytesInAddress()) {
   for (const SectionRef &Section : Obj.sections()) {
     StringRef name;
     Section.getName(name);
     // Skip BSS and Virtual sections, they aren't interesting.
     bool IsBSS = Section.isBSS();
     if (IsBSS)
       continue;
     bool IsVirtual = Section.isVirtual();
     if (IsVirtual)
       continue;
     StringRef data;
 
     section_iterator RelocatedSection = Section.getRelocatedSection();
     // Try to obtain an already relocated version of this section.
     // Else use the unrelocated section from the object file. We'll have to
     // apply relocations ourselves later.
     if (!L || !L->getLoadedSectionContents(*RelocatedSection, data))
       Section.getContents(data);
 
     if (auto Err = maybeDecompress(Section, name, data)) {
-      errs() << "error: failed to decompress '" + name + "', " +
-                    toString(std::move(Err))
-             << '\n';
+      ErrorPolicy EP = HandleError(
+          createError("failed to decompress '" + name + "', ", std::move(Err)));
+      if (EP == ErrorPolicy::Halt)
+        return;
       continue;
     }
 
     // Compressed sections names in GNU style starts from ".z",
     // at this point section is decompressed and we drop compression prefix.
     name = name.substr(
         name.find_first_not_of("._z")); // Skip ".", "z" and "_" prefixes.
 
for (const SectionRef &Section : Obj.sections()) {
     for (const RelocationRef &Reloc : Section.relocations()) {
       // FIXME: it's not clear how to correctly handle scattered
       // relocations.
       if (isRelocScattered(Obj, Reloc))
         continue;
 
       Expected<SymInfo> SymInfoOrErr = getSymbolInfo(Obj, Reloc, L, AddrCache);
       if (!SymInfoOrErr) {
-        errs() << toString(SymInfoOrErr.takeError()) << '\n';
+        if (HandleError(SymInfoOrErr.takeError()) == ErrorPolicy::Halt)
+          return;
         continue;
       }
 
       object::RelocVisitor V(Obj);
       uint64_t Val = V.visit(Reloc.getType(), Reloc, SymInfoOrErr->Address);
       if (V.error()) {
         SmallString<32> Name;
         Reloc.getTypeName(Name);
-        errs() << "error: failed to compute relocation: " << Name << "\n";
+        ErrorPolicy EP = HandleError(
+            createError("failed to compute relocation: " + name + ", ",
+                        errorCodeToError(object_error::parse_failed)));
+        if (EP == ErrorPolicy::Halt)
+          return;
         continue;
       }
       RelocAddrEntry Rel = {SymInfoOrErr->SectionIndex, Val};
       Map->insert({Reloc.getOffset(), Rel});
     }
   }
 }
 

llvm/trunk/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp

 #include "DwarfGenerator.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/BinaryFormat/Dwarf.h"
 #include "llvm/Config/llvm-config.h"
+#include "llvm/Codegen/AsmPrinter.h"
 #include "llvm/DebugInfo/DWARF/DWARFCompileUnit.h"
 #include "llvm/DebugInfo/DWARF/DWARFContext.h"
 #include "llvm/DebugInfo/DWARF/DWARFDie.h"
 #include "llvm/DebugInfo/DWARF/DWARFFormValue.h"
+#include "llvm/MC/MCContext.h"
+#include "llvm/MC/MCSectionELF.h"
+#include "llvm/MC/MCStreamer.h"
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/ObjectYAML/DWARFEmitter.h"
 #include "llvm/ObjectYAML/DWARFYAML.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/TargetSelect.h"
 #include "gtest/gtest.h"
 #include <climits>
TEST(DWARFDebugInfo, TestDwarfVerifyCUDontShareLineTable) {
   auto ErrOrSections = DWARFYAML::EmitDebugSections(yamldata);
   ASSERT_TRUE((bool)ErrOrSections);
   DWARFContextInMemory DwarfContext(*ErrOrSections, 8);
   VerifyError(DwarfContext, "error: two compile unit DIEs, 0x0000000b and "
                             "0x0000001f, have the same DW_AT_stmt_list section "
                             "offset:");
 }
 
+TEST(DWARFDebugInfo, TestErrorReportingPolicy) {
+  initLLVMIfNeeded();
+  auto ExpectedDG = dwarfgen::Generator::create(Triple("x86_64-pc-linux"),
+                                                4 /*DwarfVersion*/);
+  if (HandleExpectedError(ExpectedDG))
+    return;
+  dwarfgen::Generator *DG = ExpectedDG.get().get();
+  AsmPrinter *AP = DG->getAsmPrinter();
+  MCContext *MC = DG->getMCContext();
+
+  // Emit two compressed sections with broken headers.
+  AP->OutStreamer->SwitchSection(
+      MC->getELFSection(".zdebug_foo", 0 /*Type*/, 0 /*Flags*/));
+  AP->OutStreamer->EmitBytes("0");
+  AP->OutStreamer->SwitchSection(
+      MC->getELFSection(".zdebug_bar", 0 /*Type*/, 0 /*Flags*/));
+  AP->OutStreamer->EmitBytes("0");
+
+  MemoryBufferRef FileBuffer(DG->generate(), "dwarf");
+  auto Obj = object::ObjectFile::createObjectFile(FileBuffer);
+  EXPECT_TRUE((bool)Obj);
+
+  // Case 1: error handler handles all errors. That allows
+  // DWARFContextInMemory
+  //         to parse whole file and find both two errors we know about.
+  int Errors = 0;
+  DWARFContextInMemory Ctx1(*Obj.get(), nullptr, [&](Error E) {
+    ++Errors;
+    consumeError(std::move(E));
+    return ErrorPolicy::Continue;
+  });
+  EXPECT_TRUE(Errors == 2);
+
+  // Case 2: error handler stops parsing of object after first error.
+  Errors = 0;
+  DWARFContextInMemory Ctx2(*Obj.get(), nullptr, [&](Error E) {
+    ++Errors;
+    consumeError(std::move(E));
+    return ErrorPolicy::Halt;
+  });
+  EXPECT_TRUE(Errors == 1);
+}
+
 } // end anonymous namespace

llvm/trunk/unittests/DebugInfo/DWARF/DwarfGenerator.h

public:
   ///
   /// \returns a dwarfgen::CompileUnit that can be used to retrieve the compile
   /// unit dwarfgen::DIE that can be used to add attributes and add child DIE
   /// objedts to.
   dwarfgen::CompileUnit &addCompileUnit();
 
   BumpPtrAllocator &getAllocator() { return Allocator; }
   AsmPrinter *getAsmPrinter() const { return Asm.get(); }
+  MCContext *getMCContext() const { return MC.get(); }
   DIEAbbrevSet &getAbbrevSet() { return Abbreviations; }
   DwarfStringPool &getStringPool() { return *StringPool; }
 
   /// Save the generated DWARF file to disk.
   ///
   /// \param Path the path to save the ELF file to.
   bool saveFile(StringRef Path);
 };
 
 } // end namespace dwarfgen
 
 } // end namespace llvm
 
 #endif // LLVM_UNITTESTS_DEBUG_INFO_DWARF_DWARFGENERATOR_H