[embedded] Handle retain/retain ops inside deinit in Embedded Swift's swift_release #76231
Conversation
kubamracek
requested review from
MaxDesiatov,
phausler,
mikeash,
al45tair,
rauhul,
zoecarver and
eeckstein
|
@swift-ci please test |
| @@ -291,6 +291,10 @@ func swift_release_n_(object: UnsafeMutablePointer<HeapObject>?, n: UInt32) { | |||
|
|
|||
| let resultingRefcount = subFetchAcquireRelease(refcount, n: Int(n)) & HeapObject.refcountMask | |||
| if resultingRefcount == 0 { | |||
| // Set the refcount to immortalRefCount before calling the object destroyer | |||
| // to prevent future retains/releases from having any effect. | |||
| storeRelaxed(refcount, newValue: HeapObject.immortalRefCount) | |||
There was a problem hiding this comment.
Maybe worth commenting explicitly that there can only be one thread holding a reference to this object at this point (unless there's a bug somewhere), which is why a relaxed store is fine here.
There was a problem hiding this comment.
Seconded. On another note, the non-embedded runtime continues tracking refcounts, and this allows it to raise a fatal error when self escapes from deinit by checking the refcount when the memory is freed. See here:
swift/stdlib/public/runtime/HeapObject.cpp
Line 850 in 03abb1f
Obviously this is an optional feature and not something you need to do now (or ever), but might be worth keeping in mind as a possible enhancement.
| @@ -291,6 +291,10 @@ func swift_release_n_(object: UnsafeMutablePointer<HeapObject>?, n: UInt32) { | |||
|
|
|||
| let resultingRefcount = subFetchAcquireRelease(refcount, n: Int(n)) & HeapObject.refcountMask | |||
| if resultingRefcount == 0 { | |||
| // Set the refcount to immortalRefCount before calling the object destroyer | |||
| // to prevent future retains/releases from having any effect. | |||
| storeRelaxed(refcount, newValue: HeapObject.immortalRefCount) | |||
There was a problem hiding this comment.
Seconded. On another note, the non-embedded runtime continues tracking refcounts, and this allows it to raise a fatal error when self escapes from deinit by checking the refcount when the memory is freed. See here:
swift/stdlib/public/runtime/HeapObject.cpp
Line 850 in 03abb1f
Obviously this is an optional feature and not something you need to do now (or ever), but might be worth keeping in mind as a possible enhancement.
| bar = nil | ||
| print("OK!") | ||
|
|
||
| // CHECK: OK! |
There was a problem hiding this comment.
I'm a bit lost as to how this test actually tests this scenario in question, and I worry it might silently stop testing this scenario with some compiler change. It might be better to use Unmanaged to do an explicit retain/release in a deinit {} block, or assign self to a global, call out to something opaque, then remove it.
There was a problem hiding this comment.
Added a more targeted test.
|
@swift-ci please test |
See attached testcase which crashes without the fix due to an infinite recursion. The scenario is that the deinit of the object contains a retain+release pair (on "self"). We need to handle this case in the Embedded Swift runtime's
swift_releasecode.Fixes #74697.