From 93672a364a8f05a4e8d2371916a90b5200c284e2 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 24 Feb 2024 17:16:05 +0100 Subject: [PATCH 1/4] promotion: do not promote const-fn calls in const when that may fail without the entire const failing --- compiler/rustc_mir_transform/src/lib.rs | 15 +-- .../rustc_mir_transform/src/promote_consts.rs | 98 +++++++++++++++---- .../const-eval/promoted_errors.noopt.stderr | 44 --------- .../const-eval/promoted_errors.opt.stderr | 44 --------- ...ted_errors.opt_with_overflow_checks.stderr | 44 --------- tests/ui/consts/const-eval/promoted_errors.rs | 52 ---------- tests/ui/consts/promote-not.rs | 9 ++ tests/ui/consts/promote-not.stderr | 50 ++++++---- tests/ui/consts/promotion.rs | 36 ++++--- tests/ui/rfcs/rfc-1623-static/rfc1623-2.rs | 12 ++- .../ui/rfcs/rfc-1623-static/rfc1623-2.stderr | 42 +++++++- 11 files changed, 196 insertions(+), 250 deletions(-) delete mode 100644 tests/ui/consts/const-eval/promoted_errors.noopt.stderr delete mode 100644 tests/ui/consts/const-eval/promoted_errors.opt.stderr delete mode 100644 tests/ui/consts/const-eval/promoted_errors.opt_with_overflow_checks.stderr delete mode 100644 tests/ui/consts/const-eval/promoted_errors.rs diff --git a/compiler/rustc_mir_transform/src/lib.rs b/compiler/rustc_mir_transform/src/lib.rs index 24bc263e5a778..1b920a3399138 100644 --- a/compiler/rustc_mir_transform/src/lib.rs +++ b/compiler/rustc_mir_transform/src/lib.rs @@ -344,13 +344,6 @@ fn mir_promoted( body.tainted_by_errors = Some(error_reported); } - let mut required_consts = Vec::new(); - let mut required_consts_visitor = RequiredConstsVisitor::new(&mut required_consts); - for (bb, bb_data) in traversal::reverse_postorder(&body) { - required_consts_visitor.visit_basic_block_data(bb, bb_data); - } - body.required_consts = required_consts; - // What we need to run borrowck etc. let promote_pass = promote_consts::PromoteTemps::default(); pm::run_passes( @@ -360,6 +353,14 @@ fn mir_promoted( Some(MirPhase::Analysis(AnalysisPhase::Initial)), ); + // Promotion generates new consts; we run this after promotion to ensure they are accounted for. + let mut required_consts = Vec::new(); + let mut required_consts_visitor = RequiredConstsVisitor::new(&mut required_consts); + for (bb, bb_data) in traversal::reverse_postorder(&body) { + required_consts_visitor.visit_basic_block_data(bb, bb_data); + } + body.required_consts = required_consts; + let promoted = promote_pass.promoted_fragments.into_inner(); (tcx.alloc_steal_mir(body), tcx.alloc_steal_promoted(promoted)) } diff --git a/compiler/rustc_mir_transform/src/promote_consts.rs b/compiler/rustc_mir_transform/src/promote_consts.rs index 9fe8c34a8bf5d..226641a5bc726 100644 --- a/compiler/rustc_mir_transform/src/promote_consts.rs +++ b/compiler/rustc_mir_transform/src/promote_consts.rs @@ -13,6 +13,7 @@ //! move analysis runs after promotion on broken MIR. use either::{Left, Right}; +use rustc_data_structures::fx::FxHashSet; use rustc_hir as hir; use rustc_middle::mir; use rustc_middle::mir::visit::{MutVisitor, MutatingUseContext, PlaceContext, Visitor}; @@ -175,6 +176,12 @@ fn collect_temps_and_candidates<'tcx>( struct Validator<'a, 'tcx> { ccx: &'a ConstCx<'a, 'tcx>, temps: &'a mut IndexSlice, + /// For backwards compatibility, we are promoting function calls in `const`/`static` + /// initializers. But we want to avoid evaluating code that might panic and that otherwise would + /// not have been evaluated, so we only promote such calls in basic blocks that are guaranteed + /// to execute. In other words, we only promote such calls in basic blocks that are definitely + /// not dead code. Here we cache the result of computing that set of basic blocks. + promotion_safe_blocks: Option>, } impl<'a, 'tcx> std::ops::Deref for Validator<'a, 'tcx> { @@ -260,7 +267,9 @@ impl<'tcx> Validator<'_, 'tcx> { self.validate_rvalue(rhs) } Right(terminator) => match &terminator.kind { - TerminatorKind::Call { func, args, .. } => self.validate_call(func, args), + TerminatorKind::Call { func, args, .. } => { + self.validate_call(func, args, loc.block) + } TerminatorKind::Yield { .. } => Err(Unpromotable), kind => { span_bug!(terminator.source_info.span, "{:?} not promotable", kind); @@ -587,29 +596,79 @@ impl<'tcx> Validator<'_, 'tcx> { Ok(()) } + /// Computes the sets of blocks of this MIR that are definitely going to be executed + /// if the function returns successfully. That makes it safe to promote calls in them + /// that might fail. + fn promotion_safe_blocks(body: &mir::Body<'tcx>) -> FxHashSet { + let mut safe_blocks = FxHashSet::default(); + let mut safe_block = START_BLOCK; + loop { + safe_blocks.insert(safe_block); + // Let's see if we can find another safe block. + safe_block = match body.basic_blocks[safe_block].terminator().kind { + TerminatorKind::Goto { target } => target, + TerminatorKind::Call { target: Some(target), .. } + | TerminatorKind::Drop { target, .. } => { + // This calls a function or the destructor. `target` does not get executed if + // the callee loops or panics. But in both cases the const already fails to + // evaluate, so we are fine considering `target` a safe block for promotion. + target + } + TerminatorKind::Assert { target, .. } => { + // Similar to above, we only consider successful execution. + target + } + _ => { + // No next safe block. + break; + } + }; + } + safe_blocks + } + + /// Returns whether the block is "safe" for promotion, which means it cannot be dead code. + /// We use this to avoid promoting operations that can fail in dead code. + fn is_promotion_safe_block(&mut self, block: BasicBlock) -> bool { + let body = self.body; + let safe_blocks = + self.promotion_safe_blocks.get_or_insert_with(|| Self::promotion_safe_blocks(body)); + safe_blocks.contains(&block) + } + fn validate_call( &mut self, callee: &Operand<'tcx>, args: &[Spanned>], + block: BasicBlock, ) -> Result<(), Unpromotable> { + // Validate the operands. If they fail, there's no question -- we cannot promote. + self.validate_operand(callee)?; + for arg in args { + self.validate_operand(&arg.node)?; + } + + // Functions marked `#[rustc_promotable]` are explicitly allowed to be promoted, so we can + // accept them at this point. let fn_ty = callee.ty(self.body, self.tcx); + if let ty::FnDef(def_id, _) = *fn_ty.kind() { + if self.tcx.is_promotable_const_fn(def_id) { + return Ok(()); + } + } - // Inside const/static items, we promote all (eligible) function calls. - // Everywhere else, we require `#[rustc_promotable]` on the callee. - let promote_all_const_fn = matches!( + // Ideally, we'd stop here and reject the rest. + // But for backward compatibility, we have to accept some promotion in const/static + // initializers. Inline consts are explicitly excluded, they are more recent so we have no + // backwards compatibility reason to allow more promotion inside of them. + let promote_all_fn = matches!( self.const_kind, Some(hir::ConstContext::Static(_) | hir::ConstContext::Const { inline: false }) ); - if !promote_all_const_fn { - if let ty::FnDef(def_id, _) = *fn_ty.kind() { - // Never promote runtime `const fn` calls of - // functions without `#[rustc_promotable]`. - if !self.tcx.is_promotable_const_fn(def_id) { - return Err(Unpromotable); - } - } + if !promote_all_fn { + return Err(Unpromotable); } - + // Make sure the callee is a `const fn`. let is_const_fn = match *fn_ty.kind() { ty::FnDef(def_id, _) => self.tcx.is_const_fn_raw(def_id), _ => false, @@ -617,12 +676,13 @@ impl<'tcx> Validator<'_, 'tcx> { if !is_const_fn { return Err(Unpromotable); } - - self.validate_operand(callee)?; - for arg in args { - self.validate_operand(&arg.node)?; + // The problem is, this may promote calls to functions that panic. + // We don't want to introduce compilation errors if there's a panic in a call in dead code. + // So we ensure that this is not dead code. + if !self.is_promotion_safe_block(block) { + return Err(Unpromotable); } - + // This passed all checks, so let's accept. Ok(()) } } @@ -633,7 +693,7 @@ fn validate_candidates( temps: &mut IndexSlice, candidates: &[Candidate], ) -> Vec { - let mut validator = Validator { ccx, temps }; + let mut validator = Validator { ccx, temps, promotion_safe_blocks: None }; candidates .iter() diff --git a/tests/ui/consts/const-eval/promoted_errors.noopt.stderr b/tests/ui/consts/const-eval/promoted_errors.noopt.stderr deleted file mode 100644 index 2a254bfde8204..0000000000000 --- a/tests/ui/consts/const-eval/promoted_errors.noopt.stderr +++ /dev/null @@ -1,44 +0,0 @@ -warning: this arithmetic operation will overflow - --> $DIR/promoted_errors.rs:15:5 - | -LL | 0 - 1 - | ^^^^^ attempt to compute `0_u32 - 1_u32`, which would overflow - | -note: the lint level is defined here - --> $DIR/promoted_errors.rs:11:9 - | -LL | #![warn(arithmetic_overflow, unconditional_panic)] - | ^^^^^^^^^^^^^^^^^^^ - -warning: this operation will panic at runtime - --> $DIR/promoted_errors.rs:19:5 - | -LL | 1 / 0 - | ^^^^^ attempt to divide `1_i32` by zero - | -note: the lint level is defined here - --> $DIR/promoted_errors.rs:11:30 - | -LL | #![warn(arithmetic_overflow, unconditional_panic)] - | ^^^^^^^^^^^^^^^^^^^ - -warning: this operation will panic at runtime - --> $DIR/promoted_errors.rs:23:5 - | -LL | 1 / (1 - 1) - | ^^^^^^^^^^^ attempt to divide `1_i32` by zero - -warning: this operation will panic at runtime - --> $DIR/promoted_errors.rs:27:5 - | -LL | 1 / (false as i32) - | ^^^^^^^^^^^^^^^^^^ attempt to divide `1_i32` by zero - -warning: this operation will panic at runtime - --> $DIR/promoted_errors.rs:31:5 - | -LL | [1, 2, 3][4] - | ^^^^^^^^^^^^ index out of bounds: the length is 3 but the index is 4 - -warning: 5 warnings emitted - diff --git a/tests/ui/consts/const-eval/promoted_errors.opt.stderr b/tests/ui/consts/const-eval/promoted_errors.opt.stderr deleted file mode 100644 index 2a254bfde8204..0000000000000 --- a/tests/ui/consts/const-eval/promoted_errors.opt.stderr +++ /dev/null @@ -1,44 +0,0 @@ -warning: this arithmetic operation will overflow - --> $DIR/promoted_errors.rs:15:5 - | -LL | 0 - 1 - | ^^^^^ attempt to compute `0_u32 - 1_u32`, which would overflow - | -note: the lint level is defined here - --> $DIR/promoted_errors.rs:11:9 - | -LL | #![warn(arithmetic_overflow, unconditional_panic)] - | ^^^^^^^^^^^^^^^^^^^ - -warning: this operation will panic at runtime - --> $DIR/promoted_errors.rs:19:5 - | -LL | 1 / 0 - | ^^^^^ attempt to divide `1_i32` by zero - | -note: the lint level is defined here - --> $DIR/promoted_errors.rs:11:30 - | -LL | #![warn(arithmetic_overflow, unconditional_panic)] - | ^^^^^^^^^^^^^^^^^^^ - -warning: this operation will panic at runtime - --> $DIR/promoted_errors.rs:23:5 - | -LL | 1 / (1 - 1) - | ^^^^^^^^^^^ attempt to divide `1_i32` by zero - -warning: this operation will panic at runtime - --> $DIR/promoted_errors.rs:27:5 - | -LL | 1 / (false as i32) - | ^^^^^^^^^^^^^^^^^^ attempt to divide `1_i32` by zero - -warning: this operation will panic at runtime - --> $DIR/promoted_errors.rs:31:5 - | -LL | [1, 2, 3][4] - | ^^^^^^^^^^^^ index out of bounds: the length is 3 but the index is 4 - -warning: 5 warnings emitted - diff --git a/tests/ui/consts/const-eval/promoted_errors.opt_with_overflow_checks.stderr b/tests/ui/consts/const-eval/promoted_errors.opt_with_overflow_checks.stderr deleted file mode 100644 index 2a254bfde8204..0000000000000 --- a/tests/ui/consts/const-eval/promoted_errors.opt_with_overflow_checks.stderr +++ /dev/null @@ -1,44 +0,0 @@ -warning: this arithmetic operation will overflow - --> $DIR/promoted_errors.rs:15:5 - | -LL | 0 - 1 - | ^^^^^ attempt to compute `0_u32 - 1_u32`, which would overflow - | -note: the lint level is defined here - --> $DIR/promoted_errors.rs:11:9 - | -LL | #![warn(arithmetic_overflow, unconditional_panic)] - | ^^^^^^^^^^^^^^^^^^^ - -warning: this operation will panic at runtime - --> $DIR/promoted_errors.rs:19:5 - | -LL | 1 / 0 - | ^^^^^ attempt to divide `1_i32` by zero - | -note: the lint level is defined here - --> $DIR/promoted_errors.rs:11:30 - | -LL | #![warn(arithmetic_overflow, unconditional_panic)] - | ^^^^^^^^^^^^^^^^^^^ - -warning: this operation will panic at runtime - --> $DIR/promoted_errors.rs:23:5 - | -LL | 1 / (1 - 1) - | ^^^^^^^^^^^ attempt to divide `1_i32` by zero - -warning: this operation will panic at runtime - --> $DIR/promoted_errors.rs:27:5 - | -LL | 1 / (false as i32) - | ^^^^^^^^^^^^^^^^^^ attempt to divide `1_i32` by zero - -warning: this operation will panic at runtime - --> $DIR/promoted_errors.rs:31:5 - | -LL | [1, 2, 3][4] - | ^^^^^^^^^^^^ index out of bounds: the length is 3 but the index is 4 - -warning: 5 warnings emitted - diff --git a/tests/ui/consts/const-eval/promoted_errors.rs b/tests/ui/consts/const-eval/promoted_errors.rs deleted file mode 100644 index e806d4a32468d..0000000000000 --- a/tests/ui/consts/const-eval/promoted_errors.rs +++ /dev/null @@ -1,52 +0,0 @@ -//@ revisions: noopt opt opt_with_overflow_checks -//@[noopt]compile-flags: -C opt-level=0 -//@[opt]compile-flags: -O -//@[opt_with_overflow_checks]compile-flags: -C overflow-checks=on -O - -//@ build-pass -//@ ignore-pass (test emits codegen-time warnings and verifies that they are not errors) - -//! This test ensures that when we promote code that fails to evaluate, the build still succeeds. - -#![warn(arithmetic_overflow, unconditional_panic)] - -// The only way to have promoteds that fail is in `const fn` called from `const`/`static`. -const fn overflow() -> u32 { - 0 - 1 - //~^ WARN this arithmetic operation will overflow -} -const fn div_by_zero1() -> i32 { - 1 / 0 - //~^ WARN this operation will panic at runtime -} -const fn div_by_zero2() -> i32 { - 1 / (1 - 1) - //~^ WARN this operation will panic at runtime -} -const fn div_by_zero3() -> i32 { - 1 / (false as i32) - //~^ WARN this operation will panic at runtime -} -const fn oob() -> i32 { - [1, 2, 3][4] - //~^ WARN this operation will panic at runtime -} - -const fn mk_false() -> bool { false } - -// An actually used constant referencing failing promoteds in dead code. -// This needs to always work. -const Y: () = { - if mk_false() { - let _x: &'static u32 = &overflow(); - let _x: &'static i32 = &div_by_zero1(); - let _x: &'static i32 = &div_by_zero2(); - let _x: &'static i32 = &div_by_zero3(); - let _x: &'static i32 = &oob(); - } - () -}; - -fn main() { - Y; -} diff --git a/tests/ui/consts/promote-not.rs b/tests/ui/consts/promote-not.rs index 9b16f32532a77..80912937f3158 100644 --- a/tests/ui/consts/promote-not.rs +++ b/tests/ui/consts/promote-not.rs @@ -51,6 +51,15 @@ const TEST_DROP_NOT_PROMOTE: &String = { }; +// We do not promote function calls in `const` initializers in dead code. +const fn mk_panic() -> u32 { panic!() } +const fn mk_false() -> bool { false } +const Y: () = { + if mk_false() { + let _x: &'static u32 = &mk_panic(); //~ ERROR temporary value dropped while borrowed + } +}; + fn main() { // We must not promote things with interior mutability. Not even if we "project it away". let _val: &'static _ = &(Cell::new(1), 2).0; //~ ERROR temporary value dropped while borrowed diff --git a/tests/ui/consts/promote-not.stderr b/tests/ui/consts/promote-not.stderr index 07d4a135ed43d..d8b6091dc9aab 100644 --- a/tests/ui/consts/promote-not.stderr +++ b/tests/ui/consts/promote-not.stderr @@ -47,6 +47,16 @@ LL | let x = &String::new(); LL | }; | - value is dropped here +error[E0716]: temporary value dropped while borrowed + --> $DIR/promote-not.rs:59:33 + | +LL | let _x: &'static u32 = &mk_panic(); + | ------------ ^^^^^^^^^^ creates a temporary value which is freed while still in use + | | + | type annotation requires that borrow lasts for `'static` +LL | } + | - temporary value is freed at the end of this statement + error[E0716]: temporary value dropped while borrowed --> $DIR/promote-not.rs:21:32 | @@ -68,7 +78,7 @@ LL | } | - temporary value is freed at the end of this statement error[E0716]: temporary value dropped while borrowed - --> $DIR/promote-not.rs:56:29 + --> $DIR/promote-not.rs:65:29 | LL | let _val: &'static _ = &(Cell::new(1), 2).0; | ---------- ^^^^^^^^^^^^^^^^^ creates a temporary value which is freed while still in use @@ -79,7 +89,7 @@ LL | } | - temporary value is freed at the end of this statement error[E0716]: temporary value dropped while borrowed - --> $DIR/promote-not.rs:57:29 + --> $DIR/promote-not.rs:66:29 | LL | let _val: &'static _ = &(Cell::new(1), 2).1; | ---------- ^^^^^^^^^^^^^^^^^ creates a temporary value which is freed while still in use @@ -90,7 +100,7 @@ LL | } | - temporary value is freed at the end of this statement error[E0716]: temporary value dropped while borrowed - --> $DIR/promote-not.rs:60:29 + --> $DIR/promote-not.rs:69:29 | LL | let _val: &'static _ = &(1/0); | ---------- ^^^^^ creates a temporary value which is freed while still in use @@ -101,7 +111,7 @@ LL | } | - temporary value is freed at the end of this statement error[E0716]: temporary value dropped while borrowed - --> $DIR/promote-not.rs:61:29 + --> $DIR/promote-not.rs:70:29 | LL | let _val: &'static _ = &(1/(1-1)); | ---------- ^^^^^^^^^ creates a temporary value which is freed while still in use @@ -112,7 +122,7 @@ LL | } | - temporary value is freed at the end of this statement error[E0716]: temporary value dropped while borrowed - --> $DIR/promote-not.rs:62:29 + --> $DIR/promote-not.rs:71:29 | LL | let _val: &'static _ = &((1+1)/(1-1)); | ---------- ^^^^^^^^^^^^^ creates a temporary value which is freed while still in use @@ -123,7 +133,7 @@ LL | } | - temporary value is freed at the end of this statement error[E0716]: temporary value dropped while borrowed - --> $DIR/promote-not.rs:63:29 + --> $DIR/promote-not.rs:72:29 | LL | let _val: &'static _ = &(i32::MIN/-1); | ---------- ^^^^^^^^^^^^^ creates a temporary value which is freed while still in use @@ -134,7 +144,7 @@ LL | } | - temporary value is freed at the end of this statement error[E0716]: temporary value dropped while borrowed - --> $DIR/promote-not.rs:64:29 + --> $DIR/promote-not.rs:73:29 | LL | let _val: &'static _ = &(i32::MIN/(0-1)); | ---------- ^^^^^^^^^^^^^^^^ creates a temporary value which is freed while still in use @@ -145,7 +155,7 @@ LL | } | - temporary value is freed at the end of this statement error[E0716]: temporary value dropped while borrowed - --> $DIR/promote-not.rs:65:29 + --> $DIR/promote-not.rs:74:29 | LL | let _val: &'static _ = &(-128i8/-1); | ---------- ^^^^^^^^^^^ creates a temporary value which is freed while still in use @@ -156,7 +166,7 @@ LL | } | - temporary value is freed at the end of this statement error[E0716]: temporary value dropped while borrowed - --> $DIR/promote-not.rs:66:29 + --> $DIR/promote-not.rs:75:29 | LL | let _val: &'static _ = &(1%0); | ---------- ^^^^^ creates a temporary value which is freed while still in use @@ -167,7 +177,7 @@ LL | } | - temporary value is freed at the end of this statement error[E0716]: temporary value dropped while borrowed - --> $DIR/promote-not.rs:67:29 + --> $DIR/promote-not.rs:76:29 | LL | let _val: &'static _ = &(1%(1-1)); | ---------- ^^^^^^^^^ creates a temporary value which is freed while still in use @@ -178,7 +188,7 @@ LL | } | - temporary value is freed at the end of this statement error[E0716]: temporary value dropped while borrowed - --> $DIR/promote-not.rs:68:29 + --> $DIR/promote-not.rs:77:29 | LL | let _val: &'static _ = &([1,2,3][4]+1); | ---------- ^^^^^^^^^^^^^^ creates a temporary value which is freed while still in use @@ -189,7 +199,7 @@ LL | } | - temporary value is freed at the end of this statement error[E0716]: temporary value dropped while borrowed - --> $DIR/promote-not.rs:72:29 + --> $DIR/promote-not.rs:81:29 | LL | let _val: &'static _ = &TEST_DROP; | ---------- ^^^^^^^^^ creates a temporary value which is freed while still in use @@ -200,7 +210,7 @@ LL | } | - temporary value is freed at the end of this statement error[E0716]: temporary value dropped while borrowed - --> $DIR/promote-not.rs:74:29 + --> $DIR/promote-not.rs:83:29 | LL | let _val: &'static _ = &&TEST_DROP; | ---------- ^^^^^^^^^^ creates a temporary value which is freed while still in use @@ -211,7 +221,7 @@ LL | } | - temporary value is freed at the end of this statement error[E0716]: temporary value dropped while borrowed - --> $DIR/promote-not.rs:74:30 + --> $DIR/promote-not.rs:83:30 | LL | let _val: &'static _ = &&TEST_DROP; | ---------- ^^^^^^^^^ creates a temporary value which is freed while still in use @@ -222,7 +232,7 @@ LL | } | - temporary value is freed at the end of this statement error[E0716]: temporary value dropped while borrowed - --> $DIR/promote-not.rs:77:29 + --> $DIR/promote-not.rs:86:29 | LL | let _val: &'static _ = &(&TEST_DROP,); | ---------- ^^^^^^^^^^^^^ creates a temporary value which is freed while still in use @@ -233,7 +243,7 @@ LL | } | - temporary value is freed at the end of this statement error[E0716]: temporary value dropped while borrowed - --> $DIR/promote-not.rs:77:31 + --> $DIR/promote-not.rs:86:31 | LL | let _val: &'static _ = &(&TEST_DROP,); | ---------- ^^^^^^^^^ creates a temporary value which is freed while still in use @@ -244,7 +254,7 @@ LL | } | - temporary value is freed at the end of this statement error[E0716]: temporary value dropped while borrowed - --> $DIR/promote-not.rs:80:29 + --> $DIR/promote-not.rs:89:29 | LL | let _val: &'static _ = &[&TEST_DROP; 1]; | ---------- ^^^^^^^^^^^^^^^ creates a temporary value which is freed while still in use @@ -255,7 +265,7 @@ LL | } | - temporary value is freed at the end of this statement error[E0716]: temporary value dropped while borrowed - --> $DIR/promote-not.rs:80:31 + --> $DIR/promote-not.rs:89:31 | LL | let _val: &'static _ = &[&TEST_DROP; 1]; | ---------- ^^^^^^^^^ - temporary value is freed at the end of this statement @@ -264,7 +274,7 @@ LL | let _val: &'static _ = &[&TEST_DROP; 1]; | type annotation requires that borrow lasts for `'static` error[E0716]: temporary value dropped while borrowed - --> $DIR/promote-not.rs:89:26 + --> $DIR/promote-not.rs:98:26 | LL | let x: &'static _ = &UnionWithCell { f1: 0 }; | ---------- ^^^^^^^^^^^^^^^^^^^^^^^ creates a temporary value which is freed while still in use @@ -274,7 +284,7 @@ LL | LL | } | - temporary value is freed at the end of this statement -error: aborting due to 26 previous errors +error: aborting due to 27 previous errors Some errors have detailed explanations: E0493, E0716. For more information about an error, try `rustc --explain E0493`. diff --git a/tests/ui/consts/promotion.rs b/tests/ui/consts/promotion.rs index b18495a4a6bf5..457e807c970b3 100644 --- a/tests/ui/consts/promotion.rs +++ b/tests/ui/consts/promotion.rs @@ -5,28 +5,30 @@ //@ build-pass +#![allow(arithmetic_overflow)] + +use std::mem; + const fn assert_static(_: &'static T) {} -#[allow(unconditional_panic)] -const fn fail() -> i32 { - 1/0 -} -const C: i32 = { - // Promoted that fails to evaluate in dead code -- this must work - // (for backwards compatibility reasons). - if false { - assert_static(&fail()); - } +// Function calls in const on the "main path" (not inside conditionals) +// do get promoted. +const fn make_thing() -> i32 { 42 +} +const C: () = { + assert_static(&make_thing()); + // Make sure this works even when there's other stuff (like function calls) above the relevant + // call in the const initializer. + assert_static(&make_thing()); }; fn main() { assert_static(&["a", "b", "c"]); assert_static(&["d", "e", "f"]); - assert_eq!(C, 42); // make sure that this does not cause trouble despite overflowing - assert_static(&(0-1)); + assert_static(&(0u32 - 1)); // div-by-non-0 (and also not MIN/-1) is okay assert_static(&(1/1)); @@ -36,12 +38,16 @@ fn main() { assert_static(&(1%1)); // in-bounds array access is okay - assert_static(&([1,2,3][0] + 1)); - assert_static(&[[1,2][1]]); + assert_static(&([1, 2, 3][0] + 1)); + assert_static(&[[1, 2][1]]); // Top-level projections are not part of the promoted, so no error here. if false { #[allow(unconditional_panic)] - assert_static(&[1,2,3][4]); + assert_static(&[1, 2, 3][4]); } + + // More complicated case involving control flow and a `#[rustc_promotable]` function + let decision = std::hint::black_box(true); + let x: &'static usize = if decision { &mem::size_of::() } else { &0 }; } diff --git a/tests/ui/rfcs/rfc-1623-static/rfc1623-2.rs b/tests/ui/rfcs/rfc-1623-static/rfc1623-2.rs index 5d11941414f43..34739b22b96e9 100644 --- a/tests/ui/rfcs/rfc-1623-static/rfc1623-2.rs +++ b/tests/ui/rfcs/rfc-1623-static/rfc1623-2.rs @@ -4,7 +4,7 @@ fn non_elidable<'a, 'b>(a: &'a u8, b: &'b u8) -> &'a u8 { a } -// The incorrect case without `for<'a>` is tested for in `rfc1623-2.rs` +// The incorrect case without `for<'a>` is tested for in `rfc1623-3.rs` static NON_ELIDABLE_FN: &for<'a> fn(&'a u8, &'a u8) -> &'a u8 = &(non_elidable as for<'a> fn(&'a u8, &'a u8) -> &'a u8); @@ -26,10 +26,14 @@ static SOME_STRUCT: &SomeStruct = &SomeStruct { foo: &Foo { bools: &[false, true] }, bar: &Bar { bools: &[true, true] }, f: &id, - //~^ ERROR implementation of `Fn` is not general enough + //~^ ERROR mismatched types + //~| ERROR mismatched types + //~| ERROR mismatched types + //~| ERROR mismatched types + //~| ERROR implementation of `Fn` is not general enough + //~| ERROR implementation of `Fn` is not general enough + //~| ERROR implementation of `Fn` is not general enough //~| ERROR implementation of `Fn` is not general enough - //~| ERROR implementation of `FnOnce` is not general enough - //~| ERROR implementation of `FnOnce` is not general enough }; // very simple test for a 'static static with default lifetime diff --git a/tests/ui/rfcs/rfc-1623-static/rfc1623-2.stderr b/tests/ui/rfcs/rfc-1623-static/rfc1623-2.stderr index 5c98a9d4fb4ce..dd662db366a0e 100644 --- a/tests/ui/rfcs/rfc-1623-static/rfc1623-2.stderr +++ b/tests/ui/rfcs/rfc-1623-static/rfc1623-2.stderr @@ -34,5 +34,45 @@ LL | f: &id, = note: `fn(&Foo<'2>) -> &Foo<'2> {id::<&Foo<'2>>}` must implement `FnOnce<(&'a Foo<'1>,)>`, for any lifetime `'1`... = note: ...but it actually implements `FnOnce<(&Foo<'2>,)>`, for some specific lifetime `'2` -error: aborting due to 4 previous errors +error[E0308]: mismatched types + --> $DIR/rfc1623-2.rs:28:8 + | +LL | f: &id, + | ^^^ one type is more general than the other + | + = note: expected trait `for<'a, 'b> Fn(&'a Foo<'b>)` + found trait `Fn(&Foo<'_>)` + = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no` + +error[E0308]: mismatched types + --> $DIR/rfc1623-2.rs:28:8 + | +LL | f: &id, + | ^^^ one type is more general than the other + | + = note: expected trait `for<'a, 'b> Fn(&'a Foo<'b>)` + found trait `Fn(&Foo<'_>)` + = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no` + +error: implementation of `FnOnce` is not general enough + --> $DIR/rfc1623-2.rs:28:8 + | +LL | f: &id, + | ^^^ implementation of `FnOnce` is not general enough + | + = note: `fn(&'2 Foo<'_>) -> &'2 Foo<'_> {id::<&'2 Foo<'_>>}` must implement `FnOnce<(&'1 Foo<'b>,)>`, for any lifetime `'1`... + = note: ...but it actually implements `FnOnce<(&'2 Foo<'_>,)>`, for some specific lifetime `'2` + = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no` + +error: implementation of `FnOnce` is not general enough + --> $DIR/rfc1623-2.rs:28:8 + | +LL | f: &id, + | ^^^ implementation of `FnOnce` is not general enough + | + = note: `fn(&Foo<'2>) -> &Foo<'2> {id::<&Foo<'2>>}` must implement `FnOnce<(&'a Foo<'1>,)>`, for any lifetime `'1`... + = note: ...but it actually implements `FnOnce<(&Foo<'2>,)>`, for some specific lifetime `'2` + = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no` + +error: aborting due to 8 previous errors From 2efb7c8f341561bb433a0446ba68a74b9095ccf9 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 12 Mar 2024 18:15:58 +0100 Subject: [PATCH 2/4] compute required_consts before promotion, and add promoteds that may fail --- compiler/rustc_mir_transform/src/lib.rs | 17 ++++--- .../rustc_mir_transform/src/promote_consts.rs | 49 +++++++++++++------ tests/ui/rfcs/rfc-1623-static/rfc1623-2.rs | 4 -- .../ui/rfcs/rfc-1623-static/rfc1623-2.stderr | 42 +--------------- 4 files changed, 44 insertions(+), 68 deletions(-) diff --git a/compiler/rustc_mir_transform/src/lib.rs b/compiler/rustc_mir_transform/src/lib.rs index 1b920a3399138..fe68dc0b4eaca 100644 --- a/compiler/rustc_mir_transform/src/lib.rs +++ b/compiler/rustc_mir_transform/src/lib.rs @@ -344,6 +344,15 @@ fn mir_promoted( body.tainted_by_errors = Some(error_reported); } + // Collect `required_consts` *before* promotion, so if there are any consts being promoted + // we still add them to the list in the outer MIR body. + let mut required_consts = Vec::new(); + let mut required_consts_visitor = RequiredConstsVisitor::new(&mut required_consts); + for (bb, bb_data) in traversal::reverse_postorder(&body) { + required_consts_visitor.visit_basic_block_data(bb, bb_data); + } + body.required_consts = required_consts; + // What we need to run borrowck etc. let promote_pass = promote_consts::PromoteTemps::default(); pm::run_passes( @@ -353,14 +362,6 @@ fn mir_promoted( Some(MirPhase::Analysis(AnalysisPhase::Initial)), ); - // Promotion generates new consts; we run this after promotion to ensure they are accounted for. - let mut required_consts = Vec::new(); - let mut required_consts_visitor = RequiredConstsVisitor::new(&mut required_consts); - for (bb, bb_data) in traversal::reverse_postorder(&body) { - required_consts_visitor.visit_basic_block_data(bb, bb_data); - } - body.required_consts = required_consts; - let promoted = promote_pass.promoted_fragments.into_inner(); (tcx.alloc_steal_mir(body), tcx.alloc_steal_promoted(promoted)) } diff --git a/compiler/rustc_mir_transform/src/promote_consts.rs b/compiler/rustc_mir_transform/src/promote_consts.rs index 226641a5bc726..de4df7d2c2ef5 100644 --- a/compiler/rustc_mir_transform/src/promote_consts.rs +++ b/compiler/rustc_mir_transform/src/promote_consts.rs @@ -687,7 +687,6 @@ impl<'tcx> Validator<'_, 'tcx> { } } -// FIXME(eddyb) remove the differences for promotability in `static`, `const`, `const fn`. fn validate_candidates( ccx: &ConstCx<'_, '_>, temps: &mut IndexSlice, @@ -712,6 +711,10 @@ struct Promoter<'a, 'tcx> { /// If true, all nested temps are also kept in the /// source MIR, not moved to the promoted MIR. keep_original: bool, + + /// If true, add the new const (the promoted) to the required_consts of the parent MIR. + /// This is initially false and then set by the visitor when it encounters a `Call` terminator. + add_to_required: bool, } impl<'a, 'tcx> Promoter<'a, 'tcx> { @@ -814,6 +817,10 @@ impl<'a, 'tcx> Promoter<'a, 'tcx> { TerminatorKind::Call { mut func, mut args, call_source: desugar, fn_span, .. } => { + // This promoted involves a function call, so it may fail to evaluate. + // Let's make sure it is added to `required_consts` so that that failure cannot get lost. + self.add_to_required = true; + self.visit_operand(&mut func, loc); for arg in &mut args { self.visit_operand(&mut arg.node, loc); @@ -848,7 +855,7 @@ impl<'a, 'tcx> Promoter<'a, 'tcx> { fn promote_candidate(mut self, candidate: Candidate, next_promoted_id: usize) -> Body<'tcx> { let def = self.source.source.def_id(); - let mut rvalue = { + let (mut rvalue, promoted_op) = { let promoted = &mut self.promoted; let promoted_id = Promoted::new(next_promoted_id); let tcx = self.tcx; @@ -858,11 +865,7 @@ impl<'a, 'tcx> Promoter<'a, 'tcx> { let args = tcx.erase_regions(GenericArgs::identity_for_item(tcx, def)); let uneval = mir::UnevaluatedConst { def, args, promoted: Some(promoted_id) }; - Operand::Constant(Box::new(ConstOperand { - span, - user_ty: None, - const_: Const::Unevaluated(uneval, ty), - })) + ConstOperand { span, user_ty: None, const_: Const::Unevaluated(uneval, ty) } }; let blocks = self.source.basic_blocks.as_mut(); @@ -898,22 +901,26 @@ impl<'a, 'tcx> Promoter<'a, 'tcx> { let promoted_ref = local_decls.push(promoted_ref); assert_eq!(self.temps.push(TempState::Unpromotable), promoted_ref); + let promoted_operand = promoted_operand(ref_ty, span); let promoted_ref_statement = Statement { source_info: statement.source_info, kind: StatementKind::Assign(Box::new(( Place::from(promoted_ref), - Rvalue::Use(promoted_operand(ref_ty, span)), + Rvalue::Use(Operand::Constant(Box::new(promoted_operand))), ))), }; self.extra_statements.push((loc, promoted_ref_statement)); - Rvalue::Ref( - tcx.lifetimes.re_erased, - *borrow_kind, - Place { - local: mem::replace(&mut place.local, promoted_ref), - projection: List::empty(), - }, + ( + Rvalue::Ref( + tcx.lifetimes.re_erased, + *borrow_kind, + Place { + local: mem::replace(&mut place.local, promoted_ref), + projection: List::empty(), + }, + ), + promoted_operand, ) }; @@ -925,6 +932,12 @@ impl<'a, 'tcx> Promoter<'a, 'tcx> { let span = self.promoted.span; self.assign(RETURN_PLACE, rvalue, span); + + // Now that we did promotion, we know whether we'll want to add this to `required_consts`. + if self.add_to_required { + self.source.required_consts.push(promoted_op); + } + self.promoted } } @@ -984,6 +997,11 @@ fn promote_candidates<'tcx>( None, body.tainted_by_errors, ); + // We keep `required_consts` of the new MIR body empty. All consts mentioned here have + // already been added to the parent MIR's `required_consts` (that is computed before + // promotion), and no matter where this promoted const ends up, our parent MIR must be + // somewhere in the reachable dependency chain so we can rely on its required consts being + // evaluated. promoted.phase = MirPhase::Analysis(AnalysisPhase::Initial); let promoter = Promoter { @@ -993,6 +1011,7 @@ fn promote_candidates<'tcx>( temps: &mut temps, extra_statements: &mut extra_statements, keep_original: false, + add_to_required: false, }; let mut promoted = promoter.promote_candidate(candidate, promotions.len()); diff --git a/tests/ui/rfcs/rfc-1623-static/rfc1623-2.rs b/tests/ui/rfcs/rfc-1623-static/rfc1623-2.rs index 34739b22b96e9..f50b72f927942 100644 --- a/tests/ui/rfcs/rfc-1623-static/rfc1623-2.rs +++ b/tests/ui/rfcs/rfc-1623-static/rfc1623-2.rs @@ -28,10 +28,6 @@ static SOME_STRUCT: &SomeStruct = &SomeStruct { f: &id, //~^ ERROR mismatched types //~| ERROR mismatched types - //~| ERROR mismatched types - //~| ERROR mismatched types - //~| ERROR implementation of `Fn` is not general enough - //~| ERROR implementation of `Fn` is not general enough //~| ERROR implementation of `Fn` is not general enough //~| ERROR implementation of `Fn` is not general enough }; diff --git a/tests/ui/rfcs/rfc-1623-static/rfc1623-2.stderr b/tests/ui/rfcs/rfc-1623-static/rfc1623-2.stderr index dd662db366a0e..5c98a9d4fb4ce 100644 --- a/tests/ui/rfcs/rfc-1623-static/rfc1623-2.stderr +++ b/tests/ui/rfcs/rfc-1623-static/rfc1623-2.stderr @@ -34,45 +34,5 @@ LL | f: &id, = note: `fn(&Foo<'2>) -> &Foo<'2> {id::<&Foo<'2>>}` must implement `FnOnce<(&'a Foo<'1>,)>`, for any lifetime `'1`... = note: ...but it actually implements `FnOnce<(&Foo<'2>,)>`, for some specific lifetime `'2` -error[E0308]: mismatched types - --> $DIR/rfc1623-2.rs:28:8 - | -LL | f: &id, - | ^^^ one type is more general than the other - | - = note: expected trait `for<'a, 'b> Fn(&'a Foo<'b>)` - found trait `Fn(&Foo<'_>)` - = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no` - -error[E0308]: mismatched types - --> $DIR/rfc1623-2.rs:28:8 - | -LL | f: &id, - | ^^^ one type is more general than the other - | - = note: expected trait `for<'a, 'b> Fn(&'a Foo<'b>)` - found trait `Fn(&Foo<'_>)` - = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no` - -error: implementation of `FnOnce` is not general enough - --> $DIR/rfc1623-2.rs:28:8 - | -LL | f: &id, - | ^^^ implementation of `FnOnce` is not general enough - | - = note: `fn(&'2 Foo<'_>) -> &'2 Foo<'_> {id::<&'2 Foo<'_>>}` must implement `FnOnce<(&'1 Foo<'b>,)>`, for any lifetime `'1`... - = note: ...but it actually implements `FnOnce<(&'2 Foo<'_>,)>`, for some specific lifetime `'2` - = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no` - -error: implementation of `FnOnce` is not general enough - --> $DIR/rfc1623-2.rs:28:8 - | -LL | f: &id, - | ^^^ implementation of `FnOnce` is not general enough - | - = note: `fn(&Foo<'2>) -> &Foo<'2> {id::<&Foo<'2>>}` must implement `FnOnce<(&'a Foo<'1>,)>`, for any lifetime `'1`... - = note: ...but it actually implements `FnOnce<(&Foo<'2>,)>`, for some specific lifetime `'2` - = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no` - -error: aborting due to 8 previous errors +error: aborting due to 4 previous errors From 679aca2d85a745220ab00e1e18383245fadf90e8 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 14 Mar 2024 07:28:25 +0100 Subject: [PATCH 3/4] interpret: sanity-check that required_consts captures all consts that can fail --- .../src/const_eval/machine.rs | 14 +++++++++++ .../src/interpret/eval_context.rs | 24 ++++++++++--------- .../rustc_const_eval/src/interpret/machine.rs | 5 ++-- .../src/dataflow_const_prop.rs | 6 +++++ compiler/rustc_mir_transform/src/inline.rs | 19 ++++----------- .../src/required_consts.rs | 22 +++++++++++------ src/tools/miri/src/machine.rs | 6 +++++ 7 files changed, 62 insertions(+), 34 deletions(-) diff --git a/compiler/rustc_const_eval/src/const_eval/machine.rs b/compiler/rustc_const_eval/src/const_eval/machine.rs index dd835279df331..2d06460f35302 100644 --- a/compiler/rustc_const_eval/src/const_eval/machine.rs +++ b/compiler/rustc_const_eval/src/const_eval/machine.rs @@ -375,6 +375,20 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir, const PANIC_ON_ALLOC_FAIL: bool = false; // will be raised as a proper error + #[inline] + fn all_required_consts_are_checked(ecx: &InterpCx<'mir, 'tcx, Self>) -> bool { + // Generally we expect required_consts to be properly filled, except for promoteds where + // storing these consts shows up negatively in benchmarks. A promoted can only be relevant + // if its parent MIR is relevant, and the consts in the promoted will be in the parent's + // `required_consts`, so we are still sure to catch any const-eval bugs, just a bit less + // directly. + if ecx.frame_idx() == 0 && ecx.frame().body.source.promoted.is_some() { + false + } else { + true + } + } + #[inline(always)] fn enforce_alignment(ecx: &InterpCx<'mir, 'tcx, Self>) -> bool { matches!(ecx.machine.check_alignment, CheckAlignment::Error) diff --git a/compiler/rustc_const_eval/src/interpret/eval_context.rs b/compiler/rustc_const_eval/src/interpret/eval_context.rs index 3283bcc4c453c..7b14bb714584d 100644 --- a/compiler/rustc_const_eval/src/interpret/eval_context.rs +++ b/compiler/rustc_const_eval/src/interpret/eval_context.rs @@ -822,15 +822,13 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { self.stack_mut().push(frame); // Make sure all the constants required by this frame evaluate successfully (post-monomorphization check). - if M::POST_MONO_CHECKS { - for &const_ in &body.required_consts { - let c = self - .instantiate_from_current_frame_and_normalize_erasing_regions(const_.const_)?; - c.eval(*self.tcx, self.param_env, const_.span).map_err(|err| { - err.emit_note(*self.tcx); - err - })?; - } + for &const_ in &body.required_consts { + let c = + self.instantiate_from_current_frame_and_normalize_erasing_regions(const_.const_)?; + c.eval(*self.tcx, self.param_env, const_.span).map_err(|err| { + err.emit_note(*self.tcx); + err + })?; } // done @@ -1179,8 +1177,12 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { ) -> InterpResult<'tcx, OpTy<'tcx, M::Provenance>> { M::eval_mir_constant(self, *val, span, layout, |ecx, val, span, layout| { let const_val = val.eval(*ecx.tcx, ecx.param_env, span).map_err(|err| { - // FIXME: somehow this is reachable even when POST_MONO_CHECKS is on. - // Are we not always populating `required_consts`? + if M::all_required_consts_are_checked(self) + && !matches!(err, ErrorHandled::TooGeneric(..)) + { + // Looks like the const is not captued by `required_consts`, that's bad. + bug!("interpret const eval failure of {val:?} which is not in required_consts"); + } err.emit_note(*ecx.tcx); err })?; diff --git a/compiler/rustc_const_eval/src/interpret/machine.rs b/compiler/rustc_const_eval/src/interpret/machine.rs index 827d8fd9417ce..a23190dd2d89c 100644 --- a/compiler/rustc_const_eval/src/interpret/machine.rs +++ b/compiler/rustc_const_eval/src/interpret/machine.rs @@ -140,8 +140,9 @@ pub trait Machine<'mir, 'tcx: 'mir>: Sized { /// Should the machine panic on allocation failures? const PANIC_ON_ALLOC_FAIL: bool; - /// Should post-monomorphization checks be run when a stack frame is pushed? - const POST_MONO_CHECKS: bool = true; + /// Determines whether `eval_mir_constant` can never fail because all required consts have + /// already been checked before. + fn all_required_consts_are_checked(ecx: &InterpCx<'mir, 'tcx, Self>) -> bool; /// Whether memory accesses should be alignment-checked. fn enforce_alignment(ecx: &InterpCx<'mir, 'tcx, Self>) -> bool; diff --git a/compiler/rustc_mir_transform/src/dataflow_const_prop.rs b/compiler/rustc_mir_transform/src/dataflow_const_prop.rs index 3389305e7eee7..975adc01d07f4 100644 --- a/compiler/rustc_mir_transform/src/dataflow_const_prop.rs +++ b/compiler/rustc_mir_transform/src/dataflow_const_prop.rs @@ -902,6 +902,12 @@ impl<'mir, 'tcx: 'mir> rustc_const_eval::interpret::Machine<'mir, 'tcx> for Dumm type MemoryKind = !; const PANIC_ON_ALLOC_FAIL: bool = true; + #[inline(always)] + fn all_required_consts_are_checked(_ecx: &InterpCx<'mir, 'tcx, Self>) -> bool { + // We want to just eval random consts in the program, so `eval_mir_const` can fail. + false + } + #[inline(always)] fn enforce_alignment(_ecx: &InterpCx<'mir, 'tcx, Self>) -> bool { false // no reason to enforce alignment diff --git a/compiler/rustc_mir_transform/src/inline.rs b/compiler/rustc_mir_transform/src/inline.rs index 4ec76eec3a930..767879706bbf4 100644 --- a/compiler/rustc_mir_transform/src/inline.rs +++ b/compiler/rustc_mir_transform/src/inline.rs @@ -706,18 +706,8 @@ impl<'tcx> Inliner<'tcx> { kind: TerminatorKind::Goto { target: integrator.map_block(START_BLOCK) }, }); - // Copy only unevaluated constants from the callee_body into the caller_body. - // Although we are only pushing `ConstKind::Unevaluated` consts to - // `required_consts`, here we may not only have `ConstKind::Unevaluated` - // because we are calling `instantiate_and_normalize_erasing_regions`. - caller_body.required_consts.extend(callee_body.required_consts.iter().copied().filter( - |&ct| match ct.const_ { - Const::Ty(_) => { - bug!("should never encounter ty::UnevaluatedConst in `required_consts`") - } - Const::Val(..) | Const::Unevaluated(..) => true, - }, - )); + // Copy required constants from the callee_body into the caller_body. + caller_body.required_consts.extend(callee_body.required_consts); // Now that we incorporated the callee's `required_consts`, we can remove the callee from // `mentioned_items` -- but we have to take their `mentioned_items` in return. This does // some extra work here to save the monomorphization collector work later. It helps a lot, @@ -733,8 +723,9 @@ impl<'tcx> Inliner<'tcx> { caller_body.mentioned_items.remove(idx); caller_body.mentioned_items.extend(callee_body.mentioned_items); } else { - // If we can't find the callee, there's no point in adding its items. - // Probably it already got removed by being inlined elsewhere in the same function. + // If we can't find the callee, there's no point in adding its items. Probably it + // already got removed by being inlined elsewhere in the same function, so we already + // took its items. } } diff --git a/compiler/rustc_mir_transform/src/required_consts.rs b/compiler/rustc_mir_transform/src/required_consts.rs index abde6a47e83aa..f0c63a6648c6a 100644 --- a/compiler/rustc_mir_transform/src/required_consts.rs +++ b/compiler/rustc_mir_transform/src/required_consts.rs @@ -1,6 +1,6 @@ use rustc_middle::mir::visit::Visitor; use rustc_middle::mir::{Const, ConstOperand, Location}; -use rustc_middle::ty::ConstKind; +use rustc_middle::ty; pub struct RequiredConstsVisitor<'a, 'tcx> { required_consts: &'a mut Vec>, @@ -14,14 +14,22 @@ impl<'a, 'tcx> RequiredConstsVisitor<'a, 'tcx> { impl<'tcx> Visitor<'tcx> for RequiredConstsVisitor<'_, 'tcx> { fn visit_constant(&mut self, constant: &ConstOperand<'tcx>, _: Location) { - let const_ = constant.const_; - match const_ { + // Only unevaluated consts have to be added to `required_consts` as only those can possibly + // still have latent const-eval errors. + let is_required = match constant.const_ { Const::Ty(c) => match c.kind() { - ConstKind::Param(_) | ConstKind::Error(_) | ConstKind::Value(_) => {} - _ => bug!("only ConstKind::Param/Value should be encountered here, got {:#?}", c), + ty::ConstKind::Value(_) => false, // already a value, cannot error + ty::ConstKind::Param(_) | ty::ConstKind::Error(_) => true, // these are errors or could be replaced by errors + _ => bug!( + "only ConstKind::Param/Value/Error should be encountered here, got {:#?}", + c + ), }, - Const::Unevaluated(..) => self.required_consts.push(*constant), - Const::Val(..) => {} + Const::Unevaluated(..) => true, + Const::Val(..) => false, // already a value, cannot error + }; + if is_required { + self.required_consts.push(*constant); } } } diff --git a/src/tools/miri/src/machine.rs b/src/tools/miri/src/machine.rs index 3a4ab32e4ab8a..671bcd73bc9b2 100644 --- a/src/tools/miri/src/machine.rs +++ b/src/tools/miri/src/machine.rs @@ -872,6 +872,12 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { const PANIC_ON_ALLOC_FAIL: bool = false; + #[inline(always)] + fn all_required_consts_are_checked(_ecx: &InterpCx<'mir, 'tcx, Self>) -> bool { + // We want to catch any cases of missing required_consts + true + } + #[inline(always)] fn enforce_alignment(ecx: &MiriInterpCx<'mir, 'tcx>) -> bool { ecx.machine.check_alignment != AlignmentCheck::None From 891783755c3edc1960c826c2d0fd77370a44f694 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 21 Mar 2024 08:28:00 +0100 Subject: [PATCH 4/4] properly fill a promoted's required_consts --- .../src/const_eval/machine.rs | 13 +---- compiler/rustc_middle/src/mir/consts.rs | 18 +++++++ compiler/rustc_mir_transform/src/inline.rs | 8 ++- .../rustc_mir_transform/src/promote_consts.rs | 14 +++-- .../src/required_consts.rs | 19 +------ .../collect-in-promoted-const.noopt.stderr | 37 +++++++++++++ .../collect-in-promoted-const.opt.stderr | 53 +++++++++++++++++++ .../collect-in-promoted-const.rs | 26 +++++++++ .../required-consts/interpret-in-promoted.rs | 2 +- tests/ui/rfcs/rfc-1623-static/rfc1623-2.rs | 4 +- 10 files changed, 156 insertions(+), 38 deletions(-) create mode 100644 tests/ui/consts/required-consts/collect-in-promoted-const.noopt.stderr create mode 100644 tests/ui/consts/required-consts/collect-in-promoted-const.opt.stderr create mode 100644 tests/ui/consts/required-consts/collect-in-promoted-const.rs diff --git a/compiler/rustc_const_eval/src/const_eval/machine.rs b/compiler/rustc_const_eval/src/const_eval/machine.rs index 2d06460f35302..6ea2e9cecb618 100644 --- a/compiler/rustc_const_eval/src/const_eval/machine.rs +++ b/compiler/rustc_const_eval/src/const_eval/machine.rs @@ -376,17 +376,8 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir, const PANIC_ON_ALLOC_FAIL: bool = false; // will be raised as a proper error #[inline] - fn all_required_consts_are_checked(ecx: &InterpCx<'mir, 'tcx, Self>) -> bool { - // Generally we expect required_consts to be properly filled, except for promoteds where - // storing these consts shows up negatively in benchmarks. A promoted can only be relevant - // if its parent MIR is relevant, and the consts in the promoted will be in the parent's - // `required_consts`, so we are still sure to catch any const-eval bugs, just a bit less - // directly. - if ecx.frame_idx() == 0 && ecx.frame().body.source.promoted.is_some() { - false - } else { - true - } + fn all_required_consts_are_checked(_ecx: &InterpCx<'mir, 'tcx, Self>) -> bool { + true } #[inline(always)] diff --git a/compiler/rustc_middle/src/mir/consts.rs b/compiler/rustc_middle/src/mir/consts.rs index 214297b98695f..56e3b2f734db3 100644 --- a/compiler/rustc_middle/src/mir/consts.rs +++ b/compiler/rustc_middle/src/mir/consts.rs @@ -238,6 +238,24 @@ impl<'tcx> Const<'tcx> { } } + /// Determines whether we need to add this const to `required_consts`. This is the case if and + /// only if evaluating it may error. + #[inline] + pub fn is_required_const(&self) -> bool { + match self { + Const::Ty(c) => match c.kind() { + ty::ConstKind::Value(_) => false, // already a value, cannot error + ty::ConstKind::Param(_) | ty::ConstKind::Error(_) => true, // these are errors or could be replaced by errors + _ => bug!( + "only ConstKind::Param/Value/Error should be encountered here, got {:#?}", + c + ), + }, + Const::Unevaluated(..) => true, + Const::Val(..) => false, // already a value, cannot error + } + } + #[inline] pub fn try_to_scalar(self) -> Option { match self { diff --git a/compiler/rustc_mir_transform/src/inline.rs b/compiler/rustc_mir_transform/src/inline.rs index 767879706bbf4..8e5fd750d831d 100644 --- a/compiler/rustc_mir_transform/src/inline.rs +++ b/compiler/rustc_mir_transform/src/inline.rs @@ -706,8 +706,12 @@ impl<'tcx> Inliner<'tcx> { kind: TerminatorKind::Goto { target: integrator.map_block(START_BLOCK) }, }); - // Copy required constants from the callee_body into the caller_body. - caller_body.required_consts.extend(callee_body.required_consts); + // Copy required constants from the callee_body into the caller_body. Although we are only + // pushing unevaluated consts to `required_consts`, here we they may have been evaluated + // because we are calling `instantiate_and_normalize_erasing_regions` -- so we filter again. + caller_body.required_consts.extend( + callee_body.required_consts.into_iter().filter(|ct| ct.const_.is_required_const()), + ); // Now that we incorporated the callee's `required_consts`, we can remove the callee from // `mentioned_items` -- but we have to take their `mentioned_items` in return. This does // some extra work here to save the monomorphization collector work later. It helps a lot, diff --git a/compiler/rustc_mir_transform/src/promote_consts.rs b/compiler/rustc_mir_transform/src/promote_consts.rs index de4df7d2c2ef5..aab8d112a7a2a 100644 --- a/compiler/rustc_mir_transform/src/promote_consts.rs +++ b/compiler/rustc_mir_transform/src/promote_consts.rs @@ -953,6 +953,14 @@ impl<'a, 'tcx> MutVisitor<'tcx> for Promoter<'a, 'tcx> { *local = self.promote_temp(*local); } } + + fn visit_constant(&mut self, constant: &mut ConstOperand<'tcx>, _location: Location) { + if constant.const_.is_required_const() { + self.promoted.required_consts.push(*constant); + } + + // Skipping `super_constant` as the visitor is otherwise only looking for locals. + } } fn promote_candidates<'tcx>( @@ -997,11 +1005,6 @@ fn promote_candidates<'tcx>( None, body.tainted_by_errors, ); - // We keep `required_consts` of the new MIR body empty. All consts mentioned here have - // already been added to the parent MIR's `required_consts` (that is computed before - // promotion), and no matter where this promoted const ends up, our parent MIR must be - // somewhere in the reachable dependency chain so we can rely on its required consts being - // evaluated. promoted.phase = MirPhase::Analysis(AnalysisPhase::Initial); let promoter = Promoter { @@ -1014,6 +1017,7 @@ fn promote_candidates<'tcx>( add_to_required: false, }; + // `required_consts` of the promoted itself gets filled while building the MIR body. let mut promoted = promoter.promote_candidate(candidate, promotions.len()); promoted.source.promoted = Some(promotions.next_index()); promotions.push(promoted); diff --git a/compiler/rustc_mir_transform/src/required_consts.rs b/compiler/rustc_mir_transform/src/required_consts.rs index f0c63a6648c6a..71ac929d35ebd 100644 --- a/compiler/rustc_mir_transform/src/required_consts.rs +++ b/compiler/rustc_mir_transform/src/required_consts.rs @@ -1,6 +1,5 @@ use rustc_middle::mir::visit::Visitor; -use rustc_middle::mir::{Const, ConstOperand, Location}; -use rustc_middle::ty; +use rustc_middle::mir::{ConstOperand, Location}; pub struct RequiredConstsVisitor<'a, 'tcx> { required_consts: &'a mut Vec>, @@ -14,21 +13,7 @@ impl<'a, 'tcx> RequiredConstsVisitor<'a, 'tcx> { impl<'tcx> Visitor<'tcx> for RequiredConstsVisitor<'_, 'tcx> { fn visit_constant(&mut self, constant: &ConstOperand<'tcx>, _: Location) { - // Only unevaluated consts have to be added to `required_consts` as only those can possibly - // still have latent const-eval errors. - let is_required = match constant.const_ { - Const::Ty(c) => match c.kind() { - ty::ConstKind::Value(_) => false, // already a value, cannot error - ty::ConstKind::Param(_) | ty::ConstKind::Error(_) => true, // these are errors or could be replaced by errors - _ => bug!( - "only ConstKind::Param/Value/Error should be encountered here, got {:#?}", - c - ), - }, - Const::Unevaluated(..) => true, - Const::Val(..) => false, // already a value, cannot error - }; - if is_required { + if constant.const_.is_required_const() { self.required_consts.push(*constant); } } diff --git a/tests/ui/consts/required-consts/collect-in-promoted-const.noopt.stderr b/tests/ui/consts/required-consts/collect-in-promoted-const.noopt.stderr new file mode 100644 index 0000000000000..784ef73e9bb7c --- /dev/null +++ b/tests/ui/consts/required-consts/collect-in-promoted-const.noopt.stderr @@ -0,0 +1,37 @@ +error[E0080]: evaluation of `Fail::::C` failed + --> $DIR/collect-in-promoted-const.rs:9:19 + | +LL | const C: () = panic!(); + | ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/collect-in-promoted-const.rs:9:19 + | + = note: this error originates in the macro `$crate::panic::panic_2015` which comes from the expansion of the macro `panic` (in Nightly builds, run with -Z macro-backtrace for more info) + +note: erroneous constant encountered + --> $DIR/collect-in-promoted-const.rs:20:21 + | +LL | let _val = &Fail::::C; + | ^^^^^^^^^^^^ + +note: erroneous constant encountered + --> $DIR/collect-in-promoted-const.rs:20:20 + | +LL | let _val = &Fail::::C; + | ^^^^^^^^^^^^^ + +note: erroneous constant encountered + --> $DIR/collect-in-promoted-const.rs:20:21 + | +LL | let _val = &Fail::::C; + | ^^^^^^^^^^^^ + | + = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no` + +note: the above error was encountered while instantiating `fn f::` + --> $DIR/collect-in-promoted-const.rs:25:5 + | +LL | f::(); + | ^^^^^^^^^^ + +error: aborting due to 1 previous error + +For more information about this error, try `rustc --explain E0080`. diff --git a/tests/ui/consts/required-consts/collect-in-promoted-const.opt.stderr b/tests/ui/consts/required-consts/collect-in-promoted-const.opt.stderr new file mode 100644 index 0000000000000..4b6f84de83afb --- /dev/null +++ b/tests/ui/consts/required-consts/collect-in-promoted-const.opt.stderr @@ -0,0 +1,53 @@ +error[E0080]: evaluation of `Fail::::C` failed + --> $DIR/collect-in-promoted-const.rs:9:19 + | +LL | const C: () = panic!(); + | ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/collect-in-promoted-const.rs:9:19 + | + = note: this error originates in the macro `$crate::panic::panic_2015` which comes from the expansion of the macro `panic` (in Nightly builds, run with -Z macro-backtrace for more info) + +note: erroneous constant encountered + --> $DIR/collect-in-promoted-const.rs:20:21 + | +LL | let _val = &Fail::::C; + | ^^^^^^^^^^^^ + +error[E0080]: evaluation of `Fail::::C` failed + --> $DIR/collect-in-promoted-const.rs:9:19 + | +LL | const C: () = panic!(); + | ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/collect-in-promoted-const.rs:9:19 + | + = note: this error originates in the macro `$crate::panic::panic_2015` which comes from the expansion of the macro `panic` (in Nightly builds, run with -Z macro-backtrace for more info) + +note: erroneous constant encountered + --> $DIR/collect-in-promoted-const.rs:20:21 + | +LL | let _val = &Fail::::C; + | ^^^^^^^^^^^^ + | + = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no` + +note: erroneous constant encountered + --> $DIR/collect-in-promoted-const.rs:20:20 + | +LL | let _val = &Fail::::C; + | ^^^^^^^^^^^^^ + +note: erroneous constant encountered + --> $DIR/collect-in-promoted-const.rs:20:21 + | +LL | let _val = &Fail::::C; + | ^^^^^^^^^^^^ + | + = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no` + +note: the above error was encountered while instantiating `fn f::` + --> $DIR/collect-in-promoted-const.rs:25:5 + | +LL | f::(); + | ^^^^^^^^^^ + +error: aborting due to 2 previous errors + +For more information about this error, try `rustc --explain E0080`. diff --git a/tests/ui/consts/required-consts/collect-in-promoted-const.rs b/tests/ui/consts/required-consts/collect-in-promoted-const.rs new file mode 100644 index 0000000000000..4a3ce92e8f90d --- /dev/null +++ b/tests/ui/consts/required-consts/collect-in-promoted-const.rs @@ -0,0 +1,26 @@ +//@revisions: noopt opt +//@ build-fail +//@[noopt] compile-flags: -Copt-level=0 +//@[opt] compile-flags: -O +//! Make sure we error on erroneous consts even if they get promoted. + +struct Fail(T); +impl Fail { + const C: () = panic!(); //~ERROR evaluation of `Fail::::C` failed + //[opt]~^ ERROR evaluation of `Fail::::C` failed + // (Not sure why optimizations lead to this being emitted twice, but as long as compilation + // fails either way it's fine.) +} + +#[inline(never)] +fn f() { + if false { + // If promotion moved `C` from our required_consts to its own, without adding itself to + // our required_consts, then we'd miss the const-eval failure here. + let _val = &Fail::::C; + } +} + +fn main() { + f::(); +} diff --git a/tests/ui/consts/required-consts/interpret-in-promoted.rs b/tests/ui/consts/required-consts/interpret-in-promoted.rs index 187494180ad1d..48caece6ff56f 100644 --- a/tests/ui/consts/required-consts/interpret-in-promoted.rs +++ b/tests/ui/consts/required-consts/interpret-in-promoted.rs @@ -1,7 +1,7 @@ //@revisions: noopt opt //@[noopt] compile-flags: -Copt-level=0 //@[opt] compile-flags: -O -//! Make sure we error on erroneous consts even if they are unused. +//! Make sure we evaluate const fn calls even if they get promoted and their result ignored. const unsafe fn ub() { std::hint::unreachable_unchecked(); diff --git a/tests/ui/rfcs/rfc-1623-static/rfc1623-2.rs b/tests/ui/rfcs/rfc-1623-static/rfc1623-2.rs index f50b72f927942..97fc1276f6175 100644 --- a/tests/ui/rfcs/rfc-1623-static/rfc1623-2.rs +++ b/tests/ui/rfcs/rfc-1623-static/rfc1623-2.rs @@ -26,8 +26,8 @@ static SOME_STRUCT: &SomeStruct = &SomeStruct { foo: &Foo { bools: &[false, true] }, bar: &Bar { bools: &[true, true] }, f: &id, - //~^ ERROR mismatched types - //~| ERROR mismatched types + //~^ ERROR implementation of `FnOnce` is not general enough + //~| ERROR implementation of `FnOnce` is not general enough //~| ERROR implementation of `Fn` is not general enough //~| ERROR implementation of `Fn` is not general enough };