[PATCH] D82955: [llvm-readelf] - Do not error out when dumping symbols.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/symbols.test:132
+# STRTAB-LINK-ERR-LLVM-NEXT: warning: '[[FILE]]': unable to get the string table for the SHT_SYMTAB section: invalid section index: 255
+# STRTAB-LINK-ERR-LLVM-NEXT: warning: '[[FILE]]': st_name (0x0) is past the end of the string table of size 0x0
+# STRTAB-LINK-ERR-LLVM-NEXT:   Symbol {
----------------
jhenderson wrote:
> 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.
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.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/symbols.test:230
+# SYMTAB-SHSTRTAB-ERR-GNU:      warning: '[[FILE]]': unable to get the name of the SHT_SYMTAB section: section header string table index 255 does not exist
+# SYMTAB-SHSTRTAB-ERR-GNU:      Symbol table for image contains 2 entries:
+# SYMTAB-SHSTRTAB-ERR-GNU-NEXT:    Num:    Value          Size Type    Bind   Vis       Ndx Name
----------------
jhenderson wrote:
> 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.
Fixed.


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

https://reviews.llvm.org/D82955