From 9e00fb0d8911dfdf9cb9583adcc05d55bef4d34f Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 4 Jun 2022 11:15:36 -0400 Subject: [PATCH 1/4] tweak some bug!s --- compiler/rustc_const_eval/src/interpret/intrinsics.rs | 6 +++--- .../src/interpret/intrinsics/caller_location.rs | 2 +- compiler/rustc_const_eval/src/interpret/operator.rs | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_const_eval/src/interpret/intrinsics.rs b/compiler/rustc_const_eval/src/interpret/intrinsics.rs index bf1cf816ddd0..c5770f505262 100644 --- a/compiler/rustc_const_eval/src/interpret/intrinsics.rs +++ b/compiler/rustc_const_eval/src/interpret/intrinsics.rs @@ -169,7 +169,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { sym::needs_drop => self.tcx.types.bool, sym::type_id => self.tcx.types.u64, sym::type_name => self.tcx.mk_static_str(), - _ => bug!("already checked for nullary intrinsics"), + _ => bug!(), }; let val = self.tcx.const_eval_global_id(self.param_env, gid, Some(self.tcx.span))?; @@ -215,7 +215,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { sym::add_with_overflow => BinOp::Add, sym::sub_with_overflow => BinOp::Sub, sym::mul_with_overflow => BinOp::Mul, - _ => bug!("Already checked for int ops"), + _ => bug!(), }; self.binop_with_overflow(bin_op, &lhs, &rhs, dest)?; } @@ -251,7 +251,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { sym::unchecked_mul => BinOp::Mul, sym::unchecked_div => BinOp::Div, sym::unchecked_rem => BinOp::Rem, - _ => bug!("Already checked for int ops"), + _ => bug!(), }; let (val, overflowed, _ty) = self.overflowing_binary_op(bin_op, &l, &r)?; if overflowed { diff --git a/compiler/rustc_const_eval/src/interpret/intrinsics/caller_location.rs b/compiler/rustc_const_eval/src/interpret/intrinsics/caller_location.rs index e66cb9837c99..23ae2db6438c 100644 --- a/compiler/rustc_const_eval/src/interpret/intrinsics/caller_location.rs +++ b/compiler/rustc_const_eval/src/interpret/intrinsics/caller_location.rs @@ -70,7 +70,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } } - bug!("no non-`#[track_caller]` frame found") + span_bug!(self.cur_span(), "no non-`#[track_caller]` frame found") } /// Allocate a `const core::panic::Location` with the provided filename and line/column numbers. diff --git a/compiler/rustc_const_eval/src/interpret/operator.rs b/compiler/rustc_const_eval/src/interpret/operator.rs index 6dae9dc72b7b..85ee88e9e474 100644 --- a/compiler/rustc_const_eval/src/interpret/operator.rs +++ b/compiler/rustc_const_eval/src/interpret/operator.rs @@ -154,14 +154,14 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { let result = match bin_op { Shl => l.checked_shl(r).unwrap(), Shr => l.checked_shr(r).unwrap(), - _ => bug!("it has already been checked that this is a shift op"), + _ => bug!(), }; result as u128 } else { match bin_op { Shl => l.checked_shl(r).unwrap(), Shr => l.checked_shr(r).unwrap(), - _ => bug!("it has already been checked that this is a shift op"), + _ => bug!(), } }; let truncated = self.truncate(result, left_layout); From 7892e1cedb0b1fe8d924516425481654b89ba834 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 4 Jun 2022 15:28:14 -0400 Subject: [PATCH 2/4] Move statement_index increment out of statement() function That function is called by const_prop, where updating the index like that is totally meaningless. --- .../rustc_const_eval/src/interpret/step.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/compiler/rustc_const_eval/src/interpret/step.rs b/compiler/rustc_const_eval/src/interpret/step.rs index 9ac86911c7d9..c511581371b8 100644 --- a/compiler/rustc_const_eval/src/interpret/step.rs +++ b/compiler/rustc_const_eval/src/interpret/step.rs @@ -55,33 +55,33 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { }; let basic_block = &self.body().basic_blocks()[loc.block]; - let old_frames = self.frame_idx(); if let Some(stmt) = basic_block.statements.get(loc.statement_index) { - assert_eq!(old_frames, self.frame_idx()); + let old_frames = self.frame_idx(); self.statement(stmt)?; + // Make sure we are not updating `statement_index` of the wrong frame. + assert_eq!(old_frames, self.frame_idx()); + // Advance the program counter. + self.frame_mut().loc.as_mut().unwrap().statement_index += 1; return Ok(true); } M::before_terminator(self)?; let terminator = basic_block.terminator(); - assert_eq!(old_frames, self.frame_idx()); self.terminator(terminator)?; Ok(true) } /// Runs the interpretation logic for the given `mir::Statement` at the current frame and - /// statement counter. This also moves the statement counter forward. + /// statement counter. + /// + /// This does NOT move the statement counter forward, the caller has to do that! pub fn statement(&mut self, stmt: &mir::Statement<'tcx>) -> InterpResult<'tcx> { info!("{:?}", stmt); use rustc_middle::mir::StatementKind::*; - // Some statements (e.g., box) push new stack frames. - // We have to record the stack frame number *before* executing the statement. - let frame_idx = self.frame_idx(); - match &stmt.kind { Assign(box (place, rvalue)) => self.eval_rvalue_into_place(rvalue, *place)?, @@ -144,7 +144,6 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { Nop => {} } - self.stack_mut()[frame_idx].loc.as_mut().unwrap().statement_index += 1; Ok(()) } @@ -300,6 +299,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { Ok(()) } + /// Evaluate the given terminator. Will also adjust the stack frame and statement position accordingly. fn terminator(&mut self, terminator: &mir::Terminator<'tcx>) -> InterpResult<'tcx> { info!("{:?}", terminator.kind); From 9ab4f876a1c5321fe451bf691eb28d87dbf5ff84 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 4 Jun 2022 15:29:51 -0400 Subject: [PATCH 3/4] const_prop_lint: ensure we have up-to-date cur_span() --- .../src/interpret/eval_context.rs | 5 +++- .../src/const_prop_lint.rs | 29 ++++++++++++------- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/compiler/rustc_const_eval/src/interpret/eval_context.rs b/compiler/rustc_const_eval/src/interpret/eval_context.rs index 0c954ac6e5f6..a82ddfb5ac50 100644 --- a/compiler/rustc_const_eval/src/interpret/eval_context.rs +++ b/compiler/rustc_const_eval/src/interpret/eval_context.rs @@ -126,7 +126,9 @@ pub struct Frame<'mir, 'tcx, Tag: Provenance = AllocId, Extra = ()> { /// this frame (can happen e.g. during frame initialization, and during unwinding on /// frames without cleanup code). /// We basically abuse `Result` as `Either`. - pub(super) loc: Result, + /// + /// Needs to be public because ConstProp does unspeakable things to it. + pub loc: Result, } /// What we store about a frame in an interpreter backtrace. @@ -320,6 +322,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> LayoutOfHelpers<'tcx> for InterpC #[inline] fn layout_tcx_at_span(&self) -> Span { + // Using the cheap root span for performance. self.tcx.span } diff --git a/compiler/rustc_mir_transform/src/const_prop_lint.rs b/compiler/rustc_mir_transform/src/const_prop_lint.rs index 5fd9db3989d0..84fdb136bd41 100644 --- a/compiler/rustc_mir_transform/src/const_prop_lint.rs +++ b/compiler/rustc_mir_transform/src/const_prop_lint.rs @@ -437,10 +437,12 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { source_info.scope.lint_root(self.source_scopes) } - fn use_ecx(&mut self, f: F) -> Option + fn use_ecx(&mut self, source_info: SourceInfo, f: F) -> Option where F: FnOnce(&mut Self) -> InterpResult<'tcx, T>, { + // Overwrite the PC -- whatever the interpreter does to it does not make any sense anyway. + self.ecx.frame_mut().loc = Err(source_info.span); match f(self) { Ok(val) => Some(val), Err(error) => { @@ -501,9 +503,9 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { } /// Returns the value, if any, of evaluating `place`. - fn eval_place(&mut self, place: Place<'tcx>) -> Option> { + fn eval_place(&mut self, place: Place<'tcx>, source_info: SourceInfo) -> Option> { trace!("eval_place(place={:?})", place); - self.use_ecx(|this| this.ecx.eval_place_to_op(place, None)) + self.use_ecx(source_info, |this| this.ecx.eval_place_to_op(place, None)) } /// Returns the value, if any, of evaluating `op`. Calls upon `eval_constant` @@ -511,7 +513,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { fn eval_operand(&mut self, op: &Operand<'tcx>, source_info: SourceInfo) -> Option> { match *op { Operand::Constant(ref c) => self.eval_constant(c, source_info), - Operand::Move(place) | Operand::Copy(place) => self.eval_place(place), + Operand::Move(place) | Operand::Copy(place) => self.eval_place(place, source_info), } } @@ -537,7 +539,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { arg: &Operand<'tcx>, source_info: SourceInfo, ) -> Option<()> { - if let (val, true) = self.use_ecx(|this| { + if let (val, true) = self.use_ecx(source_info, |this| { let val = this.ecx.read_immediate(&this.ecx.eval_operand(arg, None)?)?; let (_res, overflow, _ty) = this.ecx.overflowing_unary_op(op, &val)?; Ok((val, overflow)) @@ -564,8 +566,12 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { right: &Operand<'tcx>, source_info: SourceInfo, ) -> Option<()> { - let r = self.use_ecx(|this| this.ecx.read_immediate(&this.ecx.eval_operand(right, None)?)); - let l = self.use_ecx(|this| this.ecx.read_immediate(&this.ecx.eval_operand(left, None)?)); + let r = self.use_ecx(source_info, |this| { + this.ecx.read_immediate(&this.ecx.eval_operand(right, None)?) + }); + let l = self.use_ecx(source_info, |this| { + this.ecx.read_immediate(&this.ecx.eval_operand(left, None)?) + }); // Check for exceeding shifts *even if* we cannot evaluate the LHS. if op == BinOp::Shr || op == BinOp::Shl { let r = r?; @@ -602,7 +608,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { if let (Some(l), Some(r)) = (&l, &r) { // The remaining operators are handled through `overflowing_binary_op`. - if self.use_ecx(|this| { + if self.use_ecx(source_info, |this| { let (_res, overflow, _ty) = this.ecx.overflowing_binary_op(op, l, r)?; Ok(overflow) })? { @@ -690,7 +696,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { return None; } - self.use_ecx(|this| this.ecx.eval_rvalue_into_place(rvalue, place)) + self.use_ecx(source_info, |this| this.ecx.eval_rvalue_into_place(rvalue, place)) } } @@ -890,7 +896,10 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { StatementKind::SetDiscriminant { ref place, .. } => { match self.ecx.machine.can_const_prop[place.local] { ConstPropMode::FullConstProp | ConstPropMode::OnlyInsideOwnBlock => { - if self.use_ecx(|this| this.ecx.statement(statement)).is_some() { + if self + .use_ecx(source_info, |this| this.ecx.statement(statement)) + .is_some() + { trace!("propped discriminant into {:?}", place); } else { Self::remove_const(&mut self.ecx, place.local); From 467e0f44463f4026922925e6666d96a772ae39d7 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 4 Jun 2022 15:48:16 -0400 Subject: [PATCH 4/4] use precise spans for recursive const evaluation --- .../src/interpret/eval_context.rs | 3 ++- .../rustc_const_eval/src/interpret/memory.rs | 3 ++- .../rustc_const_eval/src/interpret/step.rs | 1 - .../rustc_middle/src/mir/interpret/queries.rs | 19 ++++++++++++++++--- .../recursive-zst-static.default.stderr | 4 ++-- .../recursive-zst-static.unleash.stderr | 4 ++-- .../write-to-static-mut-in-static.stderr | 4 ++-- .../recursive-static-definition.stderr | 4 ++-- 8 files changed, 28 insertions(+), 14 deletions(-) diff --git a/compiler/rustc_const_eval/src/interpret/eval_context.rs b/compiler/rustc_const_eval/src/interpret/eval_context.rs index a82ddfb5ac50..85de5908a23a 100644 --- a/compiler/rustc_const_eval/src/interpret/eval_context.rs +++ b/compiler/rustc_const_eval/src/interpret/eval_context.rs @@ -926,7 +926,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { self.param_env }; let param_env = param_env.with_const(); - let val = self.tcx.eval_to_allocation_raw(param_env.and(gid))?; + // Use a precise span for better cycle errors. + let val = self.tcx.at(self.cur_span()).eval_to_allocation_raw(param_env.and(gid))?; self.raw_const_to_mplace(val) } diff --git a/compiler/rustc_const_eval/src/interpret/memory.rs b/compiler/rustc_const_eval/src/interpret/memory.rs index 9c032c55fe54..17b8ab8742a5 100644 --- a/compiler/rustc_const_eval/src/interpret/memory.rs +++ b/compiler/rustc_const_eval/src/interpret/memory.rs @@ -504,7 +504,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { throw_unsup!(ReadExternStatic(def_id)); } - (self.tcx.eval_static_initializer(def_id)?, Some(def_id)) + // Use a precise span for better cycle errors. + (self.tcx.at(self.cur_span()).eval_static_initializer(def_id)?, Some(def_id)) } }; M::before_access_global(*self.tcx, &self.machine, id, alloc, def_id, is_write)?; diff --git a/compiler/rustc_const_eval/src/interpret/step.rs b/compiler/rustc_const_eval/src/interpret/step.rs index c511581371b8..98f69456e49a 100644 --- a/compiler/rustc_const_eval/src/interpret/step.rs +++ b/compiler/rustc_const_eval/src/interpret/step.rs @@ -55,7 +55,6 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { }; let basic_block = &self.body().basic_blocks()[loc.block]; - if let Some(stmt) = basic_block.statements.get(loc.statement_index) { let old_frames = self.frame_idx(); self.statement(stmt)?; diff --git a/compiler/rustc_middle/src/mir/interpret/queries.rs b/compiler/rustc_middle/src/mir/interpret/queries.rs index 5fb8e911124d..8fc957cf49c8 100644 --- a/compiler/rustc_middle/src/mir/interpret/queries.rs +++ b/compiler/rustc_middle/src/mir/interpret/queries.rs @@ -3,9 +3,9 @@ use super::{ErrorHandled, EvalToConstValueResult, GlobalId}; use crate::mir; use crate::ty::fold::TypeFoldable; use crate::ty::subst::InternalSubsts; -use crate::ty::{self, TyCtxt}; +use crate::ty::{self, query::TyCtxtAt, TyCtxt}; use rustc_hir::def_id::DefId; -use rustc_span::Span; +use rustc_span::{Span, DUMMY_SP}; impl<'tcx> TyCtxt<'tcx> { /// Evaluates a constant without providing any substitutions. This is useful to evaluate consts @@ -86,6 +86,17 @@ impl<'tcx> TyCtxt<'tcx> { } } + /// Evaluate a static's initializer, returning the allocation of the initializer's memory. + #[inline(always)] + pub fn eval_static_initializer( + self, + def_id: DefId, + ) -> Result, ErrorHandled> { + self.at(DUMMY_SP).eval_static_initializer(def_id) + } +} + +impl<'tcx> TyCtxtAt<'tcx> { /// Evaluate a static's initializer, returning the allocation of the initializer's memory. pub fn eval_static_initializer( self, @@ -93,7 +104,7 @@ impl<'tcx> TyCtxt<'tcx> { ) -> Result, ErrorHandled> { trace!("eval_static_initializer: Need to compute {:?}", def_id); assert!(self.is_static(def_id)); - let instance = ty::Instance::mono(self, def_id); + let instance = ty::Instance::mono(*self, def_id); let gid = GlobalId { instance, promoted: None }; self.eval_to_allocation(gid, ty::ParamEnv::reveal_all()) } @@ -109,7 +120,9 @@ impl<'tcx> TyCtxt<'tcx> { let raw_const = self.eval_to_allocation_raw(param_env.and(gid))?; Ok(self.global_alloc(raw_const.alloc_id).unwrap_memory()) } +} +impl<'tcx> TyCtxt<'tcx> { /// Destructure a type-level constant ADT or array into its variant index and its field values. /// Panics if the destructuring fails, use `try_destructure_const` for fallible version. pub fn destructure_const( diff --git a/src/test/ui/consts/recursive-zst-static.default.stderr b/src/test/ui/consts/recursive-zst-static.default.stderr index 03f8f5c5a0e5..2a4ad5825ecf 100644 --- a/src/test/ui/consts/recursive-zst-static.default.stderr +++ b/src/test/ui/consts/recursive-zst-static.default.stderr @@ -5,10 +5,10 @@ LL | static FOO: () = FOO; | ^^^^^^^^^^^^^^^^^^^^^ | note: ...which requires const-evaluating + checking `FOO`... - --> $DIR/recursive-zst-static.rs:10:1 + --> $DIR/recursive-zst-static.rs:10:18 | LL | static FOO: () = FOO; - | ^^^^^^^^^^^^^^^^^^^^^ + | ^^^ = note: ...which again requires const-evaluating + checking `FOO`, completing the cycle = note: cycle used when running analysis passes on this crate diff --git a/src/test/ui/consts/recursive-zst-static.unleash.stderr b/src/test/ui/consts/recursive-zst-static.unleash.stderr index 03f8f5c5a0e5..2a4ad5825ecf 100644 --- a/src/test/ui/consts/recursive-zst-static.unleash.stderr +++ b/src/test/ui/consts/recursive-zst-static.unleash.stderr @@ -5,10 +5,10 @@ LL | static FOO: () = FOO; | ^^^^^^^^^^^^^^^^^^^^^ | note: ...which requires const-evaluating + checking `FOO`... - --> $DIR/recursive-zst-static.rs:10:1 + --> $DIR/recursive-zst-static.rs:10:18 | LL | static FOO: () = FOO; - | ^^^^^^^^^^^^^^^^^^^^^ + | ^^^ = note: ...which again requires const-evaluating + checking `FOO`, completing the cycle = note: cycle used when running analysis passes on this crate diff --git a/src/test/ui/consts/write-to-static-mut-in-static.stderr b/src/test/ui/consts/write-to-static-mut-in-static.stderr index 789919bd1668..ab4b8844e5b5 100644 --- a/src/test/ui/consts/write-to-static-mut-in-static.stderr +++ b/src/test/ui/consts/write-to-static-mut-in-static.stderr @@ -11,10 +11,10 @@ LL | pub static mut C: u32 = unsafe { C = 1; 0 }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | note: ...which requires const-evaluating + checking `C`... - --> $DIR/write-to-static-mut-in-static.rs:5:1 + --> $DIR/write-to-static-mut-in-static.rs:5:34 | LL | pub static mut C: u32 = unsafe { C = 1; 0 }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^^^^^ = note: ...which again requires const-evaluating + checking `C`, completing the cycle = note: cycle used when running analysis passes on this crate diff --git a/src/test/ui/recursion/recursive-static-definition.stderr b/src/test/ui/recursion/recursive-static-definition.stderr index ee73b026a0b7..be4f09f9286d 100644 --- a/src/test/ui/recursion/recursive-static-definition.stderr +++ b/src/test/ui/recursion/recursive-static-definition.stderr @@ -5,10 +5,10 @@ LL | pub static FOO: u32 = FOO; | ^^^^^^^^^^^^^^^^^^^^^^^^^^ | note: ...which requires const-evaluating + checking `FOO`... - --> $DIR/recursive-static-definition.rs:1:1 + --> $DIR/recursive-static-definition.rs:1:23 | LL | pub static FOO: u32 = FOO; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^^^ = note: ...which again requires const-evaluating + checking `FOO`, completing the cycle = note: cycle used when running analysis passes on this crate