[PATCH] D77289: [Object] Fix crash caused by unhandled error.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:628
 
-  auto DotSymtabSecSyms = EF.symbols(DotSymtabSec);
-  if (DotSymtabSecSyms && ESym == (*DotSymtabSecSyms).begin())
-    Result |= SymbolRef::SF_FormatSpecific;
-  auto DotDynSymSecSyms = EF.symbols(DotDynSymSec);
-  if (DotDynSymSecSyms && ESym == (*DotDynSymSecSyms).begin())
-    Result |= SymbolRef::SF_FormatSpecific;
+  if (auto DotSymtabSecSymsOrErr = EF.symbols(DotSymtabSec)) {
+    if (ESym == DotSymtabSecSymsOrErr->begin())
----------------
I wonder if this should be changed to not use `auto`? I think that would have made it less likely for this mistake to have got through unnoticed.


================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:634
+    consumeError(DotSymtabSecSymsOrErr.takeError());
+  if (auto DotDynSymSecSymsOrErr = EF.symbols(DotDynSymSec)) {
+    if (ESym == DotDynSymSecSymsOrErr->begin())
----------------
Ditto.


================
Comment at: llvm/test/tools/llvm-nm/X86/invalid-section-size.test:1
+## This test ensures llvm-nm will not crash when dumping symbol table
+## whose sh_size isn't a multiple of the symbol size (sh_size % sizeof(Elf_Sym) != 0).
----------------
"when dumping a symbol table"

I wonder if this test should be moved to the `Object` test folder, since this is really testing the Object library, rather than llvm-nm specifically. What do you think?


================
Comment at: llvm/test/tools/llvm-nm/X86/invalid-section-size.test:3
+## whose sh_size isn't a multiple of the symbol size (sh_size % sizeof(Elf_Sym) != 0).
+## It will be helpful if we could warn about this in future.
+
----------------
Perhaps make this last line a TODO in the test.


================
Comment at: llvm/test/tools/llvm-nm/X86/invalid-section-size.test:6
+# RUN: yaml2obj -DBITS=32 -DSIZE=33 %s -o %t.32-bit.o
+# RUN: llvm-nm %t.32-bit.o | FileCheck %s
+# RUN: yaml2obj -DBITS=64 -DSIZE=49 %s -o %t.64-bit.o
----------------
It might be a good idea to add `2>&1` to the llvm-nm run and `--implicit-check-not=warning` to the FileCheck run so that this test starts failing if somebody implements the warning handling described, so that the test can be updated at that point, rather than it becoming redundant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77289