From 8a1214ade30c01fd809aa6e352774b3f73d2740e Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 14 Mar 2024 07:28:25 +0100 Subject: [PATCH] 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 | 12 +--------- .../src/required_consts.rs | 23 ++++++++++++------ src/tools/miri/src/machine.rs | 6 +++++ 7 files changed, 59 insertions(+), 31 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 a484fbd892c6d..c9036343f2710 100644 --- a/compiler/rustc_const_eval/src/interpret/eval_context.rs +++ b/compiler/rustc_const_eval/src/interpret/eval_context.rs @@ -792,15 +792,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, Some(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, Some(const_.span)).map_err(|err| { + err.emit_note(*self.tcx); + err + })?; } // done @@ -1149,8 +1147,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 305f7ade101ec..88ff5794478ea 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 c3e932fe18726..45ce964b84286 100644 --- a/compiler/rustc_mir_transform/src/dataflow_const_prop.rs +++ b/compiler/rustc_mir_transform/src/dataflow_const_prop.rs @@ -900,6 +900,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 f6a0945c22297..e9706b2a36a19 100644 --- a/compiler/rustc_mir_transform/src/inline.rs +++ b/compiler/rustc_mir_transform/src/inline.rs @@ -706,17 +706,7 @@ impl<'tcx> Inliner<'tcx> { }); // 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, - }, - )); + caller_body.required_consts.extend(callee_body.required_consts); } fn make_call_args( diff --git a/compiler/rustc_mir_transform/src/required_consts.rs b/compiler/rustc_mir_transform/src/required_consts.rs index abde6a47e83aa..6e106bb56c0e9 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,23 @@ 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 unevalated 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::Error(_) => true, // already an error, so clearly this can error :) + ty::ConstKind::Value(_) => false, // already a value, cannot error + ty::ConstKind::Param(_) => false, // if this errors it should fail further up the call chain + _ => 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 f0e3c43a5c5f3..51bcb3f4da221 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