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

[grimar]

Summary

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.

[jhenderson]

Thanks for this! I think it's a good approach, and reminds me of the approach I took in the DWARFDebugLine parser code.

include/llvm/Object/ELF.h

101

what -> which

104

a error -> an error

test/tools/llvm-readobj/elf-invalid-shstrndx.test

11

It makes me sad that we've lost the file name here. Can we do anything about it, by using an error function that prepends the file name?

tools/llvm-readobj/ELFDumper.cpp

353

as a warnings -> as warnings

398

Should Warnings be private? Also set -> unordered_set.

[grimar]

test/tools/llvm-readobj/elf-invalid-shstrndx.test

11

Yes, I also came to conclusion before going to sleep yesterday, that keeping the file name is very important here.
I think if we dump an archive then absence of the file name in the error message might "deduplicate" them when dumping file A
and prevent showing the same errors for the file B. I though that we either can reset the deduplication string set before starting dumping the new file or ensure we included the file name. And the last one seems to be more handy way.

I think I could just include the file name in the new warnings handler. I'll try.

[grimar]

test/tools/llvm-readobj/elf-invalid-shstrndx.test

11

I think I could just include the file name in the new warnings handler. I'll try.

Ah, I was thinking/talking about the "warning" case in the test case below. Here something different is needed. Anyways I'll take a look.

[grimar]

• Addressed review comments.
• Included a file name to the errors and warnings produced.

[grimar]

tools/llvm-readobj/ELFDumper.cpp

398

I made it private. Making set to be unordered_set was a good idea,
but I can't do that now, when I have a set of pairs.
(It just doesn't compile, the problem is that (taken from stackoverflow):
"std::unordered_set is using std::hash template to compute hashes for its entries and there is no std::hash specialization for pairs.")
So I had to use set, it does not seem to be critical here.

[grimar]

• Fix a missing test.
• A cosmetic change.

[jhenderson]

Is there a test showing that we a) emit a warning only once per input, and b) multiple times, for multiple inputs (both archive members and when listing multiple objects on the command-line)?

tools/llvm-readobj/ELFDumper.cpp

398

You could always implement std::hash for pairs, but I agree that it's not important.

tools/llvm-readobj/llvm-readobj.h

42 ↗

(On Diff #212757)

depricated -> deprecated

Do you plan to remove this function soon? I don't see why you shouldn't (apart from time to write it...!)

What about the ErrorOr version? Should that be updated too?

[grimar]

In D65515#1616514, @jhenderson wrote:

Is there a test showing that we a) emit a warning only once per input, and b) multiple times, for multiple inputs (both archive members and when listing multiple objects on the command-line)?

We have a), it is elf-wrong-shstrtab-type.test, but I do not think we have b) (mostly because I thought it could be a separate patch testing the warnings reported + changing all warnings we have at once),
but I can add a simple initial test for b) here, NP.

tools/llvm-readobj/llvm-readobj.h

42 ↗

(On Diff #212757)

Do you plan to remove this function soon? I don't see why you shouldn't (apart from time to write it...!)

I'd be happy, so yes, I'd do that.

What about the ErrorOr version? Should that be updated too?

May be. (I just did not look deep yet about which functions should be eliminated and which cant't be).
I'll probably add a comment.

[grimar]

• Addressed review comments, simplified a bit.

tools/llvm-readobj/ELFDumper.cpp

398

I found there was no reason to store the file name here, because a new dumper object actually is
created for each input object. So I made it to be unordered_set just like you suggested initially.

tools/llvm-readobj/llvm-readobj.h

42 ↗

(On Diff #212757)

What about the ErrorOr version? Should that be updated too?

May be. (I just did not look deep yet about which functions should be eliminated and which cant't be).
I'll probably add a comment.

So I think it should be updated, but not in this patch. I can't just add a comment about deprication,
because we do not have a version with StringRef Input yet. (And it is not needed for this patch).

[jhenderson]

test/tools/llvm-readobj/elf-wrong-shstrtab-type.test

4

I'd rephrase:

Check we report only one warning for the issue for each input object.

18

a one -> one

27–29

Do we really need these lines for this and the following test cases? We already show that we can dump the section as well as warn above.

52

in -> on

[grimar]

test/tools/llvm-readobj/elf-wrong-shstrtab-type.test

27–29

I tried to show how we dump archives with warnings. A test case below only check the messages reported,
but here I left the full output. I think this might be worth testing. It checks that the custom warning handler
reports messages in time, for example.

I am OK to remove it if you feel it is excessive here though.

[jhenderson]

test/tools/llvm-readobj/elf-wrong-shstrtab-type.test

27–29

I think it's excessive here. The timing is shown above.

[grimar]

• Addressed review comments.

[jhenderson]

LGTM.

include/llvm/Object/ELF.h

std::string getSecIndexForError(const ELFFile<ELFT> *Obj,
   // To make this helper be more convenient for error reporting purposes we
   // drop the error. But really it should never be triggered. Before this point,
   // our code should have called 'sections()' and reported a proper error on
   // failure.
   llvm::consumeError(TableOrErr.takeError());
   return "[unknown index]";
 }
 
+static Error defaultWarningHandler(const Twine &Msg) { return createError(Msg); }
+
 template <class ELFT>
 class ELFFile {
 public:
   LLVM_ELF_IMPORT_TYPES_ELFT(ELFT)
   using uintX_t = typename ELFT::uint;
   using Elf_Ehdr = typename ELFT::Ehdr;
   using Elf_Shdr = typename ELFT::Shdr;
   using Elf_Sym = typename ELFT::Sym;
public:
   using Elf_Dyn_Range = typename ELFT::DynRange;
   using Elf_Shdr_Range = typename ELFT::ShdrRange;
   using Elf_Sym_Range = typename ELFT::SymRange;
   using Elf_Rel_Range = typename ELFT::RelRange;
   using Elf_Rela_Range = typename ELFT::RelaRange;
   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), what is
+  // useful for dumpers, like llvm-readobj.
+  // It accepts a warning message string and returns a success
+  // when the warning should be ignored or a 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(); }
 
 private:
   StringRef Buf;
 
   ELFFile(StringRef Object);
 
 public:
   const Elf_Ehdr *getHeader() const {
     return reinterpret_cast<const Elf_Ehdr *>(base());
   }
 
   template <typename T>
   Expected<const T *> getEntry(uint32_t Section, uint32_t Entry) const;
   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;
 
   Expected<ArrayRef<Elf_Word>> getSHNDXTable(const Elf_Shdr &Section) const;
   Expected<ArrayRef<Elf_Word>> getSHNDXTable(const Elf_Shdr &Section,
                                              Elf_Shdr_Range Sections) const;
 
public:
   /// \param Shdr the section to iterate over.
   /// \param Err [out] an error to support fallible iteration, which should
   ///  be checked after iteration ends.
   iterator_range<Elf_Note_Iterator> notes(const Elf_Shdr &Shdr,
                                           Error &Err) const {
     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,
                                         const Elf_Shdr *SymTab,
                                         ArrayRef<Elf_Word> ShndxTable) const;
   Expected<const Elf_Shdr *> getSection(const Elf_Sym *Sym,
                                         Elf_Sym_Range Symtab,
                                         ArrayRef<Elf_Word> ShndxTable) const;
   Expected<const Elf_Shdr *> getSection(uint32_t Index) const;
   Expected<const Elf_Shdr *> getSection(const StringRef SectionName) const;
 
   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>
   Expected<ArrayRef<T>> getSectionContentsAsArray(const Elf_Shdr *Sec) const;
   Expected<ArrayRef<uint8_t>> getSectionContents(const Elf_Shdr *Sec) const;
 };
 
 using ELF32LEFile = ELFFile<ELF32LE>;
ELFFile<ELFT>::getRelocationSymbol(const Elf_Rel *Rel,
   uint32_t Index = Rel->getSymbol(isMips64EL());
   if (Index == 0)
     return nullptr;
   return getEntry<Elf_Sym>(SymTab, Index);
 }
 
 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;
 
   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())
     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) {}
 
 template <class ELFT>
 Expected<ELFFile<ELFT>> ELFFile<ELFT>::create(StringRef Object) {
   if (sizeof(Elf_Ehdr) > Object.size())
     return createError("invalid buffer: the size (" + Twine(Object.size()) +
if (*SecNameOrErr == SectionName)
       return &Sec;
   }
   // TODO: this error is untested.
   return createError("invalid section name");
 }
 
 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 " +
+    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));
+                              object::getELFSectionTypeName(
+                                  getHeader()->e_machine, Section->sh_type)))
+      return std::move(E);
+
   auto V = getSectionContentsAsArray<char>(Section);
   if (!V)
     return V.takeError();
   ArrayRef<char> Data = *V;
   if (Data.empty())
     return createError("SHT_STRTAB string table section " +
                        getSecIndexForError(this, Section) + " is empty");
   if (Data.back() != '\0')
ELFFile<ELFT>::getStringTableForSymtab(const Elf_Shdr &Sec,
   auto SectionOrErr = object::getSection<ELFT>(Sections, Sec.sh_link);
   if (!SectionOrErr)
     return SectionOrErr.takeError();
   return getStringTable(*SectionOrErr);
 }
 
 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);
 }
 
 template <class ELFT>
 Expected<StringRef> ELFFile<ELFT>::getSectionName(const Elf_Shdr *Section,
                                                   StringRef DotShstrtab) const {

test/tools/llvm-readobj/elf-invalid-shstrndx.test

 # RUN: yaml2obj %s -o %t
-# RUN: not llvm-readelf --headers -S 2>&1 %t | FileCheck %s -DFILE=%t --check-prefix=GNU
-# RUN: not llvm-readobj --headers -S 2>&1 %t | FileCheck %s -DFILE=%t --check-prefix=LLVM
+# RUN: not llvm-readelf --headers -S 2>&1 %t | FileCheck %s --check-prefix=GNU
+# RUN: not llvm-readobj --headers -S 2>&1 %t | FileCheck %s --check-prefix=LLVM
 
 # GNU:      ELF Header:
 # GNU:        Section header string table index: 255
 # GNU-NEXT:   There are 4 section headers, starting at offset 0x40:
 # GNU:      Section Headers:
 # GNU-NEXT:  [Nr] Name
 # GNU-EMPTY:
-# GNU-NEXT:  error: '[[FILE]]': section header string table index 255 does not exist
+# GNU-NEXT:  error: section header string table index 255 does not exist
 
 # LLVM:      ElfHeader {
 # LLVM:        StringTableSectionIndex: 255
 # LLVM-NEXT: }
 # LLVM-NEXT: Sections [
 # LLVM-EMPTY:
-# LLVM-NEXT: error: '[[FILE]]': section header string table index 255 does not exist
+# LLVM-NEXT: error: section header string table index 255 does not exist
 
 --- !ELF
 FileHeader:
   Class:     ELFCLASS64
   Data:      ELFDATA2LSB
   Type:      ET_REL
   Machine:   EM_X86_64
   SHStrNdx:  0xFF

test/tools/llvm-readobj/elf-wrong-shstrtab-type.test

 ## Check we do not fail to dump the section headers when
 ## a .shstrtab section does not have a SHT_STRTAB type.
 
 # 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 --implicit-check-not warning --check-prefix LLVM
+# RUN: llvm-readelf -S %t1 2>&1 | FileCheck %s --implicit-check-not warning --check-prefix GNU
 
-# LLVM:      Name: .shstrtab
-# LLVM-NEXT: Type: SHT_PROGBITS
+# LLVM: warning: 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: [Nr] Name      Type
-# GNU: [ 1] .shstrtab PROGBITS
+# GNU: Section Headers:
+# GNU:   [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
+# GNU: warning: 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
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_DYN
   Machine: EM_X86_64
 Sections:
   - Name: .shstrtab
     Type: SHT_PROGBITS

tools/llvm-readobj/ELFDumper.cpp

 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <cinttypes>
 #include <cstddef>
 #include <cstdint>
 #include <cstdlib>
 #include <iterator>
 #include <memory>
+#include <set>
 #include <string>
 #include <system_error>
 #include <vector>
 
 using namespace llvm;
 using namespace llvm::object;
 using namespace ELF;
 

 
 template <class ELFT> class MipsGOTParser;
 
 template <typename ELFT> class DumpStyle {
 public:
   using Elf_Shdr = typename ELFT::Shdr;
   using Elf_Sym = typename ELFT::Sym;
 
-  DumpStyle(ELFDumper<ELFT> *Dumper) : Dumper(Dumper) {}
+  DumpStyle(ELFDumper<ELFT> *Dumper) : Dumper(Dumper) {
+    // Dumper reports all non-critical errors as a warnings.
+    // It does not print the same warning more than once.
+    WarningHandler = [this](const Twine &Msg) {
+      if (Warnings.insert(Msg.str()).second)
+        reportWarning(Msg);
+      return Error::success();
+    };
+  }
+
   virtual ~DumpStyle() = default;
 
   virtual void printFileHeaders(const ELFFile<ELFT> *Obj) = 0;
   virtual void printGroupSections(const ELFFile<ELFT> *Obj) = 0;
   virtual void printRelocations(const ELFFile<ELFT> *Obj) = 0;
   virtual void printSectionHeaders(const ELFFile<ELFT> *Obj) = 0;
   virtual void printSymbols(const ELFFile<ELFT> *Obj, bool PrintSymbols,
                             bool PrintDynamicSymbols) = 0;
public:
   virtual void printCGProfile(const ELFFile<ELFT> *Obj) = 0;
   virtual void printAddrsig(const ELFFile<ELFT> *Obj) = 0;
   virtual void printNotes(const ELFFile<ELFT> *Obj) = 0;
   virtual void printELFLinkerOptions(const ELFFile<ELFT> *Obj) = 0;
   virtual void printMipsGOT(const MipsGOTParser<ELFT> &Parser) = 0;
   virtual void printMipsPLT(const MipsGOTParser<ELFT> &Parser) = 0;
   const ELFDumper<ELFT> *dumper() const { return Dumper; }
 
+protected:
+  std::function<Error(const Twine &Msg)> WarningHandler;
+  std::set<std::string> Warnings;
+
 private:
   const ELFDumper<ELFT> *Dumper;
 };
 
 template <typename ELFT> class GNUStyle : public DumpStyle<ELFT> {
   formatted_raw_ostream &OS;
 
 public:
case SHT_GNU_versym:
     return "VERSYM";
   default:
     return getSectionTypeOffsetString(Type);
   }
   return "";
 }
 
 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());
   OS << "There are " << to_string(Sections.size())
      << " section headers, starting at offset "
      << "0x" << to_hexString(Obj->getHeader()->e_shoff, false) << ":\n\n";
   OS << "Section Headers:\n";
   Field Fields[11] = {
       {"[Nr]", 2},        {"Name", 7},        {"Type", 25},
       {"Address", 41},    {"Off", 58 - Bias}, {"Size", 65 - Bias},
       {"ES", 72 - Bias},  {"Flg", 75 - Bias}, {"Lk", 79 - Bias},
       {"Inf", 82 - Bias}, {"Al", 86 - Bias}};
   for (auto &F : Fields)
     printField(F);
   OS << "\n";
 
-  const ELFObjectFile<ELFT> *ElfObj = this->dumper()->getElfObject();
   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(Obj->getSectionName(&Sec, this->WarningHandler));
     Fields[2].Str =
         getSectionTypeString(Obj->getHeader()->e_machine, Sec.sh_type);
     Fields[3].Str =
         to_string(format_hex_no_prefix(Sec.sh_addr, ELFT::Is64Bits ? 16 : 8));
     Fields[4].Str = to_string(format_hex_no_prefix(Sec.sh_offset, 6));
     Fields[5].Str = to_string(format_hex_no_prefix(Sec.sh_size, 6));
     Fields[6].Str = to_string(format_hex_no_prefix(Sec.sh_entsize, 2));
     Fields[7].Str = getGNUFlags(Sec.sh_flags);

 }
 
 template <class ELFT>
 void LLVMStyle<ELFT>::printSectionHeaders(const ELFO *Obj) {
   ListScope SectionsD(W, "Sections");
 
   int SectionIndex = -1;
   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(Obj->getSectionName(&Sec, this->WarningHandler));
     DictScope SectionD(W, "Section");
     W.printNumber("Index", ++SectionIndex);
     W.printNumber("Name", Name, Sec.sh_name);
     W.printHex(
         "Type",
         object::getELFSectionTypeName(Obj->getHeader()->e_machine, Sec.sh_type),
         Sec.sh_type);
     std::vector<EnumEntry<unsigned>> SectionFlags(std::begin(ElfSectionFlags),