[5.3] [semantic-arc-opts] Use all consuming uses instead of just destroying uses when validating if a LiveRange is alive in a scope. #32130
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… uses when validating if a LiveRange is alive in a scope. The reason why this is important is that if our destroy_value is elided due to the destroy_value being in a dead end block, we can promote a load [copy] to a load_borrow even if the load [copy] has a forwarding consuming use outside of a begin_access region. I changed every place in SemanticARCOpts that did this sort of thing to use this pattern instead of just destroys so that no one cargo cults the original pattern by mistake. <rdar://problem/61774105> (cherry picked from commit ba1ac78) Cherry-pick radar: rdar://63188362
|
@swift-ci test |
|
@swift-ci test |
|
Just redoing the test since it has been a minute since I put this up. |
|
Build failed |
|
Build failed |
|
The OS X failure was a Jenkins failure. And the Linux I think was one of those non-deterministic swiftpm ones: Going to retest |
|
@swift-ci test |
|
Build failed |
|
Going to wait for things to calm down on the CI |
|
Build failed |
|
@swift-ci test |
1 similar comment
|
@swift-ci test |
|
@swift-ci nominate |
tkremenek
approved these changes
Jun 10, 2020
AnthonyLatsis
added
swift 5.3
🍒 release cherry pick
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Just cherry-picking this fix.
rdar://63188362
Explanation: SemanticARCOpts is converting a legally leakable load [copy] from an access scope to a load_borrow in an harmless, incorrect way. Specifically to transform a load [copy] from an access_scope to a load_borrow, we need to validate that all uses of a load [copy] are within the access scope since load_borrows can only be used while the access_scope is valid. In the algorithm as written, we validate only that the destroy_value of the load [copy] are within that scope since all non-destroy uses will be earlier than such destroy_value. This is generally correct, except if our load [copy] is post-dominated by an unreachable. In such cases we may eliminate all destroy_value on the load [copy] since the program is going to end and we are going to leak the object. This can then cause us to ignore any non-destroy uses of the load [copy] outside of the access scope that would disqualify the load [copy] from being converted to a load_borrow. This change fixes the problem conservatively by just validating that /all/ uses of a load [copy] (rather than just destroy_value uses) are strictly within the access scope.
Scope: Fixes a miscompile. In terms of how likely this is in practice, I think it is not likely. That is because the optimizer is pretty aggressive about leaking objects on program exit. So given that we need to be post-dominated by unreachables, the base object is likely to be leaked rather than destroyed.
SR Issue: rdar://63188362
Risk: Low. I am only making the algorithm more conservative by adding all uses to a check instead of just the destroy_values.
Testing: Added a test case. Just run the compiler test suite.
Reviewer: @atrick