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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Apologies for the delay in coming back to this: I've been caught up with several weeks worth of high priority work with my job, and haven't had the time to look at LLVM reviews beyond a quick glance.

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

I'm okay with the direction @dblaikie has suggested ultimately, namely that we restructure the code so that we open the file early, as long as we keep the file open thereafter (so as not to open the file twice). However, I think it could be a separate subsequent patch, so that we can get this immediate improvement added.

By the way, there is prior art for existence checking long before the file is needed. Specifically, LLD checks that certain files exist early on, so that it doesn't need the relatively slow work of the link before discovering that the file doesn't exist or the directory is invalid in some manner etc.



================
Comment at: llvm/lib/DebugInfo/Symbolize/Symbolize.cpp:565
   }
+    return std::make_pair(BinaryName.str(), ArchName);
+}
----------------
Nit: have you run clang-format on this?


================
Comment at: llvm/lib/DebugInfo/Symbolize/Symbolize.cpp:570-572
+  std::string BinaryName;
+  std::string ArchName;
+  std::tie(BinaryName, ArchName) = splitBinaryFileName(ModuleName);
----------------
We have C++17 structured bindings now.


================
Comment at: llvm/lib/DebugInfo/Symbolize/Symbolize.cpp:719
+/// \param BinName Name of the file to search for.
+/// \returns Error::success() if the file was successfully opened, error object
+///          otherwise.
----------------
There's no need for doxygen comments in a .cpp file like this, as far as I know. Regardless, the comment is a bit stale: you're not opening the file here.


================
Comment at: llvm/lib/DebugInfo/Symbolize/Symbolize.cpp:724
+  std::string ArchName;
+  std::tie(BinaryName, ArchName) = splitBinaryFileName(BinName);
+  sys::fs::file_status Stat;
----------------
As above re. structured bindings.


================
Comment at: llvm/test/tools/llvm-symbolizer/errors.test:1
+RUN: not llvm-addr2line -e %p/Inputs/inexistent 0x12 2>&1 | FileCheck %s --check-prefix=CHECK-INEXISTENT-A2L -DMSG=%errc_ENOENT
+RUN: not llvm-addr2line -e %p/Inputs/inexistent 2>&1 | FileCheck %s --check-prefix=CHECK-INEXISTENT-A2L -DMSG=%errc_ENOENT
----------------
"Inexistent" isn't a word - you probably mean "nonexistent".

It would be good to have a test case that tries to open a directory.


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:486
+    if (!Symbolizer.findBinaryFile(InputFile, Message)) {
+      errs() << ToolName << ": '" << InputFile << "': " << Message << '\n';
+      return EXIT_FAILURE;
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147652