From 01d470ce3280c9d04ad32420aae1d9cd4dacaae3 Mon Sep 17 00:00:00 2001 From: Kavon Farvardin Date: Fri, 25 Feb 2022 16:30:25 -0700 Subject: [PATCH 1/2] Simplify lifetime extension for async ObjC calls. Since the clean-up is not unwind only, we can rely on the clean-ups to be emitted on both result and throw paths. This does slightly change when the hop_to_executor is emitted, in those paths, but that shouldn't matter. refactors solution for rdar://78982371 --- lib/SILGen/ManagedValue.cpp | 8 ++++---- lib/SILGen/ManagedValue.h | 2 +- lib/SILGen/SILGenApply.cpp | 25 +++++++++---------------- test/SILGen/objc_async.swift | 2 +- 4 files changed, 15 insertions(+), 22 deletions(-) diff --git a/lib/SILGen/ManagedValue.cpp b/lib/SILGen/ManagedValue.cpp index cad4fab9efeb4..98ab68d0e09b9 100644 --- a/lib/SILGen/ManagedValue.cpp +++ b/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. diff --git a/lib/SILGen/ManagedValue.h b/lib/SILGen/ManagedValue.h index a7702f226bb6e..4294a71ac450c 100644 --- a/lib/SILGen/ManagedValue.h +++ b/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. diff --git a/lib/SILGen/SILGenApply.cpp b/lib/SILGen/SILGenApply.cpp index 551eaaec4fdae..4993183588511 100644 --- a/lib/SILGen/SILGenApply.cpp +++ b/lib/SILGen/SILGenApply.cpp @@ -4573,7 +4573,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 unmanagedCopies; + SmallVector unmanagedCopies; if (calleeTypeInfo.foreign.async) { for (auto arg : args) { if (arg.hasCleanup()) { @@ -4660,12 +4660,14 @@ 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(value); - unmanagedCopies[i] = ManagedValue(value, Cleanups.getTopCleanup()); + // For objc async calls, push cleanup 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(unmanagedCopy); + } + } else { + assert(unmanagedCopies.empty()); } auto directResultsArray = makeArrayRef(directResults); @@ -4673,16 +4675,7 @@ RValue SILGenFunction::emitApply( 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); } diff --git a/test/SILGen/objc_async.swift b/test/SILGen/objc_async.swift index 3fad6864861f8..a5e9174a11d99 100644 --- a/test/SILGen/objc_async.swift +++ b/test/SILGen/objc_async.swift @@ -195,9 +195,9 @@ 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") } From afd26d397475fc7a54538718df45f1f0f66e36a8 Mon Sep 17 00:00:00 2001 From: Kavon Farvardin Date: Fri, 25 Feb 2022 18:31:52 -0700 Subject: [PATCH 2/2] [SR-15703] Fix missing hop in error path of foreign async throws call resolves rdar://89479707 --- lib/SILGen/ExecutorBreadcrumb.h | 5 ++++ lib/SILGen/SILGenApply.cpp | 29 ++++++++++++++---- test/SILGen/objc_async.swift | 40 +++++++++++++++++++++++++ test/SILGen/objc_async_from_swift.swift | 17 ++++++++--- 4 files changed, 82 insertions(+), 9 deletions(-) diff --git a/lib/SILGen/ExecutorBreadcrumb.h b/lib/SILGen/ExecutorBreadcrumb.h index 32c4edea1170e..e2734e488823d 100644 --- a/lib/SILGen/ExecutorBreadcrumb.h +++ b/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 diff --git a/lib/SILGen/SILGenApply.cpp b/lib/SILGen/SILGenApply.cpp index 4993183588511..170727d230a7f 100644 --- a/lib/SILGen/SILGenApply.cpp +++ b/lib/SILGen/SILGenApply.cpp @@ -4414,6 +4414,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 //===----------------------------------------------------------------------===// @@ -4660,12 +4681,14 @@ RValue SILGenFunction::emitApply( *foreignError, calleeTypeInfo.foreign.async); } - // For objc async calls, push cleanup to be used on + // 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(unmanagedCopy); } + // save breadcrumb as a clean-up so it is emitted in result / throw cases. + Cleanups.pushCleanup(std::move(breadcrumb)); } else { assert(unmanagedCopies.empty()); } @@ -4675,10 +4698,6 @@ RValue SILGenFunction::emitApply( directResultsArray, bridgedForeignError); assert(directResultsArray.empty() && "didn't claim all direct results"); - if (calleeTypeInfo.foreign.async) { - breadcrumb.emit(*this, loc); - } - return result; } diff --git a/test/SILGen/objc_async.swift b/test/SILGen/objc_async.swift index a5e9174a11d99..bd2f04a309ebe 100644 --- a/test/SILGen/objc_async.swift +++ b/test/SILGen/objc_async.swift @@ -201,3 +201,43 @@ func testSlowServerFromMain(slowServer: SlowServer) async throws { // 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 ([[RAW_CONT]] : $Builtin.RawUnsafeContinuation) +// CHECK: [[STORE_ALLOC:%.*]] = alloc_stack $@block_storage UnsafeContinuation +// CHECK: [[PROJECTED:%.*]] = project_block_storage [[STORE_ALLOC]] : $*@block_storage +// CHECK: store [[CONT]] to [trivial] [[PROJECTED]] : $*UnsafeContinuation +// 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, Optional) -> ()>, SlowServer) -> () +// CHECK: [[STRING_ARG_COPY:%.*]] = copy_value [[STRING_ARG]] : $NSString +// CHECK: dealloc_stack [[STORE_ALLOC]] : $*@block_storage UnsafeContinuation +// 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" + } +} diff --git a/test/SILGen/objc_async_from_swift.swift b/test/SILGen/objc_async_from_swift.swift index 85dd87c9c83ed..40500c664eebf 100644 --- a/test/SILGen/objc_async_from_swift.swift +++ b/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, Optional) -> (), τ_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, Optional) -> (), τ_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, #Optional.none + // CHECK: hop_to_executor [[GENERIC_EXECUTOR]] : + // CHECK: objc_method {{.*}} $@convention(objc_method) <τ_0_0 where τ_0_0 : SlowServing> (@convention(block) (Optional, Optional) -> (), τ_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, #Optional.none