basic retagging (no fn_entry); this also makes us catch more bugs even with optimizations and we can finally stop mutating the state on deref

src/lib.rs

@@ -528,9 +528,11 @@ impl<'a, 'mir, 'tcx> Machine<'a, 'mir, 'tcx> for Evaluator<'tcx> {
         fn_entry: bool,
         place: PlaceTy<'tcx, Borrow>,
     ) -> EvalResult<'tcx> {
-        if !ecx.tcx.sess.opts.debugging_opts.mir_emit_retag || !ecx.machine.validate {
+        if !ecx.tcx.sess.opts.debugging_opts.mir_emit_retag || !Self::enforce_validity(ecx) {
             // No tracking, or no retagging. This is possible because a dependency of ours might be
             // called with different flags than we are,
+            // Also, honor the whitelist in `enforce_validity` because otherwise we might retag
+            // uninitialized data.
             return Ok(())
         }
         ecx.retag(fn_entry, place)

src/stacked_borrows.rs

@@ -1,6 +1,6 @@
-use std::cell::{Cell, RefCell};
+use std::cell::RefCell;
 
-use rustc::ty::{Ty, layout::Size};
+use rustc::ty::{self, Ty, layout::Size};
 use rustc::hir;
 
 use super::{
@@ -101,12 +101,12 @@ impl From<Option<hir::Mutability>> for RefKind {
 /// Extra global machine state
 #[derive(Clone, Debug)]
 pub struct State {
-    clock: Cell<Timestamp>
+    clock: Timestamp
 }
 
 impl State {
     pub fn new() -> State {
-        State { clock: Cell::new(0) }
+        State { clock: 0 }
     }
 }
 
@@ -129,6 +129,7 @@ impl Default for Stack {
 /// Extra per-allocation state
 #[derive(Clone, Debug, Default)]
 pub struct Stacks {
+    // Even reading memory can have effects on the stack, so we need a `RefCell` here.
     stacks: RefCell<RangeMap<Stack>>,
 }
 
@@ -249,9 +250,9 @@ impl<'tcx> Stack {
 }
 
 impl State {
-    fn increment_clock(&self) -> Timestamp {
-        let val = self.clock.get();
-        self.clock.set(val + 1);
+    fn increment_clock(&mut self) -> Timestamp {
+        let val = self.clock;
+        self.clock = val + 1;
         val
     }
 }
@@ -334,14 +335,8 @@ impl<'tcx> Stacks {
 }
 
 pub trait EvalContextExt<'tcx> {
-    fn tag_for_pointee(
-        &self,
-        pointee_ty: Ty<'tcx>,
-        ref_kind: RefKind,
-    ) -> Borrow;
-
     fn tag_reference(
-        &self,
+        &mut self,
         ptr: Pointer<Borrow>,
         pointee_ty: Ty<'tcx>,
         size: Size,
@@ -371,13 +366,16 @@ pub trait EvalContextExt<'tcx> {
 }
 
 impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, 'tcx> {
-    fn tag_for_pointee(
-        &self,
+    /// Called for place-to-value conversion.
+    fn tag_reference(
+        &mut self,
+        ptr: Pointer<Borrow>,
         pointee_ty: Ty<'tcx>,
+        size: Size,
         ref_kind: RefKind,
-    ) -> Borrow {
+    ) -> EvalResult<'tcx, Borrow> {
         let time = self.machine.stacked_borrows.increment_clock();
-        match ref_kind {
+        let new_bor = match ref_kind {
             RefKind::Mut => Borrow::Mut(Mut::Uniq(time)),
             RefKind::Shr =>
                 // FIXME This does not do enough checking when only part of the data has
@@ -390,18 +388,7 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, '
                     Borrow::Mut(Mut::Raw)
                 },
             RefKind::Raw => Borrow::Mut(Mut::Raw),
-        }
-    }
-
-    /// Called for place-to-value conversion.
-    fn tag_reference(
-        &self,
-        ptr: Pointer<Borrow>,
-        pointee_ty: Ty<'tcx>,
-        size: Size,
-        ref_kind: RefKind,
-    ) -> EvalResult<'tcx, Borrow> {
-        let new_bor = self.tag_for_pointee(pointee_ty, ref_kind);
+        };
         trace!("tag_reference: Creating new reference ({:?}) for {:?} (pointee {}, size {}): {:?}",
             ref_kind, ptr, pointee_ty, size.bytes(), new_bor);
 
@@ -424,7 +411,7 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, '
     fn tag_dereference(
         &self,
         ptr: Pointer<Borrow>,
-        pointee_ty: Ty<'tcx>,
+        _pointee_ty: Ty<'tcx>,
         size: Size,
         ref_kind: RefKind,
     ) -> EvalResult<'tcx, Borrow> {
@@ -454,13 +441,7 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, '
                 // also using `var`, and that would be okay.
             }
             (RefKind::Shr, Borrow::Mut(Mut::Uniq(_))) => {
-                // A mut got transmuted to shr.  High time we freeze this location!
-                // Make this a delayed reborrow.  Redundant reborows to shr are okay,
-                // so we do not have to be worried about doing too much.
-                // FIXME: Reconsider if we really want to mutate things while doing just a deref,
-                // which, in particular, validation does.
-                trace!("tag_dereference: Lazy freezing of {:?}", ptr);
-                return self.tag_reference(ptr, pointee_ty, size, ref_kind);
+                // A mut got transmuted to shr.  The mut borrow must be reactivatable.
             }
             (RefKind::Mut, Borrow::Frz(_)) => {
                 // This is just invalid.
@@ -516,9 +497,24 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, '
     fn retag(
         &mut self,
         _fn_entry: bool,
-        _place: PlaceTy<'tcx, Borrow>
+        place: PlaceTy<'tcx, Borrow>
     ) -> EvalResult<'tcx> {
-        // TODO do something
+        // For now, we only retag if the toplevel type is a reference.
+        // TODO: Recurse into structs and enums, sharing code with validation.
+        let mutbl = match place.layout.ty.sty {
+            ty::Ref(_, _, mutbl) => mutbl, // go ahead
+            _ => return Ok(()), // don't do a thing
+        };
+        // We want to reborrow the reference stored there. This will call the hooks
+        // above.  First deref.
+        // (This is somewhat redundant because validation already did the same thing,
+        // but what can you do.)
+        let val = self.read_value(self.place_to_op(place)?)?;
+        let dest = self.ref_to_mplace(val)?;
+        // Now put a new ref into the old place.
+        // FIXME: Honor `fn_entry`!
+        let val = self.create_ref(dest, Some(mutbl))?;
+        self.write_value(val, place)?;
         Ok(())
     }
 }

tests/compile-fail/stacked_borrows/alias_through_mutation.rs

@@ -1,6 +1,3 @@
-// With optimizations, we just store a raw in `x`, and there is no problem.
-// compile-flags: -Zmir-opt-level=0
-
 #![allow(unused_variables)]
 
 // This makes a ref that was passed to us via &mut alias with things it should not alias with

tests/compile-fail/stacked_borrows/buggy_as_mut_slice.rs

@@ -1,6 +1,3 @@
-// FIXME: Without retagging, optimization kills finding this problem
-// compile-flags: -Zmir-opt-level=0
-
 #![allow(unused_variables)]
 
 mod safe {

tests/compile-fail/stacked_borrows/buggy_split_at_mut.rs

@@ -1,6 +1,3 @@
-// FIXME: Without retagging, optimization kills finding this problem
-// compile-flags: -Zmir-opt-level=0
-
 #![allow(unused_variables)]
 
 mod safe {

tests/compile-fail/stacked_borrows/illegal_write2.rs

@@ -1,6 +1,3 @@
-// The reborow gets optimized away, so we can only detect this issue without optimizations
-// compile-flags: -Zmir-opt-level=0
-
 #![allow(unused_variables)]
 
 fn main() {

tests/compile-fail/stacked_borrows/illegal_write4.rs

@@ -1,31 +1,13 @@
-// The compiler inserts some reborrows, enable optimizations to
-// get rid of them.
-// compile-flags: -Zmir-opt-level=1
-
 use std::mem;
 
-// This is an example of a piece of code that intuitively seems like we might
-// want to reject it, but that doesn't turn out to be possible.
-
 fn main() {
-    let target = 42;
-    // Make sure a cannot use a raw-tagged `&mut` pointing to a frozen location, not
-    // even to create a raw.
-    let reference = ⌖ // freeze
+    let mut target = 42;
+    // Make sure we cannot use a raw-tagged `&mut` pointing to a frozen location.
+    // Even just creating it unfreezes.
+    let raw = &mut target as *mut _; // let this leak to raw
+    let reference = unsafe { &*raw }; // freeze
     let ptr = reference as *const _ as *mut i32; // raw ptr, with raw tag
-    let mut_ref: &mut i32 = unsafe { mem::transmute(ptr) }; // &mut, with raw tag
-    // Now we have an &mut to a frozen location, but that is completely normal:
-    // We'd just unfreeze the location if we used it.
-    let bad_ptr = mut_ref as *mut i32; // even just creating this is like a use of `mut_ref`.
-    // That violates the location being frozen!  However, we do not properly detect this:
-    // We first see a `&mut` with a `Raw` tag being deref'd for a frozen location,
-    // which can happen legitimately if the compiler optimized away an `&mut*` that
-    // turns a raw into a `&mut`.  Next, we create a raw ref to a frozen location
-    // from a `Raw` tag, which can happen legitimately when interior mutability
-    // is involved.
-    let _val = *reference; // Make sure it is still frozen.
-
-    // We only actually unfreeze once we muteate through the bad pointer.
-    unsafe { *bad_ptr = 42 }; //~ ERROR does not exist on the stack
-    let _val = *reference;
+    let _mut_ref: &mut i32 = unsafe { mem::transmute(ptr) }; // &mut, with raw tag
+    // Now we retag, making our ref top-of-stack -- and, in particular, unfreezing.
+    let _val = *reference; //~ ERROR Location should be frozen
 }

tests/compile-fail/stacked_borrows/shared_confusion.rs

@@ -1,3 +1,11 @@
+// Optimization kills all the reborrows, enough to make this error go away.  There are
+// no retags either because we don't retag immediately after a `&[mut]`; we rely on
+// that creating a fresh reference.
+// See `shared_confusion_opt.rs` for a variant that is caught even with optimizations.
+// Keep this test to make sure that without optimizations, we do not have to actually
+// use the `x_inner_shr`.
+// compile-flags: -Zmir-opt-level=0
+
 #![allow(unused_variables)]
 use std::cell::RefCell;
 
@@ -9,7 +17,7 @@ fn test(r: &mut RefCell<i32>) {
     {
         let x_inner_shr = &*x_inner; // frozen
         let y = &*r; // outer ref, not freezing
-        let x_inner_shr2 = &*x_inner; // freezing again
+        let x_inner_shr = &*x_inner; // freezing again
     }
     // Our old raw should be dead by now
     unsafe { *x_evil = 0; } // this falls back to some Raw higher up the stack

tests/compile-fail/stacked_borrows/shared_confusion_opt.rs

@@ -0,0 +1,25 @@
+// A variant of `shared_confusion.rs` that gets flagged even with optimizations.
+
+#![allow(unused_variables)]
+use std::cell::RefCell;
+
+fn test(r: &mut RefCell<i32>) {
+    let x = &*r; // not freezing because interior mutability
+    let mut x_ref = x.borrow_mut();
+    let x_inner : &mut i32 = &mut *x_ref; // Uniq reference
+    let x_evil = x_inner as *mut _;
+    {
+        let x_inner_shr = &*x_inner; // frozen
+        let _val = *x_inner_shr;
+        let y = &*r; // outer ref, not freezing
+        let x_inner_shr = &*x_inner; // freezing again
+        let _val = *x_inner_shr;
+    }
+    // Our old raw should be dead by now
+    unsafe { *x_evil = 0; } // this falls back to some Raw higher up the stack
+    *x_inner = 12; //~ ERROR Mut reference with non-reactivatable tag
+}
+
+fn main() {
+    test(&mut RefCell::new(0));
+}