Do not insert dynamic exclusivity checks in closure cycles

Closure cycles were originally enforced "conservatively". Real code
does, however, use recursive local functions. And it's surprisingly
easy to create false exclusivity violations in those cases.

This is safe as long as we can rely on SILGen to keep captured
variables in boxes if the capture escapes in any closure that may call
other closures that capture the same variable.

Fixes #61404
Dynamic exclusivity checking bug with nested functions. #61404

Fixes rdar://102056143 (assertion failure due to exclusivity checker -
AccessEnforcementSelection should handle recursive local captures)

Author: atrick

lib/SILOptimizer/Mandatory/AccessEnforcementSelection.cpp

@@ -99,6 +99,15 @@ raw_ostream &operator<<(raw_ostream &os, const AddressCapture &capture) {
 
 // For each non-escaping closure, record the indices of arguments that
 // require dynamic enforcement.
+//
+// A note on closure cycles: local functions can be recursive, creating closure
+// cycles. DynamicCaptures ignores such cycles, simply processing the call graph
+// top-down. This relies on a simple rule: if a captured variable is passed as a
+// box a local function (presumably because the function escapes), then it must
+// also be passed as a box to any other local function called by the
+// first. Therefore, if any capture escapes in a closure cycle, then it must be
+// passed as a box in all closures within the cycle. DynamicCaptures does not
+// care about boxes, because they are always dynamically enforced.
 class DynamicCaptures {
   const ClosureFunctionOrder &closureOrder;
 
@@ -127,10 +136,10 @@ class DynamicCaptures {
   }
 
   bool isDynamic(SILFunctionArgument *arg) const {
-    // If the current function is a local function that directly or indirectly
-    // refers to itself, then conservatively assume dynamic enforcement.
-    if (closureOrder.isHeadOfClosureCycle(arg->getFunction()))
-      return true;
+    // This closure may be the head of a closure cycle. That's ok, because we
+    // only care about whether this argument escapes in the calling function
+    // this is *not* part of the cycle. If the capture escapes anywhere in the
+    // cycle, then it is passed as a box to all closures in that cycle.
 
     auto pos = dynamicCaptureMap.find(arg->getFunction());
     if (pos == dynamicCaptureMap.end())

test/Interpreter/enforce_exclusive_access.swift

@@ -383,4 +383,56 @@ ExclusiveAccessTestSuite.test("directlyAppliedEscapingConflict") {
   }()
 }
 
+// rdar://102056143 - recursive locals inside a mutating method
+// effectively capture self as inout even if they don't modify
+// self. This should not trigger a runtime trap.
+struct RecursiveLocalDuringMutation {
+  var flag: Bool = false
+
+  mutating func update() {
+    func local1(_ done: Bool) -> Bool {
+      if !done {
+        return local2()
+      }
+      return flag
+    }
+    func local2() -> Bool {
+      return local1(true)
+    }
+    flag = !local1(false)
+  }
+}
+
+ExclusiveAccessTestSuite.test("recursiveLocalDuringMutation") {
+  var v = RecursiveLocalDuringMutation()
+  v.update()
+  _blackHole(v.flag)
+}
+
+// rdar://102056143 - negative test. Make sure we still catch true
+// violations on recursive captures.
+struct RecursiveCaptureViolation {
+  func testBadness() -> Bool {
+    var flag = false
+    func local1(_ x: inout Bool) {
+      if !flag {
+        x = true
+        local2()
+      }
+    }
+    func local2() {
+      local1(&flag)
+    }
+    local2()
+    return flag
+  }
+}
+
+ExclusiveAccessTestSuite.test("recursiveCaptureViolation") {
+  var s = RecursiveCaptureViolation()
+  expectCrashLater()
+  s.testBadness()
+  _blackHole(s)
+}
+
 runAllTests()

test/SILOptimizer/access_enforcement_selection.swift

@@ -101,17 +101,16 @@ public func recaptureStack() -> Int {
 
 
 // -----------------------------------------------------------------------------
-// Inout access in a recursive closure currently has dynamic enforcement
-// because enforcement selection cannot visit all closure scopes before
-// selecting enforcement within the closure.
+// Inout access in a recursive non-escaping closure currently has
+// static enforcement.
 
 public protocol Apply {
     func apply(_ body: (Apply) -> ())
 }
 
 // CHECK-LABEL: sil private @$s28access_enforcement_selection20testRecursiveClosure1a1xyAA5Apply_p_SiztF9localFuncL_1byAaE_p_tF : $@convention(thin) (@in_guaranteed any Apply, @inout_aliasable Int) -> () {
 // CHECK: bb0(%0 : $*any Apply, %1 : @closureCapture $*Int):
-// CHECK:   begin_access [modify] [dynamic] %1 : $*Int
+// CHECK:   begin_access [modify] [static] %1 : $*Int
 // CHECK-LABEL: } // end sil function '$s28access_enforcement_selection20testRecursiveClosure1a1xyAA5Apply_p_SiztF9localFuncL_1byAaE_p_tF'
 public func testRecursiveClosure(a: Apply, x: inout Int) {
     func localFunc(b: Apply) {
@@ -123,3 +122,19 @@ public func testRecursiveClosure(a: Apply, x: inout Int) {
     }
     a.apply(localFunc)
 }
+
+// CHECK-LABEL: sil private @$s28access_enforcement_selection25testRecursiveLocalCapture1iys5Int64Vz_tF6local2L_yyF : $@convention(thin) (@inout_aliasable Int64) -> () {
+// CHECK: begin_access [modify] [static] %0 : $*Int64
+// CHECK-LABEL: } // end sil function '$s28access_enforcement_selection25testRecursiveLocalCapture1iys5Int64Vz_tF6local2L_yyF'
+func testRecursiveLocalCapture(i: inout Int64) {
+  func local1() {
+    if i < 1 {
+      local2()
+    }
+  }
+  func local2() {
+    i = i + 1
+    local1()
+  }
+  local1()
+}