Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix #76803 miscompilation #76844

Merged
merged 3 commits into from
Sep 25, 2020
Merged

Fix #76803 miscompilation #76844

merged 3 commits into from
Sep 25, 2020

Conversation

simonvandel
Copy link
Contributor

@simonvandel simonvandel commented Sep 17, 2020 •
edited
Loading

Fixes #76803
Seems like it was an oversight that the discriminant value being set was not compared to the target value from the SwitchInt, as a comment says this is a requirement for the optimization to be sound.

r? @wesleywiser since you are probably familiar with the optimization and made #76837 to workaround the bug

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 17, 2020
@jyn514 jyn514 added A-mir-opt Area: MIR optimizations T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 17, 2020
@bors
Copy link
Contributor

bors commented Sep 18, 2020

☔ The latest upstream changes (presumably #76837) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@simonvandel
Add regression test showcasing miscompilation
cad050b
@simonvandel
Fix rust-lang#76803
738ed9b 
Check that the variant index matches the target value from the SwitchInt we came from
@simonvandel
Copy link
Contributor Author

Rebased
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

wesleywiser
wesleywiser reviewed Sep 19, 2020
View reviewed changes
Copy link
Member

@wesleywiser wesleywiser left a comment

Choose a reason for hiding this comment

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

Looks good to me other than that one small comment.

compiler/rustc_mir/src/transform/simplify_try.rs
TerminatorKind::SwitchInt { targets, discr, .. } => (discr, targets),
let (discr_switched_on, targets_and_values):(_, Vec<_>) = match &bb.terminator().kind {
TerminatorKind::SwitchInt { targets, discr, values, .. } => {
// if values.len() == targets.len() - 1, we need to include None where no value is present
Copy link
Member

Choose a reason for hiding this comment

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

We should assert or at least debug_assert that targets.len() == targets_and_values.len()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in latest commit

@simonvandel
Copy link
Contributor Author

Hi @wesleywiser
Just wanted to ping you that I have resolved your comment.

@wesleywiser
Copy link
Member

Thanks @simonvandel, sorry for the delay!

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Sep 24, 2020

📌 Commit a875c7a has been approved by wesleywiser

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 24, 2020
@bors
Copy link
Contributor

bors commented Sep 24, 2020

⌛ Testing commit a875c7a with merge 326c728a6df1d37e97f9c5bbccff5925a8113838...

@bors
Copy link
Contributor

bors commented Sep 24, 2020

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 24, 2020
@wesleywiser
Copy link
Member

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 25, 2020
@bors
Copy link
Contributor

bors commented Sep 25, 2020

⌛ Testing commit a875c7a with merge 9d74efe...

@bors
Copy link
Contributor

bors commented Sep 25, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: wesleywiser
Pushing 9d74efe to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 25, 2020
@bors bors merged commit 9d74efe into rust-lang:master Sep 25, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 25, 2020
@bors bors mentioned this pull request Sep 25, 2020
Merged
@spastorino spastorino added beta-nominated Nominated for backporting to the compiler in the beta channel. and removed beta-nominated Nominated for backporting to the compiler in the beta channel. labels Sep 25, 2020
@spastorino
Copy link
Member

The PR that was beta-nominated was #76837

@wesleywiser wesleywiser mentioned this pull request Sep 30, 2020
Closed
@nbdd0121 nbdd0121 mentioned this pull request Oct 3, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wesleywiser wesleywiser wesleywiser approved these changes

Assignees

@wesleywiser wesleywiser

Labels
A-mir-opt Area: MIR optimizations merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.48.0
Development

Successfully merging this pull request may close these issues.

starting from nightly-2020-08-18 rustls can't connect to some websites
7 participants
@simonvandel @bors @wesleywiser @spastorino @rust-highfive @jyn514 @rustbot