Auto merge of rust-lang#132356 - jieyouxu:unsound-simplify_aggregate_to_copy, r=cjgillot,DianQK

Mark `simplify_aggregate_to_copy` mir-opt as unsound

Mark the `simplify_aggregate_to_copy` mir-opt added in rust-lang#128299 as unsound as it seems to miscompile the MCVE reported in rust-lang#132353. The mir-opt can be re-enabled once this case is fixed.

```rs
fn pop_min(mut score2head: Vec<Option<usize>>) -> Option<usize> {
    loop {
        if let Some(col) = score2head[0] {
            score2head[0] = None;
            return Some(col);
        }
    }
}

fn main() {
    let min = pop_min(vec![Some(1)]);
    println!("min: {:?}", min);
    // panic happens here on beta in release mode
    // but not in debug mode
    min.unwrap();
}
```

This MCVE is included as a `run-pass` ui regression test in the first commit. I built the ui test with a nightly manually, and can reproduce the behavioral difference with `-C opt-level=0` and `-C opt-level=1`. Locally, this ui test will fail unless it was run on a compiler built with the second commit marking the mir-opt as unsound thus disabling it by default.

This PR **partially reverts** commit e7386b3, reversing changes made to 02b1be1. The mir-opt implementation is just marked as unsound but **not** reverted to make reland reviews easier. Test changes are **reverted if they were not pure additions**. Tests added by the original PR received `-Z unsound-mir-opts` compile-flags.

cc `@DianQK` `@cjgillot` (PR author and reviewer of rust-lang#128299)

Author: bors

compiler/rustc_mir_transform/src/gvn.rs

@@ -1082,7 +1082,9 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {
             }
         }
 
-        if let AggregateTy::Def(_, _) = ty
+        // unsound: https://github.com/rust-lang/rust/issues/132353
+        if tcx.sess.opts.unstable_opts.unsound_mir_opts
+            && let AggregateTy::Def(_, _) = ty
             && let Some(value) =
                 self.simplify_aggregate_to_copy(rvalue, location, &fields, variant_index)
         {

tests/codegen/clone_as_copy.rs

@@ -1,4 +1,6 @@
 //@ revisions: DEBUGINFO NODEBUGINFO
+//@ compile-flags: -Zunsound-mir-opts
+// FIXME: see <https://github.com/rust-lang/rust/issues/132353>
 //@ compile-flags: -O -Cno-prepopulate-passes
 //@ [DEBUGINFO] compile-flags: -Cdebuginfo=full
 

tests/codegen/try_question_mark_nop.rs

@@ -1,10 +1,7 @@
 //@ compile-flags: -O -Z merge-functions=disabled --edition=2021
 //@ only-x86_64
 // FIXME: Remove the `min-llvm-version`.
-//@ revisions: NINETEEN TWENTY
-//@[NINETEEN] min-llvm-version: 19
-//@[NINETEEN] ignore-llvm-version: 20-99
-//@[TWENTY] min-llvm-version: 20
+//@ min-llvm-version: 19
 
 #![crate_type = "lib"]
 #![feature(try_blocks)]
@@ -16,12 +13,9 @@ use std::ptr::NonNull;
 #[no_mangle]
 pub fn option_nop_match_32(x: Option<u32>) -> Option<u32> {
     // CHECK: start:
-    // NINETEEN-NEXT: [[TRUNC:%.*]] = trunc nuw i32 %0 to i1
-    // NINETEEN-NEXT: [[FIRST:%.*]] = select i1 [[TRUNC]], i32 %0
-    // NINETEEN-NEXT: insertvalue { i32, i32 } poison, i32 [[FIRST]], 0
-    // TWENTY-NEXT: insertvalue { i32, i32 } poison, i32 %0, 0
-    // CHECK-NEXT: insertvalue { i32, i32 }
-    // CHECK-NEXT: ret { i32, i32 }
+    // CHECK-NEXT: [[REG1:%.*]] = insertvalue { i32, i32 } poison, i32 %0, 0
+    // CHECK-NEXT: [[REG2:%.*]] = insertvalue { i32, i32 } [[REG1]], i32 %1, 1
+    // CHECK-NEXT: ret { i32, i32 } [[REG2]]
     match x {
         Some(x) => Some(x),
         None => None,

tests/mir-opt/gvn_clone.rs

@@ -1,3 +1,5 @@
+//@ compile-flags: -Zunsound-mir-opts
+// FIXME: see <https://github.com/rust-lang/rust/issues/132353>
 //@ test-mir-pass: GVN
 //@ compile-flags: -Zmir-enable-passes=+InstSimplify-before-inline
 

tests/mir-opt/gvn_clone.{impl#0}-clone.GVN.diff

@@ -1,7 +1,7 @@
-- // MIR for `<impl at $DIR/gvn_clone.rs:12:10: 12:15>::clone` before GVN
-+ // MIR for `<impl at $DIR/gvn_clone.rs:12:10: 12:15>::clone` after GVN
+- // MIR for `<impl at $DIR/gvn_clone.rs:14:10: 14:15>::clone` before GVN
++ // MIR for `<impl at $DIR/gvn_clone.rs:14:10: 14:15>::clone` after GVN
 
-  fn <impl at $DIR/gvn_clone.rs:12:10: 12:15>::clone(_1: &AllCopy) -> AllCopy {
+  fn <impl at $DIR/gvn_clone.rs:14:10: 14:15>::clone(_1: &AllCopy) -> AllCopy {
       debug self => _1;
       let mut _0: AllCopy;
       let mut _2: i32;

tests/mir-opt/gvn_copy_aggregate.rs

@@ -1,3 +1,5 @@
+//@ compile-flags: -Zunsound-mir-opts
+// FIXME: see <https://github.com/rust-lang/rust/issues/132353.
 //@ test-mir-pass: GVN
 //@ compile-flags: -Cpanic=abort
 

tests/mir-opt/pre-codegen/clone_as_copy.rs

@@ -1,3 +1,5 @@
+//@ compile-flags: -Zunsound-mir-opts
+// FIXME: see <https://github.com/rust-lang/rust/issues/132353>
 //@ compile-flags: -Cdebuginfo=full
 
 // Check if we have transformed the nested clone to the copy in the complete pipeline.

tests/mir-opt/pre-codegen/no_inlined_clone.{impl#0}-clone.PreCodegen.after.mir

@@ -3,9 +3,13 @@
 fn <impl at $DIR/no_inlined_clone.rs:9:10: 9:15>::clone(_1: &Foo) -> Foo {
     debug self => _1;
     let mut _0: Foo;
+    let mut _2: i32;
 
     bb0: {
-        _0 = copy (*_1);
+        StorageLive(_2);
+        _2 = copy ((*_1).0: i32);
+        _0 = Foo { a: move _2 };
+        StorageDead(_2);
         return;
     }
 }

tests/mir-opt/pre-codegen/try_identity.old.PreCodegen.after.mir

@@ -19,14 +19,14 @@ fn old(_1: Result<T, E>) -> Result<T, E> {
     }
 
     bb1: {
-        _3 = copy ((_1 as Ok).0: T);
-        _0 = copy _1;
+        _3 = move ((_1 as Ok).0: T);
+        _0 = Result::<T, E>::Ok(copy _3);
         goto -> bb3;
     }
 
     bb2: {
-        _4 = copy ((_1 as Err).0: E);
-        _0 = copy _1;
+        _4 = move ((_1 as Err).0: E);
+        _0 = Result::<T, E>::Err(copy _4);
         goto -> bb3;
     }
 

tests/mir-opt/pre-codegen/vec_deref.vec_deref_to_slice.PreCodegen.after.panic-abort.mir

@@ -7,7 +7,7 @@ fn vec_deref_to_slice(_1: &Vec<u8>) -> &[u8] {
         debug self => _1;
         scope 2 (inlined Vec::<u8>::as_slice) {
             debug self => _1;
-            let mut _6: usize;
+            let mut _7: usize;
             scope 3 (inlined Vec::<u8>::as_ptr) {
                 debug self => _1;
                 let mut _2: &alloc::raw_vec::RawVec<u8>;
@@ -16,6 +16,7 @@ fn vec_deref_to_slice(_1: &Vec<u8>) -> &[u8] {
                     let mut _3: &alloc::raw_vec::RawVecInner;
                     scope 5 (inlined alloc::raw_vec::RawVecInner::ptr::<u8>) {
                         debug self => _3;
+                        let mut _6: std::ptr::NonNull<u8>;
                         scope 6 (inlined alloc::raw_vec::RawVecInner::non_null::<u8>) {
                             debug self => _3;
                             let mut _4: std::ptr::NonNull<u8>;
@@ -31,20 +32,20 @@ fn vec_deref_to_slice(_1: &Vec<u8>) -> &[u8] {
                                 }
                             }
                             scope 10 (inlined Unique::<u8>::as_non_null_ptr) {
-                                debug ((self: Unique<u8>).0: std::ptr::NonNull<u8>) => _4;
+                                debug ((self: Unique<u8>).0: std::ptr::NonNull<u8>) => _6;
                                 debug ((self: Unique<u8>).1: std::marker::PhantomData<u8>) => const PhantomData::<u8>;
                             }
                         }
                         scope 11 (inlined NonNull::<u8>::as_ptr) {
-                            debug self => _4;
+                            debug self => _6;
                         }
                     }
                 }
             }
             scope 12 (inlined std::slice::from_raw_parts::<'_, u8>) {
                 debug data => _5;
-                debug len => _6;
-                let _7: *const [u8];
+                debug len => _7;
+                let _8: *const [u8];
                 scope 13 (inlined core::ub_checks::check_language_ub) {
                     scope 14 (inlined core::ub_checks::check_language_ub::runtime) {
                     }
@@ -55,10 +56,10 @@ fn vec_deref_to_slice(_1: &Vec<u8>) -> &[u8] {
                 }
                 scope 17 (inlined slice_from_raw_parts::<u8>) {
                     debug data => _5;
-                    debug len => _6;
+                    debug len => _7;
                     scope 18 (inlined std::ptr::from_raw_parts::<[u8], u8>) {
                         debug data_pointer => _5;
-                        debug metadata => _6;
+                        debug metadata => _7;
                     }
                 }
             }
@@ -70,17 +71,22 @@ fn vec_deref_to_slice(_1: &Vec<u8>) -> &[u8] {
         _2 = &((*_1).0: alloc::raw_vec::RawVec<u8>);
         StorageLive(_3);
         _3 = &(((*_1).0: alloc::raw_vec::RawVec<u8>).0: alloc::raw_vec::RawVecInner);
+        StorageLive(_6);
+        StorageLive(_4);
         _4 = copy (((((*_1).0: alloc::raw_vec::RawVec<u8>).0: alloc::raw_vec::RawVecInner).0: std::ptr::Unique<u8>).0: std::ptr::NonNull<u8>);
         _5 = copy (_4.0: *const u8);
+        _6 = NonNull::<u8> { pointer: copy _5 };
+        StorageDead(_4);
+        StorageDead(_6);
         StorageDead(_3);
         StorageDead(_2);
-        StorageLive(_6);
-        _6 = copy ((*_1).1: usize);
         StorageLive(_7);
-        _7 = *const [u8] from (copy _5, copy _6);
-        _0 = &(*_7);
+        _7 = copy ((*_1).1: usize);
+        StorageLive(_8);
+        _8 = *const [u8] from (copy _5, copy _7);
+        _0 = &(*_8);
+        StorageDead(_8);
         StorageDead(_7);
-        StorageDead(_6);
         return;
     }
 }

tests/mir-opt/pre-codegen/vec_deref.vec_deref_to_slice.PreCodegen.after.panic-unwind.mir

@@ -7,7 +7,7 @@ fn vec_deref_to_slice(_1: &Vec<u8>) -> &[u8] {
         debug self => _1;
         scope 2 (inlined Vec::<u8>::as_slice) {
             debug self => _1;
-            let mut _6: usize;
+            let mut _7: usize;
             scope 3 (inlined Vec::<u8>::as_ptr) {
                 debug self => _1;
                 let mut _2: &alloc::raw_vec::RawVec<u8>;
@@ -16,6 +16,7 @@ fn vec_deref_to_slice(_1: &Vec<u8>) -> &[u8] {
                     let mut _3: &alloc::raw_vec::RawVecInner;
                     scope 5 (inlined alloc::raw_vec::RawVecInner::ptr::<u8>) {
                         debug self => _3;
+                        let mut _6: std::ptr::NonNull<u8>;
                         scope 6 (inlined alloc::raw_vec::RawVecInner::non_null::<u8>) {
                             debug self => _3;
                             let mut _4: std::ptr::NonNull<u8>;
@@ -31,20 +32,20 @@ fn vec_deref_to_slice(_1: &Vec<u8>) -> &[u8] {
                                 }
                             }
                             scope 10 (inlined Unique::<u8>::as_non_null_ptr) {
-                                debug ((self: Unique<u8>).0: std::ptr::NonNull<u8>) => _4;
+                                debug ((self: Unique<u8>).0: std::ptr::NonNull<u8>) => _6;
                                 debug ((self: Unique<u8>).1: std::marker::PhantomData<u8>) => const PhantomData::<u8>;
                             }
                         }
                         scope 11 (inlined NonNull::<u8>::as_ptr) {
-                            debug self => _4;
+                            debug self => _6;
                         }
                     }
                 }
             }
             scope 12 (inlined std::slice::from_raw_parts::<'_, u8>) {
                 debug data => _5;
-                debug len => _6;
-                let _7: *const [u8];
+                debug len => _7;
+                let _8: *const [u8];
                 scope 13 (inlined core::ub_checks::check_language_ub) {
                     scope 14 (inlined core::ub_checks::check_language_ub::runtime) {
                     }
@@ -55,10 +56,10 @@ fn vec_deref_to_slice(_1: &Vec<u8>) -> &[u8] {
                 }
                 scope 17 (inlined slice_from_raw_parts::<u8>) {
                     debug data => _5;
-                    debug len => _6;
+                    debug len => _7;
                     scope 18 (inlined std::ptr::from_raw_parts::<[u8], u8>) {
                         debug data_pointer => _5;
-                        debug metadata => _6;
+                        debug metadata => _7;
                     }
                 }
             }
@@ -70,17 +71,22 @@ fn vec_deref_to_slice(_1: &Vec<u8>) -> &[u8] {
         _2 = &((*_1).0: alloc::raw_vec::RawVec<u8>);
         StorageLive(_3);
         _3 = &(((*_1).0: alloc::raw_vec::RawVec<u8>).0: alloc::raw_vec::RawVecInner);
+        StorageLive(_6);
+        StorageLive(_4);
         _4 = copy (((((*_1).0: alloc::raw_vec::RawVec<u8>).0: alloc::raw_vec::RawVecInner).0: std::ptr::Unique<u8>).0: std::ptr::NonNull<u8>);
         _5 = copy (_4.0: *const u8);
+        _6 = NonNull::<u8> { pointer: copy _5 };
+        StorageDead(_4);
+        StorageDead(_6);
         StorageDead(_3);
         StorageDead(_2);
-        StorageLive(_6);
-        _6 = copy ((*_1).1: usize);
         StorageLive(_7);
-        _7 = *const [u8] from (copy _5, copy _6);
-        _0 = &(*_7);
+        _7 = copy ((*_1).1: usize);
+        StorageLive(_8);
+        _8 = *const [u8] from (copy _5, copy _7);
+        _0 = &(*_8);
+        StorageDead(_8);
         StorageDead(_7);
-        StorageDead(_6);
         return;
     }
 }

tests/mir-opt/simplify_aggregate_to_copy_miscompile.foo.GVN.diff

@@ -0,0 +1,72 @@
+- // MIR for `foo` before GVN
++ // MIR for `foo` after GVN
+  
+  fn foo(_1: &mut Option<i32>) -> Option<i32> {
+      debug v => _1;
+      let mut _0: std::option::Option<i32>;
+      let mut _2: &std::option::Option<i32>;
+      let mut _3: &std::option::Option<i32>;
+      let _4: &&mut std::option::Option<i32>;
+      let mut _5: isize;
+      let mut _7: !;
+      let mut _8: std::option::Option<i32>;
+      let mut _9: i32;
+      let mut _10: !;
+      let mut _11: &mut std::option::Option<i32>;
+      scope 1 {
+          debug col => _6;
+          let _6: i32;
+      }
+  
+      bb0: {
+-         StorageLive(_2);
++         nop;
+          StorageLive(_3);
+          StorageLive(_4);
+          _4 = &_1;
+-         _11 = deref_copy (*_4);
+-         _3 = &(*_11);
++         _11 = copy _1;
++         _3 = &(*_1);
+          _2 = get(move _3) -> [return: bb1, unwind unreachable];
+      }
+  
+      bb1: {
+          StorageDead(_3);
+          _5 = discriminant((*_2));
+          switchInt(move _5) -> [1: bb2, otherwise: bb3];
+      }
+  
+      bb2: {
+-         StorageLive(_6);
++         nop;
+          _6 = copy (((*_2) as Some).0: i32);
+          StorageLive(_8);
+-         _8 = Option::<i32>::None;
+-         (*_1) = move _8;
++         _8 = const Option::<i32>::None;
++         (*_1) = const Option::<i32>::None;
+          StorageDead(_8);
+          StorageLive(_9);
+          _9 = copy _6;
+-         _0 = Option::<i32>::Some(move _9);
++         _0 = copy (*_2);
+          StorageDead(_9);
+-         StorageDead(_6);
++         nop;
+          StorageDead(_4);
+-         StorageDead(_2);
++         nop;
+          return;
+      }
+  
+      bb3: {
+          StorageLive(_10);
+          unreachable;
+      }
++ }
++ 
++ ALLOC0 (size: 8, align: 4) {
++     00 00 00 00 __ __ __ __                         │ ....░░░░
+  }
+  

tests/mir-opt/simplify_aggregate_to_copy_miscompile.rs

@@ -0,0 +1,32 @@
+//! The `simplify_aggregate_to_copy` mir-opt introduced in
+//! <https://github.com/rust-lang/rust/pull/128299> caused a miscompile because the initial
+//! implementation
+//!
+//! > introduce[d] new dereferences without checking for aliasing
+//!
+//! This test demonstrates the behavior, and should be adjusted or removed when fixing and relanding
+//! the mir-opt.
+#![crate_type = "lib"]
+// skip-filecheck
+//@ compile-flags: -O -Zunsound-mir-opts
+//@ test-mir-pass: GVN
+#![allow(internal_features)]
+#![feature(rustc_attrs, core_intrinsics)]
+
+// EMIT_MIR simplify_aggregate_to_copy_miscompile.foo.GVN.diff
+#[no_mangle]
+fn foo(v: &mut Option<i32>) -> Option<i32> {
+    if let &Some(col) = get(&v) {
+        *v = None;
+        return Some(col);
+    } else {
+        unsafe { std::intrinsics::unreachable() }
+    }
+}
+
+#[no_mangle]
+#[inline(never)]
+#[rustc_nounwind]
+fn get(v: &Option<i32>) -> &Option<i32> {
+    v
+}