From f78523616ea4989148f60c0943bd2c6cae364339 Mon Sep 17 00:00:00 2001 From: Kunshan Wang Date: Fri, 2 Aug 2024 10:18:56 +0800 Subject: [PATCH] Extra assertions for mutators and epilogues (#1182) This commit adds assertions about the number of mutators and the list of mutators returned from the VM binding. Also asserts that epilogues are not silently dropped before they are executed. It could happen if a buggy VM binding returns a number of mutators that does not match the actual list of mutators. This should detect bugs like https://github.com/mmtk/mmtk-ruby/issues/84 --- src/plan/marksweep/global.rs | 4 +++ src/policy/immix/immixspace.rs | 8 +++++- src/policy/marksweepspace/native_ms/global.rs | 14 ++++++++++ src/scheduler/gc_work.rs | 28 +++++++++++++------ src/util/epilogue.rs | 13 +++++++++ src/util/mod.rs | 1 + 6 files changed, 59 insertions(+), 9 deletions(-) create mode 100644 src/util/epilogue.rs diff --git a/src/plan/marksweep/global.rs b/src/plan/marksweep/global.rs index fb6c271cab..4a19f77841 100644 --- a/src/plan/marksweep/global.rs +++ b/src/plan/marksweep/global.rs @@ -65,6 +65,10 @@ impl Plan for MarkSweep { self.common.release(tls, true); } + fn end_of_gc(&mut self, _tls: VMWorkerThread) { + self.ms.end_of_gc(); + } + fn collection_required(&self, space_full: bool, _space: Option>) -> bool { self.base().collection_required(self, space_full) } diff --git a/src/policy/immix/immixspace.rs b/src/policy/immix/immixspace.rs index 73756d2f90..e5be6bbcb4 100644 --- a/src/policy/immix/immixspace.rs +++ b/src/policy/immix/immixspace.rs @@ -9,7 +9,6 @@ use crate::policy::sft_map::SFTMap; use crate::policy::space::{CommonSpace, Space}; use crate::util::alloc::allocator::AllocatorContext; use crate::util::constants::LOG_BYTES_IN_PAGE; -use crate::util::copy::*; use crate::util::heap::chunk_map::*; use crate::util::heap::BlockPageResource; use crate::util::heap::PageResource; @@ -19,6 +18,7 @@ use crate::util::metadata::side_metadata::SideMetadataSpec; use crate::util::metadata::vo_bit; use crate::util::metadata::{self, MetadataSpec}; use crate::util::object_forwarding; +use crate::util::{copy::*, epilogue}; use crate::util::{Address, ObjectReference}; use crate::vm::*; use crate::{ @@ -979,6 +979,12 @@ impl FlushPageResource { } } +impl Drop for FlushPageResource { + fn drop(&mut self) { + epilogue::debug_assert_counter_zero(&self.counter, "FlushPageResource::counter"); + } +} + use crate::policy::copy_context::PolicyCopyContext; use crate::util::alloc::Allocator; use crate::util::alloc::ImmixAllocator; diff --git a/src/policy/marksweepspace/native_ms/global.rs b/src/policy/marksweepspace/native_ms/global.rs index 41773be244..b4699db30c 100644 --- a/src/policy/marksweepspace/native_ms/global.rs +++ b/src/policy/marksweepspace/native_ms/global.rs @@ -8,6 +8,7 @@ use crate::{ scheduler::{GCWorkScheduler, GCWorker, WorkBucketStage}, util::{ copy::CopySemantics, + epilogue, heap::{BlockPageResource, PageResource}, metadata::{self, side_metadata::SideMetadataSpec, MetadataSpec}, ObjectReference, @@ -373,6 +374,13 @@ impl MarkSweepSpace { self.scheduler.work_buckets[crate::scheduler::WorkBucketStage::Release].add(work_packet); } + pub fn end_of_gc(&mut self) { + epilogue::debug_assert_counter_zero( + &self.pending_release_packets, + "pending_release_packets", + ); + } + /// Release a block. pub fn release_block(&self, block: Block) { self.block_clear_metadata(block); @@ -581,3 +589,9 @@ impl RecycleBlocks { } } } + +impl Drop for RecycleBlocks { + fn drop(&mut self) { + epilogue::debug_assert_counter_zero(&self.counter, "RecycleBlocks::counter"); + } +} diff --git a/src/scheduler/gc_work.rs b/src/scheduler/gc_work.rs index 4335ceea16..ff2fb75511 100644 --- a/src/scheduler/gc_work.rs +++ b/src/scheduler/gc_work.rs @@ -60,11 +60,17 @@ impl GCWork for Prepare { plan_mut.prepare(worker.tls); if plan_mut.constraints().needs_prepare_mutator { - for mutator in ::VMActivePlan::mutators() { - mmtk.scheduler.work_buckets[WorkBucketStage::Prepare] - .add(PrepareMutator::::new(mutator)); - } + let prepare_mutator_packets = ::VMActivePlan::mutators() + .map(|mutator| Box::new(PrepareMutator::::new(mutator)) as _) + .collect::>(); + // Just in case the VM binding is inconsistent about the number of mutators and the actual mutator list. + debug_assert_eq!( + prepare_mutator_packets.len(), + ::VMActivePlan::number_of_mutators() + ); + mmtk.scheduler.work_buckets[WorkBucketStage::Prepare].bulk_add(prepare_mutator_packets); } + for w in &mmtk.scheduler.worker_group.workers_shared { let result = w.designated_work.push(Box::new(PrepareCollector)); debug_assert!(result.is_ok()); @@ -133,10 +139,16 @@ impl GCWork for Release { let plan_mut: &mut C::PlanType = unsafe { &mut *(self.plan as *const _ as *mut _) }; plan_mut.release(worker.tls); - for mutator in ::VMActivePlan::mutators() { - mmtk.scheduler.work_buckets[WorkBucketStage::Release] - .add(ReleaseMutator::::new(mutator)); - } + let release_mutator_packets = ::VMActivePlan::mutators() + .map(|mutator| Box::new(ReleaseMutator::::new(mutator)) as _) + .collect::>(); + // Just in case the VM binding is inconsistent about the number of mutators and the actual mutator list. + debug_assert_eq!( + release_mutator_packets.len(), + ::VMActivePlan::number_of_mutators() + ); + mmtk.scheduler.work_buckets[WorkBucketStage::Release].bulk_add(release_mutator_packets); + for w in &mmtk.scheduler.worker_group.workers_shared { let result = w.designated_work.push(Box::new(ReleaseCollector)); debug_assert!(result.is_ok()); diff --git a/src/util/epilogue.rs b/src/util/epilogue.rs new file mode 100644 index 0000000000..b97a88fc74 --- /dev/null +++ b/src/util/epilogue.rs @@ -0,0 +1,13 @@ +//! Utilities for implementing epilogues. +//! +//! Epilogues are operations that are done when several other operations are done. + +use std::sync::atomic::{AtomicUsize, Ordering}; + +/// A debugging method for detecting the case when the epilogue is never executed. +pub fn debug_assert_counter_zero(counter: &AtomicUsize, what: &'static str) { + let value = counter.load(Ordering::SeqCst); + if value != 0 { + panic!("{what} is still {value}."); + } +} diff --git a/src/util/mod.rs b/src/util/mod.rs index 8036ad7ae1..54ccc3ba8e 100644 --- a/src/util/mod.rs +++ b/src/util/mod.rs @@ -44,6 +44,7 @@ pub mod test_util; /// An analysis framework for collecting data and profiling in GC. #[cfg(feature = "analysis")] pub(crate) mod analysis; +pub(crate) mod epilogue; /// Non-generic refs to generic types of ``. pub(crate) mod erase_vm; /// Finalization implementation.