[MachineVerifier] Report errors from one thread at a time (#111605)

Create the `ReportedErrors` class to track the number of reported errors
during verification. The class will block reporting errors if some other
thread is currently reporting an error.

I've encountered a case where there were many different verifications
reporting errors at the same time on different threads. This ensures
that we don't start printing the error from one case until we are
completely done printing errors from other cases. Most of the time
`AbortOnError = true` so we usually abort after reporting the first
error.

Depends on #111602.

Author: ellishg

llvm/lib/CodeGen/MachineVerifier.cpp

@@ -77,8 +77,10 @@
 #include "llvm/Pass.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/ModRef.h"
+#include "llvm/Support/Mutex.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Target/TargetMachine.h"
 #include <algorithm>
@@ -93,21 +95,31 @@ using namespace llvm;
 
 namespace {
 
+/// Used the by the ReportedErrors class to guarantee only one error is reported
+/// at one time.
+static ManagedStatic<sys::SmartMutex<true>> ReportedErrorsLock;
+
 struct MachineVerifier {
   MachineVerifier(MachineFunctionAnalysisManager &MFAM, const char *b,
-                  raw_ostream *OS)
-      : MFAM(&MFAM), OS(OS ? *OS : nulls()), Banner(b) {}
+                  raw_ostream *OS, bool AbortOnError = true)
+      : MFAM(&MFAM), OS(OS ? *OS : nulls()), Banner(b),
+        ReportedErrs(AbortOnError) {}
 
-  MachineVerifier(Pass *pass, const char *b, raw_ostream *OS)
-      : PASS(pass), OS(OS ? *OS : nulls()), Banner(b) {}
+  MachineVerifier(Pass *pass, const char *b, raw_ostream *OS,
+                  bool AbortOnError = true)
+      : PASS(pass), OS(OS ? *OS : nulls()), Banner(b),
+        ReportedErrs(AbortOnError) {}
 
   MachineVerifier(const char *b, LiveVariables *LiveVars,
                   LiveIntervals *LiveInts, LiveStacks *LiveStks,
-                  SlotIndexes *Indexes, raw_ostream *OS)
+                  SlotIndexes *Indexes, raw_ostream *OS,
+                  bool AbortOnError = true)
       : OS(OS ? *OS : nulls()), Banner(b), LiveVars(LiveVars),
-        LiveInts(LiveInts), LiveStks(LiveStks), Indexes(Indexes) {}
+        LiveInts(LiveInts), LiveStks(LiveStks), Indexes(Indexes),
+        ReportedErrs(AbortOnError) {}
 
-  unsigned verify(const MachineFunction &MF);
+  /// \returns true if no problems were found.
+  bool verify(const MachineFunction &MF);
 
   MachineFunctionAnalysisManager *MFAM = nullptr;
   Pass *const PASS = nullptr;
@@ -120,8 +132,6 @@ struct MachineVerifier {
   const MachineRegisterInfo *MRI = nullptr;
   const RegisterBankInfo *RBI = nullptr;
 
-  unsigned foundErrors = 0;
-
   // Avoid querying the MachineFunctionProperties for each operand.
   bool isFunctionRegBankSelected = false;
   bool isFunctionSelected = false;
@@ -231,6 +241,44 @@ struct MachineVerifier {
   LiveStacks *LiveStks = nullptr;
   SlotIndexes *Indexes = nullptr;
 
+  /// A class to track the number of reported error and to guarantee that only
+  /// one error is reported at one time.
+  class ReportedErrors {
+    unsigned NumReported = 0;
+    bool AbortOnError;
+
+  public:
+    /// \param AbortOnError -- If set, abort after printing the first error.
+    ReportedErrors(bool AbortOnError) : AbortOnError(AbortOnError) {}
+
+    ~ReportedErrors() {
+      if (!hasError())
+        return;
+      if (AbortOnError)
+        report_fatal_error("Found " + Twine(NumReported) +
+                           " machine code errors.");
+      // Since we haven't aborted, release the lock to allow other threads to
+      // report errors.
+      ReportedErrorsLock->unlock();
+    }
+
+    /// Increment the number of reported errors.
+    /// \returns true if this is the first reported error.
+    bool increment() {
+      // If this is the first error this thread has encountered, grab the lock
+      // to prevent other threads from reporting errors at the same time.
+      // Otherwise we assume we already have the lock.
+      if (!hasError())
+        ReportedErrorsLock->lock();
+      ++NumReported;
+      return NumReported == 1;
+    }
+
+    /// \returns true if an error was reported.
+    bool hasError() { return NumReported; }
+  };
+  ReportedErrors ReportedErrs;
+
   // This is calculated only when trying to verify convergence control tokens.
   // Similar to the LLVM IR verifier, we calculate this locally instead of
   // relying on the pass manager.
@@ -337,11 +385,7 @@ struct MachineVerifierLegacyPass : public MachineFunctionPass {
             MachineFunctionProperties::Property::FailsVerification))
       return false;
 
-    unsigned FoundErrors =
-        MachineVerifier(this, Banner.c_str(), &errs()).verify(MF);
-    if (FoundErrors)
-      report_fatal_error("Found " + Twine(FoundErrors) +
-                         " machine code errors.");
+    MachineVerifier(this, Banner.c_str(), &errs()).verify(MF);
     return false;
   }
 };
@@ -357,10 +401,7 @@ MachineVerifierPass::run(MachineFunction &MF,
   if (MF.getProperties().hasProperty(
           MachineFunctionProperties::Property::FailsVerification))
     return PreservedAnalyses::all();
-  unsigned FoundErrors =
-      MachineVerifier(MFAM, Banner.c_str(), &errs()).verify(MF);
-  if (FoundErrors)
-    report_fatal_error("Found " + Twine(FoundErrors) + " machine code errors.");
+  MachineVerifier(MFAM, Banner.c_str(), &errs()).verify(MF);
   return PreservedAnalyses::all();
 }
 
@@ -380,31 +421,20 @@ void llvm::verifyMachineFunction(const std::string &Banner,
   // LiveIntervals *LiveInts;
   // LiveStacks *LiveStks;
   // SlotIndexes *Indexes;
-  unsigned FoundErrors =
-      MachineVerifier(nullptr, Banner.c_str(), &errs()).verify(MF);
-  if (FoundErrors)
-    report_fatal_error("Found " + Twine(FoundErrors) + " machine code errors.");
+  MachineVerifier(nullptr, Banner.c_str(), &errs()).verify(MF);
 }
 
 bool MachineFunction::verify(Pass *p, const char *Banner, raw_ostream *OS,
-                             bool AbortOnErrors) const {
-  MachineFunction &MF = const_cast<MachineFunction&>(*this);
-  unsigned FoundErrors = MachineVerifier(p, Banner, OS).verify(MF);
-  if (AbortOnErrors && FoundErrors)
-    report_fatal_error("Found "+Twine(FoundErrors)+" machine code errors.");
-  return FoundErrors == 0;
+                             bool AbortOnError) const {
+  return MachineVerifier(p, Banner, OS, AbortOnError).verify(*this);
 }
 
 bool MachineFunction::verify(LiveIntervals *LiveInts, SlotIndexes *Indexes,
                              const char *Banner, raw_ostream *OS,
-                             bool AbortOnErrors) const {
-  MachineFunction &MF = const_cast<MachineFunction &>(*this);
-  unsigned FoundErrors =
-      MachineVerifier(Banner, nullptr, LiveInts, nullptr, Indexes, OS)
-          .verify(MF);
-  if (AbortOnErrors && FoundErrors)
-    report_fatal_error("Found " + Twine(FoundErrors) + " machine code errors.");
-  return FoundErrors == 0;
+                             bool AbortOnError) const {
+  return MachineVerifier(Banner, /*LiveVars=*/nullptr, LiveInts,
+                         /*LiveStks=*/nullptr, Indexes, OS, AbortOnError)
+      .verify(*this);
 }
 
 void MachineVerifier::verifySlotIndexes() const {
@@ -430,9 +460,7 @@ void MachineVerifier::verifyProperties(const MachineFunction &MF) {
     report("Function has NoVRegs property but there are VReg operands", &MF);
 }
 
-unsigned MachineVerifier::verify(const MachineFunction &MF) {
-  foundErrors = 0;
-
+bool MachineVerifier::verify(const MachineFunction &MF) {
   this->MF = &MF;
   TM = &MF.getTarget();
   TII = MF.getSubtarget().getInstrInfo();
@@ -447,7 +475,7 @@ unsigned MachineVerifier::verify(const MachineFunction &MF) {
   // it's expected that the MIR is somewhat broken but that's ok since we'll
   // reset it and clear the FailedISel attribute in ResetMachineFunctions.
   if (isFunctionFailedISel)
-    return foundErrors;
+    return true;
 
   isFunctionRegBankSelected = MF.getProperties().hasProperty(
       MachineFunctionProperties::Property::RegBankSelected);
@@ -544,13 +572,13 @@ unsigned MachineVerifier::verify(const MachineFunction &MF) {
   regMasks.clear();
   MBBInfoMap.clear();
 
-  return foundErrors;
+  return !ReportedErrs.hasError();
 }
 
 void MachineVerifier::report(const char *msg, const MachineFunction *MF) {
   assert(MF);
   OS << '\n';
-  if (!foundErrors++) {
+  if (ReportedErrs.increment()) {
     if (Banner)
       OS << "# " << Banner << '\n';
 

llvm/test/MachineVerifier/test_multiple_errors.mir

@@ -0,0 +1,22 @@
+# RUN: not --crash llc -mtriple=aarch64 -o /dev/null -run-pass=none %s 2>&1 | FileCheck %s --implicit-check-not="Bad machine code"
+# REQUIRES: aarch64-registered-target
+
+# Since we abort after reporting the first error, we should only expect one error to be reported.
+# CHECK: *** Bad machine code: Generic virtual register use cannot be undef ***
+# CHECK: Found 1 machine code errors.
+
+---
+name:            foo
+liveins:
+body:             |
+  bb.0:
+    $x0 = COPY undef %0:_(s64)
+...
+
+---
+name:            bar
+liveins:
+body:             |
+  bb.0:
+    $x0 = COPY undef %0:_(s64)
+...