Skip to content

interpret: get rid of MemPlaceMeta::Poison #99013

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 15, 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
1 change: 1 addition & 0 deletions compiler/rustc_const_eval/src/const_eval/eval_queries.rs
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ pub(super) fn op_to_const<'tcx>(
Ok(ref mplace) => to_const_value(mplace),
// see comment on `let try_as_immediate` above
Err(imm) => match *imm {
_ if imm.layout.is_zst() => ConstValue::ZeroSized,
Immediate::Scalar(x) => match x {
ScalarMaybeUninit::Scalar(s) => ConstValue::Scalar(s),
ScalarMaybeUninit::Uninit => to_const_value(&op.assert_mem_place()),
Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_const_eval/src/const_eval/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,6 @@ pub(crate) fn deref_mir_constant<'tcx>(

let ty = match mplace.meta {
MemPlaceMeta::None => mplace.layout.ty,
MemPlaceMeta::Poison => bug!("poison metadata in `deref_mir_constant`: {:#?}", mplace),
// In case of unsized types, figure out the real type behind.
MemPlaceMeta::Meta(scalar) => match mplace.layout.ty.kind() {
ty::Str => bug!("there's no sized equivalent of a `str`"),
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_const_eval/src/interpret/eval_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -987,7 +987,7 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> std::fmt::Debug
" by {} ref {:?}:",
match mplace.meta {
MemPlaceMeta::Meta(meta) => format!(" meta({:?})", meta),
MemPlaceMeta::Poison | MemPlaceMeta::None => String::new(),
MemPlaceMeta::None => String::new(),
},
mplace.ptr,
)?;
Expand Down
25 changes: 9 additions & 16 deletions compiler/rustc_const_eval/src/interpret/place.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,6 @@ pub enum MemPlaceMeta<Tag: Provenance = AllocId> {
Meta(Scalar<Tag>),
/// `Sized` types or unsized `extern type`
None,
/// The address of this place may not be taken. This protects the `MemPlace` from coming from
/// a ZST Operand without a backing allocation and being converted to an integer address. This
/// should be impossible, because you can't take the address of an operand, but this is a second
/// protection layer ensuring that we don't mess up.
Poison,
}

#[cfg(all(target_arch = "x86_64", target_pointer_width = "64"))]
Expand All @@ -38,15 +33,16 @@ impl<Tag: Provenance> MemPlaceMeta<Tag> {
pub fn unwrap_meta(self) -> Scalar<Tag> {
match self {
Self::Meta(s) => s,
Self::None | Self::Poison => {
Self::None => {
bug!("expected wide pointer extra data (e.g. slice length or trait object vtable)")
}
}
}

pub fn has_meta(self) -> bool {
match self {
Self::Meta(_) => true,
Self::None | Self::Poison => false,
Self::None => false,
}
}
}
Expand Down Expand Up @@ -163,10 +159,6 @@ impl<Tag: Provenance> MemPlace<Tag> {
MemPlaceMeta::Meta(meta) => {
Immediate::ScalarPair(Scalar::from_maybe_pointer(self.ptr, cx).into(), meta.into())
}
MemPlaceMeta::Poison => bug!(
"MPlaceTy::dangling may never be used to produce a \
place that will have the address of its pointee taken"
),
}
}

Expand Down Expand Up @@ -195,13 +187,15 @@ impl<Tag: Provenance> Place<Tag> {
}

impl<'tcx, Tag: Provenance> MPlaceTy<'tcx, Tag> {
/// Produces a MemPlace that works for ZST but nothing else
/// Produces a MemPlace that works for ZST but nothing else.
/// Conceptually this is a new allocation, but it doesn't actually create an allocation so you
/// don't need to worry about memory leaks.
#[inline]
pub fn dangling(layout: TyAndLayout<'tcx>) -> Self {
pub fn fake_alloc_zst(layout: TyAndLayout<'tcx>) -> Self {
assert!(layout.is_zst());
let align = layout.align.abi;
let ptr = Pointer::from_addr(align.bytes()); // no provenance, absolute address
// `Poison` this to make sure that the pointer value `ptr` is never observable by the program.
MPlaceTy { mplace: MemPlace { ptr, meta: MemPlaceMeta::Poison }, layout, align }
MPlaceTy { mplace: MemPlace { ptr, meta: MemPlaceMeta::None }, layout, align }
}

#[inline]
Expand Down Expand Up @@ -273,7 +267,6 @@ impl<'tcx, Tag: Provenance> OpTy<'tcx, Tag> {
Operand::Indirect(mplace) => {
Ok(MPlaceTy { mplace, layout: self.layout, align: self.align.unwrap() })
}
Operand::Immediate(_) if self.layout.is_zst() => Ok(MPlaceTy::dangling(self.layout)),
Copy link
Member Author

Choose a reason for hiding this comment

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

This was the main motivation for poisoning: we'd come up with a place (that has an address) for operands that don't have an address.

All other uses of MPlaceTy::dangling are fine, they are actually conceptually properly creating an allocation. I was looking for good ways to reflect that in its name and failed at that. MPlaceTy::alloc_zst sounds like it actually creates an allocation. Maybe fake_alloc_zst or so?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe fake_alloc_zst or so?

I like this idea. dangling has a lot of meaning attached to it that doesn't immediately convey what we mean here.

Copy link
Member Author

Choose a reason for hiding this comment

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

All right, done that.

Operand::Immediate(imm) => Err(ImmTy::from_immediate(imm, self.layout)),
}
}
Expand Down
6 changes: 2 additions & 4 deletions compiler/rustc_const_eval/src/interpret/terminator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -617,16 +617,14 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
place.to_ref(self),
self.layout_of(self.tcx.mk_mut_ptr(place.layout.ty))?,
);

let ty = self.tcx.mk_unit(); // return type is ()
let dest = MPlaceTy::dangling(self.layout_of(ty)?);
let ret = MPlaceTy::fake_alloc_zst(self.layout_of(self.tcx.types.unit)?);

self.eval_fn_call(
FnVal::Instance(instance),
(Abi::Rust, fn_abi),
&[arg.into()],
false,
&dest.into(),
&ret.into(),
Some(target),
match unwind {
Some(cleanup) => StackPopUnwind::Cleanup(cleanup),
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_middle/src/mir/interpret/allocation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,8 @@ impl<Tag> Allocation<Tag> {

/// Try to create an Allocation of `size` bytes, failing if there is not enough memory
/// available to the compiler to do so.
///
/// If `panic_on_fail` is true, this will never return `Err`.
pub fn uninit<'tcx>(size: Size, align: Align, panic_on_fail: bool) -> InterpResult<'tcx, Self> {
let bytes = Box::<[u8]>::try_new_zeroed_slice(size.bytes_usize()).map_err(|_| {
// This results in an error that can happen non-deterministically, since the memory
Expand Down