From 6e29b7b247a4f3e82396cd9255eda894898904da Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Tue, 3 Oct 2023 21:04:42 -0400 Subject: [PATCH 01/14] Extend the BorTag GC to AllocIds --- src/borrow_tracker/mod.rs | 18 +- src/borrow_tracker/stacked_borrows/mod.rs | 6 +- src/borrow_tracker/tree_borrows/tree.rs | 6 +- src/concurrency/data_race.rs | 10 +- src/concurrency/init_once.rs | 6 +- src/concurrency/sync.rs | 6 +- src/concurrency/thread.rs | 38 ++--- src/concurrency/weak_memory.rs | 6 +- src/intptrcast.rs | 24 ++- src/lib.rs | 4 +- src/machine.rs | 50 +++--- src/provenance_gc.rs | 193 ++++++++++++++++++++++ src/shims/env.rs | 8 +- src/shims/foreign_items.rs | 4 + src/shims/panic.rs | 10 +- src/shims/time.rs | 4 +- src/shims/tls.rs | 8 +- src/shims/unix/fs.rs | 10 +- src/shims/unix/linux/sync.rs | 6 +- src/shims/unix/sync.rs | 6 +- src/shims/windows/sync.rs | 18 +- src/tag_gc.rs | 169 ------------------- tests/pass/ptr_int_casts.rs | 18 ++ tests/utils/miri_extern.rs | 8 +- 24 files changed, 352 insertions(+), 284 deletions(-) create mode 100644 src/provenance_gc.rs delete mode 100644 src/tag_gc.rs diff --git a/src/borrow_tracker/mod.rs b/src/borrow_tracker/mod.rs index f9cc3acbd5..5a7e245f54 100644 --- a/src/borrow_tracker/mod.rs +++ b/src/borrow_tracker/mod.rs @@ -75,8 +75,8 @@ pub struct FrameState { protected_tags: SmallVec<[(AllocId, BorTag); 2]>, } -impl VisitTags for FrameState { - fn visit_tags(&self, _visit: &mut dyn FnMut(BorTag)) { +impl VisitProvenance for FrameState { + fn visit_provenance(&self, _visit: &mut VisitWith<'_>) { // `protected_tags` are already recorded by `GlobalStateInner`. } } @@ -110,10 +110,10 @@ pub struct GlobalStateInner { unique_is_unique: bool, } -impl VisitTags for GlobalStateInner { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { +impl VisitProvenance for GlobalStateInner { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { for &tag in self.protected_tags.keys() { - visit(tag); + visit(None, Some(tag)); } // The only other candidate is base_ptr_tags, and that does not need visiting since we don't ever // GC the bottommost/root tag. @@ -503,11 +503,11 @@ impl AllocState { } } -impl VisitTags for AllocState { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { +impl VisitProvenance for AllocState { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { match self { - AllocState::StackedBorrows(sb) => sb.visit_tags(visit), - AllocState::TreeBorrows(tb) => tb.visit_tags(visit), + AllocState::StackedBorrows(sb) => sb.visit_provenance(visit), + AllocState::TreeBorrows(tb) => tb.visit_provenance(visit), } } } diff --git a/src/borrow_tracker/stacked_borrows/mod.rs b/src/borrow_tracker/stacked_borrows/mod.rs index a74c69d52f..91d924976f 100644 --- a/src/borrow_tracker/stacked_borrows/mod.rs +++ b/src/borrow_tracker/stacked_borrows/mod.rs @@ -462,10 +462,10 @@ impl Stacks { } } -impl VisitTags for Stacks { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { +impl VisitProvenance for Stacks { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { for tag in self.exposed_tags.iter().copied() { - visit(tag); + visit(None, Some(tag)); } } } diff --git a/src/borrow_tracker/tree_borrows/tree.rs b/src/borrow_tracker/tree_borrows/tree.rs index 4232cd396c..6801397b2c 100644 --- a/src/borrow_tracker/tree_borrows/tree.rs +++ b/src/borrow_tracker/tree_borrows/tree.rs @@ -742,11 +742,11 @@ impl Tree { } } -impl VisitTags for Tree { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { +impl VisitProvenance for Tree { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { // To ensure that the root never gets removed, we visit it // (the `root` node of `Tree` is not an `Option<_>`) - visit(self.nodes.get(self.root).unwrap().tag) + visit(None, Some(self.nodes.get(self.root).unwrap().tag)) } } diff --git a/src/concurrency/data_race.rs b/src/concurrency/data_race.rs index 5d109a7d55..80d0402fc8 100644 --- a/src/concurrency/data_race.rs +++ b/src/concurrency/data_race.rs @@ -790,9 +790,9 @@ pub struct VClockAlloc { alloc_ranges: RefCell>, } -impl VisitTags for VClockAlloc { - fn visit_tags(&self, _visit: &mut dyn FnMut(BorTag)) { - // No tags here. +impl VisitProvenance for VClockAlloc { + fn visit_provenance(&self, _visit: &mut VisitWith<'_>) { + // No tags or allocIds here. } } @@ -1404,8 +1404,8 @@ pub struct GlobalState { pub track_outdated_loads: bool, } -impl VisitTags for GlobalState { - fn visit_tags(&self, _visit: &mut dyn FnMut(BorTag)) { +impl VisitProvenance for GlobalState { + fn visit_provenance(&self, _visit: &mut VisitWith<'_>) { // We don't have any tags. } } diff --git a/src/concurrency/init_once.rs b/src/concurrency/init_once.rs index 71582c75ea..9a848d5034 100644 --- a/src/concurrency/init_once.rs +++ b/src/concurrency/init_once.rs @@ -45,10 +45,10 @@ pub(super) struct InitOnce<'mir, 'tcx> { data_race: VClock, } -impl<'mir, 'tcx> VisitTags for InitOnce<'mir, 'tcx> { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { +impl<'mir, 'tcx> VisitProvenance for InitOnce<'mir, 'tcx> { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { for waiter in self.waiters.iter() { - waiter.callback.visit_tags(visit); + waiter.callback.visit_provenance(visit); } } } diff --git a/src/concurrency/sync.rs b/src/concurrency/sync.rs index 62f6d57ef3..b288b69e0c 100644 --- a/src/concurrency/sync.rs +++ b/src/concurrency/sync.rs @@ -181,10 +181,10 @@ pub(crate) struct SynchronizationState<'mir, 'tcx> { pub(super) init_onces: IndexVec>, } -impl<'mir, 'tcx> VisitTags for SynchronizationState<'mir, 'tcx> { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { +impl<'mir, 'tcx> VisitProvenance for SynchronizationState<'mir, 'tcx> { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { for init_once in self.init_onces.iter() { - init_once.visit_tags(visit); + init_once.visit_provenance(visit); } } } diff --git a/src/concurrency/thread.rs b/src/concurrency/thread.rs index 6449ed29cf..754cfa4d2a 100644 --- a/src/concurrency/thread.rs +++ b/src/concurrency/thread.rs @@ -43,7 +43,7 @@ pub enum TlsAllocAction { } /// Trait for callbacks that can be executed when some event happens, such as after a timeout. -pub trait MachineCallback<'mir, 'tcx>: VisitTags { +pub trait MachineCallback<'mir, 'tcx>: VisitProvenance { fn call(&self, ecx: &mut InterpCx<'mir, 'tcx, MiriMachine<'mir, 'tcx>>) -> InterpResult<'tcx>; } @@ -228,8 +228,8 @@ impl<'mir, 'tcx> Thread<'mir, 'tcx> { } } -impl VisitTags for Thread<'_, '_> { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { +impl VisitProvenance for Thread<'_, '_> { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { let Thread { panic_payloads: panic_payload, last_error, @@ -242,17 +242,17 @@ impl VisitTags for Thread<'_, '_> { } = self; for payload in panic_payload { - payload.visit_tags(visit); + payload.visit_provenance(visit); } - last_error.visit_tags(visit); + last_error.visit_provenance(visit); for frame in stack { - frame.visit_tags(visit) + frame.visit_provenance(visit) } } } -impl VisitTags for Frame<'_, '_, Provenance, FrameExtra<'_>> { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { +impl VisitProvenance for Frame<'_, '_, Provenance, FrameExtra<'_>> { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { let Frame { return_place, locals, @@ -266,22 +266,22 @@ impl VisitTags for Frame<'_, '_, Provenance, FrameExtra<'_>> { } = self; // Return place. - return_place.visit_tags(visit); + return_place.visit_provenance(visit); // Locals. for local in locals.iter() { match local.as_mplace_or_imm() { None => {} Some(Either::Left((ptr, meta))) => { - ptr.visit_tags(visit); - meta.visit_tags(visit); + ptr.visit_provenance(visit); + meta.visit_provenance(visit); } Some(Either::Right(imm)) => { - imm.visit_tags(visit); + imm.visit_provenance(visit); } } } - extra.visit_tags(visit); + extra.visit_provenance(visit); } } @@ -341,8 +341,8 @@ pub struct ThreadManager<'mir, 'tcx> { timeout_callbacks: FxHashMap>, } -impl VisitTags for ThreadManager<'_, '_> { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { +impl VisitProvenance for ThreadManager<'_, '_> { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { let ThreadManager { threads, thread_local_alloc_ids, @@ -353,15 +353,15 @@ impl VisitTags for ThreadManager<'_, '_> { } = self; for thread in threads { - thread.visit_tags(visit); + thread.visit_provenance(visit); } for ptr in thread_local_alloc_ids.borrow().values() { - ptr.visit_tags(visit); + ptr.visit_provenance(visit); } for callback in timeout_callbacks.values() { - callback.callback.visit_tags(visit); + callback.callback.visit_provenance(visit); } - sync.visit_tags(visit); + sync.visit_provenance(visit); } } diff --git a/src/concurrency/weak_memory.rs b/src/concurrency/weak_memory.rs index 2ff344bb1a..39e89ce7fa 100644 --- a/src/concurrency/weak_memory.rs +++ b/src/concurrency/weak_memory.rs @@ -108,15 +108,15 @@ pub struct StoreBufferAlloc { store_buffers: RefCell>, } -impl VisitTags for StoreBufferAlloc { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { +impl VisitProvenance for StoreBufferAlloc { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { let Self { store_buffers } = self; for val in store_buffers .borrow() .iter() .flat_map(|buf| buf.buffer.iter().map(|element| &element.val)) { - val.visit_tags(visit); + val.visit_provenance(visit); } } } diff --git a/src/intptrcast.rs b/src/intptrcast.rs index 9966ee3fd9..76a3b9a030 100644 --- a/src/intptrcast.rs +++ b/src/intptrcast.rs @@ -46,9 +46,23 @@ pub struct GlobalStateInner { provenance_mode: ProvenanceMode, } -impl VisitTags for GlobalStateInner { - fn visit_tags(&self, _visit: &mut dyn FnMut(BorTag)) { - // Nothing to visit here. +impl VisitProvenance for GlobalStateInner { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { + let GlobalStateInner { + int_to_ptr_map: _, + base_addr: _, + exposed, + next_base_addr: _, + provenance_mode: _, + } = self; + // Though base_addr and int_to_ptr_map contain AllocIds, we do not want to visit them. + // We're trying to remove unused elements from base_addr, so visiting it would prevent + // removing anything. int_to_ptr_map is managed by free_alloc_id, and entries in it do not + // actually make allocation base addresses reachable so we don't need to visit it. + // But exposed tags do make base addresses reachable. + for id in exposed { + id.visit_provenance(visit) + } } } @@ -62,6 +76,10 @@ impl GlobalStateInner { provenance_mode: config.provenance_mode, } } + + pub fn remove_unreachable_allocs(&mut self, reachable_allocs: &FxHashSet) { + self.base_addr.retain(|id, _| reachable_allocs.contains(id)); + } } /// Shifts `addr` to make it aligned with `align` by rounding `addr` to the smallest multiple diff --git a/src/lib.rs b/src/lib.rs index b12aae6d41..715d3d9587 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -77,9 +77,9 @@ mod intptrcast; mod machine; mod mono_hash_map; mod operator; +mod provenance_gc; mod range_map; mod shims; -mod tag_gc; // Establish a "crate-wide prelude": we often import `crate::*`. @@ -125,8 +125,8 @@ pub use crate::machine::{ }; pub use crate::mono_hash_map::MonoHashMap; pub use crate::operator::EvalContextExt as _; +pub use crate::provenance_gc::{EvalContextExt as _, VisitProvenance, VisitWith}; pub use crate::range_map::RangeMap; -pub use crate::tag_gc::{EvalContextExt as _, VisitTags}; /// Insert rustc arguments at the beginning of the argument list that Miri wants to be /// set per default, for maximal validation power. diff --git a/src/machine.rs b/src/machine.rs index 2085df7d06..66d7dfcf3a 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -77,12 +77,12 @@ impl<'tcx> std::fmt::Debug for FrameExtra<'tcx> { } } -impl VisitTags for FrameExtra<'_> { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { +impl VisitProvenance for FrameExtra<'_> { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { let FrameExtra { catch_unwind, borrow_tracker, timing: _, is_user_relevant: _ } = self; - catch_unwind.visit_tags(visit); - borrow_tracker.visit_tags(visit); + catch_unwind.visit_provenance(visit); + borrow_tracker.visit_provenance(visit); } } @@ -311,13 +311,13 @@ pub struct AllocExtra<'tcx> { pub backtrace: Option>>, } -impl VisitTags for AllocExtra<'_> { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { +impl VisitProvenance for AllocExtra<'_> { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { let AllocExtra { borrow_tracker, data_race, weak_memory, backtrace: _ } = self; - borrow_tracker.visit_tags(visit); - data_race.visit_tags(visit); - weak_memory.visit_tags(visit); + borrow_tracker.visit_provenance(visit); + data_race.visit_provenance(visit); + weak_memory.visit_provenance(visit); } } @@ -793,8 +793,8 @@ impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> { } } -impl VisitTags for MiriMachine<'_, '_> { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { +impl VisitProvenance for MiriMachine<'_, '_> { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { #[rustfmt::skip] let MiriMachine { threads, @@ -843,20 +843,20 @@ impl VisitTags for MiriMachine<'_, '_> { allocation_spans: _, } = self; - threads.visit_tags(visit); - tls.visit_tags(visit); - env_vars.visit_tags(visit); - dir_handler.visit_tags(visit); - file_handler.visit_tags(visit); - data_race.visit_tags(visit); - borrow_tracker.visit_tags(visit); - intptrcast.visit_tags(visit); - main_fn_ret_place.visit_tags(visit); - argc.visit_tags(visit); - argv.visit_tags(visit); - cmd_line.visit_tags(visit); + threads.visit_provenance(visit); + tls.visit_provenance(visit); + env_vars.visit_provenance(visit); + dir_handler.visit_provenance(visit); + file_handler.visit_provenance(visit); + data_race.visit_provenance(visit); + borrow_tracker.visit_provenance(visit); + intptrcast.visit_provenance(visit); + main_fn_ret_place.visit_provenance(visit); + argc.visit_provenance(visit); + argv.visit_provenance(visit); + cmd_line.visit_provenance(visit); for ptr in extern_statics.values() { - ptr.visit_tags(visit); + ptr.visit_provenance(visit); } } } @@ -1380,7 +1380,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { // where it mistakenly removes an important tag become visible. if ecx.machine.gc_interval > 0 && ecx.machine.since_gc >= ecx.machine.gc_interval { ecx.machine.since_gc = 0; - ecx.garbage_collect_tags()?; + ecx.run_provenance_gc(); } // These are our preemption points. diff --git a/src/provenance_gc.rs b/src/provenance_gc.rs new file mode 100644 index 0000000000..e76ae70ca2 --- /dev/null +++ b/src/provenance_gc.rs @@ -0,0 +1,193 @@ +use either::Either; + +use rustc_data_structures::fx::FxHashSet; + +use crate::*; + +pub type VisitWith<'a> = dyn FnMut(Option, Option) + 'a; + +pub trait VisitProvenance { + fn visit_provenance(&self, visit: &mut VisitWith<'_>); +} + +impl VisitProvenance for Option { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { + if let Some(x) = self { + x.visit_provenance(visit); + } + } +} + +impl VisitProvenance for std::cell::RefCell { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { + self.borrow().visit_provenance(visit) + } +} + +impl VisitProvenance for BorTag { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { + visit(None, Some(*self)) + } +} + +impl VisitProvenance for AllocId { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { + visit(Some(*self), None) + } +} + +impl VisitProvenance for Provenance { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { + if let Provenance::Concrete { alloc_id, tag, .. } = self { + visit(Some(*alloc_id), Some(*tag)); + } + } +} + +impl VisitProvenance for Pointer { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { + let (prov, _offset) = self.into_parts(); + prov.visit_provenance(visit); + } +} + +impl VisitProvenance for Pointer> { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { + let (prov, _offset) = self.into_parts(); + prov.visit_provenance(visit); + } +} + +impl VisitProvenance for Scalar { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { + match self { + Scalar::Ptr(ptr, _) => ptr.visit_provenance(visit), + Scalar::Int(_) => (), + } + } +} + +impl VisitProvenance for Immediate { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { + match self { + Immediate::Scalar(s) => { + s.visit_provenance(visit); + } + Immediate::ScalarPair(s1, s2) => { + s1.visit_provenance(visit); + s2.visit_provenance(visit); + } + Immediate::Uninit => {} + } + } +} + +impl VisitProvenance for MemPlaceMeta { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { + match self { + MemPlaceMeta::Meta(m) => m.visit_provenance(visit), + MemPlaceMeta::None => {} + } + } +} + +impl VisitProvenance for ImmTy<'_, Provenance> { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { + (**self).visit_provenance(visit) + } +} + +impl VisitProvenance for MPlaceTy<'_, Provenance> { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { + self.ptr().visit_provenance(visit); + self.meta().visit_provenance(visit); + } +} + +impl VisitProvenance for PlaceTy<'_, Provenance> { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { + match self.as_mplace_or_local() { + Either::Left(mplace) => mplace.visit_provenance(visit), + Either::Right(_) => (), + } + } +} + +impl VisitProvenance for OpTy<'_, Provenance> { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { + match self.as_mplace_or_imm() { + Either::Left(mplace) => mplace.visit_provenance(visit), + Either::Right(imm) => imm.visit_provenance(visit), + } + } +} + +impl VisitProvenance for Allocation> { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { + for prov in self.provenance().provenances() { + prov.visit_provenance(visit); + } + + self.extra.visit_provenance(visit); + } +} + +impl VisitProvenance for crate::MiriInterpCx<'_, '_> { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { + // Memory. + self.memory.alloc_map().iter(|it| { + for (id, (_kind, alloc)) in it { + id.visit_provenance(visit); + alloc.visit_provenance(visit); + } + }); + + // And all the other machine values. + self.machine.visit_provenance(visit); + } +} + +impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriInterpCx<'mir, 'tcx> {} +pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> { + fn run_provenance_gc(&mut self) { + let this = self.eval_context_mut(); + + let mut tags = FxHashSet::default(); + + // Initially populate the set of reachable AllocIds with all live allocations. This ensures + // that we do not remove state associated with leaked allocations. + let mut alloc_ids = FxHashSet::default(); + this.memory.alloc_map().iter(|it| { + for (id, _) in it { + alloc_ids.insert(*id); + } + }); + this.visit_provenance(&mut |id, tag| { + if let Some(id) = id { + alloc_ids.insert(id); + } + if let Some(tag) = tag { + tags.insert(tag); + } + }); + self.remove_unreachable_tags(tags); + self.remove_unreachable_allocs(alloc_ids); + } + + fn remove_unreachable_tags(&mut self, tags: FxHashSet) { + let this = self.eval_context_mut(); + this.memory.alloc_map().iter(|it| { + for (_id, (_kind, alloc)) in it { + if let Some(bt) = &alloc.extra.borrow_tracker { + bt.remove_unreachable_tags(&tags); + } + } + }); + } + + fn remove_unreachable_allocs(&mut self, allocs: FxHashSet) { + let this = self.eval_context_mut(); + this.machine.allocation_spans.borrow_mut().retain(|id, _| allocs.contains(id)); + this.machine.intptrcast.borrow_mut().remove_unreachable_allocs(&allocs); + } +} diff --git a/src/shims/env.rs b/src/shims/env.rs index 154a7f6983..4243898590 100644 --- a/src/shims/env.rs +++ b/src/shims/env.rs @@ -37,13 +37,13 @@ pub struct EnvVars<'tcx> { pub(crate) environ: Option>, } -impl VisitTags for EnvVars<'_> { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { +impl VisitProvenance for EnvVars<'_> { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { let EnvVars { map, environ } = self; - environ.visit_tags(visit); + environ.visit_provenance(visit); for ptr in map.values() { - ptr.visit_tags(visit); + ptr.visit_provenance(visit); } } } diff --git a/src/shims/foreign_items.rs b/src/shims/foreign_items.rs index 2d5df30374..28073d9018 100644 --- a/src/shims/foreign_items.rs +++ b/src/shims/foreign_items.rs @@ -459,6 +459,10 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { // shim, add it to the corresponding submodule. match link_name.as_str() { // Miri-specific extern functions + "miri_run_provenance_gc" => { + let [] = this.check_shim(abi, Abi::Rust, link_name, args)?; + this.run_provenance_gc(); + } "miri_get_alloc_id" => { let [ptr] = this.check_shim(abi, Abi::Rust, link_name, args)?; let ptr = this.read_pointer(ptr)?; diff --git a/src/shims/panic.rs b/src/shims/panic.rs index 5c0f828e4e..28652c25c2 100644 --- a/src/shims/panic.rs +++ b/src/shims/panic.rs @@ -35,12 +35,12 @@ pub struct CatchUnwindData<'tcx> { ret: mir::BasicBlock, } -impl VisitTags for CatchUnwindData<'_> { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { +impl VisitProvenance for CatchUnwindData<'_> { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { let CatchUnwindData { catch_fn, data, dest, ret: _ } = self; - catch_fn.visit_tags(visit); - data.visit_tags(visit); - dest.visit_tags(visit); + catch_fn.visit_provenance(visit); + data.visit_provenance(visit); + dest.visit_provenance(visit); } } diff --git a/src/shims/time.rs b/src/shims/time.rs index 4918698c6b..792122a001 100644 --- a/src/shims/time.rs +++ b/src/shims/time.rs @@ -274,8 +274,8 @@ struct UnblockCallback { thread_to_unblock: ThreadId, } -impl VisitTags for UnblockCallback { - fn visit_tags(&self, _visit: &mut dyn FnMut(BorTag)) {} +impl VisitProvenance for UnblockCallback { + fn visit_provenance(&self, _visit: &mut VisitWith<'_>) {} } impl<'mir, 'tcx: 'mir> MachineCallback<'mir, 'tcx> for UnblockCallback { diff --git a/src/shims/tls.rs b/src/shims/tls.rs index 62bd087e7e..b319516c25 100644 --- a/src/shims/tls.rs +++ b/src/shims/tls.rs @@ -207,15 +207,15 @@ impl<'tcx> TlsData<'tcx> { } } -impl VisitTags for TlsData<'_> { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { +impl VisitProvenance for TlsData<'_> { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { let TlsData { keys, macos_thread_dtors, next_key: _ } = self; for scalar in keys.values().flat_map(|v| v.data.values()) { - scalar.visit_tags(visit); + scalar.visit_provenance(visit); } for (_, scalar) in macos_thread_dtors.values() { - scalar.visit_tags(visit); + scalar.visit_provenance(visit); } } } diff --git a/src/shims/unix/fs.rs b/src/shims/unix/fs.rs index 062623a7f6..471eadb8c3 100644 --- a/src/shims/unix/fs.rs +++ b/src/shims/unix/fs.rs @@ -288,8 +288,8 @@ pub struct FileHandler { pub handles: BTreeMap>, } -impl VisitTags for FileHandler { - fn visit_tags(&self, _visit: &mut dyn FnMut(BorTag)) { +impl VisitProvenance for FileHandler { + fn visit_provenance(&self, _visit: &mut VisitWith<'_>) { // All our FileDescriptor do not have any tags. } } @@ -490,12 +490,12 @@ impl Default for DirHandler { } } -impl VisitTags for DirHandler { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { +impl VisitProvenance for DirHandler { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { let DirHandler { streams, next_id: _ } = self; for dir in streams.values() { - dir.entry.visit_tags(visit); + dir.entry.visit_provenance(visit); } } } diff --git a/src/shims/unix/linux/sync.rs b/src/shims/unix/linux/sync.rs index ff25b8120b..10e06226b3 100644 --- a/src/shims/unix/linux/sync.rs +++ b/src/shims/unix/linux/sync.rs @@ -182,10 +182,10 @@ pub fn futex<'tcx>( dest: PlaceTy<'tcx, Provenance>, } - impl<'tcx> VisitTags for Callback<'tcx> { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { + impl<'tcx> VisitProvenance for Callback<'tcx> { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { let Callback { thread: _, addr_usize: _, dest } = self; - dest.visit_tags(visit); + dest.visit_provenance(visit); } } diff --git a/src/shims/unix/sync.rs b/src/shims/unix/sync.rs index 1a91219e01..45b47450b7 100644 --- a/src/shims/unix/sync.rs +++ b/src/shims/unix/sync.rs @@ -886,10 +886,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { dest: PlaceTy<'tcx, Provenance>, } - impl<'tcx> VisitTags for Callback<'tcx> { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { + impl<'tcx> VisitProvenance for Callback<'tcx> { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { let Callback { active_thread: _, mutex_id: _, id: _, dest } = self; - dest.visit_tags(visit); + dest.visit_provenance(visit); } } diff --git a/src/shims/windows/sync.rs b/src/shims/windows/sync.rs index 2c9603097c..2b9801fea6 100644 --- a/src/shims/windows/sync.rs +++ b/src/shims/windows/sync.rs @@ -204,10 +204,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { pending_place: PlaceTy<'tcx, Provenance>, } - impl<'tcx> VisitTags for Callback<'tcx> { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { + impl<'tcx> VisitProvenance for Callback<'tcx> { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { let Callback { init_once_id: _, pending_place } = self; - pending_place.visit_tags(visit); + pending_place.visit_provenance(visit); } } @@ -337,10 +337,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { dest: PlaceTy<'tcx, Provenance>, } - impl<'tcx> VisitTags for Callback<'tcx> { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { + impl<'tcx> VisitProvenance for Callback<'tcx> { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { let Callback { thread: _, addr: _, dest } = self; - dest.visit_tags(visit); + dest.visit_provenance(visit); } } @@ -441,10 +441,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { dest: PlaceTy<'tcx, Provenance>, } - impl<'tcx> VisitTags for Callback<'tcx> { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { + impl<'tcx> VisitProvenance for Callback<'tcx> { + fn visit_provenance(&self, visit: &mut VisitWith<'_>) { let Callback { thread: _, condvar_id: _, lock_id: _, mode: _, dest } = self; - dest.visit_tags(visit); + dest.visit_provenance(visit); } } diff --git a/src/tag_gc.rs b/src/tag_gc.rs deleted file mode 100644 index 3cccdd3635..0000000000 --- a/src/tag_gc.rs +++ /dev/null @@ -1,169 +0,0 @@ -use either::Either; - -use rustc_data_structures::fx::FxHashSet; - -use crate::*; - -pub trait VisitTags { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)); -} - -impl VisitTags for Option { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { - if let Some(x) = self { - x.visit_tags(visit); - } - } -} - -impl VisitTags for std::cell::RefCell { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { - self.borrow().visit_tags(visit) - } -} - -impl VisitTags for BorTag { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { - visit(*self) - } -} - -impl VisitTags for Provenance { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { - if let Provenance::Concrete { tag, .. } = self { - visit(*tag); - } - } -} - -impl VisitTags for Pointer { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { - let (prov, _offset) = self.into_parts(); - prov.visit_tags(visit); - } -} - -impl VisitTags for Pointer> { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { - let (prov, _offset) = self.into_parts(); - prov.visit_tags(visit); - } -} - -impl VisitTags for Scalar { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { - match self { - Scalar::Ptr(ptr, _) => ptr.visit_tags(visit), - Scalar::Int(_) => (), - } - } -} - -impl VisitTags for Immediate { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { - match self { - Immediate::Scalar(s) => { - s.visit_tags(visit); - } - Immediate::ScalarPair(s1, s2) => { - s1.visit_tags(visit); - s2.visit_tags(visit); - } - Immediate::Uninit => {} - } - } -} - -impl VisitTags for MemPlaceMeta { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { - match self { - MemPlaceMeta::Meta(m) => m.visit_tags(visit), - MemPlaceMeta::None => {} - } - } -} - -impl VisitTags for ImmTy<'_, Provenance> { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { - (**self).visit_tags(visit) - } -} - -impl VisitTags for MPlaceTy<'_, Provenance> { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { - self.ptr().visit_tags(visit); - self.meta().visit_tags(visit); - } -} - -impl VisitTags for PlaceTy<'_, Provenance> { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { - match self.as_mplace_or_local() { - Either::Left(mplace) => mplace.visit_tags(visit), - Either::Right(_) => (), - } - } -} - -impl VisitTags for OpTy<'_, Provenance> { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { - match self.as_mplace_or_imm() { - Either::Left(mplace) => mplace.visit_tags(visit), - Either::Right(imm) => imm.visit_tags(visit), - } - } -} - -impl VisitTags for Allocation> { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { - for prov in self.provenance().provenances() { - prov.visit_tags(visit); - } - - self.extra.visit_tags(visit); - } -} - -impl VisitTags for crate::MiriInterpCx<'_, '_> { - fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) { - // Memory. - self.memory.alloc_map().iter(|it| { - for (_id, (_kind, alloc)) in it { - alloc.visit_tags(visit); - } - }); - - // And all the other machine values. - self.machine.visit_tags(visit); - } -} - -impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriInterpCx<'mir, 'tcx> {} -pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> { - fn garbage_collect_tags(&mut self) -> InterpResult<'tcx> { - let this = self.eval_context_mut(); - // No reason to do anything at all if stacked borrows is off. - if this.machine.borrow_tracker.is_none() { - return Ok(()); - } - - let mut tags = FxHashSet::default(); - this.visit_tags(&mut |tag| { - tags.insert(tag); - }); - self.remove_unreachable_tags(tags); - - Ok(()) - } - - fn remove_unreachable_tags(&mut self, tags: FxHashSet) { - let this = self.eval_context_mut(); - this.memory.alloc_map().iter(|it| { - for (_id, (_kind, alloc)) in it { - if let Some(bt) = &alloc.extra.borrow_tracker { - bt.remove_unreachable_tags(&tags); - } - } - }); - } -} diff --git a/tests/pass/ptr_int_casts.rs b/tests/pass/ptr_int_casts.rs index a2fcd09810..820bd33ea4 100644 --- a/tests/pass/ptr_int_casts.rs +++ b/tests/pass/ptr_int_casts.rs @@ -1,6 +1,10 @@ //@revisions: stack tree //@[tree]compile-flags: -Zmiri-tree-borrows //@compile-flags: -Zmiri-permissive-provenance + +#[path = "../utils/mod.rs"] +mod utils; + use std::mem; use std::ptr; @@ -46,6 +50,20 @@ fn ptr_int_casts() { // involving types other than usize assert_eq!((-1i32) as usize as *const i32 as usize, (-1i32) as usize); + + // Check that the GC doesn't delete context that would prevent us from casting from int + // to pointer correctly after the allocation is dead. + let (ptr, int) = { + let local = 0u8; + let ptr = &local as *const u8; + (ptr, ptr as usize) + }; + // Manually run the GC, instead of just hoping that it runs at the right time. + unsafe { utils::miri_run_provenance_gc() } + let later_int = ptr as usize; + assert_eq!(int, later_int); + let later_ptr = int as *const u8; + assert_eq!(ptr, later_ptr); } fn ptr_int_ops() { diff --git a/tests/utils/miri_extern.rs b/tests/utils/miri_extern.rs index c0ef2c5064..943eebf46a 100644 --- a/tests/utils/miri_extern.rs +++ b/tests/utils/miri_extern.rs @@ -7,8 +7,7 @@ pub struct MiriFrame { // The size of filename of the function being executed, encoded in UTF-8 pub filename_len: usize, // The line number currently being executed in `filename`, starting from '1'. - pub lineno: u32, - // The column number currently being executed in `filename`, starting from '1'. + pub lineno: u32, // The column number currently being executed in `filename`, starting from '1'. pub colno: u32, // The function pointer to the function currently being executed. // This can be compared against function pointers obtained by @@ -137,4 +136,9 @@ extern "Rust" { out: *mut std::ffi::c_char, out_size: usize, ) -> usize; + + /// Run the provenance GC. The GC will run automatically at some cadence, but tests we want to + /// have control of when it runs so that we can run it for sure at certain points to make sure + /// that it doesn't break anything. + pub fn miri_run_provenance_gc(); } From 98acef88f6b4972cc5b85140aa24ad03084db42f Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Sun, 12 Nov 2023 15:56:59 -0500 Subject: [PATCH 02/14] Patch up the now-unlucky test --- tests/pass/float_nan.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/pass/float_nan.rs b/tests/pass/float_nan.rs index 9b0a40c41b..2ac6ea8269 100644 --- a/tests/pass/float_nan.rs +++ b/tests/pass/float_nan.rs @@ -20,8 +20,8 @@ use NaNKind::*; #[track_caller] fn check_all_outcomes(expected: HashSet, generate: impl Fn() -> T) { let mut seen = HashSet::new(); - // Let's give it 8x as many tries as we are expecting values. - let tries = expected.len() * 8; + // Let's give it 10x as many tries as we are expecting values. + let tries = expected.len() * 10; for _ in 0..tries { let val = generate(); assert!(expected.contains(&val), "got an unexpected value: {val}"); From e82e323b8cfe50de4823d2e11f30f5b50c4780e8 Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Sun, 12 Nov 2023 16:18:19 -0500 Subject: [PATCH 03/14] Also GC base_ptr_tags --- src/provenance_gc.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/provenance_gc.rs b/src/provenance_gc.rs index e76ae70ca2..b7a7511ea2 100644 --- a/src/provenance_gc.rs +++ b/src/provenance_gc.rs @@ -189,5 +189,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> { let this = self.eval_context_mut(); this.machine.allocation_spans.borrow_mut().retain(|id, _| allocs.contains(id)); this.machine.intptrcast.borrow_mut().remove_unreachable_allocs(&allocs); + if let Some(borrow_tracker) = &this.machine.borrow_tracker { + borrow_tracker.borrow_mut().base_ptr_tags.retain(|id, _| allocs.contains(id)); + } } } From 5cc1eb9ecd1026e40c2b567ebb942a41e9f74900 Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Mon, 13 Nov 2023 09:32:49 -0500 Subject: [PATCH 04/14] Improve comment about initial reachable set Co-authored-by: Ralf Jung --- src/provenance_gc.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/provenance_gc.rs b/src/provenance_gc.rs index b7a7511ea2..e0fcfc4e86 100644 --- a/src/provenance_gc.rs +++ b/src/provenance_gc.rs @@ -156,6 +156,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> { // Initially populate the set of reachable AllocIds with all live allocations. This ensures // that we do not remove state associated with leaked allocations. + // Here we exploit that `adjust_allocation` always returns `Owned`, to all + // tcx-managed allocations ever read or written will be copied in `alloc_map`. let mut alloc_ids = FxHashSet::default(); this.memory.alloc_map().iter(|it| { for (id, _) in it { From b220bdf800af30f1c45b54b4600fc806e429136f Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Mon, 13 Nov 2023 09:38:06 -0500 Subject: [PATCH 05/14] Clarify comment about miri_run_provenance_gc Co-authored-by: Ralf Jung --- tests/utils/miri_extern.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/utils/miri_extern.rs b/tests/utils/miri_extern.rs index 943eebf46a..c92c3d69a2 100644 --- a/tests/utils/miri_extern.rs +++ b/tests/utils/miri_extern.rs @@ -137,8 +137,8 @@ extern "Rust" { out_size: usize, ) -> usize; - /// Run the provenance GC. The GC will run automatically at some cadence, but tests we want to - /// have control of when it runs so that we can run it for sure at certain points to make sure + /// Run the provenance GC. The GC will run automatically at some cadence, + /// but in tests we want to for sure run it at certain points to check /// that it doesn't break anything. pub fn miri_run_provenance_gc(); } From 18117c213364b9bdd44dd1485a61e6a3a6d669aa Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Mon, 13 Nov 2023 09:51:59 -0500 Subject: [PATCH 06/14] Clear up VisitProvenance for intptrcast state --- src/intptrcast.rs | 18 ++++++++---------- src/provenance_gc.rs | 10 +++++----- tests/utils/miri_extern.rs | 3 ++- 3 files changed, 15 insertions(+), 16 deletions(-) diff --git a/src/intptrcast.rs b/src/intptrcast.rs index 76a3b9a030..57ae813fc0 100644 --- a/src/intptrcast.rs +++ b/src/intptrcast.rs @@ -47,22 +47,20 @@ pub struct GlobalStateInner { } impl VisitProvenance for GlobalStateInner { - fn visit_provenance(&self, visit: &mut VisitWith<'_>) { + fn visit_provenance(&self, _visit: &mut VisitWith<'_>) { let GlobalStateInner { int_to_ptr_map: _, base_addr: _, - exposed, + exposed: _, next_base_addr: _, provenance_mode: _, } = self; - // Though base_addr and int_to_ptr_map contain AllocIds, we do not want to visit them. - // We're trying to remove unused elements from base_addr, so visiting it would prevent - // removing anything. int_to_ptr_map is managed by free_alloc_id, and entries in it do not - // actually make allocation base addresses reachable so we don't need to visit it. - // But exposed tags do make base addresses reachable. - for id in exposed { - id.visit_provenance(visit) - } + // Though base_addr, int_to_ptr_map, and exposed contain AllocIds, we do not want to visit them. + // int_to_ptr_map and exposed must contain live allocations, and those were already handled + // by visiting all AllocIds in our Machine::MemoryMap. + // base_addr is only relevant if we have a pointer to an AllocId and need to look up its + // base address; so if an AllocId is not reachable from somewhere else we can remove it + // here. } } diff --git a/src/provenance_gc.rs b/src/provenance_gc.rs index e0fcfc4e86..3ce1b43a64 100644 --- a/src/provenance_gc.rs +++ b/src/provenance_gc.rs @@ -134,7 +134,10 @@ impl VisitProvenance for Allocation> { impl VisitProvenance for crate::MiriInterpCx<'_, '_> { fn visit_provenance(&self, visit: &mut VisitWith<'_>) { - // Memory. + // Visit the contents of the allocations and the IDs themselves, to account for all + // live allocation IDs and all provenance in the allocation bytes, even if they are leaked. + // Here we exploit that `adjust_allocation` always returns `Owned`, to all + // tcx-managed allocations ever read or written will be copied in `alloc_map`. self.memory.alloc_map().iter(|it| { for (id, (_kind, alloc)) in it { id.visit_provenance(visit); @@ -154,16 +157,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> { let mut tags = FxHashSet::default(); - // Initially populate the set of reachable AllocIds with all live allocations. This ensures - // that we do not remove state associated with leaked allocations. - // Here we exploit that `adjust_allocation` always returns `Owned`, to all - // tcx-managed allocations ever read or written will be copied in `alloc_map`. let mut alloc_ids = FxHashSet::default(); this.memory.alloc_map().iter(|it| { for (id, _) in it { alloc_ids.insert(*id); } }); + let mut alloc_ids = FxHashSet::default(); this.visit_provenance(&mut |id, tag| { if let Some(id) = id { alloc_ids.insert(id); diff --git a/tests/utils/miri_extern.rs b/tests/utils/miri_extern.rs index c92c3d69a2..ed604a0c04 100644 --- a/tests/utils/miri_extern.rs +++ b/tests/utils/miri_extern.rs @@ -7,7 +7,8 @@ pub struct MiriFrame { // The size of filename of the function being executed, encoded in UTF-8 pub filename_len: usize, // The line number currently being executed in `filename`, starting from '1'. - pub lineno: u32, // The column number currently being executed in `filename`, starting from '1'. + pub lineno: u32, + // The column number currently being executed in `filename`, starting from '1'. pub colno: u32, // The function pointer to the function currently being executed. // This can be compared against function pointers obtained by From f7124b9ed2d13bbf86262dff02fded817707f25f Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Mon, 13 Nov 2023 10:17:39 -0500 Subject: [PATCH 07/14] Double-check that we don't expose dead allocs --- src/intptrcast.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/intptrcast.rs b/src/intptrcast.rs index 57ae813fc0..c9cbc2f4da 100644 --- a/src/intptrcast.rs +++ b/src/intptrcast.rs @@ -195,6 +195,9 @@ impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriInterpCx<'mir, pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { fn expose_ptr(&mut self, alloc_id: AllocId, tag: BorTag) -> InterpResult<'tcx> { let ecx = self.eval_context_mut(); + // Make sure we aren't trying to expose a dead allocation; this is part of justifying the + // way we treat the exposed set in VisitProvenance. + assert!(!matches!(ecx.get_alloc_info(alloc_id).2, AllocKind::Dead)); let global_state = ecx.machine.intptrcast.get_mut(); // In strict mode, we don't need this, so we can save some cycles by not tracking it. if global_state.provenance_mode != ProvenanceMode::Strict { From 2fd4fb589d175150abdb68b69e45440fd8d75340 Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Mon, 13 Nov 2023 10:25:31 -0500 Subject: [PATCH 08/14] Fix merge goof --- src/provenance_gc.rs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/provenance_gc.rs b/src/provenance_gc.rs index 3ce1b43a64..8e1775cfb9 100644 --- a/src/provenance_gc.rs +++ b/src/provenance_gc.rs @@ -156,13 +156,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> { let this = self.eval_context_mut(); let mut tags = FxHashSet::default(); - - let mut alloc_ids = FxHashSet::default(); - this.memory.alloc_map().iter(|it| { - for (id, _) in it { - alloc_ids.insert(*id); - } - }); let mut alloc_ids = FxHashSet::default(); this.visit_provenance(&mut |id, tag| { if let Some(id) = id { From 2eb618f036cb79a52a0a5239f0cdaf6549dd6c90 Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Mon, 13 Nov 2023 16:55:25 -0500 Subject: [PATCH 09/14] Update src/intptrcast.rs Co-authored-by: Ralf Jung --- src/intptrcast.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/intptrcast.rs b/src/intptrcast.rs index c9cbc2f4da..104649c891 100644 --- a/src/intptrcast.rs +++ b/src/intptrcast.rs @@ -56,7 +56,7 @@ impl VisitProvenance for GlobalStateInner { provenance_mode: _, } = self; // Though base_addr, int_to_ptr_map, and exposed contain AllocIds, we do not want to visit them. - // int_to_ptr_map and exposed must contain live allocations, and those were already handled + // int_to_ptr_map and exposed must contain only live allocations, and those were already handled // by visiting all AllocIds in our Machine::MemoryMap. // base_addr is only relevant if we have a pointer to an AllocId and need to look up its // base address; so if an AllocId is not reachable from somewhere else we can remove it From 3560ef2b44cbec7937e166d947e9f652ed0e8aee Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Mon, 13 Nov 2023 16:55:41 -0500 Subject: [PATCH 10/14] Update src/intptrcast.rs Co-authored-by: Ralf Jung --- src/intptrcast.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/intptrcast.rs b/src/intptrcast.rs index 104649c891..6fccfaa78d 100644 --- a/src/intptrcast.rs +++ b/src/intptrcast.rs @@ -76,6 +76,8 @@ impl GlobalStateInner { } pub fn remove_unreachable_allocs(&mut self, reachable_allocs: &FxHashSet) { + // `exposed` and `int_to_ptr_map` are cleared immediately when an allocation + // is freed, so `base_addr` is the only one we have to clean up based on the GC. self.base_addr.retain(|id, _| reachable_allocs.contains(id)); } } From db9b0ad58339f5d6881e615592641d8004b5aa69 Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Mon, 13 Nov 2023 16:59:59 -0500 Subject: [PATCH 11/14] Go through remove_unreachable_allocs --- src/borrow_tracker/mod.rs | 4 ++++ src/provenance_gc.rs | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/borrow_tracker/mod.rs b/src/borrow_tracker/mod.rs index 5a7e245f54..fbd5127c53 100644 --- a/src/borrow_tracker/mod.rs +++ b/src/borrow_tracker/mod.rs @@ -236,6 +236,10 @@ impl GlobalStateInner { tag }) } + + pub fn remove_unreachable_allocs(&mut self, reachable: &FxHashSet) { + self.base_ptr_tags.retain(|id, _| reachable.contains(id)); + } } /// Which borrow tracking method to use diff --git a/src/provenance_gc.rs b/src/provenance_gc.rs index 8e1775cfb9..cfef34e52a 100644 --- a/src/provenance_gc.rs +++ b/src/provenance_gc.rs @@ -185,7 +185,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> { this.machine.allocation_spans.borrow_mut().retain(|id, _| allocs.contains(id)); this.machine.intptrcast.borrow_mut().remove_unreachable_allocs(&allocs); if let Some(borrow_tracker) = &this.machine.borrow_tracker { - borrow_tracker.borrow_mut().base_ptr_tags.retain(|id, _| allocs.contains(id)); + borrow_tracker.borrow_mut().remove_unreachable_allocs(&allocs); } } } From 2b236d8c5395a4f5ad1193c6e0c2c32b534e282e Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Mon, 13 Nov 2023 17:12:50 -0500 Subject: [PATCH 12/14] Rearrange the check for dead allocs in expose_ptr --- src/intptrcast.rs | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/intptrcast.rs b/src/intptrcast.rs index 6fccfaa78d..d3c0352484 100644 --- a/src/intptrcast.rs +++ b/src/intptrcast.rs @@ -197,17 +197,20 @@ impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriInterpCx<'mir, pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { fn expose_ptr(&mut self, alloc_id: AllocId, tag: BorTag) -> InterpResult<'tcx> { let ecx = self.eval_context_mut(); - // Make sure we aren't trying to expose a dead allocation; this is part of justifying the - // way we treat the exposed set in VisitProvenance. - assert!(!matches!(ecx.get_alloc_info(alloc_id).2, AllocKind::Dead)); + // Exposing a dead alloc is a no-op, because it's not possible to get a dead allocation + // via int2ptr. + if matches!(ecx.get_alloc_info(alloc_id).2, AllocKind::Dead) { + return Ok(()); + } let global_state = ecx.machine.intptrcast.get_mut(); // In strict mode, we don't need this, so we can save some cycles by not tracking it. - if global_state.provenance_mode != ProvenanceMode::Strict { - trace!("Exposing allocation id {alloc_id:?}"); - global_state.exposed.insert(alloc_id); - if ecx.machine.borrow_tracker.is_some() { - ecx.expose_tag(alloc_id, tag)?; - } + if global_state.provenance_mode == ProvenanceMode::Strict { + return Ok(()); + } + trace!("Exposing allocation id {alloc_id:?}"); + global_state.exposed.insert(alloc_id); + if ecx.machine.borrow_tracker.is_some() { + ecx.expose_tag(alloc_id, tag)?; } Ok(()) } From 68ec1210ab36640feb865de9bb7f54149ae2e81d Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Thu, 16 Nov 2023 17:04:33 -0500 Subject: [PATCH 13/14] Use TyCtxt::iter_allocs --- src/provenance_gc.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/provenance_gc.rs b/src/provenance_gc.rs index cfef34e52a..6e576c598f 100644 --- a/src/provenance_gc.rs +++ b/src/provenance_gc.rs @@ -136,15 +136,16 @@ impl VisitProvenance for crate::MiriInterpCx<'_, '_> { fn visit_provenance(&self, visit: &mut VisitWith<'_>) { // Visit the contents of the allocations and the IDs themselves, to account for all // live allocation IDs and all provenance in the allocation bytes, even if they are leaked. - // Here we exploit that `adjust_allocation` always returns `Owned`, to all - // tcx-managed allocations ever read or written will be copied in `alloc_map`. self.memory.alloc_map().iter(|it| { for (id, (_kind, alloc)) in it { id.visit_provenance(visit); alloc.visit_provenance(visit); } }); - + // Visit all the tcx-managed allocations, which includes functions and vtables. + self.tcx.iter_allocs(|id| { + id.visit_provenance(visit); + }); // And all the other machine values. self.machine.visit_provenance(visit); } From 23b4af84884261fec154b291265eb0ea2143b2bc Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Thu, 16 Nov 2023 17:06:11 -0500 Subject: [PATCH 14/14] Check provenance mode first --- src/intptrcast.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/intptrcast.rs b/src/intptrcast.rs index d3c0352484..a66d9ab95f 100644 --- a/src/intptrcast.rs +++ b/src/intptrcast.rs @@ -197,17 +197,19 @@ impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriInterpCx<'mir, pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { fn expose_ptr(&mut self, alloc_id: AllocId, tag: BorTag) -> InterpResult<'tcx> { let ecx = self.eval_context_mut(); - // Exposing a dead alloc is a no-op, because it's not possible to get a dead allocation - // via int2ptr. - if matches!(ecx.get_alloc_info(alloc_id).2, AllocKind::Dead) { - return Ok(()); - } let global_state = ecx.machine.intptrcast.get_mut(); // In strict mode, we don't need this, so we can save some cycles by not tracking it. if global_state.provenance_mode == ProvenanceMode::Strict { return Ok(()); } + // Exposing a dead alloc is a no-op, because it's not possible to get a dead allocation + // via int2ptr. + if matches!(ecx.get_alloc_info(alloc_id).2, AllocKind::Dead) { + return Ok(()); + } trace!("Exposing allocation id {alloc_id:?}"); + let ecx = self.eval_context_mut(); + let global_state = ecx.machine.intptrcast.get_mut(); global_state.exposed.insert(alloc_id); if ecx.machine.borrow_tracker.is_some() { ecx.expose_tag(alloc_id, tag)?;