From f3f7e083dc92aba7a4c1818e56464fcb79f22f19 Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Sat, 12 Mar 2022 17:23:22 -0500 Subject: [PATCH 1/6] Print spans where tags are created and invalidated --- src/diagnostics.rs | 46 +++++- src/eval.rs | 21 +++ src/helpers.rs | 9 ++ src/machine.rs | 6 +- src/stacked_borrows.rs | 311 +++++++++++++++++++++++++++++++++++------ 5 files changed, 341 insertions(+), 52 deletions(-) diff --git a/src/diagnostics.rs b/src/diagnostics.rs index 1a39a1ff33..c6b6c3388c 100644 --- a/src/diagnostics.rs +++ b/src/diagnostics.rs @@ -7,7 +7,8 @@ use log::trace; use rustc_middle::ty; use rustc_span::{source_map::DUMMY_SP, Span, SpanData, Symbol}; -use crate::stacked_borrows::{AccessKind, SbTag}; +use crate::helpers::HexRange; +use crate::stacked_borrows::{AccessKind, SbTag, TagHistory}; use crate::*; /// Details of premature program termination. @@ -19,6 +20,7 @@ pub enum TerminationInfo { msg: String, help: Option, url: String, + history: Option, }, Deadlock, MultipleSymbolDefinitions { @@ -155,12 +157,46 @@ pub fn report_error<'tcx, 'mir>( (None, format!("pass the flag `-Zmiri-disable-isolation` to disable isolation;")), (None, format!("or pass `-Zmiri-isolation-error=warn` to configure Miri to return an error code from isolated operations (if supported for that operation) and continue with a warning")), ], - ExperimentalUb { url, help, .. } => { + ExperimentalUb { url, help, history, .. } => { msg.extend(help.clone()); - vec![ + let mut helps = vec![ (None, format!("this indicates a potential bug in the program: it performed an invalid operation, but the rules it violated are still experimental")), - (None, format!("see {} for further information", url)) - ] + (None, format!("see {} for further information", url)), + ]; + match history { + Some(TagHistory::Tagged {tag, created: (created_range, created_span), invalidated, protected }) => { + let msg = format!("{:?} was created due to a retag at offsets {}", tag, HexRange(*created_range)); + helps.push((Some(created_span.clone()), msg)); + if let Some((invalidated_range, invalidated_span)) = invalidated { + let msg = format!("{:?} was later invalidated due to a retag at offsets {}", tag, HexRange(*invalidated_range)); + helps.push((Some(invalidated_span.clone()), msg)); + } + if let Some((protecting_tag, protecting_tag_span, protection_span)) = protected { + helps.push((Some(protecting_tag_span.clone()), format!("{:?} was protected due to {:?} which was created here", tag, protecting_tag))); + helps.push((Some(protection_span.clone()), "this protector is live for this call".to_string())); + } + } + Some(TagHistory::Untagged{ recently_created, recently_invalidated, matching_created, protected }) => { + if let Some((range, span)) = recently_created { + let msg = format!("tag was most recently created at offsets {}", HexRange(*range)); + helps.push((Some(span.clone()), msg)); + } + if let Some((range, span)) = recently_invalidated { + let msg = format!("tag was later invalidated at offsets {}", HexRange(*range)); + helps.push((Some(span.clone()), msg)); + } + if let Some((range, span)) = matching_created { + let msg = format!("this tag was also created here at offsets {}", HexRange(*range)); + helps.push((Some(span.clone()), msg)); + } + if let Some((protecting_tag, protecting_tag_span, protection_span)) = protected { + helps.push((Some(protecting_tag_span.clone()), format!("{:?} was protected due to a tag which was created here", protecting_tag))); + helps.push((Some(protection_span.clone()), "this protector is live for this call".to_string())); + } + } + None => {} + } + helps } MultipleSymbolDefinitions { first, first_crate, second, second_crate, .. } => vec![ diff --git a/src/eval.rs b/src/eval.rs index f8d23cb827..8b964ba90f 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -283,6 +283,24 @@ pub fn create_ecx<'mir, 'tcx: 'mir>( Ok((ecx, ret_place)) } +// 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>(ecx: &mut MiriEvalContext<'mir, 'tcx>) { + let current_span = Machine::stack(&ecx) + .into_iter() + .rev() + .find(|frame| { + let info = + FrameInfo { instance: frame.instance, span: frame.current_span(), lint_root: None }; + ecx.machine.is_local(&info) + }) + .map(|frame| frame.current_span()) + .unwrap_or(rustc_span::DUMMY_SP); + if let Some(sb) = ecx.machine.stacked_borrows.as_mut() { + sb.get_mut().current_span = current_span; + } +} + /// Evaluates the entry function specified by `entry_id`. /// Returns `Some(return_code)` if program executed completed. /// Returns `None` if an evaluation error occured. @@ -310,6 +328,9 @@ pub fn eval_entry<'tcx>( let info = ecx.preprocess_diagnostics(); match ecx.schedule()? { SchedulingAction::ExecuteStep => { + if ecx.machine.stacked_borrows.is_some() { + set_current_span(&mut ecx); + } assert!(ecx.step()?, "a terminated thread was scheduled for execution"); } SchedulingAction::ExecuteTimeoutCallback => { diff --git a/src/helpers.rs b/src/helpers.rs index 107a255199..8d7147fff7 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -813,3 +813,12 @@ pub fn get_local_crates(tcx: &TyCtxt<'_>) -> Vec { } local_crates } + +/// Formats an AllocRange like [0x1..0x3], for use in diagnostics. +pub struct HexRange(pub AllocRange); + +impl std::fmt::Display for HexRange { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "[{:#x}..{:#x}]", self.0.start.bytes(), self.0.end().bytes()) + } +} diff --git a/src/machine.rs b/src/machine.rs index 0a8a229c8a..5f25fd2988 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -632,7 +632,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> { alloc_id, tag, range, - machine.stacked_borrows.as_ref().unwrap(), + machine.stacked_borrows.as_ref().unwrap(), ) } else { Ok(()) @@ -655,7 +655,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> { alloc_id, tag, range, - machine.stacked_borrows.as_mut().unwrap(), + machine.stacked_borrows.as_ref().unwrap(), ) } else { Ok(()) @@ -681,7 +681,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> { alloc_id, tag, range, - machine.stacked_borrows.as_mut().unwrap(), + machine.stacked_borrows.as_ref().unwrap(), ) } else { Ok(()) diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index d6caba8171..225407a7bb 100644 --- a/src/stacked_borrows.rs +++ b/src/stacked_borrows.rs @@ -3,6 +3,7 @@ use log::trace; use std::cell::RefCell; +use std::collections::HashMap; use std::fmt; use std::num::NonZeroU64; @@ -14,9 +15,11 @@ use rustc_middle::ty::{ layout::{HasParamEnv, LayoutOf}, }; use rustc_span::DUMMY_SP; +use rustc_span::{Span, SpanData}; use rustc_target::abi::Size; use std::collections::HashSet; +use crate::helpers::HexRange; use crate::*; pub type PtrId = NonZeroU64; @@ -111,7 +114,53 @@ pub struct GlobalStateInner { tracked_call_ids: HashSet, /// Whether to track raw pointers. tag_raw: bool, + /// Extra per-allocation information + extras: HashMap, + /// Current span + pub(crate) current_span: Span, } + +#[derive(Debug, Default)] +struct AllocHistory { + // The time tags can be compressed down to one bit per event, by just storing a Vec + // where each bit is set to indicate if the event was a creation or a retag + current_time: usize, + creations: Vec, + invalidations: Vec, + protectors: Vec, +} + +#[derive(Debug)] +struct Protection { + orig_tag: SbTag, + tag: SbTag, + span: Span, +} + +#[derive(Debug)] +struct Event { + time: usize, + parent: Option, + tag: SbTag, + range: AllocRange, + span: Span, +} + +pub enum TagHistory { + Tagged { + tag: SbTag, + created: (AllocRange, SpanData), + invalidated: Option<(AllocRange, SpanData)>, + protected: Option<(SbTag, SpanData, SpanData)>, + }, + Untagged { + recently_created: Option<(AllocRange, SpanData)>, + recently_invalidated: Option<(AllocRange, SpanData)>, + matching_created: Option<(AllocRange, SpanData)>, + protected: Option<(SbTag, SpanData, SpanData)>, + }, +} + /// We need interior mutable access to the global state. pub type GlobalState = RefCell; @@ -171,6 +220,8 @@ impl GlobalStateInner { tracked_pointer_tags, tracked_call_ids, tag_raw, + extras: HashMap::new(), + current_span: DUMMY_SP, } } @@ -218,16 +269,155 @@ impl GlobalStateInner { self.base_ptr_ids.try_insert(id, tag).unwrap(); tag } + + fn add_creation( + &mut self, + parent: Option, + tag: SbTag, + alloc: AllocId, + range: AllocRange, + ) { + let extras = self.extras.entry(alloc).or_default(); + extras.creations.push(Event { + parent, + tag, + range, + span: self.current_span, + time: extras.current_time, + }); + extras.current_time += 1; + } + + fn add_invalidation(&mut self, tag: SbTag, alloc: AllocId, range: AllocRange) { + let extras = self.extras.entry(alloc).or_default(); + extras.invalidations.push(Event { + parent: None, + tag, + range, + span: self.current_span, + time: extras.current_time, + }); + extras.current_time += 1; + } + + fn add_protector(&mut self, orig_tag: SbTag, tag: SbTag, alloc: AllocId) { + let extras = self.extras.entry(alloc).or_default(); + extras.protectors.push(Protection { orig_tag, tag, span: self.current_span }); + extras.current_time += 1; + } + + fn get_stack_history( + &self, + tag: SbTag, + alloc: AllocId, + alloc_range: AllocRange, + offset: Size, + protector_tag: Option, + ) -> Option { + let extras = self.extras.get(&alloc)?; + let protected = protector_tag + .and_then(|protector| { + extras.protectors.iter().find_map(|protection| { + if protection.tag == protector { + Some((protection.orig_tag, protection.span.data())) + } else { + None + } + }) + }) + .and_then(|(tag, call_span)| { + extras.creations.iter().rev().find_map(|event| { + if event.tag == tag { + Some((event.parent?, event.span.data(), call_span)) + } else { + None + } + }) + }); + if let SbTag::Tagged(_) = tag { + let get_matching = |events: &[Event]| { + events.iter().rev().find_map(|event| { + if event.tag == tag { Some((event.range, event.span.data())) } else { None } + }) + }; + Some(TagHistory::Tagged { + tag, + created: get_matching(&extras.creations)?, + invalidated: get_matching(&extras.invalidations), + protected, + }) + } else { + let mut created_time = 0; + // Find the most recently created tag that satsfies this offset + let recently_created = extras.creations.iter().rev().find_map(|event| { + if event.tag == tag && offset >= event.range.start && offset < event.range.end() { + created_time = event.time; + Some((event.range, event.span.data())) + } else { + None + } + }); + + // Find a different recently created tag that satisfies this whole operation, predates + // the recently created tag, and has a different span. + // We're trying to make a guess at which span the user wanted to provide the tag that + // they're using. + let matching_created = if let Some((_created_range, created_span)) = recently_created { + extras.creations.iter().rev().find_map(|event| { + if event.tag == tag + && alloc_range.start >= event.range.start + && alloc_range.end() <= event.range.end() + && event.span.data() != created_span + && event.time != created_time + { + Some((event.range, event.span.data())) + } else { + None + } + }) + } else { + None + }; + + let recently_invalidated = if recently_created.is_some() { + // Find the most recent invalidation of this tag which post-dates the creation + let mut found = None; + for event in extras.invalidations.iter().rev() { + if event.time < created_time { + break; + } + if event.tag == tag && offset >= event.range.start && offset < event.range.end() + { + found = Some((event.range, event.span.data())) + } + } + found + } else { + None + }; + Some(TagHistory::Untagged { + recently_created, + matching_created, + recently_invalidated, + protected, + }) + } + } } /// Error reporting -fn err_sb_ub(msg: String, help: Option) -> InterpError<'static> { +fn err_sb_ub( + msg: String, + help: Option, + history: Option, +) -> InterpError<'static> { err_machine_stop!(TerminationInfo::ExperimentalUb { msg, help, url: format!( "https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md" ), + history }) } @@ -308,31 +498,39 @@ impl<'tcx> Stack { /// `None` during a deallocation. fn check_protector( item: &Item, - provoking_access: Option<(SbTag, AccessKind)>, + provoking_access: Option<(SbTag, AllocId, AllocRange, Size)>, // just for debug printing amd error messages global: &GlobalStateInner, ) -> InterpResult<'tcx> { if let SbTag::Tagged(id) = item.tag { if global.tracked_pointer_tags.contains(&id) { register_diagnostic(NonHaltingDiagnostic::PoppedPointerTag( *item, - provoking_access, + None, )); } } if let Some(call) = item.protector { if global.is_active(call) { - if let Some((tag, _)) = provoking_access { + if let Some((tag, alloc_id, alloc_range, offset)) = provoking_access { Err(err_sb_ub( format!( "not granting access to tag {:?} because incompatible item is protected: {:?}", tag, item ), None, + global.get_stack_history( + tag, + alloc_id, + alloc_range, + offset, + Some(item.tag), + ), ))? } else { Err(err_sb_ub( format!("deallocating while item is protected: {:?}", item), None, + None, ))? } } @@ -348,15 +546,15 @@ impl<'tcx> Stack { &mut self, access: AccessKind, tag: SbTag, - (alloc_id, range, offset): (AllocId, AllocRange, Size), // just for debug printing and error messages - global: &GlobalStateInner, + (alloc_id, alloc_range, offset): (AllocId, AllocRange, Size), // just for debug printing and error messages + global: &mut GlobalStateInner, ) -> InterpResult<'tcx> { // Two main steps: Find granting item, remove incompatible items above. // Step 1: Find granting item. - let granting_idx = self - .find_granting(access, tag) - .ok_or_else(|| self.access_error(access, tag, alloc_id, range, offset))?; + let granting_idx = self.find_granting(access, tag).ok_or_else(|| { + self.access_error(access, tag, alloc_id, alloc_range, offset, global) + })?; // Step 2: Remove incompatible items above them. Make sure we do not remove protected // items. Behavior differs for reads and writes. @@ -366,7 +564,8 @@ impl<'tcx> Stack { let first_incompatible_idx = self.find_first_write_incompatible(granting_idx); for item in self.borrows.drain(first_incompatible_idx..).rev() { trace!("access: popping item {:?}", item); - Stack::check_protector(&item, Some((tag, access)), global)?; + Stack::check_protector(&item, Some((tag, alloc_id, alloc_range, offset)), global)?; + global.add_invalidation(item.tag, alloc_id, alloc_range); } } else { // On a read, *disable* all `Unique` above the granting item. This ensures U2 for read accesses. @@ -381,8 +580,13 @@ impl<'tcx> Stack { let item = &mut self.borrows[idx]; if item.perm == Permission::Unique { trace!("access: disabling item {:?}", item); - Stack::check_protector(item, Some((tag, access)), global)?; + Stack::check_protector( + item, + Some((tag, alloc_id, alloc_range, offset)), + global, + )?; item.perm = Permission::Disabled; + global.add_invalidation(item.tag, alloc_id, alloc_range); } } } @@ -396,15 +600,18 @@ impl<'tcx> Stack { fn dealloc( &mut self, tag: SbTag, - dbg_ptr: Pointer, // just for debug printing and error messages + (alloc_id, alloc_range, offset): (AllocId, AllocRange, Size), // just for debug printing amd error messages global: &GlobalStateInner, ) -> InterpResult<'tcx> { // Step 1: Find granting item. self.find_granting(AccessKind::Write, tag).ok_or_else(|| { err_sb_ub(format!( "no item granting write access for deallocation to tag {:?} at {:?} found in borrow stack", - tag, dbg_ptr, - ), None) + tag, alloc_id, + ), + None, + global.get_stack_history(tag, alloc_id, alloc_range, offset, None), + ) })?; // Step 2: Remove all items. Also checks for protectors. @@ -426,16 +633,16 @@ impl<'tcx> Stack { derived_from: SbTag, new: Item, (alloc_id, alloc_range, offset): (AllocId, AllocRange, Size), // just for debug printing and error messages - global: &GlobalStateInner, + global: &mut GlobalStateInner, ) -> InterpResult<'tcx> { // Figure out which access `perm` corresponds to. let access = if new.perm.grants(AccessKind::Write) { AccessKind::Write } else { AccessKind::Read }; // Now we figure out which item grants our parent (`derived_from`) this kind of access. // We use that to determine where to put the new item. - let granting_idx = self - .find_granting(access, derived_from) - .ok_or_else(|| self.grant_error(derived_from, new, alloc_id, alloc_range, offset))?; + let granting_idx = self.find_granting(access, derived_from).ok_or_else(|| { + self.grant_error(derived_from, new, alloc_id, alloc_range, offset, global) + })?; // Compute where to put the new item. // Either way, we ensure that we insert the new item in a way such that between @@ -483,6 +690,7 @@ impl<'tcx> Stack { alloc_id: AllocId, alloc_range: AllocRange, error_offset: Size, + global: &GlobalStateInner, ) -> InterpError<'static> { let action = format!( "trying to reborrow {:?} for {:?} permission at {}[{:#x}]", @@ -494,6 +702,7 @@ impl<'tcx> Stack { err_sb_ub( format!("{}{}", action, self.error_cause(derived_from)), Some(Self::operation_summary("a reborrow", alloc_id, alloc_range)), + global.get_stack_history(derived_from, alloc_id, alloc_range, error_offset, None), ) } @@ -505,6 +714,7 @@ impl<'tcx> Stack { alloc_id: AllocId, alloc_range: AllocRange, error_offset: Size, + global: &GlobalStateInner, ) -> InterpError<'static> { let action = format!( "attempting a {} using {:?} at {}[{:#x}]", @@ -516,6 +726,7 @@ impl<'tcx> Stack { err_sb_ub( format!("{}{}", action, self.error_cause(tag)), Some(Self::operation_summary("an access", alloc_id, alloc_range)), + global.get_stack_history(tag, alloc_id, alloc_range, error_offset, None), ) } @@ -525,11 +736,10 @@ impl<'tcx> Stack { alloc_range: AllocRange, ) -> String { format!( - "this error occurs as part of {} at {:?}[{:#x}..{:#x}]", + "this error occurs as part of {} at {:?}{}", operation, alloc_id, - alloc_range.start.bytes(), - alloc_range.end().bytes() + HexRange(alloc_range) ) } @@ -620,6 +830,7 @@ impl Stacks { (tag, Permission::SharedReadWrite) } }; + extra.add_creation(None, base_tag, id, alloc_range(Size::ZERO, size)); Stacks::new(size, perm, base_tag) } @@ -637,11 +848,11 @@ impl Stacks { Pointer::new(alloc_id, range.start), range.size.bytes() ); - let global = &*state.borrow(); self.for_each(range, move |offset, stack| { - stack.access(AccessKind::Read, tag, (alloc_id, range, offset), global) + let mut state = state.borrow_mut(); + stack.access(AccessKind::Read, tag, (alloc_id, range, offset), &mut state) }) - } + } #[inline(always)] pub fn memory_written<'tcx>( @@ -649,7 +860,7 @@ impl Stacks { alloc_id: AllocId, tag: SbTag, range: AllocRange, - state: &mut GlobalState, + state: &GlobalState, ) -> InterpResult<'tcx> { trace!( "write access with tag {:?}: {:?}, size {}", @@ -657,9 +868,9 @@ impl Stacks { Pointer::new(alloc_id, range.start), range.size.bytes() ); - let global = state.get_mut(); self.for_each_mut(range, move |offset, stack| { - stack.access(AccessKind::Write, tag, (alloc_id, range, offset), global) + let mut state = state.borrow_mut(); + stack.access(AccessKind::Write, tag, (alloc_id, range, offset), &mut state) }) } @@ -669,13 +880,15 @@ impl Stacks { alloc_id: AllocId, tag: SbTag, range: AllocRange, - state: &mut GlobalState, + state: &GlobalState, ) -> InterpResult<'tcx> { trace!("deallocation with tag {:?}: {:?}, size {}", tag, alloc_id, range.size.bytes()); - let global = state.get_mut(); self.for_each_mut(range, move |offset, stack| { - stack.dealloc(tag, Pointer::new(alloc_id, offset), global) - }) + let mut state = state.borrow_mut(); + stack.dealloc(tag, (alloc_id, range, offset), &mut state) + })?; + state.borrow_mut().extras.remove(&alloc_id); + Ok(()) } } @@ -705,6 +918,17 @@ 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( + Some(orig_tag), + new_tag, + alloc_id, + alloc_range(base_offset, base_offset + size), + ); + if protect { + mem_extra.add_protector(orig_tag, new_tag, alloc_id); + } + // Ensure we bail out if the pointer goes out-of-bounds (see miri#1050). let (alloc_size, _) = this.get_alloc_size_and_align(alloc_id, AllocCheck::Dereferenceable)?; @@ -753,7 +977,6 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let extra = this.get_alloc_extra(alloc_id)?; let stacked_borrows = extra.stacked_borrows.as_ref().expect("we should have Stacked Borrows data"); - let global = this.machine.stacked_borrows.as_ref().unwrap().borrow(); this.visit_freeze_sensitive(place, size, |mut range, frozen| { // Adjust range. range.start += base_offset; @@ -765,7 +988,9 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx }; let item = Item { perm, tag: new_tag, protector }; stacked_borrows.for_each(range, |offset, stack| { - stack.grant(orig_tag, item, (alloc_id, range, offset), &*global) + let mut global = + this.machine.stacked_borrows.as_ref().unwrap().borrow_mut(); + stack.grant(orig_tag, item, (alloc_id, range, offset), &mut *global) }) })?; return Ok(()); @@ -777,11 +1002,11 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let (alloc_extra, memory_extra) = this.get_alloc_extra_mut(alloc_id)?; let stacked_borrows = alloc_extra.stacked_borrows.as_mut().expect("we should have Stacked Borrows data"); - let global = memory_extra.stacked_borrows.as_mut().unwrap().get_mut(); let item = Item { perm, tag: new_tag, protector }; let range = alloc_range(base_offset, size); - stacked_borrows.for_each_mut(alloc_range(base_offset, size), |offset, stack| { - stack.grant(orig_tag, item, (alloc_id, range, offset), global) + 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) })?; Ok(()) } @@ -807,14 +1032,12 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx }; // Compute new borrow. - 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()), - } + 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()), }; // Reborrow. From 5861d137b25997a7f4947fe6046003d2d0facde2 Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Fri, 15 Apr 2022 23:04:07 -0400 Subject: [PATCH 2/6] Set the current span (somewhat) lazily --- src/diagnostics.rs | 4 ++-- src/eval.rs | 23 +++----------------- src/machine.rs | 48 +++++++++++++++++++++++++++++++++++++++--- src/stacked_borrows.rs | 14 ++++++------ src/thread.rs | 2 +- 5 files changed, 58 insertions(+), 33 deletions(-) diff --git a/src/diagnostics.rs b/src/diagnostics.rs index c6b6c3388c..bba1989e2d 100644 --- a/src/diagnostics.rs +++ b/src/diagnostics.rs @@ -165,10 +165,10 @@ pub fn report_error<'tcx, 'mir>( ]; match history { Some(TagHistory::Tagged {tag, created: (created_range, created_span), invalidated, protected }) => { - let msg = format!("{:?} was created due to a retag at offsets {}", tag, HexRange(*created_range)); + let msg = format!("{:?} was created by a retag at offsets {}", tag, HexRange(*created_range)); helps.push((Some(created_span.clone()), msg)); if let Some((invalidated_range, invalidated_span)) = invalidated { - let msg = format!("{:?} was later invalidated due to a retag at offsets {}", tag, HexRange(*invalidated_range)); + let msg = format!("{:?} was later invalidated at offsets {}", tag, HexRange(*invalidated_range)); helps.push((Some(invalidated_span.clone()), msg)); } if let Some((protecting_tag, protecting_tag_span, protection_span)) = protected { diff --git a/src/eval.rs b/src/eval.rs index 8b964ba90f..80738d33e0 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -16,6 +16,7 @@ use rustc_target::spec::abi::Abi; use rustc_session::config::EntryFnType; use std::collections::HashSet; +use rustc_span::DUMMY_SP; use crate::*; @@ -283,24 +284,6 @@ pub fn create_ecx<'mir, 'tcx: 'mir>( Ok((ecx, ret_place)) } -// 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>(ecx: &mut MiriEvalContext<'mir, 'tcx>) { - let current_span = Machine::stack(&ecx) - .into_iter() - .rev() - .find(|frame| { - let info = - FrameInfo { instance: frame.instance, span: frame.current_span(), lint_root: None }; - ecx.machine.is_local(&info) - }) - .map(|frame| frame.current_span()) - .unwrap_or(rustc_span::DUMMY_SP); - if let Some(sb) = ecx.machine.stacked_borrows.as_mut() { - sb.get_mut().current_span = current_span; - } -} - /// Evaluates the entry function specified by `entry_id`. /// Returns `Some(return_code)` if program executed completed. /// Returns `None` if an evaluation error occured. @@ -328,8 +311,8 @@ pub fn eval_entry<'tcx>( let info = ecx.preprocess_diagnostics(); match ecx.schedule()? { SchedulingAction::ExecuteStep => { - if ecx.machine.stacked_borrows.is_some() { - set_current_span(&mut ecx); + 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"); } diff --git a/src/machine.rs b/src/machine.rs index 5f25fd2988..de9aaa2859 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -25,6 +25,7 @@ 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; @@ -561,6 +562,7 @@ 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)); } @@ -589,6 +591,7 @@ 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) @@ -624,6 +627,7 @@ 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())?; } @@ -632,7 +636,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> { alloc_id, tag, range, - machine.stacked_borrows.as_ref().unwrap(), + machine.stacked_borrows.as_ref().unwrap(), ) } else { Ok(()) @@ -647,6 +651,7 @@ 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())?; } @@ -670,6 +675,7 @@ 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)); } @@ -694,7 +700,12 @@ 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() { ecx.retag(kind, place) } else { Ok(()) } + if ecx.machine.stacked_borrows.is_some() { + set_current_span(&ecx.machine); + ecx.retag(kind, place) + } else { + Ok(()) + } } #[inline(always)] @@ -740,7 +751,12 @@ 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() { ecx.retag_return_place() } else { Ok(()) } + if ecx.machine.stacked_borrows.is_some() { + set_current_span(&ecx.machine); + ecx.retag_return_place() + } else { + Ok(()) + } } #[inline(always)] @@ -757,3 +773,29 @@ 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 225407a7bb..fcb702ea6b 100644 --- a/src/stacked_borrows.rs +++ b/src/stacked_borrows.rs @@ -125,9 +125,9 @@ struct AllocHistory { // The time tags can be compressed down to one bit per event, by just storing a Vec // where each bit is set to indicate if the event was a creation or a retag current_time: usize, - creations: Vec, - invalidations: Vec, - protectors: Vec, + creations: smallvec::SmallVec<[Event; 2]>, + invalidations: smallvec::SmallVec<[Event; 1]>, + protectors: smallvec::SmallVec<[Protection; 1]>, } #[derive(Debug)] @@ -552,9 +552,9 @@ impl<'tcx> Stack { // Two main steps: Find granting item, remove incompatible items above. // Step 1: Find granting item. - let granting_idx = self.find_granting(access, tag).ok_or_else(|| { - self.access_error(access, tag, alloc_id, alloc_range, offset, global) - })?; + let granting_idx = self + .find_granting(access, tag) + .ok_or_else(|| self.access_error(access, tag, alloc_id, alloc_range, offset, global))?; // Step 2: Remove incompatible items above them. Make sure we do not remove protected // items. Behavior differs for reads and writes. @@ -852,7 +852,7 @@ impl Stacks { let mut state = state.borrow_mut(); stack.access(AccessKind::Read, tag, (alloc_id, range, offset), &mut state) }) - } + } #[inline(always)] pub fn memory_written<'tcx>( diff --git a/src/thread.rs b/src/thread.rs index 8edd6672a7..5673af048f 100644 --- a/src/thread.rs +++ b/src/thread.rs @@ -263,7 +263,7 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { } /// Borrow the stack of the active thread. - fn active_thread_stack(&self) -> &[Frame<'mir, 'tcx, Tag, FrameData<'tcx>>] { + pub fn active_thread_stack(&self) -> &[Frame<'mir, 'tcx, Tag, FrameData<'tcx>>] { &self.threads[self.active_thread].stack } From cddd85e2f30fb185de55209f5843a235cdc79a88 Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Mon, 25 Apr 2022 14:18:52 -0400 Subject: [PATCH 3/6] Move SB diagnostics to a module --- src/diagnostics.rs | 2 +- src/eval.rs | 2 +- src/machine.rs | 1 + src/stacked_borrows.rs | 258 +------------------------ src/stacked_borrows/diagnostics.rs | 299 +++++++++++++++++++++++++++++ 5 files changed, 311 insertions(+), 251 deletions(-) create mode 100644 src/stacked_borrows/diagnostics.rs diff --git a/src/diagnostics.rs b/src/diagnostics.rs index bba1989e2d..56cd3bb83d 100644 --- a/src/diagnostics.rs +++ b/src/diagnostics.rs @@ -8,7 +8,7 @@ use rustc_middle::ty; use rustc_span::{source_map::DUMMY_SP, Span, SpanData, Symbol}; use crate::helpers::HexRange; -use crate::stacked_borrows::{AccessKind, SbTag, TagHistory}; +use crate::stacked_borrows::{diagnostics::TagHistory, AccessKind, SbTag}; use crate::*; /// Details of premature program termination. diff --git a/src/eval.rs b/src/eval.rs index 80738d33e0..a3228f763a 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -15,8 +15,8 @@ use rustc_target::spec::abi::Abi; use rustc_session::config::EntryFnType; -use std::collections::HashSet; use rustc_span::DUMMY_SP; +use std::collections::HashSet; use crate::*; diff --git a/src/machine.rs b/src/machine.rs index de9aaa2859..2fb5dca6dd 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -776,6 +776,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> { // 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 { diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index fcb702ea6b..6ffa89087e 100644 --- a/src/stacked_borrows.rs +++ b/src/stacked_borrows.rs @@ -14,14 +14,18 @@ use rustc_middle::ty::{ self, layout::{HasParamEnv, LayoutOf}, }; +use rustc_span::Span; use rustc_span::DUMMY_SP; -use rustc_span::{Span, SpanData}; use rustc_target::abi::Size; use std::collections::HashSet; -use crate::helpers::HexRange; use crate::*; +pub mod diagnostics; +use diagnostics::{AllocHistory, GlobalStateExt, StackExt}; + +use diagnostics::TagHistory; + pub type PtrId = NonZeroU64; pub type CallId = NonZeroU64; pub type AllocExtra = Stacks; @@ -120,47 +124,6 @@ pub struct GlobalStateInner { pub(crate) current_span: Span, } -#[derive(Debug, Default)] -struct AllocHistory { - // The time tags can be compressed down to one bit per event, by just storing a Vec - // where each bit is set to indicate if the event was a creation or a retag - current_time: usize, - creations: smallvec::SmallVec<[Event; 2]>, - invalidations: smallvec::SmallVec<[Event; 1]>, - protectors: smallvec::SmallVec<[Protection; 1]>, -} - -#[derive(Debug)] -struct Protection { - orig_tag: SbTag, - tag: SbTag, - span: Span, -} - -#[derive(Debug)] -struct Event { - time: usize, - parent: Option, - tag: SbTag, - range: AllocRange, - span: Span, -} - -pub enum TagHistory { - Tagged { - tag: SbTag, - created: (AllocRange, SpanData), - invalidated: Option<(AllocRange, SpanData)>, - protected: Option<(SbTag, SpanData, SpanData)>, - }, - Untagged { - recently_created: Option<(AllocRange, SpanData)>, - recently_invalidated: Option<(AllocRange, SpanData)>, - matching_created: Option<(AllocRange, SpanData)>, - protected: Option<(SbTag, SpanData, SpanData)>, - }, -} - /// We need interior mutable access to the global state. pub type GlobalState = RefCell; @@ -269,144 +232,10 @@ impl GlobalStateInner { self.base_ptr_ids.try_insert(id, tag).unwrap(); tag } - - fn add_creation( - &mut self, - parent: Option, - tag: SbTag, - alloc: AllocId, - range: AllocRange, - ) { - let extras = self.extras.entry(alloc).or_default(); - extras.creations.push(Event { - parent, - tag, - range, - span: self.current_span, - time: extras.current_time, - }); - extras.current_time += 1; - } - - fn add_invalidation(&mut self, tag: SbTag, alloc: AllocId, range: AllocRange) { - let extras = self.extras.entry(alloc).or_default(); - extras.invalidations.push(Event { - parent: None, - tag, - range, - span: self.current_span, - time: extras.current_time, - }); - extras.current_time += 1; - } - - fn add_protector(&mut self, orig_tag: SbTag, tag: SbTag, alloc: AllocId) { - let extras = self.extras.entry(alloc).or_default(); - extras.protectors.push(Protection { orig_tag, tag, span: self.current_span }); - extras.current_time += 1; - } - - fn get_stack_history( - &self, - tag: SbTag, - alloc: AllocId, - alloc_range: AllocRange, - offset: Size, - protector_tag: Option, - ) -> Option { - let extras = self.extras.get(&alloc)?; - let protected = protector_tag - .and_then(|protector| { - extras.protectors.iter().find_map(|protection| { - if protection.tag == protector { - Some((protection.orig_tag, protection.span.data())) - } else { - None - } - }) - }) - .and_then(|(tag, call_span)| { - extras.creations.iter().rev().find_map(|event| { - if event.tag == tag { - Some((event.parent?, event.span.data(), call_span)) - } else { - None - } - }) - }); - if let SbTag::Tagged(_) = tag { - let get_matching = |events: &[Event]| { - events.iter().rev().find_map(|event| { - if event.tag == tag { Some((event.range, event.span.data())) } else { None } - }) - }; - Some(TagHistory::Tagged { - tag, - created: get_matching(&extras.creations)?, - invalidated: get_matching(&extras.invalidations), - protected, - }) - } else { - let mut created_time = 0; - // Find the most recently created tag that satsfies this offset - let recently_created = extras.creations.iter().rev().find_map(|event| { - if event.tag == tag && offset >= event.range.start && offset < event.range.end() { - created_time = event.time; - Some((event.range, event.span.data())) - } else { - None - } - }); - - // Find a different recently created tag that satisfies this whole operation, predates - // the recently created tag, and has a different span. - // We're trying to make a guess at which span the user wanted to provide the tag that - // they're using. - let matching_created = if let Some((_created_range, created_span)) = recently_created { - extras.creations.iter().rev().find_map(|event| { - if event.tag == tag - && alloc_range.start >= event.range.start - && alloc_range.end() <= event.range.end() - && event.span.data() != created_span - && event.time != created_time - { - Some((event.range, event.span.data())) - } else { - None - } - }) - } else { - None - }; - - let recently_invalidated = if recently_created.is_some() { - // Find the most recent invalidation of this tag which post-dates the creation - let mut found = None; - for event in extras.invalidations.iter().rev() { - if event.time < created_time { - break; - } - if event.tag == tag && offset >= event.range.start && offset < event.range.end() - { - found = Some((event.range, event.span.data())) - } - } - found - } else { - None - }; - Some(TagHistory::Untagged { - recently_created, - matching_created, - recently_invalidated, - protected, - }) - } - } } /// Error reporting -fn err_sb_ub( +pub fn err_sb_ub( msg: String, help: Option, history: Option, @@ -498,7 +327,7 @@ impl<'tcx> Stack { /// `None` during a deallocation. fn check_protector( item: &Item, - provoking_access: Option<(SbTag, AllocId, AllocRange, Size)>, // just for debug printing amd error messages + provoking_access: Option<(SbTag, AllocId, AllocRange, Size)>, // just for debug printing and error messages global: &GlobalStateInner, ) -> InterpResult<'tcx> { if let SbTag::Tagged(id) = item.tag { @@ -600,7 +429,7 @@ impl<'tcx> Stack { fn dealloc( &mut self, tag: SbTag, - (alloc_id, alloc_range, offset): (AllocId, AllocRange, Size), // just for debug printing amd error messages + (alloc_id, alloc_range, offset): (AllocId, AllocRange, Size), // just for debug printing and error messages global: &GlobalStateInner, ) -> InterpResult<'tcx> { // Step 1: Find granting item. @@ -681,75 +510,6 @@ impl<'tcx> Stack { Ok(()) } - - /// Report a descriptive error when `new` could not be granted from `derived_from`. - fn grant_error( - &self, - derived_from: SbTag, - new: Item, - alloc_id: AllocId, - alloc_range: AllocRange, - error_offset: Size, - global: &GlobalStateInner, - ) -> InterpError<'static> { - let action = format!( - "trying to reborrow {:?} for {:?} permission at {}[{:#x}]", - derived_from, - new.perm, - alloc_id, - error_offset.bytes(), - ); - err_sb_ub( - format!("{}{}", action, self.error_cause(derived_from)), - Some(Self::operation_summary("a reborrow", alloc_id, alloc_range)), - global.get_stack_history(derived_from, alloc_id, alloc_range, error_offset, None), - ) - } - - /// Report a descriptive error when `access` is not permitted based on `tag`. - fn access_error( - &self, - access: AccessKind, - tag: SbTag, - alloc_id: AllocId, - alloc_range: AllocRange, - error_offset: Size, - global: &GlobalStateInner, - ) -> InterpError<'static> { - let action = format!( - "attempting a {} using {:?} at {}[{:#x}]", - access, - tag, - alloc_id, - error_offset.bytes(), - ); - err_sb_ub( - format!("{}{}", action, self.error_cause(tag)), - Some(Self::operation_summary("an access", alloc_id, alloc_range)), - global.get_stack_history(tag, alloc_id, alloc_range, error_offset, None), - ) - } - - fn operation_summary( - operation: &'static str, - alloc_id: AllocId, - alloc_range: AllocRange, - ) -> String { - format!( - "this error occurs as part of {} at {:?}{}", - operation, - alloc_id, - HexRange(alloc_range) - ) - } - - fn error_cause(&self, tag: SbTag) -> &'static str { - if self.borrows.iter().any(|item| item.tag == tag && item.perm != Permission::Disabled) { - ", but that tag only grants SharedReadOnly permission for this location" - } else { - ", but that tag does not exist in the borrow stack for this location" - } - } } // # Stacked Borrows Core End diff --git a/src/stacked_borrows/diagnostics.rs b/src/stacked_borrows/diagnostics.rs new file mode 100644 index 0000000000..d5a65825bc --- /dev/null +++ b/src/stacked_borrows/diagnostics.rs @@ -0,0 +1,299 @@ +use rustc_middle::mir::interpret::{AllocId, AllocRange}; +use rustc_span::{Span, SpanData}; +use rustc_target::abi::Size; + +use crate::helpers::HexRange; +use crate::stacked_borrows::{err_sb_ub, AccessKind, GlobalStateInner, Permission}; +use crate::Item; +use crate::SbTag; +use crate::Stack; + +use rustc_middle::mir::interpret::InterpError; + +#[derive(Debug, Default)] +pub struct AllocHistory { + // The time tags can be compressed down to one bit per event, by just storing a Vec + // where each bit is set to indicate if the event was a creation or a retag + current_time: usize, + creations: smallvec::SmallVec<[Event; 2]>, + invalidations: smallvec::SmallVec<[Event; 1]>, + protectors: smallvec::SmallVec<[Protection; 1]>, +} + +#[derive(Debug)] +struct Protection { + orig_tag: SbTag, + tag: SbTag, + span: Span, +} + +#[derive(Debug)] +struct Event { + time: usize, + parent: Option, + tag: SbTag, + range: AllocRange, + span: Span, +} + +pub enum TagHistory { + Tagged { + tag: SbTag, + created: (AllocRange, SpanData), + invalidated: Option<(AllocRange, SpanData)>, + protected: Option<(SbTag, SpanData, SpanData)>, + }, + Untagged { + recently_created: Option<(AllocRange, SpanData)>, + recently_invalidated: Option<(AllocRange, SpanData)>, + matching_created: Option<(AllocRange, SpanData)>, + protected: Option<(SbTag, SpanData, SpanData)>, + }, +} + +pub trait GlobalStateExt { + fn add_creation( + &mut self, + parent: Option, + tag: SbTag, + alloc: AllocId, + range: AllocRange, + ); + + fn add_invalidation(&mut self, tag: SbTag, alloc: AllocId, range: AllocRange); + + fn add_protector(&mut self, orig_tag: SbTag, tag: SbTag, alloc: AllocId); + + fn get_stack_history( + &self, + tag: SbTag, + alloc: AllocId, + alloc_range: AllocRange, + offset: Size, + protector_tag: Option, + ) -> Option; +} + +impl GlobalStateExt for GlobalStateInner { + fn add_creation( + &mut self, + parent: Option, + tag: SbTag, + alloc: AllocId, + range: AllocRange, + ) { + let extras = self.extras.entry(alloc).or_default(); + extras.creations.push(Event { + parent, + tag, + range, + span: self.current_span, + time: extras.current_time, + }); + extras.current_time += 1; + } + + fn add_invalidation(&mut self, tag: SbTag, alloc: AllocId, range: AllocRange) { + let extras = self.extras.entry(alloc).or_default(); + extras.invalidations.push(Event { + parent: None, + tag, + range, + span: self.current_span, + time: extras.current_time, + }); + extras.current_time += 1; + } + + fn add_protector(&mut self, orig_tag: SbTag, tag: SbTag, alloc: AllocId) { + let extras = self.extras.entry(alloc).or_default(); + extras.protectors.push(Protection { orig_tag, tag, span: self.current_span }); + extras.current_time += 1; + } + + fn get_stack_history( + &self, + tag: SbTag, + alloc: AllocId, + alloc_range: AllocRange, + offset: Size, + protector_tag: Option, + ) -> Option { + let extras = self.extras.get(&alloc)?; + let protected = protector_tag + .and_then(|protector| { + extras.protectors.iter().find_map(|protection| { + if protection.tag == protector { + Some((protection.orig_tag, protection.span.data())) + } else { + None + } + }) + }) + .and_then(|(tag, call_span)| { + extras.creations.iter().rev().find_map(|event| { + if event.tag == tag { + Some((event.parent?, event.span.data(), call_span)) + } else { + None + } + }) + }); + if let SbTag::Tagged(_) = tag { + let get_matching = |events: &[Event]| { + events.iter().rev().find_map(|event| { + if event.tag == tag { Some((event.range, event.span.data())) } else { None } + }) + }; + Some(TagHistory::Tagged { + tag, + created: get_matching(&extras.creations)?, + invalidated: get_matching(&extras.invalidations), + protected, + }) + } else { + let mut created_time = 0; + // Find the most recently created tag that satsfies this offset + let recently_created = extras.creations.iter().rev().find_map(|event| { + if event.tag == tag && offset >= event.range.start && offset < event.range.end() { + created_time = event.time; + Some((event.range, event.span.data())) + } else { + None + } + }); + + // Find a different recently created tag that satisfies this whole operation, predates + // the recently created tag, and has a different span. + // We're trying to make a guess at which span the user wanted to provide the tag that + // they're using. + let matching_created = if let Some((_created_range, created_span)) = recently_created { + extras.creations.iter().rev().find_map(|event| { + if event.tag == tag + && alloc_range.start >= event.range.start + && alloc_range.end() <= event.range.end() + && event.span.data() != created_span + && event.time != created_time + { + Some((event.range, event.span.data())) + } else { + None + } + }) + } else { + None + }; + + let recently_invalidated = if recently_created.is_some() { + // Find the most recent invalidation of this tag which post-dates the creation + let mut found = None; + for event in extras.invalidations.iter().rev() { + if event.time < created_time { + break; + } + if event.tag == tag && offset >= event.range.start && offset < event.range.end() + { + found = Some((event.range, event.span.data())) + } + } + found + } else { + None + }; + Some(TagHistory::Untagged { + recently_created, + matching_created, + recently_invalidated, + protected, + }) + } + } +} + +pub trait StackExt { + fn grant_error( + &self, + derived_from: SbTag, + new: Item, + alloc_id: AllocId, + alloc_range: AllocRange, + error_offset: Size, + global: &GlobalStateInner, + ) -> InterpError<'static>; + + fn access_error( + &self, + access: AccessKind, + tag: SbTag, + alloc_id: AllocId, + alloc_range: AllocRange, + error_offset: Size, + global: &GlobalStateInner, + ) -> InterpError<'static>; +} + +impl StackExt for Stack { + /// Report a descriptive error when `new` could not be granted from `derived_from`. + fn grant_error( + &self, + derived_from: SbTag, + new: Item, + alloc_id: AllocId, + alloc_range: AllocRange, + error_offset: Size, + global: &GlobalStateInner, + ) -> InterpError<'static> { + let action = format!( + "trying to reborrow {:?} for {:?} permission at {}[{:#x}]", + derived_from, + new.perm, + alloc_id, + error_offset.bytes(), + ); + err_sb_ub( + format!("{}{}", action, error_cause(self, derived_from)), + Some(operation_summary("a reborrow", alloc_id, alloc_range)), + global.get_stack_history(derived_from, alloc_id, alloc_range, error_offset, None), + ) + } + + /// Report a descriptive error when `access` is not permitted based on `tag`. + fn access_error( + &self, + access: AccessKind, + tag: SbTag, + alloc_id: AllocId, + alloc_range: AllocRange, + error_offset: Size, + global: &GlobalStateInner, + ) -> InterpError<'static> { + let action = format!( + "attempting a {} using {:?} at {}[{:#x}]", + access, + tag, + alloc_id, + error_offset.bytes(), + ); + err_sb_ub( + format!("{}{}", action, error_cause(self, tag)), + Some(operation_summary("an access", alloc_id, alloc_range)), + global.get_stack_history(tag, alloc_id, alloc_range, error_offset, None), + ) + } +} + +fn operation_summary( + operation: &'static str, + alloc_id: AllocId, + alloc_range: AllocRange, +) -> String { + format!("this error occurs as part of {} at {:?}{}", operation, alloc_id, HexRange(alloc_range)) +} + +fn error_cause(stack: &Stack, tag: SbTag) -> &'static str { + if stack.borrows.iter().any(|item| item.tag == tag && item.perm != Permission::Disabled) { + ", but that tag only grants SharedReadOnly permission for this location" + } else { + ", but that tag does not exist in the borrow stack for this location" + } +} From 68ab457aa04090d52b7b1a91058957afd56ecc52 Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Sat, 30 Apr 2022 13:50:35 -0400 Subject: [PATCH 4/6] Pass AccessKind to check_protector --- src/stacked_borrows.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index 6ffa89087e..fa7d72222e 100644 --- a/src/stacked_borrows.rs +++ b/src/stacked_borrows.rs @@ -327,20 +327,21 @@ impl<'tcx> Stack { /// `None` during a deallocation. fn check_protector( item: &Item, - provoking_access: Option<(SbTag, AllocId, AllocRange, Size)>, // just for debug printing and error messages + provoking_access: Option<(SbTag, AllocId, AllocRange, Size, AccessKind)>, // just for debug printing and error messages global: &GlobalStateInner, ) -> InterpResult<'tcx> { if let SbTag::Tagged(id) = item.tag { if global.tracked_pointer_tags.contains(&id) { register_diagnostic(NonHaltingDiagnostic::PoppedPointerTag( *item, - None, + provoking_access + .map(|(tag, _alloc_id, _alloc_range, _size, access)| (tag, access)), )); } } if let Some(call) = item.protector { if global.is_active(call) { - if let Some((tag, alloc_id, alloc_range, offset)) = provoking_access { + if let Some((tag, alloc_id, alloc_range, offset, _access)) = provoking_access { Err(err_sb_ub( format!( "not granting access to tag {:?} because incompatible item is protected: {:?}", @@ -393,7 +394,11 @@ impl<'tcx> Stack { let first_incompatible_idx = self.find_first_write_incompatible(granting_idx); for item in self.borrows.drain(first_incompatible_idx..).rev() { trace!("access: popping item {:?}", item); - Stack::check_protector(&item, Some((tag, alloc_id, alloc_range, offset)), global)?; + Stack::check_protector( + &item, + Some((tag, alloc_id, alloc_range, offset, access)), + global, + )?; global.add_invalidation(item.tag, alloc_id, alloc_range); } } else { @@ -411,7 +416,7 @@ impl<'tcx> Stack { trace!("access: disabling item {:?}", item); Stack::check_protector( item, - Some((tag, alloc_id, alloc_range, offset)), + Some((tag, alloc_id, alloc_range, offset, access)), global, )?; item.perm = Permission::Disabled; From 972b3b340ad99553618e551937d93bf28e2d2f5c Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Wed, 11 May 2022 19:13:00 -0400 Subject: [PATCH 5/6] 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; } From 8ff0aac06c73d75d5eebea173889fb2ca94c0c75 Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Fri, 13 May 2022 19:04:51 -0400 Subject: [PATCH 6/6] More review feedback * Store the local crates in an Rc<[CrateNum]> * Move all the allocation history into Stacks * Clean up the implementation of get_logs_relevant_to a bit --- src/helpers.rs | 5 +- src/machine.rs | 5 +- src/stacked_borrows.rs | 151 ++++++++++++++-------- src/stacked_borrows/diagnostics.rs | 194 ++++++++++------------------- 4 files changed, 168 insertions(+), 187 deletions(-) diff --git a/src/helpers.rs b/src/helpers.rs index 8d7147fff7..2198bba4fe 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -1,5 +1,6 @@ use std::mem; use std::num::NonZeroUsize; +use std::rc::Rc; use std::time::Duration; use log::trace; @@ -797,7 +798,7 @@ pub fn isolation_abort_error(name: &str) -> InterpResult<'static> { /// Retrieve the list of local crates that should have been passed by cargo-miri in /// MIRI_LOCAL_CRATES and turn them into `CrateNum`s. -pub fn get_local_crates(tcx: &TyCtxt<'_>) -> Vec { +pub fn get_local_crates(tcx: &TyCtxt<'_>) -> Rc<[CrateNum]> { // Convert the local crate names from the passed-in config into CrateNums so that they can // be looked up quickly during execution let local_crate_names = std::env::var("MIRI_LOCAL_CRATES") @@ -811,7 +812,7 @@ pub fn get_local_crates(tcx: &TyCtxt<'_>) -> Vec { local_crates.push(crate_num); } } - local_crates + Rc::from(local_crates.as_slice()) } /// Formats an AllocRange like [0x1..0x3], for use in diagnostics. diff --git a/src/machine.rs b/src/machine.rs index 7cb08066a6..f215f465c0 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -6,6 +6,7 @@ use std::cell::RefCell; use std::collections::HashSet; use std::fmt; use std::num::NonZeroU64; +use std::rc::Rc; use std::time::Instant; use rand::rngs::StdRng; @@ -273,7 +274,7 @@ pub struct Evaluator<'mir, 'tcx> { pub(crate) backtrace_style: BacktraceStyle, /// Crates which are considered local for the purposes of error reporting. - pub(crate) local_crates: Vec, + pub(crate) local_crates: Rc<[CrateNum]>, /// Mapping extern static names to their base pointer. extern_statics: FxHashMap>, @@ -307,7 +308,6 @@ impl<'mir, 'tcx> Evaluator<'mir, 'tcx> { config.tracked_pointer_tags.clone(), config.tracked_call_ids.clone(), config.tag_raw, - local_crates.clone(), ))) } else { None @@ -575,6 +575,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> { stacked_borrows, kind, &ecx.machine.threads, + ecx.machine.local_crates.clone(), )) } else { None diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index 1aec3c0e5e..8dda4a9e22 100644 --- a/src/stacked_borrows.rs +++ b/src/stacked_borrows.rs @@ -3,9 +3,9 @@ use log::trace; use std::cell::RefCell; -use std::collections::HashMap; use std::fmt; use std::num::NonZeroU64; +use std::rc::Rc; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_hir::Mutability; @@ -22,7 +22,7 @@ use std::collections::HashSet; use crate::*; pub mod diagnostics; -use diagnostics::{AllocHistory, GlobalStateExt, StackExt}; +use diagnostics::AllocHistory; use diagnostics::TagHistory; @@ -97,6 +97,8 @@ pub struct Stack { pub struct Stacks { // Even reading memory can have effects on the stack, so we need a `RefCell` here. stacks: RefCell>, + /// Stores past operations on this allocation + history: RefCell, } /// Extra global state, available to the memory access hooks. @@ -118,10 +120,6 @@ 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, } /// We need interior mutable access to the global state. @@ -174,7 +172,6 @@ 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(), @@ -184,8 +181,6 @@ impl GlobalStateInner { tracked_pointer_tags, tracked_call_ids, tag_raw, - local_crates, - extras: HashMap::new(), } } @@ -331,30 +326,29 @@ impl<'tcx> Stack { /// currently checking. fn check_protector( item: &Item, - provoking_access: Option<(SbTag, AllocId, AllocRange, Size, AccessKind)>, // just for debug printing and error messages + provoking_access: Option<(SbTag, AllocRange, Size, AccessKind)>, // just for debug printing and error messages global: &GlobalStateInner, + alloc_history: &mut AllocHistory, ) -> InterpResult<'tcx> { if let SbTag::Tagged(id) = item.tag { if global.tracked_pointer_tags.contains(&id) { register_diagnostic(NonHaltingDiagnostic::PoppedPointerTag( *item, - provoking_access - .map(|(tag, _alloc_id, _alloc_range, _size, access)| (tag, access)), + provoking_access.map(|(tag, _alloc_range, _size, access)| (tag, access)), )); } } if let Some(call) = item.protector { if global.is_active(call) { - if let Some((tag, alloc_id, alloc_range, offset, _access)) = provoking_access { + if let Some((tag, alloc_range, offset, _access)) = provoking_access { Err(err_sb_ub( format!( "not granting access to tag {:?} because incompatible item is protected: {:?}", tag, item ), None, - global.get_stack_history( + alloc_history.get_logs_relevant_to( tag, - alloc_id, alloc_range, offset, Some(item.tag), @@ -383,13 +377,14 @@ impl<'tcx> Stack { (alloc_id, alloc_range, offset): (AllocId, AllocRange, Size), // just for debug printing and error messages global: &mut GlobalStateInner, threads: &ThreadManager<'_, 'tcx>, + alloc_history: &mut AllocHistory, ) -> InterpResult<'tcx> { // Two main steps: Find granting item, remove incompatible items above. // Step 1: Find granting item. - let granting_idx = self - .find_granting(access, tag) - .ok_or_else(|| self.access_error(access, tag, alloc_id, alloc_range, offset, global))?; + let granting_idx = self.find_granting(access, tag).ok_or_else(|| { + alloc_history.access_error(access, tag, alloc_id, alloc_range, offset, self) + })?; // Step 2: Remove incompatible items above them. Make sure we do not remove protected // items. Behavior differs for reads and writes. @@ -401,10 +396,11 @@ impl<'tcx> Stack { trace!("access: popping item {:?}", item); Stack::check_protector( &item, - Some((tag, alloc_id, alloc_range, offset, access)), + Some((tag, alloc_range, offset, access)), global, + alloc_history, )?; - global.log_invalidation(item.tag, alloc_id, alloc_range, threads); + alloc_history.log_invalidation(item.tag, alloc_range, threads); } } else { // On a read, *disable* all `Unique` above the granting item. This ensures U2 for read accesses. @@ -421,11 +417,12 @@ impl<'tcx> Stack { trace!("access: disabling item {:?}", item); Stack::check_protector( item, - Some((tag, alloc_id, alloc_range, offset, access)), + Some((tag, alloc_range, offset, access)), global, + alloc_history, )?; item.perm = Permission::Disabled; - global.log_invalidation(item.tag, alloc_id, alloc_range, threads); + alloc_history.log_invalidation(item.tag, alloc_range, threads); } } } @@ -441,6 +438,7 @@ impl<'tcx> Stack { tag: SbTag, (alloc_id, alloc_range, offset): (AllocId, AllocRange, Size), // just for debug printing and error messages global: &GlobalStateInner, + alloc_history: &mut AllocHistory, ) -> InterpResult<'tcx> { // Step 1: Find granting item. self.find_granting(AccessKind::Write, tag).ok_or_else(|| { @@ -449,13 +447,13 @@ impl<'tcx> Stack { tag, alloc_id, ), None, - global.get_stack_history(tag, alloc_id, alloc_range, offset, None), + alloc_history.get_logs_relevant_to(tag, alloc_range, offset, None), ) })?; // Step 2: Remove all items. Also checks for protectors. for item in self.borrows.drain(..).rev() { - Stack::check_protector(&item, None, global)?; + Stack::check_protector(&item, None, global, alloc_history)?; } Ok(()) @@ -474,6 +472,7 @@ impl<'tcx> Stack { (alloc_id, alloc_range, offset): (AllocId, AllocRange, Size), // just for debug printing and error messages global: &mut GlobalStateInner, threads: &ThreadManager<'_, 'tcx>, + alloc_history: &mut AllocHistory, ) -> InterpResult<'tcx> { // Figure out which access `perm` corresponds to. let access = @@ -481,7 +480,7 @@ impl<'tcx> Stack { // Now we figure out which item grants our parent (`derived_from`) this kind of access. // We use that to determine where to put the new item. let granting_idx = self.find_granting(access, derived_from).ok_or_else(|| { - self.grant_error(derived_from, new, alloc_id, alloc_range, offset, global) + alloc_history.grant_error(derived_from, new, alloc_id, alloc_range, offset, self) })?; // Compute where to put the new item. @@ -501,7 +500,14 @@ 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, threads)?; + self.access( + access, + derived_from, + (alloc_id, alloc_range, offset), + global, + threads, + alloc_history, + )?; // 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 @@ -527,22 +533,26 @@ impl<'tcx> Stack { /// Map per-stack operations to higher-level per-location-range operations. impl<'tcx> Stacks { /// Creates new stack with initial tag. - fn new(size: Size, perm: Permission, tag: SbTag) -> Self { + fn new(size: Size, perm: Permission, tag: SbTag, local_crates: Rc<[CrateNum]>) -> Self { let item = Item { perm, tag, protector: None }; let stack = Stack { borrows: vec![item] }; - Stacks { stacks: RefCell::new(RangeMap::new(size, stack)) } + Stacks { + stacks: RefCell::new(RangeMap::new(size, stack)), + history: RefCell::new(AllocHistory::new(local_crates)), + } } /// Call `f` on every stack in the range. fn for_each( &self, range: AllocRange, - mut f: impl FnMut(Size, &mut Stack) -> InterpResult<'tcx>, + mut f: impl FnMut(Size, &mut Stack, &mut AllocHistory) -> InterpResult<'tcx>, ) -> InterpResult<'tcx> { let mut stacks = self.stacks.borrow_mut(); + let history = &mut *self.history.borrow_mut(); for (offset, stack) in stacks.iter_mut(range.start, range.size) { - f(offset, stack)?; + f(offset, stack, history)?; } Ok(()) } @@ -551,11 +561,12 @@ impl<'tcx> Stacks { fn for_each_mut( &mut self, range: AllocRange, - mut f: impl FnMut(Size, &mut Stack) -> InterpResult<'tcx>, + mut f: impl FnMut(Size, &mut Stack, &mut AllocHistory) -> InterpResult<'tcx>, ) -> InterpResult<'tcx> { let stacks = self.stacks.get_mut(); + let history = &mut *self.history.borrow_mut(); for (offset, stack) in stacks.iter_mut(range.start, range.size) { - f(offset, stack)?; + f(offset, stack, history)?; } Ok(()) } @@ -569,6 +580,7 @@ impl Stacks { state: &GlobalState, kind: MemoryKind, threads: &ThreadManager<'_, '_>, + local_crates: Rc<[CrateNum]>, ) -> Self { let mut extra = state.borrow_mut(); let (base_tag, perm) = match kind { @@ -602,8 +614,14 @@ impl Stacks { (tag, Permission::SharedReadWrite) } }; - extra.log_creation(None, base_tag, id, alloc_range(Size::ZERO, size), threads); - Stacks::new(size, perm, base_tag) + let stacks = Stacks::new(size, perm, base_tag, local_crates); + stacks.history.borrow_mut().log_creation( + None, + base_tag, + alloc_range(Size::ZERO, size), + threads, + ); + stacks } #[inline(always)] @@ -622,8 +640,15 @@ impl Stacks { range.size.bytes() ); let mut state = state.borrow_mut(); - self.for_each(range, |offset, stack| { - stack.access(AccessKind::Read, tag, (alloc_id, range, offset), &mut state, threads) + self.for_each(range, |offset, stack, history| { + stack.access( + AccessKind::Read, + tag, + (alloc_id, range, offset), + &mut state, + threads, + history, + ) }) } @@ -643,8 +668,15 @@ impl Stacks { range.size.bytes() ); 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) + self.for_each_mut(range, |offset, stack, history| { + stack.access( + AccessKind::Write, + tag, + (alloc_id, range, offset), + &mut state, + threads, + history, + ) }) } @@ -658,10 +690,9 @@ impl Stacks { ) -> InterpResult<'tcx> { trace!("deallocation with tag {:?}: {:?}, size {}", tag, alloc_id, range.size.bytes()); let mut state = state.borrow_mut(); - self.for_each_mut(range, |offset, stack| { - stack.dealloc(tag, (alloc_id, range, offset), &mut state) + self.for_each_mut(range, |offset, stack, history| { + stack.dealloc(tag, (alloc_id, range, offset), &mut state, history) })?; - state.extras.remove(&alloc_id); Ok(()) } } @@ -692,16 +723,20 @@ 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.log_creation( - Some(orig_tag), - new_tag, - alloc_id, - alloc_range(base_offset, base_offset + size), - &this.machine.threads, - ); - if protect { - mem_extra.log_protector(orig_tag, new_tag, alloc_id, &this.machine.threads); + { + let extra = this.get_alloc_extra(alloc_id)?; + let stacked_borrows = + extra.stacked_borrows.as_ref().expect("we should have Stacked Borrows data"); + let mut alloc_history = stacked_borrows.history.borrow_mut(); + alloc_history.log_creation( + Some(orig_tag), + new_tag, + alloc_range(base_offset, base_offset + size), + &this.machine.threads, + ); + if protect { + alloc_history.log_protector(orig_tag, new_tag, &this.machine.threads); + } } // Ensure we bail out if the pointer goes out-of-bounds (see miri#1050). @@ -763,13 +798,14 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx }; 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| { + stacked_borrows.for_each(range, |offset, stack, history| { stack.grant( orig_tag, item, (alloc_id, range, offset), &mut *global, &this.machine.threads, + history, ) }) })?; @@ -785,8 +821,15 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx 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| { - stack.grant(orig_tag, item, (alloc_id, range, offset), &mut global, &machine.threads) + stacked_borrows.for_each_mut(range, |offset, stack, history| { + stack.grant( + orig_tag, + item, + (alloc_id, range, offset), + &mut global, + &machine.threads, + history, + ) })?; Ok(()) diff --git a/src/stacked_borrows/diagnostics.rs b/src/stacked_borrows/diagnostics.rs index f657a926c0..734c3a14e3 100644 --- a/src/stacked_borrows/diagnostics.rs +++ b/src/stacked_borrows/diagnostics.rs @@ -1,9 +1,13 @@ +use smallvec::SmallVec; +use std::rc::Rc; + use rustc_middle::mir::interpret::{AllocId, AllocRange}; +use rustc_span::def_id::CrateNum; use rustc_span::{Span, SpanData}; use rustc_target::abi::Size; use crate::helpers::HexRange; -use crate::stacked_borrows::{err_sb_ub, AccessKind, GlobalStateInner, Permission}; +use crate::stacked_borrows::{err_sb_ub, AccessKind, Permission}; use crate::Item; use crate::SbTag; use crate::Stack; @@ -11,7 +15,7 @@ use crate::ThreadManager; use rustc_middle::mir::interpret::InterpError; -#[derive(Debug, Default)] +#[derive(Clone, Debug)] pub struct AllocHistory { // The time tags can be compressed down to one bit per event, by just storing a Vec // where each bit is set to indicate if the event was a creation or a retag @@ -19,16 +23,18 @@ pub struct AllocHistory { creations: smallvec::SmallVec<[Event; 2]>, invalidations: smallvec::SmallVec<[Event; 1]>, protectors: smallvec::SmallVec<[Protection; 1]>, + /// This field is a clone of the `local_crates` field on `Evaluator`. + local_crates: Rc<[CrateNum]>, } -#[derive(Debug)] +#[derive(Clone, Debug)] struct Protection { orig_tag: SbTag, tag: SbTag, span: Span, } -#[derive(Debug)] +#[derive(Clone, Debug)] struct Event { time: usize, parent: Option, @@ -52,45 +58,17 @@ pub enum TagHistory { }, } -pub trait GlobalStateExt { - fn current_span(&self, threads: &ThreadManager<'_, '_>) -> Span; - - fn log_creation( - &mut self, - parent: Option, - tag: SbTag, - alloc: AllocId, - range: AllocRange, - threads: &ThreadManager<'_, '_>, - ); - - fn log_invalidation( - &mut self, - tag: SbTag, - alloc: AllocId, - range: AllocRange, - threads: &ThreadManager<'_, '_>, - ); - - fn log_protector( - &mut self, - orig_tag: SbTag, - tag: SbTag, - alloc: AllocId, - threads: &ThreadManager<'_, '_>, - ); - - fn get_stack_history( - &self, - tag: SbTag, - alloc: AllocId, - alloc_range: AllocRange, - offset: Size, - protector_tag: Option, - ) -> Option; -} +impl AllocHistory { + pub fn new(local_crates: Rc<[CrateNum]>) -> Self { + Self { + current_time: 0, + creations: SmallVec::new(), + invalidations: SmallVec::new(), + protectors: SmallVec::new(), + local_crates, + } + } -impl GlobalStateExt for GlobalStateInner { fn current_span(&self, threads: &ThreadManager<'_, '_>) -> Span { threads .active_thread_stack() @@ -104,64 +82,45 @@ impl GlobalStateExt for GlobalStateInner { .unwrap_or(rustc_span::DUMMY_SP) } - fn log_creation( + pub 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, time: extras.current_time }); - extras.current_time += 1; + self.creations.push(Event { parent, tag, range, span, time: self.current_time }); + self.current_time += 1; } - fn log_invalidation( + pub 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, - time: extras.current_time, - }); - extras.current_time += 1; + self.invalidations.push(Event { parent: None, tag, range, span, time: self.current_time }); + self.current_time += 1; } - fn log_protector( - &mut self, - orig_tag: SbTag, - tag: SbTag, - alloc: AllocId, - threads: &ThreadManager<'_, '_>, - ) { + pub fn log_protector(&mut self, orig_tag: SbTag, tag: SbTag, threads: &ThreadManager<'_, '_>) { let span = self.current_span(threads); - let extras = self.extras.entry(alloc).or_default(); - extras.protectors.push(Protection { orig_tag, tag, span }); - extras.current_time += 1; + self.protectors.push(Protection { orig_tag, tag, span }); + self.current_time += 1; } - fn get_stack_history( + pub fn get_logs_relevant_to( &self, tag: SbTag, - alloc: AllocId, alloc_range: AllocRange, offset: Size, protector_tag: Option, ) -> Option { - let extras = self.extras.get(&alloc)?; let protected = protector_tag .and_then(|protector| { - extras.protectors.iter().find_map(|protection| { + self.protectors.iter().find_map(|protection| { if protection.tag == protector { Some((protection.orig_tag, protection.span.data())) } else { @@ -170,7 +129,7 @@ impl GlobalStateExt for GlobalStateInner { }) }) .and_then(|(tag, call_span)| { - extras.creations.iter().rev().find_map(|event| { + self.creations.iter().rev().find_map(|event| { if event.tag == tag { Some((event.parent?, event.span.data(), call_span)) } else { @@ -178,6 +137,7 @@ impl GlobalStateExt for GlobalStateInner { } }) }); + if let SbTag::Tagged(_) = tag { let get_matching = |events: &[Event]| { events.iter().rev().find_map(|event| { @@ -186,14 +146,14 @@ impl GlobalStateExt for GlobalStateInner { }; Some(TagHistory::Tagged { tag, - created: get_matching(&extras.creations)?, - invalidated: get_matching(&extras.invalidations), + created: get_matching(&self.creations)?, + invalidated: get_matching(&self.invalidations), protected, }) } else { let mut created_time = 0; // Find the most recently created tag that satsfies this offset - let recently_created = extras.creations.iter().rev().find_map(|event| { + let recently_created = self.creations.iter().rev().find_map(|event| { if event.tag == tag && offset >= event.range.start && offset < event.range.end() { created_time = event.time; Some((event.range, event.span.data())) @@ -206,8 +166,8 @@ impl GlobalStateExt for GlobalStateInner { // the recently created tag, and has a different span. // We're trying to make a guess at which span the user wanted to provide the tag that // they're using. - let matching_created = if let Some((_created_range, created_span)) = recently_created { - extras.creations.iter().rev().find_map(|event| { + let matching_created = recently_created.and_then(|(_created_range, created_span)| { + self.creations.iter().rev().find_map(|event| { if event.tag == tag && alloc_range.start >= event.range.start && alloc_range.end() <= event.range.end() @@ -219,26 +179,26 @@ impl GlobalStateExt for GlobalStateInner { None } }) - } else { - None - }; + }); + + // Find the most recent invalidation of this tag which post-dates the creation + let recently_invalidated = recently_created.and_then(|_| { + self.invalidations + .iter() + .rev() + .take_while(|event| event.time > created_time) + .find_map(|event| { + if event.tag == tag + && offset >= event.range.start + && offset < event.range.end() + { + Some((event.range, event.span.data())) + } else { + None + } + }) + }); - let recently_invalidated = if recently_created.is_some() { - // Find the most recent invalidation of this tag which post-dates the creation - let mut found = None; - for event in extras.invalidations.iter().rev() { - if event.time < created_time { - break; - } - if event.tag == tag && offset >= event.range.start && offset < event.range.end() - { - found = Some((event.range, event.span.data())) - } - } - found - } else { - None - }; Some(TagHistory::Untagged { recently_created, matching_created, @@ -247,40 +207,16 @@ impl GlobalStateExt for GlobalStateInner { }) } } -} - -pub trait StackExt { - fn grant_error( - &self, - derived_from: SbTag, - new: Item, - alloc_id: AllocId, - alloc_range: AllocRange, - error_offset: Size, - global: &GlobalStateInner, - ) -> InterpError<'static>; - - fn access_error( - &self, - access: AccessKind, - tag: SbTag, - alloc_id: AllocId, - alloc_range: AllocRange, - error_offset: Size, - global: &GlobalStateInner, - ) -> InterpError<'static>; -} -impl StackExt for Stack { /// Report a descriptive error when `new` could not be granted from `derived_from`. - fn grant_error( + pub fn grant_error( &self, derived_from: SbTag, new: Item, alloc_id: AllocId, alloc_range: AllocRange, error_offset: Size, - global: &GlobalStateInner, + stack: &Stack, ) -> InterpError<'static> { let action = format!( "trying to reborrow {:?} for {:?} permission at {}[{:#x}]", @@ -290,21 +226,21 @@ impl StackExt for Stack { error_offset.bytes(), ); err_sb_ub( - format!("{}{}", action, error_cause(self, derived_from)), + format!("{}{}", action, error_cause(stack, derived_from)), Some(operation_summary("a reborrow", alloc_id, alloc_range)), - global.get_stack_history(derived_from, alloc_id, alloc_range, error_offset, None), + self.get_logs_relevant_to(derived_from, alloc_range, error_offset, None), ) } /// Report a descriptive error when `access` is not permitted based on `tag`. - fn access_error( + pub fn access_error( &self, access: AccessKind, tag: SbTag, alloc_id: AllocId, alloc_range: AllocRange, error_offset: Size, - global: &GlobalStateInner, + stack: &Stack, ) -> InterpError<'static> { let action = format!( "attempting a {} using {:?} at {}[{:#x}]", @@ -314,9 +250,9 @@ impl StackExt for Stack { error_offset.bytes(), ); err_sb_ub( - format!("{}{}", action, error_cause(self, tag)), + format!("{}{}", action, error_cause(stack, tag)), Some(operation_summary("an access", alloc_id, alloc_range)), - global.get_stack_history(tag, alloc_id, alloc_range, error_offset, None), + self.get_logs_relevant_to(tag, alloc_range, error_offset, None), ) } }