Merge branch 'master' into stacked-borrows-2-phase

Author: RalfJung

rust-version

@@ -1 +1 @@
-nightly-2018-11-30
+nightly-2018-12-03

src/bin/cargo-miri.rs

@@ -155,6 +155,9 @@ fn setup(ask_user: bool) {
     File::create(dir.join("Xargo.toml")).unwrap()
         .write_all(br#"
 [dependencies.std]
+default_features = false
+# We need the `panic_unwind` feature because we use the `unwind` panic strategy.
+# Using `abort` works for libstd, but then libtest will not compile.
 features = ["panic_unwind"]
 
 [dependencies.test]

src/lib.rs

@@ -310,26 +310,9 @@ impl<'a, 'mir, 'tcx> Machine<'a, 'mir, 'tcx> for Evaluator<'tcx> {
 
     const STATIC_KIND: Option<MiriMemoryKind> = Some(MiriMemoryKind::MutStatic);
 
+    #[inline(always)]
     fn enforce_validity(ecx: &EvalContext<'a, 'mir, 'tcx, Self>) -> bool {
-        if !ecx.machine.validate {
-            return false;
-        }
-
-        // Some functions are whitelisted until we figure out how to fix them.
-        // We walk up the stack a few frames to also cover their callees.
-        const WHITELIST: &[(&str, &str)] = &[
-            // Uses mem::uninitialized
-            ("std::sys::windows::mutex::Mutex::", ""),
-        ];
-        for frame in ecx.stack().iter()
-            .rev().take(3)
-        {
-            let name = frame.instance.to_string();
-            if WHITELIST.iter().any(|(prefix, suffix)| name.starts_with(prefix) && name.ends_with(suffix)) {
-                return false;
-            }
-        }
-        true
+        ecx.machine.validate
     }
 
     /// Returns Ok() when the function was handled, fail otherwise

src/stacked_borrows.rs

@@ -303,14 +303,28 @@ impl<'tcx> Stack {
     /// is met: We cannot push `Uniq` onto frozen stacks.
     /// `kind` indicates which kind of reference is being created.
     fn create(&mut self, bor: Borrow, kind: RefKind) {
-        if self.frozen_since.is_some() {
-            // A frozen location?  Possible if we create a barrier, then push again.
-            assert!(bor.is_shared(), "We should never try creating a unique borrow for a frozen stack");
-            trace!("create: Not doing anything on frozen location");
+        // When creating a frozen reference, freeze.  This ensures F1.
+        // We also do *not* push anything else to the stack, making sure that no nother kind
+        // of access (like writing through raw pointers) is permitted.
+        if kind == RefKind::Frozen {
+            let bor_t = match bor {
+                Borrow::Shr(Some(t)) => t,
+                _ => bug!("Creating illegal borrow {:?} for frozen ref", bor),
+            };
+            // It is possible that we already are frozen (e.g. if we just pushed a barrier,
+            // the redundancy check would not have kicked in).
+            match self.frozen_since {
+                Some(loc_t) => assert!(loc_t <= bor_t, "Trying to freeze location for longer than it was already frozen"),
+                None => {
+                    trace!("create: Freezing");
+                    self.frozen_since = Some(bor_t);
+                }
+            }
             return;
         }
-        // First, push.  We do this even if we will later freeze, because we
-        // will allow mutation of shared data at the expense of unfreezing.
+        assert!(self.frozen_since.is_none(), "Trying to create non-frozen reference to frozen location");
+
+        // Push new item to the stack.
         let itm = match bor {
             Borrow::Uniq(t) => BorStackItem::Uniq(t),
             Borrow::Shr(_) => BorStackItem::Shr,
@@ -325,15 +339,6 @@ impl<'tcx> Stack {
             trace!("create: Pushing {:?}", itm);
             self.borrows.push(itm);
         }
-        // Then, maybe freeze.  This is part 2 of ensuring F1.
-        if kind == RefKind::Frozen {
-            let bor_t = match bor {
-                Borrow::Shr(Some(t)) => t,
-                _ => bug!("Creating illegal borrow {:?} for frozen ref", bor),
-            };
-            trace!("create: Freezing");
-            self.frozen_since = Some(bor_t);
-        }
     }
 
     /// Add a barrier

tests/compile-fail-fullmir/stacked_borrows/illegal_write3.rs

@@ -3,6 +3,6 @@ fn main() {
     // Make sure raw ptr with raw tag cannot mutate frozen location without breaking the shared ref.
     let r#ref = ⌖ // freeze
     let ptr = r#ref as *const _ as *mut _; // raw ptr, with raw tag
-    unsafe { *ptr = 42; }
-    let _val = *r#ref; //~ ERROR is not frozen
+    unsafe { *ptr = 42; } //~ ERROR does not exist on the stack
+    let _val = *r#ref;
 }

tests/compile-fail-fullmir/stacked_borrows/pass_invalid_shr.rs

@@ -3,7 +3,7 @@ fn foo(_: &i32) {}
 
 fn main() {
     let x = &mut 42;
-    let xraw = &*x as *const _ as *mut _;
+    let xraw = x as *mut _;
     let xref = unsafe { &*xraw };
     unsafe { *xraw = 42 }; // unfreeze
     foo(xref); //~ ERROR is not frozen

tests/compile-fail-fullmir/stacked_borrows/shr_frozen_violation1.rs

@@ -0,0 +1,16 @@
+fn foo(x: &mut i32) -> i32 {
+  *x = 5;
+  unknown_code(&*x);
+  *x // must return 5
+}
+
+fn main() {
+    println!("{}", foo(&mut 0));
+}
+
+// If we replace the `*const` by `&`, my current dev version of miri
+// *does* find the problem, but not for a good reason: It finds it because
+// of barriers, and we shouldn't rely on unknown code using barriers.
+fn unknown_code(x: *const i32) {
+    unsafe { *(x as *mut i32) = 7; } //~ ERROR barrier
+}

tests/compiletest.rs

@@ -124,16 +124,33 @@ fn is_target_dir<P: Into<PathBuf>>(path: P) -> bool {
     path.metadata().map(|m| m.is_dir()).unwrap_or(false)
 }
 
-fn for_all_targets<F: FnMut(String)>(sysroot: &Path, mut f: F) {
+fn target_has_std<P: Into<PathBuf>>(path: P) -> bool {
+    let mut path = path.into();
+    path.push("lib");
+    std::fs::read_dir(path)
+        .expect("invalid target")
+        .map(|entry| entry.unwrap())
+        .filter(|entry| entry.file_type().unwrap().is_file())
+        .filter_map(|entry| entry.file_name().into_string().ok())
+        .any(|file_name| file_name.starts_with("libstd") && file_name.ends_with(".rlib"))
+}
+
+
+fn for_all_targets<F: FnMut(String)>(sysroot: &Path, f: F) {
     let target_dir = sysroot.join("lib").join("rustlib");
-    for entry in std::fs::read_dir(target_dir).expect("invalid sysroot") {
-        let entry = entry.unwrap();
-        if !is_target_dir(entry.path()) {
-            continue;
-        }
-        let target = entry.file_name().into_string().unwrap();
-        f(target);
+    let mut targets = std::fs::read_dir(target_dir)
+        .expect("invalid sysroot")
+        .map(|entry| entry.unwrap())
+        .filter(|entry| is_target_dir(entry.path()))
+        .filter(|entry| target_has_std(entry.path()))
+        .map(|entry| entry.file_name().into_string().unwrap())
+        .peekable();
+
+    if targets.peek().is_none() {
+        panic!("No valid targets found");
     }
+
+    targets.for_each(f);
 }
 
 fn get_sysroot() -> PathBuf {

tests/run-pass-fullmir/vecdeque.rs

@@ -1,3 +1,6 @@
+// FIXME: Validation disabled until https://github.com/rust-lang/rust/pull/56161 lands
+// compile-flags: -Zmiri-disable-validation
+
 use std::collections::VecDeque;
 
 fn main() {

tests/run-pass/refcell.rs

@@ -39,6 +39,13 @@ fn aliasing_mut_and_shr() {
         *aliasing += 4;
         let _shr = &*rc;
         *aliasing += 4;
+        // also turning this into a frozen ref now must work
+        let aliasing = &*aliasing;
+        let _val = *aliasing;
+        let _escape_to_raw = rc as *const _; // this must NOT unfreeze
+        let _val = *aliasing;
+        let _shr = &*rc; // this must NOT unfreeze
+        let _val = *aliasing;
     }
 
     let rc = RefCell::new(23);
@@ -48,7 +55,23 @@ fn aliasing_mut_and_shr() {
     assert_eq!(*rc.borrow(), 23+12);
 }
 
+fn aliasing_frz_and_shr() {
+    fn inner(rc: &RefCell<i32>, aliasing: &i32) {
+        let _val = *aliasing;
+        let _escape_to_raw = rc as *const _; // this must NOT unfreeze
+        let _val = *aliasing;
+        let _shr = &*rc; // this must NOT unfreeze
+        let _val = *aliasing;
+    }
+
+    let rc = RefCell::new(23);
+    let bshr = rc.borrow();
+    inner(&rc, &*bshr);
+    assert_eq!(*rc.borrow(), 23);
+}
+
 fn main() {
     lots_of_funny_borrows();
     aliasing_mut_and_shr();
+    aliasing_frz_and_shr();
 }

tests/run-pass/stacked-borrows.rs

@@ -4,10 +4,11 @@ fn main() {
     read_does_not_invalidate1();
     read_does_not_invalidate2();
     ref_raw_int_raw();
-    mut_shr_raw();
     mut_raw_then_mut_shr();
+    mut_shr_then_mut_raw();
     mut_raw_mut();
     partially_invalidate_mut();
+    drop_after_sharing();
 }
 
 // Deref a raw ptr to access a field of a large struct, where the field
@@ -53,18 +54,6 @@ fn ref_raw_int_raw() {
     assert_eq!(unsafe { *xraw }, 3);
 }
 
-// Creating a raw from a `&mut` through an `&` works, even if we
-// write through that raw.
-fn mut_shr_raw() {
-    let mut x = 2;
-    {
-        let xref = &mut x;
-        let xraw = &*xref as *const i32 as *mut i32;
-        unsafe { *xraw = 4; }
-    }
-    assert_eq!(x, 4);
-}
-
 // Escape a mut to raw, then share the same mut and use the share, then the raw.
 // That should work.
 fn mut_raw_then_mut_shr() {
@@ -77,6 +66,16 @@ fn mut_raw_then_mut_shr() {
     assert_eq!(x, 4);
 }
 
+// Create first a shared reference and then a raw pointer from a `&mut`
+// should permit mutation through that raw pointer.
+fn mut_shr_then_mut_raw() {
+    let xref = &mut 2;
+    let _xshr = &*xref;
+    let xraw = xref as *mut _;
+    unsafe { *xraw = 3; }
+    assert_eq!(*xref, 3);
+}
+
 // Ensure that if we derive from a mut a raw, and then from that a mut,
 // and then read through the original mut, that does not invalidate the raw.
 // This shows that the read-exception for `&mut` applies even if the `Shr` item
@@ -107,3 +106,9 @@ fn partially_invalidate_mut() {
     *shard += 1; // so we can still use `shard`.
     assert_eq!(*data, (1, 1));
 }
+
+// Make sure that we can handle the situation where a loaction is frozen when being dropped.
+fn drop_after_sharing() {
+    let x = String::from("hello!");
+    let _len = x.len();
+}