Skip to content

Concurrency: fix inconsistent _asyncLet_get signatures #41487

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

Conversation

kateinoigakukun
Copy link
Member

@kateinoigakukun kateinoigakukun commented Feb 20, 2022

The implementation of swift_asyncLet_get asynchronously returns only async context, and the result is written in the given buffer. But its Swift level declaration in Concurrency module has async -> Builtin.RawPointer result.
This extra result type affects the number of parameters of a resumption function that is passed as a continuation function of swift_asyncLet_get, and it results in having an extra parameter in a resumption function.
But the implementation only passes only async context to the resumption function, so callee and caller signatures are mismatched. It causes crash in WebAssembly target.

This patch fixes the signature inconsistency between the caller and callee assumptions.

https://github.com/apple/swift/blob/b0043966cd868e06fdebbfe1ae16b20d954d8089/stdlib/public/Concurrency/AsyncLet.cpp#L228-L247

Link: swiftwasm#4288

@kateinoigakukun
Copy link
Member Author

@swift-ci Please smoke test

@kateinoigakukun
Copy link
Member Author

@swift-ci please test Windows platform

@kateinoigakukun
Copy link
Member Author

preset=buildbot_incremental_linux_crosscompile_wasm
@swift-ci please test with preset Linux Platform

@kateinoigakukun
Copy link
Member Author

kateinoigakukun commented Feb 20, 2022

This is an ABI breaking change, but all use of swift_asyncLet_get in the compiler did not expect the removed result value, so I think it doesn't break anything if no one uses the declaration other than the compiler. But I'm not confident here. WDYT @jckarter ?

@MaxDesiatov
Copy link
Contributor

@swift-ci please test macOS platform

@kateinoigakukun kateinoigakukun force-pushed the katei/fix-asynclet-get-sigs-upstream branch from 5c7c328 to 61e093e Compare March 30, 2022 09:42
@kateinoigakukun
Copy link
Member Author

@swift-ci please test

@kateinoigakukun kateinoigakukun force-pushed the katei/fix-asynclet-get-sigs-upstream branch from 61e093e to 588cd71 Compare March 30, 2022 11:57
@kateinoigakukun kateinoigakukun force-pushed the katei/fix-asynclet-get-sigs-upstream branch from 588cd71 to 30b9fc9 Compare March 30, 2022 11:58
@kateinoigakukun
Copy link
Member Author

@swift-ci please test

Func _asyncLet_get(_:_:) has mangled name changing from '_Concurrency._asyncLet_get(Builtin.RawPointer, Builtin.RawPointer) async -> Builtin.RawPointer' to '_Concurrency._asyncLet_get(Builtin.RawPointer, Builtin.RawPointer) async -> ()'
Func _asyncLet_get(_:_:) has return type change from Builtin.RawPointer to ()
Func _asyncLet_get_throwing(_:_:) has mangled name changing from '_Concurrency._asyncLet_get_throwing(Builtin.RawPointer, Builtin.RawPointer) async throws -> Builtin.RawPointer' to '_Concurrency._asyncLet_get_throwing(Builtin.RawPointer, Builtin.RawPointer) async throws -> ()'
Func _asyncLet_get_throwing(_:_:) has return type change from Builtin.RawPointer to ()
Copy link
Contributor

Choose a reason for hiding this comment

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

The declarations are annotated with _silgen_name, so the emitted symbols ought to remain the same even though the signature changed. Is this just a bug in the API digester that it flags it as an ABI change?

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems there was a patch to use mangled name instead of USR #39846 but reverted for some reason. cf. #39906 #39920 (comment)

@nkcsgexi Could you tell me the reason if possible?

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. This change looks like it should be ABI-neutral as long as I'm not missing something.

@kateinoigakukun
Copy link
Member Author

OK, let's go ahead. The api-digester issue will be fixed in another PR. CC @nkcsgexi

@kateinoigakukun kateinoigakukun merged commit 3c805ee into swiftlang:main Apr 8, 2022
@kateinoigakukun kateinoigakukun deleted the katei/fix-asynclet-get-sigs-upstream branch April 8, 2022 09:53
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