From 05a5895f0db7c33c5c2a489ab21b6c3a7b66a7a1 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 5 Apr 2020 11:55:52 +0200 Subject: [PATCH 1/3] miri assignment check: compare types after normalizing all late-bound regions away --- src/librustc_mir/interpret/eval_context.rs | 52 +++++++++++++++------- src/librustc_mir/interpret/operand.rs | 2 +- src/librustc_mir/interpret/place.rs | 4 +- 3 files changed, 39 insertions(+), 19 deletions(-) diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index 10d3101ebb8..35f2aadff6a 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/src/librustc_mir/interpret/eval_context.rs @@ -14,11 +14,11 @@ sign_extend, truncate, AllocId, FrameInfo, GlobalId, InterpResult, Pointer, Scalar, }; use rustc_middle::ty::layout::{self, TyAndLayout}; -use rustc_middle::ty::query::TyCtxtAt; -use rustc_middle::ty::subst::SubstsRef; -use rustc_middle::ty::{self, Ty, TyCtxt, TypeFoldable}; +use rustc_middle::ty::{ + self, fold::BottomUpFolder, query::TyCtxtAt, subst::SubstsRef, Ty, TyCtxt, TypeFoldable, +}; use rustc_span::source_map::DUMMY_SP; -use rustc_target::abi::{Abi, Align, HasDataLayout, LayoutOf, Size, TargetDataLayout}; +use rustc_target::abi::{Align, HasDataLayout, LayoutOf, Size, TargetDataLayout}; use super::{ Immediate, MPlaceTy, Machine, MemPlace, MemPlaceMeta, Memory, OpTy, Operand, Place, PlaceTy, @@ -213,6 +213,7 @@ fn layout_of(&self, ty: Ty<'tcx>) -> Self::TyAndLayout { /// 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>( + tcx: TyCtxt<'tcx>, src: TyAndLayout<'tcx>, dest: TyAndLayout<'tcx>, ) -> bool { @@ -220,23 +221,42 @@ pub(super) fn mir_assign_valid_types<'tcx>( // 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, + if src.layout != dest.layout { + // Layout differs, definitely not equal. + // We do this here because Miri would *do the wrong thing* if we allowed layout-changing + // assignments. + return false; } + + // Type-changing assignments can happen for (at least) two reasons: + // 1. `&mut T` -> `&T` gets optimized from a reborrow to a mere assignment. + // 2. Subtyping is used. While all normal lifetimes are erased, higher-ranked lifetime + // bounds with their late-bound regions are still around and can lead to type differences. + // Normalize both of them away. + let normalize = |ty: Ty<'tcx>| { + ty.fold_with(&mut BottomUpFolder { + tcx, + // Normalize all references to immutable. + ty_op: |ty| match ty.kind { + ty::Ref(_, pointee, _) => tcx.mk_imm_ref(tcx.lifetimes.re_erased, pointee), + _ => ty, + }, + // We just erase all late-bound regions, but this is not fully correct (FIXME): + // lifetimes in invariant positions could matter (e.g. through associated types). + // We rely on the fact that layout was confirmed to be equal above. + lt_op: |_| tcx.lifetimes.re_erased, + // Leave consts unchanged. + ct_op: |ct| ct, + }) + }; + normalize(src.ty) == normalize(dest.ty) } /// 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>( + tcx: TyCtxt<'tcx>, known_layout: Option>, compute: impl FnOnce() -> InterpResult<'tcx, TyAndLayout<'tcx>>, ) -> InterpResult<'tcx, TyAndLayout<'tcx>> { @@ -246,7 +266,7 @@ pub(super) fn from_known_layout<'tcx>( if cfg!(debug_assertions) { let check_layout = compute()?; assert!( - mir_assign_valid_types(check_layout, known_layout), + mir_assign_valid_types(tcx, check_layout, known_layout), "expected type differs from actual type.\nexpected: {:?}\nactual: {:?}", known_layout.ty, check_layout.ty, @@ -424,7 +444,7 @@ pub fn layout_of_local( // have to support that case (mostly by skipping all caching). match frame.locals.get(local).and_then(|state| state.layout.get()) { None => { - let layout = from_known_layout(layout, || { + let layout = from_known_layout(self.tcx.tcx, 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/operand.rs b/src/librustc_mir/interpret/operand.rs index 12595e4e4d9..03614b2803f 100644 --- a/src/librustc_mir/interpret/operand.rs +++ b/src/librustc_mir/interpret/operand.rs @@ -529,7 +529,7 @@ pub(super) fn eval_operands( ty::ConstKind::Value(val_val) => val_val, }; // Other cases need layout. - let layout = from_known_layout(layout, || self.layout_of(val.ty))?; + let layout = from_known_layout(self.tcx.tcx, layout, || self.layout_of(val.ty))?; let op = match val_val { ConstValue::ByRef { alloc, offset } => { let id = self.tcx.alloc_map.lock().create_memory_alloc(alloc); diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index ec299cdd213..716c7c7d933 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -868,7 +868,7 @@ fn copy_op_no_validate( // 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, dest.layout), + mir_assign_valid_types(self.tcx.tcx, src.layout, dest.layout), "type mismatch when copying!\nsrc: {:?},\ndest: {:?}", src.layout.ty, dest.layout.ty, @@ -922,7 +922,7 @@ pub fn copy_op_transmute( src: OpTy<'tcx, M::PointerTag>, dest: PlaceTy<'tcx, M::PointerTag>, ) -> InterpResult<'tcx> { - if mir_assign_valid_types(src.layout, dest.layout) { + if mir_assign_valid_types(self.tcx.tcx, src.layout, dest.layout) { // Fast path: Just use normal `copy_op` return self.copy_op(src, dest); } From 315a3b96a4c8c793ee1161671b78fd5b0490fb9e Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 5 Apr 2020 11:56:02 +0200 Subject: [PATCH 2/3] add test --- .../ui/consts/const-eval/issue-70804-fn-subtyping.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 src/test/ui/consts/const-eval/issue-70804-fn-subtyping.rs diff --git a/src/test/ui/consts/const-eval/issue-70804-fn-subtyping.rs b/src/test/ui/consts/const-eval/issue-70804-fn-subtyping.rs new file mode 100644 index 00000000000..59d46ea66c9 --- /dev/null +++ b/src/test/ui/consts/const-eval/issue-70804-fn-subtyping.rs @@ -0,0 +1,10 @@ +// check-pass +#![feature(const_fn)] + +const fn nested(x: (for<'a> fn(&'a ()), String)) -> (fn(&'static ()), String) { + x +} + +pub const TEST: (fn(&'static ()), String) = nested((|_x| (), String::new())); + +fn main() {} From e52a4519131bccc732f53ec13582016bb2b1d04a Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 5 Apr 2020 12:11:33 +0200 Subject: [PATCH 3/3] fix comment --- src/librustc_mir/interpret/eval_context.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index 35f2aadff6a..0b182d42287 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/src/librustc_mir/interpret/eval_context.rs @@ -230,8 +230,8 @@ pub(super) fn mir_assign_valid_types<'tcx>( // Type-changing assignments can happen for (at least) two reasons: // 1. `&mut T` -> `&T` gets optimized from a reborrow to a mere assignment. - // 2. Subtyping is used. While all normal lifetimes are erased, higher-ranked lifetime - // bounds with their late-bound regions are still around and can lead to type differences. + // 2. Subtyping is used. While all normal lifetimes are erased, higher-ranked types + // with their late-bound lifetimes are still around and can lead to type differences. // Normalize both of them away. let normalize = |ty: Ty<'tcx>| { ty.fold_with(&mut BottomUpFolder { @@ -241,7 +241,7 @@ pub(super) fn mir_assign_valid_types<'tcx>( ty::Ref(_, pointee, _) => tcx.mk_imm_ref(tcx.lifetimes.re_erased, pointee), _ => ty, }, - // We just erase all late-bound regions, but this is not fully correct (FIXME): + // We just erase all late-bound lifetimes, but this is not fully correct (FIXME): // lifetimes in invariant positions could matter (e.g. through associated types). // We rely on the fact that layout was confirmed to be equal above. lt_op: |_| tcx.lifetimes.re_erased,