Auto merge of rust-lang#52626 - brunocodutra:issue-52475, r=oli-obk

Fix issue rust-lang#52475: Make loop detector only consider reachable memory

As [suggested](rust-lang#51702 (comment)) by @oli-obk `alloc_id`s should be ignored by traversing all `Allocation`s in interpreter memory at a given moment in time, beginning by `ByRef` locals in the stack.

- [x] Generalize the implementation of `Hash` for `EvalSnapshot` to traverse `Allocation`s
- [x] Generalize the implementation of `PartialEq` for `EvalSnapshot` to traverse `Allocation`s
- [x] Commit regression tests

Fixes rust-lang#52626
Fixes rust-lang#52849

Author: bors

src/librustc/ich/hcx.rs

@@ -15,7 +15,6 @@ use hir::map::definitions::Definitions;
 use ich::{self, CachingSourceMapView, Fingerprint};
 use middle::cstore::CrateStore;
 use ty::{TyCtxt, fast_reject};
-use mir::interpret::AllocId;
 use session::Session;
 
 use std::cmp::Ord;
@@ -60,8 +59,6 @@ pub struct StableHashingContext<'a> {
     // CachingSourceMapView, so we initialize it lazily.
     raw_source_map: &'a SourceMap,
     caching_source_map: Option<CachingSourceMapView<'a>>,
-
-    pub(super) alloc_id_recursion_tracker: FxHashSet<AllocId>,
 }
 
 #[derive(PartialEq, Eq, Clone, Copy)]
@@ -105,7 +102,6 @@ impl<'a> StableHashingContext<'a> {
             hash_spans: hash_spans_initial,
             hash_bodies: true,
             node_id_hashing_mode: NodeIdHashingMode::HashDefPath,
-            alloc_id_recursion_tracker: Default::default(),
         }
     }
 

src/librustc/ich/impls_ty.rs

@@ -412,7 +412,7 @@ impl<'a> HashStable<StableHashingContext<'a>> for mir::interpret::AllocId {
         ty::tls::with_opt(|tcx| {
             trace!("hashing {:?}", *self);
             let tcx = tcx.expect("can't hash AllocIds during hir lowering");
-            let alloc_kind = tcx.alloc_map.lock().get(*self).expect("no value for AllocId");
+            let alloc_kind = tcx.alloc_map.lock().get(*self);
             alloc_kind.hash_stable(hcx, hasher);
         });
     }

src/librustc/mir/interpret/mod.rs

@@ -133,9 +133,14 @@ pub trait PointerArithmetic: layout::HasDataLayout {
 impl<T: layout::HasDataLayout> PointerArithmetic for T {}
 
 
+/// Pointer is generic over the type that represents a reference to Allocations,
+/// thus making it possible for the most convenient representation to be used in
+/// each context.
+///
+/// Defaults to the index based and loosely coupled AllocId.
 #[derive(Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd, RustcEncodable, RustcDecodable, Hash)]
-pub struct Pointer {
-    pub alloc_id: AllocId,
+pub struct Pointer<Id=AllocId> {
+    pub alloc_id: Id,
     pub offset: Size,
 }
 
@@ -543,16 +548,16 @@ impl Allocation {
 impl<'tcx> ::serialize::UseSpecializedDecodable for &'tcx Allocation {}
 
 #[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Debug, RustcEncodable, RustcDecodable)]
-pub struct Relocations(SortedMap<Size, AllocId>);
+pub struct Relocations<Id=AllocId>(SortedMap<Size, Id>);
 
-impl Relocations {
-    pub fn new() -> Relocations {
+impl<Id> Relocations<Id> {
+    pub fn new() -> Self {
         Relocations(SortedMap::new())
     }
 
     // The caller must guarantee that the given relocations are already sorted
     // by address and contain no duplicates.
-    pub fn from_presorted(r: Vec<(Size, AllocId)>) -> Relocations {
+    pub fn from_presorted(r: Vec<(Size, Id)>) -> Self {
         Relocations(SortedMap::from_presorted_elements(r))
     }
 }

src/librustc/mir/interpret/value.rs

@@ -326,7 +326,7 @@ impl From<Pointer> for Scalar {
 /// size. Like a range of bytes in an `Allocation`, a `Scalar` can either represent the raw bytes
 /// of a simple value or a pointer into another `Allocation`
 #[derive(Clone, Copy, Debug, Eq, PartialEq, Ord, PartialOrd, RustcEncodable, RustcDecodable, Hash)]
-pub enum Scalar {
+pub enum Scalar<Id=AllocId> {
     /// The raw bytes of a simple value.
     Bits {
         /// The first `size` bytes are the value.
@@ -338,12 +338,12 @@ pub enum Scalar {
     /// A pointer into an `Allocation`. An `Allocation` in the `memory` module has a list of
     /// relocations, but a `Scalar` is only large enough to contain one, so we just represent the
     /// relocation and its associated offset together as a `Pointer` here.
-    Ptr(Pointer),
+    Ptr(Pointer<Id>),
 }
 
 #[derive(Clone, Copy, Debug, Eq, PartialEq, Ord, PartialOrd, RustcEncodable, RustcDecodable, Hash)]
-pub enum ScalarMaybeUndef {
-    Scalar(Scalar),
+pub enum ScalarMaybeUndef<Id=AllocId> {
+    Scalar(Scalar<Id>),
     Undef,
 }
 

src/librustc_data_structures/stable_hasher.rs

@@ -281,6 +281,23 @@ impl<T1, T2, T3, CTX> HashStable<CTX> for (T1, T2, T3)
     }
 }
 
+impl<T1, T2, T3, T4, CTX> HashStable<CTX> for (T1, T2, T3, T4)
+     where T1: HashStable<CTX>,
+           T2: HashStable<CTX>,
+           T3: HashStable<CTX>,
+           T4: HashStable<CTX>,
+{
+    fn hash_stable<W: StableHasherResult>(&self,
+                                          ctx: &mut CTX,
+                                          hasher: &mut StableHasher<W>) {
+        let (ref _0, ref _1, ref _2, ref _3) = *self;
+        _0.hash_stable(ctx, hasher);
+        _1.hash_stable(ctx, hasher);
+        _2.hash_stable(ctx, hasher);
+        _3.hash_stable(ctx, hasher);
+    }
+}
+
 impl<T: HashStable<CTX>, CTX> HashStable<CTX> for [T] {
     default fn hash_stable<W: StableHasherResult>(&self,
                                                   ctx: &mut CTX,

src/librustc_mir/const_eval.rs

@@ -196,6 +196,8 @@ impl<'tcx> Into<EvalError<'tcx>> for ConstEvalError {
     }
 }
 
+impl_stable_hash_for!(struct CompileTimeEvaluator {});
+
 #[derive(Clone, Debug)]
 enum ConstEvalError {
     NeedsRfc(String),

src/librustc_mir/interpret/eval_context.rs

@@ -9,23 +9,23 @@
 // except according to those terms.
 
 use std::fmt::Write;
-use std::hash::{Hash, Hasher};
 use std::mem;
 
 use rustc::hir::def_id::DefId;
 use rustc::hir::def::Def;
 use rustc::hir::map::definitions::DefPathData;
+use rustc::ich::StableHashingContext;
 use rustc::mir;
 use rustc::ty::layout::{
     self, Size, Align, HasDataLayout, LayoutOf, TyLayout
 };
 use rustc::ty::subst::{Subst, Substs};
 use rustc::ty::{self, Ty, TyCtxt, TypeFoldable};
 use rustc::ty::query::TyCtxtAt;
-use rustc_data_structures::fx::{FxHashSet, FxHasher};
 use rustc_data_structures::indexed_vec::IndexVec;
+use rustc_data_structures::stable_hasher::{HashStable, StableHasher, StableHasherResult};
 use rustc::mir::interpret::{
-    GlobalId, Scalar, FrameInfo,
+    GlobalId, Scalar, FrameInfo, AllocId,
     EvalResult, EvalErrorKind,
     ScalarMaybeUndef,
     truncate, sign_extend,
@@ -38,6 +38,8 @@ use super::{
     Memory, Machine
 };
 
+use super::snapshot::InfiniteLoopDetector;
+
 pub struct EvalContext<'a, 'mir, 'tcx: 'a + 'mir, M: Machine<'mir, 'tcx>> {
     /// Stores the `Machine` instance.
     pub machine: M,
@@ -95,7 +97,7 @@ pub struct Frame<'mir, 'tcx: 'mir> {
     /// The locals are stored as `Option<Value>`s.
     /// `None` represents a local that is currently dead, while a live local
     /// can either directly contain `Scalar` or refer to some part of an `Allocation`.
-    pub locals: IndexVec<mir::Local, LocalValue>,
+    pub locals: IndexVec<mir::Local, LocalValue<AllocId>>,
 
     ////////////////////////////////////////////////////////////////////////////////
     // Current position within the function
@@ -108,51 +110,25 @@ pub struct Frame<'mir, 'tcx: 'mir> {
     pub stmt: usize,
 }
 
-impl<'mir, 'tcx: 'mir> Eq for Frame<'mir, 'tcx> {}
-
-impl<'mir, 'tcx: 'mir> PartialEq for Frame<'mir, 'tcx> {
-    fn eq(&self, other: &Self) -> bool {
-        let Frame {
-            mir: _,
-            instance,
-            span: _,
-            return_to_block,
-            return_place,
-            locals,
-            block,
-            stmt,
-        } = self;
-
-        // Some of these are constant during evaluation, but are included
-        // anyways for correctness.
-        *instance == other.instance
-            && *return_to_block == other.return_to_block
-            && *return_place == other.return_place
-            && *locals == other.locals
-            && *block == other.block
-            && *stmt == other.stmt
-    }
-}
+impl<'a, 'mir, 'tcx: 'mir> HashStable<StableHashingContext<'a>> for Frame<'mir, 'tcx> {
+    fn hash_stable<W: StableHasherResult>(
+        &self,
+        hcx: &mut StableHashingContext<'a>,
+        hasher: &mut StableHasher<W>) {
 
-impl<'mir, 'tcx: 'mir> Hash for Frame<'mir, 'tcx> {
-    fn hash<H: Hasher>(&self, state: &mut H) {
         let Frame {
-            mir: _,
+            mir,
             instance,
-            span: _,
+            span,
             return_to_block,
             return_place,
             locals,
             block,
             stmt,
         } = self;
 
-        instance.hash(state);
-        return_to_block.hash(state);
-        return_place.hash(state);
-        locals.hash(state);
-        block.hash(state);
-        stmt.hash(state);
+        (mir, instance, span, return_to_block).hash_stable(hcx, hasher);
+        (return_place, locals, block, stmt).hash_stable(hcx, hasher);
     }
 }
 
@@ -168,15 +144,27 @@ pub enum StackPopCleanup {
     None { cleanup: bool },
 }
 
+impl<'a> HashStable<StableHashingContext<'a>> for StackPopCleanup {
+    fn hash_stable<W: StableHasherResult>(
+        &self,
+        hcx: &mut StableHashingContext<'a>,
+        hasher: &mut StableHasher<W>) {
+        match self {
+            StackPopCleanup::Goto(ref block) => block.hash_stable(hcx, hasher),
+            StackPopCleanup::None { cleanup } => cleanup.hash_stable(hcx, hasher),
+        }
+    }
+}
+
 // State of a local variable
 #[derive(Copy, Clone, PartialEq, Eq, Hash)]
-pub enum LocalValue {
+pub enum LocalValue<Id=AllocId> {
     Dead,
     // Mostly for convenience, we re-use the `Operand` type here.
     // This is an optimization over just always having a pointer here;
     // we can thus avoid doing an allocation when the local just stores
     // immediate values *and* never has its address taken.
-    Live(Operand),
+    Live(Operand<Id>),
 }
 
 impl<'tcx> LocalValue {
@@ -195,72 +183,10 @@ impl<'tcx> LocalValue {
     }
 }
 
-/// The virtual machine state during const-evaluation at a given point in time.
-type EvalSnapshot<'a, 'mir, 'tcx, M>
-    = (M, Vec<Frame<'mir, 'tcx>>, Memory<'a, 'mir, 'tcx, M>);
-
-pub(super) struct InfiniteLoopDetector<'a, 'mir, 'tcx: 'a + 'mir, M: Machine<'mir, 'tcx>> {
-    /// The set of all `EvalSnapshot` *hashes* observed by this detector.
-    ///
-    /// When a collision occurs in this table, we store the full snapshot in
-    /// `snapshots`.
-    hashes: FxHashSet<u64>,
-
-    /// The set of all `EvalSnapshot`s observed by this detector.
-    ///
-    /// An `EvalSnapshot` will only be fully cloned once it has caused a
-    /// collision in `hashes`. As a result, the detector must observe at least
-    /// *two* full cycles of an infinite loop before it triggers.
-    snapshots: FxHashSet<EvalSnapshot<'a, 'mir, 'tcx, M>>,
-}
-
-impl<'a, 'mir, 'tcx, M> Default for InfiniteLoopDetector<'a, 'mir, 'tcx, M>
-    where M: Machine<'mir, 'tcx>,
-          'tcx: 'a + 'mir,
-{
-    fn default() -> Self {
-        InfiniteLoopDetector {
-            hashes: FxHashSet::default(),
-            snapshots: FxHashSet::default(),
-        }
-    }
-}
-
-impl<'a, 'mir, 'tcx, M> InfiniteLoopDetector<'a, 'mir, 'tcx, M>
-    where M: Machine<'mir, 'tcx>,
-          'tcx: 'a + 'mir,
-{
-    /// Returns `true` if the loop detector has not yet observed a snapshot.
-    pub fn is_empty(&self) -> bool {
-        self.hashes.is_empty()
-    }
-
-    pub fn observe_and_analyze(
-        &mut self,
-        machine: &M,
-        stack: &Vec<Frame<'mir, 'tcx>>,
-        memory: &Memory<'a, 'mir, 'tcx, M>,
-    ) -> EvalResult<'tcx, ()> {
-        let snapshot = (machine, stack, memory);
-
-        let mut fx = FxHasher::default();
-        snapshot.hash(&mut fx);
-        let hash = fx.finish();
-
-        if self.hashes.insert(hash) {
-            // No collision
-            return Ok(())
-        }
-
-        if self.snapshots.insert((machine.clone(), stack.clone(), memory.clone())) {
-            // Spurious collision or first cycle
-            return Ok(())
-        }
-
-        // Second cycle
-        Err(EvalErrorKind::InfiniteLoop.into())
-    }
-}
+impl_stable_hash_for!(enum self::LocalValue {
+    Dead,
+    Live(x),
+});
 
 impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> HasDataLayout for &'a EvalContext<'a, 'mir, 'tcx, M> {
     #[inline]

src/librustc_mir/interpret/machine.rs

@@ -15,17 +15,19 @@
 use std::hash::Hash;
 
 use rustc::hir::def_id::DefId;
+use rustc::ich::StableHashingContext;
 use rustc::mir::interpret::{Allocation, EvalResult, Scalar};
 use rustc::mir;
 use rustc::ty::{self, layout::TyLayout, query::TyCtxtAt};
+use rustc_data_structures::stable_hasher::HashStable;
 
 use super::{EvalContext, PlaceTy, OpTy};
 
 /// Methods of this trait signifies a point where CTFE evaluation would fail
 /// and some use case dependent behaviour can instead be applied
-pub trait Machine<'mir, 'tcx>: Clone + Eq + Hash {
+pub trait Machine<'mir, 'tcx>: Clone + Eq + Hash + for<'a> HashStable<StableHashingContext<'a>> {
     /// Additional data that can be accessed via the Memory
-    type MemoryData: Clone + Eq + Hash;
+    type MemoryData: Clone + Eq + Hash + for<'a> HashStable<StableHashingContext<'a>>;
 
     /// Additional memory kinds a machine wishes to distinguish from the builtin ones
     type MemoryKinds: ::std::fmt::Debug + Copy + Clone + Eq + Hash;