Remove const deduplication from the interpreter.

Author: cjgillot

compiler/rustc_const_eval/src/interpret/eval_context.rs

@@ -559,8 +559,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
         span: Span,
         layout: Option<TyAndLayout<'tcx>>,
     ) -> InterpResult<'tcx, OpTy<'tcx, M::Provenance>> {
-        M::eval_mir_constant(self, *val, span, layout, |ecx, val, span, layout| {
-            let const_val = val.eval(*ecx.tcx, ecx.typing_env, span).map_err(|err| {
+        let const_val = val.eval(*self.tcx, self.typing_env, span).map_err(|err| {
                 if M::ALL_CONSTS_ARE_PRECHECKED {
                     match err {
                         ErrorHandled::TooGeneric(..) => {},
@@ -576,11 +575,10 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
                         }
                     }
                 }
-                err.emit_note(*ecx.tcx);
+                err.emit_note(*self.tcx);
                 err
             })?;
-            ecx.const_val_to_op(const_val, val.ty(), layout)
-        })
+        self.const_val_to_op(const_val, val.ty(), layout)
     }
 
     #[must_use]

compiler/rustc_const_eval/src/interpret/machine.rs

@@ -12,7 +12,6 @@ use rustc_middle::query::TyCtxtAt;
 use rustc_middle::ty::Ty;
 use rustc_middle::ty::layout::TyAndLayout;
 use rustc_middle::{mir, ty};
-use rustc_span::Span;
 use rustc_span::def_id::DefId;
 use rustc_target::callconv::FnAbi;
 
@@ -587,27 +586,6 @@ pub trait Machine<'tcx>: Sized {
         interp_ok(())
     }
 
-    /// Evaluate the given constant. The `eval` function will do all the required evaluation,
-    /// but this hook has the chance to do some pre/postprocessing.
-    #[inline(always)]
-    fn eval_mir_constant<F>(
-        ecx: &InterpCx<'tcx, Self>,
-        val: mir::Const<'tcx>,
-        span: Span,
-        layout: Option<TyAndLayout<'tcx>>,
-        eval: F,
-    ) -> InterpResult<'tcx, OpTy<'tcx, Self::Provenance>>
-    where
-        F: Fn(
-            &InterpCx<'tcx, Self>,
-            mir::Const<'tcx>,
-            Span,
-            Option<TyAndLayout<'tcx>>,
-        ) -> InterpResult<'tcx, OpTy<'tcx, Self::Provenance>>,
-    {
-        eval(ecx, val, span, layout)
-    }
-
     /// Returns the salt to be used for a deduplicated global alloation.
     /// If the allocation is for a function, the instance is provided as well
     /// (this lets Miri ensure unique addresses for some functions).

src/tools/miri/src/machine.rs

@@ -4,7 +4,6 @@
 use std::any::Any;
 use std::borrow::Cow;
 use std::cell::{Cell, RefCell};
-use std::collections::hash_map::Entry;
 use std::path::Path;
 use std::rc::Rc;
 use std::{fmt, process};
@@ -70,12 +69,6 @@ pub struct FrameExtra<'tcx> {
     /// This is used by `MiriMachine::current_span` and `MiriMachine::caller_span`
     pub is_user_relevant: bool,
 
-    /// We have a cache for the mapping from [`mir::Const`] to resulting [`AllocId`].
-    /// However, we don't want all frames to always get the same result, so we insert
-    /// an additional bit of "salt" into the cache key. This salt is fixed per-frame
-    /// so that within a call, a const will have a stable address.
-    salt: usize,
-
     /// Data race detector per-frame data.
     pub data_race: Option<data_race::FrameState>,
 }
@@ -88,14 +81,12 @@ impl<'tcx> std::fmt::Debug for FrameExtra<'tcx> {
             catch_unwind,
             timing: _,
             is_user_relevant,
-            salt,
             data_race,
         } = self;
         f.debug_struct("FrameData")
             .field("borrow_tracker", borrow_tracker)
             .field("catch_unwind", catch_unwind)
             .field("is_user_relevant", is_user_relevant)
-            .field("salt", salt)
             .field("data_race", data_race)
             .finish()
     }
@@ -108,7 +99,6 @@ impl VisitProvenance for FrameExtra<'_> {
             borrow_tracker,
             timing: _,
             is_user_relevant: _,
-            salt: _,
             data_race: _,
         } = self;
 
@@ -579,11 +569,6 @@ pub struct MiriMachine<'tcx> {
     /// diagnostics.
     pub(crate) allocation_spans: RefCell<FxHashMap<AllocId, (Span, Option<Span>)>>,
 
-    /// Maps MIR consts to their evaluated result. We combine the const with a "salt" (`usize`)
-    /// that is fixed per stack frame; this lets us have sometimes different results for the
-    /// same const while ensuring consistent results within a single call.
-    const_cache: RefCell<FxHashMap<(mir::Const<'tcx>, usize), OpTy<'tcx>>>,
-
     /// For each allocation, an offset inside that allocation that was deemed aligned even for
     /// symbolic alignment checks. This cannot be stored in `AllocExtra` since it needs to be
     /// tracked for vtables and function allocations as well as regular allocations.
@@ -764,7 +749,6 @@ impl<'tcx> MiriMachine<'tcx> {
             stack_size,
             collect_leak_backtraces: config.collect_leak_backtraces,
             allocation_spans: RefCell::new(FxHashMap::default()),
-            const_cache: RefCell::new(FxHashMap::default()),
             symbolic_alignment: RefCell::new(FxHashMap::default()),
             union_data_ranges: FxHashMap::default(),
             pthread_mutex_sanity: Cell::new(false),
@@ -942,7 +926,6 @@ impl VisitProvenance for MiriMachine<'_> {
             stack_size: _,
             collect_leak_backtraces: _,
             allocation_spans: _,
-            const_cache: _,
             symbolic_alignment: _,
             union_data_ranges: _,
             pthread_mutex_sanity: _,
@@ -1579,7 +1562,6 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> {
             catch_unwind: None,
             timing,
             is_user_relevant: ecx.machine.is_user_relevant(&frame),
-            salt: ecx.machine.rng.borrow_mut().random_range(0..ADDRS_PER_ANON_GLOBAL),
             data_race: ecx
                 .machine
                 .data_race
@@ -1738,33 +1720,6 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> {
         interp_ok(())
     }
 
-    fn eval_mir_constant<F>(
-        ecx: &InterpCx<'tcx, Self>,
-        val: mir::Const<'tcx>,
-        span: Span,
-        layout: Option<TyAndLayout<'tcx>>,
-        eval: F,
-    ) -> InterpResult<'tcx, OpTy<'tcx>>
-    where
-        F: Fn(
-            &InterpCx<'tcx, Self>,
-            mir::Const<'tcx>,
-            Span,
-            Option<TyAndLayout<'tcx>>,
-        ) -> InterpResult<'tcx, OpTy<'tcx>>,
-    {
-        let frame = ecx.active_thread_stack().last().unwrap();
-        let mut cache = ecx.machine.const_cache.borrow_mut();
-        match cache.entry((val, frame.extra.salt)) {
-            Entry::Vacant(ve) => {
-                let op = eval(ecx, val, span, layout)?;
-                ve.insert(op.clone());
-                interp_ok(op)
-            }
-            Entry::Occupied(oe) => interp_ok(oe.get().clone()),
-        }
-    }
-
     fn get_global_alloc_salt(
         ecx: &InterpCx<'tcx, Self>,
         instance: Option<ty::Instance<'tcx>>,

src/tools/miri/tests/pass/const-addrs.rs

@@ -1,11 +1,7 @@
-// The const fn interpreter creates a new AllocId every time it evaluates any const.
-// If we do that in Miri, repeatedly evaluating a const causes unbounded memory use
-// we need to keep track of the base address for that AllocId, and the allocation is never
-// deallocated.
-// In Miri we explicitly store previously-assigned AllocIds for each const and ensure
-// that we only hand out a finite number of AllocIds per const.
-// MIR inlining will put every evaluation of the const we're repeatedly evaluating into the same
-// stack frame, breaking this test.
+// The interpreter used to create a new AllocId every time it evaluates any const.
+// This caused unbounded memory use in Miri.
+// This test verifies that we only create a bounded amount of addresses for any given const.
+// In practice, the interpreter always returns the same address, but we *do not guarantee* it.
 //@compile-flags: -Zinline-mir=no
 
 const EVALS: usize = 256;
@@ -16,10 +12,8 @@ fn main() {
     for _ in 0..EVALS {
         addrs.insert(const_addr());
     }
-    // Check that the const allocation has multiple base addresses
-    assert!(addrs.len() > 1);
-    // But also that we get a limited number of unique base addresses
-    assert!(addrs.len() < EVALS);
+    // Check that we always return the same base address for the const allocation.
+    assert_eq!(addrs.len(), 1);
 
     // Check that within a call we always produce the same address
     let mut prev = 0;