This is an archive of the discontinued LLVM Phabricator instance.

[symbolizer] Change error message if module not found
ClosedPublic

Authored by sepavloff on Apr 11 2023, 10:13 AM.

Details

Summary

If llvm-symbolize did not find module, the error looked like:

LLVMSymbolizer: error reading file: No such file or directory

This message does not follow common practice: LLVMSymbolizer is not an
utility name. Also the message did not not contain the name of missed file.

With this change the error message looks differently:

llvm-symbolizer: error: 'abc': No such file or directory

This format is closer to messages produced by other utilities and allow
proper coloring.

Diff Detail

Event Timeline

sepavloff created this revision.Apr 11 2023, 10:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2023, 10:13 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
sepavloff requested review of this revision.Apr 11 2023, 10:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2023, 10:13 AM

This is attempt to implement item 3 described in https://reviews.llvm.org/D147652#4257424.

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

Strict reproduction of addr2line messages is not a goal, the patch only tries making output slightly more convenient and looking similar to other utilities.

jhenderson added inline comments.Apr 12 2023, 1:11 AM
llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp
100

Other tools use the full tool path, as per the example I gave earlier. You can how this is done here: https://github.com/llvm/llvm-project/blob/43575719d0c6d8cf5afedf39f6d89f69231aedc4/llvm/tools/llvm-nm/llvm-nm.cpp#L155

sepavloff updated this revision to Diff 512843.Apr 12 2023, 8:16 AM

Updated patch

  • Use arv[0] as tool name,
  • Modified DIPrinter::printError,
  • Use callback in PlainPrinterBase to print error message,
  • Support color printing,
  • Fixed more tests.
MaskRay accepted this revision.Apr 12 2023, 6:31 PM

LGTM, thanks for making the change. I was confused by the error messages as well.

I only got a question about a variable name and its class name. Perhaps other reviewers can make suggestions.

llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h
73 ↗(On Diff #512843)

This is used to print error messages to errs(), not other types of messages. Shall we name this ErrorPrinter?

llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp
100

This comment about the full tool path has been resolved.

This revision is now accepted and ready to land.Apr 12 2023, 6:31 PM
MaskRay added inline comments.Apr 12 2023, 6:50 PM
llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h
73 ↗(On Diff #512843)

ErrorHandler may be better than ErrorPrinter.

llvm-readobj has a WarningHandler.

Updated patch

  • Renamed MessagePrinter to ErrorHandler
  • Fixed test for MS DIA

Thanks!

llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h
73 ↗(On Diff #512843)

Renamed to ErrorHandler.

jhenderson accepted this revision.Apr 13 2023, 12:11 AM

One minor suggestion. Otherwise, looks good to me.

llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h
68 ↗(On Diff #513071)

I think it's more normal to use llvm::function_ref rather than a function pointer.

sepavloff updated this revision to Diff 513176.Apr 13 2023, 4:56 AM

Rebased, use function_ref.

sepavloff updated this revision to Diff 513261.Apr 13 2023, 8:50 AM

Avoid double file name printing

If error message already contains file name, module file should not be
printed, otherwise two file names are printed. It happened in the test
llvm/test/tools/llvm-symbolizer/pdb/missing_pdb.test. Function
'printError' is modified to avoid such messages.

MaskRay accepted this revision.Apr 13 2023, 4:38 PM
This revision was automatically updated to reflect the committed changes.

Note that I submitted https://github.com/llvm/llvm-project/commit/de4c038c7ba2c6a8d529cb094f1a7c3deaae9b75 to fix up the build failure before this got reverted. I subsequently reverted the fix https://github.com/llvm/llvm-project/commit/de4c038c7ba2c6a8d529cb094f1a7c3deaae9b75 in https://github.com/llvm/llvm-project/commit/de088dd3a0aa8f1a1e1ffb7ac4af264d5477dbfe to un-break the sanitizer after the revert of this patch, feel free to pick up the fix and continue.