Auto merge of #872 - RalfJung:retag-shallow, r=oli-obk

Make Retag shallow

A shallow retag does not traverse into fields of compound typed to search for references to retag. It only retags "top-level"/"bare" references (and boxes).

This helps with rust-lang/unsafe-code-guidelines#125 because it also means that we do not add protectors for references passed in fields of a struct (or other compound types). Until we know what the rules should be for protectors, I prefer to be less aggressive about what we are rejecting.
This also matches our work-in-progress Coq formalization.

src/stacked_borrows.rs

@@ -12,7 +12,7 @@ use rustc::hir::{MutMutable, MutImmutable};
 use rustc::mir::RetagKind;
 
 use crate::{
-    InterpResult, InterpError, MiriEvalContext, HelpersEvalContextExt, Evaluator, MutValueVisitor,
+    InterpResult, InterpError, HelpersEvalContextExt,
     MemoryKind, MiriMemoryKind, RangeMap, AllocId, Pointer, Immediate, ImmTy, PlaceTy, MPlaceTy,
 };
 
@@ -632,54 +632,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             }
         }
 
-        // We need a visitor to visit all references. However, that requires
-        // a `MemPlace`, so we have a fast path for reference types that
-        // avoids allocating.
+        // We only reborrow "bare" references/boxes.
+        // Not traversing into fields helps with <https://github.com/rust-lang/unsafe-code-guidelines/issues/125>,
+        // but might also cost us optimization and analyses. We will have to experiment more with this.
         if let Some((mutbl, protector)) = qualify(place.layout.ty, kind) {
             // Fast path.
             let val = this.read_immediate(this.place_to_op(place)?)?;
             let val = this.retag_reference(val, mutbl, protector)?;
             this.write_immediate(val, place)?;
-            return Ok(());
-        }
-        let place = this.force_allocation(place)?;
-
-        let mut visitor = RetagVisitor { ecx: this, kind };
-        visitor.visit_value(place)?;
-
-        // The actual visitor.
-        struct RetagVisitor<'ecx, 'mir, 'tcx> {
-            ecx: &'ecx mut MiriEvalContext<'mir, 'tcx>,
-            kind: RetagKind,
-        }
-        impl<'ecx, 'mir, 'tcx>
-            MutValueVisitor<'mir, 'tcx, Evaluator<'tcx>>
-        for
-            RetagVisitor<'ecx, 'mir, 'tcx>
-        {
-            type V = MPlaceTy<'tcx, Tag>;
-
-            #[inline(always)]
-            fn ecx(&mut self) -> &mut MiriEvalContext<'mir, 'tcx> {
-                &mut self.ecx
-            }
-
-            // Primitives of reference type, that is the one thing we are interested in.
-            fn visit_primitive(&mut self, place: MPlaceTy<'tcx, Tag>) -> InterpResult<'tcx>
-            {
-                // Cannot use `builtin_deref` because that reports *immutable* for `Box`,
-                // making it useless.
-                if let Some((mutbl, protector)) = qualify(place.layout.ty, self.kind) {
-                    let val = self.ecx.read_immediate(place.into())?;
-                    let val = self.ecx.retag_reference(
-                        val,
-                        mutbl,
-                        protector
-                    )?;
-                    self.ecx.write_immediate(val, place.into())?;
-                }
-                Ok(())
-            }
         }
 
         Ok(())

tests/compile-fail/stacked_borrows/buggy_split_at_mut.rs

@@ -9,7 +9,6 @@ mod safe {
             assert!(mid <= len);
 
             (from_raw_parts_mut(ptr, len - mid), // BUG: should be "mid" instead of "len - mid"
-            //~^ ERROR borrow stack
             from_raw_parts_mut(ptr.offset(mid as isize), len - mid))
         }
     }
@@ -18,6 +17,7 @@ mod safe {
 fn main() {
     let mut array = [1,2,3,4];
     let (a, b) = safe::split_at_mut(&mut array, 0);
+    //~^ ERROR borrow stack
     a[1] = 5;
     b[1] = 6;
 }

tests/compile-fail/stacked_borrows/return_invalid_mut_option.rs

@@ -1,11 +1,16 @@
 // Make sure that we cannot return a `&mut` that got already invalidated, not even in an `Option`.
+// Due to shallow reborrowing, the error only surfaces when we look into the `Option`.
 fn foo(x: &mut (i32, i32)) -> Option<&mut i32> {
     let xraw = x as *mut (i32, i32);
-    let ret = Some(unsafe { &mut (*xraw).1 });
+    let ret = unsafe { &mut (*xraw).1 }; // let-bind to avoid 2phase
+    let ret = Some(ret);
     let _val = unsafe { *xraw }; // invalidate xref
-    ret //~ ERROR borrow stack
+    ret
 }
 
 fn main() {
-    foo(&mut (1, 2));
+    match foo(&mut (1, 2)) {
+        Some(_x) => {}, //~ ERROR borrow stack
+        None => {},
+    }
 }

tests/compile-fail/stacked_borrows/return_invalid_mut_tuple.rs

@@ -1,11 +1,12 @@
 // Make sure that we cannot return a `&mut` that got already invalidated, not even in a tuple.
+// Due to shallow reborrowing, the error only surfaces when we look into the tuple.
 fn foo(x: &mut (i32, i32)) -> (&mut i32,) {
     let xraw = x as *mut (i32, i32);
     let ret = (unsafe { &mut (*xraw).1 },);
     let _val = unsafe { *xraw }; // invalidate xref
-    ret //~ ERROR borrow stack
+    ret
 }
 
 fn main() {
-    foo(&mut (1, 2));
+    foo(&mut (1, 2)).0; //~ ERROR: borrow stack
 }

tests/compile-fail/stacked_borrows/return_invalid_shr_option.rs

@@ -1,11 +1,15 @@
 // Make sure that we cannot return a `&` that got already invalidated, not even in an `Option`.
+// Due to shallow reborrowing, the error only surfaces when we look into the `Option`.
 fn foo(x: &mut (i32, i32)) -> Option<&i32> {
     let xraw = x as *mut (i32, i32);
     let ret = Some(unsafe { &(*xraw).1 });
     unsafe { *xraw = (42, 23) }; // unfreeze
-    ret //~ ERROR borrow stack
+    ret
 }
 
 fn main() {
-    foo(&mut (1, 2));
+    match foo(&mut (1, 2)) {
+        Some(_x) => {}, //~ ERROR borrow stack
+        None => {},
+    }
 }

tests/compile-fail/stacked_borrows/return_invalid_shr_tuple.rs

@@ -1,11 +1,12 @@
 // Make sure that we cannot return a `&` that got already invalidated, not even in a tuple.
+// Due to shallow reborrowing, the error only surfaces when we look into the tuple.
 fn foo(x: &mut (i32, i32)) -> (&i32,) {
     let xraw = x as *mut (i32, i32);
     let ret = (unsafe { &(*xraw).1 },);
     unsafe { *xraw = (42, 23) }; // unfreeze
-    ret //~ ERROR borrow stack
+    ret
 }
 
 fn main() {
-    foo(&mut (1, 2));
+    foo(&mut (1, 2)).0; //~ ERROR borrow stack
 }

tests/run-pass/stacked-borrows/refcell.rs

@@ -0,0 +1,68 @@
+use std::cell::{RefCell, Ref, RefMut};
+
+fn main() {
+    basic();
+    ref_protector();
+    ref_mut_protector();
+}
+
+fn basic() {
+    let c = RefCell::new(42);
+    {
+        let s1 = c.borrow();
+        let _x: i32 = *s1;
+        let s2 = c.borrow();
+        let _x: i32 = *s1;
+        let _y: i32 = *s2;
+        let _x: i32 = *s1;
+        let _y: i32 = *s2;
+    }
+    {
+        let mut m = c.borrow_mut();
+        let _z: i32 = *m;
+        {
+            let s: &i32 = &*m;
+            let _x = *s;
+        }
+        *m = 23;
+        let _z: i32 = *m;
+    }
+    {
+        let s1 = c.borrow();
+        let _x: i32 = *s1;
+        let s2 = c.borrow();
+        let _x: i32 = *s1;
+        let _y: i32 = *s2;
+        let _x: i32 = *s1;
+        let _y: i32 = *s2;
+    }
+}
+
+// Adding a Stacked Borrows protector for `Ref` would break this
+fn ref_protector() {
+    fn break_it(rc: &RefCell<i32>, r: Ref<'_, i32>) {
+        // `r` has a shared reference, it is passed in as argument and hence
+        // a protector is added that marks this memory as read-only for the entire
+        // duration of this function.
+        drop(r);
+        // *oops* here we can mutate that memory.
+        *rc.borrow_mut() = 2;
+    }
+
+    let rc = RefCell::new(0);
+    break_it(&rc, rc.borrow())
+}
+
+fn ref_mut_protector() {
+    fn break_it(rc: &RefCell<i32>, r: RefMut<'_, i32>) {
+        // `r` has a shared reference, it is passed in as argument and hence
+        // a protector is added that marks this memory as inaccessible for the entire
+        // duration of this function
+        drop(r);
+        // *oops* here we can mutate that memory.
+        *rc.borrow_mut() = 2;
+    }
+
+    let rc = RefCell::new(0);
+    break_it(&rc, rc.borrow_mut())
+}