Create a special ValidationMachine that just outright forbids almost every machine hook

Author: oli-obk

compiler/rustc_const_eval/src/const_eval/eval_queries.rs

@@ -21,7 +21,7 @@ use crate::interpret::{
     InternKind, InterpCx, InterpError, InterpResult, MPlaceTy, MemoryKind, OpTy, RefTracking,
     StackPopCleanup,
 };
-use crate::interpret::{eval_nullary_intrinsic, Machine};
+use crate::interpret::{eval_nullary_intrinsic, Machine, ValidationMachine};
 
 // Returns a pointer to where the result lives
 #[instrument(level = "trace", skip(ecx, body), ret)]
@@ -307,7 +307,9 @@ impl<'tcx> InterpretationResult<'tcx> for ConstAlloc<'tcx> {
         ecx: InterpCx<'mir, 'tcx, CompileTimeInterpreter<'mir, 'tcx>>,
         cid: GlobalId<'tcx>,
     ) -> Result<Self, ErrorHandled> {
-        const_validate_mplace(&ecx, mplace, cid)?;
+        let validate_ecx =
+            InterpCx::new(ecx.tcx.tcx, ecx.tcx.span, ecx.param_env, ValidationMachine(&ecx));
+        const_validate_mplace(&validate_ecx, mplace, cid)?;
         Ok(ConstAlloc {
             alloc_id: mplace.ptr().provenance.unwrap().alloc_id(),
             ty: mplace.layout.ty,
@@ -406,7 +408,7 @@ fn eval_in_interpreter<'mir, 'tcx, R: InterpretationResult<'tcx>>(
 
 #[inline(always)]
 pub fn const_validate_mplace<'mir, 'tcx>(
-    ecx: &InterpCx<'mir, 'tcx, CompileTimeInterpreter<'mir, 'tcx>>,
+    ecx: &InterpCx<'mir, 'tcx, ValidationMachine<'_, 'mir, 'tcx>>,
     mplace: &MPlaceTy<'tcx>,
     cid: GlobalId<'tcx>,
 ) -> Result<(), ErrorHandled> {

compiler/rustc_const_eval/src/interpret/machine.rs

@@ -391,8 +391,6 @@ pub trait Machine<'mir, 'tcx: 'mir>: Sized {
 
     /// Hook for performing extra checks on a memory read access.
     ///
-    /// This will *not* be called during validation!
-    ///
     /// Takes read-only access to the allocation so we can keep all the memory read
     /// operations take `&self`. Use a `RefCell` in `AllocExtra` if you
     /// need to mutate.
@@ -412,8 +410,6 @@ pub trait Machine<'mir, 'tcx: 'mir>: Sized {
     /// Hook for performing extra checks on any memory read access,
     /// that involves an allocation, even ZST reads.
     ///
-    /// This will *not* be called during validation!
-    ///
     /// Used to prevent statics from self-initializing by reading from their own memory
     /// as it is being initialized.
     fn before_alloc_read(

compiler/rustc_const_eval/src/interpret/memory.rs

@@ -8,7 +8,6 @@
 
 use std::assert_matches::assert_matches;
 use std::borrow::Cow;
-use std::cell::Cell;
 use std::collections::VecDeque;
 use std::fmt;
 use std::ptr;
@@ -112,11 +111,6 @@ pub struct Memory<'mir, 'tcx, M: Machine<'mir, 'tcx>> {
     /// that do not exist any more.
     // 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.
-    /// Needless to say, this must only be set with great care!
-    validation_in_progress: Cell<bool>,
 }
 
 /// A reference to some allocation that was already bounds-checked for the given region
@@ -143,7 +137,6 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
             alloc_map: M::MemoryMap::default(),
             extra_fn_ptr_map: FxIndexMap::default(),
             dead_alloc_map: FxIndexMap::default(),
-            validation_in_progress: Cell::new(false),
         }
     }
 
@@ -631,28 +624,19 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
             size,
             CheckInAllocMsg::MemoryAccessTest,
             |alloc_id, offset, prov| {
-                if !self.memory.validation_in_progress.get() {
-                    // We want to call the hook on *all* accesses that involve an AllocId,
-                    // including zero-sized accesses. That means we have to do it here
-                    // rather than below in the `Some` branch.
-                    M::before_alloc_read(self, alloc_id)?;
-                }
+                // We want to call the hook on *all* accesses that involve an AllocId,
+                // including zero-sized accesses. That means we have to do it here
+                // rather than below in the `Some` branch.
+                M::before_alloc_read(self, alloc_id)?;
                 let alloc = self.get_alloc_raw(alloc_id)?;
                 Ok((alloc.size(), alloc.align, (alloc_id, offset, prov, alloc)))
             },
         )?;
 
         if let Some((alloc_id, offset, prov, alloc)) = ptr_and_alloc {
             let range = alloc_range(offset, size);
-            if !self.memory.validation_in_progress.get() {
-                M::before_memory_read(
-                    self.tcx,
-                    &self.machine,
-                    &alloc.extra,
-                    (alloc_id, prov),
-                    range,
-                )?;
-            }
+            M::before_memory_read(self.tcx, &self.machine, &alloc.extra, (alloc_id, prov), range)?;
+
             Ok(Some(AllocRef { alloc, range, tcx: *self.tcx, alloc_id }))
         } else {
             Ok(None)
@@ -926,21 +910,6 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
             }
         })
     }
-
-    /// Runs the close in "validation" 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.
-    pub(super) fn run_for_validation<R>(&self, f: impl FnOnce() -> R) -> R {
-        assert!(
-            self.memory.validation_in_progress.replace(true) == false,
-            "`validation_in_progress` was already set"
-        );
-        let res = f();
-        assert!(
-            self.memory.validation_in_progress.replace(false) == true,
-            "`validation_in_progress` was unset by someone else"
-        );
-        res
-    }
 }
 
 #[doc(hidden)]
@@ -1186,7 +1155,6 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, '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");
         M::before_memory_read(
             tcx,
             &self.machine,

compiler/rustc_const_eval/src/interpret/mod.rs

@@ -30,7 +30,7 @@ pub use self::operand::{ImmTy, Immediate, OpTy, Readable};
 pub use self::place::{MPlaceTy, MemPlaceMeta, PlaceTy, Writeable};
 pub use self::projection::{OffsetMode, Projectable};
 pub use self::terminator::FnArg;
-pub use self::validity::{CtfeValidationMode, RefTracking};
+pub use self::validity::{CtfeValidationMode, RefTracking, ValidationMachine};
 pub use self::visitor::ValueVisitor;
 
 use self::{

compiler/rustc_const_eval/src/interpret/util.rs

@@ -9,7 +9,7 @@ use rustc_middle::ty::{
 use rustc_span::def_id::DefId;
 use std::ops::ControlFlow;
 
-use super::{InterpCx, MPlaceTy};
+use super::{InterpCx, MPlaceTy, ValidationMachine};
 
 /// Checks whether a type contains generic parameters which must be instantiated.
 ///
@@ -87,6 +87,10 @@ impl<'tcx> InterpretationResult<'tcx> for mir::interpret::ConstAllocation<'tcx>
         mut ecx: InterpCx<'mir, 'tcx, CompileTimeInterpreter<'mir, 'tcx>>,
         cid: GlobalId<'tcx>,
     ) -> Result<Self, ErrorHandled> {
+        let memory = std::mem::take(&mut ecx.memory.alloc_map);
+        let mut ecx =
+            InterpCx::new(ecx.tcx.tcx, ecx.tcx.span, ecx.param_env, ValidationMachine(&ecx));
+        ecx.memory.alloc_map = memory;
         crate::const_eval::const_validate_mplace(&ecx, mplace, cid)?;
         let alloc_id = mplace.ptr().provenance.unwrap().alloc_id();
         let alloc = ecx.memory.alloc_map.swap_remove(&alloc_id).unwrap().1;

compiler/rustc_const_eval/src/interpret/validity.rs

@@ -10,19 +10,22 @@ use std::num::NonZero;
 use either::{Left, Right};
 
 use hir::def::DefKind;
+use hir::def_id::DefId;
 use rustc_ast::Mutability;
 use rustc_data_structures::fx::FxHashSet;
 use rustc_hir as hir;
 use rustc_middle::mir::interpret::{
     ExpectedKind, InterpError, InvalidMetaKind, Misalignment, PointerKind, Provenance,
     ValidationErrorInfo, ValidationErrorKind, ValidationErrorKind::*,
 };
-use rustc_middle::ty;
+use rustc_middle::query::TyCtxtAt;
 use rustc_middle::ty::layout::{LayoutOf, TyAndLayout};
+use rustc_middle::{mir, ty};
 use rustc_span::symbol::{sym, Symbol};
 use rustc_target::abi::{
     Abi, FieldIdx, Scalar as ScalarAbi, Size, VariantIdx, Variants, WrappingRange,
 };
+use rustc_target::spec::abi::Abi as CallAbi;
 
 use std::hash::Hash;
 
@@ -37,6 +40,7 @@ use super::InterpError::UndefinedBehavior as Ub;
 use super::InterpError::Unsupported as Unsup;
 use super::UndefinedBehaviorInfo::*;
 use super::UnsupportedOpInfo::*;
+use super::{FnArg, Frame};
 
 macro_rules! throw_validation_failure {
     ($where:expr, $kind: expr) => {{
@@ -967,7 +971,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         let mut visitor = ValidityVisitor { path, ref_tracking, ctfe_mode, ecx: self };
 
         // Run it.
-        match self.run_for_validation(|| visitor.visit_value(op)) {
+        match visitor.visit_value(op) {
             Ok(()) => Ok(()),
             // Pass through validation failures and "invalid program" issues.
             Err(err)
@@ -1022,3 +1026,154 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         self.validate_operand_internal(op, vec![], None, None)
     }
 }
+
+pub struct ValidationMachine<'a, 'mir, 'tcx>(
+    pub &'a InterpCx<'mir, 'tcx, crate::const_eval::CompileTimeInterpreter<'mir, 'tcx>>,
+);
+
+impl<'a, 'mir, 'tcx> std::ops::Deref for ValidationMachine<'a, 'mir, 'tcx> {
+    type Target = crate::const_eval::CompileTimeInterpreter<'mir, 'tcx>;
+
+    fn deref(&self) -> &Self::Target {
+        &self.0.machine
+    }
+}
+
+impl<'a, 'mir, 'tcx> Machine<'mir, 'tcx> for ValidationMachine<'a, 'mir, 'tcx> {
+    super::compile_time_machine!(<'mir, 'tcx>);
+
+    type MemoryKind = crate::const_eval::MemoryKind;
+
+    const PANIC_ON_ALLOC_FAIL: bool = false; // will be raised as a proper error
+
+    #[inline(always)]
+    fn enforce_alignment(ecx: &InterpCx<'mir, 'tcx, Self>) -> bool {
+        Machine::enforce_alignment(ecx.machine.0)
+    }
+
+    #[inline(always)]
+    fn enforce_validity(ecx: &InterpCx<'mir, 'tcx, Self>, layout: TyAndLayout<'tcx>) -> bool {
+        ecx.tcx.sess.opts.unstable_opts.extra_const_ub_checks || layout.abi.is_uninhabited()
+    }
+
+    fn load_mir(
+        _ecx: &InterpCx<'mir, 'tcx, Self>,
+        instance: ty::InstanceDef<'tcx>,
+    ) -> InterpResult<'tcx, &'tcx mir::Body<'tcx>> {
+        unreachable!("validation tried to load mir of {instance:?}")
+    }
+
+    fn find_mir_or_eval_fn(
+        _ecx: &mut InterpCx<'mir, 'tcx, Self>,
+        orig_instance: ty::Instance<'tcx>,
+        _abi: CallAbi,
+        _args: &[FnArg<'tcx>],
+        _dest: &MPlaceTy<'tcx>,
+        _ret: Option<mir::BasicBlock>,
+        _unwind: mir::UnwindAction, // unwinding is not supported in consts
+    ) -> InterpResult<'tcx, Option<(&'mir mir::Body<'tcx>, ty::Instance<'tcx>)>> {
+        unreachable!("validation tried to evaluate mir of {orig_instance:?}")
+    }
+
+    fn panic_nounwind(_ecx: &mut InterpCx<'mir, 'tcx, Self>, msg: &str) -> InterpResult<'tcx> {
+        unreachable!("validation tried to panic: {msg}")
+    }
+
+    fn call_intrinsic(
+        _ecx: &mut InterpCx<'mir, 'tcx, Self>,
+        instance: ty::Instance<'tcx>,
+        _args: &[OpTy<'tcx>],
+        _dest: &MPlaceTy<'tcx, Self::Provenance>,
+        _target: Option<mir::BasicBlock>,
+        _unwind: mir::UnwindAction,
+    ) -> InterpResult<'tcx> {
+        unreachable!("validation tried to evaluate intrinsic {instance:?}")
+    }
+
+    fn assert_panic(
+        _ecx: &mut InterpCx<'mir, 'tcx, Self>,
+        msg: &mir::AssertMessage<'tcx>,
+        _unwind: mir::UnwindAction,
+    ) -> InterpResult<'tcx> {
+        unreachable!("validation tried to panic: {msg:?}")
+    }
+
+    fn binary_ptr_op(
+        _ecx: &InterpCx<'mir, 'tcx, Self>,
+        _bin_op: mir::BinOp,
+        _left: &ImmTy<'tcx>,
+        _right: &ImmTy<'tcx>,
+    ) -> InterpResult<'tcx, (ImmTy<'tcx>, bool)> {
+        unreachable!("validation does not do binary pointer ops");
+    }
+
+    fn increment_const_eval_counter(_ecx: &mut InterpCx<'mir, 'tcx, Self>) -> InterpResult<'tcx> {
+        unreachable!("validation does not interpret statements")
+    }
+
+    #[inline(always)]
+    fn expose_ptr(_ecx: &mut InterpCx<'mir, 'tcx, Self>, _ptr: Pointer) -> InterpResult<'tcx> {
+        unreachable!("validation does not exposing pointers")
+    }
+
+    #[inline(always)]
+    fn init_frame_extra(
+        _ecx: &mut InterpCx<'mir, 'tcx, Self>,
+        _frame: Frame<'mir, 'tcx>,
+    ) -> InterpResult<'tcx, Frame<'mir, 'tcx>> {
+        unreachable!("validation does not create stack frames")
+    }
+
+    #[inline(always)]
+    fn stack<'b>(
+        ecx: &'b InterpCx<'mir, 'tcx, Self>,
+    ) -> &'b [Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra>] {
+        ecx.machine.0.stack()
+    }
+
+    #[inline(always)]
+    fn stack_mut<'b>(
+        _ecx: &'b mut InterpCx<'mir, 'tcx, Self>,
+    ) -> &'b mut Vec<Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra>> {
+        unreachable!("validation cannot mutate stack")
+    }
+
+    fn before_access_global(
+        _tcx: TyCtxtAt<'tcx>,
+        _machine: &Self,
+        _alloc_id: AllocId,
+        _alloc: mir::interpret::ConstAllocation<'tcx>,
+        _static_def_id: Option<DefId>,
+        is_write: bool,
+    ) -> InterpResult<'tcx> {
+        assert!(!is_write);
+        // Do nothing, validation may read globals.
+        Ok(())
+    }
+
+    fn retag_ptr_value(
+        _ecx: &mut InterpCx<'mir, 'tcx, Self>,
+        kind: mir::RetagKind,
+        val: &ImmTy<'tcx, mir::interpret::CtfeProvenance>,
+    ) -> InterpResult<'tcx, ImmTy<'tcx, mir::interpret::CtfeProvenance>> {
+        unreachable!("validation does not retag pointers: {val:?}, {kind:?}")
+    }
+
+    fn before_memory_write(
+        _tcx: TyCtxtAt<'tcx>,
+        _machine: &mut Self,
+        _alloc_extra: &mut Self::AllocExtra,
+        (_alloc_id, _immutable): (AllocId, bool),
+        _range: mir::interpret::AllocRange,
+    ) -> InterpResult<'tcx> {
+        unreachable!("validation does not write to memory")
+    }
+
+    fn before_alloc_read(
+        _ecx: &InterpCx<'mir, 'tcx, Self>,
+        _alloc_id: AllocId,
+    ) -> InterpResult<'tcx> {
+        // Do nothing, validation may read all memory
+        Ok(())
+    }
+}

src/tools/miri/tests/pass/alloc-access-tracking.stderr

@@ -16,6 +16,15 @@ LL |         *ptr = 42; // Crucially, only a write is printed here, no read!
    = note: BACKTRACE:
    = note: inside `start` at $DIR/alloc-access-tracking.rs:LL:CC
 
+note: tracking was triggered
+  --> $DIR/alloc-access-tracking.rs:LL:CC
+   |
+LL |         *ptr = 42; // Crucially, only a write is printed here, no read!
+   |         ^^^^^^^^^ read access to allocation with id 17
+   |
+   = note: BACKTRACE:
+   = note: inside `start` at $DIR/alloc-access-tracking.rs:LL:CC
+
 note: tracking was triggered
   --> $DIR/alloc-access-tracking.rs:LL:CC
    |