[symbolizer] Change error message if module not found

[sepavloff]

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.

[sepavloff]

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

In D147652#4257424, @jhenderson wrote:

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

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.

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]

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

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.

[MaskRay]

llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h

73 ↗

(On Diff #512843)

ErrorHandler may be better than ErrorPrinter.

llvm-readobj has a WarningHandler.

[sepavloff]

Updated patch

• Renamed MessagePrinter to ErrorHandler
• Fixed test for MS DIA

[sepavloff]

Thanks!

llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h

73 ↗

(On Diff #512843)

Renamed to ErrorHandler.

[jhenderson]

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]

Rebased, use function_ref.

[sepavloff]

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.

[krasimir]

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.

[sepavloff]

@krasimir Thank you for the fix! I put it back in https://reviews.llvm.org/rG1d5fa4f88f636f99f55933c919789662518cb951.

llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp

 void PlainPrinterBase::printInvalidCommand(const Request &Request,
                                            StringRef Command) {
   OS << Command << '\n';
 }
 
 bool PlainPrinterBase::printError(const Request &Request,
                                   const ErrorInfoBase &ErrorInfo,
                                   StringRef ErrorBanner) {
-  ES << ErrorBanner;
+  ES << ErrorBanner << '\'' << Request.ModuleName << "': ";
   ErrorInfo.log(ES);
   ES << '\n';
   // Print an empty struct too.
   return true;
 }
 
 static std::string toHex(uint64_t V) {
   return ("0x" + Twine::utohexstr(V)).str();

llvm/lib/DebugInfo/Symbolize/Symbolize.cpp

LLVMSymbolizer::getOrCreateModuleInfo(const ObjectFile &Obj) {
   return createModuleInfo(&Obj, std::move(Context), ObjName);
 }
 
 Expected<SymbolizableModule *>
 LLVMSymbolizer::getOrCreateModuleInfo(ArrayRef<uint8_t> BuildID) {
   std::string Path;
   if (!getOrFindDebugBinary(BuildID, Path)) {
     return createStringError(errc::no_such_file_or_directory,
-                             Twine("could not find build ID '") +
-                                 toHex(BuildID) + "'");
+                             "could not find build ID");
   }
   return getOrCreateModuleInfo(Path);
 }
 
 namespace {
 
 // Undo these various manglings for Win32 extern "C" functions:
 // cdecl       - _foo

llvm/test/tools/llvm-symbolizer/debuginfod-missing-build-id.test

 RUN: llvm-symbolizer --build-id=abad 0x1234 0x5678 > %t.stdout 2> %t.stderr
 RUN: FileCheck %s --check-prefix=STDOUT < %t.stdout
 RUN: FileCheck %s --check-prefix=STDERR < %t.stderr
 
 STDOUT: ??
 STDOUT: ??:0:0
 STDOUT: ??
 STDOUT: ??:0:0
 
-STDERR-COUNT-2: LLVMSymbolizer: error reading file: could not find build ID 'ABAD'
+STDERR-COUNT-2: llvm-symbolizer: error: 'ABAD': could not find build ID

llvm/test/tools/llvm-symbolizer/output-style-inlined.test

 ## Check that we are able to produce an output properly when the --no-inlines option
 ## is specified, but a file doesn't exist. Check we report an error.
 
 RUN: llvm-symbolizer --output-style=GNU --obj=%p/Inputs/not.exist 0x1 0x2 --no-inlines 2>&1 \
 RUN:   | FileCheck %s --check-prefix=NOT-EXIST-GNU -DMSG=%errc_ENOENT
 RUN: llvm-symbolizer --output-style=LLVM --obj=%p/Inputs/not.exist 0x1 0x2 --no-inlines 2>&1 \
 RUN:   | FileCheck %s --check-prefix=NOT-EXIST-LLVM -DMSG=%errc_ENOENT
 
-# NOT-EXIST-GNU:      LLVMSymbolizer: error reading file: [[MSG]]
+# NOT-EXIST-GNU:      llvm-symbolizer: error: '{{.*}}Inputs/not.exist': [[MSG]]
 # NOT-EXIST-GNU-NEXT: ??
 # NOT-EXIST-GNU-NEXT: ??:0
 # NOT-EXIST-GNU-NEXT: ??
 # NOT-EXIST-GNU-NEXT: ??:0
 
-# NOT-EXIST-LLVM:       LLVMSymbolizer: error reading file: [[MSG]]
+# NOT-EXIST-LLVM:       llvm-symbolizer: error: '{{.*}}Inputs/not.exist': [[MSG]]
 # NOT-EXIST-LLVM-NEXT:  ??
 # NOT-EXIST-LLVM-NEXT:  ??:0:0
 # NOT-EXIST-LLVM-EMPTY:
 # NOT-EXIST-LLVM-NEXT:  ??
 # NOT-EXIST-LLVM-NEXT:  ??:0:0

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

static void print(const Request &Request, Expected<T> &ResOrErr,
   if (ResOrErr) {
     // No error, print the result.
     Printer.print(Request, *ResOrErr);
     return;
   }
 
   // Handle the error.
   bool PrintEmpty = true;
-  handleAllErrors(std::move(ResOrErr.takeError()),
-                  [&](const ErrorInfoBase &EI) {
-                    PrintEmpty = Printer.printError(
-                        Request, EI, "LLVMSymbolizer: error reading file: ");
-                  });
+  handleAllErrors(
+      std::move(ResOrErr.takeError()), [&](const ErrorInfoBase &EI) {
+        PrintEmpty =
+            Printer.printError(Request, EI, "llvm-symbolizer: error: ");
 
   if (PrintEmpty)
     Printer.print(Request, T());
 }
 
 enum class OutputStyle { LLVM, GNU, JSON };
 
 enum class Command {