[NFC][llvm-readobj] Split getSectionIndexName function into two

[jhenderson]

Summary

getSectionIndexName was trying to fetch two things at once, which led to a somewhat tricky to understand interface involving passing output parameters in, and also made it hard to return Errors further up the stack.

This change is in preparation for changing the error handling.

Additionally, update a related test now that yaml2obj supports SHT_SYMTAB_SHNDX properly, and add missing LLVM-style coverage for symbols with shndx SHN_XINDEX. This test (after fixing) caught a mistake in my first attempt at this patch, hence I'm including it as part of this patch.

Depends on D69669.

[MaskRay]

llvm/test/tools/llvm-readobj/elf-symbol-shndx.test

68–69

Nice cleanup after the yaml2obj change D65446.

[grimar]

LGTM with a nit.

llvm/tools/llvm-readobj/ELFDumper.cpp

7024

Seems you should be able to typename ELFT::Shdr -> Elf_Shdr here?

llvm/test/tools/llvm-readobj/elf-symbol-shndx.test

 # Show that llvm-readobj prints symbol shndxes correctly, for valid indexes,
 # invalid indexes (i.e. section indexes that don't correspond to a real
 # section), reserved values and processor/os-specific index values, for both GNU
 # and LLVM styles.
 
-# Use --dyn-symbols because yaml2obj does not currently support large section indexes
-# and also does not allow hand-crafting of static symbol tables.
 # RUN: yaml2obj --docnum=1 %s > %t1
-# RUN: llvm-readobj --symbols --dyn-symbols %t1 | FileCheck %s --check-prefix=LLVM
-# RUN: llvm-readelf --symbols --dyn-symbols %t1 | FileCheck %s --check-prefix=GNU1
+# RUN: llvm-readobj --symbols %t1 | FileCheck %s --check-prefix=LLVM1
+# RUN: llvm-readelf --symbols %t1 | FileCheck %s --check-prefix=GNU1
 
 # llvm-readobj doesn't accept shndx values that are not valid section indexes
 # for LLVM style, so only test GNU output in this case.
 # RUN: yaml2obj --docnum=2 %s > %t2
 # RUN: llvm-readelf --symbols %t2 | FileCheck %s --check-prefix=GNU2
 
-# LLVM: Name:    undef
-# LLVM: Section: Undefined (0x0)
-# LLVM: Name:    normal
-# LLVM: Section: .text (0x1)
-# LLVM: Name:    common
-# LLVM: Section: Common (0xFFF2)
-# LLVM: Name:    absolute
-# LLVM: Section: Absolute (0xFFF1)
-# LLVM: Name:    proc
-# LLVM: Section: Processor Specific (0xFF01)
-# LLVM: Name:    os
-# LLVM: Section: Operating System Specific (0xFF21)
-# LLVM: Name:    reserved
-# LLVM: Section: Reserved (0xFFFE)
+# LLVM1: Name:    undef
+# LLVM1: Section:
+# LLVM1-SAME:     Undefined (0x0)
+# LLVM1: Name:    normal
+# LLVM1: Section:
+# LLVM1-SAME:     .text (0x1)
+# LLVM1: Name:    common
+# LLVM1: Section:
+# LLVM1-SAME:     Common (0xFFF2)
+# LLVM1: Name:    absolute
+# LLVM1: Section:
+# LLVM1-SAME:     Absolute (0xFFF1)
+# LLVM1: Name:    proc
+# LLVM1: Section:
+# LLVM1-SAME:     Processor Specific (0xFF01)
+# LLVM1: Name:    os
+# LLVM1: Section:
+# LLVM1-SAME:     Operating System Specific (0xFF21)
+# LLVM1: Name:    reserved
+# LLVM1: Section:
+# LLVM1-SAME:     Reserved (0xFFFE)
+# LLVM1: Name:    xindex
+# LLVM1: Section:
+# LLVM1:          .text (0x1)
 
-# GNU1:      Symbol table '.dynsym' contains 2 entries:
-# GNU1-NEXT:   Num: {{.*}} Ndx Name
-# GNU1-NEXT:     0: {{.*}} UND
-# GNU1-NEXT:     1: {{.*}}   1 xindex
-# GNU1:      Symbol table '.symtab' contains 8 entries:
+# GNU1:      Symbol table '.symtab' contains 9 entries:
 # GNU1-NEXT:   Num: {{.*}} Ndx Name
 # GNU1-NEXT:     0: {{.*}} UND
 # GNU1-NEXT:     1: {{.*}} UND undef
 # GNU1-NEXT:     2: {{.*}}   1 normal
 # GNU1-NEXT:     3: {{.*}} COM common
 # GNU1-NEXT:     4: {{.*}} ABS absolute
 # GNU1-NEXT:     5: {{.*}} PRC[0xff01] proc
 # GNU1-NEXT:     6: {{.*}} OS[0xff21] os
 # GNU1-NEXT:     7: {{.*}} RSV[0xfffe] reserved
+# GNU1-NEXT:     8: {{.*}}   1 xindex
 
 # GNU2:      Symbol table '.symtab' contains 2 entries:
 # GNU2-NEXT:   Num: {{.*}} Ndx Name
 # GNU2-NEXT:     0: {{.*}} UND
 # GNU2-NEXT:     1: {{.*}}  66 bad
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS32
   Data:    ELFDATA2LSB
   Type:    ET_REL
   Machine: EM_386
 Sections:
   - Name: .text
     Type: SHT_PROGBITS
-  - Name: .dynstr
-    Type: SHT_STRTAB
-    #\0xindex\0
-    Content: "0078696e64657800"
-  - Name: .dynsym
-    Type: SHT_DYNSYM
-    Link: .dynstr
-    EntSize: 16
-    # Null symbol
-    # Symbol with st_name = 1, st_shndx = SHN_XINDEX
-    Content: "000000000000000000000000000000000100000000000000000000000000ffff"
   - Name: .symtab_shndx
     Type: SHT_SYMTAB_SHNDX
-    Link: .dynsym
-    Entries: [ 0, 1 ]
+    Link: .symtab
+    Entries: [ 0, 0, 0, 0, 0, 0, 0, 0, 1 ]
 Symbols:
   - Name:    undef
     Binding: STB_GLOBAL
   - Name:    normal
     Section: .text
     Binding: STB_GLOBAL
   - Name:    common
     Index:   SHN_COMMON
     Binding: STB_GLOBAL
   - Name:    absolute
     Index:   SHN_ABS
     Binding: STB_GLOBAL
   - Name:    proc
     Index:   0xff01
     Binding: STB_GLOBAL
   - Name:    os
     Index:   0xff21
     Binding: STB_GLOBAL
   - Name:    reserved
     Index:   0xfffe
     Binding: STB_GLOBAL
+  - Name:    xindex
+    Index:   SHN_XINDEX
+    Binding: STB_GLOBAL
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS32
   Data:    ELFDATA2LSB
   Type:    ET_REL
   Machine: EM_386
 Symbols:
   - Name:    bad
     Index:   0x42
     Binding: STB_GLOBAL

llvm/tools/llvm-readobj/ELFDumper.cpp

Elf_Sym_Range dynamic_symbols() const {
     return DynSymRegion.getAsArrayRef<Elf_Sym>();
   }
 
   Elf_Rel_Range dyn_rels() const;
   Elf_Rela_Range dyn_relas() const;
   Elf_Relr_Range dyn_relrs() const;
   std::string getFullSymbolName(const Elf_Sym *Symbol, StringRef StrTable,
                                 bool IsDynamic) const;
-  void getSectionNameIndex(const Elf_Sym *Symbol, const Elf_Sym *FirstSym,
-                           StringRef &SectionName,
-                           unsigned &SectionIndex) const;
+  Expected<unsigned> getSymbolSectionIndex(const Elf_Sym *Symbol,
+                                           const Elf_Sym *FirstSym) const;
+  Expected<StringRef> getSymbolSectionName(const Elf_Sym *Symbol,
+                                           unsigned SectionIndex) const;
   Expected<std::string> getStaticSymbolName(uint32_t Index) const;
   std::string getDynamicString(uint64_t Value) const;
   StringRef getSymbolVersionByIndex(StringRef StrTab,
                                     uint32_t VersionSymbolIndex,
                                     bool &IsDefault) const;
 
   void printSymbolsHelper(bool IsDynamic) const;
   void printDynamicEntry(raw_ostream &OS, uint64_t Type, uint64_t Value) const;

 template <typename ELFT>
 std::string ELFDumper<ELFT>::getFullSymbolName(const Elf_Sym *Symbol,
                                                StringRef StrTable,
                                                bool IsDynamic) const {
   std::string SymbolName = maybeDemangle(
       unwrapOrError(ObjF->getFileName(), Symbol->getName(StrTable)));
 
   if (SymbolName.empty() && Symbol->getType() == ELF::STT_SECTION) {
-    unsigned SectionIndex;
-    StringRef SectionName;
     Elf_Sym_Range Syms = unwrapOrError(
         ObjF->getFileName(), ObjF->getELFFile()->symbols(DotSymtabSec));
-    getSectionNameIndex(Symbol, Syms.begin(), SectionName, SectionIndex);
-    return SectionName;
+    unsigned SectionIndex = unwrapOrError(
+        ObjF->getFileName(), getSymbolSectionIndex(Symbol, Syms.begin()));
+    return unwrapOrError(ObjF->getFileName(),
+                         getSymbolSectionName(Symbol, SectionIndex));
   }
 
   if (!IsDynamic)
     return SymbolName;
 
   bool IsDefault;
   StringRef Version = getSymbolVersion(StrTable, &*Symbol, IsDefault);
   if (!Version.empty()) {
     SymbolName += (IsDefault ? "@@" : "@");
     SymbolName += Version;
   }
   return SymbolName;
 }
 
 template <typename ELFT>
-void ELFDumper<ELFT>::getSectionNameIndex(const Elf_Sym *Symbol,
-                                          const Elf_Sym *FirstSym,
-                                          StringRef &SectionName,
-                                          unsigned &SectionIndex) const {
-  SectionIndex = Symbol->st_shndx;
+Expected<unsigned>
+ELFDumper<ELFT>::getSymbolSectionIndex(const Elf_Sym *Symbol,
+                                       const Elf_Sym *FirstSym) const {
+  return Symbol->st_shndx == SHN_XINDEX
+             ? object::getExtendedSymbolTableIndex<ELFT>(Symbol, FirstSym,
+                                                         ShndxTable)
+             : Symbol->st_shndx;
+}
+
+// If the Symbol has a reserved st_shndx other than SHN_XINDEX, return a
+// descriptive interpretation of the st_shndx value. Otherwise, return the name
+// of the section with index SectionIndex. This function assumes that if the
+// Symbol has st_shndx == SHN_XINDEX the SectionIndex will be the value derived
+// from the SHT_SYMTAB_SHNDX section.
+template <typename ELFT>
+Expected<StringRef>
+ELFDumper<ELFT>::getSymbolSectionName(const Elf_Sym *Symbol,
+                                      unsigned SectionIndex) const {
   if (Symbol->isUndefined())
-    SectionName = "Undefined";
-  else if (Symbol->isProcessorSpecific())
-    SectionName = "Processor Specific";
-  else if (Symbol->isOSSpecific())
-    SectionName = "Operating System Specific";
-  else if (Symbol->isAbsolute())
-    SectionName = "Absolute";
-  else if (Symbol->isCommon())
-    SectionName = "Common";
-  else if (Symbol->isReserved() && SectionIndex != SHN_XINDEX)
-    SectionName = "Reserved";
-  else {
-    if (SectionIndex == SHN_XINDEX)
-      SectionIndex = unwrapOrError(ObjF->getFileName(),
-                                   object::getExtendedSymbolTableIndex<ELFT>(
-                                       Symbol, FirstSym, ShndxTable));
-    const ELFFile<ELFT> *Obj = ObjF->getELFFile();
-    const typename ELFT::Shdr *Sec =
-        unwrapOrError(ObjF->getFileName(), Obj->getSection(SectionIndex));
-    SectionName = unwrapOrError(ObjF->getFileName(), Obj->getSectionName(Sec));
-  }
+    return "Undefined";
+  if (Symbol->isProcessorSpecific())
+    return "Processor Specific";
+  if (Symbol->isOSSpecific())
+    return "Operating System Specific";
+  if (Symbol->isAbsolute())
+    return "Absolute";
+  if (Symbol->isCommon())
+    return "Common";
+  if (Symbol->isReserved() && Symbol->st_shndx != SHN_XINDEX)
+    return "Reserved";
+
+  Expected<const Elf_Shdr *> SecOrErr =
+      Obj->getSection(SectionIndex);
+  if (!SecOrErr)
+    return SecOrErr.takeError();
+  return Obj->getSectionName(*SecOrErr);
 }
 
 template <class ELFO>
 static const typename ELFO::Elf_Shdr *
 findNotEmptySectionByAddress(const ELFO *Obj, StringRef FileName,
                              uint64_t Addr) {
   for (const auto &Shdr : unwrapOrError(FileName, Obj->sections()))
     if (Shdr.sh_addr == Addr && Shdr.sh_size > 0)
if (opts::SectionData && Sec.sh_type != ELF::SHT_NOBITS) {
           StringRef(reinterpret_cast<const char *>(Data.data()), Data.size()));
     }
   }
 }
 
 template <class ELFT>
 void LLVMStyle<ELFT>::printSymbolSection(const Elf_Sym *Symbol,
                                          const Elf_Sym *First) {
-  unsigned SectionIndex = 0;
-  StringRef SectionName;
-  this->dumper()->getSectionNameIndex(Symbol, First, SectionName, SectionIndex);
+  unsigned SectionIndex = unwrapOrError(
+      this->FileName, this->dumper()->getSymbolSectionIndex(Symbol, First));
+  StringRef SectionName = unwrapOrError(
+      this->FileName, this->dumper()->getSymbolSectionName(Symbol, SectionIndex));
   W.printHex("Section", SectionName, SectionIndex);
 }
 
 template <class ELFT>
 void LLVMStyle<ELFT>::printSymbol(const ELFO *Obj, const Elf_Sym *Symbol,
                                   const Elf_Sym *First, StringRef StrTable,
                                   bool IsDynamic,
                                   bool /*NonVisibilityBitsUsed*/) {
void LLVMStyle<ELFT>::printMipsABIFlags(const ELFObjectFile<ELFT> *ObjF) {
   W.printEnum("ISA Extension", Flags->isa_ext, makeArrayRef(ElfMipsISAExtType));
   W.printFlags("ASEs", Flags->ases, makeArrayRef(ElfMipsASEFlags));
   W.printEnum("FP ABI", Flags->fp_abi, makeArrayRef(ElfMipsFpABIType));
   W.printNumber("GPR size", getMipsRegisterSize(Flags->gpr_size));
   W.printNumber("CPR1 size", getMipsRegisterSize(Flags->cpr1_size));
   W.printNumber("CPR2 size", getMipsRegisterSize(Flags->cpr2_size));
   W.printFlags("Flags 1", Flags->flags1, makeArrayRef(ElfMipsFlags1));
   W.printHex("Flags 2", Flags->flags2);
 }