Skip to content

[SILGen] Use opaque AP for ObjC-async returns. #42485

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
Apr 20, 2022

Conversation

nate-chandler
Copy link
Contributor

@nate-chandler nate-chandler commented Apr 20, 2022

Previously, the AbstractionPattern that was used for the value "returned" (i.e. via a completion handler) from ObjC mostly (but not quite always) was "type".

The generated completion handler correctly (because this is required in order to call _resumeUnsafeContinuation) reabstracted the block (e.g. from @convention(block) () -> ()) to @substituted <T> () -> @out T for <()>). The callee of the ObjC function, however, loaded the function from the block as if it were not reabstracted (e.g. () -> ()).

On arm64e, that difference in types caused in a difference in pointer signing, resulting in a failure at runtime.

rdar://85526879
rdar://85526916

Previously, the AbstractionPattern that was used for the value
"returned" (i.e. via a completion handler) from ObjC mostly (but not
quite always) was "type".

The generated completion handler correctly (because this is required in
order to call _resumeUnsafeContinuation) reabstracted the block (e.g.
from @convention(block) to @Substituted <T> () -> @out T for <()>).  The
callee of the ObjC function, however, loaded the function from the block
as if it were not reabstracted (e.g. () -> ()).

On most platforms, that happened to work.  On arm64e, that difference in
types caused in a difference in pointer signing, resulting in a failure
at runtime.

rdar://85526879
rdar://85526916
@nate-chandler
Copy link
Contributor Author

@swift-ci please test

@nate-chandler
Copy link
Contributor Author

@swift-ci Please test Apple Silicon macOS Platform

@nate-chandler
Copy link
Contributor Author

@swift-ci please test macOS platform

Copy link
Contributor

@aschwaighofer aschwaighofer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me but I would defer to @jckarter

Copy link
Contributor

@jckarter jckarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks right. Maybe we could add an execution test that tries to call through the function value too—if it's not correctly abstracted, then we're likely to crash on any platform, not just arm64e.

@nate-chandler
Copy link
Contributor Author

nate-chandler commented Apr 20, 2022

Definitely! Actually the execution tests do call through the function value that's returned. That's what was failing on arm64e. For example,

print(try await object.performSingleFlaggy1()?())

@jckarter
Copy link
Contributor

Interesting that that wouldn't crash on other platforms already too. Maybe an execution test that passes through a function with nontrivial arguments and returns would be more likely to expose the mismatch.

nate-chandler added a commit to nate-chandler/swift that referenced this pull request Apr 20, 2022
@nate-chandler
Copy link
Contributor Author

Added more execution tests which return differently shaped function pointers in #42508 .

@nate-chandler nate-chandler merged commit 45ec725 into swiftlang:main Apr 20, 2022
@nate-chandler nate-chandler deleted the rdar85526916 branch April 20, 2022 23:43
nate-chandler added a commit to nate-chandler/swift that referenced this pull request Apr 21, 2022
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.

3 participants