Fix #76803 miscompilation#76844

simonvandel · 2020-09-17T17:18:23Z

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

bors · 2020-09-18T14:33:18Z

☔ 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

Check that the variant index matches the target value from the SwitchInt we came from

simonvandel · 2020-09-19T13:42:35Z

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

wesleywiser

Looks good to me other than that one small comment.

wesleywiser · 2020-09-19T18:54:14Z

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


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

Added in latest commit

simonvandel · 2020-09-23T16:27:30Z

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

wesleywiser · 2020-09-24T17:45:57Z

Thanks @simonvandel, sorry for the delay!

@bors r+ rollup=never

bors · 2020-09-24T17:45:58Z

📌 Commit a875c7a has been approved by wesleywiser

bors · 2020-09-24T19:35:27Z

⌛ Testing commit a875c7a with merge 326c728a6df1d37e97f9c5bbccff5925a8113838...

bors · 2020-09-24T23:50:27Z

💥 Test timed out

wesleywiser · 2020-09-25T00:28:15Z

@bors retry

bors · 2020-09-25T04:17:09Z

⌛ Testing commit a875c7a with merge 9d74efe...

bors · 2020-09-25T06:23:48Z

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

spastorino · 2020-09-25T19:21:50Z

The PR that was beta-nominated was #76837

rust-highfive assigned wesleywiser Sep 17, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 17, 2020

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

simonvandel added 2 commits September 19, 2020 15:29

Add regression test showcasing miscompilation

cad050b

Fix rust-lang#76803

738ed9b

Check that the variant index matches the target value from the SwitchInt we came from

simonvandel force-pushed the fix-76803 branch from 2fcea07 to 738ed9b Compare September 19, 2020 13:40

wesleywiser reviewed Sep 19, 2020

View reviewed changes

Add assertion for len of vecs

a875c7a

wesleywiser approved these changes Sep 24, 2020

View reviewed changes

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

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 added the merged-by-bors This PR was explicitly merged by bors. label Sep 25, 2020

bors merged commit 9d74efe into rust-lang:master Sep 25, 2020

rustbot added this to the 1.48.0 milestone Sep 25, 2020

bors mentioned this pull request Sep 25, 2020

[mir-opt] Introduce a new flag to enable experimental/unsound mir opts #76899

Merged

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

wesleywiser mentioned this pull request Sep 30, 2020

Segfaults/corruption when reading an enum in release mode #77359

Closed

nbdd0121 mentioned this pull request Oct 3, 2021

SimplifyBranchSame assumes discriminant is the same as variant index. #89485

Closed

Uh oh!

Conversation

simonvandel commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Sep 18, 2020

Uh oh!

simonvandel commented Sep 19, 2020

Uh oh!

wesleywiser left a comment

Choose a reason for hiding this comment

Uh oh!

wesleywiser Sep 19, 2020

Choose a reason for hiding this comment

Uh oh!

simonvandel Sep 19, 2020

Choose a reason for hiding this comment

Uh oh!

simonvandel commented Sep 23, 2020

Uh oh!

wesleywiser commented Sep 24, 2020

Uh oh!

bors commented Sep 24, 2020

Uh oh!

bors commented Sep 24, 2020

Uh oh!

bors commented Sep 24, 2020

Uh oh!

wesleywiser commented Sep 25, 2020

Uh oh!

bors commented Sep 25, 2020

Uh oh!

bors commented Sep 25, 2020

Uh oh!

spastorino commented Sep 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

simonvandel commented Sep 17, 2020 •

edited

Loading