Rust: Slice index becomes wrong (beta regression)

[djc]

One of my tests (test_nested_rest) is failing on beta (in CI and locally) but not stable (locally).

https://github.com/djc/mendes/pull/18/checks?check_run_id=759480560

Minimal reproduction:

https://play.rust-lang.org/?version=beta&mode=debug&edition=2018&gist=e31da4955ca52b19e2131b51473bcb4e

When executed with miri, it doesn't panic. When the call to nothing() is moved before the assignment to self.prev, it also doesn't panic. Works on stable, panics on both beta and nightly-2020-06-10.

searched nightlies: from nightly-2020-04-12 to nightly-2020-06-10
regressed nightly: nightly-2020-05-15
searched commits: from https://github.com/rust-lang/rust/commit/75e1463c52aaea25bd32ed53c73797357e561cce to https://github.com/rust-lang/rust/commit/a74d1862d4d87a56244958416fd05976c58ca1a8
regressed commit: https://github.com/rust-lang/rust/commit/7c34d8d6629506a596215886e5fc4bb2b04b00ae

bisected with cargo-bisect-rustc v0.5.2

Host triple: x86_64-apple-darwin
Reproduce with:

cargo bisect-rustc --start 2020-04-12 --end 2020-06-10 -- run 

[Dylan-DPC]

This is the test in question

[djc]

Here's a reduced test case that doesn't quite reproduce what I'm seeing but has what I think should be a smoking gun:

https://play.rust-lang.org/?version=beta&mode=debug&edition=2018&gist=d1fa510430df349361ab658c7d51ad08

If you comment out the dbg!() statement on line 19, the slicing at line 21 panics even though start should be well within bounds. However, if you allow the dbg!() to execute, this code doesn't panic and prints the expected result.

[djc]

So here's a link to a further reduced panicking version:

https://play.rust-lang.org/?version=beta&mode=debug&edition=2018&gist=e31da4955ca52b19e2131b51473bcb4e

When executed with miri, it doesn't panic. When the call to nothing() is moved before the assignment to self.prev, it also doesn't panic.

Here's a reduced diff between the LLVM IR for a working and a panicking version:

--- okay.ir     2020-06-11 10:05:02.000000000 +0200
+++ panic.ir    2020-06-11 10:05:07.000000000 +0200
@@ -2,16 +2,15 @@
 ; Function Attrs: nonlazybind uwtable
 define internal void @_ZN10playground4main17he15b3beb12586d44E() unnamed_addr #1 {
 start:
-  %split.dbg.spill = alloca i64, align 8
-  %v.dbg.spill = alloca i64, align 8
-  %prev = alloca { i64, i64 }, align 8
-  %_14 = alloca i64, align 8
+  %prev.dbg.spill = alloca { i64, i64 }, align 8
+  %_15 = alloca i64, align 8
   %_5 = alloca { i64, i64 }, align 8
   %_3 = alloca { i64, i64 }, align 8
+  %split = alloca i64, align 8
   %ls = alloca [2 x i32], align 4
   %0 = alloca {}, align 1
   call void @llvm.dbg.declare(metadata [2 x i32]* %ls, metadata !679, metadata !DIExpression()),
-  call void @llvm.dbg.declare(metadata { i64, i64 }* %prev, metadata !688, metadata !DIExpression()),
+  call void @llvm.dbg.declare(metadata i64* %split, metadata !684, metadata !DIExpression()),
   %1 = bitcast [2 x i32]* %ls to i32*,
   store i32 0, i32* %1, align 4,
   %2 = getelementptr inbounds [2 x i32], [2 x i32]* %ls, i32 0, i32 1,
@@ -41,33 +40,31 @@
   unreachable,

 bb4:                                              ; preds = %bb1
-  %8 = bitcast { i64, i64 }* %_3 to %"core::option::Option<usize>::Some"*,
-  %9 = getelementptr inbounds %"core::option::Option<usize>::Some", %"core::option::Option<usize>::Some"* %8, i32 0, i32 1,
-  %v = load i64, i64* %9, align 8,
-  store i64 %v, i64* %v.dbg.spill, align 8,
-  call void @llvm.dbg.declare(metadata i64* %v.dbg.spill, metadata !686, metadata !DIExpression()),
-  store i64 %v, i64* %split.dbg.spill, align 8,
-  call void @llvm.dbg.declare(metadata i64* %split.dbg.spill, metadata !684, metadata !DIExpression()),
-  %_12.0 = bitcast [2 x i32]* %ls to [0 x i32]*,
-  store i64 %v, i64* %_14, align 8,
-  %10 = load i64, i64* %_14, align 8,
+  %8 = getelementptr inbounds { i64, i64 }, { i64, i64 }* %_3, i32 0, i32 0,
+  %prev.0 = load i64, i64* %8, align 8,, !range !192
+  %9 = getelementptr inbounds { i64, i64 }, { i64, i64 }* %_3, i32 0, i32 1,
+  %prev.1 = load i64, i64* %9, align 8,
+  %10 = getelementptr inbounds { i64, i64 }, { i64, i64 }* %prev.dbg.spill, i32 0, i32 0,
+  store i64 %prev.0, i64* %10, align 8,
+  %11 = getelementptr inbounds { i64, i64 }, { i64, i64 }* %prev.dbg.spill, i32 0, i32 1,
+  store i64 %prev.1, i64* %11, align 8,
+  call void @llvm.dbg.declare(metadata { i64, i64 }* %prev.dbg.spill, metadata !688, metadata !DIExpression()),
+  %_13.0 = bitcast [2 x i32]* %ls to [0 x i32]*,
+  %_16 = load i64, i64* %split, align 8,
+  store i64 %_16, i64* %_15, align 8,
+  %12 = load i64, i64* %_15, align 8,
 ; call core::slice::<impl core::ops::index::Index<I> for [T]>::index
-  %11 = call { [0 x i32]*, i64 } @"_ZN4core5slice74_$LT$impl$u20$core..ops..index..Index$LT$I$GT$$u20$for$u20$$u5b$T$u5d$$GT$5index17h58cba0627d7192e9E"([0 x i32]* noalias nonnull readonly align 4 %_12.0, i64 2, i64 %10, %"core::panic::Location"* noalias readonly align 8 dereferenceable(24) bitcast (<{ i8*, [16 x i8] }>* @alloc23 to %"core::panic::Location"*)),
-  %_11.0 = extractvalue { [0 x i32]*, i64 } %11, 0,
-  %_11.1 = extractvalue { [0 x i32]*, i64 } %11, 1,
+  %13 = call { [0 x i32]*, i64 } @"_ZN4core5slice74_$LT$impl$u20$core..ops..index..Index$LT$I$GT$$u20$for$u20$$u5b$T$u5d$$GT$5index17h58cba0627d7192e9E"([0 x i32]* noalias nonnull readonly align 4 %_13.0, i64 2, i64 %12, %"core::panic::Location"* noalias readonly align 8 dereferenceable(24) bitcast (<{ i8*, [16 x i8] }>* @alloc23 to %"core::panic::Location"*)),
+  %_12.0 = extractvalue { [0 x i32]*, i64 } %13, 0,
+  %_12.1 = extractvalue { [0 x i32]*, i64 } %13, 1,
   br label %bb5,

 bb5:                                              ; preds = %bb4
 ; call playground::nothing
-  call void @_ZN10playground7nothing17h8446c2fa6397d61fE([0 x i32]* noalias nonnull readonly align 4 %_11.0, i64 %_11.1),
+  call void @_ZN10playground7nothing17h8446c2fa6397d61fE([0 x i32]* noalias nonnull readonly align 4 %_12.0, i64 %_12.1),
   br label %bb6,

 bb6:                                              ; preds = %bb5
-  %12 = bitcast { i64, i64 }* %prev to %"core::option::Option<usize>::Some"*,
-  %13 = getelementptr inbounds %"core::option::Option<usize>::Some", %"core::option::Option<usize>::Some"* %12, i32 0, i32 1,
-  store i64 %v, i64* %13, align 8,
-  %14 = bitcast { i64, i64 }* %prev to i64*,
-  store i64 1, i64* %14, align 8,
   br label %bb7,

 bb7:                                              ; preds = %bb2, %bb6

[djc]

In my original test, adding a dbg!() statement before the self.prev = Some(start); statement also appears to fix the test. It's somewhat interesting that in the test, the slice index always becomes 0 in the wrong version, rather than the random values seen with the playground test.

[djc]

Bisection blames #69756. @wesleywiser / @oli-obk care to take a look?

[lcnr]

Another small repro:

fn main() {
    let split = match Some(1) {
        Some(v) => v,
        None => return,
    };

    let _prev = Some(split);
    assert_eq!(split, 1);
    //~^ fails with `left: 0, right: 1`
}

[spastorino]

Assigning P-critical as discussed as part of the Prioritization Working Group process and removing I-prioritize.

[Aaron1011]

Relevant MIR from before and after SimplifyArmIdentity:

before:

    bb3: {
        StorageLive(_4);                 // scope 0 at bad_arm.rs:3:14: 3:15
        _4 = ((_2 as Some).0: i32);      // scope 0 at bad_arm.rs:3:14: 3:15
        _1 = _4;                         // scope 2 at bad_arm.rs:3:20: 3:21
        StorageDead(_4);                 // scope 0 at bad_arm.rs:3:21: 3:22
        StorageDead(_2);                 // scope 0 at bad_arm.rs:5:6: 5:7
        StorageLive(_6);                 // scope 1 at bad_arm.rs:7:9: 7:14
        StorageLive(_7);                 // scope 1 at bad_arm.rs:7:22: 7:27
        _7 = _1;                         // scope 1 at bad_arm.rs:7:22: 7:27
        ((_6 as Some).0: i32) = move _7; // scope 1 at bad_arm.rs:7:17: 7:28
        discriminant(_6) = 1;            // scope 1 at bad_arm.rs:7:17: 7:28
        StorageDead(_7);                 // scope 1 at bad_arm.rs:7:27: 7:28

after:

    bb3: {
        _6 = move _2;                    // scope 1 at bad_arm.rs:7:17: 7:28
        StorageDead(_2);                 // scope 0 at bad_arm.rs:5:6: 5:7
        StorageLive(_6);                 // scope 1 at bad_arm.rs:7:9: 7:14

Note how we are assigning to _6 before the StorageLive(_6) statement.

[jonas-schievink]

cc https://github.com/rust-lang/rust/issues/72800

[RalfJung]

https://github.com/rust-lang/rust/pull/73262 landed, what is needed to close this regression ticket?

[oli-obk]

the PR needs to get backported

[LeSeulArtichaut]

PR has been backported. Unless we need to add some regression tests I think we can close this.

[JohnTitor]

I think #73210 will add regression tests (cc @wesleywiser). I'd +1 for closing issue or at least downgrading the prio.

[wesleywiser]

@JohnTitor Did you mean to link a different PR? I don't think that one is related to this issue.

[JohnTitor]

Did you mean to link a different PR? I don't think that one is related to this issue.

@wesleywiser Ahhh, sorry! I saw your comment that you would add regression tests in next PR so I commented without checking carefully. So, don't we have any PRs that add tests? Marking as E-needs-test so that we don't forget.

[wesleywiser]

@JohnTitor All good! The PR you're looking for is #73949 🙂