interpret: fix after_local_read handling

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(())
     }
 

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<AllocId, (Size, Align)>,
 
-    /// 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<bool>,
+    ghost_mode: Cell<bool>,
 }
 
 /// 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,15 +768,15 @@ 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)?;
             }
         }
 
         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<AllocRefMut<'a, 'tcx, M::Provenance, M::AllocExtra, M::Bytes>>>
     {
         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<R>(&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<R>(&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<R>(&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<R>(&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

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<TyAndLayout<'tcx>>,
     ) -> 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<TyAndLayout<'tcx>>,
     ) -> 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 })
     }
 

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<mir::Local, LocalState<'tcx, Prov>>,
+    pub(super) locals: IndexVec<mir::Local, LocalState<'tcx, Prov>>,
 
     /// 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<Either<(Pointer<Option<Prov>>, MemPlaceMeta<Prov>), Immediate<Prov>>> {
         match self.value {
@@ -293,6 +294,10 @@ impl<'tcx, Prov: Provenance, Extra> Frame<'tcx, Prov, Extra> {
         self.return_cont
     }
 
+    pub fn locals(&self) -> &IndexVec<mir::Local, LocalState<'tcx, Prov>> {
+        &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))

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.

src/tools/miri/priroda/src/debugger.rs

@@ -890,13 +890,14 @@ impl<'tcx> PrirodaContext<'tcx> {
             value: "<unsupported>".to_string(),
         };
 
-        match &frame.locals[local].as_mplace_or_imm() {
+        match &frame.locals()[local].as_mplace_or_imm_ghost() {
             None => {
                 local_desc.value = "<dead>".to_string();
             }
             Some(Either::Right(Uninit)) => local_desc.value = "<uninit>".to_string(),
 
             Some(Either::Left(_) | Either::Right(_)) => {
+                // FIXME: This seems wrong, it ignore the frame.
                 let op = self
                     .ecx
                     .local_to_op(local, None)

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() {

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);

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);
         }
     }

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);
         }