[ownership] Change the ownership verifier textual error dumper to emit error counts on a per function instead of global basis.

This will just make them easier to update over time since adding a new function
doesn't require one to renumber the /entire/ file... *face-palm*.

Originally the reason why I added this is b/c I need to ensure that we handle
all errors exactly once so making sure we control the exact amount of emitted
errors is important. I can still do this with the new approach by just doing
per-function max error counts. Thus I also in this commit added FileCheck
patterns that implemented this scheme so now we have everything.

Author: gottesmm

lib/SIL/Verifier/LinearLifetimeChecker.cpp

@@ -32,8 +32,6 @@
 
 using namespace swift;
 
-unsigned LinearLifetimeChecker::ErrorBuilder::errorMessageCount = 0;
-
 //===----------------------------------------------------------------------===//
 //                                Declarations
 //===----------------------------------------------------------------------===//
@@ -61,7 +59,7 @@ struct State {
   ///
   /// It also handles any asserts/messages that need to be emitted if we are
   /// supposed to fail hard.
-  LinearLifetimeChecker::ErrorBuilder errorBuilder;
+  LinearLifetimeChecker::ErrorBuilder &errorBuilder;
 
   /// The blocks that we have already visited.
   SmallPtrSetImpl<SILBasicBlock *> &visitedBlocks;
@@ -92,7 +90,7 @@ struct State {
   SmallSetVector<SILBasicBlock *, 8> successorBlocksThatMustBeVisited;
 
   State(SILValue value, SmallPtrSetImpl<SILBasicBlock *> &visitedBlocks,
-        LinearLifetimeChecker::ErrorBuilder errorBuilder,
+        LinearLifetimeChecker::ErrorBuilder &errorBuilder,
         Optional<function_ref<void(SILBasicBlock *)>> leakingBlockCallback,
         ArrayRef<Operand *> consumingUses, ArrayRef<Operand *> nonConsumingUses)
       : value(value), beginInst(value->getDefiningInsertionPoint()),
@@ -578,7 +576,7 @@ LinearLifetimeChecker::Error LinearLifetimeChecker::checkValueImpl(
       });
     }
 
-    return std::move(state.errorBuilder).getFinalError();
+    return std::move(state.errorBuilder).consumeAndGetFinalError();
   }
 
   // Ok, we may have multiple consuming uses. Add the user block of each of our
@@ -613,7 +611,7 @@ LinearLifetimeChecker::Error LinearLifetimeChecker::checkValueImpl(
   // ...and then check that the end state shows that we have a valid linear
   // typed value.
   state.checkDataflowEndState(deadEndBlocks);
-  return std::move(state.errorBuilder).getFinalError();
+  return std::move(state.errorBuilder).consumeAndGetFinalError();
 }
 
 LinearLifetimeChecker::Error LinearLifetimeChecker::checkValue(

lib/SIL/Verifier/LinearLifetimeCheckerPrivate.h

@@ -79,36 +79,64 @@ class LLVM_LIBRARY_VISIBILITY LinearLifetimeChecker::ErrorBuilder {
   StringRef functionName;
   ErrorBehaviorKind behavior;
   Optional<Error> error;
-
-  // NOTE: This is only here so that we can emit a unique id for all errors to
-  // ease working with FileCheck.
-  static unsigned errorMessageCount;
+  unsigned *errorMessageCounter;
 
 public:
   ErrorBuilder(const SILFunction &fn,
-               LinearLifetimeChecker::ErrorBehaviorKind behavior)
-      : functionName(fn.getName()), behavior(behavior), error(Error()) {}
+               LinearLifetimeChecker::ErrorBehaviorKind behavior,
+               unsigned *errorMessageCounter = nullptr)
+      : functionName(fn.getName()), behavior(behavior), error(Error()),
+        errorMessageCounter(errorMessageCounter) {}
 
   ErrorBuilder(const SILFunction &fn,
-               LinearLifetimeChecker::ErrorBehaviorKind::inner_t behavior)
-      : functionName(fn.getName()), behavior(behavior), error(Error()) {}
+               LinearLifetimeChecker::ErrorBehaviorKind::inner_t behavior,
+               unsigned *errorMessageCounter = nullptr)
+      : ErrorBuilder(fn, LinearLifetimeChecker::ErrorBehaviorKind(behavior),
+                     errorMessageCounter) {}
+
+  ErrorBuilder(const ErrorBuilder &other)
+      : functionName(other.functionName), behavior(other.behavior),
+        error(other.error), errorMessageCounter(other.errorMessageCounter) {}
+
+  ErrorBuilder &operator=(const ErrorBuilder &other) {
+    functionName = other.functionName;
+    behavior = other.behavior;
+    error = other.error;
+    errorMessageCounter = other.errorMessageCounter;
+    return *this;
+  }
 
-  Error getFinalError() && {
+  Error consumeAndGetFinalError() && {
     auto result = *error;
     error = None;
+    errorMessageCounter = nullptr;
     return result;
   }
 
+  void tryDumpErrorCounter() const {
+    if (!errorMessageCounter) {
+      return;
+    }
+    llvm::errs() << "Error#: " << *errorMessageCounter << ". ";
+  }
+
+  void tryIncrementErrorCounter() {
+    if (!errorMessageCounter) {
+      return;
+    }
+    ++(*errorMessageCounter);
+  }
+
   bool handleLeak(llvm::function_ref<void()> &&messagePrinterFunc) {
     error->foundLeak = true;
 
     if (behavior.shouldPrintMessage()) {
-      llvm::errs() << "Error#: " << errorMessageCount
-                   << ". Begin Error in Function: '" << functionName << "'\n";
+      tryDumpErrorCounter();
+      llvm::errs() << "Begin Error in Function: '" << functionName << "'\n";
       messagePrinterFunc();
-      llvm::errs() << "Error#: " << errorMessageCount
-                   << ". End Error in Function: '" << functionName << "'\n";
-      ++errorMessageCount;
+      tryDumpErrorCounter();
+      llvm::errs() << "End Error in Function: '" << functionName << "'\n";
+      tryIncrementErrorCounter();
     }
 
     if (behavior.shouldReturnFalseOnLeak()) {
@@ -139,14 +167,15 @@ class LLVM_LIBRARY_VISIBILITY LinearLifetimeChecker::ErrorBuilder {
                    bool quiet = false) const {
     if (behavior.shouldPrintMessage()) {
       if (!quiet) {
-        llvm::errs() << "Error#: " << errorMessageCount
-                     << ". Begin Error in Function: '" << functionName << "'\n";
+        tryDumpErrorCounter();
+        llvm::errs() << "Begin Error in Function: '" << functionName << "'\n";
       }
       messagePrinterFunc();
       if (!quiet) {
-        llvm::errs() << "Error#: " << errorMessageCount
-                     << ". End Error in Function: '" << functionName << "'\n";
-        ++errorMessageCount;
+        tryDumpErrorCounter();
+        llvm::errs() << "End Error in Function: '" << functionName << "'\n";
+        auto *self = const_cast<ErrorBuilder *>(this);
+        self->tryIncrementErrorCounter();
       }
     }
 

lib/SIL/Verifier/SILOwnershipVerifier.cpp

@@ -89,7 +89,7 @@ class SILValueOwnershipChecker {
 
   /// The builder that the checker uses to emit error messages, crash if asked
   /// for, or supply back interesting info to the caller.
-  LinearLifetimeChecker::ErrorBuilder errorBuilder;
+  LinearLifetimeChecker::ErrorBuilder &errorBuilder;
 
   /// The list of lifetime ending users that we found. Only valid if check is
   /// successful.
@@ -113,7 +113,7 @@ class SILValueOwnershipChecker {
 public:
   SILValueOwnershipChecker(
       DeadEndBlocks &deadEndBlocks, SILValue value,
-      LinearLifetimeChecker::ErrorBuilder errorBuilder,
+      LinearLifetimeChecker::ErrorBuilder &errorBuilder,
       llvm::SmallPtrSetImpl<SILBasicBlock *> &visitedBlocks)
       : result(), deadEndBlocks(deadEndBlocks), value(value),
         errorBuilder(errorBuilder), visitedBlocks(visitedBlocks) {
@@ -882,8 +882,10 @@ void SILInstruction::verifyOperandOwnership() const {
   }
 }
 
-static void verifySILValueHelper(const SILFunction *f, SILValue value,
-                                 DeadEndBlocks *deadEndBlocks) {
+static void
+verifySILValueHelper(const SILFunction *f, SILValue value,
+                     LinearLifetimeChecker::ErrorBuilder &errorBuilder,
+                     DeadEndBlocks *deadEndBlocks) {
   assert(!isa<SILUndef>(value) &&
          "We assume we are always passed arguments or instruction results");
 
@@ -892,21 +894,13 @@ static void verifySILValueHelper(const SILFunction *f, SILValue value,
   if (!f->hasOwnership() || !f->shouldVerifyOwnership())
     return;
 
-  using BehaviorKind = LinearLifetimeChecker::ErrorBehaviorKind;
-  Optional<LinearLifetimeChecker::ErrorBuilder> errorBuilder;
-  if (IsSILOwnershipVerifierTestingEnabled) {
-    errorBuilder.emplace(*f, BehaviorKind::PrintMessageAndReturnFalse);
-  } else {
-    errorBuilder.emplace(*f, BehaviorKind::PrintMessageAndAssert);
-  }
-
   SmallPtrSet<SILBasicBlock *, 32> liveBlocks;
   if (deadEndBlocks) {
-    SILValueOwnershipChecker(*deadEndBlocks, value, *errorBuilder, liveBlocks)
+    SILValueOwnershipChecker(*deadEndBlocks, value, errorBuilder, liveBlocks)
         .check();
   } else {
     DeadEndBlocks deadEndBlocks(f);
-    SILValueOwnershipChecker(deadEndBlocks, value, *errorBuilder, liveBlocks)
+    SILValueOwnershipChecker(deadEndBlocks, value, errorBuilder, liveBlocks)
         .check();
   }
 }
@@ -951,7 +945,11 @@ void SILValue::verifyOwnership(DeadEndBlocks *deadEndBlocks) const {
   // a real function. Assert in case this assumption is no longer true.
   auto *f = (*this)->getFunction();
   assert(f && "Instructions and arguments should have a function");
-  verifySILValueHelper(f, *this, deadEndBlocks);
+
+  using BehaviorKind = LinearLifetimeChecker::ErrorBehaviorKind;
+  LinearLifetimeChecker::ErrorBuilder errorBuilder(
+      *f, BehaviorKind::PrintMessageAndAssert);
+  verifySILValueHelper(f, *this, errorBuilder, deadEndBlocks);
 }
 
 void SILFunction::verifyOwnership(DeadEndBlocks *deadEndBlocks) const {
@@ -976,14 +974,26 @@ void SILFunction::verifyOwnership(DeadEndBlocks *deadEndBlocks) const {
   if (!hasOwnership() || !shouldVerifyOwnership())
     return;
 
+  using BehaviorKind = LinearLifetimeChecker::ErrorBehaviorKind;
+  unsigned errorCounter = 0;
+  Optional<LinearLifetimeChecker::ErrorBuilder> errorBuilder;
+  if (IsSILOwnershipVerifierTestingEnabled) {
+    errorBuilder.emplace(*this, BehaviorKind::PrintMessageAndReturnFalse,
+                         &errorCounter);
+  } else {
+    errorBuilder.emplace(*this, BehaviorKind::PrintMessageAndAssert);
+  }
+
   for (auto &block : *this) {
     for (auto *arg : block.getArguments()) {
-      verifySILValueHelper(this, arg, deadEndBlocks);
+      LinearLifetimeChecker::ErrorBuilder newBuilder = *errorBuilder;
+      verifySILValueHelper(this, arg, newBuilder, deadEndBlocks);
     }
 
     for (auto &inst : block) {
       for (auto result : inst.getResults()) {
-        verifySILValueHelper(this, result, deadEndBlocks);
+        LinearLifetimeChecker::ErrorBuilder newBuilder = *errorBuilder;
+        verifySILValueHelper(this, result, newBuilder, deadEndBlocks);
       }
     }
   }