Fix mark_dependence [nonescaping] ownership #79177
Merged
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
This test case would always pass, even without the fix it was intending to test: commit 79b6498 Author: Meghana Gupta <[email protected]> Date: Wed Jan 22 01:24:49 2025 Fix operand ownership of mark_dependence [nonescaping] of address values
This is necessary to fix a recent OSSA bug that breaks common occurrences on mark_dependence [nonescaping]. Rather than reverting that change above, we make forward progress toward implicit borrows scopes, as was the original intention. In the near future, all InteriorPointer instructions will create an implicit borrow scope. This means we have the option of not emitting extraneous begin/end_borrow instructions around intructions like ref_element_addr, open_existential, and project_box. After that, we can also migrate GuaranteedForwarding instructions like tuple_extract and struct_extract.
This fixes an OSSA verification bug introduced here:
commit 79b6498
Author: Meghana Gupta <[email protected]>
Date: Wed Jan 22 01:24:49 2025
Fix operand ownership of mark_dependence [nonescaping] of address values
The bug only showed up with mark_dependence [nonescaping], which means mainly
affects `~Escapable` types. Adddressors happened to work because SILGen was
emitting a borrow scope around them.
Rather than reverting the change above, this fix migrates mark_dependence
[nonescaping] to an implicit borrow scope.
Fixes rdar://144199759 (Assert: mark_dependence [nonescaping]:
ownership incompatible with an owned value)
|
LGTM! |
|
@swift-ci test |
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.
Temporarily introduce AnyInteriorPointer operand ownership.
This is necessary to fix a recent OSSA bug that breaks common occurrences on mark_dependence [nonescaping]. Rather than reverting that change above, we make forward progress toward implicit borrows scopes, as was the original intention.
This fixes an OSSA verification bug:
introduced here:
The bug only showed up with mark_dependence [nonescaping], which mainly affected
~Escapabletypes. Adddressors happened to work because SILGen was emitting a borrow scope around them.Rather than reverting the change above, this fix migrates mark_dependence [nonescaping] to an implicit borrow scope.
In the near future, all InteriorPointer instructions will create an implicit borrow scope. This means we have the option of not emitting extraneous begin/end_borrow instructions around intructions like ref_element_addr, open_existential, and project_box. After that, we can also migrate GuaranteedForwarding instructions like tuple_extract and struct_extract
The only reason to introduce the special case: AnyInteriorPointer, is timing. We are currently trying to enable OSSA modules. We don't want to simultaneously change the representation of OSSA, because, in the short term, that will make it harder to screen bugs and migrate projects.
As soon as OSSA modules is enabled, instead of this temporary change, we should make the single-line change to allow InteriorPointer to accept owned values. That will significantly affect SIL, because a side effect will be to stop emitting extraneous begin_borrow instructions in some cases. Our OSSA utilities should fully support elimination of extraneous borrow scopes--we already rely on InteriorPointer traversal in key places. This is especially important for lifetime completion, which we are rapidly adopting throughout the pipeline. So any problem with InteriorPointer traversal is an existing bug that will surface eventually, regardless whether we have extraneous borrow scopes.