Merge pull request #63607 from gottesmm/pr-07ebd2119844b7d0b1be15f9a72f66f06c7cc9cc

[move-only] Teach the checker how to handle ref_element_addr and global_addr with assignable_but_not_consumable semantics

Author: gottesmm

include/swift/AST/DiagnosticsSIL.def

@@ -748,6 +748,25 @@ ERROR(sil_moveonlychecker_exclusivity_violation, none,
 ERROR(sil_moveonlychecker_moveonly_field_consumed, none,
       "'%0' has a move only field that was consumed before later uses", (StringRef))
 
+ERROR(sil_moveonlychecker_notconsumable_but_assignable_was_consumed_classfield_let, none,
+      "'%0' was consumed but it is illegal to consume a noncopyable class let field. One can only read from it",
+      (StringRef))
+ERROR(sil_moveonlychecker_notconsumable_but_assignable_was_consumed_classfield_var, none,
+      "'%0' was consumed but it is illegal to consume a noncopyable class var field. One can only read from it or assign to it",
+      (StringRef))
+ERROR(sil_moveonlychecker_notconsumable_but_assignable_was_consumed_global_var, none,
+      "'%0' was consumed but it is illegal to consume a noncopyable global var. One can only read from it or assign to it",
+      (StringRef))
+ERROR(sil_moveonlychecker_notconsumable_but_assignable_was_consumed_global_let, none,
+      "'%0' was consumed but it is illegal to consume a noncopyable global let. One can only read from it",
+      (StringRef))
+ERROR(sil_moveonlychecker_notconsumable_but_assignable_was_consumed_escaping_let, none,
+      "'%0' was consumed but it is illegal to consume a noncopyable escaping immutable capture. One can only read from it",
+      (StringRef))
+ERROR(sil_moveonlychecker_notconsumable_but_assignable_was_consumed_escaping_var, none,
+      "'%0' was consumed but it is illegal to consume a noncopyable escaping mutable capture. One can only read from it or assign over it",
+      (StringRef))
+
 NOTE(sil_moveonlychecker_moveonly_field_consumed_here, none,
      "move only field consumed here", ())
 NOTE(sil_moveonlychecker_boundary_use, none,

lib/SILGen/SILGenGlobalVariable.cpp

@@ -100,14 +100,22 @@ SILGenFunction::emitGlobalVariableRef(SILLocation loc, VarDecl *var,
     return ManagedValue::forLValue(addr);
   }
 
-  // Global variables can be accessed directly with global_addr.  Emit this
-  // instruction into the prolog of the function so we can memoize/CSE it in
-  // VarLocs.
+  // Global variables can be accessed directly with global_addr. If we have a
+  // noncopyable type, just emit the global_addr so each individual access has
+  // its own base projection. This is important so that the checker can
+  // distinguish in between different accesses to the same global.
+  auto *silG = SGM.getSILGlobalVariable(var, NotForDefinition);
+  if (silG->getLoweredType().isMoveOnly()) {
+    SILValue addr = B.createGlobalAddr(var, silG);
+    return ManagedValue::forLValue(addr);
+  }
+
+  // If we have a copyable type, emit this instruction into the prolog of the
+  // function so we can memoize/CSE it via the VarLocs map.
   auto *entryBB = &*getFunction().begin();
   SILGenBuilder prologueB(*this, entryBB, entryBB->begin());
   prologueB.setTrackingList(B.getTrackingList());
 
-  auto *silG = SGM.getSILGlobalVariable(var, NotForDefinition);
   SILValue addr = prologueB.createGlobalAddr(var, silG);
 
   VarLocs[var] = SILGenFunction::VarLoc::get(addr);

lib/SILGen/SILGenLValue.cpp

@@ -788,6 +788,17 @@ namespace {
         SGF.B.createRefElementAddr(loc, base.getUnmanagedValue(),
                                    Field, SubstFieldType);
 
+      // If we have a move only type...
+      if (result->getType().isMoveOnly()) {
+        auto checkKind =
+            MarkMustCheckInst::CheckKind::AssignableButNotConsumable;
+        if (isReadAccess(getAccessKind())) {
+          // Add a mark_must_check [no_consume_or_assign].
+          checkKind = MarkMustCheckInst::CheckKind::NoConsumeOrAssign;
+        }
+        result = SGF.B.createMarkMustCheckInst(loc, result, checkKind);
+      }
+
       // Avoid emitting access markers completely for non-accesses or immutable
       // declarations. Access marker verification is aware of these cases.
       if (!IsNonAccessing && !Field->isLet()) {
@@ -2977,6 +2988,14 @@ void LValue::addNonMemberVarComponent(SILGenFunction &SGF, SILLocation loc,
       // The only other case that should get here is a global variable.
       if (!address) {
         address = SGF.emitGlobalVariableRef(Loc, Storage, ActorIso);
+        if (address.getType().isMoveOnly()) {
+            auto checkKind =
+                MarkMustCheckInst::CheckKind::AssignableButNotConsumable;
+            if (isReadAccess(AccessKind)) {
+              checkKind = MarkMustCheckInst::CheckKind::NoConsumeOrAssign;
+            }
+            address = SGF.B.createMarkMustCheckInst(Loc, address, checkKind);
+        }
       } else {
         assert((!ActorIso || Storage->isTopLevelGlobal()) &&
                "local var should not be actor isolated!");

lib/SILOptimizer/Mandatory/MoveOnlyAddressChecker.cpp

@@ -541,16 +541,19 @@ void UseState::initializeLiveness(
     FieldSensitiveMultiDefPrunedLiveRange &liveness) {
   // We begin by initializing all of our init uses.
   for (auto initInstAndValue : initInsts) {
+    LLVM_DEBUG(llvm::dbgs() << "Found def: " << *initInstAndValue.first);
     liveness.initializeDef(initInstAndValue.first, initInstAndValue.second);
   }
 
   // If we have a reinitInstAndValue that we are going to be able to convert
   // into a simple init, add it as an init. We are going to consider the rest of
   // our reinit uses to be liveness uses.
   for (auto reinitInstAndValue : reinitInsts) {
-    if (isReinitToInitConvertibleInst(reinitInstAndValue.first))
+    if (isReinitToInitConvertibleInst(reinitInstAndValue.first)) {
+      LLVM_DEBUG(llvm::dbgs() << "Found def: " << *reinitInstAndValue.first);
       liveness.initializeDef(reinitInstAndValue.first,
                              reinitInstAndValue.second);
+    }
   }
 
   // Then check if our markedValue is from an argument that is in,
@@ -603,6 +606,24 @@ void UseState::initializeLiveness(
     }
   }
 
+  // Check if our address is from a ref_element_addr. In such a case, we treat
+  // the mark_must_check as the initialization.
+  if (auto *refEltAddr = dyn_cast<RefElementAddrInst>(address->getOperand())) {
+    LLVM_DEBUG(llvm::dbgs() << "Found ref_element_addr use... "
+                               "adding mark_must_check as init!\n");
+    initInsts.insert({address, liveness.getTopLevelSpan()});
+    liveness.initializeDef(address, liveness.getTopLevelSpan());
+  }
+
+  // Check if our address is from a global_addr. In such a case, we treat the
+  // mark_must_check as the initialization.
+  if (auto *globalAddr = dyn_cast<GlobalAddrInst>(address->getOperand())) {
+    LLVM_DEBUG(llvm::dbgs() << "Found global_addr use... "
+                               "adding mark_must_check as init!\n");
+    initInsts.insert({address, liveness.getTopLevelSpan()});
+    liveness.initializeDef(address, liveness.getTopLevelSpan());
+  }
+
   // Now that we have finished initialization of defs, change our multi-maps
   // from their array form to their map form.
   liveness.finishedInitializationOfDefs();
@@ -1157,15 +1178,17 @@ bool GatherUsesVisitor::visitUse(Operand *op, AccessUseType useTy) {
       // Canonicalize the lifetime of the load [take], load [copy].
       moveChecker.changed |= moveChecker.canonicalizer.canonicalize(li);
 
-      // If we are asked to perform guaranteed checking, emit an error if we
-      // have /any/ consuming uses. This is a case that can always be converted
-      // to a load_borrow if we pass the check.
+      // If we are asked to perform no_consume_or_assign checking or
+      // assignable_but_not_consumable checking, if we found any consumes of our
+      // load, then we need to emit an error.
       if (markedValue->getCheckKind() ==
-          MarkMustCheckInst::CheckKind::NoConsumeOrAssign) {
-        if (!moveChecker.canonicalizer.foundAnyConsumingUses()) {
+              MarkMustCheckInst::CheckKind::NoConsumeOrAssign ||
+          markedValue->getCheckKind() ==
+              MarkMustCheckInst::CheckKind::AssignableButNotConsumable) {
+        if (moveChecker.canonicalizer.foundAnyConsumingUses()) {
           LLVM_DEBUG(llvm::dbgs()
                      << "Found mark must check [nocopy] error: " << *user);
-          moveChecker.diagnosticEmitter.emitObjectGuaranteedDiagnostic(
+          moveChecker.diagnosticEmitter.emitAddressInstLoadedAndConsumed(
               markedValue);
           emittedEarlyDiagnostic = true;
           return true;
@@ -1856,14 +1879,18 @@ void MoveOnlyChecker::rewriteUses(
         // destroy_value and use then to create a new load_borrow scope.
         SILBuilderWithScope builder(li);
         auto *lbi = builder.createLoadBorrow(li->getLoc(), li->getOperand());
-
+        // We use this auxillary list to avoid iterator invalidation of
+        // li->getConsumingUse();
+        StackList<DestroyValueInst *> toDelete(lbi->getFunction());
         for (auto *consumeUse : li->getConsumingUses()) {
           auto *dvi = cast<DestroyValueInst>(consumeUse->getUser());
           SILBuilderWithScope destroyBuilder(dvi);
           destroyBuilder.createEndBorrow(dvi->getLoc(), lbi);
-          dvi->eraseFromParent();
+          toDelete.push_back(dvi);
           changed = true;
         }
+        while (!toDelete.empty())
+          toDelete.pop_back_val()->eraseFromParent();
 
         li->replaceAllUsesWith(lbi);
         li->eraseFromParent();
@@ -1903,7 +1930,7 @@ bool MoveOnlyChecker::performSingleCheck(MarkMustCheckInst *markedAddress) {
                             diagnosticEmitter, gatherUsesLiveness);
   SWIFT_DEFER { visitor.clear(); };
   visitor.reset(markedAddress);
-  if (!visitAccessPathUses(visitor, accessPath, fn)) {
+  if (!visitAccessPathBaseUses(visitor, accessPathWithBase, fn)) {
     LLVM_DEBUG(llvm::dbgs() << "Failed access path visit: " << *markedAddress);
     return false;
   }