Merge pull request swiftlang#29860 from gottesmm/pr-2c1006ea5973927c581d0cea0268e77ae33bc076

Author: swift-ci

lib/SIL/SILOwnershipVerifier.cpp

@@ -127,13 +127,17 @@ class SILValueOwnershipChecker {
 
 private:
   bool checkUses();
-  bool isCompatibleDefUse(Operand *op, ValueOwnershipKind ownershipKind,
-                          bool isGuaranteed);
+  bool isCompatibleDefUse(Operand *op, ValueOwnershipKind ownershipKind);
 
   bool gatherUsers(SmallVectorImpl<Operand *> &lifetimeEndingUsers,
                    SmallVectorImpl<Operand *> &regularUsers,
                    SmallVectorImpl<Operand *> &implicitRegularUsers);
 
+  bool
+  gatherNonGuaranteedUsers(SmallVectorImpl<Operand *> &lifetimeEndingUsers,
+                           SmallVectorImpl<Operand *> &regularUsers,
+                           SmallVectorImpl<Operand *> &implicitRegularUsers);
+
   bool checkValueWithoutLifetimeEndingUses();
 
   bool checkFunctionArgWithoutLifetimeEndingUses(SILFunctionArgument *arg);
@@ -188,9 +192,10 @@ bool SILValueOwnershipChecker::check() {
 }
 
 bool SILValueOwnershipChecker::isCompatibleDefUse(
-    Operand *op, ValueOwnershipKind ownershipKind, bool isGuaranteed) {
+    Operand *op, ValueOwnershipKind ownershipKind) {
   bool isGuaranteedSubValue = false;
-  if (isGuaranteed && isGuaranteedForwardingInst(op->getUser())) {
+  if (ownershipKind == ValueOwnershipKind::Guaranteed &&
+      isGuaranteedForwardingInst(op->getUser())) {
     isGuaranteedSubValue = true;
   }
   auto *user = op->getUser();
@@ -230,6 +235,64 @@ bool SILValueOwnershipChecker::isCompatibleDefUse(
   return false;
 }
 
+bool SILValueOwnershipChecker::gatherNonGuaranteedUsers(
+    SmallVectorImpl<Operand *> &lifetimeEndingUsers,
+    SmallVectorImpl<Operand *> &nonLifetimeEndingUsers,
+    SmallVectorImpl<Operand *> &implicitRegularUsers) {
+  bool foundError = false;
+
+  auto ownershipKind = value.getOwnershipKind();
+  bool isOwned = ownershipKind == ValueOwnershipKind::Owned;
+
+  // Since we are dealing with a non-guaranteed user, we do not have to recurse.
+  for (auto *op : value->getUses()) {
+    auto *user = op->getUser();
+
+    // If this op is a type dependent operand, skip it. It is not interesting
+    // from an ownership perspective.
+    if (user->isTypeDependentOperand(*op))
+      continue;
+
+    // First check if this recursive use is compatible with our values ownership
+    // kind. If not, flag the error and continue so that we can report more
+    // errors.
+    if (!isCompatibleDefUse(op, ownershipKind)) {
+      foundError = true;
+      continue;
+    }
+
+    // First do a quick check if we have a consuming use. If so, stash the value
+    // and continue.
+    if (op->isConsumingUse()) {
+      LLVM_DEBUG(llvm::dbgs() << "        Lifetime Ending User: " << *user);
+      lifetimeEndingUsers.push_back(op);
+      continue;
+    }
+
+    // Otherwise, we have a non lifetime ending user. Add it to our non lifetime
+    // ending user list.
+    LLVM_DEBUG(llvm::dbgs() << "        Regular User: " << *user);
+    nonLifetimeEndingUsers.push_back(op);
+
+    // If we do not have an owned value at this point, continue, we do not have
+    // any further work to do.
+    if (!isOwned) {
+      continue;
+    }
+
+    // Otherwise, check if we have a borrow scope operand. In such a case, add
+    // the borrow scope operand's end scope instructions as implicit regular
+    // users so we can ensure that the borrow scope operand's scope is
+    // completely within our value's scope.
+    if (auto scopedOperand = BorrowScopeOperand::get(op)) {
+      scopedOperand->visitEndScopeInstructions(
+          [&](Operand *op) { implicitRegularUsers.push_back(op); });
+    }
+  }
+
+  return foundError;
+}
+
 bool SILValueOwnershipChecker::gatherUsers(
     SmallVectorImpl<Operand *> &lifetimeEndingUsers,
     SmallVectorImpl<Operand *> &nonLifetimeEndingUsers,
@@ -239,14 +302,22 @@ bool SILValueOwnershipChecker::gatherUsers(
   // we need to look through subobject uses for more uses. Otherwise, if we are
   // forwarding, we do not create any lifetime ending users/non lifetime ending
   // users since we verify against our base.
-  auto ownershipKind = value.getOwnershipKind();
-  bool isGuaranteed = ownershipKind == ValueOwnershipKind::Guaranteed;
-  bool isOwned = ownershipKind == ValueOwnershipKind::Owned;
+  if (value.getOwnershipKind() != ValueOwnershipKind::Guaranteed) {
+    return !gatherNonGuaranteedUsers(
+        lifetimeEndingUsers, nonLifetimeEndingUsers, implicitRegularUsers);
+  }
 
-  if (isGuaranteed && isGuaranteedForwardingValue(value))
+  // Ok, we have a value with guarantee ownership. Before we continue, check if
+  // this value forwards guaranteed ownership. In such a case, we are going to
+  // validate it as part of the borrow introducer from which the forwarding
+  // value originates. So we can just return true and continue.
+  if (isGuaranteedForwardingValue(value))
     return true;
 
-  // Then gather up our initial list of users.
+  // Ok, we have some sort of borrow introducer. We need to recursively validate
+  // that all of its uses (including sub-scopes) are before any end_borrows that
+  // end the lifetime of the borrow introducer. With that in mind, gather up our
+  // initial list of users.
   SmallVector<Operand *, 8> users;
   llvm::copy(value->getUses(), std::back_inserter(users));
 
@@ -263,7 +334,7 @@ bool SILValueOwnershipChecker::gatherUsers(
     // First check if this recursive use is compatible with our values ownership
     // kind. If not, flag the error and continue so that we can report more
     // errors.
-    if (!isCompatibleDefUse(op, ownershipKind, isGuaranteed)) {
+    if (!isCompatibleDefUse(op, ValueOwnershipKind::Guaranteed)) {
       foundError = true;
       continue;
     }
@@ -276,30 +347,6 @@ bool SILValueOwnershipChecker::gatherUsers(
       nonLifetimeEndingUsers.push_back(op);
     }
 
-    // If our base value is not guaranteed, we do not to try to visit
-    // subobjects.
-    if (!isGuaranteed) {
-      // But if we are owned, check if we have any end_borrows. We
-      // need to treat these as sub-scope users. We can rely on the
-      // end_borrow to prevent recursion.
-      if (isOwned) {
-        // Do a check if any of our users are begin_borrows. If we find such a
-        // use, then we want to include the end_borrow associated with the
-        // begin_borrow in our NonLifetimeEndingUser lists.
-        //
-        // For correctness reasons we use indices to make sure that we can
-        // append to NonLifetimeEndingUsers without needing to deal with
-        // iterator invalidation.
-        for (auto *op : nonLifetimeEndingUsers) {
-          if (auto scopedOperand = BorrowScopeOperand::get(op)) {
-            scopedOperand->visitEndScopeInstructions(
-                [&](Operand *op) { implicitRegularUsers.push_back(op); });
-          }
-        }
-      }
-      continue;
-    }
-
     // If we are guaranteed, but are not a guaranteed forwarding inst, we add
     // the end scope instructions of any new sub-scopes. This ensures that the
     // parent scope completely encloses the child borrow scope.
@@ -371,7 +418,8 @@ bool SILValueOwnershipChecker::gatherUsers(
           // needing to be verified. If it isn't verified appropriately, assert
           // when the verifier is destroyed.
           auto succArgOwnershipKind = succArg->getOwnershipKind();
-          if (!succArgOwnershipKind.isCompatibleWith(ownershipKind)) {
+          if (!succArgOwnershipKind.isCompatibleWith(
+                  ValueOwnershipKind::Guaranteed)) {
             // This is where the error would go.
             continue;
           }
@@ -416,7 +464,8 @@ bool SILValueOwnershipChecker::gatherUsers(
         // needing to be verified. If it isn't verified appropriately, assert
         // when the verifier is destroyed.
         auto succArgOwnershipKind = succArg->getOwnershipKind();
-        if (!succArgOwnershipKind.isCompatibleWith(ownershipKind)) {
+        if (!succArgOwnershipKind.isCompatibleWith(
+                ValueOwnershipKind::Guaranteed)) {
           // This is where the error would go.
           continue;
         }

test/SIL/ownership-verifier/arguments.sil

@@ -322,7 +322,7 @@ bb7:
 // CHECK-LABEL: Function: 'owned_argument_overuse_br1'
 // CHECK-NEXT: Found over consume?!
 // CHECK-NEXT: Value: %0 = argument of bb0 : $Builtin.NativeObject
-// CHECK-NEXT: User:   br bb1(%0 : $Builtin.NativeObject)
+// CHECK-NEXT: User:   destroy_value %0 : $Builtin.NativeObject
 // CHECK-NEXT: Block: bb0
 
 // CHECK-LABEL: Function: 'owned_argument_overuse_br1'
@@ -343,7 +343,7 @@ bb1(%1 : @owned $Builtin.NativeObject):
 // CHECK-LABEL: Function: 'owned_argument_overuse_br2'
 // CHECK-NEXT: Found over consume?!
 // CHECK-NEXT: Value: %0 = argument of bb0 : $Builtin.NativeObject
-// CHECK-NEXT: User:   br bb1(%0 : $Builtin.NativeObject)
+// CHECK-NEXT: User:   destroy_value %0 : $Builtin.NativeObject
 // CHECK-NEXT: Block: bb0
 // CHECK-NOT: Block
 // CHECK-NOT: Function: 'owned_argument_overuse_br2'

test/SIL/ownership-verifier/cond_br_crash.sil

@@ -39,7 +39,7 @@ bb2:
 // CHECK-LABEL: Function: 'test2'
 // CHECK: Found over consume?!
 // CHECK: Value:   %0 = argument of bb0 : $Builtin.NativeObject
-// CHECK: User:   destroy_value %0 : $Builtin.NativeObject
+// CHECK: User:   cond_br undef, bb1(%0 : $Builtin.NativeObject), bb2
 // CHECK: Block: bb1
 sil [ossa] @test2 : $@convention(thin) (@owned Builtin.NativeObject) -> () {
 bb0(%0 : @owned $Builtin.NativeObject):

test/SIL/ownership-verifier/over_consume.sil

@@ -182,7 +182,7 @@ bb0(%0 : @owned $RefWithInt):
 // CHECK-LABEL: Function: 'unchecked_enum_data_propagates_ownership'
 // CHECK: Found over consume?!
 // CHECK: Value: %0 = argument of bb0 : $Optional<Builtin.NativeObject>
-// CHECK: User: destroy_value %0 : $Optional<Builtin.NativeObject>
+// CHECK: User:   %1 = unchecked_enum_data %0 : $Optional<Builtin.NativeObject>, #Optional.some!enumelt.1
 // CHECK: Block: bb0
 sil [ossa] @unchecked_enum_data_propagates_ownership : $@convention(thin) (@owned Optional<Builtin.NativeObject>) -> () {
 bb0(%0 : @owned $Optional<Builtin.NativeObject>):