interpret: remove LocalValue::Unallocated, add Operand::Uninit

Operand::Uninit is an *allocated* operand that is fully uninitialized.
This lets us lazily allocate the actual backing store of *all* locals (no matter their ABI).

I also reordered things in pop_stack_frame at the same time.
I should probably have made that a separate commit...

Author: RalfJung

compiler/rustc_const_eval/src/const_eval/eval_queries.rs

@@ -189,6 +189,7 @@ pub(super) fn op_to_const<'tcx>(
                 let len: usize = len.try_into().unwrap();
                 ConstValue::Slice { data, start, end: start + len }
             }
+            Immediate::Uninit => to_const_value(&op.assert_mem_place()),
         },
     }
 }

compiler/rustc_const_eval/src/interpret/cast.rs

@@ -153,14 +153,15 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                 assert_eq!(dest_layout.size, self.pointer_size());
                 assert!(src.layout.ty.is_unsafe_ptr());
                 return match **src {
-                    Immediate::ScalarPair(data, _) => Ok(data.into()),
+                    Immediate::ScalarPair(data, _) => Ok(data.check_init()?.into()),
                     Immediate::Scalar(..) => span_bug!(
                         self.cur_span(),
                         "{:?} input to a fat-to-thin cast ({:?} -> {:?})",
                         *src,
                         src.layout.ty,
                         cast_ty
                     ),
+                    Immediate::Uninit => throw_ub!(InvalidUninitBytes(None)),
                 };
             }
         }

compiler/rustc_const_eval/src/interpret/eval_context.rs

@@ -112,6 +112,8 @@ pub struct Frame<'mir, 'tcx, Tag: Provenance = AllocId, Extra = ()> {
     /// The locals are stored as `Option<Value>`s.
     /// `None` represents a local that is currently dead, while a live local
     /// can either directly contain `Scalar` or refer to some part of an `Allocation`.
+    ///
+    /// Do *not* access this directly; always go through the machine hook!
     pub locals: IndexVec<mir::Local, LocalState<'tcx, Tag>>,
 
     /// The span of the `tracing` crate is stored here.
@@ -179,10 +181,6 @@ pub struct LocalState<'tcx, Tag: Provenance = AllocId> {
 pub enum LocalValue<Tag: Provenance = AllocId> {
     /// This local is not currently alive, and cannot be used at all.
     Dead,
-    /// This local is alive but not yet allocated. It cannot be read from or have its address taken,
-    /// and will be allocated on the first write. This is to support unsized locals, where we cannot
-    /// know their size in advance.
-    Unallocated,
     /// A normal, live local.
     /// Mostly for convenience, we re-use the `Operand` type here.
     /// This is an optimization over just always having a pointer here;
@@ -196,12 +194,10 @@ impl<'tcx, Tag: Provenance + 'static> LocalState<'tcx, Tag> {
     ///
     /// Note: This may only be invoked from the `Machine::access_local` hook and not from
     /// anywhere else. You may be invalidating machine invariants if you do!
-    pub fn access(&self) -> InterpResult<'tcx, Operand<Tag>> {
-        match self.value {
-            LocalValue::Dead => throw_ub!(DeadLocal),
-            LocalValue::Unallocated => {
-                bug!("The type checker should prevent reading from a never-written local")
-            }
+    #[inline]
+    pub fn access(&self) -> InterpResult<'tcx, &Operand<Tag>> {
+        match &self.value {
+            LocalValue::Dead => throw_ub!(DeadLocal), // could even be "invalid program"?
             LocalValue::Live(val) => Ok(val),
         }
     }
@@ -211,15 +207,11 @@ impl<'tcx, Tag: Provenance + 'static> LocalState<'tcx, Tag> {
     ///
     /// 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!
-    pub fn access_mut(
-        &mut self,
-    ) -> InterpResult<'tcx, Result<&mut LocalValue<Tag>, MemPlace<Tag>>> {
-        match self.value {
-            LocalValue::Dead => throw_ub!(DeadLocal),
-            LocalValue::Live(Operand::Indirect(mplace)) => Ok(Err(mplace)),
-            ref mut local @ (LocalValue::Live(Operand::Immediate(_)) | LocalValue::Unallocated) => {
-                Ok(Ok(local))
-            }
+    #[inline]
+    pub fn access_mut(&mut self) -> InterpResult<'tcx, &mut Operand<Tag>> {
+        match &mut self.value {
+            LocalValue::Dead => throw_ub!(DeadLocal), // could even be "invalid program"?
+            LocalValue::Live(val) => Ok(val),
         }
     }
 }
@@ -710,16 +702,15 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
             })?;
         }
 
-        // Locals are initially unallocated.
-        let dummy = LocalState { value: LocalValue::Unallocated, layout: Cell::new(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 dead that we do not want to initialize
-        // Mark locals that use `Storage*` annotations as dead on function entry.
+        // Now mark those locals as live that have no `Storage*` annotations.
         let always_live = always_live_locals(self.body());
         for local in locals.indices() {
-            if !always_live.contains(local) {
-                locals[local].value = LocalValue::Dead;
+            if always_live.contains(local) {
+                locals[local].value = LocalValue::Live(Operand::Immediate(Immediate::Uninit));
             }
         }
         // done
@@ -791,59 +782,69 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
             if unwinding { "during unwinding" } else { "returning from function" }
         );
 
-        // Sanity check `unwinding`.
+        // Check `unwinding`.
         assert_eq!(
             unwinding,
             match self.frame().loc {
                 Ok(loc) => self.body().basic_blocks()[loc.block].is_cleanup,
                 Err(_) => true,
             }
         );
-
         if unwinding && self.frame_idx() == 0 {
             throw_ub_format!("unwinding past the topmost frame of the stack");
         }
 
-        let frame =
-            self.stack_mut().pop().expect("tried to pop a stack frame, but there were none");
-
-        if !unwinding {
-            let op = self.local_to_op(&frame, mir::RETURN_PLACE, None)?;
-            self.copy_op_transmute(&op, &frame.return_place)?;
-            trace!("{:?}", self.dump_place(*frame.return_place));
-        }
-
-        let return_to_block = frame.return_to_block;
-
-        // Now where do we jump next?
+        // Copy return value. Must of course happen *before* we deallocate the locals.
+        let copy_ret_result = if !unwinding {
+            let op = self
+                .local_to_op(self.frame(), mir::RETURN_PLACE, None)
+                .expect("return place should always be live");
+            let dest = self.frame().return_place;
+            let err = self.copy_op_transmute(&op, &dest);
+            trace!("return value: {:?}", self.dump_place(*dest));
+            // We delay actually short-circuiting on this error until *after* the stack frame is
+            // popped, since we want this error to be attributed to the caller, whose type defines
+            // this transmute.
+            err
+        } else {
+            Ok(())
+        };
 
+        // Cleanup: deallocate locals.
         // Usually we want to clean up (deallocate locals), but in a few rare cases we don't.
-        // In that case, we return early. We also avoid validation in that case,
-        // because this is CTFE and the final value will be thoroughly validated anyway.
+        // We do this while the frame is still on the stack, so errors point to the callee.
+        let return_to_block = self.frame().return_to_block;
         let cleanup = match return_to_block {
             StackPopCleanup::Goto { .. } => true,
             StackPopCleanup::Root { cleanup, .. } => cleanup,
         };
+        if cleanup {
+            // We need to take the locals out, since we need to mutate while iterating.
+            let locals = mem::take(&mut self.frame_mut().locals);
+            for local in &locals {
+                self.deallocate_local(local.value)?;
+            }
+        }
+
+        // All right, now it is time to actually pop the frame.
+        // Note that its locals are gone already, but that's fine.
+        let frame =
+            self.stack_mut().pop().expect("tried to pop a stack frame, but there were none");
+        // Report error from return value copy, if any.
+        copy_ret_result?;
 
+        // If we are not doing cleanup, also skip everything else.
         if !cleanup {
             assert!(self.stack().is_empty(), "only the topmost frame should ever be leaked");
             assert!(!unwinding, "tried to skip cleanup during unwinding");
-            // Leak the locals, skip validation, skip machine hook.
+            // Skip machine hook.
             return Ok(());
         }
-
-        trace!("locals: {:#?}", frame.locals);
-
-        // Cleanup: deallocate all locals that are backed by an allocation.
-        for local in &frame.locals {
-            self.deallocate_local(local.value)?;
-        }
-
         if M::after_stack_pop(self, frame, unwinding)? == StackPopJump::NoJump {
             // The hook already did everything.
-            // We want to skip the `info!` below, hence early return.
             return Ok(());
         }
+
         // Normal return, figure out where to jump.
         if unwinding {
             // Follow the unwind edge.
@@ -874,7 +875,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         assert!(local != mir::RETURN_PLACE, "Cannot make return place live");
         trace!("{:?} is now live", local);
 
-        let local_val = LocalValue::Unallocated;
+        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) {
@@ -977,7 +978,9 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> std::fmt::Debug
 
                 match self.ecx.stack()[frame].locals[local].value {
                     LocalValue::Dead => write!(fmt, " is dead")?,
-                    LocalValue::Unallocated => write!(fmt, " is unallocated")?,
+                    LocalValue::Live(Operand::Immediate(Immediate::Uninit)) => {
+                        write!(fmt, " is uninitialized")?
+                    }
                     LocalValue::Live(Operand::Indirect(mplace)) => {
                         write!(
                             fmt,

compiler/rustc_const_eval/src/interpret/machine.rs

@@ -14,8 +14,7 @@ use rustc_target::spec::abi::Abi;
 
 use super::{
     AllocId, AllocRange, Allocation, ConstAllocation, Frame, ImmTy, InterpCx, InterpResult,
-    LocalValue, MemPlace, MemoryKind, OpTy, Operand, PlaceTy, Pointer, Provenance, Scalar,
-    StackPopUnwind,
+    MemoryKind, OpTy, Operand, PlaceTy, Pointer, Provenance, Scalar, StackPopUnwind,
 };
 
 /// Data returned by Machine::stack_pop,
@@ -226,11 +225,13 @@ pub trait Machine<'mir, 'tcx>: Sized {
     /// Since reading a ZST is not actually accessing memory or locals, this is never invoked
     /// for ZST reads.
     #[inline]
-    fn access_local(
-        _ecx: &InterpCx<'mir, 'tcx, Self>,
-        frame: &Frame<'mir, 'tcx, Self::PointerTag, Self::FrameExtra>,
+    fn access_local<'a>(
+        frame: &'a Frame<'mir, 'tcx, Self::PointerTag, Self::FrameExtra>,
         local: mir::Local,
-    ) -> InterpResult<'tcx, Operand<Self::PointerTag>> {
+    ) -> InterpResult<'tcx, &'a Operand<Self::PointerTag>>
+    where
+        'tcx: 'mir,
+    {
         frame.locals[local].access()
     }
 
@@ -242,7 +243,7 @@ pub trait Machine<'mir, 'tcx>: Sized {
         ecx: &'a mut InterpCx<'mir, 'tcx, Self>,
         frame: usize,
         local: mir::Local,
-    ) -> InterpResult<'tcx, Result<&'a mut LocalValue<Self::PointerTag>, MemPlace<Self::PointerTag>>>
+    ) -> InterpResult<'tcx, &'a mut Operand<Self::PointerTag>>
     where
         'tcx: 'mir,
     {
@@ -418,12 +419,14 @@ pub trait Machine<'mir, 'tcx>: Sized {
     }
 
     /// Called immediately after a stack frame got popped, but before jumping back to the caller.
+    /// The `locals` have already been destroyed!
     fn after_stack_pop(
         _ecx: &mut InterpCx<'mir, 'tcx, Self>,
         _frame: Frame<'mir, 'tcx, Self::PointerTag, Self::FrameExtra>,
-        _unwinding: bool,
+        unwinding: bool,
     ) -> InterpResult<'tcx, StackPopJump> {
         // By default, we do not support unwinding from panics
+        assert!(!unwinding);
         Ok(StackPopJump::Normal)
     }
 }

compiler/rustc_const_eval/src/interpret/operand.rs

@@ -14,7 +14,7 @@ use rustc_target::abi::{self, Abi, Align, HasDataLayout, Size, TagEncoding};
 use rustc_target::abi::{VariantIdx, Variants};
 
 use super::{
-    alloc_range, from_known_layout, mir_assign_valid_types, AllocId, ConstValue, GlobalId,
+    alloc_range, from_known_layout, mir_assign_valid_types, AllocId, ConstValue, Frame, GlobalId,
     InterpCx, InterpResult, MPlaceTy, Machine, MemPlace, Place, PlaceTy, Pointer,
     PointerArithmetic, Provenance, Scalar, ScalarMaybeUninit,
 };
@@ -28,8 +28,15 @@ use super::{
 /// defined on `Immediate`, and do not have to work with a `Place`.
 #[derive(Copy, Clone, PartialEq, Eq, HashStable, Hash, Debug)]
 pub enum Immediate<Tag: Provenance = AllocId> {
+    /// A single scalar value (must have *initialized* `Scalar` ABI).
+    /// FIXME: we also currently often use this for ZST.
+    /// `ScalarMaybeUninit` should reject ZST, and we should use `Uninit` for them instead.
     Scalar(ScalarMaybeUninit<Tag>),
+    /// A pair of two scalar value (must have `ScalarPair` ABI where both fields are
+    /// `Scalar::Initialized`).
     ScalarPair(ScalarMaybeUninit<Tag>, ScalarMaybeUninit<Tag>),
+    /// A value of fully uninitialized memory. Can have and size and layout.
+    Uninit,
 }
 
 #[cfg(all(target_arch = "x86_64", target_pointer_width = "64"))]
@@ -75,6 +82,7 @@ impl<'tcx, Tag: Provenance> Immediate<Tag> {
         match self {
             Immediate::Scalar(val) => val,
             Immediate::ScalarPair(..) => bug!("Got a scalar pair where a scalar was expected"),
+            Immediate::Uninit => ScalarMaybeUninit::Uninit,
         }
     }
 
@@ -88,6 +96,7 @@ impl<'tcx, Tag: Provenance> Immediate<Tag> {
         match self {
             Immediate::ScalarPair(val1, val2) => (val1, val2),
             Immediate::Scalar(..) => bug!("Got a scalar where a scalar pair was expected"),
+            Immediate::Uninit => (ScalarMaybeUninit::Uninit, ScalarMaybeUninit::Uninit),
         }
     }
 
@@ -149,7 +158,10 @@ impl<Tag: Provenance> std::fmt::Display for ImmTy<'_, Tag> {
                 }
                 Immediate::ScalarPair(a, b) => {
                     // FIXME(oli-obk): at least print tuples and slices nicely
-                    write!(f, "({:x}, {:x}): {}", a, b, self.layout.ty,)
+                    write!(f, "({:x}, {:x}): {}", a, b, self.layout.ty)
+                }
+                Immediate::Uninit => {
+                    write!(f, "uninit: {}", self.layout.ty)
                 }
             }
         })
@@ -397,7 +409,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         self.scalar_to_ptr(self.read_scalar(op)?.check_init()?)
     }
 
-    // Turn the wide MPlace into a string (must already be dereferenced!)
+    /// Turn the wide MPlace into a string (must already be dereferenced!)
     pub fn read_str(&self, mplace: &MPlaceTy<'tcx, M::PointerTag>) -> InterpResult<'tcx, &str> {
         let len = mplace.len(self)?;
         let bytes = self.read_bytes_ptr(mplace.ptr, Size::from_bytes(len))?;
@@ -528,10 +540,10 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
     /// Will not access memory, instead an indirect `Operand` is returned.
     ///
     /// This is public because it is used by [priroda](https://github.com/oli-obk/priroda) to get an
-    /// OpTy from a local
+    /// OpTy from a local.
     pub fn local_to_op(
         &self,
-        frame: &super::Frame<'mir, 'tcx, M::PointerTag, M::FrameExtra>,
+        frame: &Frame<'mir, 'tcx, M::PointerTag, M::FrameExtra>,
         local: mir::Local,
         layout: Option<TyAndLayout<'tcx>>,
     ) -> InterpResult<'tcx, OpTy<'tcx, M::PointerTag>> {
@@ -540,7 +552,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
             // Do not read from ZST, they might not be initialized
             Operand::Immediate(Scalar::ZST.into())
         } else {
-            M::access_local(&self, frame, local)?
+            *M::access_local(frame, local)?
         };
         Ok(OpTy { op, layout, align: Some(layout.align.abi) })
     }