[llvm-exegesis] Improve error reporting

[mstojanovic]

Summary

Fix inconsistencies in error reporting created by mixing report_fatal_error() and ExitOnErr(), and add additional information to the error message to make it more user friendly. Minimize the use report_fatal_error() because it's meant for use in very rare cases and it results in low information density of the error messages.

Summary of the new design:

• For command line argument errors output llvm-exegesis: <error_message>, which is consistent with the error output format emitted by the backend which checks correctness of the command line arguments.
• For other errors the format llvm-exegesis error: <error_message> is used.
  • If the error occured during file access <error_message> will have of two parts: '<file_name>': <rest_of_the_error_message>

[courbet]

Thanks for doing this.

[hans]

A bunch of tests are now failing with "llvm-exegesis error: benchmarking unavailable, LLVM was built without libpfm."

See for example: http://45.33.8.238/linux/9540/step_12.txt

[hans]

In D74085#1861496, @hans wrote:

A bunch of tests are now failing with "llvm-exegesis error: benchmarking unavailable, LLVM was built without libpfm."

See for example: http://45.33.8.238/linux/9540/step_12.txt

I guess e.g. llvm/test/tools/llvm-exegesis/X86/lit.local.cfg needs to be updated to handle the new error handling.

I've reverted these changes in abe01e17f648a97666d4fbed41f0861686a17972 in the meantime.

llvm/tools/llvm-exegesis/llvm-exegesis.cpp

static cl::opt<std::string> CpuName(
     cl::cat(Options), cl::init(""));
 
 static cl::opt<bool>
     DumpObjectToDisk("dump-object-to-disk",
                      cl::desc("dumps the generated benchmark object to disk "
                               "and prints a message to access it"),
                      cl::cat(BenchmarkOptions), cl::init(true));
 
-static ExitOnError ExitOnErr;
+static ExitOnError ExitOnErr("llvm-exegesis error: ");
+
+// Helper function that logs the error(s) and exits.
+template <typename... ArgTs> static void ExitWithError(ArgTs &&... Args) {
+  ExitOnErr(make_error<Failure>(std::forward<ArgTs>(Args)...));
+}
+
+// Check Err. If it's in a failure state log the file error(s) and exit.
+static void ExitOnFileError(const Twine &FileName, Error Err) {
+  if (Err) {
+    ExitOnErr(createFileError(FileName, std::move(Err)));
+  }
+}
+
+// Check E. If it's in a success state then return the contained value.
+// If it's in a failure state log the file error(s) and exit.
+template <typename T>
+T ExitOnFileError(const Twine &FileName, Expected<T> &&E) {
+  ExitOnFileError(FileName, E.takeError());
+  return std::move(*E);
+}
 
 // Checks that only one of OpcodeNames, OpcodeIndex or SnippetsFile is provided,
 // and returns the opcode indices or {} if snippets should be read from
 // `SnippetsFile`.
 static std::vector<unsigned> getOpcodesOrDie(const MCInstrInfo &MCInstrInfo) {
   const size_t NumSetFlags = (OpcodeNames.empty() ? 0 : 1) +
                              (OpcodeIndex == 0 ? 0 : 1) +
                              (SnippetsFile.empty() ? 0 : 1);
-  if (NumSetFlags != 1)
-    report_fatal_error(
-        "please provide one and only one of 'opcode-index', 'opcode-name' or "
-        "'snippets-file'");
+  if (NumSetFlags != 1) {
+    ExitOnErr.setBanner("llvm-exegesis: ");
+    ExitWithError("please provide one and only one of 'opcode-index', "
+                  "'opcode-name' or 'snippets-file'");
+  }
   if (!SnippetsFile.empty())
     return {};
   if (OpcodeIndex > 0)
     return {static_cast<unsigned>(OpcodeIndex)};
   if (OpcodeIndex < 0) {
     std::vector<unsigned> Result;
     for (unsigned I = 1, E = MCInstrInfo.getNumOpcodes(); I < E; ++I)
       Result.push_back(I);
static std::vector<unsigned> getOpcodesOrDie(const MCInstrInfo &MCInstrInfo) {
   SmallVector<StringRef, 2> Pieces;
   StringRef(OpcodeNames.getValue())
       .split(Pieces, ",", /* MaxSplit */ -1, /* KeepEmpty */ false);
   std::vector<unsigned> Result;
   for (const StringRef &OpcodeName : Pieces) {
     if (unsigned Opcode = ResolveName(OpcodeName))
       Result.push_back(Opcode);
     else
-      report_fatal_error(Twine("unknown opcode ").concat(OpcodeName));
+      ExitWithError(Twine("unknown opcode ").concat(OpcodeName));
   }
   return Result;
 }
 
 // Generates code snippets for opcode `Opcode`.
 static Expected<std::vector<BenchmarkCode>>
 generateSnippets(const LLVMState &State, unsigned Opcode,
                  const BitVector &ForbiddenRegs) {
   const Instruction &Instr = State.getIC().getInstr(Opcode);
   const MCInstrDesc &InstrDesc = Instr.Description;
   // Ignore instructions that we cannot run.
   if (InstrDesc.isPseudo())
     return make_error<Failure>("Unsupported opcode: isPseudo");
   if (InstrDesc.isBranch() || InstrDesc.isIndirectBranch())
     return make_error<Failure>("Unsupported opcode: isBranch/isIndirectBranch");
   if (InstrDesc.isCall() || InstrDesc.isReturn())
     return make_error<Failure>("Unsupported opcode: isCall/isReturn");
 
   SnippetGenerator::Options SnippetOptions;
   SnippetOptions.MaxConfigsPerOpcode = MaxConfigsPerOpcode;
   const std::unique_ptr<SnippetGenerator> Generator =
       State.getExegesisTarget().createSnippetGenerator(BenchmarkMode, State,
                                                        SnippetOptions);
   if (!Generator)
-    report_fatal_error("cannot create snippet generator");
+    ExitWithError("cannot create snippet generator");
   return Generator->generateConfigurations(Instr, ForbiddenRegs);
 }
 
 void benchmarkMain() {
 #ifndef HAVE_LIBPFM
-  report_fatal_error(
-      "benchmarking unavailable, LLVM was built without libpfm.");
+  ExitWithError("benchmarking unavailable, LLVM was built without libpfm.");
 #endif
 
   if (exegesis::pfm::pfmInitialize())
-    report_fatal_error("cannot initialize libpfm");
+    ExitWithError("cannot initialize libpfm");
 
   InitializeNativeTarget();
   InitializeNativeTargetAsmPrinter();
   InitializeNativeTargetAsmParser();
   InitializeNativeExegesisTarget();
 
   const LLVMState State(CpuName);
 
   const std::unique_ptr<BenchmarkRunner> Runner =
       State.getExegesisTarget().createBenchmarkRunner(BenchmarkMode, State);
   if (!Runner) {
-    report_fatal_error("cannot create benchmark runner");
+    ExitWithError("cannot create benchmark runner");
   }
 
   const auto Opcodes = getOpcodesOrDie(State.getInstrInfo());
 
   const auto Repetitor = SnippetRepetitor::Create(RepetitionMode, State);
 
   std::vector<BenchmarkCode> Configurations;
   if (!Opcodes.empty()) {
for (const unsigned Opcode : Opcodes) {
       }
       std::move(ConfigsForInstr->begin(), ConfigsForInstr->end(),
                 std::back_inserter(Configurations));
     }
   } else {
     Configurations = ExitOnErr(readSnippets(State, SnippetsFile));
   }
 
-  if (NumRepetitions == 0)
-    report_fatal_error("--num-repetitions must be greater than zero");
+  if (NumRepetitions == 0) {
+    ExitOnErr.setBanner("llvm-exegesis: ");
+    ExitWithError("--num-repetitions must be greater than zero");
+  }
 
   // Write to standard output if file is not set.
   if (BenchmarkFile.empty())
     BenchmarkFile = "-";
 
   for (const BenchmarkCode &Conf : Configurations) {
     InstructionBenchmark Result = Runner->runConfiguration(
         Conf, NumRepetitions, *Repetitor, DumpObjectToDisk);
-    ExitOnErr(Result.writeYaml(State, BenchmarkFile));
+    ExitOnFileError(BenchmarkFile, Result.writeYaml(State, BenchmarkFile));
   }
   exegesis::pfm::pfmTerminate();
 }
 
 // Prints the results of running analysis pass `Pass` to file `OutputFilename`
 // if OutputFilename is non-empty.
 template <typename Pass>
 static void maybeRunAnalysis(const Analysis &Analyzer, const std::string &Name,
                              const std::string &OutputFilename) {
   if (OutputFilename.empty())
     return;
   if (OutputFilename != "-") {
     errs() << "Printing " << Name << " results to file '" << OutputFilename
            << "'\n";
   }
   std::error_code ErrorCode;
   raw_fd_ostream ClustersOS(OutputFilename, ErrorCode,
                             sys::fs::FA_Read | sys::fs::FA_Write);
   if (ErrorCode)
-    report_fatal_error("cannot open out file: " + OutputFilename);
+    ExitOnFileError(OutputFilename, errorCodeToError(ErrorCode));
   if (auto Err = Analyzer.run<Pass>(ClustersOS))
-    report_fatal_error(std::move(Err));
+    ExitOnFileError(OutputFilename, std::move(Err));
 }
 
 static void analysisMain() {
+  ExitOnErr.setBanner("llvm-exegesis: ");
   if (BenchmarkFile.empty())
-    report_fatal_error("--benchmarks-file must be set.");
+    ExitWithError("--benchmarks-file must be set");
 
   if (AnalysisClustersOutputFile.empty() &&
       AnalysisInconsistenciesOutputFile.empty()) {
-    report_fatal_error(
-        "At least one of --analysis-clusters-output-file and "
-        "--analysis-inconsistencies-output-file must be specified.");
+    ExitWithError(
+        "for --mode=analysis: At least one of --analysis-clusters-output-file"
+        "and --analysis-inconsistencies-output-file must be specified");
   }
 
   InitializeNativeTarget();
   InitializeNativeTargetAsmPrinter();
   InitializeNativeTargetDisassembler();
+
   // Read benchmarks.
   const LLVMState State("");
-  const std::vector<InstructionBenchmark> Points =
-      ExitOnErr(InstructionBenchmark::readYamls(State, BenchmarkFile));
+  const std::vector<InstructionBenchmark> Points = ExitOnFileError(
+      BenchmarkFile, InstructionBenchmark::readYamls(State, BenchmarkFile));
+
   outs() << "Parsed " << Points.size() << " benchmark points\n";
   if (Points.empty()) {
     errs() << "no benchmarks to analyze\n";
     return;
   }
   // FIXME: Check that all points have the same triple/cpu.
   // FIXME: Merge points from several runs (latency and uops).