dnrops
/
rust
mirror of https://github.com/rust-lang/rust.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Auto merge of #122243 - RalfJung:local-place-sanity-check, r=oli-obk 

interpret: ensure that Place is never used for a different frame

We store the address where the stack frame stores its `locals`. The idea is that even if we pop and push, or switch to a different thread with a larger number of frames, then the `locals` address will most likely change so we'll notice that problem. This is made possible by some recent changes by `@WaffleLapkin,` where we no longer use `Place` across things that change the number of stack frames.

I made these debug assertions for now, just to make sure this can't cost us any perf.

The first commit is unrelated but it's a one-line comment change so it didn't warrant a separate PR...

r? `@oli-obk`
This commit is contained in:
bors 2024-03-14 07:28:07 +00:00
parent 6f3eb1ce3d c3342b41b5
commit cb580ff677
10 changed files with 62 additions and 74 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

22
compiler/rustc_const_eval/src/interpret/eval_context.rs
View File

@ -220,9 +220,6 @@ impl<'tcx, Prov: Provenance> LocalState<'tcx, Prov> {
/// Overwrite the local. If the local can be overwritten in place, return a reference
/// to do so; otherwise return the `MemPlace` to consult instead.
///
/// Note: Before calling this, call the `before_access_local_mut` machine hook! You may be
/// invalidating machine invariants otherwise!
#[inline(always)]
pub(super) fn access_mut(&mut self) -> InterpResult<'tcx, &mut Operand<Prov>> {
match &mut self.value {
@ -279,6 +276,13 @@ impl<'mir, 'tcx, Prov: Provenance, Extra> Frame<'mir, 'tcx, Prov, Extra> {
}
})
}
/// Returns the address of the buffer where the locals are stored. This is used by `Place` as a
/// sanity check to detect bugs where we mix up which stack frame a place refers to.
#[inline(always)]
pub(super) fn locals_addr(&self) -> usize {
self.locals.raw.as_ptr().addr()
}
}
// FIXME: only used by miri, should be removed once translatable.
@ -645,7 +649,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
}
#[inline(always)]
pub fn layout_of_local(
pub(super) fn layout_of_local(
&self,
frame: &Frame<'mir, 'tcx, M::Provenance, M::FrameExtra>,
local: mir::Local,
@ -896,7 +900,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
// Copy return value. Must of course happen *before* we deallocate the locals.
let copy_ret_result = if !unwinding {
let op = self
.local_to_op(self.frame(), mir::RETURN_PLACE, None)
.local_to_op(mir::RETURN_PLACE, None)
.expect("return place should always be live");
let dest = self.frame().return_place.clone();
let err = if self.stack().len() == 1 {
@ -1212,18 +1216,16 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> std::fmt::Debug
{
fn fmt(&self, fmt: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self.place {
Place::Local { frame, local, offset } => {
Place::Local { local, offset, locals_addr } => {
debug_assert_eq!(locals_addr, self.ecx.frame().locals_addr());
let mut allocs = Vec::new();
write!(fmt, "{local:?}")?;
if let Some(offset) = offset {
write!(fmt, "+{:#x}", offset.bytes())?;
}
if frame != self.ecx.frame_idx() {
write!(fmt, " ({} frames up)", self.ecx.frame_idx() - frame)?;
}
write!(fmt, ":")?;
match self.ecx.stack()[frame].locals[local].value {
match self.ecx.frame().locals[local].value {
LocalValue::Dead => write!(fmt, " is dead")?,
LocalValue::Live(Operand::Immediate(Immediate::Uninit)) => {
write!(fmt, " is uninitialized")?

19
compiler/rustc_const_eval/src/interpret/machine.rs
View File

@ -260,24 +260,6 @@ pub trait Machine<'mir, 'tcx: 'mir>: Sized {
F2::NAN
}
/// Called before writing the specified `local` of the `frame`.
/// Since writing a ZST is not actually accessing memory or locals, this is never invoked
/// for ZST reads.
///
/// Due to borrow checker trouble, we indicate the `frame` as an index rather than an `&mut
/// Frame`.
#[inline(always)]
fn before_access_local_mut<'a>(
_ecx: &'a mut InterpCx<'mir, 'tcx, Self>,
_frame: usize,
_local: mir::Local,
) -> InterpResult<'tcx>
where
'tcx: 'mir,
{
Ok(())
}
/// Called before a basic block terminator is executed.
#[inline]
fn before_terminator(_ecx: &mut InterpCx<'mir, 'tcx, Self>) -> InterpResult<'tcx> {
@ -531,7 +513,6 @@ pub trait Machine<'mir, 'tcx: 'mir>: Sized {
#[inline(always)]
fn after_local_allocated(
_ecx: &mut InterpCx<'mir, 'tcx, Self>,
_frame: usize,
_local: mir::Local,
_mplace: &MPlaceTy<'tcx, Self::Provenance>,
) -> InterpResult<'tcx> {

17
compiler/rustc_const_eval/src/interpret/operand.rs
View File

@ -13,9 +13,9 @@ use rustc_middle::{mir, ty};
use rustc_target::abi::{self, Abi, HasDataLayout, Size};
use super::{
alloc_range, from_known_layout, mir_assign_valid_types, CtfeProvenance, Frame, InterpCx,
InterpResult, MPlaceTy, Machine, MemPlace, MemPlaceMeta, OffsetMode, PlaceTy, Pointer,
Projectable, Provenance, Scalar,
alloc_range, from_known_layout, mir_assign_valid_types, CtfeProvenance, InterpCx, InterpResult,
MPlaceTy, Machine, MemPlace, MemPlaceMeta, OffsetMode, PlaceTy, Pointer, Projectable,
Provenance, Scalar,
};
/// An `Immediate` represents a single immediate self-contained Rust value.
@ -633,17 +633,17 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
}
}
/// Read from a local.
/// Read from a local of the current frame.
/// Will not access memory, instead an indirect `Operand` is returned.
///
/// This is public because it is used by [priroda](https://github.com/oli-obk/priroda) to get an
/// OpTy from a local.
pub fn local_to_op(
&self,
frame: &Frame<'mir, 'tcx, M::Provenance, M::FrameExtra>,
local: mir::Local,
layout: Option<TyAndLayout<'tcx>>,
) -> InterpResult<'tcx, OpTy<'tcx, M::Provenance>> {
let frame = self.frame();
let layout = self.layout_of_local(frame, local, layout)?;
let op = *frame.locals[local].access()?;
if matches!(op, Operand::Immediate(_)) {
@ -661,9 +661,10 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
) -> InterpResult<'tcx, OpTy<'tcx, M::Provenance>> {
match place.as_mplace_or_local() {
Left(mplace) => Ok(mplace.into()),
Right((frame, local, offset)) => {
Right((local, offset, locals_addr)) => {
debug_assert!(place.layout.is_sized()); // only sized locals can ever be `Place::Local`.
let base = self.local_to_op(&self.stack()[frame], local, None)?;
debug_assert_eq!(locals_addr, self.frame().locals_addr());
let base = self.local_to_op(local, None)?;
Ok(match offset {
Some(offset) => base.offset(offset, place.layout, self)?,
None => {
@ -687,7 +688,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
// here is not the entire place.
let layout = if mir_place.projection.is_empty() { layout } else { None };
let mut op = self.local_to_op(self.frame(), mir_place.local, layout)?;
let mut op = self.local_to_op(mir_place.local, layout)?;
// Using `try_fold` turned out to be bad for performance, hence the loop.
for elem in mir_place.projection.iter() {
op = self.project(&op, elem)?

66
compiler/rustc_const_eval/src/interpret/place.rs
View File

@ -187,11 +187,13 @@ pub(super) enum Place<Prov: Provenance = CtfeProvenance> {
/// where in the local this place is located; if it is `None`, no projection has been applied.
/// Such projections are meaningful even if the offset is 0, since they can change layouts.
/// (Without that optimization, we'd just always be a `MemPlace`.)
/// Note that this only stores the frame index, not the thread this frame belongs to -- that is
/// implicit. This means a `Place` must never be moved across interpreter thread boundaries!
/// `Local` places always refer to the current stack frame, so they are unstable under
/// function calls/returns and switching betweens stacks of different threads!
/// We carry around the address of the `locals` buffer of the correct stack frame as a sanity
/// chec to be able to catch some cases of using a dangling `Place`.
///
/// This variant shall not be used for unsized types -- those must always live in memory.
Local { frame: usize, local: mir::Local, offset: Option<Size> },
Local { local: mir::Local, offset: Option<Size>, locals_addr: usize },
}
/// An evaluated place, together with its type.
@ -233,10 +235,10 @@ impl<'tcx, Prov: Provenance> PlaceTy<'tcx, Prov> {
#[inline(always)]
pub fn as_mplace_or_local(
&self,
) -> Either<MPlaceTy<'tcx, Prov>, (usize, mir::Local, Option<Size>)> {
) -> Either<MPlaceTy<'tcx, Prov>, (mir::Local, Option<Size>, usize)> {
match self.place {
Place::Ptr(mplace) => Left(MPlaceTy { mplace, layout: self.layout }),
Place::Local { frame, local, offset } => Right((frame, local, offset)),
Place::Local { local, offset, locals_addr } => Right((local, offset, locals_addr)),
}
}
@ -279,7 +281,7 @@ impl<'tcx, Prov: Provenance> Projectable<'tcx, Prov> for PlaceTy<'tcx, Prov> {
) -> InterpResult<'tcx, Self> {
Ok(match self.as_mplace_or_local() {
Left(mplace) => mplace.offset_with_meta(offset, mode, meta, layout, ecx)?.into(),
Right((frame, local, old_offset)) => {
Right((local, old_offset, locals_addr)) => {
debug_assert!(layout.is_sized(), "unsized locals should live in memory");
assert_matches!(meta, MemPlaceMeta::None); // we couldn't store it anyway...
// `Place::Local` are always in-bounds of their surrounding local, so we can just
@ -292,7 +294,10 @@ impl<'tcx, Prov: Provenance> Projectable<'tcx, Prov> for PlaceTy<'tcx, Prov> {
.offset(old_offset.unwrap_or(Size::ZERO).bytes(), offset.bytes())?,
);
PlaceTy { place: Place::Local { frame, local, offset: Some(new_offset) }, layout }
PlaceTy {
place: Place::Local { local, offset: Some(new_offset), locals_addr },
layout,
}
}
})
}
@ -331,7 +336,7 @@ impl<'tcx, Prov: Provenance> OpTy<'tcx, Prov> {
pub trait Writeable<'tcx, Prov: Provenance>: Projectable<'tcx, Prov> {
fn as_mplace_or_local(
&self,
) -> Either<MPlaceTy<'tcx, Prov>, (usize, mir::Local, Option<Size>, TyAndLayout<'tcx>)>;
) -> Either<MPlaceTy<'tcx, Prov>, (mir::Local, Option<Size>, usize, TyAndLayout<'tcx>)>;
fn force_mplace<'mir, M: Machine<'mir, 'tcx, Provenance = Prov>>(
&self,
@ -343,9 +348,9 @@ impl<'tcx, Prov: Provenance> Writeable<'tcx, Prov> for PlaceTy<'tcx, Prov> {
#[inline(always)]
fn as_mplace_or_local(
&self,
) -> Either<MPlaceTy<'tcx, Prov>, (usize, mir::Local, Option<Size>, TyAndLayout<'tcx>)> {
) -> Either<MPlaceTy<'tcx, Prov>, (mir::Local, Option<Size>, usize, TyAndLayout<'tcx>)> {
self.as_mplace_or_local()
.map_right(|(frame, local, offset)| (frame, local, offset, self.layout))
.map_right(|(local, offset, locals_addr)| (local, offset, locals_addr, self.layout))
}
#[inline(always)]
@ -361,7 +366,7 @@ impl<'tcx, Prov: Provenance> Writeable<'tcx, Prov> for MPlaceTy<'tcx, Prov> {
#[inline(always)]
fn as_mplace_or_local(
&self,
) -> Either<MPlaceTy<'tcx, Prov>, (usize, mir::Local, Option<Size>, TyAndLayout<'tcx>)> {
) -> Either<MPlaceTy<'tcx, Prov>, (mir::Local, Option<Size>, usize, TyAndLayout<'tcx>)> {
Left(self.clone())
}
@ -501,21 +506,21 @@ where
Ok((mplace, len))
}
/// Turn a local in the current frame into a place.
pub fn local_to_place(
&self,
frame: usize,
local: mir::Local,
) -> InterpResult<'tcx, PlaceTy<'tcx, M::Provenance>> {
// Other parts of the system rely on `Place::Local` never being unsized.
// So we eagerly check here if this local has an MPlace, and if yes we use it.
let frame_ref = &self.stack()[frame];
let layout = self.layout_of_local(frame_ref, local, None)?;
let frame = self.frame();
let layout = self.layout_of_local(frame, local, None)?;
let place = if layout.is_sized() {
// We can just always use the `Local` for sized values.
Place::Local { frame, local, offset: None }
Place::Local { local, offset: None, locals_addr: frame.locals_addr() }
} else {
// Unsized `Local` isn't okay (we cannot store the metadata).
match frame_ref.locals[local].access()? {
match frame.locals[local].access()? {
Operand::Immediate(_) => bug!(),
Operand::Indirect(mplace) => Place::Ptr(*mplace),
}
@ -530,7 +535,7 @@ where
&self,
mir_place: mir::Place<'tcx>,
) -> InterpResult<'tcx, PlaceTy<'tcx, M::Provenance>> {
let mut place = self.local_to_place(self.frame_idx(), mir_place.local)?;
let mut place = self.local_to_place(mir_place.local)?;
// Using `try_fold` turned out to be bad for performance, hence the loop.
for elem in mir_place.projection.iter() {
place = self.project(&place, elem)?
@ -611,15 +616,15 @@ where
// See if we can avoid an allocation. This is the counterpart to `read_immediate_raw`,
// but not factored as a separate function.
let mplace = match dest.as_mplace_or_local() {
Right((frame, local, offset, layout)) => {
Right((local, offset, locals_addr, layout)) => {
if offset.is_some() {
// This has been projected to a part of this local. We could have complicated
// logic to still keep this local as an `Operand`... but it's much easier to
// just fall back to the indirect path.
dest.force_mplace(self)?
} else {
M::before_access_local_mut(self, frame, local)?;
match self.stack_mut()[frame].locals[local].access_mut()? {
debug_assert_eq!(locals_addr, self.frame().locals_addr());
match self.frame_mut().locals[local].access_mut()? {
Operand::Immediate(local_val) => {
// Local can be updated in-place.
*local_val = src;
@ -627,7 +632,7 @@ where
// (*After* doing the update for borrow checker reasons.)
if cfg!(debug_assertions) {
let local_layout =
self.layout_of_local(&self.stack()[frame], local, None)?;
self.layout_of_local(&self.frame(), local, None)?;
match (src, local_layout.abi) {
(Immediate::Scalar(scalar), Abi::Scalar(s)) => {
assert_eq!(scalar.size(), s.size(self))
@ -725,7 +730,7 @@ where
) -> InterpResult<'tcx> {
let mplace = match dest.as_mplace_or_local() {
Left(mplace) => mplace,
Right((frame, local, offset, layout)) => {
Right((local, offset, locals_addr, layout)) => {
if offset.is_some() {
// This has been projected to a part of this local. We could have complicated
// logic to still keep this local as an `Operand`... but it's much easier to
@ -733,8 +738,8 @@ where
// FIXME: share the logic with `write_immediate_no_validate`.
dest.force_mplace(self)?
} else {
M::before_access_local_mut(self, frame, local)?;
match self.stack_mut()[frame].locals[local].access_mut()? {
debug_assert_eq!(locals_addr, self.frame().locals_addr());
match self.frame_mut().locals[local].access_mut()? {
Operand::Immediate(local) => {
*local = Immediate::Uninit;
return Ok(());
@ -912,17 +917,16 @@ where
place: &PlaceTy<'tcx, M::Provenance>,
) -> InterpResult<'tcx, MPlaceTy<'tcx, M::Provenance>> {
let mplace = match place.place {
Place::Local { frame, local, offset } => {
M::before_access_local_mut(self, frame, local)?;
let whole_local = match self.stack_mut()[frame].locals[local].access_mut()? {
Place::Local { local, offset, locals_addr } => {
debug_assert_eq!(locals_addr, self.frame().locals_addr());
let whole_local = match self.frame_mut().locals[local].access_mut()? {
&mut Operand::Immediate(local_val) => {
// We need to make an allocation.
// We need the layout of the local. We can NOT use the layout we got,
// that might e.g., be an inner field of a struct with `Scalar` layout,
// that has different alignment than the outer field.
let local_layout =
self.layout_of_local(&self.stack()[frame], local, None)?;
let local_layout = self.layout_of_local(&self.frame(), local, None)?;
assert!(local_layout.is_sized(), "unsized locals cannot be immediate");
let mplace = self.allocate(local_layout, MemoryKind::Stack)?;
// Preserve old value. (As an optimization, we can skip this if it was uninit.)
@ -936,11 +940,11 @@ where
mplace.mplace,
)?;
}
M::after_local_allocated(self, frame, local, &mplace)?;
M::after_local_allocated(self, local, &mplace)?;
// Now we can call `access_mut` again, asserting it goes well, and actually
// overwrite things. This points to the entire allocation, not just the part
// the place refers to, i.e. we do this before we apply `offset`.
*self.stack_mut()[frame].locals[local].access_mut().unwrap() =
*self.frame_mut().locals[local].access_mut().unwrap() =
Operand::Indirect(mplace.mplace);
mplace.mplace
}

2
compiler/rustc_const_eval/src/interpret/projection.rs
View File

@ -357,7 +357,7 @@ where
Deref => self.deref_pointer(&base.to_op(self)?)?.into(),
Index(local) => {
let layout = self.layout_of(self.tcx.types.usize)?;
let n = self.local_to_op(self.frame(), local, Some(layout))?;
let n = self.local_to_op(local, Some(layout))?;
let n = self.read_target_usize(&n)?;
self.project_index(base, n)?
}

2
compiler/rustc_const_eval/src/interpret/terminator.rs
View File

@ -631,7 +631,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
body.args_iter()
.map(|local| (
local,
self.layout_of_local(self.frame(), local, None).unwrap().ty
self.layout_of_local(self.frame(), local, None).unwrap().ty,
))
.collect::<Vec<_>>()
);

1
compiler/rustc_const_eval/src/lib.rs
View File

@ -14,6 +14,7 @@ Rust MIR: a lowered representation of Rust.
#![feature(generic_nonzero)]
#![feature(let_chains)]
#![feature(slice_ptr_get)]
#![feature(strict_provenance)]
#![feature(never_type)]
#![feature(trait_alias)]
#![feature(try_blocks)]

2
compiler/rustc_mir_transform/src/add_retag.rs
View File

@ -118,7 +118,7 @@ impl<'tcx> MirPass<'tcx> for AddRetag {
}
// PART 3
// Add retag after assignments where data "enters" this function: the RHS is behind a deref and the LHS is not.
// Add retag after assignments.
for block_data in basic_blocks {
// We want to insert statements as we iterate. To this end, we
// iterate backwards using indices.

2
src/tools/miri/src/helpers.rs
View File

@ -413,7 +413,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
.ok_or_else(|| err_ub_format!("callee has fewer arguments than expected"))?;
// Make the local live, and insert the initial value.
this.storage_live(local)?;
let callee_arg = this.local_to_place(this.frame_idx(), local)?;
let callee_arg = this.local_to_place(local)?;
this.write_immediate(*arg, &callee_arg)?;
}
if callee_args.next().is_some() {

3
src/tools/miri/src/machine.rs
View File

@ -1488,14 +1488,13 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> {
fn after_local_allocated(
ecx: &mut InterpCx<'mir, 'tcx, Self>,
frame: usize,
local: mir::Local,
mplace: &MPlaceTy<'tcx, Provenance>,
) -> InterpResult<'tcx> {
let Some(Provenance::Concrete { alloc_id, .. }) = mplace.ptr().provenance else {
panic!("after_local_allocated should only be called on fresh allocations");
};
let local_decl = &ecx.active_thread_stack()[frame].body.local_decls[local];
let local_decl = &ecx.frame().body.local_decls[local];
let span = local_decl.source_info.span;
ecx.machine.allocation_spans.borrow_mut().insert(alloc_id, (span, None));
Ok(())