diff --git a/compiler/rustc_const_eval/src/const_eval/machine.rs b/compiler/rustc_const_eval/src/const_eval/machine.rs index f9f645af41f..a20f0276b3e 100644 --- a/compiler/rustc_const_eval/src/const_eval/machine.rs +++ b/compiler/rustc_const_eval/src/const_eval/machine.rs @@ -22,7 +22,7 @@ use rustc_target::spec::abi::Abi as CallAbi; use crate::errors::{LongRunning, LongRunningWarn}; use crate::interpret::{ - self, compile_time_machine, AllocId, ConstAllocation, FnVal, Frame, ImmTy, InterpCx, + self, compile_time_machine, AllocId, ConstAllocation, FnArg, FnVal, Frame, ImmTy, InterpCx, InterpResult, OpTy, PlaceTy, Pointer, Scalar, }; use crate::{errors, fluent_generated as fluent}; @@ -201,7 +201,7 @@ impl<'mir, 'tcx: 'mir> CompileTimeEvalContext<'mir, 'tcx> { fn hook_special_const_fn( &mut self, instance: ty::Instance<'tcx>, - args: &[OpTy<'tcx>], + args: &[FnArg<'tcx>], dest: &PlaceTy<'tcx>, ret: Option, ) -> InterpResult<'tcx, Option>> { @@ -210,6 +210,7 @@ impl<'mir, 'tcx: 'mir> CompileTimeEvalContext<'mir, 'tcx> { if Some(def_id) == self.tcx.lang_items().panic_display() || Some(def_id) == self.tcx.lang_items().begin_panic_fn() { + let args = self.copy_fn_args(args)?; // &str or &&str assert!(args.len() == 1); @@ -236,8 +237,9 @@ impl<'mir, 'tcx: 'mir> CompileTimeEvalContext<'mir, 'tcx> { return Ok(Some(new_instance)); } else if Some(def_id) == self.tcx.lang_items().align_offset_fn() { + let args = self.copy_fn_args(args)?; // For align_offset, we replace the function call if the pointer has no address. - match self.align_offset(instance, args, dest, ret)? { + match self.align_offset(instance, &args, dest, ret)? { ControlFlow::Continue(()) => return Ok(Some(instance)), ControlFlow::Break(()) => return Ok(None), } @@ -293,7 +295,7 @@ impl<'mir, 'tcx: 'mir> CompileTimeEvalContext<'mir, 'tcx> { self.eval_fn_call( FnVal::Instance(instance), (CallAbi::Rust, fn_abi), - &[addr, align], + &[FnArg::Copy(addr), FnArg::Copy(align)], /* with_caller_location = */ false, dest, ret, @@ -427,7 +429,7 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir, ecx: &mut InterpCx<'mir, 'tcx, Self>, instance: ty::Instance<'tcx>, _abi: CallAbi, - args: &[OpTy<'tcx>], + args: &[FnArg<'tcx>], dest: &PlaceTy<'tcx>, ret: Option, _unwind: mir::UnwindAction, // unwinding is not supported in consts diff --git a/compiler/rustc_const_eval/src/interpret/eval_context.rs b/compiler/rustc_const_eval/src/interpret/eval_context.rs index 36606ff69a6..5e5ad31d8c2 100644 --- a/compiler/rustc_const_eval/src/interpret/eval_context.rs +++ b/compiler/rustc_const_eval/src/interpret/eval_context.rs @@ -682,11 +682,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { return_to_block: StackPopCleanup, ) -> InterpResult<'tcx> { trace!("body: {:#?}", body); - // Clobber previous return place contents, nobody is supposed to be able to see them any more - // This also checks dereferenceable, but not align. We rely on all constructed places being - // sufficiently aligned (in particular we rely on `deref_operand` checking alignment). - self.write_uninit(return_place)?; - // first push a stack frame so we have access to the local substs + // First push a stack frame so we have access to the local substs let pre_frame = Frame { body, loc: Right(body.span), // Span used for errors caused during preamble. @@ -805,6 +801,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, '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. let copy_ret_result = if !unwinding { let op = self diff --git a/compiler/rustc_const_eval/src/interpret/intern.rs b/compiler/rustc_const_eval/src/interpret/intern.rs index 7b11ad33091..107e5bec614 100644 --- a/compiler/rustc_const_eval/src/interpret/intern.rs +++ b/compiler/rustc_const_eval/src/interpret/intern.rs @@ -30,7 +30,7 @@ use super::{ use crate::const_eval; use crate::errors::{DanglingPtrInFinal, UnsupportedUntypedPointer}; -pub trait CompileTimeMachine<'mir, 'tcx, T> = Machine< +pub trait CompileTimeMachine<'mir, 'tcx: 'mir, T> = Machine< 'mir, 'tcx, MemoryKind = T, diff --git a/compiler/rustc_const_eval/src/interpret/machine.rs b/compiler/rustc_const_eval/src/interpret/machine.rs index b448e3a24c6..e101785b6e2 100644 --- a/compiler/rustc_const_eval/src/interpret/machine.rs +++ b/compiler/rustc_const_eval/src/interpret/machine.rs @@ -17,7 +17,7 @@ use rustc_target::spec::abi::Abi as CallAbi; use crate::const_eval::CheckAlignment; use super::{ - AllocBytes, AllocId, AllocRange, Allocation, ConstAllocation, Frame, ImmTy, InterpCx, + AllocBytes, AllocId, AllocRange, Allocation, ConstAllocation, FnArg, Frame, ImmTy, InterpCx, InterpResult, MemoryKind, OpTy, Operand, PlaceTy, Pointer, Provenance, Scalar, }; @@ -84,7 +84,7 @@ pub trait AllocMap { /// Methods of this trait signifies a point where CTFE evaluation would fail /// and some use case dependent behaviour can instead be applied. -pub trait Machine<'mir, 'tcx>: Sized { +pub trait Machine<'mir, 'tcx: 'mir>: Sized { /// Additional memory kinds a machine wishes to distinguish from the builtin ones type MemoryKind: Debug + std::fmt::Display + MayLeak + Eq + 'static; @@ -182,7 +182,7 @@ pub trait Machine<'mir, 'tcx>: Sized { ecx: &mut InterpCx<'mir, 'tcx, Self>, instance: ty::Instance<'tcx>, abi: CallAbi, - args: &[OpTy<'tcx, Self::Provenance>], + args: &[FnArg<'tcx, Self::Provenance>], destination: &PlaceTy<'tcx, Self::Provenance>, target: Option, unwind: mir::UnwindAction, @@ -194,7 +194,7 @@ pub trait Machine<'mir, 'tcx>: Sized { ecx: &mut InterpCx<'mir, 'tcx, Self>, fn_val: Self::ExtraFnVal, abi: CallAbi, - args: &[OpTy<'tcx, Self::Provenance>], + args: &[FnArg<'tcx, Self::Provenance>], destination: &PlaceTy<'tcx, Self::Provenance>, target: Option, unwind: mir::UnwindAction, @@ -418,6 +418,18 @@ pub trait Machine<'mir, 'tcx>: Sized { Ok(()) } + /// Called on places used for in-place function argument and return value handling. + /// + /// These places need to be protected to make sure the program cannot tell whether the + /// argument/return value was actually copied or passed in-place.. + fn protect_in_place_function_argument( + ecx: &mut InterpCx<'mir, 'tcx, Self>, + place: &PlaceTy<'tcx, Self::Provenance>, + ) -> InterpResult<'tcx> { + // Without an aliasing model, all we can do is put `Uninit` into the place. + ecx.write_uninit(place) + } + /// Called immediately before a new stack frame gets pushed. fn init_frame_extra( ecx: &mut InterpCx<'mir, 'tcx, Self>, @@ -439,6 +451,14 @@ pub trait Machine<'mir, 'tcx>: Sized { Ok(()) } + /// Called just before the return value is copied to the caller-provided return place. + fn before_stack_pop( + _ecx: &InterpCx<'mir, 'tcx, Self>, + _frame: &Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra>, + ) -> InterpResult<'tcx> { + Ok(()) + } + /// 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( @@ -484,7 +504,7 @@ pub macro compile_time_machine(<$mir: lifetime, $tcx: lifetime>) { _ecx: &mut InterpCx<$mir, $tcx, Self>, fn_val: !, _abi: CallAbi, - _args: &[OpTy<$tcx>], + _args: &[FnArg<$tcx>], _destination: &PlaceTy<$tcx, Self::Provenance>, _target: Option, _unwind: mir::UnwindAction, diff --git a/compiler/rustc_const_eval/src/interpret/mod.rs b/compiler/rustc_const_eval/src/interpret/mod.rs index 898d62361ab..f657f954f9c 100644 --- a/compiler/rustc_const_eval/src/interpret/mod.rs +++ b/compiler/rustc_const_eval/src/interpret/mod.rs @@ -26,6 +26,7 @@ pub use self::machine::{compile_time_machine, AllocMap, Machine, MayLeak, StackP pub use self::memory::{AllocKind, AllocRef, AllocRefMut, FnVal, Memory, MemoryKind}; pub use self::operand::{ImmTy, Immediate, OpTy, Operand}; pub use self::place::{MPlaceTy, MemPlace, MemPlaceMeta, Place, PlaceTy}; +pub use self::terminator::FnArg; pub use self::validity::{CtfeValidationMode, RefTracking}; pub use self::visitor::{MutValueVisitor, Value, ValueVisitor}; diff --git a/compiler/rustc_const_eval/src/interpret/operand.rs b/compiler/rustc_const_eval/src/interpret/operand.rs index 5f89d652fab..6727937363e 100644 --- a/compiler/rustc_const_eval/src/interpret/operand.rs +++ b/compiler/rustc_const_eval/src/interpret/operand.rs @@ -575,14 +575,6 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { Ok(op) } - /// Evaluate a bunch of operands at once - pub(super) fn eval_operands( - &self, - ops: &[mir::Operand<'tcx>], - ) -> InterpResult<'tcx, Vec>> { - ops.iter().map(|op| self.eval_operand(op, None)).collect() - } - fn eval_ty_constant( &self, val: ty::Const<'tcx>, diff --git a/compiler/rustc_const_eval/src/interpret/place.rs b/compiler/rustc_const_eval/src/interpret/place.rs index ca1106384fd..f1c777a077b 100644 --- a/compiler/rustc_const_eval/src/interpret/place.rs +++ b/compiler/rustc_const_eval/src/interpret/place.rs @@ -354,25 +354,27 @@ where #[inline] pub(super) fn get_place_alloc( &self, - place: &MPlaceTy<'tcx, M::Provenance>, + mplace: &MPlaceTy<'tcx, M::Provenance>, ) -> InterpResult<'tcx, Option>> { - assert!(place.layout.is_sized()); - assert!(!place.meta.has_meta()); - let size = place.layout.size; - self.get_ptr_alloc(place.ptr, size, place.align) + let (size, _align) = self + .size_and_align_of_mplace(&mplace)? + .unwrap_or((mplace.layout.size, mplace.layout.align.abi)); + // Due to packed places, only `mplace.align` matters. + self.get_ptr_alloc(mplace.ptr, size, mplace.align) } #[inline] pub(super) fn get_place_alloc_mut( &mut self, - place: &MPlaceTy<'tcx, M::Provenance>, + mplace: &MPlaceTy<'tcx, M::Provenance>, ) -> InterpResult<'tcx, Option>> { - assert!(place.layout.is_sized()); - assert!(!place.meta.has_meta()); - let size = place.layout.size; - self.get_ptr_alloc_mut(place.ptr, size, place.align) + let (size, _align) = self + .size_and_align_of_mplace(&mplace)? + .unwrap_or((mplace.layout.size, mplace.layout.align.abi)); + // Due to packed places, only `mplace.align` matters. + self.get_ptr_alloc_mut(mplace.ptr, size, mplace.align) } /// Check if this mplace is dereferenceable and sufficiently aligned. diff --git a/compiler/rustc_const_eval/src/interpret/terminator.rs b/compiler/rustc_const_eval/src/interpret/terminator.rs index 15823a5975e..97d7a68e190 100644 --- a/compiler/rustc_const_eval/src/interpret/terminator.rs +++ b/compiler/rustc_const_eval/src/interpret/terminator.rs @@ -1,7 +1,8 @@ use std::borrow::Cow; +use either::Either; use rustc_ast::ast::InlineAsmOptions; -use rustc_middle::ty::layout::{FnAbiOf, LayoutOf}; +use rustc_middle::ty::layout::{FnAbiOf, LayoutOf, TyAndLayout}; use rustc_middle::ty::Instance; use rustc_middle::{ mir, @@ -12,12 +13,63 @@ use rustc_target::abi::call::{ArgAbi, ArgAttribute, ArgAttributes, FnAbi, PassMo use rustc_target::spec::abi::Abi; use super::{ - FnVal, ImmTy, Immediate, InterpCx, InterpResult, MPlaceTy, Machine, MemoryKind, OpTy, Operand, - PlaceTy, Scalar, StackPopCleanup, + AllocId, FnVal, ImmTy, Immediate, InterpCx, InterpResult, MPlaceTy, Machine, MemoryKind, OpTy, + Operand, PlaceTy, Provenance, Scalar, StackPopCleanup, }; use crate::fluent_generated as fluent; +/// An argment passed to a function. +#[derive(Clone, Debug)] +pub enum FnArg<'tcx, Prov: Provenance = AllocId> { + /// Pass a copy of the given operand. + Copy(OpTy<'tcx, Prov>), + /// Allow for the argument to be passed in-place: destroy the value originally stored at that place and + /// make the place inaccessible for the duration of the function call. + InPlace(PlaceTy<'tcx, Prov>), +} + +impl<'tcx, Prov: Provenance> FnArg<'tcx, Prov> { + pub fn layout(&self) -> &TyAndLayout<'tcx> { + match self { + FnArg::Copy(op) => &op.layout, + FnArg::InPlace(place) => &place.layout, + } + } +} + impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { + /// Make a copy of the given fn_arg. Any `InPlace` are degenerated to copies, no protection of the + /// original memory occurs. + pub fn copy_fn_arg( + &self, + arg: &FnArg<'tcx, M::Provenance>, + ) -> InterpResult<'tcx, OpTy<'tcx, M::Provenance>> { + match arg { + FnArg::Copy(op) => Ok(op.clone()), + FnArg::InPlace(place) => self.place_to_op(&place), + } + } + + /// Make a copy of the given fn_args. Any `InPlace` are degenerated to copies, no protection of the + /// original memory occurs. + pub fn copy_fn_args( + &self, + args: &[FnArg<'tcx, M::Provenance>], + ) -> InterpResult<'tcx, Vec>> { + args.iter().map(|fn_arg| self.copy_fn_arg(fn_arg)).collect() + } + + pub fn fn_arg_field( + &mut self, + arg: &FnArg<'tcx, M::Provenance>, + field: usize, + ) -> InterpResult<'tcx, FnArg<'tcx, M::Provenance>> { + Ok(match arg { + FnArg::Copy(op) => FnArg::Copy(self.operand_field(op, field)?), + FnArg::InPlace(place) => FnArg::InPlace(self.place_field(place, field)?), + }) + } + pub(super) fn eval_terminator( &mut self, terminator: &mir::Terminator<'tcx>, @@ -68,14 +120,14 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { let old_stack = self.frame_idx(); let old_loc = self.frame().loc; let func = self.eval_operand(func, None)?; - let args = self.eval_operands(args)?; + let args = self.eval_fn_call_arguments(args)?; let fn_sig_binder = func.layout.ty.fn_sig(*self.tcx); let fn_sig = self.tcx.normalize_erasing_late_bound_regions(self.param_env, fn_sig_binder); let extra_args = &args[fn_sig.inputs().len()..]; let extra_args = - self.tcx.mk_type_list_from_iter(extra_args.iter().map(|arg| arg.layout.ty)); + self.tcx.mk_type_list_from_iter(extra_args.iter().map(|arg| arg.layout().ty)); let (fn_val, fn_abi, with_caller_location) = match *func.layout.ty.kind() { ty::FnPtr(_sig) => { @@ -185,6 +237,21 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { Ok(()) } + /// Evaluate the arguments of a function call + pub(super) fn eval_fn_call_arguments( + &mut self, + ops: &[mir::Operand<'tcx>], + ) -> InterpResult<'tcx, Vec>> { + ops.iter() + .map(|op| { + Ok(match op { + mir::Operand::Move(place) => FnArg::InPlace(self.eval_place(*place)?), + _ => FnArg::Copy(self.eval_operand(op, None)?), + }) + }) + .collect() + } + fn check_argument_compat( caller_abi: &ArgAbi<'tcx, Ty<'tcx>>, callee_abi: &ArgAbi<'tcx, Ty<'tcx>>, @@ -275,7 +342,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { fn pass_argument<'x, 'y>( &mut self, caller_args: &mut impl Iterator< - Item = (&'x OpTy<'tcx, M::Provenance>, &'y ArgAbi<'tcx, Ty<'tcx>>), + Item = (&'x FnArg<'tcx, M::Provenance>, &'y ArgAbi<'tcx, Ty<'tcx>>), >, callee_abi: &ArgAbi<'tcx, Ty<'tcx>>, callee_arg: &PlaceTy<'tcx, M::Provenance>, @@ -295,21 +362,25 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // Now, check if !Self::check_argument_compat(caller_abi, callee_abi) { let callee_ty = format!("{}", callee_arg.layout.ty); - let caller_ty = format!("{}", caller_arg.layout.ty); + let caller_ty = format!("{}", caller_arg.layout().ty); throw_ub_custom!( fluent::const_eval_incompatible_types, callee_ty = callee_ty, caller_ty = caller_ty, ) } + // We work with a copy of the argument for now; if this is in-place argument passing, we + // will later protect the source it comes from. This means the callee cannot observe if we + // did in-place of by-copy argument passing, except for pointer equality tests. + let caller_arg_copy = self.copy_fn_arg(&caller_arg)?; // Special handling for unsized parameters. - if caller_arg.layout.is_unsized() { + if caller_arg_copy.layout.is_unsized() { // `check_argument_compat` ensures that both have the same type, so we know they will use the metadata the same way. - assert_eq!(caller_arg.layout.ty, callee_arg.layout.ty); + assert_eq!(caller_arg_copy.layout.ty, callee_arg.layout.ty); // We have to properly pre-allocate the memory for the callee. - // So let's tear down some wrappers. + // So let's tear down some abstractions. // This all has to be in memory, there are no immediate unsized values. - let src = caller_arg.assert_mem_place(); + let src = caller_arg_copy.assert_mem_place(); // The destination cannot be one of these "spread args". let (dest_frame, dest_local) = callee_arg.assert_local(); // We are just initializing things, so there can't be anything here yet. @@ -331,7 +402,12 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // FIXME: Depending on the PassMode, this should reset some padding to uninitialized. (This // is true for all `copy_op`, but there are a lot of special cases for argument passing // specifically.) - self.copy_op(&caller_arg, callee_arg, /*allow_transmute*/ true) + self.copy_op(&caller_arg_copy, callee_arg, /*allow_transmute*/ true)?; + // If this was an in-place pass, protect the place it comes from for the duration of the call. + if let FnArg::InPlace(place) = caller_arg { + M::protect_in_place_function_argument(self, place)?; + } + Ok(()) } /// Call this function -- pushing the stack frame and initializing the arguments. @@ -346,7 +422,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { &mut self, fn_val: FnVal<'tcx, M::ExtraFnVal>, (caller_abi, caller_fn_abi): (Abi, &FnAbi<'tcx, Ty<'tcx>>), - args: &[OpTy<'tcx, M::Provenance>], + args: &[FnArg<'tcx, M::Provenance>], with_caller_location: bool, destination: &PlaceTy<'tcx, M::Provenance>, target: Option, @@ -372,8 +448,15 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { match instance.def { ty::InstanceDef::Intrinsic(def_id) => { assert!(self.tcx.is_intrinsic(def_id)); - // caller_fn_abi is not relevant here, we interpret the arguments directly for each intrinsic. - M::call_intrinsic(self, instance, args, destination, target, unwind) + // FIXME: Should `InPlace` arguments be reset to uninit? + M::call_intrinsic( + self, + instance, + &self.copy_fn_args(args)?, + destination, + target, + unwind, + ) } ty::InstanceDef::VTableShim(..) | ty::InstanceDef::ReifyShim(..) @@ -428,7 +511,13 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { "caller ABI: {:?}, args: {:#?}", caller_abi, args.iter() - .map(|arg| (arg.layout.ty, format!("{:?}", **arg))) + .map(|arg| ( + arg.layout().ty, + match arg { + FnArg::Copy(op) => format!("copy({:?})", *op), + FnArg::InPlace(place) => format!("in-place({:?})", *place), + } + )) .collect::>() ); trace!( @@ -449,7 +538,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // last incoming argument. These two iterators do not have the same type, // so to keep the code paths uniform we accept an allocation // (for RustCall ABI only). - let caller_args: Cow<'_, [OpTy<'tcx, M::Provenance>]> = + let caller_args: Cow<'_, [FnArg<'tcx, M::Provenance>]> = if caller_abi == Abi::RustCall && !args.is_empty() { // Untuple let (untuple_arg, args) = args.split_last().unwrap(); @@ -458,11 +547,10 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { args.iter() .map(|a| Ok(a.clone())) .chain( - (0..untuple_arg.layout.fields.count()) - .map(|i| self.operand_field(untuple_arg, i)), + (0..untuple_arg.layout().fields.count()) + .map(|i| self.fn_arg_field(untuple_arg, i)), ) - .collect::>>>( - )?, + .collect::>>()?, ) } else { // Plain arg passing @@ -523,6 +611,14 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { caller_ty = caller_ty, ) } + // Ensure the return place is aligned and dereferenceable, and protect it for + // in-place return value passing. + if let Either::Left(mplace) = destination.as_mplace_or_local() { + self.check_mplace(mplace)?; + } else { + // Nothing to do for locals, they are always properly allocated and aligned. + } + M::protect_in_place_function_argument(self, destination)?; }; match res { Err(err) => { @@ -538,7 +634,10 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // We have to implement all "object safe receivers". So we have to go search for a // pointer or `dyn Trait` type, but it could be wrapped in newtypes. So recursively // unwrap those newtypes until we are there. - let mut receiver = args[0].clone(); + // An `InPlace` does nothing here, we keep the original receiver intact. We can't + // really pass the argument in-place anyway, and we are constructing a new + // `Immediate` receiver. + let mut receiver = self.copy_fn_arg(&args[0])?; let receiver_place = loop { match receiver.layout.ty.kind() { ty::Ref(..) | ty::RawPtr(..) => { @@ -648,11 +747,13 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } // Adjust receiver argument. Layout can be any (thin) ptr. - args[0] = ImmTy::from_immediate( - Scalar::from_maybe_pointer(adjusted_receiver, self).into(), - self.layout_of(Ty::new_mut_ptr(self.tcx.tcx, dyn_ty))?, - ) - .into(); + args[0] = FnArg::Copy( + ImmTy::from_immediate( + Scalar::from_maybe_pointer(adjusted_receiver, self).into(), + self.layout_of(Ty::new_mut_ptr(self.tcx.tcx, dyn_ty))?, + ) + .into(), + ); trace!("Patched receiver operand to {:#?}", args[0]); // recurse with concrete function self.eval_fn_call( @@ -710,7 +811,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { self.eval_fn_call( FnVal::Instance(instance), (Abi::Rust, fn_abi), - &[arg.into()], + &[FnArg::Copy(arg.into())], false, &ret.into(), Some(target), diff --git a/compiler/rustc_const_eval/src/interpret/visitor.rs b/compiler/rustc_const_eval/src/interpret/visitor.rs index 7a14459399c..879ae198f7e 100644 --- a/compiler/rustc_const_eval/src/interpret/visitor.rs +++ b/compiler/rustc_const_eval/src/interpret/visitor.rs @@ -13,7 +13,7 @@ use super::{InterpCx, MPlaceTy, Machine, OpTy, PlaceTy}; /// A thing that we can project into, and that has a layout. /// This wouldn't have to depend on `Machine` but with the current type inference, /// that's just more convenient to work with (avoids repeating all the `Machine` bounds). -pub trait Value<'mir, 'tcx, M: Machine<'mir, 'tcx>>: Sized { +pub trait Value<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>>: Sized { /// Gets this value's layout. fn layout(&self) -> TyAndLayout<'tcx>; @@ -54,7 +54,7 @@ pub trait Value<'mir, 'tcx, M: Machine<'mir, 'tcx>>: Sized { /// A thing that we can project into given *mutable* access to `ecx`, and that has a layout. /// This wouldn't have to depend on `Machine` but with the current type inference, /// that's just more convenient to work with (avoids repeating all the `Machine` bounds). -pub trait ValueMut<'mir, 'tcx, M: Machine<'mir, 'tcx>>: Sized { +pub trait ValueMut<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>>: Sized { /// Gets this value's layout. fn layout(&self) -> TyAndLayout<'tcx>; diff --git a/compiler/rustc_mir_transform/src/const_prop.rs b/compiler/rustc_mir_transform/src/const_prop.rs index 2f2c7357b00..f3ac679f97b 100644 --- a/compiler/rustc_mir_transform/src/const_prop.rs +++ b/compiler/rustc_mir_transform/src/const_prop.rs @@ -22,8 +22,8 @@ use rustc_target::spec::abi::Abi as CallAbi; use crate::MirPass; use rustc_const_eval::interpret::{ - self, compile_time_machine, AllocId, ConstAllocation, ConstValue, Frame, ImmTy, Immediate, - InterpCx, InterpResult, LocalValue, MemoryKind, OpTy, PlaceTy, Pointer, Scalar, + self, compile_time_machine, AllocId, ConstAllocation, ConstValue, FnArg, Frame, ImmTy, + Immediate, InterpCx, InterpResult, LocalValue, MemoryKind, OpTy, PlaceTy, Pointer, Scalar, StackPopCleanup, }; @@ -185,7 +185,7 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine<'mir, 'tcx> _ecx: &mut InterpCx<'mir, 'tcx, Self>, _instance: ty::Instance<'tcx>, _abi: CallAbi, - _args: &[OpTy<'tcx>], + _args: &[FnArg<'tcx>], _destination: &PlaceTy<'tcx>, _target: Option, _unwind: UnwindAction, diff --git a/compiler/rustc_mir_transform/src/dataflow_const_prop.rs b/compiler/rustc_mir_transform/src/dataflow_const_prop.rs index 78fb196358f..5b6cbb5577c 100644 --- a/compiler/rustc_mir_transform/src/dataflow_const_prop.rs +++ b/compiler/rustc_mir_transform/src/dataflow_const_prop.rs @@ -532,7 +532,7 @@ impl<'tcx, 'map, 'a> Visitor<'tcx> for OperandCollector<'tcx, 'map, 'a> { struct DummyMachine; -impl<'mir, 'tcx> rustc_const_eval::interpret::Machine<'mir, 'tcx> for DummyMachine { +impl<'mir, 'tcx: 'mir> rustc_const_eval::interpret::Machine<'mir, 'tcx> for DummyMachine { rustc_const_eval::interpret::compile_time_machine!(<'mir, 'tcx>); type MemoryKind = !; const PANIC_ON_ALLOC_FAIL: bool = true; @@ -557,7 +557,7 @@ impl<'mir, 'tcx> rustc_const_eval::interpret::Machine<'mir, 'tcx> for DummyMachi _ecx: &mut InterpCx<'mir, 'tcx, Self>, _instance: ty::Instance<'tcx>, _abi: rustc_target::spec::abi::Abi, - _args: &[rustc_const_eval::interpret::OpTy<'tcx, Self::Provenance>], + _args: &[rustc_const_eval::interpret::FnArg<'tcx, Self::Provenance>], _destination: &rustc_const_eval::interpret::PlaceTy<'tcx, Self::Provenance>, _target: Option, _unwind: UnwindAction, diff --git a/src/tools/miri/src/borrow_tracker/mod.rs b/src/tools/miri/src/borrow_tracker/mod.rs index faa23fd2620..a2cf7c80950 100644 --- a/src/tools/miri/src/borrow_tracker/mod.rs +++ b/src/tools/miri/src/borrow_tracker/mod.rs @@ -302,12 +302,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { } } - fn retag_return_place(&mut self) -> InterpResult<'tcx> { + fn protect_place(&mut self, place: &MPlaceTy<'tcx, Provenance>) -> InterpResult<'tcx> { let this = self.eval_context_mut(); let method = this.machine.borrow_tracker.as_ref().unwrap().borrow().borrow_tracker_method; match method { - BorrowTrackerMethod::StackedBorrows => this.sb_retag_return_place(), - BorrowTrackerMethod::TreeBorrows => this.tb_retag_return_place(), + BorrowTrackerMethod::StackedBorrows => this.sb_protect_place(place), + BorrowTrackerMethod::TreeBorrows => this.tb_protect_place(place), } } diff --git a/src/tools/miri/src/borrow_tracker/stacked_borrows/diagnostics.rs b/src/tools/miri/src/borrow_tracker/stacked_borrows/diagnostics.rs index de307a3c5f5..5ec8d80fb32 100644 --- a/src/tools/miri/src/borrow_tracker/stacked_borrows/diagnostics.rs +++ b/src/tools/miri/src/borrow_tracker/stacked_borrows/diagnostics.rs @@ -189,7 +189,7 @@ struct RetagOp { #[derive(Debug, Clone, Copy, PartialEq)] pub enum RetagCause { Normal, - FnReturnPlace, + InPlaceFnPassing, FnEntry, TwoPhase, } @@ -501,7 +501,7 @@ impl RetagCause { match self { RetagCause::Normal => "retag", RetagCause::FnEntry => "function-entry retag", - RetagCause::FnReturnPlace => "return-place retag", + RetagCause::InPlaceFnPassing => "in-place function argument/return passing protection", RetagCause::TwoPhase => "two-phase retag", } .to_string() diff --git a/src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs b/src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs index ca0f69450c9..e22b352e740 100644 --- a/src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs +++ b/src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs @@ -994,35 +994,25 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { } } - /// After a stack frame got pushed, retag the return place so that we are sure - /// it does not alias with anything. - /// - /// This is a HACK because there is nothing in MIR that would make the retag - /// explicit. Also see . - fn sb_retag_return_place(&mut self) -> InterpResult<'tcx> { + /// Protect a place so that it cannot be used any more for the duration of the current function + /// call. + /// + /// This is used to ensure soundness of in-place function argument/return passing. + fn sb_protect_place(&mut self, place: &MPlaceTy<'tcx, Provenance>) -> InterpResult<'tcx> { let this = self.eval_context_mut(); - let return_place = &this.frame().return_place; - if return_place.layout.is_zst() { - // There may not be any memory here, nothing to do. - return Ok(()); - } - // We need this to be in-memory to use tagged pointers. - let return_place = this.force_allocation(&return_place.clone())?; - // We have to turn the place into a pointer to use the existing code. + // We have to turn the place into a pointer to use the usual retagging logic. // (The pointer type does not matter, so we use a raw pointer.) - let ptr_layout = this.layout_of(Ty::new_mut_ptr(this.tcx.tcx, return_place.layout.ty))?; - let val = ImmTy::from_immediate(return_place.to_ref(this), ptr_layout); - // Reborrow it. With protection! That is part of the point. + let ptr_layout = this.layout_of(Ty::new_mut_ptr(this.tcx.tcx, place.layout.ty))?; + let ptr = ImmTy::from_immediate(place.to_ref(this), ptr_layout); + // Reborrow it. With protection! That is the entire point. let new_perm = NewPermission::Uniform { perm: Permission::Unique, access: Some(AccessKind::Write), protector: Some(ProtectorKind::StrongProtector), }; - let val = this.sb_retag_reference(&val, new_perm, RetagCause::FnReturnPlace)?; - // And use reborrowed pointer for return place. - let return_place = this.ref_to_mplace(&val)?; - this.frame_mut().return_place = return_place.into(); + let _new_ptr = this.sb_retag_reference(&ptr, new_perm, RetagCause::InPlaceFnPassing)?; + // We just throw away `new_ptr`, so nobody can access this memory while it is protected. Ok(()) } diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs index 3bb38a249ff..a3f0310f1df 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs @@ -493,36 +493,25 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { } } - /// After a stack frame got pushed, retag the return place so that we are sure - /// it does not alias with anything. - /// - /// This is a HACK because there is nothing in MIR that would make the retag - /// explicit. Also see . - fn tb_retag_return_place(&mut self) -> InterpResult<'tcx> { + /// Protect a place so that it cannot be used any more for the duration of the current function + /// call. + /// + /// This is used to ensure soundness of in-place function argument/return passing. + fn tb_protect_place(&mut self, place: &MPlaceTy<'tcx, Provenance>) -> InterpResult<'tcx> { let this = self.eval_context_mut(); - //this.debug_hint_location(); - let return_place = &this.frame().return_place; - if return_place.layout.is_zst() { - // There may not be any memory here, nothing to do. - return Ok(()); - } - // We need this to be in-memory to use tagged pointers. - let return_place = this.force_allocation(&return_place.clone())?; - // We have to turn the place into a pointer to use the existing code. + // We have to turn the place into a pointer to use the usual retagging logic. // (The pointer type does not matter, so we use a raw pointer.) - let ptr_layout = this.layout_of(Ty::new_mut_ptr(this.tcx.tcx, return_place.layout.ty))?; - let val = ImmTy::from_immediate(return_place.to_ref(this), ptr_layout); - // Reborrow it. With protection! That is part of the point. + let ptr_layout = this.layout_of(Ty::new_mut_ptr(this.tcx.tcx, place.layout.ty))?; + let ptr = ImmTy::from_immediate(place.to_ref(this), ptr_layout); + // Reborrow it. With protection! That is the entire point. let new_perm = Some(NewPermission { initial_state: Permission::new_active(), zero_size: false, protector: Some(ProtectorKind::StrongProtector), }); - let val = this.tb_retag_reference(&val, new_perm)?; - // And use reborrowed pointer for return place. - let return_place = this.ref_to_mplace(&val)?; - this.frame_mut().return_place = return_place.into(); + let _new_ptr = this.tb_retag_reference(&ptr, new_perm)?; + // We just throw away `new_ptr`, so nobody can access this memory while it is protected. Ok(()) } diff --git a/src/tools/miri/src/lib.rs b/src/tools/miri/src/lib.rs index 7e92dc7a0c7..4a093d7bcc6 100644 --- a/src/tools/miri/src/lib.rs +++ b/src/tools/miri/src/lib.rs @@ -55,6 +55,7 @@ extern crate rustc_index; extern crate rustc_session; extern crate rustc_span; extern crate rustc_target; +extern crate either; // the one from rustc // Necessary to pull in object code as the rest of the rustc crates are shipped only as rmeta // files. diff --git a/src/tools/miri/src/machine.rs b/src/tools/miri/src/machine.rs index ac2bad22119..5510e3f94b7 100644 --- a/src/tools/miri/src/machine.rs +++ b/src/tools/miri/src/machine.rs @@ -7,6 +7,7 @@ use std::fmt; use std::path::Path; use std::process; +use either::Either; use rand::rngs::StdRng; use rand::SeedableRng; @@ -533,7 +534,7 @@ impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> { let target = &tcx.sess.target; match target.arch.as_ref() { "wasm32" | "wasm64" => 64 * 1024, // https://webassembly.github.io/spec/core/exec/runtime.html#memory-instances - "aarch64" => + "aarch64" => { if target.options.vendor.as_ref() == "apple" { // No "definitive" source, but see: // https://www.wwdcnotes.com/notes/wwdc20/10214/ @@ -541,7 +542,8 @@ impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> { 16 * 1024 } else { 4 * 1024 - }, + } + } _ => 4 * 1024, } }; @@ -892,7 +894,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { ecx: &mut MiriInterpCx<'mir, 'tcx>, instance: ty::Instance<'tcx>, abi: Abi, - args: &[OpTy<'tcx, Provenance>], + args: &[FnArg<'tcx, Provenance>], dest: &PlaceTy<'tcx, Provenance>, ret: Option, unwind: mir::UnwindAction, @@ -905,12 +907,13 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { ecx: &mut MiriInterpCx<'mir, 'tcx>, fn_val: Dlsym, abi: Abi, - args: &[OpTy<'tcx, Provenance>], + args: &[FnArg<'tcx, Provenance>], dest: &PlaceTy<'tcx, Provenance>, ret: Option, _unwind: mir::UnwindAction, ) -> InterpResult<'tcx> { - ecx.call_dlsym(fn_val, abi, args, dest, ret) + let args = ecx.copy_fn_args(args)?; // FIXME: Should `InPlace` arguments be reset to uninit? + ecx.call_dlsym(fn_val, abi, &args, dest, ret) } #[inline(always)] @@ -1094,8 +1097,9 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { ptr: Pointer, ) -> InterpResult<'tcx> { match ptr.provenance { - Provenance::Concrete { alloc_id, tag } => - intptrcast::GlobalStateInner::expose_ptr(ecx, alloc_id, tag), + Provenance::Concrete { alloc_id, tag } => { + intptrcast::GlobalStateInner::expose_ptr(ecx, alloc_id, tag) + } Provenance::Wildcard => { // No need to do anything for wildcard pointers as // their provenances have already been previously exposed. @@ -1206,6 +1210,25 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { Ok(()) } + fn protect_in_place_function_argument( + ecx: &mut InterpCx<'mir, 'tcx, Self>, + place: &PlaceTy<'tcx, Provenance>, + ) -> InterpResult<'tcx> { + // We do need to write `uninit` so that even after the call ends, the former contents of + // this place cannot be observed any more. + ecx.write_uninit(place)?; + // If we have a borrow tracker, we also have it set up protection so that all reads *and + // writes* during this call are insta-UB. + if ecx.machine.borrow_tracker.is_some() { + if let Either::Left(place) = place.as_mplace_or_local() { + ecx.protect_place(&place)?; + } else { + // Locals that don't have their address taken are as protected as they can ever be. + } + } + Ok(()) + } + #[inline(always)] fn init_frame_extra( ecx: &mut InterpCx<'mir, 'tcx, Self>, @@ -1288,8 +1311,17 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { let stack_len = ecx.active_thread_stack().len(); ecx.active_thread_mut().set_top_user_relevant_frame(stack_len - 1); } - if ecx.machine.borrow_tracker.is_some() { - ecx.retag_return_place()?; + Ok(()) + } + + fn before_stack_pop( + ecx: &InterpCx<'mir, 'tcx, Self>, + frame: &Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra>, + ) -> InterpResult<'tcx> { + // We want this *before* the return value copy, because the return place itself is protected + // until we do `end_call` here. + if let Some(borrow_tracker) = &ecx.machine.borrow_tracker { + borrow_tracker.borrow_mut().end_call(&frame.extra); } Ok(()) } @@ -1308,9 +1340,6 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { ecx.active_thread_mut().recompute_top_user_relevant_frame(); } let timing = frame.extra.timing.take(); - if let Some(borrow_tracker) = &ecx.machine.borrow_tracker { - borrow_tracker.borrow_mut().end_call(&frame.extra); - } let res = ecx.handle_stack_pop_unwind(frame.extra, unwinding); if let Some(profiler) = ecx.machine.profiler.as_ref() { profiler.finish_recording_interval_event(timing.unwrap()); diff --git a/src/tools/miri/src/shims/mod.rs b/src/tools/miri/src/shims/mod.rs index a423a0786b7..1027b24e301 100644 --- a/src/tools/miri/src/shims/mod.rs +++ b/src/tools/miri/src/shims/mod.rs @@ -31,7 +31,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { &mut self, instance: ty::Instance<'tcx>, abi: Abi, - args: &[OpTy<'tcx, Provenance>], + args: &[FnArg<'tcx, Provenance>], dest: &PlaceTy<'tcx, Provenance>, ret: Option, unwind: mir::UnwindAction, @@ -41,7 +41,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { // There are some more lang items we want to hook that CTFE does not hook (yet). if this.tcx.lang_items().align_offset_fn() == Some(instance.def.def_id()) { - let [ptr, align] = check_arg_count(args)?; + let args = this.copy_fn_args(args)?; + let [ptr, align] = check_arg_count(&args)?; if this.align_offset(ptr, align, dest, ret, unwind)? { return Ok(None); } @@ -55,7 +56,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { // to run extra MIR), and Ok(Some(body)) if we found MIR to run for the // foreign function // Any needed call to `goto_block` will be performed by `emulate_foreign_item`. - return this.emulate_foreign_item(instance.def_id(), abi, args, dest, ret, unwind); + let args = this.copy_fn_args(args)?; // FIXME: Should `InPlace` arguments be reset to uninit? + return this.emulate_foreign_item(instance.def_id(), abi, &args, dest, ret, unwind); } // Otherwise, load the MIR. diff --git a/src/tools/miri/tests/fail/function_calls/arg_inplace_mutate.rs b/src/tools/miri/tests/fail/function_calls/arg_inplace_mutate.rs new file mode 100644 index 00000000000..625a8bda8af --- /dev/null +++ b/src/tools/miri/tests/fail/function_calls/arg_inplace_mutate.rs @@ -0,0 +1,34 @@ +//@revisions: stack tree +//@[tree]compile-flags: -Zmiri-tree-borrows +#![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 non_copy = S(42); + let ptr = std::ptr::addr_of_mut!(non_copy); + // Inside `callee`, the first argument and `*ptr` are basically + // aliasing places! + Call(unit, after_call, callee(Move(*ptr), ptr)) + } + after_call = { + Return() + } + + } +} + +pub fn callee(x: S, ptr: *mut S) { + // With the setup above, if `x` is indeed moved in + // (i.e. we actually just get a pointer to the underlying storage), + // then writing to `ptr` will change the value stored in `x`! + unsafe { ptr.write(S(0)) }; + //~[stack]^ ERROR: not granting access + //~[tree]| ERROR: /write access .* forbidden/ + assert_eq!(x.0, 42); +} diff --git a/src/tools/miri/tests/fail/function_calls/arg_inplace_mutate.stack.stderr b/src/tools/miri/tests/fail/function_calls/arg_inplace_mutate.stack.stderr new file mode 100644 index 00000000000..471dc1dd6dd --- /dev/null +++ b/src/tools/miri/tests/fail/function_calls/arg_inplace_mutate.stack.stderr @@ -0,0 +1,37 @@ +error: Undefined Behavior: not granting access to tag because that would remove [Unique for ] which is strongly protected because it is an argument of call ID + --> $DIR/arg_inplace_mutate.rs:LL:CC + | +LL | unsafe { ptr.write(S(0)) }; + | ^^^^^^^^^^^^^^^ not granting access to tag because that would remove [Unique for ] which is strongly protected because it is an argument of call ID + | + = 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 by a SharedReadWrite retag at offsets [0x0..0x4] + --> $DIR/arg_inplace_mutate.rs:LL:CC + | +LL | / mir! { +LL | | let unit: (); +LL | | { +LL | | let non_copy = S(42); +... | +LL | | +LL | | } + | |_____^ +help: is this argument + --> $DIR/arg_inplace_mutate.rs:LL:CC + | +LL | unsafe { ptr.write(S(0)) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: BACKTRACE (of the first span): + = note: inside `callee` at $DIR/arg_inplace_mutate.rs:LL:CC +note: inside `main` + --> $DIR/arg_inplace_mutate.rs:LL:CC + | +LL | Call(unit, after_call, callee(Move(*ptr), ptr)) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: this error originates in the macro `::core::intrinsics::mir::__internal_remove_let` which comes from the expansion of the macro `mir` (in Nightly builds, run with -Z macro-backtrace for more info) + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to previous error + diff --git a/src/tools/miri/tests/fail/function_calls/arg_inplace_mutate.tree.stderr b/src/tools/miri/tests/fail/function_calls/arg_inplace_mutate.tree.stderr new file mode 100644 index 00000000000..35c02cc2ebd --- /dev/null +++ b/src/tools/miri/tests/fail/function_calls/arg_inplace_mutate.tree.stderr @@ -0,0 +1,39 @@ +error: Undefined Behavior: write access through (root of the allocation) is forbidden + --> $DIR/arg_inplace_mutate.rs:LL:CC + | +LL | unsafe { ptr.write(S(0)) }; + | ^^^^^^^^^^^^^^^ write access through (root of the allocation) is forbidden + | + = 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 write access would cause the protected tag to transition from Active to Disabled + = help: this transition would be a loss of read and write permissions, which is not allowed for protected tags +help: the accessed tag was created here + --> $DIR/arg_inplace_mutate.rs:LL:CC + | +LL | / mir! { +LL | | let unit: (); +LL | | { +LL | | let non_copy = S(42); +... | +LL | | +LL | | } + | |_____^ +help: the protected tag was created here, in the initial state Active + --> $DIR/arg_inplace_mutate.rs:LL:CC + | +LL | unsafe { ptr.write(S(0)) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: BACKTRACE (of the first span): + = note: inside `callee` at $DIR/arg_inplace_mutate.rs:LL:CC +note: inside `main` + --> $DIR/arg_inplace_mutate.rs:LL:CC + | +LL | Call(unit, after_call, callee(Move(*ptr), ptr)) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: this error originates in the macro `::core::intrinsics::mir::__internal_remove_let` which comes from the expansion of the macro `mir` (in Nightly builds, run with -Z macro-backtrace for more info) + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to previous error + diff --git a/src/tools/miri/tests/fail/function_calls/arg_inplace_observe_after.rs b/src/tools/miri/tests/fail/function_calls/arg_inplace_observe_after.rs new file mode 100644 index 00000000000..8eda913feb4 --- /dev/null +++ b/src/tools/miri/tests/fail/function_calls/arg_inplace_observe_after.rs @@ -0,0 +1,27 @@ +#![feature(custom_mir, core_intrinsics)] +use std::intrinsics::mir::*; + +pub struct S(i32); + +#[custom_mir(dialect = "runtime", phase = "optimized")] +fn main() { + // FIXME: the span is not great (probably caused by custom MIR) + mir! { //~ERROR: uninitialized + let unit: (); + { + let non_copy = S(42); + // This could change `non_copy` in-place + Call(unit, after_call, change_arg(Move(non_copy))) + } + after_call = { + // So now we must not be allowed to observe non-copy again. + let _observe = non_copy.0; + Return() + } + + } +} + +pub fn change_arg(mut x: S) { + x.0 = 0; +} diff --git a/src/tools/miri/tests/fail/function_calls/arg_inplace_observe_after.stderr b/src/tools/miri/tests/fail/function_calls/arg_inplace_observe_after.stderr new file mode 100644 index 00000000000..3ff7976c70b --- /dev/null +++ b/src/tools/miri/tests/fail/function_calls/arg_inplace_observe_after.stderr @@ -0,0 +1,22 @@ +error: Undefined Behavior: using uninitialized data, but this operation requires initialized memory + --> $DIR/arg_inplace_observe_after.rs:LL:CC + | +LL | / mir! { +LL | | let unit: (); +LL | | { +LL | | let non_copy = S(42); +... | +LL | | +LL | | } + | |_____^ using uninitialized data, but this operation requires initialized memory + | + = 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: + = note: inside `main` at RUSTLIB/core/src/intrinsics/mir.rs:LL:CC + = note: this error originates in the macro `::core::intrinsics::mir::__internal_remove_let` which comes from the expansion of the macro `mir` (in Nightly builds, run with -Z macro-backtrace for more info) + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to previous error + diff --git a/src/tools/miri/tests/fail/function_calls/arg_inplace_observe_during.none.stderr b/src/tools/miri/tests/fail/function_calls/arg_inplace_observe_during.none.stderr new file mode 100644 index 00000000000..baa91484793 --- /dev/null +++ b/src/tools/miri/tests/fail/function_calls/arg_inplace_observe_during.none.stderr @@ -0,0 +1,20 @@ +error: Undefined Behavior: using uninitialized data, but this operation requires initialized memory + --> $DIR/arg_inplace_observe_during.rs:LL:CC + | +LL | unsafe { ptr.read() }; + | ^^^^^^^^^^ using uninitialized data, but this operation requires initialized memory + | + = 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: + = note: inside `change_arg` at $DIR/arg_inplace_observe_during.rs:LL:CC +note: inside `main` + --> $DIR/arg_inplace_observe_during.rs:LL:CC + | +LL | Call(unit, after_call, change_arg(Move(*ptr), ptr)) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to previous error + diff --git a/src/tools/miri/tests/fail/function_calls/arg_inplace_observe_during.rs b/src/tools/miri/tests/fail/function_calls/arg_inplace_observe_during.rs new file mode 100644 index 00000000000..2e57872db96 --- /dev/null +++ b/src/tools/miri/tests/fail/function_calls/arg_inplace_observe_during.rs @@ -0,0 +1,34 @@ +//@revisions: stack tree none +//@[tree]compile-flags: -Zmiri-tree-borrows +//@[none]compile-flags: -Zmiri-disable-stacked-borrows +#![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 non_copy = S(42); + let ptr = std::ptr::addr_of_mut!(non_copy); + // This could change `non_copy` in-place + Call(unit, after_call, change_arg(Move(*ptr), ptr)) + } + after_call = { + Return() + } + + } +} + +pub fn change_arg(mut x: S, ptr: *mut S) { + x.0 = 0; + // If `x` got passed in-place, we'd see the write through `ptr`! + // Make sure we are not allowed to do that read. + unsafe { ptr.read() }; + //~[stack]^ ERROR: not granting access + //~[tree]| ERROR: /read access .* forbidden/ + //~[none]| ERROR: uninitialized +} diff --git a/src/tools/miri/tests/fail/function_calls/arg_inplace_observe_during.stack.stderr b/src/tools/miri/tests/fail/function_calls/arg_inplace_observe_during.stack.stderr new file mode 100644 index 00000000000..a842d3a8044 --- /dev/null +++ b/src/tools/miri/tests/fail/function_calls/arg_inplace_observe_during.stack.stderr @@ -0,0 +1,37 @@ +error: Undefined Behavior: not granting access to tag because that would remove [Unique for ] which is strongly protected because it is an argument of call ID + --> $DIR/arg_inplace_observe_during.rs:LL:CC + | +LL | unsafe { ptr.read() }; + | ^^^^^^^^^^ not granting access to tag because that would remove [Unique for ] which is strongly protected because it is an argument of call ID + | + = 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 by a SharedReadWrite retag at offsets [0x0..0x4] + --> $DIR/arg_inplace_observe_during.rs:LL:CC + | +LL | / mir! { +LL | | let unit: (); +LL | | { +LL | | let non_copy = S(42); +... | +LL | | +LL | | } + | |_____^ +help: is this argument + --> $DIR/arg_inplace_observe_during.rs:LL:CC + | +LL | x.0 = 0; + | ^^^^^^^ + = note: BACKTRACE (of the first span): + = note: inside `change_arg` at $DIR/arg_inplace_observe_during.rs:LL:CC +note: inside `main` + --> $DIR/arg_inplace_observe_during.rs:LL:CC + | +LL | Call(unit, after_call, change_arg(Move(*ptr), ptr)) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: this error originates in the macro `::core::intrinsics::mir::__internal_remove_let` which comes from the expansion of the macro `mir` (in Nightly builds, run with -Z macro-backtrace for more info) + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to previous error + diff --git a/src/tools/miri/tests/fail/function_calls/arg_inplace_observe_during.tree.stderr b/src/tools/miri/tests/fail/function_calls/arg_inplace_observe_during.tree.stderr new file mode 100644 index 00000000000..cbd76c38f62 --- /dev/null +++ b/src/tools/miri/tests/fail/function_calls/arg_inplace_observe_during.tree.stderr @@ -0,0 +1,39 @@ +error: Undefined Behavior: read access through (root of the allocation) is forbidden + --> $DIR/arg_inplace_observe_during.rs:LL:CC + | +LL | unsafe { ptr.read() }; + | ^^^^^^^^^^ read access through (root of the allocation) is forbidden + | + = 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 to transition from Active to Frozen + = help: this transition would be a loss of write permissions, which is not allowed for protected tags +help: the accessed tag was created here + --> $DIR/arg_inplace_observe_during.rs:LL:CC + | +LL | / mir! { +LL | | let unit: (); +LL | | { +LL | | let non_copy = S(42); +... | +LL | | +LL | | } + | |_____^ +help: the protected tag was created here, in the initial state Active + --> $DIR/arg_inplace_observe_during.rs:LL:CC + | +LL | x.0 = 0; + | ^^^^^^^ + = note: BACKTRACE (of the first span): + = note: inside `change_arg` at $DIR/arg_inplace_observe_during.rs:LL:CC +note: inside `main` + --> $DIR/arg_inplace_observe_during.rs:LL:CC + | +LL | Call(unit, after_call, change_arg(Move(*ptr), ptr)) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: this error originates in the macro `::core::intrinsics::mir::__internal_remove_let` which comes from the expansion of the macro `mir` (in Nightly builds, run with -Z macro-backtrace for more info) + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to previous error + diff --git a/src/tools/miri/tests/fail/function_calls/return_pointer_aliasing.none.stderr b/src/tools/miri/tests/fail/function_calls/return_pointer_aliasing.none.stderr new file mode 100644 index 00000000000..9d9dfc89f89 --- /dev/null +++ b/src/tools/miri/tests/fail/function_calls/return_pointer_aliasing.none.stderr @@ -0,0 +1,20 @@ +error: Undefined Behavior: using uninitialized data, but this operation requires initialized memory + --> $DIR/return_pointer_aliasing.rs:LL:CC + | +LL | unsafe { ptr.read() }; + | ^^^^^^^^^^ using uninitialized data, but this operation requires initialized memory + | + = 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: + = note: inside `myfun` at $DIR/return_pointer_aliasing.rs:LL:CC +note: inside `main` + --> $DIR/return_pointer_aliasing.rs:LL:CC + | +LL | Call(*ptr, after_call, myfun(ptr)) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to previous error + diff --git a/src/tools/miri/tests/fail/both_borrows/return_pointer_aliasing.rs b/src/tools/miri/tests/fail/function_calls/return_pointer_aliasing.rs similarity index 70% rename from src/tools/miri/tests/fail/both_borrows/return_pointer_aliasing.rs rename to src/tools/miri/tests/fail/function_calls/return_pointer_aliasing.rs index 03886e7874d..829809102fa 100644 --- a/src/tools/miri/tests/fail/both_borrows/return_pointer_aliasing.rs +++ b/src/tools/miri/tests/fail/function_calls/return_pointer_aliasing.rs @@ -1,11 +1,11 @@ -//@revisions: stack tree +//@revisions: stack tree none //@[tree]compile-flags: -Zmiri-tree-borrows +//@[none]compile-flags: -Zmiri-disable-stacked-borrows #![feature(raw_ref_op)] #![feature(core_intrinsics)] #![feature(custom_mir)] use std::intrinsics::mir::*; -use std::mem::MaybeUninit; #[custom_mir(dialect = "runtime", phase = "optimized")] pub fn main() { @@ -25,8 +25,10 @@ pub fn main() { } fn myfun(ptr: *mut i32) -> i32 { - unsafe { ptr.cast::>().read() }; - //~[stack]^ ERROR: /not granting access/ + unsafe { ptr.read() }; + //~[stack]^ ERROR: not granting access //~[tree]| ERROR: /read access .* forbidden/ + //~[none]| ERROR: uninitialized + // Without an aliasing model, reads are "fine" but at least they return uninit data. 13 } diff --git a/src/tools/miri/tests/fail/both_borrows/return_pointer_aliasing.stack.stderr b/src/tools/miri/tests/fail/function_calls/return_pointer_aliasing.stack.stderr similarity index 78% rename from src/tools/miri/tests/fail/both_borrows/return_pointer_aliasing.stack.stderr rename to src/tools/miri/tests/fail/function_calls/return_pointer_aliasing.stack.stderr index 5bdddf5e49c..d486dcb95df 100644 --- a/src/tools/miri/tests/fail/both_borrows/return_pointer_aliasing.stack.stderr +++ b/src/tools/miri/tests/fail/function_calls/return_pointer_aliasing.stack.stderr @@ -1,8 +1,8 @@ error: Undefined Behavior: not granting access to tag because that would remove [Unique for ] which is strongly protected because it is an argument of call ID --> $DIR/return_pointer_aliasing.rs:LL:CC | -LL | unsafe { ptr.cast::>().read() }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not granting access to tag because that would remove [Unique for ] which is strongly protected because it is an argument of call ID +LL | unsafe { ptr.read() }; + | ^^^^^^^^^^ not granting access to tag because that would remove [Unique for ] which is strongly protected because it is an argument of call ID | = 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 @@ -20,13 +20,8 @@ LL | | } help: is this argument --> $DIR/return_pointer_aliasing.rs:LL:CC | -LL | / fn myfun(ptr: *mut i32) -> i32 { -LL | | unsafe { ptr.cast::>().read() }; -LL | | -LL | | -LL | | 13 -LL | | } - | |_^ +LL | unsafe { ptr.read() }; + | ^^^^^^^^^^^^^^^^^^^^^ = note: BACKTRACE (of the first span): = note: inside `myfun` at $DIR/return_pointer_aliasing.rs:LL:CC note: inside `main` diff --git a/src/tools/miri/tests/fail/both_borrows/return_pointer_aliasing.tree.stderr b/src/tools/miri/tests/fail/function_calls/return_pointer_aliasing.tree.stderr similarity index 83% rename from src/tools/miri/tests/fail/both_borrows/return_pointer_aliasing.tree.stderr rename to src/tools/miri/tests/fail/function_calls/return_pointer_aliasing.tree.stderr index dd76d65c7ca..c2c9de3f4ee 100644 --- a/src/tools/miri/tests/fail/both_borrows/return_pointer_aliasing.tree.stderr +++ b/src/tools/miri/tests/fail/function_calls/return_pointer_aliasing.tree.stderr @@ -1,8 +1,8 @@ error: Undefined Behavior: read access through (root of the allocation) is forbidden --> $DIR/return_pointer_aliasing.rs:LL:CC | -LL | unsafe { ptr.cast::>().read() }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ read access through (root of the allocation) is forbidden +LL | unsafe { ptr.read() }; + | ^^^^^^^^^^ read access through (root of the allocation) is forbidden | = 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) @@ -22,13 +22,8 @@ LL | | } help: the protected tag was created here, in the initial state Active --> $DIR/return_pointer_aliasing.rs:LL:CC | -LL | / fn myfun(ptr: *mut i32) -> i32 { -LL | | unsafe { ptr.cast::>().read() }; -LL | | -LL | | -LL | | 13 -LL | | } - | |_^ +LL | unsafe { ptr.read() }; + | ^^^^^^^^^^^^^^^^^^^^^ = note: BACKTRACE (of the first span): = note: inside `myfun` at $DIR/return_pointer_aliasing.rs:LL:CC note: inside `main` diff --git a/src/tools/miri/tests/pass/function_calls/return_place_on_heap.rs b/src/tools/miri/tests/pass/function_calls/return_place_on_heap.rs new file mode 100644 index 00000000000..dcfebd0f82b --- /dev/null +++ b/src/tools/miri/tests/pass/function_calls/return_place_on_heap.rs @@ -0,0 +1,25 @@ +#![feature(raw_ref_op)] +#![feature(core_intrinsics)] +#![feature(custom_mir)] + +use std::intrinsics::mir::*; + +// Make sure calls with the return place "on the heap" work. +#[custom_mir(dialect = "runtime", phase = "optimized")] +pub fn main() { + mir! { + { + let x = 0; + let ptr = &raw mut x; + Call(*ptr, after_call, myfun()) + } + + after_call = { + Return() + } + } +} + +fn myfun() -> i32 { + 13 +}