Auto merge of #141406 - RalfJung:less-force-allocate, r=oli-obk

interpret: do not force_allocate all return places

A while ago I cleaned up our `PlaceTy` a little, but as a side-effect of that, return places had to always be force-allocated. That turns out to cause quite a few extra allocations, and for a project we are doing where we marry Miri with a model checker, that means a lot of extra work -- local variables are just so much easier to reason about than allocations.

So, this PR brings back the ability to have the return place be just a local of the caller. To make this work cleanly I had to rework stack pop handling a bit, which also changes the output of Miri in some cases as the span for errors occurring during a particular phase of stack pop changed.

With these changes, a no-std binary with a function of functions that just take and return scalar types and that uses no pointers now does not move *any* local variables into memory. :)

r? `@oli-obk`

Author: bors

compiler/rustc_const_eval/src/const_eval/dummy_machine.rs

@@ -90,7 +90,7 @@ impl<'tcx> interpret::Machine<'tcx> for DummyMachine {
         _instance: ty::Instance<'tcx>,
         _abi: &FnAbi<'tcx, Ty<'tcx>>,
         _args: &[interpret::FnArg<'tcx, Self::Provenance>],
-        _destination: &interpret::MPlaceTy<'tcx, Self::Provenance>,
+        _destination: &interpret::PlaceTy<'tcx, Self::Provenance>,
         _target: Option<BasicBlock>,
         _unwind: UnwindAction,
     ) -> interpret::InterpResult<'tcx, Option<(&'tcx Body<'tcx>, ty::Instance<'tcx>)>> {
@@ -108,7 +108,7 @@ impl<'tcx> interpret::Machine<'tcx> for DummyMachine {
         _ecx: &mut InterpCx<'tcx, Self>,
         _instance: ty::Instance<'tcx>,
         _args: &[interpret::OpTy<'tcx, Self::Provenance>],
-        _destination: &interpret::MPlaceTy<'tcx, Self::Provenance>,
+        _destination: &interpret::PlaceTy<'tcx, Self::Provenance>,
         _target: Option<BasicBlock>,
         _unwind: UnwindAction,
     ) -> interpret::InterpResult<'tcx, Option<ty::Instance<'tcx>>> {

compiler/rustc_const_eval/src/const_eval/eval_queries.rs

@@ -71,7 +71,12 @@ fn eval_body_using_ecx<'tcx, R: InterpretationResult<'tcx>>(
 
     // This can't use `init_stack_frame` since `body` is not a function,
     // so computing its ABI would fail. It's also not worth it since there are no arguments to pass.
-    ecx.push_stack_frame_raw(cid.instance, body, &ret, StackPopCleanup::Root { cleanup: false })?;
+    ecx.push_stack_frame_raw(
+        cid.instance,
+        body,
+        &ret.clone().into(),
+        StackPopCleanup::Root { cleanup: false },
+    )?;
     ecx.storage_live_for_always_live_locals()?;
 
     // The main interpreter loop.

compiler/rustc_const_eval/src/const_eval/machine.rs

@@ -22,7 +22,7 @@ use crate::errors::{LongRunning, LongRunningWarn};
 use crate::fluent_generated as fluent;
 use crate::interpret::{
     self, AllocId, AllocInit, AllocRange, ConstAllocation, CtfeProvenance, FnArg, Frame,
-    GlobalAlloc, ImmTy, InterpCx, InterpResult, MPlaceTy, OpTy, Pointer, RangeSet, Scalar,
+    GlobalAlloc, ImmTy, InterpCx, InterpResult, OpTy, PlaceTy, Pointer, RangeSet, Scalar,
     compile_time_machine, interp_ok, throw_exhaust, throw_inval, throw_ub, throw_ub_custom,
     throw_unsup, throw_unsup_format,
 };
@@ -226,7 +226,7 @@ impl<'tcx> CompileTimeInterpCx<'tcx> {
         &mut self,
         instance: ty::Instance<'tcx>,
         args: &[FnArg<'tcx>],
-        _dest: &MPlaceTy<'tcx>,
+        _dest: &PlaceTy<'tcx>,
         _ret: Option<mir::BasicBlock>,
     ) -> InterpResult<'tcx, Option<ty::Instance<'tcx>>> {
         let def_id = instance.def_id();
@@ -343,7 +343,7 @@ impl<'tcx> interpret::Machine<'tcx> for CompileTimeMachine<'tcx> {
         orig_instance: ty::Instance<'tcx>,
         _abi: &FnAbi<'tcx, Ty<'tcx>>,
         args: &[FnArg<'tcx>],
-        dest: &MPlaceTy<'tcx>,
+        dest: &PlaceTy<'tcx>,
         ret: Option<mir::BasicBlock>,
         _unwind: mir::UnwindAction, // unwinding is not supported in consts
     ) -> InterpResult<'tcx, Option<(&'tcx mir::Body<'tcx>, ty::Instance<'tcx>)>> {
@@ -385,7 +385,7 @@ impl<'tcx> interpret::Machine<'tcx> for CompileTimeMachine<'tcx> {
         ecx: &mut InterpCx<'tcx, Self>,
         instance: ty::Instance<'tcx>,
         args: &[OpTy<'tcx>],
-        dest: &MPlaceTy<'tcx, Self::Provenance>,
+        dest: &PlaceTy<'tcx, Self::Provenance>,
         target: Option<mir::BasicBlock>,
         _unwind: mir::UnwindAction,
     ) -> InterpResult<'tcx, Option<ty::Instance<'tcx>>> {

compiler/rustc_const_eval/src/interpret/call.rs

@@ -339,7 +339,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
         caller_fn_abi: &FnAbi<'tcx, Ty<'tcx>>,
         args: &[FnArg<'tcx, M::Provenance>],
         with_caller_location: bool,
-        destination: &MPlaceTy<'tcx, M::Provenance>,
+        destination: &PlaceTy<'tcx, M::Provenance>,
         mut stack_pop: StackPopCleanup,
     ) -> InterpResult<'tcx> {
         // Compute callee information.
@@ -487,7 +487,10 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
             }
 
             // Protect return place for in-place return value passing.
-            M::protect_in_place_function_argument(self, &destination)?;
+            // We only need to protect anything if this is actually an in-memory place.
+            if let Left(mplace) = destination.as_mplace_or_local() {
+                M::protect_in_place_function_argument(self, &mplace)?;
+            }
 
             // Don't forget to mark "initially live" locals as live.
             self.storage_live_for_always_live_locals()?;
@@ -512,7 +515,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
         (caller_abi, caller_fn_abi): (ExternAbi, &FnAbi<'tcx, Ty<'tcx>>),
         args: &[FnArg<'tcx, M::Provenance>],
         with_caller_location: bool,
-        destination: &MPlaceTy<'tcx, M::Provenance>,
+        destination: &PlaceTy<'tcx, M::Provenance>,
         target: Option<mir::BasicBlock>,
         unwind: mir::UnwindAction,
     ) -> InterpResult<'tcx> {
@@ -776,10 +779,11 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
         // Note that we are using `pop_stack_frame_raw` and not `return_from_current_stack_frame`,
         // as the latter "executes" the goto to the return block, but we don't want to,
         // only the tail called function should return to the current return block.
-        M::before_stack_pop(self, self.frame())?;
-
-        let StackPopInfo { return_action, return_to_block, return_place } =
-            self.pop_stack_frame_raw(false)?;
+        let StackPopInfo { return_action, return_to_block, return_place } = self
+            .pop_stack_frame_raw(false, |_this, _return_place| {
+                // This function's return value is just discarded, the tail-callee will fill in the return place instead.
+                interp_ok(())
+            })?;
 
         assert_eq!(return_action, ReturnAction::Normal);
 
@@ -850,7 +854,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
             (ExternAbi::Rust, fn_abi),
             &[FnArg::Copy(arg.into())],
             false,
-            &ret,
+            &ret.into(),
             Some(target),
             unwind,
         )
@@ -891,28 +895,16 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
             throw_ub_custom!(fluent::const_eval_unwind_past_top);
         }
 
-        M::before_stack_pop(self, self.frame())?;
-
-        // Copy return value. Must of course happen *before* we deallocate the locals.
-        // Must be *after* `before_stack_pop` as otherwise the return place might still be protected.
-        let copy_ret_result = if !unwinding {
-            let op = self
-                .local_to_op(mir::RETURN_PLACE, None)
-                .expect("return place should always be live");
-            let dest = self.frame().return_place.clone();
-            let res = self.copy_op_allow_transmute(&op, &dest);
-            trace!("return value: {:?}", self.dump_place(&dest.into()));
-            // 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.
-            res
-        } else {
+        // Get out the return value. Must happen *before* the frame is popped as we have to get the
+        // local's value out.
+        let return_op =
+            self.local_to_op(mir::RETURN_PLACE, None).expect("return place should always be live");
+        // Do the actual pop + copy.
+        let stack_pop_info = self.pop_stack_frame_raw(unwinding, |this, return_place| {
+            this.copy_op_allow_transmute(&return_op, return_place)?;
+            trace!("return value: {:?}", this.dump_place(return_place));
             interp_ok(())
-        };
-
-        // All right, now it is time to actually pop the frame.
-        // An error here takes precedence over the copy error.
-        let (stack_pop_info, ()) = self.pop_stack_frame_raw(unwinding).and(copy_ret_result)?;
+        })?;
 
         match stack_pop_info.return_action {
             ReturnAction::Normal => {}
@@ -924,7 +916,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
                 // If we are not doing cleanup, also skip everything else.
                 assert!(self.stack().is_empty(), "only the topmost frame should ever be leaked");
                 assert!(!unwinding, "tried to skip cleanup during unwinding");
-                // Skip machine hook.
+                // Don't jump anywhere.
                 return interp_ok(());
             }
         }

compiler/rustc_const_eval/src/interpret/intrinsics.rs

@@ -17,9 +17,9 @@ use tracing::trace;
 use super::memory::MemoryKind;
 use super::util::ensure_monomorphic_enough;
 use super::{
-    Allocation, CheckInAllocMsg, ConstAllocation, GlobalId, ImmTy, InterpCx, InterpResult,
-    MPlaceTy, Machine, OpTy, Pointer, PointerArithmetic, Provenance, Scalar, err_inval,
-    err_ub_custom, err_unsup_format, interp_ok, throw_inval, throw_ub_custom, throw_ub_format,
+    Allocation, CheckInAllocMsg, ConstAllocation, GlobalId, ImmTy, InterpCx, InterpResult, Machine,
+    OpTy, PlaceTy, Pointer, PointerArithmetic, Provenance, Scalar, err_inval, err_ub_custom,
+    err_unsup_format, interp_ok, throw_inval, throw_ub_custom, throw_ub_format,
 };
 use crate::fluent_generated as fluent;
 
@@ -112,7 +112,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
         &mut self,
         instance: ty::Instance<'tcx>,
         args: &[OpTy<'tcx, M::Provenance>],
-        dest: &MPlaceTy<'tcx, M::Provenance>,
+        dest: &PlaceTy<'tcx, M::Provenance>,
         ret: Option<mir::BasicBlock>,
     ) -> InterpResult<'tcx, bool> {
         let instance_args = instance.args;
@@ -587,7 +587,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
         &mut self,
         a: &ImmTy<'tcx, M::Provenance>,
         b: &ImmTy<'tcx, M::Provenance>,
-        dest: &MPlaceTy<'tcx, M::Provenance>,
+        dest: &PlaceTy<'tcx, M::Provenance>,
     ) -> InterpResult<'tcx> {
         assert_eq!(a.layout.ty, b.layout.ty);
         assert_matches!(a.layout.ty.kind(), ty::Int(..) | ty::Uint(..));
@@ -801,7 +801,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
     fn float_min_intrinsic<F>(
         &mut self,
         args: &[OpTy<'tcx, M::Provenance>],
-        dest: &MPlaceTy<'tcx, M::Provenance>,
+        dest: &PlaceTy<'tcx, M::Provenance>,
     ) -> InterpResult<'tcx, ()>
     where
         F: rustc_apfloat::Float + rustc_apfloat::FloatConvert<F> + Into<Scalar<M::Provenance>>,
@@ -822,7 +822,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
     fn float_max_intrinsic<F>(
         &mut self,
         args: &[OpTy<'tcx, M::Provenance>],
-        dest: &MPlaceTy<'tcx, M::Provenance>,
+        dest: &PlaceTy<'tcx, M::Provenance>,
     ) -> InterpResult<'tcx, ()>
     where
         F: rustc_apfloat::Float + rustc_apfloat::FloatConvert<F> + Into<Scalar<M::Provenance>>,
@@ -843,7 +843,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
     fn float_minimum_intrinsic<F>(
         &mut self,
         args: &[OpTy<'tcx, M::Provenance>],
-        dest: &MPlaceTy<'tcx, M::Provenance>,
+        dest: &PlaceTy<'tcx, M::Provenance>,
     ) -> InterpResult<'tcx, ()>
     where
         F: rustc_apfloat::Float + rustc_apfloat::FloatConvert<F> + Into<Scalar<M::Provenance>>,
@@ -859,7 +859,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
     fn float_maximum_intrinsic<F>(
         &mut self,
         args: &[OpTy<'tcx, M::Provenance>],
-        dest: &MPlaceTy<'tcx, M::Provenance>,
+        dest: &PlaceTy<'tcx, M::Provenance>,
     ) -> InterpResult<'tcx, ()>
     where
         F: rustc_apfloat::Float + rustc_apfloat::FloatConvert<F> + Into<Scalar<M::Provenance>>,
@@ -875,7 +875,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
     fn float_copysign_intrinsic<F>(
         &mut self,
         args: &[OpTy<'tcx, M::Provenance>],
-        dest: &MPlaceTy<'tcx, M::Provenance>,
+        dest: &PlaceTy<'tcx, M::Provenance>,
     ) -> InterpResult<'tcx, ()>
     where
         F: rustc_apfloat::Float + rustc_apfloat::FloatConvert<F> + Into<Scalar<M::Provenance>>,
@@ -890,7 +890,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
     fn float_abs_intrinsic<F>(
         &mut self,
         args: &[OpTy<'tcx, M::Provenance>],
-        dest: &MPlaceTy<'tcx, M::Provenance>,
+        dest: &PlaceTy<'tcx, M::Provenance>,
     ) -> InterpResult<'tcx, ()>
     where
         F: rustc_apfloat::Float + rustc_apfloat::FloatConvert<F> + Into<Scalar<M::Provenance>>,

compiler/rustc_const_eval/src/interpret/machine.rs

@@ -208,7 +208,7 @@ pub trait Machine<'tcx>: Sized {
         instance: ty::Instance<'tcx>,
         abi: &FnAbi<'tcx, Ty<'tcx>>,
         args: &[FnArg<'tcx, Self::Provenance>],
-        destination: &MPlaceTy<'tcx, Self::Provenance>,
+        destination: &PlaceTy<'tcx, Self::Provenance>,
         target: Option<mir::BasicBlock>,
         unwind: mir::UnwindAction,
     ) -> InterpResult<'tcx, Option<(&'tcx mir::Body<'tcx>, ty::Instance<'tcx>)>>;
@@ -220,7 +220,7 @@ pub trait Machine<'tcx>: Sized {
         fn_val: Self::ExtraFnVal,
         abi: &FnAbi<'tcx, Ty<'tcx>>,
         args: &[FnArg<'tcx, Self::Provenance>],
-        destination: &MPlaceTy<'tcx, Self::Provenance>,
+        destination: &PlaceTy<'tcx, Self::Provenance>,
         target: Option<mir::BasicBlock>,
         unwind: mir::UnwindAction,
     ) -> InterpResult<'tcx>;
@@ -234,7 +234,7 @@ pub trait Machine<'tcx>: Sized {
         ecx: &mut InterpCx<'tcx, Self>,
         instance: ty::Instance<'tcx>,
         args: &[OpTy<'tcx, Self::Provenance>],
-        destination: &MPlaceTy<'tcx, Self::Provenance>,
+        destination: &PlaceTy<'tcx, Self::Provenance>,
         target: Option<mir::BasicBlock>,
         unwind: mir::UnwindAction,
     ) -> InterpResult<'tcx, Option<ty::Instance<'tcx>>>;
@@ -536,11 +536,9 @@ pub trait Machine<'tcx>: Sized {
         interp_ok(())
     }
 
-    /// Called just before the return value is copied to the caller-provided return place.
-    fn before_stack_pop(
-        _ecx: &InterpCx<'tcx, Self>,
-        _frame: &Frame<'tcx, Self::Provenance, Self::FrameExtra>,
-    ) -> InterpResult<'tcx> {
+    /// Called just before the frame is removed from the stack (followed by return value copy and
+    /// local cleanup).
+    fn before_stack_pop(_ecx: &mut InterpCx<'tcx, Self>) -> InterpResult<'tcx> {
         interp_ok(())
     }
 
@@ -675,7 +673,7 @@ pub macro compile_time_machine(<$tcx: lifetime>) {
         fn_val: !,
         _abi: &FnAbi<$tcx, Ty<$tcx>>,
         _args: &[FnArg<$tcx>],
-        _destination: &MPlaceTy<$tcx, Self::Provenance>,
+        _destination: &PlaceTy<$tcx, Self::Provenance>,
         _target: Option<mir::BasicBlock>,
         _unwind: mir::UnwindAction,
     ) -> InterpResult<$tcx> {