[SILGen] Make sure enum element is always cleaned up. #31779
Conversation
|
@slavapestov not really. The old behavior doesn't destroy the enum for trivial types. It still deallocates it, though. The reason I'm fixing this is so that the verifier doesn't error when the dead |
There was a problem hiding this comment.
LGTM. Thanks @zoecarver for following through on this! This is the right fix = ).
|
@swift-ci test |
|
@gottesmm of course :) I agree, this is a better solution. Sorry, should have posted something here earlier. When running the interpreter tests I discovered that this sometimes destroys the enum twice: once when destroying the data and once when destroying the enum itself. I'm working on a fix right now. |
|
The problem is that there are code paths that are assuming the value is forwarded. |
zoecarver
force-pushed
the
fix/enum-take-guaranteed
branch
from
bb456f4 to
a14d76c
Compare
|
Build failed |
|
Build failed |
zoecarver
force-pushed
the
fix/enum-take-guaranteed
branch
from
a14d76c to
c65fe19
Compare
|
@swift-ci Please smoke test |
1 similar comment
|
@swift-ci Please smoke test |
|
@slavapestov thanks. I found a case where it's a pretty obvious leak. Is there a way to run this program through a sanitizer or something so that we hit a crash/error? I would use something like |
|
I found the sanitizer tests and am trying to get one of them to fail on master. The issue is the parent object always seems to be destroyed in the scope it's created in so not destroying one of its elements seems to be OK. I think I can get this to fail, though. If nothing else with a sil test (but ideally a swift one). The OS X bot failed because of an LLDB issue. Seems unrelated. I'll re-trigger it after I find a test that crashes on master. |
zoecarver
force-pushed
the
fix/enum-take-guaranteed
branch
from
c65fe19 to
a84b4fd
Compare
|
@swift-ci please test |
|
Build failed |
zoecarver
force-pushed
the
fix/enum-take-guaranteed
branch
from
a84b4fd to
662b5da
Compare
| @@ -0,0 +1,45 @@ | |||
| // RUN: %target-swift-emit-silgen -module-name switch %s > %t.sil | |||
| // RUN: %target-sil-opt %t.sil -sil-combine -enable-sil-verify-all | %FileCheck %s | |||
|
|
|||
There was a problem hiding this comment.
This test crashes on master. It's a bit unconventional so, I want to get this re-approved before I merge anything.
This comment has been minimized.
This comment has been minimized.
|
@swift-ci please test linux platform |
|
@slavapestov and @gottesmm thanks for bearing with me on this. Sorry for all the back and forth. I ran the tests locally and I think this patch is now good (both in terms of logic and tests). |
This comment has been minimized.
This comment has been minimized.
zoecarver
force-pushed
the
fix/enum-take-guaranteed
branch
from
662b5da to
af1462a
Compare
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
|
@swift-ci please test windows platform |
|
Do I need re-approval to commit this? |
|
@gottesmm and @slavapestov friendly ping. Can one of you verify this is still good to go? |
Makes the source enum value a "managed value" so that it is cleaned up after the data is extracted.
zoecarver
force-pushed
the
fix/enum-take-guaranteed
branch
from
af1462a to
92b4185
Compare
…formal access cleanups. This has been a long standing issue that we have been hacking around in various points in SILGen. Now CleanupCloner just does the right thing. I was unable to cause any issues to pop up in tree (since I believe we hacked around this and converged on correctness). But it does come up in combination with new code in swiftlang#31779. rdar://63514765
|
@gottesmm can you take another look at this when you have a minute? |
Makes the source enum value a "managed value" so that it is cleaned up after the data is extracted.
The original issue was exposed in #31077 and I attempted to fix this incorrectly in #31157.