Skip to content

Commit

Permalink
Auto merge of rust-lang#121985 - RalfJung:interpret-return-place, r=o…
Browse files Browse the repository at this point in the history
…li-obk

interpret: avoid a long-lived PlaceTy in stack frames

`PlaceTy` uses a representation that's not very stable under changes to the stack. I'd feel better if we didn't have one in the long-term machine state.

r? `@oli-obk`
  • Loading branch information
bors committed Mar 7, 2024
2 parents 8c9a75b + 3f0b6a0 commit 52f8aec
Show file tree
Hide file tree
Showing 32 changed files with 140 additions and 145 deletions.
10 changes: 5 additions & 5 deletions compiler/rustc_const_eval/src/const_eval/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use crate::errors::{LongRunning, LongRunningWarn};
use crate::fluent_generated as fluent;
use crate::interpret::{
self, compile_time_machine, AllocId, AllocRange, ConstAllocation, CtfeProvenance, FnArg, FnVal,
Frame, ImmTy, InterpCx, InterpResult, OpTy, PlaceTy, Pointer, PointerArithmetic, Scalar,
Frame, ImmTy, InterpCx, InterpResult, MPlaceTy, OpTy, Pointer, PointerArithmetic, Scalar,
};

use super::error::*;
Expand Down Expand Up @@ -219,7 +219,7 @@ impl<'mir, 'tcx: 'mir> CompileTimeEvalContext<'mir, 'tcx> {
&mut self,
instance: ty::Instance<'tcx>,
args: &[FnArg<'tcx>],
dest: &PlaceTy<'tcx>,
dest: &MPlaceTy<'tcx>,
ret: Option<mir::BasicBlock>,
) -> InterpResult<'tcx, Option<ty::Instance<'tcx>>> {
let def_id = instance.def_id();
Expand Down Expand Up @@ -280,7 +280,7 @@ impl<'mir, 'tcx: 'mir> CompileTimeEvalContext<'mir, 'tcx> {
&mut self,
instance: ty::Instance<'tcx>,
args: &[OpTy<'tcx>],
dest: &PlaceTy<'tcx>,
dest: &MPlaceTy<'tcx>,
ret: Option<mir::BasicBlock>,
) -> InterpResult<'tcx, ControlFlow<()>> {
assert_eq!(args.len(), 2);
Expand Down Expand Up @@ -410,7 +410,7 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
orig_instance: ty::Instance<'tcx>,
_abi: CallAbi,
args: &[FnArg<'tcx>],
dest: &PlaceTy<'tcx>,
dest: &MPlaceTy<'tcx>,
ret: Option<mir::BasicBlock>,
_unwind: mir::UnwindAction, // unwinding is not supported in consts
) -> InterpResult<'tcx, Option<(&'mir mir::Body<'tcx>, ty::Instance<'tcx>)>> {
Expand Down Expand Up @@ -455,7 +455,7 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
ecx: &mut InterpCx<'mir, 'tcx, Self>,
instance: ty::Instance<'tcx>,
args: &[OpTy<'tcx>],
dest: &PlaceTy<'tcx, Self::Provenance>,
dest: &MPlaceTy<'tcx, Self::Provenance>,
target: Option<mir::BasicBlock>,
_unwind: mir::UnwindAction,
) -> InterpResult<'tcx> {
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_const_eval/src/interpret/eval_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ pub struct Frame<'mir, 'tcx, Prov: Provenance = CtfeProvenance, Extra = ()> {

/// The location where the result of the current stack frame should be written to,
/// and its layout in the caller.
pub return_place: PlaceTy<'tcx, Prov>,
pub return_place: MPlaceTy<'tcx, Prov>,

/// The list of locals for this stack frame, stored in order as
/// `[return_ptr, arguments..., variables..., temporaries...]`.
Expand Down Expand Up @@ -771,7 +771,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
&mut self,
instance: ty::Instance<'tcx>,
body: &'mir mir::Body<'tcx>,
return_place: &PlaceTy<'tcx, M::Provenance>,
return_place: &MPlaceTy<'tcx, M::Provenance>,
return_to_block: StackPopCleanup,
) -> InterpResult<'tcx> {
trace!("body: {:#?}", body);
Expand Down Expand Up @@ -912,7 +912,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
} else {
self.copy_op_allow_transmute(&op, &dest)
};
trace!("return value: {:?}", self.dump_place(&dest));
trace!("return value: {:?}", self.dump_place(&dest.into()));
// We delay actually short-circuiting on this error until *after* the stack frame is
// popped, since we want this error to be attributed to the caller, whose type defines
// this transmute.
Expand Down
12 changes: 6 additions & 6 deletions compiler/rustc_const_eval/src/interpret/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use rustc_span::symbol::{sym, Symbol};
use rustc_target::abi::Size;

use super::{
util::ensure_monomorphic_enough, CheckInAllocMsg, ImmTy, InterpCx, Machine, OpTy, PlaceTy,
util::ensure_monomorphic_enough, CheckInAllocMsg, ImmTy, InterpCx, MPlaceTy, Machine, OpTy,
Pointer,
};

Expand Down Expand Up @@ -104,7 +104,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
&mut self,
instance: ty::Instance<'tcx>,
args: &[OpTy<'tcx, M::Provenance>],
dest: &PlaceTy<'tcx, M::Provenance>,
dest: &MPlaceTy<'tcx, M::Provenance>,
ret: Option<mir::BasicBlock>,
) -> InterpResult<'tcx, bool> {
let instance_args = instance.args;
Expand Down Expand Up @@ -377,7 +377,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
let index = u64::from(self.read_scalar(&args[1])?.to_u32()?);
let elem = &args[2];
let (input, input_len) = self.operand_to_simd(&args[0])?;
let (dest, dest_len) = self.place_to_simd(dest)?;
let (dest, dest_len) = self.mplace_to_simd(dest)?;
assert_eq!(input_len, dest_len, "Return vector length must match input length");
// Bounds are not checked by typeck so we have to do it ourselves.
if index >= input_len {
Expand Down Expand Up @@ -430,7 +430,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
_ => return Ok(false),
}

trace!("{:?}", self.dump_place(dest));
trace!("{:?}", self.dump_place(&dest.clone().into()));
self.go_to_block(ret);
Ok(true)
}
Expand Down Expand Up @@ -488,7 +488,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
&mut self,
a: &ImmTy<'tcx, M::Provenance>,
b: &ImmTy<'tcx, M::Provenance>,
dest: &PlaceTy<'tcx, M::Provenance>,
dest: &MPlaceTy<'tcx, M::Provenance>,
) -> InterpResult<'tcx> {
assert_eq!(a.layout.ty, b.layout.ty);
assert!(matches!(a.layout.ty.kind(), ty::Int(..) | ty::Uint(..)));
Expand All @@ -506,7 +506,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
)
}
// `Rem` says this is all right, so we can let `Div` do its job.
self.binop_ignore_overflow(BinOp::Div, a, b, dest)
self.binop_ignore_overflow(BinOp::Div, a, b, &dest.clone().into())
}

pub fn saturating_arith(
Expand Down
8 changes: 4 additions & 4 deletions compiler/rustc_const_eval/src/interpret/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ pub trait Machine<'mir, 'tcx: 'mir>: Sized {
instance: ty::Instance<'tcx>,
abi: CallAbi,
args: &[FnArg<'tcx, Self::Provenance>],
destination: &PlaceTy<'tcx, Self::Provenance>,
destination: &MPlaceTy<'tcx, Self::Provenance>,
target: Option<mir::BasicBlock>,
unwind: mir::UnwindAction,
) -> InterpResult<'tcx, Option<(&'mir mir::Body<'tcx>, ty::Instance<'tcx>)>>;
Expand All @@ -208,7 +208,7 @@ pub trait Machine<'mir, 'tcx: 'mir>: Sized {
fn_val: Self::ExtraFnVal,
abi: CallAbi,
args: &[FnArg<'tcx, Self::Provenance>],
destination: &PlaceTy<'tcx, Self::Provenance>,
destination: &MPlaceTy<'tcx, Self::Provenance>,
target: Option<mir::BasicBlock>,
unwind: mir::UnwindAction,
) -> InterpResult<'tcx>;
Expand All @@ -219,7 +219,7 @@ pub trait Machine<'mir, 'tcx: 'mir>: Sized {
ecx: &mut InterpCx<'mir, 'tcx, Self>,
instance: ty::Instance<'tcx>,
args: &[OpTy<'tcx, Self::Provenance>],
destination: &PlaceTy<'tcx, Self::Provenance>,
destination: &MPlaceTy<'tcx, Self::Provenance>,
target: Option<mir::BasicBlock>,
unwind: mir::UnwindAction,
) -> InterpResult<'tcx>;
Expand Down Expand Up @@ -584,7 +584,7 @@ pub macro compile_time_machine(<$mir: lifetime, $tcx: lifetime>) {
fn_val: !,
_abi: CallAbi,
_args: &[FnArg<$tcx>],
_destination: &PlaceTy<$tcx, Self::Provenance>,
_destination: &MPlaceTy<$tcx, Self::Provenance>,
_target: Option<mir::BasicBlock>,
_unwind: mir::UnwindAction,
) -> InterpResult<$tcx> {
Expand Down
16 changes: 6 additions & 10 deletions compiler/rustc_const_eval/src/interpret/place.rs
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,12 @@ pub(super) enum Place<Prov: Provenance = CtfeProvenance> {
Local { frame: usize, local: mir::Local, offset: Option<Size> },
}

/// An evaluated place, together with its type.
///
/// This may reference a stack frame by its index, so `PlaceTy` should generally not be kept around
/// for longer than a single operation. Popping and then pushing a stack frame can make `PlaceTy`
/// point to the wrong destination. If the interpreter has multiple stacks, stack switching will
/// also invalidate a `PlaceTy`.
#[derive(Clone)]
pub struct PlaceTy<'tcx, Prov: Provenance = CtfeProvenance> {
place: Place<Prov>, // Keep this private; it helps enforce invariants.
Expand Down Expand Up @@ -495,16 +501,6 @@ where
Ok((mplace, len))
}

/// Converts a repr(simd) place into a place where `place_index` accesses the SIMD elements.
/// Also returns the number of elements.
pub fn place_to_simd(
&mut self,
place: &PlaceTy<'tcx, M::Provenance>,
) -> InterpResult<'tcx, (MPlaceTy<'tcx, M::Provenance>, u64)> {
let mplace = self.force_allocation(place)?;
self.mplace_to_simd(&mplace)
}

pub fn local_to_place(
&self,
frame: usize,
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_const_eval/src/interpret/terminator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
),
};

let destination = self.eval_place(destination)?;
let destination = self.force_allocation(&self.eval_place(destination)?)?;
self.eval_fn_call(
fn_val,
(fn_sig.abi, fn_abi),
Expand Down Expand Up @@ -497,7 +497,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
(caller_abi, caller_fn_abi): (Abi, &FnAbi<'tcx, Ty<'tcx>>),
args: &[FnArg<'tcx, M::Provenance>],
with_caller_location: bool,
destination: &PlaceTy<'tcx, M::Provenance>,
destination: &MPlaceTy<'tcx, M::Provenance>,
target: Option<mir::BasicBlock>,
mut unwind: mir::UnwindAction,
) -> InterpResult<'tcx> {
Expand Down Expand Up @@ -726,7 +726,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
});
}
// Protect return place for in-place return value passing.
M::protect_in_place_function_argument(self, destination)?;
M::protect_in_place_function_argument(self, &destination.clone().into())?;

// Don't forget to mark "initially live" locals as live.
self.storage_live_for_always_live_locals()?;
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_mir_transform/src/dataflow_const_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -929,7 +929,7 @@ impl<'mir, 'tcx: 'mir> rustc_const_eval::interpret::Machine<'mir, 'tcx> for Dumm
_instance: ty::Instance<'tcx>,
_abi: rustc_target::spec::abi::Abi,
_args: &[rustc_const_eval::interpret::FnArg<'tcx, Self::Provenance>],
_destination: &rustc_const_eval::interpret::PlaceTy<'tcx, Self::Provenance>,
_destination: &rustc_const_eval::interpret::MPlaceTy<'tcx, Self::Provenance>,
_target: Option<BasicBlock>,
_unwind: UnwindAction,
) -> interpret::InterpResult<'tcx, Option<(&'mir Body<'tcx>, ty::Instance<'tcx>)>> {
Expand All @@ -947,7 +947,7 @@ impl<'mir, 'tcx: 'mir> rustc_const_eval::interpret::Machine<'mir, 'tcx> for Dumm
_ecx: &mut InterpCx<'mir, 'tcx, Self>,
_instance: ty::Instance<'tcx>,
_args: &[rustc_const_eval::interpret::OpTy<'tcx, Self::Provenance>],
_destination: &rustc_const_eval::interpret::PlaceTy<'tcx, Self::Provenance>,
_destination: &rustc_const_eval::interpret::MPlaceTy<'tcx, Self::Provenance>,
_target: Option<BasicBlock>,
_unwind: UnwindAction,
) -> interpret::InterpResult<'tcx> {
Expand Down
2 changes: 1 addition & 1 deletion src/tools/miri/src/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
f: ty::Instance<'tcx>,
caller_abi: Abi,
args: &[Immediate<Provenance>],
dest: Option<&PlaceTy<'tcx, Provenance>>,
dest: Option<&MPlaceTy<'tcx, Provenance>>,
stack_pop: StackPopCleanup,
) -> InterpResult<'tcx> {
let this = self.eval_context_mut();
Expand Down
6 changes: 3 additions & 3 deletions src/tools/miri/src/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -950,7 +950,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> {
instance: ty::Instance<'tcx>,
abi: Abi,
args: &[FnArg<'tcx, Provenance>],
dest: &PlaceTy<'tcx, Provenance>,
dest: &MPlaceTy<'tcx, Provenance>,
ret: Option<mir::BasicBlock>,
unwind: mir::UnwindAction,
) -> InterpResult<'tcx, Option<(&'mir mir::Body<'tcx>, ty::Instance<'tcx>)>> {
Expand All @@ -977,7 +977,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> {
fn_val: DynSym,
abi: Abi,
args: &[FnArg<'tcx, Provenance>],
dest: &PlaceTy<'tcx, Provenance>,
dest: &MPlaceTy<'tcx, Provenance>,
ret: Option<mir::BasicBlock>,
unwind: mir::UnwindAction,
) -> InterpResult<'tcx> {
Expand All @@ -990,7 +990,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> {
ecx: &mut MiriInterpCx<'mir, 'tcx>,
instance: ty::Instance<'tcx>,
args: &[OpTy<'tcx, Provenance>],
dest: &PlaceTy<'tcx, Provenance>,
dest: &MPlaceTy<'tcx, Provenance>,
ret: Option<mir::BasicBlock>,
unwind: mir::UnwindAction,
) -> InterpResult<'tcx> {
Expand Down
21 changes: 10 additions & 11 deletions src/tools/miri/src/shims/backtrace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
abi: Abi,
link_name: Symbol,
args: &[OpTy<'tcx, Provenance>],
dest: &PlaceTy<'tcx, Provenance>,
dest: &MPlaceTy<'tcx, Provenance>,
) -> InterpResult<'tcx> {
let this = self.eval_context_mut();
let [flags] = this.check_shim(abi, Abi::Rust, link_name, args)?;
Expand All @@ -32,7 +32,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
abi: Abi,
link_name: Symbol,
args: &[OpTy<'tcx, Provenance>],
dest: &PlaceTy<'tcx, Provenance>,
dest: &MPlaceTy<'tcx, Provenance>,
) -> InterpResult<'tcx> {
let this = self.eval_context_mut();
let tcx = this.tcx;
Expand Down Expand Up @@ -145,7 +145,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
abi: Abi,
link_name: Symbol,
args: &[OpTy<'tcx, Provenance>],
dest: &PlaceTy<'tcx, Provenance>,
dest: &MPlaceTy<'tcx, Provenance>,
) -> InterpResult<'tcx> {
let this = self.eval_context_mut();
let [ptr, flags] = this.check_shim(abi, Abi::Rust, link_name, args)?;
Expand Down Expand Up @@ -174,7 +174,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
// `lo.col` is 0-based - add 1 to make it 1-based for the caller.
let colno: u32 = u32::try_from(lo.col.0.saturating_add(1)).unwrap_or(0);

let dest = this.force_allocation(dest)?;
if let ty::Adt(adt, _) = dest.layout.ty.kind() {
if !adt.repr().c() {
throw_ub_format!(
Expand All @@ -191,29 +190,29 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
let filename_alloc =
this.allocate_str(&filename, MiriMemoryKind::Rust.into(), Mutability::Mut)?;

this.write_immediate(name_alloc.to_ref(this), &this.project_field(&dest, 0)?)?;
this.write_immediate(filename_alloc.to_ref(this), &this.project_field(&dest, 1)?)?;
this.write_immediate(name_alloc.to_ref(this), &this.project_field(dest, 0)?)?;
this.write_immediate(filename_alloc.to_ref(this), &this.project_field(dest, 1)?)?;
}
1 => {
this.write_scalar(
Scalar::from_target_usize(name.len().try_into().unwrap(), this),
&this.project_field(&dest, 0)?,
&this.project_field(dest, 0)?,
)?;
this.write_scalar(
Scalar::from_target_usize(filename.len().try_into().unwrap(), this),
&this.project_field(&dest, 1)?,
&this.project_field(dest, 1)?,
)?;
}
_ => throw_unsup_format!("unknown `miri_resolve_frame` flags {}", flags),
}

this.write_scalar(Scalar::from_u32(lineno), &this.project_field(&dest, 2)?)?;
this.write_scalar(Scalar::from_u32(colno), &this.project_field(&dest, 3)?)?;
this.write_scalar(Scalar::from_u32(lineno), &this.project_field(dest, 2)?)?;
this.write_scalar(Scalar::from_u32(colno), &this.project_field(dest, 3)?)?;

// Support a 4-field struct for now - this is deprecated
// and slated for removal.
if num_fields == 5 {
this.write_pointer(fn_ptr, &this.project_field(&dest, 4)?)?;
this.write_pointer(fn_ptr, &this.project_field(dest, 4)?)?;
}

Ok(())
Expand Down
4 changes: 2 additions & 2 deletions src/tools/miri/src/shims/ffi_support.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
fn call_external_c_and_store_return<'a>(
&mut self,
link_name: Symbol,
dest: &PlaceTy<'tcx, Provenance>,
dest: &MPlaceTy<'tcx, Provenance>,
ptr: CodePtr,
libffi_args: Vec<libffi::high::Arg<'a>>,
) -> InterpResult<'tcx, ()> {
Expand Down Expand Up @@ -205,7 +205,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
fn call_external_c_fct(
&mut self,
link_name: Symbol,
dest: &PlaceTy<'tcx, Provenance>,
dest: &MPlaceTy<'tcx, Provenance>,
args: &[OpTy<'tcx, Provenance>],
) -> InterpResult<'tcx, bool> {
// Get the pointer to the function in the shared object file if it exists.
Expand Down
Loading

0 comments on commit 52f8aec

Please sign in to comment.