[Exclusivity] Use dynamic enforcement on boxes whose projections escape

In AccessEnforcementSelection, treat passing a projection from a box to
a partial_apply as an escape to force using dynamic enforcement on the box.

This will now correctly use dynamic enforcement on variables that are taken
as inout and also captured by storage address in a closure:

  var x = ...

  x.mutatingMethod { ... use x ...}

but does pessimize some of our existing enforcement into dynamic since
access enforcement selection.

Ideally we would distinguish between escaping via an nonescaping closures
(which can only conflict with accesses that are in progress) and
escaping via escaping closures (which can conflict for any reachable code
after the escape)

Author: devincoughlin

lib/SILOptimizer/Mandatory/AccessEnforcementSelection.cpp

@@ -85,6 +85,10 @@ class SelectEnforcement {
   void analyzeUsesOfBox(SILInstruction *source);
   void analyzeProjection(ProjectBoxInst *projection);
 
+  /// Note that the given instruction is a use of the box (or a use of
+  /// a projection from it) in which the address escapes.
+  void noteEscapingUse(SILInstruction *inst);
+
   void propagateEscapes();
   void propagateEscapesFrom(SILBasicBlock *bb);
 
@@ -137,20 +141,7 @@ void SelectEnforcement::analyzeUsesOfBox(SILInstruction *source) {
       continue;
 
     // Treat everything else as an escape:
-
-    // Add it to the escapes set.
-    Escapes.insert(user);
-
-    // Record this point as escaping.
-    auto userBB = user->getParent();
-    auto &state = StateMap[userBB];
-    if (!state.IsInWorklist) {
-      state.HasEscape = true;
-      state.IsInWorklist = true;
-      Worklist.push_back(userBB);
-    }
-    assert(state.HasEscape);
-    assert(state.IsInWorklist);
+    noteEscapingUse(user);
   }
 
   assert(!Accesses.empty() && "didn't find original access!");
@@ -165,9 +156,34 @@ void SelectEnforcement::analyzeProjection(ProjectBoxInst *projection) {
       if (access->getEnforcement() == SILAccessEnforcement::Unknown)
         Accesses.push_back(access);
     }
+
+    // FIXME: We should distinguish between escapes via arguments to escaping
+    // closures (which must propagate to to all reachable blocks because they
+    // could access the address at any later point in the program) and
+    // non-escaping closures (which only force dynamic enforcement for
+    // accesses that are in progress at the time of the escape).
+    if (auto partialApply = dyn_cast<PartialApplyInst>(user)) {
+      noteEscapingUse(partialApply);
+    }
   }
 }
 
+void SelectEnforcement::noteEscapingUse(SILInstruction *inst) {
+  // Add it to the escapes set.
+  Escapes.insert(inst);
+
+  // Record this point as escaping.
+  auto userBB = inst->getParent();
+  auto &state = StateMap[userBB];
+  if (!state.IsInWorklist) {
+    state.HasEscape = true;
+    state.IsInWorklist = true;
+    Worklist.push_back(userBB);
+  }
+  assert(state.HasEscape);
+  assert(state.IsInWorklist);
+}
+
 void SelectEnforcement::propagateEscapes() {
   while (!Worklist.empty()) {
     auto bb = Worklist.pop_back_val();

test/Interpreter/enforce_exclusive_access.swift

@@ -101,19 +101,60 @@ ExclusiveAccessTestSuite.test("ModifyFollowedByModify") {
   globalX = X() // no-trap
 }
 
-ExclusiveAccessTestSuite.test("ClosureCaptureModifyModify") {
+ExclusiveAccessTestSuite.test("ClosureCaptureModifyModify")
+.skip(.custom(
+    { _isFastAssertConfiguration() },
+    reason: "this trap is not guaranteed to happen in -Ounchecked"))
+  .crashOutputMatches("modify/modify access conflict detected on address")
+  .code
+{
+  var x = X()
+  modifyAndPerform(&x) {
+    expectCrashLater()
+    x.i = 12
+  }
+}
+
+ExclusiveAccessTestSuite.test("ClosureCaptureReadModify")
+.skip(.custom(
+    { _isFastAssertConfiguration() },
+    reason: "this trap is not guaranteed to happen in -Ounchecked"))
+  .crashOutputMatches("read/modify access conflict detected on address")
+  .code
+{
   var x = X()
   modifyAndPerform(&x) {
-    // FIXME: This should be caught dynamically.
+    expectCrashLater()
+    _blackHole(x.i)
+  }
+}
+
+ExclusiveAccessTestSuite.test("ClosureCaptureModifyRead")
+.skip(.custom(
+    { _isFastAssertConfiguration() },
+    reason: "this trap is not guaranteed to happen in -Ounchecked"))
+  .crashOutputMatches("modify/read access conflict detected on address")
+  .code
+{
+  var x = X()
+  readAndPerform(&x) {
+    expectCrashLater()
     x.i = 12
   }
 }
 
+ExclusiveAccessTestSuite.test("ClosureCaptureReadRead") {
+  var x = X()
+  readAndPerform(&x) {
+    _blackHole(x.i) // no-trap
+  }
+}
+
 // Test for per-thread enforcement. Don't trap when two different threads
 // have overlapping accesses
 ExclusiveAccessTestSuite.test("PerThreadEnforcement") {
   modifyAndPerform(&globalX) {
-    var (_, otherThread) = _stdlib_pthread_create_block(nil, { (_ : Void) -> () in
+    let (_, otherThread) = _stdlib_pthread_create_block(nil, { (_ : Void) -> () in
       globalX.i = 12 // no-trap
       return ()
     }, ())

test/SILOptimizer/access_enforcement_selection.sil

@@ -64,6 +64,41 @@ sil hidden @markFuncEscape : $@convention(thin) () -> () {
   %99 = return %98 : $()
 }
 
+
+sil @takesInoutAndClosure : $@convention(thin) (@inout Builtin.Int64, @owned @callee_owned () -> ()) -> ()
+sil @closureCapturingByStorageAddress : $@convention(thin) (@inout_aliasable Builtin.Int64) -> ()
+
+// Test dynamic enforcement of box addresses that escape via closure
+// partial_applys.
+// application.
+// CHECK-LABEL: sil hidden @escapeAsArgumentToPartialApply : $@convention(thin) () -> () {
+// CHECK: bb0:
+// CHECK:  [[BOX:%.*]] = alloc_box ${ var Builtin.Int64 }, var, name "x"
+// CHECK:  [[ADR:%.*]] = project_box [[BOX]] : ${ var Builtin.Int64 }, 0
+// CHECK:  [[FUNC:%.*]] = function_ref @takesInoutAndClosure : $@convention(thin) (@inout Builtin.Int64, @owned @callee_owned () -> ()) -> ()
+// CHECK:  [[CLOSURE:%.*]] = function_ref @closureCapturingByStorageAddress : $@convention(thin) (@inout_aliasable Builtin.Int64) -> ()
+// CHECK:  [[PA:%.*]] = partial_apply [[CLOSURE]]([[ADR]]) : $@convention(thin) (@inout_aliasable Builtin.Int64) -> ()
+// CHECK:  [[MODIFY:%.*]] = begin_access [modify] [dynamic] [[ADR]] : $*Builtin.Int64
+// CHECK:  %{{.*}} = apply [[FUNC]]([[MODIFY]], [[PA]]) : $@convention(thin) (@inout Builtin.Int64, @owned @callee_owned () -> ()) -> ()
+// CHECK:  end_access [[MODIFY]] : $*Builtin.Int64
+// CHECK:  destroy_value [[BOX]] : ${ var Builtin.Int64 }
+// CHECK:  return %{{.*}} : $()
+// CHECK-LABEL:} // end sil function 'escapeAsArgumentToPartialApply'
+sil hidden @escapeAsArgumentToPartialApply : $@convention(thin) () -> () {
+  %2 = alloc_box ${ var Builtin.Int64 }, var, name "x"
+  %3 = project_box %2 : ${ var Builtin.Int64 }, 0
+  %4 = function_ref @takesInoutAndClosure : $@convention(thin) (@inout Builtin.Int64, @owned @callee_owned () -> ()) -> ()
+  %5 = function_ref @closureCapturingByStorageAddress : $@convention(thin) (@inout_aliasable Builtin.Int64) -> ()
+  %6 = partial_apply %5(%3) : $@convention(thin) (@inout_aliasable Builtin.Int64) -> ()
+  %7 = begin_access [modify] [unknown] %3 : $*Builtin.Int64
+  %8 = apply %4(%7, %6) : $@convention(thin) (@inout Builtin.Int64, @owned @callee_owned () -> ()) -> ()
+  end_access %7 : $*Builtin.Int64
+  destroy_value %2 : ${ var Builtin.Int64 }
+  %9 = tuple ()
+  %10 = return %9 : $()
+}
+
+
 // Test static enforcement of copied boxes.
 // FIXME: Oops... We make this dynamic.
 //

test/SILOptimizer/access_enforcement_selection.swift

@@ -12,6 +12,11 @@ public func takesInout(_ i: inout Int) {
 // Helper taking a basic, no-escape closure.
 func takeClosure(_: ()->Int) {}
 
+// Helper taking a basic, no-escape closure.
+func takeClosureAndInout(_: inout Int, _: ()->Int) {}
+
+// Helper taking an escaping closure.
+func takeEscapingClosure(_: @escaping ()->Int) {}
 
 // Generate an alloc_stack that escapes into a closure.
 public func captureStack() -> Int {
@@ -21,8 +26,10 @@ public func captureStack() -> Int {
   return x
 }
 // CHECK-LABEL: Access Enforcement Selection in _T028access_enforcement_selection12captureStackSiyF
-// Static access for `return x`.
-// CHECK: Static Access: %{{.*}} = begin_access [read] [static] %{{.*}} : $*Int
+// Dynamic access for `return x`. Since the closure is non-escaping, using
+// dynamic enforcement here is more conservative than it needs to be -- static
+// is sufficient here.
+// CHECK: Dynamic Access: %{{.*}} = begin_access [read] [dynamic] %{{.*}} : $*Int
 
 // The access inside the closure is dynamic, until we have the logic necessary to
 // prove that no other closures are passed to `takeClosure` that may write to
@@ -31,6 +38,49 @@ public func captureStack() -> Int {
 // CHECK-LABEL: Access Enforcement Selection in _T028access_enforcement_selection12captureStackSiyFSiycfU_
 // CHECK: Dynamic Access: %{{.*}} = begin_access [read] [dynamic] %{{.*}} : $*Int
 
+
+// Generate an alloc_stack that does not escape into a closure.
+public func nocaptureStack() -> Int {
+  var x = 3
+  takeClosure { return 5 }
+  return x
+}
+// CHECK-LABEL: Access Enforcement Selection in _T028access_enforcement_selection14nocaptureStackSiyF
+// Static access for `return x`.
+// CHECK: Static Access: %{{.*}} = begin_access [read] [static] %{{.*}} : $*Int
+//
+// CHECK-LABEL: Access Enforcement Selection in _T028access_enforcement_selection14nocaptureStackSiyFSiycfU_
+
+// Generate an alloc_stack that escapes into a closure while an access is
+// in progress.
+public func captureStackWithInoutInProgress() -> Int {
+  // Use a `var` so `x` isn't treated as a literal.
+  var x = 3
+  takeClosureAndInout(&x) { return x }
+  return x
+}
+// CHECK-LABEL: Access Enforcement Selection in _T028access_enforcement_selection31captureStackWithInoutInProgressSiyF
+// Dynamic access for `&x`. This must be dynamic so that we catch the conflict
+// in the closure.
+// CHECK-DAG: Dynamic Access: %{{.*}} = begin_access [modify] [dynamic] %{{.*}} : $*Int
+// Dynamic access for `return x`. This is more conservative than it needs
+// to be; it can be static.
+// CHECK-DAG: Dynamic Access: %{{.*}} = begin_access [read] [dynamic] %{{.*}} : $*Int
+//
+// CHECK-LABEL: Access Enforcement Selection in _T028access_enforcement_selection31captureStackWithInoutInProgressSiyFSiycfU_
+// CHECK: Dynamic Access: %{{.*}} = begin_access [read] [dynamic] %{{.*}} : $*Int
+
+// Generate an alloc_box that escapes into a closure.
+public func captureBox() -> Int {
+  var x = 3
+  takeEscapingClosure { x = 4; return x }
+  return x
+}
+// CHECK-LABEL: Access Enforcement Selection in _T028access_enforcement_selection10captureBoxSiyF
+// Dynamic access for `return x`.
+// CHECK: Dynamic Access: %{{.*}} = begin_access [read] [dynamic] %{{.*}} : $*Int
+// CHECK-LABEL: _T028access_enforcement_selection10captureBoxSiyFSiycfU_
+
 // Generate a closure in which the @inout_aliasing argument
 // escapes to an @inout function `bar`.
 public func recaptureStack() -> Int {
@@ -40,8 +90,9 @@ public func recaptureStack() -> Int {
 }
 // CHECK-LABEL: Access Enforcement Selection in _T028access_enforcement_selection14recaptureStackSiyF
 //
-// Static access for `return x`.
-// CHECK: Static Access:   %{{.*}} = begin_access [read] [static] %{{.*}} : $*Int
+// Dynamic access for `return x`. This is more conservative than it needs
+// to be; static is sufficient.
+// CHECK: Dynamic Access:   %{{.*}} = begin_access [read] [dynamic] %{{.*}} : $*Int
 
 // CHECK-LABEL: Access Enforcement Selection in _T028access_enforcement_selection14recaptureStackSiyFSiycfU_
 //