From 1678c5c32810978da826b1c53aada6b9197144b1 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 9 Mar 2024 17:42:41 +0100 Subject: [PATCH 1/5] add_retag: fix comment that does not match the code --- compiler/rustc_mir_transform/src/add_retag.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/rustc_mir_transform/src/add_retag.rs b/compiler/rustc_mir_transform/src/add_retag.rs index 430d9572e75..6f668aa4ce8 100644 --- a/compiler/rustc_mir_transform/src/add_retag.rs +++ b/compiler/rustc_mir_transform/src/add_retag.rs @@ -118,7 +118,7 @@ impl<'tcx> MirPass<'tcx> for AddRetag { } // PART 3 - // Add retag after assignments where data "enters" this function: the RHS is behind a deref and the LHS is not. + // Add retag after assignments. for block_data in basic_blocks { // We want to insert statements as we iterate. To this end, we // iterate backwards using indices. From 2005c2e54dcb575b1d308ad711266ba1d5102549 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 9 Mar 2024 18:13:50 +0100 Subject: [PATCH 2/5] interpret: ensure that Place is never used for a different frame --- .../src/interpret/eval_context.rs | 14 +++-- .../rustc_const_eval/src/interpret/operand.rs | 5 +- .../rustc_const_eval/src/interpret/place.rs | 59 +++++++++++-------- compiler/rustc_const_eval/src/lib.rs | 1 + 4 files changed, 46 insertions(+), 33 deletions(-) diff --git a/compiler/rustc_const_eval/src/interpret/eval_context.rs b/compiler/rustc_const_eval/src/interpret/eval_context.rs index a484fbd892c..d98d2107102 100644 --- a/compiler/rustc_const_eval/src/interpret/eval_context.rs +++ b/compiler/rustc_const_eval/src/interpret/eval_context.rs @@ -1,4 +1,5 @@ use std::cell::Cell; +use std::ptr; use std::{fmt, mem}; use either::{Either, Left, Right}; @@ -279,6 +280,11 @@ impl<'mir, 'tcx, Prov: Provenance, Extra> Frame<'mir, 'tcx, Prov, Extra> { } }) } + + #[inline(always)] + pub(super) fn locals_addr(&self) -> usize { + ptr::addr_of!(self.locals).addr() + } } // FIXME: only used by miri, should be removed once translatable. @@ -1212,18 +1218,16 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> std::fmt::Debug { fn fmt(&self, fmt: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self.place { - Place::Local { frame, local, offset } => { + Place::Local { local, offset, locals_addr } => { + debug_assert_eq!(locals_addr, self.ecx.frame().locals_addr()); let mut allocs = Vec::new(); write!(fmt, "{local:?}")?; if let Some(offset) = offset { write!(fmt, "+{:#x}", offset.bytes())?; } - if frame != self.ecx.frame_idx() { - write!(fmt, " ({} frames up)", self.ecx.frame_idx() - frame)?; - } write!(fmt, ":")?; - match self.ecx.stack()[frame].locals[local].value { + match self.ecx.frame().locals[local].value { LocalValue::Dead => write!(fmt, " is dead")?, LocalValue::Live(Operand::Immediate(Immediate::Uninit)) => { write!(fmt, " is uninitialized")? diff --git a/compiler/rustc_const_eval/src/interpret/operand.rs b/compiler/rustc_const_eval/src/interpret/operand.rs index 317e5673b51..48b0dad47ff 100644 --- a/compiler/rustc_const_eval/src/interpret/operand.rs +++ b/compiler/rustc_const_eval/src/interpret/operand.rs @@ -661,9 +661,10 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { ) -> InterpResult<'tcx, OpTy<'tcx, M::Provenance>> { match place.as_mplace_or_local() { Left(mplace) => Ok(mplace.into()), - Right((frame, local, offset)) => { + Right((local, offset, locals_addr)) => { debug_assert!(place.layout.is_sized()); // only sized locals can ever be `Place::Local`. - let base = self.local_to_op(&self.stack()[frame], local, None)?; + debug_assert_eq!(locals_addr, self.frame().locals_addr()); + let base = self.local_to_op(&self.frame(), local, None)?; Ok(match offset { Some(offset) => base.offset(offset, place.layout, self)?, None => { diff --git a/compiler/rustc_const_eval/src/interpret/place.rs b/compiler/rustc_const_eval/src/interpret/place.rs index 60f7710c11d..52a74f43405 100644 --- a/compiler/rustc_const_eval/src/interpret/place.rs +++ b/compiler/rustc_const_eval/src/interpret/place.rs @@ -187,11 +187,13 @@ pub(super) enum Place { /// where in the local this place is located; if it is `None`, no projection has been applied. /// Such projections are meaningful even if the offset is 0, since they can change layouts. /// (Without that optimization, we'd just always be a `MemPlace`.) - /// Note that this only stores the frame index, not the thread this frame belongs to -- that is - /// implicit. This means a `Place` must never be moved across interpreter thread boundaries! + /// `Local` places always refer to the current stack frame, so they are unstable under + /// function calls/returns and switching betweens stacks of different threads! + /// We carry around the address of the `locals` buffer of the correct stack frame as a sanity + /// chec to be able to catch some cases of using a dangling `Place`. /// /// This variant shall not be used for unsized types -- those must always live in memory. - Local { frame: usize, local: mir::Local, offset: Option }, + Local { local: mir::Local, offset: Option, locals_addr: usize }, } /// An evaluated place, together with its type. @@ -233,10 +235,10 @@ impl<'tcx, Prov: Provenance> PlaceTy<'tcx, Prov> { #[inline(always)] pub fn as_mplace_or_local( &self, - ) -> Either, (usize, mir::Local, Option)> { + ) -> Either, (mir::Local, Option, usize)> { match self.place { Place::Ptr(mplace) => Left(MPlaceTy { mplace, layout: self.layout }), - Place::Local { frame, local, offset } => Right((frame, local, offset)), + Place::Local { local, offset, locals_addr } => Right((local, offset, locals_addr)), } } @@ -279,7 +281,7 @@ impl<'tcx, Prov: Provenance> Projectable<'tcx, Prov> for PlaceTy<'tcx, Prov> { ) -> InterpResult<'tcx, Self> { Ok(match self.as_mplace_or_local() { Left(mplace) => mplace.offset_with_meta(offset, mode, meta, layout, ecx)?.into(), - Right((frame, local, old_offset)) => { + Right((local, old_offset, locals_addr)) => { debug_assert!(layout.is_sized(), "unsized locals should live in memory"); assert_matches!(meta, MemPlaceMeta::None); // we couldn't store it anyway... // `Place::Local` are always in-bounds of their surrounding local, so we can just @@ -292,7 +294,10 @@ impl<'tcx, Prov: Provenance> Projectable<'tcx, Prov> for PlaceTy<'tcx, Prov> { .offset(old_offset.unwrap_or(Size::ZERO).bytes(), offset.bytes())?, ); - PlaceTy { place: Place::Local { frame, local, offset: Some(new_offset) }, layout } + PlaceTy { + place: Place::Local { local, offset: Some(new_offset), locals_addr }, + layout, + } } }) } @@ -331,7 +336,7 @@ impl<'tcx, Prov: Provenance> OpTy<'tcx, Prov> { pub trait Writeable<'tcx, Prov: Provenance>: Projectable<'tcx, Prov> { fn as_mplace_or_local( &self, - ) -> Either, (usize, mir::Local, Option, TyAndLayout<'tcx>)>; + ) -> Either, (mir::Local, Option, usize, TyAndLayout<'tcx>)>; fn force_mplace<'mir, M: Machine<'mir, 'tcx, Provenance = Prov>>( &self, @@ -343,9 +348,9 @@ impl<'tcx, Prov: Provenance> Writeable<'tcx, Prov> for PlaceTy<'tcx, Prov> { #[inline(always)] fn as_mplace_or_local( &self, - ) -> Either, (usize, mir::Local, Option, TyAndLayout<'tcx>)> { + ) -> Either, (mir::Local, Option, usize, TyAndLayout<'tcx>)> { self.as_mplace_or_local() - .map_right(|(frame, local, offset)| (frame, local, offset, self.layout)) + .map_right(|(local, offset, locals_addr)| (local, offset, locals_addr, self.layout)) } #[inline(always)] @@ -361,7 +366,7 @@ impl<'tcx, Prov: Provenance> Writeable<'tcx, Prov> for MPlaceTy<'tcx, Prov> { #[inline(always)] fn as_mplace_or_local( &self, - ) -> Either, (usize, mir::Local, Option, TyAndLayout<'tcx>)> { + ) -> Either, (mir::Local, Option, usize, TyAndLayout<'tcx>)> { Left(self.clone()) } @@ -512,7 +517,7 @@ where let layout = self.layout_of_local(frame_ref, local, None)?; let place = if layout.is_sized() { // We can just always use the `Local` for sized values. - Place::Local { frame, local, offset: None } + Place::Local { local, offset: None, locals_addr: frame_ref.locals_addr() } } else { // Unsized `Local` isn't okay (we cannot store the metadata). match frame_ref.locals[local].access()? { @@ -611,15 +616,16 @@ where // See if we can avoid an allocation. This is the counterpart to `read_immediate_raw`, // but not factored as a separate function. let mplace = match dest.as_mplace_or_local() { - Right((frame, local, offset, layout)) => { + Right((local, offset, locals_addr, layout)) => { if offset.is_some() { // This has been projected to a part of this local. We could have complicated // logic to still keep this local as an `Operand`... but it's much easier to // just fall back to the indirect path. dest.force_mplace(self)? } else { - M::before_access_local_mut(self, frame, local)?; - match self.stack_mut()[frame].locals[local].access_mut()? { + debug_assert_eq!(locals_addr, self.frame().locals_addr()); + M::before_access_local_mut(self, self.frame_idx(), local)?; + match self.frame_mut().locals[local].access_mut()? { Operand::Immediate(local_val) => { // Local can be updated in-place. *local_val = src; @@ -627,7 +633,7 @@ where // (*After* doing the update for borrow checker reasons.) if cfg!(debug_assertions) { let local_layout = - self.layout_of_local(&self.stack()[frame], local, None)?; + self.layout_of_local(&self.frame(), local, None)?; match (src, local_layout.abi) { (Immediate::Scalar(scalar), Abi::Scalar(s)) => { assert_eq!(scalar.size(), s.size(self)) @@ -725,7 +731,7 @@ where ) -> InterpResult<'tcx> { let mplace = match dest.as_mplace_or_local() { Left(mplace) => mplace, - Right((frame, local, offset, layout)) => { + Right((local, offset, locals_addr, layout)) => { if offset.is_some() { // This has been projected to a part of this local. We could have complicated // logic to still keep this local as an `Operand`... but it's much easier to @@ -733,8 +739,9 @@ where // FIXME: share the logic with `write_immediate_no_validate`. dest.force_mplace(self)? } else { - M::before_access_local_mut(self, frame, local)?; - match self.stack_mut()[frame].locals[local].access_mut()? { + debug_assert_eq!(locals_addr, self.frame().locals_addr()); + M::before_access_local_mut(self, self.frame_idx(), local)?; + match self.frame_mut().locals[local].access_mut()? { Operand::Immediate(local) => { *local = Immediate::Uninit; return Ok(()); @@ -912,17 +919,17 @@ where place: &PlaceTy<'tcx, M::Provenance>, ) -> InterpResult<'tcx, MPlaceTy<'tcx, M::Provenance>> { let mplace = match place.place { - Place::Local { frame, local, offset } => { - M::before_access_local_mut(self, frame, local)?; - let whole_local = match self.stack_mut()[frame].locals[local].access_mut()? { + Place::Local { local, offset, locals_addr } => { + debug_assert_eq!(locals_addr, self.frame().locals_addr()); + M::before_access_local_mut(self, self.frame_idx(), local)?; + let whole_local = match self.frame_mut().locals[local].access_mut()? { &mut Operand::Immediate(local_val) => { // We need to make an allocation. // We need the layout of the local. We can NOT use the layout we got, // that might e.g., be an inner field of a struct with `Scalar` layout, // that has different alignment than the outer field. - let local_layout = - self.layout_of_local(&self.stack()[frame], local, None)?; + let local_layout = self.layout_of_local(&self.frame(), local, None)?; assert!(local_layout.is_sized(), "unsized locals cannot be immediate"); let mplace = self.allocate(local_layout, MemoryKind::Stack)?; // Preserve old value. (As an optimization, we can skip this if it was uninit.) @@ -936,11 +943,11 @@ where mplace.mplace, )?; } - M::after_local_allocated(self, frame, local, &mplace)?; + M::after_local_allocated(self, self.frame_idx(), local, &mplace)?; // Now we can call `access_mut` again, asserting it goes well, and actually // overwrite things. This points to the entire allocation, not just the part // the place refers to, i.e. we do this before we apply `offset`. - *self.stack_mut()[frame].locals[local].access_mut().unwrap() = + *self.frame_mut().locals[local].access_mut().unwrap() = Operand::Indirect(mplace.mplace); mplace.mplace } diff --git a/compiler/rustc_const_eval/src/lib.rs b/compiler/rustc_const_eval/src/lib.rs index 51836063945..1e7ee208af1 100644 --- a/compiler/rustc_const_eval/src/lib.rs +++ b/compiler/rustc_const_eval/src/lib.rs @@ -14,6 +14,7 @@ Rust MIR: a lowered representation of Rust. #![feature(generic_nonzero)] #![feature(let_chains)] #![feature(slice_ptr_get)] +#![feature(strict_provenance)] #![feature(never_type)] #![feature(trait_alias)] #![feature(try_blocks)] From b888e895dec646657d6444652615a69c87276e5a Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 9 Mar 2024 18:15:56 +0100 Subject: [PATCH 3/5] remove a machine hook that is no longer used --- .../src/interpret/eval_context.rs | 3 --- .../rustc_const_eval/src/interpret/machine.rs | 18 ------------------ .../rustc_const_eval/src/interpret/place.rs | 3 --- 3 files changed, 24 deletions(-) diff --git a/compiler/rustc_const_eval/src/interpret/eval_context.rs b/compiler/rustc_const_eval/src/interpret/eval_context.rs index d98d2107102..f3ebc3f51d8 100644 --- a/compiler/rustc_const_eval/src/interpret/eval_context.rs +++ b/compiler/rustc_const_eval/src/interpret/eval_context.rs @@ -221,9 +221,6 @@ impl<'tcx, Prov: Provenance> LocalState<'tcx, Prov> { /// Overwrite the local. If the local can be overwritten in place, return a reference /// to do so; otherwise return the `MemPlace` to consult instead. - /// - /// Note: Before calling this, call the `before_access_local_mut` machine hook! You may be - /// invalidating machine invariants otherwise! #[inline(always)] pub(super) fn access_mut(&mut self) -> InterpResult<'tcx, &mut Operand> { match &mut self.value { diff --git a/compiler/rustc_const_eval/src/interpret/machine.rs b/compiler/rustc_const_eval/src/interpret/machine.rs index 90a654a1229..5e6862ad475 100644 --- a/compiler/rustc_const_eval/src/interpret/machine.rs +++ b/compiler/rustc_const_eval/src/interpret/machine.rs @@ -260,24 +260,6 @@ pub trait Machine<'mir, 'tcx: 'mir>: Sized { F2::NAN } - /// Called before writing the specified `local` of the `frame`. - /// Since writing a ZST is not actually accessing memory or locals, this is never invoked - /// for ZST reads. - /// - /// Due to borrow checker trouble, we indicate the `frame` as an index rather than an `&mut - /// Frame`. - #[inline(always)] - fn before_access_local_mut<'a>( - _ecx: &'a mut InterpCx<'mir, 'tcx, Self>, - _frame: usize, - _local: mir::Local, - ) -> InterpResult<'tcx> - where - 'tcx: 'mir, - { - Ok(()) - } - /// Called before a basic block terminator is executed. #[inline] fn before_terminator(_ecx: &mut InterpCx<'mir, 'tcx, Self>) -> InterpResult<'tcx> { diff --git a/compiler/rustc_const_eval/src/interpret/place.rs b/compiler/rustc_const_eval/src/interpret/place.rs index 52a74f43405..7bd44634066 100644 --- a/compiler/rustc_const_eval/src/interpret/place.rs +++ b/compiler/rustc_const_eval/src/interpret/place.rs @@ -624,7 +624,6 @@ where dest.force_mplace(self)? } else { debug_assert_eq!(locals_addr, self.frame().locals_addr()); - M::before_access_local_mut(self, self.frame_idx(), local)?; match self.frame_mut().locals[local].access_mut()? { Operand::Immediate(local_val) => { // Local can be updated in-place. @@ -740,7 +739,6 @@ where dest.force_mplace(self)? } else { debug_assert_eq!(locals_addr, self.frame().locals_addr()); - M::before_access_local_mut(self, self.frame_idx(), local)?; match self.frame_mut().locals[local].access_mut()? { Operand::Immediate(local) => { *local = Immediate::Uninit; @@ -921,7 +919,6 @@ where let mplace = match place.place { Place::Local { local, offset, locals_addr } => { debug_assert_eq!(locals_addr, self.frame().locals_addr()); - M::before_access_local_mut(self, self.frame_idx(), local)?; let whole_local = match self.frame_mut().locals[local].access_mut()? { &mut Operand::Immediate(local_val) => { // We need to make an allocation. From 4497990dff8ce181fa12757eb4e786b85025ae61 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 9 Mar 2024 18:23:26 +0100 Subject: [PATCH 4/5] remove some frame parameters that are no longer needed --- .../rustc_const_eval/src/interpret/eval_context.rs | 4 ++-- compiler/rustc_const_eval/src/interpret/operand.rs | 14 +++++++------- compiler/rustc_const_eval/src/interpret/place.rs | 12 ++++++------ .../rustc_const_eval/src/interpret/projection.rs | 2 +- .../rustc_const_eval/src/interpret/terminator.rs | 2 +- src/tools/miri/src/helpers.rs | 2 +- 6 files changed, 18 insertions(+), 18 deletions(-) diff --git a/compiler/rustc_const_eval/src/interpret/eval_context.rs b/compiler/rustc_const_eval/src/interpret/eval_context.rs index f3ebc3f51d8..50e481030a3 100644 --- a/compiler/rustc_const_eval/src/interpret/eval_context.rs +++ b/compiler/rustc_const_eval/src/interpret/eval_context.rs @@ -648,7 +648,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } #[inline(always)] - pub fn layout_of_local( + pub(super) fn layout_of_local( &self, frame: &Frame<'mir, 'tcx, M::Provenance, M::FrameExtra>, local: mir::Local, @@ -899,7 +899,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // Copy return value. Must of course happen *before* we deallocate the locals. let copy_ret_result = if !unwinding { let op = self - .local_to_op(self.frame(), mir::RETURN_PLACE, None) + .local_to_op(mir::RETURN_PLACE, None) .expect("return place should always be live"); let dest = self.frame().return_place.clone(); let err = if self.stack().len() == 1 { diff --git a/compiler/rustc_const_eval/src/interpret/operand.rs b/compiler/rustc_const_eval/src/interpret/operand.rs index 48b0dad47ff..e49067a2f00 100644 --- a/compiler/rustc_const_eval/src/interpret/operand.rs +++ b/compiler/rustc_const_eval/src/interpret/operand.rs @@ -13,9 +13,9 @@ use rustc_middle::{mir, ty}; use rustc_target::abi::{self, Abi, HasDataLayout, Size}; use super::{ - alloc_range, from_known_layout, mir_assign_valid_types, CtfeProvenance, Frame, InterpCx, - InterpResult, MPlaceTy, Machine, MemPlace, MemPlaceMeta, OffsetMode, PlaceTy, Pointer, - Projectable, Provenance, Scalar, + alloc_range, from_known_layout, mir_assign_valid_types, CtfeProvenance, InterpCx, InterpResult, + MPlaceTy, Machine, MemPlace, MemPlaceMeta, OffsetMode, PlaceTy, Pointer, Projectable, + Provenance, Scalar, }; /// An `Immediate` represents a single immediate self-contained Rust value. @@ -633,17 +633,17 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } } - /// Read from a local. + /// Read from a local of the current frame. /// Will not access memory, instead an indirect `Operand` is returned. /// /// This is public because it is used by [priroda](https://github.com/oli-obk/priroda) to get an /// OpTy from a local. pub fn local_to_op( &self, - frame: &Frame<'mir, 'tcx, M::Provenance, M::FrameExtra>, local: mir::Local, layout: Option>, ) -> InterpResult<'tcx, OpTy<'tcx, M::Provenance>> { + let frame = self.frame(); let layout = self.layout_of_local(frame, local, layout)?; let op = *frame.locals[local].access()?; if matches!(op, Operand::Immediate(_)) { @@ -664,7 +664,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { Right((local, offset, locals_addr)) => { debug_assert!(place.layout.is_sized()); // only sized locals can ever be `Place::Local`. debug_assert_eq!(locals_addr, self.frame().locals_addr()); - let base = self.local_to_op(&self.frame(), local, None)?; + let base = self.local_to_op(local, None)?; Ok(match offset { Some(offset) => base.offset(offset, place.layout, self)?, None => { @@ -688,7 +688,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // here is not the entire place. let layout = if mir_place.projection.is_empty() { layout } else { None }; - let mut op = self.local_to_op(self.frame(), mir_place.local, layout)?; + let mut op = self.local_to_op(mir_place.local, layout)?; // Using `try_fold` turned out to be bad for performance, hence the loop. for elem in mir_place.projection.iter() { op = self.project(&op, elem)? diff --git a/compiler/rustc_const_eval/src/interpret/place.rs b/compiler/rustc_const_eval/src/interpret/place.rs index 7bd44634066..90ded6c09db 100644 --- a/compiler/rustc_const_eval/src/interpret/place.rs +++ b/compiler/rustc_const_eval/src/interpret/place.rs @@ -506,21 +506,21 @@ where Ok((mplace, len)) } + /// Turn a local in the current frame into a place. pub fn local_to_place( &self, - frame: usize, local: mir::Local, ) -> InterpResult<'tcx, PlaceTy<'tcx, M::Provenance>> { // Other parts of the system rely on `Place::Local` never being unsized. // So we eagerly check here if this local has an MPlace, and if yes we use it. - let frame_ref = &self.stack()[frame]; - let layout = self.layout_of_local(frame_ref, local, None)?; + let frame = self.frame(); + let layout = self.layout_of_local(frame, local, None)?; let place = if layout.is_sized() { // We can just always use the `Local` for sized values. - Place::Local { local, offset: None, locals_addr: frame_ref.locals_addr() } + Place::Local { local, offset: None, locals_addr: frame.locals_addr() } } else { // Unsized `Local` isn't okay (we cannot store the metadata). - match frame_ref.locals[local].access()? { + match frame.locals[local].access()? { Operand::Immediate(_) => bug!(), Operand::Indirect(mplace) => Place::Ptr(*mplace), } @@ -535,7 +535,7 @@ where &self, mir_place: mir::Place<'tcx>, ) -> InterpResult<'tcx, PlaceTy<'tcx, M::Provenance>> { - let mut place = self.local_to_place(self.frame_idx(), mir_place.local)?; + let mut place = self.local_to_place(mir_place.local)?; // Using `try_fold` turned out to be bad for performance, hence the loop. for elem in mir_place.projection.iter() { place = self.project(&place, elem)? diff --git a/compiler/rustc_const_eval/src/interpret/projection.rs b/compiler/rustc_const_eval/src/interpret/projection.rs index 7b68a37fdf3..5ff78f7b8c9 100644 --- a/compiler/rustc_const_eval/src/interpret/projection.rs +++ b/compiler/rustc_const_eval/src/interpret/projection.rs @@ -357,7 +357,7 @@ where Deref => self.deref_pointer(&base.to_op(self)?)?.into(), Index(local) => { let layout = self.layout_of(self.tcx.types.usize)?; - let n = self.local_to_op(self.frame(), local, Some(layout))?; + let n = self.local_to_op(local, Some(layout))?; let n = self.read_target_usize(&n)?; self.project_index(base, n)? } diff --git a/compiler/rustc_const_eval/src/interpret/terminator.rs b/compiler/rustc_const_eval/src/interpret/terminator.rs index bafb8cb0018..82fb7ff1840 100644 --- a/compiler/rustc_const_eval/src/interpret/terminator.rs +++ b/compiler/rustc_const_eval/src/interpret/terminator.rs @@ -631,7 +631,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { body.args_iter() .map(|local| ( local, - self.layout_of_local(self.frame(), local, None).unwrap().ty + self.layout_of_local(self.frame(), local, None).unwrap().ty, )) .collect::>() ); diff --git a/src/tools/miri/src/helpers.rs b/src/tools/miri/src/helpers.rs index 9e4b5fe8ad7..ba9239085ce 100644 --- a/src/tools/miri/src/helpers.rs +++ b/src/tools/miri/src/helpers.rs @@ -411,7 +411,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { .ok_or_else(|| err_ub_format!("callee has fewer arguments than expected"))?; // Make the local live, and insert the initial value. this.storage_live(local)?; - let callee_arg = this.local_to_place(this.frame_idx(), local)?; + let callee_arg = this.local_to_place(local)?; this.write_immediate(*arg, &callee_arg)?; } if callee_args.next().is_some() { From c3342b41b52aa4f8a4497a696ee783952fca36da Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 9 Mar 2024 18:28:29 +0100 Subject: [PATCH 5/5] remove unnecessary frame parameter from after_local_allocated --- compiler/rustc_const_eval/src/interpret/eval_context.rs | 5 +++-- compiler/rustc_const_eval/src/interpret/machine.rs | 1 - compiler/rustc_const_eval/src/interpret/place.rs | 2 +- src/tools/miri/src/machine.rs | 3 +-- 4 files changed, 5 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_const_eval/src/interpret/eval_context.rs b/compiler/rustc_const_eval/src/interpret/eval_context.rs index 50e481030a3..7526acf1454 100644 --- a/compiler/rustc_const_eval/src/interpret/eval_context.rs +++ b/compiler/rustc_const_eval/src/interpret/eval_context.rs @@ -1,5 +1,4 @@ use std::cell::Cell; -use std::ptr; use std::{fmt, mem}; use either::{Either, Left, Right}; @@ -278,9 +277,11 @@ impl<'mir, 'tcx, Prov: Provenance, Extra> Frame<'mir, 'tcx, Prov, Extra> { }) } + /// Returns the address of the buffer where the locals are stored. This is used by `Place` as a + /// sanity check to detect bugs where we mix up which stack frame a place refers to. #[inline(always)] pub(super) fn locals_addr(&self) -> usize { - ptr::addr_of!(self.locals).addr() + self.locals.raw.as_ptr().addr() } } diff --git a/compiler/rustc_const_eval/src/interpret/machine.rs b/compiler/rustc_const_eval/src/interpret/machine.rs index 5e6862ad475..04de5162250 100644 --- a/compiler/rustc_const_eval/src/interpret/machine.rs +++ b/compiler/rustc_const_eval/src/interpret/machine.rs @@ -508,7 +508,6 @@ pub trait Machine<'mir, 'tcx: 'mir>: Sized { #[inline(always)] fn after_local_allocated( _ecx: &mut InterpCx<'mir, 'tcx, Self>, - _frame: usize, _local: mir::Local, _mplace: &MPlaceTy<'tcx, Self::Provenance>, ) -> InterpResult<'tcx> { diff --git a/compiler/rustc_const_eval/src/interpret/place.rs b/compiler/rustc_const_eval/src/interpret/place.rs index 90ded6c09db..1a2f1194f89 100644 --- a/compiler/rustc_const_eval/src/interpret/place.rs +++ b/compiler/rustc_const_eval/src/interpret/place.rs @@ -940,7 +940,7 @@ where mplace.mplace, )?; } - M::after_local_allocated(self, self.frame_idx(), local, &mplace)?; + M::after_local_allocated(self, local, &mplace)?; // Now we can call `access_mut` again, asserting it goes well, and actually // overwrite things. This points to the entire allocation, not just the part // the place refers to, i.e. we do this before we apply `offset`. diff --git a/src/tools/miri/src/machine.rs b/src/tools/miri/src/machine.rs index 19d02c6f746..20deeb22887 100644 --- a/src/tools/miri/src/machine.rs +++ b/src/tools/miri/src/machine.rs @@ -1473,14 +1473,13 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { fn after_local_allocated( ecx: &mut InterpCx<'mir, 'tcx, Self>, - frame: usize, local: mir::Local, mplace: &MPlaceTy<'tcx, Provenance>, ) -> InterpResult<'tcx> { let Some(Provenance::Concrete { alloc_id, .. }) = mplace.ptr().provenance else { panic!("after_local_allocated should only be called on fresh allocations"); }; - let local_decl = &ecx.active_thread_stack()[frame].body.local_decls[local]; + let local_decl = &ecx.frame().body.local_decls[local]; let span = local_decl.source_info.span; ecx.machine.allocation_spans.borrow_mut().insert(alloc_id, (span, None)); Ok(())