Auto merge of #55270 - RalfJung:stacked-borrows-ng, r=oli-obk

miri engine: Stacked Borrows NG

For more refined tracking in miri, we do return untagged pointers from the memory abstraction after allocations and let the caller decide how to tag these.

Also refactor the `tag_(de)reference` hooks so they can be more easily called in the ref-to-place and place-to-ref methods, and reorder things in validation: validation calls ref-to-place which (when running in miri) triggers some checks, so we want to run it rather late and catch other problems first. We also do not need to redundantly check the ref to be allocated any more, the checks miri does anyway imply thath.

r? @oli-obk

Author: bors

src/librustc_mir/const_eval.rs

@@ -20,8 +20,8 @@ use rustc::hir::{self, def_id::DefId};
 use rustc::hir::def::Def;
 use rustc::mir::interpret::{ConstEvalErr, ErrorHandled};
 use rustc::mir;
-use rustc::ty::{self, Ty, TyCtxt, Instance, query::TyCtxtAt};
-use rustc::ty::layout::{self, Size, LayoutOf, TyLayout};
+use rustc::ty::{self, TyCtxt, Instance, query::TyCtxtAt};
+use rustc::ty::layout::{self, LayoutOf, TyLayout};
 use rustc::ty::subst::Subst;
 use rustc::traits::Reveal;
 use rustc_data_structures::indexed_vec::IndexVec;
@@ -32,7 +32,7 @@ use syntax::ast::Mutability;
 use syntax::source_map::{Span, DUMMY_SP};
 
 use interpret::{self,
-    PlaceTy, MemPlace, OpTy, Operand, Value, Pointer, Scalar, ConstValue,
+    PlaceTy, MemPlace, OpTy, Operand, Value, Scalar, ConstValue, Pointer,
     EvalResult, EvalError, EvalErrorKind, GlobalId, EvalContext, StackPopCleanup,
     Allocation, AllocId, MemoryKind,
     snapshot, RefTracking,
@@ -426,7 +426,7 @@ impl<'a, 'mir, 'tcx> interpret::Machine<'a, 'mir, 'tcx>
     }
 
     #[inline(always)]
-    fn static_with_default_tag(
+    fn adjust_static_allocation(
         alloc: &'_ Allocation
     ) -> Cow<'_, Allocation<Self::PointerTag>> {
         // We do not use a tag so we can just cheaply forward the reference
@@ -467,23 +467,12 @@ impl<'a, 'mir, 'tcx> interpret::Machine<'a, 'mir, 'tcx>
     }
 
     #[inline(always)]
-    fn tag_reference(
+    fn tag_new_allocation(
         _ecx: &mut EvalContext<'a, 'mir, 'tcx, Self>,
-        _ptr: Pointer<Self::PointerTag>,
-        _pointee_ty: Ty<'tcx>,
-        _pointee_size: Size,
-        _borrow_kind: Option<mir::BorrowKind>,
-    ) -> EvalResult<'tcx, Self::PointerTag> {
-        Ok(())
-    }
-
-    #[inline(always)]
-    fn tag_dereference(
-        _ecx: &EvalContext<'a, 'mir, 'tcx, Self>,
-        _ptr: Pointer<Self::PointerTag>,
-        _ptr_ty: Ty<'tcx>,
-    ) -> EvalResult<'tcx, Self::PointerTag> {
-        Ok(())
+        ptr: Pointer,
+        _kind: MemoryKind<Self::MemoryKinds>,
+    ) -> EvalResult<'tcx, Pointer> {
+        Ok(ptr)
     }
 }
 

src/librustc_mir/interpret/cast.rs

@@ -110,7 +110,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M>
                             def_id,
                             substs,
                         ).ok_or_else(|| EvalErrorKind::TooGeneric.into());
-                        let fn_ptr = self.memory.create_fn_alloc(instance?);
+                        let fn_ptr = self.memory.create_fn_alloc(instance?).with_default_tag();
                         self.write_scalar(Scalar::Ptr(fn_ptr.into()), dest)?;
                     }
                     ref other => bug!("reify fn pointer on {:?}", other),
@@ -143,7 +143,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M>
                             substs,
                             ty::ClosureKind::FnOnce,
                         );
-                        let fn_ptr = self.memory.create_fn_alloc(instance);
+                        let fn_ptr = self.memory.create_fn_alloc(instance).with_default_tag();
                         let val = Value::Scalar(Scalar::Ptr(fn_ptr.into()).into());
                         self.write_value(val, dest)?;
                     }

src/librustc_mir/interpret/eval_context.rs

@@ -47,7 +47,7 @@ pub struct EvalContext<'a, 'mir, 'tcx: 'a + 'mir, M: Machine<'a, 'mir, 'tcx>> {
     pub(crate) param_env: ty::ParamEnv<'tcx>,
 
     /// The virtual memory system.
-    pub memory: Memory<'a, 'mir, 'tcx, M>,
+    pub(crate) memory: Memory<'a, 'mir, 'tcx, M>,
 
     /// The virtual call stack.
     pub(crate) stack: Vec<Frame<'mir, 'tcx, M::PointerTag>>,
@@ -334,7 +334,7 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tc
     }
 
     pub fn str_to_value(&mut self, s: &str) -> EvalResult<'tcx, Value<M::PointerTag>> {
-        let ptr = self.memory.allocate_static_bytes(s.as_bytes());
+        let ptr = self.memory.allocate_static_bytes(s.as_bytes()).with_default_tag();
         Ok(Value::new_slice(Scalar::Ptr(ptr), s.len() as u64, self.tcx.tcx))
     }
 

src/librustc_mir/interpret/machine.rs

@@ -15,13 +15,13 @@
 use std::borrow::{Borrow, Cow};
 use std::hash::Hash;
 
-use rustc::hir::def_id::DefId;
+use rustc::hir::{self, def_id::DefId};
 use rustc::mir;
 use rustc::ty::{self, Ty, layout::{Size, TyLayout}, query::TyCtxtAt};
 
 use super::{
     Allocation, AllocId, EvalResult, Scalar,
-    EvalContext, PlaceTy, OpTy, Pointer, MemoryKind,
+    EvalContext, PlaceTy, OpTy, Pointer, MemPlace, MemoryKind,
 };
 
 /// Classifying memory accesses
@@ -81,6 +81,7 @@ pub trait Machine<'a, 'mir, 'tcx>: Sized {
 
     /// Tag tracked alongside every pointer.  This is used to implement "Stacked Borrows"
     /// <https://www.ralfj.de/blog/2018/08/07/stacked-borrows.html>.
+    /// The `default()` is used for pointers to consts, statics, vtables and functions.
     type PointerTag: ::std::fmt::Debug + Default + Copy + Eq + Hash + 'static;
 
     /// Extra data stored in every allocation.
@@ -151,13 +152,13 @@ pub trait Machine<'a, 'mir, 'tcx>: Sized {
     ) -> EvalResult<'tcx, Cow<'tcx, Allocation<Self::PointerTag, Self::AllocExtra>>>;
 
     /// Called to turn an allocation obtained from the `tcx` into one that has
-    /// the appropriate tags on each pointer.
+    /// the right type for this machine.
     ///
     /// This should avoid copying if no work has to be done! If this returns an owned
-    /// allocation (because a copy had to be done to add the tags), machine memory will
+    /// allocation (because a copy had to be done to add tags or metadata), machine memory will
     /// cache the result. (This relies on `AllocMap::get_or` being able to add the
     /// owned allocation to the map even when the map is shared.)
-    fn static_with_default_tag(
+    fn adjust_static_allocation(
         alloc: &'_ Allocation
     ) -> Cow<'_, Allocation<Self::PointerTag, Self::AllocExtra>>;
 
@@ -204,24 +205,40 @@ pub trait Machine<'a, 'mir, 'tcx>: Sized {
         Ok(())
     }
 
+    /// Add the tag for a newly allocated pointer.
+    fn tag_new_allocation(
+        ecx: &mut EvalContext<'a, 'mir, 'tcx, Self>,
+        ptr: Pointer,
+        kind: MemoryKind<Self::MemoryKinds>,
+    ) -> EvalResult<'tcx, Pointer<Self::PointerTag>>;
+
     /// Executed when evaluating the `&` operator: Creating a new reference.
-    /// This has the chance to adjust the tag.
-    /// `borrow_kind` can be `None` in case a raw ptr is being created.
+    /// This has the chance to adjust the tag.  It should not change anything else!
+    /// `mutability` can be `None` in case a raw ptr is being created.
+    #[inline]
     fn tag_reference(
-        ecx: &mut EvalContext<'a, 'mir, 'tcx, Self>,
-        ptr: Pointer<Self::PointerTag>,
-        pointee_ty: Ty<'tcx>,
-        pointee_size: Size,
-        borrow_kind: Option<mir::BorrowKind>,
-    ) -> EvalResult<'tcx, Self::PointerTag>;
+        _ecx: &mut EvalContext<'a, 'mir, 'tcx, Self>,
+        place: MemPlace<Self::PointerTag>,
+        _ty: Ty<'tcx>,
+        _size: Size,
+        _mutability: Option<hir::Mutability>,
+    ) -> EvalResult<'tcx, MemPlace<Self::PointerTag>> {
+        Ok(place)
+    }
 
     /// Executed when evaluating the `*` operator: Following a reference.
-    /// This has the change to adjust the tag.
+    /// This has the change to adjust the tag.  It should not change anything else!
+    /// `mutability` can be `None` in case a raw ptr is being dereferenced.
+    #[inline]
     fn tag_dereference(
-        ecx: &EvalContext<'a, 'mir, 'tcx, Self>,
-        ptr: Pointer<Self::PointerTag>,
-        ptr_ty: Ty<'tcx>,
-    ) -> EvalResult<'tcx, Self::PointerTag>;
+        _ecx: &EvalContext<'a, 'mir, 'tcx, Self>,
+        place: MemPlace<Self::PointerTag>,
+        _ty: Ty<'tcx>,
+        _size: Size,
+        _mutability: Option<hir::Mutability>,
+    ) -> EvalResult<'tcx, MemPlace<Self::PointerTag>> {
+        Ok(place)
+    }
 
     /// Execute a validation operation
     #[inline]

src/librustc_mir/interpret/memory.rs

@@ -117,12 +117,12 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> {
         }
     }
 
-    pub fn create_fn_alloc(&mut self, instance: Instance<'tcx>) -> Pointer<M::PointerTag> {
-        Pointer::from(self.tcx.alloc_map.lock().create_fn_alloc(instance)).with_default_tag()
+    pub fn create_fn_alloc(&mut self, instance: Instance<'tcx>) -> Pointer {
+        Pointer::from(self.tcx.alloc_map.lock().create_fn_alloc(instance))
     }
 
-    pub fn allocate_static_bytes(&mut self, bytes: &[u8]) -> Pointer<M::PointerTag> {
-        Pointer::from(self.tcx.allocate_bytes(bytes)).with_default_tag()
+    pub fn allocate_static_bytes(&mut self, bytes: &[u8]) -> Pointer {
+        Pointer::from(self.tcx.allocate_bytes(bytes))
     }
 
     pub fn allocate_with(
@@ -140,9 +140,8 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> {
         size: Size,
         align: Align,
         kind: MemoryKind<M::MemoryKinds>,
-    ) -> EvalResult<'tcx, Pointer<M::PointerTag>> {
-        let ptr = Pointer::from(self.allocate_with(Allocation::undef(size, align), kind)?);
-        Ok(ptr.with_default_tag())
+    ) -> EvalResult<'tcx, Pointer> {
+        Ok(Pointer::from(self.allocate_with(Allocation::undef(size, align), kind)?))
     }
 
     pub fn reallocate(
@@ -153,17 +152,18 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> {
         new_size: Size,
         new_align: Align,
         kind: MemoryKind<M::MemoryKinds>,
-    ) -> EvalResult<'tcx, Pointer<M::PointerTag>> {
+    ) -> EvalResult<'tcx, Pointer> {
         if ptr.offset.bytes() != 0 {
             return err!(ReallocateNonBasePtr);
         }
 
-        // For simplicities' sake, we implement reallocate as "alloc, copy, dealloc"
+        // For simplicities' sake, we implement reallocate as "alloc, copy, dealloc".
+        // This happens so rarely, the perf advantage is outweighed by the maintenance cost.
         let new_ptr = self.allocate(new_size, new_align, kind)?;
         self.copy(
             ptr.into(),
             old_align,
-            new_ptr.into(),
+            new_ptr.with_default_tag().into(),
             new_align,
             old_size.min(new_size),
             /*nonoverlapping*/ true,
@@ -347,7 +347,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> {
             Some(AllocType::Memory(mem)) => {
                 // We got tcx memory. Let the machine figure out whether and how to
                 // turn that into memory with the right pointer tag.
-                return Ok(M::static_with_default_tag(mem))
+                return Ok(M::adjust_static_allocation(mem))
             }
             Some(AllocType::Function(..)) => {
                 return err!(DerefFunctionPointer)
@@ -381,7 +381,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> {
             if let ConstValue::ByRef(_, allocation, _) = const_val.val {
                 // We got tcx memory. Let the machine figure out whether and how to
                 // turn that into memory with the right pointer tag.
-                M::static_with_default_tag(allocation)
+                M::adjust_static_allocation(allocation)
             } else {
                 bug!("Matching on non-ByRef static")
             }

src/librustc_mir/interpret/operand.rs

@@ -217,6 +217,16 @@ impl<'tcx, Tag> Value<Tag> {
             Value::ScalarPair(ptr, _) => ptr.not_undef(),
         }
     }
+
+    /// Convert the value into its metadata.
+    /// Throws away the first half of a ScalarPair!
+    #[inline]
+    pub fn to_meta(self) -> EvalResult<'tcx, Option<Scalar<Tag>>> {
+        Ok(match self {
+            Value::Scalar(_) => None,
+            Value::ScalarPair(_, meta) => Some(meta.not_undef()?),
+        })
+    }
 }
 
 // ScalarPair needs a type to interpret, so we often have a value and a type together

src/librustc_mir/interpret/place.rs

@@ -15,6 +15,7 @@
 use std::convert::TryFrom;
 use std::hash::Hash;
 
+use rustc::hir;
 use rustc::mir;
 use rustc::ty::{self, Ty};
 use rustc::ty::layout::{self, Size, Align, LayoutOf, TyLayout, HasDataLayout};
@@ -270,24 +271,28 @@ where
         &self,
         val: ValTy<'tcx, M::PointerTag>,
     ) -> EvalResult<'tcx, MPlaceTy<'tcx, M::PointerTag>> {
-        let ptr = match val.to_scalar_ptr()? {
-            Scalar::Ptr(ptr) if M::ENABLE_PTR_TRACKING_HOOKS => {
-                // Machine might want to track the `*` operator
-                let tag = M::tag_dereference(self, ptr, val.layout.ty)?;
-                Scalar::Ptr(Pointer::new_with_tag(ptr.alloc_id, ptr.offset, tag))
-            }
-            other => other,
-        };
-
         let pointee_type = val.layout.ty.builtin_deref(true).unwrap().ty;
         let layout = self.layout_of(pointee_type)?;
-        let align = layout.align;
 
-        let mplace = match *val {
-            Value::Scalar(_) =>
-                MemPlace { ptr, align, meta: None },
-            Value::ScalarPair(_, meta) =>
-                MemPlace { ptr, align, meta: Some(meta.not_undef()?) },
+        let align = layout.align;
+        let meta = val.to_meta()?;
+        let ptr = val.to_scalar_ptr()?;
+        let mplace = MemPlace { ptr, align, meta };
+        // Pointer tag tracking might want to adjust the tag.
+        let mplace = if M::ENABLE_PTR_TRACKING_HOOKS {
+            let (size, _) = self.size_and_align_of(meta, layout)?
+                // for extern types, just cover what we can
+                .unwrap_or_else(|| layout.size_and_align());
+            let mutbl = match val.layout.ty.sty {
+                // `builtin_deref` considers boxes immutable, that's useless for our purposes
+                ty::Ref(_, _, mutbl) => Some(mutbl),
+                ty::Adt(def, _) if def.is_box() => Some(hir::MutMutable),
+                ty::RawPtr(_) => None,
+                _ => bug!("Unexpected pointer type {}", val.layout.ty.sty),
+            };
+            M::tag_dereference(self, mplace, pointee_type, size, mutbl)?
+        } else {
+            mplace
         };
         Ok(MPlaceTy { mplace, layout })
     }
@@ -299,19 +304,25 @@ where
         place: MPlaceTy<'tcx, M::PointerTag>,
         borrow_kind: Option<mir::BorrowKind>,
     ) -> EvalResult<'tcx, Value<M::PointerTag>> {
-        let ptr = match place.ptr {
-            Scalar::Ptr(ptr) if M::ENABLE_PTR_TRACKING_HOOKS => {
-                // Machine might want to track the `&` operator
-                let (size, _) = self.size_and_align_of_mplace(place)?
-                    .expect("create_ref cannot determine size");
-                let tag = M::tag_reference(self, ptr, place.layout.ty, size, borrow_kind)?;
-                Scalar::Ptr(Pointer::new_with_tag(ptr.alloc_id, ptr.offset, tag))
-            },
-            other => other,
+        // Pointer tag tracking might want to adjust the tag
+        let place = if M::ENABLE_PTR_TRACKING_HOOKS {
+            let (size, _) = self.size_and_align_of_mplace(place)?
+                // for extern types, just cover what we can
+                .unwrap_or_else(|| place.layout.size_and_align());
+            let mutbl = match borrow_kind {
+                Some(mir::BorrowKind::Mut { .. }) |
+                Some(mir::BorrowKind::Unique) =>
+                    Some(hir::MutMutable),
+                Some(_) => Some(hir::MutImmutable),
+                None => None,
+            };
+            M::tag_reference(self, *place, place.layout.ty, size, mutbl)?
+        } else {
+            *place
         };
         Ok(match place.meta {
-            None => Value::Scalar(ptr.into()),
-            Some(meta) => Value::ScalarPair(ptr.into(), meta.into()),
+            None => Value::Scalar(place.ptr.into()),
+            Some(meta) => Value::ScalarPair(place.ptr.into(), meta.into()),
         })
     }
 
@@ -845,6 +856,8 @@ where
     }
 
     /// Make sure that a place is in memory, and return where it is.
+    /// If the place currently refers to a local that doesn't yet have a matching allocation,
+    /// create such an allocation.
     /// This is essentially `force_to_memplace`.
     pub fn force_allocation(
         &mut self,
@@ -888,10 +901,11 @@ where
     ) -> EvalResult<'tcx, MPlaceTy<'tcx, M::PointerTag>> {
         if layout.is_unsized() {
             assert!(self.tcx.features().unsized_locals, "cannot alloc memory for unsized type");
-            // FIXME: What should we do here?
+            // FIXME: What should we do here? We should definitely also tag!
             Ok(MPlaceTy::dangling(layout, &self))
         } else {
             let ptr = self.memory.allocate(layout.size, layout.align, kind)?;
+            let ptr = M::tag_new_allocation(self, ptr, kind)?;
             Ok(MPlaceTy::from_aligned_ptr(ptr, layout))
         }
     }