From 3dbdd7ae1076fb022f483b8b91cf37425dc6f0ee Mon Sep 17 00:00:00 2001
From: Kunal Sareen <kunal.sareen@anu.edu.au>
Date: Thu, 5 May 2022 14:03:03 +1000
Subject: [PATCH] Don't panic if forwarding word is not FORWARDED and add debug
 assertions (#580)

* Don't panic if forwarding word is not FORWARDED and add debug assertions

Some policies (such as Immix) can leave objects inplace and can reset
the forwarding word while tracing (such as when Immix is out of space in
its copy allocators). In such a case, we simply want to return the
current object instead of attempting to read the forwarding pointer.
This commit removes our faulty assumption and assertion and adds further
debug assertions for a sanity check.

Closes #579
---
 src/policy/immix/immixspace.rs | 35 +++++++++++++++++++++++++++++++++-
 src/util/object_forwarding.rs  | 14 +++++++++-----
 2 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/src/policy/immix/immixspace.rs b/src/policy/immix/immixspace.rs
index 3c4802f80e..b550a60a0b 100644
--- a/src/policy/immix/immixspace.rs
+++ b/src/policy/immix/immixspace.rs
@@ -391,11 +391,44 @@ impl<VM: VMBinding> ImmixSpace<VM> {
         debug_assert!(!super::BLOCK_ONLY);
         let forwarding_status = ForwardingWord::attempt_to_forward::<VM>(object);
         if ForwardingWord::state_is_forwarded_or_being_forwarded(forwarding_status) {
-            ForwardingWord::spin_and_get_forwarded_object::<VM>(object, forwarding_status)
+            // We lost the forwarding race as some other thread has set the forwarding word; wait
+            // until the object has been forwarded by the winner. Note that the object may not
+            // necessarily get forwarded since Immix opportunistically moves objects.
+            #[allow(clippy::let_and_return)]
+            let new_object =
+                ForwardingWord::spin_and_get_forwarded_object::<VM>(object, forwarding_status);
+            #[cfg(debug_assertions)]
+            {
+                if new_object == object {
+                    debug_assert!(
+                        self.is_marked(object, self.mark_state) || self.defrag.space_exhausted() || Self::is_pinned(object),
+                        "Forwarded object is the same as original object {} even though it should have been copied",
+                        object,
+                    );
+                } else {
+                    // new_object != object
+                    debug_assert!(
+                        !Block::containing::<VM>(new_object).is_defrag_source(),
+                        "Block {:?} containing forwarded object {} should not be a defragmentation source",
+                        Block::containing::<VM>(new_object),
+                        new_object,
+                    );
+                }
+            }
+            new_object
         } else if self.is_marked(object, self.mark_state) {
+            // We won the forwarding race but the object is already marked so we clear the
+            // forwarding status and return the unmoved object
+            debug_assert!(
+                self.defrag.space_exhausted() || Self::is_pinned(object),
+                "Forwarded object is the same as original object {} even though it should have been copied",
+                object,
+            );
             ForwardingWord::clear_forwarding_bits::<VM>(object);
             object
         } else {
+            // We won the forwarding race; actually forward and copy the object if it is not pinned
+            // and we have sufficient space in our copy allocator
             let new_object = if Self::is_pinned(object) || self.defrag.space_exhausted() {
                 self.attempt_mark(object, self.mark_state);
                 ForwardingWord::clear_forwarding_bits::<VM>(object);
diff --git a/src/util/object_forwarding.rs b/src/util/object_forwarding.rs
index 4b167cb463..1253de8582 100644
--- a/src/util/object_forwarding.rs
+++ b/src/util/object_forwarding.rs
@@ -63,12 +63,16 @@ pub fn spin_and_get_forwarded_object<VM: VMBinding>(
     if forwarding_bits == FORWARDED {
         read_forwarding_pointer::<VM>(object)
     } else {
-        panic!(
-            "Invalid forwarding state 0x{:x} 0x{:x} for object {}",
+        // For some policies (such as Immix), we can have interleaving such that one thread clears
+        // the forwarding word while another thread was stuck spinning in the above loop.
+        // See: https://github.com/mmtk/mmtk-core/issues/579
+        debug_assert!(
+            forwarding_bits == FORWARDING_NOT_TRIGGERED_YET,
+            "Invalid/Corrupted forwarding word {:x} for object {}",
             forwarding_bits,
-            read_forwarding_pointer::<VM>(object),
-            object
-        )
+            object,
+        );
+        object
     }
 }