Add tombstone checks to make sure incr-comp reads aren't dirty

cramertj · cramertj · commit 1ef95aae503c · 2017-03-07T16:44:00.000-08:00
diff --git a/src/librustc/dep_graph/dep_tracking_map.rs b/src/librustc/dep_graph/dep_tracking_map.rs
@@ -10,6 +10,10 @@
 
 use hir::def_id::DefId;
 use rustc_data_structures::fx::FxHashMap;
+
+#[cfg(debug_assertions)]
+use rustc_data_structures::fx::FxHashSet;
+
 use std::cell::RefCell;
 use std::collections::hash_map::Entry;
 use std::ops::Index;
@@ -26,6 +30,8 @@ pub struct DepTrackingMap<M: DepTrackingMapConfig> {
     phantom: PhantomData<M>,
     graph: DepGraph,
     map: FxHashMap<M::Key, M::Value>,
+    #[cfg(debug_assertions)]
+    tombstones: RefCell<FxHashSet<M::Key>>,
 }
 
 pub trait DepTrackingMapConfig {
@@ -35,11 +41,22 @@ pub trait DepTrackingMapConfig {
 }
 
 impl<M: DepTrackingMapConfig> DepTrackingMap<M> {
+    #[cfg(debug_assertions)]
+    pub fn new(graph: DepGraph) -> DepTrackingMap<M> {
+        DepTrackingMap {
+            phantom: PhantomData,
+            graph: graph,
+            map: FxHashMap(),
+            tombstones: RefCell::new(FxHashSet()),
+        }
+    }
+
+    #[cfg(not(debug_assertions))]
     pub fn new(graph: DepGraph) -> DepTrackingMap<M> {
         DepTrackingMap {
             phantom: PhantomData,
             graph: graph,
-            map: FxHashMap()
+            map: FxHashMap(),
         }
     }
 
@@ -59,13 +76,31 @@ impl<M: DepTrackingMapConfig> DepTrackingMap<M> {
 
     pub fn get(&self, k: &M::Key) -> Option<&M::Value> {
         self.read(k);
-        self.map.get(k)
+        let result = self.map.get(k);
+
+        #[cfg(debug_assertions)]
+        {
+            if result.is_none() {
+                self.tombstones.borrow_mut().insert(k.clone());
+            }
+        }
+
+        result
     }
 
     pub fn insert(&mut self, k: M::Key, v: M::Value) {
         self.write(&k);
+
+        // If we ever read a `None` value for this key, we do not want
+        // to permit a later write to it. The only valid use case for
+        // this is the memoization pattern: for that, use `memoize()`
+        // below
+        #[cfg(debug_assertions)]
+        assert!(!self.tombstones.borrow().contains(&k),
+                "inserting to `{0:?}` after reading from `{0:?}`",
+                M::to_dep_node(&k));
         let old_value = self.map.insert(k, v);
-        assert!(old_value.is_none());
+        assert!(old_value.is_none(), "inserted value twice");
     }
 
     pub fn entry(&mut self, k: M::Key) -> Entry<M::Key, M::Value> {
@@ -75,7 +110,13 @@ impl<M: DepTrackingMapConfig> DepTrackingMap<M> {
 
     pub fn contains_key(&self, k: &M::Key) -> bool {
         self.read(k);
-        self.map.contains_key(k)
+        if self.map.contains_key(k) {
+            true
+        } else {
+            #[cfg(debug_assertions)]
+            self.tombstones.borrow_mut().insert(k.clone());
+            false
+        }
     }
 
     pub fn keys(&self) -> Vec<M::Key> {
@@ -125,7 +166,7 @@ impl<M: DepTrackingMapConfig> MemoizationMap for RefCell<DepTrackingMap<M>> {
     ///     let item_def_id = ccx.tcx.hir.local_def_id(it.id);
     ///     ccx.tcx.item_types.memoized(item_def_id, || {
     ///         ccx.tcx.dep_graph.read(DepNode::Hir(item_def_id)); // (*)
-    ///         compute_type_of_item(ccx, item)
+    ///         Some(compute_type_of_item(ccx, item))
     ///     });
     /// }
     /// ```
@@ -134,7 +175,7 @@ impl<M: DepTrackingMapConfig> MemoizationMap for RefCell<DepTrackingMap<M>> {
     /// accesses the body of the item `item`, so we register a read
     /// from `Hir(item_def_id)`.
     fn memoize<OP>(&self, key: M::Key, op: OP) -> M::Value
-        where OP: FnOnce() -> M::Value
+        where OP: FnOnce() -> Option<M::Value>
     {
         let graph;
         {
@@ -147,9 +188,18 @@ impl<M: DepTrackingMapConfig> MemoizationMap for RefCell<DepTrackingMap<M>> {
         }
 
         let _task = graph.in_task(M::to_dep_node(&key));
-        let result = op();
-        self.borrow_mut().map.insert(key, result.clone());
-        result
+        if let Some(result) = op() {
+            let old_value = self.borrow_mut().map.insert(key, result.clone());
+            assert!(old_value.is_none());
+            result
+        } else {
+            self.borrow().map
+                         .get(&key)
+                         .unwrap_or_else(|| bug!(
+                            "memoize closure failed to write to {:?}", M::to_dep_node(&key)
+                         ))
+                         .clone()
+        }
     }
 }
 
diff --git a/src/librustc/ty/contents.rs b/src/librustc/ty/contents.rs
@@ -125,7 +125,7 @@ impl fmt::Debug for TypeContents {
 
 impl<'a, 'tcx> ty::TyS<'tcx> {
     pub fn type_contents(&'tcx self, tcx: TyCtxt<'a, 'tcx, 'tcx>) -> TypeContents {
-        return tcx.tc_cache.memoize(self, || tc_ty(tcx, self, &mut FxHashMap()));
+        return tcx.tc_cache.memoize(self, || Some(tc_ty(tcx, self, &mut FxHashMap())));
 
         fn tc_ty<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
                            ty: Ty<'tcx>,
diff --git a/src/librustc/ty/mod.rs b/src/librustc/ty/mod.rs
@@ -1633,65 +1633,51 @@ impl<'a, 'gcx, 'tcx> AdtDef {
     ///       check should catch this case.
     fn calculate_sized_constraint_inner(&self,
                                         tcx: TyCtxt<'a, 'tcx, 'tcx>,
-                                        stack: &mut Vec<DefId>)
+                                        stack: &mut Vec<(DefId, bool)>)
                                         -> Ty<'tcx>
     {
-        if let Some(ty) = tcx.maps.adt_sized_constraint.borrow().get(&self.did) {
-            return ty;
-        }
-
-        // Follow the memoization pattern: push the computation of
-        // DepNode::SizedConstraint as our current task.
-        let _task = tcx.dep_graph.in_task(DepNode::SizedConstraint(self.did));
+        if let Some(index) = stack.iter().position(|pair| pair.0 == self.did) {
+            stack[index].1 = true;
 
-        if stack.contains(&self.did) {
             debug!("calculate_sized_constraint: {:?} is recursive", self);
             // This should be reported as an error by `check_representable`.
             //
             // Consider the type as Sized in the meanwhile to avoid
             // further errors.
-            tcx.maps.adt_sized_constraint.borrow_mut().insert(self.did, tcx.types.err);
             return tcx.types.err;
         }
 
-        stack.push(self.did);
+        tcx.maps.adt_sized_constraint.memoize(self.did, || {
+            stack.push((self.did, false));
 
-        let tys : Vec<_> =
+            let tys: Vec<_> =
             self.variants.iter().flat_map(|v| {
                 v.fields.last()
             }).flat_map(|f| {
                 let ty = tcx.item_type(f.did);
                 self.sized_constraint_for_ty(tcx, stack, ty)
             }).collect();
 
-        let self_ = stack.pop().unwrap();
-        assert_eq!(self_, self.did);
+            let (self_, cycle) = stack.pop().unwrap();
+            assert_eq!(self_, self.did);
 
-        let ty = match tys.len() {
-            _ if tys.references_error() => tcx.types.err,
-            0 => tcx.types.bool,
-            1 => tys[0],
-            _ => tcx.intern_tup(&tys[..], false)
-        };
+            let ty = if cycle || tys.references_error() {
+                tcx.types.err
+            } else {
+                match tys.len() {
+                    0 => tcx.types.bool,
+                    1 => tys[0],
+                    _ => tcx.intern_tup(&tys[..], false)
+                }
+            };
 
-        let old = tcx.maps.adt_sized_constraint.borrow().get(&self.did).cloned();
-        match old {
-            Some(old_ty) => {
-                debug!("calculate_sized_constraint: {:?} recurred", self);
-                assert_eq!(old_ty, tcx.types.err);
-                old_ty
-            }
-            None => {
-                debug!("calculate_sized_constraint: {:?} => {:?}", self, ty);
-                tcx.maps.adt_sized_constraint.borrow_mut().insert(self.did, ty);
-                ty
-            }
-        }
+            Some(ty)
+        })
     }
 
     fn sized_constraint_for_ty(&self,
                                tcx: TyCtxt<'a, 'tcx, 'tcx>,
-                               stack: &mut Vec<DefId>,
+                               stack: &mut Vec<(DefId, bool)>,
                                ty: Ty<'tcx>)
                                -> Vec<Ty<'tcx>> {
         let result = match ty.sty {
@@ -2094,9 +2080,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
                 }
             }
 
-            // memoize wants us to return something, so return
-            // the one we generated for this def-id
-            *self.maps.associated_item.borrow().get(&def_id).unwrap()
+            None
         })
     }
 
@@ -2176,7 +2160,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
                 }
                 _ => span_bug!(item.span, "associated_item_def_ids: not impl or trait")
             };
-            Rc::new(vec)
+            Some(Rc::new(vec))
         })
     }
 
diff --git a/src/librustc/util/common.rs b/src/librustc/util/common.rs
@@ -208,7 +208,7 @@ pub trait MemoizationMap {
     /// added into the dep graph. See the `DepTrackingMap` impl for
     /// more details!
     fn memoize<OP>(&self, key: Self::Key, op: OP) -> Self::Value
-        where OP: FnOnce() -> Self::Value;
+        where OP: FnOnce() -> Option<Self::Value>;
 }
 
 impl<K, V, S> MemoizationMap for RefCell<HashMap<K,V,S>>
@@ -218,15 +218,18 @@ impl<K, V, S> MemoizationMap for RefCell<HashMap<K,V,S>>
     type Value = V;
 
     fn memoize<OP>(&self, key: K, op: OP) -> V
-        where OP: FnOnce() -> V
+        where OP: FnOnce() -> Option<V>
     {
         let result = self.borrow().get(&key).cloned();
         match result {
             Some(result) => result,
             None => {
-                let result = op();
-                self.borrow_mut().insert(key, result.clone());
-                result
+                if let Some(result) = op() {
+                    self.borrow_mut().insert(key, result.clone());
+                    result
+                } else {
+                    self.borrow().get(&key).unwrap().clone()
+                }
             }
         }
     }
diff --git a/src/librustc_trans/common.rs b/src/librustc_trans/common.rs
@@ -488,7 +488,7 @@ pub fn fulfill_obligation<'a, 'tcx>(scx: &SharedCrateContext<'a, 'tcx>,
             let vtable = infcx.drain_fulfillment_cx_or_panic(span, &mut fulfill_cx, &vtable);
 
             info!("Cache miss: {:?} => {:?}", trait_ref, vtable);
-            vtable
+            Some(vtable)
         })
     })
 }
diff --git a/src/librustc_trans/monomorphize.rs b/src/librustc_trans/monomorphize.rs
@@ -121,7 +121,7 @@ impl<'a, 'b, 'gcx> TypeFolder<'gcx, 'gcx> for AssociatedTypeNormalizer<'a, 'b, '
         } else {
             self.shared.project_cache().memoize(ty, || {
                 debug!("AssociatedTypeNormalizer: ty={:?}", ty);
-                self.shared.tcx().normalize_associated_type(&ty)
+                Some(self.shared.tcx().normalize_associated_type(&ty))
             })
         }
     }
diff --git a/src/librustc_typeck/collect.rs b/src/librustc_typeck/collect.rs
@@ -617,7 +617,7 @@ fn convert_enum_variant_types<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
         prev_discr = Some(if let Some(e) = variant.node.disr_expr {
             let expr_did = tcx.hir.local_def_id(e.node_id);
             let result = tcx.maps.monomorphic_const_eval.memoize(expr_did, || {
-                evaluate_disr_expr(tcx, e)
+                Some(evaluate_disr_expr(tcx, e))
             });
 
             match result {

Original file line number	Diff line number	Diff line change
`@@ -208,7 +208,7 @@ pub trait MemoizationMap {`
`208`	`208`	/// added into the dep graph. See the `DepTrackingMap` impl for
`209`	`209`	`/// more details!`
`210`	`210`	`fn memoize<OP>(&self, key: Self::Key, op: OP) -> Self::Value`
`211`		`- where OP: FnOnce() -> Self::Value;`
	`211`	`+ where OP: FnOnce() -> Option<Self::Value>;`
`212`	`212`	`}`
`213`	`213`
`214`	`214`	`impl<K, V, S> MemoizationMap for RefCell<HashMap<K,V,S>>`
`@@ -218,15 +218,18 @@ impl<K, V, S> MemoizationMap for RefCell<HashMap<K,V,S>>`
`218`	`218`	`type Value = V;`
`219`	`219`
`220`	`220`	`fn memoize<OP>(&self, key: K, op: OP) -> V`
`221`		`- where OP: FnOnce() -> V`
	`221`	`+ where OP: FnOnce() -> Option<V>`
`222`	`222`	`{`
`223`	`223`	`let result = self.borrow().get(&key).cloned();`
`224`	`224`	`match result {`
`225`	`225`	`Some(result) => result,`
`226`	`226`	`None => {`
`227`		`- let result = op();`
`228`		`- self.borrow_mut().insert(key, result.clone());`
`229`		`- result`
	`227`	`+ if let Some(result) = op() {`
	`228`	`+ self.borrow_mut().insert(key, result.clone());`
	`229`	`+ result`
	`230`	`+ } else {`
	`231`	`+ self.borrow().get(&key).unwrap().clone()`
	`232`	`+ }`
`230`	`233`	`}`
`231`	`234`	`}`
`232`	`235`	`}`
Original file line number	Diff line number	Diff line change
`@@ -488,7 +488,7 @@ pub fn fulfill_obligation<'a, 'tcx>(scx: &SharedCrateContext<'a, 'tcx>,`
`488`	`488`	`let vtable = infcx.drain_fulfillment_cx_or_panic(span, &mut fulfill_cx, &vtable);`
`489`	`489`
`490`	`490`	`info!("Cache miss: {:?} => {:?}", trait_ref, vtable);`
`491`		`- vtable`
	`491`	`+ Some(vtable)`
`492`	`492`	`})`
`493`	`493`	`})`
`494`	`494`	`}`
Original file line number	Diff line number	Diff line change
`@@ -121,7 +121,7 @@ impl<'a, 'b, 'gcx> TypeFolder<'gcx, 'gcx> for AssociatedTypeNormalizer<'a, 'b, '`
`121`	`121`	`} else {`
`122`	`122`	`self.shared.project_cache().memoize(ty, \|\| {`
`123`	`123`	`debug!("AssociatedTypeNormalizer: ty={:?}", ty);`
`124`		`- self.shared.tcx().normalize_associated_type(&ty)`
	`124`	`+ Some(self.shared.tcx().normalize_associated_type(&ty))`
`125`	`125`	`})`
`126`	`126`	`}`
`127`	`127`	`}`