Auto merge of rust-lang#98831 - RalfJung:no-more-unsized-locals, r=oli-obk

interpret: remove support for unsized_locals

I added support for unsized_locals in rust-lang#59780 but the current implementation is a crude hack and IMO definitely not the right way to have unsized locals in MIR. It also [causes problems](https://rust-lang.zulipchat.com/#narrow/stream/146212-t-compiler.2Fconst-eval/topic/Missing.20Layout.20Check.20in.20.60interpret.2Foperand.2Ers.60.3F). and what codegen does is unsound and has been for years since clearly nobody cares (so I hope nobody actually relies on that implementation and I'll be happy if Miri ensures they do not). I think if we want to have unsized locals in Miri/MIR we should add them properly, either by having a `StorageLive` that takes metadata or by having an `alloca` that returns a pointer (making the ptr indirection explicit) or something like that.

So, this PR removes the `LocalValue::Unallocated` hack. It adds `Immediate::Uninit`, for several reasons:
- This lets us still do fairly little work in `push_stack_frame`, in particular we do not actually have to create any allocations.
- If/when I remove `ScalarMaybeUninit`, we will need something like this to have an "optimized" representation of uninitialized locals. Without this we'd have to put uninitialized integers into the heap!
- const-prop needs some way to indicate "I don't know the value of this local'; it used to use `LocalValue::Unallocated` for that, now it can use `Immediate::Uninit`.

There is still a fundamental difference between `LocalValue::Unallocated` and `Immediate::Uninit`: the latter is considered a regular local that you can read from and write to, it just has a more optimized representation when compared with an actual `Allocation` that is fully uninit. In contrast, `LocalValue::Unallocated`  had this really odd behavior where you would write to it but not read from it. (This is in fact what caused the problems mentioned above.)

While at it I also did two drive-by cleanups/improvements:
- In `pop_stack_frame`, do the return value copying and local deallocation while the frame is still on the stack. This leads to better error locations being reported. The old errors were [sometimes rather confusing](https://rust-lang.zulipchat.com/#narrow/stream/269128-miri/topic/Cron.20Job.20Failure.202022-06-24/near/287445522).
- Deduplicate `copy_op` and `copy_op_transmute`.

r? `@oli-obk`

Author: bors

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)),
                 };
             }
         }
@@ -358,7 +359,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                     let src_field = self.operand_field(src, i)?;
                     let dst_field = self.place_field(dest, i)?;
                     if src_field.layout.ty == cast_ty_field.ty {
-                        self.copy_op(&src_field, &dst_field)?;
+                        self.copy_op(&src_field, &dst_field, /*allow_transmute*/ false)?;
                     } else {
                         self.unsize_into(&src_field, cast_ty_field, &dst_field)?;
                     }

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(&op, &dest, /*allow_transmute*/ true);
+            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/intrinsics.rs

@@ -174,7 +174,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                 let val =
                     self.tcx.const_eval_global_id(self.param_env, gid, Some(self.tcx.span))?;
                 let val = self.const_val_to_op(val, ty, Some(dest.layout))?;
-                self.copy_op(&val, dest)?;
+                self.copy_op(&val, dest, /*allow_transmute*/ false)?;
             }
 
             sym::ctpop
@@ -394,7 +394,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
             }
 
             sym::transmute => {
-                self.copy_op_transmute(&args[0], dest)?;
+                self.copy_op(&args[0], dest, /*allow_transmute*/ true)?;
             }
             sym::assert_inhabited | sym::assert_zero_valid | sym::assert_uninit_valid => {
                 let ty = instance.substs.type_at(0);
@@ -461,7 +461,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                     let place = self.mplace_index(&dest, i)?;
                     let value =
                         if i == index { *elem } else { self.mplace_index(&input, i)?.into() };
-                    self.copy_op(&value, &place.into())?;
+                    self.copy_op(&value, &place.into(), /*allow_transmute*/ false)?;
                 }
             }
             sym::simd_extract => {
@@ -473,11 +473,15 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                     index,
                     input_len
                 );
-                self.copy_op(&self.mplace_index(&input, index)?.into(), dest)?;
+                self.copy_op(
+                    &self.mplace_index(&input, index)?.into(),
+                    dest,
+                    /*allow_transmute*/ false,
+                )?;
             }
             sym::likely | sym::unlikely | sym::black_box => {
                 // These just return their argument
-                self.copy_op(&args[0], dest)?;
+                self.copy_op(&args[0], dest, /*allow_transmute*/ false)?;
             }
             sym::assume => {
                 let cond = self.read_scalar(&args[0])?.check_init()?.to_bool()?;

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)
     }
 }