From 9007296a202b62f26aaa66393cefd93e731b9602 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 6 Jul 2019 12:46:08 +0200 Subject: [PATCH] Add check_mplace_ptr convenience method; provide ptr-normalization methods for mplace and op to avoid repeated int-to-ptr casting during validation. Also change Memory::copy to work on `Pointer` instead of `Scalar`. Also rename some methods from to_* to assert_* that will panic if their precondition is not met. --- src/librustc_mir/const_eval.rs | 2 +- src/librustc_mir/interpret/memory.rs | 36 ++++-------- src/librustc_mir/interpret/operand.rs | 26 ++++++--- src/librustc_mir/interpret/place.rs | 72 ++++++++++++++++++------ src/librustc_mir/interpret/step.rs | 13 +++-- src/librustc_mir/interpret/terminator.rs | 2 +- src/librustc_mir/interpret/validity.rs | 19 ++++--- src/librustc_mir/interpret/visitor.rs | 4 +- 8 files changed, 108 insertions(+), 66 deletions(-) diff --git a/src/librustc_mir/const_eval.rs b/src/librustc_mir/const_eval.rs index f8de1cfaea098..de0f8dbcf21af 100644 --- a/src/librustc_mir/const_eval.rs +++ b/src/librustc_mir/const_eval.rs @@ -109,7 +109,7 @@ fn op_to_const<'tcx>( // `Immediate` is when we are called from `const_field`, and that `Immediate` // comes from a constant so it can happen have `Undef`, because the indirect // memory that was read had undefined bytes. - let mplace = op.to_mem_place(); + let mplace = op.assert_mem_place(); let ptr = mplace.ptr.to_ptr().unwrap(); let alloc = ecx.tcx.alloc_map.lock().unwrap_memory(ptr.alloc_id); ConstValue::ByRef { offset: ptr.offset, align: mplace.align, alloc } diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index 33cd7330069e3..3f2a76a77be36 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -214,10 +214,8 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { None => Size::from_bytes(self.get(ptr.alloc_id)?.bytes.len() as u64), }; self.copy( - ptr.into(), - Align::from_bytes(1).unwrap(), // old_align anyway gets checked below by `deallocate` - new_ptr.into(), - new_align, + ptr, + new_ptr, old_size.min(new_size), /*nonoverlapping*/ true, )?; @@ -310,6 +308,9 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { /// `Pointer` they need. And even if you already have a `Pointer`, call this method /// to make sure it is sufficiently aligned and not dangling. Not doing that may /// cause ICEs. + /// + /// Most of the time you should use `check_mplace_access`, but when you just have a pointer, + /// this method is still appropriate. pub fn check_ptr_access( &self, sptr: Scalar, @@ -751,39 +752,26 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { self.get(ptr.alloc_id)?.read_c_str(self, ptr) } - /// Performs appropriate bounds checks. + /// Expects the caller to have checked bounds and alignment. pub fn copy( &mut self, - src: Scalar, - src_align: Align, - dest: Scalar, - dest_align: Align, + src: Pointer, + dest: Pointer, size: Size, nonoverlapping: bool, ) -> InterpResult<'tcx> { - self.copy_repeatedly(src, src_align, dest, dest_align, size, 1, nonoverlapping) + self.copy_repeatedly(src, dest, size, 1, nonoverlapping) } - /// Performs appropriate bounds checks. + /// Expects the caller to have checked bounds and alignment. pub fn copy_repeatedly( &mut self, - src: Scalar, - src_align: Align, - dest: Scalar, - dest_align: Align, + src: Pointer, + dest: Pointer, size: Size, length: u64, nonoverlapping: bool, ) -> InterpResult<'tcx> { - // We need to check *both* before early-aborting due to the size being 0. - let (src, dest) = match (self.check_ptr_access(src, size, src_align)?, - self.check_ptr_access(dest, size * length, dest_align)?) - { - (Some(src), Some(dest)) => (src, dest), - // One of the two sizes is 0. - _ => return Ok(()), - }; - // first copy the relocations to a temporary buffer, because // `get_bytes_mut` will clear the relocations, which is correct, // since we don't want to keep any relocations at the target. diff --git a/src/librustc_mir/interpret/operand.rs b/src/librustc_mir/interpret/operand.rs index 68c9047f7b708..b43c810b8e626 100644 --- a/src/librustc_mir/interpret/operand.rs +++ b/src/librustc_mir/interpret/operand.rs @@ -123,23 +123,23 @@ pub enum Operand { impl Operand { #[inline] - pub fn to_mem_place(self) -> MemPlace + pub fn assert_mem_place(self) -> MemPlace where Tag: ::std::fmt::Debug { match self { Operand::Indirect(mplace) => mplace, - _ => bug!("to_mem_place: expected Operand::Indirect, got {:?}", self), + _ => bug!("assert_mem_place: expected Operand::Indirect, got {:?}", self), } } #[inline] - pub fn to_immediate(self) -> Immediate + pub fn assert_immediate(self) -> Immediate where Tag: ::std::fmt::Debug { match self { Operand::Immediate(imm) => imm, - _ => bug!("to_immediate: expected Operand::Immediate, got {:?}", self), + _ => bug!("assert_immediate: expected Operand::Immediate, got {:?}", self), } } @@ -214,6 +214,19 @@ pub(super) fn from_known_layout<'tcx>( } impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { + /// Normalice `place.ptr` to a `Pointer` if this is a place and not a ZST. + /// Can be helpful to avoid lots of `force_ptr` calls later, if this place is used a lot. + #[inline] + pub fn normalize_op_ptr( + &self, + op: OpTy<'tcx, M::PointerTag>, + ) -> InterpResult<'tcx, OpTy<'tcx, M::PointerTag>> { + match op.try_as_mplace() { + Ok(mplace) => Ok(self.normalize_mplace_ptr(mplace)?.into()), + Err(imm) => Ok(imm.into()), // Nothing to normalize + } + } + /// Try reading an immediate in memory; this is interesting particularly for `ScalarPair`. /// Returns `None` if the layout does not permit loading this as a value. fn try_read_immediate_from_mplace( @@ -224,9 +237,8 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // Don't touch unsized return Ok(None); } - let (ptr, ptr_align) = mplace.to_scalar_ptr_align(); - let ptr = match self.memory.check_ptr_access(ptr, mplace.layout.size, ptr_align)? { + let ptr = match self.check_mplace_access(mplace, None)? { Some(ptr) => ptr, None => return Ok(Some(ImmTy { // zero-sized type imm: Immediate::Scalar(Scalar::zst().into()), @@ -396,7 +408,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } else { // The rest should only occur as mplace, we do not use Immediates for types // allowing such operations. This matches place_projection forcing an allocation. - let mplace = base.to_mem_place(); + let mplace = base.assert_mem_place(); self.mplace_projection(mplace, proj_elem)?.into() } }) diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index 4f3727fbd8d9a..9641164b11907 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -230,6 +230,7 @@ impl<'tcx, Tag> MPlaceTy<'tcx, Tag> { } } +// These are defined here because they produce a place. impl<'tcx, Tag: ::std::fmt::Debug + Copy> OpTy<'tcx, Tag> { #[inline(always)] pub fn try_as_mplace(self) -> Result, ImmTy<'tcx, Tag>> { @@ -240,7 +241,7 @@ impl<'tcx, Tag: ::std::fmt::Debug + Copy> OpTy<'tcx, Tag> { } #[inline(always)] - pub fn to_mem_place(self) -> MPlaceTy<'tcx, Tag> { + pub fn assert_mem_place(self) -> MPlaceTy<'tcx, Tag> { self.try_as_mplace().unwrap() } } @@ -263,29 +264,29 @@ impl<'tcx, Tag: ::std::fmt::Debug> Place { } #[inline] - pub fn to_mem_place(self) -> MemPlace { + pub fn assert_mem_place(self) -> MemPlace { match self { Place::Ptr(mplace) => mplace, - _ => bug!("to_mem_place: expected Place::Ptr, got {:?}", self), + _ => bug!("assert_mem_place: expected Place::Ptr, got {:?}", self), } } #[inline] pub fn to_scalar_ptr_align(self) -> (Scalar, Align) { - self.to_mem_place().to_scalar_ptr_align() + self.assert_mem_place().to_scalar_ptr_align() } #[inline] pub fn to_ptr(self) -> InterpResult<'tcx, Pointer> { - self.to_mem_place().to_ptr() + self.assert_mem_place().to_ptr() } } impl<'tcx, Tag: ::std::fmt::Debug> PlaceTy<'tcx, Tag> { #[inline] - pub fn to_mem_place(self) -> MPlaceTy<'tcx, Tag> { - MPlaceTy { mplace: self.place.to_mem_place(), layout: self.layout } + pub fn assert_mem_place(self) -> MPlaceTy<'tcx, Tag> { + MPlaceTy { mplace: self.place.assert_mem_place(), layout: self.layout } } } @@ -322,8 +323,8 @@ where Ok(MPlaceTy { mplace, layout }) } - // Take an operand, representing a pointer, and dereference it to a place -- that - // will always be a MemPlace. Lives in `place.rs` because it creates a place. + /// Take an operand, representing a pointer, and dereference it to a place -- that + /// will always be a MemPlace. Lives in `place.rs` because it creates a place. pub fn deref_operand( &self, src: OpTy<'tcx, M::PointerTag>, @@ -333,6 +334,38 @@ where self.ref_to_mplace(val) } + /// Check if the given place is good for memory access with the given + /// size, falling back to the layout's size if `None` (in the latter case, + /// this must be a statically sized type). + /// + /// On success, returns `None` for zero-sized accesses (where nothing else is + /// left to do) and a `Pointer` to use for the actual access otherwise. + #[inline] + pub fn check_mplace_access( + &self, + place: MPlaceTy<'tcx, M::PointerTag>, + size: Option, + ) -> InterpResult<'tcx, Option>> { + let size = size.unwrap_or_else(|| { + assert!(!place.layout.is_unsized()); + assert!(place.meta.is_none()); + place.layout.size + }); + self.memory.check_ptr_access(place.ptr, size, place.align) + } + + /// Normalice `place.ptr` to a `Pointer` if this is not a ZST. + /// Can be helpful to avoid lots of `force_ptr` calls later, if this place is used a lot. + pub fn normalize_mplace_ptr( + &self, + mut place: MPlaceTy<'tcx, M::PointerTag>, + ) -> InterpResult<'tcx, MPlaceTy<'tcx, M::PointerTag>> { + if !place.layout.is_zst() { + place.mplace.ptr = self.force_ptr(place.mplace.ptr)?.into(); + } + Ok(place) + } + /// Offset a pointer to project to a field. Unlike `place_field`, this is always /// possible without allocating, so it can take `&self`. Also return the field's layout. /// This supports both struct and array fields. @@ -741,14 +774,12 @@ where value: Immediate, dest: MPlaceTy<'tcx, M::PointerTag>, ) -> InterpResult<'tcx> { - let (ptr, ptr_align) = dest.to_scalar_ptr_align(); // Note that it is really important that the type here is the right one, and matches the // type things are read at. In case `src_val` is a `ScalarPair`, we don't do any magic here // to handle padding properly, which is only correct if we never look at this data with the // wrong type. - assert!(!dest.layout.is_unsized()); - let ptr = match self.memory.check_ptr_access(ptr, dest.layout.size, ptr_align)? { + let ptr = match self.check_mplace_access(dest, None)? { Some(ptr) => ptr, None => return Ok(()), // zero-sized access }; @@ -850,14 +881,21 @@ where dest.layout.size }); assert_eq!(src.meta, dest.meta, "Can only copy between equally-sized instances"); + + let src = self.check_mplace_access(src, Some(size))?; + let dest = self.check_mplace_access(dest, Some(size))?; + let (src_ptr, dest_ptr) = match (src, dest) { + (Some(src_ptr), Some(dest_ptr)) => (src_ptr, dest_ptr), + (None, None) => return Ok(()), // zero-sized copy + _ => bug!("The pointers should both be Some or both None"), + }; + self.memory.copy( - src.ptr, src.align, - dest.ptr, dest.align, + src_ptr, + dest_ptr, size, /*nonoverlapping*/ true, - )?; - - Ok(()) + ) } /// Copies the data from an operand to a place. The layouts may disagree, but they must diff --git a/src/librustc_mir/interpret/step.rs b/src/librustc_mir/interpret/step.rs index dc5302eb18fc4..246c90ba48e3a 100644 --- a/src/librustc_mir/interpret/step.rs +++ b/src/librustc_mir/interpret/step.rs @@ -209,17 +209,18 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { let dest = self.force_allocation(dest)?; let length = dest.len(self)?; - if length > 0 { - // write the first + if let Some(first_ptr) = self.check_mplace_access(dest, None)? { + // Write the first. let first = self.mplace_field(dest, 0)?; self.copy_op(op, first.into())?; if length > 1 { - // copy the rest - let (dest, dest_align) = first.to_scalar_ptr_align(); - let rest = dest.ptr_offset(first.layout.size, self)?; + let elem_size = first.layout.size; + // Copy the rest. This is performance-sensitive code + // for big static/const arrays! + let rest_ptr = first_ptr.offset(elem_size, self)?; self.memory.copy_repeatedly( - dest, dest_align, rest, dest_align, first.layout.size, length - 1, true + first_ptr, rest_ptr, elem_size, length - 1, /*nonoverlapping:*/true )?; } } diff --git a/src/librustc_mir/interpret/terminator.rs b/src/librustc_mir/interpret/terminator.rs index 0ab428628de68..c11e5e119237f 100644 --- a/src/librustc_mir/interpret/terminator.rs +++ b/src/librustc_mir/interpret/terminator.rs @@ -426,7 +426,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } None => { // Unsized self. - args[0].to_mem_place() + args[0].assert_mem_place() } }; // Find and consult vtable diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 34892f5b8ca01..bf062ac68a5c1 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -440,9 +440,11 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> } } } - // Check if we have encountered this pointer+layout combination - // before. Proceed recursively even for ZST, no - // reason to skip them! E.g., `!` is a ZST and we want to validate it. + // Proceed recursively even for ZST, no reason to skip them! + // `!` is a ZST and we want to validate it. + // Normalize before handing `place` to tracking because that will + // check for duplicates. + let place = self.ecx.normalize_mplace_ptr(place)?; let path = &self.path; ref_tracking.track(place, || { // We need to clone the path anyway, make sure it gets created @@ -548,7 +550,7 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> ) -> InterpResult<'tcx> { match op.layout.ty.sty { ty::Str => { - let mplace = op.to_mem_place(); // strings are never immediate + let mplace = op.assert_mem_place(); // strings are never immediate try_validation!(self.ecx.read_str(mplace), "uninitialized or non-UTF-8 data in str", self.path); } @@ -565,7 +567,7 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> return Ok(()); } // non-ZST array cannot be immediate, slices are never immediate - let mplace = op.to_mem_place(); + let mplace = op.assert_mem_place(); // This is the length of the array/slice. let len = mplace.len(self.ecx)?; // zero length slices have nothing to be checked @@ -576,8 +578,8 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> let ty_size = self.ecx.layout_of(tys)?.size; // This is the size in bytes of the whole array. let size = ty_size * len; - - let ptr = self.ecx.force_ptr(mplace.ptr)?; + // Size is not 0, get a pointer (no cast because we normalized in validate_operand). + let ptr = mplace.ptr.assert_ptr(); // NOTE: Keep this in sync with the handling of integer and float // types above, in `visit_primitive`. @@ -633,7 +635,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { /// `ref_tracking_for_consts` can be `None` to avoid recursive checking below references. /// This also toggles between "run-time" (no recursion) and "compile-time" (with recursion) /// validation (e.g., pointer values are fine in integers at runtime) and various other const - /// specific validation checks + /// specific validation checks. pub fn validate_operand( &self, op: OpTy<'tcx, M::PointerTag>, @@ -653,6 +655,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { }; // Run it + let op = self.normalize_op_ptr(op)?; // avoid doing ptr-to-int all the time visitor.visit_value(op) } } diff --git a/src/librustc_mir/interpret/visitor.rs b/src/librustc_mir/interpret/visitor.rs index 783d252263735..91fbd307db121 100644 --- a/src/librustc_mir/interpret/visitor.rs +++ b/src/librustc_mir/interpret/visitor.rs @@ -242,7 +242,7 @@ macro_rules! make_value_visitor { match v.layout().ty.sty { ty::Dynamic(..) => { // immediate trait objects are not a thing - let dest = v.to_op(self.ecx())?.to_mem_place(); + let dest = v.to_op(self.ecx())?.assert_mem_place(); let inner = self.ecx().unpack_dyn_trait(dest)?.1; trace!("walk_value: dyn object layout: {:#?}", inner.layout); // recurse with the inner type @@ -316,7 +316,7 @@ macro_rules! make_value_visitor { MPlaceTy::dangling(v.layout(), self.ecx()) } else { // non-ZST array/slice/str cannot be immediate - v.to_op(self.ecx())?.to_mem_place() + v.to_op(self.ecx())?.assert_mem_place() }; // Now we can go over all the fields. let iter = self.ecx().mplace_array_fields(mplace)?