Skip to content

[SILGen] avoid hop before autoreleased foreign error is retained #68415

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 20, 2023

Conversation

kavon
Copy link
Member

@kavon kavon commented Sep 9, 2023

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, because switching actors will trigger the autorelease. We weren't doing that retain beforehand 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

@kavon
Copy link
Member Author

kavon commented Sep 9, 2023

@swift-ci please smoke test

@kavon kavon force-pushed the autoreleasing-isolated-foreign-errors branch 2 times, most recently from 40a184e to fd0a16c Compare September 12, 2023 01:09
@kavon
Copy link
Member Author

kavon commented Sep 12, 2023

@swift-ci please smoke test

@kavon kavon marked this pull request as ready for review September 12, 2023 01:10
@kavon kavon requested review from jckarter and beccadax September 12, 2023 01:11
@kavon
Copy link
Member Author

kavon commented Sep 27, 2023

@swift-ci please smoke test

@kavon kavon force-pushed the autoreleasing-isolated-foreign-errors branch from fd0a16c to b0a6f67 Compare November 14, 2023 00:23
@kavon
Copy link
Member Author

kavon commented Nov 14, 2023

@swift-ci please test

@kavon kavon enabled auto-merge November 14, 2023 00:24
@kavon
Copy link
Member Author

kavon commented Nov 29, 2023

@swift-ci please test

@kavon kavon force-pushed the autoreleasing-isolated-foreign-errors branch from b0a6f67 to 1f099f5 Compare December 19, 2023 03:40
@kavon
Copy link
Member Author

kavon commented Dec 19, 2023

@swift-ci smoke test

@kavon
Copy link
Member Author

kavon commented Dec 19, 2023

@swift-ci Please Build Toolchain macOS Platform

@kavon kavon force-pushed the autoreleasing-isolated-foreign-errors branch from 1f099f5 to bd11b28 Compare December 19, 2023 20:13
@kavon kavon requested a review from ktoso as a code owner December 19, 2023 20:13
@kavon
Copy link
Member Author

kavon commented Dec 19, 2023

@swift-ci please smoke test

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
@kavon kavon force-pushed the autoreleasing-isolated-foreign-errors branch from bd11b28 to 06ac850 Compare December 19, 2023 21:17
@kavon
Copy link
Member Author

kavon commented Dec 19, 2023

@swift-ci smoke test

@kavon
Copy link
Member Author

kavon commented Dec 19, 2023

linux failure unrelated

@kavon
Copy link
Member Author

kavon commented Dec 19, 2023

@swift-ci please smoke test linux platform

@kavon kavon merged commit 25a6838 into swiftlang:main Dec 20, 2023
@kavon kavon deleted the autoreleasing-isolated-foreign-errors branch December 20, 2023 01:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants