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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Thanks for the change. I think the interface change makes sense (and is in keeping with other similar functions). However, I think it may deserve pulling into a separate change from the new error check. That would make it an NFC interface change. This change would then implement the behaviour change and add appropriate testing for said behaviour, which would cover some of the new code paths.

As for the other code paths I've highlighted, it's probably okay to just put a TODO saying "needs testing" or something to that effect for most of them. @grimar, what do you think?



================
Comment at: llvm/include/llvm/Object/ObjectFile.h:301
+    if (!SymbolFlagsOrErr)
+      report_fatal_error(SymbolFlagsOrErr.takeError());
+    assert(*SymbolFlagsOrErr & SymbolRef::SF_Common);
----------------
It might be worth putting a TODO here saying that this should be pushed further up the stack. Probably also wants mentioning that this is untested currently.


================
Comment at: llvm/lib/ExecutionEngine/Orc/Mangling.cpp:98
+    if (!SymFlagsOrErr)
+      return SymFlagsOrErr.takeError();
+
----------------
This new bit of code should either be tested or at least have a TODO comment saying it needs to be.


================
Comment at: llvm/lib/ExecutionEngine/Orc/RTDyldObjectLinkingLayer.cpp:130
+      if (!SymFlags) {
+        ES.reportError(SymFlags.takeError());
+        R.failMaterialization();
----------------
Test?


================
Comment at: llvm/lib/ExecutionEngine/Orc/RTDyldObjectLinkingLayer.cpp:212
+      if (!SymFlags)
+        return SymFlags.takeError();
+      if (*SymFlags & object::BasicSymbolRef::SF_Undefined)
----------------
Test?


================
Comment at: llvm/lib/ExecutionEngine/RuntimeDyld/JITSymbol.cpp:60
+  if (!SymbolFlagsOrErr)
+    return SymbolFlagsOrErr.takeError();
+
----------------
Test?


================
Comment at: llvm/lib/ExecutionEngine/RuntimeDyld/JITSymbol.cpp:84
+  if (!SymbolFlagsOrErr)
+    report_fatal_error(SymbolFlagsOrErr.takeError());
   ARMJITSymbolFlags Flags;
----------------
TODO for pushing error up stack + test.


================
Comment at: llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp:219
+      if (!FlagsOrErr)
+        return FlagsOrErr.takeError();
+      if ((*FlagsOrErr & SymbolRef::SF_Common) ||
----------------
Test?


================
Comment at: llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp:242
+    if (!FlagsOrErr)
+      return FlagsOrErr.takeError();
 
----------------
Test?


================
Comment at: llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp:602
+    if (!FlagsOrErr)
+      return FlagsOrErr.takeError();
+    if (*FlagsOrErr & SymbolRef::SF_Common) {
----------------
Test?


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:269
+  if (!SymFlagsOrErr)
+    report_fatal_error(SymFlagsOrErr.takeError());
+  if (*SymFlagsOrErr & object::SymbolRef::SF_FormatSpecific)
----------------
Test/TODO to echo further up stack?


================
Comment at: llvm/lib/Object/MachOObjectFile.cpp:1810
+    // getSymbolFlags() on Mach-O symbol can not fail.
+    report_fatal_error(FlagsOrErr.takeError());
+  if (*FlagsOrErr & SymbolRef::SF_Common) {
----------------
`cantFail(FlagsOrErr.takeError())`

Then you can delete the comment.


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

Ditto in other test.


================
Comment at: llvm/test/tools/llvm-nm/invalid-symbol-table-size.test:13
+#       CHECK: error: {{.*}} section [index 2] has an invalid sh_size ([[SIZE]]) which is not a multiple of its sh_entsize ([[SYMSIZE]])
+# CHECK-EMPTY:
+
----------------
I don't think you need the CHECK-EMPTY for when an error is reported.

Ditto below.


================
Comment at: llvm/tools/dsymutil/MachODebugMapParser.cpp:499
+      // getFlags() on Mach-O symbol can not fail.
+      report_fatal_error(FlagsOrErr.takeError());
+    if (*FlagsOrErr & SymbolRef::SF_Absolute) {
----------------
`cantFail`


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:312
+    // Symbol flags have been checked in caller.
+    report_fatal_error(AFlagsOrErr.takeError());
   if (A.Sym.getRawDataRefImpl().p)
----------------
If it's impossible for this to fail, use `cantFail` instead. Same below and elsewhere.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:380
+      // getFlags() on Mach-O symbol can not fail.
+      report_fatal_error(SymFlags.takeError());
+    if (*SymFlags & SymbolRef::SF_Global)
----------------
`cantFail`


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1059
+    // getFlags() on WASM symbol can not fail.
+    report_fatal_error(FlagsOrErr.takeError());
+  if (*FlagsOrErr & SymbolRef::SF_Executable)
----------------
`cantFail`


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1069
+    // getFlags() on IR symbol can not fail.
+    report_fatal_error(FlagsOrErr.takeError());
   // FIXME: should we print 'b'? At the IR level we cannot be sure if this
----------------
`cantFail`


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1103
+  if (!SymFlagsOrErr) {
+    consumeError(SymFlagsOrErr.takeError());
+    return '?';
----------------
Why is `consumeError` here okay? Should it be `cantFail`? A comment is probably needed anyway.


================
Comment at: llvm/tools/llvm-objdump/MachODump.cpp:7565
+        // getSymbolFlags() on Mach-O symbol can not fail.
+        report_fatal_error(SymbolFlagsOrErr.takeError());
+      bool IsThumb = *SymbolFlagsOrErr & SymbolRef::SF_Thumb;
----------------
`cantFail`


================
Comment at: llvm/tools/llvm-size/llvm-size.cpp:204-205
+    if (!SymFlagsOrErr)
+      // FIXME: Actually report errors helpfully.
+      report_fatal_error(SymFlagsOrErr.takeError());
+    if (*SymFlagsOrErr & SymbolRef::SF_Common)
----------------
You can probably directly call llvm-size's `error` reporting routine here, since this is in the core llvm-size code. You would also need to test it then.


================
Comment at: llvm/tools/sancov/sancov.cpp:661
+    Expected<uint32_t> FlagsOrErr = Symbol.getFlags();
+    failIfError(FlagsOrErr);
+    uint32_t Flags = FlagsOrErr.get();
----------------
Test?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77289