Merge pull request #58476 from kavon/5.7-undo-fix-cont-jump-to-null

[5.7][🍒] Back out "Fix jump-to-null issue for ObjC async calls."

Author: kavon

lib/SIL/IR/SILInstruction.cpp

@@ -1646,6 +1646,10 @@ bool SILInstruction::maySuspend() const {
   // await_async_continuation always suspends the current task.
   if (isa<AwaitAsyncContinuationInst>(this))
     return true;
+
+  // hop_to_executor also may cause a suspension
+  if (isa<HopToExecutorInst>(this))
+    return true;
 
   // Fully applying an async function may suspend the caller.
   if (auto applySite = FullApplySite::isa(const_cast<SILInstruction*>(this))) {

lib/SILGen/ExecutorBreadcrumb.h

@@ -37,6 +37,11 @@ class ExecutorBreadcrumb {
   // Emits the hop back sequence, if any, necessary to get back to
   // the executor represented by this breadcrumb.
   void emit(SILGenFunction &SGF, SILLocation loc);
+
+#ifndef NDEBUG
+  // FOR ASSERTS ONLY: returns true if calling `emit` will emit a hop-back.
+  bool needsEmit() const { return mustReturnToExecutor; }
+#endif
 };
 
 } // namespace Lowering

lib/SILGen/ManagedValue.cpp

@@ -58,20 +58,20 @@ ManagedValue ManagedValue::copy(SILGenFunction &SGF, SILLocation loc) const {
 
 // Emit an unmanaged copy of this value
 // WARNING: Callers of this API should manage the cleanup of this value!
-ManagedValue ManagedValue::unmanagedCopy(SILGenFunction &SGF,
+SILValue ManagedValue::unmanagedCopy(SILGenFunction &SGF,
                                          SILLocation loc) const {
   auto &lowering = SGF.getTypeLowering(getType());
   if (lowering.isTrivial())
-    return *this;
+    return getValue();
 
   if (getType().isObject()) {
     auto copy = SGF.B.emitCopyValueOperation(loc, getValue());
-    return ManagedValue::forUnmanaged(copy);
+    return copy;
   }
 
   SILValue buf = SGF.emitTemporaryAllocation(loc, getType());
   SGF.B.createCopyAddr(loc, getValue(), buf, IsNotTake, IsInitialization);
-  return ManagedValue::forUnmanaged(buf);
+  return buf;
 }
 
 /// Emit a copy of this value with independent ownership.

lib/SILGen/ManagedValue.h

@@ -280,7 +280,7 @@ class ManagedValue {
 
   /// Returns an unmanaged copy of this value.
   /// WARNING: Callers of this API should manage the cleanup of this value!
-  ManagedValue unmanagedCopy(SILGenFunction &SGF, SILLocation loc) const;
+  SILValue unmanagedCopy(SILGenFunction &SGF, SILLocation loc) const;
 
   /// Emit a copy of this value with independent ownership into the current
   /// formal evaluation scope.

lib/SILGen/SILGenApply.cpp

@@ -4427,6 +4427,27 @@ class FixLifetimeDestroyCleanup : public Cleanup {
 #endif
   }
 };
+
+class EmitBreadcrumbCleanup : public Cleanup {
+  ExecutorBreadcrumb breadcrumb;
+
+public:
+  EmitBreadcrumbCleanup(ExecutorBreadcrumb &&breadcrumb)
+    : breadcrumb(std::move(breadcrumb)) {}
+
+  void emit(SILGenFunction &SGF, CleanupLocation l,
+            ForUnwind_t forUnwind) override {
+    breadcrumb.emit(SGF, l);
+  }
+
+  void dump(SILGenFunction &SGF) const override {
+#ifndef NDEBUG
+    llvm::errs() << "EmitBreadcrumbCleanup "
+                 << "State:" << getState()
+                 << "NeedsEmit:" << breadcrumb.needsEmit();
+#endif
+  }
+};
 } // end anonymous namespace
 
 //===----------------------------------------------------------------------===//
@@ -4586,7 +4607,7 @@ RValue SILGenFunction::emitApply(
   // generates `await_async_continuation`.
   // Lifetime is extended by creating unmanaged copies here and by pushing the
   // cleanups required just before the result plan is generated.
-  SmallVector<ManagedValue, 8> unmanagedCopies;
+  SmallVector<SILValue, 8> unmanagedCopies;
   if (calleeTypeInfo.foreign.async) {
     for (auto arg : args) {
       if (arg.hasCleanup()) {
@@ -4673,32 +4694,23 @@ RValue SILGenFunction::emitApply(
                               *foreignError, calleeTypeInfo.foreign.async);
   }
 
-  // For objc async calls, push cleanup to be used on throw paths in the result
-  // planner.
-  for (unsigned i : indices(unmanagedCopies)) {
-    SILValue value = unmanagedCopies[i].getValue();
-    Cleanups.pushCleanup<FixLifetimeDestroyCleanup>(value);
-    unmanagedCopies[i] = ManagedValue(value, Cleanups.getTopCleanup());
+  // For objc async calls, push cleanups to be used on
+  // both result and throw paths prior to finishing the result plan.
+  if (calleeTypeInfo.foreign.async) {
+    for (auto unmanagedCopy : unmanagedCopies) {
+      Cleanups.pushCleanup<FixLifetimeDestroyCleanup>(unmanagedCopy);
+    }
+    // save breadcrumb as a clean-up so it is emitted in result / throw cases.
+    Cleanups.pushCleanup<EmitBreadcrumbCleanup>(std::move(breadcrumb));
+  } else {
+    assert(unmanagedCopies.empty());
   }
 
   auto directResultsArray = makeArrayRef(directResults);
   RValue result = resultPlan->finish(*this, loc, substResultType,
                                      directResultsArray, bridgedForeignError);
   assert(directResultsArray.empty() && "didn't claim all direct results");
 
-  // For objc async calls, generate cleanup on the resume path here and forward
-  // the previously pushed cleanups.
-  if (calleeTypeInfo.foreign.async) {
-    for (auto unmanagedCopy : unmanagedCopies) {
-      auto value = unmanagedCopy.forward(*this);
-      B.emitFixLifetime(loc, value);
-      B.emitDestroyOperation(loc, value);
-    }
-
-    // hop back to the current executor
-    breadcrumb.emit(*this, loc);
-  }
-
   return result;
 }
 

test/SIL/verifier-no-nested-suspend.sil

@@ -0,0 +1,26 @@
+// RUN: not --crash %target-sil-opt %s 2>&1 | %FileCheck %s
+// REQUIRES: asserts
+
+// CHECK: cannot suspend async task while unawaited continuation is active
+
+sil_stage raw
+
+import Builtin
+import Swift
+import SwiftShims
+import _Concurrency
+
+// hello(_:)
+sil hidden [ossa] @$s4main5helloyS2bYaF : $@convention(thin) @async (Bool) -> Bool {
+bb0(%0 : $Bool):
+  debug_value %0 : $Bool, let, name "b", argno 1
+  %2 = enum $Optional<Builtin.Executor>, #Optional.none!enumelt
+  %22 = alloc_stack $Bool
+  %27 = get_async_continuation_addr Bool, %22 : $*Bool
+  hop_to_executor %2 : $Optional<Builtin.Executor> // <- the bad nested suspension
+  await_async_continuation %27 : $Builtin.RawUnsafeContinuation, resume bb1
+
+bb1:
+  dealloc_stack %22 : $*Bool
+  return %0 : $Bool
+}

test/SILGen/objc_async.swift

@@ -195,9 +195,49 @@ func testSlowServerFromMain(slowServer: SlowServer) async throws {
   // CHECK: await_async_continuation [[CONT]] {{.*}}, resume [[RESUME:bb[0-9]+]]
   // CHECK: [[RESUME]]:
   // CHECK: [[RESULT:%.*]] = load [trivial] [[RESUME_BUF]]
+  // CHECK: hop_to_executor %6 : $MainActor
   // CHECK: fix_lifetime [[COPY]]
   // CHECK: destroy_value [[COPY]]
-  // CHECK: hop_to_executor %6 : $MainActor
   // CHECK: dealloc_stack [[RESUME_BUF]]
   let _: Int = await slowServer.doSomethingSlow("mail")
 }
+
+// CHECK-LABEL: sil {{.*}}@${{.*}}26testThrowingMethodFromMain
+@MainActor
+func testThrowingMethodFromMain(slowServer: SlowServer) async -> String {
+// CHECK:  [[RESULT_BUF:%.*]] = alloc_stack $String
+// CHECK:  [[STRING_ARG:%.*]] = apply {{%.*}}({{%.*}}) : $@convention(method) (@guaranteed String) -> @owned NSString
+// CHECK:  [[METH:%.*]] = objc_method {{%.*}} : $SlowServer, #SlowServer.doSomethingDangerous!foreign
+// CHECK:  [[RAW_CONT:%.*]] = get_async_continuation_addr [throws] String, [[RESULT_BUF]] : $*String
+// CHECK:  [[CONT:%.*]] = struct $UnsafeContinuation<String, Error> ([[RAW_CONT]] : $Builtin.RawUnsafeContinuation)
+// CHECK:  [[STORE_ALLOC:%.*]] = alloc_stack $@block_storage UnsafeContinuation<String, Error>
+// CHECK:  [[PROJECTED:%.*]] = project_block_storage [[STORE_ALLOC]] : $*@block_storage
+// CHECK:  store [[CONT]] to [trivial] [[PROJECTED]] : $*UnsafeContinuation<String, Error>
+// CHECK:  [[INVOKER:%.*]] = function_ref @$sSo8NSStringCSgSo7NSErrorCSgIeyByy_SSTz_
+// CHECK:  [[BLOCK:%.*]] = init_block_storage_header [[STORE_ALLOC]] {{.*}}, invoke [[INVOKER]]
+// CHECK:  [[OPTIONAL_BLK:%.*]] = enum {{.*}}, #Optional.some!enumelt, [[BLOCK]]
+// CHECK:  %28 = apply [[METH]]([[STRING_ARG]], [[OPTIONAL_BLK]], {{%.*}}) : $@convention(objc_method) (NSString, Optional<@convention(block) (Optional<NSString>, Optional<NSError>) -> ()>, SlowServer) -> ()
+// CHECK:  [[STRING_ARG_COPY:%.*]] = copy_value [[STRING_ARG]] : $NSString
+// CHECK:  dealloc_stack [[STORE_ALLOC]] : $*@block_storage UnsafeContinuation<String, Error>
+// CHECK:  destroy_value [[STRING_ARG]] : $NSString
+// CHECK:  await_async_continuation [[RAW_CONT]] : $Builtin.RawUnsafeContinuation, resume [[RESUME:bb[0-9]+]], error [[ERROR:bb[0-9]+]]
+
+// CHECK: [[RESUME]]
+// CHECK:   {{.*}} = load [take] [[RESULT_BUF]] : $*String
+// CHECK:   hop_to_executor {{%.*}} : $MainActor
+// CHECK:   fix_lifetime [[STRING_ARG_COPY]] : $NSString
+// CHECK:   destroy_value [[STRING_ARG_COPY]] : $NSString
+// CHECK:   dealloc_stack [[RESULT_BUF]] : $*String
+
+// CHECK: [[ERROR]]
+// CHECK:   hop_to_executor {{%.*}} : $MainActor
+// CHECK:   fix_lifetime [[STRING_ARG_COPY]] : $NSString
+// CHECK:   destroy_value [[STRING_ARG_COPY]] : $NSString
+// CHECK:   dealloc_stack [[RESULT_BUF]] : $*String
+
+  do {
+    return try await slowServer.doSomethingDangerous("run-with-scissors")
+  } catch {
+    return "none"
+  }
+}

test/SILGen/objc_async_from_swift.swift

@@ -25,19 +25,28 @@ func testSlowServing(p: SlowServing) async throws {
     // CHECK: hop_to_executor [[GENERIC_EXECUTOR]] :
     let _: String = await p.requestString()
 
-    // CHECK: objc_method {{.*}} $@convention(objc_method) <τ_0_0 where τ_0_0 : SlowServing> (@convention(block) (Optional<NSString>, Optional<NSError>) -> (), τ_0_0) -> ()
-    // CHECK: hop_to_executor [[GENERIC_EXECUTOR]] :
-    let _: String = try await p.tryRequestString()
-
     // CHECK: objc_method {{.*}} $@convention(objc_method) <τ_0_0 where τ_0_0 : SlowServing> (@convention(block) (Int, NSString) -> (), τ_0_0) -> ()
     // CHECK: hop_to_executor [[GENERIC_EXECUTOR]] :
     let _: (Int, String) = await p.requestIntAndString()
 
     // CHECK: objc_method {{.*}} $@convention(objc_method) <τ_0_0 where τ_0_0 : SlowServing> (@convention(block) (Int, Optional<NSString>, Optional<NSError>) -> (), τ_0_0) -> ()
     // CHECK: hop_to_executor [[GENERIC_EXECUTOR]] :
+    // CHECK:      builtin "willThrow"
+    // CHECK-NEXT: hop_to_executor [[GENERIC_EXECUTOR]] :
     let _: (Int, String) = try await p.tryRequestIntAndString()
 }
 
+// CHECK-LABEL: sil {{.*}}@{{.*}}20testSlowServingAgain
+func testSlowServingAgain(p: SlowServing) async throws {
+  // CHECK: [[GENERIC_EXECUTOR:%.*]] = enum $Optional<Builtin.Executor>, #Optional.none
+  // CHECK: hop_to_executor [[GENERIC_EXECUTOR]] :
+  // CHECK: objc_method {{.*}} $@convention(objc_method) <τ_0_0 where τ_0_0 : SlowServing> (@convention(block) (Optional<NSString>, Optional<NSError>) -> (), τ_0_0) -> ()
+  // CHECK: hop_to_executor [[GENERIC_EXECUTOR]] :
+  // CHECK:      builtin "willThrow"
+  // CHECK-NEXT: hop_to_executor [[GENERIC_EXECUTOR]] :
+  let _: String = try await p.tryRequestString()
+}
+
 class SlowSwiftServer: NSObject, SlowServing {
     // CHECK-LABEL: sil {{.*}} @$s21objc_async_from_swift15SlowSwiftServerC10requestIntSiyYaF
     // CHECK:         [[GENERIC_EXECUTOR:%.*]] = enum $Optional<Builtin.Executor>, #Optional.none