Improve error handling in llvm-dwarfdump

[aprantl]

Summary

Without this patch we're only showing a generic error message derived from the error code to the end user.

rdar://79378794

[jhenderson]

llvm/test/tools/llvm-dwarfdump/X86/lc_malformed.test

2

This test looks somewhat tangential to the code change?

llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp

273

I think the code would be a lot simpler if you switched which is the "base" error reporting function. If I'm not mistaken, you can use toString to convert an Error into an error message. You could convert an error code to an Error and report things that way. Approximate code:

static void error(StringRef Prefix, Error Err) {
  if (!Err)
    return;
  WithColor::error() << Prefix << ": " << toString(std::move(Err)) << "\n";
  exit(1);  
}

static void error(StringRef Prefix, std::error_code EC) {
  error(errorCodeToError(EC));
}

[JDevlieghere]

llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp

273

I was going to suggest the same thing, so +1 from me

[aprantl]

llvm/test/tools/llvm-dwarfdump/X86/lc_malformed.test

3

Without the patch you would see Invalid data was encountered while parsing the file.

[aprantl]

Thanks, this is much cleaner!

[aprantl]

It wasn't clear to me that toString was able to deal with BinaryError.

[jhenderson]

LGTM. If you want to bring back the test in a separate patch, that's absolutely fine (it sounds like there is some missing test coverage from what you said).

llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp

271

Nit: looks like there's some trailing whitespace here.

[aprantl]

LGTM. If you want to bring back the test in a separate patch,

I forgot to git-add it before uploading the patch....

[thakis]

This breaks check-llvm http://45.33.8.238/linux/49596/step_12.txt

Please take a look, and please revert if it takes a bit to fix.

[aprantl]

Fixed in 5076d27ff01913a280e8ca8b40d38fb11fe7113d

llvm/test/tools/llvm-dwarfdump/X86/lc_malformed.test

+# RUN: yaml2obj %s | not llvm-dwarfdump --uuid - 2>&1 | FileCheck %s
+# CHECK: load command 0 filesize field in LC_SEGMENT_64 greater than vmsize field
+
+--- !mach-o
+FileHeader:
+  magic:           0xFEEDFACF
+  cputype:         0x01000007
+  cpusubtype:      0x00000003
+  filetype:        0x00000001
+  ncmds:           1
+  sizeofcmds:      24
+  flags:           0x00002000
+  reserved:        0x00000000
+LoadCommands:
+  - cmd:             LC_SEGMENT_64
+    cmdsize:         232
+    segname:         ''
+    vmaddr:          0
+    vmsize:          80
+    fileoff:         384
+    filesize:        81
+    maxprot:         7
+    initprot:        7
+    nsects:          1
+    flags:           0
+    Sections:
+      - sectname:        __text
+        segname:         __TEXT
+        addr:            0x0000000000000000
+        size:            15
+        offset:          0x00000180
+        align:           4
+        reloff:          0x00000000
+        nreloc:          0
+        flags:           0x80000400
+        reserved1:       0x00000000
+        reserved2:       0x00000000
+        reserved3:       0x00000000
+...

llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp

 static alias VerboseAlias("v", desc("Alias for --verbose."), aliasopt(Verbose),
                           cat(DwarfDumpCategory), cl::NotHidden);
 static cl::extrahelp
     HelpResponse("\nPass @FILE as argument to read options from FILE.\n");
 } // namespace
 /// @}
 //===----------------------------------------------------------------------===//
 
-static void error(StringRef Prefix, std::error_code EC) {
-  if (!EC)
+static void error(StringRef Prefix, Error Err) {
+  if (!Err)
     return;
-  WithColor::error() << Prefix << ": " << EC.message() << "\n";
+  WithColor::error() << Prefix << ": " << toString(std::move(Err)) << "\n";
   exit(1);
 }
 
+static void error(StringRef Prefix, std::error_code EC) {
+  error(Prefix, errorCodeToError(EC));
+}
+
 static DIDumpOptions getDumpOpts(DWARFContext &C) {
   DIDumpOptions DumpOpts;
   DumpOpts.DumpType = DumpType;
   DumpOpts.ChildRecurseDepth = ChildRecurseDepth;
   DumpOpts.ParentRecurseDepth = ParentRecurseDepth;
   DumpOpts.ShowAddresses = !Diff;
   DumpOpts.ShowChildren = ShowChildren;
   DumpOpts.ShowParents = ShowParents;
static bool handleBuffer(StringRef Filename, MemoryBufferRef Buffer,
                          HandlerFn HandleObj, raw_ostream &OS);
 
 static bool handleArchive(StringRef Filename, Archive &Arch,
                           HandlerFn HandleObj, raw_ostream &OS) {
   bool Result = true;
   Error Err = Error::success();
   for (auto Child : Arch.children(Err)) {
     auto BuffOrErr = Child.getMemoryBufferRef();
-    error(Filename, errorToErrorCode(BuffOrErr.takeError()));
+    error(Filename, BuffOrErr.takeError());
     auto NameOrErr = Child.getName();
-    error(Filename, errorToErrorCode(NameOrErr.takeError()));
+    error(Filename, NameOrErr.takeError());
     std::string Name = (Filename + "(" + NameOrErr.get() + ")").str();
     Result &= handleBuffer(Name, BuffOrErr.get(), HandleObj, OS);
   }
-  error(Filename, errorToErrorCode(std::move(Err)));
+  error(Filename, std::move(Err));
 
   return Result;
 }
 
 static bool handleBuffer(StringRef Filename, MemoryBufferRef Buffer,
                          HandlerFn HandleObj, raw_ostream &OS) {
   Expected<std::unique_ptr<Binary>> BinOrErr = object::createBinary(Buffer);
-  error(Filename, errorToErrorCode(BinOrErr.takeError()));
+  error(Filename, BinOrErr.takeError());
 
   bool Result = true;
   auto RecoverableErrorHandler = [&](Error E) {
     Result = false;
     WithColor::defaultErrorHandler(std::move(E));
   };
   if (auto *Obj = dyn_cast<ObjectFile>(BinOrErr->get())) {
     if (filterArch(*Obj)) {
for (auto &ObjForArch : Fat->objects()) {
               DWARFContext::create(Obj, nullptr, "", RecoverableErrorHandler);
           if (!HandleObj(Obj, *DICtx, ObjName, OS))
             Result = false;
         }
         continue;
       } else
         consumeError(MachOOrErr.takeError());
       if (auto ArchiveOrErr = ObjForArch.getAsArchive()) {
-        error(ObjName, errorToErrorCode(ArchiveOrErr.takeError()));
+        error(ObjName, ArchiveOrErr.takeError());
         if (!handleArchive(ObjName, *ArchiveOrErr.get(), HandleObj, OS))
           Result = false;
         continue;
       } else
         consumeError(ArchiveOrErr.takeError());
     }
   else if (auto *Arch = dyn_cast<Archive>(BinOrErr->get()))
     Result = handleArchive(Filename, *Arch, HandleObj, OS);