[llvm-readelf/obj] - Improve error reporting when dumping group sections.

[grimar]

Summary

Our code that dumps groups has 3 noticeable issues:

1. It uses unwrapOrError in many places.
2. It doesn't allow reporting unique warnings, because the getGroups helper is not a member of DumpStyle<ELFT>.
3. It might just crash. See the comment for StrTableOrErr->data() + Sym.st_name line.

In this patch I am starting addressing these points.
For start I've converted one of unwrapOrError calls to a unique warning.

[MaskRay]

Looks great!

llvm/tools/llvm-readobj/ELFDumper.cpp

173

Place in an anonymous namespace

[grimar]

llvm/tools/llvm-readobj/ELFDumper.cpp

173

It is already here. See line 113.

[jhenderson]

LGTM!

llvm/test/tools/llvm-readobj/ELF/groups.test

- Name: .rela.text.foo
     Link: .symtab
     Info: .text.foo
   - Name: .text.bar
     Type: SHT_PROGBITS
   - Name: .rela.text.bar
     Type: SHT_RELA
     Link: .symtab
     Info: .text.bar
+  - Name: .symtab
+    Type: SHT_SYMTAB
+    Link: [[SYMTABLINK=.strtab]]
 Symbols:
   - Name:    foo
     Section: .text.foo
   - Name:    bar
     Section: .text.bar
 
 ## Check that we report a warning and continue dumping when a section is included
 ## in two group sections at the same time.

 # DUP-GNU-NEXT:  [    3]   .text.foo
 # DUP-GNU-NEXT:  [    4]   .rela.text.foo
 
 # DUP-GNU:      COMDAT group section [    2] `.group1' [bar] contains 2 sections:
 # DUP-GNU-NEXT:    [Index]    Name
 # DUP-GNU-NEXT: warning: '[[FILE]]': section with index 3, included in the group section with index 1, was also found in the group section with index 2
 # DUP-GNU-NEXT:    [    3]   .text.foo
 # DUP-GNU-NEXT:    [    6]   .rela.text.bar
+
+# RUN: yaml2obj %s -DSYMTABLINK=0xFF -o %t.symtab.o
+# RUN: llvm-readobj --elf-section-groups %t.symtab.o 2>&1 | \
+# RUN:   FileCheck -DFILE=%t.symtab.o %s --check-prefix=SYMTAB-LLVM --implicit-check-not=warning:
+# RUN: llvm-readelf --elf-section-groups %t.symtab.o 2>&1 | \
+# RUN:   FileCheck -DFILE=%t.symtab.o %s --check-prefix=SYMTAB-GNU --implicit-check-not=warning:
+
+# SYMTAB-LLVM:      Groups {
+# SYMTAB-LLVM-NEXT: warning: '[[FILE]]': unable to get the string table for SHT_SYMTAB section with index 7: invalid section index: 255
+# SYMTAB-LLVM-NEXT:   Group {
+# SYMTAB-LLVM-NEXT:     Name: .group (16)
+# SYMTAB-LLVM-NEXT:     Index: 1
+# SYMTAB-LLVM-NEXT:     Link: 7
+# SYMTAB-LLVM-NEXT:     Info: 1
+# SYMTAB-LLVM-NEXT:     Type: COMDAT (0x1)
+# SYMTAB-LLVM-NEXT:     Signature: <?>
+# SYMTAB-LLVM-NEXT:     Section(s) in group [
+# SYMTAB-LLVM-NEXT:       .text.foo (3)
+# SYMTAB-LLVM-NEXT:       .rela.text.foo (4)
+# SYMTAB-LLVM-NEXT:     ]
+# SYMTAB-LLVM-NEXT:   }
+# SYMTAB-LLVM-NEXT:   Group {
+# SYMTAB-LLVM-NEXT:     Name: .group1 (64)
+# SYMTAB-LLVM-NEXT:     Index: 2
+# SYMTAB-LLVM-NEXT:     Link: 7
+# SYMTAB-LLVM-NEXT:     Info: 2
+# SYMTAB-LLVM-NEXT:     Type: COMDAT (0x1)
+# SYMTAB-LLVM-NEXT:     Signature: <?>
+# SYMTAB-LLVM-NEXT:     Section(s) in group [
+# SYMTAB-LLVM-NEXT:       .text.bar (5)
+# SYMTAB-LLVM-NEXT:       .rela.text.bar (6)
+# SYMTAB-LLVM-NEXT:     ]
+# SYMTAB-LLVM-NEXT:   }
+# SYMTAB-LLVM-NEXT: }
+
+# SYMTAB-GNU:        warning: '[[FILE]]': unable to get the string table for SHT_SYMTAB section with index 7: invalid section index: 255
+# SYMTAB-GNU-EMPTY:
+# SYMTAB-GNU-NEXT:   COMDAT group section [    1] `.group' [<?>] contains 2 sections:
+# SYMTAB-GNU-NEXT:      [Index]    Name
+# SYMTAB-GNU-NEXT:      [    3]   .text.foo
+# SYMTAB-GNU-NEXT:      [    4]   .rela.text.foo
+# SYMTAB-GNU-EMPTY:
+# SYMTAB-GNU-NEXT:   COMDAT group section [    2] `.group1' [<?>] contains 2 sections:
+# SYMTAB-GNU-NEXT:      [Index]    Name
+# SYMTAB-GNU-NEXT:      [    5]   .text.bar
+# SYMTAB-GNU-NEXT:      [    6]   .rela.text.bar

llvm/tools/llvm-readobj/ELFDumper.cpp

if (!EntSizePrintName.empty())
           (" or " + EntSizePrintName + " (0x" + Twine::utohexstr(EntSize) + ")")
               .str();
 
     reportWarning(createError(Msg.c_str()), FileName);
     return {Start, Start};
   }
 };
 
+struct GroupMember {
+  StringRef Name;
+  uint64_t Index;
+};
+
+struct GroupSection {
+  StringRef Name;
+  std::string Signature;
+  uint64_t ShName;
+  uint64_t Index;
+  uint32_t Link;
+  uint32_t Info;
+  uint32_t Type;
+  std::vector<GroupMember> Members;
+};
+
 namespace {
 struct VerdAux {
   unsigned Offset;
   std::string Name;
 };
 
 struct VerDef {
   unsigned Offset;
void printStackSize(RelocationRef Rel, const Elf_Shdr *FunctionSec,
                       const RelocationResolver &Resolver, DataExtractor Data);
   virtual void printStackSizeEntry(uint64_t Size, StringRef FuncName) = 0;
   virtual void printMipsGOT(const MipsGOTParser<ELFT> &Parser) = 0;
   virtual void printMipsPLT(const MipsGOTParser<ELFT> &Parser) = 0;
   virtual void printMipsABIFlags() = 0;
   const ELFDumper<ELFT> &dumper() const { return Dumper; }
 
 protected:
+  std::vector<GroupSection> getGroups();
+
   void printDependentLibsHelper(
       function_ref<void(const Elf_Shdr &)> OnSectionStart,
       function_ref<void(StringRef, uint64_t)> OnSectionEntry);
 
   virtual void printReloc(const Relocation<ELFT> &R, unsigned RelIndex,
                           const Elf_Shdr &Sec, const Elf_Shdr *SymTab) = 0;
   virtual void printRelrReloc(const Elf_Relr &R) = 0;
   virtual void printDynamicReloc(const Relocation<ELFT> &R) = 0;
template <class ELFT> void GNUStyle<ELFT>::printFileHeaders() {
   Str = to_string(e.e_shentsize) + " (bytes)";
   printFields(OS, "Size of section headers:", Str);
   Str = getSectionHeadersNumString(this->Obj, this->FileName);
   printFields(OS, "Number of section headers:", Str);
   Str = getSectionHeaderTableIndexString(this->Obj, this->FileName);
   printFields(OS, "Section header string table index:", Str);
 }
 
-namespace {
-struct GroupMember {
-  StringRef Name;
-  uint64_t Index;
-};
+template <class ELFT> std::vector<GroupSection> DumpStyle<ELFT>::getGroups() {
+  auto GetSignature = [&](const Elf_Sym &Sym,
+                          const Elf_Shdr &Symtab) -> StringRef {
+    Expected<StringRef> StrTableOrErr = Obj.getStringTableForSymtab(Symtab);
+    if (!StrTableOrErr) {
+      reportUniqueWarning(createError("unable to get the string table for " +
+                                      describe(Obj, Symtab) + ": " +
+                                      toString(StrTableOrErr.takeError())));
+      return "<?>";
+    }
 
-struct GroupSection {
-  StringRef Name;
-  std::string Signature;
-  uint64_t ShName;
-  uint64_t Index;
-  uint32_t Link;
-  uint32_t Info;
-  uint32_t Type;
-  std::vector<GroupMember> Members;
+    // TODO: this might lead to a crash or produce a wrong result, when the
+    // st_name goes past the end of the string table.
+    return StrTableOrErr->data() + Sym.st_name;
   };
 
-template <class ELFT>
-std::vector<GroupSection> getGroups(const ELFFile<ELFT> &Obj,
-                                    StringRef FileName) {
-  using Elf_Shdr = typename ELFT::Shdr;
-  using Elf_Sym = typename ELFT::Sym;
-  using Elf_Word = typename ELFT::Word;
-
   std::vector<GroupSection> Ret;
   uint64_t I = 0;
   for (const Elf_Shdr &Sec : cantFail(Obj.sections())) {
     ++I;
     if (Sec.sh_type != ELF::SHT_GROUP)
       continue;
 
     const Elf_Shdr *Symtab =
         unwrapOrError(FileName, Obj.getSection(Sec.sh_link));
-    StringRef StrTable =
-        unwrapOrError(FileName, Obj.getStringTableForSymtab(*Symtab));
+
     const Elf_Sym *Sym = unwrapOrError(
         FileName, Obj.template getEntry<Elf_Sym>(*Symtab, Sec.sh_info));
     auto Data = unwrapOrError(
         FileName, Obj.template getSectionContentsAsArray<Elf_Word>(Sec));
 
     StringRef Name = unwrapOrError(FileName, Obj.getSectionName(Sec));
-    StringRef Signature = StrTable.data() + Sym->st_name;
+    StringRef Signature = GetSignature(*Sym, *Symtab);
     Ret.push_back({Name,
                    maybeDemangle(Signature),
                    Sec.sh_name,
                    I - 1,
                    Sec.sh_link,
                    Sec.sh_info,
                    Data[0],
                    {}});
 
     std::vector<GroupMember> &GM = Ret.back().Members;
     for (uint32_t Ndx : Data.slice(1)) {
       const Elf_Shdr &Sec = *unwrapOrError(FileName, Obj.getSection(Ndx));
       const StringRef Name = unwrapOrError(FileName, Obj.getSectionName(Sec));
       GM.push_back({Name, Ndx});
     }
   }
   return Ret;
 }
 
-DenseMap<uint64_t, const GroupSection *>
+static DenseMap<uint64_t, const GroupSection *>
 mapSectionsToGroups(ArrayRef<GroupSection> Groups) {
   DenseMap<uint64_t, const GroupSection *> Ret;
   for (const GroupSection &G : Groups)
     for (const GroupMember &GM : G.Members)
       Ret.insert({GM.Index, &G});
   return Ret;
 }
 
-} // namespace
-
 template <class ELFT> void GNUStyle<ELFT>::printGroupSections() {
-  std::vector<GroupSection> V = getGroups<ELFT>(this->Obj, this->FileName);
+  std::vector<GroupSection> V = this->getGroups();
   DenseMap<uint64_t, const GroupSection *> Map = mapSectionsToGroups(V);
   for (const GroupSection &G : V) {
     OS << "\n"
        << getGroupType(G.Type) << " group section ["
        << format_decimal(G.Index, 5) << "] `" << G.Name << "' [" << G.Signature
        << "] contains " << G.Members.size() << " sections:\n"
        << "   [Index]    Name\n";
     for (const GroupMember &GM : G.Members) {
W.printString("SectionHeaderCount",
                   getSectionHeadersNumString(this->Obj, this->FileName));
     W.printString("StringTableSectionIndex",
                   getSectionHeaderTableIndexString(this->Obj, this->FileName));
   }
 }
 
 template <class ELFT> void LLVMStyle<ELFT>::printGroupSections() {
   DictScope Lists(W, "Groups");
-  std::vector<GroupSection> V = getGroups<ELFT>(this->Obj, this->FileName);
+  std::vector<GroupSection> V = this->getGroups();
   DenseMap<uint64_t, const GroupSection *> Map = mapSectionsToGroups(V);
   for (const GroupSection &G : V) {
     DictScope D(W, "Group");
     W.printNumber("Name", G.Name, G.ShName);
     W.printNumber("Index", G.Index);
     W.printNumber("Link", G.Link);
     W.printNumber("Info", G.Info);
     W.printHex("Type", getGroupType(G.Type), G.Type);