[silgen] When emitting a foreign async completion handler for a method, use merge isolation region to tie self and the block storage into the same region.

gottesmm · gottesmm · commit 714cc0073ebf · 2025-05-08T12:38:10.000-07:00
This is an extension of a similar problem that I had fixed earlier where due to the usage of intermediate Sendable types we do not propagate regions correctly. The previous issue I fixed was that we were not properly tieing the result of a foreign async completion handler to the block storage since we used an intervening UnsafeContinuation (which is Sendable) to propagate the result into the block storage. I fixed this by changing SILGen to insert a merge_isolation_region that explicitly ties the result to the block storage. This new issue is that the block that we create and then pass as the completion handler is an @sendable block. Thus when we call the actual objc_method, the block storage and self are not viewed as being in the same region. In this PR, I change it so that we add a merge_isolation_region from self onto the block storage. The end result of this is that we have that self, the result of the call, and the block storage are all in the same region meaning that we properly diagnose that returning an NSObject from the imported Objective-C function is task isolated and thus we cannot return it as a sending result. rdar://131422332 (cherry picked from commit 227ab37)
diff --git a/lib/SILGen/ResultPlan.cpp b/lib/SILGen/ResultPlan.cpp
@@ -792,10 +792,9 @@ class ForeignAsyncInitializationPlan final : public ResultPlan {
     return std::make_tuple(blockStorage, blockStorageTy, continuationTy);
   }
 
-  ManagedValue
-  emitForeignAsyncCompletionHandler(SILGenFunction &SGF,
-                                    AbstractionPattern origFormalType,
-                                    SILLocation loc) override {
+  ManagedValue emitForeignAsyncCompletionHandler(
+      SILGenFunction &SGF, AbstractionPattern origFormalType, ManagedValue self,
+      SILLocation loc) override {
     // Get the current continuation for the task.
     bool throws =
         calleeTypeInfo.foreign.async->completionHandlerErrorParamIndex()
@@ -845,7 +844,14 @@ class ForeignAsyncInitializationPlan final : public ResultPlan {
     SILValue block = SGF.B.createInitBlockStorageHeader(loc, blockStorage,
                           impRef, SILType::getPrimitiveObjectType(impFnTy),
                           SGF.getForwardingSubstitutionMap());
-    
+
+    // If our block is Sendable, we have lost the connection in between self and
+    // blockStorage. We need to restore that connection by using a merge
+    // isolation region.
+    if (self && block->getType().isSendable(&SGF.F)) {
+      SGF.B.createMergeIsolationRegion(loc, {self.getValue(), blockStorage});
+    }
+
     // Wrap it in optional if the callee expects it.
     if (handlerIsOptional) {
       block = SGF.B.createOptionalSome(loc, block, impTy);
@@ -1091,11 +1097,11 @@ class ForeignErrorInitializationPlan final : public ResultPlan {
     subPlan->gatherIndirectResultAddrs(SGF, loc, outList);
   }
 
-  ManagedValue
-  emitForeignAsyncCompletionHandler(SILGenFunction &SGF,
-                                    AbstractionPattern origFormalType,
-                                    SILLocation loc) override {
-    return subPlan->emitForeignAsyncCompletionHandler(SGF, origFormalType, loc);
+  ManagedValue emitForeignAsyncCompletionHandler(
+      SILGenFunction &SGF, AbstractionPattern origFormalType, ManagedValue self,
+      SILLocation loc) override {
+    return subPlan->emitForeignAsyncCompletionHandler(SGF, origFormalType, self,
+                                                      loc);
   }
 
   std::optional<std::pair<ManagedValue, ManagedValue>>
diff --git a/lib/SILGen/ResultPlan.h b/lib/SILGen/ResultPlan.h
@@ -65,8 +65,10 @@ class ResultPlan {
     return std::nullopt;
   }
 
-  virtual ManagedValue emitForeignAsyncCompletionHandler(
-      SILGenFunction &SGF, AbstractionPattern origFormalType, SILLocation loc) {
+  virtual ManagedValue
+  emitForeignAsyncCompletionHandler(SILGenFunction &SGF,
+                                    AbstractionPattern origFormalType,
+                                    ManagedValue self, SILLocation loc) {
     return {};
   }
 };
diff --git a/lib/SILGen/SILGenApply.cpp b/lib/SILGen/SILGenApply.cpp
@@ -5819,9 +5819,16 @@ RValue SILGenFunction::emitApply(
     // we left during the first pass.
     auto &completionArgSlot = const_cast<ManagedValue &>(args[completionIndex]);
 
+    // We have already lowered foreign self/moved it into position at this
+    // point, so we know that self will be back.
+    ManagedValue self;
+    if (substFnType->hasSelfParam()) {
+      self = args.back();
+    }
+
     auto origFormalType = *calleeTypeInfo.origFormalType;
     completionArgSlot = resultPlan->emitForeignAsyncCompletionHandler(
-        *this, origFormalType, loc);
+        *this, origFormalType, self, loc);
   }
   if (auto foreignError = calleeTypeInfo.foreign.error) {
     unsigned errorParamIndex =
diff --git a/test/ClangImporter/Inputs/regionbasedisolation.h b/test/ClangImporter/Inputs/regionbasedisolation.h
@@ -0,0 +1,18 @@
+
+#import <Foundation/Foundation.h>
+
+NS_ASSUME_NONNULL_BEGIN
+
+@interface ObjCObject : NSObject
+
+- (void)loadObjectsWithCompletionHandler:
+    (void (^NS_SWIFT_SENDABLE)(NSArray<NSObject *> *_Nullable,
+                               NSError *_Nullable))completionHandler;
+
+- (void)loadObjects2WithCompletionHandler:
+    (void (^)(NSArray<NSObject *> *_Nullable,
+                               NSError *_Nullable))completionHandler;
+
+@end
+
+NS_ASSUME_NONNULL_END
diff --git a/test/ClangImporter/regionbasedisolation.swift b/test/ClangImporter/regionbasedisolation.swift
@@ -0,0 +1,125 @@
+// RUN: %target-swift-frontend %s -import-objc-header %S/Inputs/regionbasedisolation.h -verify -c -swift-version 6
+// RUN: %target-swift-frontend %s -import-objc-header %S/Inputs/regionbasedisolation.h -emit-silgen -swift-version 6 | %FileCheck %s
+
+// REQUIRES: objc_interop
+
+extension ObjCObject {
+  // CHECK-LABEL: sil hidden [ossa] @$sSo10ObjCObjectC20regionbasedisolationE11sendObjectsSaySo8NSObjectCGyYaKF : $@convention(method) @async (@guaranteed ObjCObject) -> (@sil_sending @owned Array<NSObject>, @error any Error) {
+  // CHECK: bb0([[SELF:%.*]] : @guaranteed $ObjCObject):
+
+  // Our result.
+  // CHECK: [[RESULT:%.*]] = alloc_stack $Array<NSObject>
+
+  // Our method.
+  // CHECK: [[METHOD:%.*]] = objc_method [[SELF]], #ObjCObject.loadObjects!foreign : (ObjCObject) -> () async throws -> [NSObject], $@convention(objc_method) (@convention(block) @Sendable (Optional<NSArray>, Optional<NSError>) -> (), ObjCObject) -> ()
+
+  // Begin setting up the unsafe continuation for our method. Importantly note
+  // that [[UNSAFE_CONT]] is Sendable, so we lose any connection from the
+  // continuation addr to any uses of the UnsafeContinuation.
+  //
+  // CHECK: [[CONT:%.*]] = get_async_continuation_addr [throws] Array<NSObject>, [[RESULT]]
+  // CHECK: [[UNSAFE_CONT:%.*]] = struct $UnsafeContinuation<Array<NSObject>, any Error> ([[CONT]])
+
+  // Then prepare the block storage.
+  // CHECK: [[BLOCK_STORAGE:%.*]] = alloc_stack $@block_storage Any
+  // CHECK: [[PROJECT_BLOCK_STORAGE:%.*]] = project_block_storage [[BLOCK_STORAGE]]
+  // CHECK: [[EXISTENTIAL_BLOCK_STORAGE:%.*]] = init_existential_addr [[PROJECT_BLOCK_STORAGE]]
+
+  // Then create a checked continuation from the unsafe continuation.
+  //
+  // CHECK: [[CREATE_CHECKED_CONT:%.*]] = function_ref @$ss34_createCheckedThrowingContinuationyScCyxs5Error_pGSccyxsAB_pGnlF : $@convention(thin) <τ_0_0> (UnsafeContinuation<τ_0_0, any Error>) -> @out CheckedContinuation<τ_0_0, any Error>
+  // CHECK: [[CHECKED_CONT:%.*]] = alloc_stack $CheckedContinuation<Array<NSObject>, any Error>
+  // CHECK: apply [[CREATE_CHECKED_CONT]]<Array<NSObject>>([[CHECKED_CONT]], [[UNSAFE_CONT]])
+
+  // Then place the checked continuation into the block storage and perform a
+  // merge_isolation_region in between the block storage and the result to
+  // propagate that the result and the block storage are apart of the same
+  // region despite the UnsafeContinuation blocking the relation in between
+  // them.
+  //
+  // CHECK: copy_addr [take] [[CHECKED_CONT]] to [init] [[EXISTENTIAL_BLOCK_STORAGE]]
+  // CHECK: merge_isolation_region [[BLOCK_STORAGE]], [[RESULT]]
+
+  // Then create the actual block. NOTE: Since the block is @Sendable, the block
+  // does not propagate regions.
+  //
+  // CHECK: [[COMPLETION_HANDLER_BLOCK:%.*]] = function_ref @$sSo7NSArrayCSgSo7NSErrorCSgIeyBhyy_SaySo8NSObjectCGTz_ : $@convention(c) @Sendable (@inout_aliasable @block_storage Any, Optional<NSArray>, Optional<NSError>) -> ()
+  // CHECK: [[COMPLETION_BLOCK:%.*]] = init_block_storage_header [[BLOCK_STORAGE]], invoke [[COMPLETION_HANDLER_BLOCK]]
+  //
+  // Since the block is @Sendable, it does not propagate the connection in
+  // between self and the block storage when we just call the method. Thus we
+  // need to perform a merge_isolation_region to communicate that the block
+  // storage and self are part of the same region.
+  //
+  // CHECK: merge_isolation_region [[SELF]], [[BLOCK_STORAGE]]
+  //
+  // Then call the method.
+  // CHECK: apply [[METHOD]]([[COMPLETION_BLOCK]], [[SELF]])
+  // CHECK: } // end sil function '$sSo10ObjCObjectC20regionbasedisolationE11sendObjectsSaySo8NSObjectCGyYaKF'
+
+  func sendObjects() async throws -> sending [NSObject] {
+    // We emit an error since loadObjects just returns an [NSObject], not a
+    // sending [NSObject].
+    try await loadObjects()
+  } // expected-error {{task or actor isolated value cannot be sent}}
+
+  // Check if we do not mark the block as NS_SWIFT_SENDABLE
+  //
+  // CHECK-LABEL: sil hidden [ossa] @$sSo10ObjCObjectC20regionbasedisolationE12sendObjects2SaySo8NSObjectCGyYaKF : $@convention(method) @async (@guaranteed ObjCObject) -> (@sil_sending @owned Array<NSObject>, @error any Error) {
+  // CHECK: bb0([[SELF:%.*]] : @guaranteed $ObjCObject):
+
+  // Our result.
+  // CHECK: [[RESULT:%.*]] = alloc_stack $Array<NSObject>
+
+  // Our method.
+  // CHECK: [[METHOD:%.*]] = objc_method [[SELF]], #ObjCObject.loadObjects2!foreign : (ObjCObject) -> () async throws -> [NSObject], $@convention(objc_method) (@convention(block) @Sendable (Optional<NSArray>, Optional<NSError>) -> (), ObjCObject) -> ()
+
+  // Begin setting up the unsafe continuation for our method. Importantly note
+  // that [[UNSAFE_CONT]] is Sendable, so we lose any connection from the
+  // continuation addr to any uses of the UnsafeContinuation.
+  //
+  // CHECK: [[CONT:%.*]] = get_async_continuation_addr [throws] Array<NSObject>, [[RESULT]]
+  // CHECK: [[UNSAFE_CONT:%.*]] = struct $UnsafeContinuation<Array<NSObject>, any Error> ([[CONT]])
+
+  // Then prepare the block storage.
+  // CHECK: [[BLOCK_STORAGE:%.*]] = alloc_stack $@block_storage Any
+  // CHECK: [[PROJECT_BLOCK_STORAGE:%.*]] = project_block_storage [[BLOCK_STORAGE]]
+  // CHECK: [[EXISTENTIAL_BLOCK_STORAGE:%.*]] = init_existential_addr [[PROJECT_BLOCK_STORAGE]]
+
+  // Then create a checked continuation from the unsafe continuation.
+  //
+  // CHECK: [[CREATE_CHECKED_CONT:%.*]] = function_ref @$ss34_createCheckedThrowingContinuationyScCyxs5Error_pGSccyxsAB_pGnlF : $@convention(thin) <τ_0_0> (UnsafeContinuation<τ_0_0, any Error>) -> @out CheckedContinuation<τ_0_0, any Error>
+  // CHECK: [[CHECKED_CONT:%.*]] = alloc_stack $CheckedContinuation<Array<NSObject>, any Error>
+  // CHECK: apply [[CREATE_CHECKED_CONT]]<Array<NSObject>>([[CHECKED_CONT]], [[UNSAFE_CONT]])
+
+  // Then place the checked continuation into the block storage and perform a
+  // merge_isolation_region in between the block storage and the result to
+  // propagate that the result and the block storage are apart of the same
+  // region despite the UnsafeContinuation blocking the relation in between
+  // them.
+  //
+  // CHECK: copy_addr [take] [[CHECKED_CONT]] to [init] [[EXISTENTIAL_BLOCK_STORAGE]]
+  // CHECK: merge_isolation_region [[BLOCK_STORAGE]], [[RESULT]]
+
+  // Then create the actual block. NOTE: Since the block is @Sendable, the block
+  // does not propagate regions.
+  //
+  // CHECK: [[COMPLETION_HANDLER_BLOCK:%.*]] = function_ref @$sSo7NSArrayCSgSo7NSErrorCSgIeyBhyy_SaySo8NSObjectCGTz_ : $@convention(c) @Sendable (@inout_aliasable @block_storage Any, Optional<NSArray>, Optional<NSError>) -> ()
+  // CHECK: [[COMPLETION_BLOCK:%.*]] = init_block_storage_header [[BLOCK_STORAGE]], invoke [[COMPLETION_HANDLER_BLOCK]]
+  //
+  // Since the block is @Sendable, it does not propagate the connection in
+  // between self and the block storage when we just call the method. Thus we
+  // need to perform a merge_isolation_region to communicate that the block
+  // storage and self are part of the same region.
+  //
+  // CHECK: merge_isolation_region [[SELF]], [[BLOCK_STORAGE]]
+  //
+  // Then call the method.
+  // CHECK: apply [[METHOD]]([[COMPLETION_BLOCK]], [[SELF]])
+  // CHECK: } // end sil function '$sSo10ObjCObjectC20regionbasedisolationE12sendObjects2SaySo8NSObjectCGyYaKF'
+  func sendObjects2() async throws -> sending [NSObject] {
+    // We emit an error since loadObjects just returns an [NSObject], not a
+    // sending [NSObject].
+    try await loadObjects2()
+  } // expected-error {{task or actor isolated value cannot be sent}}
+}

Original file line number	Diff line number	Diff line change
`@@ -65,8 +65,10 @@ class ResultPlan {`
`65`	`65`	`return std::nullopt;`
`66`	`66`	`}`
`67`	`67`
`68`		`- virtual ManagedValue emitForeignAsyncCompletionHandler(`
`69`		`- SILGenFunction &SGF, AbstractionPattern origFormalType, SILLocation loc) {`
	`68`	`+ virtual ManagedValue`
	`69`	`+ emitForeignAsyncCompletionHandler(SILGenFunction &SGF,`
	`70`	`+ AbstractionPattern origFormalType,`
	`71`	`+ ManagedValue self, SILLocation loc) {`
`70`	`72`	`return {};`
`71`	`73`	`}`
`72`	`74`	`};`