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

src/concurrency/thread.rs

@@ -218,34 +218,37 @@ impl<'tcx> Thread<'tcx> {
         }
     }
 
-    /// Return the top user-relevant frame, if there is one.
+    /// Return the top user-relevant frame, if there is one. `skip` indicates how many top frames
+    /// should be skipped.
     /// Note that the choice to return `None` here when there is no user-relevant frame is part of
     /// justifying the optimization that only pushes of user-relevant frames require updating the
     /// `top_user_relevant_frame` field.
-    fn compute_top_user_relevant_frame(&self) -> Option<usize> {
+    fn compute_top_user_relevant_frame(&self, skip: usize) -> Option<usize> {
         self.stack
             .iter()
             .enumerate()
             .rev()
+            .skip(skip)
             .find_map(|(idx, frame)| if frame.extra.is_user_relevant { Some(idx) } else { None })
     }
 
-    /// Re-compute the top user-relevant frame from scratch.
-    pub fn recompute_top_user_relevant_frame(&mut self) {
-        self.top_user_relevant_frame = self.compute_top_user_relevant_frame();
+    /// Re-compute the top user-relevant frame from scratch. `skip` indicates how many top frames
+    /// should be skipped.
+    pub fn recompute_top_user_relevant_frame(&mut self, skip: usize) {
+        self.top_user_relevant_frame = self.compute_top_user_relevant_frame(skip);
     }
 
     /// Set the top user-relevant frame to the given value. Must be equal to what
     /// `get_top_user_relevant_frame` would return!
     pub fn set_top_user_relevant_frame(&mut self, frame_idx: usize) {
-        debug_assert_eq!(Some(frame_idx), self.compute_top_user_relevant_frame());
+        debug_assert_eq!(Some(frame_idx), self.compute_top_user_relevant_frame(0));
         self.top_user_relevant_frame = Some(frame_idx);
     }
 
     /// Returns the topmost frame that is considered user-relevant, or the
     /// top of the stack if there is no such frame, or `None` if the stack is empty.
     pub fn top_user_relevant_frame(&self) -> Option<usize> {
-        debug_assert_eq!(self.top_user_relevant_frame, self.compute_top_user_relevant_frame());
+        debug_assert_eq!(self.top_user_relevant_frame, self.compute_top_user_relevant_frame(0));
         // This can be called upon creation of an allocation. We create allocations while setting up
         // parts of the Rust runtime when we do not have any stack frames yet, so we need to handle
         // empty stacks.

src/eval.rs

@@ -354,11 +354,10 @@ pub fn create_ecx<'tcx>(
             argvs.push(arg_place.to_ref(&ecx));
         }
         // Make an array with all these pointers, in the Miri memory.
-        let argvs_layout = ecx.layout_of(Ty::new_array(
-            tcx,
-            Ty::new_imm_ptr(tcx, tcx.types.u8),
-            u64::try_from(argvs.len()).unwrap(),
-        ))?;
+        let u8_ptr_type = Ty::new_imm_ptr(tcx, tcx.types.u8);
+        let u8_ptr_ptr_type = Ty::new_imm_ptr(tcx, u8_ptr_type);
+        let argvs_layout =
+            ecx.layout_of(Ty::new_array(tcx, u8_ptr_type, u64::try_from(argvs.len()).unwrap()))?;
         let argvs_place = ecx.allocate(argvs_layout, MiriMemoryKind::Machine.into())?;
         for (idx, arg) in argvs.into_iter().enumerate() {
             let place = ecx.project_field(&argvs_place, idx)?;
@@ -373,10 +372,8 @@ pub fn create_ecx<'tcx>(
             ecx.mark_immutable(&argc_place);
             ecx.machine.argc = Some(argc_place.ptr());
 
-            let argv_place = ecx.allocate(
-                ecx.layout_of(Ty::new_imm_ptr(tcx, tcx.types.unit))?,
-                MiriMemoryKind::Machine.into(),
-            )?;
+            let argv_place =
+                ecx.allocate(ecx.layout_of(u8_ptr_ptr_type)?, MiriMemoryKind::Machine.into())?;
             ecx.write_pointer(argvs_place.ptr(), &argv_place)?;
             ecx.mark_immutable(&argv_place);
             ecx.machine.argv = Some(argv_place.ptr());
@@ -398,7 +395,9 @@ pub fn create_ecx<'tcx>(
             }
             ecx.mark_immutable(&cmd_place);
         }
-        ecx.mplace_to_ref(&argvs_place)?
+        let imm = argvs_place.to_ref(&ecx);
+        let layout = ecx.layout_of(u8_ptr_ptr_type)?;
+        ImmTy::from_immediate(imm, layout)
     };
 
     // Return place (in static memory so that it does not count as leak).

src/helpers.rs

@@ -470,7 +470,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             caller_fn_abi,
             &args.iter().map(|a| FnArg::Copy(a.clone().into())).collect::<Vec<_>>(),
             /*with_caller_location*/ false,
-            &dest,
+            &dest.into(),
             stack_pop,
         )
     }

src/intrinsics/mod.rs

@@ -22,7 +22,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         &mut self,
         instance: ty::Instance<'tcx>,
         args: &[OpTy<'tcx>],
-        dest: &MPlaceTy<'tcx>,
+        dest: &PlaceTy<'tcx>,
         ret: Option<mir::BasicBlock>,
         unwind: mir::UnwindAction,
     ) -> InterpResult<'tcx, Option<ty::Instance<'tcx>>> {
@@ -45,7 +45,10 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         let intrinsic_name = this.tcx.item_name(instance.def_id());
         let intrinsic_name = intrinsic_name.as_str();
 
-        match this.emulate_intrinsic_by_name(intrinsic_name, instance.args, args, dest, ret)? {
+        // FIXME: avoid allocating memory
+        let dest = this.force_allocation(dest)?;
+
+        match this.emulate_intrinsic_by_name(intrinsic_name, instance.args, args, &dest, ret)? {
             EmulateItemResult::NotSupported => {
                 // We haven't handled the intrinsic, let's see if we can use a fallback body.
                 if this.tcx.intrinsic(instance.def_id()).unwrap().must_be_overridden {

src/machine.rs

@@ -1115,7 +1115,7 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> {
         instance: ty::Instance<'tcx>,
         abi: &FnAbi<'tcx, Ty<'tcx>>,
         args: &[FnArg<'tcx, Provenance>],
-        dest: &MPlaceTy<'tcx>,
+        dest: &PlaceTy<'tcx>,
         ret: Option<mir::BasicBlock>,
         unwind: mir::UnwindAction,
     ) -> InterpResult<'tcx, Option<(&'tcx mir::Body<'tcx>, ty::Instance<'tcx>)>> {
@@ -1142,7 +1142,7 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> {
         fn_val: DynSym,
         abi: &FnAbi<'tcx, Ty<'tcx>>,
         args: &[FnArg<'tcx, Provenance>],
-        dest: &MPlaceTy<'tcx>,
+        dest: &PlaceTy<'tcx>,
         ret: Option<mir::BasicBlock>,
         unwind: mir::UnwindAction,
     ) -> InterpResult<'tcx> {
@@ -1155,7 +1155,7 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> {
         ecx: &mut MiriInterpCx<'tcx>,
         instance: ty::Instance<'tcx>,
         args: &[OpTy<'tcx>],
-        dest: &MPlaceTy<'tcx>,
+        dest: &PlaceTy<'tcx>,
         ret: Option<mir::BasicBlock>,
         unwind: mir::UnwindAction,
     ) -> InterpResult<'tcx, Option<ty::Instance<'tcx>>> {
@@ -1634,15 +1634,21 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> {
         interp_ok(())
     }
 
-    fn before_stack_pop(
-        ecx: &InterpCx<'tcx, Self>,
-        frame: &Frame<'tcx, Self::Provenance, Self::FrameExtra>,
-    ) -> InterpResult<'tcx> {
+    fn before_stack_pop(ecx: &mut InterpCx<'tcx, Self>) -> InterpResult<'tcx> {
+        let frame = ecx.frame();
         // We want this *before* the return value copy, because the return place itself is protected
         // until we do `end_call` here.
         if ecx.machine.borrow_tracker.is_some() {
             ecx.on_stack_pop(frame)?;
         }
+        if frame.extra.is_user_relevant {
+            // All that we store is whether or not the frame we just removed is local, so now we
+            // have no idea where the next topmost local frame is. So we recompute it.
+            // (If this ever becomes a bottleneck, we could have `push` store the previous
+            // user-relevant frame and restore that here.)
+            // We have to skip the frame that is just being popped.
+            ecx.active_thread_mut().recompute_top_user_relevant_frame(/* skip */ 1);
+        }
         // tracing-tree can autoamtically annotate scope changes, but it gets very confused by our
         // concurrency and what it prints is just plain wrong. So we print our own information
         // instead. (Cc https://github.com/rust-lang/miri/issues/2266)
@@ -1656,15 +1662,8 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> {
         frame: Frame<'tcx, Provenance, FrameExtra<'tcx>>,
         unwinding: bool,
     ) -> InterpResult<'tcx, ReturnAction> {
-        if frame.extra.is_user_relevant {
-            // All that we store is whether or not the frame we just removed is local, so now we
-            // have no idea where the next topmost local frame is. So we recompute it.
-            // (If this ever becomes a bottleneck, we could have `push` store the previous
-            // user-relevant frame and restore that here.)
-            ecx.active_thread_mut().recompute_top_user_relevant_frame();
-        }
         let res = {
-            // Move `frame`` into a sub-scope so we control when it will be dropped.
+            // Move `frame` into a sub-scope so we control when it will be dropped.
             let mut frame = frame;
             let timing = frame.extra.timing.take();
             let res = ecx.handle_stack_pop_unwind(frame.extra, unwinding);

src/shims/foreign_items.rs

@@ -43,7 +43,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         link_name: Symbol,
         abi: &FnAbi<'tcx, Ty<'tcx>>,
         args: &[OpTy<'tcx>],
-        dest: &MPlaceTy<'tcx>,
+        dest: &PlaceTy<'tcx>,
         ret: Option<mir::BasicBlock>,
         unwind: mir::UnwindAction,
     ) -> InterpResult<'tcx, Option<(&'tcx mir::Body<'tcx>, ty::Instance<'tcx>)>> {
@@ -69,8 +69,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             _ => {}
         }
 
+        // FIXME: avoid allocating memory
+        let dest = this.force_allocation(dest)?;
+
         // The rest either implements the logic, or falls back to `lookup_exported_symbol`.
-        match this.emulate_foreign_item_inner(link_name, abi, args, dest)? {
+        match this.emulate_foreign_item_inner(link_name, abi, args, &dest)? {
             EmulateItemResult::NeedsReturn => {
                 trace!("{:?}", this.dump_place(&dest.clone().into()));
                 this.return_to_block(ret)?;
@@ -111,7 +114,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         sym: DynSym,
         abi: &FnAbi<'tcx, Ty<'tcx>>,
         args: &[OpTy<'tcx>],
-        dest: &MPlaceTy<'tcx>,
+        dest: &PlaceTy<'tcx>,
         ret: Option<mir::BasicBlock>,
         unwind: mir::UnwindAction,
     ) -> InterpResult<'tcx> {

tests/fail/data_race/stack_pop_race.rs

@@ -8,7 +8,7 @@ struct MakeSend(*const i32);
 unsafe impl Send for MakeSend {}
 
 fn main() {
-    race(0);
+    race(0); //~ERROR: Data race detected between (1) non-atomic read on thread `unnamed-1` and (2) deallocation on thread `main`
 }
 
 // Using an argument for the ptr to point to, since those do not get StorageDead.
@@ -22,5 +22,4 @@ fn race(local: i32) {
     thread::yield_now();
     // Deallocating the local (when `main` returns)
     // races with the read in the other thread.
-    // Make sure the error points at this function's end, not just the call site.
-} //~ERROR: Data race detected between (1) non-atomic read on thread `unnamed-1` and (2) deallocation on thread `main`
+}

tests/fail/data_race/stack_pop_race.stderr

@@ -1,8 +1,8 @@
 error: Undefined Behavior: Data race detected between (1) non-atomic read on thread `unnamed-ID` and (2) deallocation on thread `main` at ALLOC. (2) just happened here
   --> tests/fail/data_race/stack_pop_race.rs:LL:CC
    |
-LL | }
-   |  ^ Data race detected between (1) non-atomic read on thread `unnamed-ID` and (2) deallocation on thread `main` at ALLOC. (2) just happened here
+LL |     race(0);
+   |     ^^^^^^^ Data race detected between (1) non-atomic read on thread `unnamed-ID` and (2) deallocation on thread `main` at ALLOC. (2) just happened here
    |
 help: and (1) occurred earlier here
   --> tests/fail/data_race/stack_pop_race.rs:LL:CC
@@ -12,12 +12,7 @@ LL |         let _val = unsafe { *ptr.0 };
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
    = note: BACKTRACE (of the first span):
-   = note: inside `race` at tests/fail/data_race/stack_pop_race.rs:LL:CC
-note: inside `main`
-  --> tests/fail/data_race/stack_pop_race.rs:LL:CC
-   |
-LL |     race(0);
-   |     ^^^^^^^
+   = note: inside `main` at tests/fail/data_race/stack_pop_race.rs:LL:CC
 
 note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
 

tests/fail/tail_calls/dangling-local-var.stderr

@@ -14,8 +14,8 @@ LL |     let local = 0;
 help: ALLOC was deallocated here:
   --> tests/fail/tail_calls/dangling-local-var.rs:LL:CC
    |
-LL |     become g(ptr)
-   |     ^^^^^^^^^^^^^
+LL |     f(std::ptr::null());
+   |     ^^^^^^^^^^^^^^^^^^^
    = note: BACKTRACE (of the first span):
    = note: inside `g` at tests/fail/tail_calls/dangling-local-var.rs:LL:CC
 note: inside `main`

tests/pass/alloc-access-tracking.rs

@@ -1,7 +1,7 @@
 #![no_std]
 #![no_main]
-//@compile-flags: -Zmiri-track-alloc-id=21 -Zmiri-track-alloc-accesses -Cpanic=abort
-//@normalize-stderr-test: "id 21" -> "id $$ALLOC"
+//@compile-flags: -Zmiri-track-alloc-id=20 -Zmiri-track-alloc-accesses -Cpanic=abort
+//@normalize-stderr-test: "id 20" -> "id $$ALLOC"
 //@only-target: linux # alloc IDs differ between OSes (due to extern static allocations)
 
 extern "Rust" {