[cxx-interop] Delay lowering unowned convention until ownership elimination #84612
Conversation
Xazax-hun
requested review from
egorzhdan,
fahadnayyar,
hnrklssn,
j-hui,
susmonteiro and
zoecarver
as code owners
|
As per some offline discussions we might want to completely eliminate the unowned result convention from the compiler and use a |
…nation Unowned result conventions do not work well with OSSA. Retain the result right after the call when we come out of OSSA so we can treat the returned value as if it was owned when we do optimizations. This fix a miscompilation due to the DestroyAddrHoisting pass hoisting destroys above copies with unowned sources. When the destroyed object was the last reference to the pointed memory the copy is happening too late resulting in a use after free. rdar://160462854
Remove SIL tests that are testing result conventions that are never emitted by the compiler.
Xazax-hun
force-pushed
the
fix-overrelease-destroy-hoist
branch
from
eeb3d0c to
5a63808
Compare
Xazax-hun
changed the title
There was a problem hiding this comment.
Re: Remove SIL tests that are testing result conventions that are never
emitted by the compiler.
Some of those tests might cover real corner cases in the optimizer (Namely CSE and OSSA RAUW) that can occur with real source. But they just happen to return an unowned result because it was convenient.
Instead of deleting those tests, can you just convert the return convention to @owned and insert a copy_value after the cast?
I know we talked about not supporting unowned at all in SIL, but I don't know how we would handle these kind of bitcasts yet. Maybe we consider them to have an implicit copy, which could be removed later if we prove the incoming value is trivial. But that's all future work.
| return %5 : $ThreeDifferingPayloadEnum | ||
| } | ||
|
|
||
| sil [ossa] @enum_cases_with_trivial_unowned_cases_arg_into_phi : $@convention(thin) (Builtin.NativeObject) -> ThreeDifferingPayloadEnum { |
There was a problem hiding this comment.
This has a version with owned return type so should be OK to remove.
| return %9999 : $() | ||
| } | ||
|
|
||
| sil [ossa] @unowned_to_ref_is_unowned_instant_use : $@convention(thin) (@guaranteed Builtin.NativeObject) -> Builtin.NativeObject { |
There was a problem hiding this comment.
These specifically testing the unowned return type which is not supported. I think these or OK to remove.
| // CHECK: ref_to_raw_pointer | ||
| // CHECK: raw_pointer_to_ref | ||
| // CHECK: } // end sil function 'unchecked_ref_cast_formation_unowned' | ||
| sil [ossa] @unchecked_ref_cast_formation_unowned : $@convention(thin) (B) -> F { |
There was a problem hiding this comment.
I think the ones in this file are OK to go, specifically testing cases that are never generated.
Xazax-hun
force-pushed
the
fix-overrelease-destroy-hoist
branch
from
518b6b1 to
0e77567
Compare
Xazax-hun
force-pushed
the
fix-overrelease-destroy-hoist
branch
from
0e77567 to
8b3b16c
Compare
|
Thanks! I updated the tests that had suggestions and ended up converting the ones in OSSA RAUW myself. Let me know if there are some more that should survive. |
|
@swift-ci please test |
|
@swift-ci please test macOS |
…elimination Explanation: Unowned result conventions do not work well with OSSA. Retain the result right after the call when we come out of OSSA so we can treat the returned value as if it was owned when we do optimizations. This fix a miscompilation due to the DestroyAddrHoisting pass hoisting destroys above copies with unowned sources. When the destroyed object was the last reference to the pointed memory the copy is happening too late resulting in a use after free. Issues: rdar://160462854 Original PRs: swiftlang#84612 Risk: We change where retaining of the unowned return values happen in the optimization pipeline. It is hard to anticipate all the possible effects but it should make the optimizer more correct. Testing: Added a compiler test. Reviewers: @eeckstein, @atrick
3 participants
Unowned result conventions do not work well with OSSA. Retain the result right after the call when we come out of OSSA so we can treat the returned value as if it was owned when we do optimizations.
This fix a miscompilation due to the DestroyAddrHoisting pass hoisting destroys above copies with unowned sources. When the destroyed object was the last reference to the pointed memory the copy is happening too late resulting in a use after free.
rdar://160462854