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

[Xing GUO via Phabricator via llvm-commits]

Higuoxing marked an inline comment as done.
Higuoxing added inline comments.


================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:635
+  } else
+    // TODO: Test this error.
+    return SymbolsOrErr.takeError();
----------------
jhenderson wrote:
> This and the below TODO in this function can be removed with appropriate testing in D77864, right?
Yes.


================
Comment at: llvm/lib/Object/ObjectFile.cpp:65
+    // TODO: Actually report errors helpfully.
+    consumeError(FlagsOrErr.takeError());
   return getSymbolValueImpl(Ref);
----------------
jhenderson wrote:
> 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)?
Sorry, I mean `report_fatal_error`, because it dumps calling stacks in debug mode, which looks like crash. I will look into it. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77860