From acb7c21507f4c6f7247e702287ab4caf64b31235 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 29 Sep 2023 22:38:52 +0200 Subject: [PATCH 1/2] dont call mir.post_mono_checks in codegen --- compiler/rustc_codegen_cranelift/src/base.rs | 11 ------- compiler/rustc_codegen_ssa/src/mir/mod.rs | 11 ------- .../src/interpret/eval_context.rs | 29 +++++++++++------- .../src/interpret/intrinsics.rs | 2 +- .../rustc_const_eval/src/interpret/memory.rs | 2 +- .../rustc_middle/src/mir/interpret/error.rs | 15 ---------- compiler/rustc_middle/src/mir/mod.rs | 30 +------------------ src/tools/miri/src/concurrency/thread.rs | 3 +- 8 files changed, 22 insertions(+), 81 deletions(-) diff --git a/compiler/rustc_codegen_cranelift/src/base.rs b/compiler/rustc_codegen_cranelift/src/base.rs index 6d55fdc307401..24c7c9135cb3b 100644 --- a/compiler/rustc_codegen_cranelift/src/base.rs +++ b/compiler/rustc_codegen_cranelift/src/base.rs @@ -250,17 +250,6 @@ pub(crate) fn verify_func( } fn codegen_fn_body(fx: &mut FunctionCx<'_, '_, '_>, start_block: Block) { - if let Err(err) = - fx.mir.post_mono_checks(fx.tcx, ty::ParamEnv::reveal_all(), |c| Ok(fx.monomorphize(c))) - { - err.emit_err(fx.tcx); - fx.bcx.append_block_params_for_function_params(fx.block_map[START_BLOCK]); - fx.bcx.switch_to_block(fx.block_map[START_BLOCK]); - // compilation should have been aborted - fx.bcx.ins().trap(TrapCode::UnreachableCodeReached); - return; - } - let arg_uninhabited = fx .mir .args_iter() diff --git a/compiler/rustc_codegen_ssa/src/mir/mod.rs b/compiler/rustc_codegen_ssa/src/mir/mod.rs index a61018f987031..06cdc47610df8 100644 --- a/compiler/rustc_codegen_ssa/src/mir/mod.rs +++ b/compiler/rustc_codegen_ssa/src/mir/mod.rs @@ -211,17 +211,6 @@ pub fn codegen_mir<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( fx.per_local_var_debug_info = fx.compute_per_local_var_debug_info(&mut start_bx); - // Rust post-monomorphization checks; we later rely on them. - if let Err(err) = - mir.post_mono_checks(cx.tcx(), ty::ParamEnv::reveal_all(), |c| Ok(fx.monomorphize(c))) - { - err.emit_err(cx.tcx()); - // This IR shouldn't ever be emitted, but let's try to guard against any of this code - // ever running. - start_bx.abort(); - return; - } - let memory_locals = analyze::non_ssa_locals(&fx); // Allocate variable and temp allocas diff --git a/compiler/rustc_const_eval/src/interpret/eval_context.rs b/compiler/rustc_const_eval/src/interpret/eval_context.rs index 94a5cc67d31c8..0ea56eb2bf8ab 100644 --- a/compiler/rustc_const_eval/src/interpret/eval_context.rs +++ b/compiler/rustc_const_eval/src/interpret/eval_context.rs @@ -750,12 +750,14 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // Make sure all the constants required by this frame evaluate successfully (post-monomorphization check). if M::POST_MONO_CHECKS { - // `ctfe_query` does some error message decoration that we want to be in effect here. - self.ctfe_query(None, |tcx| { - body.post_mono_checks(*tcx, self.param_env, |c| { - self.subst_from_current_frame_and_normalize_erasing_regions(c) - }) - })?; + for &const_ in &body.required_consts { + let c = + self.subst_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 @@ -1054,14 +1056,14 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { Ok(()) } - /// Call a query that can return `ErrorHandled`. If `span` is `Some`, point to that span when an error occurs. + /// Call a query that can return `ErrorHandled`. Should be used for statics and other globals. + /// (`mir::Const`/`ty::Const` have `eval` methods that can be used directly instead.) pub fn ctfe_query( &self, - span: Option, query: impl FnOnce(TyCtxtAt<'tcx>) -> Result, ) -> Result { // Use a precise span for better cycle errors. - query(self.tcx.at(span.unwrap_or_else(|| self.cur_span()))).map_err(|err| { + query(self.tcx.at(self.cur_span())).map_err(|err| { err.emit_note(*self.tcx); err }) @@ -1082,7 +1084,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } else { self.param_env }; - let val = self.ctfe_query(None, |tcx| tcx.eval_to_allocation_raw(param_env.and(gid)))?; + let val = self.ctfe_query(|tcx| tcx.eval_to_allocation_raw(param_env.and(gid)))?; self.raw_const_to_mplace(val) } @@ -1092,7 +1094,12 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { span: Option, layout: Option>, ) -> InterpResult<'tcx, OpTy<'tcx, M::Provenance>> { - let const_val = self.ctfe_query(span, |tcx| val.eval(*tcx, self.param_env, span))?; + let const_val = val.eval(*self.tcx, self.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`? + err.emit_note(*self.tcx); + err + })?; self.const_val_to_op(const_val, val.ty(), layout) } diff --git a/compiler/rustc_const_eval/src/interpret/intrinsics.rs b/compiler/rustc_const_eval/src/interpret/intrinsics.rs index 2c0ba9b2673f8..2c6a4de456d06 100644 --- a/compiler/rustc_const_eval/src/interpret/intrinsics.rs +++ b/compiler/rustc_const_eval/src/interpret/intrinsics.rs @@ -164,7 +164,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { sym::type_name => Ty::new_static_str(self.tcx.tcx), _ => bug!(), }; - let val = self.ctfe_query(None, |tcx| { + let val = self.ctfe_query(|tcx| { tcx.const_eval_global_id(self.param_env, gid, Some(tcx.span)) })?; let val = self.const_val_to_op(val, ty, Some(dest.layout))?; diff --git a/compiler/rustc_const_eval/src/interpret/memory.rs b/compiler/rustc_const_eval/src/interpret/memory.rs index 436c4d521a51c..ce666e6af3bdf 100644 --- a/compiler/rustc_const_eval/src/interpret/memory.rs +++ b/compiler/rustc_const_eval/src/interpret/memory.rs @@ -536,7 +536,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } // We don't give a span -- statics don't need that, they cannot be generic or associated. - let val = self.ctfe_query(None, |tcx| tcx.eval_static_initializer(def_id))?; + let val = self.ctfe_query(|tcx| tcx.eval_static_initializer(def_id))?; (val, Some(def_id)) } }; diff --git a/compiler/rustc_middle/src/mir/interpret/error.rs b/compiler/rustc_middle/src/mir/interpret/error.rs index bc464aca5f3c0..9de40b3f97459 100644 --- a/compiler/rustc_middle/src/mir/interpret/error.rs +++ b/compiler/rustc_middle/src/mir/interpret/error.rs @@ -43,21 +43,6 @@ impl ErrorHandled { } } - pub fn emit_err(&self, tcx: TyCtxt<'_>) -> ErrorGuaranteed { - match self { - &ErrorHandled::Reported(err, span) => { - if !err.is_tainted_by_errors && !span.is_dummy() { - tcx.sess.emit_err(error::ErroneousConstant { span }); - } - err.error - } - &ErrorHandled::TooGeneric(span) => tcx.sess.delay_span_bug( - span, - "encountered TooGeneric error when monomorphic data was expected", - ), - } - } - pub fn emit_note(&self, tcx: TyCtxt<'_>) { match self { &ErrorHandled::Reported(err, span) => { diff --git a/compiler/rustc_middle/src/mir/mod.rs b/compiler/rustc_middle/src/mir/mod.rs index 0bb1c66da0cbb..d9f87db79b7de 100644 --- a/compiler/rustc_middle/src/mir/mod.rs +++ b/compiler/rustc_middle/src/mir/mod.rs @@ -2,7 +2,7 @@ //! //! [rustc dev guide]: https://rustc-dev-guide.rust-lang.org/mir/index.html -use crate::mir::interpret::{AllocRange, ConstAllocation, ErrorHandled, Scalar}; +use crate::mir::interpret::{AllocRange, ConstAllocation, Scalar}; use crate::mir::visit::MirVisitable; use crate::ty::codec::{TyDecoder, TyEncoder}; use crate::ty::fold::{FallibleTypeFolder, TypeFoldable}; @@ -568,34 +568,6 @@ impl<'tcx> Body<'tcx> { pub fn is_custom_mir(&self) -> bool { self.injection_phase.is_some() } - - /// *Must* be called once the full substitution for this body is known, to ensure that the body - /// is indeed fit for code generation or consumption more generally. - /// - /// Sadly there's no nice way to represent an "arbitrary normalizer", so we take one for - /// constants specifically. (`Option` could be used for that, but the fact - /// that `Instance::args_for_mir_body` is private and instead instance exposes normalization - /// functions makes it seem like exposing the generic args is not the intended strategy.) - /// - /// Also sadly, CTFE doesn't even know whether it runs on MIR that is already polymorphic or still monomorphic, - /// so we cannot just immediately ICE on TooGeneric. - /// - /// Returns Ok(()) if everything went fine, and `Err` if a problem occurred and got reported. - pub fn post_mono_checks( - &self, - tcx: TyCtxt<'tcx>, - param_env: ty::ParamEnv<'tcx>, - normalize_const: impl Fn(Const<'tcx>) -> Result, ErrorHandled>, - ) -> Result<(), ErrorHandled> { - // For now, the only thing we have to check is is to ensure that all the constants used in - // the body successfully evaluate. - for &const_ in &self.required_consts { - let c = normalize_const(const_.const_)?; - c.eval(tcx, param_env, Some(const_.span))?; - } - - Ok(()) - } } #[derive(Copy, Clone, PartialEq, Eq, Debug, TyEncodable, TyDecodable, HashStable)] diff --git a/src/tools/miri/src/concurrency/thread.rs b/src/tools/miri/src/concurrency/thread.rs index ef4e7df3aba93..9041683fbc46b 100644 --- a/src/tools/miri/src/concurrency/thread.rs +++ b/src/tools/miri/src/concurrency/thread.rs @@ -803,8 +803,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { if tcx.is_foreign_item(def_id) { throw_unsup_format!("foreign thread-local statics are not supported"); } - // We don't give a span -- statics don't need that, they cannot be generic or associated. - let allocation = this.ctfe_query(None, |tcx| tcx.eval_static_initializer(def_id))?; + let allocation = this.ctfe_query(|tcx| tcx.eval_static_initializer(def_id))?; let mut allocation = allocation.inner().clone(); // This allocation will be deallocated when the thread dies, so it is not in read-only memory. allocation.mutability = Mutability::Mut; From fa248cd9e6927dd6981078963aa47feb941a0d10 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 6 Oct 2023 18:25:23 +0200 Subject: [PATCH 2/2] add some comments explaining how the required_consts stuff fits together --- compiler/rustc_codegen_ssa/src/mir/constant.rs | 2 ++ compiler/rustc_codegen_ssa/src/mir/mod.rs | 4 ++++ compiler/rustc_middle/src/mir/visit.rs | 2 ++ compiler/rustc_monomorphize/src/collector.rs | 2 ++ 4 files changed, 10 insertions(+) diff --git a/compiler/rustc_codegen_ssa/src/mir/constant.rs b/compiler/rustc_codegen_ssa/src/mir/constant.rs index fde4e85f96629..558f64fffc263 100644 --- a/compiler/rustc_codegen_ssa/src/mir/constant.rs +++ b/compiler/rustc_codegen_ssa/src/mir/constant.rs @@ -21,6 +21,8 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { } pub fn eval_mir_constant(&self, constant: &mir::ConstOperand<'tcx>) -> mir::ConstValue<'tcx> { + // `MirUsedCollector` visited all constants before codegen began, so if we got here there + // can be no more constants that fail to evaluate. self.monomorphize(constant.const_) .eval(self.cx.tcx(), ty::ParamEnv::reveal_all(), Some(constant.span)) .expect("erroneous constant not captured by required_consts") diff --git a/compiler/rustc_codegen_ssa/src/mir/mod.rs b/compiler/rustc_codegen_ssa/src/mir/mod.rs index 06cdc47610df8..d0b799e087b2a 100644 --- a/compiler/rustc_codegen_ssa/src/mir/mod.rs +++ b/compiler/rustc_codegen_ssa/src/mir/mod.rs @@ -209,6 +209,10 @@ pub fn codegen_mir<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( caller_location: None, }; + // It may seem like we should iterate over `required_consts` to ensure they all successfully + // evaluate; however, the `MirUsedCollector` already did that during the collection phase of + // monomorphization so we don't have to do it again. + fx.per_local_var_debug_info = fx.compute_per_local_var_debug_info(&mut start_bx); let memory_locals = analyze::non_ssa_locals(&fx); diff --git a/compiler/rustc_middle/src/mir/visit.rs b/compiler/rustc_middle/src/mir/visit.rs index 51ec6da1ac8ea..cc135f732cb79 100644 --- a/compiler/rustc_middle/src/mir/visit.rs +++ b/compiler/rustc_middle/src/mir/visit.rs @@ -184,6 +184,8 @@ macro_rules! make_mir_visitor { visit_place_fns!($($mutability)?); + /// This is called for every constant in the MIR body and every `required_consts` + /// (i.e., including consts that have been dead-code-eliminated). fn visit_constant( &mut self, constant: & $($mutability)? ConstOperand<'tcx>, diff --git a/compiler/rustc_monomorphize/src/collector.rs b/compiler/rustc_monomorphize/src/collector.rs index 1a9f0e8352e73..8525a4e74c29d 100644 --- a/compiler/rustc_monomorphize/src/collector.rs +++ b/compiler/rustc_monomorphize/src/collector.rs @@ -1426,6 +1426,8 @@ fn collect_used_items<'tcx>( ); } + // Here we rely on the visitor also visiting `required_consts`, so that we evaluate them + // and abort compilation if any of them errors. MirUsedCollector { tcx, body: &body,