From 1c5f1762b71c46d537c9cbfd3cc60f82b9175793 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Wed, 23 Aug 2023 16:00:42 +0000 Subject: [PATCH 1/2] Do not convert copies of packed projections to moves. --- .../src/dead_store_elimination.rs | 4 +++ ...cked.DeadStoreElimination.panic-abort.diff | 15 +++++++++++ ...ked.DeadStoreElimination.panic-unwind.diff | 15 +++++++++++ .../dead-store-elimination/call_arg_copy.rs | 26 +++++++++++++++++++ 4 files changed, 60 insertions(+) create mode 100644 tests/mir-opt/dead-store-elimination/call_arg_copy.move_packed.DeadStoreElimination.panic-abort.diff create mode 100644 tests/mir-opt/dead-store-elimination/call_arg_copy.move_packed.DeadStoreElimination.panic-unwind.diff diff --git a/compiler/rustc_mir_transform/src/dead_store_elimination.rs b/compiler/rustc_mir_transform/src/dead_store_elimination.rs index 3f988930b5e66..47c1c465e5635 100644 --- a/compiler/rustc_mir_transform/src/dead_store_elimination.rs +++ b/compiler/rustc_mir_transform/src/dead_store_elimination.rs @@ -12,6 +12,7 @@ //! will still not cause any further changes. //! +use crate::util::is_disaligned; use rustc_index::bit_set::BitSet; use rustc_middle::mir::visit::Visitor; use rustc_middle::mir::*; @@ -31,6 +32,8 @@ pub fn eliminate<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>, borrowed: &BitS .iterate_to_fixpoint() .into_results_cursor(body); + let param_env = tcx.param_env_reveal_all_normalized(body.source.def_id()); + // For blocks with a call terminator, if an argument copy can be turned into a move, // record it as (block, argument index). let mut call_operands_to_move = Vec::new(); @@ -49,6 +52,7 @@ pub fn eliminate<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>, borrowed: &BitS && !place.is_indirect() && !borrowed.contains(place.local) && !state.contains(place.local) + && !is_disaligned(tcx, body, param_env, place) { call_operands_to_move.push((bb, index)); } diff --git a/tests/mir-opt/dead-store-elimination/call_arg_copy.move_packed.DeadStoreElimination.panic-abort.diff b/tests/mir-opt/dead-store-elimination/call_arg_copy.move_packed.DeadStoreElimination.panic-abort.diff new file mode 100644 index 0000000000000..dc0f9073416fd --- /dev/null +++ b/tests/mir-opt/dead-store-elimination/call_arg_copy.move_packed.DeadStoreElimination.panic-abort.diff @@ -0,0 +1,15 @@ +- // MIR for `move_packed` before DeadStoreElimination ++ // MIR for `move_packed` after DeadStoreElimination + + fn move_packed(_1: Packed) -> () { + let mut _0: (); + + bb0: { + _0 = use_both(const 0_i32, (_1.1: i32)) -> [return: bb1, unwind unreachable]; + } + + bb1: { + return; + } + } + diff --git a/tests/mir-opt/dead-store-elimination/call_arg_copy.move_packed.DeadStoreElimination.panic-unwind.diff b/tests/mir-opt/dead-store-elimination/call_arg_copy.move_packed.DeadStoreElimination.panic-unwind.diff new file mode 100644 index 0000000000000..86ef026ec5c88 --- /dev/null +++ b/tests/mir-opt/dead-store-elimination/call_arg_copy.move_packed.DeadStoreElimination.panic-unwind.diff @@ -0,0 +1,15 @@ +- // MIR for `move_packed` before DeadStoreElimination ++ // MIR for `move_packed` after DeadStoreElimination + + fn move_packed(_1: Packed) -> () { + let mut _0: (); + + bb0: { + _0 = use_both(const 0_i32, (_1.1: i32)) -> [return: bb1, unwind continue]; + } + + bb1: { + return; + } + } + diff --git a/tests/mir-opt/dead-store-elimination/call_arg_copy.rs b/tests/mir-opt/dead-store-elimination/call_arg_copy.rs index 41f91fc130611..f09cdee14822e 100644 --- a/tests/mir-opt/dead-store-elimination/call_arg_copy.rs +++ b/tests/mir-opt/dead-store-elimination/call_arg_copy.rs @@ -2,6 +2,12 @@ // unit-test: DeadStoreElimination // compile-flags: -Zmir-enable-passes=+CopyProp +#![feature(core_intrinsics)] +#![feature(custom_mir)] +#![allow(internal_features)] + +use std::intrinsics::mir::*; + #[inline(never)] fn use_both(_: i32, _: i32) {} @@ -10,6 +16,26 @@ fn move_simple(x: i32) { use_both(x, x); } +#[repr(packed)] +struct Packed { + x: u8, + y: i32, +} + +// EMIT_MIR call_arg_copy.move_packed.DeadStoreElimination.diff +#[custom_mir(dialect = "analysis")] +fn move_packed(packed: Packed) { + mir!( + { + Call(RET = use_both(0, packed.y), ret) + } + ret = { + Return() + } + ) +} + fn main() { move_simple(1); + move_packed(Packed { x: 0, y: 1 }); } From 15a68610dd06351feac543afcd0802e6e2622ac8 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Thu, 24 Aug 2023 15:42:55 +0000 Subject: [PATCH 2/2] Only check packed ADT. --- compiler/rustc_const_eval/src/util/alignment.rs | 2 +- compiler/rustc_const_eval/src/util/mod.rs | 2 +- .../rustc_mir_transform/src/dead_store_elimination.rs | 10 ++++++---- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_const_eval/src/util/alignment.rs b/compiler/rustc_const_eval/src/util/alignment.rs index 4f39dad205ae3..c1f0ff260d2d3 100644 --- a/compiler/rustc_const_eval/src/util/alignment.rs +++ b/compiler/rustc_const_eval/src/util/alignment.rs @@ -40,7 +40,7 @@ where } } -fn is_within_packed<'tcx, L>( +pub fn is_within_packed<'tcx, L>( tcx: TyCtxt<'tcx>, local_decls: &L, place: Place<'tcx>, diff --git a/compiler/rustc_const_eval/src/util/mod.rs b/compiler/rustc_const_eval/src/util/mod.rs index 289e342259547..0aef7fa469e4b 100644 --- a/compiler/rustc_const_eval/src/util/mod.rs +++ b/compiler/rustc_const_eval/src/util/mod.rs @@ -5,7 +5,7 @@ mod check_validity_requirement; mod compare_types; mod type_name; -pub use self::alignment::is_disaligned; +pub use self::alignment::{is_disaligned, is_within_packed}; pub use self::check_validity_requirement::check_validity_requirement; pub use self::compare_types::{is_equal_up_to_subtyping, is_subtype}; pub use self::type_name::type_name; diff --git a/compiler/rustc_mir_transform/src/dead_store_elimination.rs b/compiler/rustc_mir_transform/src/dead_store_elimination.rs index 47c1c465e5635..ef14105041b4f 100644 --- a/compiler/rustc_mir_transform/src/dead_store_elimination.rs +++ b/compiler/rustc_mir_transform/src/dead_store_elimination.rs @@ -12,7 +12,7 @@ //! will still not cause any further changes. //! -use crate::util::is_disaligned; +use crate::util::is_within_packed; use rustc_index::bit_set::BitSet; use rustc_middle::mir::visit::Visitor; use rustc_middle::mir::*; @@ -32,8 +32,6 @@ pub fn eliminate<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>, borrowed: &BitS .iterate_to_fixpoint() .into_results_cursor(body); - let param_env = tcx.param_env_reveal_all_normalized(body.source.def_id()); - // For blocks with a call terminator, if an argument copy can be turned into a move, // record it as (block, argument index). let mut call_operands_to_move = Vec::new(); @@ -52,7 +50,11 @@ pub fn eliminate<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>, borrowed: &BitS && !place.is_indirect() && !borrowed.contains(place.local) && !state.contains(place.local) - && !is_disaligned(tcx, body, param_env, place) + // If `place` is a projection of a disaligned field in a packed ADT, + // the move may be codegened as a pointer to that field. + // Using that disaligned pointer may trigger UB in the callee, + // so do nothing. + && is_within_packed(tcx, body, place).is_none() { call_operands_to_move.push((bb, index)); }