Rollup merge of #70087 - ecstatic-morse:remove-const-eval-loop-detector, r=RalfJung

Remove const eval loop detector

Now that there is a configurable instruction limit for CTFE (see #67260), we can replace the loop detector with something much simpler. See #66946 for more discussion about this. Although the instruction limit is nightly-only, the only practical way to reach the default limit uses nightly-only features as well (although CTFE will still execute code using such features inside an array initializer on stable).

This will at the very least require a crater run, since it will result in an error wherever the "long running const eval" warning appeared before. We may need to increase the default for `const_eval_limit` to work around this.

Resolves #54384 cc #49980
r? @oli-obk cc @RalfJung

Author: Centril

src/librustc/mir/interpret/error.rs

@@ -494,8 +494,10 @@ impl fmt::Debug for UnsupportedOpInfo {
 pub enum ResourceExhaustionInfo {
     /// The stack grew too big.
     StackFrameLimitReached,
-    /// The program ran into an infinite loop.
-    InfiniteLoop,
+    /// The program ran for too long.
+    ///
+    /// The exact limit is set by the `const_eval_limit` attribute.
+    StepLimitReached,
 }
 
 impl fmt::Debug for ResourceExhaustionInfo {
@@ -505,11 +507,9 @@ impl fmt::Debug for ResourceExhaustionInfo {
             StackFrameLimitReached => {
                 write!(f, "reached the configured maximum number of stack frames")
             }
-            InfiniteLoop => write!(
-                f,
-                "duplicate interpreter state observed here, const evaluation will never \
-                    terminate"
-            ),
+            StepLimitReached => {
+                write!(f, "exceeded interpreter step limit (see `#[const_eval_limit]`)")
+            }
         }
     }
 }

src/librustc_mir/const_eval/machine.rs

@@ -3,7 +3,6 @@ use rustc::ty::layout::HasTyCtxt;
 use rustc::ty::{self, Ty};
 use std::borrow::{Borrow, Cow};
 use std::collections::hash_map::Entry;
-use std::convert::TryFrom;
 use std::hash::Hash;
 
 use rustc_data_structures::fx::FxHashMap;
@@ -13,13 +12,13 @@ use rustc_span::source_map::Span;
 use rustc_span::symbol::Symbol;
 
 use crate::interpret::{
-    self, snapshot, AllocId, Allocation, GlobalId, ImmTy, InterpCx, InterpResult, Memory,
-    MemoryKind, OpTy, PlaceTy, Pointer, Scalar,
+    self, AllocId, Allocation, GlobalId, ImmTy, InterpCx, InterpResult, Memory, MemoryKind, OpTy,
+    PlaceTy, Pointer, Scalar,
 };
 
 use super::error::*;
 
-impl<'mir, 'tcx> InterpCx<'mir, 'tcx, CompileTimeInterpreter<'mir, 'tcx>> {
+impl<'mir, 'tcx> InterpCx<'mir, 'tcx, CompileTimeInterpreter> {
     /// Evaluate a const function where all arguments (if any) are zero-sized types.
     /// The evaluation is memoized thanks to the query system.
     ///
@@ -86,22 +85,13 @@ impl<'mir, 'tcx> InterpCx<'mir, 'tcx, CompileTimeInterpreter<'mir, 'tcx>> {
     }
 }
 
-/// The number of steps between loop detector snapshots.
-/// Should be a power of two for performance reasons.
-const DETECTOR_SNAPSHOT_PERIOD: isize = 256;
-
-// Extra machine state for CTFE, and the Machine instance
-pub struct CompileTimeInterpreter<'mir, 'tcx> {
-    /// When this value is negative, it indicates the number of interpreter
-    /// steps *until* the loop detector is enabled. When it is positive, it is
-    /// the number of steps after the detector has been enabled modulo the loop
-    /// detector period.
-    pub(super) steps_since_detector_enabled: isize,
-
-    pub(super) is_detector_enabled: bool,
-
-    /// Extra state to detect loops.
-    pub(super) loop_detector: snapshot::InfiniteLoopDetector<'mir, 'tcx>,
+/// Extra machine state for CTFE, and the Machine instance
+pub struct CompileTimeInterpreter {
+    /// For now, the number of terminators that can be evaluated before we throw a resource
+    /// exhuastion error.
+    ///
+    /// Setting this to `0` disables the limit and allows the interpreter to run forever.
+    pub steps_remaining: usize,
 }
 
 #[derive(Copy, Clone, Debug)]
@@ -110,16 +100,9 @@ pub struct MemoryExtra {
     pub(super) can_access_statics: bool,
 }
 
-impl<'mir, 'tcx> CompileTimeInterpreter<'mir, 'tcx> {
+impl CompileTimeInterpreter {
     pub(super) fn new(const_eval_limit: usize) -> Self {
-        let steps_until_detector_enabled =
-            isize::try_from(const_eval_limit).unwrap_or(std::isize::MAX);
-
-        CompileTimeInterpreter {
-            loop_detector: Default::default(),
-            steps_since_detector_enabled: -steps_until_detector_enabled,
-            is_detector_enabled: const_eval_limit != 0,
-        }
+        CompileTimeInterpreter { steps_remaining: const_eval_limit }
     }
 }
 
@@ -173,8 +156,7 @@ impl<K: Hash + Eq, V> interpret::AllocMap<K, V> for FxHashMap<K, V> {
     }
 }
 
-crate type CompileTimeEvalContext<'mir, 'tcx> =
-    InterpCx<'mir, 'tcx, CompileTimeInterpreter<'mir, 'tcx>>;
+crate type CompileTimeEvalContext<'mir, 'tcx> = InterpCx<'mir, 'tcx, CompileTimeInterpreter>;
 
 impl interpret::MayLeak for ! {
     #[inline(always)]
@@ -184,7 +166,7 @@ impl interpret::MayLeak for ! {
     }
 }
 
-impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir, 'tcx> {
+impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter {
     type MemoryKinds = !;
     type PointerTag = ();
     type ExtraFnVal = !;
@@ -345,26 +327,17 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
     }
 
     fn before_terminator(ecx: &mut InterpCx<'mir, 'tcx, Self>) -> InterpResult<'tcx> {
-        if !ecx.machine.is_detector_enabled {
+        // The step limit has already been hit in a previous call to `before_terminator`.
+        if ecx.machine.steps_remaining == 0 {
             return Ok(());
         }
 
-        {
-            let steps = &mut ecx.machine.steps_since_detector_enabled;
-
-            *steps += 1;
-            if *steps < 0 {
-                return Ok(());
-            }
-
-            *steps %= DETECTOR_SNAPSHOT_PERIOD;
-            if *steps != 0 {
-                return Ok(());
-            }
+        ecx.machine.steps_remaining -= 1;
+        if ecx.machine.steps_remaining == 0 {
+            throw_exhaust!(StepLimitReached)
         }
 
-        let span = ecx.frame().span;
-        ecx.machine.loop_detector.observe_and_analyze(*ecx.tcx, span, &ecx.memory, &ecx.stack[..])
+        Ok(())
     }
 
     #[inline(always)]

src/librustc_mir/interpret/memory.rs

@@ -112,25 +112,6 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> HasDataLayout for Memory<'mir, 'tcx, M>
     }
 }
 
-// FIXME: Really we shouldn't clone memory, ever. Snapshot machinery should instead
-// carefully copy only the reachable parts.
-impl<'mir, 'tcx, M> Clone for Memory<'mir, 'tcx, M>
-where
-    M: Machine<'mir, 'tcx, PointerTag = (), AllocExtra = ()>,
-    M::MemoryExtra: Copy,
-    M::MemoryMap: AllocMap<AllocId, (MemoryKind<M::MemoryKinds>, Allocation)>,
-{
-    fn clone(&self) -> Self {
-        Memory {
-            alloc_map: self.alloc_map.clone(),
-            extra_fn_ptr_map: self.extra_fn_ptr_map.clone(),
-            dead_alloc_map: self.dead_alloc_map.clone(),
-            extra: self.extra,
-            tcx: self.tcx,
-        }
-    }
-}
-
 impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
     pub fn new(tcx: TyCtxtAt<'tcx>, extra: M::MemoryExtra) -> Self {
         Memory {

src/librustc_mir/interpret/mod.rs

@@ -9,7 +9,6 @@ mod memory;
 mod operand;
 mod operator;
 mod place;
-pub(crate) mod snapshot; // for const_eval
 mod step;
 mod terminator;
 mod traits;