From afcd33a6fc37a5bfc4dbeebefffb73d54e95211c Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Wed, 9 Mar 2022 15:27:28 +0000 Subject: [PATCH 1/2] Add regression test --- src/test/ui/consts/issue-90762.rs | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 src/test/ui/consts/issue-90762.rs diff --git a/src/test/ui/consts/issue-90762.rs b/src/test/ui/consts/issue-90762.rs new file mode 100644 index 0000000000000..ef7c7deb62f27 --- /dev/null +++ b/src/test/ui/consts/issue-90762.rs @@ -0,0 +1,26 @@ +// run-pass +#![allow(unreachable_code)] + +use std::sync::atomic::{AtomicBool, Ordering}; + +struct Print(usize); + +impl Drop for Print { + fn drop(&mut self) { + FOO[self.0].store(true, Ordering::Relaxed); + } +} + +const A: Print = Print(0); +const B: Print = Print(1); + +static FOO: [AtomicBool; 3] = [AtomicBool::new(false), AtomicBool::new(false), AtomicBool::new(false)]; + +fn main() { + loop { + std::mem::forget(({A}, B, Print(2), break)); + } + for (i, b) in FOO.iter().enumerate() { + assert!(b.load(Ordering::Relaxed), "{} not set", i); + } +} \ No newline at end of file From db02e61038a83cdd0208019a9a5e8037c5c42a21 Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Wed, 9 Mar 2022 17:10:48 +0000 Subject: [PATCH 2/2] Generate an intermediate temporary for `Drop` constants. To limit the fallout from this, don't do this for the last (or only) operand in an rvalue. --- .../src/build/expr/as_operand.rs | 17 ++-- .../src/build/expr/as_rvalue.rs | 85 +++++++++++++++---- .../rustc_mir_build/src/build/expr/into.rs | 16 +++- compiler/rustc_mir_build/src/build/mod.rs | 12 +++ compiler/rustc_mir_build/src/lib.rs | 1 + ...line_into_box_place.main.Inline.32bit.diff | 8 +- ...line_into_box_place.main.Inline.64bit.diff | 8 +- src/test/ui/consts/issue-90762.rs | 13 ++- 8 files changed, 122 insertions(+), 38 deletions(-) diff --git a/compiler/rustc_mir_build/src/build/expr/as_operand.rs b/compiler/rustc_mir_build/src/build/expr/as_operand.rs index b627b0763a286..c413b8ff82b2b 100644 --- a/compiler/rustc_mir_build/src/build/expr/as_operand.rs +++ b/compiler/rustc_mir_build/src/build/expr/as_operand.rs @@ -1,7 +1,7 @@ //! See docs in build/expr/mod.rs use crate::build::expr::category::Category; -use crate::build::{BlockAnd, BlockAndExtension, Builder}; +use crate::build::{BlockAnd, BlockAndExtension, Builder, NeedsTemporary}; use rustc_middle::middle::region; use rustc_middle::mir::*; use rustc_middle::thir::*; @@ -20,7 +20,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { expr: &Expr<'tcx>, ) -> BlockAnd> { let local_scope = self.local_scope(); - self.as_operand(block, Some(local_scope), expr, None) + self.as_operand(block, Some(local_scope), expr, None, NeedsTemporary::Maybe) } /// Returns an operand suitable for use until the end of the current scope expression and @@ -94,32 +94,33 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { /// /// Like `as_local_call_operand`, except that the argument will /// not be valid once `scope` ends. + #[instrument(level = "debug", skip(self, scope))] crate fn as_operand( &mut self, mut block: BasicBlock, scope: Option, expr: &Expr<'tcx>, local_info: Option>>, + needs_temporary: NeedsTemporary, ) -> BlockAnd> { - debug!("as_operand(block={:?}, expr={:?} local_info={:?})", block, expr, local_info); let this = self; if let ExprKind::Scope { region_scope, lint_level, value } = expr.kind { let source_info = this.source_info(expr.span); let region_scope = (region_scope, source_info); return this.in_scope(region_scope, lint_level, |this| { - this.as_operand(block, scope, &this.thir[value], local_info) + this.as_operand(block, scope, &this.thir[value], local_info, needs_temporary) }); } let category = Category::of(&expr.kind).unwrap(); - debug!("as_operand: category={:?} for={:?}", category, expr.kind); + debug!(?category, ?expr.kind); match category { - Category::Constant => { + Category::Constant if let NeedsTemporary::No = needs_temporary || !expr.ty.needs_drop(this.tcx, this.param_env) => { let constant = this.as_constant(expr); block.and(Operand::Constant(Box::new(constant))) } - Category::Place | Category::Rvalue(..) => { + Category::Constant | Category::Place | Category::Rvalue(..) => { let operand = unpack!(block = this.as_temp(block, scope, expr, Mutability::Mut)); if this.local_decls[operand].local_info.is_none() { this.local_decls[operand].local_info = local_info; @@ -176,6 +177,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { } } - this.as_operand(block, scope, expr, None) + this.as_operand(block, scope, expr, None, NeedsTemporary::Maybe) } } diff --git a/compiler/rustc_mir_build/src/build/expr/as_rvalue.rs b/compiler/rustc_mir_build/src/build/expr/as_rvalue.rs index be777418433a5..d807500f1fbdc 100644 --- a/compiler/rustc_mir_build/src/build/expr/as_rvalue.rs +++ b/compiler/rustc_mir_build/src/build/expr/as_rvalue.rs @@ -4,7 +4,7 @@ use rustc_index::vec::Idx; use crate::build::expr::as_place::PlaceBase; use crate::build::expr::category::{Category, RvalueFunc}; -use crate::build::{BlockAnd, BlockAndExtension, Builder}; +use crate::build::{BlockAnd, BlockAndExtension, Builder, NeedsTemporary}; use rustc_hir::lang_items::LangItem; use rustc_middle::middle::region; use rustc_middle::mir::AssertKind; @@ -52,17 +52,28 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { }) } ExprKind::Repeat { value, count } => { - let value_operand = - unpack!(block = this.as_operand(block, scope, &this.thir[value], None)); + let value_operand = unpack!( + block = + this.as_operand(block, scope, &this.thir[value], None, NeedsTemporary::No) + ); block.and(Rvalue::Repeat(value_operand, count)) } ExprKind::Binary { op, lhs, rhs } => { - let lhs = unpack!(block = this.as_operand(block, scope, &this.thir[lhs], None)); - let rhs = unpack!(block = this.as_operand(block, scope, &this.thir[rhs], None)); + let lhs = unpack!( + block = + this.as_operand(block, scope, &this.thir[lhs], None, NeedsTemporary::Maybe) + ); + let rhs = unpack!( + block = + this.as_operand(block, scope, &this.thir[rhs], None, NeedsTemporary::No) + ); this.build_binary_op(block, op, expr_span, expr.ty, lhs, rhs) } ExprKind::Unary { op, arg } => { - let arg = unpack!(block = this.as_operand(block, scope, &this.thir[arg], None)); + let arg = unpack!( + block = + this.as_operand(block, scope, &this.thir[arg], None, NeedsTemporary::No) + ); // Check for -MIN on signed integers if this.check_overflow && op == UnOp::Neg && expr.ty.is_signed() { let bool_ty = this.tcx.types.bool; @@ -167,13 +178,17 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { block.and(Rvalue::Use(Operand::Move(Place::from(result)))) } ExprKind::Cast { source } => { - let source = - unpack!(block = this.as_operand(block, scope, &this.thir[source], None)); + let source = unpack!( + block = + this.as_operand(block, scope, &this.thir[source], None, NeedsTemporary::No) + ); block.and(Rvalue::Cast(CastKind::Misc, source, expr.ty)) } ExprKind::Pointer { cast, source } => { - let source = - unpack!(block = this.as_operand(block, scope, &this.thir[source], None)); + let source = unpack!( + block = + this.as_operand(block, scope, &this.thir[source], None, NeedsTemporary::No) + ); block.and(Rvalue::Cast(CastKind::Pointer(cast), source, expr.ty)) } ExprKind::Array { ref fields } => { @@ -208,7 +223,17 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { let fields: Vec<_> = fields .into_iter() .copied() - .map(|f| unpack!(block = this.as_operand(block, scope, &this.thir[f], None))) + .map(|f| { + unpack!( + block = this.as_operand( + block, + scope, + &this.thir[f], + None, + NeedsTemporary::Maybe + ) + ) + }) .collect(); block.and(Rvalue::Aggregate(Box::new(AggregateKind::Array(el_ty)), fields)) @@ -219,7 +244,17 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { let fields: Vec<_> = fields .into_iter() .copied() - .map(|f| unpack!(block = this.as_operand(block, scope, &this.thir[f], None))) + .map(|f| { + unpack!( + block = this.as_operand( + block, + scope, + &this.thir[f], + None, + NeedsTemporary::Maybe + ) + ) + }) .collect(); block.and(Rvalue::Aggregate(Box::new(AggregateKind::Tuple), fields)) @@ -296,7 +331,15 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { ) ), _ => { - unpack!(block = this.as_operand(block, scope, upvar, None)) + unpack!( + block = this.as_operand( + block, + scope, + upvar, + None, + NeedsTemporary::Maybe + ) + ) } } } @@ -325,13 +368,18 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { literal: ConstantKind::zero_sized(this.tcx.types.unit), })))) } - ExprKind::Yield { .. } - | ExprKind::Literal { .. } + + ExprKind::Literal { .. } | ExprKind::NamedConst { .. } | ExprKind::NonHirLiteral { .. } | ExprKind::ConstParam { .. } | ExprKind::ConstBlock { .. } - | ExprKind::StaticRef { .. } + | ExprKind::StaticRef { .. } => { + let constant = this.as_constant(expr); + block.and(Rvalue::Use(Operand::Constant(Box::new(constant)))) + } + + ExprKind::Yield { .. } | ExprKind::Block { .. } | ExprKind::Match { .. } | ExprKind::If { .. } @@ -359,9 +407,10 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { // so make an operand and then return that debug_assert!(!matches!( Category::of(&expr.kind), - Some(Category::Rvalue(RvalueFunc::AsRvalue)) + Some(Category::Rvalue(RvalueFunc::AsRvalue) | Category::Constant) )); - let operand = unpack!(block = this.as_operand(block, scope, expr, None)); + let operand = + unpack!(block = this.as_operand(block, scope, expr, None, NeedsTemporary::No)); block.and(Rvalue::Use(operand)) } } diff --git a/compiler/rustc_mir_build/src/build/expr/into.rs b/compiler/rustc_mir_build/src/build/expr/into.rs index cee657e9da244..4399fdf8520a1 100644 --- a/compiler/rustc_mir_build/src/build/expr/into.rs +++ b/compiler/rustc_mir_build/src/build/expr/into.rs @@ -1,7 +1,7 @@ //! See docs in build/expr/mod.rs use crate::build::expr::category::{Category, RvalueFunc}; -use crate::build::{BlockAnd, BlockAndExtension, BlockFrame, Builder}; +use crate::build::{BlockAnd, BlockAndExtension, BlockFrame, Builder, NeedsTemporary}; use rustc_ast::InlineAsmOptions; use rustc_data_structures::fx::FxHashMap; use rustc_data_structures::stack::ensure_sufficient_stack; @@ -329,7 +329,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { block, Some(scope), &this.thir[f.expr], - Some(local_info) + Some(local_info), + NeedsTemporary::Maybe, ) ), ) @@ -516,8 +517,15 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { ExprKind::Yield { value } => { let scope = this.local_scope(); - let value = - unpack!(block = this.as_operand(block, Some(scope), &this.thir[value], None)); + let value = unpack!( + block = this.as_operand( + block, + Some(scope), + &this.thir[value], + None, + NeedsTemporary::No + ) + ); let resume = this.cfg.start_new_block(); this.cfg.terminate( block, diff --git a/compiler/rustc_mir_build/src/build/mod.rs b/compiler/rustc_mir_build/src/build/mod.rs index 3c51f79186274..ce57e5fe846eb 100644 --- a/compiler/rustc_mir_build/src/build/mod.rs +++ b/compiler/rustc_mir_build/src/build/mod.rs @@ -549,6 +549,18 @@ rustc_index::newtype_index! { struct ScopeId { .. } } +#[derive(Debug)] +enum NeedsTemporary { + /// Use this variant when whatever you are converting with `as_operand` + /// is the last thing you are converting. This means that if we introduced + /// an intermediate temporary, we'd only read it immediately after, so we can + /// also avoid it. + No, + /// For all cases where you aren't sure or that are too expensive to compute + /// for now. It is always safe to fall back to this. + Maybe, +} + /////////////////////////////////////////////////////////////////////////// /// The `BlockAnd` "monad" packages up the new basic block along with a /// produced value (sometimes just unit, of course). The `unpack!` diff --git a/compiler/rustc_mir_build/src/lib.rs b/compiler/rustc_mir_build/src/lib.rs index 6687e1160ede8..a2a1a86ad989c 100644 --- a/compiler/rustc_mir_build/src/lib.rs +++ b/compiler/rustc_mir_build/src/lib.rs @@ -6,6 +6,7 @@ #![feature(box_patterns)] #![feature(control_flow_enum)] #![feature(crate_visibility_modifier)] +#![feature(if_let_guard)] #![feature(let_chains)] #![feature(let_else)] #![feature(min_specialization)] diff --git a/src/test/mir-opt/inline/inline_into_box_place.main.Inline.32bit.diff b/src/test/mir-opt/inline/inline_into_box_place.main.Inline.32bit.diff index 074ad067ff899..7613afdf4fef5 100644 --- a/src/test/mir-opt/inline/inline_into_box_place.main.Inline.32bit.diff +++ b/src/test/mir-opt/inline/inline_into_box_place.main.Inline.32bit.diff @@ -16,6 +16,7 @@ scope 2 { } + scope 3 (inlined Vec::::new) { // at $DIR/inline-into-box-place.rs:8:33: 8:43 ++ let mut _8: alloc::raw_vec::RawVec; // in scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL + } bb0: { @@ -34,8 +35,8 @@ - (*_5) = Vec::::new() -> [return: bb2, unwind: bb5]; // scope 0 at $DIR/inline-into-box-place.rs:8:33: 8:43 + StorageLive(_7); // scope 0 at $DIR/inline-into-box-place.rs:8:33: 8:43 + _7 = &mut (*_5); // scope 0 at $DIR/inline-into-box-place.rs:8:33: 8:43 -+ Deinit((*_7)); // scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL -+ ((*_7).0: alloc::raw_vec::RawVec) = const alloc::raw_vec::RawVec:: { ptr: Unique:: { pointer: NonNull:: { pointer: {0x4 as *const u32} }, _marker: PhantomData:: }, cap: 0_usize, alloc: std::alloc::Global }; // scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL ++ StorageLive(_8); // scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL ++ _8 = const alloc::raw_vec::RawVec:: { ptr: Unique:: { pointer: NonNull:: { pointer: {0x4 as *const u32} }, _marker: PhantomData:: }, cap: 0_usize, alloc: std::alloc::Global }; // scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL // mir::Constant - // + span: $DIR/inline-into-box-place.rs:8:33: 8:41 - // + user_ty: UserType(1) @@ -46,7 +47,10 @@ + // + span: $SRC_DIR/alloc/src/vec/mod.rs:LL:COL + // + user_ty: UserType(0) + // + literal: Const { ty: alloc::raw_vec::RawVec, val: Value(ByRef { alloc: Allocation { bytes: [4, 0, 0, 0, 0, 0, 0, 0], relocations: Relocations(SortedMap { data: [] }), init_mask: InitMask { blocks: [255], len: Size { raw: 8 } }, align: Align { pow2: 2 }, mutability: Not, extra: () }, offset: Size { raw: 0 } }) } ++ Deinit((*_7)); // scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL ++ ((*_7).0: alloc::raw_vec::RawVec) = move _8; // scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL + ((*_7).1: usize) = const 0_usize; // scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL ++ StorageDead(_8); // scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL + StorageDead(_7); // scope 0 at $DIR/inline-into-box-place.rs:8:33: 8:43 _1 = move _5; // scope 0 at $DIR/inline-into-box-place.rs:8:29: 8:43 StorageDead(_5); // scope 0 at $DIR/inline-into-box-place.rs:8:42: 8:43 diff --git a/src/test/mir-opt/inline/inline_into_box_place.main.Inline.64bit.diff b/src/test/mir-opt/inline/inline_into_box_place.main.Inline.64bit.diff index a055ae9864f5f..a2f70f61cac9d 100644 --- a/src/test/mir-opt/inline/inline_into_box_place.main.Inline.64bit.diff +++ b/src/test/mir-opt/inline/inline_into_box_place.main.Inline.64bit.diff @@ -16,6 +16,7 @@ scope 2 { } + scope 3 (inlined Vec::::new) { // at $DIR/inline-into-box-place.rs:8:33: 8:43 ++ let mut _8: alloc::raw_vec::RawVec; // in scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL + } bb0: { @@ -34,8 +35,8 @@ - (*_5) = Vec::::new() -> [return: bb2, unwind: bb5]; // scope 0 at $DIR/inline-into-box-place.rs:8:33: 8:43 + StorageLive(_7); // scope 0 at $DIR/inline-into-box-place.rs:8:33: 8:43 + _7 = &mut (*_5); // scope 0 at $DIR/inline-into-box-place.rs:8:33: 8:43 -+ Deinit((*_7)); // scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL -+ ((*_7).0: alloc::raw_vec::RawVec) = const alloc::raw_vec::RawVec:: { ptr: Unique:: { pointer: NonNull:: { pointer: {0x4 as *const u32} }, _marker: PhantomData:: }, cap: 0_usize, alloc: std::alloc::Global }; // scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL ++ StorageLive(_8); // scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL ++ _8 = const alloc::raw_vec::RawVec:: { ptr: Unique:: { pointer: NonNull:: { pointer: {0x4 as *const u32} }, _marker: PhantomData:: }, cap: 0_usize, alloc: std::alloc::Global }; // scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL // mir::Constant - // + span: $DIR/inline-into-box-place.rs:8:33: 8:41 - // + user_ty: UserType(1) @@ -46,7 +47,10 @@ + // + span: $SRC_DIR/alloc/src/vec/mod.rs:LL:COL + // + user_ty: UserType(0) + // + literal: Const { ty: alloc::raw_vec::RawVec, val: Value(ByRef { alloc: Allocation { bytes: [4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0], relocations: Relocations(SortedMap { data: [] }), init_mask: InitMask { blocks: [65535], len: Size { raw: 16 } }, align: Align { pow2: 3 }, mutability: Not, extra: () }, offset: Size { raw: 0 } }) } ++ Deinit((*_7)); // scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL ++ ((*_7).0: alloc::raw_vec::RawVec) = move _8; // scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL + ((*_7).1: usize) = const 0_usize; // scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL ++ StorageDead(_8); // scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL + StorageDead(_7); // scope 0 at $DIR/inline-into-box-place.rs:8:33: 8:43 _1 = move _5; // scope 0 at $DIR/inline-into-box-place.rs:8:29: 8:43 StorageDead(_5); // scope 0 at $DIR/inline-into-box-place.rs:8:42: 8:43 diff --git a/src/test/ui/consts/issue-90762.rs b/src/test/ui/consts/issue-90762.rs index ef7c7deb62f27..78d387386f895 100644 --- a/src/test/ui/consts/issue-90762.rs +++ b/src/test/ui/consts/issue-90762.rs @@ -1,26 +1,31 @@ // run-pass #![allow(unreachable_code)] -use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::atomic::{AtomicBool, Ordering, AtomicUsize}; struct Print(usize); impl Drop for Print { fn drop(&mut self) { + println!("{}", self.0); FOO[self.0].store(true, Ordering::Relaxed); + assert_eq!(BAR.fetch_sub(1, Ordering::Relaxed), self.0); } } const A: Print = Print(0); const B: Print = Print(1); -static FOO: [AtomicBool; 3] = [AtomicBool::new(false), AtomicBool::new(false), AtomicBool::new(false)]; +static FOO: [AtomicBool; 3] = + [AtomicBool::new(false), AtomicBool::new(false), AtomicBool::new(false)]; +static BAR: AtomicUsize = AtomicUsize::new(2); fn main() { loop { - std::mem::forget(({A}, B, Print(2), break)); + std::mem::forget(({ A }, B, Print(2), break)); } for (i, b) in FOO.iter().enumerate() { assert!(b.load(Ordering::Relaxed), "{} not set", i); } -} \ No newline at end of file + assert_eq!(BAR.fetch_add(1, Ordering::Relaxed), usize::max_value()); +}