[PATCH] D147652: [symbolizer] Check existence of input file in GNU mode

[James Henderson via Phabricator via llvm-commits]

jhenderson requested changes to this revision.
jhenderson added a comment.
This revision now requires changes to proceed.

Thanks for the patch! I'm not convinced you've made the right change in the right place, and there are a number of other issues with this patch, which need to be addressed. See my inline comments for details.



================
Comment at: llvm/lib/DebugInfo/Symbolize/Symbolize.cpp:718
+/// \returns True if the file was successfully opened.
+bool LLVMSymbolizer::findBinaryFile(StringRef BinName, std::string &Message) {
+  Expected<SymbolizableModule *> BinFile = getOrCreateModuleInfo(BinName.str());
----------------
Can this instead return an `Error` to be handled up the call stack? An Error contains as much information as a `std::string/bool` return pairing like you're doing.

Doing this means the function boils down to:
```
Error LLVMSymbolizer::findBinaryFile(StringRef BinName) {
  Expected<SymbolizableModule *> BinFile = getOrCreateModuleInfo(BinName.str());
  return BinFile ? Error::success() : BinFile.takeError();
}
```
That then raises the question: is the place you're currently calling this the right place? I suspect not, and that you should instead by failing and reporting the error around the point where `getOrCreateModuleInfo` is called "for real".


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:482-483
+    StringRef InputFile = Args.getLastArgValue(OPT_obj_EQ);
+    if (InputFile.empty())
+      InputFile = "a.out";
+    std::string Message;
----------------
Does llvm-addr2line already assume `a.out` is the input file if no input file is specified? If not, this seems like it's incorrect behaviour.

I was under the impression that if no input file is specified, it entered "interactive" mode.


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:486
+    if (!Symbolizer.findBinaryFile(InputFile, Message)) {
+      errs() << ToolName << ": '" << InputFile << "': " << Message << '\n';
+      return EXIT_FAILURE;
----------------
This is a different style to how other warnings and errors are printed for llvm-symbolizer (see above e.g. line 467 in this patch).

It seems to me like a follow-up change to normalise the printing format for all warnings and errors to use `WithColor`, probably within `reportWarning`/`reportError` functions is in order. Are you happy to make that patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147652