Skip to content

interpret: make some large types not Copy #99309

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 6 additions & 38 deletions compiler/rustc_const_eval/src/interpret/eval_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,8 @@ use std::cell::Cell;
use std::fmt;
use std::mem;

use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
use rustc_hir::{self as hir, def_id::DefId, definitions::DefPathData};
use rustc_index::vec::IndexVec;
use rustc_macros::HashStable;
use rustc_middle::mir;
use rustc_middle::mir::interpret::{InterpError, InvalidProgramInfo};
use rustc_middle::ty::layout::{
Expand All @@ -16,7 +14,6 @@ use rustc_middle::ty::{
self, query::TyCtxtAt, subst::SubstsRef, ParamEnv, Ty, TyCtxt, TypeFoldable,
};
use rustc_mir_dataflow::storage::always_storage_live_locals;
use rustc_query_system::ich::StableHashingContext;
use rustc_session::Limit;
use rustc_span::{Pos, Span};
use rustc_target::abi::{call::FnAbi, Align, HasDataLayout, Size, TargetDataLayout};
Expand Down Expand Up @@ -142,7 +139,7 @@ pub struct FrameInfo<'tcx> {
}

/// Unwind information.
#[derive(Clone, Copy, Eq, PartialEq, Debug, HashStable)]
#[derive(Clone, Copy, Eq, PartialEq, Debug)]
pub enum StackPopUnwind {
/// The cleanup block.
Cleanup(mir::BasicBlock),
Expand All @@ -152,7 +149,7 @@ pub enum StackPopUnwind {
NotAllowed,
}

#[derive(Clone, Copy, Eq, PartialEq, Debug, HashStable)] // Miri debug-prints these
#[derive(Clone, Copy, Eq, PartialEq, Debug)] // Miri debug-prints these
pub enum StackPopCleanup {
/// Jump to the next block in the caller, or cause UB if None (that's a function
/// that may never return). Also store layout of return place so
Expand All @@ -168,16 +165,15 @@ pub enum StackPopCleanup {
}

/// State of a local variable including a memoized layout
#[derive(Clone, Debug, PartialEq, Eq, HashStable)]
#[derive(Clone, Debug)]
pub struct LocalState<'tcx, Tag: Provenance = AllocId> {
pub value: LocalValue<Tag>,
/// Don't modify if `Some`, this is only used to prevent computing the layout twice
#[stable_hasher(ignore)]
pub layout: Cell<Option<TyAndLayout<'tcx>>>,
}

/// Current value of a local variable
#[derive(Copy, Clone, PartialEq, Eq, HashStable, Debug)] // Miri debug-prints these
#[derive(Copy, Clone, Debug)] // Miri debug-prints these
pub enum LocalValue<Tag: Provenance = AllocId> {
/// This local is not currently alive, and cannot be used at all.
Dead,
Expand Down Expand Up @@ -678,7 +674,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
body,
loc: Err(body.span), // Span used for errors caused during preamble.
return_to_block,
return_place: *return_place,
return_place: return_place.clone(),
// empty local array, we fill it in below, after we are inside the stack frame and
// all methods actually know about the frame
locals: IndexVec::new(),
Expand Down Expand Up @@ -799,7 +795,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
let op = self
.local_to_op(self.frame(), mir::RETURN_PLACE, None)
.expect("return place should always be live");
let dest = self.frame().return_place;
let dest = self.frame().return_place.clone();
let err = self.copy_op(&op, &dest, /*allow_transmute*/ true);
trace!("return value: {:?}", self.dump_place(*dest));
// We delay actually short-circuiting on this error until *after* the stack frame is
Expand Down Expand Up @@ -1021,31 +1017,3 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> std::fmt::Debug
}
}
}

impl<'ctx, 'mir, 'tcx, Tag: Provenance, Extra> HashStable<StableHashingContext<'ctx>>
for Frame<'mir, 'tcx, Tag, Extra>
where
Extra: HashStable<StableHashingContext<'ctx>>,
Tag: HashStable<StableHashingContext<'ctx>>,
{
fn hash_stable(&self, hcx: &mut StableHashingContext<'ctx>, hasher: &mut StableHasher) {
// Exhaustive match on fields to make sure we forget no field.
let Frame {
body,
instance,
return_to_block,
return_place,
locals,
loc,
extra,
tracing_span: _,
} = self;
body.hash_stable(hcx, hasher);
instance.hash_stable(hcx, hasher);
return_to_block.hash_stable(hcx, hasher);
return_place.hash_stable(hcx, hasher);
locals.hash_stable(hcx, hasher);
loc.hash_stable(hcx, hasher);
extra.hash_stable(hcx, hasher);
}
}
7 changes: 5 additions & 2 deletions compiler/rustc_const_eval/src/interpret/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -457,8 +457,11 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {

for i in 0..dest_len {
let place = self.mplace_index(&dest, i)?;
let value =
if i == index { *elem } else { self.mplace_index(&input, i)?.into() };
let value = if i == index {
elem.clone()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not going to matter much in the grand scheme of things by saving one (1) clone per call to this intrinsic, but this one should be easy to remove by calling copy_op in the branches ^^

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I considered this and decided, for the reasons you mentioned, that reducing code duplication is more important.

} else {
self.mplace_index(&input, i)?.into()
};
self.copy_op(&value, &place.into(), /*allow_transmute*/ false)?;
}
}
Expand Down
12 changes: 7 additions & 5 deletions compiler/rustc_const_eval/src/interpret/operand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
use std::fmt::Write;

use rustc_hir::def::Namespace;
use rustc_macros::HashStable;
use rustc_middle::ty::layout::{LayoutOf, PrimitiveExt, TyAndLayout};
use rustc_middle::ty::print::{FmtPrinter, PrettyPrinter, Printer};
use rustc_middle::ty::{ConstInt, DelaySpanBugEmitted, Ty};
Expand All @@ -25,7 +24,7 @@ use super::{
/// operations and wide pointers. This idea was taken from rustc's codegen.
/// In particular, thanks to `ScalarPair`, arithmetic operations and casts can be entirely
/// defined on `Immediate`, and do not have to work with a `Place`.
#[derive(Copy, Clone, PartialEq, Eq, HashStable, Hash, Debug)]
#[derive(Copy, Clone, Debug)]
pub enum Immediate<Tag: Provenance = AllocId> {
/// A single scalar value (must have *initialized* `Scalar` ABI).
/// FIXME: we also currently often use this for ZST.
Expand Down Expand Up @@ -112,7 +111,7 @@ impl<'tcx, Tag: Provenance> Immediate<Tag> {

// ScalarPair needs a type to interpret, so we often have an immediate and a type together
// as input for binary and cast operations.
#[derive(Copy, Clone, Debug)]
#[derive(Clone, Debug)]
pub struct ImmTy<'tcx, Tag: Provenance = AllocId> {
imm: Immediate<Tag>,
pub layout: TyAndLayout<'tcx>,
Expand Down Expand Up @@ -182,13 +181,16 @@ impl<'tcx, Tag: Provenance> std::ops::Deref for ImmTy<'tcx, Tag> {
/// An `Operand` is the result of computing a `mir::Operand`. It can be immediate,
/// or still in memory. The latter is an optimization, to delay reading that chunk of
/// memory and to avoid having to store arbitrary-sized data here.
#[derive(Copy, Clone, PartialEq, Eq, HashStable, Hash, Debug)]
#[derive(Copy, Clone, Debug)]
pub enum Operand<Tag: Provenance = AllocId> {
Immediate(Immediate<Tag>),
Indirect(MemPlace<Tag>),
}

#[derive(Copy, Clone, PartialEq, Eq, Hash, Debug)]
#[cfg(all(target_arch = "x86_64", target_pointer_width = "64"))]
rustc_data_structures::static_assert_size!(Operand, 64);

#[derive(Clone, Debug)]
pub struct OpTy<'tcx, Tag: Provenance = AllocId> {
op: Operand<Tag>, // Keep this private; it helps enforce invariants.
pub layout: TyAndLayout<'tcx>,
Expand Down
39 changes: 19 additions & 20 deletions compiler/rustc_const_eval/src/interpret/place.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
use std::hash::Hash;

use rustc_ast::Mutability;
use rustc_macros::HashStable;
use rustc_middle::mir;
use rustc_middle::ty;
use rustc_middle::ty::layout::{LayoutOf, PrimitiveExt, TyAndLayout};
Expand All @@ -17,7 +16,7 @@ use super::{
Pointer, Provenance, Scalar, ScalarMaybeUninit,
};

#[derive(Copy, Clone, Hash, PartialEq, Eq, HashStable, Debug)]
#[derive(Copy, Clone, Hash, PartialEq, Eq, Debug)]
/// Information required for the sound usage of a `MemPlace`.
pub enum MemPlaceMeta<Tag: Provenance = AllocId> {
/// The unsized payload (e.g. length for slices or vtable pointer for trait objects).
Expand Down Expand Up @@ -47,7 +46,7 @@ impl<Tag: Provenance> MemPlaceMeta<Tag> {
}
}

#[derive(Copy, Clone, Hash, PartialEq, Eq, HashStable, Debug)]
#[derive(Copy, Clone, Hash, PartialEq, Eq, Debug)]
pub struct MemPlace<Tag: Provenance = AllocId> {
/// The pointer can be a pure integer, with the `None` tag.
pub ptr: Pointer<Option<Tag>>,
Expand All @@ -60,7 +59,22 @@ pub struct MemPlace<Tag: Provenance = AllocId> {
#[cfg(all(target_arch = "x86_64", target_pointer_width = "64"))]
rustc_data_structures::static_assert_size!(MemPlace, 40);

#[derive(Copy, Clone, Hash, PartialEq, Eq, HashStable, Debug)]
/// A MemPlace with its layout. Constructing it is only possible in this module.
#[derive(Copy, Clone, Hash, Eq, PartialEq, Debug)]
pub struct MPlaceTy<'tcx, Tag: Provenance = AllocId> {
mplace: MemPlace<Tag>,
pub layout: TyAndLayout<'tcx>,
/// rustc does not have a proper way to represent the type of a field of a `repr(packed)` struct:
/// it needs to have a different alignment than the field type would usually have.
/// So we represent this here with a separate field that "overwrites" `layout.align`.
/// This means `layout.align` should never be used for a `MPlaceTy`!
pub align: Align,
}

#[cfg(all(target_arch = "x86_64", target_pointer_width = "64"))]
rustc_data_structures::static_assert_size!(MPlaceTy<'_>, 64);

#[derive(Copy, Clone, Debug)]
pub enum Place<Tag: Provenance = AllocId> {
/// A place referring to a value allocated in the `Memory` system.
Ptr(MemPlace<Tag>),
Expand All @@ -73,7 +87,7 @@ pub enum Place<Tag: Provenance = AllocId> {
#[cfg(all(target_arch = "x86_64", target_pointer_width = "64"))]
rustc_data_structures::static_assert_size!(Place, 48);

#[derive(Copy, Clone, Debug)]
#[derive(Clone, Debug)]
pub struct PlaceTy<'tcx, Tag: Provenance = AllocId> {
place: Place<Tag>, // Keep this private; it helps enforce invariants.
pub layout: TyAndLayout<'tcx>,
Expand All @@ -95,21 +109,6 @@ impl<'tcx, Tag: Provenance> std::ops::Deref for PlaceTy<'tcx, Tag> {
}
}

/// A MemPlace with its layout. Constructing it is only possible in this module.
#[derive(Copy, Clone, Hash, Eq, PartialEq, Debug)]
pub struct MPlaceTy<'tcx, Tag: Provenance = AllocId> {
mplace: MemPlace<Tag>,
pub layout: TyAndLayout<'tcx>,
/// rustc does not have a proper way to represent the type of a field of a `repr(packed)` struct:
/// it needs to have a different alignment than the field type would usually have.
/// So we represent this here with a separate field that "overwrites" `layout.align`.
/// This means `layout.align` should never be used for a `MPlaceTy`!
pub align: Align,
}

#[cfg(all(target_arch = "x86_64", target_pointer_width = "64"))]
rustc_data_structures::static_assert_size!(MPlaceTy<'_>, 64);

impl<'tcx, Tag: Provenance> std::ops::Deref for MPlaceTy<'tcx, Tag> {
type Target = MemPlace<Tag>;
#[inline(always)]
Expand Down
8 changes: 4 additions & 4 deletions compiler/rustc_const_eval/src/interpret/projection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ where
variant: VariantIdx,
) -> InterpResult<'tcx, PlaceTy<'tcx, M::PointerTag>> {
// Downcast just changes the layout
let mut base = *base;
let mut base = base.clone();
base.layout = base.layout.for_variant(self, variant);
Ok(base)
}
Expand All @@ -168,7 +168,7 @@ where
variant: VariantIdx,
) -> InterpResult<'tcx, OpTy<'tcx, M::PointerTag>> {
// Downcast just changes the layout
let mut base = *base;
let mut base = base.clone();
base.layout = base.layout.for_variant(self, variant);
Ok(base)
}
Expand Down Expand Up @@ -350,7 +350,7 @@ where
use rustc_middle::mir::ProjectionElem::*;
Ok(match proj_elem {
OpaqueCast(ty) => {
let mut place = *base;
let mut place = base.clone();
place.layout = self.layout_of(ty)?;
place
}
Expand Down Expand Up @@ -379,7 +379,7 @@ where
use rustc_middle::mir::ProjectionElem::*;
Ok(match proj_elem {
OpaqueCast(ty) => {
let mut op = *base;
let mut op = base.clone();
op.layout = self.layout_of(ty)?;
op
}
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_const_eval/src/interpret/terminator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -444,7 +444,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
trace!("eval_fn_call: Will pass last argument by untupling");
Cow::from(
args.iter()
.map(|&a| Ok(a))
.map(|a| Ok(a.clone()))
.chain(
(0..untuple_arg.layout.fields.count())
.map(|i| self.operand_field(untuple_arg, i)),
Expand Down Expand Up @@ -525,7 +525,7 @@ 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];
let mut receiver = args[0].clone();
let receiver_place = loop {
match receiver.layout.ty.kind() {
ty::Ref(..) | ty::RawPtr(..) => break self.deref_operand(&receiver)?,
Expand Down
14 changes: 7 additions & 7 deletions compiler/rustc_const_eval/src/interpret/visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>>: Copy {
pub trait Value<'mir, 'tcx, M: Machine<'mir, 'tcx>>: Sized {
/// Gets this value's layout.
fn layout(&self) -> TyAndLayout<'tcx>;

Expand Down Expand Up @@ -54,7 +54,7 @@ pub trait Value<'mir, 'tcx, M: Machine<'mir, 'tcx>>: Copy {
/// 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>>: Copy {
pub trait ValueMut<'mir, 'tcx, M: Machine<'mir, 'tcx>>: Sized {
/// Gets this value's layout.
fn layout(&self) -> TyAndLayout<'tcx>;

Expand Down Expand Up @@ -106,12 +106,12 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> Value<'mir, 'tcx, M> for OpTy<'tc
&self,
_ecx: &InterpCx<'mir, 'tcx, M>,
) -> InterpResult<'tcx, OpTy<'tcx, M::PointerTag>> {
Ok(*self)
Ok(self.clone())
}

#[inline(always)]
fn from_op(op: &OpTy<'tcx, M::PointerTag>) -> Self {
*op
op.clone()
}

#[inline(always)]
Expand Down Expand Up @@ -146,20 +146,20 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueMut<'mir, 'tcx, M>
&self,
_ecx: &InterpCx<'mir, 'tcx, M>,
) -> InterpResult<'tcx, OpTy<'tcx, M::PointerTag>> {
Ok(*self)
Ok(self.clone())
}

#[inline(always)]
fn to_op_for_proj(
&self,
_ecx: &mut InterpCx<'mir, 'tcx, M>,
) -> InterpResult<'tcx, OpTy<'tcx, M::PointerTag>> {
Ok(*self)
Ok(self.clone())
}

#[inline(always)]
fn from_op(op: &OpTy<'tcx, M::PointerTag>) -> Self {
*op
op.clone()
}

#[inline(always)]
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir_transform/src/const_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -516,7 +516,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
let l = self.use_ecx(|this| this.ecx.read_immediate(&this.ecx.eval_operand(left, None)?));
// Check for exceeding shifts *even if* we cannot evaluate the LHS.
if op == BinOp::Shr || op == BinOp::Shl {
let r = r?;
let r = r.clone()?;
// We need the type of the LHS. We cannot use `place_layout` as that is the type
// of the result, which for checked binops is not the same!
let left_ty = left.ty(self.local_decls, self.tcx);
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_mir_transform/src/const_prop_lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -584,7 +584,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
});
// Check for exceeding shifts *even if* we cannot evaluate the LHS.
if op == BinOp::Shr || op == BinOp::Shl {
let r = r?;
let r = r.clone()?;
// We need the type of the LHS. We cannot use `place_layout` as that is the type
// of the result, which for checked binops is not the same!
let left_ty = left.ty(self.local_decls, self.tcx);
Expand Down Expand Up @@ -616,10 +616,10 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
}
}

if let (Some(l), Some(r)) = (&l, &r) {
if let (Some(l), Some(r)) = (l, r) {
// The remaining operators are handled through `overflowing_binary_op`.
if self.use_ecx(source_info, |this| {
let (_res, overflow, _ty) = this.ecx.overflowing_binary_op(op, l, r)?;
let (_res, overflow, _ty) = this.ecx.overflowing_binary_op(op, &l, &r)?;
Ok(overflow)
})? {
self.report_assert_as_lint(
Expand Down