From 048900c5b6bd8d5f6499b60703b3b5773b614608 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 18 Oct 2018 18:01:42 +0200 Subject: [PATCH 1/5] make (de)reference hooks more consistent --- src/librustc_mir/const_eval.rs | 6 +++-- src/librustc_mir/interpret/machine.rs | 11 +++++--- src/librustc_mir/interpret/operand.rs | 10 ++++++++ src/librustc_mir/interpret/place.rs | 37 +++++++++++++++++---------- 4 files changed, 45 insertions(+), 19 deletions(-) diff --git a/src/librustc_mir/const_eval.rs b/src/librustc_mir/const_eval.rs index 362fbc4b1355b..41a70c88f5666 100644 --- a/src/librustc_mir/const_eval.rs +++ b/src/librustc_mir/const_eval.rs @@ -472,7 +472,7 @@ impl<'a, 'mir, 'tcx> interpret::Machine<'a, 'mir, 'tcx> _ptr: Pointer, _pointee_ty: Ty<'tcx>, _pointee_size: Size, - _borrow_kind: Option, + _borrow_kind: Option, ) -> EvalResult<'tcx, Self::PointerTag> { Ok(()) } @@ -481,7 +481,9 @@ impl<'a, 'mir, 'tcx> interpret::Machine<'a, 'mir, 'tcx> fn tag_dereference( _ecx: &EvalContext<'a, 'mir, 'tcx, Self>, _ptr: Pointer, - _ptr_ty: Ty<'tcx>, + _pointee_ty: Ty<'tcx>, + _pointee_size: Size, + _borrow_kind: Option, ) -> EvalResult<'tcx, Self::PointerTag> { Ok(()) } diff --git a/src/librustc_mir/interpret/machine.rs b/src/librustc_mir/interpret/machine.rs index 1318bbe1c2bf2..7184be6cd1635 100644 --- a/src/librustc_mir/interpret/machine.rs +++ b/src/librustc_mir/interpret/machine.rs @@ -15,7 +15,7 @@ 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}; @@ -206,21 +206,24 @@ pub trait Machine<'a, 'mir, 'tcx>: Sized { /// 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. + /// `mutability` can be `None` in case a raw ptr is being created. fn tag_reference( ecx: &mut EvalContext<'a, 'mir, 'tcx, Self>, ptr: Pointer, pointee_ty: Ty<'tcx>, pointee_size: Size, - borrow_kind: Option, + mutability: Option, ) -> EvalResult<'tcx, Self::PointerTag>; /// Executed when evaluating the `*` operator: Following a reference. /// This has the change to adjust the tag. + /// `mutability` can be `None` in case a raw ptr is being dereferenced. fn tag_dereference( ecx: &EvalContext<'a, 'mir, 'tcx, Self>, ptr: Pointer, - ptr_ty: Ty<'tcx>, + pointee_ty: Ty<'tcx>, + pointee_size: Size, + mutability: Option, ) -> EvalResult<'tcx, Self::PointerTag>; /// Execute a validation operation diff --git a/src/librustc_mir/interpret/operand.rs b/src/librustc_mir/interpret/operand.rs index 021e2d58f84b1..d0a32161485b4 100644 --- a/src/librustc_mir/interpret/operand.rs +++ b/src/librustc_mir/interpret/operand.rs @@ -217,6 +217,16 @@ impl<'tcx, Tag> Value { 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>> { + 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 diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index 4fa08e8c3111b..0bd9094ebcde6 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/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,26 +271,31 @@ where &self, val: ValTy<'tcx, M::PointerTag>, ) -> EvalResult<'tcx, MPlaceTy<'tcx, M::PointerTag>> { + let pointee_type = val.layout.ty.builtin_deref(true).unwrap().ty; + let layout = self.layout_of(pointee_type)?; + + let align = layout.align; + let meta = val.to_meta()?; + 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)?; + let (size, _) = self.size_and_align_of(meta, layout)? + .expect("ref_to_mplace cannot determine size"); + 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), + }; + let tag = M::tag_dereference(self, ptr, pointee_type, size, mutbl)?; 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()?) }, - }; - Ok(MPlaceTy { mplace, layout }) + Ok(MPlaceTy { mplace: MemPlace { ptr, align, meta }, layout }) } /// Turn a mplace into a (thin or fat) pointer, as a reference, pointing to the same space. @@ -304,7 +310,12 @@ where // 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)?; + let mutbl = match borrow_kind { + Some(mir::BorrowKind::Mut { .. }) => Some(hir::MutMutable), + Some(_) => Some(hir::MutImmutable), + None => None, + }; + let tag = M::tag_reference(self, ptr, place.layout.ty, size, mutbl)?; Scalar::Ptr(Pointer::new_with_tag(ptr.alloc_id, ptr.offset, tag)) }, other => other, From f5e8830278d5e677635dfd1fa774ae733700b4e8 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 19 Oct 2018 17:11:23 +0200 Subject: [PATCH 2/5] validity in non-const mode relies on ref_to_mplace checking bounds; (de)reference hooks work on places --- src/librustc_mir/const_eval.rs | 28 +------ src/librustc_mir/interpret/machine.rs | 36 +++++---- src/librustc_mir/interpret/place.rs | 70 ++++++++-------- src/librustc_mir/interpret/validity.rs | 106 ++++++++++++++----------- 4 files changed, 119 insertions(+), 121 deletions(-) diff --git a/src/librustc_mir/const_eval.rs b/src/librustc_mir/const_eval.rs index 41a70c88f5666..3a4f4b36dd589 100644 --- a/src/librustc_mir/const_eval.rs +++ b/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, EvalResult, EvalError, EvalErrorKind, GlobalId, EvalContext, StackPopCleanup, Allocation, AllocId, MemoryKind, snapshot, RefTracking, @@ -465,28 +465,6 @@ impl<'a, 'mir, 'tcx> interpret::Machine<'a, 'mir, 'tcx> &ecx.stack[..], ) } - - #[inline(always)] - fn tag_reference( - _ecx: &mut EvalContext<'a, 'mir, 'tcx, Self>, - _ptr: Pointer, - _pointee_ty: Ty<'tcx>, - _pointee_size: Size, - _borrow_kind: Option, - ) -> EvalResult<'tcx, Self::PointerTag> { - Ok(()) - } - - #[inline(always)] - fn tag_dereference( - _ecx: &EvalContext<'a, 'mir, 'tcx, Self>, - _ptr: Pointer, - _pointee_ty: Ty<'tcx>, - _pointee_size: Size, - _borrow_kind: Option, - ) -> EvalResult<'tcx, Self::PointerTag> { - Ok(()) - } } /// Project to a field of a (variant of a) const diff --git a/src/librustc_mir/interpret/machine.rs b/src/librustc_mir/interpret/machine.rs index 7184be6cd1635..3d45728f4ec3e 100644 --- a/src/librustc_mir/interpret/machine.rs +++ b/src/librustc_mir/interpret/machine.rs @@ -21,7 +21,7 @@ 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 @@ -205,26 +205,32 @@ pub trait Machine<'a, 'mir, 'tcx>: Sized { } /// Executed when evaluating the `&` operator: Creating a new reference. - /// This has the chance to adjust the tag. + /// 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, - pointee_ty: Ty<'tcx>, - pointee_size: Size, - mutability: Option, - ) -> EvalResult<'tcx, Self::PointerTag>; + _ecx: &mut EvalContext<'a, 'mir, 'tcx, Self>, + place: MemPlace, + _ty: Ty<'tcx>, + _size: Size, + _mutability: Option, + ) -> EvalResult<'tcx, MemPlace> { + 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, - pointee_ty: Ty<'tcx>, - pointee_size: Size, - mutability: Option, - ) -> EvalResult<'tcx, Self::PointerTag>; + _ecx: &EvalContext<'a, 'mir, 'tcx, Self>, + place: MemPlace, + _ty: Ty<'tcx>, + _size: Size, + _mutability: Option, + ) -> EvalResult<'tcx, MemPlace> { + Ok(place) + } /// Execute a validation operation #[inline] diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index 0bd9094ebcde6..bbdda8ed68d83 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -276,26 +276,25 @@ where let align = layout.align; let meta = val.to_meta()?; - - let ptr = match val.to_scalar_ptr()? { - Scalar::Ptr(ptr) if M::ENABLE_PTR_TRACKING_HOOKS => { - // Machine might want to track the `*` operator - let (size, _) = self.size_and_align_of(meta, layout)? - .expect("ref_to_mplace cannot determine size"); - 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), - }; - let tag = M::tag_dereference(self, ptr, pointee_type, size, mutbl)?; - Scalar::Ptr(Pointer::new_with_tag(ptr.alloc_id, ptr.offset, tag)) - } - other => other, + 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: MemPlace { ptr, align, meta }, layout }) + Ok(MPlaceTy { mplace, layout }) } /// Turn a mplace into a (thin or fat) pointer, as a reference, pointing to the same space. @@ -305,24 +304,25 @@ where place: MPlaceTy<'tcx, M::PointerTag>, borrow_kind: Option, ) -> EvalResult<'tcx, Value> { - 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 mutbl = match borrow_kind { - Some(mir::BorrowKind::Mut { .. }) => Some(hir::MutMutable), - Some(_) => Some(hir::MutImmutable), - None => None, - }; - let tag = M::tag_reference(self, ptr, place.layout.ty, size, mutbl)?; - 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()), }) } diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 25e2ff6edb763..226717538a294 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -12,7 +12,7 @@ use std::fmt::Write; use std::hash::Hash; use syntax_pos::symbol::Symbol; -use rustc::ty::layout::{self, Size, Align, TyLayout}; +use rustc::ty::layout::{self, Size, Align, TyLayout, LayoutOf}; use rustc::ty; use rustc_data_structures::fx::FxHashSet; use rustc::mir::interpret::{ @@ -176,19 +176,27 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> // undef. We should fix that, but let's start low. } } - _ if ty.is_box() || ty.is_region_ptr() || ty.is_unsafe_ptr() => { - // Handle fat pointers. We also check fat raw pointers, - // their metadata must be valid! - // This also checks that the ptr itself is initialized, which - // seems reasonable even for raw pointers. - let place = try_validation!(self.ref_to_mplace(value), - "undefined data in pointer", path); + ty::RawPtr(..) => { + // No undef allowed here. Eventually this should be consistent with + // the integer types. + let _ptr = try_validation!(value.to_scalar_ptr(), + "undefined address in pointer", path); + let _meta = try_validation!(value.to_meta(), + "uninitialized data in fat pointer metadata", path); + } + _ if ty.is_box() || ty.is_region_ptr() => { + // Handle fat pointers. // Check metadata early, for better diagnostics - if place.layout.is_unsized() { - let tail = self.tcx.struct_tail(place.layout.ty); + let ptr = try_validation!(value.to_scalar_ptr(), + "undefined address in pointer", path); + let meta = try_validation!(value.to_meta(), + "uninitialized data in fat pointer metadata", path); + let layout = self.layout_of(value.layout.ty.builtin_deref(true).unwrap().ty)?; + if layout.is_unsized() { + let tail = self.tcx.struct_tail(layout.ty); match tail.sty { ty::Dynamic(..) => { - let vtable = try_validation!(place.meta.unwrap().to_ptr(), + let vtable = try_validation!(meta.unwrap().to_ptr(), "non-pointer vtable in fat pointer", path); try_validation!(self.read_drop_type_from_vtable(vtable), "invalid drop fn in vtable", path); @@ -197,7 +205,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> // FIXME: More checks for the vtable. } ty::Slice(..) | ty::Str => { - try_validation!(place.meta.unwrap().to_usize(self), + try_validation!(meta.unwrap().to_usize(self), "non-integer slice length in fat pointer", path); } ty::Foreign(..) => { @@ -207,17 +215,17 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> bug!("Unexpected unsized type tail: {:?}", tail), } } - // for safe ptrs, also check the ptr values itself - if !ty.is_unsafe_ptr() { - // Make sure this is non-NULL and aligned - let (size, align) = self.size_and_align_of(place.meta, place.layout)? - // for the purpose of validity, consider foreign types to have - // alignment and size determined by the layout (size will be 0, - // alignment should take attributes into account). - .unwrap_or_else(|| place.layout.size_and_align()); - match self.memory.check_align(place.ptr, align) { - Ok(_) => {}, - Err(err) => match err.kind { + // Make sure this is non-NULL and aligned + let (size, align) = self.size_and_align_of(meta, layout)? + // for the purpose of validity, consider foreign types to have + // alignment and size determined by the layout (size will be 0, + // alignment should take attributes into account). + .unwrap_or_else(|| layout.size_and_align()); + match self.memory.check_align(ptr, align) { + Ok(_) => {}, + Err(err) => { + error!("{:?} is not aligned to {:?}", ptr, align); + match err.kind { EvalErrorKind::InvalidNullPointerUsage => return validation_failure!("NULL reference", path), EvalErrorKind::AlignmentCheckFailed { .. } => @@ -225,41 +233,47 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> _ => return validation_failure!( "dangling (out-of-bounds) reference (might be NULL at \ - run-time)", + run-time)", path ), } } - // non-ZST also have to be dereferenceable + } + // Turn ptr into place. + // `ref_to_mplace` also calls the machine hook for (re)activating the tag, + // which in turn will (in full miri) check if the pointer is dereferencable. + let place = self.ref_to_mplace(value)?; + // Recursive checking + if let Some(ref_tracking) = ref_tracking { + assert!(const_mode, "We should only do recursie checking in const mode"); if size != Size::ZERO { + // Non-ZST also have to be dereferencable let ptr = try_validation!(place.ptr.to_ptr(), "integer pointer in non-ZST reference", path); - if const_mode { - // Skip validation entirely for some external statics - let alloc_kind = self.tcx.alloc_map.lock().get(ptr.alloc_id); - if let Some(AllocType::Static(did)) = alloc_kind { - // `extern static` cannot be validated as they have no body. - // FIXME: Statics from other crates are also skipped. - // They might be checked at a different type, but for now we - // want to avoid recursing too deeply. This is not sound! - if !did.is_local() || self.tcx.is_foreign_item(did) { - return Ok(()); - } + // Skip validation entirely for some external statics + let alloc_kind = self.tcx.alloc_map.lock().get(ptr.alloc_id); + if let Some(AllocType::Static(did)) = alloc_kind { + // `extern static` cannot be validated as they have no body. + // FIXME: Statics from other crates are also skipped. + // They might be checked at a different type, but for now we + // want to avoid recursing too deeply. This is not sound! + if !did.is_local() || self.tcx.is_foreign_item(did) { + return Ok(()); } } + // Maintain the invariant that the place we are checking is + // already verified to be in-bounds. try_validation!(self.memory.check_bounds(ptr, size, false), "dangling (not entirely in bounds) reference", path); } - if let Some(ref_tracking) = ref_tracking { - // Check if we have encountered this pointer+layout combination - // before. Proceed recursively even for integer pointers, no - // reason to skip them! They are (recursively) valid for some ZST, - // but not for others (e.g. `!` is a ZST). - let op = place.into(); - if ref_tracking.seen.insert(op) { - trace!("Recursing below ptr {:#?}", *op); - ref_tracking.todo.push((op, path_clone_and_deref(path))); - } + // Check if we have encountered this pointer+layout combination + // before. Proceed recursively even for integer pointers, no + // reason to skip them! They are (recursively) valid for some ZST, + // but not for others (e.g. `!` is a ZST). + let op = place.into(); + if ref_tracking.seen.insert(op) { + trace!("Recursing below ptr {:#?}", *op); + ref_tracking.todo.push((op, path_clone_and_deref(path))); } } } From ff3b29fc54b9eb430040d80f164f347746508997 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 19 Oct 2018 19:39:52 +0200 Subject: [PATCH 3/5] make memory private; that's what we have `memory_mut` for --- src/librustc_mir/interpret/eval_context.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index b578ec659002c..1e21eff9e7823 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/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>, From b2612cbaf7f793f1b9411143434a85d7c8194db7 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 22 Oct 2018 17:15:42 +0200 Subject: [PATCH 4/5] don't tag new memory inside memory.rs; add machine hook to tag new memory --- src/librustc_mir/const_eval.rs | 13 ++++++++++-- src/librustc_mir/interpret/cast.rs | 4 ++-- src/librustc_mir/interpret/eval_context.rs | 2 +- src/librustc_mir/interpret/machine.rs | 14 ++++++++++--- src/librustc_mir/interpret/memory.rs | 24 +++++++++++----------- src/librustc_mir/interpret/place.rs | 5 ++++- src/librustc_mir/interpret/traits.rs | 6 +++--- 7 files changed, 44 insertions(+), 24 deletions(-) diff --git a/src/librustc_mir/const_eval.rs b/src/librustc_mir/const_eval.rs index 3a4f4b36dd589..f951674ab7396 100644 --- a/src/librustc_mir/const_eval.rs +++ b/src/librustc_mir/const_eval.rs @@ -32,7 +32,7 @@ use syntax::ast::Mutability; use syntax::source_map::{Span, DUMMY_SP}; use interpret::{self, - PlaceTy, MemPlace, OpTy, Operand, Value, 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> { // We do not use a tag so we can just cheaply forward the reference @@ -465,6 +465,15 @@ impl<'a, 'mir, 'tcx> interpret::Machine<'a, 'mir, 'tcx> &ecx.stack[..], ) } + + #[inline(always)] + fn tag_new_allocation( + _ecx: &mut EvalContext<'a, 'mir, 'tcx, Self>, + ptr: Pointer, + _kind: MemoryKind, + ) -> EvalResult<'tcx, Pointer> { + Ok(ptr) + } } /// Project to a field of a (variant of a) const diff --git a/src/librustc_mir/interpret/cast.rs b/src/librustc_mir/interpret/cast.rs index 81e7a6e4373d2..b2c8cba480259 100644 --- a/src/librustc_mir/interpret/cast.rs +++ b/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)?; } diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index 1e21eff9e7823..bc7ad16dc97bc 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/src/librustc_mir/interpret/eval_context.rs @@ -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> { - 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)) } diff --git a/src/librustc_mir/interpret/machine.rs b/src/librustc_mir/interpret/machine.rs index 3d45728f4ec3e..7811dcb0663d5 100644 --- a/src/librustc_mir/interpret/machine.rs +++ b/src/librustc_mir/interpret/machine.rs @@ -81,6 +81,7 @@ pub trait Machine<'a, 'mir, 'tcx>: Sized { /// Tag tracked alongside every pointer. This is used to implement "Stacked Borrows" /// . + /// 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>>; /// 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>; @@ -204,6 +205,13 @@ 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, + ) -> EvalResult<'tcx, Pointer>; + /// Executed when evaluating the `&` operator: Creating a new reference. /// 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. diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index ff3fdffcd7622..982463740974a 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/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 { - 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 { - 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, - ) -> EvalResult<'tcx, Pointer> { - 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, - ) -> EvalResult<'tcx, Pointer> { + ) -> 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". + // FIXME: Do something more efficient. 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") } diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index bbdda8ed68d83..0eae2bfb226c6 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -856,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, @@ -899,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)) } } diff --git a/src/librustc_mir/interpret/traits.rs b/src/librustc_mir/interpret/traits.rs index a2d4eee2842c7..c189ec0ca5c77 100644 --- a/src/librustc_mir/interpret/traits.rs +++ b/src/librustc_mir/interpret/traits.rs @@ -54,10 +54,10 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> ptr_size * (3 + methods.len() as u64), ptr_align, MemoryKind::Vtable, - )?; + )?.with_default_tag(); let drop = ::monomorphize::resolve_drop_in_place(*self.tcx, ty); - let drop = self.memory.create_fn_alloc(drop); + let drop = self.memory.create_fn_alloc(drop).with_default_tag(); self.memory.write_ptr_sized(vtable, ptr_align, Scalar::Ptr(drop).into())?; let size_ptr = vtable.offset(ptr_size, &self)?; @@ -69,7 +69,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> for (i, method) in methods.iter().enumerate() { if let Some((def_id, substs)) = *method { let instance = self.resolve(def_id, substs)?; - let fn_ptr = self.memory.create_fn_alloc(instance); + let fn_ptr = self.memory.create_fn_alloc(instance).with_default_tag(); let method_ptr = vtable.offset(ptr_size * (3 + i as u64), &self)?; self.memory.write_ptr_sized(method_ptr, ptr_align, Scalar::Ptr(fn_ptr).into())?; } From 95b19bbb6f270f584b1e15a874737b59e8203544 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 22 Oct 2018 19:17:37 +0200 Subject: [PATCH 5/5] don't be too perf-greedy --- src/librustc_mir/interpret/memory.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index 982463740974a..689a29cff6e9e 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -158,7 +158,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> { } // For simplicities' sake, we implement reallocate as "alloc, copy, dealloc". - // FIXME: Do something more efficient. + // 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(),