[move-only] Teach the borrow to destructure transform how to handle consuming/non-consuming uses on the same instruction.

NOTE: The additional errors that are occuring in the move only object checker is
b/c I tweaked checkDestructureUsesOnBoundary so that when it detects an error it
continues instead of returns. This ensures that we get more that we emit errors
for multiple violations instead of just the first one.

rdar://104900171

Author: gottesmm

include/swift/AST/DiagnosticsSIL.def

@@ -727,6 +727,8 @@ ERROR(noimplicitcopy_used_on_generic_or_existential, none,
 // move only checker diagnostics
 ERROR(sil_moveonlychecker_owned_value_consumed_more_than_once, none,
       "'%0' consumed more than once", (StringRef))
+ERROR(sil_moveonlychecker_owned_value_consumed_and_used_at_same_time, none,
+      "'%0' consumed and used at the same time", (StringRef))
 ERROR(sil_moveonlychecker_value_used_after_consume, none,
       "'%0' used after consume. Lifetime extension of variable requires a copy", (StringRef))
 ERROR(sil_moveonlychecker_guaranteed_value_consumed, none,
@@ -752,6 +754,10 @@ NOTE(sil_moveonlychecker_boundary_use, none,
      "boundary use here", ())
 NOTE(sil_moveonlychecker_consuming_use_here, none,
      "consuming use", ())
+NOTE(sil_moveonlychecker_two_consuming_uses_here, none,
+     "two consuming uses here", ())
+NOTE(sil_moveonlychecker_consuming_and_non_consuming_uses_here, none,
+     "consuming and non-consuming uses here", ())
 NOTE(sil_moveonlychecker_consuming_closure_use_here, none,
      "closure capture", ())
 NOTE(sil_moveonlychecker_nonconsuming_use_here, none,

lib/SILOptimizer/Mandatory/MoveOnlyBorrowToDestructureTransform.cpp

@@ -28,18 +28,20 @@
 #include "MoveOnlyDiagnostics.h"
 #include "MoveOnlyObjectChecker.h"
 
+#include "swift/Basic/BlotSetVector.h"
 #include "swift/Basic/Defer.h"
+#include "swift/Basic/FrozenMultiMap.h"
+#include "swift/SIL/FieldSensitivePrunedLiveness.h"
 #include "swift/SIL/SILBuilder.h"
 #include "swift/SIL/SILInstruction.h"
 #include "swift/SILOptimizer/Analysis/Analysis.h"
 #include "swift/SILOptimizer/Analysis/DeadEndBlocksAnalysis.h"
-#include "swift/Basic/BlotSetVector.h"
-#include "swift/Basic/FrozenMultiMap.h"
 #include "swift/SILOptimizer/Analysis/PostOrderAnalysis.h"
 #include "swift/SILOptimizer/PassManager/Passes.h"
 #include "swift/SILOptimizer/PassManager/Transforms.h"
 #include "swift/SILOptimizer/Utils/CFGOptUtils.h"
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/SmallBitVector.h"
 
 using namespace swift;
 using namespace swift::siloptimizer;
@@ -143,6 +145,7 @@ bool BorrowToDestructureTransform::gatherUses(
           {nextUse, {*leafRange, false /*is lifetime ending*/}});
       liveness.updateForUse(nextUse->getUser(), *leafRange,
                             false /*is lifetime ending*/);
+      instToInterestingOperandIndexMap.insert(nextUse->getUser(), nextUse);
       continue;
     }
 
@@ -166,6 +169,7 @@ bool BorrowToDestructureTransform::gatherUses(
                           {nextUse, {*leafRange, true /*is lifetime ending*/}});
       liveness.updateForUse(nextUse->getUser(), *leafRange,
                             true /*is lifetime ending*/);
+      instToInterestingOperandIndexMap.insert(nextUse->getUser(), nextUse);
       continue;
     }
 
@@ -198,34 +202,144 @@ bool BorrowToDestructureTransform::gatherUses(
   return true;
 }
 
+void BorrowToDestructureTransform::checkForErrorsOnSameInstruction() {
+  // At this point, we have emitted all boundary checks. We also now need to
+  // check if any of our consuming uses that are on the boundary are used by the
+  // same instruction as a different consuming or non-consuming use.
+  instToInterestingOperandIndexMap.setFrozen();
+  SmallBitVector usedBits(liveness.getNumSubElements());
+
+  for (auto instRangePair : instToInterestingOperandIndexMap.getRange()) {
+    SWIFT_DEFER { usedBits.reset(); };
+
+    // First loop through our uses and handle any consuming twice errors. We
+    // also setup usedBits to check for non-consuming uses that may overlap.
+    Operand *badOperand = nullptr;
+    Optional<TypeTreeLeafTypeRange> badRange;
+    for (auto *use : instRangePair.second) {
+      if (!use->isConsuming())
+        continue;
+
+      auto destructureUseSpan = *TypeTreeLeafTypeRange::get(use->get(), mmci);
+      for (unsigned index : destructureUseSpan.getRange()) {
+        if (usedBits[index]) {
+          // If we get that we used the same bit twice, we have an error. We set
+          // the badIndex error and break early.
+          badOperand = use;
+          badRange = destructureUseSpan;
+          break;
+        }
+
+        usedBits[index] = true;
+      }
+
+      // If we set badOperand, break so we can emit an error for this
+      // instruction.
+      if (badOperand)
+        break;
+    }
+
+    // If we did not set badIndex for consuming uses, we did not have any
+    // conflicts among consuming uses. see if we have any conflicts with
+    // non-consuming uses. Otherwise, we continue.
+    if (!badOperand) {
+      for (auto *use : instRangePair.second) {
+        if (use->isConsuming())
+          continue;
+
+        auto destructureUseSpan = *TypeTreeLeafTypeRange::get(use->get(), mmci);
+        for (unsigned index : destructureUseSpan.getRange()) {
+          if (!usedBits[index])
+            continue;
+
+          // If we get that we used the same bit twice, we have an error. We set
+          // the badIndex error and break early.
+          badOperand = use;
+          badRange = destructureUseSpan;
+          break;
+        }
+
+        // If we set badOperand, break so we can emit an error for this
+        // instruction.
+        if (badOperand)
+          break;
+      }
+
+      // If we even did not find a non-consuming use that conflicts, then
+      // continue.
+      if (!badOperand)
+        continue;
+    }
+
+    // If badIndex is set, we broke out of the inner loop and need to emit an
+    // error. Use a little more compile time to identify the other operand that
+    // caused the failure. NOTE: badOperand /could/ be a non-consuming use, but
+    // the use we are identifying here will always be consuming.
+    usedBits.reset();
+
+    // Reinitialize use bits with the bad bits.
+    for (unsigned index : badRange->getRange())
+      usedBits[index] = true;
+
+    // Now loop back through looking for the original operand that set the used
+    // bits. This will always be a consuming use.
+    for (auto *use : instRangePair.second) {
+      if (!use->isConsuming())
+        continue;
+
+      auto destructureUseSpan = *TypeTreeLeafTypeRange::get(use->get(), mmci);
+      bool emittedError = false;
+      for (unsigned index : destructureUseSpan.getRange()) {
+        if (!usedBits[index])
+          continue;
+
+        if (badOperand->isConsuming())
+          diagnosticEmitter.emitObjectConsumesDestructuredValueTwice(
+              mmci, use, badOperand);
+        else
+          diagnosticEmitter.emitObjectConsumesAndUsesDestructuredValue(
+              mmci, use, badOperand);
+        emittedError = true;
+      }
+
+      // Once we have emitted the error, just break out of the loop.
+      if (emittedError)
+        break;
+    }
+  }
+}
+
 void BorrowToDestructureTransform::checkDestructureUsesOnBoundary() const {
   LLVM_DEBUG(llvm::dbgs() << "Checking destructure uses on boundary!\n");
+
   // Now that we have found all of our destructure needing uses and liveness
   // needing uses, make sure that none of our destructure needing uses are
-  // within our boundary. If so, we have an automatic error.
+  // within our boundary. If so, we have an automatic error since we have a
+  // use-after-free.
   for (auto *use : destructureNeedingUses) {
     LLVM_DEBUG(llvm::dbgs()
                << "    DestructureNeedingUse: " << *use->getUser());
+
     auto destructureUseSpan = *TypeTreeLeafTypeRange::get(use->get(), mmci);
-    if (liveness.isWithinBoundary(use->getUser(), destructureUseSpan)) {
-      LLVM_DEBUG(llvm::dbgs() << "        Within boundary! Emitting error!\n");
-      // Emit an error. We have a use after free.
-      //
-      // NOTE: Since we are going to emit an error here, we do the boundary
-      // computation to ensure that we only do the boundary computation once:
-      // when we emit an error or once we know we need to do rewriting.
-      //
-      // TODO: Fix diagnostic to use destructure needing use and boundary
-      // uses.
-      FieldSensitivePrunedLivenessBoundary boundary(
-          liveness.getNumSubElements());
-      liveness.computeBoundary(boundary);
-      diagnosticEmitter.emitObjectDestructureNeededWithinBorrowBoundary(
-          mmci, use->getUser(), destructureUseSpan, boundary);
-      return;
-    } else {
-      LLVM_DEBUG(llvm::dbgs() << "        On boundary! No error!\n");
+    if (!liveness.isWithinBoundary(use->getUser(), destructureUseSpan)) {
+      LLVM_DEBUG(llvm::dbgs()
+                 << "        On boundary or within boundary! No error!\n");
+      continue;
     }
+
+    // Emit an error. We have a use after free.
+    //
+    // NOTE: Since we are going to emit an error here, we do the boundary
+    // computation to ensure that we only do the boundary computation once:
+    // when we emit an error or once we know we need to do rewriting.
+    //
+    // TODO: Fix diagnostic to use destructure needing use and boundary
+    // uses.
+    LLVM_DEBUG(llvm::dbgs() << "        Within boundary! Emitting error!\n");
+    FieldSensitivePrunedLivenessBoundary boundary(liveness.getNumSubElements());
+    liveness.computeBoundary(boundary);
+    diagnosticEmitter.emitObjectDestructureNeededWithinBorrowBoundary(
+        mmci, use->getUser(), destructureUseSpan, boundary);
   }
 }
 

lib/SILOptimizer/Mandatory/MoveOnlyBorrowToDestructureTransformTester.cpp

@@ -106,6 +106,19 @@ static bool runTransform(SILFunction *fn,
       continue;
     }
 
+    // Then check if we had two consuming uses on the same instruction or a
+    // consuming/non-consuming use on the same isntruction.
+    transform.checkForErrorsOnSameInstruction();
+
+    // If we emitted any diagnostic, set madeChange to true, eliminate our mmci,
+    // and continue.
+    if (currentDiagnosticCount != diagnosticEmitter.getDiagnosticCount()) {
+      mmci->replaceAllUsesWith(mmci->getOperand());
+      mmci->eraseFromParent();
+      madeChange = true;
+      continue;
+    }
+
     // At this point, we know that all of our destructure requiring uses are on
     // the boundary of our live range. Now we need to do the rewriting.
     transform.blockToAvailableValues.emplace(transform.liveness);

lib/SILOptimizer/Mandatory/MoveOnlyDiagnostics.cpp

@@ -453,3 +453,56 @@ void DiagnosticEmitter::emitObjectDestructureNeededWithinBorrowBoundary(
   }
   registerDiagnosticEmitted(markedValue);
 }
+
+void DiagnosticEmitter::emitObjectConsumesDestructuredValueTwice(
+    MarkMustCheckInst *markedValue, Operand *firstUse, Operand *secondUse) {
+  assert(firstUse->getUser() == secondUse->getUser());
+  assert(firstUse->isConsuming());
+  assert(secondUse->isConsuming());
+
+  LLVM_DEBUG(
+      llvm::dbgs() << "Emitting object consumes destructure twice error!\n");
+  LLVM_DEBUG(llvm::dbgs() << "    Mark: " << *markedValue);
+  LLVM_DEBUG(llvm::dbgs() << "    User: " << *firstUse->getUser());
+  LLVM_DEBUG(llvm::dbgs() << "    First Conflicting Operand: "
+                          << firstUse->getOperandNumber() << '\n');
+  LLVM_DEBUG(llvm::dbgs() << "    Second Conflicting Operand: "
+                          << secondUse->getOperandNumber() << '\n');
+
+  auto &astContext = markedValue->getModule().getASTContext();
+  StringRef varName = getVariableNameForValue(markedValue);
+  diagnose(astContext,
+           markedValue->getDefiningInstruction()->getLoc().getSourceLoc(),
+           diag::sil_moveonlychecker_owned_value_consumed_more_than_once,
+           varName);
+  diagnose(astContext, firstUse->getUser()->getLoc().getSourceLoc(),
+           diag::sil_moveonlychecker_two_consuming_uses_here);
+  registerDiagnosticEmitted(markedValue);
+}
+
+void DiagnosticEmitter::emitObjectConsumesAndUsesDestructuredValue(
+    MarkMustCheckInst *markedValue, Operand *consumingUse,
+    Operand *nonConsumingUse) {
+  assert(consumingUse->getUser() == nonConsumingUse->getUser());
+  assert(consumingUse->isConsuming());
+  assert(!nonConsumingUse->isConsuming());
+
+  LLVM_DEBUG(
+      llvm::dbgs() << "Emitting object consumes destructure twice error!\n");
+  LLVM_DEBUG(llvm::dbgs() << "    Mark: " << *markedValue);
+  LLVM_DEBUG(llvm::dbgs() << "    User: " << *consumingUse->getUser());
+  LLVM_DEBUG(llvm::dbgs() << "    Consuming Operand: "
+                          << consumingUse->getOperandNumber() << '\n');
+  LLVM_DEBUG(llvm::dbgs() << "    Non Consuming Operand: "
+                          << nonConsumingUse->getOperandNumber() << '\n');
+
+  auto &astContext = markedValue->getModule().getASTContext();
+  StringRef varName = getVariableNameForValue(markedValue);
+  diagnose(astContext,
+           markedValue->getDefiningInstruction()->getLoc().getSourceLoc(),
+           diag::sil_moveonlychecker_owned_value_consumed_and_used_at_same_time,
+           varName);
+  diagnose(astContext, consumingUse->getUser()->getLoc().getSourceLoc(),
+           diag::sil_moveonlychecker_consuming_and_non_consuming_uses_here);
+  registerDiagnosticEmitted(markedValue);
+}

lib/SILOptimizer/Mandatory/MoveOnlyDiagnostics.h

@@ -91,6 +91,14 @@ class DiagnosticEmitter {
       TypeTreeLeafTypeRange destructureNeededBits,
       FieldSensitivePrunedLivenessBoundary &boundary);
 
+  void emitObjectConsumesDestructuredValueTwice(MarkMustCheckInst *markedValue,
+                                                Operand *firstConsumingUse,
+                                                Operand *secondConsumingUse);
+  void
+  emitObjectConsumesAndUsesDestructuredValue(MarkMustCheckInst *markedValue,
+                                             Operand *consumingUse,
+                                             Operand *nonConsumingUse);
+
 private:
   /// Emit diagnostics for the final consuming uses and consuming uses needing
   /// copy. If filter is non-null, allow for the caller to pre-process operands

lib/SILOptimizer/Mandatory/MoveOnlyObjectChecker.cpp

@@ -387,6 +387,18 @@ bool MoveOnlyChecker::convertBorrowExtractsToOwnedDestructures(
   unsigned diagnosticCount = diagnosticEmitter.getDiagnosticCount();
   transform.checkDestructureUsesOnBoundary();
 
+  // If we emitted any diagnostic, break out. We return true since we actually
+  // succeeded in our processing by finding the error. We only return false if
+  // we want to tell the rest of the checker that there was an internal
+  // compiler error that we need to emit a "compiler doesn't understand
+  // error".
+  if (diagnosticCount != diagnosticEmitter.getDiagnosticCount())
+    return true;
+
+  // Then check if we had two consuming uses on the same instruction or a
+  // consuming/non-consuming use on the same isntruction.
+  transform.checkForErrorsOnSameInstruction();
+
   // If we emitted any diagnostic, break out. We return true since we actually
   // succeeded in our processing by finding the error. We only return false if
   // we want to tell the rest of the checker that there was an internal

lib/SILOptimizer/Mandatory/MoveOnlyObjectChecker.h

@@ -20,6 +20,7 @@
 #ifndef SWIFT_SILOPTIMIZER_MANDATORY_MOVEONLYOBJECTCHECKER_H
 #define SWIFT_SILOPTIMIZER_MANDATORY_MOVEONLYOBJECTCHECKER_H
 
+#include "swift/Basic/FrozenMultiMap.h"
 #include "swift/SIL/FieldSensitivePrunedLiveness.h"
 #include "swift/SILOptimizer/Utils/CanonicalizeOSSALifetime.h"
 #include "llvm/ADT/IntervalMap.h"
@@ -188,6 +189,11 @@ struct BorrowToDestructureTransform {
   SmallFrozenMultiMap<SILBasicBlock *, std::pair<Operand *, InterestingUser>, 8>
       blocksToUses;
 
+  /// A frozen multi-map we use to diagnose consuming uses that are used by the
+  /// same instruction as another consuming use or non-consuming use.
+  SmallFrozenMultiMap<SILInstruction *, Operand *, 8>
+      instToInterestingOperandIndexMap;
+
   BorrowToDestructureTransform(
       IntervalMapAllocator &allocator, MarkMustCheckInst *mmci,
       DiagnosticEmitter &diagnosticEmitter, PostOrderAnalysis *poa,
@@ -221,6 +227,10 @@ struct BorrowToDestructureTransform {
   /// transform.
   void checkDestructureUsesOnBoundary() const;
 
+  /// Check for cases where we have two consuming uses on the same instruction
+  /// or a consuming/non-consuming use on the same instruction.
+  void checkForErrorsOnSameInstruction();
+
   /// Rewrite all of the uses of our borrow on our borrow operand, performing
   /// destructures as appropriate.
   void rewriteUses();