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

Tue Apr 11 01:54:43 PDT 2023

jhenderson added a comment.

It seems to me like there are 4(?) different issues that have come up here, and they probably need splitting into separate patches, and possibly even an RFC or two. I think the separate issues are:

1. GNU addr2line defaults to reading from a.out if no input file is specified.
2. If the input file can't be found, llvm-addr2line and llvm-symbolizer both enter interactive mode (but any addresses without an accompanying file will result in an error), whereas addr2line emits an error and exits.
3. The error message returned (in interactive mode) from llvm-addr2line does not match the GNU addr2line error message.
4. The error message is printed directly within the LLVMSymbolizer library, so has weird formatting etc. More generally, the errors printed by llvm-symbolizer are inconsistently formatted.

Feel free to correct any of the above or add more.

My opinions on the topics are:

1. It should be easy for llvm-addr2line to default to a value (specifically "a.out") for the "-e" command-line arg, if no input file is explicitly specified. It should be possible to implement it in such a way that the rest of the code doesn't require any changes for this to just work. I don't think we want to change llvm-symbolizer behaviour here, and it's a big enough difference that the llvm-addr2line docs should be updated to note the difference.
2. IMHO, if an explicitly specified input file does not exist (at the time of command-line parsing at least), I could get behind an error message being reported by BOTH llvm-symbolizer and llvm-addr2line during command-line parsing (or more likely, slightly after, but before the business logic of the symbolizer code kicks in). We'd still need to check for file existence as in the current library code, in case the file has been deleted since command-line parsing happened. The early check should just be a file existence check (don't try to open the file), and should exit the program in the event of failure. It might be worth raising this issue on the mailing lists to confirm changing the llvm-symbolizer behaviour too is okay with others. My reasoning for why this should impact both tools is that a user has requested an explicit file, so if it doesn't exist, trying to use it doesn't make sense, but I could see theoretical use cases where the file is created after the opening of the program in interactive mode (so hence the need for discussion).
3. Re. error message format, I don't think an exact match of error message format should be necessary. That being said, I do think we should broadly match the error style emitted by our other binutils replacements (and certainly messages from the same tool should be formatted the same way). This goes for both llvm-symbolizer and llvm-addr2line. For example, if the input file is missing for llvm-nm, the following error is emitted on my Windows machine: "D:\llvm\build\Debug\bin\llvm-nm.exe: error: missing.bin: no such file or directory". The "error" is in red. Given we already deviate (slightly) from the GNU tools in the exact message format (GNU tools, apart from readelf, don't print the "error:" part), I think normalising with the rest of our tools makes more sense. Where there's a clear message from the OS, we should print that (along with any appropriate additional context). Where there isn't (e.g. you report an error on a missing file, without having attempted to actually open that file), it doesn't hurt to match the GNU message content, if the GNU one is sensible.
4. My opinions on library error reporting are documented in this lightning talk: https://www.youtube.com/watch?v=YSEY4pg1YB0. It's probably a non-trivial task (albeit hopefully not too complex), but in my opinion, this is a perfect case of where a callback should be used to report failures, rather than for the library to try to handle them directly.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147652