Fix a bug in CopyForwarding. Bailout during destoy hoisting. #19103
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
Once the algorithm has begun hoisting destroys globally, there's no way to cleanly bailout. The previous attempt to bailout could result in an assert or lost destroy in release mode. This is continued fall-out from changes in the previous release to upstream SILGen or mandatory passes, such as PredictableMemOps, that no longer preserve natural variable lifetimes. In this case, we end up with SIL like this before CopyForwarding: bb(%arg) %local_addr = alloc_stack store %arg to %local_addr %payload = switch_enum(%arg) retain %arg store %arg to %some_addr destroy_addr %local_addr release_value %arg We're attempting to hoist the destroy_addr to its last use, but can't because the lifetimes of the alloc_stack (%local_addr) and the value being stored on the stack (%arg) have become mixed up by an upstream pass. We actually detect this situation now in order to bail-out of destroy hoisting. Sadly, the bailout might only partially recover in the case of interesting control flow, as happens in the test case's Graph.init function. This triggers an assert, but in release mode it simply drops the destroy. Fixed <rdar://problem/43888666> [SR-8526]: Memory leak after switch in release configuration.
|
@swift-ci test. |
|
Build failed |
|
@swift-ci test. |
|
Build failed |
|
lldb-Suite :: api/check_public_api_headers/TestPublicAPIHeaders.py' FAILED |
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.
Once the algorithm has begun hoisting destroys globally, there's no
way to cleanly bailout. The previous attempt to bailout could result
in an assert or lost destroy in release mode.
This is continued fall-out from changes in the previous release to
upstream SILGen or mandatory passes, such as PredictableMemOps, that
no longer preserve natural variable lifetimes.
In this case, we end up with SIL like this before CopyForwarding:
bb(%arg)
%local_addr = alloc_stack
store %arg to %local_addr
%payload = switch_enum(%arg)
retain %arg
store %arg to %some_addr
destroy_addr %local_addr
release_value %arg
We're attempting to hoist the destroy_addr to its last use, but can't
because the lifetimes of the alloc_stack (%local_addr) and the value
being stored on the stack (%arg) have become mixed up by an upstream
pass. We actually detect this situation now in order to bail-out of
destroy hoisting. Sadly, the bailout might only partially recover in
the case of interesting control flow, as happens in the test case's
Graph.init function. This triggers an assert, but in release mode it
simply drops the destroy.
Fixed rdar://problem/43888666 [SR-8526]: Memory leak after switch in
release configuration.