From f1ea2731335b1e6c84ebd2d4cd767626ee1073df Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 29 Mar 2020 14:10:16 +0200 Subject: [PATCH 1/4] Miri engine: stronger type-based sanity check for assignments --- src/librustc_mir/interpret/place.rs | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index ece54daf4c61d..b16c7ffbffd9f 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -283,6 +283,18 @@ impl<'tcx, Tag: ::std::fmt::Debug> PlaceTy<'tcx, Tag> { } } +/// Test if it is valid for a MIR assignment to assign `src`-typed place to `dest`-typed value. +fn mir_assign_valid_types<'tcx>(src: Ty<'tcx>, dest: Ty<'tcx>) -> bool { + src == dest + || match (&src.kind, &dest.kind) { + // After MIR optimizations, there can be assignments that change reference mutability. + (ty::Ref(_, src_pointee, _), ty::Ref(_, dest_pointee, _)) => { + src_pointee == dest_pointee + } + _ => false, + } +} + // separating the pointer tag for `impl Trait`, see https://github.com/rust-lang/rust/issues/54385 impl<'mir, 'tcx, Tag, M> InterpCx<'mir, 'tcx, M> where @@ -869,10 +881,10 @@ where // We do NOT compare the types for equality, because well-typed code can // actually "transmute" `&mut T` to `&T` in an assignment without a cast. assert!( - src.layout.layout == dest.layout.layout, - "Layout mismatch when copying!\nsrc: {:#?}\ndest: {:#?}", - src, - dest + mir_assign_valid_types(src.layout.ty, dest.layout.ty), + "type mismatch when copying!\nsrc: {:?}, dest: {:?}", + src.layout.ty, + dest.layout.ty, ); // Let us see if the layout is simple so we take a shortcut, avoid force_allocation. @@ -923,7 +935,7 @@ where src: OpTy<'tcx, M::PointerTag>, dest: PlaceTy<'tcx, M::PointerTag>, ) -> InterpResult<'tcx> { - if src.layout.layout == dest.layout.layout { + if mir_assign_valid_types(src.layout.ty, dest.layout.ty) { // Fast path: Just use normal `copy_op` return self.copy_op(src, dest); } From 100c809386149ef2a9dcda3f79d1416bd07dbb59 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 29 Mar 2020 15:14:49 +0200 Subject: [PATCH 2/4] also accept fn-ptr-type-changing assignments --- src/librustc_mir/interpret/place.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index b16c7ffbffd9f..5d4976c09bbb4 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -287,10 +287,15 @@ impl<'tcx, Tag: ::std::fmt::Debug> PlaceTy<'tcx, Tag> { fn mir_assign_valid_types<'tcx>(src: Ty<'tcx>, dest: Ty<'tcx>) -> bool { src == dest || match (&src.kind, &dest.kind) { - // After MIR optimizations, there can be assignments that change reference mutability. (ty::Ref(_, src_pointee, _), ty::Ref(_, dest_pointee, _)) => { + // After optimizations, there can be assignments that change reference mutability. + // This does not affect reference layout, so that is fine. src_pointee == dest_pointee } + (ty::FnPtr(_), ty::FnPtr(_)) => { + // All function pointers have equal layout, and thus can be assigned. + true + } _ => false, } } @@ -882,7 +887,7 @@ where // actually "transmute" `&mut T` to `&T` in an assignment without a cast. assert!( mir_assign_valid_types(src.layout.ty, dest.layout.ty), - "type mismatch when copying!\nsrc: {:?}, dest: {:?}", + "type mismatch when copying!\nsrc: {:?},\ndest: {:?}", src.layout.ty, dest.layout.ty, ); From 351b7d099a90868c8021485af4221ec35c84fcd2 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 29 Mar 2020 15:43:36 +0200 Subject: [PATCH 3/4] also use mir_assign_valid_types in from_known_layout check --- src/librustc_mir/interpret/eval_context.rs | 44 +++++++++++++++++++++- src/librustc_mir/interpret/mod.rs | 18 +++------ src/librustc_mir/interpret/operand.rs | 35 +++-------------- src/librustc_mir/interpret/place.rs | 24 ++---------- 4 files changed, 58 insertions(+), 63 deletions(-) diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index 9aa42e107dc84..ce098f583d055 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/src/librustc_mir/interpret/eval_context.rs @@ -210,6 +210,48 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> LayoutOf for InterpCx<'mir, 'tcx, M> { } } +/// Test if it is valid for a MIR assignment to assign `src`-typed place to `dest`-typed value. +/// This test should be symmetric, as it is primarily about layout compatibility. +pub(super) fn mir_assign_valid_types<'tcx>(src: Ty<'tcx>, dest: Ty<'tcx>) -> bool { + src == dest + || match (&src.kind, &dest.kind) { + (ty::Ref(_, src_pointee, _), ty::Ref(_, dest_pointee, _)) => { + // After optimizations, there can be assignments that change reference mutability. + // This does not affect reference layout, so that is fine. + src_pointee == dest_pointee + } + (ty::FnPtr(_), ty::FnPtr(_)) => { + // All function pointers have equal layout, and thus can be assigned. + true + } + _ => false, + } +} + +/// Use the already known layout if given (but sanity check in debug mode), +/// or compute the layout. +#[cfg_attr(not(debug_assertions), inline(always))] +pub(super) fn from_known_layout<'tcx>( + known_layout: Option>, + compute: impl FnOnce() -> InterpResult<'tcx, TyAndLayout<'tcx>>, +) -> InterpResult<'tcx, TyAndLayout<'tcx>> { + match known_layout { + None => compute(), + Some(known_layout) => { + if cfg!(debug_assertions) { + let check_layout = compute()?; + assert!( + mir_assign_valid_types(check_layout.ty, known_layout.ty), + "expected type differs from actual type.\nexpected: {:?}\nactual: {:?}", + known_layout.ty, + check_layout.ty, + ); + } + Ok(known_layout) + } + } +} + impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { pub fn new( tcx: TyCtxtAt<'tcx>, @@ -377,7 +419,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // have to support that case (mostly by skipping all caching). match frame.locals.get(local).and_then(|state| state.layout.get()) { None => { - let layout = crate::interpret::operand::from_known_layout(layout, || { + let layout = from_known_layout(layout, || { let local_ty = frame.body.local_decls[local].ty; let local_ty = self.subst_from_frame_and_normalize_erasing_regions(frame, local_ty); diff --git a/src/librustc_mir/interpret/mod.rs b/src/librustc_mir/interpret/mod.rs index 4e30da829079c..fb59a177b9716 100644 --- a/src/librustc_mir/interpret/mod.rs +++ b/src/librustc_mir/interpret/mod.rs @@ -18,19 +18,13 @@ mod visitor; pub use rustc_middle::mir::interpret::*; // have all the `interpret` symbols in one place: here pub use self::eval_context::{Frame, InterpCx, LocalState, LocalValue, StackPopCleanup}; - -pub use self::place::{MPlaceTy, MemPlace, MemPlaceMeta, Place, PlaceTy}; - -pub use self::memory::{AllocCheck, FnVal, Memory, MemoryKind}; - +pub use self::intern::{intern_const_alloc_recursive, InternKind}; pub use self::machine::{AllocMap, Machine, MayLeak, StackPopJump}; - -pub use self::operand::{ImmTy, Immediate, OpTy, Operand, ScalarMaybeUndef}; - -pub use self::visitor::{MutValueVisitor, ValueVisitor}; - +pub use self::memory::{AllocCheck, FnVal, Memory, MemoryKind}; +pub use self::operand::{ImmTy, Immediate, OpTy, Operand}; +pub use self::place::{MPlaceTy, MemPlace, MemPlaceMeta, Place, PlaceTy}; pub use self::validity::RefTracking; - -pub use self::intern::{intern_const_alloc_recursive, InternKind}; +pub use self::visitor::{MutValueVisitor, ValueVisitor}; crate use self::intrinsics::eval_nullary_intrinsic; +use eval_context::{from_known_layout, mir_assign_valid_types}; diff --git a/src/librustc_mir/interpret/operand.rs b/src/librustc_mir/interpret/operand.rs index 999d0d26ab34a..12595e4e4d926 100644 --- a/src/librustc_mir/interpret/operand.rs +++ b/src/librustc_mir/interpret/operand.rs @@ -2,21 +2,21 @@ //! All high-level functions to read from memory work on operands as sources. use std::convert::TryFrom; +use std::fmt::Write; -use super::{InterpCx, MPlaceTy, Machine, MemPlace, Place, PlaceTy}; use rustc_hir::def::Namespace; use rustc_macros::HashStable; -pub use rustc_middle::mir::interpret::ScalarMaybeUndef; -use rustc_middle::mir::interpret::{ - sign_extend, truncate, AllocId, ConstValue, GlobalId, InterpResult, Pointer, Scalar, -}; use rustc_middle::ty::layout::{IntegerExt, PrimitiveExt, TyAndLayout}; use rustc_middle::ty::print::{FmtPrinter, PrettyPrinter, Printer}; use rustc_middle::ty::Ty; use rustc_middle::{mir, ty}; use rustc_target::abi::{Abi, DiscriminantKind, HasDataLayout, Integer, LayoutOf, Size}; use rustc_target::abi::{VariantIdx, Variants}; -use std::fmt::Write; + +use super::{ + from_known_layout, sign_extend, truncate, AllocId, ConstValue, GlobalId, InterpCx, + InterpResult, MPlaceTy, Machine, MemPlace, Place, PlaceTy, Pointer, Scalar, ScalarMaybeUndef, +}; /// An `Immediate` represents a single immediate self-contained Rust value. /// @@ -203,29 +203,6 @@ impl<'tcx, Tag: Copy> ImmTy<'tcx, Tag> { } } -// Use the existing layout if given (but sanity check in debug mode), -// or compute the layout. -#[inline(always)] -pub(super) fn from_known_layout<'tcx>( - layout: Option>, - compute: impl FnOnce() -> InterpResult<'tcx, TyAndLayout<'tcx>>, -) -> InterpResult<'tcx, TyAndLayout<'tcx>> { - match layout { - None => compute(), - Some(layout) => { - if cfg!(debug_assertions) { - let layout2 = compute()?; - assert_eq!( - layout.layout, layout2.layout, - "mismatch in layout of supposedly equal-layout types {:?} and {:?}", - layout.ty, layout2.ty - ); - } - Ok(layout) - } - } -} - impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { /// Normalice `place.ptr` to a `Pointer` if this is a place and not a ZST. /// Can be helpful to avoid lots of `force_ptr` calls later, if this place is used a lot. diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index 5d4976c09bbb4..c9999ef7c95e5 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -7,16 +7,15 @@ use std::hash::Hash; use rustc_macros::HashStable; use rustc_middle::mir; -use rustc_middle::mir::interpret::truncate; use rustc_middle::ty::layout::{PrimitiveExt, TyAndLayout}; use rustc_middle::ty::{self, Ty}; use rustc_target::abi::{Abi, Align, DiscriminantKind, FieldsShape}; use rustc_target::abi::{HasDataLayout, LayoutOf, Size, VariantIdx, Variants}; use super::{ - AllocId, AllocMap, Allocation, AllocationExtra, ImmTy, Immediate, InterpCx, InterpResult, - LocalValue, Machine, MemoryKind, OpTy, Operand, Pointer, PointerArithmetic, RawConst, Scalar, - ScalarMaybeUndef, + mir_assign_valid_types, truncate, AllocId, AllocMap, Allocation, AllocationExtra, ImmTy, + Immediate, InterpCx, InterpResult, LocalValue, Machine, MemoryKind, OpTy, Operand, Pointer, + PointerArithmetic, RawConst, Scalar, ScalarMaybeUndef, }; #[derive(Copy, Clone, Debug, Hash, PartialEq, Eq, HashStable)] @@ -283,23 +282,6 @@ impl<'tcx, Tag: ::std::fmt::Debug> PlaceTy<'tcx, Tag> { } } -/// Test if it is valid for a MIR assignment to assign `src`-typed place to `dest`-typed value. -fn mir_assign_valid_types<'tcx>(src: Ty<'tcx>, dest: Ty<'tcx>) -> bool { - src == dest - || match (&src.kind, &dest.kind) { - (ty::Ref(_, src_pointee, _), ty::Ref(_, dest_pointee, _)) => { - // After optimizations, there can be assignments that change reference mutability. - // This does not affect reference layout, so that is fine. - src_pointee == dest_pointee - } - (ty::FnPtr(_), ty::FnPtr(_)) => { - // All function pointers have equal layout, and thus can be assigned. - true - } - _ => false, - } -} - // separating the pointer tag for `impl Trait`, see https://github.com/rust-lang/rust/issues/54385 impl<'mir, 'tcx, Tag, M> InterpCx<'mir, 'tcx, M> where From 343b3f010c23d2ff23dd08b62541a2dd9461916e Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 2 Apr 2020 22:49:38 +0200 Subject: [PATCH 4/4] switch assignment check back to testing layout equality --- src/librustc_mir/interpret/eval_context.rs | 37 ++++++++++++---------- src/librustc_mir/interpret/place.rs | 4 +-- 2 files changed, 23 insertions(+), 18 deletions(-) diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index ce098f583d055..10d3101ebb87a 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/src/librustc_mir/interpret/eval_context.rs @@ -18,7 +18,7 @@ use rustc_middle::ty::query::TyCtxtAt; use rustc_middle::ty::subst::SubstsRef; use rustc_middle::ty::{self, Ty, TyCtxt, TypeFoldable}; use rustc_span::source_map::DUMMY_SP; -use rustc_target::abi::{Align, HasDataLayout, LayoutOf, Size, TargetDataLayout}; +use rustc_target::abi::{Abi, Align, HasDataLayout, LayoutOf, Size, TargetDataLayout}; use super::{ Immediate, MPlaceTy, Machine, MemPlace, MemPlaceMeta, Memory, OpTy, Operand, Place, PlaceTy, @@ -212,20 +212,25 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> LayoutOf for InterpCx<'mir, 'tcx, M> { /// Test if it is valid for a MIR assignment to assign `src`-typed place to `dest`-typed value. /// This test should be symmetric, as it is primarily about layout compatibility. -pub(super) fn mir_assign_valid_types<'tcx>(src: Ty<'tcx>, dest: Ty<'tcx>) -> bool { - src == dest - || match (&src.kind, &dest.kind) { - (ty::Ref(_, src_pointee, _), ty::Ref(_, dest_pointee, _)) => { - // After optimizations, there can be assignments that change reference mutability. - // This does not affect reference layout, so that is fine. - src_pointee == dest_pointee - } - (ty::FnPtr(_), ty::FnPtr(_)) => { - // All function pointers have equal layout, and thus can be assigned. - true - } - _ => false, - } +pub(super) fn mir_assign_valid_types<'tcx>( + src: TyAndLayout<'tcx>, + dest: TyAndLayout<'tcx>, +) -> bool { + if src.ty == dest.ty { + // Equal types, all is good. + return true; + } + // Type-changing assignments can happen for (at least) two reasons: + // - `&mut T` -> `&T` gets optimized from a reborrow to a mere assignment. + // - Subtyping is used. While all normal lifetimes are erased, higher-ranked lifetime + // bounds are still around and can lead to type differences. + // There is no good way to check the latter, so we compare layouts instead -- but only + // for values with `Scalar`/`ScalarPair` abi. + // FIXME: Do something more accurate, type-based. + match &src.abi { + Abi::Scalar(..) | Abi::ScalarPair(..) => src.layout == dest.layout, + _ => false, + } } /// Use the already known layout if given (but sanity check in debug mode), @@ -241,7 +246,7 @@ pub(super) fn from_known_layout<'tcx>( if cfg!(debug_assertions) { let check_layout = compute()?; assert!( - mir_assign_valid_types(check_layout.ty, known_layout.ty), + mir_assign_valid_types(check_layout, known_layout), "expected type differs from actual type.\nexpected: {:?}\nactual: {:?}", known_layout.ty, check_layout.ty, diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index c9999ef7c95e5..ec299cdd2134b 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -868,7 +868,7 @@ where // We do NOT compare the types for equality, because well-typed code can // actually "transmute" `&mut T` to `&T` in an assignment without a cast. assert!( - mir_assign_valid_types(src.layout.ty, dest.layout.ty), + mir_assign_valid_types(src.layout, dest.layout), "type mismatch when copying!\nsrc: {:?},\ndest: {:?}", src.layout.ty, dest.layout.ty, @@ -922,7 +922,7 @@ where src: OpTy<'tcx, M::PointerTag>, dest: PlaceTy<'tcx, M::PointerTag>, ) -> InterpResult<'tcx> { - if mir_assign_valid_types(src.layout.ty, dest.layout.ty) { + if mir_assign_valid_types(src.layout, dest.layout) { // Fast path: Just use normal `copy_op` return self.copy_op(src, dest); }