Skip to content

[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

Conversation

gottesmm
Copy link
Contributor

…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

@gottesmm gottesmm requested review from atrick and aschwaighofer May 24, 2019 03:09
@gottesmm
Copy link
Contributor Author

@swift-ci smoke test

@atrick
Copy link
Contributor

atrick commented May 24, 2019

@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?

@gottesmm
Copy link
Contributor Author

@atrick that isn't the problem. The problem is that it is moving the SILBuilder without unmoving it.

@gottesmm gottesmm force-pushed the pr-d02a03999626f1d50a4bf0996dadb34e72d9f355 branch from e3b306d to 51b1fe4 Compare May 24, 2019 16:58
…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
@gottesmm gottesmm force-pushed the pr-d02a03999626f1d50a4bf0996dadb34e72d9f355 branch from 51b1fe4 to ad67685 Compare May 24, 2019 16:58
@gottesmm
Copy link
Contributor Author

@swift-ci smoke test

2 similar comments
@gottesmm
Copy link
Contributor Author

@swift-ci smoke test

@gottesmm
Copy link
Contributor Author

@swift-ci smoke test

Copy link
Contributor

@atrick atrick left a 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.

@gottesmm
Copy link
Contributor Author

@swift-ci smoke test

@gottesmm
Copy link
Contributor Author

@swift-ci smoke test linux platform

1 similar comment
@gottesmm
Copy link
Contributor Author

@swift-ci smoke test linux platform

@gottesmm gottesmm merged commit a042112 into swiftlang:master May 25, 2019
@gottesmm gottesmm deleted the pr-d02a03999626f1d50a4bf0996dadb34e72d9f355 branch May 25, 2019 03:48
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