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

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff added inline comments.


================
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;
----------------
jhenderson wrote:
> 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.
`llvm-addr2line` did not assume `a.out` if input file was not specified. Instead it followed logic of `llvm-symbolizer` and expected binary file name in the input stream, provided that addresses were not specified in command line also.

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

`llvm-symbolizer` and `addr2line` enter "interactive" mode if addresses are not specified in command line. If binary file is not specified as well, `llvm-symbolizer` tries reading both binary file and addresses. `addr2line` never reads binary file from input stream.


================
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:
> 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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147652