[SILGen] avoid hop before autoreleased foreign error is retained

kavon · kavon · commit 6658c38a626f · 2023-12-19T13:51:34.000-08:00
For an isolated ObjC function that is not async, we emit a hops around the call. But if that function returns an autoreleased pointer, we need to ensure we're retaining that pointer before hopping back after the call. We weren't doing that in the case of an autoreleased NSError: ``` %10 = alloc_stack $@sil_unmanaged Optional<NSError> %19 = ... a bunch of steps to wrap up %10 ... %20 = enum $Optional<AutoreleasingUnsafeMutablePointer<Optional<NSError>>>, #Optional.some!enumelt, %19 : $AutoreleasingUnsafeMutablePointer<Optional<NSError>> hop_to_executor $MainActor %26 = apply X(Y, %20) : $@convention(objc_method) (NSObject, Optional<AutoreleasingUnsafeMutablePointer<Optional<NSError>>>) -> @autoreleased Optional<NSString> hop_to_executor $Optional<Builtin.Executor> // retain the autoreleased pointer written-out. %28 = load [trivial] %10 : $*@sil_unmanaged Optional<NSError> %29 = unmanaged_to_ref %28 : $@sil_unmanaged Optional<NSError> to $Optional<NSError> %30 = copy_value %29 : $Optional<NSError> assign %31 to %7 : $*Optional<NSError> ``` This patch sinks the hop emission after the call so it happens after doing that copy. rdar://114049646 (cherry picked from commit 06ac850)
diff --git a/lib/SILGen/SILGenApply.cpp b/lib/SILGen/SILGenApply.cpp
@@ -5554,10 +5554,6 @@ RValue SILGenFunction::emitApply(
     // plan's finish method is called, because it must happen in the
     // successors of the `await_async_continuation` terminator.
     resultPlan->deferExecutorBreadcrumb(std::move(breadcrumb));
-
-  } else {
-    // In the ordinary case, we hop back to the current executor
-    breadcrumb.emit(*this, loc);
   }
 
   // Pop the argument scope.
@@ -5630,6 +5626,14 @@ RValue SILGenFunction::emitApply(
         });
   }
 
+  if (!calleeTypeInfo.foreign.async) {
+    // For a non-foreign-async callee, we hop back to the current executor
+    // _after_ popping the argument scope and collecting the results. This is
+    // important because we may need to, for example, retain one of the results
+    // prior to changing actors in the case of an autorelease'd return value.
+    breadcrumb.emit(*this, loc);
+  }
+
   SILValue bridgedForeignError;
   // If there was a foreign error convention, consider it.
   // TODO: maybe this should happen after managing the result if it's
diff --git a/test/Inputs/clang-importer-sdk/usr/include/ObjCConcurrency.h b/test/Inputs/clang-importer-sdk/usr/include/ObjCConcurrency.h
@@ -333,4 +333,11 @@ MAIN_ACTOR
 @property(readonly) BOOL isVisible;
 @end
 
+// rdar://114049646
+MAIN_ACTOR
+@protocol HotdogCompetitor
+- (nullable NSString *)pileOfHotdogsToEatWithLimit:(NSObject *)limit
+                                                   error:(NSError * __autoreleasing *)error;
+@end
+
 #pragma clang assume_nonnull end
diff --git a/test/SILGen/async_initializer.swift b/test/SILGen/async_initializer.swift
@@ -131,7 +131,7 @@ enum Birb {
 // CHECK-NEXT:     hop_to_executor [[GENERIC_EXEC]] : $Optional<Builtin.Executor>
 // CHECK:          hop_to_executor {{%[0-9]+}} : $MainActor
 // CHECK-NEXT:     {{%[0-9]+}} = apply {{%[0-9]+}}({{%[0-9]+}}, {{%[0-9]+}}) : $@convention(method) (@owned String, @thick Cat.Type) -> @owned Cat
-// CHECK-NEXT:     hop_to_executor [[GENERIC_EXEC]] : $Optional<Builtin.Executor>
+// CHECK:          hop_to_executor [[GENERIC_EXEC]] : $Optional<Builtin.Executor>
 // CHECK:        } // end sil function '$s12initializers7makeCatyyYaF'
 func makeCat() async {
   _ = await Cat(name: "Socks")
@@ -142,7 +142,7 @@ func makeCat() async {
 // CHECK-NEXT:     hop_to_executor [[GENERIC_EXEC]] : $Optional<Builtin.Executor>
 // CHECK:          hop_to_executor {{%[0-9]+}} : $MainActor
 // CHECK-NEXT:     {{%[0-9]+}} = apply {{%[0-9]+}}({{%[0-9]+}}, {{%[0-9]+}}) : $@convention(method) (@owned String, @thin Dog.Type) -> Dog
-// CHECK-NEXT:     hop_to_executor [[GENERIC_EXEC]] : $Optional<Builtin.Executor>
+// CHECK:          hop_to_executor [[GENERIC_EXEC]] : $Optional<Builtin.Executor>
 // CHECK:        } // end sil function '$s12initializers7makeDogyyYaF'
 func makeDog() async {
   _ = await Dog(name: "Lassie")
@@ -153,7 +153,7 @@ func makeDog() async {
 // CHECK-NEXT:     hop_to_executor [[GENERIC_EXEC]] : $Optional<Builtin.Executor>
 // CHECK:          hop_to_executor {{%[0-9]+}} : $MainActor
 // CHECK-NEXT:     {{%[0-9]+}} = apply {{%[0-9]+}}({{%[0-9]+}}, {{%[0-9]+}}) : $@convention(method) (@owned String, @thin Birb.Type) -> Birb
-// CHECK-NEXT:     hop_to_executor [[GENERIC_EXEC]] : $Optional<Builtin.Executor>
+// CHECK:          hop_to_executor [[GENERIC_EXEC]] : $Optional<Builtin.Executor>
 // CHECK:        } // end sil function '$s12initializers8makeBirbyyYaF'
 func makeBirb() async {
   _ = await Birb(name: "Chirpy")
diff --git a/test/SILGen/hop_to_executor.swift b/test/SILGen/hop_to_executor.swift
@@ -47,7 +47,7 @@ actor MyActor {
   // CHECK:        [[BORROWED_SELF:%[0-9]+]] = begin_borrow %0 : $MyActor
   // CHECK:        hop_to_executor [[BORROWED_SELF]] : $MyActor 
   // CHECK:        = apply {{.*}} : $@convention(method) @async (Int, @guaranteed MyActor) -> ()
-  // CHECK-NEXT:   hop_to_executor [[BORROWED_SELF]] : $MyActor 
+  // CHECK:        hop_to_executor [[BORROWED_SELF]] : $MyActor
   // CHECK:      } // end sil function '$s4test7MyActorC0A22ConsumingAsyncFunctionyyYaF'
   __consuming func testConsumingAsyncFunction() async {
     await callee(p)
@@ -225,7 +225,7 @@ actor BlueActorImpl {
 // CHECK:       hop_to_executor [[REDBORROW]] : $RedActorImpl
 // CHECK-NEXT:  = apply [[METH]]([[INTARG]], [[REDBORROW]]) : $@convention(method) (Int, @guaranteed RedActorImpl) -> ()
 // CHECK-NEXT:  hop_to_executor [[BLUE]] : $BlueActorImpl
-// CHECK:       end_borrow [[REDBORROW]] : $RedActorImpl
+// CHECK-NEXT:  end_borrow [[REDBORROW]] : $RedActorImpl
 // CHECK:       destroy_value [[RED]] : $RedActorImpl
 // CHECK: } // end sil function '$s4test13BlueActorImplC14createAndGreetyyYaF'
   func createAndGreet() async {
@@ -271,9 +271,9 @@ struct BlueActor {
 // CHECK:       hop_to_executor [[REDEXE]] : $RedActorImpl
       // ---- now invoke redFn, hop back to Blue, and clean-up ----
 // CHECK-NEXT:  {{%[0-9]+}} = apply [[CALLEE]]([[ARG]]) : $@convention(thin) (Int) -> ()
+// CHECK-NEXT:  end_borrow [[REDEXE]] : $RedActorImpl
+// CHECK-NEXT:  destroy_value [[R]] : $RedActorImpl
 // CHECK-NEXT:  hop_to_executor [[BLUEEXE]] : $BlueActorImpl
-// CHECK:       end_borrow [[REDEXE]] : $RedActorImpl
-// CHECK:       destroy_value [[R]] : $RedActorImpl
 // CHECK:       end_borrow [[BLUEEXE]] : $BlueActorImpl
 // CHECK:       destroy_value [[B]] : $BlueActorImpl
 // CHECK: } // end sil function '$s4test6blueFnyyYaF'
@@ -287,8 +287,9 @@ struct BlueActor {
 // CHECK:         [[BORROW:%[0-9]+]] = begin_borrow {{%[0-9]+}} : $RedActorImpl
 // CHECK-NEXT:    hop_to_executor [[BORROW]] : $RedActorImpl
 // CHECK-NEXT:    {{%[0-9]+}} = apply {{%[0-9]+}}({{%[0-9]+}}) : $@convention(thin) (Int) -> ()
-// CHECK-NEXT:    hop_to_executor [[GENERIC_EXEC]]
 // CHECK-NEXT:    end_borrow [[BORROW]] : $RedActorImpl
+// CHECK-NEXT:    destroy_value
+// CHECK-NEXT:    hop_to_executor [[GENERIC_EXEC]]
 // CHECK: } // end sil function '$s4test20unspecifiedAsyncFuncyyYaF'
 func unspecifiedAsyncFunc() async {
   await redFn(200)
@@ -315,7 +316,7 @@ func anotherUnspecifiedAsyncFunc(_ red : RedActorImpl) async {
 // CHECK: hop_to_executor [[RED:%[0-9]+]] : $RedActorImpl
 // CHECK-NEXT: begin_borrow
 // CHECK-NEXT: apply
-// CHECK-NEXT: hop_to_executor [[GENERIC_EXEC:%[0-9]+]] : $Optional<Builtin.Executor>
+// CHECK:      hop_to_executor [[GENERIC_EXEC:%[0-9]+]] : $Optional<Builtin.Executor>
 func testGlobalActorFuncValue(_ fn: @RedActor () -> Void) async {
   await fn()
 }
diff --git a/test/SILGen/hop_to_executor_async_prop.swift b/test/SILGen/hop_to_executor_async_prop.swift
@@ -123,9 +123,9 @@ func accessSweaterOfSweater(cat : Cat) async -> Sweater {
 
 // CHECK:    hop_to_executor [[GLOBAL_CAT]] : $Cat
 // CHECK:    [[THE_STRING:%[0-9]+]] = apply [[GETTER]]([[CAT]]) : $@convention(method) (@guaranteed Cat) -> @owned String
-// CHECK:    hop_to_executor [[GENERIC_EXEC]]
 // CHECK:    end_borrow [[GLOBAL_CAT]] : $Cat
 // CHECK:    destroy_value [[GLOBAL_CAT_REF]] : $Cat
+// CHECK:    hop_to_executor [[GENERIC_EXEC]]
 // CHECK:    return [[THE_STRING]] : $String
 // CHECK: } // end sil function '$s4test26accessGlobalIsolatedMember3catSSAA3CatC_tYaF'
 func accessGlobalIsolatedMember(cat : Cat) async -> String {
diff --git a/test/SILGen/objc_async.swift b/test/SILGen/objc_async.swift
@@ -301,3 +301,21 @@ extension OptionalMemberLookups {
     await self.generateMaybe!()
   }
 }
+
+
+// CHECK-LABEL: sil {{.*}} @$s10objc_async12checkHotdogsySSSgx_So8NSObjectCtYaKSo16HotdogCompetitorRzlF
+// CHECK: hop_to_executor {{.*}} : $MainActor
+// CHECK: [[AUTO_REL_STR:%.*]] = apply {{.*}}<some HotdogCompetitor>({{.*}}) : $@convention(objc_method)
+// CHECK: [[UNMANAGED_OPTIONAL:%.*]] = load [trivial] {{.*}} : $*@sil_unmanaged Optional<NSError>
+// CHECK: [[MANAGED_OPTIONAL:%.*]] = unmanaged_to_ref [[UNMANAGED_OPTIONAL]] : $@sil_unmanaged Optional<NSError> to $Optional<NSError>
+// CHECK: [[RETAINED_OPTIONAL:%.*]] = copy_value [[MANAGED_OPTIONAL]] : $Optional<NSError>
+// CHECK: [[MARKED:%.*]] = mark_dependence [[RETAINED_OPTIONAL]] : $Optional<NSError> on {{.*}} : $*Optional<NSError> // user: %32
+// CHECK: assign [[MARKED]] to {{.*}} : $*Optional<NSError>
+// CHECK: destroy_value {{.*}} : $MainActor
+// CHECK: dealloc_stack {{.*}} : $*AutoreleasingUnsafeMutablePointer<Optional<NSError>>
+// CHECK: dealloc_stack {{.*}} : $*@sil_unmanaged Optional<NSError>
+// CHECK: hop_to_executor {{.*}} : $Optional<Builtin.Executor>
+// CHECK: switch_enum
+func checkHotdogs(_ v: some HotdogCompetitor, _ timeLimit: NSObject) async throws -> String? {
+    return try await v.pileOfHotdogsToEat(withLimit: timeLimit)
+}
diff --git a/test/SILGen/toplevel_globalactorvars.swift b/test/SILGen/toplevel_globalactorvars.swift
@@ -66,8 +66,8 @@ await printFromMyActor(value: a)
 // CHECK: [[ACTORREF:%[0-9]+]] = begin_borrow {{%[0-9]+}} : $MyActorImpl
 // CHECK: hop_to_executor [[ACTORREF]] : $MyActorImpl
 // CHECK: {{%[0-9]+}} = apply [[PRINTFROMMYACTOR_FUNC]]([[AGLOBAL]])
-// CHECK: hop_to_executor [[MAIN_OPTIONAL]]
 // CHECK: end_borrow [[ACTORREF]]
+// CHECK: hop_to_executor [[MAIN_OPTIONAL]]
 
 if a < 10 {
 // CHECK: [[AACCESS:%[0-9]+]] = begin_access [read] [dynamic] [[AREF]] : $*Int
@@ -120,6 +120,6 @@ if a < 10 {
     // CHECK: [[ACTORREF:%[0-9]+]] = begin_borrow {{%[0-9]+}} : $MyActorImpl
     // CHECK: hop_to_executor [[ACTORREF]] : $MyActorImpl
     // CHECK: {{%[0-9]+}} = apply [[PRINTFROMMYACTOR_FUNC]]([[AGLOBAL]])
-    // CHECK: hop_to_executor [[MAIN_OPTIONAL]]
     // CHECK: end_borrow [[ACTORREF]]
+    // CHECK: hop_to_executor [[MAIN_OPTIONAL]]
 }
