Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Miri casts: do not blindly rely on dest type #72525

Merged
merged 5 commits into from
May 25, 2020
Merged

Miri casts: do not blindly rely on dest type #72525

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fbc6f2c
make some cast helpers infallible
RalfJung Apr 14, 2020
b97ed1a
Miri casts: do not blindly rely on dest type
RalfJung May 24, 2020
b9b1554
Fix unsizing casts
RalfJung May 24, 2020
7b98552
use helper method for determining size of int type
RalfJung May 24, 2020
8b5ba4a
comment nit
RalfJung May 24, 2020
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
182 changes: 96 additions & 86 deletions src/librustc_mir/interpret/cast.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
use std::convert::TryFrom;

use super::{FnVal, ImmTy, Immediate, InterpCx, Machine, OpTy, PlaceTy};
use rustc_apfloat::ieee::{Double, Single};
use rustc_apfloat::{Float, FloatConvert};
use rustc_ast::ast::FloatTy;
Expand All @@ -12,25 +11,36 @@ use rustc_middle::ty::{self, Ty, TypeAndMut, TypeFoldable};
use rustc_span::symbol::sym;
use rustc_target::abi::{LayoutOf, Size, Variants};

use super::{truncate, FnVal, ImmTy, Immediate, InterpCx, Machine, OpTy, PlaceTy};

impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
pub fn cast(
&mut self,
src: OpTy<'tcx, M::PointerTag>,
kind: CastKind,
cast_kind: CastKind,
cast_ty: Ty<'tcx>,
dest: PlaceTy<'tcx, M::PointerTag>,
) -> InterpResult<'tcx> {
use rustc_middle::mir::CastKind::*;
match kind {
// FIXME: In which cases should we trigger UB when the source is uninit?
Copy link
Member

Choose a reason for hiding this comment

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

Hmm presumably the non-UB cases are at most those that slice/reuse bits? (so some pointer casts and integer truncations/zero-extensions)

Sign-extensions and any int <-> float casts should definitely be UB on uninit (and I'm guessing they are already?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sign-extensions and any int <-> float casts should definitely be UB on uninit (and I'm guessing they are already?)

Yeah anything that looks at the bits is already UB. But e.g. currently casting one raw ptr type to another would not be UB on uninitialized inputs. Maybe that's good, maybe not.

match cast_kind {
Pointer(PointerCast::Unsize) => {
self.unsize_into(src, dest)?;
let cast_ty = self.layout_of(cast_ty)?;
self.unsize_into(src, cast_ty, dest)?;
}

Misc | Pointer(PointerCast::MutToConstPointer | PointerCast::ArrayToPointer) => {
Misc => {
let src = self.read_immediate(src)?;
let res = self.cast_immediate(src, dest.layout)?;
let res = self.misc_cast(src, cast_ty)?;
self.write_immediate(res, dest)?;
}

Pointer(PointerCast::MutToConstPointer | PointerCast::ArrayToPointer) => {
Copy link
Member

Choose a reason for hiding this comment

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

ArrayToPointer is confusing, I'm guessing ArrayToElemPointer makes more sense (or should it be ArayRefToElemPointer? since a regular raw pointer cast shouldn't care about the pointee type changing).

Although PointerCast variants might not need the Pointer suffix. Anyway, this is not that important to this PR.

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 well I never understood most of these (and at some point whoever designed this clearly gave up and stuffed everything else into Misc).

// These are NOPs, but can be wide pointers.
Copy link
Member

Choose a reason for hiding this comment

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

Not important but I wonder if "noop" or "no-op" is more common.

let v = self.read_immediate(src)?;
self.write_immediate(*v, dest)?;
Comment on lines +41 to +42
Copy link
Member

Choose a reason for hiding this comment

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

Is this relying on the type/layout equality check being forgiving around raw pointers?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm... actually I am not sure I am happy with this. write_immediate cannot know the layout of the immediate so in particular when it is a ScalarPair it cannot know the offset of the 2nd field. We do check that both fields have the same size as what dest expects, but I am not sure if that is enough?

}

Pointer(PointerCast::ReifyFnPointer) => {
// The src operand does not matter, just its type
match src.layout.ty.kind {
Expand Down Expand Up @@ -61,12 +71,12 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {

Pointer(PointerCast::UnsafeFnPointer) => {
let src = self.read_immediate(src)?;
match dest.layout.ty.kind {
match cast_ty.kind {
ty::FnPtr(_) => {
// No change to value
self.write_immediate(*src, dest)?;
}
_ => bug!("fn to unsafe fn cast on {:?}", dest.layout.ty),
_ => bug!("fn to unsafe fn cast on {:?}", cast_ty),
}
}

Expand Down Expand Up @@ -95,25 +105,21 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
Ok(())
}

fn cast_immediate(
fn misc_cast(
&self,
src: ImmTy<'tcx, M::PointerTag>,
dest_layout: TyAndLayout<'tcx>,
cast_ty: Ty<'tcx>,
) -> InterpResult<'tcx, Immediate<M::PointerTag>> {
use rustc_middle::ty::TyKind::*;
trace!("Casting {:?}: {:?} to {:?}", *src, src.layout.ty, dest_layout.ty);
trace!("Casting {:?}: {:?} to {:?}", *src, src.layout.ty, cast_ty);

match src.layout.ty.kind {
// Floating point
Float(FloatTy::F32) => {
return Ok(self
.cast_from_float(src.to_scalar()?.to_f32()?, dest_layout.ty)?
.into());
return Ok(self.cast_from_float(src.to_scalar()?.to_f32()?, cast_ty).into());
}
Float(FloatTy::F64) => {
return Ok(self
.cast_from_float(src.to_scalar()?.to_f64()?, dest_layout.ty)?
.into());
return Ok(self.cast_from_float(src.to_scalar()?.to_f64()?, cast_ty).into());
}
// The rest is integer/pointer-"like", including fn ptr casts and casts from enums that
// are represented as integers.
Expand All @@ -128,95 +134,97 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
),
}

// # First handle non-scalar source values.

// Handle cast from a univariant (ZST) enum.
match src.layout.variants {
Variants::Single { index } => {
if let Some(discr) = src.layout.ty.discriminant_for_variant(*self.tcx, index) {
assert!(src.layout.is_zst());
let discr_layout = self.layout_of(discr.ty)?;
return Ok(self
.cast_from_int_like(discr.val, discr_layout, dest_layout)?
.into());
return Ok(self.cast_from_scalar(discr.val, discr_layout, cast_ty).into());
}
}
Variants::Multiple { .. } => {}
}

// Handle casting the metadata away from a fat pointer.
if src.layout.ty.is_unsafe_ptr()
&& dest_layout.ty.is_unsafe_ptr()
&& dest_layout.size != src.layout.size
{
assert_eq!(src.layout.size, 2 * self.memory.pointer_size());
assert_eq!(dest_layout.size, self.memory.pointer_size());
assert!(dest_layout.ty.is_unsafe_ptr());
match *src {
Immediate::ScalarPair(data, _) => return Ok(data.into()),
Immediate::Scalar(..) => bug!(
"{:?} input to a fat-to-thin cast ({:?} -> {:?})",
*src,
src.layout.ty,
dest_layout.ty
),
};
}

// Handle casting any ptr to raw ptr (might be a fat ptr).
if src.layout.ty.is_any_ptr() && dest_layout.ty.is_unsafe_ptr() {
// The only possible size-unequal case was handled above.
assert_eq!(src.layout.size, dest_layout.size);
return Ok(*src);
if src.layout.ty.is_any_ptr() && cast_ty.is_unsafe_ptr() {
let dest_layout = self.layout_of(cast_ty)?;
if dest_layout.size == src.layout.size {
// Thin or fat pointer that just hast the ptr kind of target type changed.
return Ok(*src);
} else {
// Casting the metadata away from a fat ptr.
assert_eq!(src.layout.size, 2 * self.memory.pointer_size());
assert_eq!(dest_layout.size, self.memory.pointer_size());
Comment on lines +160 to +161
Copy link
Member

Choose a reason for hiding this comment

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

Uhh, this is a bit hacky, but I guess pre-existing. Should probably rely on wide pointers having their first field be a simple pointer. So really just need to extract the first field and then cast it as usual.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well that's basically what we do except we also have a bunch of extra assertions?

assert!(src.layout.ty.is_unsafe_ptr());
return match *src {
Immediate::ScalarPair(data, _) => Ok(data.into()),
Immediate::Scalar(..) => bug!(
"{:?} input to a fat-to-thin cast ({:?} -> {:?})",
*src,
src.layout.ty,
cast_ty
),
};
}
}

// # The remaining source values are scalar.

// For all remaining casts, we either
// (a) cast a raw ptr to usize, or
// (b) cast from an integer-like (including bool, char, enums).
// In both cases we want the bits.
let bits = self.force_bits(src.to_scalar()?, src.layout.size)?;
Ok(self.cast_from_int_like(bits, src.layout, dest_layout)?.into())
Ok(self.cast_from_scalar(bits, src.layout, cast_ty).into())
}

fn cast_from_int_like(
pub(super) fn cast_from_scalar(
&self,
v: u128, // raw bits
v: u128, // raw bits (there is no ScalarTy so we separate data+layout)
src_layout: TyAndLayout<'tcx>,
dest_layout: TyAndLayout<'tcx>,
) -> InterpResult<'tcx, Scalar<M::PointerTag>> {
cast_ty: Ty<'tcx>,
) -> Scalar<M::PointerTag> {
// Let's make sure v is sign-extended *if* it has a signed type.
let signed = src_layout.abi.is_signed();
let signed = src_layout.abi.is_signed(); // also checks that abi is `Scalar`.
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
let v = if signed { self.sign_extend(v, src_layout) } else { v };
trace!("cast_from_int: {}, {}, {}", v, src_layout.ty, dest_layout.ty);
trace!("cast_from_scalar: {}, {} -> {}", v, src_layout.ty, cast_ty);
use rustc_middle::ty::TyKind::*;
match dest_layout.ty.kind {
match cast_ty.kind {
Int(_) | Uint(_) | RawPtr(_) => {
let v = self.truncate(v, dest_layout);
Ok(Scalar::from_uint(v, dest_layout.size))
let size = match cast_ty.kind {
// FIXME: Isn't there a helper for this? The same pattern occurs below.
Int(t) => {
t.bit_width().map(Size::from_bits).unwrap_or_else(|| self.pointer_size())
}
Uint(t) => {
t.bit_width().map(Size::from_bits).unwrap_or_else(|| self.pointer_size())
}
RawPtr(_) => self.pointer_size(),
_ => bug!(),
};
Copy link
Member

Choose a reason for hiding this comment

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

So much work to avoid calling layout_of 😁

Copy link
Member Author

Choose a reason for hiding this comment

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

I learned the hard way that layout_of is expensive...

let v = truncate(v, size);
Scalar::from_uint(v, size)
}

Float(FloatTy::F32) if signed => {
Ok(Scalar::from_f32(Single::from_i128(v as i128).value))
}
Float(FloatTy::F64) if signed => {
Ok(Scalar::from_f64(Double::from_i128(v as i128).value))
}
Float(FloatTy::F32) => Ok(Scalar::from_f32(Single::from_u128(v).value)),
Float(FloatTy::F64) => Ok(Scalar::from_f64(Double::from_u128(v).value)),
Float(FloatTy::F32) if signed => Scalar::from_f32(Single::from_i128(v as i128).value),
Float(FloatTy::F64) if signed => Scalar::from_f64(Double::from_i128(v as i128).value),
Float(FloatTy::F32) => Scalar::from_f32(Single::from_u128(v).value),
Float(FloatTy::F64) => Scalar::from_f64(Double::from_u128(v).value),

Char => {
// `u8` to `char` cast
Ok(Scalar::from_u32(u8::try_from(v).unwrap().into()))
Scalar::from_u32(u8::try_from(v).unwrap().into())
}

// Casts to bool are not permitted by rustc, no need to handle them here.
_ => bug!("invalid int to {:?} cast", dest_layout.ty),
_ => bug!("invalid int to {:?} cast", cast_ty),
}
}

fn cast_from_float<F>(
&self,
f: F,
dest_ty: Ty<'tcx>,
) -> InterpResult<'tcx, Scalar<M::PointerTag>>
fn cast_from_float<F>(&self, f: F, dest_ty: Ty<'tcx>) -> Scalar<M::PointerTag>
where
F: Float + Into<Scalar<M::PointerTag>> + FloatConvert<Single> + FloatConvert<Double>,
{
Expand All @@ -229,20 +237,20 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
// (https://doc.rust-lang.org/nightly/nightly-rustc/rustc_apfloat/trait.Float.html#method.to_i128_r).
let v = f.to_u128(usize::try_from(width).unwrap()).value;
// This should already fit the bit width
Ok(Scalar::from_uint(v, Size::from_bits(width)))
Scalar::from_uint(v, Size::from_bits(width))
}
// float -> int
Int(t) => {
let width = t.bit_width().unwrap_or_else(|| self.pointer_size().bits());
// `to_i128` is a saturating cast, which is what we need
// (https://doc.rust-lang.org/nightly/nightly-rustc/rustc_apfloat/trait.Float.html#method.to_i128_r).
let v = f.to_i128(usize::try_from(width).unwrap()).value;
Ok(Scalar::from_int(v, Size::from_bits(width)))
Scalar::from_int(v, Size::from_bits(width))
}
// float -> f32
Float(FloatTy::F32) => Ok(Scalar::from_f32(f.convert(&mut false).value)),
Float(FloatTy::F32) => Scalar::from_f32(f.convert(&mut false).value),
// float -> f64
Float(FloatTy::F64) => Ok(Scalar::from_f64(f.convert(&mut false).value)),
Float(FloatTy::F64) => Scalar::from_f64(f.convert(&mut false).value),
// That's it.
_ => bug!("invalid float to {:?} cast", dest_ty),
}
Expand All @@ -254,11 +262,11 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
dest: PlaceTy<'tcx, M::PointerTag>,
// The pointee types
source_ty: Ty<'tcx>,
dest_ty: Ty<'tcx>,
cast_ty: Ty<'tcx>,
) -> InterpResult<'tcx> {
// A<Struct> -> A<Trait> conversion
let (src_pointee_ty, dest_pointee_ty) =
self.tcx.struct_lockstep_tails_erasing_lifetimes(source_ty, dest_ty, self.param_env);
self.tcx.struct_lockstep_tails_erasing_lifetimes(source_ty, cast_ty, self.param_env);

match (&src_pointee_ty.kind, &dest_pointee_ty.kind) {
(&ty::Array(_, length), &ty::Slice(_)) => {
Expand Down Expand Up @@ -286,48 +294,50 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
self.write_immediate(val, dest)
}

_ => bug!("invalid unsizing {:?} -> {:?}", src.layout.ty, dest.layout.ty),
_ => bug!("invalid unsizing {:?} -> {:?}", src.layout.ty, cast_ty),
}
}

fn unsize_into(
&mut self,
src: OpTy<'tcx, M::PointerTag>,
cast_ty: TyAndLayout<'tcx>,
dest: PlaceTy<'tcx, M::PointerTag>,
) -> InterpResult<'tcx> {
trace!("Unsizing {:?} into {:?}", src, dest);
match (&src.layout.ty.kind, &dest.layout.ty.kind) {
(&ty::Ref(_, s, _), &ty::Ref(_, d, _) | &ty::RawPtr(TypeAndMut { ty: d, .. }))
| (&ty::RawPtr(TypeAndMut { ty: s, .. }), &ty::RawPtr(TypeAndMut { ty: d, .. })) => {
self.unsize_into_ptr(src, dest, s, d)
trace!("Unsizing {:?} of type {} into {:?}", *src, src.layout.ty, cast_ty.ty);
match (&src.layout.ty.kind, &cast_ty.ty.kind) {
(&ty::Ref(_, s, _), &ty::Ref(_, c, _) | &ty::RawPtr(TypeAndMut { ty: c, .. }))
| (&ty::RawPtr(TypeAndMut { ty: s, .. }), &ty::RawPtr(TypeAndMut { ty: c, .. })) => {
self.unsize_into_ptr(src, dest, s, c)
Comment on lines +303 to +307
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, in other parts of the compiler these are clearly marked "source" and "target", but I guess not consistently enough.

Either way, feel free to use "target" to be more symmetrical. (But we can leave it as-is)

Copy link
Member Author

Choose a reason for hiding this comment

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

"target" and "dest" are a bit too similar for my taste...

}
(&ty::Adt(def_a, _), &ty::Adt(def_b, _)) => {
assert_eq!(def_a, def_b);
if def_a.is_box() || def_b.is_box() {
if !def_a.is_box() || !def_b.is_box() {
bug!("invalid unsizing between {:?} -> {:?}", src.layout, dest.layout);
bug!("invalid unsizing between {:?} -> {:?}", src.layout.ty, cast_ty.ty);
}
return self.unsize_into_ptr(
src,
dest,
src.layout.ty.boxed_ty(),
dest.layout.ty.boxed_ty(),
cast_ty.ty.boxed_ty(),
);
}

// unsizing of generic struct with pointer fields
// Example: `Arc<T>` -> `Arc<Trait>`
// here we need to increase the size of every &T thin ptr field to a fat ptr
for i in 0..src.layout.fields.count() {
let dst_field = self.place_field(dest, i)?;
if dst_field.layout.is_zst() {
let cast_ty_field = cast_ty.field(self, i)?;
if cast_ty_field.is_zst() {
continue;
}
let src_field = self.operand_field(src, i)?;
if src_field.layout.ty == dst_field.layout.ty {
let dst_field = self.place_field(dest, i)?;
if src_field.layout.ty == cast_ty_field.ty {
self.copy_op(src_field, dst_field)?;
} else {
self.unsize_into(src_field, dst_field)?;
self.unsize_into(src_field, cast_ty_field, dst_field)?;
}
}
Ok(())
Expand Down
5 changes: 3 additions & 2 deletions src/librustc_mir/interpret/step.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,9 +253,10 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
self.write_scalar(Scalar::from_machine_usize(layout.size.bytes(), self), dest)?;
}

Cast(kind, ref operand, _) => {
Cast(cast_kind, ref operand, cast_ty) => {
let src = self.eval_operand(operand, None)?;
self.cast(src, kind, dest)?;
let cast_ty = self.subst_from_current_frame_and_normalize_erasing_regions(cast_ty);
self.cast(src, cast_kind, cast_ty, dest)?;
}

Discriminant(place) => {
Expand Down