[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

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

1976

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/tools/llvm-readobj/ELFDumper.cpp

void getSectionNameIndex(const Elf_Sym *Symbol, const Elf_Sym *FirstSym,
                            unsigned &SectionIndex) const;
   std::string getStaticSymbolName(uint32_t Index) 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;
+  void printDynamicString(uint64_t Value, raw_ostream &OS,
+                          bool WithBracket = true) const;
 
   const Elf_Shdr *getDotSymtabSec() const { return DotSymtabSec; }
   const Elf_Shdr *getDotCGProfileSec() const { return DotCGProfileSec; }
   const Elf_Shdr *getDotAddrsigSec() const { return DotAddrsigSec; }
   ArrayRef<Elf_Word> getShndxTable() const { return ShndxTable; }
   StringRef getDynamicStringTable() const { return DynamicStringTable; }
   const DynRegionInfo &getDynRelRegion() const { return DynRelRegion; }
   const DynRegionInfo &getDynRelaRegion() const { return DynRelaRegion; }
const std::map<uint64_t, const char*> TagNames = {
       {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) << ">";
+    printDynamicString(Value, OS, true);
     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>
+void ELFDumper<ELFT>::printDynamicString(uint64_t Value, raw_ostream &OS,
+                                         bool WithBracket) const {
+  if (DynamicStringTable.empty())
+    OS << "<String table is empty or was not found> ";
+  else if (Value < DynamicStringTable.size()) {
+    if (WithBracket)
+      OS << "[";
+    OS << StringRef(DynamicStringTable.data() + Value);
+    if (WithBracket)
+      OS << "]";
+  } else
+    OS << "<Invalid offset 0x" << utohexstr(Value) << ">";
+}
+
 template <class ELFT> void ELFDumper<ELFT>::printUnwindInfo() {
   DwarfCFIEH::PrinterContext<ELFT> Ctx(W, ObjF);
   Ctx.printUnwindInformation();
 }
 
 namespace {
 
 template <> void ELFDumper<ELF32LE>::printUnwindInfo() {