From bdd5855b8e127f4a258b0bd90cd5d2dbade1b3cc Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 4 Aug 2023 21:42:35 +0200 Subject: [PATCH 1/5] 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(); } From a09df43d9fbbd21dfa2d872ff0671f80161c15f1 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 6 Aug 2023 18:40:37 +0200 Subject: [PATCH 2/5] move marking-locals-live out of push_stack_frame, so it happens with argument passing this entirely avoids even creating unsized locals in Immediate::Uninitialized state --- compiler/rustc_const_eval/messages.ftl | 2 +- .../src/const_eval/eval_queries.rs | 1 + compiler/rustc_const_eval/src/errors.rs | 3 +- .../src/interpret/eval_context.rs | 44 ++++--- .../rustc_const_eval/src/interpret/operand.rs | 43 ++++--- .../rustc_const_eval/src/interpret/place.rs | 30 +++-- .../src/interpret/terminator.rs | 113 ++++++++++++------ .../rustc_middle/src/mir/interpret/error.rs | 2 + .../rustc_mir_transform/src/const_prop.rs | 10 ++ .../src/const_prop_lint.rs | 10 ++ src/tools/miri/src/diagnostics.rs | 2 +- src/tools/miri/src/helpers.rs | 23 ++-- src/tools/miri/tests/fail/type-too-large.rs | 4 +- .../miri/tests/fail/type-too-large.stderr | 4 +- .../miri/tests/fail/unsized-local.stderr | 2 +- 15 files changed, 188 insertions(+), 105 deletions(-) diff --git a/compiler/rustc_const_eval/messages.ftl b/compiler/rustc_const_eval/messages.ftl index e5dd5729d89..020402fe25e 100644 --- a/compiler/rustc_const_eval/messages.ftl +++ b/compiler/rustc_const_eval/messages.ftl @@ -384,7 +384,7 @@ const_eval_unreachable_unwind = const_eval_unsigned_offset_from_overflow = `ptr_offset_from_unsigned` called when first pointer has smaller offset than second: {$a_offset} < {$b_offset} - +const_eval_unsized_local = unsized locals are not supported const_eval_unstable_const_fn = `{$def_path}` is not yet stable as a const fn const_eval_unstable_in_stable = diff --git a/compiler/rustc_const_eval/src/const_eval/eval_queries.rs b/compiler/rustc_const_eval/src/const_eval/eval_queries.rs index 4c7e9194401..fc0dba6b67b 100644 --- a/compiler/rustc_const_eval/src/const_eval/eval_queries.rs +++ b/compiler/rustc_const_eval/src/const_eval/eval_queries.rs @@ -61,6 +61,7 @@ fn eval_body_using_ecx<'mir, 'tcx>( &ret.clone().into(), StackPopCleanup::Root { cleanup: false }, )?; + ecx.storage_live_for_always_live_locals()?; // The main interpreter loop. while ecx.step()? {} diff --git a/compiler/rustc_const_eval/src/errors.rs b/compiler/rustc_const_eval/src/errors.rs index 4362cae7ed7..c74fed0e47f 100644 --- a/compiler/rustc_const_eval/src/errors.rs +++ b/compiler/rustc_const_eval/src/errors.rs @@ -795,6 +795,7 @@ impl ReportErrorExt for UnsupportedOpInfo { use crate::fluent_generated::*; match self { UnsupportedOpInfo::Unsupported(s) => s.clone().into(), + UnsupportedOpInfo::UnsizedLocal => const_eval_unsized_local, UnsupportedOpInfo::OverwritePartialPointer(_) => const_eval_partial_pointer_overwrite, UnsupportedOpInfo::ReadPartialPointer(_) => const_eval_partial_pointer_copy, UnsupportedOpInfo::ReadPointerAsInt(_) => const_eval_read_pointer_as_int, @@ -814,7 +815,7 @@ impl ReportErrorExt for UnsupportedOpInfo { // `ReadPointerAsInt(Some(info))` is never printed anyway, it only serves as an error to // be further processed by validity checking which then turns it into something nice to // print. So it's not worth the effort of having diagnostics that can print the `info`. - Unsupported(_) | ReadPointerAsInt(_) => {} + UnsizedLocal | Unsupported(_) | ReadPointerAsInt(_) => {} OverwritePartialPointer(ptr) | ReadPartialPointer(ptr) => { builder.set_arg("ptr", ptr); } diff --git a/compiler/rustc_const_eval/src/interpret/eval_context.rs b/compiler/rustc_const_eval/src/interpret/eval_context.rs index 6902988d64d..58674a1eec5 100644 --- a/compiler/rustc_const_eval/src/interpret/eval_context.rs +++ b/compiler/rustc_const_eval/src/interpret/eval_context.rs @@ -158,7 +158,9 @@ pub enum StackPopCleanup { #[derive(Clone, Debug)] pub struct LocalState<'tcx, Prov: Provenance = AllocId> { pub value: LocalValue, - /// Don't modify if `Some`, this is only used to prevent computing the layout twice + /// Don't modify if `Some`, this is only used to prevent computing the layout twice. + /// Layout needs to be computed lazily because ConstProp wants to run on frames where we can't + /// compute the layout of all locals. pub layout: Cell>>, } @@ -483,7 +485,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } #[inline(always)] - pub(super) fn body(&self) -> &'mir mir::Body<'tcx> { + pub fn body(&self) -> &'mir mir::Body<'tcx> { self.frame().body } @@ -705,15 +707,15 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { return_to_block: StackPopCleanup, ) -> InterpResult<'tcx> { trace!("body: {:#?}", body); + let dead_local = LocalState { value: LocalValue::Dead, layout: Cell::new(None) }; + let locals = IndexVec::from_elem(dead_local, &body.local_decls); // First push a stack frame so we have access to the local args let pre_frame = Frame { body, loc: Right(body.span), // Span used for errors caused during preamble. return_to_block, return_place: return_place.clone(), - // empty local array, we fill it in below, after we are inside the stack frame and - // all methods actually know about the frame - locals: IndexVec::new(), + locals, instance, tracing_span: SpanGuard::new(), extra: (), @@ -728,19 +730,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { self.eval_mir_constant(&ct, Some(span), None)?; } - // Most locals are initially dead. - let dummy = LocalState { value: LocalValue::Dead, layout: Cell::new(None) }; - let mut locals = IndexVec::from_elem(dummy, &body.local_decls); - - // Now mark those locals as live that have no `Storage*` annotations. - let always_live = always_storage_live_locals(self.body()); - for local in locals.indices() { - if always_live.contains(local) { - locals[local].value = LocalValue::Live(Operand::Immediate(Immediate::Uninit)); - } - } // done - self.frame_mut().locals = locals; M::after_stack_push(self)?; self.frame_mut().loc = Left(mir::Location::START); @@ -907,11 +897,29 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } } + /// In the current stack frame, mark all locals as live that are not arguments and don't have + /// `Storage*` annotations (this includes the return place). + pub fn storage_live_for_always_live_locals(&mut self) -> InterpResult<'tcx> { + self.storage_live(mir::RETURN_PLACE)?; + + let body = self.body(); + let always_live = always_storage_live_locals(body); + for local in body.vars_and_temps_iter() { + if always_live.contains(local) { + self.storage_live(local)?; + } + } + Ok(()) + } + /// Mark a storage as live, killing the previous content. pub fn storage_live(&mut self, local: mir::Local) -> InterpResult<'tcx> { - assert!(local != mir::RETURN_PLACE, "Cannot make return place live"); trace!("{:?} is now live", local); + if self.layout_of_local(self.frame(), local, None)?.is_unsized() { + throw_unsup!(UnsizedLocal); + } + let local_val = LocalValue::Live(Operand::Immediate(Immediate::Uninit)); // StorageLive expects the local to be dead, and marks it live. let old = mem::replace(&mut self.frame_mut().locals[local].value, local_val); diff --git a/compiler/rustc_const_eval/src/interpret/operand.rs b/compiler/rustc_const_eval/src/interpret/operand.rs index 55b8bf7fbb3..557cc596e16 100644 --- a/compiler/rustc_const_eval/src/interpret/operand.rs +++ b/compiler/rustc_const_eval/src/interpret/operand.rs @@ -33,7 +33,7 @@ pub enum Immediate { /// A pair of two scalar value (must have `ScalarPair` ABI where both fields are /// `Scalar::Initialized`). ScalarPair(Scalar, Scalar), - /// A value of fully uninitialized memory. Can have arbitrary size and layout. + /// A value of fully uninitialized memory. Can have arbitrary size and layout, but must be sized. Uninit, } @@ -190,16 +190,19 @@ impl<'tcx, Prov: Provenance> From> for OpTy<'tcx, Prov> { impl<'tcx, Prov: Provenance> ImmTy<'tcx, Prov> { #[inline] pub fn from_scalar(val: Scalar, layout: TyAndLayout<'tcx>) -> Self { + debug_assert!(layout.abi.is_scalar(), "`ImmTy::from_scalar` on non-scalar layout"); ImmTy { imm: val.into(), layout } } - #[inline] + #[inline(always)] pub fn from_immediate(imm: Immediate, layout: TyAndLayout<'tcx>) -> Self { + debug_assert!(layout.is_sized(), "immediates must be sized"); ImmTy { imm, layout } } #[inline] pub fn uninit(layout: TyAndLayout<'tcx>) -> Self { + debug_assert!(layout.is_sized(), "immediates must be sized"); ImmTy { imm: Immediate::Uninit, layout } } @@ -322,15 +325,12 @@ impl<'tcx, Prov: Provenance + 'static> Projectable<'tcx, Prov> for OpTy<'tcx, Pr self.layout } + #[inline] 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); - } + debug_assert!(self.layout.is_sized(), "unsized immediates are not a thing"); MemPlaceMeta::None } }) @@ -346,9 +346,10 @@ impl<'tcx, Prov: Provenance + 'static> Projectable<'tcx, Prov> for OpTy<'tcx, Pr match self.as_mplace_or_imm() { Left(mplace) => Ok(mplace.offset_with_meta(offset, meta, layout, ecx)?.into()), Right(imm) => { - assert!(!meta.has_meta()); // no place to store metadata here + debug_assert!(layout.is_sized(), "unsized immediates are not a thing"); + assert_matches!(meta, MemPlaceMeta::None); // no place to store metadata here // Every part of an uninit is uninit. - Ok(imm.offset(offset, layout, ecx)?.into()) + Ok(imm.offset_(offset, layout, ecx).into()) } } } @@ -576,6 +577,13 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { ) -> InterpResult<'tcx, OpTy<'tcx, M::Provenance>> { let layout = self.layout_of_local(frame, local, layout)?; let op = *frame.locals[local].access()?; + if matches!(op, Operand::Immediate(_)) { + if layout.is_unsized() { + // ConstProp marks *all* locals as `Immediate::Uninit` since it cannot + // efficiently check whether they are sized. We have to catch that case here. + throw_inval!(ConstPropNonsense); + } + } Ok(OpTy { op, layout, align: Some(layout.align.abi) }) } @@ -589,16 +597,15 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { match place.as_mplace_or_local() { Left(mplace) => Ok(mplace.into()), Right((frame, local, offset)) => { + debug_assert!(place.layout.is_sized()); // only sized locals can ever be `Place::Local`. let base = self.local_to_op(&self.stack()[frame], local, None)?; - let mut field = if let Some(offset) = offset { - // This got offset. We can be sure that the field is sized. - base.offset(offset, place.layout, self)? - } else { - assert_eq!(place.layout, base.layout); - // Unsized cases are possible here since an unsized local will be a - // `Place::Local` until the first projection calls `place_to_op` to extract the - // underlying mplace. - base + let mut field = match offset { + Some(offset) => base.offset(offset, place.layout, self)?, + None => { + // In the common case this hasn't been projected. + debug_assert_eq!(place.layout, base.layout); + base + } }; field.align = Some(place.align); Ok(field) diff --git a/compiler/rustc_const_eval/src/interpret/place.rs b/compiler/rustc_const_eval/src/interpret/place.rs index b73bc661946..0a731189981 100644 --- a/compiler/rustc_const_eval/src/interpret/place.rs +++ b/compiler/rustc_const_eval/src/interpret/place.rs @@ -41,6 +41,7 @@ impl MemPlaceMeta { } } + #[inline(always)] pub fn has_meta(self) -> bool { match self { Self::Meta(_) => true, @@ -255,15 +256,12 @@ impl<'tcx, Prov: Provenance + 'static> Projectable<'tcx, Prov> for PlaceTy<'tcx, self.layout } + #[inline] 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); - } + debug_assert!(self.layout.is_sized(), "unsized locals should live in memory"); MemPlaceMeta::None } }) @@ -331,7 +329,7 @@ impl<'tcx, Prov: Provenance> OpTy<'tcx, Prov> { impl<'tcx, Prov: Provenance + 'static> PlaceTy<'tcx, Prov> { /// A place is either an mplace or some local. - #[inline] + #[inline(always)] pub fn as_mplace_or_local( &self, ) -> Either, (usize, mir::Local, Option)> { @@ -535,9 +533,19 @@ where // 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), + let place = if layout.is_sized() { + // We can just always use the `Local` for sized values. + Place::Local { frame, local, offset: None } + } else { + // Unsized `Local` isn't okay (we cannot store the metadata). + match frame_ref.locals[local].access()? { + Operand::Immediate(_) => { + // ConstProp marks *all* locals as `Immediate::Uninit` since it cannot + // efficiently check whether they are sized. We have to catch that case here. + throw_inval!(ConstPropNonsense); + } + Operand::Indirect(mplace) => Place::Ptr(*mplace), + } }; Ok(PlaceTy { place, layout, align: layout.align.abi }) } @@ -896,9 +904,7 @@ where // that has different alignment than the outer field. let local_layout = self.layout_of_local(&self.stack()[frame], local, None)?; - if local_layout.is_unsized() { - throw_unsup_format!("unsized locals are not supported"); - } + assert!(local_layout.is_sized(), "unsized locals cannot be immediate"); let mplace = self.allocate(local_layout, MemoryKind::Stack)?; // Preserve old value. (As an optimization, we can skip this if it was uninit.) if !matches!(local_val, Immediate::Uninit) { diff --git a/compiler/rustc_const_eval/src/interpret/terminator.rs b/compiler/rustc_const_eval/src/interpret/terminator.rs index f3c38e363de..fd65758b064 100644 --- a/compiler/rustc_const_eval/src/interpret/terminator.rs +++ b/compiler/rustc_const_eval/src/interpret/terminator.rs @@ -1,19 +1,21 @@ use std::borrow::Cow; +use std::mem; use either::Either; use rustc_ast::ast::InlineAsmOptions; +use rustc_middle::mir::ProjectionElem; use rustc_middle::ty::layout::{FnAbiOf, LayoutOf, TyAndLayout}; use rustc_middle::ty::Instance; use rustc_middle::{ mir, ty::{self, Ty}, }; -use rustc_target::abi; use rustc_target::abi::call::{ArgAbi, ArgAttribute, ArgAttributes, FnAbi, PassMode}; +use rustc_target::abi::{self, FieldIdx}; use rustc_target::spec::abi::Abi; use super::{ - AllocId, FnVal, ImmTy, Immediate, InterpCx, InterpResult, MPlaceTy, Machine, MemoryKind, OpTy, + AllocId, FnVal, ImmTy, InterpCx, InterpResult, LocalValue, MPlaceTy, Machine, MemoryKind, OpTy, Operand, PlaceTy, Provenance, Scalar, StackPopCleanup, }; use crate::fluent_generated as fluent; @@ -358,23 +360,28 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { Item = (&'x FnArg<'tcx, M::Provenance>, &'y ArgAbi<'tcx, Ty<'tcx>>), >, callee_abi: &ArgAbi<'tcx, Ty<'tcx>>, - callee_arg: &PlaceTy<'tcx, M::Provenance>, + callee_arg: &mir::Place<'tcx>, + callee_ty: Ty<'tcx>, + already_live: bool, ) -> InterpResult<'tcx> where 'tcx: 'x, 'tcx: 'y, { if matches!(callee_abi.mode, PassMode::Ignore) { - // This one is skipped. + // This one is skipped. Still must be made live though! + if !already_live { + self.storage_live(callee_arg.as_local().unwrap())?; + } return Ok(()); } // Find next caller arg. let Some((caller_arg, caller_abi)) = caller_args.next() else { throw_ub_custom!(fluent::const_eval_not_enough_caller_args); }; - // Now, check + // Check compatibility if !Self::check_argument_compat(caller_abi, callee_abi) { - let callee_ty = format!("{}", callee_arg.layout.ty); + let callee_ty = format!("{}", callee_ty); let caller_ty = format!("{}", caller_arg.layout().ty); throw_ub_custom!( fluent::const_eval_incompatible_types, @@ -386,35 +393,37 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // will later protect the source it comes from. This means the callee cannot observe if we // did in-place of by-copy argument passing, except for pointer equality tests. let caller_arg_copy = self.copy_fn_arg(&caller_arg)?; - // Special handling for unsized parameters. - if caller_arg_copy.layout.is_unsized() { - // `check_argument_compat` ensures that both have the same type, so we know they will use the metadata the same way. - assert_eq!(caller_arg_copy.layout.ty, callee_arg.layout.ty); - // We have to properly pre-allocate the memory for the callee. - // So let's tear down some abstractions. - // This all has to be in memory, there are no immediate unsized values. - let src = caller_arg_copy.assert_mem_place(); - // The destination cannot be one of these "spread args". - let (dest_frame, dest_local, dest_offset) = callee_arg - .as_mplace_or_local() - .right() - .expect("callee fn arguments must be locals"); - // We are just initializing things, so there can't be anything here yet. - assert!(matches!( - *self.local_to_op(&self.stack()[dest_frame], dest_local, None)?, - Operand::Immediate(Immediate::Uninit) - )); - assert_eq!(dest_offset, None); - // Allocate enough memory to hold `src`. - let dest_place = self.allocate_dyn(src.layout, MemoryKind::Stack, src.meta)?; - // Update the local to be that new place. - *M::access_local_mut(self, dest_frame, dest_local)? = Operand::Indirect(*dest_place); + if !already_live { + // Special handling for unsized parameters: they are harder to make live. + if caller_arg_copy.layout.is_unsized() { + // `check_argument_compat` ensures that both have the same type, so we know they will use the metadata the same way. + assert_eq!(caller_arg_copy.layout.ty, callee_ty); + // We have to properly pre-allocate the memory for the callee. + // So let's tear down some abstractions. + // This all has to be in memory, there are no immediate unsized values. + let src = caller_arg_copy.assert_mem_place(); + // The destination cannot be one of these "spread args". + let dest_local = callee_arg.as_local().expect("unsized arguments cannot be spread"); + // Allocate enough memory to hold `src`. + let dest_place = self.allocate_dyn(src.layout, MemoryKind::Stack, src.meta)?; + // Update the local to be that new place. This is essentially a "dyn-sized StorageLive". + let old = mem::replace( + &mut self.frame_mut().locals[dest_local].value, + LocalValue::Live(Operand::Indirect(*dest_place)), + ); + assert!(matches!(old, LocalValue::Dead)); + } else { + // Just make the local live. + self.storage_live(callee_arg.as_local().unwrap())?; + } } + // Now we can finally actually evaluate the callee place. + let callee_arg = self.eval_place(*callee_arg)?; // We allow some transmutes here. // FIXME: Depending on the PassMode, this should reset some padding to uninitialized. (This // is true for all `copy_op`, but there are a lot of special cases for argument passing // specifically.) - self.copy_op(&caller_arg_copy, callee_arg, /*allow_transmute*/ true)?; + self.copy_op(&caller_arg_copy, &callee_arg, /*allow_transmute*/ true)?; // If this was an in-place pass, protect the place it comes from for the duration of the call. if let FnArg::InPlace(place) = caller_arg { M::protect_in_place_function_argument(self, place)?; @@ -600,18 +609,47 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // not advance `caller_iter` for ZSTs. let mut callee_args_abis = callee_fn_abi.args.iter(); for local in body.args_iter() { - let dest = self.eval_place(mir::Place::from(local))?; + // Construct the destination place for this argument. At this point all + // locals are still dead, so we cannot construct a `PlaceTy`. + let dest = mir::Place::from(local); + // `layout_of_local` does more than just the substitution we need to get the + // type, but the result gets cached so this avoids calling the substitution + // query *again* the next time this local is accessed. + let ty = self.layout_of_local(self.frame(), local, None)?.ty; if Some(local) == body.spread_arg { + // Make the local live once, then fill in the value field by field. + self.storage_live(local)?; // Must be a tuple - for i in 0..dest.layout.fields.count() { - let dest = self.project_field(&dest, i)?; + let ty::Tuple(fields) = ty.kind() else { + span_bug!( + self.cur_span(), + "non-tuple type for `spread_arg`: {ty:?}" + ) + }; + for (i, field_ty) in fields.iter().enumerate() { + let dest = dest.project_deeper( + &[ProjectionElem::Field(FieldIdx::from_usize(i), field_ty)], + *self.tcx, + ); let callee_abi = callee_args_abis.next().unwrap(); - self.pass_argument(&mut caller_args, callee_abi, &dest)?; + self.pass_argument( + &mut caller_args, + callee_abi, + &dest, + field_ty, + /* already_live */ true, + )?; } } else { - // Normal argument + // Normal argument. Cannot mark it as live yet, it might be unsized! let callee_abi = callee_args_abis.next().unwrap(); - self.pass_argument(&mut caller_args, callee_abi, &dest)?; + self.pass_argument( + &mut caller_args, + callee_abi, + &dest, + ty, + /* already_live */ false, + )?; } } // If the callee needs a caller location, pretend we consume one more argument from the ABI. @@ -644,6 +682,9 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // Nothing to do for locals, they are always properly allocated and aligned. } M::protect_in_place_function_argument(self, destination)?; + + // Don't forget to mark "initially live" locals as live. + self.storage_live_for_always_live_locals()?; }; match res { Err(err) => { diff --git a/compiler/rustc_middle/src/mir/interpret/error.rs b/compiler/rustc_middle/src/mir/interpret/error.rs index e6ef5a41ee0..577cd20b74c 100644 --- a/compiler/rustc_middle/src/mir/interpret/error.rs +++ b/compiler/rustc_middle/src/mir/interpret/error.rs @@ -415,6 +415,8 @@ pub enum UnsupportedOpInfo { /// Free-form case. Only for errors that are never caught! // FIXME still use translatable diagnostics Unsupported(String), + /// Unsized local variables. + UnsizedLocal, // // The variants below are only reachable from CTFE/const prop, miri will never emit them. // diff --git a/compiler/rustc_mir_transform/src/const_prop.rs b/compiler/rustc_mir_transform/src/const_prop.rs index a793b384d81..0aaa3f75c82 100644 --- a/compiler/rustc_mir_transform/src/const_prop.rs +++ b/compiler/rustc_mir_transform/src/const_prop.rs @@ -376,6 +376,16 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { ) .expect("failed to push initial stack frame"); + for local in body.local_decls.indices() { + // Mark everything initially live. + // This is somewhat dicey since some of them might be unsized and it is incoherent to + // mark those as live... We rely on `local_to_place`/`local_to_op` in the interpreter + // stopping us before those unsized immediates can cause issues deeper in the + // interpreter. + ecx.frame_mut().locals[local].value = + LocalValue::Live(interpret::Operand::Immediate(Immediate::Uninit)); + } + ConstPropagator { ecx, tcx, param_env, local_decls: &dummy_body.local_decls } } diff --git a/compiler/rustc_mir_transform/src/const_prop_lint.rs b/compiler/rustc_mir_transform/src/const_prop_lint.rs index 755b3985791..dec79ddf58c 100644 --- a/compiler/rustc_mir_transform/src/const_prop_lint.rs +++ b/compiler/rustc_mir_transform/src/const_prop_lint.rs @@ -206,6 +206,16 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { ) .expect("failed to push initial stack frame"); + for local in body.local_decls.indices() { + // Mark everything initially live. + // This is somewhat dicey since some of them might be unsized and it is incoherent to + // mark those as live... We rely on `local_to_place`/`local_to_op` in the interpreter + // stopping us before those unsized immediates can cause issues deeper in the + // interpreter. + ecx.frame_mut().locals[local].value = + LocalValue::Live(interpret::Operand::Immediate(Immediate::Uninit)); + } + ConstPropagator { ecx, tcx, diff --git a/src/tools/miri/src/diagnostics.rs b/src/tools/miri/src/diagnostics.rs index 799a9a8e4be..75c6911bcbe 100644 --- a/src/tools/miri/src/diagnostics.rs +++ b/src/tools/miri/src/diagnostics.rs @@ -283,7 +283,7 @@ pub fn report_error<'tcx, 'mir>( "resource exhaustion", Unsupported( // We list only the ones that can actually happen. - UnsupportedOpInfo::Unsupported(_) + UnsupportedOpInfo::Unsupported(_) | UnsupportedOpInfo::UnsizedLocal ) => "unsupported operation", InvalidProgram( diff --git a/src/tools/miri/src/helpers.rs b/src/tools/miri/src/helpers.rs index b5cc5c7e486..844827889a7 100644 --- a/src/tools/miri/src/helpers.rs +++ b/src/tools/miri/src/helpers.rs @@ -14,7 +14,7 @@ use rustc_middle::mir; use rustc_middle::ty::{ self, layout::{IntegerExt as _, LayoutOf, TyAndLayout}, - List, Ty, TyCtxt, + Ty, TyCtxt, }; use rustc_span::{def_id::CrateNum, sym, Span, Symbol}; use rustc_target::abi::{Align, FieldIdx, FieldsShape, Integer, Size, Variants}; @@ -282,13 +282,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { Ok(ptr.addr().bytes() == 0) } - /// Get the `Place` for a local - 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) - } - /// Generate some random bytes, and write them to `dest`. fn gen_random(&mut self, ptr: Pointer>, len: u64) -> InterpResult<'tcx> { // Some programs pass in a null pointer and a length of 0 @@ -350,17 +343,21 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { // Initialize arguments. let mut callee_args = this.frame().body.args_iter(); for arg in args { - let callee_arg = this.local_place( - callee_args - .next() - .ok_or_else(|| err_ub_format!("callee has fewer arguments than expected"))?, - )?; + let local = callee_args + .next() + .ok_or_else(|| err_ub_format!("callee has fewer arguments than expected"))?; + // Make the local live, and insert the initial value. + this.storage_live(local)?; + let callee_arg = this.local_to_place(this.frame_idx(), local)?; this.write_immediate(*arg, &callee_arg)?; } if callee_args.next().is_some() { throw_ub_format!("callee has more arguments than expected"); } + // Initialize remaining locals. + this.storage_live_for_always_live_locals()?; + Ok(()) } diff --git a/src/tools/miri/tests/fail/type-too-large.rs b/src/tools/miri/tests/fail/type-too-large.rs index 21b272f8ec3..2e23b79d5f8 100644 --- a/src/tools/miri/tests/fail/type-too-large.rs +++ b/src/tools/miri/tests/fail/type-too-large.rs @@ -1,6 +1,6 @@ //@ignore-32bit fn main() { - let _fat: [u8; (1 << 61) + (1 << 31)]; - _fat = [0; (1u64 << 61) as usize + (1u64 << 31) as usize]; //~ ERROR: post-monomorphization error + let _fat: [u8; (1 << 61) + (1 << 31)]; //~ ERROR: post-monomorphization error + _fat = [0; (1u64 << 61) as usize + (1u64 << 31) as usize]; } diff --git a/src/tools/miri/tests/fail/type-too-large.stderr b/src/tools/miri/tests/fail/type-too-large.stderr index cb1d725ec87..70891fcc22b 100644 --- a/src/tools/miri/tests/fail/type-too-large.stderr +++ b/src/tools/miri/tests/fail/type-too-large.stderr @@ -1,8 +1,8 @@ error: post-monomorphization error: values of the type `[u8; 2305843011361177600]` are too big for the current architecture --> $DIR/type-too-large.rs:LL:CC | -LL | _fat = [0; (1u64 << 61) as usize + (1u64 << 31) as usize]; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ values of the type `[u8; 2305843011361177600]` are too big for the current architecture +LL | let _fat: [u8; (1 << 61) + (1 << 31)]; + | ^^^^ values of the type `[u8; 2305843011361177600]` are too big for the current architecture | = note: inside `main` at $DIR/type-too-large.rs:LL:CC diff --git a/src/tools/miri/tests/fail/unsized-local.stderr b/src/tools/miri/tests/fail/unsized-local.stderr index 66d93c6f503..07f94f3b91b 100644 --- a/src/tools/miri/tests/fail/unsized-local.stderr +++ b/src/tools/miri/tests/fail/unsized-local.stderr @@ -2,7 +2,7 @@ error: unsupported operation: unsized locals are not supported --> $DIR/unsized-local.rs:LL:CC | LL | let x = *(Box::new(A) as Box); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ unsized locals are not supported + | ^ unsized locals are not supported | = help: this is likely not a bug in the program; it indicates that the program performed an operation that the interpreter does not support = note: BACKTRACE: From 7cdeff266c9b95f509588d89ffb5558b12c2dd94 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 17 Aug 2023 18:58:56 +0200 Subject: [PATCH 3/5] a bit of meta-related cleanup on Projectable --- .../rustc_const_eval/src/interpret/operand.rs | 10 +++--- .../rustc_const_eval/src/interpret/place.rs | 33 ++++--------------- .../src/interpret/projection.rs | 25 +++++++++++--- 3 files changed, 32 insertions(+), 36 deletions(-) diff --git a/compiler/rustc_const_eval/src/interpret/operand.rs b/compiler/rustc_const_eval/src/interpret/operand.rs index 557cc596e16..8d948eb10a7 100644 --- a/compiler/rustc_const_eval/src/interpret/operand.rs +++ b/compiler/rustc_const_eval/src/interpret/operand.rs @@ -295,9 +295,9 @@ impl<'tcx, Prov: Provenance> Projectable<'tcx, Prov> for ImmTy<'tcx, Prov> { } #[inline(always)] - fn meta(&self) -> InterpResult<'tcx, MemPlaceMeta> { + fn meta(&self) -> MemPlaceMeta { debug_assert!(self.layout.is_sized()); // unsized ImmTy can only exist temporarily and should never reach this here - Ok(MemPlaceMeta::None) + MemPlaceMeta::None } fn offset_with_meta<'mir, M: Machine<'mir, 'tcx, Provenance = Prov>>( @@ -326,14 +326,14 @@ impl<'tcx, Prov: Provenance + 'static> Projectable<'tcx, Prov> for OpTy<'tcx, Pr } #[inline] - fn meta(&self) -> InterpResult<'tcx, MemPlaceMeta> { - Ok(match self.as_mplace_or_imm() { + fn meta(&self) -> MemPlaceMeta { + match self.as_mplace_or_imm() { Left(mplace) => mplace.meta, Right(_) => { debug_assert!(self.layout.is_sized(), "unsized immediates are not a thing"); MemPlaceMeta::None } - }) + } } fn offset_with_meta<'mir, M: Machine<'mir, 'tcx, Provenance = Prov>>( diff --git a/compiler/rustc_const_eval/src/interpret/place.rs b/compiler/rustc_const_eval/src/interpret/place.rs index 0a731189981..65c5cd656cb 100644 --- a/compiler/rustc_const_eval/src/interpret/place.rs +++ b/compiler/rustc_const_eval/src/interpret/place.rs @@ -13,7 +13,7 @@ use rustc_middle::mir::interpret::PointerArithmetic; use rustc_middle::ty; use rustc_middle::ty::layout::{LayoutOf, TyAndLayout}; use rustc_middle::ty::Ty; -use rustc_target::abi::{self, Abi, Align, FieldIdx, HasDataLayout, Size, FIRST_VARIANT}; +use rustc_target::abi::{Abi, Align, FieldIdx, HasDataLayout, Size, FIRST_VARIANT}; use super::{ alloc_range, mir_assign_valid_types, AllocId, AllocRef, AllocRefMut, CheckInAllocMsg, @@ -48,27 +48,6 @@ impl MemPlaceMeta { Self::None => false, } } - - pub(crate) fn len<'tcx>( - &self, - layout: TyAndLayout<'tcx>, - cx: &impl HasDataLayout, - ) -> InterpResult<'tcx, u64> { - if layout.is_unsized() { - // We need to consult `meta` metadata - match layout.ty.kind() { - ty::Slice(..) | ty::Str => self.unwrap_meta().to_target_usize(cx), - _ => bug!("len not supported on unsized type {:?}", layout.ty), - } - } else { - // Go through the layout. There are lots of types that support a length, - // e.g., SIMD types. (But not all repr(simd) types even have FieldsShape::Array!) - match layout.fields { - abi::FieldsShape::Array { count, .. } => Ok(count), - _ => bug!("len not supported on sized type {:?}", layout.ty), - } - } - } } #[derive(Copy, Clone, Hash, PartialEq, Eq, Debug)] @@ -224,8 +203,8 @@ impl<'tcx, Prov: Provenance + 'static> Projectable<'tcx, Prov> for MPlaceTy<'tcx } #[inline(always)] - fn meta(&self) -> InterpResult<'tcx, MemPlaceMeta> { - Ok(self.meta) + fn meta(&self) -> MemPlaceMeta { + self.meta } fn offset_with_meta<'mir, M: Machine<'mir, 'tcx, Provenance = Prov>>( @@ -257,14 +236,14 @@ impl<'tcx, Prov: Provenance + 'static> Projectable<'tcx, Prov> for PlaceTy<'tcx, } #[inline] - fn meta(&self) -> InterpResult<'tcx, MemPlaceMeta> { - Ok(match self.as_mplace_or_local() { + fn meta(&self) -> MemPlaceMeta { + match self.as_mplace_or_local() { Left(mplace) => mplace.meta, Right(_) => { debug_assert!(self.layout.is_sized(), "unsized locals should live in memory"); MemPlaceMeta::None } - }) + } } fn offset_with_meta<'mir, M: Machine<'mir, 'tcx, Provenance = Prov>>( diff --git a/compiler/rustc_const_eval/src/interpret/projection.rs b/compiler/rustc_const_eval/src/interpret/projection.rs index ecf594773fe..c69321d109e 100644 --- a/compiler/rustc_const_eval/src/interpret/projection.rs +++ b/compiler/rustc_const_eval/src/interpret/projection.rs @@ -25,13 +25,28 @@ pub trait Projectable<'tcx, Prov: Provenance>: Sized + std::fmt::Debug { fn layout(&self) -> TyAndLayout<'tcx>; /// Get the metadata of a wide value. - fn meta(&self) -> InterpResult<'tcx, MemPlaceMeta>; + fn meta(&self) -> MemPlaceMeta; + /// Get the length of a slice/string/array stored here. fn len<'mir, M: Machine<'mir, 'tcx, Provenance = Prov>>( &self, ecx: &InterpCx<'mir, 'tcx, M>, ) -> InterpResult<'tcx, u64> { - self.meta()?.len(self.layout(), ecx) + let layout = self.layout(); + if layout.is_unsized() { + // We need to consult `meta` metadata + match layout.ty.kind() { + ty::Slice(..) | ty::Str => self.meta().unwrap_meta().to_target_usize(ecx), + _ => bug!("len not supported on unsized type {:?}", layout.ty), + } + } else { + // Go through the layout. There are lots of types that support a length, + // e.g., SIMD types. (But not all repr(simd) types even have FieldsShape::Array!) + match layout.fields { + abi::FieldsShape::Array { count, .. } => Ok(count), + _ => bug!("len not supported on sized type {:?}", layout.ty), + } + } } /// Offset the value by the given amount, replacing the layout and metadata. @@ -43,6 +58,7 @@ pub trait Projectable<'tcx, Prov: Provenance>: Sized + std::fmt::Debug { ecx: &InterpCx<'mir, 'tcx, M>, ) -> InterpResult<'tcx, Self>; + #[inline] fn offset<'mir, M: Machine<'mir, 'tcx, Provenance = Prov>>( &self, offset: Size, @@ -53,6 +69,7 @@ pub trait Projectable<'tcx, Prov: Provenance>: Sized + std::fmt::Debug { self.offset_with_meta(offset, MemPlaceMeta::None, layout, ecx) } + #[inline] fn transmute<'mir, M: Machine<'mir, 'tcx, Provenance = Prov>>( &self, layout: TyAndLayout<'tcx>, @@ -125,7 +142,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()?; + 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. @@ -153,7 +170,7 @@ where base: &P, variant: VariantIdx, ) -> InterpResult<'tcx, P> { - assert!(!base.meta()?.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.) From f87e91de7db20e1c3804f7d90c487bd83ef80d76 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 17 Aug 2023 18:59:39 +0200 Subject: [PATCH 4/5] unify passing of sized and unsized function arguments :-) --- .../src/interpret/eval_context.rs | 34 +++++++++++++++---- .../src/interpret/terminator.rs | 34 +++++-------------- 2 files changed, 36 insertions(+), 32 deletions(-) diff --git a/compiler/rustc_const_eval/src/interpret/eval_context.rs b/compiler/rustc_const_eval/src/interpret/eval_context.rs index 58674a1eec5..babc5790f9e 100644 --- a/compiler/rustc_const_eval/src/interpret/eval_context.rs +++ b/compiler/rustc_const_eval/src/interpret/eval_context.rs @@ -912,6 +912,32 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { Ok(()) } + pub fn storage_live_dyn( + &mut self, + local: mir::Local, + meta: MemPlaceMeta, + ) -> InterpResult<'tcx> { + trace!("{:?} is now live", local); + + let layout = self.layout_of_local(self.frame(), local, None)?; + let local_val = LocalValue::Live(if layout.is_sized() { + assert!(matches!(meta, MemPlaceMeta::None)); // we're dropping the metadata + // Just make this an efficient immediate. + Operand::Immediate(Immediate::Uninit) + } else { + // Need to allocate some memory. + let dest_place = self.allocate_dyn(layout, MemoryKind::Stack, meta)?; + Operand::Indirect(*dest_place) + }); + + // StorageLive expects the local to be dead, and marks it live. + let old = mem::replace(&mut self.frame_mut().locals[local].value, local_val); + if !matches!(old, LocalValue::Dead) { + throw_ub_custom!(fluent::const_eval_double_storage_live); + } + Ok(()) + } + /// Mark a storage as live, killing the previous content. pub fn storage_live(&mut self, local: mir::Local) -> InterpResult<'tcx> { trace!("{:?} is now live", local); @@ -920,13 +946,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { throw_unsup!(UnsizedLocal); } - let local_val = LocalValue::Live(Operand::Immediate(Immediate::Uninit)); - // StorageLive expects the local to be dead, and marks it live. - let old = mem::replace(&mut self.frame_mut().locals[local].value, local_val); - if !matches!(old, LocalValue::Dead) { - throw_ub_custom!(fluent::const_eval_double_storage_live); - } - Ok(()) + self.storage_live_dyn(local, MemPlaceMeta::None) } pub fn storage_dead(&mut self, local: mir::Local) -> InterpResult<'tcx> { diff --git a/compiler/rustc_const_eval/src/interpret/terminator.rs b/compiler/rustc_const_eval/src/interpret/terminator.rs index fd65758b064..279a06d3868 100644 --- a/compiler/rustc_const_eval/src/interpret/terminator.rs +++ b/compiler/rustc_const_eval/src/interpret/terminator.rs @@ -1,5 +1,4 @@ use std::borrow::Cow; -use std::mem; use either::Either; use rustc_ast::ast::InlineAsmOptions; @@ -15,8 +14,8 @@ use rustc_target::abi::{self, FieldIdx}; use rustc_target::spec::abi::Abi; use super::{ - AllocId, FnVal, ImmTy, InterpCx, InterpResult, LocalValue, MPlaceTy, Machine, MemoryKind, OpTy, - Operand, PlaceTy, Provenance, Scalar, StackPopCleanup, + AllocId, FnVal, ImmTy, InterpCx, InterpResult, MPlaceTy, Machine, OpTy, PlaceTy, Projectable, + Provenance, Scalar, StackPopCleanup, }; use crate::fluent_generated as fluent; @@ -394,28 +393,13 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // did in-place of by-copy argument passing, except for pointer equality tests. let caller_arg_copy = self.copy_fn_arg(&caller_arg)?; if !already_live { - // Special handling for unsized parameters: they are harder to make live. - if caller_arg_copy.layout.is_unsized() { - // `check_argument_compat` ensures that both have the same type, so we know they will use the metadata the same way. - assert_eq!(caller_arg_copy.layout.ty, callee_ty); - // We have to properly pre-allocate the memory for the callee. - // So let's tear down some abstractions. - // This all has to be in memory, there are no immediate unsized values. - let src = caller_arg_copy.assert_mem_place(); - // The destination cannot be one of these "spread args". - let dest_local = callee_arg.as_local().expect("unsized arguments cannot be spread"); - // Allocate enough memory to hold `src`. - let dest_place = self.allocate_dyn(src.layout, MemoryKind::Stack, src.meta)?; - // Update the local to be that new place. This is essentially a "dyn-sized StorageLive". - let old = mem::replace( - &mut self.frame_mut().locals[dest_local].value, - LocalValue::Live(Operand::Indirect(*dest_place)), - ); - assert!(matches!(old, LocalValue::Dead)); - } else { - // Just make the local live. - self.storage_live(callee_arg.as_local().unwrap())?; - } + let local = callee_arg.as_local().unwrap(); + let meta = caller_arg_copy.meta(); + // `check_argument_compat` ensures that if metadata is needed, both have the same type, + // so we know they will use the metadata the same way. + assert!(!meta.has_meta() || caller_arg_copy.layout.ty == callee_ty); + + self.storage_live_dyn(local, meta)?; } // Now we can finally actually evaluate the callee place. let callee_arg = self.eval_place(*callee_arg)?; From 6d1ce9bd131d16166000f232cca00bc446bc9e35 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 7 Aug 2023 08:18:50 +0200 Subject: [PATCH 5/5] storage_live: avoid computing the layout unless necessary --- .../src/interpret/eval_context.rs | 79 +++++++++++++++---- src/tools/miri/tests/fail/type-too-large.rs | 4 +- .../miri/tests/fail/type-too-large.stderr | 4 +- 3 files changed, 69 insertions(+), 18 deletions(-) diff --git a/compiler/rustc_const_eval/src/interpret/eval_context.rs b/compiler/rustc_const_eval/src/interpret/eval_context.rs index babc5790f9e..1c90ce29b02 100644 --- a/compiler/rustc_const_eval/src/interpret/eval_context.rs +++ b/compiler/rustc_const_eval/src/interpret/eval_context.rs @@ -159,8 +159,7 @@ pub enum StackPopCleanup { pub struct LocalState<'tcx, Prov: Provenance = AllocId> { pub value: LocalValue, /// Don't modify if `Some`, this is only used to prevent computing the layout twice. - /// Layout needs to be computed lazily because ConstProp wants to run on frames where we can't - /// compute the layout of all locals. + /// Avoids computing the layout of locals that are never actually initialized. pub layout: Cell>>, } @@ -919,15 +918,72 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { ) -> InterpResult<'tcx> { trace!("{:?} is now live", local); - let layout = self.layout_of_local(self.frame(), local, None)?; - let local_val = LocalValue::Live(if layout.is_sized() { - assert!(matches!(meta, MemPlaceMeta::None)); // we're dropping the metadata - // Just make this an efficient immediate. - Operand::Immediate(Immediate::Uninit) + // We avoid `ty.is_trivially_sized` since that (a) cannot assume WF, so it recurses through + // all fields of a tuple, and (b) does something expensive for ADTs. + fn is_very_trivially_sized(ty: Ty<'_>) -> bool { + match ty.kind() { + ty::Infer(ty::IntVar(_) | ty::FloatVar(_)) + | ty::Uint(_) + | ty::Int(_) + | ty::Bool + | ty::Float(_) + | ty::FnDef(..) + | ty::FnPtr(_) + | ty::RawPtr(..) + | ty::Char + | ty::Ref(..) + | ty::Generator(..) + | ty::GeneratorWitness(..) + | ty::GeneratorWitnessMIR(..) + | ty::Array(..) + | ty::Closure(..) + | ty::Never + | ty::Error(_) => true, + + ty::Str | ty::Slice(_) | ty::Dynamic(..) | ty::Foreign(..) => false, + + ty::Tuple(tys) => tys.last().iter().all(|ty| is_very_trivially_sized(**ty)), + + // We don't want to do any queries, so there is not much we can do with ADTs. + ty::Adt(..) => false, + + ty::Alias(..) | ty::Param(_) | ty::Placeholder(..) => false, + + ty::Infer(ty::TyVar(_)) => false, + + ty::Bound(..) + | ty::Infer(ty::FreshTy(_) | ty::FreshIntTy(_) | ty::FreshFloatTy(_)) => { + bug!("`is_very_trivially_sized` applied to unexpected type: {:?}", ty) + } + } + } + + // This is a hot function, we avoid computing the layout when possible. + // `unsized_` will be `None` for sized types and `Some(layout)` for unsized types. + let unsized_ = if is_very_trivially_sized(self.body().local_decls[local].ty) { + None } else { - // Need to allocate some memory. + // We need the layout. + let layout = self.layout_of_local(self.frame(), local, None)?; + if layout.is_sized() { None } else { Some(layout) } + }; + + let local_val = LocalValue::Live(if let Some(layout) = unsized_ { + if !meta.has_meta() { + throw_unsup!(UnsizedLocal); + } + // Need to allocate some memory, since `Immediate::Uninit` cannot be unsized. let dest_place = self.allocate_dyn(layout, MemoryKind::Stack, meta)?; Operand::Indirect(*dest_place) + } else { + assert!(!meta.has_meta()); // we're dropping the metadata + // Just make this an efficient immediate. + // Note that not calling `layout_of` here does have one real consequence: + // if the type is too big, we'll only notice this when the local is actually initialized, + // which is a bit too late -- we should ideally notice this alreayd here, when the memory + // is conceptually allocated. But given how rare that error is and that this is a hot function, + // we accept this downside for now. + Operand::Immediate(Immediate::Uninit) }); // StorageLive expects the local to be dead, and marks it live. @@ -939,13 +995,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } /// Mark a storage as live, killing the previous content. + #[inline(always)] pub fn storage_live(&mut self, local: mir::Local) -> InterpResult<'tcx> { - trace!("{:?} is now live", local); - - if self.layout_of_local(self.frame(), local, None)?.is_unsized() { - throw_unsup!(UnsizedLocal); - } - self.storage_live_dyn(local, MemPlaceMeta::None) } diff --git a/src/tools/miri/tests/fail/type-too-large.rs b/src/tools/miri/tests/fail/type-too-large.rs index 2e23b79d5f8..81ecc6145d7 100644 --- a/src/tools/miri/tests/fail/type-too-large.rs +++ b/src/tools/miri/tests/fail/type-too-large.rs @@ -1,6 +1,6 @@ //@ignore-32bit fn main() { - let _fat: [u8; (1 << 61) + (1 << 31)]; //~ ERROR: post-monomorphization error - _fat = [0; (1u64 << 61) as usize + (1u64 << 31) as usize]; + let _fat: [u8; (1 << 61) + (1 << 31)]; // ideally we'd error here, but we avoid computing the layout until absolutely necessary + _fat = [0; (1u64 << 61) as usize + (1u64 << 31) as usize]; //~ ERROR: post-monomorphization error } diff --git a/src/tools/miri/tests/fail/type-too-large.stderr b/src/tools/miri/tests/fail/type-too-large.stderr index 70891fcc22b..cb1d725ec87 100644 --- a/src/tools/miri/tests/fail/type-too-large.stderr +++ b/src/tools/miri/tests/fail/type-too-large.stderr @@ -1,8 +1,8 @@ error: post-monomorphization error: values of the type `[u8; 2305843011361177600]` are too big for the current architecture --> $DIR/type-too-large.rs:LL:CC | -LL | let _fat: [u8; (1 << 61) + (1 << 31)]; - | ^^^^ values of the type `[u8; 2305843011361177600]` are too big for the current architecture +LL | _fat = [0; (1u64 << 61) as usize + (1u64 << 31) as usize]; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ values of the type `[u8; 2305843011361177600]` are too big for the current architecture | = note: inside `main` at $DIR/type-too-large.rs:LL:CC