[llvm-readobj] Improve error message for for --string-dump

[StephenTozer]

Summary

Fixes bug 40630: https://bugs.llvm.org/show_bug.cgi?id=40630

This patch changes the error message when the section specified by
--string-dump cannot be found by including the name of the section in
the error message and changing the prefix text to not imply that the
file itself was invalid. As part of this change some uses of
std::error_code have been replaced with the llvm Error class to better
encapsulate the error info (rather than passing File strings around),
and the WithColor class replaces string literal error prefixes.

[jhenderson]

I've added a few more reviewers who have been reviewing stuff in this area recently. It would be good to get some other comments on the interface changes with regards to error message printing.

llvm/test/tools/llvm-readobj/elf-dynamic-malformed.test

6

I assume this is testing the file name? If so could you do similar to what is done above, e.g. in basic.test.

llvm/tools/llvm-readobj/ObjDumper.cpp

58–61

I think you'd be better off using createStringError() here, which takes variadic arguments and formats in printf style.

llvm/tools/llvm-readobj/llvm-readobj.cpp

401–403

Nit: Could you move this above the reportError immediately above, so that it lines up well in diffs with the old version, please?

[rupprecht]

I just have a couple nits on top of what James said, but overall this is already an improvement. Thanks!

llvm/test/tools/llvm-readobj/elf-malformed-pt-dynamic.test

24

Whatever is throwing this error should probably be creating a FileError
(I'm fine if this part is in a separate patch tho)

llvm/tools/llvm-readobj/llvm-readobj.cpp

371–372

... completely unrelated to this patch, but is this even necessary? I thought stderr was always unbuffered. There will never be anything to flush.

372

This can go on the previous line, i.e. << Msg << "\n";

401–403

I'm guessing this is actually necessary in order to delegate to reportError(StringRef, Error)

[grimar]

llvm/test/tools/llvm-readobj/basic.test

3–4

It is strange to me to see the quotes wrapping the file name here.
I think tools usually do not do that?

llvm/tools/llvm-readobj/llvm-readobj.cpp

377

You renamed EC->Err here, but the code below still use EC and this renaming
does not seem necessary (it looks as NFC). Not sure why it was done?

[StephenTozer]

llvm/tools/llvm-readobj/ObjDumper.cpp

58–61

The SecName variable in this case is a StringRef, which doesn't work as nicely with printf-style formatting: it has an internal char*, but this is not necessarily null terminated, so would need to be converted to an intermediate string variable to be compatible. Since it has built-in formatting with formatv, I thought that the more appropriate choice.

llvm/tools/llvm-readobj/llvm-readobj.cpp

401–403

The move is messy, but since this function now calls the other reportError function it has to be below it. The two alternatives to changing the function order would be to declare the above function earlier in this .cpp file, or declare it in the .h file. The latter seems like a bad idea to me, since it doesn't need to be made available outside of this file. I thought inserting an early declaration for one of the 3 overloads in the .cpp file was messier than changing the order, but if preserving the diff is more important then I can do that instead.

[StephenTozer]

llvm/tools/llvm-readobj/llvm-readobj.cpp

377

Leftover change from a previous edit of the function that was undone; the reason behind the rename however is that "EC" is short for Error Code; "Err" is the generic name for Error variables.

[StephenTozer]

llvm/test/tools/llvm-readobj/basic.test

3–4

The FileError class automatically wraps the file name in quotes when it produces a message output, while the old std::error_code errors left it entirely up to the error handler.

[StephenTozer]

Minor code cleanups.

[jhenderson]

LGTM, but please wait for another person to confirm approval.

[grimar]

LGTM with a nit.

llvm/tools/llvm-readobj/llvm-readobj.cpp

382

The code is still different from original, why?
(I am not against this renaming, but ideally, such things should be done in an NFC commits I think.)
So if there are no reasons for that, please revert.

[grimar]

llvm/tools/llvm-readobj/llvm-readobj.cpp

382

Sorry, I was not clear I guess.

1. I am not against the renaming EC->Err in a different patch/commit.
2. The current change affects how the functor is called (i.e. removes the reference), what is also seems like an improvement, but it is not relative to this patch still.

[StephenTozer]

Remove unnecessary/unrelated changes.

[grimar]

Still LGTM, thanks for the update!

[sbc100]

llvm/test/tools/llvm-readobj/basic.test

3–4

FWIW gnu objdump puts the filename in quotes in error messages too:

$ objdump -s /dev/null
objdump: Warning: '/dev/null' is not an ordinary file

[StephenTozer]

Fixes an Object test that I missed in the last revision.

llvm/test/Object/corrupt.test

 
 SYMNAME: invalid string offset
 
 
 // Version index in .gnu.version overflows the version map.
 RUN: not llvm-readobj %p/Inputs/corrupt-version.elf-x86_64 -dt \
 RUN:     2>&1 | FileCheck --check-prefix=VER %s
 
-VER: Error reading file: Invalid version entry.
+VER: error: Invalid version entry
 
 
 // The file is missing the dynamic string table but has references to it.
 RUN: not llvm-readobj -dynamic-table %p/Inputs/corrupt-invalid-strtab.elf.x86-64 \
 RUN:     2>&1 | FileCheck --check-prefix=STRTAB %s
 
 STRTAB: Invalid dynamic string table reference
 

 
 DYN-TABLE-SIZE:  Invalid entity size
 
 
 RUN: not llvm-readobj -dyn-relocations \
 RUN:   %p/Inputs/corrupt-invalid-dynamic-table-offset.elf.x86-64 2>&1 | \
 RUN:   FileCheck --check-prefix=DYN-TABLE-OFFSET %s
 
-DYN-TABLE-OFFSET: Invalid data was encountered while parsing the file.
+DYN-TABLE-OFFSET: error: Invalid data was encountered while parsing the file
 
 
 RUN: not llvm-readobj -dyn-relocations \
 RUN:   %p/Inputs/corrupt-invalid-dynamic-table-too-large.elf.x86-64 2>&1 | \
 RUN:   FileCheck --check-prefix=DYN-TABLE-TOO-LARGE %s
 
-DYN-TABLE-TOO-LARGE: Invalid data was encountered while parsing the file.
+DYN-TABLE-TOO-LARGE: error: Invalid data was encountered while parsing the file

llvm/test/tools/llvm-readobj/basic.test

 # Test case where input file does not exit.
 RUN: not llvm-readobj %t.blah 2>&1 | FileCheck --check-prefix=ENOENT %s
-ENOENT: {{.*}}.blah: {{[Nn]}}o such file or directory
+ENOENT: '{{.*}}.blah': {{[Nn]}}o such file or directory
 
 # Test case where input file is too small to be a recognised object file.
 RUN: touch %t.empty
 RUN: not llvm-readobj %t.empty 2>&1 | FileCheck --check-prefix=EMPTY %s
-EMPTY: {{.*}}.empty: The file was not recognized as a valid object file
+EMPTY: '{{.*}}.empty': The file was not recognized as a valid object file
 
 # Test that unrecognised files in archives are ignored.
 RUN: rm -f %t.a
 RUN: llvm-ar rc %t.a %t.empty
 RUN: llvm-readobj --all %t.a 2>&1 | FileCheck --check-prefix=NO-OUTPUT --allow-empty %s
 NO-OUTPUT-NOT: {{.}}
 
 # Test case where no switches are specified.

llvm/test/tools/llvm-readobj/codeview-merging-cycle.test

 ; RUN: not llvm-readobj -codeview-merged-types %S/Inputs/codeview-cycle.obj 2>&1 | FileCheck %s
 
-; CHECK: Error{{.*}} Input type graph contains cycles
+; CHECK: error{{.*}} Input type graph contains cycles
 
 ; To reproduce codeview-cycle.obj:
 ; $ cat codeview-cycle.asm
 ;       .model  flat, C
 ;       .code
 ; pfoo_list TYPEDEF PTR foo_list
 ; foo_list STRUCT
 ;       next pfoo_list ?
 ;       data dd ?
 ; foo_list ENDS
 ;       public  foo
 ; foo proc dst:ptr foo_list
 ;       ret
 ; foo   endp
 ;       end
 ; $ ml -c -Zi codeview-cycle.asm

llvm/test/tools/llvm-readobj/elf-dynamic-malformed.test

 # Test handling of a dynamic section size which is not a multiple of its entry size.
 # RUN: yaml2obj %s --docnum=1 -o %t.bad-size
 # RUN: not llvm-readobj --dynamic-table %t.bad-size 2>&1 | FileCheck %s --check-prefix ERR-SIZE
 # RUN: not llvm-readelf --dynamic-table %t.bad-size 2>&1 | FileCheck %s --check-prefix ERR-SIZE
 
-# ERR-SIZE: Error reading file: Invalid entity size.
+# ERR-SIZE: error: Invalid entity size
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_X86_64
 Sections:
Sections:
       - Section: .dynamic
 
 # Test handling of string references pointing past the end of the dynamic string table. In this case,
 # we have a DT_NEEDED tag pointing at offset 1 in a 1-byte string table.
 # RUN: yaml2obj %s --docnum=3 -o %t.bad-string
 # RUN: not llvm-readobj --dynamic-table %t.bad-string 2>&1 | FileCheck %s --check-prefix BAD-STRING
 # RUN: not llvm-readelf --dynamic-table %t.bad-string 2>&1 | FileCheck %s --check-prefix BAD-STRING
 
-# BAD-STRING: Error reading file: Invalid dynamic string table reference.
+# BAD-STRING: Invalid dynamic string table reference
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_X86_64
 Sections:

llvm/test/tools/llvm-readobj/elf-malformed-pt-dynamic.test

 # RUN: %python -c "with open(r'%t.truncated1', 'r+') as f: f.truncate(0x1001)"
 # RUN: not llvm-readobj %t.truncated1 --dynamic-table 2>&1 | FileCheck %s
 
 # Test case where the offset is too large to be in the file.
 # RUN: cp %t.stripped %t.truncated2
 # RUN: %python -c "with open(r'%t.truncated2', 'r+') as f: f.truncate(0xFFF)"
 # RUN: not llvm-readobj %t.truncated2 --dynamic-table 2>&1 | FileCheck %s
 
-# CHECK: Error reading file: Invalid data was encountered while parsing the file.
+# CHECK: error: Invalid data was encountered while parsing the file
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_X86_64
 Sections:

llvm/test/tools/llvm-readobj/elf-packed-relocs-error1.s

 // REQUIRES: x86-registered-target
 // RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu %s -o - | not llvm-readobj -relocations - 2>&1 | FileCheck %s
 
-// CHECK: Error reading file: invalid packed relocation header
+// CHECK: error: invalid packed relocation header
 
 .section .rela.dyn, "a", @0x60000001
 .ascii "APS9"

llvm/test/tools/llvm-readobj/elf-packed-relocs-error2.s

 // REQUIRES: x86-registered-target
 // RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu %s -o - | not llvm-readobj -relocations - 2>&1 | FileCheck %s
 
-// CHECK: Error reading file: malformed sleb128, extends past end
+// CHECK: error: malformed sleb128, extends past end
 
 .section .rela.dyn, "a", @0x60000001
 .ascii "APS2"

llvm/test/tools/llvm-readobj/elf-packed-relocs-error3.s

 // REQUIRES: x86-registered-target
 // RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu %s -o - | not llvm-readobj -relocations - 2>&1 | FileCheck %s
 
-// CHECK: Error reading file: malformed sleb128, extends past end
+// CHECK: error: malformed sleb128, extends past end
 
 .section .rela.dyn, "a", @0x60000001
 .ascii "APS2"
 .sleb128 4 // Number of relocations
 .sleb128 0 // Initial offset

llvm/test/tools/llvm-readobj/elf-packed-relocs-error4.s

 // REQUIRES: x86-registered-target
 // RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu %s -o - | not llvm-readobj -relocations - 2>&1 | FileCheck %s
 
-// CHECK: Error reading file: malformed sleb128, extends past end
+// CHECK: error: malformed sleb128, extends past end
 
 .section .rela.dyn, "a", @0x60000001
 .ascii "APS2"
 .sleb128 4 // Number of relocations
 .sleb128 0 // Initial offset
 
 .sleb128 2 // Number of relocations in group
 .sleb128 2 // RELOCATION_GROUPED_BY_OFFSET_DELTA_FLAG
 .sleb128 8 // offset delta

llvm/test/tools/llvm-readobj/elf-packed-relocs-error5.s

 // REQUIRES: x86-registered-target
 // RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu %s -o - | not llvm-readobj -relocations - 2>&1 | FileCheck %s
 
-// CHECK: Error reading file: relocation group unexpectedly large
+// CHECK: error: relocation group unexpectedly large
 
 .section .rela.dyn, "a", @0x60000001
 .ascii "APS2"
 .sleb128 4 // Number of relocations
 .sleb128 0 // Initial offset
 
 .sleb128 5 // Number of relocations in group
 .sleb128 2 // RELOCATION_GROUPED_BY_OFFSET_DELTA_FLAG
 .sleb128 8 // offset delta

llvm/test/tools/llvm-readobj/mips-got.test

 RUN:   FileCheck %s -check-prefix GNU-GOT-TLS
 RUN: llvm-readobj -mips-plt-got %p/Inputs/got-empty.exe.mipsel \
 RUN:                  --elf-output-style=GNU | \
 RUN:   FileCheck %s -check-prefix GNU-GOT-EMPTY
 RUN: llvm-readobj -mips-plt-got %p/Inputs/got-static.exe.mips \
 RUN:                  --elf-output-style=GNU | \
 RUN:   FileCheck %s -check-prefix GNU-GOT-STATIC
 
-GOT-OBJ: Error reading file: Cannot find .got section.
+GOT-OBJ: error: Cannot find .got section
 
 GOT-EXE:      Primary GOT {
 GOT-EXE-NEXT:   Canonical gp value: 0x418880
 GOT-EXE-NEXT:   Reserved entries [
 GOT-EXE-NEXT:     Entry {
 GOT-EXE-NEXT:       Address: 0x410890
 GOT-EXE-NEXT:       Access: -32752
 GOT-EXE-NEXT:       Initial: 0x0

 GOT-STATIC-NEXT:     Entry {
 GOT-STATIC-NEXT:       Address: 0x410120
 GOT-STATIC-NEXT:       Access: -32736
 GOT-STATIC-NEXT:       Initial: 0x400104
 GOT-STATIC-NEXT:     }
 GOT-STATIC-NEXT:   ]
 GOT-STATIC-NEXT: }
 
-GNU-GOT-OBJ: Error reading file: Cannot find .got section.
+GNU-GOT-OBJ: error: Cannot find .got section
 
 GNU-GOT-EXE:      Primary GOT:
 GNU-GOT-EXE-NEXT:  Canonical gp value: 00418880
 
 GNU-GOT-EXE:       Reserved entries:
 GNU-GOT-EXE-NEXT:    Address     Access  Initial Purpose
 GNU-GOT-EXE-NEXT:   00410890 -32752(gp) 00000000 Lazy resolver
 GNU-GOT-EXE-NEXT:   00410894 -32748(gp) 80000000 Module pointer (GNU extension)

llvm/test/tools/llvm-readobj/string-dump.test

 # CHECK-NEXT: [ 5] are
 # CHECK-NEXT: [ 9] some
 # CHECK-NEXT: [ e] strings
 # CHECK-NEXT: String dump of section '.not_null_terminated':
 # CHECK-NEXT: [ 0] no
 # CHECK-NEXT: [ 3] null{{$}}
 # CHECK-NOT:  {{.}}
 
-# RUN: not llvm-readobj --string-dump=does_not_exist %t 2>&1 | FileCheck %s --check-prefix=ERR
-# RUN: not llvm-readobj --string-dump=42 %t 2>&1 | FileCheck %s --check-prefix=ERR
+# RUN: not llvm-readobj --string-dump=does_not_exist %t 2>&1 | FileCheck %s --check-prefix=ERR1
+# RUN: not llvm-readobj --string-dump=42 %t 2>&1 | FileCheck %s --check-prefix=ERR2
 
-# RUN: not llvm-readelf --string-dump=does_not_exist %t 2>&1 | FileCheck %s --check-prefix=ERR
-# RUN: not llvm-readelf --string-dump=42 %t 2>&1 | FileCheck %s --check-prefix=ERR
+# RUN: not llvm-readelf --string-dump=does_not_exist %t 2>&1 | FileCheck %s --check-prefix=ERR1
+# RUN: not llvm-readelf --string-dump=42 %t 2>&1 | FileCheck %s --check-prefix=ERR2
 
-# ERR: Error reading file: invalid section reference.
+# ERR1: error: could not find section 'does_not_exist'
+# ERR2: error: could not find section '42'
 
 --- !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_REL
   Machine:         EM_X86_64
 Sections:
   - Name: .strings
     Type: SHT_PROGBITS
     Content: 686572650061726500736f6d6500737472696e677300
   - Name: .not_null_terminated
     Type: SHT_PROGBITS
     Content: 6e6f006e756c6c

llvm/test/tools/llvm-readobj/thin-archive-paths.test

 # RUN: llvm-readobj --file-headers a/relative.a | FileCheck %s --check-prefix=REL
 # REL: File: b/1.o
 
 # Show that relative paths in an error message for both archive and member look
 # sensible.
 # RUN: rm a/b/1.o
 # RUN: not llvm-readobj --file-headers a/relative.a 2>&1 | FileCheck %s --check-prefix=ERR1
 # RUN: not llvm-readelf --file-headers a/relative.a 2>&1 | FileCheck %s --check-prefix=ERR1
-# ERR1: Error reading file: a/relative.a: 'b/1.o': {{[Nn]}}o such file or directory
+# ERR1: error: 'a/relative.a': 'b/1.o': {{[Nn]}}o such file or directory
 
 # RUN: mkdir -p %t/c
 # RUN: cp %p/Inputs/trivial.obj.elf-x86-64 a/b/1.o
 # RUN: llvm-ar rcT c/absolute.a %t/a/b/1.o
 
 # Show that absolute paths in the file header printing are correct.
 # RUN: llvm-readobj --file-headers c/absolute.a | FileCheck %s --check-prefix=ABS -DDIR=%t
 # ABS: File: [[DIR]]/a/b/1.o
 
 # Show that absolute paths in an error message for both archive and member are correct.
 # RUN: rm a/b/1.o
 # RUN: not llvm-readobj --file-headers %t/c/absolute.a 2>&1 | FileCheck %s --check-prefix=ERR2 -DDIR=%t
 # RUN: not llvm-readelf --file-headers %t/c/absolute.a 2>&1 | FileCheck %s --check-prefix=ERR2 -DDIR=%t
-# ERR2: Error reading file: [[DIR]]/c/absolute.a: '[[DIR]]/a/b/1.o': {{[Nn]}}o such file or directory
+# ERR2: error: '[[DIR]]/c/absolute.a': '[[DIR]]/a/b/1.o': {{[Nn]}}o such file or directory

llvm/test/tools/llvm-readobj/thin-archive.test

 # Remove the second member and show that the first can still be dumped, but that the last isn't.
 # RUN: rm %t/2.o
 # RUN: not llvm-readobj --file-headers %t.a 2> %t.err | FileCheck %s --check-prefix=MISSING
 # RUN: FileCheck %s --check-prefix=ERR --input-file=%t.err
 
 # MISSING:     File: {{.*}}1.o
 # MISSING:     SectionHeaderCount: 10
 # MISSING-NOT: File: {{.*}}3.o
-# ERR: Error reading file: {{.*}}.a: '{{.*}}2.o': {{[Nn]}}o such file or directory
+# ERR: error: '{{.*}}.a': '{{.*}}2.o': {{[Nn]}}o such file or directory

llvm/test/tools/llvm-readobj/wasm-invalid.test

 # RUN: yaml2obj %s | not llvm-readobj -t - 2>&1 | FileCheck %s
 
 --- !WASM
 FileHeader:
   Version:         0x0000000c
 
-# CHECK: Error reading file: <stdin>: Bad version number
+# CHECK: error: '<stdin>': Bad version number

llvm/tools/llvm-readobj/ObjDumper.cpp

 ///
 //===----------------------------------------------------------------------===//
 
 #include "ObjDumper.h"
 #include "Error.h"
 #include "llvm-readobj.h"
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Support/Error.h"
+#include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "llvm/Support/raw_ostream.h"
 
 namespace llvm {
 
 ObjDumper::ObjDumper(ScopedPrinter &Writer) : W(Writer) {}
 
 ObjDumper::~ObjDumper() {
if (*StrPtr) {
 
       if (SectionName == SecName)
         return SecRef;
     } else if (SecIndex == SectionIndex)
       return SecRef;
 
     SecIndex++;
   }
-  return make_error<StringError>("invalid section reference",
-                                 object::object_error::parse_failed);
+  return make_error<StringError>(
+      formatv("could not find section '{0}'", SecName),
 }
 
 void ObjDumper::printSectionAsString(const object::ObjectFile *Obj,
                                      StringRef SecName) {
   Expected<object::SectionRef> SectionRefOrError =
       getSecNameOrIndexAsSecRef(Obj, SecName);
   if (!SectionRefOrError)
     error(std::move(SectionRefOrError));
   object::SectionRef Section = *SectionRefOrError;

llvm/tools/llvm-readobj/llvm-readobj.cpp

 #include "llvm/Support/DataTypes.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "llvm/Support/TargetRegistry.h"
+#include "llvm/Support/WithColor.h"
 
 using namespace llvm;
 using namespace llvm::object;
 
 namespace opts {
   cl::list<std::string> InputFilenames(cl::Positional,
     cl::desc("<input object files>"),
     cl::ZeroOrMore);
cl::opt<OutputStyleTy>
              cl::values(clEnumVal(LLVM, "LLVM default style"),
                         clEnumVal(GNU, "GNU readelf style")),
              cl::init(LLVM));
 } // namespace opts
 
 namespace llvm {
 
 LLVM_ATTRIBUTE_NORETURN void reportError(Twine Msg) {
-  errs() << "\nError reading file: " << Msg << ".\n";
-  errs().flush();
+  errs() << "\n";
+  WithColor::error(errs()) << Msg << "\n";
   exit(1);
 }
 
 void error(Error EC) {
   if (!EC)
     return;
   handleAllErrors(std::move(EC),
                   [&](const ErrorInfoBase &EI) { reportError(EI.message()); });
 }
 
 void error(std::error_code EC) {
   if (!EC)
     return;
   reportError(EC.message());
 }
 
 bool relocAddressLess(RelocationRef a, RelocationRef b) {
   return a.getOffset() < b.getOffset();
 }
 
 } // namespace llvm
 
-static void reportError(StringRef Input, std::error_code EC) {
-  if (Input == "-")
-    Input = "<stdin>";
-
-  reportError(Twine(Input) + ": " + EC.message());
-}
-
 static void reportError(StringRef Input, Error Err) {
   if (Input == "-")
     Input = "<stdin>";
-  std::string ErrMsg;
-  {
-    raw_string_ostream ErrStream(ErrMsg);
-    logAllUnhandledErrors(std::move(Err), ErrStream, Input + ": ");
-  }
-  reportError(ErrMsg);
+  error(createFileError(Input, std::move(Err)));
+
+static void reportError(StringRef Input, std::error_code EC) {
+  reportError(Input, errorCodeToError(EC));
 }
 
 static bool isMipsArch(unsigned Arch) {
   switch (Arch) {
   case llvm::Triple::mips:
   case llvm::Triple::mipsel:
   case llvm::Triple::mips64:
   case llvm::Triple::mips64el:
     return true;