-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[cast-opt] Fix miscompile when we tried to optimize take_on_success t… #25041
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
[cast-opt] Fix miscompile when we tried to optimize take_on_success t… #25041
Conversation
@swift-ci smoke test |
@gottesmm According to SIL.rst this change is a nop. The specification for release_value and destroy_value is word-for-word identical. So how could it be a miscompile? |
@atrick that isn't the problem. The problem is that it is moving the SILBuilder without unmoving it. |
e3b306d
to
51b1fe4
Compare
…hat resulted in invalid IR being emitted. The specific problem here is that we are setting the insertion point of a SILBuilder and not unsetting it. I fixed the problem by creating a separate builder so the original builder stays put. I originally came across this in my work on moving ownership stripping after the diagnostic passes. This patch fixes it without my other changes to ease cherry-picking to 5.1. I also added more test coverage by expanding the test case to also handle copy_on_success and take_always. rdar://51093557
51b1fe4
to
ad67685
Compare
@swift-ci smoke test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Thanks for explaining in the commit message.
For anyone following along, please stop using SILBuilder.setInsertionPoint
. It's the source of endless bugs and we're trying to get rid of it.
@swift-ci smoke test |
@swift-ci smoke test linux platform |
1 similar comment
@swift-ci smoke test linux platform |
…hat resulted in invalid IR being emitted.
I originally came across this in my work on moving ownership stripping after the
diagnostic passes. This patch fixes it without my other changes to ease
cherry-picking to 5.1.
I also added more test coverage by expanding the test case to also handle
copy_on_success and take_always.
rdar://51093557