[llvm-objdump] Change errors to warnings for symbol section name dumping

[MaskRay]

Summary

Port D69671 (llvm-readobj) to llvm-objdump. Add a class llvm::objdump::Dumper
and move some free functions into Dumper so that they can call
reportUniqueWarning.

Warnings seems preferable in these cases as the issue is localized and we can
continue dumping other information.

[MaskRay]

implement reportUniqueWarning

[MaskRay]

simplify printRelocations

[mtrofin]

llvm/tools/llvm-objdump/llvm-objdump.h

120 ↗

(On Diff #538299)

nit: final

120 ↗

(On Diff #538299)

How about renaming this to WarningsCollector and scoping it down to just managing the warnings, and then the print functions that can tolerate failures take it as a parameter (instead of being its members)?

[MaskRay]

llvm/tools/llvm-objdump/llvm-objdump.h

120 ↗

(On Diff #538299)

There is no virtual function so I think adding final has no performance advantage. I think we will soon derive llvm::objdump::Dumper like we do for llvm::ObjDumper (llvm/tools/llvm-readobj/ObjDumper.h). So adding final here will lead to one extra line diff in the future...

The name Dumper is to match llvm-readobj. In the future, we shall probably move more print* functions into member functions and extend Dumper with more states, so I think Dumper is proper.

[mtrofin]

llvm/tools/llvm-objdump/llvm-objdump.h

120 ↗

(On Diff #538299)

I see - could you put a TODO comment on top of Dumper, then, so the design intent is clear during the period when not all of the print functions have been moved - I assume doing that (the move) in this patch would be overkill?

[MaskRay]

add a TODO to dumpObject in llvm-objdump.cpp

[MaskRay]

llvm/tools/llvm-objdump/llvm-objdump.h

120 ↗

(On Diff #538299)

Thank you! I am going to add the comment to dumpObject beside those print* free function calls to remind myself and future contributors.

llvm/test/tools/llvm-objdump/ELF/section-symbols.test

+## ELF section symbols use the corresponding section names when printing
+## unnamed symbols. This test verifies this and also that appropriate things
+## are printed if the section is somehow invalid.
+
+# RUN: yaml2obj %s -o %t1
+# RUN: llvm-objdump --syms %t1 2>&1 | FileCheck %s -DFILE=%t1
+
+# CHECK:      SYMBOL TABLE:
+# CHECK-NEXT: 00000000 l    d  .foo   00000000 .foo
+# CHECK-NEXT: 00000000 l    d  .foo   00000000 .foo
+# CHECK-NEXT: warning: '[[FILE]]': invalid section index: 67
+# CHECK-NEXT: warning: '[[FILE]]': invalid section index: 68
+# CHECK-NOT:  {{.}}
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS32
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+  - Name: .foo
+    Type: SHT_PROGBITS
+Symbols:
+## Case 1: a valid unnamed section symbol.
+  - Name: ""
+    Section: .foo
+    Type: STT_SECTION
+## Case 2: a valid named section symbol.
+  - Name: "symbol1"
+    Section: .foo
+    Type: STT_SECTION
+## Case 3: an unnamed section symbol with invalid index.
+  - Name: ""
+    Index: 0x43
+    Type: STT_SECTION
+## Case 4: a named section symbol with invalid index.
+  - Name: "symbol2"
+    Index: 0x44
+    Type: STT_SECTION

llvm/test/tools/llvm-objdump/invalid-symbol-table-size.test

 ## This test ensures llvm-objdump emits a helpful error message when dumping a symbol table
 ## whose sh_size isn't a multiple of the symbol size (sh_size % sizeof(Elf_Sym) != 0).
 
 ## a) Broken .symtab symbol table. Valid .dynsym symbol table.
 # RUN: yaml2obj -DBITS=32 -DSIZE=33 -DDYNSIZE=32 %s -o %t.32-bit.o
-# RUN: not llvm-objdump --syms %t.32-bit.o 2>&1 | FileCheck -DSIZE=33 -DSYMSIZE=16 -DINDEX=2 %s
+# RUN: llvm-objdump --syms %t.32-bit.o 2>&1 | FileCheck -DSIZE=33 -DSYMSIZE=16 -DINDEX=2 %s
 # RUN: yaml2obj -DBITS=64 -DSIZE=49 -DDYNSIZE=48 %s -o %t.64-bit.o
-# RUN: not llvm-objdump --syms %t.64-bit.o 2>&1 | FileCheck -DSIZE=49 -DSYMSIZE=24 -DINDEX=2 %s
+# RUN: llvm-objdump --syms %t.64-bit.o 2>&1 | FileCheck -DSIZE=49 -DSYMSIZE=24 -DINDEX=2 %s
 
 ## b) Broken .dynsym symbol table. Valid .symtab symbol table.
 # RUN: yaml2obj -DBITS=32 -DSIZE=32 -DDYNSIZE=33 %s -o %t.32-bit.o
-# RUN: not llvm-objdump --dynamic-syms %t.32-bit.o 2>&1 | FileCheck -DSIZE=33 -DSYMSIZE=16 -DINDEX=3 %s
+# RUN: llvm-objdump --dynamic-syms %t.32-bit.o 2>&1 | FileCheck -DSIZE=33 -DSYMSIZE=16 -DINDEX=3 %s
 # RUN: yaml2obj -DBITS=64 -DSIZE=48 -DDYNSIZE=49 %s -o %t.64-bit.o
-# RUN: not llvm-objdump --dynamic-syms %t.64-bit.o 2>&1 | FileCheck -DSIZE=49 -DSYMSIZE=24 -DINDEX=3 %s
+# RUN: llvm-objdump --dynamic-syms %t.64-bit.o 2>&1 | FileCheck -DSIZE=49 -DSYMSIZE=24 -DINDEX=3 %s
 
-# CHECK: error: {{.*}} section [index [[INDEX]]] has an invalid sh_size ([[SIZE]]) which is not a multiple of its sh_entsize ([[SYMSIZE]])
+# CHECK: warning: {{.*}} section [index [[INDEX]]] has an invalid sh_size ([[SIZE]]) which is not a multiple of its sh_entsize ([[SYMSIZE]])
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS[[BITS]]
   Data:    ELFDATA2LSB
   Type:    ET_DYN
   Machine: EM_X86_64
 Sections:

llvm/tools/llvm-objdump/llvm-objdump.cpp

printSymbol(O, Sym, *SymbolVersionsOrErr, FileName, ArchiveName,
                 ArchitectureName, DumpDynamic);
 }
 
 void objdump::printSymbol(const ObjectFile &O, const SymbolRef &Symbol,
                           ArrayRef<VersionEntry> SymbolVersions,
                           StringRef FileName, StringRef ArchiveName,
                           StringRef ArchitectureName, bool DumpDynamic) {
   const MachOObjectFile *MachO = dyn_cast<const MachOObjectFile>(&O);
-  uint64_t Address = unwrapOrError(Symbol.getAddress(), FileName, ArchiveName,
-                                   ArchitectureName);
+  Expected<uint64_t> AddrOrErr = Symbol.getAddress();
+  if (!AddrOrErr) {
+    reportWarning(toString(AddrOrErr.takeError()), FileName);
+    return;
+  }
+  uint64_t Address = *AddrOrErr;
   if ((Address < StartAddress) || (Address > StopAddress))
     return;
   SymbolRef::Type Type =
       unwrapOrError(Symbol.getType(), FileName, ArchiveName, ArchitectureName);
   uint32_t Flags =
       unwrapOrError(Symbol.getFlags(), FileName, ArchiveName, ArchitectureName);
 
   // Don't ask a Mach-O STAB symbol for its section unless you know that