Fix unchecked_bitwise_cast to "escape" its operand's ownership #61393
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 makes it impossible for OSSA utilities to reason about the lifetime of the value being cast. This fixes bugs in which SILGen generates bitwise casts without protecting that lifetime. Fixes rdar://100527903 (Overrelease of optional closure when using shorthand syntax) The previous OSSA strategy was to gradually eliminate all "pointer escape" SIL patterns. That would define away a class of bugs in the OSSA utilities themselves. This was not friendly to developers working on SILGen, and we still don't have verification that can catch these bugs. The new strategy is to make all OSSA utilities aware of pointer escapes. This acknowledges reality that SIL is imperfect. Instead, optimizer support for transforming SIL always needs to recognize potentially dangerous patterns. The downside of this new strategy is that it hides performance problems because SIL optimizations can give up whenever they happen to see a "pointer escape". We need to keep working on this problem without being motivated by miscompiling code.
Fix the utilities used by LexicalDestroyHoisting that finds all uses to report a "PointerEscape". We can't rely on lifetime analysis when such a use is present.
Test that both canonicalization and lexical destroy hoisting recognize an "bail out" in the presence of unchecked_bitwise_cast.
|
@swift-ci test |
|
@swift-ci benchmark |
nate-chandler
approved these changes
Sep 30, 2022
| @@ -238,9 +238,31 @@ OPERAND_OWNERSHIP(PointerEscape, ProjectExistentialBox) | |||
| OPERAND_OWNERSHIP(PointerEscape, UncheckedOwnershipConversion) | |||
| OPERAND_OWNERSHIP(PointerEscape, ConvertEscapeToNoEscape) | |||
|
|
|||
| // UncheckedBitwiseCast ownership behaves like RefToUnowned. It produces an | |||
| // Unowned values from a non-trivial value, without consuming or borrowing the | |||
| bool swift::findUsesOfSimpleValue(SILValue value, | ||
| SmallVectorImpl<Operand *> *usePoints) { | ||
| for (auto *use : value->getUses()) { | ||
| if (use->getOperandOwnership() == OperandOwnership::Borrow) { | ||
| switch (use->getOperandOwnership()) { |
There was a problem hiding this comment.
Do we need the same change in LexicalDestroyFolding's findBorroweeUsage?
There was a problem hiding this comment.
👍 I'll need to figure out a test case for that one
atrick
changed the title
|
@nate-chandler I'll merge this since CI is happy with it and the sooner it gets into builds the better. I'll immediately follow up with the destroy folding fix and comment grammar. |
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.
Fix unchecked_bitwise_cast to "escape" its operand's ownership
This makes it impossible for OSSA utilities to reason about the
lifetime of the value being cast. This fixes bugs in which SILGen
generates bitwise casts without protecting that lifetime.
Also fix LexicalDestroyHoisting to bail out when ownership escapes via a
PointerEscapeoperand.Fixes rdar://100527903 (Overrelease of optional closure when using shorthand syntax)
The previous OSSA strategy was to gradually eliminate all "pointer
escape" SIL patterns. That would define away a class of bugs in the
OSSA utilities themselves. This was not friendly to developers working
on SILGen, and we still don't have verification that can catch these
bugs.
The new strategy is to make all OSSA utilities aware of pointer
escapes. This acknowledges reality that SIL is imperfect. Instead,
optimizer support for transforming SIL always needs to recognize
potentially dangerous patterns.
The downside of this new strategy is that it hides performance
problems because SIL optimizations can give up whenever they happen to
see a "pointer escape". We need to keep working on this problem
without being motivated by miscompiling code.