Fix a race between forwarding bits and VO bits. (#1214)

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

@@ -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);

src/policy/immix/immixspace.rs

@@ -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(),

src/util/object_forwarding.rs

@@ -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,