-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Emit function bodies for functions annotated with @_backDeploy #41348
Emit function bodies for functions annotated with @_backDeploy #41348
Conversation
Resolves rdar://88650341
…rsing for the attribute so that -verify-syntax-tree passes. The SILGen test doesn't verify anything special yet since we're not emitting thunks yet at call sites.
@swift-ci please test |
@@ -17,27 +17,24 @@ | |||
|
|||
public struct TopLevelStruct { | |||
// CHECK: @_backDeploy(macOS 11.0) | |||
// CHECK: public func backDeployedFunc1_SinglePlatform() -> Swift.Int | |||
// FROMSOURCE: public func backDeployedFunc1_SinglePlatform() -> Swift.Int { return 42 } | |||
// FROMMODULE: public func backDeployedFunc1_SinglePlatform() -> Swift.Int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it expected to lose the inlinable code when serialized?
I'm aware of some issues with losing classic @inlinable
code with merge-module, that might explain it and may not be worth fixing in such a case. It was one source of broken swiftinterfaces when they were generated in incremental/merge-module mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll see something similar in the inlinable-functions.swift
test case; I'm not sure if it's correct behavior but it does seem like this is currently expected to happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting... I never looked closely at inlinable-function.swift
. IIUC the way the test is set up, it's testing a behavior that's not applied in practice, it would be compiling a swiftinterface in the SDK to a cached swiftmodule, and then generating a swiftinterface for it from the cached swiftmodule. Back to this test, I think we should expect to keep the inlinable code after merge-module, but it's merge-module so we don't need to support it anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe there's a better way to accomplish what I'm trying to accomplish with that aspect of the test, which is just to verify that the attribute can be deserialized from a serialized module successfully. I think on a previous PR we talked about how this step is maybe unnecessary once there is another test case where there are separate client and library source files, compiled separately
|
||
// CHECK: @_backDeploy(macOS 11.0) | ||
// CHECK: @_backDeploy(iOS 14.0) | ||
// CHECK: public func backDeployedFunc2_MultiPlatform() -> Swift.Int | ||
// FROMSOURCE: public func backDeployedFunc2_MultiPlatform() -> Swift.Int { return 43 } | ||
// FROMMODULE: public func backDeployedFunc2_MultiPlatform() -> Swift.Int | ||
@available(macOS 12.0, *) | ||
@_backDeploy(macOS 11.0, iOS 14.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably from the previous PR but I just noticed it. The logic here uses @available
to describe the OS version with the function in the dylib and @_backDeploy
points to older OS versions where we can emit the code in the clients, is that right?
Is there a specific motivation for this order? I'm wondering if having @available
pointing to the older OS where this is first available with no concern for back-deployment, wouldn't make it easier to support existing availability checking. Then we'd need to concern ourselves with the @_backDeploy
's version only for inlining and linking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, your description of how they are intended to interact is right. There were a couple reasons I went with this design:
- It makes it very simple to add back deployment to an existing declaration since you only need to add the new attribute and fill it in.
- It keeps the effect of
@available
on ABI unchanged since the attributed declaration continues to only indicate that the declaration became ABI starting in the introduced OS versions.
I was thinking of this as the least disruptive implementation and wanted to wait for community feedback on the design before refining it, but maybe I am underestimating how much would need to change in availability checking to ensure the current design works.
Emit function bodies for functions with
@_backDeploy
into.swiftinterface
files. Also, add a basic SILGen test for the@_backDeploy
attribute and fix parsing for the attribute so that-verify-syntax-tree
passes. The SILGen test will become more interesting when thunks are generated for the back deployed calls.Resolves rdar://88650341