From 972b3b340ad99553618e551937d93bf28e2d2f5c Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Wed, 11 May 2022 19:13:00 -0400 Subject: [PATCH] Cleanup/Refactoring from review * Pass a ThreadInfo down to grant/access to get the current span lazily * Rename add_* to log_* for clarity * Hoist borrow_mut calls out of loops by tweaking the for_each signature * Explain the parameters of check_protector a bit more --- src/eval.rs | 4 -- src/machine.rs | 58 +++++---------------- src/stacked_borrows.rs | 84 ++++++++++++++++++------------ src/stacked_borrows/diagnostics.rs | 69 ++++++++++++++++++------ 4 files changed, 117 insertions(+), 98 deletions(-) diff --git a/src/eval.rs b/src/eval.rs index a3228f763a..f8d23cb827 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -15,7 +15,6 @@ use rustc_target::spec::abi::Abi; use rustc_session::config::EntryFnType; -use rustc_span::DUMMY_SP; use std::collections::HashSet; use crate::*; @@ -311,9 +310,6 @@ pub fn eval_entry<'tcx>( let info = ecx.preprocess_diagnostics(); match ecx.schedule()? { SchedulingAction::ExecuteStep => { - if let Some(sb) = ecx.machine.stacked_borrows.as_mut() { - sb.get_mut().current_span = DUMMY_SP; - } assert!(ecx.step()?, "a terminated thread was scheduled for execution"); } SchedulingAction::ExecuteTimeoutCallback => { diff --git a/src/machine.rs b/src/machine.rs index 2fb5dca6dd..7cb08066a6 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -25,7 +25,6 @@ use rustc_middle::{ }; use rustc_span::def_id::{CrateNum, DefId}; use rustc_span::symbol::{sym, Symbol}; -use rustc_span::DUMMY_SP; use rustc_target::abi::Size; use rustc_target::spec::abi::Abi; @@ -308,6 +307,7 @@ impl<'mir, 'tcx> Evaluator<'mir, 'tcx> { config.tracked_pointer_tags.clone(), config.tracked_call_ids.clone(), config.tag_raw, + local_crates.clone(), ))) } else { None @@ -562,7 +562,6 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> { alloc: Cow<'b, Allocation>, kind: Option>, ) -> Cow<'b, Allocation> { - set_current_span(&ecx.machine); if ecx.machine.tracked_alloc_ids.contains(&id) { register_diagnostic(NonHaltingDiagnostic::CreatedAlloc(id)); } @@ -570,7 +569,13 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> { let kind = kind.expect("we set our STATIC_KIND so this cannot be None"); let alloc = alloc.into_owned(); let stacks = if let Some(stacked_borrows) = &ecx.machine.stacked_borrows { - Some(Stacks::new_allocation(id, alloc.size(), stacked_borrows, kind)) + Some(Stacks::new_allocation( + id, + alloc.size(), + stacked_borrows, + kind, + &ecx.machine.threads, + )) } else { None }; @@ -591,7 +596,6 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> { ecx: &MiriEvalContext<'mir, 'tcx>, ptr: Pointer, ) -> Pointer { - set_current_span(&ecx.machine); let absolute_addr = intptrcast::GlobalStateInner::rel_ptr_to_addr(ecx, ptr); let sb_tag = if let Some(stacked_borrows) = &ecx.machine.stacked_borrows { stacked_borrows.borrow_mut().base_tag(ptr.provenance) @@ -627,7 +631,6 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> { (alloc_id, tag): (AllocId, Self::TagExtra), range: AllocRange, ) -> InterpResult<'tcx> { - set_current_span(&machine); if let Some(data_race) = &alloc_extra.data_race { data_race.read(alloc_id, range, machine.data_race.as_ref().unwrap())?; } @@ -637,6 +640,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> { tag, range, machine.stacked_borrows.as_ref().unwrap(), + &machine.threads, ) } else { Ok(()) @@ -651,7 +655,6 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> { (alloc_id, tag): (AllocId, Self::TagExtra), range: AllocRange, ) -> InterpResult<'tcx> { - set_current_span(&machine); if let Some(data_race) = &mut alloc_extra.data_race { data_race.write(alloc_id, range, machine.data_race.as_mut().unwrap())?; } @@ -661,6 +664,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> { tag, range, machine.stacked_borrows.as_ref().unwrap(), + &machine.threads, ) } else { Ok(()) @@ -675,7 +679,6 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> { (alloc_id, tag): (AllocId, Self::TagExtra), range: AllocRange, ) -> InterpResult<'tcx> { - set_current_span(&machine); if machine.tracked_alloc_ids.contains(&alloc_id) { register_diagnostic(NonHaltingDiagnostic::FreedAlloc(alloc_id)); } @@ -700,12 +703,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> { kind: mir::RetagKind, place: &PlaceTy<'tcx, Tag>, ) -> InterpResult<'tcx> { - if ecx.machine.stacked_borrows.is_some() { - set_current_span(&ecx.machine); - ecx.retag(kind, place) - } else { - Ok(()) - } + if ecx.machine.stacked_borrows.is_some() { ecx.retag(kind, place) } else { Ok(()) } } #[inline(always)] @@ -751,12 +749,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> { #[inline(always)] fn after_stack_push(ecx: &mut InterpCx<'mir, 'tcx, Self>) -> InterpResult<'tcx> { - if ecx.machine.stacked_borrows.is_some() { - set_current_span(&ecx.machine); - ecx.retag_return_place() - } else { - Ok(()) - } + if ecx.machine.stacked_borrows.is_some() { ecx.retag_return_place() } else { Ok(()) } } #[inline(always)] @@ -773,30 +766,3 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> { res } } - -// This is potentially a performance hazard. -// Factoring it into its own function lets us keep an eye on how much it shows up in a profile. -/// -fn set_current_span<'mir, 'tcx: 'mir>(machine: &Evaluator<'mir, 'tcx>) { - if let Some(sb) = machine.stacked_borrows.as_ref() { - if sb.borrow().current_span != DUMMY_SP { - return; - } - let current_span = machine - .threads - .active_thread_stack() - .into_iter() - .rev() - .find(|frame| { - let info = FrameInfo { - instance: frame.instance, - span: frame.current_span(), - lint_root: None, - }; - machine.is_local(&info) - }) - .map(|frame| frame.current_span()) - .unwrap_or(rustc_span::DUMMY_SP); - sb.borrow_mut().current_span = current_span; - } -} diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index fa7d72222e..1aec3c0e5e 100644 --- a/src/stacked_borrows.rs +++ b/src/stacked_borrows.rs @@ -14,7 +14,7 @@ use rustc_middle::ty::{ self, layout::{HasParamEnv, LayoutOf}, }; -use rustc_span::Span; +use rustc_span::def_id::CrateNum; use rustc_span::DUMMY_SP; use rustc_target::abi::Size; use std::collections::HashSet; @@ -118,10 +118,10 @@ pub struct GlobalStateInner { tracked_call_ids: HashSet, /// Whether to track raw pointers. tag_raw: bool, + /// Crates which are considered local for the purposes of error reporting. + local_crates: Vec, /// Extra per-allocation information extras: HashMap, - /// Current span - pub(crate) current_span: Span, } /// We need interior mutable access to the global state. @@ -174,6 +174,7 @@ impl GlobalStateInner { tracked_pointer_tags: HashSet, tracked_call_ids: HashSet, tag_raw: bool, + local_crates: Vec, ) -> Self { GlobalStateInner { next_ptr_id: NonZeroU64::new(1).unwrap(), @@ -183,8 +184,8 @@ impl GlobalStateInner { tracked_pointer_tags, tracked_call_ids, tag_raw, + local_crates, extras: HashMap::new(), - current_span: DUMMY_SP, } } @@ -325,6 +326,9 @@ impl<'tcx> Stack { /// The `provoking_access` argument is only used to produce diagnostics. /// It is `Some` when we are granting the contained access for said tag, and it is /// `None` during a deallocation. + /// Within `provoking_access, the `AllocRange` refers the entire operation, and + /// the `Size` refers to the specific location in the `AllocRange` that we are + /// currently checking. fn check_protector( item: &Item, provoking_access: Option<(SbTag, AllocId, AllocRange, Size, AccessKind)>, // just for debug printing and error messages @@ -378,6 +382,7 @@ impl<'tcx> Stack { tag: SbTag, (alloc_id, alloc_range, offset): (AllocId, AllocRange, Size), // just for debug printing and error messages global: &mut GlobalStateInner, + threads: &ThreadManager<'_, 'tcx>, ) -> InterpResult<'tcx> { // Two main steps: Find granting item, remove incompatible items above. @@ -399,7 +404,7 @@ impl<'tcx> Stack { Some((tag, alloc_id, alloc_range, offset, access)), global, )?; - global.add_invalidation(item.tag, alloc_id, alloc_range); + global.log_invalidation(item.tag, alloc_id, alloc_range, threads); } } else { // On a read, *disable* all `Unique` above the granting item. This ensures U2 for read accesses. @@ -420,7 +425,7 @@ impl<'tcx> Stack { global, )?; item.perm = Permission::Disabled; - global.add_invalidation(item.tag, alloc_id, alloc_range); + global.log_invalidation(item.tag, alloc_id, alloc_range, threads); } } } @@ -468,6 +473,7 @@ impl<'tcx> Stack { new: Item, (alloc_id, alloc_range, offset): (AllocId, AllocRange, Size), // just for debug printing and error messages global: &mut GlobalStateInner, + threads: &ThreadManager<'_, 'tcx>, ) -> InterpResult<'tcx> { // Figure out which access `perm` corresponds to. let access = @@ -495,7 +501,7 @@ impl<'tcx> Stack { // A "safe" reborrow for a pointer that actually expects some aliasing guarantees. // Here, creating a reference actually counts as an access. // This ensures F2b for `Unique`, by removing offending `SharedReadOnly`. - self.access(access, derived_from, (alloc_id, alloc_range, offset), global)?; + self.access(access, derived_from, (alloc_id, alloc_range, offset), global, threads)?; // We insert "as far up as possible": We know only compatible items are remaining // on top of `derived_from`, and we want the new item at the top so that we @@ -532,7 +538,7 @@ impl<'tcx> Stacks { fn for_each( &self, range: AllocRange, - f: impl Fn(Size, &mut Stack) -> InterpResult<'tcx>, + mut f: impl FnMut(Size, &mut Stack) -> InterpResult<'tcx>, ) -> InterpResult<'tcx> { let mut stacks = self.stacks.borrow_mut(); for (offset, stack) in stacks.iter_mut(range.start, range.size) { @@ -545,7 +551,7 @@ impl<'tcx> Stacks { fn for_each_mut( &mut self, range: AllocRange, - f: impl Fn(Size, &mut Stack) -> InterpResult<'tcx>, + mut f: impl FnMut(Size, &mut Stack) -> InterpResult<'tcx>, ) -> InterpResult<'tcx> { let stacks = self.stacks.get_mut(); for (offset, stack) in stacks.iter_mut(range.start, range.size) { @@ -562,6 +568,7 @@ impl Stacks { size: Size, state: &GlobalState, kind: MemoryKind, + threads: &ThreadManager<'_, '_>, ) -> Self { let mut extra = state.borrow_mut(); let (base_tag, perm) = match kind { @@ -595,7 +602,7 @@ impl Stacks { (tag, Permission::SharedReadWrite) } }; - extra.add_creation(None, base_tag, id, alloc_range(Size::ZERO, size)); + extra.log_creation(None, base_tag, id, alloc_range(Size::ZERO, size), threads); Stacks::new(size, perm, base_tag) } @@ -606,6 +613,7 @@ impl Stacks { tag: SbTag, range: AllocRange, state: &GlobalState, + threads: &ThreadManager<'_, 'tcx>, ) -> InterpResult<'tcx> { trace!( "read access with tag {:?}: {:?}, size {}", @@ -613,9 +621,9 @@ impl Stacks { Pointer::new(alloc_id, range.start), range.size.bytes() ); - self.for_each(range, move |offset, stack| { - let mut state = state.borrow_mut(); - stack.access(AccessKind::Read, tag, (alloc_id, range, offset), &mut state) + let mut state = state.borrow_mut(); + self.for_each(range, |offset, stack| { + stack.access(AccessKind::Read, tag, (alloc_id, range, offset), &mut state, threads) }) } @@ -626,6 +634,7 @@ impl Stacks { tag: SbTag, range: AllocRange, state: &GlobalState, + threads: &ThreadManager<'_, 'tcx>, ) -> InterpResult<'tcx> { trace!( "write access with tag {:?}: {:?}, size {}", @@ -633,9 +642,9 @@ impl Stacks { Pointer::new(alloc_id, range.start), range.size.bytes() ); - self.for_each_mut(range, move |offset, stack| { - let mut state = state.borrow_mut(); - stack.access(AccessKind::Write, tag, (alloc_id, range, offset), &mut state) + let mut state = state.borrow_mut(); + self.for_each_mut(range, |offset, stack| { + stack.access(AccessKind::Write, tag, (alloc_id, range, offset), &mut state, threads) }) } @@ -648,11 +657,11 @@ impl Stacks { state: &GlobalState, ) -> InterpResult<'tcx> { trace!("deallocation with tag {:?}: {:?}, size {}", tag, alloc_id, range.size.bytes()); - self.for_each_mut(range, move |offset, stack| { - let mut state = state.borrow_mut(); + let mut state = state.borrow_mut(); + self.for_each_mut(range, |offset, stack| { stack.dealloc(tag, (alloc_id, range, offset), &mut state) })?; - state.borrow_mut().extras.remove(&alloc_id); + state.extras.remove(&alloc_id); Ok(()) } } @@ -684,14 +693,15 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let (alloc_id, base_offset, orig_tag) = this.ptr_get_alloc_id(place.ptr)?; let mem_extra = this.machine.stacked_borrows.as_mut().unwrap().get_mut(); - mem_extra.add_creation( + mem_extra.log_creation( Some(orig_tag), new_tag, alloc_id, alloc_range(base_offset, base_offset + size), + &this.machine.threads, ); if protect { - mem_extra.add_protector(orig_tag, new_tag, alloc_id); + mem_extra.log_protector(orig_tag, new_tag, alloc_id, &this.machine.threads); } // Ensure we bail out if the pointer goes out-of-bounds (see miri#1050). @@ -752,10 +762,15 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx Permission::SharedReadWrite }; let item = Item { perm, tag: new_tag, protector }; + let mut global = this.machine.stacked_borrows.as_ref().unwrap().borrow_mut(); stacked_borrows.for_each(range, |offset, stack| { - let mut global = - this.machine.stacked_borrows.as_ref().unwrap().borrow_mut(); - stack.grant(orig_tag, item, (alloc_id, range, offset), &mut *global) + stack.grant( + orig_tag, + item, + (alloc_id, range, offset), + &mut *global, + &this.machine.threads, + ) }) })?; return Ok(()); @@ -764,15 +779,16 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // Here we can avoid `borrow()` calls because we have mutable references. // Note that this asserts that the allocation is mutable -- but since we are creating a // mutable pointer, that seems reasonable. - let (alloc_extra, memory_extra) = this.get_alloc_extra_mut(alloc_id)?; + let (alloc_extra, machine) = this.get_alloc_extra_mut(alloc_id)?; let stacked_borrows = alloc_extra.stacked_borrows.as_mut().expect("we should have Stacked Borrows data"); let item = Item { perm, tag: new_tag, protector }; let range = alloc_range(base_offset, size); + let mut global = machine.stacked_borrows.as_ref().unwrap().borrow_mut(); stacked_borrows.for_each_mut(range, |offset, stack| { - let mut global = memory_extra.stacked_borrows.as_ref().unwrap().borrow_mut(); - stack.grant(orig_tag, item, (alloc_id, range, offset), &mut *global) + stack.grant(orig_tag, item, (alloc_id, range, offset), &mut global, &machine.threads) })?; + Ok(()) } @@ -797,12 +813,14 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx }; // Compute new borrow. - let mem_extra = this.machine.stacked_borrows.as_mut().unwrap().get_mut(); - let new_tag = match kind { - // Give up tracking for raw pointers. - RefKind::Raw { .. } if !mem_extra.tag_raw => SbTag::Untagged, - // All other pointers are properly tracked. - _ => SbTag::Tagged(mem_extra.new_ptr()), + let new_tag = { + let mem_extra = this.machine.stacked_borrows.as_mut().unwrap().get_mut(); + match kind { + // Give up tracking for raw pointers. + RefKind::Raw { .. } if !mem_extra.tag_raw => SbTag::Untagged, + // All other pointers are properly tracked. + _ => SbTag::Tagged(mem_extra.new_ptr()), + } }; // Reborrow. diff --git a/src/stacked_borrows/diagnostics.rs b/src/stacked_borrows/diagnostics.rs index d5a65825bc..f657a926c0 100644 --- a/src/stacked_borrows/diagnostics.rs +++ b/src/stacked_borrows/diagnostics.rs @@ -7,6 +7,7 @@ use crate::stacked_borrows::{err_sb_ub, AccessKind, GlobalStateInner, Permission use crate::Item; use crate::SbTag; use crate::Stack; +use crate::ThreadManager; use rustc_middle::mir::interpret::InterpError; @@ -52,17 +53,32 @@ pub enum TagHistory { } pub trait GlobalStateExt { - fn add_creation( + fn current_span(&self, threads: &ThreadManager<'_, '_>) -> Span; + + fn log_creation( &mut self, parent: Option, tag: SbTag, alloc: AllocId, range: AllocRange, + threads: &ThreadManager<'_, '_>, ); - fn add_invalidation(&mut self, tag: SbTag, alloc: AllocId, range: AllocRange); + fn log_invalidation( + &mut self, + tag: SbTag, + alloc: AllocId, + range: AllocRange, + threads: &ThreadManager<'_, '_>, + ); - fn add_protector(&mut self, orig_tag: SbTag, tag: SbTag, alloc: AllocId); + fn log_protector( + &mut self, + orig_tag: SbTag, + tag: SbTag, + alloc: AllocId, + threads: &ThreadManager<'_, '_>, + ); fn get_stack_history( &self, @@ -75,39 +91,62 @@ pub trait GlobalStateExt { } impl GlobalStateExt for GlobalStateInner { - fn add_creation( + fn current_span(&self, threads: &ThreadManager<'_, '_>) -> Span { + threads + .active_thread_stack() + .into_iter() + .rev() + .find(|frame| { + let def_id = frame.instance.def_id(); + def_id.is_local() || self.local_crates.contains(&def_id.krate) + }) + .map(|frame| frame.current_span()) + .unwrap_or(rustc_span::DUMMY_SP) + } + + fn log_creation( &mut self, parent: Option, tag: SbTag, alloc: AllocId, range: AllocRange, + threads: &ThreadManager<'_, '_>, ) { + let span = self.current_span(threads); let extras = self.extras.entry(alloc).or_default(); - extras.creations.push(Event { - parent, - tag, - range, - span: self.current_span, - time: extras.current_time, - }); + extras.creations.push(Event { parent, tag, range, span, time: extras.current_time }); extras.current_time += 1; } - fn add_invalidation(&mut self, tag: SbTag, alloc: AllocId, range: AllocRange) { + fn log_invalidation( + &mut self, + tag: SbTag, + alloc: AllocId, + range: AllocRange, + threads: &ThreadManager<'_, '_>, + ) { + let span = self.current_span(threads); let extras = self.extras.entry(alloc).or_default(); extras.invalidations.push(Event { parent: None, tag, range, - span: self.current_span, + span, time: extras.current_time, }); extras.current_time += 1; } - fn add_protector(&mut self, orig_tag: SbTag, tag: SbTag, alloc: AllocId) { + fn log_protector( + &mut self, + orig_tag: SbTag, + tag: SbTag, + alloc: AllocId, + threads: &ThreadManager<'_, '_>, + ) { + let span = self.current_span(threads); let extras = self.extras.entry(alloc).or_default(); - extras.protectors.push(Protection { orig_tag, tag, span: self.current_span }); + extras.protectors.push(Protection { orig_tag, tag, span }); extras.current_time += 1; }