gh-129236: Use stackpointer in free threaded GC

colesbury · colesbury · commit 1ec055b42842 · 2025-01-23T20:17:00.000Z
The stack pointers in interpreter frames are nearly always valid now, so
use them when visiting each thread's frame. For now, don't collect
objects with deferred references in the rare case that we see a frame
with a NULL stack pointer.
diff --git a/Include/internal/pycore_frame.h b/Include/internal/pycore_frame.h
@@ -159,13 +159,6 @@ static inline void _PyFrame_Copy(_PyInterpreterFrame *src, _PyInterpreterFrame *
     // Don't leave a dangling pointer to the old frame when creating generators
     // and coroutines:
     dest->previous = NULL;
-
-#ifdef Py_GIL_DISABLED
-    PyCodeObject *co = _PyFrame_GetCode(dest);
-    for (int i = stacktop; i < co->co_nlocalsplus + co->co_stacksize; i++) {
-        dest->localsplus[i] = PyStackRef_NULL;
-    }
-#endif
 }
 
 #ifdef Py_GIL_DISABLED
diff --git a/Python/gc_free_threading.c b/Python/gc_free_threading.c
@@ -67,6 +67,10 @@ struct collection_state {
     PyInterpreterState *interp;
     GCState *gcstate;
     _PyGC_Reason reason;
+    // GH-129236: If we see a an active frame without a valid stack pointer,
+    // we can't collect objects with deferred references because we may not
+    // see all references.
+    int skip_deferred_objects;
     Py_ssize_t collected;
     Py_ssize_t uncollectable;
     Py_ssize_t long_lived_total;
@@ -413,9 +417,6 @@ gc_visit_heaps(PyInterpreterState *interp, mi_block_visit_fun *visitor,
 static inline void
 gc_visit_stackref(_PyStackRef stackref)
 {
-    // Note: we MUST check that it is deferred before checking the rest.
-    // Otherwise we might read into invalid memory due to non-deferred references
-    // being dead already.
     if (PyStackRef_IsDeferred(stackref) && !PyStackRef_IsNull(stackref)) {
         PyObject *obj = PyStackRef_AsPyObjectBorrow(stackref);
         if (_PyObject_GC_IS_TRACKED(obj) && !gc_is_frozen(obj)) {
@@ -426,20 +427,26 @@ gc_visit_stackref(_PyStackRef stackref)
 
 // Add 1 to the gc_refs for every deferred reference on each thread's stack.
 static void
-gc_visit_thread_stacks(PyInterpreterState *interp)
+gc_visit_thread_stacks(PyInterpreterState *interp, struct collection_state *state)
 {
     _Py_FOR_EACH_TSTATE_BEGIN(interp, p) {
         for (_PyInterpreterFrame *f = p->current_frame; f != NULL; f = f->previous) {
-            PyObject *executable = PyStackRef_AsPyObjectBorrow(f->f_executable);
-            if (executable == NULL || !PyCode_Check(executable)) {
+            if (f->owner >= FRAME_OWNED_BY_INTERPRETER) {
+                continue;
+            }
+
+            _PyStackRef *top = f->stackpointer;
+            if (top == NULL) {
+                // GH-129236: The stackpointer may be NULL. Skip this frame
+                // and don't collect objects with deferred references.
+                state->skip_deferred_objects = 1;
                 continue;
             }
 
-            PyCodeObject *co = (PyCodeObject *)executable;
-            int max_stack = co->co_nlocalsplus + co->co_stacksize;
             gc_visit_stackref(f->f_executable);
-            for (int i = 0; i < max_stack; i++) {
-                gc_visit_stackref(f->localsplus[i]);
+            while (top != f->localsplus) {
+                --top;
+                gc_visit_stackref(*top);
             }
         }
     }
@@ -519,10 +526,7 @@ gc_abort_mark_alive(PyInterpreterState *interp,
 static int
 gc_visit_stackref_mark_alive(_PyObjectStack *stack, _PyStackRef stackref)
 {
-    // Note: we MUST check that it is deferred before checking the rest.
-    // Otherwise we might read into invalid memory due to non-deferred references
-    // being dead already.
-    if (PyStackRef_IsDeferred(stackref) && !PyStackRef_IsNull(stackref)) {
+    if (!PyStackRef_IsNull(stackref)) {
         PyObject *op = PyStackRef_AsPyObjectBorrow(stackref);
         if (mark_alive_stack_push(op, stack) < 0) {
             return -1;
@@ -534,27 +538,37 @@ gc_visit_stackref_mark_alive(_PyObjectStack *stack, _PyStackRef stackref)
 static int
 gc_visit_thread_stacks_mark_alive(PyInterpreterState *interp, _PyObjectStack *stack)
 {
+    int err = 0;
     _Py_FOR_EACH_TSTATE_BEGIN(interp, p) {
         for (_PyInterpreterFrame *f = p->current_frame; f != NULL; f = f->previous) {
-            PyObject *executable = PyStackRef_AsPyObjectBorrow(f->f_executable);
-            if (executable == NULL || !PyCode_Check(executable)) {
+            if (f->owner >= FRAME_OWNED_BY_INTERPRETER) {
                 continue;
             }
 
-            PyCodeObject *co = (PyCodeObject *)executable;
-            int max_stack = co->co_nlocalsplus + co->co_stacksize;
+            if (f->stackpointer == NULL) {
+                // GH-129236: The stackpointer may be NULL in cases where
+                // the GC is run during a PyStackRef_CLOSE() call. Skip this
+                // frame for now.
+                continue;
+            }
+
+            _PyStackRef *top = f->stackpointer;
             if (gc_visit_stackref_mark_alive(stack, f->f_executable) < 0) {
-                return -1;
+                err = -1;
+                goto exit;
             }
-            for (int i = 0; i < max_stack; i++) {
-                if (gc_visit_stackref_mark_alive(stack, f->localsplus[i]) < 0) {
-                    return -1;
+            while (top != f->localsplus) {
+                --top;
+                if (gc_visit_stackref_mark_alive(stack, *top) < 0) {
+                    err = -1;
+                    goto exit;
                 }
             }
         }
     }
+exit:
     _Py_FOR_EACH_TSTATE_END(interp);
-    return 0;
+    return err;
 }
 #endif // GC_MARK_ALIVE_STACKS
 #endif // GC_ENABLE_MARK_ALIVE
@@ -789,14 +803,22 @@ mark_heap_visitor(const mi_heap_t *heap, const mi_heap_area_t *area,
         return true;
     }
 
-    if (gc_is_alive(op)) {
+    _PyObject_ASSERT_WITH_MSG(op, gc_get_refs(op) >= 0,
+                                  "refcount is too small");
+
+    if (gc_is_alive(op) || !gc_is_unreachable(op)) {
+        // Object was already marked as reachable.
         return true;
     }
 
-    _PyObject_ASSERT_WITH_MSG(op, gc_get_refs(op) >= 0,
-                                  "refcount is too small");
+    // GH-129236: Hack to avoid collect objects with deferred references
+    // if we see a frame without a valid stack pointer.
+    struct collection_state *state = (struct collection_state *)args;
+    if (state->skip_deferred_objects && _PyObject_HasDeferredRefcount(op)) {
+        gc_add_refs(op, 1);
+    }
 
-    if (gc_is_unreachable(op) && gc_get_refs(op) != 0) {
+    if (gc_get_refs(op) != 0) {
         // Object is reachable but currently marked as unreachable.
         // Mark it as reachable and traverse its pointers to find
         // any other object that may be directly reachable from it.
@@ -985,7 +1007,7 @@ deduce_unreachable_heap(PyInterpreterState *interp,
 #endif
 
     // Visit the thread stacks to account for any deferred references.
-    gc_visit_thread_stacks(interp);
+    gc_visit_thread_stacks(interp, state);
 
     // Transitively mark reachable objects by clearing the
     // _PyGC_BITS_UNREACHABLE flag.
