miri: also detect aliasing of in-place argument and return place

Author: RalfJung

compiler/rustc_const_eval/src/interpret/step.rs

@@ -2,6 +2,8 @@
 //!
 //! The main entry point is the `step` method.
 
+use std::iter;
+
 use either::Either;
 use rustc_abi::{FIRST_VARIANT, FieldIdx};
 use rustc_data_structures::fx::FxHashSet;
@@ -426,6 +428,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
         terminator: &mir::Terminator<'tcx>,
         func: &mir::Operand<'tcx>,
         args: &[Spanned<mir::Operand<'tcx>>],
+        dest: &mir::Place<'tcx>,
     ) -> InterpResult<'tcx, EvaluatedCalleeAndArgs<'tcx, M>> {
         let func = self.eval_operand(func, None)?;
 
@@ -435,14 +438,20 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
         // protection, but then we'd force *a lot* of arguments into memory. So we do some syntactic
         // pre-processing here where if all `move` arguments are syntactically distinct local
         // variables (and none is indirect), we can skip the in-memory forcing.
+        // We have to include `dest` in that list so that we can detect aliasing of an in-place
+        // argument with the return place.
         let move_definitely_disjoint = 'move_definitely_disjoint: {
             let mut previous_locals = FxHashSet::<mir::Local>::default();
-            for arg in args {
-                let mir::Operand::Move(place) = arg.node else {
-                    continue; // we can skip non-`Move` arguments.
-                };
+            for place in args
+                .iter()
+                .filter_map(|a| {
+                    // We only have to care about `Move` arguments.
+                    if let mir::Operand::Move(place) = &a.node { Some(place) } else { None }
+                })
+                .chain(iter::once(dest))
+            {
                 if place.is_indirect_first_projection() {
-                    // An indirect `Move` argument could alias with anything else...
+                    // An indirect in-place argument could alias with anything else...
                     break 'move_definitely_disjoint false;
                 }
                 if !previous_locals.insert(place.local) {
@@ -544,7 +553,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
                 let old_loc = self.frame().loc;
 
                 let EvaluatedCalleeAndArgs { callee, args, fn_sig, fn_abi, with_caller_location } =
-                    self.eval_callee_and_args(terminator, func, args)?;
+                    self.eval_callee_and_args(terminator, func, args, &destination)?;
 
                 let destination = self.eval_place(destination)?;
                 self.init_fn_call(
@@ -567,7 +576,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
                 let old_frame_idx = self.frame_idx();
 
                 let EvaluatedCalleeAndArgs { callee, args, fn_sig, fn_abi, with_caller_location } =
-                    self.eval_callee_and_args(terminator, func, args)?;
+                    self.eval_callee_and_args(terminator, func, args, &mir::Place::return_place())?;
 
                 self.init_fn_tail_call(callee, (fn_sig.abi, fn_abi), &args, with_caller_location)?;
 

src/tools/miri/tests/fail/function_calls/arg_inplace_locals_alias.rs

@@ -1,3 +1,5 @@
+//! Ensure we detect aliasing of two in-place arguments for the tricky case where they do not
+//! live in memory.
 //@revisions: stack tree
 //@[tree]compile-flags: -Zmiri-tree-borrows
 // Validation forces more things into memory, which we can't have here.

src/tools/miri/tests/fail/function_calls/arg_inplace_locals_alias_ret.rs

@@ -0,0 +1,34 @@
+//! Ensure we detect aliasing of a in-place argument with the return place for the tricky case where
+//! they do not live in memory.
+//@revisions: stack tree
+//@[tree]compile-flags: -Zmiri-tree-borrows
+// Validation forces more things into memory, which we can't have here.
+//@compile-flags: -Zmiri-disable-validation
+#![feature(custom_mir, core_intrinsics)]
+use std::intrinsics::mir::*;
+
+#[allow(unused)]
+pub struct S(i32);
+
+#[custom_mir(dialect = "runtime", phase = "optimized")]
+fn main() {
+    mir! {
+        let _unit: ();
+        {
+            let staging = S(42); // This forces `staging` into memory...
+            let _non_copy = staging; // ... so we move it to a non-inmemory local here.
+            // This specifically uses a type with scalar representation to tempt Miri to use the
+            // efficient way of storing local variables (outside adressable memory).
+            Call(_non_copy = callee(Move(_non_copy)), ReturnTo(after_call), UnwindContinue())
+            //~[stack]^ ERROR: not granting access
+            //~[tree]| ERROR: /reborrow .* forbidden/
+        }
+        after_call = {
+            Return()
+        }
+    }
+}
+
+pub fn callee(x: S) -> S {
+    x
+}

src/tools/miri/tests/fail/function_calls/arg_inplace_locals_alias_ret.stack.stderr

@@ -0,0 +1,25 @@
+error: Undefined Behavior: not granting access to tag <TAG> because that would remove [Unique for <TAG>] which is strongly protected
+  --> tests/fail/function_calls/arg_inplace_locals_alias_ret.rs:LL:CC
+   |
+LL |             Call(_non_copy = callee(Move(_non_copy)), ReturnTo(after_call), UnwindContinue())
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Undefined Behavior occurred here
+   |
+   = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
+   = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
+help: <TAG> was created here, as the root tag for ALLOC
+  --> tests/fail/function_calls/arg_inplace_locals_alias_ret.rs:LL:CC
+   |
+LL |             Call(_non_copy = callee(Move(_non_copy)), ReturnTo(after_call), UnwindContinue())
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+help: <TAG> is this argument
+  --> tests/fail/function_calls/arg_inplace_locals_alias_ret.rs:LL:CC
+   |
+LL |     x
+   |     ^
+   = note: BACKTRACE (of the first span):
+   = note: inside `main` at tests/fail/function_calls/arg_inplace_locals_alias_ret.rs:LL:CC
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to 1 previous error
+

src/tools/miri/tests/fail/function_calls/arg_inplace_locals_alias_ret.tree.stderr

@@ -0,0 +1,34 @@
+error: Undefined Behavior: reborrow through <TAG> (root of the allocation) at ALLOC[0x0] is forbidden
+  --> tests/fail/function_calls/arg_inplace_locals_alias_ret.rs:LL:CC
+   |
+LL |             Call(_non_copy = callee(Move(_non_copy)), ReturnTo(after_call), UnwindContinue())
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Undefined Behavior occurred here
+   |
+   = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental
+   = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/tree-borrows.md for further information
+   = help: the accessed tag <TAG> (root of the allocation) is foreign to the protected tag <TAG> (i.e., it is not a child)
+   = help: this reborrow (acting as a foreign read access) would cause the protected tag <TAG> (currently Active) to become Disabled
+   = help: protected tags must never be Disabled
+help: the accessed tag <TAG> was created here
+  --> tests/fail/function_calls/arg_inplace_locals_alias_ret.rs:LL:CC
+   |
+LL |             Call(_non_copy = callee(Move(_non_copy)), ReturnTo(after_call), UnwindContinue())
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+help: the protected tag <TAG> was created here, in the initial state Reserved
+  --> tests/fail/function_calls/arg_inplace_locals_alias_ret.rs:LL:CC
+   |
+LL |     x
+   |     ^
+help: the protected tag <TAG> later transitioned to Active due to a child write access at offsets [0x0..0x4]
+  --> tests/fail/function_calls/arg_inplace_locals_alias_ret.rs:LL:CC
+   |
+LL |     x
+   |     ^
+   = help: this transition corresponds to the first write to a 2-phase borrowed mutable reference
+   = note: BACKTRACE (of the first span):
+   = note: inside `main` at tests/fail/function_calls/arg_inplace_locals_alias_ret.rs:LL:CC
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to 1 previous error
+