Skip to content

Commit

Permalink
Fix a race between forwarding bits and VO bits.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
wks committed Oct 11, 2024
1 parent 58b3b35 commit 3b04648
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 12 deletions.
7 changes: 4 additions & 3 deletions src/policy/copyspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -252,11 +252,12 @@ impl<VM: VMBinding> CopySpace<VM> {
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);
Expand Down
18 changes: 9 additions & 9 deletions src/policy/immix/immixspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -660,15 +660,15 @@ impl<VM: VMBinding> ImmixSpace<VM> {
// 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::<VM>(object, semantics, copy_context);

#[cfg(feature = "vo_bit")]
vo_bit::helper::on_object_forwarded::<VM>(new_object);

new_object
object_forwarding::forward_object::<VM>(
object,
semantics,
copy_context,
|_new_object| {
#[cfg(feature = "vo_bit")]
vo_bit::helper::on_object_forwarded::<VM>(_new_object);
},
)
};
debug_assert_eq!(
Block::containing(new_object).get_state(),
Expand Down
18 changes: 18 additions & 0 deletions src/util/object_forwarding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,12 +74,30 @@ pub fn spin_and_get_forwarded_object<VM: VMBinding>(
}
}

/// 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<VM: VMBinding>(
object: ObjectReference,
semantics: CopySemantics,
copy_context: &mut GCWorkerCopyContext<VM>,
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>() {
VM::VMObjectModel::LOCAL_FORWARDING_POINTER_SPEC.store_atomic::<VM, usize>(
object,
Expand Down

0 comments on commit 3b04648

Please sign in to comment.