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.

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

StorageLive is added on a local that was already live #88649

Closed
camelid opened this issue Sep 4, 2021 · 5 comments · Fixed by #88678
Closed

StorageLive is added on a local that was already live #88649

camelid opened this issue Sep 4, 2021 · 5 comments · Fixed by #88678
Labels
A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-bug Category: This is a bug. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@camelid
Copy link
Member

camelid commented Sep 4, 2021

Miri's CI started failing recently, with an error similar to:

error: Undefined Behavior: StorageLive on a local that was already live
 --> foo.rs:9:52
  |
9 |             Foo::Variant1(x) | Foo::Variant2(x) if x => {}
  |                                                    ^ StorageLive on a local that was already live
  |
  = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
  = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information

It seems likely that this is a regression from #88572. cc @matthewjasper

Thanks to @hyd-dev for doing the original minimization.

MCVE

enum Foo {
    Variant1(bool),
    Variant2(bool),
}

fn main() {
    loop {
        match Foo::Variant1(true) {
            Foo::Variant1(x) | Foo::Variant2(x) if x => {}
            _ => {}
        }
    }
}

Generated MIR (from right after building)

// MIR for `main` 0 mir_map

fn main() -> () {
    let mut _0: ();                      // return place in scope 0 at foo.rs:6:11: 6:11
    let mut _1: !;                       // in scope 0 at foo.rs:7:5: 12:6
    let mut _2: ();                      // in scope 0 at foo.rs:6:1: 13:2
    let mut _3: Foo;                     // in scope 0 at foo.rs:8:15: 8:34
    let mut _4: isize;                   // in scope 0 at foo.rs:9:13: 9:29
    let mut _5: &Foo;                    // in scope 0 at foo.rs:8:15: 8:34
    let _6: bool;                        // in scope 0 at foo.rs:9:27: 9:28
    let _7: &bool;                       // in scope 0 at foo.rs:9:27: 9:28
    let mut _8: bool;                    // in scope 0 at foo.rs:9:52: 9:53
    let mut _9: bool;                    // in scope 0 at foo.rs:9:52: 9:53
    scope 1 {
        debug x => _6;                   // in scope 1 at foo.rs:9:27: 9:28
        debug x => _7;                   // in scope 1 at foo.rs:9:27: 9:28
    }

    bb0: {
        StorageLive(_1);                 // scope 0 at foo.rs:7:5: 12:6
        goto -> bb1;                     // scope 0 at foo.rs:7:5: 12:6
    }

    bb1: {
        falseUnwind -> [real: bb2, cleanup: bb19]; // scope 0 at foo.rs:7:5: 12:6
    }

    bb2: {
        StorageLive(_3);                 // scope 0 at foo.rs:8:15: 8:34
        _3 = Foo::Variant1(const true);  // scope 0 at foo.rs:8:15: 8:34
        FakeRead(ForMatchedPlace(None), _3); // scope 0 at foo.rs:8:15: 8:34
        _4 = discriminant(_3);           // scope 0 at foo.rs:8:15: 8:34
        switchInt(move _4) -> [0_isize: bb3, 1_isize: bb5, otherwise: bb4]; // scope 0 at foo.rs:8:9: 8:34
    }

    bb3: {
        falseEdge -> [real: bb8, imaginary: bb5]; // scope 0 at foo.rs:9:13: 9:29
    }

    bb4: {
        _2 = const ();                   // scope 0 at foo.rs:10:18: 10:20
        goto -> bb16;                    // scope 0 at foo.rs:10:18: 10:20
    }

    bb5: {
        falseEdge -> [real: bb12, imaginary: bb4]; // scope 0 at foo.rs:9:32: 9:48
    }

    bb6: {
        goto -> bb4;                     // scope 0 at foo.rs:8:9: 8:34
    }

    bb7: {
        _2 = const ();                   // scope 1 at foo.rs:9:57: 9:59
        StorageDead(_6);                 // scope 0 at foo.rs:9:58: 9:59
        StorageDead(_9);                 // scope 0 at foo.rs:9:58: 9:59
        StorageDead(_7);                 // scope 0 at foo.rs:9:58: 9:59
        goto -> bb16;                    // scope 0 at foo.rs:9:58: 9:59
    }

    bb8: {
        StorageLive(_7);                 // scope 0 at foo.rs:9:27: 9:28
        _7 = &((_3 as Variant1).0: bool); // scope 0 at foo.rs:9:27: 9:28
        _5 = &shallow _3;                // scope 0 at foo.rs:8:15: 8:34
        StorageLive(_8);                 // scope 0 at foo.rs:9:52: 9:53
        _8 = (*_7);                      // scope 0 at foo.rs:9:52: 9:53
        switchInt(move _8) -> [false: bb10, otherwise: bb9]; // scope 0 at foo.rs:9:52: 9:53
    }

    bb9: {
        FakeRead(ForMatchGuard, _5);     // scope 0 at foo.rs:9:52: 9:53
        FakeRead(ForGuardBinding, _7);   // scope 0 at foo.rs:9:52: 9:53
        StorageLive(_6);                 // scope 0 at foo.rs:9:27: 9:28
        _6 = ((_3 as Variant1).0: bool); // scope 0 at foo.rs:9:27: 9:28
        goto -> bb7;                     // scope 0 at foo.rs:8:9: 11:10
    }

    bb10: {
        goto -> bb11;                    // scope 0 at foo.rs:9:52: 9:53
    }

    bb11: {
        StorageDead(_8);                 // scope 0 at foo.rs:9:58: 9:59
        StorageDead(_7);                 // scope 0 at foo.rs:9:58: 9:59
        falseEdge -> [real: bb4, imaginary: bb5]; // scope 0 at foo.rs:9:52: 9:53
    }

    bb12: {
        StorageLive(_7);                 // scope 0 at foo.rs:9:46: 9:47
        _7 = &((_3 as Variant2).0: bool); // scope 0 at foo.rs:9:46: 9:47
        _5 = &shallow _3;                // scope 0 at foo.rs:8:15: 8:34
        StorageLive(_9);                 // scope 0 at foo.rs:9:52: 9:53
        _9 = (*_7);                      // scope 0 at foo.rs:9:52: 9:53
        switchInt(move _9) -> [false: bb14, otherwise: bb13]; // scope 0 at foo.rs:9:52: 9:53
    }

    bb13: {
        FakeRead(ForMatchGuard, _5);     // scope 0 at foo.rs:9:52: 9:53
        FakeRead(ForGuardBinding, _7);   // scope 0 at foo.rs:9:52: 9:53
        StorageLive(_6);                 // scope 0 at foo.rs:9:46: 9:47
        _6 = ((_3 as Variant2).0: bool); // scope 0 at foo.rs:9:46: 9:47
        goto -> bb7;                     // scope 0 at foo.rs:8:9: 11:10
    }

    bb14: {
        goto -> bb15;                    // scope 0 at foo.rs:9:52: 9:53
    }

    bb15: {
        StorageDead(_9);                 // scope 0 at foo.rs:9:58: 9:59
        StorageDead(_7);                 // scope 0 at foo.rs:9:58: 9:59
        falseEdge -> [real: bb6, imaginary: bb4]; // scope 0 at foo.rs:9:52: 9:53
    }

    bb16: {
        StorageDead(_3);                 // scope 0 at foo.rs:12:5: 12:6
        goto -> bb1;                     // scope 0 at foo.rs:7:5: 12:6
    }

    bb17: {
        unreachable;                     // scope 0 at foo.rs:7:5: 12:6
    }

    bb18: {
        StorageDead(_1);                 // scope 0 at foo.rs:12:5: 12:6
        return;                          // scope 0 at foo.rs:13:2: 13:2
    }

    bb19 (cleanup): {
        resume;                          // scope 0 at foo.rs:6:1: 13:2
    }
}

EDIT: updated the MIR output so it's from the latest nightly

@camelid camelid added regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-bug Category: This is a bug. labels Sep 4, 2021
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Sep 4, 2021
@camelid
Copy link
Member Author

camelid commented Sep 4, 2021

It looks like the issue is that the locals are marked as live during the loop, which means that each iteration they get marked as live again.

@camelid
Copy link
Member Author

camelid commented Sep 4, 2021

Regression is in nightly-2021-09-04.

Commit range: 371f3cd...b7404c8

Confirmed that #88572 is the cause by checking locally with CI artifacts.

@camelid
Copy link
Member Author

camelid commented Sep 4, 2021

Here's a diff of the MIR for the MCVE before and after #88572 (note that all of the changes are from statements being moved around):

diff --git a/mir_dump-old/foo.main.-------.mir_map.0.mir b/mir_dump-new/foo.main.-------.mir_map.0.mir
index 468567de..4f94cb3e 100644
--- a/mir_dump-old/foo.main.-------.mir_map.0.mir
+++ b/mir_dump-new/foo.main.-------.mir_map.0.mir
@@ -53,6 +53,7 @@ fn main() -> () {
     bb7: {
         _2 = const ();                   // scope 1 at foo.rs:9:57: 9:59
         StorageDead(_6);                 // scope 0 at foo.rs:9:58: 9:59
+        StorageDead(_9);                 // scope 0 at foo.rs:9:58: 9:59
         StorageDead(_7);                 // scope 0 at foo.rs:9:58: 9:59
         goto -> bb16;                    // scope 0 at foo.rs:9:58: 9:59
     }
@@ -67,7 +68,6 @@ fn main() -> () {
     }
 
     bb9: {
-        StorageDead(_8);                 // scope 0 at foo.rs:9:58: 9:59
         FakeRead(ForMatchGuard, _5);     // scope 0 at foo.rs:9:52: 9:53
         FakeRead(ForGuardBinding, _7);   // scope 0 at foo.rs:9:52: 9:53
         StorageLive(_6);                 // scope 0 at foo.rs:9:27: 9:28
@@ -76,12 +76,12 @@ fn main() -> () {
     }
 
     bb10: {
-        StorageDead(_8);                 // scope 0 at foo.rs:9:58: 9:59
-        StorageDead(_7);                 // scope 0 at foo.rs:9:58: 9:59
         goto -> bb11;                    // scope 0 at foo.rs:9:52: 9:53
     }
 
     bb11: {
+        StorageDead(_8);                 // scope 0 at foo.rs:9:58: 9:59
+        StorageDead(_7);                 // scope 0 at foo.rs:9:58: 9:59
         falseEdge -> [real: bb4, imaginary: bb5]; // scope 0 at foo.rs:9:52: 9:53
     }
 
@@ -95,7 +95,6 @@ fn main() -> () {
     }
 
     bb13: {
-        StorageDead(_9);                 // scope 0 at foo.rs:9:58: 9:59
         FakeRead(ForMatchGuard, _5);     // scope 0 at foo.rs:9:52: 9:53
         FakeRead(ForGuardBinding, _7);   // scope 0 at foo.rs:9:52: 9:53
         StorageLive(_6);                 // scope 0 at foo.rs:9:46: 9:47
@@ -104,12 +103,12 @@ fn main() -> () {
     }
 
     bb14: {
-        StorageDead(_9);                 // scope 0 at foo.rs:9:58: 9:59
-        StorageDead(_7);                 // scope 0 at foo.rs:9:58: 9:59
         goto -> bb15;                    // scope 0 at foo.rs:9:52: 9:53
     }
 
     bb15: {
+        StorageDead(_9);                 // scope 0 at foo.rs:9:58: 9:59
+        StorageDead(_7);                 // scope 0 at foo.rs:9:58: 9:59
         falseEdge -> [real: bb6, imaginary: bb4]; // scope 0 at foo.rs:9:52: 9:53
     }

@RalfJung
Copy link
Member

RalfJung commented Sep 4, 2021

Yeah, these storage markers are supposed to be used in a well-bracketed way... I can only make wild guesses for what LLVM will do with them otherwise.^^

@matthewjasper any idea how hard this will be to fix? Miri will be at least partially broken until this issue is resolved.

@RalfJung
Copy link
Member

RalfJung commented Sep 4, 2021

Also Cc @oli-obk

@bors bors closed this as completed in 1c858ba Sep 6, 2021
@camelid camelid removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Sep 6, 2021
jackh726 added a commit to jackh726/rust that referenced this issue Sep 8, 2021
Add a regression test for rust-lang#88649

I noticed that rust-lang#88649 does not have a regression test, so I add one in this PR.

The test fails with this without rust-lang#88678:
```
error[E0080]: evaluation of constant value failed
  --> /checkout/src/test/ui/consts/issue-88649.rs:13:52
   |
LL |             Foo::Variant1(x) | Foo::Variant2(x) if x => {}
   |                                                    ^ StorageLive on a local that was already live

error: aborting due to previous error

For more information about this error, try `rustc --explain E0080`.
```
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 8, 2021
Rollup of 9 pull requests

Successful merges:

 - rust-lang#86263 (Rustdoc: Report Layout of enum variants)
 - rust-lang#88541 (Add regression test for rust-lang#74400)
 - rust-lang#88553 (Improve diagnostics for unary plus operators (rust-lang#88276))
 - rust-lang#88594 (More symbolic doc aliases)
 - rust-lang#88648 (Correct “copies” to “moves” in `<Option<T> as From<T>>::from` doc, and other copyediting)
 - rust-lang#88691 (Add a regression test for rust-lang#88649)
 - rust-lang#88694 (Drop 1.56 stabilizations from 1.55 release notes)
 - rust-lang#88712 (Fix docs for `uX::checked_next_multiple_of`)
 - rust-lang#88726 (Fix typo in `const_generics` replaced with `adt_const_params` note)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
cuviper pushed a commit to cuviper/rust that referenced this issue Sep 16, 2021
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 17, 2021
[beta] backports

- rustdoc: Fix ICE with `doc(hidden)` on tuple variant fields rust-lang#88639
- Fix 2021 `dyn` suggestion that used code as label rust-lang#88657
- Workaround blink/chromium grid layout limitation of 1000 rows rust-lang#88776
- Change scope of temporaries in match guards rust-lang#88678
- Add a regression test for rust-lang#88649 rust-lang#88691
- Revert anon union parsing rust-lang#88775
- Disable validate_maintainers. rust-lang#88977

Also drop stage0 rustfmt, because that's only supposed to be used on master.

r? `@Mark-Simulacrum`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-bug Category: This is a bug. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants