Auto merge of rust-lang#62441 - RalfJung:place-ptr-normalization, r=oli-obk

Miri: Provide pointer forcing methods for MemPlace and Op

These are useful when one wants to to a lot of work with some place or operand and not to int-to-ptr casts all the time. In particular, this is needed to fix some test failures in Miri: we need to normalize before starting a visitor that walks a run-time value, so that we can later be sure (during the visitor walk) that we have a proper `Pointer`.

Also see the Miri side at rust-lang/miri#830.

Cc @eddyb @oli-obk

Author: bors

src/librustc/mir/interpret/value.rs

@@ -262,19 +262,6 @@ impl<'tcx, Tag> Scalar<Tag> {
         }
     }
 
-    /// Returns this pointer's offset from the allocation base, or from NULL (for
-    /// integer pointers).
-    #[inline]
-    pub fn get_ptr_offset(self, cx: &impl HasDataLayout) -> Size {
-        match self {
-            Scalar::Raw { data, size } => {
-                assert_eq!(size as u64, cx.pointer_size().bytes());
-                Size::from_bytes(data as u64)
-            }
-            Scalar::Ptr(ptr) => ptr.offset,
-        }
-    }
-
     #[inline]
     pub fn from_bool(b: bool) -> Self {
         Scalar::Raw { data: b as u128, size: 1 }
@@ -339,6 +326,10 @@ impl<'tcx, Tag> Scalar<Tag> {
         Scalar::Raw { data: f.to_bits(), size: 8 }
     }
 
+    /// This is very rarely the method you want!  You should dispatch on the type
+    /// and use `force_bits`/`assert_bits`/`force_ptr`/`assert_ptr`.
+    /// This method only exists for the benefit of low-level memory operations
+    /// as well as the implementation of the `force_*` methods.
     #[inline]
     pub fn to_bits_or_ptr(
         self,
@@ -359,6 +350,7 @@ impl<'tcx, Tag> Scalar<Tag> {
         }
     }
 
+    /// Do not call this method!  Use either `assert_bits` or `force_bits`.
     #[inline]
     pub fn to_bits(self, target_size: Size) -> InterpResult<'tcx, u128> {
         match self {
@@ -372,6 +364,12 @@ impl<'tcx, Tag> Scalar<Tag> {
         }
     }
 
+    #[inline(always)]
+    pub fn assert_bits(self, target_size: Size) -> u128 {
+        self.to_bits(target_size).expect("Expected Raw bits but got a Pointer")
+    }
+
+    /// Do not call this method!  Use either `assert_ptr` or `force_ptr`.
     #[inline]
     pub fn to_ptr(self) -> InterpResult<'tcx, Pointer<Tag>> {
         match self {
@@ -381,6 +379,12 @@ impl<'tcx, Tag> Scalar<Tag> {
         }
     }
 
+    #[inline(always)]
+    pub fn assert_ptr(self) -> Pointer<Tag> {
+        self.to_ptr().expect("Expected a Pointer but got Raw bits")
+    }
+
+    /// Do not call this method!  Dispatch based on the type instead.
     #[inline]
     pub fn is_bits(self) -> bool {
         match self {
@@ -389,6 +393,7 @@ impl<'tcx, Tag> Scalar<Tag> {
         }
     }
 
+    /// Do not call this method!  Dispatch based on the type instead.
     #[inline]
     pub fn is_ptr(self) -> bool {
         match self {
@@ -536,11 +541,13 @@ impl<'tcx, Tag> ScalarMaybeUndef<Tag> {
         }
     }
 
+    /// Do not call this method!  Use either `assert_ptr` or `force_ptr`.
     #[inline(always)]
     pub fn to_ptr(self) -> InterpResult<'tcx, Pointer<Tag>> {
         self.not_undef()?.to_ptr()
     }
 
+    /// Do not call this method!  Use either `assert_bits` or `force_bits`.
     #[inline(always)]
     pub fn to_bits(self, target_size: Size) -> InterpResult<'tcx, u128> {
         self.not_undef()?.to_bits(target_size)

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 }
@@ -661,7 +661,7 @@ pub fn const_eval_raw_provider<'tcx>(
         |body| eval_body_using_ecx(&mut ecx, cid, body, key.param_env)
     ).and_then(|place| {
         Ok(RawConst {
-            alloc_id: place.to_ptr().expect("we allocated this ptr!").alloc_id,
+            alloc_id: place.ptr.assert_ptr().alloc_id,
             ty: place.layout.ty
         })
     }).map_err(|error| {

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<M::PointerTag>,
@@ -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<M::PointerTag>,
-        src_align: Align,
-        dest: Scalar<M::PointerTag>,
-        dest_align: Align,
+        src: Pointer<M::PointerTag>,
+        dest: Pointer<M::PointerTag>,
         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<M::PointerTag>,
-        src_align: Align,
-        dest: Scalar<M::PointerTag>,
-        dest_align: Align,
+        src: Pointer<M::PointerTag>,
+        dest: Pointer<M::PointerTag>,
         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.

src/librustc_mir/interpret/operand.rs

@@ -123,23 +123,23 @@ pub enum Operand<Tag=(), Id=AllocId> {
 
 impl<Tag> Operand<Tag> {
     #[inline]
-    pub fn to_mem_place(self) -> MemPlace<Tag>
+    pub fn assert_mem_place(self) -> MemPlace<Tag>
         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<Tag>
+    pub fn assert_immediate(self) -> Immediate<Tag>
         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 force_op_ptr(
+        &self,
+        op: OpTy<'tcx, M::PointerTag>,
+    ) -> InterpResult<'tcx, OpTy<'tcx, M::PointerTag>> {
+        match op.try_as_mplace() {
+            Ok(mplace) => Ok(self.force_mplace_ptr(mplace)?.into()),
+            Err(imm) => Ok(imm.into()), // Nothing to cast/force
+        }
+    }
+
     /// 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()
             }
         })