Auto merge of #114011 - RalfJung:place-projection, r=oli-obk

interpret: Unify projections for MPlaceTy, PlaceTy, OpTy

For ~forever, we didn't really have proper shared code for handling projections into those three types. This is mostly because `PlaceTy` projections require `&mut self`: they might have to `force_allocate` to be able to represent a project part-way into a local.

This PR finally fixes that, by enhancing `Place::Local` with an `offset` so that such an optimized place can point into a part of a place without having requiring an in-memory representation. If we later write to that place, we will still do `force_allocate` -- for now we don't have an optimized path in `write_immediate` that would avoid allocation for partial overwrites of immediately stored locals. But in `write_immediate` we have `&mut self` so at least this no longer pollutes all our type signatures.

(Ironically, I seem to distantly remember that many years ago, `Place::Local` *did* have an `offset`, and I removed it to simplify things. I guess I didn't realize why it was so useful... I am also not sure if this was actually used to achieve place projection on `&self` back then.)

The `offset` had type `Option<Size>`, where `None` represent "no projection was applied". This is needed because locals *can* be unsized (when they are arguments) but `Place::Local` cannot store metadata: if the offset is `None`, this refers to the entire local, so we can use the metadata of the local itself (which must be indirect); if a projection gets applied, since the local is indirect, it will turn into a `Place::Ptr`. (Note that even for indirect locals we can have `Place::Local`: when the local appears in MIR, we always start with `Place::Local`, and only check `frame.locals` later. We could eagerly normalize to `Place::Ptr` but I don't think that would actually simplify things much.)

Having done all that, we can finally properly abstract projections: we have a new `Projectable` trait that has the basic methods required for projecting, and then all projection methods are implemented for anything that implements that trait. We can even implement it for `ImmTy`! (Not that we need that, but it seems neat.) The visitor can be greatly simplified; it doesn't need its own trait any more but it can use the `Projectable` trait. We also don't need the separate `Mut` visitor any more; that was required only to reflect that projections on `PlaceTy` needed `&mut self`.

It is possible that there are some more `&mut self` that can now become `&self`... I guess we'll notice that over time.

r? `@oli-obk`

src/borrow_tracker/stacked_borrows/mod.rs

@@ -928,13 +928,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                 Ok(())
             }
         }
-        impl<'ecx, 'mir, 'tcx> MutValueVisitor<'mir, 'tcx, MiriMachine<'mir, 'tcx>>
+        impl<'ecx, 'mir, 'tcx> ValueVisitor<'mir, 'tcx, MiriMachine<'mir, 'tcx>>
             for RetagVisitor<'ecx, 'mir, 'tcx>
         {
             type V = PlaceTy<'tcx, Provenance>;
 
             #[inline(always)]
-            fn ecx(&mut self) -> &mut MiriInterpCx<'mir, 'tcx> {
+            fn ecx(&self) -> &MiriInterpCx<'mir, 'tcx> {
                 self.ecx
             }
 

src/borrow_tracker/tree_borrows/mod.rs

@@ -413,13 +413,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                 Ok(())
             }
         }
-        impl<'ecx, 'mir, 'tcx> MutValueVisitor<'mir, 'tcx, MiriMachine<'mir, 'tcx>>
+        impl<'ecx, 'mir, 'tcx> ValueVisitor<'mir, 'tcx, MiriMachine<'mir, 'tcx>>
             for RetagVisitor<'ecx, 'mir, 'tcx>
         {
             type V = PlaceTy<'tcx, Provenance>;
 
             #[inline(always)]
-            fn ecx(&mut self) -> &mut MiriInterpCx<'mir, 'tcx> {
+            fn ecx(&self) -> &MiriInterpCx<'mir, 'tcx> {
                 self.ecx
             }
 
@@ -578,22 +578,22 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
 /// I.e. input is what you get from the visitor upon encountering an `adt` that is `Unique`,
 /// and output can be used by `retag_ptr_inplace`.
 fn inner_ptr_of_unique<'tcx>(
-    ecx: &mut MiriInterpCx<'_, 'tcx>,
+    ecx: &MiriInterpCx<'_, 'tcx>,
     place: &PlaceTy<'tcx, Provenance>,
 ) -> InterpResult<'tcx, PlaceTy<'tcx, Provenance>> {
     // Follows the same layout as `interpret/visitor.rs:walk_value` for `Box` in
     // `rustc_const_eval`, just with one fewer layer.
     // Here we have a `Unique(NonNull(*mut), PhantomData)`
     assert_eq!(place.layout.fields.count(), 2, "Unique must have exactly 2 fields");
-    let (nonnull, phantom) = (ecx.place_field(place, 0)?, ecx.place_field(place, 1)?);
+    let (nonnull, phantom) = (ecx.project_field(place, 0)?, ecx.project_field(place, 1)?);
     assert!(
         phantom.layout.ty.ty_adt_def().is_some_and(|adt| adt.is_phantom_data()),
         "2nd field of `Unique` should be `PhantomData` but is `{:?}`",
         phantom.layout.ty,
     );
     // Now down to `NonNull(*mut)`
     assert_eq!(nonnull.layout.fields.count(), 1, "NonNull must have exactly 1 field");
-    let ptr = ecx.place_field(&nonnull, 0)?;
+    let ptr = ecx.project_field(&nonnull, 0)?;
     // Finally a plain `*mut`
     Ok(ptr)
 }

src/eval.rs

@@ -320,7 +320,7 @@ pub fn create_ecx<'mir, 'tcx: 'mir>(
         ))?;
         let argvs_place = ecx.allocate(argvs_layout, MiriMemoryKind::Machine.into())?;
         for (idx, arg) in argvs.into_iter().enumerate() {
-            let place = ecx.mplace_field(&argvs_place, idx)?;
+            let place = ecx.project_field(&argvs_place, idx)?;
             ecx.write_immediate(arg, &place.into())?;
         }
         ecx.mark_immutable(&argvs_place);
@@ -354,7 +354,7 @@ pub fn create_ecx<'mir, 'tcx: 'mir>(
             ecx.machine.cmd_line = Some(*cmd_place);
             // Store the UTF-16 string. We just allocated so we know the bounds are fine.
             for (idx, &c) in cmd_utf16.iter().enumerate() {
-                let place = ecx.mplace_field(&cmd_place, idx)?;
+                let place = ecx.project_field(&cmd_place, idx)?;
                 ecx.write_scalar(Scalar::from_u16(c), &place.into())?;
             }
             ecx.mark_immutable(&cmd_place);

src/helpers.rs

@@ -10,14 +10,15 @@ use log::trace;
 
 use rustc_hir::def::{DefKind, Namespace};
 use rustc_hir::def_id::{DefId, CRATE_DEF_INDEX};
+use rustc_index::IndexVec;
 use rustc_middle::mir;
 use rustc_middle::ty::{
     self,
     layout::{LayoutOf, TyAndLayout},
     List, TyCtxt,
 };
 use rustc_span::{def_id::CrateNum, sym, Span, Symbol};
-use rustc_target::abi::{Align, FieldsShape, Size, Variants};
+use rustc_target::abi::{Align, FieldIdx, FieldsShape, Size, Variants};
 use rustc_target::spec::abi::Abi;
 
 use rand::RngCore;
@@ -229,20 +230,20 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         this.layout_of(ty).unwrap()
     }
 
-    /// Project to the given *named* field of the mplace (which must be a struct or union type).
-    fn mplace_field_named(
+    /// Project to the given *named* field (which must be a struct or union type).
+    fn project_field_named<P: Projectable<'mir, 'tcx, Provenance>>(
         &self,
-        mplace: &MPlaceTy<'tcx, Provenance>,
+        base: &P,
         name: &str,
-    ) -> InterpResult<'tcx, MPlaceTy<'tcx, Provenance>> {
+    ) -> InterpResult<'tcx, P> {
         let this = self.eval_context_ref();
-        let adt = mplace.layout.ty.ty_adt_def().unwrap();
+        let adt = base.layout().ty.ty_adt_def().unwrap();
         for (idx, field) in adt.non_enum_variant().fields.iter().enumerate() {
             if field.name.as_str() == name {
-                return this.mplace_field(mplace, idx);
+                return this.project_field(base, idx);
             }
         }
-        bug!("No field named {} in type {}", name, mplace.layout.ty);
+        bug!("No field named {} in type {}", name, base.layout().ty);
     }
 
     /// Write an int of the appropriate size to `dest`. The target type may be signed or unsigned,
@@ -270,7 +271,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
     ) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
         for (idx, &val) in values.iter().enumerate() {
-            let field = this.mplace_field(dest, idx)?;
+            let field = this.project_field(dest, idx)?;
             this.write_int(val, &field.into())?;
         }
         Ok(())
@@ -284,7 +285,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
     ) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
         for &(name, val) in values.iter() {
-            let field = this.mplace_field_named(dest, name)?;
+            let field = this.project_field_named(dest, name)?;
             this.write_int(val, &field.into())?;
         }
         Ok(())
@@ -301,8 +302,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
     }
 
     /// Get the `Place` for a local
-    fn local_place(&mut self, local: mir::Local) -> InterpResult<'tcx, PlaceTy<'tcx, Provenance>> {
-        let this = self.eval_context_mut();
+    fn local_place(&self, local: mir::Local) -> InterpResult<'tcx, PlaceTy<'tcx, Provenance>> {
+        let this = self.eval_context_ref();
         let place = mir::Place { local, projection: List::empty() };
         this.eval_place(place)
     }
@@ -479,6 +480,16 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                 self.ecx
             }
 
+            fn aggregate_field_order(memory_index: &IndexVec<FieldIdx, u32>, idx: usize) -> usize {
+                // We need to do an *inverse* lookup: find the field that has position `idx` in memory order.
+                for (src_field, &mem_pos) in memory_index.iter_enumerated() {
+                    if mem_pos as usize == idx {
+                        return src_field.as_usize();
+                    }
+                }
+                panic!("invalid `memory_index`, could not find {}-th field in memory order", idx);
+            }
+
             // Hook to detect `UnsafeCell`.
             fn visit_value(&mut self, v: &MPlaceTy<'tcx, Provenance>) -> InterpResult<'tcx> {
                 trace!("UnsafeCellVisitor: {:?} {:?}", *v, v.layout.ty);
@@ -524,33 +535,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                 }
             }
 
-            // Make sure we visit aggregates in increasing offset order.
-            fn visit_aggregate(
-                &mut self,
-                place: &MPlaceTy<'tcx, Provenance>,
-                fields: impl Iterator<Item = InterpResult<'tcx, MPlaceTy<'tcx, Provenance>>>,
-            ) -> InterpResult<'tcx> {
-                match place.layout.fields {
-                    FieldsShape::Array { .. } => {
-                        // For the array layout, we know the iterator will yield sorted elements so
-                        // we can avoid the allocation.
-                        self.walk_aggregate(place, fields)
-                    }
-                    FieldsShape::Arbitrary { .. } => {
-                        // Gather the subplaces and sort them before visiting.
-                        let mut places = fields
-                            .collect::<InterpResult<'tcx, Vec<MPlaceTy<'tcx, Provenance>>>>()?;
-                        // we just compare offsets, the abs. value never matters
-                        places.sort_by_key(|place| place.ptr.addr());
-                        self.walk_aggregate(place, places.into_iter().map(Ok))
-                    }
-                    FieldsShape::Union { .. } | FieldsShape::Primitive => {
-                        // Uh, what?
-                        bug!("unions/primitives are not aggregates we should ever visit")
-                    }
-                }
-            }
-
             fn visit_union(
                 &mut self,
                 _v: &MPlaceTy<'tcx, Provenance>,
@@ -746,7 +730,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         Ok(mplace)
     }
 
-    fn deref_pointer_as(
+    /// Deref' a pointer *without* checking that the place is dereferenceable.
+    fn deref_pointer_unchecked(
         &self,
         val: &ImmTy<'tcx, Provenance>,
         layout: TyAndLayout<'tcx>,
@@ -811,10 +796,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         tp: &MPlaceTy<'tcx, Provenance>,
     ) -> InterpResult<'tcx, Option<Duration>> {
         let this = self.eval_context_mut();
-        let seconds_place = this.mplace_field(tp, 0)?;
+        let seconds_place = this.project_field(tp, 0)?;
         let seconds_scalar = this.read_scalar(&seconds_place.into())?;
         let seconds = seconds_scalar.to_target_isize(this)?;
-        let nanoseconds_place = this.mplace_field(tp, 1)?;
+        let nanoseconds_place = this.project_field(tp, 1)?;
         let nanoseconds_scalar = this.read_scalar(&nanoseconds_place.into())?;
         let nanoseconds = nanoseconds_scalar.to_target_isize(this)?;
 

src/shims/backtrace.rs

@@ -83,7 +83,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
 
                 // Write pointers into array
                 for (i, ptr) in ptrs.into_iter().enumerate() {
-                    let place = this.mplace_index(&alloc, i as u64)?;
+                    let place = this.project_index(&alloc, i as u64)?;
 
                     this.write_pointer(ptr, &place.into())?;
                 }
@@ -196,33 +196,33 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
 
                 this.write_immediate(
                     name_alloc.to_ref(this),
-                    &this.mplace_field(&dest, 0)?.into(),
+                    &this.project_field(&dest, 0)?.into(),
                 )?;
                 this.write_immediate(
                     filename_alloc.to_ref(this),
-                    &this.mplace_field(&dest, 1)?.into(),
+                    &this.project_field(&dest, 1)?.into(),
                 )?;
             }
             1 => {
                 this.write_scalar(
                     Scalar::from_target_usize(name.len().try_into().unwrap(), this),
-                    &this.mplace_field(&dest, 0)?.into(),
+                    &this.project_field(&dest, 0)?.into(),
                 )?;
                 this.write_scalar(
                     Scalar::from_target_usize(filename.len().try_into().unwrap(), this),
-                    &this.mplace_field(&dest, 1)?.into(),
+                    &this.project_field(&dest, 1)?.into(),
                 )?;
             }
             _ => throw_unsup_format!("unknown `miri_resolve_frame` flags {}", flags),
         }
 
-        this.write_scalar(Scalar::from_u32(lineno), &this.mplace_field(&dest, 2)?.into())?;
-        this.write_scalar(Scalar::from_u32(colno), &this.mplace_field(&dest, 3)?.into())?;
+        this.write_scalar(Scalar::from_u32(lineno), &this.project_field(&dest, 2)?.into())?;
+        this.write_scalar(Scalar::from_u32(colno), &this.project_field(&dest, 3)?.into())?;
 
         // Support a 4-field struct for now - this is deprecated
         // and slated for removal.
         if num_fields == 5 {
-            this.write_pointer(fn_ptr, &this.mplace_field(&dest, 4)?.into())?;
+            this.write_pointer(fn_ptr, &this.project_field(&dest, 4)?.into())?;
         }
 
         Ok(())

src/shims/env.rs

@@ -456,7 +456,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         ))?;
         let vars_place = this.allocate(vars_layout, MiriMemoryKind::Runtime.into())?;
         for (idx, var) in vars.into_iter().enumerate() {
-            let place = this.mplace_field(&vars_place, idx)?;
+            let place = this.project_field(&vars_place, idx)?;
             this.write_pointer(var, &place.into())?;
         }
         this.write_pointer(vars_place.ptr, &this.machine.env_vars.environ.unwrap().into())?;

src/shims/foreign_items.rs

@@ -942,9 +942,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                 #[allow(clippy::arithmetic_side_effects)] // it's a u128, we can shift by 64
                 let (c_out, sum) = ((wide_sum >> 64).truncate::<u8>(), wide_sum.truncate::<u64>());
 
-                let c_out_field = this.place_field(dest, 0)?;
+                let c_out_field = this.project_field(dest, 0)?;
                 this.write_scalar(Scalar::from_u8(c_out), &c_out_field)?;
-                let sum_field = this.place_field(dest, 1)?;
+                let sum_field = this.project_field(dest, 1)?;
                 this.write_scalar(Scalar::from_u64(sum), &sum_field)?;
             }
             "llvm.x86.sse2.pause"