[llvm-readobj] Refactor dynamic string table indexing into a function.

[ychen]

Summary

Introduce getDynamicString to provide better
error handling whenever indexing dynamic string table is needed.

[MaskRay]

Do you find another use case of printDynamicString? If yes, can you name it?

llvm/tools/llvm-readobj/ELFDumper.cpp

300 ↗

(On Diff #209624)

Delete = true. The default argument is not used I think.

[ychen]

• Replace printDynamicString with getDynamicString.
• Change tests accordingly.

[ychen]

In D64674#1584157, @MaskRay wrote:

Do you find another use case of printDynamicString? If yes, can you name it?

Patch updated. I think we could use a function to indexing the dynamic string table.

[grimar]

llvm/test/tools/llvm-readobj/elf-dynamic-malformed.test

172 ↗

(On Diff #209752)

Isn't it inconsistent? For line above you print <error>, but here you print [<error>].
Was it intentional change? (it is not NFC then).

llvm/tools/llvm-readobj/ELFDumper.cpp

1973 ↗

(On Diff #209752)

You do not need else after return:

if (DynamicStringTable.empty())
  return "<String table is empty or was not found>";
if (Value < DynamicStringTable.size())
  return DynamicStringTable.data() + Value;

[ychen]

• Address reviewer's comments.

[ychen]

llvm/test/tools/llvm-readobj/elf-dynamic-malformed.test

172 ↗

(On Diff #209752)

Title updated to reflect that it is not NFC.

[grimar]

llvm/test/tools/llvm-readobj/elf-dynamic-malformed.test

172 ↗

(On Diff #209752)

So, previously we printed [valid string] and <error text>.
After this patch can print [valid string], [<error text>] and <error text>.
What is not very consistent. Why the new behavior is desired?

@jhenderson, what do you think?

[MaskRay]

llvm/test/tools/llvm-readobj/elf-dynamic-malformed.test

172 ↗

(On Diff #209752)

I'm fine with either choice (though I have a slight preference for non-capitalized messages)

[grimar]

llvm/test/tools/llvm-readobj/elf-dynamic-malformed.test

172 ↗

(On Diff #209752)

I am talking about the types of brackets used, not about capitalization. I.e. before this patch we had 2 forms: [..], <..> and it introduced a third one: [<..>]

[jhenderson]

llvm/test/tools/llvm-readobj/elf-dynamic-malformed.test

172 ↗

(On Diff #209752)

My thought is that [<...>] looks a bit weird, especially when it's inconsistent. That being said, the two are significantly different parts of the output - one is in the header blurb at the start of the output, the other is in the dynamic table output, so actually I'm not sure consistency here is such an issue.

In dynamic table printing, everything is surrounded by square brackets for good output. Not having the square brackets could be confusing for consumers when they hit bad output, I guess.

I think I could make an argument for it either way around, and I don't think either stands out more than the other. Therefore, I think we should just go with what is easiest in the code. In this case, I think that's the current proposed patch, since it avoids having something come back from getDynamicString saying whether to add the square brackets or not. I don't really mind though, if a good alternative implementation can be found for the other approach.

[grimar]

I am OK with this patch.

llvm/test/tools/llvm-readobj/elf-dynamic-malformed.test

172 ↗

(On Diff #209752)

I do not really like the new output, but it is not so critical probably.

llvm/trunk/test/tools/llvm-readobj/elf-dynamic-malformed.test

Sections:
       - Section: .dynamic
 
 ## Test handling of string references pointing past the end of the dynamic string table.
 # RUN: yaml2obj %s --docnum=3 -o %t.bad-string
 # RUN: llvm-readobj --dynamic-table %t.bad-string | FileCheck %s --check-prefix BAD-STRING-LLVM
 # RUN: llvm-readelf --dynamic-table %t.bad-string | FileCheck %s --check-prefix BAD-STRING-GNU
 
 # BAD-STRING-LLVM: 0x000000000000000A STRSZ         1 (bytes)
-# BAD-STRING-LLVM: 0x0000000000000001 NEEDED        Shared library: <Invalid offset 0x1>
-# BAD-STRING-LLVM: 0x000000007FFFFFFF FILTER        Filter library: <Invalid offset 0x1>
-# BAD-STRING-LLVM: 0x000000007FFFFFFD AUXILIARY     Auxiliary library: <Invalid offset 0x1>
-# BAD-STRING-LLVM: 0x000000007FFFFFFE USED          Not needed object: <Invalid offset 0x1>
-# BAD-STRING-LLVM: 0x000000000000000E SONAME        Library soname: <Invalid offset 0x1>
-# BAD-STRING-LLVM: 0x000000000000000F RPATH         Library rpath: <Invalid offset 0x1>
-# BAD-STRING-LLVM: 0x000000000000001D RUNPATH       Library runpath: <Invalid offset 0x1>
+# BAD-STRING-LLVM: 0x0000000000000001 NEEDED        Shared library: [<Invalid offset 0x1>]
+# BAD-STRING-LLVM: 0x000000007FFFFFFF FILTER        Filter library: [<Invalid offset 0x1>]
+# BAD-STRING-LLVM: 0x000000007FFFFFFD AUXILIARY     Auxiliary library: [<Invalid offset 0x1>]
+# BAD-STRING-LLVM: 0x000000007FFFFFFE USED          Not needed object: [<Invalid offset 0x1>]
+# BAD-STRING-LLVM: 0x000000000000000E SONAME        Library soname: [<Invalid offset 0x1>]
+# BAD-STRING-LLVM: 0x000000000000000F RPATH         Library rpath: [<Invalid offset 0x1>]
+# BAD-STRING-LLVM: 0x000000000000001D RUNPATH       Library runpath: [<Invalid offset 0x1>]
 
 # BAD-STRING-GNU:  0x000000000000000a (STRSZ)       1 (bytes)
-# BAD-STRING-GNU:  0x0000000000000001 (NEEDED)      Shared library: <Invalid offset 0x1>
-# BAD-STRING-GNU:  0x000000007fffffff (FILTER)      Filter library: <Invalid offset 0x1>
-# BAD-STRING-GNU:  0x000000007ffffffd (AUXILIARY)   Auxiliary library: <Invalid offset 0x1>
-# BAD-STRING-GNU:  0x000000007ffffffe (USED)        Not needed object: <Invalid offset 0x1>
-# BAD-STRING-GNU:  0x000000000000000e (SONAME)      Library soname: <Invalid offset 0x1>
-# BAD-STRING-GNU:  0x000000000000000f (RPATH)       Library rpath: <Invalid offset 0x1>
-# BAD-STRING-GNU:  0x000000000000001d (RUNPATH)     Library runpath: <Invalid offset 0x1>
+# BAD-STRING-GNU:  0x0000000000000001 (NEEDED)      Shared library: [<Invalid offset 0x1>]
+# BAD-STRING-GNU:  0x000000007fffffff (FILTER)      Filter library: [<Invalid offset 0x1>]
+# BAD-STRING-GNU:  0x000000007ffffffd (AUXILIARY)   Auxiliary library: [<Invalid offset 0x1>]
+# BAD-STRING-GNU:  0x000000007ffffffe (USED)        Not needed object: [<Invalid offset 0x1>]
+# BAD-STRING-GNU:  0x000000000000000e (SONAME)      Library soname: [<Invalid offset 0x1>]
+# BAD-STRING-GNU:  0x000000000000000f (RPATH)       Library rpath: [<Invalid offset 0x1>]
+# BAD-STRING-GNU:  0x000000000000001d (RUNPATH)     Library runpath: [<Invalid offset 0x1>]
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_X86_64
 Sections:

 # RUN: yaml2obj %s --docnum=4 -o %t.bad-strtab
 
 # RUN: llvm-readobj --dynamic-table %t.bad-strtab 2>&1 >/dev/null | FileCheck %s --check-prefix BAD-STRTAB-ERR
 # RUN: llvm-readelf --dynamic-table %t.bad-strtab 2>&1 >/dev/null | FileCheck %s --check-prefix BAD-STRTAB-ERR
 # BAD-STRTAB-ERR: warning: Unable to parse DT_STRTAB: virtual address is not in any segment: 0x2000000
 
 # RUN: llvm-readobj --dynamic-table --needed-libs %t.bad-strtab | FileCheck %s --check-prefixes=BAD-STRTAB,BAD-STRTAB-LLVM
 # RUN: llvm-readelf --dynamic-table --needed-libs %t.bad-strtab | FileCheck %s --check-prefixes=BAD-STRTAB,BAD-STRTAB-GNU
-# BAD-STRTAB-LLVM: LoadName: <Not found>
-# BAD-STRTAB-LLVM: 0x0000000000000001  NEEDED   Shared library: <String table is empty or was not found>
-# BAD-STRTAB-GNU:  0x0000000000000001 (NEEDED)  Shared library: <String table is empty or was not found>
+# BAD-STRTAB-LLVM: LoadName: <String table is empty or was not found>
+# BAD-STRTAB-LLVM: 0x0000000000000001  NEEDED   Shared library: [<String table is empty or was not found>]
+# BAD-STRTAB-GNU:  0x0000000000000001 (NEEDED)  Shared library: [<String table is empty or was not found>]
 # BAD-STRTAB:      NeededLibraries [
-# BAD-STRTAB:        <Library name index out of range>
+# BAD-STRTAB:        <String table is empty or was not found>
 # BAD-STRTAB:      ]
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_X86_64

llvm/trunk/tools/llvm-readobj/ELFDumper.cpp

private:
   const object::ELFObjectFile<ELFT> *ObjF;
   DynRegionInfo DynRelRegion;
   DynRegionInfo DynRelaRegion;
   DynRegionInfo DynRelrRegion;
   DynRegionInfo DynPLTRelRegion;
   DynRegionInfo DynSymRegion;
   DynRegionInfo DynamicTable;
   StringRef DynamicStringTable;
-  StringRef SOName = "<Not found>";
+  std::string SOName = "<Not found>";
   const Elf_Hash *HashTable = nullptr;
   const Elf_GnuHash *GnuHashTable = nullptr;
   const Elf_Shdr *DotSymtabSec = nullptr;
   const Elf_Shdr *DotCGProfileSec = nullptr;
   const Elf_Shdr *DotAddrsigSec = nullptr;
   StringRef DynSymtabName;
   ArrayRef<Elf_Word> ShndxTable;
 
public:
   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;
   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;
 
   const Elf_Shdr *getDotSymtabSec() const { return DotSymtabSec; }
case ELF::DT_JMPREL:
       break;
     case ELF::DT_PLTRELSZ:
       DynPLTRelRegion.Size = Dyn.getVal();
       break;
     }
   }
   if (StringTableBegin)
     DynamicStringTable = StringRef(StringTableBegin, StringTableSize);
-  if (SONameOffset && SONameOffset < DynamicStringTable.size())
-    SOName = DynamicStringTable.data() + SONameOffset;
+  SOName = getDynamicString(SONameOffset);
 }
 
 template <typename ELFT>
 typename ELFDumper<ELFT>::Elf_Rel_Range ELFDumper<ELFT>::dyn_rels() const {
   return DynRelRegion.getAsArrayRef<Elf_Rel>();
 }
 
 template <typename ELFT>
const std::map<uint64_t, const char*> TagNames = {
       {DT_NEEDED,    "Shared library"},
       {DT_SONAME,    "Library soname"},
       {DT_AUXILIARY, "Auxiliary library"},
       {DT_USED,      "Not needed object"},
       {DT_FILTER,    "Filter library"},
       {DT_RPATH,     "Library rpath"},
       {DT_RUNPATH,   "Library runpath"},
     };
-    OS << TagNames.at(Type) << ": ";
-    if (DynamicStringTable.empty())
-      OS << "<String table is empty or was not found> ";
-    else if (Value < DynamicStringTable.size())
-      OS << "[" << StringRef(DynamicStringTable.data() + Value) << "]";
-    else
-      OS << "<Invalid offset 0x" << utohexstr(Value) << ">";
+    OS << TagNames.at(Type) << ": [" << getDynamicString(Value) << "]";
     break;
   }
   case DT_FLAGS:
     printFlags(Value, makeArrayRef(ElfDynamicDTFlags), OS);
     break;
   case DT_FLAGS_1:
     printFlags(Value, makeArrayRef(ElfDynamicDTFlags1), OS);
     break;
   default:
     OS << format(ConvChar, Value);
     break;
   }
 }
 
+template <class ELFT>
+std::string ELFDumper<ELFT>::getDynamicString(uint64_t Value) const {
+  if (DynamicStringTable.empty())
+    return "<String table is empty or was not found>";
+  if (Value < DynamicStringTable.size())
+    return DynamicStringTable.data() + Value;
+  return Twine("<Invalid offset 0x" + utohexstr(Value) + ">").str();
+}
+
 template <class ELFT> void ELFDumper<ELFT>::printUnwindInfo() {
   DwarfCFIEH::PrinterContext<ELFT> Ctx(W, ObjF);
   Ctx.printUnwindInformation();
 }
 
 namespace {
 
 template <> void ELFDumper<ELF32LE>::printUnwindInfo() {

 
 template <class ELFT> void ELFDumper<ELFT>::printDynamicTable() {
   ELFDumperStyle->printDynamic(ObjF->getELFFile());
 }
 
 template <class ELFT> void ELFDumper<ELFT>::printNeededLibraries() {
   ListScope D(W, "NeededLibraries");
 
-  using LibsTy = std::vector<StringRef>;
-  LibsTy Libs;
-
+  std::vector<std::string> Libs;
   for (const auto &Entry : dynamic_table())
-    if (Entry.d_tag == ELF::DT_NEEDED) {
-      uint64_t Value = Entry.d_un.d_val;
-      if (Value < DynamicStringTable.size())
-        Libs.push_back(StringRef(DynamicStringTable.data() + Value));
-      else
-        Libs.push_back("<Library name index out of range>");
-    }
+    if (Entry.d_tag == ELF::DT_NEEDED)
+      Libs.push_back(getDynamicString(Entry.d_un.d_val));
 
   llvm::stable_sort(Libs);
 
   for (const auto &L : Libs)
     W.startLine() << L << "\n";
 }
 
 template <typename ELFT> void ELFDumper<ELFT>::printHashTable() {