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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:463-464
+  Expected<DenseMap<uint64_t, StringRef>> StrTableOrErr = Obj.getStringTable();
+  if (!StrTableOrErr)
+    reportUniqueWarning("unable to get the string table: " +
+                        toString(StrTableOrErr.takeError()));
----------------
Esme wrote:
> jhenderson wrote:
> > You need to add a test case that will exercise this warning. You also need to return after the warning, or you'll attempt to print the table contents when you don't have a valid map!
> Well, I'm unable to construct a lit test to exercise this warning. The warning occurs only if the function XCOFFObjectFile::getStringTableEntry(uint32_t Offset) returns an error. I don't think this should happen unless yaml2obj writes the wrong data for symbol name offset or the string table.
One of yaml2obj's intentions is to be able to craft broken inputs so that these things can be tested. That suggests to me you need to extend yaml2obj a little more, to allow you to force writing a broken value.

That being said, your comment made me think that the approach in getStringTable is wrong. That should be able to dump the entire string table, by just iterating over null terminated strings from whatever offset indicates the string table start, rather than dumping the individual string entries as derived from symbol information. What if, for example, the string table had strings that didn't correspond to symbol names? I'd expect this option to print them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104613