From 187d05f2159b216cb16142967f897103449fac31 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Tue, 16 Apr 2019 21:04:54 -0400 Subject: [PATCH 01/29] Add hooks for Miri panic unwinding This commits adds in some additional hooks to allow Miri to properly handle panic unwinding. None of this should have any impact on CTFE mode --- src/librustc_mir/const_eval.rs | 11 +- src/librustc_mir/interpret/eval_context.rs | 173 +++++++++++++++++---- src/librustc_mir/interpret/intrinsics.rs | 11 +- src/librustc_mir/interpret/machine.rs | 21 ++- src/librustc_mir/interpret/mod.rs | 2 +- src/librustc_mir/interpret/snapshot.rs | 2 +- src/librustc_mir/interpret/step.rs | 1 + src/librustc_mir/interpret/terminator.rs | 49 ++++-- 8 files changed, 211 insertions(+), 59 deletions(-) diff --git a/src/librustc_mir/const_eval.rs b/src/librustc_mir/const_eval.rs index 707ad1511826a..1af8190e569f7 100644 --- a/src/librustc_mir/const_eval.rs +++ b/src/librustc_mir/const_eval.rs @@ -24,7 +24,7 @@ use crate::interpret::{self, PlaceTy, MPlaceTy, OpTy, ImmTy, Immediate, Scalar, Pointer, RawConst, ConstValue, Machine, InterpResult, InterpErrorInfo, GlobalId, InterpCx, StackPopCleanup, - Allocation, AllocId, MemoryKind, Memory, + Allocation, AllocId, MemoryKind, Memory, StackPopInfo, snapshot, RefTracking, intern_const_alloc_recursive, }; @@ -325,6 +325,7 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir, args: &[OpTy<'tcx>], dest: Option>, ret: Option, + _unwind: Option // unwinding is not supported in consts ) -> InterpResult<'tcx, Option<&'mir mir::Body<'tcx>>> { debug!("eval_fn_call: {:?}", instance); // Only check non-glue functions @@ -374,7 +375,7 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir, span: Span, instance: ty::Instance<'tcx>, args: &[OpTy<'tcx>], - dest: PlaceTy<'tcx>, + dest: Option>, ) -> InterpResult<'tcx> { if ecx.emulate_intrinsic(span, instance, args, dest)? { return Ok(()); @@ -472,8 +473,10 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir, /// Called immediately before a stack frame gets popped. #[inline(always)] - fn stack_pop(_ecx: &mut InterpCx<'mir, 'tcx, Self>, _extra: ()) -> InterpResult<'tcx> { - Ok(()) + fn stack_pop( + _ecx: &mut InterpCx<'mir, 'tcx, Self>, _extra: ()) -> InterpResult<'tcx, StackPopInfo> { + // Const-eval mode does not support unwinding from panics + Ok(StackPopInfo::Normal) } } diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index 8e901068a8d26..6371ced2f6bca 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/src/librustc_mir/interpret/eval_context.rs @@ -21,7 +21,7 @@ use rustc_data_structures::fx::FxHashMap; use super::{ Immediate, Operand, MemPlace, MPlaceTy, Place, PlaceTy, ScalarMaybeUndef, - Memory, Machine + Memory, Machine, PointerArithmetic, FnVal, StackPopInfo }; pub struct InterpCx<'mir, 'tcx, M: Machine<'mir, 'tcx>> { @@ -96,7 +96,9 @@ pub enum StackPopCleanup { /// Jump to the next block in the caller, or cause UB if None (that's a function /// that may never return). Also store layout of return place so /// we can validate it at that layout. - Goto(Option), + /// 'ret' stores the block we jump to on a normal return, while 'unwind' + /// stores the block used for cleanup during unwinding + Goto { ret: Option, unwind: Option }, /// Just do nohing: Used by Main and for the box_alloc hook in miri. /// `cleanup` says whether locals are deallocated. Static computation /// wants them leaked to intern what they need (and just throw away @@ -547,22 +549,30 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } } - pub(super) fn pop_stack_frame(&mut self) -> InterpResult<'tcx> { - info!("LEAVING({}) {}", self.cur_frame(), self.frame().instance); + pub(super) fn pop_stack_frame_internal( + &mut self, + unwinding: bool + ) -> InterpResult<'tcx, (StackPopCleanup, StackPopInfo)> { + info!("LEAVING({}) {} (unwinding = {})", + self.cur_frame(), self.frame().instance, unwinding); + ::log_settings::settings().indentation -= 1; let frame = self.stack.pop().expect( "tried to pop a stack frame, but there were none", ); - M::stack_pop(self, frame.extra)?; + let stack_pop_info = M::stack_pop(self, frame.extra)?; + // Abort early if we do not want to clean up: We also avoid validation in that case, // because this is CTFE and the final value will be thoroughly validated anyway. match frame.return_to_block { - StackPopCleanup::Goto(_) => {}, - StackPopCleanup::None { cleanup } => { + StackPopCleanup::Goto{ .. } => {}, + StackPopCleanup::None { cleanup, .. } => { + assert!(!unwinding, "Encountered StackPopCleanup::None while unwinding"); + if !cleanup { assert!(self.stack.is_empty(), "only the topmost frame should ever be leaked"); // Leak the locals, skip validation. - return Ok(()); + return Ok((frame.return_to_block, stack_pop_info)); } } } @@ -570,33 +580,111 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { for local in frame.locals { self.deallocate_local(local.value)?; } - // Validate the return value. Do this after deallocating so that we catch dangling - // references. - if let Some(return_place) = frame.return_place { - if M::enforce_validity(self) { - // Data got changed, better make sure it matches the type! - // It is still possible that the return place held invalid data while - // the function is running, but that's okay because nobody could have - // accessed that same data from the "outside" to observe any broken - // invariant -- that is, unless a function somehow has a ptr to - // its return place... but the way MIR is currently generated, the - // return place is always a local and then this cannot happen. - self.validate_operand( - self.place_to_op(return_place)?, - vec![], - None, - )?; + + // If we're popping frames due to unwinding, and we didn't just exit + // unwinding, we skip a bunch of validation and cleanup logic (including + // jumping to the regular return block specified in the StackPopCleanu) + let cur_unwinding = unwinding && stack_pop_info != StackPopInfo::StopUnwinding; + + info!("StackPopCleanup: {:?} StackPopInfo: {:?} cur_unwinding = {:?}", + frame.return_to_block, stack_pop_info, cur_unwinding); + + + // When we're popping a stack frame for unwinding purposes, + // we don't care at all about returning-related stuff (i.e. return_place + // and return_to_block), because we're not performing a return from this frame. + if !cur_unwinding { + // Validate the return value. Do this after deallocating so that we catch dangling + // references. + if let Some(return_place) = frame.return_place { + if M::enforce_validity(self) { + // Data got changed, better make sure it matches the type! + // It is still possible that the return place held invalid data while + // the function is running, but that's okay because nobody could have + // accessed that same data from the "outside" to observe any broken + // invariant -- that is, unless a function somehow has a ptr to + // its return place... but the way MIR is currently generated, the + // return place is always a local and then this cannot happen. + self.validate_operand( + self.place_to_op(return_place)?, + vec![], + None, + )?; + } + } else { + // Uh, that shouldn't happen... the function did not intend to return + throw_ub!(Unreachable); + } + + // Jump to new block -- *after* validation so that the spans make more sense. + match frame.return_to_block { + StackPopCleanup::Goto { ret, .. } => { + self.goto_block(ret)?; + } + StackPopCleanup::None { .. } => {} } - } else { - // Uh, that shouldn't happen... the function did not intend to return - throw_ub!(Unreachable) } - // Jump to new block -- *after* validation so that the spans make more sense. - match frame.return_to_block { - StackPopCleanup::Goto(block) => { - self.goto_block(block)?; + + + Ok((frame.return_to_block, stack_pop_info)) + } + + pub(super) fn pop_stack_frame(&mut self, unwinding: bool) -> InterpResult<'tcx> { + let (mut cleanup, mut stack_pop_info) = self.pop_stack_frame_internal(unwinding)?; + + // There are two cases where we want to unwind the stack: + // * The caller explicitly told us (i.e. we hit a Resume terminator) + // * The machine indicated that we've just started unwinding (i.e. + // a panic has just occured) + if unwinding || stack_pop_info == StackPopInfo::StartUnwinding { + trace!("unwinding: starting stack unwind..."); + // Overwrite our current stack_pop_info, so that the check + // below doesn't fail. + stack_pop_info = StackPopInfo::Normal; + // There are three posible ways that we can exit the loop: + // 1) We find an unwind block - we jump to it to allow cleanup + // to occur for that frame + // 2) pop_stack_frame_internal reports that we're no longer unwinding + // - this means that the panic has been caught, and that execution + // should continue as normal + // 3) We pop all of our frames off the stack - this should never happen. + while !self.stack.is_empty() { + match stack_pop_info { + // We tried to start unwinding while we were already + // unwinding. Note that this **is not** the same thing + // as a double panic, which will be intercepted by + // libcore/libstd before we actually attempt to unwind. + StackPopInfo::StartUnwinding => { + throw_ub_format!("Attempted to start unwinding while already unwinding!"); + }, + StackPopInfo::StopUnwinding => { + trace!("unwinding: no longer unwinding!"); + break; + } + StackPopInfo::Normal => {} + } + + match cleanup { + StackPopCleanup::Goto { unwind, .. } if unwind.is_some() => { + + info!("unwind: found cleanup block {:?}", unwind); + self.goto_block(unwind)?; + break; + }, + _ => {} + } + + info!("unwinding: popping frame!"); + let res = self.pop_stack_frame_internal(true)?; + cleanup = res.0; + stack_pop_info = res.1; + } + if self.stack.is_empty() { + // We should never get here: + // The 'start_fn' lang item should always install a panic handler + throw_ub!(Unreachable); } - StackPopCleanup::None { .. } => {} + } if self.stack.len() > 0 { @@ -760,4 +848,25 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { trace!("generate stacktrace: {:#?}, {:?}", frames, explicit_span); frames } + + /// Resolve the function at the specified slot in the provided + /// vtable. An index of '0' corresponds to the first method + /// declared in the trait of the provided vtable + pub fn get_vtable_slot( + &self, + vtable: Scalar, + idx: usize + ) -> InterpResult<'tcx, FnVal<'tcx, M::ExtraFnVal>> { + let ptr_size = self.pointer_size(); + // Skip over the 'drop_ptr', 'size', and 'align' fields + let vtable_slot = vtable.ptr_offset(ptr_size * (idx as u64 + 3), self)?; + let vtable_slot = self.memory.check_ptr_access( + vtable_slot, + ptr_size, + self.tcx.data_layout.pointer_align.abi, + )?.expect("cannot be a ZST"); + let fn_ptr = self.memory.get(vtable_slot.alloc_id)? + .read_ptr_sized(self, vtable_slot)?.not_undef()?; + Ok(self.memory.get_fn(fn_ptr)?) + } } diff --git a/src/librustc_mir/interpret/intrinsics.rs b/src/librustc_mir/interpret/intrinsics.rs index 39f10d8e6045d..d6d53a740bb2a 100644 --- a/src/librustc_mir/interpret/intrinsics.rs +++ b/src/librustc_mir/interpret/intrinsics.rs @@ -91,11 +91,18 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { span: Span, instance: ty::Instance<'tcx>, args: &[OpTy<'tcx, M::PointerTag>], - dest: PlaceTy<'tcx, M::PointerTag>, + dest: Option>, ) -> InterpResult<'tcx, bool> { let substs = instance.substs; - let intrinsic_name = &*self.tcx.item_name(instance.def_id()).as_str(); + // The intrinsic itself cannot diverge, so if we got here without a return + // place... (can happen e.g., for transmute returning `!`) + let dest = match dest { + Some(dest) => dest, + None => throw_ub!(Unreachable) + }; + let intrinsic_name = &self.tcx.item_name(instance.def_id()).as_str(); + match intrinsic_name { "caller_location" => { let topmost = span.ctxt().outer_expn().expansion_cause().unwrap_or(span); diff --git a/src/librustc_mir/interpret/machine.rs b/src/librustc_mir/interpret/machine.rs index 870e50a3cbb9a..78c789fc1ca3f 100644 --- a/src/librustc_mir/interpret/machine.rs +++ b/src/librustc_mir/interpret/machine.rs @@ -16,6 +16,22 @@ use super::{ Frame, Operand, }; +/// Data returned by Machine::stack_pop, +/// to provide further control over the popping of the stack frame +#[derive(Eq, PartialEq, Debug)] +pub enum StackPopInfo { + /// Indicates that we have just started unwinding + /// as the result of panic + StartUnwinding, + + /// Indicates that we performed a normal return + Normal, + + /// Indicates that we should stop unwinding, + /// as we've reached a catch frame + StopUnwinding +} + /// Whether this kind of memory is allowed to leak pub trait MayLeak: Copy { fn may_leak(self) -> bool; @@ -137,6 +153,7 @@ pub trait Machine<'mir, 'tcx>: Sized { args: &[OpTy<'tcx, Self::PointerTag>], dest: Option>, ret: Option, + unwind: Option ) -> InterpResult<'tcx, Option<&'mir mir::Body<'tcx>>>; /// Execute `fn_val`. it is the hook's responsibility to advance the instruction @@ -156,7 +173,7 @@ pub trait Machine<'mir, 'tcx>: Sized { span: Span, instance: ty::Instance<'tcx>, args: &[OpTy<'tcx, Self::PointerTag>], - dest: PlaceTy<'tcx, Self::PointerTag>, + dest: Option>, ) -> InterpResult<'tcx>; /// Called for read access to a foreign static item. @@ -253,7 +270,7 @@ pub trait Machine<'mir, 'tcx>: Sized { fn stack_pop( ecx: &mut InterpCx<'mir, 'tcx, Self>, extra: Self::FrameExtra, - ) -> InterpResult<'tcx>; + ) -> InterpResult<'tcx, StackPopInfo>; fn int_to_ptr( _mem: &Memory<'mir, 'tcx, Self>, diff --git a/src/librustc_mir/interpret/mod.rs b/src/librustc_mir/interpret/mod.rs index 0c61be283dfd0..520bd434faa25 100644 --- a/src/librustc_mir/interpret/mod.rs +++ b/src/librustc_mir/interpret/mod.rs @@ -26,7 +26,7 @@ pub use self::place::{Place, PlaceTy, MemPlace, MPlaceTy}; pub use self::memory::{Memory, MemoryKind, AllocCheck, FnVal}; -pub use self::machine::{Machine, AllocMap, MayLeak}; +pub use self::machine::{Machine, AllocMap, MayLeak, StackPopInfo}; pub use self::operand::{ScalarMaybeUndef, Immediate, ImmTy, Operand, OpTy}; diff --git a/src/librustc_mir/interpret/snapshot.rs b/src/librustc_mir/interpret/snapshot.rs index 1df98f079cc10..a463263120c86 100644 --- a/src/librustc_mir/interpret/snapshot.rs +++ b/src/librustc_mir/interpret/snapshot.rs @@ -315,7 +315,7 @@ impl<'a, Ctx> Snapshot<'a, Ctx> for &'a Allocation } impl_stable_hash_for!(enum crate::interpret::eval_context::StackPopCleanup { - Goto(block), + Goto { ret, unwind }, None { cleanup }, }); diff --git a/src/librustc_mir/interpret/step.rs b/src/librustc_mir/interpret/step.rs index daca7a25787ca..96e5a372afdde 100644 --- a/src/librustc_mir/interpret/step.rs +++ b/src/librustc_mir/interpret/step.rs @@ -290,6 +290,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { let old_stack = self.cur_frame(); let old_bb = self.frame().block; + self.eval_terminator(terminator)?; if !self.stack.is_empty() { // This should change *something* diff --git a/src/librustc_mir/interpret/terminator.rs b/src/librustc_mir/interpret/terminator.rs index e10bb85d52df8..af6fe39973999 100644 --- a/src/librustc_mir/interpret/terminator.rs +++ b/src/librustc_mir/interpret/terminator.rs @@ -31,7 +31,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { match terminator.kind { Return => { self.frame().return_place.map(|r| self.dump_place(*r)); - self.pop_stack_frame()? + self.pop_stack_frame(/* unwinding */ false)? } Goto { target } => self.goto_block(Some(target))?, @@ -67,6 +67,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { ref func, ref args, ref destination, + ref cleanup, .. } => { let (dest, ret) = match *destination { @@ -98,13 +99,14 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { &args[..], dest, ret, + *cleanup )?; } Drop { ref location, target, - .. + unwind, } => { // FIXME(CTFE): forbid drop in const eval let place = self.eval_place(location)?; @@ -117,6 +119,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { instance, terminator.source_info.span, target, + unwind )?; } @@ -160,10 +163,21 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } } + + // When we encounter Resume, we've finished unwinding + // cleanup for the current stack frame. We pop it in order + // to continue unwinding the next frame + Resume => { + trace!("unwinding: resuming from cleanup"); + // By definition, a Resume terminaor means + // that we're unwinding + self.pop_stack_frame(/* unwinding */ true)?; + return Ok(()) + }, + Yield { .. } | GeneratorDrop | DropAndReplace { .. } | - Resume | Abort => unimplemented!("{:#?}", terminator.kind), FalseEdges { .. } => bug!("should have been eliminated by\ `simplify_branches` mir pass"), @@ -237,6 +251,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { args: &[OpTy<'tcx, M::PointerTag>], dest: Option>, ret: Option, + unwind: Option ) -> InterpResult<'tcx> { trace!("eval_fn_call: {:#?}", fn_val); @@ -249,6 +264,11 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { match instance.def { ty::InstanceDef::Intrinsic(..) => { + + if caller_abi != Abi::RustIntrinsic { + throw_unsup!(FunctionAbiMismatch(caller_abi, Abi::RustIntrinsic)) + } + // The intrinsic itself cannot diverge, so if we got here without a return // place... (can happen e.g., for transmute returning `!`) let dest = match dest { @@ -259,7 +279,9 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // No stack frame gets pushed, the main loop will just act as if the // call completed. self.goto_block(ret)?; - self.dump_place(*dest); + if let Some(dest) = dest { + self.dump_place(*dest) + } Ok(()) } ty::InstanceDef::VtableShim(..) | @@ -294,7 +316,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } // We need MIR for this fn - let body = match M::find_fn(self, instance, args, dest, ret)? { + let body = match M::find_fn(self, instance, args, dest, ret, unwind)? { Some(body) => body, None => return Ok(()), }; @@ -304,7 +326,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { span, body, dest, - StackPopCleanup::Goto(ret), + StackPopCleanup::Goto { ret, unwind } )?; // We want to pop this frame again in case there was an error, to put @@ -422,7 +444,6 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // cannot use the shim here, because that will only result in infinite recursion ty::InstanceDef::Virtual(_, idx) => { let mut args = args.to_vec(); - let ptr_size = self.pointer_size(); // We have to implement all "object safe receivers". Currently we // support built-in pointers (&, &mut, Box) as well as unsized-self. We do // not yet support custom self types. @@ -439,15 +460,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { }; // Find and consult vtable let vtable = receiver_place.vtable(); - let vtable_slot = vtable.ptr_offset(ptr_size * (idx as u64 + 3), self)?; - let vtable_slot = self.memory.check_ptr_access( - vtable_slot, - ptr_size, - self.tcx.data_layout.pointer_align.abi, - )?.expect("cannot be a ZST"); - let fn_ptr = self.memory.get_raw(vtable_slot.alloc_id)? - .read_ptr_sized(self, vtable_slot)?.not_undef()?; - let drop_fn = self.memory.get_fn(fn_ptr)?; + let drop_fn = self.get_vtable_slot(vtable, idx)?; // `*mut receiver_place.layout.ty` is almost the layout that we // want for args[0]: We have to project to field 0 because we want @@ -462,7 +475,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { }); trace!("Patched self operand to {:#?}", args[0]); // recurse with concrete function - self.eval_fn_call(drop_fn, span, caller_abi, &args, dest, ret) + self.eval_fn_call(drop_fn, span, caller_abi, &args, dest, ret, unwind) } } } @@ -473,6 +486,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { instance: ty::Instance<'tcx>, span: Span, target: mir::BasicBlock, + unwind: Option ) -> InterpResult<'tcx> { trace!("drop_in_place: {:?},\n {:?}, {:?}", *place, place.layout.ty, instance); // We take the address of the object. This may well be unaligned, which is fine @@ -503,6 +517,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { &[arg.into()], Some(dest.into()), Some(target), + unwind ) } } From 199101822ad4595760055151318fa1c6d7733dbd Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Thu, 19 Sep 2019 13:47:59 -0400 Subject: [PATCH 02/29] Formatting improvements Co-Authored-By: Oliver Scherer --- src/librustc_mir/const_eval.rs | 3 ++- src/librustc_mir/interpret/eval_context.rs | 2 +- src/librustc_mir/interpret/terminator.rs | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/librustc_mir/const_eval.rs b/src/librustc_mir/const_eval.rs index 1af8190e569f7..83d86bf2934d1 100644 --- a/src/librustc_mir/const_eval.rs +++ b/src/librustc_mir/const_eval.rs @@ -474,7 +474,8 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir, /// Called immediately before a stack frame gets popped. #[inline(always)] fn stack_pop( - _ecx: &mut InterpCx<'mir, 'tcx, Self>, _extra: ()) -> InterpResult<'tcx, StackPopInfo> { + _ecx: &mut InterpCx<'mir, 'tcx, Self>, _extra: (), + ) -> InterpResult<'tcx, StackPopInfo> { // Const-eval mode does not support unwinding from panics Ok(StackPopInfo::Normal) } diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index 6371ced2f6bca..d126d10620bdb 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/src/librustc_mir/interpret/eval_context.rs @@ -583,7 +583,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // If we're popping frames due to unwinding, and we didn't just exit // unwinding, we skip a bunch of validation and cleanup logic (including - // jumping to the regular return block specified in the StackPopCleanu) + // jumping to the regular return block specified in the StackPopCleanup) let cur_unwinding = unwinding && stack_pop_info != StackPopInfo::StopUnwinding; info!("StackPopCleanup: {:?} StackPopInfo: {:?} cur_unwinding = {:?}", diff --git a/src/librustc_mir/interpret/terminator.rs b/src/librustc_mir/interpret/terminator.rs index af6fe39973999..983497dc4297e 100644 --- a/src/librustc_mir/interpret/terminator.rs +++ b/src/librustc_mir/interpret/terminator.rs @@ -169,7 +169,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // to continue unwinding the next frame Resume => { trace!("unwinding: resuming from cleanup"); - // By definition, a Resume terminaor means + // By definition, a Resume terminator means // that we're unwinding self.pop_stack_frame(/* unwinding */ true)?; return Ok(()) From fa4f1b79ecfb0f852a82dc817f878f7aaf1ddc37 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Thu, 26 Sep 2019 13:11:56 -0400 Subject: [PATCH 03/29] Fix incorrect unwrap of dest --- src/librustc_mir/interpret/terminator.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/librustc_mir/interpret/terminator.rs b/src/librustc_mir/interpret/terminator.rs index 983497dc4297e..c8fc0409f0704 100644 --- a/src/librustc_mir/interpret/terminator.rs +++ b/src/librustc_mir/interpret/terminator.rs @@ -269,12 +269,6 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { throw_unsup!(FunctionAbiMismatch(caller_abi, Abi::RustIntrinsic)) } - // The intrinsic itself cannot diverge, so if we got here without a return - // place... (can happen e.g., for transmute returning `!`) - let dest = match dest { - Some(dest) => dest, - None => throw_ub!(Unreachable) - }; M::call_intrinsic(self, span, instance, args, dest)?; // No stack frame gets pushed, the main loop will just act as if the // call completed. From d87e2daccfe6d7759ee4b7612858ac9f1466df3d Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Thu, 26 Sep 2019 13:45:26 -0400 Subject: [PATCH 04/29] Remove old intrinsic check --- src/librustc_mir/interpret/terminator.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/librustc_mir/interpret/terminator.rs b/src/librustc_mir/interpret/terminator.rs index c8fc0409f0704..f0ef1ef192f42 100644 --- a/src/librustc_mir/interpret/terminator.rs +++ b/src/librustc_mir/interpret/terminator.rs @@ -264,11 +264,6 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { match instance.def { ty::InstanceDef::Intrinsic(..) => { - - if caller_abi != Abi::RustIntrinsic { - throw_unsup!(FunctionAbiMismatch(caller_abi, Abi::RustIntrinsic)) - } - M::call_intrinsic(self, span, instance, args, dest)?; // No stack frame gets pushed, the main loop will just act as if the // call completed. From 499eb5d8315b0a8715e1f21e291fd70cb00e46ce Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Wed, 9 Oct 2019 17:33:41 -0400 Subject: [PATCH 05/29] More fixes for rustc changes --- src/librustc_mir/transform/const_prop.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index a0d04bd593212..5c83fb8c821f6 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, + StackPopInfo, Operand as InterpOperand, }; use crate::const_eval::error_to_const_error; use crate::transform::{MirPass, MirSource}; @@ -143,6 +143,7 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine { _args: &[OpTy<'tcx>], _dest: Option>, _ret: Option, + _unwind: Option, ) -> InterpResult<'tcx, Option<&'mir Body<'tcx>>> { Ok(None) } @@ -162,7 +163,7 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine { _span: Span, _instance: ty::Instance<'tcx>, _args: &[OpTy<'tcx>], - _dest: PlaceTy<'tcx>, + _dest: Option>, ) -> InterpResult<'tcx> { throw_unsup_format!("calling intrinsics isn't supported in ConstProp"); } @@ -254,8 +255,11 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine { /// Called immediately before a stack frame gets popped. #[inline(always)] - fn stack_pop(_ecx: &mut InterpCx<'mir, 'tcx, Self>, _extra: ()) -> InterpResult<'tcx> { - Ok(()) + fn stack_pop( + _ecx: &mut InterpCx<'mir, 'tcx, Self>, + _extra: () + ) -> InterpResult<'tcx, StackPopInfo> { + Ok(StackPopInfo::Normal) } } From 01c11f9fb501cbaea01bc94fcdae7e1e32f73581 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 20 Oct 2019 18:51:25 +0200 Subject: [PATCH 06/29] avoid the loop in unwinding stack popping --- src/librustc_mir/interpret/eval_context.rs | 173 +++++++++------------ src/librustc_mir/interpret/machine.rs | 2 +- src/librustc_mir/interpret/snapshot.rs | 4 +- src/librustc_mir/interpret/step.rs | 11 +- src/librustc_mir/interpret/terminator.rs | 2 +- 5 files changed, 85 insertions(+), 107 deletions(-) diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index d126d10620bdb..b02a42ff0d8d5 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/src/librustc_mir/interpret/eval_context.rs @@ -60,6 +60,9 @@ pub struct Frame<'mir, 'tcx, Tag=(), Extra=()> { /// The span of the call site. pub span: source_map::Span, + /// Extra data for the machine. + pub extra: Extra, + //////////////////////////////////////////////////////////////////////////////// // Return place and locals //////////////////////////////////////////////////////////////////////////////// @@ -82,13 +85,12 @@ pub struct Frame<'mir, 'tcx, Tag=(), Extra=()> { //////////////////////////////////////////////////////////////////////////////// /// The block that is currently executed (or will be executed after the above call stacks /// return). - pub block: mir::BasicBlock, + /// If this is `None`, we are unwinding and this function doesn't need any clean-up. + /// Just continue the same as with + pub block: Option, /// The index of the currently evaluated statement. pub stmt: usize, - - /// Extra data for the machine. - pub extra: Extra, } #[derive(Clone, Eq, PartialEq, Debug)] // Miri debug-prints these @@ -491,7 +493,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { let extra = M::stack_push(self)?; self.stack.push(Frame { body, - block: mir::START_BLOCK, + block: Some(mir::START_BLOCK), return_to_block, return_place, // empty local array, we fill it in below, after we are inside the stack frame and @@ -549,51 +551,75 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } } - pub(super) fn pop_stack_frame_internal( + pub(super) fn pop_stack_frame( &mut self, unwinding: bool - ) -> InterpResult<'tcx, (StackPopCleanup, StackPopInfo)> { + ) -> InterpResult<'tcx> { info!("LEAVING({}) {} (unwinding = {})", self.cur_frame(), self.frame().instance, unwinding); + // Sanity check `unwinding`. + assert_eq!( + unwinding, + match self.frame().block { + None => true, + Some(block) => self.body().basic_blocks()[block].is_cleanup + } + ); + ::log_settings::settings().indentation -= 1; let frame = self.stack.pop().expect( "tried to pop a stack frame, but there were none", ); let stack_pop_info = M::stack_pop(self, frame.extra)?; + match (unwinding, stack_pop_info) { + (true, StackPopInfo::StartUnwinding) => + bug!("Attempted to start unwinding while already unwinding!"), + (false, StackPopInfo::StopUnwinding) => + bug!("Attempted to stop unwinding while there is no unwinding!"), + _ => {} + } - // Abort early if we do not want to clean up: We also avoid validation in that case, + // Usually we want to clean up (deallocate locals), but in a few rare cases we don't. + // In that case, we return early. We also avoid validation in that case, // because this is CTFE and the final value will be thoroughly validated anyway. - match frame.return_to_block { - StackPopCleanup::Goto{ .. } => {}, + let cleanup = unwinding || match frame.return_to_block { + StackPopCleanup::Goto{ .. } => true, StackPopCleanup::None { cleanup, .. } => { - assert!(!unwinding, "Encountered StackPopCleanup::None while unwinding"); - - if !cleanup { - assert!(self.stack.is_empty(), "only the topmost frame should ever be leaked"); - // Leak the locals, skip validation. - return Ok((frame.return_to_block, stack_pop_info)); - } + cleanup } + }; + if !cleanup { + assert!(self.stack.is_empty(), "only the topmost frame should ever be leaked"); + // Leak the locals, skip validation. + return Ok(()); } - // Deallocate all locals that are backed by an allocation. + + // Cleanup: deallocate all locals that are backed by an allocation. for local in frame.locals { self.deallocate_local(local.value)?; } - // If we're popping frames due to unwinding, and we didn't just exit - // unwinding, we skip a bunch of validation and cleanup logic (including - // jumping to the regular return block specified in the StackPopCleanup) - let cur_unwinding = unwinding && stack_pop_info != StackPopInfo::StopUnwinding; + // Now where do we jump next? - info!("StackPopCleanup: {:?} StackPopInfo: {:?} cur_unwinding = {:?}", + // Determine if we leave this function normally or via unwinding. + let cur_unwinding = unwinding && stack_pop_info != StackPopInfo::StopUnwinding; + trace!("StackPopCleanup: {:?} StackPopInfo: {:?} cur_unwinding = {:?}", frame.return_to_block, stack_pop_info, cur_unwinding); - - - // When we're popping a stack frame for unwinding purposes, - // we don't care at all about returning-related stuff (i.e. return_place - // and return_to_block), because we're not performing a return from this frame. - if !cur_unwinding { + if cur_unwinding { + // Follow the unwind edge. + match frame.return_to_block { + StackPopCleanup::Goto { unwind, .. } => { + let next_frame = self.frame_mut(); + // If `unwind` is `None`, we'll leave that function immediately again. + next_frame.block = unwind; + next_frame.stmt = 0; + }, + StackPopCleanup::None { .. } => + bug!("Encountered StackPopCleanup::None while unwinding"), + } + } else { + // Follow the normal return edge. // Validate the return value. Do this after deallocating so that we catch dangling // references. if let Some(return_place) = frame.return_place { @@ -625,70 +651,9 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } } - - Ok((frame.return_to_block, stack_pop_info)) - } - - pub(super) fn pop_stack_frame(&mut self, unwinding: bool) -> InterpResult<'tcx> { - let (mut cleanup, mut stack_pop_info) = self.pop_stack_frame_internal(unwinding)?; - - // There are two cases where we want to unwind the stack: - // * The caller explicitly told us (i.e. we hit a Resume terminator) - // * The machine indicated that we've just started unwinding (i.e. - // a panic has just occured) - if unwinding || stack_pop_info == StackPopInfo::StartUnwinding { - trace!("unwinding: starting stack unwind..."); - // Overwrite our current stack_pop_info, so that the check - // below doesn't fail. - stack_pop_info = StackPopInfo::Normal; - // There are three posible ways that we can exit the loop: - // 1) We find an unwind block - we jump to it to allow cleanup - // to occur for that frame - // 2) pop_stack_frame_internal reports that we're no longer unwinding - // - this means that the panic has been caught, and that execution - // should continue as normal - // 3) We pop all of our frames off the stack - this should never happen. - while !self.stack.is_empty() { - match stack_pop_info { - // We tried to start unwinding while we were already - // unwinding. Note that this **is not** the same thing - // as a double panic, which will be intercepted by - // libcore/libstd before we actually attempt to unwind. - StackPopInfo::StartUnwinding => { - throw_ub_format!("Attempted to start unwinding while already unwinding!"); - }, - StackPopInfo::StopUnwinding => { - trace!("unwinding: no longer unwinding!"); - break; - } - StackPopInfo::Normal => {} - } - - match cleanup { - StackPopCleanup::Goto { unwind, .. } if unwind.is_some() => { - - info!("unwind: found cleanup block {:?}", unwind); - self.goto_block(unwind)?; - break; - }, - _ => {} - } - - info!("unwinding: popping frame!"); - let res = self.pop_stack_frame_internal(true)?; - cleanup = res.0; - stack_pop_info = res.1; - } - if self.stack.is_empty() { - // We should never get here: - // The 'start_fn' lang item should always install a panic handler - throw_ub!(Unreachable); - } - - } - if self.stack.len() > 0 { - info!("CONTINUING({}) {}", self.cur_frame(), self.frame().instance); + info!("CONTINUING({}) {} (unwinding = {})", + self.cur_frame(), self.frame().instance, cur_unwinding); } Ok(()) @@ -833,16 +798,20 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } else { last_span = Some(span); } - let block = &body.basic_blocks()[block]; - let source_info = if stmt < block.statements.len() { - block.statements[stmt].source_info - } else { - block.terminator().source_info - }; - let lint_root = match body.source_scope_local_data { - mir::ClearCrossCrate::Set(ref ivs) => Some(ivs[source_info.scope].lint_root), - mir::ClearCrossCrate::Clear => None, - }; + + let lint_root = block.and_then(|block| { + let block = &body.basic_blocks()[block]; + let source_info = if stmt < block.statements.len() { + block.statements[stmt].source_info + } else { + block.terminator().source_info + }; + match body.source_scope_local_data { + mir::ClearCrossCrate::Set(ref ivs) => Some(ivs[source_info.scope].lint_root), + mir::ClearCrossCrate::Clear => None, + } + }); + frames.push(FrameInfo { call_site: span, instance, lint_root }); } trace!("generate stacktrace: {:#?}, {:?}", frames, explicit_span); diff --git a/src/librustc_mir/interpret/machine.rs b/src/librustc_mir/interpret/machine.rs index 78c789fc1ca3f..995f1199047c3 100644 --- a/src/librustc_mir/interpret/machine.rs +++ b/src/librustc_mir/interpret/machine.rs @@ -18,7 +18,7 @@ use super::{ /// Data returned by Machine::stack_pop, /// to provide further control over the popping of the stack frame -#[derive(Eq, PartialEq, Debug)] +#[derive(Eq, PartialEq, Debug, Copy, Clone)] pub enum StackPopInfo { /// Indicates that we have just started unwinding /// as the result of panic diff --git a/src/librustc_mir/interpret/snapshot.rs b/src/librustc_mir/interpret/snapshot.rs index a463263120c86..7f3ea0283cda3 100644 --- a/src/librustc_mir/interpret/snapshot.rs +++ b/src/librustc_mir/interpret/snapshot.rs @@ -326,7 +326,7 @@ struct FrameSnapshot<'a, 'tcx> { return_to_block: &'a StackPopCleanup, return_place: Option>>, locals: IndexVec>>, - block: &'a mir::BasicBlock, + block: Option, stmt: usize, } @@ -364,7 +364,7 @@ impl<'a, 'mir, 'tcx, Ctx> Snapshot<'a, Ctx> for &'a Frame<'mir, 'tcx> instance: *instance, span: *span, return_to_block, - block, + block: *block, stmt: *stmt, return_place: return_place.map(|r| r.snapshot(ctx)), locals: locals.iter().map(|local| local.snapshot(ctx)).collect(), diff --git a/src/librustc_mir/interpret/step.rs b/src/librustc_mir/interpret/step.rs index 96e5a372afdde..d7493338a5352 100644 --- a/src/librustc_mir/interpret/step.rs +++ b/src/librustc_mir/interpret/step.rs @@ -49,7 +49,16 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { return Ok(false); } - let block = self.frame().block; + let block = match self.frame().block { + Some(block) => block, + None => { + // We are unwinding and this fn has no cleanup code. + // Just go on unwinding. + trace!("unwinding: skipping frame"); + self.pop_stack_frame(/* unwinding */ true)?; + return Ok(true) + } + }; let stmt_id = self.frame().stmt; let body = self.body(); let basic_block = &body.basic_blocks()[block]; diff --git a/src/librustc_mir/interpret/terminator.rs b/src/librustc_mir/interpret/terminator.rs index f0ef1ef192f42..bed6c5bea5436 100644 --- a/src/librustc_mir/interpret/terminator.rs +++ b/src/librustc_mir/interpret/terminator.rs @@ -15,7 +15,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { #[inline] pub fn goto_block(&mut self, target: Option) -> InterpResult<'tcx> { if let Some(target) = target { - self.frame_mut().block = target; + self.frame_mut().block = Some(target); self.frame_mut().stmt = 0; Ok(()) } else { From 64a43f45d22d0d49da5363873903e63803457e61 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sun, 20 Oct 2019 21:31:43 -0400 Subject: [PATCH 07/29] A few minor tweaks --- src/librustc_mir/const_eval.rs | 3 ++- src/librustc_mir/interpret/eval_context.rs | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/librustc_mir/const_eval.rs b/src/librustc_mir/const_eval.rs index 83d86bf2934d1..c50fae4549077 100644 --- a/src/librustc_mir/const_eval.rs +++ b/src/librustc_mir/const_eval.rs @@ -474,7 +474,8 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir, /// Called immediately before a stack frame gets popped. #[inline(always)] fn stack_pop( - _ecx: &mut InterpCx<'mir, 'tcx, Self>, _extra: (), + _ecx: &mut InterpCx<'mir, 'tcx, Self>, + _extra: (), ) -> InterpResult<'tcx, StackPopInfo> { // Const-eval mode does not support unwinding from panics Ok(StackPopInfo::Normal) diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index b02a42ff0d8d5..5be8135fee924 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/src/librustc_mir/interpret/eval_context.rs @@ -98,7 +98,7 @@ pub enum StackPopCleanup { /// Jump to the next block in the caller, or cause UB if None (that's a function /// that may never return). Also store layout of return place so /// we can validate it at that layout. - /// 'ret' stores the block we jump to on a normal return, while 'unwind' + /// `ret` stores the block we jump to on a normal return, while 'unwind' /// stores the block used for cleanup during unwinding Goto { ret: Option, unwind: Option }, /// Just do nohing: Used by Main and for the box_alloc hook in miri. From fe3e1c1cc33d0627af0e0e78582f5c64db0db2ee Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sun, 20 Oct 2019 21:40:38 -0400 Subject: [PATCH 08/29] Add doc comment --- src/librustc_mir/interpret/eval_context.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index 5be8135fee924..5e505521b7283 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/src/librustc_mir/interpret/eval_context.rs @@ -551,6 +551,19 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } } + /// Pops the current frame from the stack, deallocating the + /// memory for allocated locals. + /// + /// If `unwinding` is `false`, then we are performing a normal return + /// from a function. In this case, we jump back into the frame of the caller, + /// and continue execution as normal. + /// + /// If `unwinding` is `true`, then we are in the middle of a panic, + /// and need to unwind this frame. In this case, we jump to the + /// `cleanup` block for the function, which is responsible for running + /// `Drop` impls for any locals that have been initialized at this point. + /// The cleanup block ends with a special `Resume` terminator, which will + /// cause us to continue unwinding where we left off. pub(super) fn pop_stack_frame( &mut self, unwinding: bool From 8df4248c71deba455dab197784708be46da185fc Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Mon, 28 Oct 2019 19:09:54 -0400 Subject: [PATCH 09/29] Some cleanup --- src/librustc_mir/const_eval.rs | 12 +--------- src/librustc_mir/interpret/eval_context.rs | 27 +++------------------- src/librustc_mir/interpret/machine.rs | 9 +++++--- src/librustc_mir/interpret/traits.rs | 21 +++++++++++++++++ 4 files changed, 31 insertions(+), 38 deletions(-) diff --git a/src/librustc_mir/const_eval.rs b/src/librustc_mir/const_eval.rs index c50fae4549077..812774fab8de4 100644 --- a/src/librustc_mir/const_eval.rs +++ b/src/librustc_mir/const_eval.rs @@ -24,7 +24,7 @@ use crate::interpret::{self, PlaceTy, MPlaceTy, OpTy, ImmTy, Immediate, Scalar, Pointer, RawConst, ConstValue, Machine, InterpResult, InterpErrorInfo, GlobalId, InterpCx, StackPopCleanup, - Allocation, AllocId, MemoryKind, Memory, StackPopInfo, + Allocation, AllocId, MemoryKind, Memory, snapshot, RefTracking, intern_const_alloc_recursive, }; @@ -470,16 +470,6 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir, fn stack_push(_ecx: &mut InterpCx<'mir, 'tcx, Self>) -> InterpResult<'tcx> { Ok(()) } - - /// Called immediately before a stack frame gets popped. - #[inline(always)] - fn stack_pop( - _ecx: &mut InterpCx<'mir, 'tcx, Self>, - _extra: (), - ) -> InterpResult<'tcx, StackPopInfo> { - // Const-eval mode does not support unwinding from panics - Ok(StackPopInfo::Normal) - } } /// Extracts a field of a (variant of a) const. diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index 5e505521b7283..6598b0f99f2a9 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/src/librustc_mir/interpret/eval_context.rs @@ -21,7 +21,7 @@ use rustc_data_structures::fx::FxHashMap; use super::{ Immediate, Operand, MemPlace, MPlaceTy, Place, PlaceTy, ScalarMaybeUndef, - Memory, Machine, PointerArithmetic, FnVal, StackPopInfo + Memory, Machine, StackPopInfo }; pub struct InterpCx<'mir, 'tcx, M: Machine<'mir, 'tcx>> { @@ -86,7 +86,7 @@ pub struct Frame<'mir, 'tcx, Tag=(), Extra=()> { /// The block that is currently executed (or will be executed after the above call stacks /// return). /// If this is `None`, we are unwinding and this function doesn't need any clean-up. - /// Just continue the same as with + /// Just continue the same as with `Resume`. pub block: Option, /// The index of the currently evaluated statement. @@ -563,7 +563,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { /// `cleanup` block for the function, which is responsible for running /// `Drop` impls for any locals that have been initialized at this point. /// The cleanup block ends with a special `Resume` terminator, which will - /// cause us to continue unwinding where we left off. + /// cause us to continue unwinding. pub(super) fn pop_stack_frame( &mut self, unwinding: bool @@ -830,25 +830,4 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { trace!("generate stacktrace: {:#?}, {:?}", frames, explicit_span); frames } - - /// Resolve the function at the specified slot in the provided - /// vtable. An index of '0' corresponds to the first method - /// declared in the trait of the provided vtable - pub fn get_vtable_slot( - &self, - vtable: Scalar, - idx: usize - ) -> InterpResult<'tcx, FnVal<'tcx, M::ExtraFnVal>> { - let ptr_size = self.pointer_size(); - // Skip over the 'drop_ptr', 'size', and 'align' fields - let vtable_slot = vtable.ptr_offset(ptr_size * (idx as u64 + 3), self)?; - let vtable_slot = self.memory.check_ptr_access( - vtable_slot, - ptr_size, - self.tcx.data_layout.pointer_align.abi, - )?.expect("cannot be a ZST"); - let fn_ptr = self.memory.get(vtable_slot.alloc_id)? - .read_ptr_sized(self, vtable_slot)?.not_undef()?; - Ok(self.memory.get_fn(fn_ptr)?) - } } diff --git a/src/librustc_mir/interpret/machine.rs b/src/librustc_mir/interpret/machine.rs index 995f1199047c3..18f831f8257bc 100644 --- a/src/librustc_mir/interpret/machine.rs +++ b/src/librustc_mir/interpret/machine.rs @@ -268,9 +268,12 @@ pub trait Machine<'mir, 'tcx>: Sized { /// Called immediately after a stack frame gets popped fn stack_pop( - ecx: &mut InterpCx<'mir, 'tcx, Self>, - extra: Self::FrameExtra, - ) -> InterpResult<'tcx, StackPopInfo>; + _ecx: &mut InterpCx<'mir, 'tcx, Self>, + _extra: Self::FrameExtra, + ) -> InterpResult<'tcx, StackPopInfo> { + // By default, we do not support unwinding from panics + Ok(StackPopInfo::Normal) + } fn int_to_ptr( _mem: &Memory<'mir, 'tcx, Self>, diff --git a/src/librustc_mir/interpret/traits.rs b/src/librustc_mir/interpret/traits.rs index c15425321ec01..55c127c56f608 100644 --- a/src/librustc_mir/interpret/traits.rs +++ b/src/librustc_mir/interpret/traits.rs @@ -97,6 +97,27 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { Ok(vtable) } + /// Resolve the function at the specified slot in the provided + /// vtable. An index of '0' corresponds to the first method + /// declared in the trait of the provided vtable + pub fn get_vtable_slot( + &self, + vtable: Scalar, + idx: usize + ) -> InterpResult<'tcx, FnVal<'tcx, M::ExtraFnVal>> { + let ptr_size = self.pointer_size(); + // Skip over the 'drop_ptr', 'size', and 'align' fields + let vtable_slot = vtable.ptr_offset(ptr_size * (idx as u64 + 3), self)?; + let vtable_slot = self.memory.check_ptr_access( + vtable_slot, + ptr_size, + self.tcx.data_layout.pointer_align.abi, + )?.expect("cannot be a ZST"); + let fn_ptr = self.memory.get(vtable_slot.alloc_id)? + .read_ptr_sized(self, vtable_slot)?.not_undef()?; + Ok(self.memory.get_fn(fn_ptr)?) + } + /// Returns the drop fn instance as well as the actual dynamic type pub fn read_drop_type_from_vtable( &self, From 4f25c91a0530d2337bff2b27c938e458a95e943b Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Mon, 28 Oct 2019 21:37:58 -0400 Subject: [PATCH 10/29] Fix unwinding logic --- src/librustc_mir/interpret/eval_context.rs | 9 +++++++-- src/librustc_mir/interpret/machine.rs | 1 + src/librustc_mir/transform/const_prop.rs | 11 +---------- 3 files changed, 9 insertions(+), 12 deletions(-) diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index 6598b0f99f2a9..fb3f4f21a9734 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/src/librustc_mir/interpret/eval_context.rs @@ -584,7 +584,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { let frame = self.stack.pop().expect( "tried to pop a stack frame, but there were none", ); - let stack_pop_info = M::stack_pop(self, frame.extra)?; + let stack_pop_info = M::stack_pop(self, frame.extra, unwinding)?; match (unwinding, stack_pop_info) { (true, StackPopInfo::StartUnwinding) => bug!("Attempted to start unwinding while already unwinding!"), @@ -616,7 +616,12 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // Now where do we jump next? // Determine if we leave this function normally or via unwinding. - let cur_unwinding = unwinding && stack_pop_info != StackPopInfo::StopUnwinding; + let cur_unwinding = match stack_pop_info { + StackPopInfo::StartUnwinding => true, + StackPopInfo::StopUnwinding => false, + _ => unwinding + }; + trace!("StackPopCleanup: {:?} StackPopInfo: {:?} cur_unwinding = {:?}", frame.return_to_block, stack_pop_info, cur_unwinding); if cur_unwinding { diff --git a/src/librustc_mir/interpret/machine.rs b/src/librustc_mir/interpret/machine.rs index 18f831f8257bc..bed6c0b67eb18 100644 --- a/src/librustc_mir/interpret/machine.rs +++ b/src/librustc_mir/interpret/machine.rs @@ -270,6 +270,7 @@ pub trait Machine<'mir, 'tcx>: Sized { fn stack_pop( _ecx: &mut InterpCx<'mir, 'tcx, Self>, _extra: Self::FrameExtra, + _unwinding: bool ) -> InterpResult<'tcx, StackPopInfo> { // By default, we do not support unwinding from panics Ok(StackPopInfo::Normal) diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index 5c83fb8c821f6..3ee9445f9c0ea 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, - StackPopInfo, Operand as InterpOperand, + Operand as InterpOperand, }; use crate::const_eval::error_to_const_error; use crate::transform::{MirPass, MirSource}; @@ -252,15 +252,6 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine { fn stack_push(_ecx: &mut InterpCx<'mir, 'tcx, Self>) -> InterpResult<'tcx> { Ok(()) } - - /// Called immediately before a stack frame gets popped. - #[inline(always)] - fn stack_pop( - _ecx: &mut InterpCx<'mir, 'tcx, Self>, - _extra: () - ) -> InterpResult<'tcx, StackPopInfo> { - Ok(StackPopInfo::Normal) - } } type Const<'tcx> = OpTy<'tcx>; From 8ff4d41ba486db16446a70ef94a3d58285880358 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Mon, 28 Oct 2019 22:23:41 -0400 Subject: [PATCH 11/29] Don't attempt to get cwd when printing backtrace under Miri This allows Miri to print backtraces in isolation mode --- src/libstd/sys_common/backtrace.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/libstd/sys_common/backtrace.rs b/src/libstd/sys_common/backtrace.rs index 9c406ec39cc45..d7296b43fbe8a 100644 --- a/src/libstd/sys_common/backtrace.rs +++ b/src/libstd/sys_common/backtrace.rs @@ -66,7 +66,14 @@ unsafe fn _print(w: &mut dyn Write, format: PrintFmt) -> io::Result<()> { } unsafe fn _print_fmt(fmt: &mut fmt::Formatter<'_>, print_fmt: PrintFmt) -> fmt::Result { - let cwd = env::current_dir().ok(); + // Always 'fail' to get the cwd when running under Miri - + // this allows Miri to display backtraces in isolation mode + let cwd = if !cfg!(miri) { + env::current_dir().ok() + } else { + None + }; + let mut print_path = move |fmt: &mut fmt::Formatter<'_>, bows: BytesOrWideString<'_>| { output_filename(fmt, bows, print_fmt, cwd.as_ref()) }; From caf3cc1fc88430caa61a8f1ae0262c3489a6481a Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Mon, 28 Oct 2019 22:44:30 -0400 Subject: [PATCH 12/29] Add explicit Miri support to libpanic_unwind --- src/libcore/intrinsics.rs | 5 +++++ src/libpanic_unwind/lib.rs | 5 ++++- src/libpanic_unwind/miri.rs | 12 ++++++++++++ 3 files changed, 21 insertions(+), 1 deletion(-) create mode 100644 src/libpanic_unwind/miri.rs diff --git a/src/libcore/intrinsics.rs b/src/libcore/intrinsics.rs index 3db85d05d7a98..f4a1ff0dd005f 100644 --- a/src/libcore/intrinsics.rs +++ b/src/libcore/intrinsics.rs @@ -1348,6 +1348,11 @@ extern "rust-intrinsic" { /// See documentation of `<*const T>::offset_from` for details. #[cfg(not(bootstrap))] pub fn ptr_offset_from(ptr: *const T, base: *const T) -> isize; + + /// Internal hook used by Miri to implement unwinding. + /// Perma-unstable: do not use + #[cfg(not(bootstrap))] + pub fn miri_start_panic(data: *mut (dyn crate::any::Any + Send)) -> !; } // Some functions are defined here because they accidentally got made diff --git a/src/libpanic_unwind/lib.rs b/src/libpanic_unwind/lib.rs index 7de347446ada1..4c6c728f6f7d4 100644 --- a/src/libpanic_unwind/lib.rs +++ b/src/libpanic_unwind/lib.rs @@ -36,7 +36,10 @@ use core::raw; use core::panic::BoxMeUp; cfg_if::cfg_if! { - if #[cfg(target_os = "emscripten")] { + if #[cfg(miri)] { + #[path = "miri.rs"] + mod imp; + } else if #[cfg(target_os = "emscripten")] { #[path = "emcc.rs"] mod imp; } else if #[cfg(target_arch = "wasm32")] { diff --git a/src/libpanic_unwind/miri.rs b/src/libpanic_unwind/miri.rs new file mode 100644 index 0000000000000..a2d52638e8b48 --- /dev/null +++ b/src/libpanic_unwind/miri.rs @@ -0,0 +1,12 @@ +pub fn payload() -> *mut u8 { + core::ptr::null_mut() +} + +pub unsafe fn panic(data: Box) -> u32 { + core::intrinsics::miri_start_panic(Box::into_raw(data)) +} + +pub unsafe fn cleanup(ptr: *mut u8) -> Box { + Box::from_raw(ptr) +} + From fe88fc03c50be57e1dc12cf308c45dc9c8d6473a Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Tue, 29 Oct 2019 17:31:54 -0400 Subject: [PATCH 13/29] Fix up intrinsic implementation --- src/libcore/intrinsics.rs | 2 +- src/libpanic_unwind/miri.rs | 8 ++++++-- src/librustc_codegen_ssa/mir/block.rs | 10 ++++++++++ src/librustc_typeck/check/intrinsic.rs | 4 ++++ 4 files changed, 21 insertions(+), 3 deletions(-) diff --git a/src/libcore/intrinsics.rs b/src/libcore/intrinsics.rs index f4a1ff0dd005f..0cfe5595af578 100644 --- a/src/libcore/intrinsics.rs +++ b/src/libcore/intrinsics.rs @@ -1352,7 +1352,7 @@ extern "rust-intrinsic" { /// Internal hook used by Miri to implement unwinding. /// Perma-unstable: do not use #[cfg(not(bootstrap))] - pub fn miri_start_panic(data: *mut (dyn crate::any::Any + Send)) -> !; + pub fn miri_start_panic(data: u128) -> !; } // Some functions are defined here because they accidentally got made diff --git a/src/libpanic_unwind/miri.rs b/src/libpanic_unwind/miri.rs index a2d52638e8b48..3b4203c3c6d9b 100644 --- a/src/libpanic_unwind/miri.rs +++ b/src/libpanic_unwind/miri.rs @@ -1,9 +1,13 @@ +use core::any::Any; +use alloc::boxed::Box; + pub fn payload() -> *mut u8 { core::ptr::null_mut() } -pub unsafe fn panic(data: Box) -> u32 { - core::intrinsics::miri_start_panic(Box::into_raw(data)) +pub unsafe fn panic(data: Box) -> ! { + let raw_val = core::mem::transmute::<_, u128>(Box::into_raw(data)); + core::intrinsics::miri_start_panic(raw_val) } pub unsafe fn cleanup(ptr: *mut u8) -> Box { diff --git a/src/librustc_codegen_ssa/mir/block.rs b/src/librustc_codegen_ssa/mir/block.rs index 13cd202158b77..9361a7c3ea452 100644 --- a/src/librustc_codegen_ssa/mir/block.rs +++ b/src/librustc_codegen_ssa/mir/block.rs @@ -528,6 +528,16 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { _ => FnAbi::new(&bx, sig, &extra_args) }; + // This should never be reachable at runtime: + // We should only emit a call to this intrinsic in #[cfg(miri)] mode, + // which means that we will never actually use the generate object files + // (we will just be interpreting the MIR) + if intrinsic == Some("miri_start_panic") { + bx.abort(); + bx.unreachable(); + return; + } + // Emit a panic or a no-op for `panic_if_uninhabited`. if intrinsic == Some("panic_if_uninhabited") { let ty = instance.unwrap().substs.type_at(0); diff --git a/src/librustc_typeck/check/intrinsic.rs b/src/librustc_typeck/check/intrinsic.rs index 1a1b98f582ff2..24b589df63a77 100644 --- a/src/librustc_typeck/check/intrinsic.rs +++ b/src/librustc_typeck/check/intrinsic.rs @@ -384,6 +384,10 @@ pub fn check_intrinsic_type(tcx: TyCtxt<'_>, it: &hir::ForeignItem) { (1, vec![ tcx.mk_mut_ptr(param(0)), param(0) ], tcx.mk_unit()) } + "miri_start_panic" => { + (0, vec![tcx.types.u128], tcx.types.never) + } + ref other => { struct_span_err!(tcx.sess, it.span, E0093, "unrecognized intrinsic function: `{}`", From 848e1d827e9a76f16cbb6d3f777edd6242390906 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Tue, 29 Oct 2019 18:23:36 -0400 Subject: [PATCH 14/29] More work on miri_start_panic --- src/libcore/intrinsics.rs | 7 ++++++- src/libpanic_unwind/miri.rs | 10 +++++++++- src/librustc_typeck/check/intrinsic.rs | 2 +- 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/src/libcore/intrinsics.rs b/src/libcore/intrinsics.rs index 0cfe5595af578..9a4c81f699b5d 100644 --- a/src/libcore/intrinsics.rs +++ b/src/libcore/intrinsics.rs @@ -1352,7 +1352,12 @@ extern "rust-intrinsic" { /// Internal hook used by Miri to implement unwinding. /// Perma-unstable: do not use #[cfg(not(bootstrap))] - pub fn miri_start_panic(data: u128) -> !; + // Note that the type is data is pretty arbitrary: + // we just need something that's guarnateed to be a fat + // pointer. Using `*mut [()]` instead of the actual type + // `*mut (Any + Send)` allows us to avoid making `Any` and `Send` + // lang items + pub fn miri_start_panic(data: *mut [()]) -> !; } // Some functions are defined here because they accidentally got made diff --git a/src/libpanic_unwind/miri.rs b/src/libpanic_unwind/miri.rs index 3b4203c3c6d9b..90fd5cafc37cc 100644 --- a/src/libpanic_unwind/miri.rs +++ b/src/libpanic_unwind/miri.rs @@ -6,7 +6,7 @@ pub fn payload() -> *mut u8 { } pub unsafe fn panic(data: Box) -> ! { - let raw_val = core::mem::transmute::<_, u128>(Box::into_raw(data)); + let raw_val: *mut [()] = core::mem::transmute::<*mut (dyn Any + Send), _>(Box::into_raw(data)); core::intrinsics::miri_start_panic(raw_val) } @@ -14,3 +14,11 @@ pub unsafe fn cleanup(ptr: *mut u8) -> Box { Box::from_raw(ptr) } + +// This is required by the compiler to exist (e.g., it's a lang item), +// but is never used by Miri. Therefore, we just use a stub here +#[lang = "eh_personality"] +#[cfg(not(test))] +fn rust_eh_personality() { + unsafe { core::intrinsics::abort() } +} diff --git a/src/librustc_typeck/check/intrinsic.rs b/src/librustc_typeck/check/intrinsic.rs index 24b589df63a77..50985a3a23200 100644 --- a/src/librustc_typeck/check/intrinsic.rs +++ b/src/librustc_typeck/check/intrinsic.rs @@ -385,7 +385,7 @@ pub fn check_intrinsic_type(tcx: TyCtxt<'_>, it: &hir::ForeignItem) { } "miri_start_panic" => { - (0, vec![tcx.types.u128], tcx.types.never) + (0, vec![tcx.mk_mut_ptr(tcx.mk_slice(tcx.mk_unit()))], tcx.types.never) } ref other => { From 5553476d490741efc8e3049401152d5d4ce6a934 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Tue, 29 Oct 2019 18:48:29 -0400 Subject: [PATCH 15/29] Use proper intrinsic type --- src/libcore/intrinsics.rs | 7 +------ src/libpanic_unwind/miri.rs | 3 +-- src/librustc_typeck/check/intrinsic.rs | 4 +++- 3 files changed, 5 insertions(+), 9 deletions(-) diff --git a/src/libcore/intrinsics.rs b/src/libcore/intrinsics.rs index 9a4c81f699b5d..686d0bbefc882 100644 --- a/src/libcore/intrinsics.rs +++ b/src/libcore/intrinsics.rs @@ -1352,12 +1352,7 @@ extern "rust-intrinsic" { /// Internal hook used by Miri to implement unwinding. /// Perma-unstable: do not use #[cfg(not(bootstrap))] - // Note that the type is data is pretty arbitrary: - // we just need something that's guarnateed to be a fat - // pointer. Using `*mut [()]` instead of the actual type - // `*mut (Any + Send)` allows us to avoid making `Any` and `Send` - // lang items - pub fn miri_start_panic(data: *mut [()]) -> !; + pub fn miri_start_panic(data: *mut (dyn crate::any::Any + crate::marker::Send)) -> !; } // Some functions are defined here because they accidentally got made diff --git a/src/libpanic_unwind/miri.rs b/src/libpanic_unwind/miri.rs index 90fd5cafc37cc..254a9383b4223 100644 --- a/src/libpanic_unwind/miri.rs +++ b/src/libpanic_unwind/miri.rs @@ -6,8 +6,7 @@ pub fn payload() -> *mut u8 { } pub unsafe fn panic(data: Box) -> ! { - let raw_val: *mut [()] = core::mem::transmute::<*mut (dyn Any + Send), _>(Box::into_raw(data)); - core::intrinsics::miri_start_panic(raw_val) + core::intrinsics::miri_start_panic(Box::into_raw(data)) } pub unsafe fn cleanup(ptr: *mut u8) -> Box { diff --git a/src/librustc_typeck/check/intrinsic.rs b/src/librustc_typeck/check/intrinsic.rs index 50985a3a23200..627b0bb0e6941 100644 --- a/src/librustc_typeck/check/intrinsic.rs +++ b/src/librustc_typeck/check/intrinsic.rs @@ -385,7 +385,9 @@ pub fn check_intrinsic_type(tcx: TyCtxt<'_>, it: &hir::ForeignItem) { } "miri_start_panic" => { - (0, vec![tcx.mk_mut_ptr(tcx.mk_slice(tcx.mk_unit()))], tcx.types.never) + // TODO - the relevant types aren't lang items, + // so it's not trivial to check this + return; } ref other => { From b06c83c2004476ed8c0ee880ed7955439da4d35f Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Tue, 29 Oct 2019 21:03:30 -0400 Subject: [PATCH 16/29] Add miri trampoline, fix handling of intrinsic return --- src/libpanic_unwind/lib.rs | 3 +++ src/librustc_mir/interpret/terminator.rs | 11 ++++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/libpanic_unwind/lib.rs b/src/libpanic_unwind/lib.rs index 4c6c728f6f7d4..c8d270406c634 100644 --- a/src/libpanic_unwind/lib.rs +++ b/src/libpanic_unwind/lib.rs @@ -97,3 +97,6 @@ pub unsafe extern "C" fn __rust_start_panic(payload: usize) -> u32 { let payload = payload as *mut &mut dyn BoxMeUp; imp::panic(Box::from_raw((*payload).box_me_up())) } + +#[cfg(miri)] +pub fn miri_panic_trampoline() {} diff --git a/src/librustc_mir/interpret/terminator.rs b/src/librustc_mir/interpret/terminator.rs index bed6c5bea5436..f77bf56b0575e 100644 --- a/src/librustc_mir/interpret/terminator.rs +++ b/src/librustc_mir/interpret/terminator.rs @@ -264,10 +264,19 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { match instance.def { ty::InstanceDef::Intrinsic(..) => { + let old_stack = self.cur_frame(); M::call_intrinsic(self, span, instance, args, dest)?; // No stack frame gets pushed, the main loop will just act as if the // call completed. - self.goto_block(ret)?; + if ret.is_some() { + self.goto_block(ret)?; + } else { + // If this intrinsic call doesn't have a ret block, + // then the intrinsic implementation should have + // changed the stack frame (otherwise, we'll end + // up trying to execute this intrinsic call again) + assert!(self.cur_frame() != old_stack); + } if let Some(dest) = dest { self.dump_place(*dest) } From d5c0acac4dc22a99c055becedaec5d579e08a034 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Tue, 29 Oct 2019 21:09:51 -0400 Subject: [PATCH 17/29] Add comment --- src/libpanic_unwind/lib.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libpanic_unwind/lib.rs b/src/libpanic_unwind/lib.rs index c8d270406c634..7c4e7bf1b179b 100644 --- a/src/libpanic_unwind/lib.rs +++ b/src/libpanic_unwind/lib.rs @@ -98,5 +98,7 @@ pub unsafe extern "C" fn __rust_start_panic(payload: usize) -> u32 { imp::panic(Box::from_raw((*payload).box_me_up())) } +// A dummy helper function for Miri. +// Used to push an empty stack frame when we start unwinding #[cfg(miri)] pub fn miri_panic_trampoline() {} From dac30115dc674b0d636018dfa7c1e31095078425 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Wed, 30 Oct 2019 14:40:21 -0400 Subject: [PATCH 18/29] Change TODO to FIXME --- src/librustc_typeck/check/intrinsic.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_typeck/check/intrinsic.rs b/src/librustc_typeck/check/intrinsic.rs index 627b0bb0e6941..84a15c6e4ebe2 100644 --- a/src/librustc_typeck/check/intrinsic.rs +++ b/src/librustc_typeck/check/intrinsic.rs @@ -385,7 +385,7 @@ pub fn check_intrinsic_type(tcx: TyCtxt<'_>, it: &hir::ForeignItem) { } "miri_start_panic" => { - // TODO - the relevant types aren't lang items, + // FIXME - the relevant types aren't lang items, // so it's not trivial to check this return; } From c062afe73ddb8aaab4c475530330404b62e2d58d Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Mon, 4 Nov 2019 13:49:50 -0500 Subject: [PATCH 19/29] Make doc comment more accurate --- src/librustc_mir/interpret/machine.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/librustc_mir/interpret/machine.rs b/src/librustc_mir/interpret/machine.rs index bed6c0b67eb18..08da5078f43a9 100644 --- a/src/librustc_mir/interpret/machine.rs +++ b/src/librustc_mir/interpret/machine.rs @@ -24,7 +24,10 @@ pub enum StackPopInfo { /// as the result of panic StartUnwinding, - /// Indicates that we performed a normal return + /// Indicates that no special handling should be + /// done - we'll either return normally or unwind + /// based on the terminator for the function + /// we're leaving. Normal, /// Indicates that we should stop unwinding, From 72b555c160a422a4953425d6a45155edc750bb2b Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Mon, 4 Nov 2019 15:00:41 -0500 Subject: [PATCH 20/29] Some code cleanup --- src/librustc_mir/interpret/eval_context.rs | 51 ++++++++++------------ 1 file changed, 23 insertions(+), 28 deletions(-) diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index fb3f4f21a9734..fa7b537b4df8d 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/src/librustc_mir/interpret/eval_context.rs @@ -593,17 +593,28 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { _ => {} } + // Now where do we jump next? + + // Determine if we leave this function normally or via unwinding. + let cur_unwinding = match stack_pop_info { + StackPopInfo::StartUnwinding => true, + StackPopInfo::StopUnwinding => false, + _ => unwinding + }; + // Usually we want to clean up (deallocate locals), but in a few rare cases we don't. // In that case, we return early. We also avoid validation in that case, // because this is CTFE and the final value will be thoroughly validated anyway. - let cleanup = unwinding || match frame.return_to_block { - StackPopCleanup::Goto{ .. } => true, - StackPopCleanup::None { cleanup, .. } => { - cleanup - } + let (cleanup, next_block) = match frame.return_to_block { + StackPopCleanup::Goto { ret, unwind } => { + (true, Some(if cur_unwinding { unwind } else { ret })) + }, + StackPopCleanup::None { cleanup, .. } => (cleanup, None) }; + if !cleanup { 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!"); // Leak the locals, skip validation. return Ok(()); } @@ -613,29 +624,16 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { self.deallocate_local(local.value)?; } - // Now where do we jump next? - - // Determine if we leave this function normally or via unwinding. - let cur_unwinding = match stack_pop_info { - StackPopInfo::StartUnwinding => true, - StackPopInfo::StopUnwinding => false, - _ => unwinding - }; trace!("StackPopCleanup: {:?} StackPopInfo: {:?} cur_unwinding = {:?}", frame.return_to_block, stack_pop_info, cur_unwinding); if cur_unwinding { // Follow the unwind edge. - match frame.return_to_block { - StackPopCleanup::Goto { unwind, .. } => { - let next_frame = self.frame_mut(); - // If `unwind` is `None`, we'll leave that function immediately again. - next_frame.block = unwind; - next_frame.stmt = 0; - }, - StackPopCleanup::None { .. } => - bug!("Encountered StackPopCleanup::None while unwinding"), - } + let unwind = next_block.expect("Encounted StackPopCleanup::None when unwinding!"); + let next_frame = self.frame_mut(); + // If `unwind` is `None`, we'll leave that function immediately again. + next_frame.block = unwind; + next_frame.stmt = 0; } else { // Follow the normal return edge. // Validate the return value. Do this after deallocating so that we catch dangling @@ -661,11 +659,8 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } // Jump to new block -- *after* validation so that the spans make more sense. - match frame.return_to_block { - StackPopCleanup::Goto { ret, .. } => { - self.goto_block(ret)?; - } - StackPopCleanup::None { .. } => {} + if let Some(ret) = next_block { + self.goto_block(ret)?; } } From 23900770ab1aa516243371dbb4b5d7554dd31dbc Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Mon, 4 Nov 2019 15:34:19 -0500 Subject: [PATCH 21/29] Move to miri.rs and re-export it --- src/libpanic_unwind/lib.rs | 8 ++++---- src/libpanic_unwind/miri.rs | 5 +++++ 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/libpanic_unwind/lib.rs b/src/libpanic_unwind/lib.rs index 7c4e7bf1b179b..f9b2edbe59d75 100644 --- a/src/libpanic_unwind/lib.rs +++ b/src/libpanic_unwind/lib.rs @@ -39,6 +39,9 @@ cfg_if::cfg_if! { if #[cfg(miri)] { #[path = "miri.rs"] mod imp; + // Export this at the root of the crate so that Miri + // has a stable palce to look it up + pub use imp::miri_panic_trampoline; } else if #[cfg(target_os = "emscripten")] { #[path = "emcc.rs"] mod imp; @@ -98,7 +101,4 @@ pub unsafe extern "C" fn __rust_start_panic(payload: usize) -> u32 { imp::panic(Box::from_raw((*payload).box_me_up())) } -// A dummy helper function for Miri. -// Used to push an empty stack frame when we start unwinding -#[cfg(miri)] -pub fn miri_panic_trampoline() {} + diff --git a/src/libpanic_unwind/miri.rs b/src/libpanic_unwind/miri.rs index 254a9383b4223..2b41d338ce2b1 100644 --- a/src/libpanic_unwind/miri.rs +++ b/src/libpanic_unwind/miri.rs @@ -21,3 +21,8 @@ pub unsafe fn cleanup(ptr: *mut u8) -> Box { fn rust_eh_personality() { unsafe { core::intrinsics::abort() } } + +// A dummy helper function for Miri. +// Used to push an empty stack frame when we start unwinding +#[cfg(miri)] +pub fn miri_panic_trampoline() {} From 6eea0ffc651c61cb08fca524298268e1a07eac82 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Mon, 4 Nov 2019 16:10:02 -0500 Subject: [PATCH 22/29] Add more detailed codegen comment --- src/librustc_codegen_ssa/mir/block.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/librustc_codegen_ssa/mir/block.rs b/src/librustc_codegen_ssa/mir/block.rs index 9361a7c3ea452..14be0e80fb482 100644 --- a/src/librustc_codegen_ssa/mir/block.rs +++ b/src/librustc_codegen_ssa/mir/block.rs @@ -532,6 +532,11 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { // We should only emit a call to this intrinsic in #[cfg(miri)] mode, // which means that we will never actually use the generate object files // (we will just be interpreting the MIR) + // + // Note that we still need to be able to codegen *something* for this intrisnic: + // Miri currently uses Xargo to build a special libstd. As a side effect, + // we generate normal object files for libstd - while these are never used, + // we still need to be able to build them. if intrinsic == Some("miri_start_panic") { bx.abort(); bx.unreachable(); From 607339f66a5883ae1c6d24d81a1e7a97c9a4298c Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Mon, 4 Nov 2019 16:10:35 -0500 Subject: [PATCH 23/29] Fix tidy --- src/libpanic_unwind/lib.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/libpanic_unwind/lib.rs b/src/libpanic_unwind/lib.rs index f9b2edbe59d75..fb2667bf883c2 100644 --- a/src/libpanic_unwind/lib.rs +++ b/src/libpanic_unwind/lib.rs @@ -100,5 +100,3 @@ pub unsafe extern "C" fn __rust_start_panic(payload: usize) -> u32 { let payload = payload as *mut &mut dyn BoxMeUp; imp::panic(Box::from_raw((*payload).box_me_up())) } - - From 4ecb80d5d83a1b781bff1235e3ba5530255c7956 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Tue, 5 Nov 2019 00:06:05 -0500 Subject: [PATCH 24/29] Remove trampoline, pass `ret` and `unwind` when handling intrinsics --- src/libpanic_unwind/lib.rs | 3 --- src/libpanic_unwind/miri.rs | 5 ----- src/librustc_mir/const_eval.rs | 2 ++ src/librustc_mir/interpret/eval_context.rs | 16 ++++++---------- src/librustc_mir/interpret/machine.rs | 6 ++---- src/librustc_mir/interpret/terminator.rs | 2 +- src/librustc_mir/transform/const_prop.rs | 2 ++ 7 files changed, 13 insertions(+), 23 deletions(-) diff --git a/src/libpanic_unwind/lib.rs b/src/libpanic_unwind/lib.rs index fb2667bf883c2..4c6c728f6f7d4 100644 --- a/src/libpanic_unwind/lib.rs +++ b/src/libpanic_unwind/lib.rs @@ -39,9 +39,6 @@ cfg_if::cfg_if! { if #[cfg(miri)] { #[path = "miri.rs"] mod imp; - // Export this at the root of the crate so that Miri - // has a stable palce to look it up - pub use imp::miri_panic_trampoline; } else if #[cfg(target_os = "emscripten")] { #[path = "emcc.rs"] mod imp; diff --git a/src/libpanic_unwind/miri.rs b/src/libpanic_unwind/miri.rs index 2b41d338ce2b1..254a9383b4223 100644 --- a/src/libpanic_unwind/miri.rs +++ b/src/libpanic_unwind/miri.rs @@ -21,8 +21,3 @@ pub unsafe fn cleanup(ptr: *mut u8) -> Box { fn rust_eh_personality() { unsafe { core::intrinsics::abort() } } - -// A dummy helper function for Miri. -// Used to push an empty stack frame when we start unwinding -#[cfg(miri)] -pub fn miri_panic_trampoline() {} diff --git a/src/librustc_mir/const_eval.rs b/src/librustc_mir/const_eval.rs index 812774fab8de4..e2d5eec6502e5 100644 --- a/src/librustc_mir/const_eval.rs +++ b/src/librustc_mir/const_eval.rs @@ -376,6 +376,8 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir, instance: ty::Instance<'tcx>, args: &[OpTy<'tcx>], dest: Option>, + _ret: Option, + _unwind: Option ) -> InterpResult<'tcx> { if ecx.emulate_intrinsic(span, instance, args, dest)? { return Ok(()); diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index fa7b537b4df8d..92358ad247e18 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/src/librustc_mir/interpret/eval_context.rs @@ -585,21 +585,17 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { "tried to pop a stack frame, but there were none", ); let stack_pop_info = M::stack_pop(self, frame.extra, unwinding)?; - match (unwinding, stack_pop_info) { - (true, StackPopInfo::StartUnwinding) => - bug!("Attempted to start unwinding while already unwinding!"), - (false, StackPopInfo::StopUnwinding) => - bug!("Attempted to stop unwinding while there is no unwinding!"), - _ => {} + if let (false, StackPopInfo::StopUnwinding) = (unwinding, stack_pop_info) { + bug!("Attempted to stop unwinding while there is no unwinding!"); } // Now where do we jump next? // Determine if we leave this function normally or via unwinding. - let cur_unwinding = match stack_pop_info { - StackPopInfo::StartUnwinding => true, - StackPopInfo::StopUnwinding => false, - _ => unwinding + let cur_unwinding = if let StackPopInfo::StopUnwinding = stack_pop_info { + false + } else { + unwinding }; // Usually we want to clean up (deallocate locals), but in a few rare cases we don't. diff --git a/src/librustc_mir/interpret/machine.rs b/src/librustc_mir/interpret/machine.rs index 08da5078f43a9..0ddd0962ba544 100644 --- a/src/librustc_mir/interpret/machine.rs +++ b/src/librustc_mir/interpret/machine.rs @@ -20,10 +20,6 @@ use super::{ /// to provide further control over the popping of the stack frame #[derive(Eq, PartialEq, Debug, Copy, Clone)] pub enum StackPopInfo { - /// Indicates that we have just started unwinding - /// as the result of panic - StartUnwinding, - /// Indicates that no special handling should be /// done - we'll either return normally or unwind /// based on the terminator for the function @@ -177,6 +173,8 @@ pub trait Machine<'mir, 'tcx>: Sized { instance: ty::Instance<'tcx>, args: &[OpTy<'tcx, Self::PointerTag>], dest: Option>, + ret: Option, + unwind: Option, ) -> InterpResult<'tcx>; /// Called for read access to a foreign static item. diff --git a/src/librustc_mir/interpret/terminator.rs b/src/librustc_mir/interpret/terminator.rs index f77bf56b0575e..f1e1c98e44eb4 100644 --- a/src/librustc_mir/interpret/terminator.rs +++ b/src/librustc_mir/interpret/terminator.rs @@ -265,7 +265,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { match instance.def { ty::InstanceDef::Intrinsic(..) => { let old_stack = self.cur_frame(); - M::call_intrinsic(self, span, instance, args, dest)?; + M::call_intrinsic(self, span, instance, args, dest, ret, unwind)?; // No stack frame gets pushed, the main loop will just act as if the // call completed. if ret.is_some() { diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index 3ee9445f9c0ea..2ede43e2111ed 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -164,6 +164,8 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine { _instance: ty::Instance<'tcx>, _args: &[OpTy<'tcx>], _dest: Option>, + _ret: Option, + _unwind: Option ) -> InterpResult<'tcx> { throw_unsup_format!("calling intrinsics isn't supported in ConstProp"); } From ee2dc4b7287a67a4e6299d61539b742d8f73f0a6 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Tue, 5 Nov 2019 00:22:49 -0500 Subject: [PATCH 25/29] Fix debug assertion --- src/librustc_mir/interpret/terminator.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/librustc_mir/interpret/terminator.rs b/src/librustc_mir/interpret/terminator.rs index f1e1c98e44eb4..4f9e404b2c635 100644 --- a/src/librustc_mir/interpret/terminator.rs +++ b/src/librustc_mir/interpret/terminator.rs @@ -265,6 +265,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { match instance.def { ty::InstanceDef::Intrinsic(..) => { let old_stack = self.cur_frame(); + let old_bb = self.frame().block; M::call_intrinsic(self, span, instance, args, dest, ret, unwind)?; // No stack frame gets pushed, the main loop will just act as if the // call completed. @@ -275,7 +276,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // then the intrinsic implementation should have // changed the stack frame (otherwise, we'll end // up trying to execute this intrinsic call again) - assert!(self.cur_frame() != old_stack); + debug_assert!(self.cur_frame() != old_stack || self.frame().block != old_bb); } if let Some(dest) = dest { self.dump_place(*dest) From 2ed1e8970675c98e1464478f0fa7eb97e988835a Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Wed, 6 Nov 2019 13:20:08 -0500 Subject: [PATCH 26/29] Rename to --- src/librustc_mir/const_eval.rs | 2 +- src/librustc_mir/interpret/intrinsics.rs | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/librustc_mir/const_eval.rs b/src/librustc_mir/const_eval.rs index e2d5eec6502e5..f280ea7aa4669 100644 --- a/src/librustc_mir/const_eval.rs +++ b/src/librustc_mir/const_eval.rs @@ -337,7 +337,7 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir, // Some functions we support even if they are non-const -- but avoid testing // that for const fn! We certainly do *not* want to actually call the fn // though, so be sure we return here. - return if ecx.hook_fn(instance, args, dest)? { + return if ecx.hook_panic_fn(instance, args, dest)? { ecx.goto_block(ret)?; // fully evaluated and done Ok(None) } else { diff --git a/src/librustc_mir/interpret/intrinsics.rs b/src/librustc_mir/interpret/intrinsics.rs index d6d53a740bb2a..ce16b89d6ea87 100644 --- a/src/librustc_mir/interpret/intrinsics.rs +++ b/src/librustc_mir/interpret/intrinsics.rs @@ -354,9 +354,10 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { Ok(true) } - /// "Intercept" a function call because we have something special to do for it. + /// "Intercept" a function call to a panic-related function + /// because we have something special to do for it. /// Returns `true` if an intercept happened. - pub fn hook_fn( + pub fn hook_panic_fn( &mut self, instance: ty::Instance<'tcx>, args: &[OpTy<'tcx, M::PointerTag>], From 68d9853985cf3a7eb00b6066473d6a27322ec748 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Wed, 6 Nov 2019 13:47:40 -0500 Subject: [PATCH 27/29] Fix rebase fallout --- src/librustc_mir/interpret/intrinsics.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_mir/interpret/intrinsics.rs b/src/librustc_mir/interpret/intrinsics.rs index ce16b89d6ea87..18d8d8c615851 100644 --- a/src/librustc_mir/interpret/intrinsics.rs +++ b/src/librustc_mir/interpret/intrinsics.rs @@ -101,7 +101,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { Some(dest) => dest, None => throw_ub!(Unreachable) }; - let intrinsic_name = &self.tcx.item_name(instance.def_id()).as_str(); + let intrinsic_name = &*self.tcx.item_name(instance.def_id()).as_str(); match intrinsic_name { "caller_location" => { From c0b972abfa4cd31d2b2d598824f578359350430f Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Wed, 6 Nov 2019 14:13:57 -0500 Subject: [PATCH 28/29] Return Ok(false) instead of throwing when handling a diverging intrinsic --- src/librustc_mir/interpret/intrinsics.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/librustc_mir/interpret/intrinsics.rs b/src/librustc_mir/interpret/intrinsics.rs index 18d8d8c615851..66b6d4ac12c40 100644 --- a/src/librustc_mir/interpret/intrinsics.rs +++ b/src/librustc_mir/interpret/intrinsics.rs @@ -95,11 +95,10 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { ) -> InterpResult<'tcx, bool> { let substs = instance.substs; - // The intrinsic itself cannot diverge, so if we got here without a return - // place... (can happen e.g., for transmute returning `!`) + // We currently do not handle any diverging intrinsics. let dest = match dest { Some(dest) => dest, - None => throw_ub!(Unreachable) + None => return Ok(false) }; let intrinsic_name = &*self.tcx.item_name(instance.def_id()).as_str(); From b4545a4ad625c68479120db845280f4c61b39640 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sat, 9 Nov 2019 11:02:15 -0500 Subject: [PATCH 29/29] Update --- src/librustc_mir/interpret/traits.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_mir/interpret/traits.rs b/src/librustc_mir/interpret/traits.rs index 55c127c56f608..e3b3df2674f41 100644 --- a/src/librustc_mir/interpret/traits.rs +++ b/src/librustc_mir/interpret/traits.rs @@ -113,7 +113,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { ptr_size, self.tcx.data_layout.pointer_align.abi, )?.expect("cannot be a ZST"); - let fn_ptr = self.memory.get(vtable_slot.alloc_id)? + let fn_ptr = self.memory.get_raw(vtable_slot.alloc_id)? .read_ptr_sized(self, vtable_slot)?.not_undef()?; Ok(self.memory.get_fn(fn_ptr)?) }