[llvm-readobj/libObject] - Introduce a custom warning handler for `ELFFile<ELFT>` methods.

Currently, we have a code duplication in llvm-readobj which was introduced in D63266.
The duplication was introduced to allow llvm-readobj to dump the partially
broken object. Methods in ELFFile<ELFT> perform a strict validation of the inputs,
what is itself good, but not for dumper tools, that might want to dump the information,
even if some pieces are broken/unexpected.

This patch introduces a warning handler which can be passed to ELFFile<ELFT> methods
and can allow skipping the non-critical errors when needed/possible.

For demonstration, I removed the duplication from llvm-readobj and implemented a warning using
the new custom warning handler. It also deduplicates the strings printed, making the output less verbose.

Differential revision: https://reviews.llvm.org/D65515

llvm-svn: 368260

6 files changed, 100 insertions, 44 deletions

diff --git a/llvm/include/llvm/Object/ELF.h b/llvm/include/llvm/Object/ELF.h
index 51216bb6929..8d046e54dfd 100644
--- a/llvm/include/llvm/Object/ELF.h
+++ b/llvm/include/llvm/Object/ELF.h
@@ -64,6 +64,8 @@ std::string getSecIndexForError(const ELFFile<ELFT> *Obj,
   return "[unknown index]";
 }
 
+static Error defaultWarningHandler(const Twine &Msg) { return createError(Msg); }
+
 template <class ELFT>
 class ELFFile {
 public:
@@ -95,6 +97,13 @@ public:
   using Elf_Relr_Range = typename ELFT::RelrRange;
   using Elf_Phdr_Range = typename ELFT::PhdrRange;
 
+  // This is a callback that can be passed to a number of functions.
+  // It can be used to ignore non-critical errors (warnings), which is
+  // useful for dumpers, like llvm-readobj.
+  // It accepts a warning message string and returns a success
+  // when the warning should be ignored or an error otherwise.
+  using WarningHandler = llvm::function_ref<Error(const Twine &Msg)>;
+
   const uint8_t *base() const { return Buf.bytes_begin(); }
 
   size_t getBufSize() const { return Buf.size(); }
@@ -114,7 +123,9 @@ public:
   template <typename T>
   Expected<const T *> getEntry(const Elf_Shdr *Section, uint32_t Entry) const;
 
-  Expected<StringRef> getStringTable(const Elf_Shdr *Section) const;
+  Expected<StringRef>
+  getStringTable(const Elf_Shdr *Section,
+                 WarningHandler WarnHandler = &defaultWarningHandler) const;
   Expected<StringRef> getStringTableForSymtab(const Elf_Shdr &Section) const;
   Expected<StringRef> getStringTableForSymtab(const Elf_Shdr &Section,
                                               Elf_Shdr_Range Sections) const;
@@ -261,7 +272,9 @@ public:
     return make_range(notes_begin(Shdr, Err), notes_end());
   }
 
-  Expected<StringRef> getSectionStringTable(Elf_Shdr_Range Sections) const;
+  Expected<StringRef> getSectionStringTable(
+      Elf_Shdr_Range Sections,
+      WarningHandler WarnHandler = &defaultWarningHandler) const;
   Expected<uint32_t> getSectionIndex(const Elf_Sym *Sym, Elf_Sym_Range Syms,
                                      ArrayRef<Elf_Word> ShndxTable) const;
   Expected<const Elf_Shdr *> getSection(const Elf_Sym *Sym,
@@ -275,7 +288,9 @@ public:
   Expected<const Elf_Sym *> getSymbol(const Elf_Shdr *Sec,
                                       uint32_t Index) const;
 
-  Expected<StringRef> getSectionName(const Elf_Shdr *Section) const;
+  Expected<StringRef>
+  getSectionName(const Elf_Shdr *Section,
+                 WarningHandler WarnHandler = &defaultWarningHandler) const;
   Expected<StringRef> getSectionName(const Elf_Shdr *Section,
                                      StringRef DotShstrtab) const;
   template <typename T>
@@ -458,7 +473,8 @@ ELFFile<ELFT>::getRelocationSymbol(const Elf_Rel *Rel,
 
 template <class ELFT>
 Expected<StringRef>
-ELFFile<ELFT>::getSectionStringTable(Elf_Shdr_Range Sections) const {
+ELFFile<ELFT>::getSectionStringTable(Elf_Shdr_Range Sections,
+                                     WarningHandler WarnHandler) const {
   uint32_t Index = getHeader()->e_shstrndx;
   if (Index == ELF::SHN_XINDEX)
     Index = Sections[0].sh_link;
@@ -468,7 +484,7 @@ ELFFile<ELFT>::getSectionStringTable(Elf_Shdr_Range Sections) const {
   if (Index >= Sections.size())
     return createError("section header string table index " + Twine(Index) +
                        " does not exist");
-  return getStringTable(&Sections[Index]);
+  return getStringTable(&Sections[Index], WarnHandler);
 }
 
 template <class ELFT> ELFFile<ELFT>::ELFFile(StringRef Object) : Buf(Object) {}
@@ -569,13 +585,16 @@ ELFFile<ELFT>::getSection(uint32_t Index) const {
 
 template <class ELFT>
 Expected<StringRef>
-ELFFile<ELFT>::getStringTable(const Elf_Shdr *Section) const {
+ELFFile<ELFT>::getStringTable(const Elf_Shdr *Section,
+                              WarningHandler WarnHandler) const {
   if (Section->sh_type != ELF::SHT_STRTAB)
-    return createError("invalid sh_type for string table section " +
-                       getSecIndexForError(this, Section) +
-                       ": expected SHT_STRTAB, but got " +
-                       object::getELFSectionTypeName(getHeader()->e_machine,
-                                                     Section->sh_type));
+    if (Error E = WarnHandler("invalid sh_type for string table section " +
+                              getSecIndexForError(this, Section) +
+                              ": expected SHT_STRTAB, but got " +
+                              object::getELFSectionTypeName(
+                                  getHeader()->e_machine, Section->sh_type)))
+      return std::move(E);
+
   auto V = getSectionContentsAsArray<char>(Section);
   if (!V)
     return V.takeError();
@@ -650,11 +669,12 @@ ELFFile<ELFT>::getStringTableForSymtab(const Elf_Shdr &Sec,
 
 template <class ELFT>
 Expected<StringRef>
-ELFFile<ELFT>::getSectionName(const Elf_Shdr *Section) const {
+ELFFile<ELFT>::getSectionName(const Elf_Shdr *Section,
+                              WarningHandler WarnHandler) const {
   auto SectionsOrErr = sections();
   if (!SectionsOrErr)
     return SectionsOrErr.takeError();
-  auto Table = getSectionStringTable(*SectionsOrErr);
+  auto Table = getSectionStringTable(*SectionsOrErr, WarnHandler);
   if (!Table)
     return Table.takeError();
   return getSectionName(Section, *Table);
diff --git a/llvm/test/Object/invalid.test b/llvm/test/Object/invalid.test
index 9086b87dda9..05f38e9d007 100644
--- a/llvm/test/Object/invalid.test
+++ b/llvm/test/Object/invalid.test
@@ -344,9 +344,9 @@ Sections:
 ## overflows the section name string table.
 
 # RUN: yaml2obj %s --docnum=17 -o %t17
-# RUN: not llvm-readobj --sections %t17 2>&1 | FileCheck --check-prefix=BROKEN-SECNAME %s
+# RUN: not llvm-readobj --sections %t17 2>&1 | FileCheck -DFILE=%t17 --check-prefix=BROKEN-SECNAME %s
 
-## BROKEN-SECNAME: error: a section [index 1] has an invalid sh_name (0x1) offset which goes past the end of the section name string table
+## BROKEN-SECNAME: error: '[[FILE]]': a section [index 1] has an invalid sh_name (0x1) offset which goes past the end of the section name string table
 
 --- !ELF
 FileHeader:
diff --git a/llvm/test/tools/llvm-readobj/elf-wrong-shstrtab-type.test b/llvm/test/tools/llvm-readobj/elf-wrong-shstrtab-type.test
index ce7fec67c0b..24e6d3c1959 100644
--- a/llvm/test/tools/llvm-readobj/elf-wrong-shstrtab-type.test
+++ b/llvm/test/tools/llvm-readobj/elf-wrong-shstrtab-type.test
@@ -1,15 +1,38 @@
 ## Check we do not fail to dump the section headers when
 ## a .shstrtab section does not have a SHT_STRTAB type.
 
+## Check we report only one warning for the issue for each input object.
+
 # RUN: yaml2obj %s -o %t1
-# RUN: llvm-readobj -S %t1 | FileCheck %s --check-prefix LLVM
-# RUN: llvm-readelf -S %t1 | FileCheck %s --check-prefix GNU
+# RUN: llvm-readobj -S %t1 2>&1 | FileCheck %s -DFILE=%t1 --implicit-check-not warning --check-prefix LLVM
+# RUN: llvm-readelf -S %t1 2>&1 | FileCheck %s -DFILE=%t1 --implicit-check-not warning --check-prefix GNU
+
+# LLVM: warning: '[[FILE]]': invalid sh_type for string table section [index 1]: expected SHT_STRTAB, but got SHT_PROGBITS
+# LLVM:  Section {
+# LLVM:    Name: .shstrtab
+# LLVM:    Type: SHT_PROGBITS
+
+# GNU: Section Headers:
+# GNU:   [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
+# GNU: warning: '[[FILE]]': invalid sh_type for string table section [index 1]: expected SHT_STRTAB, but got SHT_PROGBITS
+# GNU:   [ 1] .shstrtab         PROGBITS        0000000000000000 000140 00001b 00 0   0  0
+
+## Test we report multiple identical warnings (one for each object) when dumping an archive.
+
+# RUN: rm -f %t.a
+# RUN: cp %t1 %t2
+# RUN: llvm-ar rc %t.a %t1 %t2 %t1
+# RUN: llvm-readobj -S %t.a 2>&1 | FileCheck %s --implicit-check-not warning --check-prefix WARNINGS
+# RUN: llvm-readelf -S %t.a 2>&1 | FileCheck %s --implicit-check-not warning --check-prefix WARNINGS
+
+# WARNINGS: warning: '{{.*}}1': invalid sh_type for string table section [index 1]: expected SHT_STRTAB, but got SHT_PROGBITS
+# WARNINGS: warning: '{{.*}}2': invalid sh_type for string table section [index 1]: expected SHT_STRTAB, but got SHT_PROGBITS
+# WARNINGS: warning: '{{.*}}1': invalid sh_type for string table section [index 1]: expected SHT_STRTAB, but got SHT_PROGBITS
 
-# LLVM:      Name: .shstrtab
-# LLVM-NEXT: Type: SHT_PROGBITS
+## Test we report the warning for each input file specified on the command line.
 
-# GNU: [Nr] Name      Type
-# GNU: [ 1] .shstrtab PROGBITS
+# RUN: llvm-readobj -S %t1 %t2 %t1 2>&1 | FileCheck %s --implicit-check-not warning --check-prefix WARNINGS
+# RUN: llvm-readelf -S %t1 %t2 %t1 2>&1 | FileCheck %s --implicit-check-not warning --check-prefix WARNINGS
 
 --- !ELF
 FileHeader:
diff --git a/llvm/tools/llvm-readobj/ELFDumper.cpp b/llvm/tools/llvm-readobj/ELFDumper.cpp
index 0f33ecd58cd..aa6bb41088a 100644
--- a/llvm/tools/llvm-readobj/ELFDumper.cpp
+++ b/llvm/tools/llvm-readobj/ELFDumper.cpp
@@ -63,6 +63,7 @@
 #include <memory>
 #include <string>
 #include <system_error>
+#include <unordered_set>
 #include <vector>
 
 using namespace llvm;
@@ -352,7 +353,17 @@ public:
   using Elf_Sym = typename ELFT::Sym;
   using Elf_Addr = typename ELFT::Addr;
 
-  DumpStyle(ELFDumper<ELFT> *Dumper) : Dumper(Dumper) {}
+  DumpStyle(ELFDumper<ELFT> *Dumper) : Dumper(Dumper) {
+    // Dumper reports all non-critical errors as warnings.
+    // It does not print the same warning more than once.
+    WarningHandler = [this](const Twine &Msg) {
+      StringRef FileName = this->Dumper->getElfObject()->getFileName();
+      if (Warnings.insert(Msg.str()).second)
+        reportWarning(FileName, createError(Msg));
+      return Error::success();
+    };
+  }
+
   virtual ~DumpStyle() = default;
 
   virtual void printFileHeaders(const ELFFile<ELFT> *Obj) = 0;
@@ -401,7 +412,11 @@ public:
   virtual void printMipsPLT(const MipsGOTParser<ELFT> &Parser) = 0;
   const ELFDumper<ELFT> *dumper() const { return Dumper; }
 
+protected:
+  std::function<Error(const Twine &Msg)> WarningHandler;
+
 private:
+  std::unordered_set<std::string> Warnings;
   const ELFDumper<ELFT> *Dumper;
 };
 
@@ -3032,26 +3047,6 @@ static std::string getSectionTypeString(unsigned Arch, unsigned Type) {
 }
 
 template <class ELFT>
-static StringRef getSectionName(const typename ELFT::Shdr &Sec,
-                                const ELFObjectFile<ELFT> &ElfObj,
-                                ArrayRef<typename ELFT::Shdr> Sections) {
-  const ELFFile<ELFT> &Obj = *ElfObj.getELFFile();
-  uint32_t Index = Obj.getHeader()->e_shstrndx;
-  if (Index == ELF::SHN_XINDEX)
-    Index = Sections[0].sh_link;
-  if (!Index) // no section string table.
-    return "";
-  // TODO: Test a case when the sh_link of the section with index 0 is broken.
-  if (Index >= Sections.size())
-    reportError(ElfObj.getFileName(),
-                createError("section header string table index " +
-                            Twine(Index) + " does not exist"));
-  StringRef Data = toStringRef(unwrapOrError(
-      Obj.template getSectionContentsAsArray<uint8_t>(&Sections[Index])));
-  return unwrapOrError(Obj.getSectionName(&Sec, Data));
-}
-
-template <class ELFT>
 void GNUStyle<ELFT>::printSectionHeaders(const ELFO *Obj) {
   unsigned Bias = ELFT::Is64Bits ? 0 : 8;
   ArrayRef<Elf_Shdr> Sections = unwrapOrError(Obj->sections());
@@ -3072,7 +3067,8 @@ void GNUStyle<ELFT>::printSectionHeaders(const ELFO *Obj) {
   size_t SectionIndex = 0;
   for (const Elf_Shdr &Sec : Sections) {
     Fields[0].Str = to_string(SectionIndex);
-    Fields[1].Str = getSectionName(Sec, *ElfObj, Sections);
+    Fields[1].Str = unwrapOrError<StringRef>(
+        ElfObj->getFileName(), Obj->getSectionName(&Sec, this->WarningHandler));
     Fields[2].Str =
         getSectionTypeString(Obj->getHeader()->e_machine, Sec.sh_type);
     Fields[3].Str =
@@ -4984,7 +4980,8 @@ void LLVMStyle<ELFT>::printSectionHeaders(const ELFO *Obj) {
   ArrayRef<Elf_Shdr> Sections = unwrapOrError(Obj->sections());
   const ELFObjectFile<ELFT> *ElfObj = this->dumper()->getElfObject();
   for (const Elf_Shdr &Sec : Sections) {
-    StringRef Name = getSectionName(Sec, *ElfObj, Sections);
+    StringRef Name = unwrapOrError(
+        ElfObj->getFileName(), Obj->getSectionName(&Sec, this->WarningHandler));
     DictScope SectionD(W, "Section");
     W.printNumber("Index", ++SectionIndex);
     W.printNumber("Name", Name, Sec.sh_name);
diff --git a/llvm/tools/llvm-readobj/llvm-readobj.cpp b/llvm/tools/llvm-readobj/llvm-readobj.cpp
index bb629c28cf1..f607490e8f8 100644
--- a/llvm/tools/llvm-readobj/llvm-readobj.cpp
+++ b/llvm/tools/llvm-readobj/llvm-readobj.cpp
@@ -394,6 +394,12 @@ void reportWarning(Twine Msg) {
   WithColor::warning(errs()) << Msg << "\n";
 }
 
+void reportWarning(StringRef Input, Error Err) {
+  if (Input == "-")
+    Input = "<stdin>";
+  warn(createFileError(Input, std::move(Err)));
+}
+
 void warn(Error Err) {
   handleAllErrors(std::move(Err), [&](const ErrorInfoBase &EI) {
     reportWarning(EI.message());
diff --git a/llvm/tools/llvm-readobj/llvm-readobj.h b/llvm/tools/llvm-readobj/llvm-readobj.h
index 0e02da4cb84..7ca1dfbec17 100644
--- a/llvm/tools/llvm-readobj/llvm-readobj.h
+++ b/llvm/tools/llvm-readobj/llvm-readobj.h
@@ -24,6 +24,7 @@ namespace llvm {
   LLVM_ATTRIBUTE_NORETURN void reportError(Twine Msg);
   void reportError(StringRef Input, Error Err); 
   void reportWarning(Twine Msg);
+  void reportWarning(StringRef Input, Error Err);
   void warn(llvm::Error Err);
   void error(std::error_code EC);
   void error(llvm::Error EC);
@@ -37,6 +38,8 @@ namespace llvm {
       return *EO;
     reportError(EO.getError().message());
   }
+
+  // TODO: This one is deprecated. Use one with a Input name below.
   template <class T> T unwrapOrError(Expected<T> EO) {
     if (EO)
       return *EO;
@@ -46,6 +49,13 @@ namespace llvm {
     OS.flush();
     reportError(Buf);
   }
+
+  template <class T> T unwrapOrError(StringRef Input, Expected<T> EO) {
+    if (EO)
+      return *EO;
+    reportError(Input, EO.takeError());
+    llvm_unreachable("reportError shouldn't return in this case");
+  }
 } // namespace llvm
 
 namespace opts {