From 3b04648bf6510c823f486ecde861ded2ba0a6841 Mon Sep 17 00:00:00 2001 From: Kunshan Wang Date: Fri, 11 Oct 2024 11:29:07 +0800 Subject: [PATCH] Fix a race between forwarding bits and VO bits. The current code sets the forwarding bits before setting the VO bit when copying an object. If another GC worker is attempting to forward the same object, it may observe the forwarding bits being `FORWARDED` but the VO bit is not set. This violates the semantics of VO bits because VO bits should be set for both from-space and to-space copies. This will affect VM bindings that assert slots always refer to a valid object when scanning objects and may update the same slot multiple times for some reasons. This revision provides a mechanism to ensure that all necessary metadata are set before setting forwarding bits to `FORWARDED`. Currently it affects the VO bits and the mark bits (which are used to update the VO bits in Immix-based plans). It may be used for other metadata introduced in the future. --- src/policy/copyspace.rs | 7 ++++--- src/policy/immix/immixspace.rs | 18 +++++++++--------- src/util/object_forwarding.rs | 18 ++++++++++++++++++ 3 files changed, 31 insertions(+), 12 deletions(-) diff --git a/src/policy/copyspace.rs b/src/policy/copyspace.rs index c2c9b35e15..6b30f80117 100644 --- a/src/policy/copyspace.rs +++ b/src/policy/copyspace.rs @@ -252,11 +252,12 @@ impl CopySpace { object, semantics.unwrap(), worker.get_copy_context_mut(), + |_new_object| { + #[cfg(feature = "vo_bit")] + crate::util::metadata::vo_bit::set_vo_bit(_new_object); + }, ); - #[cfg(feature = "vo_bit")] - crate::util::metadata::vo_bit::set_vo_bit(new_object); - trace!("Forwarding pointer"); queue.enqueue(new_object); trace!("Copied [{:?} -> {:?}]", object, new_object); diff --git a/src/policy/immix/immixspace.rs b/src/policy/immix/immixspace.rs index bc24e3c960..6ebd9c255e 100644 --- a/src/policy/immix/immixspace.rs +++ b/src/policy/immix/immixspace.rs @@ -660,15 +660,15 @@ impl ImmixSpace { // We are forwarding objects. When the copy allocator allocates the block, it should // mark the block. So we do not need to explicitly mark it here. - // Clippy complains if the "vo_bit" feature is not enabled. - #[allow(clippy::let_and_return)] - let new_object = - object_forwarding::forward_object::(object, semantics, copy_context); - - #[cfg(feature = "vo_bit")] - vo_bit::helper::on_object_forwarded::(new_object); - - new_object + object_forwarding::forward_object::( + object, + semantics, + copy_context, + |_new_object| { + #[cfg(feature = "vo_bit")] + vo_bit::helper::on_object_forwarded::(_new_object); + }, + ) }; debug_assert_eq!( Block::containing(new_object).get_state(), diff --git a/src/util/object_forwarding.rs b/src/util/object_forwarding.rs index 85c2aaaad5..4b3f2411e9 100644 --- a/src/util/object_forwarding.rs +++ b/src/util/object_forwarding.rs @@ -74,12 +74,30 @@ pub fn spin_and_get_forwarded_object( } } +/// Copy an object and set the forwarding state. +/// +/// The caller can use `on_after_forwarding` to set extra metadata (including VO bits, mark bits, +/// etc.) after the object is copied, but before the forwarding state is changed to `FORWARDED`. The +/// atomic memory operation that sets the forwarding bits to `FORWARDED` has the `SeqCst` order. It +/// will guarantee that if another GC worker thread that attempts to forward the same object sees +/// the forwarding bits being `FORWARDED`, it is guaranteed to see those extra metadata set. +/// +/// Arguments: +/// +/// * `object`: The object to copy. +/// * `semantics`: The copy semantics. +/// * `copy_context`: A reference ot the `CopyContext` instance of the current GC worker. +/// * `on_after_forwarding`: A callback function that is called after `object` is copied, but +/// before the forwarding bits are set. Its argument is a reference to the new copy of +/// `object`. pub fn forward_object( object: ObjectReference, semantics: CopySemantics, copy_context: &mut GCWorkerCopyContext, + on_after_forwarding: impl FnOnce(ObjectReference), ) -> ObjectReference { let new_object = VM::VMObjectModel::copy(object, semantics, copy_context); + on_after_forwarding(new_object); if let Some(shift) = forwarding_bits_offset_in_forwarding_pointer::() { VM::VMObjectModel::LOCAL_FORWARDING_POINTER_SPEC.store_atomic::( object,