From 5c72788318a451d74e8b928b92f39cb869097f61 Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Thu, 3 Nov 2022 00:09:00 -0400 Subject: [PATCH 1/5] Incrementally track which frame to use for diagnostics --- src/concurrency/thread.rs | 53 +++++++++++++++++-- src/helpers.rs | 74 +++++++------------------- src/lib.rs | 2 +- src/machine.rs | 41 +++++++++------ src/stacked_borrows/diagnostics.rs | 83 +++++++++++------------------- src/stacked_borrows/mod.rs | 55 ++++++++------------ 6 files changed, 148 insertions(+), 160 deletions(-) diff --git a/src/concurrency/thread.rs b/src/concurrency/thread.rs index 8fbee9a352..96d988eb9a 100644 --- a/src/concurrency/thread.rs +++ b/src/concurrency/thread.rs @@ -118,6 +118,13 @@ pub struct Thread<'mir, 'tcx> { /// The virtual call stack. stack: Vec>>, + /// The index of the topmost user-relevant frame in `stack`. This field must contain + /// the value produced by `get_top_user_relevant_frame`. + /// The `None` state here represents + /// This field is a cache to reduce how often we call that method. The cache is manually + /// maintained inside `MiriMachine::after_stack_push` and `MiriMachine::after_stack_pop`. + top_user_relevant_frame: Option, + /// The join status. join_status: ThreadJoinStatus, @@ -147,6 +154,38 @@ impl<'mir, 'tcx> Thread<'mir, 'tcx> { fn thread_name(&self) -> &[u8] { if let Some(ref thread_name) = self.thread_name { thread_name } else { b"" } } + + /// Return the top user-relevant frame, if there is one. + /// Note that the choice to return `None` here when there is no user-relevant frame is part of + /// justifying the optimization that only pushes of user-relevant frames require updating the + /// `top_user_relevant_frame` field. + fn compute_top_user_relevant_frame(&self) -> Option { + self.stack + .iter() + .enumerate() + .rev() + .find_map(|(idx, frame)| if frame.extra.is_user_relevant { Some(idx) } else { None }) + } + + /// Re-compute the top user-relevant frame from scratch. + pub fn recompute_top_user_relevant_frame(&mut self) { + self.top_user_relevant_frame = self.compute_top_user_relevant_frame(); + } + + /// Set the top user-relevant frame to the given value. Must be equal to what + /// `get_top_user_relevant_frame` would return! + pub fn set_top_user_relevant_frame(&mut self, frame_idx: usize) { + debug_assert_eq!(Some(frame_idx), self.compute_top_user_relevant_frame()); + self.top_user_relevant_frame = Some(frame_idx); + } + + pub fn top_user_relevant_frame(&self) -> usize { + debug_assert_eq!(self.top_user_relevant_frame, self.compute_top_user_relevant_frame()); + // This can be called upon creation of an allocation. We create allocations while setting up + // parts of the Rust runtime when we do not have any stack frames yet, so we need to handle + // empty stacks. + self.top_user_relevant_frame.unwrap_or_else(|| self.stack.len().saturating_sub(1)) + } } impl<'mir, 'tcx> std::fmt::Debug for Thread<'mir, 'tcx> { @@ -167,6 +206,7 @@ impl<'mir, 'tcx> Default for Thread<'mir, 'tcx> { state: ThreadState::Enabled, thread_name: None, stack: Vec::new(), + top_user_relevant_frame: None, join_status: ThreadJoinStatus::Joinable, panic_payload: None, last_error: None, @@ -184,8 +224,15 @@ impl<'mir, 'tcx> Thread<'mir, 'tcx> { impl VisitTags for Thread<'_, '_> { fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { - let Thread { panic_payload, last_error, stack, state: _, thread_name: _, join_status: _ } = - self; + let Thread { + panic_payload, + last_error, + stack, + top_user_relevant_frame: _, + state: _, + thread_name: _, + join_status: _, + } = self; panic_payload.visit_tags(visit); last_error.visit_tags(visit); @@ -414,7 +461,7 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { } /// Get a shared borrow of the currently active thread. - fn active_thread_ref(&self) -> &Thread<'mir, 'tcx> { + pub fn active_thread_ref(&self) -> &Thread<'mir, 'tcx> { &self.threads[self.active_thread] } diff --git a/src/helpers.rs b/src/helpers.rs index cd5e989b43..958ed50496 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -936,31 +936,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { } impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> { - pub fn current_span(&self) -> CurrentSpan<'_, 'mir, 'tcx> { - CurrentSpan { current_frame_idx: None, machine: self } - } -} - -/// A `CurrentSpan` should be created infrequently (ideally once) per interpreter step. It does -/// nothing on creation, but when `CurrentSpan::get` is called, searches the current stack for the -/// topmost frame which corresponds to a local crate, and returns the current span in that frame. -/// The result of that search is cached so that later calls are approximately free. -#[derive(Clone)] -pub struct CurrentSpan<'a, 'mir, 'tcx> { - current_frame_idx: Option, - machine: &'a MiriMachine<'mir, 'tcx>, -} - -impl<'a, 'mir: 'a, 'tcx: 'a + 'mir> CurrentSpan<'a, 'mir, 'tcx> { - pub fn machine(&self) -> &'a MiriMachine<'mir, 'tcx> { - self.machine - } - - /// Get the current span, skipping non-local frames. + /// Get the current span in the topmost function which is workspace-local and not + /// `#[track_caller]`. /// This function is backed by a cache, and can be assumed to be very fast. - pub fn get(&mut self) -> Span { - let idx = self.current_frame_idx(); - self.stack().get(idx).map(Frame::current_span).unwrap_or(rustc_span::DUMMY_SP) + pub fn current_span(&self) -> Span { + self.stack() + .get(self.top_user_relevant_frame()) + .map(Frame::current_span) + .unwrap_or(rustc_span::DUMMY_SP) } /// Returns the span of the *caller* of the current operation, again @@ -968,46 +951,27 @@ impl<'a, 'mir: 'a, 'tcx: 'a + 'mir> CurrentSpan<'a, 'mir, 'tcx> { /// current operation is not in a local crate. /// This is useful when we are processing something which occurs on function-entry and we want /// to point at the call to the function, not the function definition generally. - pub fn get_caller(&mut self) -> Span { + pub fn caller_span(&self) -> Span { // We need to go down at least to the caller (len - 2), or however - // far we have to go to find a frame in a local crate. - let local_frame_idx = self.current_frame_idx(); + // far we have to go to find a frame in a local crate which is also not #[track_caller]. + let frame_idx = self.top_user_relevant_frame(); let stack = self.stack(); - let idx = cmp::min(local_frame_idx, stack.len().saturating_sub(2)); - stack.get(idx).map(Frame::current_span).unwrap_or(rustc_span::DUMMY_SP) + let frame_idx = cmp::min(frame_idx, stack.len().saturating_sub(2)); + stack.get(frame_idx).map(Frame::current_span).unwrap_or(rustc_span::DUMMY_SP) } fn stack(&self) -> &[Frame<'mir, 'tcx, Provenance, machine::FrameData<'tcx>>] { - self.machine.threads.active_thread_stack() + self.threads.active_thread_stack() } - fn current_frame_idx(&mut self) -> usize { - *self - .current_frame_idx - .get_or_insert_with(|| Self::compute_current_frame_index(self.machine)) + fn top_user_relevant_frame(&self) -> usize { + self.threads.active_thread_ref().top_user_relevant_frame() } - // Find the position of the inner-most frame which is part of the crate being - // compiled/executed, part of the Cargo workspace, and is also not #[track_caller]. - #[inline(never)] - fn compute_current_frame_index(machine: &MiriMachine<'_, '_>) -> usize { - machine - .threads - .active_thread_stack() - .iter() - .enumerate() - .rev() - .find_map(|(idx, frame)| { - let def_id = frame.instance.def_id(); - if (def_id.is_local() || machine.local_crates.contains(&def_id.krate)) - && !frame.instance.def.requires_caller_location(machine.tcx) - { - Some(idx) - } else { - None - } - }) - .unwrap_or(0) + pub fn is_user_relevant(&self, frame: &Frame<'mir, 'tcx, Provenance>) -> bool { + let def_id = frame.instance.def_id(); + (def_id.is_local() || self.local_crates.contains(&def_id.krate)) + && !frame.instance.def.requires_caller_location(self.tcx) } } diff --git a/src/lib.rs b/src/lib.rs index 66df0d737c..8913f8aa10 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -97,7 +97,7 @@ pub use crate::diagnostics::{ pub use crate::eval::{ create_ecx, eval_entry, AlignmentCheck, BacktraceStyle, IsolatedOp, MiriConfig, RejectOpWith, }; -pub use crate::helpers::{CurrentSpan, EvalContextExt as _}; +pub use crate::helpers::EvalContextExt as _; pub use crate::intptrcast::ProvenanceMode; pub use crate::machine::{ AllocExtra, FrameData, MiriInterpCx, MiriInterpCxExt, MiriMachine, MiriMemoryKind, diff --git a/src/machine.rs b/src/machine.rs index 8243ccd90a..f5fa8f5f09 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -50,12 +50,18 @@ pub struct FrameData<'tcx> { /// for the start of this frame. When we finish executing this frame, /// we use this to register a completed event with `measureme`. pub timing: Option, + + /// Indicates whether a `Frame` is part of a workspace-local crate and is also not + /// `#[track_caller]`. We compute this once on creation and store the result, as an + /// optimization. + /// This is used by `MiriMachine::current_span` and `MiriMachine::caller_span` + pub is_user_relevant: bool, } impl<'tcx> std::fmt::Debug for FrameData<'tcx> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { // Omitting `timing`, it does not support `Debug`. - let FrameData { stacked_borrows, catch_unwind, timing: _ } = self; + let FrameData { stacked_borrows, catch_unwind, timing: _, is_user_relevant: _ } = self; f.debug_struct("FrameData") .field("stacked_borrows", stacked_borrows) .field("catch_unwind", catch_unwind) @@ -65,7 +71,7 @@ impl<'tcx> std::fmt::Debug for FrameData<'tcx> { impl VisitTags for FrameData<'_> { fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { - let FrameData { catch_unwind, stacked_borrows, timing: _ } = self; + let FrameData { catch_unwind, stacked_borrows, timing: _, is_user_relevant: _ } = self; catch_unwind.visit_tags(visit); stacked_borrows.visit_tags(visit); @@ -895,13 +901,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { let alloc = alloc.into_owned(); let stacks = ecx.machine.stacked_borrows.as_ref().map(|stacked_borrows| { - Stacks::new_allocation( - id, - alloc.size(), - stacked_borrows, - kind, - ecx.machine.current_span(), - ) + Stacks::new_allocation(id, alloc.size(), stacked_borrows, kind, &ecx.machine) }); let race_alloc = ecx.machine.data_race.as_ref().map(|data_race| { data_race::AllocExtra::new_allocation( @@ -1016,8 +1016,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { prov_extra, range, machine.stacked_borrows.as_ref().unwrap(), - machine.current_span(), - &machine.threads, + machine, )?; } if let Some(weak_memory) = &alloc_extra.weak_memory { @@ -1048,8 +1047,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { prov_extra, range, machine.stacked_borrows.as_ref().unwrap(), - machine.current_span(), - &machine.threads, + machine, )?; } if let Some(weak_memory) = &alloc_extra.weak_memory { @@ -1083,8 +1081,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { prove_extra, range, machine.stacked_borrows.as_ref().unwrap(), - machine.current_span(), - &machine.threads, + machine, ) } else { Ok(()) @@ -1126,7 +1123,9 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { stacked_borrows: stacked_borrows.map(|sb| sb.borrow_mut().new_frame(&ecx.machine)), catch_unwind: None, timing, + is_user_relevant: ecx.machine.is_user_relevant(&frame), }; + Ok(frame.with_extra(extra)) } @@ -1174,6 +1173,13 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { #[inline(always)] fn after_stack_push(ecx: &mut InterpCx<'mir, 'tcx, Self>) -> InterpResult<'tcx> { + if ecx.frame().extra.is_user_relevant { + // We just pushed a local frame, so we know that the topmost local frame is the topmost + // frame. If we push a non-local frame, there's no need to do anything. + let stack_len = ecx.active_thread_stack().len(); + ecx.active_thread_mut().set_top_user_relevant_frame(stack_len - 1); + } + if ecx.machine.stacked_borrows.is_some() { ecx.retag_return_place() } else { Ok(()) } } @@ -1183,6 +1189,11 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { mut frame: Frame<'mir, 'tcx, Provenance, FrameData<'tcx>>, unwinding: bool, ) -> InterpResult<'tcx, StackPopJump> { + if frame.extra.is_user_relevant { + // All that we store is whether or not the frame we just removed is local, so now we + // have no idea where the next topmost local frame is. So we recompute it. + ecx.active_thread_mut().recompute_top_user_relevant_frame(); + } let timing = frame.extra.timing.take(); if let Some(stacked_borrows) = &ecx.machine.stacked_borrows { stacked_borrows.borrow_mut().end_call(&frame.extra); diff --git a/src/stacked_borrows/diagnostics.rs b/src/stacked_borrows/diagnostics.rs index f307bf11ed..662d8ada73 100644 --- a/src/stacked_borrows/diagnostics.rs +++ b/src/stacked_borrows/diagnostics.rs @@ -5,7 +5,6 @@ use rustc_middle::mir::interpret::{alloc_range, AllocId, AllocRange}; use rustc_span::{Span, SpanData}; use rustc_target::abi::Size; -use crate::helpers::CurrentSpan; use crate::stacked_borrows::{err_sb_ub, AccessKind, GlobalStateInner, Permission, ProtectorKind}; use crate::*; @@ -110,42 +109,29 @@ pub struct TagHistory { pub protected: Option<(String, SpanData)>, } -pub struct DiagnosticCxBuilder<'span, 'ecx, 'mir, 'tcx> { +pub struct DiagnosticCxBuilder<'ecx, 'mir, 'tcx> { operation: Operation, - // 'span cannot be merged with any other lifetime since they appear invariantly, under the - // mutable ref. - current_span: &'span mut CurrentSpan<'ecx, 'mir, 'tcx>, - threads: &'ecx ThreadManager<'mir, 'tcx>, + machine: &'ecx MiriMachine<'mir, 'tcx>, } -pub struct DiagnosticCx<'span, 'history, 'ecx, 'mir, 'tcx> { +pub struct DiagnosticCx<'history, 'ecx, 'mir, 'tcx> { operation: Operation, - // 'span and 'history cannot be merged, since when we call `unbuild` we need - // to return the exact 'span that was used when calling `build`. - current_span: &'span mut CurrentSpan<'ecx, 'mir, 'tcx>, - threads: &'ecx ThreadManager<'mir, 'tcx>, + machine: &'ecx MiriMachine<'mir, 'tcx>, history: &'history mut AllocHistory, offset: Size, } -impl<'span, 'ecx, 'mir, 'tcx> DiagnosticCxBuilder<'span, 'ecx, 'mir, 'tcx> { +impl<'ecx, 'mir, 'tcx> DiagnosticCxBuilder<'ecx, 'mir, 'tcx> { pub fn build<'history>( self, history: &'history mut AllocHistory, offset: Size, - ) -> DiagnosticCx<'span, 'history, 'ecx, 'mir, 'tcx> { - DiagnosticCx { - operation: self.operation, - current_span: self.current_span, - threads: self.threads, - history, - offset, - } + ) -> DiagnosticCx<'history, 'ecx, 'mir, 'tcx> { + DiagnosticCx { operation: self.operation, machine: self.machine, history, offset } } pub fn retag( - current_span: &'span mut CurrentSpan<'ecx, 'mir, 'tcx>, - threads: &'ecx ThreadManager<'mir, 'tcx>, + machine: &'ecx MiriMachine<'mir, 'tcx>, cause: RetagCause, new_tag: SbTag, orig_tag: ProvenanceExtra, @@ -154,46 +140,36 @@ impl<'span, 'ecx, 'mir, 'tcx> DiagnosticCxBuilder<'span, 'ecx, 'mir, 'tcx> { let operation = Operation::Retag(RetagOp { cause, new_tag, orig_tag, range, permission: None }); - DiagnosticCxBuilder { current_span, threads, operation } + DiagnosticCxBuilder { machine, operation } } pub fn read( - current_span: &'span mut CurrentSpan<'ecx, 'mir, 'tcx>, - threads: &'ecx ThreadManager<'mir, 'tcx>, + machine: &'ecx MiriMachine<'mir, 'tcx>, tag: ProvenanceExtra, range: AllocRange, ) -> Self { let operation = Operation::Access(AccessOp { kind: AccessKind::Read, tag, range }); - DiagnosticCxBuilder { current_span, threads, operation } + DiagnosticCxBuilder { machine, operation } } pub fn write( - current_span: &'span mut CurrentSpan<'ecx, 'mir, 'tcx>, - threads: &'ecx ThreadManager<'mir, 'tcx>, + machine: &'ecx MiriMachine<'mir, 'tcx>, tag: ProvenanceExtra, range: AllocRange, ) -> Self { let operation = Operation::Access(AccessOp { kind: AccessKind::Write, tag, range }); - DiagnosticCxBuilder { current_span, threads, operation } + DiagnosticCxBuilder { machine, operation } } - pub fn dealloc( - current_span: &'span mut CurrentSpan<'ecx, 'mir, 'tcx>, - threads: &'ecx ThreadManager<'mir, 'tcx>, - tag: ProvenanceExtra, - ) -> Self { + pub fn dealloc(machine: &'ecx MiriMachine<'mir, 'tcx>, tag: ProvenanceExtra) -> Self { let operation = Operation::Dealloc(DeallocOp { tag }); - DiagnosticCxBuilder { current_span, threads, operation } + DiagnosticCxBuilder { machine, operation } } } -impl<'span, 'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'span, 'history, 'ecx, 'mir, 'tcx> { - pub fn unbuild(self) -> DiagnosticCxBuilder<'span, 'ecx, 'mir, 'tcx> { - DiagnosticCxBuilder { - operation: self.operation, - current_span: self.current_span, - threads: self.threads, - } +impl<'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'history, 'ecx, 'mir, 'tcx> { + pub fn unbuild(self) -> DiagnosticCxBuilder<'ecx, 'mir, 'tcx> { + DiagnosticCxBuilder { machine: self.machine, operation: self.operation } } } @@ -234,10 +210,10 @@ struct DeallocOp { } impl AllocHistory { - pub fn new(id: AllocId, item: Item, current_span: &mut CurrentSpan<'_, '_, '_>) -> Self { + pub fn new(id: AllocId, item: Item, machine: &MiriMachine<'_, '_>) -> Self { Self { id, - base: (item, current_span.get()), + base: (item, machine.current_span()), creations: SmallVec::new(), invalidations: SmallVec::new(), protectors: SmallVec::new(), @@ -245,7 +221,7 @@ impl AllocHistory { } } -impl<'span, 'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'span, 'history, 'ecx, 'mir, 'tcx> { +impl<'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'history, 'ecx, 'mir, 'tcx> { pub fn start_grant(&mut self, perm: Permission) { let Operation::Retag(op) = &mut self.operation else { unreachable!("start_grant must only be called during a retag, this is: {:?}", self.operation) @@ -274,15 +250,17 @@ impl<'span, 'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'span, 'history, 'ecx, 'mir let Operation::Retag(op) = &self.operation else { unreachable!("log_creation must only be called during a retag") }; - self.history.creations.push(Creation { retag: op.clone(), span: self.current_span.get() }); + self.history + .creations + .push(Creation { retag: op.clone(), span: self.machine.current_span() }); } pub fn log_invalidation(&mut self, tag: SbTag) { - let mut span = self.current_span.get(); + let mut span = self.machine.current_span(); let (range, cause) = match &self.operation { Operation::Retag(RetagOp { cause, range, permission, .. }) => { if *cause == RetagCause::FnEntry { - span = self.current_span.get_caller(); + span = self.machine.caller_span(); } (*range, InvalidationCause::Retag(permission.unwrap(), *cause)) } @@ -301,7 +279,9 @@ impl<'span, 'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'span, 'history, 'ecx, 'mir let Operation::Retag(op) = &self.operation else { unreachable!("Protectors can only be created during a retag") }; - self.history.protectors.push(Protection { tag: op.new_tag, span: self.current_span.get() }); + self.history + .protectors + .push(Protection { tag: op.new_tag, span: self.machine.current_span() }); } pub fn get_logs_relevant_to( @@ -418,6 +398,7 @@ impl<'span, 'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'span, 'history, 'ecx, 'mir ProtectorKind::StrongProtector => "strongly protected", }; let call_id = self + .machine .threads .all_stacks() .flatten() @@ -482,9 +463,7 @@ impl<'span, 'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'span, 'history, 'ecx, 'mir Some((orig_tag, kind)) } }; - self.current_span - .machine() - .emit_diagnostic(NonHaltingDiagnostic::PoppedPointerTag(*item, summary)); + self.machine.emit_diagnostic(NonHaltingDiagnostic::PoppedPointerTag(*item, summary)); } } diff --git a/src/stacked_borrows/mod.rs b/src/stacked_borrows/mod.rs index bc52428910..e49b3e4f72 100644 --- a/src/stacked_borrows/mod.rs +++ b/src/stacked_borrows/mod.rs @@ -340,7 +340,7 @@ impl<'tcx> Stack { fn item_invalidated( item: &Item, global: &GlobalStateInner, - dcx: &mut DiagnosticCx<'_, '_, '_, '_, 'tcx>, + dcx: &mut DiagnosticCx<'_, '_, '_, 'tcx>, cause: ItemInvalidationCause, ) -> InterpResult<'tcx> { if !global.tracked_pointer_tags.is_empty() { @@ -385,7 +385,7 @@ impl<'tcx> Stack { access: AccessKind, tag: ProvenanceExtra, global: &GlobalStateInner, - dcx: &mut DiagnosticCx<'_, '_, '_, '_, 'tcx>, + dcx: &mut DiagnosticCx<'_, '_, '_, 'tcx>, exposed_tags: &FxHashSet, ) -> InterpResult<'tcx> { // Two main steps: Find granting item, remove incompatible items above. @@ -471,7 +471,7 @@ impl<'tcx> Stack { &mut self, tag: ProvenanceExtra, global: &GlobalStateInner, - dcx: &mut DiagnosticCx<'_, '_, '_, '_, 'tcx>, + dcx: &mut DiagnosticCx<'_, '_, '_, 'tcx>, exposed_tags: &FxHashSet, ) -> InterpResult<'tcx> { // Step 1: Make a write access. @@ -499,7 +499,7 @@ impl<'tcx> Stack { derived_from: ProvenanceExtra, new: Item, global: &GlobalStateInner, - dcx: &mut DiagnosticCx<'_, '_, '_, '_, 'tcx>, + dcx: &mut DiagnosticCx<'_, '_, '_, 'tcx>, exposed_tags: &FxHashSet, ) -> InterpResult<'tcx> { dcx.start_grant(new.perm()); @@ -590,14 +590,14 @@ impl<'tcx> Stacks { perm: Permission, tag: SbTag, id: AllocId, - current_span: &mut CurrentSpan<'_, '_, '_>, + machine: &MiriMachine<'_, '_>, ) -> Self { let item = Item::new(tag, perm, false); let stack = Stack::new(item); Stacks { stacks: RangeMap::new(size, stack), - history: AllocHistory::new(id, item, current_span), + history: AllocHistory::new(id, item, machine), exposed_tags: FxHashSet::default(), modified_since_last_gc: false, } @@ -607,10 +607,10 @@ impl<'tcx> Stacks { fn for_each( &mut self, range: AllocRange, - mut dcx_builder: DiagnosticCxBuilder<'_, '_, '_, 'tcx>, + mut dcx_builder: DiagnosticCxBuilder<'_, '_, 'tcx>, mut f: impl FnMut( &mut Stack, - &mut DiagnosticCx<'_, '_, '_, '_, 'tcx>, + &mut DiagnosticCx<'_, '_, '_, 'tcx>, &mut FxHashSet, ) -> InterpResult<'tcx>, ) -> InterpResult<'tcx> { @@ -631,7 +631,7 @@ impl Stacks { size: Size, state: &GlobalState, kind: MemoryKind, - mut current_span: CurrentSpan<'_, '_, '_>, + machine: &MiriMachine<'_, '_>, ) -> Self { let mut extra = state.borrow_mut(); let (base_tag, perm) = match kind { @@ -640,12 +640,11 @@ impl Stacks { // not through a pointer). That is, whenever we directly write to a local, this will pop // everything else off the stack, invalidating all previous pointers, // and in particular, *all* raw pointers. - MemoryKind::Stack => - (extra.base_ptr_tag(id, current_span.machine()), Permission::Unique), + MemoryKind::Stack => (extra.base_ptr_tag(id, machine), Permission::Unique), // Everything else is shared by default. - _ => (extra.base_ptr_tag(id, current_span.machine()), Permission::SharedReadWrite), + _ => (extra.base_ptr_tag(id, machine), Permission::SharedReadWrite), }; - Stacks::new(size, perm, base_tag, id, &mut current_span) + Stacks::new(size, perm, base_tag, id, machine) } #[inline(always)] @@ -655,8 +654,7 @@ impl Stacks { tag: ProvenanceExtra, range: AllocRange, state: &GlobalState, - mut current_span: CurrentSpan<'ecx, 'mir, 'tcx>, - threads: &'ecx ThreadManager<'mir, 'tcx>, + machine: &'ecx MiriMachine<'mir, 'tcx>, ) -> InterpResult<'tcx> where 'tcx: 'ecx, @@ -667,7 +665,7 @@ impl Stacks { Pointer::new(alloc_id, range.start), range.size.bytes() ); - let dcx = DiagnosticCxBuilder::read(&mut current_span, threads, tag, range); + let dcx = DiagnosticCxBuilder::read(machine, tag, range); let state = state.borrow(); self.for_each(range, dcx, |stack, dcx, exposed_tags| { stack.access(AccessKind::Read, tag, &state, dcx, exposed_tags) @@ -681,8 +679,7 @@ impl Stacks { tag: ProvenanceExtra, range: AllocRange, state: &GlobalState, - mut current_span: CurrentSpan<'ecx, 'mir, 'tcx>, - threads: &'ecx ThreadManager<'mir, 'tcx>, + machine: &'ecx MiriMachine<'mir, 'tcx>, ) -> InterpResult<'tcx> { trace!( "write access with tag {:?}: {:?}, size {}", @@ -690,7 +687,7 @@ impl Stacks { Pointer::new(alloc_id, range.start), range.size.bytes() ); - let dcx = DiagnosticCxBuilder::write(&mut current_span, threads, tag, range); + let dcx = DiagnosticCxBuilder::write(machine, tag, range); let state = state.borrow(); self.for_each(range, dcx, |stack, dcx, exposed_tags| { stack.access(AccessKind::Write, tag, &state, dcx, exposed_tags) @@ -704,11 +701,10 @@ impl Stacks { tag: ProvenanceExtra, range: AllocRange, state: &GlobalState, - mut current_span: CurrentSpan<'ecx, 'mir, 'tcx>, - threads: &'ecx ThreadManager<'mir, 'tcx>, + machine: &'ecx MiriMachine<'mir, 'tcx>, ) -> InterpResult<'tcx> { trace!("deallocation with tag {:?}: {:?}, size {}", tag, alloc_id, range.size.bytes()); - let dcx = DiagnosticCxBuilder::dealloc(&mut current_span, threads, tag); + let dcx = DiagnosticCxBuilder::dealloc(machine, tag); let state = state.borrow(); self.for_each(range, dcx, |stack, dcx, exposed_tags| { stack.dealloc(tag, &state, dcx, exposed_tags) @@ -773,7 +769,6 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' let (_size, _align, alloc_kind) = this.get_alloc_info(alloc_id); match alloc_kind { AllocKind::LiveData => { - let current_span = &mut this.machine.current_span(); // This should have alloc_extra data, but `get_alloc_extra` can still fail // if converting this alloc_id from a global to a local one // uncovers a non-supported `extern static`. @@ -783,12 +778,10 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' .as_ref() .expect("we should have Stacked Borrows data") .borrow_mut(); - let threads = &this.machine.threads; // Note that we create a *second* `DiagnosticCxBuilder` below for the actual retag. // FIXME: can this be done cleaner? let dcx = DiagnosticCxBuilder::retag( - current_span, - threads, + &this.machine, retag_cause, new_tag, orig_tag, @@ -895,8 +888,6 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' .as_ref() .expect("we should have Stacked Borrows data") .borrow_mut(); - // FIXME: can't share this with the current_span inside log_creation - let mut current_span = this.machine.current_span(); this.visit_freeze_sensitive(place, size, |mut range, frozen| { // Adjust range. range.start += base_offset; @@ -916,8 +907,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' let item = Item::new(new_tag, perm, protected); let global = this.machine.stacked_borrows.as_ref().unwrap().borrow(); let dcx = DiagnosticCxBuilder::retag( - &mut current_span, // FIXME avoid this `clone` - &this.machine.threads, + &this.machine, retag_cause, new_tag, orig_tag, @@ -943,11 +933,8 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' let item = Item::new(new_tag, perm, protect.is_some()); let range = alloc_range(base_offset, size); let global = machine.stacked_borrows.as_ref().unwrap().borrow(); - // FIXME: can't share this with the current_span inside log_creation - let current_span = &mut machine.current_span(); let dcx = DiagnosticCxBuilder::retag( - current_span, - &machine.threads, + machine, retag_cause, new_tag, orig_tag, From 6f2f3d4bdfc52e88fe7f66ee4ed8b0a966139485 Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Tue, 22 Nov 2022 10:19:29 -0500 Subject: [PATCH 2/5] Document is_user_relevant Co-authored-by: Ralf Jung --- src/helpers.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/helpers.rs b/src/helpers.rs index 958ed50496..70971cdc20 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -968,6 +968,7 @@ impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> { self.threads.active_thread_ref().top_user_relevant_frame() } + /// This is the source of truth for the `is_user_relevant` flag in our `FrameExtra`. pub fn is_user_relevant(&self, frame: &Frame<'mir, 'tcx, Provenance>) -> bool { let def_id = frame.instance.def_id(); (def_id.is_local() || self.local_crates.contains(&def_id.krate)) From 68223d6206e7f335c168e1aaa0a0ed2014610647 Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Tue, 22 Nov 2022 22:22:47 -0500 Subject: [PATCH 3/5] Update src/machine.rs Co-authored-by: Ralf Jung --- src/machine.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/machine.rs b/src/machine.rs index f5fa8f5f09..b7f434b555 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -1192,6 +1192,8 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { if frame.extra.is_user_relevant { // All that we store is whether or not the frame we just removed is local, so now we // have no idea where the next topmost local frame is. So we recompute it. + // (If this ever becomes a bottleneck, we could have `push` store the previous + // user-relevant frame and restore that here.) ecx.active_thread_mut().recompute_top_user_relevant_frame(); } let timing = frame.extra.timing.take(); From 2c76f0c0daa98db0b355f5c200004b98abe78c06 Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Fri, 25 Nov 2022 13:12:43 -0500 Subject: [PATCH 4/5] Use None when the stack is empty --- src/concurrency/thread.rs | 6 ++++-- src/helpers.rs | 15 +++++++-------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/concurrency/thread.rs b/src/concurrency/thread.rs index 96d988eb9a..dacb3a9b88 100644 --- a/src/concurrency/thread.rs +++ b/src/concurrency/thread.rs @@ -179,12 +179,14 @@ impl<'mir, 'tcx> Thread<'mir, 'tcx> { self.top_user_relevant_frame = Some(frame_idx); } - pub fn top_user_relevant_frame(&self) -> usize { + /// Returns the topmost frame that is considered user-relevant, or the + /// top of the stack if there is no such frame, or `None` if the stack is empty. + pub fn top_user_relevant_frame(&self) -> Option { debug_assert_eq!(self.top_user_relevant_frame, self.compute_top_user_relevant_frame()); // This can be called upon creation of an allocation. We create allocations while setting up // parts of the Rust runtime when we do not have any stack frames yet, so we need to handle // empty stacks. - self.top_user_relevant_frame.unwrap_or_else(|| self.stack.len().saturating_sub(1)) + self.top_user_relevant_frame.or_else(|| self.stack.len().checked_sub(1)) } } diff --git a/src/helpers.rs b/src/helpers.rs index 70971cdc20..cfa47c5c00 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -940,9 +940,8 @@ impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> { /// `#[track_caller]`. /// This function is backed by a cache, and can be assumed to be very fast. pub fn current_span(&self) -> Span { - self.stack() - .get(self.top_user_relevant_frame()) - .map(Frame::current_span) + self.top_user_relevant_frame() + .map(|frame_idx| self.stack()[frame_idx].current_span()) .unwrap_or(rustc_span::DUMMY_SP) } @@ -954,17 +953,17 @@ impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> { pub fn caller_span(&self) -> Span { // We need to go down at least to the caller (len - 2), or however // far we have to go to find a frame in a local crate which is also not #[track_caller]. - let frame_idx = self.top_user_relevant_frame(); - let stack = self.stack(); - let frame_idx = cmp::min(frame_idx, stack.len().saturating_sub(2)); - stack.get(frame_idx).map(Frame::current_span).unwrap_or(rustc_span::DUMMY_SP) + self.top_user_relevant_frame() + .map(|frame_idx| cmp::min(frame_idx, self.stack().len() - 2)) + .map(|frame_idx| self.stack()[frame_idx].current_span()) + .unwrap_or(rustc_span::DUMMY_SP) } fn stack(&self) -> &[Frame<'mir, 'tcx, Provenance, machine::FrameData<'tcx>>] { self.threads.active_thread_stack() } - fn top_user_relevant_frame(&self) -> usize { + fn top_user_relevant_frame(&self) -> Option { self.threads.active_thread_ref().top_user_relevant_frame() } From 86052a95515fbb301bdff0b97c8f8b15bf5b3cb3 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 26 Nov 2022 14:01:05 +0100 Subject: [PATCH 5/5] caller_span only makes sense when there are 2 frames on the stack --- src/helpers.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/helpers.rs b/src/helpers.rs index cfa47c5c00..8c7bc9eff0 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -939,6 +939,7 @@ impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> { /// Get the current span in the topmost function which is workspace-local and not /// `#[track_caller]`. /// This function is backed by a cache, and can be assumed to be very fast. + /// It will work even when the stack is empty. pub fn current_span(&self) -> Span { self.top_user_relevant_frame() .map(|frame_idx| self.stack()[frame_idx].current_span()) @@ -953,10 +954,9 @@ impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> { pub fn caller_span(&self) -> Span { // We need to go down at least to the caller (len - 2), or however // far we have to go to find a frame in a local crate which is also not #[track_caller]. - self.top_user_relevant_frame() - .map(|frame_idx| cmp::min(frame_idx, self.stack().len() - 2)) - .map(|frame_idx| self.stack()[frame_idx].current_span()) - .unwrap_or(rustc_span::DUMMY_SP) + let frame_idx = self.top_user_relevant_frame().unwrap(); + let frame_idx = cmp::min(frame_idx, self.stack().len().checked_sub(2).unwrap()); + self.stack()[frame_idx].current_span() } fn stack(&self) -> &[Frame<'mir, 'tcx, Provenance, machine::FrameData<'tcx>>] {