[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

nit: final

120

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

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

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

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 -r --syms %t1 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t1 --implicit-check-not=warning:
+
+# 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:      RELOCATION RECORDS FOR [.foo]:
+# CHECK-NEXT: OFFSET   TYPE                     VALUE
+# CHECK-NEXT: 00000001 R_X86_64_NONE            .foo
+# CHECK-NEXT: 00000002 R_X86_64_NONE            .foo
+# CHECK-NEXT: 00000003 R_X86_64_NONE            {{$}}
+# CHECK-NEXT: 00000004 R_X86_64_NONE            {{$}}
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS32
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+  - Name: .foo
+    Type: SHT_PROGBITS
+  - Name: .rela.foo
+    Type: SHT_RELA
+    Link: .symtab
+    Info: .foo
+    Relocations:
+      - Offset: 0x1
+        Symbol: 1
+        Type:   R_X86_64_NONE
+      - Offset: 0x2
+        Symbol: 2
+        Type:   R_X86_64_NONE
+      - Offset: 0x3
+        Symbol: 3
+        Type:   R_X86_64_NONE
+      - Offset: 0x4
+        Symbol: 4
+        Type:   R_X86_64_NONE
+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/MachODump.cpp

 
 // ProcessMachO() is passed a single opened Mach-O file, which may be an
 // archive member and or in a slice of a universal file.  It prints the
 // the file name and header info and then processes it according to the
 // command line options.
 static void ProcessMachO(StringRef Name, MachOObjectFile *MachOOF,
                          StringRef ArchiveMemberName = StringRef(),
                          StringRef ArchitectureName = StringRef()) {
+  Dumper D(*MachOOF);
   // If we are doing some processing here on the Mach-O file print the header
   // info.  And don't print it otherwise like in the case of printing the
   // UniversalHeaders or ArchiveHeaders.
   if (Disassemble || Relocations || PrivateHeaders || ExportsTrie || Rebase ||
       Bind || SymbolTable || LazyBind || WeakBind || IndirectSymbols ||
       DataInCode || FunctionStartsType != FunctionStartsMode::None ||
       LinkOptHints || ChainedFixups || DyldInfo || DylibsUsed || DylibId ||
       Rpaths || ObjcMetaData || (!FilterSections.empty())) {
if (DyldInfo)
     PrintDyldInfo(MachOOF);
   if (ChainedFixups)
     PrintChainedFixups(MachOOF);
   if (DylibsUsed)
     PrintDylibs(MachOOF, false);
   if (DylibId)
     PrintDylibs(MachOOF, true);
   if (SymbolTable)
-    printSymbolTable(*MachOOF, ArchiveName, ArchitectureName);
+    D.printSymbolTable(ArchiveName, ArchitectureName);
   if (UnwindInfo)
     printMachOUnwindInfo(MachOOF);
   if (PrivateHeaders) {
     printMachOFileHeader(MachOOF);
     printMachOLoadCommands(MachOOF);
   }
   if (FirstPrivateHeader)
     printMachOFileHeader(MachOOF);

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

return SectionFilterIterator(Predicate, Object.section_end(),
                                  Object.section_end());
   }
 
 private:
   FilterPredicate Predicate;
   llvm::object::ObjectFile const &Object;
 };
 
+class Dumper {
+  const object::ObjectFile &O;
+  StringSet<> Warnings;
+
+public:
+  Dumper(const object::ObjectFile &O) : O(O) {}
+
+  void reportUniqueWarning(Error Err);
+  void reportUniqueWarning(const Twine &Msg);
+
+  void printSymbolTable(StringRef ArchiveName,
+                        StringRef ArchitectureName = StringRef(),
+                        bool DumpDynamic = false);
+  void printSymbol(const object::SymbolRef &Symbol,
+                   ArrayRef<object::VersionEntry> SymbolVersions,
+                   StringRef FileName, StringRef ArchiveName,
+                   StringRef ArchitectureName, bool DumpDynamic);
+  void printRelocations(const object::ObjectFile *O);
+};
+
 // Various helper functions.
 
 /// Creates a SectionFilter with a standard predicate that conditionally skips
 /// sections when the --section objdump flag is provided.
 ///
 /// Idx is an optional output parameter that keeps track of which section index
 /// this is. This may be different than the actual section number, as some
 /// sections may be filtered (e.g. symbol tables).
 SectionFilter ToolSectionFilter(llvm::object::ObjectFile const &O,
                                 uint64_t *Idx = nullptr);
 
 bool isRelocAddressLess(object::RelocationRef A, object::RelocationRef B);
-void printRelocations(const object::ObjectFile *O);
 void printDynamicRelocations(const object::ObjectFile *O);
 void printSectionHeaders(object::ObjectFile &O);
 void printSectionContents(const object::ObjectFile *O);
-void printSymbolTable(const object::ObjectFile &O, StringRef ArchiveName,
-                      StringRef ArchitectureName = StringRef(),
-                      bool DumpDynamic = false);
-void printSymbol(const object::ObjectFile &O, const object::SymbolRef &Symbol,
-                 ArrayRef<object::VersionEntry> SymbolVersions,
-                 StringRef FileName, StringRef ArchiveName,
-                 StringRef ArchitectureName, bool DumpDynamic);
 [[noreturn]] void reportError(StringRef File, const Twine &Message);
 [[noreturn]] void reportError(Error E, StringRef FileName,
                               StringRef ArchiveName = "",
                               StringRef ArchitectureName = "");
 void reportWarning(const Twine &Message, StringRef File);
 
 template <typename T, typename... Ts>
 T unwrapOrError(Expected<T> EO, Ts &&... Args) {

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

 
 static StringSet<> DisasmSymbolSet;
 StringSet<> objdump::FoundSectionSet;
 static StringRef ToolName;
 
 std::unique_ptr<BuildIDFetcher> BIDFetcher;
 ExitOnError ExitOnErr;
 
+void Dumper::reportUniqueWarning(Error Err) {
+  reportUniqueWarning(toString(std::move(Err)));
+}
+
+void Dumper::reportUniqueWarning(const Twine &Msg) {
+  if (Warnings.insert(StringRef(Msg.str())).second)
+    reportWarning(Msg, O.getFileName());
+}
+
 namespace {
 struct FilterResult {
   // True if the section should not be skipped.
   bool Keep;
 
   // True if the index counter should be incremented, even if the section should
   // be skipped. For example, sections may be skipped if they are not included
   // in the --section flag, but we still want those to count toward the section
if (!IP->applyTargetSpecificCLOption(Opt))
       reportError(Obj->getFileName(),
                   "Unrecognized disassembler option: " + Opt);
 
   disassembleObject(TheTarget, *Obj, *DbgObj, Ctx, DisAsm.get(),
                     SecondaryDisAsm.get(), MIA.get(), IP.get(), STI.get(),
                     SecondarySTI.get(), PIP, SP, InlineRelocs);
 }
 
-void objdump::printRelocations(const ObjectFile *Obj) {
+void Dumper::printRelocations(const ObjectFile *Obj) {
   StringRef Fmt = Obj->getBytesInAddress() > 4 ? "%016" PRIx64 :
                                                  "%08" PRIx64;
 
   // Build a mapping from relocation target to a vector of relocation
   // sections. Usually, there is an only one relocation section for
   // each relocated section.
   MapVector<SectionRef, std::vector<SectionRef>> SecToRelSec;
   uint64_t Ndx;
for (SectionRef Section : P.second) {
       for (const RelocationRef &Reloc : Section.relocations()) {
         uint64_t Address = Reloc.getOffset();
         SmallString<32> RelocName;
         SmallString<32> ValueStr;
         if (Address < StartAddress || Address > StopAddress || getHidden(Reloc))
           continue;
         Reloc.getTypeName(RelocName);
         if (Error E = getRelocationValueString(Reloc, ValueStr))
-          reportError(std::move(E), Obj->getFileName());
+          reportUniqueWarning(std::move(E));
 
         outs() << format(Fmt.data(), Address) << " "
                << left_justify(RelocName, TypePadding) << " " << ValueStr
                << "\n";
       }
     }
   }
 }
for (std::size_t Addr = 0, End = Contents.size(); Addr < End; Addr += 16) {
         else
           outs() << ".";
       }
       outs() << "\n";
     }
   }
 }
 
-void objdump::printSymbolTable(const ObjectFile &O, StringRef ArchiveName,
-                               StringRef ArchitectureName, bool DumpDynamic) {
+void Dumper::printSymbolTable(StringRef ArchiveName, StringRef ArchitectureName,
+                              bool DumpDynamic) {
   if (O.isCOFF() && !DumpDynamic) {
     outs() << "\nSYMBOL TABLE:\n";
     printCOFFSymbolTable(cast<const COFFObjectFile>(O));
     return;
   }
 
   const StringRef FileName = O.getFileName();
 
   if (!DumpDynamic) {
     outs() << "\nSYMBOL TABLE:\n";
     for (auto I = O.symbol_begin(); I != O.symbol_end(); ++I)
-      printSymbol(O, *I, {}, FileName, ArchiveName, ArchitectureName,
-                  DumpDynamic);
+      printSymbol(*I, {}, FileName, ArchiveName, ArchitectureName, DumpDynamic);
     return;
   }
 
   outs() << "\nDYNAMIC SYMBOL TABLE:\n";
   if (!O.isELF()) {
     reportWarning(
         "this operation is not currently supported for this file format",
         FileName);
     return;
   }
 
   const ELFObjectFileBase *ELF = cast<const ELFObjectFileBase>(&O);
   auto Symbols = ELF->getDynamicSymbolIterators();
   Expected<std::vector<VersionEntry>> SymbolVersionsOrErr =
       ELF->readDynsymVersions();
   if (!SymbolVersionsOrErr) {
     reportWarning(toString(SymbolVersionsOrErr.takeError()), FileName);
     SymbolVersionsOrErr = std::vector<VersionEntry>();
     (void)!SymbolVersionsOrErr;
   }
   for (auto &Sym : Symbols)
-    printSymbol(O, Sym, *SymbolVersionsOrErr, FileName, ArchiveName,
+    printSymbol(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) {
+void Dumper::printSymbol(const SymbolRef &Symbol,
   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) {
+    reportUniqueWarning(AddrOrErr.takeError());
+    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
else
     reportWarning("no section overlaps the range [0x" +
                       Twine::utohexstr(Start) + ",0x" + Twine::utohexstr(Stop) +
                       ") specified by --start-address/--stop-address",
                   Obj->getFileName());
 }
 
 static void dumpObject(ObjectFile *O, const Archive *A = nullptr,
                        const Archive::Child *C = nullptr) {
+  Dumper D(*O);
+
   // Avoid other output when using a raw option.
   if (!RawClangAST) {
     outs() << '\n';
     if (A)
       outs() << A->getFileName() << "(" << O->getFileName() << ")";
     else
       outs() << O->getFileName();
     outs() << ":\tfile format " << O->getFileFormatName().lower() << "\n";
   }
 
   if (HasStartAddressFlag || HasStopAddressFlag)
     checkForInvalidStartStopAddress(O, StartAddress, StopAddress);
 
   // Note: the order here matches GNU objdump for compatability.
   StringRef ArchiveName = A ? A->getFileName() : "";
   if (ArchiveHeaders && !MachOOpt && C)
     printArchiveChild(ArchiveName, *C);
   if (FileHeaders)
     printFileHeaders(O);
   if (PrivateHeaders || FirstPrivateHeader)
     printPrivateFileHeaders(O, FirstPrivateHeader);
   if (SectionHeaders)
     printSectionHeaders(*O);
   if (SymbolTable)
-    printSymbolTable(*O, ArchiveName);
+    D.printSymbolTable(ArchiveName);
   if (DynamicSymbolTable)
-    printSymbolTable(*O, ArchiveName, /*ArchitectureName=*/"",
+    D.printSymbolTable(ArchiveName, /*ArchitectureName=*/"",
                        /*DumpDynamic=*/true);
   if (DwarfDumpType != DIDT_Null) {
     std::unique_ptr<DIContext> DICtx = DWARFContext::create(*O);
     // Dump the complete DWARF structure.
     DIDumpOptions DumpOpts;
     DumpOpts.DumpType = DwarfDumpType;
     DICtx->dump(outs(), DumpOpts);
   }
   if (Relocations && !Disassemble)
-    printRelocations(O);
+    D.printRelocations(O);
   if (DynamicRelocations)
     printDynamicRelocations(O);
   if (SectionContents)
     printSectionContents(O);
   if (Disassemble)
     disassembleObject(O, Relocations);
   if (UnwindInfo)
     printUnwindInfo(O);