From 8c406dc29cd49402609700469a313735622ec80c Mon Sep 17 00:00:00 2001 From: Wesley Wiser Date: Sun, 27 Oct 2019 14:58:00 -0400 Subject: [PATCH 1/4] Allow miri allocation interning to work im generic Machines This is necessary so that the `ComstPropMachine` can intern allocations. --- src/librustc/mir/interpret/value.rs | 2 +- src/librustc_mir/interpret/intern.rs | 106 ++++++++++++++++++++------ src/librustc_mir/interpret/operand.rs | 6 +- 3 files changed, 87 insertions(+), 27 deletions(-) diff --git a/src/librustc/mir/interpret/value.rs b/src/librustc/mir/interpret/value.rs index 52c72de7579e1..f82af62c5f39d 100644 --- a/src/librustc/mir/interpret/value.rs +++ b/src/librustc/mir/interpret/value.rs @@ -458,7 +458,7 @@ impl From> for Scalar { } } -#[derive(Clone, Copy, Eq, PartialEq, RustcEncodable, RustcDecodable, HashStable)] +#[derive(Clone, Copy, Eq, PartialEq, RustcEncodable, RustcDecodable, HashStable, Hash)] pub enum ScalarMaybeUndef { Scalar(Scalar), Undef, diff --git a/src/librustc_mir/interpret/intern.rs b/src/librustc_mir/interpret/intern.rs index 68bb0a3e435df..f3c503b2970ab 100644 --- a/src/librustc_mir/interpret/intern.rs +++ b/src/librustc_mir/interpret/intern.rs @@ -3,22 +3,34 @@ //! After a const evaluation has computed a value, before we destroy the const evaluator's session //! memory, we need to extract all memory allocations to the global memory pool so they stay around. -use rustc::ty::{Ty, self}; -use rustc::mir::interpret::{InterpResult, ErrorHandled}; -use rustc::hir; use super::validity::RefTracking; -use rustc_data_structures::fx::FxHashSet; +use rustc::hir; +use rustc::mir::interpret::{ErrorHandled, InterpResult}; +use rustc::ty::{self, Ty}; +use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use syntax::ast::Mutability; use super::{ - ValueVisitor, MemoryKind, AllocId, MPlaceTy, Scalar, + AllocId, Allocation, InterpCx, Machine, MemoryKind, MPlaceTy, Scalar, ValueVisitor, }; -use crate::const_eval::{CompileTimeInterpreter, CompileTimeEvalContext}; -struct InternVisitor<'rt, 'mir, 'tcx> { +struct InternVisitor<'rt, 'mir, 'tcx, M> +where + M: Machine< + 'mir, + 'tcx, + MemoryKinds = !, + PointerTag = (), + ExtraFnVal = !, + FrameExtra = (), + MemoryExtra = (), + AllocExtra = (), + MemoryMap = FxHashMap, Allocation)>, + >, +{ /// The ectx from which we intern. - ecx: &'rt mut CompileTimeEvalContext<'mir, 'tcx>, + ecx: &'rt mut InterpCx<'mir, 'tcx, M>, /// Previously encountered safe references. ref_tracking: &'rt mut RefTracking<(MPlaceTy<'tcx>, Mutability, InternMode)>, /// A list of all encountered allocations. After type-based interning, we traverse this list to @@ -58,18 +70,28 @@ struct IsStaticOrFn; /// `immutable` things might become mutable if `ty` is not frozen. /// `ty` can be `None` if there is no potential interior mutability /// to account for (e.g. for vtables). -fn intern_shallow<'rt, 'mir, 'tcx>( - ecx: &'rt mut CompileTimeEvalContext<'mir, 'tcx>, +fn intern_shallow<'rt, 'mir, 'tcx, M>( + ecx: &'rt mut InterpCx<'mir, 'tcx, M>, leftover_allocations: &'rt mut FxHashSet, mode: InternMode, alloc_id: AllocId, mutability: Mutability, ty: Option>, -) -> InterpResult<'tcx, Option> { - trace!( - "InternVisitor::intern {:?} with {:?}", - alloc_id, mutability, - ); +) -> InterpResult<'tcx, Option> +where + M: Machine< + 'mir, + 'tcx, + MemoryKinds = !, + PointerTag = (), + ExtraFnVal = !, + FrameExtra = (), + MemoryExtra = (), + AllocExtra = (), + MemoryMap = FxHashMap, Allocation)>, + >, +{ + trace!("InternVisitor::intern {:?} with {:?}", alloc_id, mutability,); // remove allocation let tcx = ecx.tcx; let (kind, mut alloc) = match ecx.memory.alloc_map.remove(&alloc_id) { @@ -130,7 +152,20 @@ fn intern_shallow<'rt, 'mir, 'tcx>( Ok(None) } -impl<'rt, 'mir, 'tcx> InternVisitor<'rt, 'mir, 'tcx> { +impl<'rt, 'mir, 'tcx, M> InternVisitor<'rt, 'mir, 'tcx, M> +where + M: Machine< + 'mir, + 'tcx, + MemoryKinds = !, + PointerTag = (), + ExtraFnVal = !, + FrameExtra = (), + MemoryExtra = (), + AllocExtra = (), + MemoryMap = FxHashMap, Allocation)>, + >, +{ fn intern_shallow( &mut self, alloc_id: AllocId, @@ -148,15 +183,27 @@ impl<'rt, 'mir, 'tcx> InternVisitor<'rt, 'mir, 'tcx> { } } -impl<'rt, 'mir, 'tcx> - ValueVisitor<'mir, 'tcx, CompileTimeInterpreter<'mir, 'tcx>> +impl<'rt, 'mir, 'tcx, M> + ValueVisitor<'mir, 'tcx, M> for - InternVisitor<'rt, 'mir, 'tcx> + InternVisitor<'rt, 'mir, 'tcx, M> +where + M: Machine< + 'mir, + 'tcx, + MemoryKinds = !, + PointerTag = (), + ExtraFnVal = !, + FrameExtra = (), + MemoryExtra = (), + AllocExtra = (), + MemoryMap = FxHashMap, Allocation)>, + >, { type V = MPlaceTy<'tcx>; #[inline(always)] - fn ecx(&self) -> &CompileTimeEvalContext<'mir, 'tcx> { + fn ecx(&self) -> &InterpCx<'mir, 'tcx, M> { &self.ecx } @@ -265,12 +312,25 @@ for } } -pub fn intern_const_alloc_recursive( - ecx: &mut CompileTimeEvalContext<'mir, 'tcx>, +pub fn intern_const_alloc_recursive( + ecx: &mut InterpCx<'mir, 'tcx, M>, // The `mutability` of the place, ignoring the type. place_mut: Option, ret: MPlaceTy<'tcx>, -) -> InterpResult<'tcx> { +) -> InterpResult<'tcx> +where + M: Machine< + 'mir, + 'tcx, + MemoryKinds = !, + PointerTag = (), + ExtraFnVal = !, + FrameExtra = (), + MemoryExtra = (), + AllocExtra = (), + MemoryMap = FxHashMap, Allocation)>, + >, +{ let tcx = ecx.tcx; let (base_mutability, base_intern_mode) = match place_mut { Some(hir::Mutability::Immutable) => (Mutability::Immutable, InternMode::Static), diff --git a/src/librustc_mir/interpret/operand.rs b/src/librustc_mir/interpret/operand.rs index cfa70164cdce4..1a01baccabf7a 100644 --- a/src/librustc_mir/interpret/operand.rs +++ b/src/librustc_mir/interpret/operand.rs @@ -27,7 +27,7 @@ use rustc_macros::HashStable; /// operations and fat 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, Debug, PartialEq, Eq, HashStable)] +#[derive(Copy, Clone, Debug, PartialEq, Eq, HashStable, Hash)] pub enum Immediate { Scalar(ScalarMaybeUndef), ScalarPair(ScalarMaybeUndef, ScalarMaybeUndef), @@ -104,7 +104,7 @@ impl<'tcx, Tag> ::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, Debug, PartialEq, Eq, HashStable)] +#[derive(Copy, Clone, Debug, PartialEq, Eq, HashStable, Hash)] pub enum Operand { Immediate(Immediate), Indirect(MemPlace), @@ -134,7 +134,7 @@ impl Operand { } } -#[derive(Copy, Clone, Debug, PartialEq)] +#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] pub struct OpTy<'tcx, Tag=()> { op: Operand, // Keep this private, it helps enforce invariants pub layout: TyLayout<'tcx>, From c5e762fd8845658bf5d70dc714d16f2a1ec75c3f Mon Sep 17 00:00:00 2001 From: Wesley Wiser Date: Fri, 22 Nov 2019 20:48:18 -0500 Subject: [PATCH 2/4] Intern allocations during constant propagation --- src/librustc_mir/transform/const_prop.rs | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index da11a7446bbe2..4333e5c51427e 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -30,7 +30,7 @@ use crate::interpret::{ self, InterpCx, ScalarMaybeUndef, Immediate, OpTy, StackPopCleanup, LocalValue, LocalState, AllocId, Frame, Allocation, MemoryKind, ImmTy, Pointer, Memory, PlaceTy, - Operand as InterpOperand, + Operand as InterpOperand, intern_const_alloc_recursive, }; use crate::const_eval::error_to_const_error; use crate::transform::{MirPass, MirSource}; @@ -655,14 +655,27 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { return false; } - match *op { + let is_scalar = match *op { interpret::Operand::Immediate(Immediate::Scalar(ScalarMaybeUndef::Scalar(s))) => s.is_bits(), interpret::Operand::Immediate(Immediate::ScalarPair(ScalarMaybeUndef::Scalar(l), ScalarMaybeUndef::Scalar(r))) => l.is_bits() && r.is_bits(), _ => false + }; + + if let interpret::Operand::Indirect(_) = *op { + if self.tcx.sess.opts.debugging_opts.mir_opt_level >= 2 { + intern_const_alloc_recursive( + &mut self.ecx, + None, + op.assert_mem_place() + ).expect("failed to intern alloc"); + return true; + } } + + return is_scalar; } } From 2b6815a69e618a96a221d8db24ace87d10ae69f0 Mon Sep 17 00:00:00 2001 From: Wesley Wiser Date: Sat, 23 Nov 2019 12:25:03 -0500 Subject: [PATCH 3/4] [const prop] Fix "alloc id without corresponding allocation" ICE Fixes #66345 --- src/librustc_mir/transform/const_prop.rs | 4 +--- .../const_prop/const_prop_fails_gracefully.rs | 6 ++--- src/test/mir-opt/const_prop/ref_deref.rs | 2 +- src/test/mir-opt/const_prop/reify_fn_ptr.rs | 2 +- src/test/mir-opt/const_prop/slice_len.rs | 4 ++-- src/test/ui/consts/issue-66345.rs | 24 +++++++++++++++++++ 6 files changed, 32 insertions(+), 10 deletions(-) create mode 100644 src/test/ui/consts/issue-66345.rs diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index 4333e5c51427e..f7572f3668426 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -649,9 +649,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { } fn should_const_prop(&mut self, op: OpTy<'tcx>) -> bool { - if self.tcx.sess.opts.debugging_opts.mir_opt_level >= 2 { - return true; - } else if self.tcx.sess.opts.debugging_opts.mir_opt_level == 0 { + if self.tcx.sess.opts.debugging_opts.mir_opt_level == 0 { return false; } diff --git a/src/test/mir-opt/const_prop/const_prop_fails_gracefully.rs b/src/test/mir-opt/const_prop/const_prop_fails_gracefully.rs index 34d4a534ad9f1..3f82b81a47de5 100644 --- a/src/test/mir-opt/const_prop/const_prop_fails_gracefully.rs +++ b/src/test/mir-opt/const_prop/const_prop_fails_gracefully.rs @@ -23,9 +23,9 @@ fn main() { // START rustc.main.ConstProp.after.mir // bb0: { // ... -// _4 = const Scalar(AllocId(1).0x0) : &i32; -// _3 = const Scalar(AllocId(1).0x0) : &i32; -// _2 = const Value(Scalar(AllocId(1).0x0)) : *const i32; +// _4 = const main::FOO; +// _3 = _4; +// _2 = move _3 as *const i32 (Misc); // ... // _1 = move _2 as usize (Misc); // ... diff --git a/src/test/mir-opt/const_prop/ref_deref.rs b/src/test/mir-opt/const_prop/ref_deref.rs index 2d04822c0e789..d45ffdc877535 100644 --- a/src/test/mir-opt/const_prop/ref_deref.rs +++ b/src/test/mir-opt/const_prop/ref_deref.rs @@ -14,7 +14,7 @@ fn main() { // START rustc.main.ConstProp.after.mir // bb0: { // ... -// _2 = const Scalar(AllocId(0).0x0) : &i32; +// _2 = &(promoted[0]: i32); // _1 = const 4i32; // ... // } diff --git a/src/test/mir-opt/const_prop/reify_fn_ptr.rs b/src/test/mir-opt/const_prop/reify_fn_ptr.rs index ad7f195676a68..4d6fe905b0c35 100644 --- a/src/test/mir-opt/const_prop/reify_fn_ptr.rs +++ b/src/test/mir-opt/const_prop/reify_fn_ptr.rs @@ -16,7 +16,7 @@ fn main() { // START rustc.main.ConstProp.after.mir // bb0: { // ... -// _3 = const main; +// _3 = const main as fn() (Pointer(ReifyFnPointer)); // _2 = move _3 as usize (Misc); // ... // _1 = move _2 as *const fn() (Misc); diff --git a/src/test/mir-opt/const_prop/slice_len.rs b/src/test/mir-opt/const_prop/slice_len.rs index 05595ce147c96..d6ff76b34b9b5 100644 --- a/src/test/mir-opt/const_prop/slice_len.rs +++ b/src/test/mir-opt/const_prop/slice_len.rs @@ -24,8 +24,8 @@ fn main() { // START rustc.main.ConstProp.after.mir // bb0: { // ... -// _4 = const Scalar(AllocId(0).0x0) : &[u32; 3]; -// _3 = const Scalar(AllocId(0).0x0) : &[u32; 3]; +// _4 = &(promoted[0]: [u32; 3]); +// _3 = _4; // _2 = move _3 as &[u32] (Pointer(Unsize)); // ... // _6 = const 1usize; diff --git a/src/test/ui/consts/issue-66345.rs b/src/test/ui/consts/issue-66345.rs new file mode 100644 index 0000000000000..7d0de73007c8c --- /dev/null +++ b/src/test/ui/consts/issue-66345.rs @@ -0,0 +1,24 @@ +// run-pass +// compile-flags: -Z mir-opt-level=3 + +// Checks that the compiler does not ICE when passing references to field of by-value struct +// with -Z mir-opt-level=3 + +fn do_nothing(_: &()) {} + +pub struct Foo { + bar: (), +} + +pub fn by_value_1(foo: Foo) { + do_nothing(&foo.bar); +} + +pub fn by_value_2(foo: Foo) { + do_nothing(&foo.bar); +} + +fn main() { + by_value_1(Foo { bar: () }); + by_value_2::<()>(Foo { bar: () }); +} From 32e78ca2e38cdc83a8a18ee38cad43a5df72c1de Mon Sep 17 00:00:00 2001 From: Wesley Wiser Date: Sun, 24 Nov 2019 19:09:58 -0500 Subject: [PATCH 4/4] Respond to CR feedback --- src/librustc_mir/interpret/intern.rs | 73 ++++-------------------- src/librustc_mir/lib.rs | 1 + src/librustc_mir/transform/const_prop.rs | 19 +++--- 3 files changed, 20 insertions(+), 73 deletions(-) diff --git a/src/librustc_mir/interpret/intern.rs b/src/librustc_mir/interpret/intern.rs index f3c503b2970ab..630f3f603449b 100644 --- a/src/librustc_mir/interpret/intern.rs +++ b/src/librustc_mir/interpret/intern.rs @@ -15,9 +15,8 @@ use super::{ AllocId, Allocation, InterpCx, Machine, MemoryKind, MPlaceTy, Scalar, ValueVisitor, }; -struct InternVisitor<'rt, 'mir, 'tcx, M> -where - M: Machine< +pub trait CompileTimeMachine<'mir, 'tcx> = + Machine< 'mir, 'tcx, MemoryKinds = !, @@ -27,8 +26,9 @@ where MemoryExtra = (), AllocExtra = (), MemoryMap = FxHashMap, Allocation)>, - >, -{ + >; + +struct InternVisitor<'rt, 'mir, 'tcx, M: CompileTimeMachine<'mir, 'tcx>> { /// The ectx from which we intern. ecx: &'rt mut InterpCx<'mir, 'tcx, M>, /// Previously encountered safe references. @@ -70,27 +70,14 @@ struct IsStaticOrFn; /// `immutable` things might become mutable if `ty` is not frozen. /// `ty` can be `None` if there is no potential interior mutability /// to account for (e.g. for vtables). -fn intern_shallow<'rt, 'mir, 'tcx, M>( +fn intern_shallow<'rt, 'mir, 'tcx, M: CompileTimeMachine<'mir, 'tcx>>( ecx: &'rt mut InterpCx<'mir, 'tcx, M>, leftover_allocations: &'rt mut FxHashSet, mode: InternMode, alloc_id: AllocId, mutability: Mutability, ty: Option>, -) -> InterpResult<'tcx, Option> -where - M: Machine< - 'mir, - 'tcx, - MemoryKinds = !, - PointerTag = (), - ExtraFnVal = !, - FrameExtra = (), - MemoryExtra = (), - AllocExtra = (), - MemoryMap = FxHashMap, Allocation)>, - >, -{ +) -> InterpResult<'tcx, Option> { trace!("InternVisitor::intern {:?} with {:?}", alloc_id, mutability,); // remove allocation let tcx = ecx.tcx; @@ -152,20 +139,7 @@ where Ok(None) } -impl<'rt, 'mir, 'tcx, M> InternVisitor<'rt, 'mir, 'tcx, M> -where - M: Machine< - 'mir, - 'tcx, - MemoryKinds = !, - PointerTag = (), - ExtraFnVal = !, - FrameExtra = (), - MemoryExtra = (), - AllocExtra = (), - MemoryMap = FxHashMap, Allocation)>, - >, -{ +impl<'rt, 'mir, 'tcx, M: CompileTimeMachine<'mir, 'tcx>> InternVisitor<'rt, 'mir, 'tcx, M> { fn intern_shallow( &mut self, alloc_id: AllocId, @@ -183,22 +157,10 @@ where } } -impl<'rt, 'mir, 'tcx, M> +impl<'rt, 'mir, 'tcx, M: CompileTimeMachine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> for InternVisitor<'rt, 'mir, 'tcx, M> -where - M: Machine< - 'mir, - 'tcx, - MemoryKinds = !, - PointerTag = (), - ExtraFnVal = !, - FrameExtra = (), - MemoryExtra = (), - AllocExtra = (), - MemoryMap = FxHashMap, Allocation)>, - >, { type V = MPlaceTy<'tcx>; @@ -312,25 +274,12 @@ where } } -pub fn intern_const_alloc_recursive( +pub fn intern_const_alloc_recursive>( ecx: &mut InterpCx<'mir, 'tcx, M>, // The `mutability` of the place, ignoring the type. place_mut: Option, ret: MPlaceTy<'tcx>, -) -> InterpResult<'tcx> -where - M: Machine< - 'mir, - 'tcx, - MemoryKinds = !, - PointerTag = (), - ExtraFnVal = !, - FrameExtra = (), - MemoryExtra = (), - AllocExtra = (), - MemoryMap = FxHashMap, Allocation)>, - >, -{ +) -> InterpResult<'tcx> { let tcx = ecx.tcx; let (base_mutability, base_intern_mode) = match place_mut { Some(hir::Mutability::Immutable) => (Mutability::Immutable, InternMode::Static), diff --git a/src/librustc_mir/lib.rs b/src/librustc_mir/lib.rs index 6d19cd63bc32e..b5273ea7bc609 100644 --- a/src/librustc_mir/lib.rs +++ b/src/librustc_mir/lib.rs @@ -27,6 +27,7 @@ Rust MIR: a lowered representation of Rust. Also: an experiment! #![feature(range_is_empty)] #![feature(stmt_expr_attributes)] #![feature(bool_to_option)] +#![feature(trait_alias)] #![recursion_limit="256"] diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index f7572f3668426..58d1a8b709797 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -649,31 +649,28 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { } fn should_const_prop(&mut self, op: OpTy<'tcx>) -> bool { - if self.tcx.sess.opts.debugging_opts.mir_opt_level == 0 { + let mir_opt_level = self.tcx.sess.opts.debugging_opts.mir_opt_level; + + if mir_opt_level == 0 { return false; } - let is_scalar = match *op { + match *op { interpret::Operand::Immediate(Immediate::Scalar(ScalarMaybeUndef::Scalar(s))) => s.is_bits(), interpret::Operand::Immediate(Immediate::ScalarPair(ScalarMaybeUndef::Scalar(l), ScalarMaybeUndef::Scalar(r))) => l.is_bits() && r.is_bits(), - _ => false - }; - - if let interpret::Operand::Indirect(_) = *op { - if self.tcx.sess.opts.debugging_opts.mir_opt_level >= 2 { + interpret::Operand::Indirect(_) if mir_opt_level >= 2 => { intern_const_alloc_recursive( &mut self.ecx, None, op.assert_mem_place() ).expect("failed to intern alloc"); - return true; - } + true + }, + _ => false } - - return is_scalar; } }