Merge pull request #19173 from gottesmm/pr-0290cd4323dba0b496df6aaa44c48ee171f647fa

[sil-ownership] Tighten up the verification of guaranteed phi arguments.

Author: gottesmm

lib/SIL/LinearLifetimeChecker.cpp

@@ -148,9 +148,9 @@ void State::initializeAllNonConsumingUses(
     // dataflow, we only need the last non-consuming use to show that all
     // consuming uses post dominate both.
     //
-    // We begin by checking if the first use is a cond_br use from the previous
-    // block. In such a case, we always use the already stored value and
-    // continue.
+    // We begin by checking if the second use is a cond_br use from the previous
+    // block. In such a case, we always use the already stored value and since
+    // it is guaranteed to be later than the cond_br use.
     if (user.isCondBranchUser()) {
       continue;
     }
@@ -179,6 +179,7 @@ bool State::initializeAllConsumingUses(
     ArrayRef<BranchPropagatedUser> consumingUses,
     SmallVectorImpl<std::pair<BranchPropagatedUser, SILBasicBlock *>>
         &predsToAddToWorklist) {
+  bool noErrors = true;
   for (BranchPropagatedUser user : consumingUses) {
     SILBasicBlock *userBlock = user.getParent();
 
@@ -187,13 +188,13 @@ bool State::initializeAllConsumingUses(
     // if we successfully associated user with userBlock.
     if (!initializeConsumingUse(user, userBlock)) {
       // We already handled the error.
-      return handleError([] {});
+      noErrors &= handleError([] {});
     }
 
     // Then check if the given block has a use after free.
     if (checkForSameBlockUseAfterFree(user, userBlock)) {
       // We already handled the error.
-      return handleError([] {});
+      noErrors &= handleError([] {});
     }
 
     // If this user is in the same block as the value, do not visit
@@ -210,7 +211,7 @@ bool State::initializeAllConsumingUses(
     }
   }
 
-  return true;
+  return noErrors;
 }
 
 bool State::initializeConsumingUse(BranchPropagatedUser consumingUser,

lib/SIL/SILOwnershipVerifier.cpp

@@ -98,6 +98,8 @@ static bool isOwnershipForwardingValueKind(SILNodeKind K) {
   case SILNodeKind::SelectEnumInst:
   case SILNodeKind::SwitchEnumInst:
   case SILNodeKind::CheckedCastBranchInst:
+  case SILNodeKind::BranchInst:
+  case SILNodeKind::CondBranchInst:
   case SILNodeKind::DestructureStructInst:
   case SILNodeKind::DestructureTupleInst:
     return true;
@@ -795,6 +797,16 @@ OwnershipCompatibilityUseChecker::visitReturnInst(ReturnInst *RI) {
 
 OwnershipUseCheckerResult
 OwnershipCompatibilityUseChecker::visitEndBorrowInst(EndBorrowInst *I) {
+  // If we are checking a subobject, make sure that we are from a guaranteed
+  // basic block argument.
+  if (isCheckingSubObject()) {
+    auto *phiArg = cast<SILPHIArgument>(Op.get());
+    (void)phiArg;
+    assert(phiArg->getOwnershipKind() == ValueOwnershipKind::Guaranteed &&
+           "Expected an end_borrow paired with an argument.");
+    return {true, UseLifetimeConstraint::MustBeLive};
+  }
+
   /// An end_borrow is modeled as invalidating the guaranteed value preventing
   /// any further uses of the value.
   return {compatibleWithOwnership(ValueOwnershipKind::Guaranteed),
@@ -1438,6 +1450,7 @@ void SILValueOwnershipChecker::gatherUsers(
   // users since we verify against our base.
   auto OwnershipKind = Value.getOwnershipKind();
   bool IsGuaranteed = OwnershipKind == ValueOwnershipKind::Guaranteed;
+  bool IsOwned = OwnershipKind == ValueOwnershipKind::Owned;
 
   if (IsGuaranteed && isGuaranteedForwardingValue(Value))
     return;
@@ -1485,27 +1498,37 @@ void SILValueOwnershipChecker::gatherUsers(
       }
     }
 
-    // If our base value is not guaranteed or our intermediate value is not an
-    // ownership forwarding inst, continue. We do not want to visit any
-    // subobjects recursively.
-    if (!IsGuaranteed || !isGuaranteedForwardingInst(User)) {
-      // 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.
-      SmallVector<SILInstruction *, 4> endBorrowInsts;
-      for (unsigned i : indices(NonLifetimeEndingUsers)) {
-        if (auto *bbi = dyn_cast<BeginBorrowInst>(
-                NonLifetimeEndingUsers[i].getInst())) {
-          copy(bbi->getEndBorrows(), std::back_inserter(ImplicitRegularUsers));
+    // 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.
+        SmallVector<SILInstruction *, 4> endBorrowInsts;
+        for (unsigned i : indices(NonLifetimeEndingUsers)) {
+          if (auto *bbi = dyn_cast<BeginBorrowInst>(
+                  NonLifetimeEndingUsers[i].getInst())) {
+            copy(bbi->getEndBorrows(),
+                 std::back_inserter(ImplicitRegularUsers));
+          }
         }
       }
       continue;
     }
 
+    // If we are guaranteed, but are not a guaranteed forwarding inst,
+    // just continue. This user is just treated as a normal use.
+    if (!isGuaranteedForwardingInst(User))
+      continue;
+
     // At this point, we know that we must have a forwarded subobject. Since the
     // base type is guaranteed, we know that the subobject is either guaranteed
     // or trivial. We now split into two cases, if the user is a terminator or
@@ -1537,9 +1560,8 @@ void SILValueOwnershipChecker::gatherUsers(
     }
 
     // Otherwise if we have a terminator, add any as uses any end_borrow to
-    // ensure that the subscope is completely enclsed within the super
-    // scope. all of the arguments to the work list. We require all of our
-    // arguments to be either trivial or guaranteed.
+    // ensure that the subscope is completely enclsed within the super scope. We
+    // require all of our arguments to be either trivial or guaranteed.
     for (auto &Succ : TI->getSuccessors()) {
       auto *BB = Succ.getBB();
 
@@ -1552,7 +1574,7 @@ void SILValueOwnershipChecker::gatherUsers(
       //
       // TODO: We could ignore this error and emit a more specific error on the
       // actual terminator.
-      for (auto *BBArg : BB->getArguments()) {
+      for (auto *BBArg : BB->getPHIArguments()) {
         // *NOTE* We do not emit an error here since we want to allow for more
         // specific errors to be found during use_verification.
         //
@@ -1569,8 +1591,16 @@ void SILValueOwnershipChecker::gatherUsers(
         if (BBArgOwnershipKind == ValueOwnershipKind::Trivial)
           continue;
 
-        // Otherwise,
-        copy(BBArg->getUses(), std::back_inserter(Users));
+        // Otherwise add all end_borrow users for this BBArg to the
+        // implicit regular user list. We know that BBArg must be
+        // completely joint post-dominated by these users, so we use
+        // them to ensure that all of BBArg's uses are completely
+        // enclosed within the end_borrow of this argument.
+        for (auto *op : BBArg->getUses()) {
+          if (auto *ebi = dyn_cast<EndBorrowInst>(op->getUser())) {
+            ImplicitRegularUsers.push_back(ebi);
+          }
+        }
       }
     }
   }