Merge pull request #513 from RalfJung/new-interior-mut

New Stacked Borrows, now with better support for interior mutability

Author: oli-obk

cargo-miri-test/run-test.py

@@ -10,9 +10,8 @@
 def test_cargo_miri():
     print("==> Testing `cargo miri` <==")
     ## Call `cargo miri`, capture all output
-    # FIXME: Disabling validation, still investigating whether there is UB here
     p = subprocess.Popen(
-        ["cargo", "miri", "-q", "--", "-Zmiri-disable-validation"],
+        ["cargo", "miri", "-q"],
         stdout=subprocess.PIPE,
         stderr=subprocess.PIPE
     )

rust-version

@@ -1 +1 @@
-nightly-2018-11-07
+nightly-2018-11-08

src/helpers.rs

@@ -1,6 +1,6 @@
 use std::mem;
 
-use rustc::ty;
+use rustc::ty::{self, layout};
 use rustc::hir::def_id::{DefId, CRATE_DEF_INDEX};
 
 use crate::*;
@@ -32,6 +32,15 @@ impl<Tag> ScalarExt for ScalarMaybeUndef<Tag> {
 
 pub trait EvalContextExt<'tcx> {
     fn resolve_path(&self, path: &[&str]) -> EvalResult<'tcx, ty::Instance<'tcx>>;
+
+    /// Visit the memory covered by `place` that is frozen -- i.e., NOT
+    /// what is inside an `UnsafeCell`.
+    fn visit_frozen(
+        &self,
+        place: MPlaceTy<'tcx, Borrow>,
+        size: Size,
+        action: impl FnMut(Pointer<Borrow>, Size) -> EvalResult<'tcx>,
+    ) -> EvalResult<'tcx>;
 }
 
 
@@ -69,4 +78,153 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for EvalContext<'a, 'mir, 'tcx, super:
                 EvalErrorKind::PathNotFound(path).into()
             })
     }
+
+    /// Visit the memory covered by `place` that is frozen -- i.e., NOT
+    /// what is inside an `UnsafeCell`.
+    fn visit_frozen(
+        &self,
+        place: MPlaceTy<'tcx, Borrow>,
+        size: Size,
+        mut frozen_action: impl FnMut(Pointer<Borrow>, Size) -> EvalResult<'tcx>,
+    ) -> EvalResult<'tcx> {
+        trace!("visit_frozen(place={:?}, size={:?})", *place, size);
+        debug_assert_eq!(size,
+            self.size_and_align_of_mplace(place)?
+            .map(|(size, _)| size)
+            .unwrap_or_else(|| place.layout.size)
+        );
+        // Store how far we proceeded into the place so far.  Everything to the left of
+        // this offset has already been handled, in the sense that the frozen parts
+        // have had `action` called on them.
+        let mut end_ptr = place.ptr;
+        // Called when we detected an `UnsafeCell` at the given offset and size.
+        // Calls `action` and advances `end_ptr`.
+        let mut unsafe_cell_action = |unsafe_cell_offset, unsafe_cell_size| {
+            // We assume that we are given the fields in increasing offset order,
+            // and nothing else changes.
+            let end_offset = end_ptr.get_ptr_offset(self);
+            assert!(unsafe_cell_offset >= end_offset);
+            let frozen_size = unsafe_cell_offset - end_offset;
+            // Everything between the end_ptr and this `UnsafeCell` is frozen.
+            if frozen_size != Size::ZERO {
+                frozen_action(end_ptr.to_ptr()?, frozen_size)?;
+            }
+            // Update end end_ptr.
+            end_ptr = end_ptr.ptr_wrapping_offset(frozen_size+unsafe_cell_size, self);
+            // Done
+            Ok(())
+        };
+        // Run a visitor
+        {
+            let mut visitor = UnsafeCellVisitor {
+                ecx: self,
+                unsafe_cell_action: |place| {
+                    trace!("unsafe_cell_action on {:?}", place.ptr);
+                    // We need a size to go on.
+                    let (unsafe_cell_size, _) = self.size_and_align_of_mplace(place)?
+                        // for extern types, just cover what we can
+                        .unwrap_or_else(|| place.layout.size_and_align());
+                    // Now handle this `UnsafeCell`, unless it is empty.
+                    if unsafe_cell_size != Size::ZERO {
+                        unsafe_cell_action(place.ptr.get_ptr_offset(self), unsafe_cell_size)
+                    } else {
+                        Ok(())
+                    }
+                },
+            };
+            visitor.visit_value(place)?;
+        }
+        // The part between the end_ptr and the end of the place is also frozen.
+        // So pretend there is a 0-sized `UnsafeCell` at the end.
+        unsafe_cell_action(place.ptr.get_ptr_offset(self) + size, Size::ZERO)?;
+        // Done!
+        return Ok(());
+
+        /// Visiting the memory covered by a `MemPlace`, being aware of
+        /// whether we are inside an `UnsafeCell` or not.
+        struct UnsafeCellVisitor<'ecx, 'a, 'mir, 'tcx, F>
+            where F: FnMut(MPlaceTy<'tcx, Borrow>) -> EvalResult<'tcx>
+        {
+            ecx: &'ecx MiriEvalContext<'a, 'mir, 'tcx>,
+            unsafe_cell_action: F,
+        }
+
+        impl<'ecx, 'a, 'mir, 'tcx, F> ValueVisitor<'a, 'mir, 'tcx, Evaluator<'tcx>>
+        for UnsafeCellVisitor<'ecx, 'a, 'mir, 'tcx, F>
+        where
+            F: FnMut(MPlaceTy<'tcx, Borrow>) -> EvalResult<'tcx>
+        {
+            type V = MPlaceTy<'tcx, Borrow>;
+
+            #[inline(always)]
+            fn ecx(&self) -> &MiriEvalContext<'a, 'mir, 'tcx> {
+                &self.ecx
+            }
+
+            // Hook to detect `UnsafeCell`
+            fn visit_value(&mut self, v: MPlaceTy<'tcx, Borrow>) -> EvalResult<'tcx>
+            {
+                trace!("UnsafeCellVisitor: {:?} {:?}", *v, v.layout.ty);
+                let is_unsafe_cell = match v.layout.ty.sty {
+                    ty::Adt(adt, _) => Some(adt.did) == self.ecx.tcx.lang_items().unsafe_cell_type(),
+                    _ => false,
+                };
+                if is_unsafe_cell {
+                    // We do not have to recurse further, this is an `UnsafeCell`.
+                    (self.unsafe_cell_action)(v)
+                } else if self.ecx.type_is_freeze(v.layout.ty) {
+                    // This is `Freeze`, there cannot be an `UnsafeCell`
+                    Ok(())
+                } else {
+                    // Proceed further
+                    self.walk_value(v)
+                }
+            }
+
+            // Make sure we visit aggregrates in increasing offset order
+            fn visit_aggregate(
+                &mut self,
+                place: MPlaceTy<'tcx, Borrow>,
+                fields: impl Iterator<Item=EvalResult<'tcx, MPlaceTy<'tcx, Borrow>>>,
+            ) -> EvalResult<'tcx> {
+                match place.layout.fields {
+                    layout::FieldPlacement::Array { .. } => {
+                        // For the array layout, we know the iterator will yield sorted elements so
+                        // we can avoid the allocation.
+                        self.walk_aggregate(place, fields)
+                    }
+                    layout::FieldPlacement::Arbitrary { .. } => {
+                        // Gather the subplaces and sort them before visiting.
+                        let mut places = fields.collect::<EvalResult<'tcx, Vec<MPlaceTy<'tcx, Borrow>>>>()?;
+                        places[..].sort_by_key(|place| place.ptr.get_ptr_offset(self.ecx()));
+                        self.walk_aggregate(place, places.into_iter().map(Ok))
+                    }
+                    layout::FieldPlacement::Union { .. } => {
+                        // Uh, what?
+                        bug!("A union is not an aggregate we should ever visit")
+                    }
+                }
+            }
+
+            // We have to do *something* for unions
+            fn visit_union(&mut self, v: MPlaceTy<'tcx, Borrow>) -> EvalResult<'tcx>
+            {
+                // With unions, we fall back to whatever the type says, to hopefully be consistent
+                // with LLVM IR.
+                // FIXME Are we consistent?  And is this really the behavior we want?
+                let frozen = self.ecx.type_is_freeze(v.layout.ty);
+                if frozen {
+                    Ok(())
+                } else {
+                    (self.unsafe_cell_action)(v)
+                }
+            }
+
+            // We should never get to a primitive, but always short-circuit somewhere above
+            fn visit_primitive(&mut self, _val: ImmTy<'tcx, Borrow>) -> EvalResult<'tcx>
+            {
+                bug!("We should always short-circit before coming to a primitive")
+            }
+        }
+    }
 }

src/lib.rs

@@ -17,7 +17,7 @@ use std::collections::HashMap;
 use std::borrow::Cow;
 use std::env;
 
-use rustc::ty::{self, Ty, TyCtxt, query::TyCtxtAt};
+use rustc::ty::{self, TyCtxt, query::TyCtxtAt};
 use rustc::ty::layout::{TyLayout, LayoutOf, Size};
 use rustc::hir::{self, def_id::DefId};
 use rustc::mir;
@@ -48,7 +48,7 @@ use crate::mono_hash_map::MonoHashMap;
 use crate::stacked_borrows::{EvalContextExt as StackedBorEvalContextExt};
 
 // Used by priroda
-pub use crate::stacked_borrows::{Borrow, Stack, Stacks, Mut as MutBorrow, BorStackItem};
+pub use crate::stacked_borrows::{Borrow, Stack, Stacks, BorStackItem};
 
 /// Insert rustc arguments at the beginning of the argument list that miri wants to be
 /// set per default, for maximal validation power.
@@ -476,38 +476,38 @@ impl<'a, 'mir, 'tcx> Machine<'a, 'mir, 'tcx> for Evaluator<'tcx> {
     #[inline(always)]
     fn tag_reference(
         ecx: &mut EvalContext<'a, 'mir, 'tcx, Self>,
-        place: MemPlace<Borrow>,
-        ty: Ty<'tcx>,
-        size: Size,
+        place: MPlaceTy<'tcx, Borrow>,
         mutability: Option<hir::Mutability>,
-    ) -> EvalResult<'tcx, MemPlace<Borrow>> {
+    ) -> EvalResult<'tcx, Scalar<Borrow>> {
+        let (size, _) = ecx.size_and_align_of_mplace(place)?
+            // for extern types, just cover what we can
+            .unwrap_or_else(|| place.layout.size_and_align());
         if !ecx.machine.validate || size == Size::ZERO {
             // No tracking
-            Ok(place)
+            Ok(place.ptr)
         } else {
             let ptr = place.ptr.to_ptr()?;
-            let tag = ecx.tag_reference(ptr, ty, size, mutability.into())?;
-            let ptr = Scalar::Ptr(Pointer::new_with_tag(ptr.alloc_id, ptr.offset, tag));
-            Ok(MemPlace { ptr, ..place })
+            let tag = ecx.tag_reference(place, size, mutability.into())?;
+            Ok(Scalar::Ptr(Pointer::new_with_tag(ptr.alloc_id, ptr.offset, tag)))
         }
     }
 
     #[inline(always)]
     fn tag_dereference(
         ecx: &EvalContext<'a, 'mir, 'tcx, Self>,
-        place: MemPlace<Borrow>,
-        ty: Ty<'tcx>,
-        size: Size,
+        place: MPlaceTy<'tcx, Borrow>,
         mutability: Option<hir::Mutability>,
-    ) -> EvalResult<'tcx, MemPlace<Borrow>> {
+    ) -> EvalResult<'tcx, Scalar<Borrow>> {
+        let (size, _) = ecx.size_and_align_of_mplace(place)?
+            // for extern types, just cover what we can
+            .unwrap_or_else(|| place.layout.size_and_align());
         if !ecx.machine.validate || size == Size::ZERO {
             // No tracking
-            Ok(place)
+            Ok(place.ptr)
         } else {
             let ptr = place.ptr.to_ptr()?;
-            let tag = ecx.tag_dereference(ptr, ty, size, mutability.into())?;
-            let ptr = Scalar::Ptr(Pointer::new_with_tag(ptr.alloc_id, ptr.offset, tag));
-            Ok(MemPlace { ptr, ..place })
+            let tag = ecx.tag_dereference(place, size, mutability.into())?;
+            Ok(Scalar::Ptr(Pointer::new_with_tag(ptr.alloc_id, ptr.offset, tag)))
         }
     }