From ece1397e3ff40e7f8a411981844a8214659da970 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 18 Aug 2025 17:22:02 +0200 Subject: interpret: fix in-place return place semantics when the return place expression is a local variable --- compiler/rustc_const_eval/src/interpret/step.rs | 1 + 1 file changed, 1 insertion(+) (limited to 'compiler/rustc_const_eval/src/interpret/step.rs') diff --git a/compiler/rustc_const_eval/src/interpret/step.rs b/compiler/rustc_const_eval/src/interpret/step.rs index 76e470b69dc..36251f774c6 100644 --- a/compiler/rustc_const_eval/src/interpret/step.rs +++ b/compiler/rustc_const_eval/src/interpret/step.rs @@ -415,6 +415,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { // caller directly access this local! // This is also crucial for tail calls, where we want the `FnArg` to // stay valid when the old stack frame gets popped. + // FIXME: How can this be right for aliasing arguments? FnArg::Copy(op) } } -- cgit 1.4.1-3-g733a5 From dc72692591c937e15cb016c57d4dee6a81340bbd Mon Sep 17 00:00:00 2001 From: Stypox Date: Tue, 12 Aug 2025 12:16:43 +0200 Subject: Add tracing to various miscellaneous functions Also use tracing macro syntax instead of format() --- compiler/rustc_const_eval/src/interpret/eval_context.rs | 4 ++-- compiler/rustc_const_eval/src/interpret/stack.rs | 5 ++++- compiler/rustc_const_eval/src/interpret/step.rs | 5 ++++- compiler/rustc_const_eval/src/interpret/validity.rs | 4 +++- src/tools/miri/src/machine.rs | 2 ++ 5 files changed, 15 insertions(+), 5 deletions(-) (limited to 'compiler/rustc_const_eval/src/interpret/step.rs') diff --git a/compiler/rustc_const_eval/src/interpret/eval_context.rs b/compiler/rustc_const_eval/src/interpret/eval_context.rs index c4b705d7124..bd312b2b8d1 100644 --- a/compiler/rustc_const_eval/src/interpret/eval_context.rs +++ b/compiler/rustc_const_eval/src/interpret/eval_context.rs @@ -325,8 +325,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { let _span = enter_trace_span!( M, "instantiate_from_frame_and_normalize_erasing_regions", - "{}", - frame.instance + %frame.instance ); frame .instance @@ -582,6 +581,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { span: Span, layout: Option>, ) -> InterpResult<'tcx, OpTy<'tcx, M::Provenance>> { + let _trace = enter_trace_span!(M, const_eval::eval_mir_constant, ?val); let const_val = val.eval(*self.tcx, self.typing_env, span).map_err(|err| { if M::ALL_CONSTS_ARE_PRECHECKED { match err { diff --git a/compiler/rustc_const_eval/src/interpret/stack.rs b/compiler/rustc_const_eval/src/interpret/stack.rs index 73cc87508ef..7cabfd96121 100644 --- a/compiler/rustc_const_eval/src/interpret/stack.rs +++ b/compiler/rustc_const_eval/src/interpret/stack.rs @@ -20,7 +20,7 @@ use super::{ MemoryKind, Operand, PlaceTy, Pointer, Provenance, ReturnAction, Scalar, from_known_layout, interp_ok, throw_ub, throw_unsup, }; -use crate::errors; +use crate::{enter_trace_span, errors}; // The Phantomdata exists to prevent this type from being `Send`. If it were sent across a thread // boundary and dropped in the other thread, it would exit the span in the other thread. @@ -386,6 +386,9 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { // Make sure all the constants required by this frame evaluate successfully (post-monomorphization check). for &const_ in body.required_consts() { + // We can't use `eval_mir_constant` here as that assumes that all required consts have + // already been checked, so we need a separate tracing call. + let _trace = enter_trace_span!(M, const_eval::required_consts, ?const_.const_); let c = self.instantiate_from_current_frame_and_normalize_erasing_regions(const_.const_)?; c.eval(*self.tcx, self.typing_env, const_.span).map_err(|err| { diff --git a/compiler/rustc_const_eval/src/interpret/step.rs b/compiler/rustc_const_eval/src/interpret/step.rs index 9df49c0f4cc..c0c00e6196c 100644 --- a/compiler/rustc_const_eval/src/interpret/step.rs +++ b/compiler/rustc_const_eval/src/interpret/step.rs @@ -436,7 +436,10 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { .map(|arg| self.eval_fn_call_argument(&arg.node)) .collect::>>()?; - let fn_sig_binder = func.layout.ty.fn_sig(*self.tcx); + let fn_sig_binder = { + let _trace = enter_trace_span!(M, "fn_sig", ty = ?func.layout.ty.kind()); + func.layout.ty.fn_sig(*self.tcx) + }; let fn_sig = self.tcx.normalize_erasing_late_bound_regions(self.typing_env, fn_sig_binder); let extra_args = &args[fn_sig.inputs().len()..]; let extra_args = diff --git a/compiler/rustc_const_eval/src/interpret/validity.rs b/compiler/rustc_const_eval/src/interpret/validity.rs index ed48f53c310..db4e8e83ff2 100644 --- a/compiler/rustc_const_eval/src/interpret/validity.rs +++ b/compiler/rustc_const_eval/src/interpret/validity.rs @@ -1418,7 +1418,9 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { let _span = enter_trace_span!( M, "validate_operand", - "recursive={recursive}, reset_provenance_and_padding={reset_provenance_and_padding}, val={val:?}" + recursive, + reset_provenance_and_padding, + ?val, ); // Note that we *could* actually be in CTFE here with `-Zextra-const-ub-checks`, but it's diff --git a/src/tools/miri/src/machine.rs b/src/tools/miri/src/machine.rs index 00c3373bb0f..75bc9705bd2 100644 --- a/src/tools/miri/src/machine.rs +++ b/src/tools/miri/src/machine.rs @@ -1111,6 +1111,7 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> { ) -> InterpResult<'tcx, Option<(&'tcx mir::Body<'tcx>, ty::Instance<'tcx>)>> { // For foreign items, try to see if we can emulate them. if ecx.tcx.is_foreign_item(instance.def_id()) { + let _trace = enter_trace_span!("emulate_foreign_item"); // An external function call that does not have a MIR body. We either find MIR elsewhere // or emulate its effect. // This will be Ok(None) if we're emulating the intrinsic entirely within Miri (no need @@ -1123,6 +1124,7 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> { } // Otherwise, load the MIR. + let _trace = enter_trace_span!("load_mir"); interp_ok(Some((ecx.load_mir(instance.def, None)?, instance))) } -- cgit 1.4.1-3-g733a5 From 7dfbc0ac1435b76bce4b6baf1ce1b3f7080538e1 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 18 Aug 2025 19:44:45 +0200 Subject: miri: detect passing the same local twice as an in-place argument --- compiler/rustc_const_eval/src/interpret/step.rs | 61 +++++++++++++++------- .../function_calls/arg_inplace_locals_alias.rs | 34 ++++++++++++ .../arg_inplace_locals_alias.stack.stderr | 25 +++++++++ .../arg_inplace_locals_alias.tree.stderr | 33 ++++++++++++ 4 files changed, 133 insertions(+), 20 deletions(-) create mode 100644 src/tools/miri/tests/fail/function_calls/arg_inplace_locals_alias.rs create mode 100644 src/tools/miri/tests/fail/function_calls/arg_inplace_locals_alias.stack.stderr create mode 100644 src/tools/miri/tests/fail/function_calls/arg_inplace_locals_alias.tree.stderr (limited to 'compiler/rustc_const_eval/src/interpret/step.rs') diff --git a/compiler/rustc_const_eval/src/interpret/step.rs b/compiler/rustc_const_eval/src/interpret/step.rs index 36251f774c6..19fecd623b6 100644 --- a/compiler/rustc_const_eval/src/interpret/step.rs +++ b/compiler/rustc_const_eval/src/interpret/step.rs @@ -4,6 +4,7 @@ use either::Either; use rustc_abi::{FIRST_VARIANT, FieldIdx}; +use rustc_data_structures::fx::FxHashSet; use rustc_index::IndexSlice; use rustc_middle::ty::{self, Instance, Ty}; use rustc_middle::{bug, mir, span_bug}; @@ -389,8 +390,9 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { /// Evaluate the arguments of a function call fn eval_fn_call_argument( - &self, + &mut self, op: &mir::Operand<'tcx>, + move_definitely_disjoint: bool, ) -> InterpResult<'tcx, FnArg<'tcx, M::Provenance>> { interp_ok(match op { mir::Operand::Copy(_) | mir::Operand::Constant(_) => { @@ -399,25 +401,19 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { FnArg::Copy(op) } mir::Operand::Move(place) => { - // If this place lives in memory, preserve its location. - // We call `place_to_op` which will be an `MPlaceTy` whenever there exists - // an mplace for this place. (This is in contrast to `PlaceTy::as_mplace_or_local` - // which can return a local even if that has an mplace.) let place = self.eval_place(*place)?; - let op = self.place_to_op(&place)?; - - match op.as_mplace_or_imm() { - Either::Left(mplace) => FnArg::InPlace(mplace), - Either::Right(_imm) => { - // This argument doesn't live in memory, so there's no place - // to make inaccessible during the call. - // We rely on there not being any stray `PlaceTy` that would let the - // caller directly access this local! - // This is also crucial for tail calls, where we want the `FnArg` to - // stay valid when the old stack frame gets popped. - // FIXME: How can this be right for aliasing arguments? - FnArg::Copy(op) + if move_definitely_disjoint { + // We still have to ensure that no *other* pointers are used to access this place, + // so *if* it is in memory then we have to treat it as `InPlace`. + // Use `place_to_op` to guarantee that we notice it being in memory. + let op = self.place_to_op(&place)?; + match op.as_mplace_or_imm() { + Either::Left(mplace) => FnArg::InPlace(mplace), + Either::Right(_imm) => FnArg::Copy(op), } + } else { + // We have to force this into memory to detect aliasing among `Move` arguments. + FnArg::InPlace(self.force_allocation(&place)?) } } }) @@ -426,15 +422,40 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { /// Shared part of `Call` and `TailCall` implementation — finding and evaluating all the /// necessary information about callee and arguments to make a call. fn eval_callee_and_args( - &self, + &mut self, terminator: &mir::Terminator<'tcx>, func: &mir::Operand<'tcx>, args: &[Spanned>], ) -> InterpResult<'tcx, EvaluatedCalleeAndArgs<'tcx, M>> { let func = self.eval_operand(func, None)?; + + // Evaluating function call arguments. The tricky part here is dealing with `Move` + // arguments: we have to ensure no two such arguments alias. This would be most easily done + // by just forcing them all into memory and then doing the usual in-place argument + // 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. + let move_definitely_disjoint = 'move_definitely_disjoint: { + let mut previous_locals = FxHashSet::::default(); + for arg in args { + let mir::Operand::Move(place) = arg.node else { + continue; // we can skip non-`Move` arguments. + }; + if place.is_indirect_first_projection() { + // An indirect `Move` argument could alias with anything else... + break 'move_definitely_disjoint false; + } + if !previous_locals.insert(place.local) { + // This local is the base for two arguments! They might overlap. + break 'move_definitely_disjoint false; + } + } + // We found no violation so they are all definitely disjoint. + true + }; let args = args .iter() - .map(|arg| self.eval_fn_call_argument(&arg.node)) + .map(|arg| self.eval_fn_call_argument(&arg.node, move_definitely_disjoint)) .collect::>>()?; let fn_sig_binder = func.layout.ty.fn_sig(*self.tcx); diff --git a/src/tools/miri/tests/fail/function_calls/arg_inplace_locals_alias.rs b/src/tools/miri/tests/fail/function_calls/arg_inplace_locals_alias.rs new file mode 100644 index 00000000000..b91a41d7650 --- /dev/null +++ b/src/tools/miri/tests/fail/function_calls/arg_inplace_locals_alias.rs @@ -0,0 +1,34 @@ +//@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::*; + +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(_unit = callee(Move(non_copy), Move(non_copy)), ReturnTo(after_call), UnwindContinue()) + //~[stack]^ ERROR: not granting access + //~[tree]| ERROR: /read access .* forbidden/ + } + after_call = { + Return() + } + } +} + +pub fn callee(x: S, mut y: S) { + // With the setup above, if `x` and `y` are both moved, + // then writing to `y` will change the value stored in `x`! + y.0 = 0; + assert_eq!(x.0, 42); +} diff --git a/src/tools/miri/tests/fail/function_calls/arg_inplace_locals_alias.stack.stderr b/src/tools/miri/tests/fail/function_calls/arg_inplace_locals_alias.stack.stderr new file mode 100644 index 00000000000..0c1100cae63 --- /dev/null +++ b/src/tools/miri/tests/fail/function_calls/arg_inplace_locals_alias.stack.stderr @@ -0,0 +1,25 @@ +error: Undefined Behavior: not granting access to tag because that would remove [Unique for ] which is strongly protected + --> tests/fail/function_calls/arg_inplace_locals_alias.rs:LL:CC + | +LL | Call(_unit = callee(Move(non_copy), 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: was created here, as the root tag for ALLOC + --> tests/fail/function_calls/arg_inplace_locals_alias.rs:LL:CC + | +LL | Call(_unit = callee(Move(non_copy), Move(non_copy)), ReturnTo(after_call), UnwindContinue()) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: is this argument + --> tests/fail/function_calls/arg_inplace_locals_alias.rs:LL:CC + | +LL | y.0 = 0; + | ^^^^^^^ + = note: BACKTRACE (of the first span): + = note: inside `main` at tests/fail/function_calls/arg_inplace_locals_alias.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 + diff --git a/src/tools/miri/tests/fail/function_calls/arg_inplace_locals_alias.tree.stderr b/src/tools/miri/tests/fail/function_calls/arg_inplace_locals_alias.tree.stderr new file mode 100644 index 00000000000..f376c30c879 --- /dev/null +++ b/src/tools/miri/tests/fail/function_calls/arg_inplace_locals_alias.tree.stderr @@ -0,0 +1,33 @@ +error: Undefined Behavior: read access through (root of the allocation) at ALLOC[0x0] is forbidden + --> tests/fail/function_calls/arg_inplace_locals_alias.rs:LL:CC + | +LL | Call(_unit = callee(Move(non_copy), 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: the accessed tag (root of the allocation) is foreign to the protected tag (i.e., it is not a child) + = help: this foreign read access would cause the protected tag (currently Active) to become Disabled + = help: protected tags must never be Disabled +help: the accessed tag was created here + --> tests/fail/function_calls/arg_inplace_locals_alias.rs:LL:CC + | +LL | Call(_unit = callee(Move(non_copy), Move(non_copy)), ReturnTo(after_call), UnwindContinue()) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: the protected tag was created here, in the initial state Reserved + --> tests/fail/function_calls/arg_inplace_locals_alias.rs:LL:CC + | +LL | y.0 = 0; + | ^^^^^^^ +help: the protected tag later transitioned to Active due to a child write access at offsets [0x0..0x4] + --> tests/fail/function_calls/arg_inplace_locals_alias.rs:LL:CC + | +LL | y.0 = 0; + | ^^^^^^^ + = 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.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 + -- cgit 1.4.1-3-g733a5