Skip to content
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

Merged
merged 2 commits into from
Feb 15, 2022

Conversation

tshortli
Copy link
Contributor

@tshortli tshortli commented Feb 11, 2022

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

…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.
@tshortli
Copy link
Contributor Author

@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
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

@tshortli tshortli Feb 14, 2022

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)
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@tshortli tshortli merged commit 70e7e84 into swiftlang:main Feb 15, 2022
@tshortli tshortli deleted the emit-backdeploy-function-bodies branch February 18, 2022 17:07
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.

None yet

2 participants