[silgen] Teach CleanupCloner how to handle OwnedValueWritebackCleanups correctly. #31898
Conversation
|
@swift-ci test |
|
@swift-ci asan test |
|
@swift-ci test source compatibility |
|
I am not sure if all of the testing is necessary, but given that I am changing DI here I just want to be careful out of an abundance of caution. |
|
This also creates the infrastructure necessary to fix the Formal vs Lexical cleanup mixup issues as well. I just didn't want to fix that here. That should be in a different PR. |
| @@ -785,6 +785,7 @@ void LifetimeChecker::doIt() { | |||
| break; | |||
| case DIUseKind::LoadForTypeOfSelf: | |||
| handleLoadForTypeOfSelfUse(Use); | |||
| break; | |||
There was a problem hiding this comment.
I don't this actually effected anything since this is the last entry in the switch, but I saw this so I just wanted to handle it quickly just to prevent future bugs.
|
@swift-ci Please Test Source Compatibility Release |
|
Build failed |
…s correctly. Previously we would just forward the cleanup and create a normal "destroy" cleanup resulting in early deallocation and use after frees along error paths. As part of this I also had to tweak how DI recognizes self init writebacks to not use SILLocations. This is approach is more robust (since we aren't relying on SourceLocs/have verifiers to make sure we don't violate SIL) and also avoids issues from the write back store not necessarily have the same SILLocation. <rdar://problem/59830255>
gottesmm
force-pushed
the
pr-40f1eb4aa2e642a971e6af034157dc9e8c1b31cf
branch
from
9f8bcd7 to
f57a6cf
Compare
|
@swift-ci test |
|
Build failed |
|
@swift-ci test windows platform |
|
Build failed |
|
@swift-ci test OS X platform |
|
@swift-ci asan |
|
@swift-ci test source compatibility |
1 similar comment
|
@swift-ci test source compatibility |
|
@swift-ci asan |
|
Debug failed due to failure to clone from GitHub. |
|
@swift-ci Please Test Source Compatibility Debug |
|
The one debug test that failed failed due to some weird configuration issue in the project. The compiler didn't crash. |
| // This is to handle situations where we do not actually consume self as a | ||
| // result of situations such as: | ||
| // | ||
| // 1. The usage of a metatype to allocate the object. |
There was a problem hiding this comment.
Using a dynamic metatype value shouldn't by itself change how we consume the original self value, because that doesn't change the ownership semantics of the init. Is that really what's going on here? I trust that it's doing the right thing, because the tests look reasonably comprehensive, but this comment is confusing. What is the actual difference in SIL generation between swift 5 dynamic self and swift 4 mode?
There was a problem hiding this comment.
This was here also for swift 4 mode. It is not related to the current change directly. What happens here is that we do a load [take] to get the metatype. The meta type is used to call a factory init that produces the actual value we care about and then we need to store back the partially initialized value so it can be destroyed.
There was a problem hiding this comment.
Specifically, this comment used to be:
// If this load's value is being stored back into the delegating
// mark_uninitialized buffer and it is a self init use, skip the
// use. This is to handle situations where due to usage of a metatype to
// allocate, we do not actually consume self.
There was a problem hiding this comment.
I explained offline to JoeG this in person. He is fine with the explanation.
Previously we would just forward the cleanup and create a normal "destroy"
cleanup resulting in early deallocation and use after frees along error paths.
As part of this I also had to tweak how DI recognizes self init writebacks to
not use SILLocations. This is approach is more robust (since we aren't relying
on SourceLocs/have verifiers to make sure we don't violate SIL) and also avoids
issues from the write back store not necessarily have the same SILLocation.
rdar://problem/59830255