[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

114

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

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

114

This comment about the full tool path has been resolved.

[MaskRay]

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

73

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

Renamed to ErrorHandler.

[jhenderson]

One minor suggestion. Otherwise, looks good to me.

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

68

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/include/llvm/DebugInfo/Symbolize/DIPrinter.h

public:
   virtual void print(const Request &Request, const DIGlobal &Global) = 0;
   virtual void print(const Request &Request,
                      const std::vector<DILocal> &Locals) = 0;
 
   virtual void printInvalidCommand(const Request &Request,
                                    StringRef Command) = 0;
 
   virtual bool printError(const Request &Request,
-                          const ErrorInfoBase &ErrorInfo,
-                          StringRef ErrorBanner) = 0;
+                          const ErrorInfoBase &ErrorInfo) = 0;
 
   virtual void listBegin() = 0;
   virtual void listEnd() = 0;
 };
 
 struct PrinterConfig {
   bool PrintAddress;
   bool PrintFunctions;
   bool Pretty;
   bool Verbose;
   int SourceContextLines;
 };
 
+using ErrorHandler = function_ref<void(const ErrorInfoBase &, StringRef)>;
+
 class PlainPrinterBase : public DIPrinter {
 protected:
   raw_ostream &OS;
-  raw_ostream &ES;
+  ErrorHandler ErrHandler;
   PrinterConfig Config;
 
   void print(const DILineInfo &Info, bool Inlined);
   void printFunctionName(StringRef FunctionName, bool Inlined);
   virtual void printSimpleLocation(StringRef Filename,
                                    const DILineInfo &Info) = 0;
   void printContext(SourceCode SourceCode);
   void printVerbose(StringRef Filename, const DILineInfo &Info);
   virtual void printStartAddress(const DILineInfo &Info) {}
   virtual void printFooter() {}
 
 private:
   void printHeader(uint64_t Address);
 
 public:
-  PlainPrinterBase(raw_ostream &OS, raw_ostream &ES, PrinterConfig &Config)
-      : OS(OS), ES(ES), Config(Config) {}
+  PlainPrinterBase(raw_ostream &OS, ErrorHandler EH, PrinterConfig &Config)
+      : OS(OS), ErrHandler(EH), Config(Config) {}
 
   void print(const Request &Request, const DILineInfo &Info) override;
   void print(const Request &Request, const DIInliningInfo &Info) override;
   void print(const Request &Request, const DIGlobal &Global) override;
   void print(const Request &Request,
              const std::vector<DILocal> &Locals) override;
 
   void printInvalidCommand(const Request &Request, StringRef Command) override;
 
-  bool printError(const Request &Request, const ErrorInfoBase &ErrorInfo,
-                  StringRef ErrorBanner) override;
+  bool printError(const Request &Request,
+                  const ErrorInfoBase &ErrorInfo) override;
 
   void listBegin() override {}
   void listEnd() override {}
 };
 
 class LLVMPrinter : public PlainPrinterBase {
 private:
   void printSimpleLocation(StringRef Filename, const DILineInfo &Info) override;
   void printStartAddress(const DILineInfo &Info) override;
   void printFooter() override;
 
 public:
-  LLVMPrinter(raw_ostream &OS, raw_ostream &ES, PrinterConfig &Config)
-      : PlainPrinterBase(OS, ES, Config) {}
+  LLVMPrinter(raw_ostream &OS, ErrorHandler EH, PrinterConfig &Config)
+      : PlainPrinterBase(OS, EH, Config) {}
 };
 
 class GNUPrinter : public PlainPrinterBase {
 private:
   void printSimpleLocation(StringRef Filename, const DILineInfo &Info) override;
 
 public:
-  GNUPrinter(raw_ostream &OS, raw_ostream &ES, PrinterConfig &Config)
-      : PlainPrinterBase(OS, ES, Config) {}
+  GNUPrinter(raw_ostream &OS, ErrorHandler EH, PrinterConfig &Config)
+      : PlainPrinterBase(OS, EH, Config) {}
+
 };
 
 class JSONPrinter : public DIPrinter {
 private:
   raw_ostream &OS;
   PrinterConfig Config;
   std::unique_ptr<json::Array> ObjectList;
 
public:
   void print(const Request &Request, const DILineInfo &Info) override;
   void print(const Request &Request, const DIInliningInfo &Info) override;
   void print(const Request &Request, const DIGlobal &Global) override;
   void print(const Request &Request,
              const std::vector<DILocal> &Locals) override;
 
   void printInvalidCommand(const Request &Request, StringRef Command) override;
 
-  bool printError(const Request &Request, const ErrorInfoBase &ErrorInfo,
-                  StringRef ErrorBanner) override;
+  bool printError(const Request &Request,
+                  const ErrorInfoBase &ErrorInfo) override;
 
   void listBegin() override;
   void listEnd() override;
 };
 } // namespace symbolize
 } // namespace llvm
 
 #endif

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;
-  ErrorInfo.log(ES);
-  ES << '\n';
+                                  const ErrorInfoBase &ErrorInfo) {
+  ErrHandler(ErrorInfo, Request.ModuleName);
   // Print an empty struct too.
   return true;
 }
 
 static std::string toHex(uint64_t V) {
   return ("0x" + Twine::utohexstr(V)).str();
 }
 
void JSONPrinter::print(const Request &Request,
   else
     printJSON(std::move(Json));
 }
 
 void JSONPrinter::printInvalidCommand(const Request &Request,
                                       StringRef Command) {
   printError(Request,
              StringError("unable to parse arguments: " + Command,
-                         std::make_error_code(std::errc::invalid_argument)),
-             "");
+                         std::make_error_code(std::errc::invalid_argument)));
 }
 
 bool JSONPrinter::printError(const Request &Request,
-                             const ErrorInfoBase &ErrorInfo,
-                             StringRef ErrorBanner) {
+                             const ErrorInfoBase &ErrorInfo) {
   json::Object Json = toJSON(Request, ErrorInfo.message());
   if (ObjectList)
     ObjectList->push_back(std::move(Json));
   else
     printJSON(std::move(Json));
   return false;
 }
 

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/DebugInfo/Symbolize/ELF/symtab-file2.yaml

- Name:    weak
     Section: .text
     Value:   2
 
 ## If st_name of the STT_FILE symbols is invalid, the symbol name is lost as well.
 ## TODO Keep the symbol name.
 # RUN: yaml2obj --docnum=2 %s -o %t2
 # RUN: llvm-symbolizer --obj=%t2 0 0 2>&1 | FileCheck %s --check-prefix=CHECK2
 
-# CHECK2:      error reading file: st_name (0xffff) is past the end of the string table of size
+# CHECK2:      llvm-symbolizer{{.*}}: error: '{{.*}}symtab-file2.yaml.tmp2': st_name (0xffff) is past the end of the string table of size
 # CHECK2-NEXT: ??
 # CHECK2-NEXT: ??:0:0
 # CHECK2-EMPTY:
 # CHECK2-NEXT: ??
 # CHECK2-NEXT: ??:0:0
 
 --- !ELF
 FileHeader:

llvm/test/DebugInfo/symbolize-missing-file.test

 RUN: llvm-symbolizer --obj=unexisting-file 0x1234 2>&1 | FileCheck -DMSG=%errc_ENOENT %s
 
-CHECK: LLVMSymbolizer: error reading file: [[MSG]]
+CHECK: llvm-symbolizer{{.*}}: error: 'unexisting-file': [[MSG]]

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/test/tools/llvm-symbolizer/pdb/missing_pdb.test

 RUN: llvm-symbolizer 0x401000 0x401001 --obj="%p/Inputs/missing_pdb.exe" 2>%t.err \
 RUN:    | FileCheck %s
 RUN: FileCheck -DMSG=%errc_ENOENT --check-prefix=ERROR %s < %t.err
 
 llvm-symbolizer should print one error and two unknown line info records.
 
-ERROR: LLVMSymbolizer: error reading file: {{.*}}: [[MSG]]
+ERROR: llvm-symbolizer{{.*}}: error: '{{.*}}missing_pdb.pdb': [[MSG]]
 ERROR-NOT: error reading file
 
 CHECK: ??
 CHECK: ??:0:0
 CHECK: ??
 CHECK: ??:0:0

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

 #include "llvm/Option/Option.h"
 #include "llvm/Support/COM.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/StringSaver.h"
+#include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <cstdio>
 #include <cstring>
 #include <iostream>
 #include <string>
 
 using namespace llvm;

 class SymbolizerOptTable : public opt::GenericOptTable {
 public:
   SymbolizerOptTable() : GenericOptTable(InfoTable) {
     setGroupedShortOptions(true);
   }
 };
 } // namespace
 
+static std::string ToolName;
+
+static void printError(const ErrorInfoBase &EI, StringRef Path) {
+  WithColor::error(errs(), ToolName);
+  if (!EI.isA<FileError>())
+    errs() << "'" << Path << "': ";
+  EI.log(errs());
+  errs() << '\n';
+}
+
 template <typename T>
 static void print(const Request &Request, Expected<T> &ResOrErr,
                   DIPrinter &Printer) {
   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: ");
+                    PrintEmpty = Printer.printError(Request, EI);
                   });
 
   if (PrintEmpty)
     Printer.print(Request, T());
 }
 
 enum class OutputStyle { LLVM, GNU, JSON };
 
 enum class Command {
   Code,
   Data,
   Frame,

 }
 
 ExitOnError ExitOnErr;
 
 int main(int argc, char **argv) {
   InitLLVM X(argc, argv);
   sys::InitializeCOMRAII COM(sys::COMThreadingMode::MultiThreaded);
 
-  bool IsAddr2Line = sys::path::stem(argv[0]).contains("addr2line");
+  ToolName = argv[0];
+  bool IsAddr2Line = sys::path::stem(ToolName).contains("addr2line");
   BumpPtrAllocator A;
   StringSaver Saver(A);
   SymbolizerOptTable Tbl;
   opt::InputArgList Args = parseOptions(argc, argv, IsAddr2Line, Saver, Tbl);
 
   LLVMSymbolizer::Options Opts;
   uint64_t AdjustVMA;
   PrinterConfig Config;
#endif
   if (Args.hasArg(OPT_build_id_EQ) && Args.hasArg(OPT_obj_EQ)) {
     errs() << "error: cannot specify both --build-id and --obj\n";
     return EXIT_FAILURE;
   }
   object::BuildID BuildID = parseBuildIDArg(Args, OPT_build_id_EQ);
 
   std::unique_ptr<DIPrinter> Printer;
   if (Style == OutputStyle::GNU)
-    Printer = std::make_unique<GNUPrinter>(outs(), errs(), Config);
+    Printer = std::make_unique<GNUPrinter>(outs(), printError, Config);
   else if (Style == OutputStyle::JSON)
     Printer = std::make_unique<JSONPrinter>(outs(), Config);
   else
-    Printer = std::make_unique<LLVMPrinter>(outs(), errs(), Config);
+    Printer = std::make_unique<LLVMPrinter>(outs(), printError, Config);
 
   std::vector<std::string> InputAddresses = Args.getAllArgValues(OPT_INPUT);
   if (InputAddresses.empty()) {
     const int kMaxInputStringLength = 1024;
     char InputString[kMaxInputStringLength];
 
     while (fgets(InputString, sizeof(InputString), stdin)) {
       // Strip newline characters.