Skip to content

DI: Correctly handle conditional destroy of 'self' in root class #33734

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

slavapestov
Copy link
Contributor

In a designated initializer of a non-root class, 'self' becomes
fully initialized after the 'super.init' call, at which point
escaping uses of 'self' become valid, and releases of 'self' are
lowered to a 'strong_release' instruction, which runs the
deinitializer.

In a root class, 'self' becomes fully initialized after all stored
properties have been initialized, at which point escaping uses of
'self' become valid.

However, DI would still lower a conditional destroy of 'self' by
destroying any initialized stored properties and freeing the object
with 'dealloc_partial_ref'. This is incorrect, because 'self' may
have escaped.

In the non-conditional destroy case, we correctly lowered the
release to a 'strong_release' if all stored properties are known
to be initialized.

Fix DI to handle the conditional destroy case by first checking if all
bits in the control variable are set, and releasing the instance with
'strong_release' if so. The 'dealloc_partial_ref' is only emitted
if not at least one stored property was not initialized.

This ensures that we don't deallocate an instance that may have
escaped.

Fixes https://bugs.swift.org/browse/SR-13439, rdar://problem/67746791,
https://bugs.swift.org/browse/SR-13355, rdar://problem/67361228.

…tializers

While we don't need to destroy these fields, we still need to know
when the class is _semantically_ fully initialized, since this will
determine if we're going to release the class, running the deinitializer,
or if we're going to deallocate the memory for the instance directly.
@slavapestov slavapestov force-pushed the conditional-destroy-root-class branch from 443b8da to 3d9d04d Compare September 1, 2020 02:09
@slavapestov
Copy link
Contributor Author

@jckarter Do you mind reviewing this? It's feels like a bit of a hack but I couldn't think of any other way of doing it. With a non-root class, we have a bit that records if super.init() was called to determine if the instance is fully initialized or not. With a root class however, the corresponding state transition happens when all stored properties become fully initialized, which can happen in multiple places in the control flow graph; it is also a dynamic property because of partial initialization.

We already did the right thing in the non-conditionally-initialized case by treating "all properties are initialized" as "object is fully initialized"; this just extends that to the dynamic case by checking if all bits in the control word are set. I did have to tweak the logic a little so that we now record a bit in the control word even for trivial fields; previously we could skip these since they did not affect the outcome, but now they do because we care about the "semantic" initialization state of the object.

In a designated initializer of a non-root class, 'self' becomes
fully initialized after the 'super.init' call, at which point
escaping uses of 'self' become valid, and releases of 'self' are
lowered to a 'strong_release' instruction, which runs the
deinitializer.

In a root class, 'self' becomes fully initialized after all stored
properties have been initialized, at which point escaping uses of
'self' become valid.

However, DI would still lower a conditional destroy of 'self' by
destroying any initialized stored properties and freeing the object
with 'dealloc_partial_ref'. This is incorrect, because 'self' may
have escaped.

In the non-conditional destroy case, we correctly lowered the
release to a 'strong_release' if all stored properties are known
to be initialized.

Fix DI to handle the conditional destroy case by first checking if all
bits in the control variable are set, and releasing the instance with
'strong_release' if so. The 'dealloc_partial_ref' is only emitted
if not at least one stored property was not initialized.

This ensures that we don't deallocate an instance that may have
escaped.

Fixes <https://bugs.swift.org/browse/SR-13439>, <rdar://problem/67746791>,
<https://bugs.swift.org/browse/SR-13355>, <rdar://problem/67361228>.
@slavapestov slavapestov force-pushed the conditional-destroy-root-class branch from 3d9d04d to 233eeea Compare September 1, 2020 02:14
@slavapestov
Copy link
Contributor Author

Also, this changes runtime behavior of existing code, because the deinitializer will now run in more cases. I believe this is OK though, because the old behavior was untenable -- the deinitializer would run if DI could statically determine that all properties were initialized at the point of the release, otherwise it would never run. This is why the test case in the second JIRA failed, because the exit block had two incoming edges, one fully initialized and one partially initialized. It seemed silly that commenting out an if false { ... } block would change whether the deinitializer runs or not.

@slavapestov
Copy link
Contributor Author

@swift-ci Please smoke test

@slavapestov
Copy link
Contributor Author

@swift-ci Please test source compatibility

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants