[SILGen] Emit dynamic actor isolation checks for closures

xedin · xedin · commit 080675170bc0 · 2024-08-22T09:57:17.000-07:00
For synchronous isolated closures passed to unsafe APIs
(the ones that have not been fully concurrency checked)
emit an expected executor check in prolog that would make
sure that they are always used in the expected context.

Resolves: rdar://133415157
diff --git a/lib/SILGen/SILGenConcurrency.cpp b/lib/SILGen/SILGenConcurrency.cpp
@@ -25,6 +25,15 @@
 using namespace swift;
 using namespace Lowering;
 
+static void loadExpectedExecutorForLocalVar(SILGenFunction &SGF, VarDecl *var) {
+  auto loc = RegularLocation::getAutoGeneratedLocation(SGF.F.getLocation());
+  Type actorType = var->getTypeInContext();
+  RValue actorInstanceRV =
+      SGF.emitRValueForDecl(loc, var, actorType, AccessSemantics::Ordinary);
+  ManagedValue actorInstance = std::move(actorInstanceRV).getScalarValue();
+  SGF.ExpectedExecutor = SGF.emitLoadActorExecutor(loc, actorInstance);
+}
+
 void SILGenFunction::emitExpectedExecutor() {
   // Whether the given declaration context is nested within an actor's
   // destructor.
@@ -80,17 +89,6 @@ void SILGenFunction::emitExpectedExecutor() {
   // which are allowed to be available on OSes where concurrency is not
   // available. rdar://106827064
 
-  // Local function to load the expected executor from a local actor
-  auto loadExpectedExecutorForLocalVar = [&](VarDecl *var) {
-    auto loc = RegularLocation::getAutoGeneratedLocation(F.getLocation());
-    Type actorType = var->getTypeInContext();
-    RValue actorInstanceRV = emitRValueForDecl(
-        loc, var, actorType, AccessSemantics::Ordinary);
-    ManagedValue actorInstance =
-        std::move(actorInstanceRV).getScalarValue();
-    ExpectedExecutor = emitLoadActorExecutor(loc, actorInstance);
-  };
-
   if (auto *funcDecl =
         dyn_cast_or_null<AbstractFunctionDecl>(FunctionDC->getAsDecl())) {
     auto actorIsolation = getActorIsolation(funcDecl);
@@ -112,7 +110,7 @@ void SILGenFunction::emitExpectedExecutor() {
           (wantDataRaceChecks && funcDecl->isLocalCapture())) {
         auto loweredCaptures = SGM.Types.getLoweredLocalCaptures(SILDeclRef(funcDecl));
         if (auto isolatedParam = loweredCaptures.getIsolatedParamCapture()) {
-          loadExpectedExecutorForLocalVar(isolatedParam);
+          loadExpectedExecutorForLocalVar(*this, isolatedParam);
         } else {
           auto loc = RegularLocation::getAutoGeneratedLocation(F.getLocation());
           ManagedValue actorArg;
@@ -131,7 +129,7 @@ void SILGenFunction::emitExpectedExecutor() {
                 actorIsolation.getActorInstanceParameter() - 1;
             auto param = funcDecl->getParameters()->get(isolatedParamIdx);
             assert(param->isIsolated());
-            loadExpectedExecutorForLocalVar(param);
+            loadExpectedExecutorForLocalVar(*this, param);
           }
         }
       }
@@ -159,7 +157,8 @@ void SILGenFunction::emitExpectedExecutor() {
 
     case ActorIsolation::ActorInstance: {
       if (wantExecutor) {
-        loadExpectedExecutorForLocalVar(actorIsolation.getActorInstance());
+        loadExpectedExecutorForLocalVar(*this,
+                                        actorIsolation.getActorInstance());
       }
       break;
     }
@@ -610,6 +609,47 @@ static bool isCheckExpectedExecutorIntrinsicAvailable(SILGenModule &SGM) {
   return true;
 }
 
+void SILGenFunction::emitExpectedExecutorPreconditionForClosure() {
+  auto closure = dyn_cast_or_null<ClosureExpr>(FunctionDC);
+  if (!closure)
+    return;
+
+  auto &ctx = closure->getASTContext();
+  if (!ctx.LangOpts.isDynamicActorIsolationCheckingEnabled())
+    return;
+
+  // Asynchronous closures would hop to the expected executor.
+  if (F.isAsync() || !closure->requiresDynamicIsolationChecking())
+    return;
+
+  auto actorIsolation = closure->getActorIsolation();
+  switch (actorIsolation) {
+  case ActorIsolation::Unspecified:
+  case ActorIsolation::Nonisolated:
+  case ActorIsolation::NonisolatedUnsafe:
+    break;
+
+  case ActorIsolation::Erased:
+    llvm_unreachable("closure cannot have erased isolation");
+
+  case ActorIsolation::ActorInstance: {
+    loadExpectedExecutorForLocalVar(*this, actorIsolation.getActorInstance());
+    break;
+  }
+
+  case ActorIsolation::GlobalActor:
+    ExpectedExecutor =
+        emitLoadGlobalActorExecutor(actorIsolation.getGlobalActor());
+    break;
+  }
+
+  if (ExpectedExecutor) {
+    emitPreconditionCheckExpectedExecutor(
+        RegularLocation::getAutoGeneratedLocation(F.getLocation()),
+        ExpectedExecutor);
+  }
+}
+
 void SILGenFunction::emitPreconditionCheckExpectedExecutor(
     SILLocation loc, ActorIsolation isolation,
     std::optional<ManagedValue> actorSelf) {
diff --git a/lib/SILGen/SILGenFunction.h b/lib/SILGen/SILGenFunction.h
@@ -1186,6 +1186,10 @@ class LLVM_LIBRARY_VISIBILITY SILGenFunction
   void emitPreconditionCheckExpectedExecutor(
       SILLocation loc, SILValue executor);
 
+  /// Emit a precondition check to ensure that the closure is executing in
+  /// the expected isolation context.
+  void emitExpectedExecutorPreconditionForClosure();
+
   /// Gets a reference to the current executor for the task.
   /// \returns a value of type Builtin.Executor
   SILValue emitGetCurrentExecutor(SILLocation loc);
diff --git a/lib/SILGen/SILGenProlog.cpp b/lib/SILGen/SILGenProlog.cpp
@@ -1381,6 +1381,7 @@ void SILGenFunction::emitProlog(
   }
 
   emitExpectedExecutor();
+  emitExpectedExecutorPreconditionForClosure();
 
   // IMPORTANT: This block should be the last one in `emitProlog`,
   // since it terminates BB and no instructions should be insterted after it.
diff --git a/test/Concurrency/dynamic_isolation_checks_for_closures.swift b/test/Concurrency/dynamic_isolation_checks_for_closures.swift
@@ -0,0 +1,108 @@
+// RUN: %empty-directory(%t/src)
+// RUN: split-file %s %t/src
+
+/// Build the library API
+// RUN: %target-swift-frontend -emit-module %t/src/API.swift \
+// RUN:   -module-name API -swift-version 5 -enable-library-evolution \
+// RUN:   -emit-module-path %t/API.swiftmodule \
+// RUN:   -emit-module-interface-path %t/API.swiftinterface
+
+// Build client with module
+// RUN: %target-swift-emit-silgen \
+// RUN:   -I %t \
+// RUN:   -disable-availability-checking \
+// RUN:   -module-name Client \
+// RUN:   -swift-version 6 \
+// RUN:    %t/src/Client.swift -verify | %FileCheck %s
+
+//--- API.swift
+public func compute(_: (() -> Void)?) {}
+
+public func computeSendable(_: @escaping @Sendable () -> Void) {}
+
+public func computeSending(_: sending @escaping () -> Void) {}
+
+//--- Client.swift
+import API
+
+func localCompute(_: (() -> Void)?) {}
+
+func forceIsolation(isolation: isolated (any Actor)?) {}
+
+// CHECK-LABEL: sil private [ossa] @$s6Client17test_global_actoryyFyycfU_ : $@convention(thin) () -> ()
+// CHECK: [[EXPECTED_EXECUTOR:%.*]] = extract_executor {{.*}} : $MainActor
+// CHECK: [[CHECK_EXECUTOR_FN:%.*]] = function_ref @$ss22_checkExpectedExecutor14_filenameStart01_D6Length01_D7IsASCII5_line9_executoryBp_BwBi1_BwBetF : $@convention(thin) (Builtin.RawPointer, Builtin.Word, Builtin.Int1, Builtin.Word, Builtin.Executor) -> ()
+// CHECK: %11 = apply [[CHECK_EXECUTOR_FN]]({{.*}}, [[EXPECTED_EXECUTOR]]) : $@convention(thin) (Builtin.RawPointer, Builtin.Word, Builtin.Int1, Builtin.Word, Builtin.Executor) -> ()
+// CHECK: } // end sil function '$s6Client17test_global_actoryyFyycfU_'
+@MainActor
+func test_global_actor() {
+  compute {
+  }
+}
+
+// CHECK-LABEL: sil private [ossa] @$s6Client13test_isolated9isolationyScMYi_tFyycfU_ : $@convention(thin) (@sil_isolated @guaranteed MainActor) -> ()
+// CHECK: [[EXPECTED_EXECUTOR:%.*]] = extract_executor {{.*}} : $MainActor
+// CHECK: [[CHECK_EXECUTOR_FN:%.*]] = function_ref @$ss22_checkExpectedExecutor14_filenameStart01_D6Length01_D7IsASCII5_line9_executoryBp_BwBi1_BwBetF : $@convention(thin) (Builtin.RawPointer, Builtin.Word, Builtin.Int1, Builtin.Word, Builtin.Executor) -> ()
+// CHECK: %11 = apply [[CHECK_EXECUTOR_FN]]({{.*}}, [[EXPECTED_EXECUTOR]]) : $@convention(thin) (Builtin.RawPointer, Builtin.Word, Builtin.Int1, Builtin.Word, Builtin.Executor) -> ()
+// CHECK: } // end sil function '$s6Client13test_isolated9isolationyScMYi_tFyycfU_'
+func test_isolated(isolation: isolated MainActor) {
+  compute {
+    forceIsolation(isolation: isolation)
+  }
+}
+
+// CHECK-LABEL: sil private [ossa] @$s6Client55test_concurrency_checked_global_isolation_has_no_checksyyFyycfU_ : $@convention(thin) () -> ()
+// CHECK-NOT: function_ref @$ss22_checkExpectedExecutor14_filenameStart01_D6Length01_D7IsASCII5_line9_executoryBp_BwBi1_BwBetF
+// CHECK: } // end sil function '$s6Client55test_concurrency_checked_global_isolation_has_no_checksyyFyycfU_'
+@MainActor
+func test_concurrency_checked_global_isolation_has_no_checks() {
+  localCompute {
+  }
+}
+
+// CHECK-LABEL: sil private [ossa] @$s6Client47test_concurrency_checked_isolated_has_no_checks9isolationyScMYi_tFyycfU_ : $@convention(thin) (@sil_isolated @guaranteed MainActor) -> ()
+// CHECK-NOT: function_ref @$ss22_checkExpectedExecutor14_filenameStart01_D6Length01_D7IsASCII5_line9_executoryBp_BwBi1_BwBetF
+// CHECK: } // end sil function '$s6Client47test_concurrency_checked_isolated_has_no_checks9isolationyScMYi_tFyycfU_'
+func test_concurrency_checked_isolated_has_no_checks(isolation: isolated MainActor) {
+  localCompute {
+    forceIsolation(isolation: isolation)
+  }
+}
+
+// CHECK-LABEL: sil private [ossa] @$s6Client24test_direct_closure_callyyFyyXEfU_ : $@convention(thin) () -> ()
+// CHECK-NOT: function_ref @$ss22_checkExpectedExecutor14_filenameStart01_D6Length01_D7IsASCII5_line9_executoryBp_BwBi1_BwBetF
+// CHECK: [[INNER_CLOSURE_REF:%.*]] = function_ref @$s6Client24test_direct_closure_callyyFyyXEfU_yycfU_ : $@convention(thin) () -> ()
+// CHECK-NEXT: [[INNER_CLOSURE:%.*]] = thin_to_thick_function [[INNER_CLOSURE_REF]] : $@convention(thin) () -> () to $@callee_guaranteed () -> ()
+// CHECK-NEXT: [[OPT_INNER_CLOSURE:%.*]] = enum $Optional<@callee_guaranteed () -> ()>, #Optional.some!enumelt, [[INNER_CLOSURE]] : $@callee_guaranteed () -> ()
+// CHECK: [[COMPUTE_FN:%.*]] = function_ref @$s3API7computeyyyycSgF : $@convention(thin) (@guaranteed Optional<@callee_guaranteed () -> ()>) -> ()
+// CHECK-NEXT: apply [[COMPUTE_FN]]([[OPT_INNER_CLOSURE]]) : $@convention(thin) (@guaranteed Optional<@callee_guaranteed () -> ()>) -> ()
+// CHECK: } // end sil function '$s6Client24test_direct_closure_callyyFyyXEfU_'
+
+// CHECK-LABEL: sil private [ossa] @$s6Client24test_direct_closure_callyyFyyXEfU_yycfU_ : $@convention(thin) () -> ()
+// CHECK: [[EXPECTED_EXECUTOR:%.*]] = extract_executor {{.*}} : $MainActor
+// CHECK: [[CHECK_EXECUTOR_FN:%.*]] = function_ref @$ss22_checkExpectedExecutor14_filenameStart01_D6Length01_D7IsASCII5_line9_executoryBp_BwBi1_BwBetF : $@convention(thin) (Builtin.RawPointer, Builtin.Word, Builtin.Int1, Builtin.Word, Builtin.Executor) -> ()
+// CHECK: %11 = apply [[CHECK_EXECUTOR_FN]]({{.*}}, [[EXPECTED_EXECUTOR]]) : $@convention(thin) (Builtin.RawPointer, Builtin.Word, Builtin.Int1, Builtin.Word, Builtin.Executor) -> ()
+// CHECK: } // end sil function '$s6Client24test_direct_closure_callyyFyyXEfU_yycfU_'
+@MainActor
+func test_direct_closure_call() {
+  {
+    compute {
+    }
+  }()
+}
+
+@MainActor
+func test_global_actor_sendable_and_sending_closures() {
+  // CHECK-LABEL: sil private [ossa] @$s6Client47test_global_actor_sendable_and_sending_closuresyyFyyYbcfU_ : $@convention(thin) @Sendable () -> ()
+  // CHECK-NOT: function_ref @$ss22_checkExpectedExecutor14_filenameStart01_D6Length01_D7IsASCII5_line9_executoryBp_BwBi1_BwBetF
+  // CHECK: } // end sil function '$s6Client47test_global_actor_sendable_and_sending_closuresyyFyyYbcfU_'
+  computeSendable {   
+  }
+
+  // CHECK-LABEL: sil private [ossa] @$s6Client47test_global_actor_sendable_and_sending_closuresyyFyycfU0_ : $@convention(thin) () -> ()
+  // CHECK-NOT: function_ref @$ss22_checkExpectedExecutor14_filenameStart01_D6Length01_D7IsASCII5_line9_executoryBp_BwBi1_BwBetF
+  // CHECK: } // end sil function '$s6Client47test_global_actor_sendable_and_sending_closuresyyFyycfU0_'
+  computeSending {
+    forceIsolation(isolation: #isolation)
+  }
+}
diff --git a/test/Interpreter/dynamic_isolation_checks_for_closures.swift b/test/Interpreter/dynamic_isolation_checks_for_closures.swift
@@ -0,0 +1,121 @@
+// RUN: %empty-directory(%t/src)
+// RUN: split-file %s %t/src
+
+// RUN: %target-build-swift %t/src/Interface.swift -emit-module -emit-library \
+// RUN:    -target %target-cpu-apple-macosx10.15 -swift-version 5 \
+// RUN:    -enable-library-evolution \
+// RUN:    -module-name Interface \
+// RUN:    -o %t/%target-library-name(Interface) \
+// RUN:    -emit-module-interface-path %t/Interface.swiftinterface
+
+// RUN: %target-build-swift -target %target-cpu-apple-macosx10.15 -swift-version 5 -Xfrontend -enable-upcoming-feature -Xfrontend DynamicActorIsolation -I %t -L %t -lInterface %t/src/Crash1.swift -o %t/crash1.out
+// RUN: %target-codesign %t/crash1.out
+// RUN: not --crash env SWIFT_IS_CURRENT_EXECUTOR_LEGACY_MODE_OVERRIDE=legacy SWIFT_UNEXPECTED_EXECUTOR_LOG_LEVEL=2 %target-run %t/crash1.out 2>&1 | %FileCheck %t/src/Crash1.swift --check-prefix=LEGACY_CHECK
+// RUN: not --crash env SWIFT_IS_CURRENT_EXECUTOR_LEGACY_MODE_OVERRIDE=swift6 SWIFT_UNEXPECTED_EXECUTOR_LOG_LEVEL=2 %target-run %t/crash1.out 2>&1 | %FileCheck %t/src/Crash1.swift --check-prefix=SWIFT6_CHECK --dump-input=always
+
+// RUN: %target-build-swift -target %target-cpu-apple-macosx10.15 -swift-version 6 -I %t -L %t -lInterface %t/src/Crash2.swift -o %t/crash2.out
+// RUN: %target-codesign %t/crash2.out
+// RUN: not --crash env SWIFT_IS_CURRENT_EXECUTOR_LEGACY_MODE_OVERRIDE=legacy SWIFT_UNEXPECTED_EXECUTOR_LOG_LEVEL=2 %target-run %t/crash2.out 2>&1 | %FileCheck %t/src/Crash2.swift --check-prefix=LEGACY_CHECK
+// RUN: not --crash env SWIFT_IS_CURRENT_EXECUTOR_LEGACY_MODE_OVERRIDE=swift6 SWIFT_UNEXPECTED_EXECUTOR_LOG_LEVEL=2 %target-run %t/crash2.out 2>&1 | %FileCheck %t/src/Crash2.swift --check-prefix=SWIFT6_CHECK --dump-input=always
+
+// RUN: %target-build-swift -target %target-cpu-apple-macosx10.15 -swift-version 6 -I %t -L %t -lInterface %t/src/Crash3.swift -o %t/crash3.out
+// RUN: %target-codesign %t/crash3.out
+// RUN: not --crash env SWIFT_IS_CURRENT_EXECUTOR_LEGACY_MODE_OVERRIDE=legacy SWIFT_UNEXPECTED_EXECUTOR_LOG_LEVEL=2 %target-run %t/crash3.out 2>&1 | %FileCheck %t/src/Crash3.swift --check-prefix=LEGACY_CHECK
+// RUN: not --crash env SWIFT_IS_CURRENT_EXECUTOR_LEGACY_MODE_OVERRIDE=swift6 SWIFT_UNEXPECTED_EXECUTOR_LOG_LEVEL=2 %target-run %t/crash3.out 2>&1 | %FileCheck %t/src/Crash3.swift --check-prefix=SWIFT6_CHECK --dump-input=always
+
+// REQUIRES: asserts
+// REQUIRES: concurrency
+// REQUIRES: concurrency_runtime
+// REQUIRES: executable_test
+// REQUIRES: OS=macosx
+
+// rdar://123810657
+// UNSUPPORTED: back_deployment_runtime
+
+//--- Interface.swift
+import Dispatch
+
+public func runTest(_ fn: @escaping () -> Void) async {
+  await Task.detached {
+    fn()
+  }.value
+}
+
+public func syncRunTest(_ fn: @escaping () -> Void) {
+  let sem = DispatchSemaphore(value: 0)
+    
+  Task.detached {
+    fn()
+    sem.signal()
+  }
+
+  sem.wait()
+}
+
+//--- crash1.swift
+import Interface
+
+@globalActor
+actor MyActor {
+  static let shared = MyActor()
+}
+
+func forceIsolation(isolation: isolated (any Actor)?) {}
+
+@MyActor
+func test() async {
+  await runTest { forceIsolation(isolation: #isolation) }
+}
+
+await test()
+
+print("OK")
+
+// LEGACY_CHECK: data race detected: actor-isolated function at crash1/Crash1.swift:12 was not called on the same actor
+
+// Crash without good message, since via 'dispatch_assert_queue'
+// SWIFT6_CHECK-NOT: OK
+
+//--- crash2.swift
+import Interface
+
+@globalActor
+actor MyActor {
+  static let shared = MyActor()
+}
+
+@MyActor
+func test() async {
+  syncRunTest { }
+}
+
+await test()
+
+print("OK")
+
+// LEGACY_CHECK: data race detected: actor-isolated function at crash2/Crash2.swift:10 was not called on the same actor
+
+// Crash without good message, since via 'dispatch_assert_queue'
+// SWIFT6_CHECK-NOT: OK
+
+//--- crash3.swift
+import Interface
+
+actor MyActor {
+}
+
+func forceIsolation(isolation: isolated MyActor) {
+}
+
+func test(isolation: isolated MyActor) async {
+  syncRunTest { forceIsolation(isolation: isolation) }
+}
+
+await test(isolation: MyActor())
+
+print("OK")
+
+// LEGACY_CHECK: data race detected: actor-isolated function at crash3/Crash3.swift:10 was not called on the same actor
+
+// Crash without good message, since via 'dispatch_assert_queue'
+// SWIFT6_CHECK-NOT: OK

Original file line number	Diff line number	Diff line change
`@@ -1381,6 +1381,7 @@ void SILGenFunction::emitProlog(`
`1381`	`1381`	`}`
`1382`	`1382`
`1383`	`1383`	`emitExpectedExecutor();`
	`1384`	`+ emitExpectedExecutorPreconditionForClosure();`
`1384`	`1385`
`1385`	`1386`	// IMPORTANT: This block should be the last one in `emitProlog`,
`1386`	`1387`	`// since it terminates BB and no instructions should be insterted after it.`