diff --git a/compiler/rustc_const_eval/src/interpret/machine.rs b/compiler/rustc_const_eval/src/interpret/machine.rs index 0528fee35031c..8d9b26cd18423 100644 --- a/compiler/rustc_const_eval/src/interpret/machine.rs +++ b/compiler/rustc_const_eval/src/interpret/machine.rs @@ -621,14 +621,10 @@ pub trait Machine<'tcx>: Sized { interp_ok(ReturnAction::Normal) } - /// Called immediately after an "immediate" local variable is read in a given frame + /// Called immediately after an "immediate" local variable is read /// (i.e., this is called for reads that do not end up accessing addressable memory). #[inline(always)] - fn after_local_read( - _ecx: &InterpCx<'tcx, Self>, - _frame: &Frame<'tcx, Self::Provenance, Self::FrameExtra>, - _local: mir::Local, - ) -> InterpResult<'tcx> { + fn after_local_read(_ecx: &InterpCx<'tcx, Self>, _local: mir::Local) -> InterpResult<'tcx> { interp_ok(()) } diff --git a/compiler/rustc_const_eval/src/interpret/memory.rs b/compiler/rustc_const_eval/src/interpret/memory.rs index 96152737d3d1e..03a621812eb57 100644 --- a/compiler/rustc_const_eval/src/interpret/memory.rs +++ b/compiler/rustc_const_eval/src/interpret/memory.rs @@ -135,10 +135,10 @@ pub struct Memory<'tcx, M: Machine<'tcx>> { // FIXME: this should not be public, but interning currently needs access to it pub(super) dead_alloc_map: FxIndexMap, - /// This stores whether we are currently doing reads purely for the purpose of validation. - /// Those reads do not trigger the machine's hooks for memory reads. + /// This stores whether we are currently doing reads/writes that aren't "real". + /// Those accesses do not trigger the machine's hooks. /// Needless to say, this must only be set with great care! - validation_in_progress: Cell, + ghost_mode: Cell, } /// A reference to some allocation that was already bounds-checked for the given region @@ -166,7 +166,7 @@ impl<'tcx, M: Machine<'tcx>> Memory<'tcx, M> { extra_fn_ptr_map: FxIndexMap::default(), va_list_map: FxIndexMap::default(), dead_alloc_map: FxIndexMap::default(), - validation_in_progress: Cell::new(false), + ghost_mode: Cell::new(false), } } @@ -768,7 +768,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { // We want to call the hook on *all* accesses that involve an AllocId, including zero-sized // accesses. That means we cannot rely on the closure above or the `Some` branch below. We // do this after `check_and_deref_ptr` to ensure some basic sanity has already been checked. - if !self.memory.validation_in_progress.get() { + if !self.memory.ghost_mode.get() { if let Ok((alloc_id, ..)) = self.ptr_try_get_alloc_id(ptr, size_i64) { M::before_alloc_access(self.tcx, &self.machine, alloc_id)?; } @@ -776,7 +776,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { if let Some((alloc_id, offset, prov, alloc)) = ptr_and_alloc { let range = alloc_range(offset, size); - if !self.memory.validation_in_progress.get() { + if !self.memory.ghost_mode.get() { M::before_memory_read( self.tcx, &self.machine, @@ -856,7 +856,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { ) -> InterpResult<'tcx, Option>> { let tcx = self.tcx; - let validation_in_progress = self.memory.validation_in_progress.get(); + let validation_in_progress = self.memory.ghost_mode.get(); let size_i64 = i64::try_from(size.bytes()).unwrap(); // it would be an error to even ask for more than isize::MAX bytes let ptr_and_alloc = Self::check_and_deref_ptr( @@ -1204,48 +1204,44 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { result } - /// Runs the closure in "validation" mode, which means the machine's memory read hooks will be + /// Runs the closure in "ghost" mode, which means the machine's memory read hooks will be /// suppressed. Needless to say, this must only be set with great care! Cannot be nested. /// /// We do this so Miri's allocation access tracking does not show the validation - /// reads as spurious accesses. - pub fn run_for_validation_mut(&mut self, f: impl FnOnce(&mut Self) -> R) -> R { + /// reads as spurious accesses as those aren't "real" reads. Also useful for debuggers + /// that want to just display the Miri machine state. + pub fn ghost_run_mut(&mut self, f: impl FnOnce(&mut Self) -> R) -> R { // This deliberately uses `==` on `bool` to follow the pattern // `assert!(val.replace(new) == old)`. - assert!( - self.memory.validation_in_progress.replace(true) == false, - "`validation_in_progress` was already set" - ); + assert!(self.memory.ghost_mode.replace(true) == false, "`ghost_mode` was already set"); let res = f(self); assert!( - self.memory.validation_in_progress.replace(false) == true, - "`validation_in_progress` was unset by someone else" + self.memory.ghost_mode.replace(false) == true, + "`ghost_mode` was unset by someone else" ); res } - /// Runs the closure in "validation" mode, which means the machine's memory read hooks will be + /// Runs the closure in "ghost" mode, which means the machine's memory read hooks will be /// suppressed. Needless to say, this must only be set with great care! Cannot be nested. /// /// We do this so Miri's allocation access tracking does not show the validation - /// reads as spurious accesses. - pub fn run_for_validation_ref(&self, f: impl FnOnce(&Self) -> R) -> R { + /// reads as spurious accesses as those aren't "real" reads. Also useful for debuggers + /// that want to just display the Miri machine state. + pub fn ghost_run(&self, f: impl FnOnce(&Self) -> R) -> R { // This deliberately uses `==` on `bool` to follow the pattern // `assert!(val.replace(new) == old)`. - assert!( - self.memory.validation_in_progress.replace(true) == false, - "`validation_in_progress` was already set" - ); + assert!(self.memory.ghost_mode.replace(true) == false, "`ghost_mode` was already set"); let res = f(self); assert!( - self.memory.validation_in_progress.replace(false) == true, - "`validation_in_progress` was unset by someone else" + self.memory.ghost_mode.replace(false) == true, + "`ghost_mode` was unset by someone else" ); res } pub(super) fn validation_in_progress(&self) -> bool { - self.memory.validation_in_progress.get() + self.memory.ghost_mode.get() } } @@ -1516,7 +1512,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { }; let src_alloc = self.get_alloc_raw(src_alloc_id)?; let src_range = alloc_range(src_offset, size); - assert!(!self.memory.validation_in_progress.get(), "we can't be copying during validation"); + assert!(!self.memory.ghost_mode.get(), "we can't be copying during validation"); // Trigger read hook. // For the overlapping case, it is crucial that we trigger the read hook diff --git a/compiler/rustc_const_eval/src/interpret/operand.rs b/compiler/rustc_const_eval/src/interpret/operand.rs index 4bafea98cd569..81f26cd7e5479 100644 --- a/compiler/rustc_const_eval/src/interpret/operand.rs +++ b/compiler/rustc_const_eval/src/interpret/operand.rs @@ -722,32 +722,38 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { interp_ok(s) } - /// Read from a local of the current frame. Convenience method for [`InterpCx::local_at_frame_to_op`]. + /// Read from a local of a current frame. + /// Will not access memory, instead an indirect `Operand` is returned. pub fn local_to_op( &self, local: mir::Local, layout: Option>, ) -> InterpResult<'tcx, OpTy<'tcx, M::Provenance>> { - self.local_at_frame_to_op(self.frame(), local, layout) + let frame = self.frame(); + let layout = self.layout_of_local(frame, local, layout)?; + let op = *frame.locals[local].access()?; + if matches!(op, Operand::Immediate(_)) { + assert!(!layout.is_unsized()); + if !self.validation_in_progress() { + M::after_local_read(self, local)?; + } + } + interp_ok(OpTy { op, layout }) } - /// Read from a local of a given frame. - /// Will not access memory, instead an indirect `Operand` is returned. + /// Tools like Priroda and [Aquascope](https://github.com/cognitive-engineering-lab/aquascope/) + /// need to access any local without triggering any access hook, since these are not actual + /// AM-level accesses. Do not call this from inside the interpreter! /// - /// This is public because it is used by [Aquascope](https://github.com/cognitive-engineering-lab/aquascope/) - /// to get an OpTy from a local. - pub fn local_at_frame_to_op( + /// Remember to use `ghost_run` when accessing memory for such purposes, to suppress + /// the access hooks for that as well. + pub fn ghost_local_in_frame_to_op( &self, frame: &Frame<'tcx, M::Provenance, M::FrameExtra>, local: mir::Local, - layout: Option>, ) -> InterpResult<'tcx, OpTy<'tcx, M::Provenance>> { - let layout = self.layout_of_local(frame, local, layout)?; + let layout = self.layout_of_local(frame, local, None)?; let op = *frame.locals[local].access()?; - if matches!(op, Operand::Immediate(_)) { - assert!(!layout.is_unsized()); - } - M::after_local_read(self, frame, local)?; interp_ok(OpTy { op, layout }) } diff --git a/compiler/rustc_const_eval/src/interpret/stack.rs b/compiler/rustc_const_eval/src/interpret/stack.rs index d291f1f6fdcbc..5686413915ca5 100644 --- a/compiler/rustc_const_eval/src/interpret/stack.rs +++ b/compiler/rustc_const_eval/src/interpret/stack.rs @@ -90,7 +90,7 @@ pub struct Frame<'tcx, Prov: Provenance = CtfeProvenance, Extra = ()> { /// can either directly contain `Scalar` or refer to some part of an `Allocation`. /// /// Do *not* access this directly; always go through the machine hook! - pub locals: IndexVec>, + pub(super) locals: IndexVec>, /// The complete variable argument list of this frame. Its elements must be dropped when the /// frame is popped. @@ -168,8 +168,9 @@ impl<'tcx, Prov: Provenance> LocalState<'tcx, Prov> { /// This is a hack because Miri needs a way to visit all the provenance in a `LocalState` /// without having a layout or `TyCtxt` available, and we want to keep the `Operand` type - /// private. - pub fn as_mplace_or_imm( + /// private. Does not count as a read of the local for the AM! It's a "ghost" read, like for + /// validation or similar purposes. + pub fn as_mplace_or_imm_ghost( &self, ) -> Option>, MemPlaceMeta), Immediate>> { match self.value { @@ -293,6 +294,10 @@ impl<'tcx, Prov: Provenance, Extra> Frame<'tcx, Prov, Extra> { self.return_cont } + pub fn locals(&self) -> &IndexVec> { + &self.locals + } + /// Return the `SourceInfo` of the current instruction. pub fn current_source_info(&self) -> Option<&mir::SourceInfo> { self.loc.left().map(|loc| self.body.source_info(loc)) diff --git a/compiler/rustc_const_eval/src/interpret/validity.rs b/compiler/rustc_const_eval/src/interpret/validity.rs index 5f0799c70aa0b..2cd4caf5ba250 100644 --- a/compiler/rustc_const_eval/src/interpret/validity.rs +++ b/compiler/rustc_const_eval/src/interpret/validity.rs @@ -1611,7 +1611,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { trace!("validate_place_internal: {:?}, {:?}", *val, val.layout.ty); // Run the visitor. - self.run_for_validation_mut(|ecx| { + self.ghost_run_mut(|ecx| { let reset_padding = reset_provenance_and_padding && { // Check if `val` is actually stored in memory. If not, padding is not even // represented and we need not reset it. diff --git a/src/tools/miri/priroda/src/debugger.rs b/src/tools/miri/priroda/src/debugger.rs index 4c4c811c01358..a4766e53314cb 100644 --- a/src/tools/miri/priroda/src/debugger.rs +++ b/src/tools/miri/priroda/src/debugger.rs @@ -890,13 +890,14 @@ impl<'tcx> PrirodaContext<'tcx> { value: "".to_string(), }; - match &frame.locals[local].as_mplace_or_imm() { + match &frame.locals()[local].as_mplace_or_imm_ghost() { None => { local_desc.value = "".to_string(); } Some(Either::Right(Uninit)) => local_desc.value = "".to_string(), Some(Either::Left(_) | Either::Right(_)) => { + // FIXME: This seems wrong, it ignore the frame. let op = self .ecx .local_to_op(local, None) diff --git a/src/tools/miri/src/concurrency/data_race.rs b/src/tools/miri/src/concurrency/data_race.rs index a1102a9642439..c086f71c2dcaa 100644 --- a/src/tools/miri/src/concurrency/data_race.rs +++ b/src/tools/miri/src/concurrency/data_race.rs @@ -779,7 +779,7 @@ pub trait EvalContextExt<'tcx>: MiriInterpCxExt<'tcx> { // Only metadata on the location itself is used. if let Some(genmc_ctx) = this.machine.data_race.as_genmc_ref() { - let old_val = this.run_for_validation_ref(|this| this.read_scalar(place)).discard_err(); + let old_val = this.ghost_run(|this| this.read_scalar(place)).discard_err(); return genmc_ctx.atomic_load( this, place.ptr().addr(), @@ -811,7 +811,7 @@ pub trait EvalContextExt<'tcx>: MiriInterpCxExt<'tcx> { // Read the previous value so we can put it in the store buffer later. // Both GenMC and Miri need this. This value is nonsense if there are concurrent writes // but the code consuming the value is aware of that. - let old_val = this.run_for_validation_ref(|this| this.read_scalar(dest)).discard_err(); + let old_val = this.ghost_run(|this| this.read_scalar(dest)).discard_err(); // Inform GenMC about the atomic store. if let Some(genmc_ctx) = this.machine.data_race.as_genmc_ref() { diff --git a/src/tools/miri/src/concurrency/thread.rs b/src/tools/miri/src/concurrency/thread.rs index 5b3b56760a6c4..f5cfc62ad659a 100644 --- a/src/tools/miri/src/concurrency/thread.rs +++ b/src/tools/miri/src/concurrency/thread.rs @@ -345,8 +345,8 @@ impl VisitProvenance for Thread<'_> { impl VisitProvenance for Frame<'_, Provenance, FrameExtra<'_>> { fn visit_provenance(&self, visit: &mut VisitWith<'_>) { let return_place = self.return_place(); + let locals = self.locals(); let Frame { - locals, extra, // There are some private fields we cannot access; they contain no tags. .. @@ -356,7 +356,8 @@ impl VisitProvenance for Frame<'_, Provenance, FrameExtra<'_>> { return_place.visit_provenance(visit); // Locals. for local in locals.iter() { - match local.as_mplace_or_imm() { + // We only need the provenance so it's good for this to not be a real read. + match local.as_mplace_or_imm_ghost() { None => {} Some(Either::Left((ptr, meta))) => { ptr.visit_provenance(visit); diff --git a/src/tools/miri/src/diagnostics.rs b/src/tools/miri/src/diagnostics.rs index 4a0869da63bff..1d88a792734f9 100644 --- a/src/tools/miri/src/diagnostics.rs +++ b/src/tools/miri/src/diagnostics.rs @@ -500,7 +500,7 @@ pub fn report_result<'tcx>( trace!("-------------------"); trace!("Frame {}", i); trace!(" return: {:?}", frame.return_place()); - for (i, local) in frame.locals.iter().enumerate() { + for (i, local) in frame.locals().iter().enumerate() { trace!(" local {}: {:?}", i, local); } } diff --git a/src/tools/miri/src/machine.rs b/src/tools/miri/src/machine.rs index 361ebef73b357..076738a5db08e 100644 --- a/src/tools/miri/src/machine.rs +++ b/src/tools/miri/src/machine.rs @@ -2000,12 +2000,8 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> { res } - fn after_local_read( - ecx: &InterpCx<'tcx, Self>, - frame: &Frame<'tcx, Provenance, FrameExtra<'tcx>>, - local: mir::Local, - ) -> InterpResult<'tcx> { - if let Some(data_race) = &frame.extra.data_race { + fn after_local_read(ecx: &InterpCx<'tcx, Self>, local: mir::Local) -> InterpResult<'tcx> { + if let Some(data_race) = &ecx.frame().extra.data_race { let _trace = enter_trace_span!(data_race::after_local_read); data_race.local_read(local, &ecx.machine); }