[PATCH] D86923: [llvm-readobj/elf] - Don't crash when the size of a dynamic symbol table, inferred from the hash table, is broken.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/dyn-symbols-size-from-hash-table.test:340
+
+# BROKEN-NCHAIN-GNU: {{^}}[[#%u,FILESIZE:]]
+# BROKEN-NCHAIN-GNU: warning: '[[FILE]]': the derived from the hash table size (0x17ffffffe8) of the dynamic symbol table at 0x[[#%x,DYNSYMOFF:]] goes past the end of the file (0x[[#%x, FILESIZE]]) and will be ignored
----------------
I don't think you need the `{{^}}` here and below, since the FileCheck value will start from the start of the number anyway, and we know it will be at the start of the file.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/dyn-symbols-size-from-hash-table.test:340-341
+
+# BROKEN-NCHAIN-GNU: {{^}}[[#%u,FILESIZE:]]
+# BROKEN-NCHAIN-GNU: warning: '[[FILE]]': the derived from the hash table size (0x17ffffffe8) of the dynamic symbol table at 0x[[#%x,DYNSYMOFF:]] goes past the end of the file (0x[[#%x, FILESIZE]]) and will be ignored
+
----------------
jhenderson wrote:
> I don't think you need the `{{^}}` here and below, since the FileCheck value will start from the start of the number anyway, and we know it will be at the start of the file.
Be consistent with your spacing after the `,` in the FileCheck numeric variables. I don't mind which you use, but you've got some cases with a space after it, and others without.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2259-2263
+          "the derived from the hash table size (0x" +
+          Twine::utohexstr(DerivedSize) +
+          ") of the dynamic symbol table at 0x" + Twine::utohexstr(Offset) +
+          " goes past the end of the file (0x" + Twine::utohexstr(FileSize) +
+          ") and will be ignored"));
----------------
I'm not sure I like this message. The first clause, whilst I can figure out what it means, feels very clunky. I think your best option is "The size (0x1234), derived from the hash table, of the dynamic symbol table..." or "The size (0x1234) of the dynamic symbol table at 0x1234, derived from the hash table, goes past ..."


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

https://reviews.llvm.org/D86923