Skip to content

Add support for lifetime dependence in parameter position #75096

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 9 commits into from
Jul 11, 2024

Conversation

meg-gupta
Copy link
Contributor

@meg-gupta meg-gupta commented Jul 9, 2024

Extend support for lifetime dependence in parameter position:

func mayReassign(span: dependsOn(a) inout Span<Int>, to a: ContiguousArray<Int>) {
  span = a.span()
}

https://github.com/swiftlang/swift-evolution/blob/e814e62c17741dfe82e2f55eb1d751157c77a853/proposals/NNNN-lifetime-dependency.md#dependent-parameters contains a detailed explanation

@meg-gupta meg-gupta force-pushed the lifetimedeparg branch 2 times, most recently from 414b128 to bcc7ff2 Compare July 9, 2024 08:11
@meg-gupta
Copy link
Contributor Author

@swift-ci test

@meg-gupta
Copy link
Contributor Author

@swift-ci test

@meg-gupta
Copy link
Contributor Author

@swift-ci test

@meg-gupta
Copy link
Contributor Author

@swift-ci test

Copy link
Contributor

@xedin xedin left a comment

Choose a reason for hiding this comment

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

LGTM!

@meg-gupta
Copy link
Contributor Author

@swift-ci test

@meg-gupta
Copy link
Contributor Author

@atrick @eeckstein I'll make the suggested mangling changes as a follow on.

@DougGregor
Copy link
Member

rdar://129862031

Please provide a comment that describes what this pull request does, because radar numbers aren't useful outside of Apple.

@DougGregor
Copy link
Member

I do have a mangling question... I assume that we're not going to allow overloading based on the lifetime dependencies of a function. In that case, do we need to have a mangling for all of the lifetime dependencies?

@meg-gupta
Copy link
Contributor Author

meg-gupta commented Jul 10, 2024

@DougGregor Yeah, we won't be overloading on lifetime dependencies. I think we do need to mangle all lifetime dependencies (result, inout parameters, self at least) because adding or changing them is abi breaking since we need the correct mark_dependence SIL instructions in the caller.

So, we are mangling to ensure we don't end up with the same symbol when lifetime dependencies change.

@DougGregor
Copy link
Member

@DougGregor Yeah, we won't be overloading on lifetime dependencies. I think we do need to mangle all lifetime dependencies (result, inout parameters, self at least) because adding or changing them is abi breaking since we need the correct mark_dependence SIL instructions in the caller. So, mangling them will ensure we don't end up with the same symbol when lifetime dependencies change.

I thought it would be ABI-compatible to remove lifetime dependencies, and that might even be a likely course of evolution: an API might initially not document its dependencies, and rely on the the inferred dependencies. Then later they realize that the inferred dependencies were too broad and document the narrower lifetime dependencies. We can cope with this via @_silgen_name or some bespoke attribute to disable this part of the mangling, but I think it's worth considering whether it's more effort to mangle this information vs. dropping it. Not all aspects of ABI are described in the mangling.

@meg-gupta
Copy link
Contributor Author

I thought it would be ABI-compatible to remove lifetime dependencies, and that might even be a likely course of evolution: an API might initially not document its dependencies, and rely on the the inferred dependencies. Then later they realize that the inferred dependencies were too broad and document the narrower lifetime dependencies. We can cope with this via @_silgen_name or some bespoke attribute to disable this part of the mangling, but I think it's worth considering whether it's more effort to mangle this information vs. dropping it. Not all aspects of ABI are described in the mangling.

Okay - I thought we want to mangle all ABI breaking changes.
Yes, removing dependencies is not abi breaking.
I'll take a look at what @_silgen_name and explore what that involves before doing the follow up on mangling. Thanks for the suggestion.

@atrick
Copy link
Contributor

atrick commented Jul 10, 2024

I thought it would be ABI-compatible to remove lifetime dependencies, and that might even be a likely course of evolution: an API might initially not document its dependencies, and rely on the the inferred dependencies. Then later they realize that the inferred dependencies were too broad and document the narrower lifetime dependencies. We can cope with this via @_silgen_name or some bespoke attribute to disable this part of the mangling, but I think it's worth considering whether it's more effort to mangle this information vs. dropping it. Not all aspects of ABI are described in the mangling.

When a function result is nonescapable, we always mangle at least one dependence. There could be more than one dependence, and the source of that dependence could generally be any argument, so the ABI is sensitive to those changes.

Generally, we want any aspect of the ABI that is sensitive to source changes to be mangled simply to catch problems at link time rather than debugging miscompiles.

If the client adds ~Escapable to an argument type, then we want to catch any possibility that the API did not know about that constraint when it was compiled.

If the client removes ~Escapable from the return type, then we want to catch any possibility that the API assumed that constraint when it was compiled.

@atrick
Copy link
Contributor

atrick commented Jul 10, 2024

@meg-gupta, just to be sure we're on the same page...

For nonescapable results, lifetime dependence inference rules are independent of mangling. We always mangle whether it could be inferred or not. We could change/fix inferrence rules without risk of miscompile.

inout arguments are a little different. In the callee, we always assume a dependence on the incoming value. The outgoing value is always valid if it only depends on its incoming value. We allow the API to specify that an inout argument has additional dependencies on other arguments. Those additional dependencies need to be represented and mangled. But we have no way of suppressing the inout argument's dependence on itself.

@meg-gupta
Copy link
Contributor Author

@swift-ci test

@meg-gupta
Copy link
Contributor Author

meg-gupta commented Jul 10, 2024

@meg-gupta, just to be sure we're on the same page...

For nonescapable results, lifetime dependence inference rules are independent of mangling. We always mangle whether it could be inferred or not. We could change/fix inferrence rules without risk of miscompile.

Yes, we should mangle/serialize/ast print both inferred and explicit dependencies.

@atrick
Copy link
Contributor

atrick commented Jul 11, 2024

ModuleInterface/swift_build_sdk_interfaces/find-modules.test-sh appears to be flaky on Windows ☹️

@atrick
Copy link
Contributor

atrick commented Jul 11, 2024

@swift-ci test Windows Platform

@atrick atrick enabled auto-merge July 11, 2024 05:06
@atrick atrick merged commit 12acde2 into swiftlang:main Jul 11, 2024
4 of 5 checks passed
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.

5 participants