Merge pull request #62668 from atrick/fix-recursive-access

Do not insert dynamic exclusivity checks in closure cycles

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()
+}