Fix BorrowingOperand.visitScopeEndingUses

Only return false if the visitor returns false. Clients were ignoring the
result.

If the BorrowingOperand does not create a borrow scope, call visitUnknownUse
instead.

Until we have complete lifetimes, to avoid breaking code that cannot handle dead
defs, consider a dead borrow scope to be an unknown use.

Author: atrick

SwiftCompilerSources/Sources/Optimizer/Utilities/LifetimeDependenceUtils.swift

@@ -778,7 +778,7 @@ extension LifetimeDependenceDefUseWalker {
     case .beginApply:
       // Skip the borrow scope; the type system enforces non-escapable
       // arguments.
-      return visitInnerBorrowUses(of: borrowInst)
+      return visitInnerBorrowUses(of: borrowInst, operand: operand)
     case .partialApply, .markDependence:
       fatalError("OwnershipUseVisitor should bypass partial_apply [on_stack] "
                  + "and mark_dependence [nonescaping]")

SwiftCompilerSources/Sources/Optimizer/Utilities/OwnershipLiveness.swift

@@ -321,13 +321,16 @@ extension OwnershipUseVisitor {
     }
   }
 
-  /// Visit only those uses of a value within an inner borrow scope
-  /// that may affect the outer lifetime. An inner borrow scope is one
-  /// in which the borrowing operand is itself a use of the outer
-  /// lifetime, including: begin_borrow, reborrow, partial_apply,
-  /// mark_dependence, or an inner adjacent phi (where original SSA
-  /// def is a phi in the same block).
-  mutating func visitInnerScopeUses(of: Value) -> WalkResult {
+  /// Visit only those uses of a value within an inner borrow scope that may affect the outer lifetime. An inner borrow
+  /// scope is one in which the borrowing operand is itself a use of the outer lifetime, including: begin_borrow,
+  /// reborrow, borrowed_from, partial_apply, mark_dependence, or an inner adjacent phi (where original SSA def is a phi
+  /// in the same block).
+  ///
+  /// An owned lifetime may also be considered an inner borrow scope if it depends on a borrowed operand, such as a
+  /// closure capture or owned mark_dependence.
+  ///
+  /// Precondition: BeginBorrowValue(value) != nil || value.ownership == .owned
+  mutating func visitInnerScopeUses(of value: Value) -> WalkResult {
     if let beginBorrow = BeginBorrowValue(value) {
       return beginBorrow.scopeEndingOperands.walk {
         switch $0.ownership {
@@ -367,10 +370,14 @@ extension OwnershipUseVisitor {
     // TODO: remove this stack by changing visitScopeEndingOperands to take a non-escaping closure.
     var stack = Stack<Operand>(context)
     defer { stack.deinitialize() }
-    _ = borrowInst.visitScopeEndingOperands(context) {
+    let result = borrowInst.visitScopeEndingOperands(context) {
       stack.push($0)
       return .continueWalk
     }
+    guard result == .continueWalk else {
+      // If the dependent value is not scoped then consider it a pointer escape.
+      return pointerEscapingUse(of: operand)
+    }
     return stack.walk { ownershipLeafUse(of: $0, isInnerLifetime: true) }
   }
 }
@@ -640,7 +647,7 @@ extension InteriorUseWalker: OwnershipUseVisitor {
         return visitAllUses(of: beginBorrow.value)
       }
     }
-    return visitInnerScopeUses(of: borrowInst)
+    return visitInnerBorrowUses(of: borrowInst, operand: operand)
   }
 
   // Visit a reborrow operand. This ends an outer lifetime and extends

include/swift/SIL/OwnershipUtils.h

@@ -365,6 +365,10 @@ struct BorrowingOperand {
   /// For an instantaneous borrow, such as apply, this visits no uses. For
   /// begin_apply it visits the end_apply uses. For borrow introducers, it
   /// visits the end of the introduced borrow scope.
+  ///
+  /// For borrows that don't introduce a separate borrow scope, this calls
+  /// visitUnknownUse on the current operand. The client may need to check each
+  /// unknown operand to avoid infinite recursion.
   bool visitScopeEndingUses(function_ref<bool(Operand *)> visitScopeEnd,
                             function_ref<bool(Operand *)> visitUnknownUse
                             = [](Operand *){ return false; })

lib/SIL/Utils/OwnershipUtils.cpp

@@ -297,23 +297,19 @@ bool swift::findInnerTransitiveGuaranteedUses(
       //
       // FIXME: visit[Extended]ScopeEndingUses can't return false here once dead
       // borrows are disallowed.
-      if (!BorrowingOperand(use).visitScopeEndingUses(
-            [&](Operand *endUse) {
-              if (endUse->getOperandOwnership() == OperandOwnership::Reborrow) {
-                foundPointerEscape = true;
-              }
-              leafUse(endUse);
-              return true;
-            },
-            [&](Operand *unknownUse) {
-              foundPointerEscape = true;
-              leafUse(unknownUse);
-              return true;
-            })) {
-        // Special case for dead borrows. This is dangerous because clients
-        // don't expect a begin_borrow to be in the use list.
-        leafUse(use);
-      }
+      BorrowingOperand(use).visitScopeEndingUses(
+        [&](Operand *endUse) {
+          if (endUse->getOperandOwnership() == OperandOwnership::Reborrow) {
+            foundPointerEscape = true;
+          }
+          leafUse(endUse);
+          return true;
+        },
+        [&](Operand *unknownUse) {
+          foundPointerEscape = true;
+          leafUse(unknownUse);
+          return true;
+        });
       break;
     }
   }
@@ -676,10 +672,12 @@ bool BorrowingOperand::visitScopeEndingUses(
           return false;
       }
     }
-    // FIXME: special case for dead borrows. This is dangerous because clients
-    // only expect visitScopeEndingUses to return false if the visitor returned
-    // false.
-    return !deadBorrow;
+    // Note: special case for dead borrows. This is dangerous because it could
+    // cause unsuspecting clients to infinitely recurse.
+    if (deadBorrow) {
+      return visitUnknownUse(op);
+    }
+    return true;
   }
   case BorrowingOperandKind::StoreBorrow: {
     bool deadBorrow = true;
@@ -690,10 +688,12 @@ bool BorrowingOperand::visitScopeEndingUses(
           return false;
       }
     }
-    // FIXME: special case for dead borrows. This is dangerous because clients
-    // only expect visitScopeEndingUses to return false if the visitor returned
-    // false.
-    return !deadBorrow;
+    // Note: special case for dead borrows. This is dangerous because it could
+    // cause unsuspecting clients to infinitely recurse.
+    if (deadBorrow) {
+      return visitUnknownUse(op);
+    }
+    return true;
   }
   case BorrowingOperandKind::BeginApply: {
     bool deadApply = true;
@@ -733,7 +733,10 @@ bool BorrowingOperand::visitScopeEndingUses(
         return false;
       }
     }
-    return !dead;
+    if (dead) {
+      return visitUnknownUse(op);
+    }
+    return true;
   }
   // These are instantaneous borrow scopes so there aren't any special end
   // scope instructions.
@@ -751,7 +754,10 @@ bool BorrowingOperand::visitScopeEndingUses(
           return false;
       }
     }
-    return !deadBranch;
+    if (deadBranch) {
+      return visitUnknownUse(op);
+    }
+    return true;
   }
   }
   llvm_unreachable("Covered switch isn't covered");

lib/SIL/Utils/PrunedLiveness.cpp

@@ -278,21 +278,18 @@ PrunedLiveRange<LivenessWithDefs>::updateForBorrowingOperand(Operand *operand) {
   // Note: Ownership liveness should follow reborrows that are dominated by the
   // ownership definition.
   auto innerBorrowKind = InnerBorrowKind::Contained;
-  if (!BorrowingOperand(operand).visitScopeEndingUses(
-        [&](Operand *end) {
-          if (end->getOperandOwnership() == OperandOwnership::Reborrow) {
-            innerBorrowKind = InnerBorrowKind::Reborrowed;
-          }
-          updateForUse(end->getUser(), /*lifetimeEnding*/ false);
-          return true;
-        }, [&](Operand *unknownUse) {
-          updateForUse(unknownUse->getUser(), /*lifetimeEnding*/ false);
-          innerBorrowKind = InnerBorrowKind::Escaped;
-          return true;
-        })) {
-    // Handle dead borrows.
-    updateForUse(operand->getUser(), /*lifetimeEnding*/ false);
-  }
+  BorrowingOperand(operand).visitScopeEndingUses(
+    [&](Operand *end) {
+      if (end->getOperandOwnership() == OperandOwnership::Reborrow) {
+        innerBorrowKind = InnerBorrowKind::Reborrowed;
+      }
+      updateForUse(end->getUser(), /*lifetimeEnding*/ false);
+      return true;
+    }, [&](Operand *unknownUse) {
+      updateForUse(unknownUse->getUser(), /*lifetimeEnding*/ false);
+      innerBorrowKind = InnerBorrowKind::Escaped;
+      return true;
+    });
   return innerBorrowKind;
 }
 

lib/SIL/Verifier/SILOwnershipVerifier.cpp

@@ -277,10 +277,23 @@ bool SILValueOwnershipChecker::gatherNonGuaranteedUsers(
 
     // If our scoped operand is not also a borrow introducer, then we know that
     // we do not need to consider guaranteed phis and thus can just add the
-    // initial end scope instructions without any further work.
-    //
-    // Maybe: Is borrow scope non-local?
-    initialScopedOperand.getImplicitUses(nonLifetimeEndingUsers);
+    // initial end scope instructions without any further work. This avoids
+    // cubic complexity in the verifier.
+    auto addLeafUse = [&](Operand *endOp) {
+      nonLifetimeEndingUsers.push_back(endOp);
+      return true;
+    };
+    auto addUnknownUse = [&](Operand *endOp) {
+      // FIXME: This ignores mark_dependence base uses and borrowed_from base
+      // uses. Instead, recursively call gatherUsers.
+      // Note: we may have op == endOp.
+      nonLifetimeEndingUsers.push_back(endOp);
+      return true;
+    };
+    // FIXME: This ignores dead-end blocks. Instead, recursively call
+    // gatherUsers.
+    initialScopedOperand.visitScopeEndingUses(addLeafUse, addUnknownUse);
+
     if (initialScopedOperand.kind == BorrowingOperandKind::BeginBorrow) {
       guaranteedPhiVerifier.verifyReborrows(
           cast<BeginBorrowInst>(op->getUser()));
@@ -391,7 +404,21 @@ bool SILValueOwnershipChecker::gatherUsers(
       if (auto scopedOperand = BorrowingOperand(op)) {
         assert(!scopedOperand.isReborrow());
 
-        scopedOperand.getImplicitUses(nonLifetimeEndingUsers);
+        auto addLeafUse = [&](Operand *endOp) {
+          nonLifetimeEndingUsers.push_back(endOp);
+          return true;
+        };
+        auto addUnknownUse = [&](Operand *endOp) {
+          // FIXME: This ignores mark_dependence base uses and borrowed_from
+          // base uses. Instead, recursively call gatherUsers.  Note: we may
+          // have op == endOp.
+          nonLifetimeEndingUsers.push_back(endOp);
+          return true;
+        };
+        // FIXME: This ignores dead-end blocks. Instead, recursively call
+        // gatherUsers.
+        scopedOperand.visitScopeEndingUses(addLeafUse, addUnknownUse);
+
         if (scopedOperand.kind == BorrowingOperandKind::BeginBorrow) {
           guaranteedPhiVerifier.verifyReborrows(
               cast<BeginBorrowInst>(op->getUser()));

lib/SILOptimizer/Mandatory/MoveOnlyBorrowToDestructureUtils.cpp

@@ -477,6 +477,9 @@ bool Implementation::gatherUses(SILValue value) {
                               false /*is lifetime ending*/);
       }
       // The liveness extends to the scope-ending uses of the borrow.
+      //
+      // FIXME: this ignores visitScopeEndingUses failure, which may result from
+      // unknown uses or dead borrows.
       BorrowingOperand(nextUse).visitScopeEndingUses([&](Operand *end) -> bool {
         if (end->getOperandOwnership() == OperandOwnership::Reborrow) {
           return false;
@@ -1090,6 +1093,7 @@ static void insertEndBorrowsForNonConsumingUse(Operand *op,
     LLVM_DEBUG(llvm::dbgs() << "    -- Ending borrow after borrow scope:\n"
                                "    ";
                op->getUser()->print(llvm::dbgs()));
+    // FIXME: ignoring the visitScopeEndingUses result ignores unknown uses.
     bOp.visitScopeEndingUses([&](Operand *endScope) -> bool {
       auto *endScopeInst = endScope->getUser();
       LLVM_DEBUG(llvm::dbgs() << "       ";
@@ -1107,6 +1111,7 @@ static void insertEndBorrowsForNonConsumingUse(Operand *op,
     // End the borrow where the original borrow of the subject was ended.
     // TODO: handle if the switch isn't directly on a borrow?
     auto beginBorrow = cast<BeginBorrowInst>(swi->getOperand());
+    // FIXME: ignoring the visitScopeEndingUses result ignores unknown uses.
     BorrowingOperand(&beginBorrow->getOperandRef())
       .visitScopeEndingUses([&](Operand *endScope) -> bool {
         auto *endScopeInst = endScope->getUser();