diff --git a/compiler/rustc_const_eval/src/transform/validate.rs b/compiler/rustc_const_eval/src/transform/validate.rs index b249ffb84b3a8..0b73691204d53 100644 --- a/compiler/rustc_const_eval/src/transform/validate.rs +++ b/compiler/rustc_const_eval/src/transform/validate.rs @@ -74,7 +74,6 @@ impl<'tcx> MirPass<'tcx> for Validator { mir_phase, unwind_edge_count: 0, reachable_blocks: traversal::reachable_as_bitset(body), - place_cache: FxHashSet::default(), value_cache: FxHashSet::default(), can_unwind, }; @@ -106,7 +105,6 @@ struct CfgChecker<'a, 'tcx> { mir_phase: MirPhase, unwind_edge_count: usize, reachable_blocks: BitSet, - place_cache: FxHashSet>, value_cache: FxHashSet, // If `false`, then the MIR must not contain `UnwindAction::Continue` or // `TerminatorKind::Resume`. @@ -294,19 +292,6 @@ impl<'a, 'tcx> Visitor<'tcx> for CfgChecker<'a, 'tcx> { fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) { match &statement.kind { - StatementKind::Assign(box (dest, rvalue)) => { - // FIXME(JakobDegen): Check this for all rvalues, not just this one. - if let Rvalue::Use(Operand::Copy(src) | Operand::Move(src)) = rvalue { - // The sides of an assignment must not alias. Currently this just checks whether - // the places are identical. - if dest == src { - self.fail( - location, - "encountered `Assign` statement with overlapping memory", - ); - } - } - } StatementKind::AscribeUserType(..) => { if self.mir_phase >= MirPhase::Runtime(RuntimePhase::Initial) { self.fail( @@ -341,7 +326,8 @@ impl<'a, 'tcx> Visitor<'tcx> for CfgChecker<'a, 'tcx> { self.fail(location, format!("explicit `{kind:?}` is forbidden")); } } - StatementKind::StorageLive(_) + StatementKind::Assign(..) + | StatementKind::StorageLive(_) | StatementKind::StorageDead(_) | StatementKind::Intrinsic(_) | StatementKind::Coverage(_) @@ -404,10 +390,7 @@ impl<'a, 'tcx> Visitor<'tcx> for CfgChecker<'a, 'tcx> { } // The call destination place and Operand::Move place used as an argument might be - // passed by a reference to the callee. Consequently they must be non-overlapping - // and cannot be packed. Currently this simply checks for duplicate places. - self.place_cache.clear(); - self.place_cache.insert(destination.as_ref()); + // passed by a reference to the callee. Consequently they cannot be packed. if is_within_packed(self.tcx, &self.body.local_decls, *destination).is_some() { // This is bad! The callee will expect the memory to be aligned. self.fail( @@ -418,10 +401,8 @@ impl<'a, 'tcx> Visitor<'tcx> for CfgChecker<'a, 'tcx> { ), ); } - let mut has_duplicates = false; for arg in args { if let Operand::Move(place) = arg { - has_duplicates |= !self.place_cache.insert(place.as_ref()); if is_within_packed(self.tcx, &self.body.local_decls, *place).is_some() { // This is bad! The callee will expect the memory to be aligned. self.fail( @@ -434,16 +415,6 @@ impl<'a, 'tcx> Visitor<'tcx> for CfgChecker<'a, 'tcx> { } } } - - if has_duplicates { - self.fail( - location, - format!( - "encountered overlapping memory in `Move` arguments to `Call` terminator: {:?}", - terminator.kind, - ), - ); - } } TerminatorKind::Assert { target, unwind, .. } => { self.check_edge(location, *target, EdgeKind::Normal); @@ -1112,17 +1083,6 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { ) } } - // FIXME(JakobDegen): Check this for all rvalues, not just this one. - if let Rvalue::Use(Operand::Copy(src) | Operand::Move(src)) = rvalue { - // The sides of an assignment must not alias. Currently this just checks whether - // the places are identical. - if dest == src { - self.fail( - location, - "encountered `Assign` statement with overlapping memory", - ); - } - } } StatementKind::AscribeUserType(..) => { if self.mir_phase >= MirPhase::Runtime(RuntimePhase::Initial) { diff --git a/compiler/rustc_mir_transform/src/dest_prop.rs b/compiler/rustc_mir_transform/src/dest_prop.rs index 15502adfb5aad..cd80f423466bc 100644 --- a/compiler/rustc_mir_transform/src/dest_prop.rs +++ b/compiler/rustc_mir_transform/src/dest_prop.rs @@ -133,7 +133,6 @@ use std::collections::hash_map::{Entry, OccupiedEntry}; -use crate::simplify::remove_dead_blocks; use crate::MirPass; use rustc_data_structures::fx::FxHashMap; use rustc_index::bit_set::BitSet; @@ -241,12 +240,6 @@ impl<'tcx> MirPass<'tcx> for DestinationPropagation { apply_merges(body, tcx, &merges, &merged_locals); } - if round_count != 0 { - // Merging can introduce overlap between moved arguments and/or call destination in an - // unreachable code, which validator considers to be ill-formed. - remove_dead_blocks(body); - } - trace!(round_count); } } diff --git a/compiler/rustc_mir_transform/src/lint.rs b/compiler/rustc_mir_transform/src/lint.rs index 3940d0ddbf344..c0c0a3f5ee649 100644 --- a/compiler/rustc_mir_transform/src/lint.rs +++ b/compiler/rustc_mir_transform/src/lint.rs @@ -1,6 +1,7 @@ //! This pass statically detects code which has undefined behaviour or is likely to be erroneous. //! It can be used to locate problems in MIR building or optimizations. It assumes that all code //! can be executed, so it has false positives. +use rustc_data_structures::fx::FxHashSet; use rustc_index::bit_set::BitSet; use rustc_middle::mir::visit::{PlaceContext, Visitor}; use rustc_middle::mir::*; @@ -11,7 +12,6 @@ use rustc_mir_dataflow::{Analysis, ResultsCursor}; use std::borrow::Cow; pub fn lint_body<'tcx>(tcx: TyCtxt<'tcx>, body: &Body<'tcx>, when: String) { - let reachable_blocks = traversal::reachable_as_bitset(body); let always_live_locals = &always_storage_live_locals(body); let maybe_storage_live = MaybeStorageLive::new(Cow::Borrowed(always_live_locals)) @@ -24,17 +24,19 @@ pub fn lint_body<'tcx>(tcx: TyCtxt<'tcx>, body: &Body<'tcx>, when: String) { .iterate_to_fixpoint() .into_results_cursor(body); - Lint { + let mut lint = Lint { tcx, when, body, is_fn_like: tcx.def_kind(body.source.def_id()).is_fn_like(), always_live_locals, - reachable_blocks, maybe_storage_live, maybe_storage_dead, + places: Default::default(), + }; + for (bb, data) in traversal::reachable(body) { + lint.visit_basic_block_data(bb, data); } - .visit_body(body); } struct Lint<'a, 'tcx> { @@ -43,9 +45,9 @@ struct Lint<'a, 'tcx> { body: &'a Body<'tcx>, is_fn_like: bool, always_live_locals: &'a BitSet, - reachable_blocks: BitSet, maybe_storage_live: ResultsCursor<'a, 'tcx, MaybeStorageLive<'a>>, maybe_storage_dead: ResultsCursor<'a, 'tcx, MaybeStorageDead<'a>>, + places: FxHashSet>, } impl<'a, 'tcx> Lint<'a, 'tcx> { @@ -67,7 +69,7 @@ impl<'a, 'tcx> Lint<'a, 'tcx> { impl<'a, 'tcx> Visitor<'tcx> for Lint<'a, 'tcx> { fn visit_local(&mut self, local: Local, context: PlaceContext, location: Location) { - if self.reachable_blocks.contains(location.block) && context.is_use() { + if context.is_use() { self.maybe_storage_dead.seek_after_primary_effect(location); if self.maybe_storage_dead.get().contains(local) { self.fail(location, format!("use of local {local:?}, which has no storage here")); @@ -76,18 +78,28 @@ impl<'a, 'tcx> Visitor<'tcx> for Lint<'a, 'tcx> { } fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) { - match statement.kind { - StatementKind::StorageLive(local) => { - if self.reachable_blocks.contains(location.block) { - self.maybe_storage_live.seek_before_primary_effect(location); - if self.maybe_storage_live.get().contains(local) { + match &statement.kind { + StatementKind::Assign(box (dest, rvalue)) => { + if let Rvalue::Use(Operand::Copy(src) | Operand::Move(src)) = rvalue { + // The sides of an assignment must not alias. Currently this just checks whether + // the places are identical. + if dest == src { self.fail( location, - format!("StorageLive({local:?}) which already has storage here"), + "encountered `Assign` statement with overlapping memory", ); } } } + StatementKind::StorageLive(local) => { + self.maybe_storage_live.seek_before_primary_effect(location); + if self.maybe_storage_live.get().contains(*local) { + self.fail( + location, + format!("StorageLive({local:?}) which already has storage here"), + ); + } + } _ => {} } @@ -95,9 +107,9 @@ impl<'a, 'tcx> Visitor<'tcx> for Lint<'a, 'tcx> { } fn visit_terminator(&mut self, terminator: &Terminator<'tcx>, location: Location) { - match terminator.kind { + match &terminator.kind { TerminatorKind::Return => { - if self.is_fn_like && self.reachable_blocks.contains(location.block) { + if self.is_fn_like { self.maybe_storage_live.seek_after_primary_effect(location); for local in self.maybe_storage_live.get().iter() { if !self.always_live_locals.contains(local) { @@ -111,6 +123,28 @@ impl<'a, 'tcx> Visitor<'tcx> for Lint<'a, 'tcx> { } } } + TerminatorKind::Call { args, destination, .. } => { + // The call destination place and Operand::Move place used as an argument might be + // passed by a reference to the callee. Consequently they must be non-overlapping. + // Currently this simply checks for duplicate places. + self.places.clear(); + self.places.insert(destination.as_ref()); + let mut has_duplicates = false; + for arg in args { + if let Operand::Move(place) = arg { + has_duplicates |= !self.places.insert(place.as_ref()); + } + } + if has_duplicates { + self.fail( + location, + format!( + "encountered overlapping memory in `Move` arguments to `Call` terminator: {:?}", + terminator.kind, + ), + ); + } + } _ => {} } diff --git a/compiler/rustc_mir_transform/src/pass_manager.rs b/compiler/rustc_mir_transform/src/pass_manager.rs index 82074f1960d25..f4c572aec128a 100644 --- a/compiler/rustc_mir_transform/src/pass_manager.rs +++ b/compiler/rustc_mir_transform/src/pass_manager.rs @@ -109,14 +109,15 @@ fn run_passes_inner<'tcx>( phase_change: Option, validate_each: bool, ) { - let lint = tcx.sess.opts.unstable_opts.lint_mir & !body.should_skip(); - let validate = validate_each & tcx.sess.opts.unstable_opts.validate_mir & !body.should_skip(); let overridden_passes = &tcx.sess.opts.unstable_opts.mir_enable_passes; trace!(?overridden_passes); let prof_arg = tcx.sess.prof.enabled().then(|| format!("{:?}", body.source.def_id())); if !body.should_skip() { + let validate = validate_each & tcx.sess.opts.unstable_opts.validate_mir; + let lint = tcx.sess.opts.unstable_opts.lint_mir; + for pass in passes { let name = pass.name(); @@ -162,7 +163,12 @@ fn run_passes_inner<'tcx>( body.pass_count = 0; dump_mir_for_phase_change(tcx, body); - if validate || new_phase == MirPhase::Runtime(RuntimePhase::Optimized) { + + let validate = + (validate_each & tcx.sess.opts.unstable_opts.validate_mir & !body.should_skip()) + || new_phase == MirPhase::Runtime(RuntimePhase::Optimized); + let lint = tcx.sess.opts.unstable_opts.lint_mir & !body.should_skip(); + if validate { validate_body(tcx, body, format!("after phase change to {}", new_phase.name())); } if lint { diff --git a/tests/mir-opt/copy-prop/custom_move_arg.f.CopyProp.panic-unwind.diff b/tests/mir-opt/copy-prop/custom_move_arg.f.CopyProp.panic-unwind.diff index eb40183c1c954..7ba8530105179 100644 --- a/tests/mir-opt/copy-prop/custom_move_arg.f.CopyProp.panic-unwind.diff +++ b/tests/mir-opt/copy-prop/custom_move_arg.f.CopyProp.panic-unwind.diff @@ -8,14 +8,14 @@ bb0: { - _2 = _1; -- _0 = opaque::(move _1) -> [return: bb1, unwind continue]; -+ _0 = opaque::(_1) -> [return: bb1, unwind continue]; +- _0 = opaque::(move _1) -> [return: bb1, unwind unreachable]; ++ _0 = opaque::(_1) -> [return: bb1, unwind unreachable]; } bb1: { - _3 = move _2; -- _0 = opaque::(_3) -> [return: bb2, unwind continue]; -+ _0 = opaque::(_1) -> [return: bb2, unwind continue]; +- _0 = opaque::(_3) -> [return: bb2, unwind unreachable]; ++ _0 = opaque::(_1) -> [return: bb2, unwind unreachable]; } bb2: { diff --git a/tests/mir-opt/copy-prop/custom_move_arg.rs b/tests/mir-opt/copy-prop/custom_move_arg.rs index 45913626a8fe5..1ee294fcfd917 100644 --- a/tests/mir-opt/copy-prop/custom_move_arg.rs +++ b/tests/mir-opt/copy-prop/custom_move_arg.rs @@ -10,15 +10,15 @@ use core::intrinsics::mir::*; struct NotCopy(bool); // EMIT_MIR custom_move_arg.f.CopyProp.diff -#[custom_mir(dialect = "analysis", phase = "post-cleanup")] +#[custom_mir(dialect = "runtime")] fn f(_1: NotCopy) { mir!({ let _2 = _1; - Call(RET = opaque(Move(_1)), ReturnTo(bb1), UnwindContinue()) + Call(RET = opaque(Move(_1)), ReturnTo(bb1), UnwindUnreachable()) } bb1 = { let _3 = Move(_2); - Call(RET = opaque(_3), ReturnTo(bb2), UnwindContinue()) + Call(RET = opaque(_3), ReturnTo(bb2), UnwindUnreachable()) } bb2 = { Return() diff --git a/tests/mir-opt/copy-prop/move_projection.f.CopyProp.panic-unwind.diff b/tests/mir-opt/copy-prop/move_projection.f.CopyProp.panic-unwind.diff index ad3889639e0cf..b2b89968d70b0 100644 --- a/tests/mir-opt/copy-prop/move_projection.f.CopyProp.panic-unwind.diff +++ b/tests/mir-opt/copy-prop/move_projection.f.CopyProp.panic-unwind.diff @@ -9,13 +9,13 @@ bb0: { - _2 = _1; - _3 = move (_2.0: u8); -- _0 = opaque::(move _1) -> [return: bb1, unwind continue]; +- _0 = opaque::(move _1) -> [return: bb1, unwind unreachable]; + _3 = (_1.0: u8); -+ _0 = opaque::(_1) -> [return: bb1, unwind continue]; ++ _0 = opaque::(_1) -> [return: bb1, unwind unreachable]; } bb1: { - _0 = opaque::(move _3) -> [return: bb2, unwind continue]; + _0 = opaque::(move _3) -> [return: bb2, unwind unreachable]; } bb2: { diff --git a/tests/mir-opt/copy-prop/move_projection.rs b/tests/mir-opt/copy-prop/move_projection.rs index f02867814ac0a..f31e82c1f0302 100644 --- a/tests/mir-opt/copy-prop/move_projection.rs +++ b/tests/mir-opt/copy-prop/move_projection.rs @@ -11,17 +11,17 @@ fn opaque(_: impl Sized) -> bool { true } struct Foo(u8); -#[custom_mir(dialect = "analysis", phase = "post-cleanup")] +#[custom_mir(dialect = "runtime")] fn f(a: Foo) -> bool { mir!( { let b = a; // This is a move out of a copy, so must become a copy of `a.0`. let c = Move(b.0); - Call(RET = opaque(Move(a)), ReturnTo(bb1), UnwindContinue()) + Call(RET = opaque(Move(a)), ReturnTo(bb1), UnwindUnreachable()) } bb1 = { - Call(RET = opaque(Move(c)), ReturnTo(ret), UnwindContinue()) + Call(RET = opaque(Move(c)), ReturnTo(ret), UnwindUnreachable()) } ret = { Return() diff --git a/tests/mir-opt/dest-prop/unreachable.f.DestinationPropagation.panic-abort.diff b/tests/mir-opt/dest-prop/unreachable.f.DestinationPropagation.panic-abort.diff deleted file mode 100644 index 80b5681ad062c..0000000000000 --- a/tests/mir-opt/dest-prop/unreachable.f.DestinationPropagation.panic-abort.diff +++ /dev/null @@ -1,82 +0,0 @@ -- // MIR for `f` before DestinationPropagation -+ // MIR for `f` after DestinationPropagation - - fn f(_1: T) -> () { - debug a => _1; - let mut _0: (); - let _2: T; - let mut _3: bool; - let _4: (); - let mut _5: T; - let mut _6: T; - let _7: (); - let mut _8: T; - let mut _9: T; - scope 1 { -- debug b => _2; -+ debug b => _1; - } - - bb0: { -- StorageLive(_2); -- _2 = _1; -+ nop; -+ nop; - StorageLive(_3); - _3 = const false; -- goto -> bb3; -+ goto -> bb1; - } - - bb1: { -- StorageLive(_4); -- StorageLive(_5); -- _5 = _1; -- StorageLive(_6); -- _6 = _1; -- _4 = g::(_1, _1) -> [return: bb2, unwind unreachable]; -- } -- -- bb2: { -- StorageDead(_6); -- StorageDead(_5); -- StorageDead(_4); -- _0 = const (); -- goto -> bb5; -- } -- -- bb3: { - StorageLive(_7); -- StorageLive(_8); -- _8 = _1; -- StorageLive(_9); -- _9 = _1; -- _7 = g::(_1, _1) -> [return: bb4, unwind unreachable]; -+ nop; -+ nop; -+ nop; -+ nop; -+ _7 = g::(_1, _1) -> [return: bb2, unwind unreachable]; - } - -- bb4: { -- StorageDead(_9); -- StorageDead(_8); -+ bb2: { -+ nop; -+ nop; - StorageDead(_7); - _0 = const (); -- goto -> bb5; -+ goto -> bb3; - } - -- bb5: { -+ bb3: { - StorageDead(_3); -- StorageDead(_2); -+ nop; - return; - } - } - diff --git a/tests/mir-opt/dest-prop/unreachable.f.DestinationPropagation.panic-unwind.diff b/tests/mir-opt/dest-prop/unreachable.f.DestinationPropagation.panic-unwind.diff deleted file mode 100644 index eae7dd17b4882..0000000000000 --- a/tests/mir-opt/dest-prop/unreachable.f.DestinationPropagation.panic-unwind.diff +++ /dev/null @@ -1,82 +0,0 @@ -- // MIR for `f` before DestinationPropagation -+ // MIR for `f` after DestinationPropagation - - fn f(_1: T) -> () { - debug a => _1; - let mut _0: (); - let _2: T; - let mut _3: bool; - let _4: (); - let mut _5: T; - let mut _6: T; - let _7: (); - let mut _8: T; - let mut _9: T; - scope 1 { -- debug b => _2; -+ debug b => _1; - } - - bb0: { -- StorageLive(_2); -- _2 = _1; -+ nop; -+ nop; - StorageLive(_3); - _3 = const false; -- goto -> bb3; -+ goto -> bb1; - } - - bb1: { -- StorageLive(_4); -- StorageLive(_5); -- _5 = _1; -- StorageLive(_6); -- _6 = _1; -- _4 = g::(_1, _1) -> [return: bb2, unwind continue]; -- } -- -- bb2: { -- StorageDead(_6); -- StorageDead(_5); -- StorageDead(_4); -- _0 = const (); -- goto -> bb5; -- } -- -- bb3: { - StorageLive(_7); -- StorageLive(_8); -- _8 = _1; -- StorageLive(_9); -- _9 = _1; -- _7 = g::(_1, _1) -> [return: bb4, unwind continue]; -+ nop; -+ nop; -+ nop; -+ nop; -+ _7 = g::(_1, _1) -> [return: bb2, unwind continue]; - } - -- bb4: { -- StorageDead(_9); -- StorageDead(_8); -+ bb2: { -+ nop; -+ nop; - StorageDead(_7); - _0 = const (); -- goto -> bb5; -+ goto -> bb3; - } - -- bb5: { -+ bb3: { - StorageDead(_3); -- StorageDead(_2); -+ nop; - return; - } - } - diff --git a/tests/mir-opt/dest-prop/unreachable.rs b/tests/mir-opt/dest-prop/unreachable.rs deleted file mode 100644 index 0bde157ff6185..0000000000000 --- a/tests/mir-opt/dest-prop/unreachable.rs +++ /dev/null @@ -1,20 +0,0 @@ -// skip-filecheck -// EMIT_MIR_FOR_EACH_PANIC_STRATEGY -// Check that unreachable code is removed after the destination propagation. -// Regression test for issue #105428. -// -// compile-flags: --crate-type=lib -Zmir-opt-level=0 -// compile-flags: -Zmir-enable-passes=+GVN,+SimplifyConstCondition-after-const-prop,+DestinationPropagation - -// EMIT_MIR unreachable.f.DestinationPropagation.diff -pub fn f(a: T) { - let b = a; - if false { - g(a, b); - } else { - g(b, b); - } -} - -#[inline(never)] -pub fn g(_: T, _: T) {} diff --git a/tests/ui/mir/lint/assignment-overlap.rs b/tests/ui/mir/lint/assignment-overlap.rs new file mode 100644 index 0000000000000..0e4a11467dc8a --- /dev/null +++ b/tests/ui/mir/lint/assignment-overlap.rs @@ -0,0 +1,19 @@ +// compile-flags: --crate-type=lib -Zlint-mir -Ztreat-err-as-bug +// build-fail +// failure-status: 101 +// dont-check-compiler-stderr +// error-pattern: encountered `Assign` statement with overlapping memory +#![feature(custom_mir, core_intrinsics)] +extern crate core; +use core::intrinsics::mir::*; + +#[custom_mir(dialect = "runtime", phase = "optimized")] +pub fn main() { + mir!( + let a: [u8; 1024]; + { + a = a; + Return() + } + ) +} diff --git a/tests/ui/mir/lint/call-overlap.rs b/tests/ui/mir/lint/call-overlap.rs new file mode 100644 index 0000000000000..df38e901e7328 --- /dev/null +++ b/tests/ui/mir/lint/call-overlap.rs @@ -0,0 +1,23 @@ +// compile-flags: -Zlint-mir -Ztreat-err-as-bug +// build-fail +// failure-status: 101 +// dont-check-compiler-stderr +// error-pattern: encountered overlapping memory in `Move` arguments to `Call` +#![feature(custom_mir, core_intrinsics)] +extern crate core; +use core::intrinsics::mir::*; + +#[custom_mir(dialect = "runtime", phase = "optimized")] +pub fn main() { + mir!( + let a: [u8; 1024]; + { + Call(a = f(Move(a)), ReturnTo(bb1), UnwindUnreachable()) + } + bb1 = { + Return() + } + ) +} + +pub fn f(a: T) -> T { a }