[PATCH] D77860: [Object] Change uint32_t getSymbolFlags() to Expected<uint32_t> getSymbolFlags().

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Object/ObjectFile.cpp:65
+    // TODO: Actually report errors helpfully.
+    consumeError(FlagsOrErr.takeError());
   return getSymbolValueImpl(Ref);
----------------
Higuoxing wrote:
> jhenderson wrote:
> > Why `consumeError` here instead of `report_fatal_error`?
> Because this is a blocker for us to land tests.
> 
> `getSymbolValue()` is used by `getSymbolAddress()` and `getSymbolAddress()` is used by `llvm-objdump`. And `getSymbolAddress()` is called before `getSymbolFlags()`. So, `llvm-objdump` still crashes.
> 
> But, we still could tweak the calling order to check `getSymbolFlags()` first and report errors early to prevent crash. I'm not sure if we want to make this an unsafe API?
Sorry, you've lost me slightly. When you say llvm-objdump crashes, do you mean due to an unchecked Error, or something else? I wouldn't consider `report_fatal_error` quite a crash, though it's not as desirable as a proper error, that's true.

If `getSymbolValue` is causing problems too, maybe we should be changing its API as well, to return `Expected` (again, as a separate commit)?


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:805
+        // TODO: Report and test this error.
+        report_fatal_error(SymFlagsOrErr.takeError());
+      SymFlags = *SymFlagsOrErr;
----------------
Higuoxing wrote:
> jhenderson wrote:
> > FWIW, I'd actually report the error properly here, and add testing only in D77864, but it's up to you.
> Oh, I was misunderstanding the meaning of "NFC", I was thinking reporting error here is a functional change ... 🤣
I think you understood NFC correctly, but `report_fatal_error` reports errors as much as anything else. Might as well hook up the correct error function instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77860