Merge pull request #524 from RalfJung/escape-to-raw

Stacked Borrows beautififcation, update for EscapeToRaw

Author: RalfJung

.gitignore

@@ -2,6 +2,5 @@ target
 /doc
 tex/*/out
 *.dot
-*.mir
 *.rs.bk
 Cargo.lock

README.md

@@ -45,14 +45,19 @@ in this directory.
 ## Running Miri
 
 ```sh
-cargo +nightly run tests/run-pass/vecs.rs # Or whatever test you like.
+cargo +nightly run -- -Zmiri-disable-validation tests/run-pass/vecs.rs # Or whatever test you like.
 ```
 
+We have to disable validation because that can lead to errors when libstd is not
+compiled the right way.
+
 ## Running Miri with full libstd
 
-Per default libstd does not contain the MIR of non-polymorphic functions. When
-Miri hits a call to such a function, execution terminates. To fix this, it is
-possible to compile libstd with full MIR:
+Per default libstd does not contain the MIR of non-polymorphic functions, and
+also does not contain some extra MIR statements that miri needs for validation.
+When Miri hits a call to such a function, execution terminates, and even when
+the MIR is present, validation can fail.  To fix this, it is possible to compile
+libstd with full MIR:
 
 ```sh
 rustup component add --toolchain nightly rust-src

rust-version

@@ -1 +1 @@
-nightly-2018-11-15
+nightly-2018-11-16

src/fn_call.rs

@@ -555,7 +555,7 @@ impl<'a, 'mir, 'tcx: 'mir + 'a> EvalContextExt<'tcx, 'mir> for super::MiriEvalCo
             }
             "pthread_attr_getstack" => {
                 // second argument is where we are supposed to write the stack size
-                let ptr = self.ref_to_mplace(self.read_immediate(args[1])?)?;
+                let ptr = self.deref_operand(args[1])?;
                 let stackaddr = Scalar::from_int(0x80000, args[1].layout.size); // just any address
                 self.write_scalar(stackaddr, ptr.into())?;
                 // return 0

src/helpers.rs

@@ -33,13 +33,13 @@ 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(
+    /// Visit the memory covered by `place`, sensitive to freezing:  The 3rd parameter
+    /// will be true if this is frozen, false if this is in an `UnsafeCell`.
+    fn visit_freeze_sensitive(
         &self,
         place: MPlaceTy<'tcx, Borrow>,
         size: Size,
-        action: impl FnMut(Pointer<Borrow>, Size) -> EvalResult<'tcx>,
+        action: impl FnMut(Pointer<Borrow>, Size, bool) -> EvalResult<'tcx>,
     ) -> EvalResult<'tcx>;
 }
 
@@ -79,13 +79,11 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for EvalContext<'a, 'mir, 'tcx, super:
             })
     }
 
-    /// Visit the memory covered by `place` that is frozen -- i.e., NOT
-    /// what is inside an `UnsafeCell`.
-    fn visit_frozen(
+    fn visit_freeze_sensitive(
         &self,
         place: MPlaceTy<'tcx, Borrow>,
         size: Size,
-        mut frozen_action: impl FnMut(Pointer<Borrow>, Size) -> EvalResult<'tcx>,
+        mut action: impl FnMut(Pointer<Borrow>, Size, bool) -> EvalResult<'tcx>,
     ) -> EvalResult<'tcx> {
         trace!("visit_frozen(place={:?}, size={:?})", *place, size);
         debug_assert_eq!(size,
@@ -99,18 +97,29 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for EvalContext<'a, 'mir, 'tcx, super:
         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| {
+        let mut unsafe_cell_action = |unsafe_cell_ptr: Scalar<Borrow>, unsafe_cell_size: Size| {
+            if unsafe_cell_size != Size::ZERO {
+                debug_assert_eq!(unsafe_cell_ptr.to_ptr().unwrap().alloc_id,
+                    end_ptr.to_ptr().unwrap().alloc_id);
+                debug_assert_eq!(unsafe_cell_ptr.to_ptr().unwrap().tag,
+                    end_ptr.to_ptr().unwrap().tag);
+            }
             // We assume that we are given the fields in increasing offset order,
             // and nothing else changes.
+            let unsafe_cell_offset = unsafe_cell_ptr.get_ptr_offset(self);
             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)?;
+                action(end_ptr.to_ptr()?, frozen_size, /*frozen*/true)?;
+            }
+            // This `UnsafeCell` is NOT frozen.
+            if unsafe_cell_size != Size::ZERO {
+                action(unsafe_cell_ptr.to_ptr()?, unsafe_cell_size, /*frozen*/false)?;
             }
             // Update end end_ptr.
-            end_ptr = end_ptr.ptr_wrapping_offset(frozen_size+unsafe_cell_size, self);
+            end_ptr = unsafe_cell_ptr.ptr_wrapping_offset(unsafe_cell_size, self);
             // Done
             Ok(())
         };
@@ -126,7 +135,7 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for EvalContext<'a, 'mir, 'tcx, super:
                         .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)
+                        unsafe_cell_action(place.ptr, unsafe_cell_size)
                     } else {
                         Ok(())
                     }
@@ -136,7 +145,7 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for EvalContext<'a, 'mir, 'tcx, super:
         }
         // 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)?;
+        unsafe_cell_action(place.ptr.ptr_wrapping_offset(size, self), Size::ZERO)?;
         // Done!
         return Ok(());
 

src/intrinsic.rs

@@ -59,7 +59,7 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, '
             "atomic_load_relaxed" |
             "atomic_load_acq" |
             "volatile_load" => {
-                let ptr = self.ref_to_mplace(self.read_immediate(args[0])?)?;
+                let ptr = self.deref_operand(args[0])?;
                 let val = self.read_scalar(ptr.into())?; // make sure it fits into a scalar; otherwise it cannot be atomic
                 self.write_scalar(val, dest)?;
             }
@@ -68,7 +68,7 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, '
             "atomic_store_relaxed" |
             "atomic_store_rel" |
             "volatile_store" => {
-                let ptr = self.ref_to_mplace(self.read_immediate(args[0])?)?;
+                let ptr = self.deref_operand(args[0])?;
                 let val = self.read_scalar(args[1])?; // make sure it fits into a scalar; otherwise it cannot be atomic
                 self.write_scalar(val, ptr.into())?;
             }
@@ -78,18 +78,18 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, '
             }
 
             _ if intrinsic_name.starts_with("atomic_xchg") => {
-                let ptr = self.ref_to_mplace(self.read_immediate(args[0])?)?;
+                let ptr = self.deref_operand(args[0])?;
                 let new = self.read_scalar(args[1])?;
                 let old = self.read_scalar(ptr.into())?;
                 self.write_scalar(old, dest)?; // old value is returned
                 self.write_scalar(new, ptr.into())?;
             }
 
             _ if intrinsic_name.starts_with("atomic_cxchg") => {
-                let ptr = self.ref_to_mplace(self.read_immediate(args[0])?)?;
-                let expect_old = self.read_immediate(args[1])?; // read as value for the sake of `binary_op_imm()`
+                let ptr = self.deref_operand(args[0])?;
+                let expect_old = self.read_immediate(args[1])?; // read as immediate for the sake of `binary_op_imm()`
                 let new = self.read_scalar(args[2])?;
-                let old = self.read_immediate(ptr.into())?; // read as value for the sake of `binary_op_imm()`
+                let old = self.read_immediate(ptr.into())?; // read as immediate for the sake of `binary_op_imm()`
                 // binary_op_imm will bail if either of them is not a scalar
                 let (eq, _) = self.binary_op_imm(mir::BinOp::Eq, old, expect_old)?;
                 let res = Immediate::ScalarPair(old.to_scalar_or_undef(), eq.into());
@@ -125,7 +125,7 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, '
             "atomic_xsub_rel" |
             "atomic_xsub_acqrel" |
             "atomic_xsub_relaxed" => {
-                let ptr = self.ref_to_mplace(self.read_immediate(args[0])?)?;
+                let ptr = self.deref_operand(args[0])?;
                 if !ptr.layout.ty.is_integral() {
                     return err!(Unimplemented(format!("Atomic arithmetic operations only work on integer types")));
                 }
@@ -167,7 +167,7 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, '
             }
 
             "discriminant_value" => {
-                let place = self.ref_to_mplace(self.read_immediate(args[0])?)?;
+                let place = self.deref_operand(args[0])?;
                 let discr_val = self.read_discriminant(place.into())?.0;
                 self.write_scalar(Scalar::from_uint(discr_val, dest.layout.size), dest)?;
             }
@@ -279,7 +279,7 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, '
             }
 
             "move_val_init" => {
-                let ptr = self.ref_to_mplace(self.read_immediate(args[0])?)?;
+                let ptr = self.deref_operand(args[0])?;
                 self.copy_op(args[1], ptr.into())?;
             }
 
@@ -347,7 +347,7 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, '
             }
 
             "size_of_val" => {
-                let mplace = self.ref_to_mplace(self.read_immediate(args[0])?)?;
+                let mplace = self.deref_operand(args[0])?;
                 let (size, _) = self.size_and_align_of_mplace(mplace)?
                     .expect("size_of_val called on extern type");
                 let ptr_size = self.pointer_size();
@@ -359,7 +359,7 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, '
 
             "min_align_of_val" |
             "align_of_val" => {
-                let mplace = self.ref_to_mplace(self.read_immediate(args[0])?)?;
+                let mplace = self.deref_operand(args[0])?;
                 let (_, align) = self.size_and_align_of_mplace(mplace)?
                     .expect("size_of_val called on extern type");
                 let ptr_size = self.pointer_size();

src/lib.rs

@@ -296,7 +296,6 @@ impl<'a, 'mir, 'tcx> Machine<'a, 'mir, 'tcx> for Evaluator<'tcx> {
 
     type AllocExtra = stacked_borrows::Stacks;
     type PointerTag = Borrow;
-    const ENABLE_PTR_TRACKING_HOOKS: bool = true;
 
     type MemoryMap = MonoHashMap<AllocId, (MemoryKind<MiriMemoryKind>, Allocation<Borrow, Self::AllocExtra>)>;
 
@@ -309,16 +308,18 @@ impl<'a, 'mir, 'tcx> Machine<'a, 'mir, 'tcx> for Evaluator<'tcx> {
 
         // 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] = &[
+        const WHITELIST: &[(&str, &str)] = &[
             // Uses mem::uninitialized
-            "std::ptr::read",
-            "std::sys::windows::mutex::Mutex::",
+            ("std::ptr::read", ""),
+            ("std::sys::windows::mutex::Mutex::", ""),
+            // Should directly take a raw reference
+            ("<std::cell::UnsafeCell<T>>", "::get"),
         ];
         for frame in ecx.stack().iter()
             .rev().take(3)
         {
             let name = frame.instance.to_string();
-            if WHITELIST.iter().any(|white| name.starts_with(white)) {
+            if WHITELIST.iter().any(|(prefix, suffix)| name.starts_with(prefix) && name.ends_with(suffix)) {
                 return false;
             }
         }
@@ -446,26 +447,6 @@ impl<'a, 'mir, 'tcx> Machine<'a, 'mir, 'tcx> for Evaluator<'tcx> {
         Cow::Owned(alloc)
     }
 
-    #[inline(always)]
-    fn tag_reference(
-        ecx: &mut EvalContext<'a, 'mir, 'tcx, Self>,
-        place: MPlaceTy<'tcx, Borrow>,
-        mutability: Option<hir::Mutability>,
-    ) -> 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.ptr)
-        } else {
-            let ptr = place.ptr.to_ptr()?;
-            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: MPlaceTy<'tcx, Borrow>,
@@ -474,11 +455,13 @@ impl<'a, 'mir, 'tcx> Machine<'a, 'mir, 'tcx> for Evaluator<'tcx> {
         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 {
+        if !ecx.tcx.sess.opts.debugging_opts.mir_emit_retag ||
+            !Self::enforce_validity(ecx) || size == Size::ZERO
+        {
             // No tracking
             Ok(place.ptr)
         } else {
-            let ptr = place.ptr.to_ptr()?;
+            let ptr = place.ptr.to_ptr()?; // assert this is not a scalar
             let tag = ecx.tag_dereference(place, size, mutability.into())?;
             Ok(Scalar::Ptr(Pointer::new_with_tag(ptr.alloc_id, ptr.offset, tag)))
         }
@@ -499,19 +482,46 @@ impl<'a, 'mir, 'tcx> Machine<'a, 'mir, 'tcx> for Evaluator<'tcx> {
         }
     }
 
+    #[inline]
+    fn escape_to_raw(
+        ecx: &mut EvalContext<'a, 'mir, 'tcx, Self>,
+        ptr: OpTy<'tcx, Self::PointerTag>,
+    ) -> EvalResult<'tcx> {
+        // It is tempting to check the type here, but drop glue does EscapeToRaw
+        // on a raw pointer.
+        // This is deliberately NOT `deref_operand` as we do not want `tag_dereference`
+        // to be called!  That would kill the original tag if we got a raw ptr.
+        let place = ecx.ref_to_mplace(ecx.read_immediate(ptr)?)?;
+        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.tcx.sess.opts.debugging_opts.mir_emit_retag ||
+            !ecx.machine.validate || size == Size::ZERO
+        {
+            // No tracking, or no retagging. The latter is possible because a dependency of ours
+            // might be called with different flags than we are, so there are `Retag`
+            // statements but we do not want to execute them.
+            Ok(())
+        } else {
+            ecx.escape_to_raw(place, size)
+        }
+    }
+
     #[inline(always)]
     fn retag(
         ecx: &mut EvalContext<'a, 'mir, 'tcx, Self>,
         fn_entry: bool,
         place: PlaceTy<'tcx, Borrow>,
     ) -> EvalResult<'tcx> {
         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,
+            // No tracking, or no retagging. The latter is possible because a dependency of ours
+            // might be called with different flags than we are, so there are `Retag`
+            // statements but we do not want to execute them.
             // Also, honor the whitelist in `enforce_validity` because otherwise we might retag
             // uninitialized data.
-            return Ok(())
+             Ok(())
+        } else {
+            ecx.retag(fn_entry, place)
         }
-        ecx.retag(fn_entry, place)
     }
 }