[PATCH] D147652: [symbolizer] Check existence of input file in GNU mode
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Apr 6 00:14:51 PDT 2023
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
More information about the llvm-commits
mailing list