From 90d566cb8d59f340ab3e19806b5eca46a1b4b499 Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Sat, 24 Sep 2022 14:20:22 -0400 Subject: [PATCH 1/7] Expand VisitMachineValues to cover more pointers in the interpreter --- src/concurrency/range_object_map.rs | 4 ++ src/concurrency/thread.rs | 78 +++++++++++++++++++---------- src/concurrency/weak_memory.rs | 13 +++++ src/machine.rs | 40 +++++++++++++-- src/shims/env.rs | 14 ++++++ src/shims/panic.rs | 7 +++ src/shims/unix/fs.rs | 8 +++ src/stacked_borrows/mod.rs | 2 +- src/stacked_borrows/stack.rs | 7 ++- src/tag_gc.rs | 16 ++++++ 10 files changed, 156 insertions(+), 33 deletions(-) diff --git a/src/concurrency/range_object_map.rs b/src/concurrency/range_object_map.rs index 50d3f8c9b2..dfe2e9f05d 100644 --- a/src/concurrency/range_object_map.rs +++ b/src/concurrency/range_object_map.rs @@ -132,6 +132,10 @@ impl RangeObjectMap { pub fn remove_from_pos(&mut self, pos: Position) { self.v.remove(pos); } + + pub fn iter(&self) -> impl Iterator { + self.v.iter().map(|e| &e.data) + } } impl Index for RangeObjectMap { diff --git a/src/concurrency/thread.rs b/src/concurrency/thread.rs index a9d144eff5..7b91f8c223 100644 --- a/src/concurrency/thread.rs +++ b/src/concurrency/thread.rs @@ -181,6 +181,41 @@ impl<'mir, 'tcx> Thread<'mir, 'tcx> { } } +impl VisitMachineValues for Thread<'_, '_> { + fn visit_machine_values(&self, visit: &mut impl FnMut(&Operand)) { + let Thread { panic_payload, last_error, stack, .. } = self; + + if let Some(payload) = panic_payload { + visit(&Operand::Immediate(Immediate::Scalar(*payload))) + } + if let Some(error) = last_error { + visit(&Operand::Indirect(**error)) + } + for frame in stack { + frame.visit_machine_values(visit) + } + } +} + +impl VisitMachineValues for Frame<'_, '_, Provenance, FrameData<'_>> { + fn visit_machine_values(&self, visit: &mut impl FnMut(&Operand)) { + let Frame { return_place, locals, extra, .. } = self; + + // Return place. + if let Place::Ptr(mplace) = **return_place { + visit(&Operand::Indirect(mplace)); + } + // Locals. + for local in locals.iter() { + if let LocalValue::Live(value) = &local.value { + visit(value); + } + } + + extra.visit_machine_values(visit); + } +} + /// A specific moment in time. #[derive(Debug)] pub enum Time { @@ -253,6 +288,22 @@ impl<'mir, 'tcx> Default for ThreadManager<'mir, 'tcx> { } } +impl VisitMachineValues for ThreadManager<'_, '_> { + fn visit_machine_values(&self, visit: &mut impl FnMut(&Operand)) { + let ThreadManager { threads, thread_local_alloc_ids, .. } = self; + + for thread in threads { + thread.visit_machine_values(visit); + } + for ptr in thread_local_alloc_ids.borrow().values().copied() { + let ptr: Pointer> = ptr.into(); + visit(&Operand::Indirect(MemPlace::from_ptr(ptr))); + } + // FIXME: Do we need to do something for TimeoutCallback? That's a Box, not sure what + // to do. + } +} + impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { pub(crate) fn init(ecx: &mut MiriInterpCx<'mir, 'tcx>) { if ecx.tcx.sess.target.os.as_ref() != "windows" { @@ -625,33 +676,6 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { } } -impl VisitMachineValues for ThreadManager<'_, '_> { - fn visit_machine_values(&self, visit: &mut impl FnMut(&Operand)) { - // FIXME some other fields also contain machine values - let ThreadManager { threads, .. } = self; - - for thread in threads { - // FIXME: implement VisitMachineValues for `Thread` and `Frame` instead. - // In particular we need to visit the `last_error` and `catch_unwind` fields. - if let Some(payload) = thread.panic_payload { - visit(&Operand::Immediate(Immediate::Scalar(payload))) - } - for frame in &thread.stack { - // Return place. - if let Place::Ptr(mplace) = *frame.return_place { - visit(&Operand::Indirect(mplace)); - } - // Locals. - for local in frame.locals.iter() { - if let LocalValue::Live(value) = &local.value { - visit(value); - } - } - } - } - } -} - // Public interface to thread management. impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriInterpCx<'mir, 'tcx> {} pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { diff --git a/src/concurrency/weak_memory.rs b/src/concurrency/weak_memory.rs index bac403e9ec..aa4e5add50 100644 --- a/src/concurrency/weak_memory.rs +++ b/src/concurrency/weak_memory.rs @@ -108,6 +108,19 @@ pub struct StoreBufferAlloc { store_buffers: RefCell>, } +impl StoreBufferAlloc { + pub fn iter(&self, mut visitor: impl FnMut(&Scalar)) { + for val in self + .store_buffers + .borrow() + .iter() + .flat_map(|buf| buf.buffer.iter().map(|element| &element.val)) + { + visitor(val) + } + } +} + #[derive(Debug, Clone, PartialEq, Eq)] pub(super) struct StoreBuffer { // Stores to this location in modification order diff --git a/src/machine.rs b/src/machine.rs index 03df78c1af..6a06df2a16 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -63,6 +63,16 @@ impl<'tcx> std::fmt::Debug for FrameData<'tcx> { } } +impl VisitMachineValues for FrameData<'_> { + fn visit_machine_values(&self, visit: &mut impl FnMut(&Operand)) { + let FrameData { catch_unwind, .. } = self; + + if let Some(catch_unwind) = catch_unwind { + catch_unwind.visit_machine_values(visit); + } + } +} + /// Extra memory kinds #[derive(Debug, Copy, Clone, PartialEq, Eq)] pub enum MiriMemoryKind { @@ -593,12 +603,36 @@ impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> { impl VisitMachineValues for MiriMachine<'_, '_> { fn visit_machine_values(&self, visit: &mut impl FnMut(&Operand)) { - // FIXME: visit the missing fields: env vars, weak mem, the MemPlace fields in the machine, - // DirHandler, extern_statics, the Stacked Borrows base pointers; maybe more. - let MiriMachine { threads, tls, .. } = self; + let MiriMachine { + threads, + tls, + env_vars, + argc, + argv, + cmd_line, + extern_statics, + dir_handler, + .. + } = self; threads.visit_machine_values(visit); tls.visit_machine_values(visit); + env_vars.visit_machine_values(visit); + dir_handler.visit_machine_values(visit); + + if let Some(argc) = argc { + visit(&Operand::Indirect(*argc)); + } + if let Some(argv) = argv { + visit(&Operand::Indirect(*argv)); + } + if let Some(cmd_line) = cmd_line { + visit(&Operand::Indirect(*cmd_line)); + } + for ptr in extern_statics.values().copied() { + let ptr: Pointer> = ptr.into(); + visit(&Operand::Indirect(MemPlace::from_ptr(ptr))); + } } } diff --git a/src/shims/env.rs b/src/shims/env.rs index 95051c998e..ad2d2eaab3 100644 --- a/src/shims/env.rs +++ b/src/shims/env.rs @@ -36,6 +36,20 @@ pub struct EnvVars<'tcx> { pub(crate) environ: Option>, } +impl VisitMachineValues for EnvVars<'_> { + fn visit_machine_values(&self, visit: &mut impl FnMut(&Operand)) { + let EnvVars { map, environ } = self; + + for ptr in map.values() { + visit(&Operand::Indirect(MemPlace::from_ptr(*ptr))); + } + + if let Some(env) = environ { + visit(&Operand::Indirect(**env)); + } + } +} + impl<'tcx> EnvVars<'tcx> { pub(crate) fn init<'mir>( ecx: &mut InterpCx<'mir, 'tcx, MiriMachine<'mir, 'tcx>>, diff --git a/src/shims/panic.rs b/src/shims/panic.rs index 2e8245acf4..dd6e2d53b4 100644 --- a/src/shims/panic.rs +++ b/src/shims/panic.rs @@ -35,6 +35,13 @@ pub struct CatchUnwindData<'tcx> { ret: mir::BasicBlock, } +impl VisitMachineValues for CatchUnwindData<'_> { + fn visit_machine_values(&self, visit: &mut impl FnMut(&Operand)) { + visit(&Operand::Indirect(MemPlace::from_ptr(self.catch_fn))); + visit(&Operand::Immediate(Immediate::Scalar(self.data))); + } +} + impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriInterpCx<'mir, 'tcx> {} pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { /// Handles the special `miri_start_panic` intrinsic, which is called diff --git a/src/shims/unix/fs.rs b/src/shims/unix/fs.rs index 8464c4589e..576e12d347 100644 --- a/src/shims/unix/fs.rs +++ b/src/shims/unix/fs.rs @@ -462,6 +462,14 @@ impl Default for DirHandler { } } +impl VisitMachineValues for DirHandler { + fn visit_machine_values(&self, visit: &mut impl FnMut(&Operand)) { + for dir in self.streams.values() { + visit(&Operand::Indirect(MemPlace::from_ptr(dir.entry))); + } + } +} + fn maybe_sync_file( file: &File, writable: bool, diff --git a/src/stacked_borrows/mod.rs b/src/stacked_borrows/mod.rs index f7f4b1357f..956f936dbc 100644 --- a/src/stacked_borrows/mod.rs +++ b/src/stacked_borrows/mod.rs @@ -79,7 +79,7 @@ pub struct Stacks { /// Stores past operations on this allocation history: AllocHistory, /// The set of tags that have been exposed inside this allocation. - exposed_tags: FxHashSet, + pub exposed_tags: FxHashSet, /// Whether this memory has been modified since the last time the tag GC ran modified_since_last_gc: bool, } diff --git a/src/stacked_borrows/stack.rs b/src/stacked_borrows/stack.rs index 494ea08b56..eb30f02393 100644 --- a/src/stacked_borrows/stack.rs +++ b/src/stacked_borrows/stack.rs @@ -43,8 +43,11 @@ impl Stack { pub fn retain(&mut self, tags: &FxHashSet) { let mut first_removed = None; - let mut read_idx = 1; - let mut write_idx = 1; + // For stacks with a known bottom, we never consider removing the bottom-most tag, because + // that is the base tag which exists whether or not there are any pointers to the + // allocation. + let mut read_idx = usize::from(self.unknown_bottom.is_none()); + let mut write_idx = read_idx; while read_idx < self.borrows.len() { let left = self.borrows[read_idx - 1]; let this = self.borrows[read_idx]; diff --git a/src/tag_gc.rs b/src/tag_gc.rs index 0bfc81ce01..9378cc1e92 100644 --- a/src/tag_gc.rs +++ b/src/tag_gc.rs @@ -71,6 +71,22 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> { tags.insert(*sb); } } + let stacks = alloc + .extra + .stacked_borrows + .as_ref() + .expect("we should not even enter the GC if Stacked Borrows is disabled"); + tags.extend(&stacks.borrow().exposed_tags); + + if let Some(store_buffers) = alloc.extra.weak_memory.as_ref() { + store_buffers.iter(|val| { + if let Scalar::Ptr(ptr, _) = val { + if let Provenance::Concrete { sb, .. } = ptr.provenance { + tags.insert(sb); + } + } + }); + } }, ); From 4f688152536ea557d85be5c1036752b7aaaa39f1 Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Mon, 26 Sep 2022 16:07:32 -0400 Subject: [PATCH 2/7] A bit of cleanup --- src/concurrency/thread.rs | 23 ++++++++++++++++++++--- src/machine.rs | 2 +- src/shims/panic.rs | 5 +++-- src/shims/unix/fs.rs | 4 +++- src/stacked_borrows/stack.rs | 2 +- 5 files changed, 28 insertions(+), 8 deletions(-) diff --git a/src/concurrency/thread.rs b/src/concurrency/thread.rs index 7b91f8c223..1b05088b3d 100644 --- a/src/concurrency/thread.rs +++ b/src/concurrency/thread.rs @@ -183,7 +183,8 @@ impl<'mir, 'tcx> Thread<'mir, 'tcx> { impl VisitMachineValues for Thread<'_, '_> { fn visit_machine_values(&self, visit: &mut impl FnMut(&Operand)) { - let Thread { panic_payload, last_error, stack, .. } = self; + let Thread { panic_payload, last_error, stack, state: _, thread_name: _, join_status: _ } = + self; if let Some(payload) = panic_payload { visit(&Operand::Immediate(Immediate::Scalar(*payload))) @@ -199,7 +200,16 @@ impl VisitMachineValues for Thread<'_, '_> { impl VisitMachineValues for Frame<'_, '_, Provenance, FrameData<'_>> { fn visit_machine_values(&self, visit: &mut impl FnMut(&Operand)) { - let Frame { return_place, locals, extra, .. } = self; + let Frame { + return_place, + locals, + extra, + body: _, + instance: _, + return_to_block: _, + loc: _, + .. + } = self; // Return place. if let Place::Ptr(mplace) = **return_place { @@ -290,7 +300,14 @@ impl<'mir, 'tcx> Default for ThreadManager<'mir, 'tcx> { impl VisitMachineValues for ThreadManager<'_, '_> { fn visit_machine_values(&self, visit: &mut impl FnMut(&Operand)) { - let ThreadManager { threads, thread_local_alloc_ids, .. } = self; + let ThreadManager { + threads, + thread_local_alloc_ids, + active_thread: _, + yield_active_thread: _, + sync: _, + timeout_callbacks: _, + } = self; for thread in threads { thread.visit_machine_values(visit); diff --git a/src/machine.rs b/src/machine.rs index 6a06df2a16..35d5c0d9a8 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -65,7 +65,7 @@ impl<'tcx> std::fmt::Debug for FrameData<'tcx> { impl VisitMachineValues for FrameData<'_> { fn visit_machine_values(&self, visit: &mut impl FnMut(&Operand)) { - let FrameData { catch_unwind, .. } = self; + let FrameData { catch_unwind, stacked_borrows: _, timing: _ } = self; if let Some(catch_unwind) = catch_unwind { catch_unwind.visit_machine_values(visit); diff --git a/src/shims/panic.rs b/src/shims/panic.rs index dd6e2d53b4..be14892f69 100644 --- a/src/shims/panic.rs +++ b/src/shims/panic.rs @@ -37,8 +37,9 @@ pub struct CatchUnwindData<'tcx> { impl VisitMachineValues for CatchUnwindData<'_> { fn visit_machine_values(&self, visit: &mut impl FnMut(&Operand)) { - visit(&Operand::Indirect(MemPlace::from_ptr(self.catch_fn))); - visit(&Operand::Immediate(Immediate::Scalar(self.data))); + let CatchUnwindData { catch_fn, data, dest: _, ret: _ } = self; + visit(&Operand::Indirect(MemPlace::from_ptr(*catch_fn))); + visit(&Operand::Immediate(Immediate::Scalar(*data))); } } diff --git a/src/shims/unix/fs.rs b/src/shims/unix/fs.rs index 576e12d347..59d24e00dc 100644 --- a/src/shims/unix/fs.rs +++ b/src/shims/unix/fs.rs @@ -464,7 +464,9 @@ impl Default for DirHandler { impl VisitMachineValues for DirHandler { fn visit_machine_values(&self, visit: &mut impl FnMut(&Operand)) { - for dir in self.streams.values() { + let DirHandler { streams, next_id: _ } = self; + + for dir in streams.values() { visit(&Operand::Indirect(MemPlace::from_ptr(dir.entry))); } } diff --git a/src/stacked_borrows/stack.rs b/src/stacked_borrows/stack.rs index eb30f02393..57de1c21c8 100644 --- a/src/stacked_borrows/stack.rs +++ b/src/stacked_borrows/stack.rs @@ -46,7 +46,7 @@ impl Stack { // For stacks with a known bottom, we never consider removing the bottom-most tag, because // that is the base tag which exists whether or not there are any pointers to the // allocation. - let mut read_idx = usize::from(self.unknown_bottom.is_none()); + let mut read_idx = if self.unknown_bottom.is_some() { 0 } else { 1 }; let mut write_idx = read_idx; while read_idx < self.borrows.len() { let left = self.borrows[read_idx - 1]; From d1563b0b1a8b20fe9526fc498b5d8a7b9bbaa79c Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Mon, 26 Sep 2022 18:06:20 -0400 Subject: [PATCH 3/7] Use VisitProvenance to factor allocation visiting better --- src/concurrency/weak_memory.rs | 10 +++++++--- src/lib.rs | 2 +- src/stacked_borrows/mod.rs | 10 +++++++++- src/tag_gc.rs | 28 +++++++++++++++------------- 4 files changed, 32 insertions(+), 18 deletions(-) diff --git a/src/concurrency/weak_memory.rs b/src/concurrency/weak_memory.rs index aa4e5add50..becd61f4fe 100644 --- a/src/concurrency/weak_memory.rs +++ b/src/concurrency/weak_memory.rs @@ -108,15 +108,19 @@ pub struct StoreBufferAlloc { store_buffers: RefCell>, } -impl StoreBufferAlloc { - pub fn iter(&self, mut visitor: impl FnMut(&Scalar)) { +impl VisitProvenance for StoreBufferAlloc { + fn visit_provenance(&self, visitor: &mut impl FnMut(SbTag)) { for val in self .store_buffers .borrow() .iter() .flat_map(|buf| buf.buffer.iter().map(|element| &element.val)) { - visitor(val) + if let Scalar::Ptr(ptr, _) = val { + if let Provenance::Concrete { sb, .. } = ptr.provenance { + visitor(sb); + } + } } } } diff --git a/src/lib.rs b/src/lib.rs index bf4e21319b..e60e1f15b5 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -112,7 +112,7 @@ pub use crate::range_map::RangeMap; pub use crate::stacked_borrows::{ CallId, EvalContextExt as StackedBorEvalContextExt, Item, Permission, SbTag, Stack, Stacks, }; -pub use crate::tag_gc::{EvalContextExt as _, VisitMachineValues}; +pub use crate::tag_gc::{EvalContextExt as _, VisitMachineValues, VisitProvenance}; /// 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/stacked_borrows/mod.rs b/src/stacked_borrows/mod.rs index 956f936dbc..ab90e35844 100644 --- a/src/stacked_borrows/mod.rs +++ b/src/stacked_borrows/mod.rs @@ -79,7 +79,7 @@ pub struct Stacks { /// Stores past operations on this allocation history: AllocHistory, /// The set of tags that have been exposed inside this allocation. - pub exposed_tags: FxHashSet, + exposed_tags: FxHashSet, /// Whether this memory has been modified since the last time the tag GC ran modified_since_last_gc: bool, } @@ -513,6 +513,14 @@ impl Stacks { } } +impl VisitProvenance for Stacks { + fn visit_provenance(&self, visit: &mut impl FnMut(SbTag)) { + for tag in self.exposed_tags.iter().copied() { + visit(tag); + } + } +} + /// Map per-stack operations to higher-level per-location-range operations. impl<'tcx> Stacks { /// Creates a new stack with an initial tag. For diagnostic purposes, we also need to know diff --git a/src/tag_gc.rs b/src/tag_gc.rs index 9378cc1e92..0a8d5d00cf 100644 --- a/src/tag_gc.rs +++ b/src/tag_gc.rs @@ -6,6 +6,10 @@ pub trait VisitMachineValues { fn visit_machine_values(&self, visit: &mut impl FnMut(&Operand)); } +pub trait VisitProvenance { + fn visit_provenance(&self, visit: &mut impl FnMut(SbTag)); +} + impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriInterpCx<'mir, 'tcx> {} pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> { /// Generic GC helper to visit everything that can store a value. The `acc` offers some chance to @@ -46,6 +50,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> { } }; + let visit_provenance = |tags: &mut FxHashSet, tag: SbTag| { + tags.insert(tag); + }; + this.visit_all_machine_values( &mut tags, |tags, op| { @@ -71,21 +79,15 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> { tags.insert(*sb); } } - let stacks = alloc - .extra - .stacked_borrows - .as_ref() - .expect("we should not even enter the GC if Stacked Borrows is disabled"); - tags.extend(&stacks.borrow().exposed_tags); + + let stacks = + alloc.extra.stacked_borrows.as_ref().expect( + "we should not even enter the tag GC if Stacked Borrows is disabled", + ); + stacks.borrow().visit_provenance(&mut |tag| visit_provenance(tags, tag)); if let Some(store_buffers) = alloc.extra.weak_memory.as_ref() { - store_buffers.iter(|val| { - if let Scalar::Ptr(ptr, _) = val { - if let Provenance::Concrete { sb, .. } = ptr.provenance { - tags.insert(sb); - } - } - }); + store_buffers.visit_provenance(&mut |tag| visit_provenance(tags, tag)); } }, ); From eb2a36b01801316a74a2419aecfe6308ba7c6b5d Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Mon, 26 Sep 2022 19:50:50 -0400 Subject: [PATCH 4/7] Use static dispatch in the visitor --- src/concurrency/thread.rs | 17 ++-- src/concurrency/weak_memory.rs | 10 +- src/lib.rs | 2 +- src/machine.rs | 27 ++++-- src/shims/env.rs | 7 +- src/shims/panic.rs | 6 +- src/shims/tls.rs | 6 +- src/shims/unix/fs.rs | 4 +- src/stacked_borrows/mod.rs | 6 +- src/tag_gc.rs | 168 ++++++++++++++++++++------------- 10 files changed, 148 insertions(+), 105 deletions(-) diff --git a/src/concurrency/thread.rs b/src/concurrency/thread.rs index 1b05088b3d..01ae4320f3 100644 --- a/src/concurrency/thread.rs +++ b/src/concurrency/thread.rs @@ -182,15 +182,15 @@ impl<'mir, 'tcx> Thread<'mir, 'tcx> { } impl VisitMachineValues for Thread<'_, '_> { - fn visit_machine_values(&self, visit: &mut impl FnMut(&Operand)) { + fn visit_machine_values(&self, visit: &mut ProvenanceVisitor) { let Thread { panic_payload, last_error, stack, state: _, thread_name: _, join_status: _ } = self; if let Some(payload) = panic_payload { - visit(&Operand::Immediate(Immediate::Scalar(*payload))) + visit.visit(*payload); } if let Some(error) = last_error { - visit(&Operand::Indirect(**error)) + visit.visit(**error); } for frame in stack { frame.visit_machine_values(visit) @@ -199,7 +199,7 @@ impl VisitMachineValues for Thread<'_, '_> { } impl VisitMachineValues for Frame<'_, '_, Provenance, FrameData<'_>> { - fn visit_machine_values(&self, visit: &mut impl FnMut(&Operand)) { + fn visit_machine_values(&self, visit: &mut ProvenanceVisitor) { let Frame { return_place, locals, @@ -213,12 +213,12 @@ impl VisitMachineValues for Frame<'_, '_, Provenance, FrameData<'_>> { // Return place. if let Place::Ptr(mplace) = **return_place { - visit(&Operand::Indirect(mplace)); + visit.visit(mplace); } // Locals. for local in locals.iter() { if let LocalValue::Live(value) = &local.value { - visit(value); + visit.visit(value); } } @@ -299,7 +299,7 @@ impl<'mir, 'tcx> Default for ThreadManager<'mir, 'tcx> { } impl VisitMachineValues for ThreadManager<'_, '_> { - fn visit_machine_values(&self, visit: &mut impl FnMut(&Operand)) { + fn visit_machine_values(&self, visit: &mut ProvenanceVisitor) { let ThreadManager { threads, thread_local_alloc_ids, @@ -313,8 +313,7 @@ impl VisitMachineValues for ThreadManager<'_, '_> { thread.visit_machine_values(visit); } for ptr in thread_local_alloc_ids.borrow().values().copied() { - let ptr: Pointer> = ptr.into(); - visit(&Operand::Indirect(MemPlace::from_ptr(ptr))); + visit.visit(ptr); } // FIXME: Do we need to do something for TimeoutCallback? That's a Box, not sure what // to do. diff --git a/src/concurrency/weak_memory.rs b/src/concurrency/weak_memory.rs index becd61f4fe..15c6c8e9c0 100644 --- a/src/concurrency/weak_memory.rs +++ b/src/concurrency/weak_memory.rs @@ -108,19 +108,15 @@ pub struct StoreBufferAlloc { store_buffers: RefCell>, } -impl VisitProvenance for StoreBufferAlloc { - fn visit_provenance(&self, visitor: &mut impl FnMut(SbTag)) { +impl VisitMachineValues for StoreBufferAlloc { + fn visit_machine_values(&self, visit: &mut ProvenanceVisitor) { for val in self .store_buffers .borrow() .iter() .flat_map(|buf| buf.buffer.iter().map(|element| &element.val)) { - if let Scalar::Ptr(ptr, _) = val { - if let Provenance::Concrete { sb, .. } = ptr.provenance { - visitor(sb); - } - } + visit.visit(val); } } } diff --git a/src/lib.rs b/src/lib.rs index e60e1f15b5..245bdc51a8 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -112,7 +112,7 @@ pub use crate::range_map::RangeMap; pub use crate::stacked_borrows::{ CallId, EvalContextExt as StackedBorEvalContextExt, Item, Permission, SbTag, Stack, Stacks, }; -pub use crate::tag_gc::{EvalContextExt as _, VisitMachineValues, VisitProvenance}; +pub use crate::tag_gc::{EvalContextExt as _, ProvenanceVisitor, VisitMachineValues}; /// 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 35d5c0d9a8..523aad22aa 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -64,7 +64,7 @@ impl<'tcx> std::fmt::Debug for FrameData<'tcx> { } impl VisitMachineValues for FrameData<'_> { - fn visit_machine_values(&self, visit: &mut impl FnMut(&Operand)) { + fn visit_machine_values(&self, visit: &mut ProvenanceVisitor) { let FrameData { catch_unwind, stacked_borrows: _, timing: _ } = self; if let Some(catch_unwind) = catch_unwind { @@ -261,6 +261,20 @@ pub struct AllocExtra { pub weak_memory: Option, } +impl VisitMachineValues for AllocExtra { + fn visit_machine_values(&self, visit: &mut ProvenanceVisitor) { + let AllocExtra { stacked_borrows, data_race: _, weak_memory } = self; + + if let Some(stacked_borrows) = stacked_borrows { + stacked_borrows.borrow().visit_machine_values(visit); + } + + if let Some(weak_memory) = weak_memory { + weak_memory.visit_machine_values(visit); + } + } +} + /// Precomputed layouts of primitive types pub struct PrimitiveLayouts<'tcx> { pub unit: TyAndLayout<'tcx>, @@ -602,7 +616,7 @@ impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> { } impl VisitMachineValues for MiriMachine<'_, '_> { - fn visit_machine_values(&self, visit: &mut impl FnMut(&Operand)) { + fn visit_machine_values(&self, visit: &mut ProvenanceVisitor) { let MiriMachine { threads, tls, @@ -621,17 +635,16 @@ impl VisitMachineValues for MiriMachine<'_, '_> { dir_handler.visit_machine_values(visit); if let Some(argc) = argc { - visit(&Operand::Indirect(*argc)); + visit.visit(argc); } if let Some(argv) = argv { - visit(&Operand::Indirect(*argv)); + visit.visit(argv); } if let Some(cmd_line) = cmd_line { - visit(&Operand::Indirect(*cmd_line)); + visit.visit(cmd_line); } for ptr in extern_statics.values().copied() { - let ptr: Pointer> = ptr.into(); - visit(&Operand::Indirect(MemPlace::from_ptr(ptr))); + visit.visit(ptr); } } } diff --git a/src/shims/env.rs b/src/shims/env.rs index ad2d2eaab3..d922014c38 100644 --- a/src/shims/env.rs +++ b/src/shims/env.rs @@ -37,15 +37,14 @@ pub struct EnvVars<'tcx> { } impl VisitMachineValues for EnvVars<'_> { - fn visit_machine_values(&self, visit: &mut impl FnMut(&Operand)) { + fn visit_machine_values(&self, visit: &mut ProvenanceVisitor) { let EnvVars { map, environ } = self; for ptr in map.values() { - visit(&Operand::Indirect(MemPlace::from_ptr(*ptr))); + visit.visit(*ptr); } - if let Some(env) = environ { - visit(&Operand::Indirect(**env)); + visit.visit(**env); } } } diff --git a/src/shims/panic.rs b/src/shims/panic.rs index be14892f69..0d681d3e09 100644 --- a/src/shims/panic.rs +++ b/src/shims/panic.rs @@ -36,10 +36,10 @@ pub struct CatchUnwindData<'tcx> { } impl VisitMachineValues for CatchUnwindData<'_> { - fn visit_machine_values(&self, visit: &mut impl FnMut(&Operand)) { + fn visit_machine_values(&self, visit: &mut ProvenanceVisitor) { let CatchUnwindData { catch_fn, data, dest: _, ret: _ } = self; - visit(&Operand::Indirect(MemPlace::from_ptr(*catch_fn))); - visit(&Operand::Immediate(Immediate::Scalar(*data))); + visit.visit(catch_fn); + visit.visit(data); } } diff --git a/src/shims/tls.rs b/src/shims/tls.rs index d1cee307d7..568eb6fa91 100644 --- a/src/shims/tls.rs +++ b/src/shims/tls.rs @@ -236,14 +236,14 @@ impl<'tcx> TlsData<'tcx> { } impl VisitMachineValues for TlsData<'_> { - fn visit_machine_values(&self, visit: &mut impl FnMut(&Operand)) { + fn visit_machine_values(&self, visit: &mut ProvenanceVisitor) { let TlsData { keys, macos_thread_dtors, next_key: _, dtors_running: _ } = self; for scalar in keys.values().flat_map(|v| v.data.values()) { - visit(&Operand::Immediate(Immediate::Scalar(*scalar))); + visit.visit(scalar); } for (_, scalar) in macos_thread_dtors.values() { - visit(&Operand::Immediate(Immediate::Scalar(*scalar))); + visit.visit(scalar); } } } diff --git a/src/shims/unix/fs.rs b/src/shims/unix/fs.rs index 59d24e00dc..5024b2ab45 100644 --- a/src/shims/unix/fs.rs +++ b/src/shims/unix/fs.rs @@ -463,11 +463,11 @@ impl Default for DirHandler { } impl VisitMachineValues for DirHandler { - fn visit_machine_values(&self, visit: &mut impl FnMut(&Operand)) { + fn visit_machine_values(&self, visit: &mut ProvenanceVisitor) { let DirHandler { streams, next_id: _ } = self; for dir in streams.values() { - visit(&Operand::Indirect(MemPlace::from_ptr(dir.entry))); + visit.visit(dir.entry); } } } diff --git a/src/stacked_borrows/mod.rs b/src/stacked_borrows/mod.rs index ab90e35844..b40358e2c1 100644 --- a/src/stacked_borrows/mod.rs +++ b/src/stacked_borrows/mod.rs @@ -513,10 +513,10 @@ impl Stacks { } } -impl VisitProvenance for Stacks { - fn visit_provenance(&self, visit: &mut impl FnMut(SbTag)) { +impl VisitMachineValues for Stacks { + fn visit_machine_values(&self, visit: &mut ProvenanceVisitor) { for tag in self.exposed_tags.iter().copied() { - visit(tag); + visit.visit(tag); } } } diff --git a/src/tag_gc.rs b/src/tag_gc.rs index 0a8d5d00cf..e2273f055d 100644 --- a/src/tag_gc.rs +++ b/src/tag_gc.rs @@ -3,34 +3,120 @@ use rustc_data_structures::fx::FxHashSet; use crate::*; pub trait VisitMachineValues { - fn visit_machine_values(&self, visit: &mut impl FnMut(&Operand)); + fn visit_machine_values(&self, visit: &mut ProvenanceVisitor); } -pub trait VisitProvenance { - fn visit_provenance(&self, visit: &mut impl FnMut(SbTag)); +pub trait MachineValue { + fn visit_provenance(&self, tags: &mut FxHashSet); +} + +pub struct ProvenanceVisitor { + tags: FxHashSet, +} + +impl ProvenanceVisitor { + pub fn visit(&mut self, v: V) + where + V: MachineValue, + { + v.visit_provenance(&mut self.tags); + } +} + +impl MachineValue for &T { + fn visit_provenance(&self, tags: &mut FxHashSet) { + (**self).visit_provenance(tags); + } +} + +impl MachineValue for Operand { + fn visit_provenance(&self, tags: &mut FxHashSet) { + match self { + Operand::Immediate(Immediate::Scalar(s)) => { + s.visit_provenance(tags); + } + Operand::Immediate(Immediate::ScalarPair(s1, s2)) => { + s1.visit_provenance(tags); + s2.visit_provenance(tags); + } + Operand::Immediate(Immediate::Uninit) => {} + Operand::Indirect(p) => { + p.visit_provenance(tags); + } + } + } +} + +impl MachineValue for Scalar { + fn visit_provenance(&self, tags: &mut FxHashSet) { + if let Scalar::Ptr(ptr, _) = self { + if let Provenance::Concrete { sb, .. } = ptr.provenance { + tags.insert(sb); + } + } + } +} + +impl MachineValue for MemPlace { + fn visit_provenance(&self, tags: &mut FxHashSet) { + if let Some(Provenance::Concrete { sb, .. }) = self.ptr.provenance { + tags.insert(sb); + } + } +} + +impl MachineValue for SbTag { + fn visit_provenance(&self, tags: &mut FxHashSet) { + tags.insert(*self); + } +} + +impl MachineValue for Pointer { + fn visit_provenance(&self, tags: &mut FxHashSet) { + let (prov, _offset) = self.into_parts(); + if let Provenance::Concrete { sb, .. } = prov { + tags.insert(sb); + } + } +} + +impl MachineValue for Pointer> { + fn visit_provenance(&self, tags: &mut FxHashSet) { + let (prov, _offset) = self.into_parts(); + if let Some(Provenance::Concrete { sb, .. }) = prov { + tags.insert(sb); + } + } +} + +impl VisitMachineValues for Allocation { + fn visit_machine_values(&self, visit: &mut ProvenanceVisitor) { + for (_size, prov) in self.provenance().iter() { + if let Provenance::Concrete { sb, .. } = prov { + visit.visit(*sb); + } + } + + self.extra.visit_machine_values(visit); + } } impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriInterpCx<'mir, 'tcx> {} pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> { - /// Generic GC helper to visit everything that can store a value. The `acc` offers some chance to - /// accumulate everything. - fn visit_all_machine_values( - &self, - acc: &mut T, - mut visit_operand: impl FnMut(&mut T, &Operand), - mut visit_alloc: impl FnMut(&mut T, &Allocation), - ) { + /// GC helper to visit everything that can store provenance. The `ProvenanceVisitor` knows how + /// to extract provenance from the interpreter data types. + fn visit_all_machine_values(&self, acc: &mut ProvenanceVisitor) { let this = self.eval_context_ref(); // Memory. this.memory.alloc_map().iter(|it| { for (_id, (_kind, alloc)) in it { - visit_alloc(acc, alloc); + alloc.visit_machine_values(acc); } }); // And all the other machine values. - this.machine.visit_machine_values(&mut |op| visit_operand(acc, op)); + this.machine.visit_machine_values(acc); } fn garbage_collect_tags(&mut self) -> InterpResult<'tcx> { @@ -40,59 +126,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> { return Ok(()); } - let mut tags = FxHashSet::default(); - - let visit_scalar = |tags: &mut FxHashSet, s: &Scalar| { - if let Scalar::Ptr(ptr, _) = s { - if let Provenance::Concrete { sb, .. } = ptr.provenance { - tags.insert(sb); - } - } - }; - - let visit_provenance = |tags: &mut FxHashSet, tag: SbTag| { - tags.insert(tag); - }; - - this.visit_all_machine_values( - &mut tags, - |tags, op| { - match op { - Operand::Immediate(Immediate::Scalar(s)) => { - visit_scalar(tags, s); - } - Operand::Immediate(Immediate::ScalarPair(s1, s2)) => { - visit_scalar(tags, s1); - visit_scalar(tags, s2); - } - Operand::Immediate(Immediate::Uninit) => {} - Operand::Indirect(MemPlace { ptr, .. }) => { - if let Some(Provenance::Concrete { sb, .. }) = ptr.provenance { - tags.insert(sb); - } - } - } - }, - |tags, alloc| { - for (_size, prov) in alloc.provenance().iter() { - if let Provenance::Concrete { sb, .. } = prov { - tags.insert(*sb); - } - } - - let stacks = - alloc.extra.stacked_borrows.as_ref().expect( - "we should not even enter the tag GC if Stacked Borrows is disabled", - ); - stacks.borrow().visit_provenance(&mut |tag| visit_provenance(tags, tag)); - - if let Some(store_buffers) = alloc.extra.weak_memory.as_ref() { - store_buffers.visit_provenance(&mut |tag| visit_provenance(tags, tag)); - } - }, - ); - - self.remove_unreachable_tags(tags); + let mut visitor = ProvenanceVisitor { tags: FxHashSet::default() }; + this.visit_all_machine_values(&mut visitor); + self.remove_unreachable_tags(visitor.tags); Ok(()) } From 8686067bb56241703b77b683adb84f1016c95ae9 Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Mon, 26 Sep 2022 21:48:18 -0400 Subject: [PATCH 5/7] Please help, where is this lifetime bound coming from --- src/concurrency/thread.rs | 16 ++++++------ src/shims/time.rs | 26 +++++++++++++------ src/shims/unix/linux/sync.rs | 38 ++++++++++++++++++++------- src/shims/unix/sync.rs | 50 +++++++++++++++++++++++++----------- 4 files changed, 90 insertions(+), 40 deletions(-) diff --git a/src/concurrency/thread.rs b/src/concurrency/thread.rs index 01ae4320f3..3c5a6786bd 100644 --- a/src/concurrency/thread.rs +++ b/src/concurrency/thread.rs @@ -32,9 +32,9 @@ pub enum SchedulingAction { /// Timeout callbacks can be created by synchronization primitives to tell the /// scheduler that they should be called once some period of time passes. -type TimeoutCallback<'mir, 'tcx> = Box< - dyn FnOnce(&mut InterpCx<'mir, 'tcx, MiriMachine<'mir, 'tcx>>) -> InterpResult<'tcx> + 'tcx, ->; +pub trait TimeoutCallback<'mir, 'tcx>: VisitMachineValues + 'tcx { + fn call(&self, ecx: &mut InterpCx<'mir, 'tcx, MiriMachine<'mir, 'tcx>>) -> InterpResult<'tcx>; +} /// A thread identifier. #[derive(Clone, Copy, Debug, PartialOrd, Ord, PartialEq, Eq, Hash)] @@ -252,7 +252,7 @@ struct TimeoutCallbackInfo<'mir, 'tcx> { /// The callback should be called no earlier than this time. call_time: Time, /// The called function. - callback: TimeoutCallback<'mir, 'tcx>, + callback: Box>, } impl<'mir, 'tcx> std::fmt::Debug for TimeoutCallbackInfo<'mir, 'tcx> { @@ -542,7 +542,7 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { &mut self, thread: ThreadId, call_time: Time, - callback: TimeoutCallback<'mir, 'tcx>, + callback: Box>, ) { self.timeout_callbacks .try_insert(thread, TimeoutCallbackInfo { call_time, callback }) @@ -558,7 +558,7 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { fn get_ready_callback( &mut self, clock: &Clock, - ) -> Option<(ThreadId, TimeoutCallback<'mir, 'tcx>)> { + ) -> Option<(ThreadId, Box>)> { // We iterate over all threads in the order of their indices because // this allows us to have a deterministic scheduler. for thread in self.threads.indices() { @@ -931,7 +931,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { &mut self, thread: ThreadId, call_time: Time, - callback: TimeoutCallback<'mir, 'tcx>, + callback: Box>, ) { let this = self.eval_context_mut(); if !this.machine.communicate() && matches!(call_time, Time::RealTime(..)) { @@ -970,7 +970,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { // 2. Make the scheduler the only place that can change the active // thread. let old_thread = this.set_active_thread(thread); - callback(this)?; + callback.call(this)?; this.set_active_thread(old_thread); Ok(()) } diff --git a/src/shims/time.rs b/src/shims/time.rs index 24fe524539..43792024e2 100644 --- a/src/shims/time.rs +++ b/src/shims/time.rs @@ -1,5 +1,6 @@ use std::time::{Duration, SystemTime}; +use crate::concurrency::thread::TimeoutCallback; use crate::*; /// Returns the time elapsed between the provided time and the unix epoch as a `Duration`. @@ -218,10 +219,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { this.register_timeout_callback( active_thread, Time::Monotonic(timeout_time), - Box::new(move |ecx| { - ecx.unblock_thread(active_thread); - Ok(()) - }), + Box::new(Callback { active_thread }), ); Ok(0) @@ -244,12 +242,24 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { this.register_timeout_callback( active_thread, Time::Monotonic(timeout_time), - Box::new(move |ecx| { - ecx.unblock_thread(active_thread); - Ok(()) - }), + Box::new(Callback { active_thread }), ); Ok(()) } } + +struct Callback { + active_thread: ThreadId, +} + +impl VisitMachineValues for Callback { + fn visit_machine_values(&self, _visit: &mut ProvenanceVisitor) {} +} + +impl<'mir, 'tcx: 'mir> TimeoutCallback<'mir, 'tcx> for Callback { + fn call(&self, ecx: &mut MiriInterpCx<'mir, 'tcx>) -> InterpResult<'tcx> { + ecx.unblock_thread(self.active_thread); + Ok(()) + } +} diff --git a/src/shims/unix/linux/sync.rs b/src/shims/unix/linux/sync.rs index 5a6ce28d25..784fa12d18 100644 --- a/src/shims/unix/linux/sync.rs +++ b/src/shims/unix/linux/sync.rs @@ -1,4 +1,4 @@ -use crate::concurrency::thread::Time; +use crate::concurrency::thread::{Time, TimeoutCallback}; use crate::*; use rustc_target::abi::{Align, Size}; use std::time::SystemTime; @@ -193,14 +193,7 @@ pub fn futex<'tcx>( this.register_timeout_callback( thread, timeout_time, - Box::new(move |this| { - this.unblock_thread(thread); - this.futex_remove_waiter(addr_usize, thread); - let etimedout = this.eval_libc("ETIMEDOUT")?; - this.set_last_error(etimedout)?; - this.write_scalar(Scalar::from_machine_isize(-1, this), &dest)?; - Ok(()) - }), + Box::new(Callback { thread, addr_usize, dest }), ); } } else { @@ -259,3 +252,30 @@ pub fn futex<'tcx>( Ok(()) } + +struct Callback<'tcx> { + thread: ThreadId, + addr_usize: u64, + dest: PlaceTy<'tcx, Provenance>, +} + +impl<'tcx> VisitMachineValues for Callback<'tcx> { + fn visit_machine_values(&self, visit: &mut ProvenanceVisitor) { + let Callback { thread: _, addr_usize: _, dest } = self; + if let Place::Ptr(place) = **dest { + visit.visit(place); + } + } +} + +impl<'mir, 'tcx: 'mir> TimeoutCallback<'mir, 'tcx> for Callback<'tcx> { + fn call(&self, this: &mut MiriInterpCx<'mir, 'tcx>) -> InterpResult<'tcx> { + this.unblock_thread(self.thread); + this.futex_remove_waiter(self.addr_usize, self.thread); + let etimedout = this.eval_libc("ETIMEDOUT")?; + this.set_last_error(etimedout)?; + this.write_scalar(Scalar::from_machine_isize(-1, this), &self.dest)?; + + Ok(()) + } +} diff --git a/src/shims/unix/sync.rs b/src/shims/unix/sync.rs index 2e972a27ff..cdb3cdc4b9 100644 --- a/src/shims/unix/sync.rs +++ b/src/shims/unix/sync.rs @@ -3,7 +3,7 @@ use std::time::SystemTime; use rustc_hir::LangItem; use rustc_middle::ty::{layout::TyAndLayout, query::TyCtxtAt, Ty}; -use crate::concurrency::thread::Time; +use crate::concurrency::thread::{Time, TimeoutCallback}; use crate::*; // pthread_mutexattr_t is either 4 or 8 bytes, depending on the platform. @@ -856,20 +856,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { this.register_timeout_callback( active_thread, timeout_time, - Box::new(move |ecx| { - // We are not waiting for the condvar any more, wait for the - // mutex instead. - reacquire_cond_mutex(ecx, active_thread, mutex_id)?; - - // Remove the thread from the conditional variable. - ecx.condvar_remove_waiter(id, active_thread); - - // Set the return value: we timed out. - let etimedout = ecx.eval_libc("ETIMEDOUT")?; - ecx.write_scalar(etimedout, &dest)?; - - Ok(()) - }), + Box::new(Callback { active_thread, mutex_id, id, dest }), ); Ok(()) @@ -898,6 +885,39 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { } } +struct Callback<'tcx> { + active_thread: ThreadId, + mutex_id: MutexId, + id: CondvarId, + dest: PlaceTy<'tcx, Provenance>, +} + +impl<'tcx> VisitMachineValues for Callback<'tcx> { + fn visit_machine_values(&self, visit: &mut ProvenanceVisitor) { + let Callback { active_thread: _, mutex_id: _, id: _, dest } = self; + if let Place::Ptr(place) = **dest { + visit.visit(place); + } + } +} + +impl<'mir, 'tcx: 'mir> TimeoutCallback<'mir, 'tcx> for Callback<'tcx> { + fn call(&self, ecx: &mut MiriInterpCx<'mir, 'tcx>) -> InterpResult<'tcx> { + // We are not waiting for the condvar any more, wait for the + // mutex instead. + reacquire_cond_mutex(ecx, self.active_thread, self.mutex_id)?; + + // Remove the thread from the conditional variable. + ecx.condvar_remove_waiter(self.id, self.active_thread); + + // Set the return value: we timed out. + let etimedout = ecx.eval_libc("ETIMEDOUT")?; + ecx.write_scalar(etimedout, &self.dest)?; + + Ok(()) + } +} + fn layout_of_maybe_uninit<'tcx>(tcx: TyCtxtAt<'tcx>, param: Ty<'tcx>) -> TyAndLayout<'tcx> { let def_id = tcx.require_lang_item(LangItem::MaybeUninit, None); let ty = tcx.bound_type_of(def_id).subst(*tcx, &[param.into()]); From e0a49154e4bcbea074ac974e3c141e9b12bd5c36 Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Tue, 27 Sep 2022 05:40:12 -0400 Subject: [PATCH 6/7] Finish TimeoutCallback --- src/concurrency/thread.rs | 19 +++++++++++-------- src/shims/time.rs | 4 ++-- src/shims/unix/linux/sync.rs | 4 ++-- src/shims/unix/sync.rs | 4 ++-- 4 files changed, 17 insertions(+), 14 deletions(-) diff --git a/src/concurrency/thread.rs b/src/concurrency/thread.rs index 3c5a6786bd..5e6fcbde69 100644 --- a/src/concurrency/thread.rs +++ b/src/concurrency/thread.rs @@ -32,10 +32,12 @@ pub enum SchedulingAction { /// Timeout callbacks can be created by synchronization primitives to tell the /// scheduler that they should be called once some period of time passes. -pub trait TimeoutCallback<'mir, 'tcx>: VisitMachineValues + 'tcx { +pub trait MachineCallback<'mir, 'tcx>: VisitMachineValues { fn call(&self, ecx: &mut InterpCx<'mir, 'tcx, MiriMachine<'mir, 'tcx>>) -> InterpResult<'tcx>; } +type TimeoutCallback<'mir, 'tcx> = Box + 'tcx>; + /// A thread identifier. #[derive(Clone, Copy, Debug, PartialOrd, Ord, PartialEq, Eq, Hash)] pub struct ThreadId(u32); @@ -252,7 +254,7 @@ struct TimeoutCallbackInfo<'mir, 'tcx> { /// The callback should be called no earlier than this time. call_time: Time, /// The called function. - callback: Box>, + callback: TimeoutCallback<'mir, 'tcx>, } impl<'mir, 'tcx> std::fmt::Debug for TimeoutCallbackInfo<'mir, 'tcx> { @@ -303,10 +305,10 @@ impl VisitMachineValues for ThreadManager<'_, '_> { let ThreadManager { threads, thread_local_alloc_ids, + timeout_callbacks, active_thread: _, yield_active_thread: _, sync: _, - timeout_callbacks: _, } = self; for thread in threads { @@ -315,8 +317,9 @@ impl VisitMachineValues for ThreadManager<'_, '_> { for ptr in thread_local_alloc_ids.borrow().values().copied() { visit.visit(ptr); } - // FIXME: Do we need to do something for TimeoutCallback? That's a Box, not sure what - // to do. + for callback in timeout_callbacks.values() { + callback.callback.visit_machine_values(visit); + } } } @@ -542,7 +545,7 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { &mut self, thread: ThreadId, call_time: Time, - callback: Box>, + callback: TimeoutCallback<'mir, 'tcx>, ) { self.timeout_callbacks .try_insert(thread, TimeoutCallbackInfo { call_time, callback }) @@ -558,7 +561,7 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { fn get_ready_callback( &mut self, clock: &Clock, - ) -> Option<(ThreadId, Box>)> { + ) -> Option<(ThreadId, TimeoutCallback<'mir, 'tcx>)> { // We iterate over all threads in the order of their indices because // this allows us to have a deterministic scheduler. for thread in self.threads.indices() { @@ -931,7 +934,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { &mut self, thread: ThreadId, call_time: Time, - callback: Box>, + callback: TimeoutCallback<'mir, 'tcx>, ) { let this = self.eval_context_mut(); if !this.machine.communicate() && matches!(call_time, Time::RealTime(..)) { diff --git a/src/shims/time.rs b/src/shims/time.rs index 43792024e2..05eff3dfd5 100644 --- a/src/shims/time.rs +++ b/src/shims/time.rs @@ -1,6 +1,6 @@ use std::time::{Duration, SystemTime}; -use crate::concurrency::thread::TimeoutCallback; +use crate::concurrency::thread::MachineCallback; use crate::*; /// Returns the time elapsed between the provided time and the unix epoch as a `Duration`. @@ -257,7 +257,7 @@ impl VisitMachineValues for Callback { fn visit_machine_values(&self, _visit: &mut ProvenanceVisitor) {} } -impl<'mir, 'tcx: 'mir> TimeoutCallback<'mir, 'tcx> for Callback { +impl<'mir, 'tcx: 'mir> MachineCallback<'mir, 'tcx> for Callback { fn call(&self, ecx: &mut MiriInterpCx<'mir, 'tcx>) -> InterpResult<'tcx> { ecx.unblock_thread(self.active_thread); Ok(()) diff --git a/src/shims/unix/linux/sync.rs b/src/shims/unix/linux/sync.rs index 784fa12d18..bbfb1c34db 100644 --- a/src/shims/unix/linux/sync.rs +++ b/src/shims/unix/linux/sync.rs @@ -1,4 +1,4 @@ -use crate::concurrency::thread::{Time, TimeoutCallback}; +use crate::concurrency::thread::{MachineCallback, Time}; use crate::*; use rustc_target::abi::{Align, Size}; use std::time::SystemTime; @@ -268,7 +268,7 @@ impl<'tcx> VisitMachineValues for Callback<'tcx> { } } -impl<'mir, 'tcx: 'mir> TimeoutCallback<'mir, 'tcx> for Callback<'tcx> { +impl<'mir, 'tcx: 'mir> MachineCallback<'mir, 'tcx> for Callback<'tcx> { fn call(&self, this: &mut MiriInterpCx<'mir, 'tcx>) -> InterpResult<'tcx> { this.unblock_thread(self.thread); this.futex_remove_waiter(self.addr_usize, self.thread); diff --git a/src/shims/unix/sync.rs b/src/shims/unix/sync.rs index cdb3cdc4b9..72b71ada8e 100644 --- a/src/shims/unix/sync.rs +++ b/src/shims/unix/sync.rs @@ -3,7 +3,7 @@ use std::time::SystemTime; use rustc_hir::LangItem; use rustc_middle::ty::{layout::TyAndLayout, query::TyCtxtAt, Ty}; -use crate::concurrency::thread::{Time, TimeoutCallback}; +use crate::concurrency::thread::{MachineCallback, Time}; use crate::*; // pthread_mutexattr_t is either 4 or 8 bytes, depending on the platform. @@ -901,7 +901,7 @@ impl<'tcx> VisitMachineValues for Callback<'tcx> { } } -impl<'mir, 'tcx: 'mir> TimeoutCallback<'mir, 'tcx> for Callback<'tcx> { +impl<'mir, 'tcx: 'mir> MachineCallback<'mir, 'tcx> for Callback<'tcx> { fn call(&self, ecx: &mut MiriInterpCx<'mir, 'tcx>) -> InterpResult<'tcx> { // We are not waiting for the condvar any more, wait for the // mutex instead. From d2552d28953a860c9a8a8a5ca8b749a145b262d8 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 2 Oct 2022 16:22:53 +0200 Subject: [PATCH 7/7] re-architect the tag visitor traits --- src/concurrency/data_race.rs | 12 +++ src/concurrency/thread.rs | 41 ++++---- src/concurrency/weak_memory.rs | 10 +- src/intptrcast.rs | 6 ++ src/lib.rs | 2 +- src/machine.rs | 93 +++++++++++------- src/shims/env.rs | 10 +- src/shims/panic.rs | 11 ++- src/shims/time.rs | 16 +-- src/shims/tls.rs | 8 +- src/shims/unix/fs.rs | 12 ++- src/shims/unix/linux/sync.rs | 52 +++++----- src/shims/unix/sync.rs | 64 ++++++------ src/stacked_borrows/mod.rs | 19 +++- src/tag_gc.rs | 172 ++++++++++++++++++++------------- 15 files changed, 306 insertions(+), 222 deletions(-) diff --git a/src/concurrency/data_race.rs b/src/concurrency/data_race.rs index 2e54ddaaba..d0fc349f1a 100644 --- a/src/concurrency/data_race.rs +++ b/src/concurrency/data_race.rs @@ -696,6 +696,12 @@ pub struct VClockAlloc { alloc_ranges: RefCell>, } +impl VisitTags for VClockAlloc { + fn visit_tags(&self, _visit: &mut dyn FnMut(SbTag)) { + // No tags here. + } +} + impl VClockAlloc { /// Create a new data-race detector for newly allocated memory. pub fn new_allocation( @@ -1239,6 +1245,12 @@ pub struct GlobalState { pub track_outdated_loads: bool, } +impl VisitTags for GlobalState { + fn visit_tags(&self, _visit: &mut dyn FnMut(SbTag)) { + // We don't have any tags. + } +} + impl GlobalState { /// Create a new global state, setup with just thread-id=0 /// advanced to timestamp = 1. diff --git a/src/concurrency/thread.rs b/src/concurrency/thread.rs index 5e6fcbde69..ec1da4138d 100644 --- a/src/concurrency/thread.rs +++ b/src/concurrency/thread.rs @@ -32,7 +32,7 @@ pub enum SchedulingAction { /// Timeout callbacks can be created by synchronization primitives to tell the /// scheduler that they should be called once some period of time passes. -pub trait MachineCallback<'mir, 'tcx>: VisitMachineValues { +pub trait MachineCallback<'mir, 'tcx>: VisitTags { fn call(&self, ecx: &mut InterpCx<'mir, 'tcx, MiriMachine<'mir, 'tcx>>) -> InterpResult<'tcx>; } @@ -183,25 +183,21 @@ impl<'mir, 'tcx> Thread<'mir, 'tcx> { } } -impl VisitMachineValues for Thread<'_, '_> { - fn visit_machine_values(&self, visit: &mut ProvenanceVisitor) { +impl VisitTags for Thread<'_, '_> { + fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { let Thread { panic_payload, last_error, stack, state: _, thread_name: _, join_status: _ } = self; - if let Some(payload) = panic_payload { - visit.visit(*payload); - } - if let Some(error) = last_error { - visit.visit(**error); - } + panic_payload.visit_tags(visit); + last_error.visit_tags(visit); for frame in stack { - frame.visit_machine_values(visit) + frame.visit_tags(visit) } } } -impl VisitMachineValues for Frame<'_, '_, Provenance, FrameData<'_>> { - fn visit_machine_values(&self, visit: &mut ProvenanceVisitor) { +impl VisitTags for Frame<'_, '_, Provenance, FrameData<'_>> { + fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { let Frame { return_place, locals, @@ -210,21 +206,20 @@ impl VisitMachineValues for Frame<'_, '_, Provenance, FrameData<'_>> { instance: _, return_to_block: _, loc: _, + // There are some private fields we cannot access; they contain no tags. .. } = self; // Return place. - if let Place::Ptr(mplace) = **return_place { - visit.visit(mplace); - } + return_place.visit_tags(visit); // Locals. for local in locals.iter() { if let LocalValue::Live(value) = &local.value { - visit.visit(value); + value.visit_tags(visit); } } - extra.visit_machine_values(visit); + extra.visit_tags(visit); } } @@ -300,8 +295,8 @@ impl<'mir, 'tcx> Default for ThreadManager<'mir, 'tcx> { } } -impl VisitMachineValues for ThreadManager<'_, '_> { - fn visit_machine_values(&self, visit: &mut ProvenanceVisitor) { +impl VisitTags for ThreadManager<'_, '_> { + fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { let ThreadManager { threads, thread_local_alloc_ids, @@ -312,13 +307,13 @@ impl VisitMachineValues for ThreadManager<'_, '_> { } = self; for thread in threads { - thread.visit_machine_values(visit); + thread.visit_tags(visit); } - for ptr in thread_local_alloc_ids.borrow().values().copied() { - visit.visit(ptr); + for ptr in thread_local_alloc_ids.borrow().values() { + ptr.visit_tags(visit); } for callback in timeout_callbacks.values() { - callback.callback.visit_machine_values(visit); + callback.callback.visit_tags(visit); } } } diff --git a/src/concurrency/weak_memory.rs b/src/concurrency/weak_memory.rs index 15c6c8e9c0..9d7a49c0b4 100644 --- a/src/concurrency/weak_memory.rs +++ b/src/concurrency/weak_memory.rs @@ -108,15 +108,15 @@ pub struct StoreBufferAlloc { store_buffers: RefCell>, } -impl VisitMachineValues for StoreBufferAlloc { - fn visit_machine_values(&self, visit: &mut ProvenanceVisitor) { - for val in self - .store_buffers +impl VisitTags for StoreBufferAlloc { + fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { + let Self { store_buffers } = self; + for val in store_buffers .borrow() .iter() .flat_map(|buf| buf.buffer.iter().map(|element| &element.val)) { - visit.visit(val); + val.visit_tags(visit); } } } diff --git a/src/intptrcast.rs b/src/intptrcast.rs index b9e5def8fa..9722b7643e 100644 --- a/src/intptrcast.rs +++ b/src/intptrcast.rs @@ -44,6 +44,12 @@ pub struct GlobalStateInner { provenance_mode: ProvenanceMode, } +impl VisitTags for GlobalStateInner { + fn visit_tags(&self, _visit: &mut dyn FnMut(SbTag)) { + // Nothing to visit here. + } +} + impl GlobalStateInner { pub fn new(config: &MiriConfig) -> Self { GlobalStateInner { diff --git a/src/lib.rs b/src/lib.rs index 245bdc51a8..463feb4dcc 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -112,7 +112,7 @@ pub use crate::range_map::RangeMap; pub use crate::stacked_borrows::{ CallId, EvalContextExt as StackedBorEvalContextExt, Item, Permission, SbTag, Stack, Stacks, }; -pub use crate::tag_gc::{EvalContextExt as _, ProvenanceVisitor, VisitMachineValues}; +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 523aad22aa..20ae908fce 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -63,13 +63,12 @@ impl<'tcx> std::fmt::Debug for FrameData<'tcx> { } } -impl VisitMachineValues for FrameData<'_> { - fn visit_machine_values(&self, visit: &mut ProvenanceVisitor) { - let FrameData { catch_unwind, stacked_borrows: _, timing: _ } = self; +impl VisitTags for FrameData<'_> { + fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { + let FrameData { catch_unwind, stacked_borrows, timing: _ } = self; - if let Some(catch_unwind) = catch_unwind { - catch_unwind.visit_machine_values(visit); - } + catch_unwind.visit_tags(visit); + stacked_borrows.visit_tags(visit); } } @@ -261,17 +260,13 @@ pub struct AllocExtra { pub weak_memory: Option, } -impl VisitMachineValues for AllocExtra { - fn visit_machine_values(&self, visit: &mut ProvenanceVisitor) { - let AllocExtra { stacked_borrows, data_race: _, weak_memory } = self; - - if let Some(stacked_borrows) = stacked_borrows { - stacked_borrows.borrow().visit_machine_values(visit); - } +impl VisitTags for AllocExtra { + fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { + let AllocExtra { stacked_borrows, data_race, weak_memory } = self; - if let Some(weak_memory) = weak_memory { - weak_memory.visit_machine_values(visit); - } + stacked_borrows.visit_tags(visit); + data_race.visit_tags(visit); + weak_memory.visit_tags(visit); } } @@ -615,8 +610,9 @@ impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> { } } -impl VisitMachineValues for MiriMachine<'_, '_> { - fn visit_machine_values(&self, visit: &mut ProvenanceVisitor) { +impl VisitTags for MiriMachine<'_, '_> { + fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { + #[rustfmt::skip] let MiriMachine { threads, tls, @@ -626,25 +622,52 @@ impl VisitMachineValues for MiriMachine<'_, '_> { cmd_line, extern_statics, dir_handler, - .. + stacked_borrows, + data_race, + intptrcast, + file_handler, + tcx: _, + isolated_op: _, + validate: _, + enforce_abi: _, + clock: _, + layouts: _, + static_roots: _, + profiler: _, + string_cache: _, + exported_symbols_cache: _, + panic_on_unsupported: _, + backtrace_style: _, + local_crates: _, + rng: _, + tracked_alloc_ids: _, + check_alignment: _, + cmpxchg_weak_failure_rate: _, + mute_stdout_stderr: _, + weak_memory: _, + preemption_rate: _, + report_progress: _, + basic_block_count: _, + #[cfg(unix)] + external_so_lib: _, + gc_interval: _, + since_gc: _, + num_cpus: _, } = self; - threads.visit_machine_values(visit); - tls.visit_machine_values(visit); - env_vars.visit_machine_values(visit); - dir_handler.visit_machine_values(visit); - - if let Some(argc) = argc { - visit.visit(argc); - } - if let Some(argv) = argv { - visit.visit(argv); - } - if let Some(cmd_line) = cmd_line { - visit.visit(cmd_line); - } - for ptr in extern_statics.values().copied() { - visit.visit(ptr); + 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); + stacked_borrows.visit_tags(visit); + intptrcast.visit_tags(visit); + argc.visit_tags(visit); + argv.visit_tags(visit); + cmd_line.visit_tags(visit); + for ptr in extern_statics.values() { + ptr.visit_tags(visit); } } } diff --git a/src/shims/env.rs b/src/shims/env.rs index d922014c38..076d3878de 100644 --- a/src/shims/env.rs +++ b/src/shims/env.rs @@ -36,15 +36,13 @@ pub struct EnvVars<'tcx> { pub(crate) environ: Option>, } -impl VisitMachineValues for EnvVars<'_> { - fn visit_machine_values(&self, visit: &mut ProvenanceVisitor) { +impl VisitTags for EnvVars<'_> { + fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { let EnvVars { map, environ } = self; + environ.visit_tags(visit); for ptr in map.values() { - visit.visit(*ptr); - } - if let Some(env) = environ { - visit.visit(**env); + ptr.visit_tags(visit); } } } diff --git a/src/shims/panic.rs b/src/shims/panic.rs index 0d681d3e09..698e025961 100644 --- a/src/shims/panic.rs +++ b/src/shims/panic.rs @@ -35,11 +35,12 @@ pub struct CatchUnwindData<'tcx> { ret: mir::BasicBlock, } -impl VisitMachineValues for CatchUnwindData<'_> { - fn visit_machine_values(&self, visit: &mut ProvenanceVisitor) { - let CatchUnwindData { catch_fn, data, dest: _, ret: _ } = self; - visit.visit(catch_fn); - visit.visit(data); +impl VisitTags for CatchUnwindData<'_> { + fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { + let CatchUnwindData { catch_fn, data, dest, ret: _ } = self; + catch_fn.visit_tags(visit); + data.visit_tags(visit); + dest.visit_tags(visit); } } diff --git a/src/shims/time.rs b/src/shims/time.rs index 05eff3dfd5..9f04034e1a 100644 --- a/src/shims/time.rs +++ b/src/shims/time.rs @@ -219,7 +219,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { this.register_timeout_callback( active_thread, Time::Monotonic(timeout_time), - Box::new(Callback { active_thread }), + Box::new(UnblockCallback { thread_to_unblock: active_thread }), ); Ok(0) @@ -242,24 +242,24 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { this.register_timeout_callback( active_thread, Time::Monotonic(timeout_time), - Box::new(Callback { active_thread }), + Box::new(UnblockCallback { thread_to_unblock: active_thread }), ); Ok(()) } } -struct Callback { - active_thread: ThreadId, +struct UnblockCallback { + thread_to_unblock: ThreadId, } -impl VisitMachineValues for Callback { - fn visit_machine_values(&self, _visit: &mut ProvenanceVisitor) {} +impl VisitTags for UnblockCallback { + fn visit_tags(&self, _visit: &mut dyn FnMut(SbTag)) {} } -impl<'mir, 'tcx: 'mir> MachineCallback<'mir, 'tcx> for Callback { +impl<'mir, 'tcx: 'mir> MachineCallback<'mir, 'tcx> for UnblockCallback { fn call(&self, ecx: &mut MiriInterpCx<'mir, 'tcx>) -> InterpResult<'tcx> { - ecx.unblock_thread(self.active_thread); + ecx.unblock_thread(self.thread_to_unblock); Ok(()) } } diff --git a/src/shims/tls.rs b/src/shims/tls.rs index 568eb6fa91..430dedbc17 100644 --- a/src/shims/tls.rs +++ b/src/shims/tls.rs @@ -235,15 +235,15 @@ impl<'tcx> TlsData<'tcx> { } } -impl VisitMachineValues for TlsData<'_> { - fn visit_machine_values(&self, visit: &mut ProvenanceVisitor) { +impl VisitTags for TlsData<'_> { + fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { let TlsData { keys, macos_thread_dtors, next_key: _, dtors_running: _ } = self; for scalar in keys.values().flat_map(|v| v.data.values()) { - visit.visit(scalar); + scalar.visit_tags(visit); } for (_, scalar) in macos_thread_dtors.values() { - visit.visit(scalar); + scalar.visit_tags(visit); } } } diff --git a/src/shims/unix/fs.rs b/src/shims/unix/fs.rs index 5024b2ab45..9713cd9265 100644 --- a/src/shims/unix/fs.rs +++ b/src/shims/unix/fs.rs @@ -256,6 +256,12 @@ pub struct FileHandler { handles: BTreeMap>, } +impl VisitTags for FileHandler { + fn visit_tags(&self, _visit: &mut dyn FnMut(SbTag)) { + // All our FileDescriptor do not have any tags. + } +} + impl FileHandler { pub(crate) fn new(mute_stdout_stderr: bool) -> FileHandler { let mut handles: BTreeMap<_, Box> = BTreeMap::new(); @@ -462,12 +468,12 @@ impl Default for DirHandler { } } -impl VisitMachineValues for DirHandler { - fn visit_machine_values(&self, visit: &mut ProvenanceVisitor) { +impl VisitTags for DirHandler { + fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { let DirHandler { streams, next_id: _ } = self; for dir in streams.values() { - visit.visit(dir.entry); + dir.entry.visit_tags(visit); } } } diff --git a/src/shims/unix/linux/sync.rs b/src/shims/unix/linux/sync.rs index bbfb1c34db..5762ee27b8 100644 --- a/src/shims/unix/linux/sync.rs +++ b/src/shims/unix/linux/sync.rs @@ -189,6 +189,31 @@ pub fn futex<'tcx>( // Register a timeout callback if a timeout was specified. // This callback will override the return value when the timeout triggers. if let Some(timeout_time) = timeout_time { + struct Callback<'tcx> { + thread: ThreadId, + addr_usize: u64, + dest: PlaceTy<'tcx, Provenance>, + } + + impl<'tcx> VisitTags for Callback<'tcx> { + fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { + let Callback { thread: _, addr_usize: _, dest } = self; + dest.visit_tags(visit); + } + } + + impl<'mir, 'tcx: 'mir> MachineCallback<'mir, 'tcx> for Callback<'tcx> { + fn call(&self, this: &mut MiriInterpCx<'mir, 'tcx>) -> InterpResult<'tcx> { + this.unblock_thread(self.thread); + this.futex_remove_waiter(self.addr_usize, self.thread); + let etimedout = this.eval_libc("ETIMEDOUT")?; + this.set_last_error(etimedout)?; + this.write_scalar(Scalar::from_machine_isize(-1, this), &self.dest)?; + + Ok(()) + } + } + let dest = dest.clone(); this.register_timeout_callback( thread, @@ -252,30 +277,3 @@ pub fn futex<'tcx>( Ok(()) } - -struct Callback<'tcx> { - thread: ThreadId, - addr_usize: u64, - dest: PlaceTy<'tcx, Provenance>, -} - -impl<'tcx> VisitMachineValues for Callback<'tcx> { - fn visit_machine_values(&self, visit: &mut ProvenanceVisitor) { - let Callback { thread: _, addr_usize: _, dest } = self; - if let Place::Ptr(place) = **dest { - visit.visit(place); - } - } -} - -impl<'mir, 'tcx: 'mir> MachineCallback<'mir, 'tcx> for Callback<'tcx> { - fn call(&self, this: &mut MiriInterpCx<'mir, 'tcx>) -> InterpResult<'tcx> { - this.unblock_thread(self.thread); - this.futex_remove_waiter(self.addr_usize, self.thread); - let etimedout = this.eval_libc("ETIMEDOUT")?; - this.set_last_error(etimedout)?; - this.write_scalar(Scalar::from_machine_isize(-1, this), &self.dest)?; - - Ok(()) - } -} diff --git a/src/shims/unix/sync.rs b/src/shims/unix/sync.rs index 72b71ada8e..5aafe76ade 100644 --- a/src/shims/unix/sync.rs +++ b/src/shims/unix/sync.rs @@ -851,6 +851,37 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { // We return success for now and override it in the timeout callback. this.write_scalar(Scalar::from_i32(0), dest)?; + struct Callback<'tcx> { + active_thread: ThreadId, + mutex_id: MutexId, + id: CondvarId, + dest: PlaceTy<'tcx, Provenance>, + } + + impl<'tcx> VisitTags for Callback<'tcx> { + fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { + let Callback { active_thread: _, mutex_id: _, id: _, dest } = self; + dest.visit_tags(visit); + } + } + + impl<'mir, 'tcx: 'mir> MachineCallback<'mir, 'tcx> for Callback<'tcx> { + fn call(&self, ecx: &mut MiriInterpCx<'mir, 'tcx>) -> InterpResult<'tcx> { + // We are not waiting for the condvar any more, wait for the + // mutex instead. + reacquire_cond_mutex(ecx, self.active_thread, self.mutex_id)?; + + // Remove the thread from the conditional variable. + ecx.condvar_remove_waiter(self.id, self.active_thread); + + // Set the return value: we timed out. + let etimedout = ecx.eval_libc("ETIMEDOUT")?; + ecx.write_scalar(etimedout, &self.dest)?; + + Ok(()) + } + } + // Register the timeout callback. let dest = dest.clone(); this.register_timeout_callback( @@ -885,39 +916,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { } } -struct Callback<'tcx> { - active_thread: ThreadId, - mutex_id: MutexId, - id: CondvarId, - dest: PlaceTy<'tcx, Provenance>, -} - -impl<'tcx> VisitMachineValues for Callback<'tcx> { - fn visit_machine_values(&self, visit: &mut ProvenanceVisitor) { - let Callback { active_thread: _, mutex_id: _, id: _, dest } = self; - if let Place::Ptr(place) = **dest { - visit.visit(place); - } - } -} - -impl<'mir, 'tcx: 'mir> MachineCallback<'mir, 'tcx> for Callback<'tcx> { - fn call(&self, ecx: &mut MiriInterpCx<'mir, 'tcx>) -> InterpResult<'tcx> { - // We are not waiting for the condvar any more, wait for the - // mutex instead. - reacquire_cond_mutex(ecx, self.active_thread, self.mutex_id)?; - - // Remove the thread from the conditional variable. - ecx.condvar_remove_waiter(self.id, self.active_thread); - - // Set the return value: we timed out. - let etimedout = ecx.eval_libc("ETIMEDOUT")?; - ecx.write_scalar(etimedout, &self.dest)?; - - Ok(()) - } -} - fn layout_of_maybe_uninit<'tcx>(tcx: TyCtxtAt<'tcx>, param: Ty<'tcx>) -> TyAndLayout<'tcx> { let def_id = tcx.require_lang_item(LangItem::MaybeUninit, None); let ty = tcx.bound_type_of(def_id).subst(*tcx, &[param.into()]); diff --git a/src/stacked_borrows/mod.rs b/src/stacked_borrows/mod.rs index b40358e2c1..829703d656 100644 --- a/src/stacked_borrows/mod.rs +++ b/src/stacked_borrows/mod.rs @@ -71,6 +71,12 @@ pub struct FrameExtra { protected_tags: SmallVec<[SbTag; 2]>, } +impl VisitTags for FrameExtra { + fn visit_tags(&self, _visit: &mut dyn FnMut(SbTag)) { + // `protected_tags` are fine to GC. + } +} + /// Extra per-allocation state. #[derive(Clone, Debug)] pub struct Stacks { @@ -109,6 +115,13 @@ pub struct GlobalStateInner { retag_fields: bool, } +impl VisitTags for GlobalStateInner { + fn visit_tags(&self, _visit: &mut dyn FnMut(SbTag)) { + // The only candidate is base_ptr_tags, and that does not need visiting since we don't ever + // GC the bottommost tag. + } +} + /// We need interior mutable access to the global state. pub type GlobalState = RefCell; @@ -513,10 +526,10 @@ impl Stacks { } } -impl VisitMachineValues for Stacks { - fn visit_machine_values(&self, visit: &mut ProvenanceVisitor) { +impl VisitTags for Stacks { + fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { for tag in self.exposed_tags.iter().copied() { - visit.visit(tag); + visit(tag); } } } diff --git a/src/tag_gc.rs b/src/tag_gc.rs index e2273f055d..5aa653632f 100644 --- a/src/tag_gc.rs +++ b/src/tag_gc.rs @@ -2,123 +2,155 @@ use rustc_data_structures::fx::FxHashSet; use crate::*; -pub trait VisitMachineValues { - fn visit_machine_values(&self, visit: &mut ProvenanceVisitor); +pub trait VisitTags { + fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)); } -pub trait MachineValue { - fn visit_provenance(&self, tags: &mut FxHashSet); +impl VisitTags for Option { + fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { + if let Some(x) = self { + x.visit_tags(visit); + } + } } -pub struct ProvenanceVisitor { - tags: FxHashSet, +impl VisitTags for std::cell::RefCell { + fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { + self.borrow().visit_tags(visit) + } } -impl ProvenanceVisitor { - pub fn visit(&mut self, v: V) - where - V: MachineValue, - { - v.visit_provenance(&mut self.tags); +impl VisitTags for SbTag { + fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { + visit(*self) } } -impl MachineValue for &T { - fn visit_provenance(&self, tags: &mut FxHashSet) { - (**self).visit_provenance(tags); +impl VisitTags for Provenance { + fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { + if let Provenance::Concrete { sb, .. } = self { + visit(*sb); + } } } -impl MachineValue for Operand { - fn visit_provenance(&self, tags: &mut FxHashSet) { +impl VisitTags for Pointer { + fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { + let (prov, _offset) = self.into_parts(); + prov.visit_tags(visit); + } +} + +impl VisitTags for Pointer> { + fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { + let (prov, _offset) = self.into_parts(); + prov.visit_tags(visit); + } +} + +impl VisitTags for Scalar { + fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { match self { - Operand::Immediate(Immediate::Scalar(s)) => { - s.visit_provenance(tags); - } - Operand::Immediate(Immediate::ScalarPair(s1, s2)) => { - s1.visit_provenance(tags); - s2.visit_provenance(tags); - } - Operand::Immediate(Immediate::Uninit) => {} - Operand::Indirect(p) => { - p.visit_provenance(tags); - } + Scalar::Ptr(ptr, _) => ptr.visit_tags(visit), + Scalar::Int(_) => (), } } } -impl MachineValue for Scalar { - fn visit_provenance(&self, tags: &mut FxHashSet) { - if let Scalar::Ptr(ptr, _) = self { - if let Provenance::Concrete { sb, .. } = ptr.provenance { - tags.insert(sb); +impl VisitTags for Immediate { + fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { + match self { + Immediate::Scalar(s) => { + s.visit_tags(visit); + } + Immediate::ScalarPair(s1, s2) => { + s1.visit_tags(visit); + s2.visit_tags(visit); } + Immediate::Uninit => {} } } } -impl MachineValue for MemPlace { - fn visit_provenance(&self, tags: &mut FxHashSet) { - if let Some(Provenance::Concrete { sb, .. }) = self.ptr.provenance { - tags.insert(sb); +impl VisitTags for MemPlaceMeta { + fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { + match self { + MemPlaceMeta::Meta(m) => m.visit_tags(visit), + MemPlaceMeta::None => {} } } } -impl MachineValue for SbTag { - fn visit_provenance(&self, tags: &mut FxHashSet) { - tags.insert(*self); +impl VisitTags for MemPlace { + fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { + let MemPlace { ptr, meta } = self; + ptr.visit_tags(visit); + meta.visit_tags(visit); } } -impl MachineValue for Pointer { - fn visit_provenance(&self, tags: &mut FxHashSet) { - let (prov, _offset) = self.into_parts(); - if let Provenance::Concrete { sb, .. } = prov { - tags.insert(sb); +impl VisitTags for MPlaceTy<'_, Provenance> { + fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { + (**self).visit_tags(visit) + } +} + +impl VisitTags for Place { + fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { + match self { + Place::Ptr(p) => p.visit_tags(visit), + Place::Local { .. } => { + // Will be visited as part of the stack frame. + } } } } -impl MachineValue for Pointer> { - fn visit_provenance(&self, tags: &mut FxHashSet) { - let (prov, _offset) = self.into_parts(); - if let Some(Provenance::Concrete { sb, .. }) = prov { - tags.insert(sb); +impl VisitTags for PlaceTy<'_, Provenance> { + fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { + (**self).visit_tags(visit) + } +} + +impl VisitTags for Operand { + fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { + match self { + Operand::Immediate(imm) => { + imm.visit_tags(visit); + } + Operand::Indirect(p) => { + p.visit_tags(visit); + } } } } -impl VisitMachineValues for Allocation { - fn visit_machine_values(&self, visit: &mut ProvenanceVisitor) { +impl VisitTags for Allocation { + fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { for (_size, prov) in self.provenance().iter() { - if let Provenance::Concrete { sb, .. } = prov { - visit.visit(*sb); - } + prov.visit_tags(visit); } - self.extra.visit_machine_values(visit); + self.extra.visit_tags(visit); } } -impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriInterpCx<'mir, 'tcx> {} -pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> { - /// GC helper to visit everything that can store provenance. The `ProvenanceVisitor` knows how - /// to extract provenance from the interpreter data types. - fn visit_all_machine_values(&self, acc: &mut ProvenanceVisitor) { - let this = self.eval_context_ref(); - +impl VisitTags for crate::MiriInterpCx<'_, '_> { + fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { // Memory. - this.memory.alloc_map().iter(|it| { + self.memory.alloc_map().iter(|it| { for (_id, (_kind, alloc)) in it { - alloc.visit_machine_values(acc); + alloc.visit_tags(visit); } }); // And all the other machine values. - this.machine.visit_machine_values(acc); + 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. @@ -126,9 +158,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> { return Ok(()); } - let mut visitor = ProvenanceVisitor { tags: FxHashSet::default() }; - this.visit_all_machine_values(&mut visitor); - self.remove_unreachable_tags(visitor.tags); + let mut tags = FxHashSet::default(); + this.visit_tags(&mut |tag| { + tags.insert(tag); + }); + self.remove_unreachable_tags(tags); Ok(()) }