Skip to content

Fix a stack-discipline bug with inlining coroutines #20335

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
Nov 5, 2018

Conversation

rjmccall
Copy link
Contributor

@rjmccall rjmccall commented Nov 5, 2018

Stack-allocating instructions in a function are required to obey a local stack discipline. Inlining a non-coroutine maintains this stack discipline in the caller, but coroutine inlining doesn't necessarily. That's because memory can be allocated after the begin_apply and deallocated after the end_apply, and in fact this is a somewhat likely output of SILGen if a temporary is required during an access. (Memory can also be allocated before the begin_apply and deallocated before the end_apply, and of course we need to handle that, but it's not a likely output of SILGen.)

One potential fix would be to require coroutine begin/end to participate in stack discipline, but that would create some pretty big problems for SILGen (which manages both access lifetime and temporary lifetime but uses different cleanup-stack mechanisms for them) and probably also the optimizers. So instead we just clean up stack lifetime by delaying deallocations using the existing StackNesting framework. As a compile-time optimization, we can avoid this if the callee doesn't have allocations that are live across the yield or if the caller's stack is the same at the end_apply sites as it was at begin_apply.

@rjmccall rjmccall requested a review from atrick November 5, 2018 22:15
@rjmccall
Copy link
Contributor Author

rjmccall commented Nov 5, 2018

@swift-ci Please smoke test.

@rjmccall rjmccall merged commit 5fc3645 into swiftlang:master Nov 5, 2018
@rjmccall rjmccall deleted the coro-inlining-stack-discipline branch November 5, 2018 23:35
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.

1 participant