From dff2e7384200b37518bfb0c359618f40c104d1c5 Mon Sep 17 00:00:00 2001
From: Mark <markjordanthom@gmail.com>
Date: Sun, 15 Oct 2023 15:14:38 -0600
Subject: [PATCH] more eagerly check for cyclicity of variables in cycle
 detecting stackless iterator (#2121)

---
 src/machine/gc.rs         | 38 ++++++++++++++++++++++----------------
 src/tests/acyclic_term.pl | 12 +++++++++++-
 2 files changed, 33 insertions(+), 17 deletions(-)

diff --git a/src/machine/gc.rs b/src/machine/gc.rs
index 5ffd79200..9b0f958f7 100644
--- a/src/machine/gc.rs
+++ b/src/machine/gc.rs
@@ -12,7 +12,7 @@ pub(crate) trait UnmarkPolicy {
     fn invert_marker(iter: &mut StacklessPreOrderHeapIter<Self>) where Self: Sized;
     fn cycle_detected(&mut self) where Self: Sized;
     fn mark_phase(&self) -> bool;
-    fn var_rooted_cycle(_iter: &mut StacklessPreOrderHeapIter<Self>, _var_loc: usize, _next: usize)
+    fn var_rooted_cycle(_iter: &mut StacklessPreOrderHeapIter<Self>, _next: usize)
     where
         Self: Sized {}
     fn detect_list_tail_cycle(_iter: &mut StacklessPreOrderHeapIter<Self>) where Self: Sized {}
@@ -104,8 +104,8 @@ impl UnmarkPolicy for CycleDetectorUMP {
         }
     }
 
-    fn var_rooted_cycle(iter: &mut StacklessPreOrderHeapIter<Self>, var_loc: usize, next: usize) {
-        if var_loc != next && iter.iter_state.mark_phase && !iter.iter_state.cycle_detected {
+    fn var_rooted_cycle(iter: &mut StacklessPreOrderHeapIter<Self>, next: usize) {
+        if iter.current != next && iter.iter_state.mark_phase && !iter.iter_state.cycle_detected {
             iter.iter_state.cycle_detected = iter.detect_list_cycle(next);
         }
     }
@@ -279,9 +279,13 @@ impl<'a, UMP: UnmarkPolicy> StacklessPreOrderHeapIter<'a, UMP> {
     }
 
     #[inline]
-    fn is_cyclic(&self, var_current: usize, var_next: usize) -> bool {
+    fn is_cyclic(&self, var_current: usize, var_next: usize, var_f: bool) -> bool {
         if self.heap[var_next].is_var() {
-            self.heap[var_next].get_mark_bit() && var_current != var_next
+            // the third conjunct covers the case where var_current
+            // was just unforwarded by forward_var() and so
+            // self.current + 1 == var_current. see acyclic_term#2121
+            // & acyclic_term_30 for examples of how this occurs.
+            self.heap[var_next].get_mark_bit() && var_current != var_next && !var_f
         } else if self.heap[var_next].is_ref() {
             self.heap[var_next].get_mark_bit()
         } else {
@@ -298,15 +302,18 @@ impl<'a, UMP: UnmarkPolicy> StacklessPreOrderHeapIter<'a, UMP> {
                     HeapCellValueTag::AttrVar => {
                         let next = self.next;
                         let current = self.current;
+                        let f = self.heap[self.current].get_forwarding_bit();
+
+                        if self.heap[next as usize].get_mark_bit() == self.iter_state.mark_phase() {
+                            UMP::var_rooted_cycle(self, next as usize);
+                        }
 
                         if let Some(cell) = UMP::forward_attr_var(self) {
-                            if self.is_cyclic(current, next as usize) {
+                            if self.is_cyclic(current, next as usize, f) {
                                 self.iter_state.cycle_detected();
                             }
 
                             return Some(cell);
-                        } else if self.heap[next as usize].get_mark_bit() == self.iter_state.mark_phase() {
-                            UMP::var_rooted_cycle(self, current, next as usize);
                         }
 
                         if self.next < self.heap.len() as u64 {
@@ -319,15 +326,18 @@ impl<'a, UMP: UnmarkPolicy> StacklessPreOrderHeapIter<'a, UMP> {
                     HeapCellValueTag::Var => {
                         let next = self.next;
                         let current = self.current;
+                        let f = self.heap[self.current].get_forwarding_bit();
+
+                        if self.heap[next as usize].get_mark_bit() == self.iter_state.mark_phase() {
+                            UMP::var_rooted_cycle(self, next as usize);
+                        }
 
                         if let Some(cell) = self.forward_var() {
-                            if self.is_cyclic(current, next as usize) {
+                            if self.is_cyclic(current, next as usize, f) {
                                 self.iter_state.cycle_detected();
                             }
 
                             return Some(cell);
-                        } else if self.heap[next as usize].get_mark_bit() == self.iter_state.mark_phase() {
-                            UMP::var_rooted_cycle(self, current, next as usize);
                         }
 
                         if self.next < self.heap.len() as u64 {
@@ -462,11 +472,7 @@ impl<'a, UMP: UnmarkPolicy> StacklessPreOrderHeapIter<'a, UMP> {
                     }
                 }
             } else {
-                if self.heap[self.current].get_tag() == HeapCellValueTag::Lis {
-                    if UMP::list_head_cycle_detecting_backward(self) {
-                        return None;
-                    }
-                } else if self.backward() {
+                if self.backward() {
                     return None;
                 }
             }
diff --git a/src/tests/acyclic_term.pl b/src/tests/acyclic_term.pl
index 047a1c427..e340d09c4 100644
--- a/src/tests/acyclic_term.pl
+++ b/src/tests/acyclic_term.pl
@@ -149,6 +149,11 @@
     acyclic_term(B), acyclic_term(Y)
 )).
 
+test("acyclic_term_30", (
+    A=str(B,B,B), C=str(A,_D,B), acyclic_term(C),
+    acyclic_term(A), acyclic_term(B)
+)).
+
 test("acyclic_term#2111_1", (
     term1(A), \+ acyclic_term(A)
 )).
@@ -189,6 +194,11 @@
     A=[]*A,B=[]*A, \+ acyclic_term(B)
 )).
 
+test("acyclic_term#2121", (
+    A=B*B, C=A*B, acyclic_term(C),
+    acyclic_term(A), acyclic_term(B)
+)).
+
 main :-
     findall(test(Name, Goal), test(Name, Goal), Tests),
     run_tests(Tests, Failed),
@@ -200,7 +210,7 @@
     run_tests_quiet(Tests, Failed),
     (   Failed = [] ->
         format("All tests passed", [])
-    ;   format("Some tests failed: ~w~n", [Failed])
+    ;   format("Some tests failed", [])
     ),
     halt.