mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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

Browse Source 
Mark `simplify_aggregate_to_copy` mir-opt as unsound

Mark the `simplify_aggregate_to_copy` mir-opt added in #128299 as unsound as it seems to miscompile the MCVE reported in https://github.com/rust-lang/rust/issues/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 #128299)
This commit is contained in:
bors 2024-10-31 15:29:14 +00:00
commit a0d98ff0e5
14 changed files with 192 additions and 43 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
compiler/rustc_mir_transform/src/gvn.rs
View File

@ -1082,7 +1082,9 @@ fn simplify_aggregate(
}
}
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)
{

2
tests/codegen/clone_as_copy.rs
View File

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

14
tests/codegen/try_question_mark_nop.rs
View File

@ -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 @@
#[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,

2
tests/mir-opt/gvn_clone.rs
View File

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

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

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

2
tests/mir-opt/gvn_copy_aggregate.rs
View File

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

2
tests/mir-opt/pre-codegen/clone_as_copy.rs
View File

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

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

@ -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;
}
}

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

@ -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;
}

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

@ -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;
}
}

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

@ -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;
}
}

72
tests/mir-opt/simplify_aggregate_to_copy_miscompile.foo.GVN.diff Normal file
View File

@ -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 __ __ __ __ │ ....░░░░
}

32
tests/mir-opt/simplify_aggregate_to_copy_miscompile.rs Normal file
View File

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

25
tests/ui/mir/clone-canonicalization-miscompile-132353.rs Normal file
View File

@ -0,0 +1,25 @@
//! The mir-opt added in <https://github.com/rust-lang/rust/pull/128299> unfortunately seems to lead
//! to a miscompile (reported in <https://github.com/rust-lang/rust/issues/132353>, minimization
//! reproduced in this test file).
//@ revisions: release debug
// Note: it's not strictly cargo's release profile, but any non-zero opt-level was sufficient to
// reproduce the miscompile.
//@[release] compile-flags: -C opt-level=1
//@[debug] compile-flags: -C opt-level=0
//@ run-pass
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 happened on 1.83.0 beta in release mode but not debug mode.
let _ = min.unwrap();
}