[PATCH] D104613: [llvm-readobj][XCOFF] Add support for printing the String Table.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Please add the new option to the llvm-readobj documentation.



================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:190
 
+Expected<std::vector<StringRef>> XCOFFObjectFile::getStringTable() const {
+  std::vector<StringRef> StringTable;
----------------
I'd make this return a map of offsets to strings. If you look at the --string-dump output of llvm-readelf, you'll see it includes the string offsets in there, and I think it would be good to use the same output format for this option.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:192-193
+  std::vector<StringRef> StringTable;
+  uint32_t NumberOfSymbols = getNumberOfSymbolTableEntries();
+  for (uint32_t I = 0; I < NumberOfSymbols; ++I) {
+    uintptr_t SymbolEntryAddress = getSymbolEntryAddressByIndex(I);
----------------
I'd inline NumberOfSymbols into the loop initialization list, since it's not needed otherwise.


================
Comment at: llvm/tools/llvm-readobj/ObjDumper.h:108-109
 
+  // Only implemented for XCOFF.
+  virtual void printXCOFFStringTable() { }
+
----------------
I'd keep the comment, but name it `printStringTable()` so that you don't need to rename the function if and when this gets implemented for other file formats - there's nothing fundamentally XCOFF-specific about this feature after all.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:462-463
+  DictScope DS(W, "StringTable");
+  std::vector<StringRef> StringTable =
+      unwrapOrError(Obj.getFileName(), Obj.getStringTable());
+  for (const StringRef &Entry : StringTable)
----------------
It would be preferable to print the error as a warning and skip dumping, so that a user who requests to dump the string table and other data can still get the other data. This is the general pattern we follow elsewhere in llvm-readobj, at least for newer code (possibly it hasn't been adopted in XCOFF, but I'm trying to encourage it).


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:464
+      unwrapOrError(Obj.getFileName(), Obj.getStringTable());
+  for (const StringRef &Entry : StringTable)
+    W.printString(Entry);
----------------
`StringRef` is designed to be passed by value, not reference.


================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:294-297
+  // --xcoff-string-table
+  cl::opt<bool>
+  XCOFFStringTable("xcoff-string-table",
+                  cl::desc("Display the XCOFF string table"));
----------------
Let's drop the "xcoff" from the name and description. Perhaps add in the help message "Currently implemented for XCOFF only." instead. That way, when the option is adopted for other formats, we don't need multiple options or to force users to change the switch name.


================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:604
   }
+  if (Obj.isXCOFF()) {
+    if (opts::XCOFFStringTable)
----------------
No need for this outer-if statement, since the virtual function has a do-nothing base version.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104613/new/

https://reviews.llvm.org/D104613