Skip to content

5.10: [OSSALifetimeCompletion] Handle unavailable blocks in dead-end regions. #68997

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

nate-chandler
Copy link
Contributor

Description: Fix overconsume on unreachable-terminated paths by handling conditional availability of values in dead-end regions in lifetime completion.

The function OSSALifetimeCompletion::visitUnreachableLifetimeEnds is used (1) to complete OSSA lifetimes and--while OSSA lifetime completion remains disabled--(2) to canonicalize lexical OSSA lifetimes. The function visits the instructions at which the lifetime implicitly ends--implicitly because with incomplete lifetimes, it's valid for a value not to be destroyed on paths that terminate in unreachable.

Previously, its determination for those ends was incorrect in the face of unavailable blocks in dead-end regions. Specifically, it simply walked forward from the non-lifetime-ending boundary until finding unreachable instructions. (Such unreachable instructions are guaranteed to exist because the lifetime is "partially complete" by precondition.) That walk alone is insufficient, however: It's possible that the value isn't available at those unreachable instructions. For example, if a value isn't destroyed on one path to the unreachable but is destroyed on another path to it, then the value isn't available at the unreachable. Inserting a destroy at the unreachable would result in an overconsume on the path on which the value was destroyed.

Here, the determination is fixed by computing availability within the region discovered by that initial walk. This is done via a forward iterative dataflow within the region, propagating availability. The visited instructions are then the terminators of the last blocks in the region in which the value is available. In the preceeding example, rather than visiting the unreachable instruction, the terminator of the last block on the path to it where the value has not yet been destroyed would be visited.

Risk: Low. The fix uses a well-known dataflow implementation to propagate availability.

Scope: Narrow. Only affects lifetimes in dead end blocks.

Original PR: #68975

Reviewed By: Andrew Trick ( @atrick )

Testing: Added tests.

Resolves: rdar://116255254

OSSALifetimeCompletion needs to insert not at unreachable instructions
that appear after the non-lifetime-ending boundary of a value but rather
at the terminators of the availability boundary of the value within that
region.  Once it does so, it will no longer be sufficient to check
whether the insertion point is an unreachable because such terminators
may be another terminator that appears on the availability boundary.
Prepare for that by recording the instructions that were found and
checking whether the destroy insertion point is such an instruction
before bailing rather than specifically checking for `unreachable`.
@nate-chandler nate-chandler marked this pull request as ready for review October 5, 2023 20:36
@nate-chandler nate-chandler requested a review from a team as a code owner October 5, 2023 20:36
@nate-chandler nate-chandler force-pushed the cherrypick/release/5.10/rdar116255254 branch from 091c492 to 0c28df9 Compare October 5, 2023 20:44
Not every block in a region which begins with the non-lifetime-ending
boundary of a value and ending with unreachable-terminated blocks has
the value available.  If the unreachable-terminated blocks in this
boundary are not available, it is incorrect to insert destroys of the
value in them: it is an overconsume on some paths.  Previously,
however, destroys were simply being inserted at the unreachable.

Here, this is fixed by finding the boundary of availability within that
region and inserting destroys before the terminators of the blocks on
that boundary.

rdar://116255254
@nate-chandler nate-chandler force-pushed the cherrypick/release/5.10/rdar116255254 branch from 0c28df9 to 983eaba Compare October 5, 2023 23:31
@nate-chandler
Copy link
Contributor Author

@swift-ci please test

@nate-chandler nate-chandler merged commit 37f652c into swiftlang:release/5.10 Oct 6, 2023
@nate-chandler nate-chandler deleted the cherrypick/release/5.10/rdar116255254 branch October 6, 2023 13:58
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.

2 participants