From c53210c5501b60165b6eb8487268e3833191316a Mon Sep 17 00:00:00 2001 From: Vytautas Astrauskas Date: Mon, 16 Mar 2020 15:12:42 -0700 Subject: [PATCH 1/8] Make the necessary changes to support concurrency in Miri. --- src/librustc_mir/const_eval/error.rs | 2 +- src/librustc_mir/const_eval/machine.rs | 30 ++++++++++++--- src/librustc_mir/interpret/cast.rs | 2 +- src/librustc_mir/interpret/eval_context.rs | 37 ++++++++++--------- src/librustc_mir/interpret/intern.rs | 7 +++- src/librustc_mir/interpret/intrinsics.rs | 2 +- .../interpret/intrinsics/caller_location.rs | 4 +- src/librustc_mir/interpret/machine.rs | 24 ++++++++++++ src/librustc_mir/interpret/memory.rs | 6 ++- src/librustc_mir/interpret/operand.rs | 6 ++- src/librustc_mir/interpret/operator.rs | 4 +- src/librustc_mir/interpret/place.rs | 11 +++--- src/librustc_mir/interpret/step.rs | 8 ++-- src/librustc_mir/interpret/terminator.rs | 4 +- src/librustc_mir/interpret/traits.rs | 2 +- src/librustc_mir/interpret/validity.rs | 6 +-- src/librustc_mir/interpret/visitor.rs | 6 ++- src/librustc_mir/transform/const_prop.rs | 31 ++++++++++++++-- 18 files changed, 135 insertions(+), 57 deletions(-) diff --git a/src/librustc_mir/const_eval/error.rs b/src/librustc_mir/const_eval/error.rs index f7e28cf8d8c2f..3c3618f390c61 100644 --- a/src/librustc_mir/const_eval/error.rs +++ b/src/librustc_mir/const_eval/error.rs @@ -50,7 +50,7 @@ impl Error for ConstEvalErrKind {} /// Turn an interpreter error into something to report to the user. /// As a side-effect, if RUSTC_CTFE_BACKTRACE is set, this prints the backtrace. /// Should be called only if the error is actually going to to be reported! -pub fn error_to_const_error<'mir, 'tcx, M: Machine<'mir, 'tcx>>( +pub fn error_to_const_error<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>>( ecx: &InterpCx<'mir, 'tcx, M>, mut error: InterpErrorInfo<'tcx>, ) -> ConstEvalErr<'tcx> { diff --git a/src/librustc_mir/const_eval/machine.rs b/src/librustc_mir/const_eval/machine.rs index e53ca6b31bb67..41ca753e79c9f 100644 --- a/src/librustc_mir/const_eval/machine.rs +++ b/src/librustc_mir/const_eval/machine.rs @@ -19,7 +19,7 @@ use crate::interpret::{ use super::error::*; -impl<'mir, 'tcx> InterpCx<'mir, 'tcx, CompileTimeInterpreter> { +impl<'mir, 'tcx> InterpCx<'mir, 'tcx, CompileTimeInterpreter<'mir, 'tcx>> { /// Evaluate a const function where all arguments (if any) are zero-sized types. /// The evaluation is memoized thanks to the query system. /// @@ -86,12 +86,15 @@ impl<'mir, 'tcx> InterpCx<'mir, 'tcx, CompileTimeInterpreter> { } /// Extra machine state for CTFE, and the Machine instance -pub struct CompileTimeInterpreter { +pub struct CompileTimeInterpreter<'mir, 'tcx> { /// For now, the number of terminators that can be evaluated before we throw a resource /// exhuastion error. /// /// Setting this to `0` disables the limit and allows the interpreter to run forever. pub steps_remaining: usize, + + /// The virtual call stack. + pub(crate) stack: Vec>, } #[derive(Copy, Clone, Debug)] @@ -100,9 +103,9 @@ pub struct MemoryExtra { pub(super) can_access_statics: bool, } -impl CompileTimeInterpreter { +impl<'mir, 'tcx> CompileTimeInterpreter<'mir, 'tcx> { pub(super) fn new(const_eval_limit: usize) -> Self { - CompileTimeInterpreter { steps_remaining: const_eval_limit } + CompileTimeInterpreter { steps_remaining: const_eval_limit, stack: Vec::new() } } } @@ -156,7 +159,8 @@ impl interpret::AllocMap for FxHashMap { } } -crate type CompileTimeEvalContext<'mir, 'tcx> = InterpCx<'mir, 'tcx, CompileTimeInterpreter>; +crate type CompileTimeEvalContext<'mir, 'tcx> = + InterpCx<'mir, 'tcx, CompileTimeInterpreter<'mir, 'tcx>>; impl interpret::MayLeak for ! { #[inline(always)] @@ -166,7 +170,7 @@ impl interpret::MayLeak for ! { } } -impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter { +impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir, 'tcx> { type MemoryKind = !; type PointerTag = (); type ExtraFnVal = !; @@ -186,6 +190,20 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter { false } + #[inline(always)] + fn stack( + ecx: &'a InterpCx<'mir, 'tcx, Self>, + ) -> &'a [Frame<'mir, 'tcx, Self::PointerTag, Self::FrameExtra>] { + &ecx.machine.stack + } + + #[inline(always)] + fn stack_mut( + ecx: &'a mut InterpCx<'mir, 'tcx, Self>, + ) -> &'a mut Vec> { + &mut ecx.machine.stack + } + #[inline(always)] fn enforce_validity(_ecx: &InterpCx<'mir, 'tcx, Self>) -> bool { false // for now, we don't enforce validity diff --git a/src/librustc_mir/interpret/cast.rs b/src/librustc_mir/interpret/cast.rs index 67696aa2da8b0..256d7eccc065b 100644 --- a/src/librustc_mir/interpret/cast.rs +++ b/src/librustc_mir/interpret/cast.rs @@ -12,7 +12,7 @@ use rustc_middle::ty::{self, Ty, TypeAndMut, TypeFoldable}; use rustc_span::symbol::sym; use rustc_target::abi::{LayoutOf, Size, Variants}; -impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { +impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { pub fn cast( &mut self, src: OpTy<'tcx, M::PointerTag>, diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index f111eecb9450e..7166503c8a832 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/src/librustc_mir/interpret/eval_context.rs @@ -39,9 +39,6 @@ pub struct InterpCx<'mir, 'tcx, M: Machine<'mir, 'tcx>> { /// The virtual memory system. pub memory: Memory<'mir, 'tcx, M>, - /// The virtual call stack. - pub(crate) stack: Vec>, - /// A cache for deduplicating vtables pub(super) vtables: FxHashMap<(Ty<'tcx>, Option>), Pointer>, @@ -295,7 +292,7 @@ pub(super) fn from_known_layout<'tcx>( } } -impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { +impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { pub fn new( tcx: TyCtxtAt<'tcx>, param_env: ty::ParamEnv<'tcx>, @@ -307,7 +304,6 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { tcx, param_env, memory: Memory::new(tcx, memory_extra), - stack: Vec::new(), vtables: FxHashMap::default(), } } @@ -348,23 +344,29 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { #[inline(always)] pub fn stack(&self) -> &[Frame<'mir, 'tcx, M::PointerTag, M::FrameExtra>] { - &self.stack + M::stack(self) + } + + #[inline(always)] + pub fn stack_mut(&mut self) -> &mut Vec> { + M::stack_mut(self) } #[inline(always)] pub fn cur_frame(&self) -> usize { - assert!(!self.stack.is_empty()); - self.stack.len() - 1 + let stack = self.stack(); + assert!(!stack.is_empty()); + stack.len() - 1 } #[inline(always)] pub fn frame(&self) -> &Frame<'mir, 'tcx, M::PointerTag, M::FrameExtra> { - self.stack.last().expect("no call frames exist") + self.stack().last().expect("no call frames exist") } #[inline(always)] pub fn frame_mut(&mut self) -> &mut Frame<'mir, 'tcx, M::PointerTag, M::FrameExtra> { - self.stack.last_mut().expect("no call frames exist") + self.stack_mut().last_mut().expect("no call frames exist") } #[inline(always)] @@ -595,7 +597,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { return_place: Option>, return_to_block: StackPopCleanup, ) -> InterpResult<'tcx> { - if !self.stack.is_empty() { + if !self.stack().is_empty() { info!("PAUSING({}) {}", self.cur_frame(), self.frame().instance); } ::log_settings::settings().indentation += 1; @@ -614,7 +616,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { extra: (), }; let frame = M::init_frame_extra(self, pre_frame)?; - self.stack.push(frame); + self.stack_mut().push(frame); // don't allocate at all for trivial constants if body.local_decls.len() > 1 { @@ -649,7 +651,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { M::after_stack_push(self)?; info!("ENTERING({}) {}", self.cur_frame(), self.frame().instance); - if self.stack.len() > *self.tcx.sess.recursion_limit.get() { + if self.stack().len() > *self.tcx.sess.recursion_limit.get() { throw_exhaust!(StackFrameLimitReached) } else { Ok(()) @@ -719,7 +721,8 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { ); ::log_settings::settings().indentation -= 1; - let frame = self.stack.pop().expect("tried to pop a stack frame, but there were none"); + let frame = + self.stack_mut().pop().expect("tried to pop a stack frame, but there were none"); // Now where do we jump next? @@ -734,7 +737,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { }; if !cleanup { - assert!(self.stack.is_empty(), "only the topmost frame should ever be leaked"); + assert!(self.stack().is_empty(), "only the topmost frame should ever be leaked"); assert!(next_block.is_none(), "tried to skip cleanup when we have a next block!"); assert!(!unwinding, "tried to skip cleanup during unwinding"); // Leak the locals, skip validation, skip machine hook. @@ -783,7 +786,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } } - if !self.stack.is_empty() { + if !self.stack().is_empty() { info!( "CONTINUING({}) {} (unwinding = {})", self.cur_frame(), @@ -899,7 +902,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } write!(msg, ":").unwrap(); - match self.stack[frame].locals[local].value { + match self.stack()[frame].locals[local].value { LocalValue::Dead => write!(msg, " is dead").unwrap(), LocalValue::Uninitialized => write!(msg, " is uninitialized").unwrap(), LocalValue::Live(Operand::Indirect(mplace)) => match mplace.ptr { diff --git a/src/librustc_mir/interpret/intern.rs b/src/librustc_mir/interpret/intern.rs index af415b3837fdf..3b8b76e2562cf 100644 --- a/src/librustc_mir/interpret/intern.rs +++ b/src/librustc_mir/interpret/intern.rs @@ -148,7 +148,7 @@ impl<'rt, 'mir, 'tcx, M: CompileTimeMachine<'mir, 'tcx>> InternVisitor<'rt, 'mir } } -impl<'rt, 'mir, 'tcx, M: CompileTimeMachine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> +impl<'rt, 'mir, 'tcx: 'mir, M: CompileTimeMachine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> for InternVisitor<'rt, 'mir, 'tcx, M> { type V = MPlaceTy<'tcx>; @@ -284,7 +284,10 @@ pub fn intern_const_alloc_recursive>( intern_kind: InternKind, ret: MPlaceTy<'tcx>, ignore_interior_mut_in_const_validation: bool, -) -> InterpResult<'tcx> { +) -> InterpResult<'tcx> +where + 'tcx: 'mir, +{ let tcx = ecx.tcx; let (base_mutability, base_intern_mode) = match intern_kind { // `static mut` doesn't care about interior mutability, it's mutable anyway diff --git a/src/librustc_mir/interpret/intrinsics.rs b/src/librustc_mir/interpret/intrinsics.rs index d976df82577ea..c8bf328ce8eee 100644 --- a/src/librustc_mir/interpret/intrinsics.rs +++ b/src/librustc_mir/interpret/intrinsics.rs @@ -73,7 +73,7 @@ crate fn eval_nullary_intrinsic<'tcx>( }) } -impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { +impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { /// Returns `true` if emulation happened. pub fn emulate_intrinsic( &mut self, diff --git a/src/librustc_mir/interpret/intrinsics/caller_location.rs b/src/librustc_mir/interpret/intrinsics/caller_location.rs index 754f45d9ec05b..91b046d7bb264 100644 --- a/src/librustc_mir/interpret/intrinsics/caller_location.rs +++ b/src/librustc_mir/interpret/intrinsics/caller_location.rs @@ -10,11 +10,11 @@ use crate::interpret::{ MPlaceTy, MemoryKind, Scalar, }; -impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { +impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { /// Walks up the callstack from the intrinsic's callsite, searching for the first callsite in a /// frame which is not `#[track_caller]`. crate fn find_closest_untracked_caller_location(&self) -> Span { - self.stack + self.stack() .iter() .rev() // Find first non-`#[track_caller]` frame. diff --git a/src/librustc_mir/interpret/machine.rs b/src/librustc_mir/interpret/machine.rs index 8bf8d904cb29e..14acaf8260713 100644 --- a/src/librustc_mir/interpret/machine.rs +++ b/src/librustc_mir/interpret/machine.rs @@ -120,6 +120,16 @@ pub trait Machine<'mir, 'tcx>: Sized { /// Whether memory accesses should be alignment-checked. fn enforce_alignment(memory_extra: &Self::MemoryExtra) -> bool; + /// Borrow the current thread's stack. + fn stack( + ecx: &'a InterpCx<'mir, 'tcx, Self>, + ) -> &'a [Frame<'mir, 'tcx, Self::PointerTag, Self::FrameExtra>]; + + /// Mutably borrow the current thread's stack. + fn stack_mut( + ecx: &'a mut InterpCx<'mir, 'tcx, Self>, + ) -> &'a mut Vec>; + /// Whether to enforce the validity invariant fn enforce_validity(ecx: &InterpCx<'mir, 'tcx, Self>) -> bool; @@ -230,6 +240,20 @@ pub trait Machine<'mir, 'tcx>: Sized { id } + /// In Rust, thread locals are just special statics. Therefore, the compiler + /// uses the same code for allocating both. However, in Miri we want to have + /// a property that each allocation has a unique id and, therefore, we + /// generate a fresh allocation id for each thread. This function takes a + /// potentially thread local allocation id and resolves the original static + /// allocation id that can be used to compute the value of the static. + #[inline] + fn resolve_thread_local_allocation_id( + _memory_extra: &Self::MemoryExtra, + id: AllocId, + ) -> AllocId { + id + } + /// Called to initialize the "extra" state of an allocation and make the pointers /// it contains (in relocations) tagged. The way we construct allocations is /// to always first construct it without extra and then add the extra. diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index bcad7855c3736..20158b9c1b6cc 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -429,7 +429,8 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { id: AllocId, is_write: bool, ) -> InterpResult<'tcx, Cow<'tcx, Allocation>> { - let alloc = tcx.alloc_map.lock().get(id); + let alloc = + tcx.alloc_map.lock().get(M::resolve_thread_local_allocation_id(memory_extra, id)); let (alloc, def_id) = match alloc { Some(GlobalAlloc::Memory(mem)) => { // Memory of a constant or promoted or anonymous memory referenced by a static. @@ -592,7 +593,8 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { // # Statics // Can't do this in the match argument, we may get cycle errors since the lock would // be held throughout the match. - let alloc = self.tcx.alloc_map.lock().get(id); + let alloc = + self.tcx.alloc_map.lock().get(M::resolve_thread_local_allocation_id(&self.extra, id)); match alloc { Some(GlobalAlloc::Static(did)) => { // Use size and align of the type. diff --git a/src/librustc_mir/interpret/operand.rs b/src/librustc_mir/interpret/operand.rs index 893f4c1db7e0a..b86a98d85984e 100644 --- a/src/librustc_mir/interpret/operand.rs +++ b/src/librustc_mir/interpret/operand.rs @@ -208,7 +208,7 @@ impl<'tcx, Tag: Copy> ImmTy<'tcx, Tag> { } } -impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { +impl<'mir, 'tcx: 'mir, 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. #[inline] @@ -439,7 +439,9 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { ) -> InterpResult<'tcx, OpTy<'tcx, M::PointerTag>> { let op = match *place { Place::Ptr(mplace) => Operand::Indirect(mplace), - Place::Local { frame, local } => *self.access_local(&self.stack[frame], local, None)?, + Place::Local { frame, local } => { + *self.access_local(&self.stack()[frame], local, None)? + } }; Ok(OpTy { op, layout: place.layout }) } diff --git a/src/librustc_mir/interpret/operator.rs b/src/librustc_mir/interpret/operator.rs index 0aa7e98f3edfa..d651267f82b79 100644 --- a/src/librustc_mir/interpret/operator.rs +++ b/src/librustc_mir/interpret/operator.rs @@ -9,7 +9,7 @@ use rustc_target::abi::LayoutOf; use super::{ImmTy, Immediate, InterpCx, Machine, PlaceTy}; -impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { +impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { /// Applies the binary operation `op` to the two operands and writes a tuple of the result /// and a boolean signifying the potential overflow to the destination. pub fn binop_with_overflow( @@ -45,7 +45,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } } -impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { +impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { fn binary_char_op( &self, bin_op: mir::BinOp, diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index 9ac4b3551fc43..12a600f23c2ce 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -283,7 +283,7 @@ impl<'tcx, Tag: ::std::fmt::Debug> PlaceTy<'tcx, Tag> { } // 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> +impl<'mir, 'tcx: 'mir, Tag, M> InterpCx<'mir, 'tcx, M> where // FIXME: Working around https://github.com/rust-lang/rust/issues/54385 Tag: ::std::fmt::Debug + Copy + Eq + Hash + 'static, @@ -755,7 +755,7 @@ where // but not factored as a separate function. let mplace = match dest.place { Place::Local { frame, local } => { - match self.stack[frame].locals[local].access_mut()? { + match self.stack_mut()[frame].locals[local].access_mut()? { Ok(local) => { // Local can be updated in-place. *local = LocalValue::Live(Operand::Immediate(src)); @@ -985,14 +985,15 @@ where ) -> InterpResult<'tcx, (MPlaceTy<'tcx, M::PointerTag>, Option)> { let (mplace, size) = match place.place { Place::Local { frame, local } => { - match self.stack[frame].locals[local].access_mut()? { + match self.stack_mut()[frame].locals[local].access_mut()? { Ok(&mut local_val) => { // We need to make an allocation. // We need the layout of the local. We can NOT use the layout we got, // that might e.g., be an inner field of a struct with `Scalar` layout, // that has different alignment than the outer field. - let local_layout = self.layout_of_local(&self.stack[frame], local, None)?; + let local_layout = + self.layout_of_local(&self.stack()[frame], local, None)?; // We also need to support unsized types, and hence cannot use `allocate`. let (size, align) = self .size_and_align_of(meta, local_layout)? @@ -1008,7 +1009,7 @@ where } // Now we can call `access_mut` again, asserting it goes well, // and actually overwrite things. - *self.stack[frame].locals[local].access_mut().unwrap().unwrap() = + *self.stack_mut()[frame].locals[local].access_mut().unwrap().unwrap() = LocalValue::Live(Operand::Indirect(mplace)); (mplace, Some(size)) } diff --git a/src/librustc_mir/interpret/step.rs b/src/librustc_mir/interpret/step.rs index 37740878f7043..cce204353d3a6 100644 --- a/src/librustc_mir/interpret/step.rs +++ b/src/librustc_mir/interpret/step.rs @@ -29,7 +29,7 @@ fn binop_right_homogeneous(op: mir::BinOp) -> bool { } } -impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { +impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { pub fn run(&mut self) -> InterpResult<'tcx> { while self.step()? {} Ok(()) @@ -42,7 +42,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { /// This is marked `#inline(always)` to work around adverserial codegen when `opt-level = 3` #[inline(always)] pub fn step(&mut self) -> InterpResult<'tcx, bool> { - if self.stack.is_empty() { + if self.stack().is_empty() { return Ok(false); } @@ -126,7 +126,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { LlvmInlineAsm { .. } => throw_unsup_format!("inline assembly is not supported"), } - self.stack[frame_idx].stmt += 1; + self.stack_mut()[frame_idx].stmt += 1; Ok(()) } @@ -278,7 +278,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { self.set_span(terminator.source_info.span); self.eval_terminator(terminator)?; - if !self.stack.is_empty() { + if !self.stack().is_empty() { if let Some(block) = self.frame().block { info!("// executing {:?}", block); } diff --git a/src/librustc_mir/interpret/terminator.rs b/src/librustc_mir/interpret/terminator.rs index 49fee1bddcb6d..7d587bab64a74 100644 --- a/src/librustc_mir/interpret/terminator.rs +++ b/src/librustc_mir/interpret/terminator.rs @@ -11,7 +11,7 @@ use super::{ FnVal, ImmTy, InterpCx, InterpResult, MPlaceTy, Machine, OpTy, PlaceTy, StackPopCleanup, }; -impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { +impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { pub(super) fn eval_terminator( &mut self, terminator: &mir::Terminator<'tcx>, @@ -372,7 +372,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { }; match res { Err(err) => { - self.stack.pop(); + self.stack_mut().pop(); Err(err) } Ok(()) => Ok(()), diff --git a/src/librustc_mir/interpret/traits.rs b/src/librustc_mir/interpret/traits.rs index fb9401c7d8f28..7edd787c986bd 100644 --- a/src/librustc_mir/interpret/traits.rs +++ b/src/librustc_mir/interpret/traits.rs @@ -6,7 +6,7 @@ use rustc_target::abi::{Align, HasDataLayout, LayoutOf, Size}; use super::{FnVal, InterpCx, Machine, MemoryKind}; -impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { +impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { /// Creates a dynamic vtable for the given type and vtable origin. This is used only for /// objects. /// diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 701e394415bbd..a4944bdcc0b4d 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -177,7 +177,7 @@ struct ValidityVisitor<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> { ecx: &'rt InterpCx<'mir, 'tcx, M>, } -impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, 'tcx, M> { +impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, 'tcx, M> { fn aggregate_field_path_elem(&mut self, layout: TyAndLayout<'tcx>, field: usize) -> PathElem { // First, check if we are projecting to a variant. match layout.variants { @@ -604,7 +604,7 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, 'tcx, M } } -impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> +impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> for ValidityVisitor<'rt, 'mir, 'tcx, M> { type V = OpTy<'tcx, M::PointerTag>; @@ -806,7 +806,7 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> } } -impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { +impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { fn validate_operand_internal( &self, op: OpTy<'tcx, M::PointerTag>, diff --git a/src/librustc_mir/interpret/visitor.rs b/src/librustc_mir/interpret/visitor.rs index e03984f4d0be5..e0e733dc40874 100644 --- a/src/librustc_mir/interpret/visitor.rs +++ b/src/librustc_mir/interpret/visitor.rs @@ -35,7 +35,7 @@ pub trait Value<'mir, 'tcx, M: Machine<'mir, 'tcx>>: Copy { // Operands and memory-places are both values. // Places in general are not due to `place_field` having to do `force_allocation`. -impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Value<'mir, 'tcx, M> for OpTy<'tcx, M::PointerTag> { +impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> Value<'mir, 'tcx, M> for OpTy<'tcx, M::PointerTag> { #[inline(always)] fn layout(&self) -> TyAndLayout<'tcx> { self.layout @@ -73,7 +73,9 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Value<'mir, 'tcx, M> for OpTy<'tcx, M:: } } -impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Value<'mir, 'tcx, M> for MPlaceTy<'tcx, M::PointerTag> { +impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> Value<'mir, 'tcx, M> + for MPlaceTy<'tcx, M::PointerTag> +{ #[inline(always)] fn layout(&self) -> TyAndLayout<'tcx> { self.layout diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index 80c12a30135ff..b7d7fc29f3040 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -158,9 +158,18 @@ impl<'tcx> MirPass<'tcx> for ConstProp { } } -struct ConstPropMachine; +struct ConstPropMachine<'mir, 'tcx> { + /// The virtual call stack. + stack: Vec>, +} -impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine { +impl<'mir, 'tcx> ConstPropMachine<'mir, 'tcx> { + fn new() -> Self { + Self { stack: Vec::new() } + } +} + +impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine<'mir, 'tcx> { type MemoryKind = !; type PointerTag = (); type ExtraFnVal = !; @@ -178,6 +187,20 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine { false } + #[inline(always)] + fn stack( + ecx: &'a InterpCx<'mir, 'tcx, Self>, + ) -> &'a [Frame<'mir, 'tcx, Self::PointerTag, Self::FrameExtra>] { + &ecx.machine.stack + } + + #[inline(always)] + fn stack_mut( + ecx: &'a mut InterpCx<'mir, 'tcx, Self>, + ) -> &'a mut Vec> { + &mut ecx.machine.stack + } + #[inline(always)] fn enforce_validity(_ecx: &InterpCx<'mir, 'tcx, Self>) -> bool { false @@ -300,7 +323,7 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine { /// Finds optimization opportunities on the MIR. struct ConstPropagator<'mir, 'tcx> { - ecx: InterpCx<'mir, 'tcx, ConstPropMachine>, + ecx: InterpCx<'mir, 'tcx, ConstPropMachine<'mir, 'tcx>>, tcx: TyCtxt<'tcx>, can_const_prop: IndexVec, param_env: ParamEnv<'tcx>, @@ -349,7 +372,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { let param_env = tcx.param_env(def_id).with_reveal_all(); let span = tcx.def_span(def_id); - let mut ecx = InterpCx::new(tcx.at(span), param_env, ConstPropMachine, ()); + let mut ecx = InterpCx::new(tcx.at(span), param_env, ConstPropMachine::new(), ()); let can_const_prop = CanConstProp::check(body); let ret = ecx From 7e6dbd2b7f271aa773f754c4e5dd49ff046f12ba Mon Sep 17 00:00:00 2001 From: Vytautas Astrauskas Date: Tue, 31 Mar 2020 13:45:54 -0700 Subject: [PATCH 2/8] Address reviewers' comments: replace resolve_thread_local_allocation_id with resolve_maybe_global_alloc, clarify comments. --- src/librustc_mir/interpret/machine.rs | 40 +++++++++++++++++---------- src/librustc_mir/interpret/memory.rs | 10 ++----- 2 files changed, 28 insertions(+), 22 deletions(-) diff --git a/src/librustc_mir/interpret/machine.rs b/src/librustc_mir/interpret/machine.rs index 14acaf8260713..e68d92555db51 100644 --- a/src/librustc_mir/interpret/machine.rs +++ b/src/librustc_mir/interpret/machine.rs @@ -6,7 +6,7 @@ use std::borrow::{Borrow, Cow}; use std::hash::Hash; use rustc_middle::mir; -use rustc_middle::ty::{self, Ty}; +use rustc_middle::ty::{self, query::TyCtxtAt, Ty}; use rustc_span::def_id::DefId; use super::{ @@ -229,10 +229,22 @@ pub trait Machine<'mir, 'tcx>: Sized { Ok(()) } - /// Called for *every* memory access to determine the real ID of the given allocation. - /// This provides a way for the machine to "redirect" certain allocations as it sees fit. + /// Called for *every* memory access to determine the real ID of the given + /// allocation. This provides a way for the machine to "redirect" certain + /// allocations as it sees fit. /// - /// This is used by Miri to redirect extern statics to real allocations. + /// This is used by Miri for two purposes: + /// 1. Redirecting extern statics to real allocations. + /// 2. Creating unique allocation ids for thread locals. + /// + /// In Rust, one way for creating a thread local is by marking a static + /// with `#[thread_local]`. On supported platforms this gets translated + /// to a LLVM thread local. The problem with supporting these thread + /// locals in Miri is that in the internals of the compiler they look as + /// normal statics, except that they have the `thread_local` attribute. + /// However, in Miri we want to have a property that each allocation has + /// a unique id and, therefore, for these thread locals we generate a + /// fresh allocation id for each thread. /// /// This function must be idempotent. #[inline] @@ -240,18 +252,18 @@ pub trait Machine<'mir, 'tcx>: Sized { id } - /// In Rust, thread locals are just special statics. Therefore, the compiler - /// uses the same code for allocating both. However, in Miri we want to have - /// a property that each allocation has a unique id and, therefore, we - /// generate a fresh allocation id for each thread. This function takes a - /// potentially thread local allocation id and resolves the original static - /// allocation id that can be used to compute the value of the static. - #[inline] - fn resolve_thread_local_allocation_id( + /// Called to obtain the `GlobalAlloc` associated with the allocation id. + /// + /// Miri uses this callback to resolve the information about the original + /// thread local static for which `canonical_alloc_id` generated a fresh + /// allocation id. + #[inline(always)] + fn resolve_maybe_global_alloc( + tcx: TyCtxtAt<'tcx>, _memory_extra: &Self::MemoryExtra, id: AllocId, - ) -> AllocId { - id + ) -> Option> { + tcx.alloc_map.lock().get(id) } /// Called to initialize the "extra" state of an allocation and make the pointers diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index 20158b9c1b6cc..817d69d287812 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -429,9 +429,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { id: AllocId, is_write: bool, ) -> InterpResult<'tcx, Cow<'tcx, Allocation>> { - let alloc = - tcx.alloc_map.lock().get(M::resolve_thread_local_allocation_id(memory_extra, id)); - let (alloc, def_id) = match alloc { + let (alloc, def_id) = match M::resolve_maybe_global_alloc(tcx, memory_extra, id) { Some(GlobalAlloc::Memory(mem)) => { // Memory of a constant or promoted or anonymous memory referenced by a static. (mem, None) @@ -591,11 +589,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { } // # Statics - // Can't do this in the match argument, we may get cycle errors since the lock would - // be held throughout the match. - let alloc = - self.tcx.alloc_map.lock().get(M::resolve_thread_local_allocation_id(&self.extra, id)); - match alloc { + match M::resolve_maybe_global_alloc(self.tcx, &self.extra, id) { Some(GlobalAlloc::Static(did)) => { // Use size and align of the type. let ty = self.tcx.type_of(did); From 2960a6cf0482ca45aab399836e8b46bad8f91a6e Mon Sep 17 00:00:00 2001 From: Vytautas Astrauskas Date: Wed, 1 Apr 2020 17:43:50 -0700 Subject: [PATCH 3/8] Clarify the comments explaining the purpose of resolve_maybe_global_alloc. --- src/librustc_mir/interpret/machine.rs | 13 +++++++++---- src/librustc_mir/interpret/memory.rs | 14 ++++++++++++++ 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/src/librustc_mir/interpret/machine.rs b/src/librustc_mir/interpret/machine.rs index e68d92555db51..8ac15bed689b3 100644 --- a/src/librustc_mir/interpret/machine.rs +++ b/src/librustc_mir/interpret/machine.rs @@ -243,8 +243,11 @@ pub trait Machine<'mir, 'tcx>: Sized { /// locals in Miri is that in the internals of the compiler they look as /// normal statics, except that they have the `thread_local` attribute. /// However, in Miri we want to have a property that each allocation has - /// a unique id and, therefore, for these thread locals we generate a - /// fresh allocation id for each thread. + /// a unique id. Therefore, for these thread locals in + /// `canonical_alloc_id` we reserve fresh allocation ids for each + /// thread. Please note that `canonical_alloc_id` only reserves the + /// allocation ids, the actual allocation for the thread local statics + /// is done in the same way as for regular statics. /// /// This function must be idempotent. #[inline] @@ -255,8 +258,10 @@ pub trait Machine<'mir, 'tcx>: Sized { /// Called to obtain the `GlobalAlloc` associated with the allocation id. /// /// Miri uses this callback to resolve the information about the original - /// thread local static for which `canonical_alloc_id` generated a fresh - /// allocation id. + /// thread local static for which `canonical_alloc_id` reserved a fresh + /// allocation id. Since `canonical_alloc_id` does not create the actual + /// allocation and the reserved allocation id has no reference to its + /// parent, we need to ask Miri to retrieve information for us. #[inline(always)] fn resolve_maybe_global_alloc( tcx: TyCtxtAt<'tcx>, diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index 817d69d287812..69ab96132b973 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -429,6 +429,14 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { id: AllocId, is_write: bool, ) -> InterpResult<'tcx, Cow<'tcx, Allocation>> { + // The call to `resolve_maybe_global_alloc` is needed to enable Miri to + // support thread local statics. In `M::canonical_alloc_id`, for a + // thread local static, Miri reserves a fresh allocation id, but the + // actual allocation is left to the code that handles statics which + // calls this function (`get_global_alloc`). Since the allocation id is + // fresh, it has no information about the original static. The call to + // `resolve_maybe_global_alloc` allows Miri to retrieve this information + // for us. let (alloc, def_id) = match M::resolve_maybe_global_alloc(tcx, memory_extra, id) { Some(GlobalAlloc::Memory(mem)) => { // Memory of a constant or promoted or anonymous memory referenced by a static. @@ -589,6 +597,12 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { } // # Statics + // The call to `resolve_maybe_global_alloc` is needed here because Miri + // via the call to `canonical_alloc_id` above reserves fresh allocation + // ids for thread local statics. However, the actual allocation is done + // not in `canonical_alloc_id`, but in `get_raw` and `get_raw_mut`. + // Since this function may get called before `get_raw`, we need to allow + // Miri to retrieve the information about the static for us. match M::resolve_maybe_global_alloc(self.tcx, &self.extra, id) { Some(GlobalAlloc::Static(did)) => { // Use size and align of the type. From 9e46807cff0f59616b277a3ecebd70a85b5175c5 Mon Sep 17 00:00:00 2001 From: Vytautas Astrauskas Date: Tue, 14 Apr 2020 14:40:08 -0700 Subject: [PATCH 4/8] Add function eval_maybe_thread_local_static_const that allows handling thread locals without touching debug info; address other PR comments. --- src/librustc_mir/interpret/eval_context.rs | 22 ++++--- src/librustc_mir/interpret/machine.rs | 67 +++++++++++++--------- src/librustc_mir/interpret/memory.rs | 18 +++--- src/librustc_mir/interpret/operand.rs | 1 + src/librustc_mir/interpret/place.rs | 2 +- src/librustc_mir/interpret/step.rs | 8 +-- src/librustc_mir/interpret/terminator.rs | 4 +- 7 files changed, 70 insertions(+), 52 deletions(-) diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index 7166503c8a832..6b3c61a83fcbd 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/src/librustc_mir/interpret/eval_context.rs @@ -28,6 +28,8 @@ use crate::util::storage::AlwaysLiveLocals; pub struct InterpCx<'mir, 'tcx, M: Machine<'mir, 'tcx>> { /// Stores the `Machine` instance. + /// + /// Note: the stack is provided by the machine. pub machine: M, /// The results of the type checker, from rustc. @@ -343,17 +345,19 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } #[inline(always)] - pub fn stack(&self) -> &[Frame<'mir, 'tcx, M::PointerTag, M::FrameExtra>] { + pub(crate) fn stack(&self) -> &[Frame<'mir, 'tcx, M::PointerTag, M::FrameExtra>] { M::stack(self) } #[inline(always)] - pub fn stack_mut(&mut self) -> &mut Vec> { + pub(crate) fn stack_mut( + &mut self, + ) -> &mut Vec> { M::stack_mut(self) } #[inline(always)] - pub fn cur_frame(&self) -> usize { + pub fn frame_idx(&self) -> usize { let stack = self.stack(); assert!(!stack.is_empty()); stack.len() - 1 @@ -598,7 +602,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { return_to_block: StackPopCleanup, ) -> InterpResult<'tcx> { if !self.stack().is_empty() { - info!("PAUSING({}) {}", self.cur_frame(), self.frame().instance); + info!("PAUSING({}) {}", self.frame_idx(), self.frame().instance); } ::log_settings::settings().indentation += 1; @@ -649,7 +653,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } M::after_stack_push(self)?; - info!("ENTERING({}) {}", self.cur_frame(), self.frame().instance); + info!("ENTERING({}) {}", self.frame_idx(), self.frame().instance); if self.stack().len() > *self.tcx.sess.recursion_limit.get() { throw_exhaust!(StackFrameLimitReached) @@ -706,7 +710,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { pub(super) fn pop_stack_frame(&mut self, unwinding: bool) -> InterpResult<'tcx> { info!( "LEAVING({}) {} (unwinding = {})", - self.cur_frame(), + self.frame_idx(), self.frame().instance, unwinding ); @@ -789,7 +793,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { if !self.stack().is_empty() { info!( "CONTINUING({}) {} (unwinding = {})", - self.cur_frame(), + self.frame_idx(), self.frame().instance, unwinding ); @@ -897,8 +901,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { Place::Local { frame, local } => { let mut allocs = Vec::new(); let mut msg = format!("{:?}", local); - if frame != self.cur_frame() { - write!(msg, " ({} frames up)", self.cur_frame() - frame).unwrap(); + if frame != self.frame_idx() { + write!(msg, " ({} frames up)", self.frame_idx() - frame).unwrap(); } write!(msg, ":").unwrap(); diff --git a/src/librustc_mir/interpret/machine.rs b/src/librustc_mir/interpret/machine.rs index 8ac15bed689b3..4daadb9726ba4 100644 --- a/src/librustc_mir/interpret/machine.rs +++ b/src/librustc_mir/interpret/machine.rs @@ -120,16 +120,6 @@ pub trait Machine<'mir, 'tcx>: Sized { /// Whether memory accesses should be alignment-checked. fn enforce_alignment(memory_extra: &Self::MemoryExtra) -> bool; - /// Borrow the current thread's stack. - fn stack( - ecx: &'a InterpCx<'mir, 'tcx, Self>, - ) -> &'a [Frame<'mir, 'tcx, Self::PointerTag, Self::FrameExtra>]; - - /// Mutably borrow the current thread's stack. - fn stack_mut( - ecx: &'a mut InterpCx<'mir, 'tcx, Self>, - ) -> &'a mut Vec>; - /// Whether to enforce the validity invariant fn enforce_validity(ecx: &InterpCx<'mir, 'tcx, Self>) -> bool; @@ -229,25 +219,10 @@ pub trait Machine<'mir, 'tcx>: Sized { Ok(()) } - /// Called for *every* memory access to determine the real ID of the given - /// allocation. This provides a way for the machine to "redirect" certain - /// allocations as it sees fit. + /// Called for *every* memory access to determine the real ID of the given allocation. + /// This provides a way for the machine to "redirect" certain allocations as it sees fit. /// - /// This is used by Miri for two purposes: - /// 1. Redirecting extern statics to real allocations. - /// 2. Creating unique allocation ids for thread locals. - /// - /// In Rust, one way for creating a thread local is by marking a static - /// with `#[thread_local]`. On supported platforms this gets translated - /// to a LLVM thread local. The problem with supporting these thread - /// locals in Miri is that in the internals of the compiler they look as - /// normal statics, except that they have the `thread_local` attribute. - /// However, in Miri we want to have a property that each allocation has - /// a unique id. Therefore, for these thread locals in - /// `canonical_alloc_id` we reserve fresh allocation ids for each - /// thread. Please note that `canonical_alloc_id` only reserves the - /// allocation ids, the actual allocation for the thread local statics - /// is done in the same way as for regular statics. + /// This is used by Miri to redirect extern statics to real allocations. /// /// This function must be idempotent. #[inline] @@ -255,6 +230,32 @@ pub trait Machine<'mir, 'tcx>: Sized { id } + /// Called when converting a `ty::Const` to an operand. + /// + /// Miri uses this callback for creating unique allocation ids for thread + /// locals. In Rust, one way for creating a thread local is by marking a + /// static with `#[thread_local]`. On supported platforms this gets + /// translated to a LLVM thread local for which LLVM automatically ensures + /// that each thread gets its own copy. Since LLVM automatically handles + /// thread locals, the Rust compiler just treats thread local statics as + /// regular statics even though accessing a thread local static should be an + /// effectful computation that depends on the current thread. The long term + /// plan is to change MIR to make accesses to thread locals explicit + /// (https://github.com/rust-lang/rust/issues/70685). While the issue 70685 + /// is not fixed, our current workaround in Miri is to use this function to + /// reserve fresh allocation ids for each thread. Please note that here we + /// only **reserve** the allocation ids; the actual allocation for the + /// thread local statics is done in `Memory::get_global_alloc`, which uses + /// `resolve_maybe_global_alloc` to retrieve information about the + /// allocation id we generated. + #[inline] + fn eval_maybe_thread_local_static_const( + _ecx: &InterpCx<'mir, 'tcx, Self>, + val: mir::interpret::ConstValue<'tcx>, + ) -> InterpResult<'tcx, mir::interpret::ConstValue<'tcx>> { + Ok(val) + } + /// Called to obtain the `GlobalAlloc` associated with the allocation id. /// /// Miri uses this callback to resolve the information about the original @@ -326,6 +327,16 @@ pub trait Machine<'mir, 'tcx>: Sized { frame: Frame<'mir, 'tcx, Self::PointerTag>, ) -> InterpResult<'tcx, Frame<'mir, 'tcx, Self::PointerTag, Self::FrameExtra>>; + /// Borrow the current thread's stack. + fn stack( + ecx: &'a InterpCx<'mir, 'tcx, Self>, + ) -> &'a [Frame<'mir, 'tcx, Self::PointerTag, Self::FrameExtra>]; + + /// Mutably borrow the current thread's stack. + fn stack_mut( + ecx: &'a mut InterpCx<'mir, 'tcx, Self>, + ) -> &'a mut Vec>; + /// Called immediately after a stack frame got pushed and its locals got initialized. fn after_stack_push(_ecx: &mut InterpCx<'mir, 'tcx, Self>) -> InterpResult<'tcx> { Ok(()) diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index 69ab96132b973..27c8bf4590648 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -430,11 +430,12 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { is_write: bool, ) -> InterpResult<'tcx, Cow<'tcx, Allocation>> { // The call to `resolve_maybe_global_alloc` is needed to enable Miri to - // support thread local statics. In `M::canonical_alloc_id`, for a - // thread local static, Miri reserves a fresh allocation id, but the - // actual allocation is left to the code that handles statics which - // calls this function (`get_global_alloc`). Since the allocation id is - // fresh, it has no information about the original static. The call to + // support thread local statics. In + // `M::eval_maybe_thread_local_static_const`, for a thread local static, + // Miri reserves a fresh allocation id, but the actual allocation is + // left to the code that handles statics which calls this function + // (`get_global_alloc`). Since the allocation id is fresh, it has no + // information about the original static. The call to // `resolve_maybe_global_alloc` allows Miri to retrieve this information // for us. let (alloc, def_id) = match M::resolve_maybe_global_alloc(tcx, memory_extra, id) { @@ -598,9 +599,10 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { // # Statics // The call to `resolve_maybe_global_alloc` is needed here because Miri - // via the call to `canonical_alloc_id` above reserves fresh allocation - // ids for thread local statics. However, the actual allocation is done - // not in `canonical_alloc_id`, but in `get_raw` and `get_raw_mut`. + // via the callback to `eval_maybe_thread_local_static_const` in + // `eval_const_to_op` reserves fresh allocation ids for thread local + // statics. However, the actual allocation is done not in + // `resolve_maybe_global_alloc`, but in `get_raw` and `get_raw_mut`. // Since this function may get called before `get_raw`, we need to allow // Miri to retrieve the information about the static for us. match M::resolve_maybe_global_alloc(self.tcx, &self.extra, id) { diff --git a/src/librustc_mir/interpret/operand.rs b/src/librustc_mir/interpret/operand.rs index b86a98d85984e..df9ce3f18a9fe 100644 --- a/src/librustc_mir/interpret/operand.rs +++ b/src/librustc_mir/interpret/operand.rs @@ -537,6 +537,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } ty::ConstKind::Value(val_val) => val_val, }; + let val_val = M::eval_maybe_thread_local_static_const(self, val_val)?; // Other cases need layout. let layout = from_known_layout(self.tcx, layout, || self.layout_of(val.ty))?; let op = match val_val { diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index 12a600f23c2ce..6dad6b9231493 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -662,7 +662,7 @@ where } local => PlaceTy { // This works even for dead/uninitialized locals; we check further when writing - place: Place::Local { frame: self.cur_frame(), local }, + place: Place::Local { frame: self.frame_idx(), local }, layout: self.layout_of_local(self.frame(), local, None)?, }, }; diff --git a/src/librustc_mir/interpret/step.rs b/src/librustc_mir/interpret/step.rs index cce204353d3a6..aae708827b953 100644 --- a/src/librustc_mir/interpret/step.rs +++ b/src/librustc_mir/interpret/step.rs @@ -60,10 +60,10 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { let body = self.body(); let basic_block = &body.basic_blocks()[block]; - let old_frames = self.cur_frame(); + let old_frames = self.frame_idx(); if let Some(stmt) = basic_block.statements.get(stmt_id) { - assert_eq!(old_frames, self.cur_frame()); + assert_eq!(old_frames, self.frame_idx()); self.statement(stmt)?; return Ok(true); } @@ -71,7 +71,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { M::before_terminator(self)?; let terminator = basic_block.terminator(); - assert_eq!(old_frames, self.cur_frame()); + assert_eq!(old_frames, self.frame_idx()); self.terminator(terminator)?; Ok(true) } @@ -84,7 +84,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // Some statements (e.g., box) push new stack frames. // We have to record the stack frame number *before* executing the statement. - let frame_idx = self.cur_frame(); + let frame_idx = self.frame_idx(); match &stmt.kind { Assign(box (place, rvalue)) => self.eval_rvalue_into_place(rvalue, *place)?, diff --git a/src/librustc_mir/interpret/terminator.rs b/src/librustc_mir/interpret/terminator.rs index 7d587bab64a74..7157225e5c9bb 100644 --- a/src/librustc_mir/interpret/terminator.rs +++ b/src/librustc_mir/interpret/terminator.rs @@ -52,7 +52,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } Call { ref func, ref args, destination, ref cleanup, .. } => { - let old_stack = self.cur_frame(); + let old_stack = self.frame_idx(); let old_bb = self.frame().block; let func = self.eval_operand(func, None)?; let (fn_val, abi) = match func.layout.ty.kind { @@ -80,7 +80,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { self.eval_fn_call(fn_val, abi, &args[..], ret, *cleanup)?; // Sanity-check that `eval_fn_call` either pushed a new frame or // did a jump to another block. - if self.cur_frame() == old_stack && self.frame().block == old_bb { + if self.frame_idx() == old_stack && self.frame().block == old_bb { span_bug!(terminator.source_info.span, "evaluating this call made no progress"); } } From 738ebcfb6ab1aa6f960f62ff78d7029e554ee51f Mon Sep 17 00:00:00 2001 From: Vytautas Astrauskas Date: Wed, 15 Apr 2020 14:05:14 -0700 Subject: [PATCH 5/8] Remove now unnecessary resolve_maybe_global_alloc. --- src/librustc_mir/interpret/machine.rs | 36 ++++++++------------------- src/librustc_mir/interpret/memory.rs | 24 +++++------------- 2 files changed, 17 insertions(+), 43 deletions(-) diff --git a/src/librustc_mir/interpret/machine.rs b/src/librustc_mir/interpret/machine.rs index 4daadb9726ba4..376ce3b239f91 100644 --- a/src/librustc_mir/interpret/machine.rs +++ b/src/librustc_mir/interpret/machine.rs @@ -6,7 +6,7 @@ use std::borrow::{Borrow, Cow}; use std::hash::Hash; use rustc_middle::mir; -use rustc_middle::ty::{self, query::TyCtxtAt, Ty}; +use rustc_middle::ty::{self, Ty}; use rustc_span::def_id::DefId; use super::{ @@ -230,10 +230,11 @@ pub trait Machine<'mir, 'tcx>: Sized { id } - /// Called when converting a `ty::Const` to an operand. + /// Called when converting a `ty::Const` to an operand (in + /// `eval_const_to_op`). /// - /// Miri uses this callback for creating unique allocation ids for thread - /// locals. In Rust, one way for creating a thread local is by marking a + /// Miri uses this callback for creating per thread allocations for thread + /// locals. In Rust, one way of creating a thread local is by marking a /// static with `#[thread_local]`. On supported platforms this gets /// translated to a LLVM thread local for which LLVM automatically ensures /// that each thread gets its own copy. Since LLVM automatically handles @@ -243,11 +244,12 @@ pub trait Machine<'mir, 'tcx>: Sized { /// plan is to change MIR to make accesses to thread locals explicit /// (https://github.com/rust-lang/rust/issues/70685). While the issue 70685 /// is not fixed, our current workaround in Miri is to use this function to - /// reserve fresh allocation ids for each thread. Please note that here we - /// only **reserve** the allocation ids; the actual allocation for the - /// thread local statics is done in `Memory::get_global_alloc`, which uses - /// `resolve_maybe_global_alloc` to retrieve information about the - /// allocation id we generated. + /// make per-thread copies of thread locals. Please note that we cannot make + /// these copies in `canonical_alloc_id` because that is too late: for + /// example, if one created a pointer in thread `t1` to a thread local and + /// sent it to another thread `t2`, resolving the access in + /// `canonical_alloc_id` would result in pointer pointing to `t2`'s thread + /// local and not `t1` as it should. #[inline] fn eval_maybe_thread_local_static_const( _ecx: &InterpCx<'mir, 'tcx, Self>, @@ -256,22 +258,6 @@ pub trait Machine<'mir, 'tcx>: Sized { Ok(val) } - /// Called to obtain the `GlobalAlloc` associated with the allocation id. - /// - /// Miri uses this callback to resolve the information about the original - /// thread local static for which `canonical_alloc_id` reserved a fresh - /// allocation id. Since `canonical_alloc_id` does not create the actual - /// allocation and the reserved allocation id has no reference to its - /// parent, we need to ask Miri to retrieve information for us. - #[inline(always)] - fn resolve_maybe_global_alloc( - tcx: TyCtxtAt<'tcx>, - _memory_extra: &Self::MemoryExtra, - id: AllocId, - ) -> Option> { - tcx.alloc_map.lock().get(id) - } - /// Called to initialize the "extra" state of an allocation and make the pointers /// it contains (in relocations) tagged. The way we construct allocations is /// to always first construct it without extra and then add the extra. diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index 27c8bf4590648..bcad7855c3736 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -429,16 +429,8 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { id: AllocId, is_write: bool, ) -> InterpResult<'tcx, Cow<'tcx, Allocation>> { - // The call to `resolve_maybe_global_alloc` is needed to enable Miri to - // support thread local statics. In - // `M::eval_maybe_thread_local_static_const`, for a thread local static, - // Miri reserves a fresh allocation id, but the actual allocation is - // left to the code that handles statics which calls this function - // (`get_global_alloc`). Since the allocation id is fresh, it has no - // information about the original static. The call to - // `resolve_maybe_global_alloc` allows Miri to retrieve this information - // for us. - let (alloc, def_id) = match M::resolve_maybe_global_alloc(tcx, memory_extra, id) { + let alloc = tcx.alloc_map.lock().get(id); + let (alloc, def_id) = match alloc { Some(GlobalAlloc::Memory(mem)) => { // Memory of a constant or promoted or anonymous memory referenced by a static. (mem, None) @@ -598,14 +590,10 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { } // # Statics - // The call to `resolve_maybe_global_alloc` is needed here because Miri - // via the callback to `eval_maybe_thread_local_static_const` in - // `eval_const_to_op` reserves fresh allocation ids for thread local - // statics. However, the actual allocation is done not in - // `resolve_maybe_global_alloc`, but in `get_raw` and `get_raw_mut`. - // Since this function may get called before `get_raw`, we need to allow - // Miri to retrieve the information about the static for us. - match M::resolve_maybe_global_alloc(self.tcx, &self.extra, id) { + // Can't do this in the match argument, we may get cycle errors since the lock would + // be held throughout the match. + let alloc = self.tcx.alloc_map.lock().get(id); + match alloc { Some(GlobalAlloc::Static(did)) => { // Use size and align of the type. let ty = self.tcx.type_of(did); From 844eead57e346444a9182e5e2eee3d41ff0dca3f Mon Sep 17 00:00:00 2001 From: Vytautas Astrauskas Date: Thu, 16 Apr 2020 07:47:10 -0700 Subject: [PATCH 6/8] Rename Machine::eval_maybe_thread_local_static_const to adjust_global_const and add an additional comment. --- src/librustc_mir/interpret/machine.rs | 2 +- src/librustc_mir/interpret/operand.rs | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/librustc_mir/interpret/machine.rs b/src/librustc_mir/interpret/machine.rs index 376ce3b239f91..dfdd95c95a364 100644 --- a/src/librustc_mir/interpret/machine.rs +++ b/src/librustc_mir/interpret/machine.rs @@ -251,7 +251,7 @@ pub trait Machine<'mir, 'tcx>: Sized { /// `canonical_alloc_id` would result in pointer pointing to `t2`'s thread /// local and not `t1` as it should. #[inline] - fn eval_maybe_thread_local_static_const( + fn adjust_global_const( _ecx: &InterpCx<'mir, 'tcx, Self>, val: mir::interpret::ConstValue<'tcx>, ) -> InterpResult<'tcx, mir::interpret::ConstValue<'tcx>> { diff --git a/src/librustc_mir/interpret/operand.rs b/src/librustc_mir/interpret/operand.rs index df9ce3f18a9fe..450d5500cfd56 100644 --- a/src/librustc_mir/interpret/operand.rs +++ b/src/librustc_mir/interpret/operand.rs @@ -537,7 +537,12 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } ty::ConstKind::Value(val_val) => val_val, }; - let val_val = M::eval_maybe_thread_local_static_const(self, val_val)?; + // This call allows the machine to create fresh allocation ids for + // thread-local statics (see the `adjust_global_const` function + // documentation). Please note that the `const_eval` call in the early + // return above calls `eval_const_to_op` again, so `adjust_global_const` + // is guaranteed to be called for all constants. + let val_val = M::adjust_global_const(self, val_val)?; // Other cases need layout. let layout = from_known_layout(self.tcx, layout, || self.layout_of(val.ty))?; let op = match val_val { From d41f8bf5fdc9ae9588cfcac968a24b131f9707ee Mon Sep 17 00:00:00 2001 From: Vytautas Astrauskas Date: Thu, 16 Apr 2020 08:09:42 -0700 Subject: [PATCH 7/8] Move the explanation why adjust_global_const is called for all constants. --- src/librustc_mir/interpret/operand.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/librustc_mir/interpret/operand.rs b/src/librustc_mir/interpret/operand.rs index 450d5500cfd56..678a81bc534fc 100644 --- a/src/librustc_mir/interpret/operand.rs +++ b/src/librustc_mir/interpret/operand.rs @@ -528,6 +528,10 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // potentially requiring the current static to be evaluated again. This is not a // problem here, because we are building an operand which means an actual read is // happening. + // + // The machine callback `adjust_global_const` below is guaranteed to + // be called for all constants because `const_eval` calls + // `eval_const_to_op` recursively. return Ok(self.const_eval(GlobalId { instance, promoted }, val.ty)?); } ty::ConstKind::Infer(..) @@ -539,9 +543,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { }; // This call allows the machine to create fresh allocation ids for // thread-local statics (see the `adjust_global_const` function - // documentation). Please note that the `const_eval` call in the early - // return above calls `eval_const_to_op` again, so `adjust_global_const` - // is guaranteed to be called for all constants. + // documentation). let val_val = M::adjust_global_const(self, val_val)?; // Other cases need layout. let layout = from_known_layout(self.tcx, layout, || self.layout_of(val.ty))?; From cbd922288e8f07361645a07fe602f97b54d3afd2 Mon Sep 17 00:00:00 2001 From: Vytautas Astrauskas Date: Thu, 16 Apr 2020 09:57:12 -0700 Subject: [PATCH 8/8] Move stack access methods in the Machine implementations out of the enforce_ method group. --- src/librustc_mir/const_eval/machine.rs | 28 ++++++++++++------------ src/librustc_mir/transform/const_prop.rs | 28 ++++++++++++------------ 2 files changed, 28 insertions(+), 28 deletions(-) diff --git a/src/librustc_mir/const_eval/machine.rs b/src/librustc_mir/const_eval/machine.rs index 41ca753e79c9f..30990853b516a 100644 --- a/src/librustc_mir/const_eval/machine.rs +++ b/src/librustc_mir/const_eval/machine.rs @@ -190,20 +190,6 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir, false } - #[inline(always)] - fn stack( - ecx: &'a InterpCx<'mir, 'tcx, Self>, - ) -> &'a [Frame<'mir, 'tcx, Self::PointerTag, Self::FrameExtra>] { - &ecx.machine.stack - } - - #[inline(always)] - fn stack_mut( - ecx: &'a mut InterpCx<'mir, 'tcx, Self>, - ) -> &'a mut Vec> { - &mut ecx.machine.stack - } - #[inline(always)] fn enforce_validity(_ecx: &InterpCx<'mir, 'tcx, Self>) -> bool { false // for now, we don't enforce validity @@ -367,6 +353,20 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir, Ok(frame) } + #[inline(always)] + fn stack( + ecx: &'a InterpCx<'mir, 'tcx, Self>, + ) -> &'a [Frame<'mir, 'tcx, Self::PointerTag, Self::FrameExtra>] { + &ecx.machine.stack + } + + #[inline(always)] + fn stack_mut( + ecx: &'a mut InterpCx<'mir, 'tcx, Self>, + ) -> &'a mut Vec> { + &mut ecx.machine.stack + } + fn before_access_global( memory_extra: &MemoryExtra, alloc_id: AllocId, diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index b7d7fc29f3040..77a9be0ed35d8 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -187,20 +187,6 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine<'mir, 'tcx> false } - #[inline(always)] - fn stack( - ecx: &'a InterpCx<'mir, 'tcx, Self>, - ) -> &'a [Frame<'mir, 'tcx, Self::PointerTag, Self::FrameExtra>] { - &ecx.machine.stack - } - - #[inline(always)] - fn stack_mut( - ecx: &'a mut InterpCx<'mir, 'tcx, Self>, - ) -> &'a mut Vec> { - &mut ecx.machine.stack - } - #[inline(always)] fn enforce_validity(_ecx: &InterpCx<'mir, 'tcx, Self>) -> bool { false @@ -319,6 +305,20 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine<'mir, 'tcx> ) -> InterpResult<'tcx, Frame<'mir, 'tcx>> { Ok(frame) } + + #[inline(always)] + fn stack( + ecx: &'a InterpCx<'mir, 'tcx, Self>, + ) -> &'a [Frame<'mir, 'tcx, Self::PointerTag, Self::FrameExtra>] { + &ecx.machine.stack + } + + #[inline(always)] + fn stack_mut( + ecx: &'a mut InterpCx<'mir, 'tcx, Self>, + ) -> &'a mut Vec> { + &mut ecx.machine.stack + } } /// Finds optimization opportunities on the MIR.