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

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff marked 5 inline comments as done.
sepavloff added a comment.

In D147652#4412823 <https://reviews.llvm.org/D147652#4412823>, @jhenderson wrote:

>> It looks like llvm-symbolizer also should check input file existence early. It need a separate patch, maybe someone uses such unusual mode.
>
> Unless the changes are significant, I don't see a reason to limit this change to llvm-addr2line only. If someone has specified `-e`, they won't ever be able to get useful output from llvm-symbolizer.

The patch is in D153219 <https://reviews.llvm.org/D153219>. It is not large, but there are several tests that expect processing addresses in the case of nonexisting binary files. It is not clear if these tests were added for better coverage or there are real cases for such usage. In the latter case it is better to have a separate patch.



================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:486
+    if (!Symbolizer.findBinaryFile(InputFile, Message)) {
+      errs() << ToolName << ": '" << InputFile << "': " << Message << '\n';
+      return EXIT_FAILURE;
----------------
jhenderson wrote:
> sepavloff wrote:
> > jhenderson wrote:
> > > 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?
> > If `llvm-addr2line` were given wrong binary file, it printed message:
> > ```
> > $ llvm-addr2line -e inexistent 0x12
> > LLVMSymbolizer: error reading file: No such file or directory
> > ??:0
> > ```
> > Output of GNU `addr2line` is different:
> > ```
> > $ addr2line -e inexistent 0x12
> > addr2line: 'inexistent': No such file
> > ```
> > With this patch output of `llvm-addr2line` is closer to GNU utility:
> > ```
> > $ llvm-addr2line -e inexistent 0x12
> > llvm-addr2line: 'inexistent': No such file or directory
> > ```
> > 
> > Actually output of `llvm-symbolizer` is not consistent. `LLVMSymbolizer` is a class name, it does not represent utility. Such message banner is OK for debug dumps, but for a user it is inconvenient.
> > 
> > > 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?
> > 
> > The main problem with such patch is decision about message format. It GNU-compatible message would be used, which part of the message `llvm-addr2line: 'inexistent': No such file or directory` would be colored red? Should the message contain text `error:`? Can it break some user scenarios due to changed output? Technically such patch looks simple, I could make it. 
> My strong preference is that libraries pass their error messages back up to the tool that invokes them, in one form or other (e.g. via a callback or an `Error`/`Expected`). As you say, end users don't care about the internal details like class names, so these shouldn't be in error messages.
> 
> I don't think we aim for direct GNU compatibility with error messages, in general, though a leading toolname is considered important. There is already helpful functionality that handles all of this though. See https://github.com/llvm/llvm-project/blob/d1ef99fe1ce3e2996c317600a1398933b9078260/llvm/tools/llvm-objdump/llvm-objdump.cpp#L320 for an example (specifically, look how `WithColor` takes a tool name as an argument).
The current implementation of error handling in llvm-symbolizer is very close to your preferences. Error messages are printed in `printError(const ErrorInfoBase &EI, StringRef Path)`, which is also passed as a callback to Printer classes. This function is very similar to `objdump::reportError` with one exception: it does not call `exit`. `llvm-symbolizer` can get things like binary file path from input stream. In such case immediate exit, if the file does not exist, is not suitable, because the next portion of data can provide valid binary file. So `exit` is called when it is clear that no useful work can be made.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147652