Skip to content

Conversation

@atrick
Copy link
Contributor

@atrick atrick commented Mar 18, 2020

Devirtualizing try_apply modified the CFG, but passes that run
devirtualization were not invalidating any CFG analyses, such as the
domtree.

This could hypothetically have caused miscompilation, but will be
caught by running with -sil-verify-all.

Devirtualizing try_apply modified the CFG, but passes that run
devirtualization were not invalidating any CFG analyses, such as the
domtree.

This could hypothetically have caused miscompilation, but will be
caught by running with -sil-verify-all.
@atrick
Copy link
Contributor Author

atrick commented Mar 18, 2020

@swift-ci test

@atrick atrick requested a review from gottesmm March 18, 2020 20:18
@atrick
Copy link
Contributor Author

atrick commented Mar 18, 2020

@gottesmm I asked for your review, but please just rewrite it however you like rather than iterating on a PR that is just a simple bug fix.

Note: it's very rare that I resort to returning unnamed tuples (std::pair). I almost always create a named struct, but it was expedient in this case.

@atrick atrick merged commit d28ab08 into swiftlang:master Mar 18, 2020
@atrick atrick deleted the fix-devirtualizer-invalidate branch March 18, 2020 20:21
Copy link
Contributor

@gottesmm gottesmm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@atrick LGTM, except for one thing. Do we have a test that exposed the problem?

@atrick
Copy link
Contributor Author

atrick commented Mar 26, 2020

@gottesmm fyi, the bug is exposed by other commits that I was originally trying to batch together with this. -sil-verify-all does catch the problem. I haven't figured out how to reduce it to a separate unit test without those changes, but have a note to myself to try again if I get time.

@gottesmm
Copy link
Contributor

@atrick ok!

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