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

[Xing GUO via Phabricator via llvm-commits]

Higuoxing marked 2 inline comments as done.
Higuoxing added inline comments.


================
Comment at: llvm/lib/Object/ObjectFile.cpp:65
+    // TODO: Actually report errors helpfully.
+    consumeError(FlagsOrErr.takeError());
   return getSymbolValueImpl(Ref);
----------------
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?


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:805
+        // TODO: Report and test this error.
+        report_fatal_error(SymFlagsOrErr.takeError());
+      SymFlags = *SymFlagsOrErr;
----------------
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 ... 🤣


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77860