This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readelf] - Do not error out when dumping symbols.
ClosedPublic

Authored by grimar on Jul 1 2020, 6:12 AM.

Details

Summary

When the --symbols option/--dyn-symbols is given we might report an
error and exit when something goes not right. E.g. when the SHT_SYMTAB
section is broken. Though we could report a warning and try to continue
dumping instead in many cases.

This patch removes unwrapOrErr calls from the code involved in the
flow described.

Diff Detail

Event Timeline

grimar created this revision.Jul 1 2020, 6:12 AM
Herald added a project: Restricted Project. · View Herald Transcript
jhenderson added inline comments.Jul 2 2020, 2:51 AM
llvm/test/tools/llvm-readobj/ELF/symbols.test
126

Nit: add an extra space for indentation of the command, here and below.

132

I guess this is fine for now, but in an ideal world, I think we'd only print the first warning and not the second, since we already said we couldn't find the string table. In fact, in some ways the extra warning ("past the end of the string table") is slightly misleading - there is no string table at all, so you can't be "past the end" of it.

192

Small nit: I assume this warning should be indented one more space if it wants to match exactly the output.

230

This is misleading. I think the intent is for "image" to be the dynamic symbol table derived from a dynamic table. A relocatable object isn't an "image" since that refers to the thing actually loaded.

Perhaps simply "object" or even "<?>" would be better in this context.

grimar updated this revision to Diff 275075.Jul 2 2020, 5:38 AM
grimar marked 6 inline comments as done.
  • Addressed review comments.
llvm/test/tools/llvm-readobj/ELF/symbols.test
132

True. Perhaps it is not that hard to improve if we make StringRef StrTable to be optional and change ELFDumperStyle->printSymbol signature and logic a bit. I'd try in a follow-up.

230

Fixed.

grimar marked an inline comment as done.
grimar added inline comments.
llvm/test/tools/llvm-readobj/ELF/symbols.test
132

I've prepared the follow-up patch for this: D83042

jhenderson accepted this revision.Jul 3 2020, 12:53 AM

Latest update LGTM.

This revision is now accepted and ready to land.Jul 3 2020, 12:53 AM