From bdd5855b8e127f4a258b0bd90cd5d2dbade1b3cc Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 4 Aug 2023 21:42:35 +0200 Subject: [PATCH] interpret: fix projecting into an unsized field of a local new invariant: Place::Local never refers to something unsized --- .../src/interpret/eval_context.rs | 4 +- .../rustc_const_eval/src/interpret/operand.rs | 56 +++++++--------- .../rustc_const_eval/src/interpret/place.rs | 66 +++++++++---------- .../src/interpret/projection.rs | 66 +++++++++++-------- .../rustc_const_eval/src/interpret/visitor.rs | 5 +- src/tools/miri/tests/pass/unsized.rs | 24 +++++++ 6 files changed, 124 insertions(+), 97 deletions(-) diff --git a/compiler/rustc_const_eval/src/interpret/eval_context.rs b/compiler/rustc_const_eval/src/interpret/eval_context.rs index bd3d87470c9..6902988d64d 100644 --- a/compiler/rustc_const_eval/src/interpret/eval_context.rs +++ b/compiler/rustc_const_eval/src/interpret/eval_context.rs @@ -177,7 +177,7 @@ pub enum LocalValue { impl<'tcx, Prov: Provenance + 'static> LocalState<'tcx, Prov> { /// Read the local's value or error if the local is not yet live or not live anymore. - #[inline] + #[inline(always)] pub fn access(&self) -> InterpResult<'tcx, &Operand> { match &self.value { LocalValue::Dead => throw_ub!(DeadLocal), // could even be "invalid program"? @@ -190,7 +190,7 @@ impl<'tcx, Prov: Provenance + 'static> LocalState<'tcx, Prov> { /// /// Note: This may only be invoked from the `Machine::access_local_mut` hook and not from /// anywhere else. You may be invalidating machine invariants if you do! - #[inline] + #[inline(always)] pub fn access_mut(&mut self) -> InterpResult<'tcx, &mut Operand> { match &mut self.value { LocalValue::Dead => throw_ub!(DeadLocal), // could even be "invalid program"? diff --git a/compiler/rustc_const_eval/src/interpret/operand.rs b/compiler/rustc_const_eval/src/interpret/operand.rs index 443d3c10871..55b8bf7fbb3 100644 --- a/compiler/rustc_const_eval/src/interpret/operand.rs +++ b/compiler/rustc_const_eval/src/interpret/operand.rs @@ -291,23 +291,21 @@ impl<'tcx, Prov: Provenance> Projectable<'tcx, Prov> for ImmTy<'tcx, Prov> { self.layout } - fn meta<'mir, M: Machine<'mir, 'tcx, Provenance = Prov>>( - &self, - _ecx: &InterpCx<'mir, 'tcx, M>, - ) -> InterpResult<'tcx, MemPlaceMeta> { - assert!(self.layout.is_sized()); // unsized ImmTy can only exist temporarily and should never reach this here + #[inline(always)] + fn meta(&self) -> InterpResult<'tcx, MemPlaceMeta> { + debug_assert!(self.layout.is_sized()); // unsized ImmTy can only exist temporarily and should never reach this here Ok(MemPlaceMeta::None) } - fn offset_with_meta( + fn offset_with_meta<'mir, M: Machine<'mir, 'tcx, Provenance = Prov>>( &self, offset: Size, meta: MemPlaceMeta, layout: TyAndLayout<'tcx>, - cx: &impl HasDataLayout, + ecx: &InterpCx<'mir, 'tcx, M>, ) -> InterpResult<'tcx, Self> { assert_matches!(meta, MemPlaceMeta::None); // we can't store this anywhere anyway - Ok(self.offset_(offset, layout, cx)) + Ok(self.offset_(offset, layout, ecx)) } fn to_op<'mir, M: Machine<'mir, 'tcx, Provenance = Prov>>( @@ -318,49 +316,39 @@ impl<'tcx, Prov: Provenance> Projectable<'tcx, Prov> for ImmTy<'tcx, Prov> { } } -impl<'tcx, Prov: Provenance> OpTy<'tcx, Prov> { - // Provided as inherent method since it doesn't need the `ecx` of `Projectable::meta`. - pub fn meta(&self) -> InterpResult<'tcx, MemPlaceMeta> { - Ok(if self.layout.is_unsized() { - if matches!(self.op, Operand::Immediate(_)) { - // Unsized immediate OpTy cannot occur. We create a MemPlace for all unsized locals during argument passing. - // However, ConstProp doesn't do that, so we can run into this nonsense situation. - throw_inval!(ConstPropNonsense); - } - // There are no unsized immediates. - self.assert_mem_place().meta - } else { - MemPlaceMeta::None - }) - } -} - impl<'tcx, Prov: Provenance + 'static> Projectable<'tcx, Prov> for OpTy<'tcx, Prov> { #[inline(always)] fn layout(&self) -> TyAndLayout<'tcx> { self.layout } - fn meta<'mir, M: Machine<'mir, 'tcx, Provenance = Prov>>( - &self, - _ecx: &InterpCx<'mir, 'tcx, M>, - ) -> InterpResult<'tcx, MemPlaceMeta> { - self.meta() + fn meta(&self) -> InterpResult<'tcx, MemPlaceMeta> { + Ok(match self.as_mplace_or_imm() { + Left(mplace) => mplace.meta, + Right(_) => { + if self.layout.is_unsized() { + // Unsized immediate OpTy cannot occur. We create a MemPlace for all unsized locals during argument passing. + // However, ConstProp doesn't do that, so we can run into this nonsense situation. + throw_inval!(ConstPropNonsense); + } + MemPlaceMeta::None + } + }) } - fn offset_with_meta( + fn offset_with_meta<'mir, M: Machine<'mir, 'tcx, Provenance = Prov>>( &self, offset: Size, meta: MemPlaceMeta, layout: TyAndLayout<'tcx>, - cx: &impl HasDataLayout, + ecx: &InterpCx<'mir, 'tcx, M>, ) -> InterpResult<'tcx, Self> { match self.as_mplace_or_imm() { - Left(mplace) => Ok(mplace.offset_with_meta(offset, meta, layout, cx)?.into()), + Left(mplace) => Ok(mplace.offset_with_meta(offset, meta, layout, ecx)?.into()), Right(imm) => { assert!(!meta.has_meta()); // no place to store metadata here // Every part of an uninit is uninit. - Ok(imm.offset(offset, layout, cx)?.into()) + Ok(imm.offset(offset, layout, ecx)?.into()) } } } diff --git a/compiler/rustc_const_eval/src/interpret/place.rs b/compiler/rustc_const_eval/src/interpret/place.rs index 40a7a0f4e56..b73bc661946 100644 --- a/compiler/rustc_const_eval/src/interpret/place.rs +++ b/compiler/rustc_const_eval/src/interpret/place.rs @@ -111,6 +111,8 @@ pub enum Place { /// (Without that optimization, we'd just always be a `MemPlace`.) /// Note that this only stores the frame index, not the thread this frame belongs to -- that is /// implicit. This means a `Place` must never be moved across interpreter thread boundaries! + /// + /// This variant shall not be used for unsized types -- those must always live in memory. Local { frame: usize, local: mir::Local, offset: Option }, } @@ -157,7 +159,7 @@ impl MemPlace { } /// Turn a mplace into a (thin or wide) pointer, as a reference, pointing to the same space. - #[inline(always)] + #[inline] pub fn to_ref(self, cx: &impl HasDataLayout) -> Immediate { match self.meta { MemPlaceMeta::None => Immediate::from(Scalar::from_maybe_pointer(self.ptr, cx)), @@ -220,22 +222,20 @@ impl<'tcx, Prov: Provenance + 'static> Projectable<'tcx, Prov> for MPlaceTy<'tcx self.layout } - fn meta<'mir, M: Machine<'mir, 'tcx, Provenance = Prov>>( - &self, - _ecx: &InterpCx<'mir, 'tcx, M>, - ) -> InterpResult<'tcx, MemPlaceMeta> { + #[inline(always)] + fn meta(&self) -> InterpResult<'tcx, MemPlaceMeta> { Ok(self.meta) } - fn offset_with_meta( + fn offset_with_meta<'mir, M: Machine<'mir, 'tcx, Provenance = Prov>>( &self, offset: Size, meta: MemPlaceMeta, layout: TyAndLayout<'tcx>, - cx: &impl HasDataLayout, + ecx: &InterpCx<'mir, 'tcx, M>, ) -> InterpResult<'tcx, Self> { Ok(MPlaceTy { - mplace: self.mplace.offset_with_meta_(offset, meta, cx)?, + mplace: self.mplace.offset_with_meta_(offset, meta, ecx)?, align: self.align.restrict_for_offset(offset), layout, }) @@ -255,25 +255,33 @@ impl<'tcx, Prov: Provenance + 'static> Projectable<'tcx, Prov> for PlaceTy<'tcx, self.layout } - fn meta<'mir, M: Machine<'mir, 'tcx, Provenance = Prov>>( - &self, - ecx: &InterpCx<'mir, 'tcx, M>, - ) -> InterpResult<'tcx, MemPlaceMeta> { - ecx.place_meta(self) + fn meta(&self) -> InterpResult<'tcx, MemPlaceMeta> { + Ok(match self.as_mplace_or_local() { + Left(mplace) => mplace.meta, + Right(_) => { + if self.layout.is_unsized() { + // Unsized `Place::Local` cannot occur. We create a MemPlace for all unsized locals during argument passing. + // However, ConstProp doesn't do that, so we can run into this nonsense situation. + throw_inval!(ConstPropNonsense); + } + MemPlaceMeta::None + } + }) } - fn offset_with_meta( + fn offset_with_meta<'mir, M: Machine<'mir, 'tcx, Provenance = Prov>>( &self, offset: Size, meta: MemPlaceMeta, layout: TyAndLayout<'tcx>, - cx: &impl HasDataLayout, + ecx: &InterpCx<'mir, 'tcx, M>, ) -> InterpResult<'tcx, Self> { Ok(match self.as_mplace_or_local() { - Left(mplace) => mplace.offset_with_meta(offset, meta, layout, cx)?.into(), + Left(mplace) => mplace.offset_with_meta(offset, meta, layout, ecx)?.into(), Right((frame, local, old_offset)) => { + debug_assert!(layout.is_sized(), "unsized locals should live in memory"); assert_matches!(meta, MemPlaceMeta::None); // we couldn't store it anyway... - let new_offset = cx + let new_offset = ecx .data_layout() .offset(old_offset.unwrap_or(Size::ZERO).bytes(), offset.bytes())?; PlaceTy { @@ -399,20 +407,6 @@ where Prov: Provenance + 'static, M: Machine<'mir, 'tcx, Provenance = Prov>, { - /// Get the metadata of the given place. - pub(super) fn place_meta( - &self, - place: &PlaceTy<'tcx, M::Provenance>, - ) -> InterpResult<'tcx, MemPlaceMeta> { - if place.layout.is_unsized() { - // For `Place::Local`, the metadata is stored with the local, not the place. So we have - // to look that up first. - self.place_to_op(place)?.meta() - } else { - Ok(MemPlaceMeta::None) - } - } - /// Take a value, which represents a (thin or wide) reference, and make it a place. /// Alignment is just based on the type. This is the inverse of `mplace_to_ref()`. /// @@ -537,8 +531,14 @@ where frame: usize, local: mir::Local, ) -> InterpResult<'tcx, PlaceTy<'tcx, M::Provenance>> { - let layout = self.layout_of_local(&self.stack()[frame], local, None)?; - let place = Place::Local { frame, local, offset: None }; + // Other parts of the system rely on `Place::Local` never being unsized. + // So we eagerly check here if this local has an MPlace, and if yes we use it. + let frame_ref = &self.stack()[frame]; + let layout = self.layout_of_local(frame_ref, local, None)?; + let place = match frame_ref.locals[local].access()? { + Operand::Immediate(_) => Place::Local { frame, local, offset: None }, + Operand::Indirect(mplace) => Place::Ptr(*mplace), + }; Ok(PlaceTy { place, layout, align: layout.align.abi }) } diff --git a/compiler/rustc_const_eval/src/interpret/projection.rs b/compiler/rustc_const_eval/src/interpret/projection.rs index 882097ad2c3..ecf594773fe 100644 --- a/compiler/rustc_const_eval/src/interpret/projection.rs +++ b/compiler/rustc_const_eval/src/interpret/projection.rs @@ -7,12 +7,13 @@ //! but we still need to do bounds checking and adjust the layout. To not duplicate that with MPlaceTy, we actually //! implement the logic on OpTy, and MPlaceTy calls that. +use std::marker::PhantomData; +use std::ops::Range; + use rustc_middle::mir; use rustc_middle::ty; use rustc_middle::ty::layout::{LayoutOf, TyAndLayout}; use rustc_middle::ty::Ty; -use rustc_middle::ty::TyCtxt; -use rustc_target::abi::HasDataLayout; use rustc_target::abi::Size; use rustc_target::abi::{self, VariantIdx}; @@ -24,44 +25,42 @@ pub trait Projectable<'tcx, Prov: Provenance>: Sized + std::fmt::Debug { fn layout(&self) -> TyAndLayout<'tcx>; /// Get the metadata of a wide value. - fn meta<'mir, M: Machine<'mir, 'tcx, Provenance = Prov>>( - &self, - ecx: &InterpCx<'mir, 'tcx, M>, - ) -> InterpResult<'tcx, MemPlaceMeta>; + fn meta(&self) -> InterpResult<'tcx, MemPlaceMeta>; fn len<'mir, M: Machine<'mir, 'tcx, Provenance = Prov>>( &self, ecx: &InterpCx<'mir, 'tcx, M>, ) -> InterpResult<'tcx, u64> { - self.meta(ecx)?.len(self.layout(), ecx) + self.meta()?.len(self.layout(), ecx) } /// Offset the value by the given amount, replacing the layout and metadata. - fn offset_with_meta( + fn offset_with_meta<'mir, M: Machine<'mir, 'tcx, Provenance = Prov>>( &self, offset: Size, meta: MemPlaceMeta, layout: TyAndLayout<'tcx>, - cx: &impl HasDataLayout, + ecx: &InterpCx<'mir, 'tcx, M>, ) -> InterpResult<'tcx, Self>; - fn offset( + fn offset<'mir, M: Machine<'mir, 'tcx, Provenance = Prov>>( &self, offset: Size, layout: TyAndLayout<'tcx>, - cx: &impl HasDataLayout, + ecx: &InterpCx<'mir, 'tcx, M>, ) -> InterpResult<'tcx, Self> { assert!(layout.is_sized()); - self.offset_with_meta(offset, MemPlaceMeta::None, layout, cx) + self.offset_with_meta(offset, MemPlaceMeta::None, layout, ecx) } - fn transmute( + fn transmute<'mir, M: Machine<'mir, 'tcx, Provenance = Prov>>( &self, layout: TyAndLayout<'tcx>, - cx: &impl HasDataLayout, + ecx: &InterpCx<'mir, 'tcx, M>, ) -> InterpResult<'tcx, Self> { + assert!(self.layout().is_sized() && layout.is_sized()); assert_eq!(self.layout().size, layout.size); - self.offset_with_meta(Size::ZERO, MemPlaceMeta::None, layout, cx) + self.offset_with_meta(Size::ZERO, MemPlaceMeta::None, layout, ecx) } /// Convert this to an `OpTy`. This might be an irreversible transformation, but is useful for @@ -72,6 +71,28 @@ pub trait Projectable<'tcx, Prov: Provenance>: Sized + std::fmt::Debug { ) -> InterpResult<'tcx, OpTy<'tcx, M::Provenance>>; } +/// A type representing iteration over the elements of an array. +pub struct ArrayIterator<'tcx, 'a, Prov: Provenance + 'static, P: Projectable<'tcx, Prov>> { + base: &'a P, + range: Range, + stride: Size, + field_layout: TyAndLayout<'tcx>, + _phantom: PhantomData, // otherwise it says `Prov` is never used... +} + +impl<'tcx, 'a, Prov: Provenance + 'static, P: Projectable<'tcx, Prov>> + ArrayIterator<'tcx, 'a, Prov, P> +{ + /// Should be the same `ecx` on each call, and match the one used to create the iterator. + pub fn next<'mir, M: Machine<'mir, 'tcx, Provenance = Prov>>( + &mut self, + ecx: &InterpCx<'mir, 'tcx, M>, + ) -> InterpResult<'tcx, Option<(u64, P)>> { + let Some(idx) = self.range.next() else { return Ok(None) }; + Ok(Some((idx, self.base.offset(self.stride * idx, self.field_layout, ecx)?))) + } +} + // FIXME: Working around https://github.com/rust-lang/rust/issues/54385 impl<'mir, 'tcx: 'mir, Prov, M> InterpCx<'mir, 'tcx, M> where @@ -104,7 +125,7 @@ where // But const-prop actually feeds us such nonsense MIR! (see test `const_prop/issue-86351.rs`) throw_inval!(ConstPropNonsense); } - let base_meta = base.meta(self)?; + let base_meta = base.meta()?; // Re-use parent metadata to determine dynamic field layout. // With custom DSTS, this *will* execute user-defined code, but the same // happens at run-time so that's okay. @@ -132,7 +153,7 @@ where base: &P, variant: VariantIdx, ) -> InterpResult<'tcx, P> { - assert!(!base.meta(self)?.has_meta()); + assert!(!base.meta()?.has_meta()); // Downcasts only change the layout. // (In particular, no check about whether this is even the active variant -- that's by design, // see https://github.com/rust-lang/rust/issues/93688#issuecomment-1032929496.) @@ -206,20 +227,13 @@ where pub fn project_array_fields<'a, P: Projectable<'tcx, M::Provenance>>( &self, base: &'a P, - ) -> InterpResult<'tcx, impl Iterator> + 'a> - where - 'tcx: 'a, - { + ) -> InterpResult<'tcx, ArrayIterator<'tcx, 'a, M::Provenance, P>> { let abi::FieldsShape::Array { stride, .. } = base.layout().fields else { span_bug!(self.cur_span(), "operand_array_fields: expected an array layout"); }; let len = base.len(self)?; let field_layout = base.layout().field(self, 0); - let tcx: TyCtxt<'tcx> = *self.tcx; - // `Size` multiplication - Ok((0..len).map(move |i| { - base.offset_with_meta(stride * i, MemPlaceMeta::None, field_layout, &tcx) - })) + Ok(ArrayIterator { base, range: 0..len, stride, field_layout, _phantom: PhantomData }) } /// Subslicing diff --git a/compiler/rustc_const_eval/src/interpret/visitor.rs b/compiler/rustc_const_eval/src/interpret/visitor.rs index 531e2bd3ee0..fc21ad1f183 100644 --- a/compiler/rustc_const_eval/src/interpret/visitor.rs +++ b/compiler/rustc_const_eval/src/interpret/visitor.rs @@ -170,8 +170,9 @@ pub trait ValueVisitor<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>>: Sized { } } FieldsShape::Array { .. } => { - for (idx, field) in self.ecx().project_array_fields(v)?.enumerate() { - self.visit_field(v, idx, &field?)?; + let mut iter = self.ecx().project_array_fields(v)?; + while let Some((idx, field)) = iter.next(self.ecx())? { + self.visit_field(v, idx.try_into().unwrap(), &field)?; } } } diff --git a/src/tools/miri/tests/pass/unsized.rs b/src/tools/miri/tests/pass/unsized.rs index c9046dc3c76..5c6929882f6 100644 --- a/src/tools/miri/tests/pass/unsized.rs +++ b/src/tools/miri/tests/pass/unsized.rs @@ -2,6 +2,7 @@ //@[tree]compile-flags: -Zmiri-tree-borrows #![feature(unsized_tuple_coercion)] #![feature(unsized_fn_params)] +#![feature(custom_mir, core_intrinsics)] use std::mem; @@ -32,7 +33,30 @@ fn unsized_params() { f3(*p); } +fn unsized_field_projection() { + use std::intrinsics::mir::*; + + pub struct S(T); + + #[custom_mir(dialect = "runtime", phase = "optimized")] + fn f(x: S<[u8]>) { + mir! { + { + let idx = 0; + // Project to an unsized field of an unsized local. + x.0[idx] = 0; + let _val = x.0[idx]; + Return() + } + } + } + + let x: Box> = Box::new(S([0])); + f(*x); +} + fn main() { unsized_tuple(); unsized_params(); + unsized_field_projection(); }