[5.0] [silgenpattern] Fix a leak in tuple pattern emission. #21757
Conversation
Today SILGenPattern maintains the following invariants:
1. All values passed into a switch must be either TakeAlways or BorrowAlways and
loadable input values will be loaded.
2. Loadable types passed to a subtree must be loaded.
3. TakeOnSuccess can only occur with address only types and only in subtrees.
4. A TakeOnSuccess value must go through "fake borrowing"
(i.e. forward/unforwarding) to ensure that along failing cases, we properly
re-enable the cleanup on the aggregate. This means that TakeOnSuccess can
never be handled as a loadable value with ownership enabled and that any
take_on_success value since the original cleanup on the parent value was
disabled must be at +1.
5. We use BorrowAlways instead of TakeOnSuccess for objects to express the
notion that the object is not owned by the sub-tree.
The bug in this tuple code occured at the a place in the code where we go from
an address only parent type to a loadable subtype via TakeOnSuccess. I was
trying to follow (5) and thus I converted the subvalue to use a {load_borrow,
BorrowAlways}. The problem with this is that I lost the cleanup from the tuple
subvalue since take_on_success is just simulating +0 and thus entails having a
cleanup for each of the underlying tuple values.
The fix here was to:
* Create a cleanup for the loadable subvalue leaving it in memory. This address
version of the subvalue we use for the purposes of unforwarding. This avoids
(4).
* load_borrow the subvalue and pass that off to the subtree. This ensures that
we respect (2), (3), (4).
* Unforward in the failure case the in memory subvalue.
This gives us both characteristics as well as in the future the possibility of
enforcing via the ownership verifier that the borrow ends before the
destroy_addr.
I also sprinkled some assertions liberally to maintain invariants.
rdar://47034816
(cherry picked from commit a53e351)
|
@swift-ci test |
gottesmm
changed the title
|
Build failed |
|
The test failure is from swift-5.0 using -assume-parsing-unqualified-ownership-sil instead of printing [ossa]. I put an extra commit on top of the original cherry-pick that removes that. |
|
@swift-ci test |
|
Build failed |
|
Build failed |
|
Build failed |
gottesmm
force-pushed
the
rdar47034816-5.0
branch
from
d604b76 to
901404f
Compare
|
@swift-ci test |
|
@swift-ci test os x platform |
|
@swift-ci test osx platform |
|
@swift-ci Please test OS X platform |
|
I checked. The macOS test is in the queue. |
|
Build failed |
|
Explanation: This is fixing a leak when we perform a switch on an address only tuple with a tuple element that is loadable. In SILGenPattern there are two code pathways, one for address only types that use the old logic and one for loadable types that use the new logic. This is the only place where we go from address only -> loadable. Address only code "simulates" +0 by enabling/disabling/re-enabling cleanups. Loadable types in contrast use actual +0 borrows. I needed to add a destroy cleanup (for the parent address only tuple) and then perform a load_borrow on the loadable component to get the right semantics. |
|
Thanks Erik! |
Today SILGenPattern maintains the following invariants:
All values passed into a switch must be either TakeAlways or BorrowAlways and
loadable input values will be loaded.
Loadable types passed to a subtree must be loaded.
TakeOnSuccess can only occur with address only types and only in subtrees.
A TakeOnSuccess value must go through "fake borrowing"
(i.e. forward/unforwarding) to ensure that along failing cases, we properly
re-enable the cleanup on the aggregate. This means that TakeOnSuccess can
never be handled as a loadable value with ownership enabled and that any
take_on_success value since the original cleanup on the parent value was
disabled must be at +1.
We use BorrowAlways instead of TakeOnSuccess for objects to express the
notion that the object is not owned by the sub-tree.
The bug in this tuple code occured at the a place in the code where we go from
an address only parent type to a loadable subtype via TakeOnSuccess. I was
trying to follow (5) and thus I converted the subvalue to use a {load_borrow,
BorrowAlways}. The problem with this is that I lost the cleanup from the tuple
subvalue since take_on_success is just simulating +0 and thus entails having a
cleanup for each of the underlying tuple values.
The fix here was to:
version of the subvalue we use for the purposes of unforwarding. This avoids
(4).
we respect (2), (3), (4).
This gives us both characteristics as well as in the future the possibility of
enforcing via the ownership verifier that the borrow ends before the
destroy_addr.
I also sprinkled some assertions liberally to maintain invariants.
rdar://47034816
(cherry picked from commit a53e351)
Replace this paragraph with a description of your changes and rationale. Provide links to external references/discussions if appropriate.
Resolves SR-NNNN.