Ensure log bits are correctly maintained (#1169)

This fixes a bug wherein we were erroneously resetting the unlog bits
for objects in the modbuf for full-heap GCs. If we reset the unlog bits
in a full-heap GC, we may end up setting the unlog bit for an object
that actually died which may cause issues for future allocations.

In major GCs, we now bulk clear unlog bits for mark-sweep space and
immix space during the prepare stage and bulk clear them for copyspace
during the release stage. We clear log bits for large objects during the
sweep. We also set the unlog bits for mature objects when tracing. This
ensures that there are no stale unlog bits and that we don't
accidentally
log nursery objects.

This PR also adds debug assertions checking that any object that has
been added to the modbuf is considered mature by MMTk.

Author: k-sareen

src/plan/barriers.rs

@@ -171,13 +171,13 @@ impl<S: BarrierSemantics> ObjectBarrier<S> {
         Self { semantics }
     }
 
-    /// Attepmt to atomically log an object.
+    /// Attempt to atomically log an object.
     /// Returns true if the object is not logged previously.
     fn object_is_unlogged(&self, object: ObjectReference) -> bool {
         unsafe { S::UNLOG_BIT_SPEC.load::<S::VM, u8>(object, None) != 0 }
     }
 
-    /// Attepmt to atomically log an object.
+    /// Attempt to atomically log an object.
     /// Returns true if the object is not logged previously.
     fn log_object(&self, object: ObjectReference) -> bool {
         #[cfg(all(feature = "vo_bit", feature = "extreme_assertions"))]

src/plan/generational/gc_work.rs

@@ -108,22 +108,24 @@ impl<E: ProcessEdgesWork> ProcessModBuf<E> {
 
 impl<E: ProcessEdgesWork> GCWork<E::VM> for ProcessModBuf<E> {
     fn do_work(&mut self, worker: &mut GCWorker<E::VM>, mmtk: &'static MMTK<E::VM>) {
-        // Flip the per-object unlogged bits to "unlogged" state.
-        for obj in &self.modbuf {
-            <E::VM as VMBinding>::VMObjectModel::GLOBAL_LOG_BIT_SPEC.store_atomic::<E::VM, u8>(
-                *obj,
-                1,
-                None,
-                Ordering::SeqCst,
-            );
-        }
-        // scan modbuf only if the current GC is a nursery GC
-        if mmtk
-            .get_plan()
-            .generational()
-            .unwrap()
-            .is_current_gc_nursery()
-        {
+        // Process and scan modbuf only if the current GC is a nursery GC
+        let gen = mmtk.get_plan().generational().unwrap();
+        if gen.is_current_gc_nursery() {
+            // Flip the per-object unlogged bits to "unlogged" state.
+            for obj in &self.modbuf {
+                debug_assert!(
+                    !gen.is_object_in_nursery(*obj),
+                    "{} was logged but is not mature. Dumping process memory maps:\n{}",
+                    *obj,
+                    crate::util::memory::get_process_memory_maps(),
+                );
+                <E::VM as VMBinding>::VMObjectModel::GLOBAL_LOG_BIT_SPEC.store_atomic::<E::VM, u8>(
+                    *obj,
+                    1,
+                    None,
+                    Ordering::SeqCst,
+                );
+            }
             // Scan objects in the modbuf and forward pointers
             let modbuf = std::mem::take(&mut self.modbuf);
             GCWork::do_work(

src/plan/generational/global.rs

@@ -227,6 +227,7 @@ impl<VM: VMBinding> CommonGenPlan<VM> {
         if self.common.get_los().in_space(object) {
             return self.common.get_los().trace_object::<Q>(queue, object);
         }
+
         object
     }
 

src/plan/generational/immix/global.rs

@@ -249,10 +249,8 @@ impl<VM: VMBinding> GenImmix<VM> {
         let immix_space = ImmixSpace::new(
             plan_args.get_space_args("immix_mature", true, false, VMRequest::discontiguous()),
             ImmixSpaceArgs {
-                reset_log_bit_in_major_gc: false,
-                // We don't need to unlog objects at tracing. Instead, we unlog objects at copying.
-                // Any object is moved into the mature space, or is copied inside the mature space. We will unlog it.
-                unlog_object_when_traced: false,
+                // We need to unlog objects at tracing time since we currently clear all log bits during a major GC
+                unlog_object_when_traced: true,
                 // In GenImmix, young objects are not allocated in ImmixSpace directly.
                 #[cfg(feature = "vo_bit")]
                 mixed_age: false,

src/plan/immix/global.rs

@@ -136,7 +136,6 @@ impl<VM: VMBinding> Immix<VM> {
         Self::new_with_args(
             plan_args,
             ImmixSpaceArgs {
-                reset_log_bit_in_major_gc: false,
                 unlog_object_when_traced: false,
                 #[cfg(feature = "vo_bit")]
                 mixed_age: false,

src/plan/marksweep/global.rs

@@ -57,7 +57,7 @@ impl<VM: VMBinding> Plan for MarkSweep<VM> {
 
     fn prepare(&mut self, tls: VMWorkerThread) {
         self.common.prepare(tls, true);
-        self.ms.prepare();
+        self.ms.prepare(true);
     }
 
     fn release(&mut self, tls: VMWorkerThread) {

src/plan/sticky/immix/global.rs

@@ -254,6 +254,7 @@ impl<VM: VMBinding> crate::plan::generational::global::GenerationalPlanExt<VM> f
                 trace!("Immix mature object {}, skip", object);
                 return object;
             } else {
+                // Nursery object
                 let object = if KIND == TRACE_KIND_TRANSITIVE_PIN || KIND == TRACE_KIND_FAST {
                     trace!(
                         "Immix nursery object {} is being traced without moving",
@@ -326,9 +327,6 @@ impl<VM: VMBinding> StickyImmix<VM> {
                 // Every object we trace in full heap GC is a mature object. Thus in both cases,
                 // they should be unlogged.
                 unlog_object_when_traced: true,
-                // In full heap GC, mature objects may die, and their unlogged bit needs to be reset.
-                // Along with the option above, we unlog them again during tracing.
-                reset_log_bit_in_major_gc: true,
                 // In StickyImmix, both young and old objects are allocated in the ImmixSpace.
                 #[cfg(feature = "vo_bit")]
                 mixed_age: true,

src/policy/copyspace.rs

@@ -196,6 +196,12 @@ impl<VM: VMBinding> CopySpace<VM> {
                 side_forwarding_status_table.bzero_metadata(start, size);
             }
 
+            if self.common.needs_log_bit {
+                if let MetadataSpec::OnSide(side) = *VM::VMObjectModel::GLOBAL_LOG_BIT_SPEC {
+                    side.bzero_metadata(start, size);
+                }
+            }
+
             // Clear VO bits because all objects in the space are dead.
             #[cfg(feature = "vo_bit")]
             crate::util::metadata::vo_bit::bzero_vo_bit(start, size);

src/policy/immix/immixspace.rs

@@ -65,14 +65,6 @@ pub struct ImmixSpaceArgs {
     /// (no matter we copy an object or not). So we have to use `PromoteToMature`, and instead
     /// just set the log bit in the space when an object is traced.
     pub unlog_object_when_traced: bool,
-    /// Reset log bit at the start of a major GC.
-    /// Normally we do not need to do this. When immix is used as the mature space,
-    /// any object should be set as unlogged, and that bit does not need to be cleared
-    /// even if the object is dead. But in sticky Immix, the mature object and
-    /// the nursery object are in the same space, we will have to use the
-    /// bit to differentiate them. So we reset all the log bits in major GCs,
-    /// and unlogged the objects when they are traced (alive).
-    pub reset_log_bit_in_major_gc: bool,
     /// Whether this ImmixSpace instance contains both young and old objects.
     /// This affects the updating of valid-object bits.  If some lines or blocks of this ImmixSpace
     /// instance contain young objects, their VO bits need to be updated during this GC.  Currently
@@ -293,7 +285,7 @@ impl<VM: VMBinding> ImmixSpace<VM> {
             Block::LOG_BYTES
         );
 
-        if space_args.unlog_object_when_traced || space_args.reset_log_bit_in_major_gc {
+        if space_args.unlog_object_when_traced {
             assert!(
                 args.constraints.needs_log_bit,
                 "Invalid args when the plan does not use log bit"
@@ -389,6 +381,14 @@ impl<VM: VMBinding> ImmixSpace<VM> {
                 unimplemented!("cyclic mark bits is not supported at the moment");
             }
 
+            if self.common.needs_log_bit {
+                if let MetadataSpec::OnSide(side) = *VM::VMObjectModel::GLOBAL_LOG_BIT_SPEC {
+                    for chunk in self.chunk_map.all_chunks() {
+                        side.bzero_metadata(chunk.start(), Chunk::BYTES);
+                    }
+                }
+            }
+
             // Prepare defrag info
             if super::DEFRAG {
                 self.defrag.prepare(self, plan_stats);
@@ -838,6 +838,7 @@ impl<VM: VMBinding> ImmixSpace<VM> {
 /// A work packet to prepare each block for a major GC.
 /// Performs the action on a range of chunks.
 pub struct PrepareBlockState<VM: VMBinding> {
+    #[allow(dead_code)]
     pub space: &'static ImmixSpace<VM>,
     pub chunk: Chunk,
     pub defrag_threshold: Option<usize>,
@@ -851,17 +852,6 @@ impl<VM: VMBinding> PrepareBlockState<VM> {
         if let MetadataSpec::OnSide(side) = *VM::VMObjectModel::LOCAL_MARK_BIT_SPEC {
             side.bzero_metadata(self.chunk.start(), Chunk::BYTES);
         }
-        if self.space.space_args.reset_log_bit_in_major_gc {
-            if let MetadataSpec::OnSide(side) = *VM::VMObjectModel::GLOBAL_LOG_BIT_SPEC {
-                // We zero all the log bits in major GC, and for every object we trace, we will mark the log bit again.
-                side.bzero_metadata(self.chunk.start(), Chunk::BYTES);
-            } else {
-                // If the log bit is not in side metadata, we cannot bulk zero. We can either
-                // clear the bit for dead objects in major GC, or clear the log bit for new
-                // objects. In either cases, we do not need to set log bit at tracing.
-                unimplemented!("We cannot bulk zero unlogged bit.")
-            }
-        }
     }
 }
 

src/policy/immortalspace.rs

@@ -213,6 +213,15 @@ impl<VM: VMBinding> ImmortalSpace<VM> {
             object
         );
         if self.mark_state.test_and_mark::<VM>(object) {
+            // Set the unlog bit if required
+            if self.common.needs_log_bit {
+                VM::VMObjectModel::GLOBAL_LOG_BIT_SPEC.store_atomic::<VM, u8>(
+                    object,
+                    1,
+                    None,
+                    Ordering::SeqCst,
+                );
+            }
             queue.enqueue(object);
         }
         object

src/policy/largeobjectspace.rs

@@ -269,7 +269,9 @@ impl<VM: VMBinding> LargeObjectSpace<VM> {
                 trace!("LOS object {} is being marked now", object);
                 self.treadmill.copy(object, nursery_object);
                 // We just moved the object out of the logical nursery, mark it as unlogged.
-                if nursery_object && self.common.needs_log_bit {
+                // We also unlog mature objects as their unlog bit may have been unset before the
+                // full-heap GC
+                if self.common.needs_log_bit {
                     VM::VMObjectModel::GLOBAL_LOG_BIT_SPEC
                         .mark_as_unlogged::<VM>(object, Ordering::SeqCst);
                 }
@@ -288,6 +290,10 @@ impl<VM: VMBinding> LargeObjectSpace<VM> {
         let sweep = |object: ObjectReference| {
             #[cfg(feature = "vo_bit")]
             crate::util::metadata::vo_bit::unset_vo_bit(object);
+            // Clear log bits for dead objects to prevent a new nursery object having the unlog bit set
+            if self.common.needs_log_bit {
+                VM::VMObjectModel::GLOBAL_LOG_BIT_SPEC.clear::<VM>(object, Ordering::SeqCst);
+            }
             self.pr
                 .release_pages(get_super_page(object.to_object_start::<VM>()));
         };

src/policy/marksweepspace/malloc_ms/global.rs

@@ -488,7 +488,7 @@ impl<VM: VMBinding> MallocSpace<VM> {
         }
     }
 
-    pub fn prepare(&mut self) {}
+    pub fn prepare(&mut self, _full_heap: bool) {}
 
     pub fn release(&mut self) {
         use crate::scheduler::WorkBucketStage;

src/policy/marksweepspace/native_ms/global.rs

@@ -405,7 +405,15 @@ impl<VM: VMBinding> MarkSweepSpace<VM> {
         self.chunk_map.set(block.chunk(), ChunkState::Allocated);
     }
 
-    pub fn prepare(&mut self) {
+    pub fn prepare(&mut self, full_heap: bool) {
+        if self.common.needs_log_bit && full_heap {
+            if let MetadataSpec::OnSide(side) = *VM::VMObjectModel::GLOBAL_LOG_BIT_SPEC {
+                for chunk in self.chunk_map.all_chunks() {
+                    side.bzero_metadata(chunk.start(), Chunk::BYTES);
+                }
+            }
+        }
+
         #[cfg(debug_assertions)]
         self.abandoned_in_gc.lock().unwrap().assert_empty();
 

src/policy/vmspace.rs

@@ -283,6 +283,17 @@ impl<VM: VMBinding> VMSpace<VM> {
         );
         debug_assert!(self.in_space(object));
         if self.mark_state.test_and_mark::<VM>(object) {
+            // Flip the per-object unlogged bits to "unlogged" state for objects inside the
+            // bootimage
+            #[cfg(feature = "set_unlog_bits_vm_space")]
+            if self.common.needs_log_bit {
+                VM::VMObjectModel::GLOBAL_LOG_BIT_SPEC.store_atomic::<VM, u8>(
+                    object,
+                    1,
+                    None,
+                    Ordering::SeqCst,
+                );
+            }
             queue.enqueue(object);
         }
         object

src/util/metadata/log_bit.rs

@@ -6,6 +6,11 @@ use std::sync::atomic::Ordering;
 use super::MetadataSpec;
 
 impl VMGlobalLogBitSpec {
+    /// Clear the unlog bit to log object (0 means logged)
+    pub fn clear<VM: VMBinding>(&self, object: ObjectReference, order: Ordering) {
+        self.store_atomic::<VM, u8>(object, 0, None, order)
+    }
+
     /// Mark the log bit as unlogged (1 means unlogged)
     pub fn mark_as_unlogged<VM: VMBinding>(&self, object: ObjectReference, order: Ordering) {
         self.store_atomic::<VM, u8>(object, 1, None, order)