From b59545023461969bb904b8c47b4a3cc0dbd930a5 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Tue, 14 Jul 2026 10:56:30 +0100 Subject: [PATCH 1/4] Fix freeze rollback for incomplete SCCs When freeze() failed or restarted from a pre-freeze hook while an SCC was still pending, rollback treated pending SCC state as if it had already been completed. Finish pending SCC bookkeeping from the DFS stack before undoing freeze work, and explicitly discard any incomplete traversal for the current item on ordinary errors. This also factors shared SCC completion logic, restores rollback for retained weakref references, clears non-GC visited state on rollback, and adds regressions for nested pre-freeze restarts and failed freezes with pending SCCs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Lib/test/test_freeze/test_prefreeze.py | 53 ++++ Python/immutability.c | 343 ++++++++++++++++++------- 2 files changed, 299 insertions(+), 97 deletions(-) diff --git a/Lib/test/test_freeze/test_prefreeze.py b/Lib/test/test_freeze/test_prefreeze.py index 2bf5f492f27c8c..3edcbdba7b23de 100644 --- a/Lib/test/test_freeze/test_prefreeze.py +++ b/Lib/test/test_freeze/test_prefreeze.py @@ -152,5 +152,58 @@ def __pre_freeze__(self): self.assertTrue(is_frozen(a)) self.assertFalse(is_frozen(b)) + def test_nested_freeze_restarts_incomplete_scc(self): + class A: + pass + + class Restart: + def __pre_freeze__(self): + freeze(self) + + a = A() + a.l = [a, Restart()] + l = a.l + + freeze(A) + freeze(a) + + self.assertTrue(is_frozen(a)) + self.assertTrue(is_frozen(l)) + self.assertTrue(is_frozen(l[1])) + + def test_nested_freeze_restart_clears_non_gc_visited(self): + class A: + pass + + class Restart: + def __pre_freeze__(self): + freeze(self) + + a = A() + a.leaf = "unique-nongc-string" + a.restart = Restart() + + freeze(a) + + self.assertTrue(is_frozen(a)) + self.assertTrue(is_frozen(a.restart)) + + def test_failure_rolls_back_incomplete_scc(self): + class A: + pass + + bad = {} + set_freezable(bad, FREEZABLE_NO) + a = A() + a.l = [a, bad] + l = a.l + + with self.assertRaises(TypeError): + freeze(a) + + self.assertFalse(is_frozen(a)) + self.assertFalse(is_frozen(l)) + self.assertFalse(is_frozen(bad)) + if __name__ == "__main__": unittest.main() diff --git a/Python/immutability.c b/Python/immutability.c index 9ca0901159bead..983c37f8db25c7 100644 --- a/Python/immutability.c +++ b/Python/immutability.c @@ -327,6 +327,14 @@ struct FreezeState { // interpreter local immutable state struct FreezeState *enclosing; bool restart; + // Error cleanup state for the item currently being traversed. If traversal + // fails after add_visited(), discard DFS entries above error_dfs_limit and + // undo add_visited() for error_visited_item before rolling back older SCCs. + Py_ssize_t error_dfs_limit; + PyObject *error_visited_item; +#ifdef Py_DEBUG + bool traversing; +#endif #ifdef Py_DEBUG // For debugging, track the stack trace of the freeze operation. PyObject* freeze_location; @@ -441,48 +449,84 @@ is_root(struct FreezeState *state, PyObject *obj) return _Py_hashtable_get(state->roots, obj) != NULL; } +static void deallocate_FreezeState(struct FreezeState *state); + static int init_freeze_state(struct FreezeState *state) { + state->dfs = NULL; + state->pending = NULL; + state->visited = NULL; + state->roots = NULL; + state->completed_sccs = NULL; + state->enclosing = NULL; + state->restart = false; + state->error_dfs_limit = -1; + state->error_visited_item = NULL; +#ifdef Py_DEBUG + state->traversing = false; + state->freeze_location = NULL; +#endif #ifndef GIL_DISABLED state->dfs = PyList_New(0); + if (state->dfs == NULL) { + goto error; + } state->pending = PyList_New(0); + if (state->pending == NULL) { + goto error; + } #endif state->visited = _Py_hashtable_new( _Py_hashtable_hash_ptr, _Py_hashtable_compare_direct); - state->completed_sccs = NULL; + if (state->visited == NULL) { + PyErr_NoMemory(); + goto error; + } state->roots = _Py_hashtable_new( _Py_hashtable_hash_ptr, _Py_hashtable_compare_direct); + if (state->roots == NULL) { + PyErr_NoMemory(); + goto error; + } - state->enclosing = NULL; - state->restart = false; -#ifdef Py_DEBUG - state->freeze_location = NULL; -#endif - - // TODO detect failure? return 0; + +error: + deallocate_FreezeState(state); + return -1; } static void deallocate_FreezeState(struct FreezeState *state) { - _Py_hashtable_destroy(state->visited); - _Py_hashtable_destroy(state->roots); + if (state->visited != NULL) { + _Py_hashtable_destroy(state->visited); + state->visited = NULL; + } + if (state->roots != NULL) { + _Py_hashtable_destroy(state->roots); + state->roots = NULL; + } #ifndef GIL_DISABLED // We can't call the destructor directly as we didn't newref the objects // on push. This is a slow path if there are still objects in the stack, // so there is no need to optimize it. - while(PyList_Size(state->pending) > 0){ - pop(state->pending); + if (state->pending != NULL) { + while(PyList_Size(state->pending) > 0){ + pop(state->pending); + } + Py_DECREF(state->pending); + state->pending = NULL; } - while(PyList_Size(state->dfs) > 0){ - pop(state->dfs); + if (state->dfs != NULL) { + while(PyList_Size(state->dfs) > 0){ + pop(state->dfs); + } + Py_DECREF(state->dfs); + state->dfs = NULL; } - - Py_DECREF(state->dfs); - Py_DECREF(state->pending); #endif } @@ -757,20 +801,6 @@ static void scc_set_refcounts_to_one(PyObject* obj) } while (n != obj); } - -static void scc_reset_root_refcount(PyObject* obj) -{ - assert(scc_root(obj) == obj); - size_t scc_rc = _Py_REFCNT(obj) * 2; - PyObject* n = obj; - do { - PyObject* c = n; - n = scc_next(c); - scc_rc -= _Py_REFCNT(c); - } while (n != obj); - obj->ob_refcnt = scc_rc; -} - // This will restore the reference counts for the interior edges of the SCC. // It calculates some properties of the SCC, to decide how it might be // finalised. Adds an RC to every element in the SCC. @@ -857,26 +887,19 @@ static void scc_return_to_gc(PyObject* obj, bool decref_required) } while (n != obj); } -static void unfreeze(PyObject* obj) +static void undo_weakref_freeze_reference(PyObject *obj) { - // Repr should not be called with an exception set. This can therefore - // only print the memory address of the object - debug("Unfreezing SCC starting at %p\n", obj); - if (scc_next(obj) == NULL) - { - // Clear Immutable flags - _Py_CLEAR_IMMUTABLE(obj); - // Return to the GC. - return_to_gc(obj); + if (!PyWeakref_Check(obj)) { return; } - debug("Unfreezing %p\n", obj); - // Note: We don't need the details of the SCC for a simple unfreeze. - struct SCCDetails scc_details; - scc_reset_root_refcount(obj); - scc_add_internal_refcounts(obj, &scc_details); - scc_make_mutable(obj); - scc_return_to_gc(obj, true); + PyObject *wr = NULL; + PyWeakref_GetRef(obj, &wr); + if (wr != NULL) { + // Drop the temporary reference and the strong reference retained + // when traverse_freeze() followed the weak reference. + Py_DECREF(wr); + Py_DECREF(wr); + } } // Copy-pasted from weakrefobject.c @@ -1235,8 +1258,13 @@ static int add_visited(PyObject* obj, struct FreezeState *state) set_direct_rc(obj); } #endif - if (_Py_hashtable_set(state->visited, obj, obj) == -1) + if (_Py_hashtable_set(state->visited, obj, obj) == -1) { +#ifndef GIL_DISABLED + // This clears the effects of set_direct_rc. + _Py_CLEAR_IMMUTABLE(obj); +#endif return -1; + } return 0; } @@ -1301,8 +1329,44 @@ static void add_internal_reference(PyObject* obj, struct FreezeState *state) assert(_Py_REFCNT(obj) > 0); } +// Dispatch for a postorder marker. complete_scc() does the actual +// refcount conversion when the marker belongs to the current representative. +static void +finish_scc_at_postorder(PyObject *item, struct FreezeState *state) +{ + PyObject* current_scc = peek(state->pending); + if (item == current_scc) + { + debug("Completed an SCC\n"); + pop(state->pending); + debug_obj("Representative: %s (%p)\n", item); + + complete_scc(item, state); + } +} + +// Handle an edge to an object that is already pending: merge the active SCC +// path as needed and subtract that internal edge from the target refcount. +static void +process_pending_internal_edge(PyObject *item, struct FreezeState *state) +{ + PyObject *current_scc = peek(state->pending); + if (current_scc == NULL) { + Py_FatalError("freeze: pending object without pending SCC"); + } + while (union_scc(current_scc, item, state)) { + debug_obj("Representative: %s (%p)\n", current_scc); + pop(state->pending); + current_scc = peek(state->pending); + if (current_scc == NULL) { + Py_FatalError("freeze: SCC union emptied pending stack"); + } + } + add_internal_reference(item, state); +} + /* - Visitor for rollback_completed_scc walk 2. + Visitor for undo_completed_scc walk 2. Re-adds internal reference counts that were subtracted by add_internal_reference during the freeze traversal. The arg is a _Py_hashtable_t* of ring members. @@ -1329,7 +1393,7 @@ static int rollback_refcount_visit(PyObject* obj, void* ring_ht) Walk 2: Re-add internal reference counts via tp_reachable Walk 3: Clear immutability flags + return objects to GC */ -static void rollback_completed_scc(PyObject* obj) +static void undo_completed_scc(PyObject* obj) { debug("Rolling back SCC starting at %p\n", obj); @@ -1353,6 +1417,9 @@ static void rollback_completed_scc(PyObject* obj) _Py_hashtable_set(ring, c, c); if (c != obj) { obj->ob_refcnt -= _Py_REFCNT(c); + // Non-root members still carry their DFS discovery edge. + // Walk 2 re-adds all internal edges, including that one. + c->ob_refcnt--; } count++; } while (n != obj); @@ -1386,6 +1453,7 @@ static void rollback_completed_scc(PyObject* obj) do { PyObject* c = n; n = scc_next(c); + undo_weakref_freeze_reference(c); _Py_CLEAR_IMMUTABLE(c); return_to_gc(c); // clears scc_next and scc_parent, re-tracks in GC } while (n != obj); @@ -1490,6 +1558,7 @@ static int check_freezable(struct _Py_immutability_state *state, PyObject* obj, "Cannot freeze instance of type %s", (obj->ob_type->tp_name)); PyErr_SetObject(PyExc_TypeError, error_msg); + Py_DECREF(error_msg); return -1; } @@ -2029,37 +2098,20 @@ static void make_weakrefs_safe(struct FreezeState* freeze_state) } -/* This undoes a freeze belonging to the given state */ -static void undo_freeze(struct FreezeState* state) { - // Artifact[Implementation]: The function that rolls back immutability on failure - debug("Unfreezing all frozen objects belonging to %p\n", state); - - // Clear dfs stack - while(PyList_Size(state->dfs) > 0){ - pop(state->dfs); - } - - // Clear pending stack - while (PyList_Size(state->pending) > 0) { - PyObject* item = pop(state->pending); - assert(item != NULL); - if (item == PostOrderMarker || item == EnsureVisitedMarker) { - continue; - } - unfreeze(item); - } - - // Unfreeze completed SCCs via intrusive linked list. +static void +undo_completed_freeze_work(struct FreezeState* state) +{ PyObject *scc = state->completed_sccs; while (scc != NULL) { // Read next link before rollback clears _gc_prev. PyObject *next = scc_parent(scc); if (scc_next(scc) == NULL) { // Single-member SCC: just clear flags and return to GC. + undo_weakref_freeze_reference(scc); _Py_CLEAR_IMMUTABLE(scc); return_to_gc(scc); } else { - rollback_completed_scc(scc); + undo_completed_scc(scc); } scc = next; } @@ -2068,6 +2120,92 @@ static void undo_freeze(struct FreezeState* state) { // Clear immutability flags on non-GC visited objects. _Py_hashtable_foreach(state->visited, clear_immutable_visitor, NULL); + _Py_hashtable_clear(state->visited); +} + +static void +discard_unfinished_traversal(struct FreezeState *state) +{ + Py_ssize_t size = state->error_dfs_limit; + if (size < 0) { + return; + } + while (PyList_Size(state->dfs) > size) { + pop(state->dfs); + } + state->error_dfs_limit = -1; +} + +static void +undo_add_visited(struct FreezeState *state) +{ + PyObject *item = state->error_visited_item; + if (item == NULL) { + return; + } + state->error_visited_item = NULL; +#ifndef GIL_DISABLED + if (_PyObject_IS_GC(item)) { + assert(scc_is_pending(item)); + if (peek(state->pending) == item) { + (void)pop(state->pending); + } + _Py_CLEAR_IMMUTABLE(item); + return_to_gc(item); + return; + } +#endif + _Py_CLEAR_IMMUTABLE(item); +} + +static void +finish_pending_sccs_for_undo(struct FreezeState *state) +{ +#ifdef Py_DEBUG + assert(!state->traversing); +#endif + + // Every object still on pending completed its own traversal. The only + // object that can be pending with incomplete traversal is the current + // error-path item, and the error handler removes it before calling here. + // Thus the DFS still holds every not-yet-processed edge needed to finish + // SCC bookkeeping, while unvisited objects can be ignored. + while (PyList_Size(state->dfs) != 0) { + PyObject* item = pop(state->dfs); + + if (item == PostOrderMarker) { + item = pop(state->dfs); + + finish_scc_at_postorder(item, state); + continue; + } + + if (item == EnsureVisitedMarker) { + (void)pop(state->dfs); + continue; + } + + if (has_visited(state, item)) { + if (is_pending(item, state)) { + process_pending_internal_edge(item, state); + } + continue; + } + } + + if (PyList_Size(state->pending) != 0) { + Py_FatalError("freeze rollback: pending SCCs left after draining DFS"); + } +} + +/* This undoes the work of an in-progress freeze operation. */ +static void +undo_partial_freeze(struct FreezeState* state) +{ + // Artifact[Implementation]: The function that rolls back immutability on failure + debug("Unfreezing all frozen objects belonging to %p\n", state); + finish_pending_sccs_for_undo(state); + undo_completed_freeze_work(state); } /* This undoes enclosing freezes and marks them to be restarted */ @@ -2080,7 +2218,7 @@ static void restart_enclosing_freezes(struct _Py_immutability_state* imm_state) freeze_state = freeze_state->enclosing; // Mark all enclosing freezes for restart while (freeze_state) { - undo_freeze(freeze_state); + undo_partial_freeze(freeze_state); freeze_state->restart = true; freeze_state = freeze_state->enclosing; } @@ -2154,6 +2292,12 @@ static int traverse_freeze(PyObject* obj, struct FreezeState* freeze_state) { // WARNING // CHANGES HERE NEED TO BE REFLECTED IN freeze_visit + int result = -1; + +#ifdef Py_DEBUG + assert(!freeze_state->traversing); + freeze_state->traversing = true; +#endif #ifdef MERMAID_TRACING freeze_state->start = obj; @@ -2186,15 +2330,22 @@ static int traverse_freeze(PyObject* obj, struct FreezeState* freeze_state) } if (res == 1) { if (freeze_visit(wr, freeze_state)) { + // freeze_visit() passes wr to push(), which consumes the + // reference even when appending to the DFS stack fails. goto error; } } } - return 0; + result = 0; + goto finally; error: - return -1; +finally: +#ifdef Py_DEBUG + freeze_state->traversing = false; +#endif + return result; } // Mark importlib's mutable state as not freezable. @@ -2269,12 +2420,16 @@ freeze_impl(PyObject *const *objs, Py_ssize_t nobjs) // Initialize the freeze state struct FreezeState freeze_state; - SUCCEEDS(init_freeze_state(&freeze_state)); + if (init_freeze_state(&freeze_state) < 0) { + result = -1; + goto finally; + } // Get Immutable state imm_state = get_immutable_state(); if(imm_state == NULL){ - goto error; + result = -1; + goto finally; } freeze_state.enclosing = imm_state->freeze_stack; imm_state->freeze_stack = &freeze_state; @@ -2354,16 +2509,7 @@ freeze_impl(PyObject *const *objs, Py_ssize_t nobjs) item = pop(freeze_state.dfs); // Have finished traversing graph reachable from item - PyObject* current_scc = peek(freeze_state.pending); - if (item == current_scc) - { - debug("Completed an SCC\n"); - pop(freeze_state.pending); - debug_obj("Representative: %s (%p)\n", item); - - // Completed an SCC do the calculation here. - complete_scc(item, &freeze_state); - } + finish_scc_at_postorder(item, &freeze_state); continue; } @@ -2394,13 +2540,7 @@ freeze_impl(PyObject *const *objs, Py_ssize_t nobjs) debug_obj("Already visited: %s (%p)\n", item); // Check if it is pending. if (is_pending(item, &freeze_state)) { - while (union_scc(peek(freeze_state.pending), item, &freeze_state)) { - debug_obj("Representative: %s (%p)\n", peek(freeze_state.pending)); - pop(freeze_state.pending); - } - // This is an SCC internal edge, we will need to remove - // it from the internal RC count. - add_internal_reference(item, &freeze_state); + process_pending_internal_edge(item, &freeze_state); } continue; } @@ -2424,8 +2564,10 @@ freeze_impl(PyObject *const *objs, Py_ssize_t nobjs) // Add to visited before putting in internal datastructures, so don't have // to account of internal RC manipulations. - add_visited(item, &freeze_state); + SUCCEEDS(add_visited(item, &freeze_state)); + freeze_state.error_dfs_limit = PyList_Size(freeze_state.dfs); + freeze_state.error_visited_item = item; if (_PyObject_IS_GC(item)) { // Add postorder step to dfs. SUCCEEDS(push(freeze_state.dfs, item)); @@ -2437,6 +2579,8 @@ freeze_impl(PyObject *const *objs, Py_ssize_t nobjs) // Traverse the fields of the current object to add to the dfs. SUCCEEDS(traverse_freeze(item, &freeze_state)); + freeze_state.error_dfs_limit = -1; + freeze_state.error_visited_item = NULL; } make_weakrefs_safe(&freeze_state); @@ -2446,7 +2590,12 @@ freeze_impl(PyObject *const *objs, Py_ssize_t nobjs) error: debug("Error during freeze\n"); - undo_freeze(&freeze_state); +#ifdef Py_DEBUG + freeze_state.traversing = false; +#endif + discard_unfinished_traversal(&freeze_state); + undo_add_visited(&freeze_state); + undo_partial_freeze(&freeze_state); result = -1; finally: From abe81024a80838878637673b4669b314c93b393f Mon Sep 17 00:00:00 2001 From: xFrednet Date: Wed, 9 Sep 2026 09:30:31 +0200 Subject: [PATCH 2/4] TRegions: tracing fixes --- Objects/tracingregionobject.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/Objects/tracingregionobject.c b/Objects/tracingregionobject.c index ee8c7c90028c39..7179b86775bf3d 100644 --- a/Objects/tracingregionobject.c +++ b/Objects/tracingregionobject.c @@ -215,6 +215,8 @@ static movable_status get_movable_status(PyObject *obj) { // Cowns are not movable, but the reference is explicitly allowed. if (Cown_Check(obj)) { + // Cowns are frozen on creation, so we just accept the reference. + assert(_Py_IsImmutable(obj)); return Py_MOVABLE_COWN; } @@ -1773,7 +1775,12 @@ static void _region_delete_contents(TracingRegionObject *self) { static int TracingRegion_traverse(TracingRegionObject *self, visitproc visit, void *arg) { - Py_VISIT(self->dict); + // If the region is closed, we know that everything inside the region is reachable. + // There is no advantage of opening the region to double check. This would also + // mess with the GC list of this region. + if (self->open) { + Py_VISIT(self->dict); + } return 0; } From 3a7e6ab301b214fb457a2d9e6ad8a3980429ac84 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Wed, 9 Sep 2026 13:26:57 +0200 Subject: [PATCH 3/4] TRegion: RegionRef's add closed terminal --- Include/internal/pycore_regionref.h | 5 +++- Objects/cownobject.c | 9 +++++++ Objects/weakrefobject.c | 42 ++++++++++++++++++++++------- 3 files changed, 45 insertions(+), 11 deletions(-) diff --git a/Include/internal/pycore_regionref.h b/Include/internal/pycore_regionref.h index acdff81d6df39e..e238a2f4cf9fa6 100644 --- a/Include/internal/pycore_regionref.h +++ b/Include/internal/pycore_regionref.h @@ -31,8 +31,11 @@ typedef enum { /* Terminal. The region is held by `value.cown`, on which the owner is * looked up dynamically. */ _Py_REGION_REF_COWN, + /* Terminal, owned by one interpreter, but hasn't been opened. + this can be restamped */ + _Py_REGION_REF_CLOSED_IPID, /* Terminal, owned by one interpreter. */ - _Py_REGION_REF_IPID, + _Py_REGION_REF_OPEN_IPID, } _PyRegionRefKind; typedef struct _PyRegionRefMetadata { diff --git a/Objects/cownobject.c b/Objects/cownobject.c index fc3304d78b82ce..7c6cfa652f16d3 100644 --- a/Objects/cownobject.c +++ b/Objects/cownobject.c @@ -107,6 +107,8 @@ static int cown_set_value_unchecked(_PyCownObject* self, PyObject* value) { // The region is moving out of the cown, so its region references answer to // the cown's owner from now on. if (self->value != value && Region_Check(self->value)) { + // FIXME(regions): If the cown is released this sets the released owner, + // not what we want _PyTracingRegion_SetMetaOwner(self->value, cown_get_owner(self)); } @@ -307,6 +309,13 @@ static int PyCown_clear(_PyCownObject *self) { /* Tears the cown down. Only the interpreter owning the cown may run this, see * `cown_handoff_dealloc`. */ static void cown_dealloc_owned(_PyCownObject *self) { + if (_PyCown_Owner(self) == RELEASED_IPID) { + _PyCown_ipid_t this_ip = _PyCown_ThisInterpreterId(); + // This should never fail, since we have the last remaining instance + int res = cown_lock(self, -1, this_ip, true); + assert(res >= 0); + } + // Clearing hands the region off, so no region reference points here any more. PyCown_clear(self); PyObject_GC_Del(self); diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 0ba2119ed92992..b9e2a6fde8f4ea 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -189,7 +189,7 @@ meta_decref_lock_held(_PyRegionRefMetadata *meta) static _PyRegionRefMetadata * meta_new_local_lock_held(void) { - _PyRegionRefMetadata *meta = meta_new_lock_held(_Py_REGION_REF_IPID); + _PyRegionRefMetadata *meta = meta_new_lock_held(_Py_REGION_REF_OPEN_IPID); if (meta != NULL) { meta->value.ipid = _PyCown_ThisInterpreterId(); } @@ -230,10 +230,18 @@ meta_set_cown_lock_held(_PyRegionRefMetadata *meta, PyObject *cown) } static void -meta_set_ipid_lock_held(_PyRegionRefMetadata *meta, _PyCown_ipid_t ipid) +meta_set_open_ipid_lock_held(_PyRegionRefMetadata *meta, _PyCown_ipid_t ipid) { meta_clear_parent_lock_held(meta); - meta->kind = _Py_REGION_REF_IPID; + meta->kind = _Py_REGION_REF_OPEN_IPID; + meta->value.ipid = ipid; +} + +static void +meta_set_closed_ipid_lock_held(_PyRegionRefMetadata *meta, _PyCown_ipid_t ipid) +{ + meta_clear_parent_lock_held(meta); + meta->kind = _Py_REGION_REF_CLOSED_IPID; meta->value.ipid = ipid; } @@ -295,11 +303,9 @@ _PyRegionRef_MetaSetCown(_PyRegionRefMetadata *meta, PyObject *cown) void _PyRegionRef_MetaSetIpid(_PyRegionRefMetadata *meta, _PyCown_ipid_t ipid) { - // FIXME(regions): `ipid` should always be the current interpreter. It isn't - // for a released cown, or when `PyCown_clear` runs on an interpreter that - // doesn't own the cown; once that is refactored this can assert it. LOCK_REGION_REF_META(); - meta_set_ipid_lock_held(meta, ipid); + assert(ipid == _PyCown_ThisInterpreterId()); + meta_set_closed_ipid_lock_held(meta, ipid); UNLOCK_REGION_REF_META(); } @@ -307,8 +313,10 @@ void _PyRegionRef_MetaRegionOpened(_PyRegionRefMetadata *meta) { LOCK_REGION_REF_META(); + // FIXME(regions): The following assert fails since some metas have a parent meta IDK why + // assert(meta->kind == _Py_REGION_REF_CLOSED_IPID || meta->kind == _Py_REGION_REF_COWN); meta->region = NULL; - meta_set_ipid_lock_held(meta, _PyCown_ThisInterpreterId()); + meta_set_open_ipid_lock_held(meta, _PyCown_ThisInterpreterId()); UNLOCK_REGION_REF_META(); } @@ -317,7 +325,7 @@ _PyRegionRef_MetaResolveWip(_PyRegionRefMetadata *meta) { LOCK_REGION_REF_META(); if (meta->kind == _Py_REGION_REF_WIP) { - meta_set_ipid_lock_held(meta, _PyCown_ThisInterpreterId()); + meta_set_closed_ipid_lock_held(meta, _PyCown_ThisInterpreterId()); } UNLOCK_REGION_REF_META(); } @@ -457,7 +465,21 @@ regionref_check_access(PyWeakReference *self, regionref_open_list_t *regions, case _Py_REGION_REF_WIP: verdict = REGIONREF_DENIED_WIP; break; - case _Py_REGION_REF_IPID: + case _Py_REGION_REF_CLOSED_IPID: + owner = meta->value.ipid; + if (owner != this_ip) { + verdict = REGIONREF_DENIED_COWN; + } + else { + // FIXME(regions): For this to work, we also need to track the TID + // inside meta. This can also be used for `_Py_REGION_REF_OPEN_IPID` + // + // locking_thread = _PyCown_LockingThread(meta->value.cown); + // wrong_thread = locking_thread != _PyCown_UnsetThreadId() + // && locking_thread != _PyCown_ThisThreadId(); + } + break; + case _Py_REGION_REF_OPEN_IPID: owner = meta->value.ipid; if (owner != this_ip) { verdict = REGIONREF_DENIED_IPID; From 62cfaf404572a837ccd3582d17a0969a51e33c2e Mon Sep 17 00:00:00 2001 From: xFrednet Date: Wed, 9 Sep 2026 16:47:55 +0200 Subject: [PATCH 4/4] TRegion: RegionRefs extract region detach and attachment --- Include/internal/pycore_immutability.h | 4 ++ Include/internal/pycore_regionref.h | 1 + Lib/test/test_freeze/test_tracing_region.py | 2 +- Objects/cownobject.c | 37 ++--------- Objects/tracingregionobject.c | 70 ++++++++++++++++++++- Objects/weakrefobject.c | 8 +++ 6 files changed, 88 insertions(+), 34 deletions(-) diff --git a/Include/internal/pycore_immutability.h b/Include/internal/pycore_immutability.h index d883e44c69c726..8175a5452dcd07 100644 --- a/Include/internal/pycore_immutability.h +++ b/Include/internal/pycore_immutability.h @@ -14,6 +14,10 @@ PyAPI_DATA(PyTypeObject) _PyTracingRegion_Type; PyAPI_FUNC(int) _PyTracingRegion_Close(PyObject* region); PyAPI_FUNC(int) _PyTracingRegion_IsClosed(PyObject* region); PyAPI_FUNC(void) _PyTracingRegion_Open(PyObject* region); +PyAPI_FUNC(int) _PyTracingRegion_Detach(PyObject* region); +PyAPI_FUNC(int) _PyTracingRegion_DetachIgnoreRegionRefs(PyObject* region); +PyAPI_FUNC(int) _PyTracingRegion_Attach(PyObject* region, uint64_t ipid, uint64_t tid); +PyAPI_FUNC(int) _PyTracingRegion_AttachIgnoreRegionRefs(PyObject* region); /* Returns the region's metadata node, allocating it if this is the first * region reference the current close has found. Borrowed, and only valid while diff --git a/Include/internal/pycore_regionref.h b/Include/internal/pycore_regionref.h index e238a2f4cf9fa6..a8a33b87a84e15 100644 --- a/Include/internal/pycore_regionref.h +++ b/Include/internal/pycore_regionref.h @@ -71,6 +71,7 @@ extern void _PyRegionRef_MetaSetCown(_PyRegionRefMetadata *meta, PyObject *cown) * be `_PyCown_ReleasedIpid()` to mean nobody owns the region. */ extern void _PyRegionRef_MetaSetIpid(_PyRegionRefMetadata *meta, _PyCown_ipid_t ipid); +extern void _PyRegionRef_MetaSetReleased(_PyRegionRefMetadata *meta); extern void _PyRegionRef_MetaRegionOpened(_PyRegionRefMetadata *meta); extern void _PyRegionRef_MetaResolveWip(_PyRegionRefMetadata *meta); diff --git a/Lib/test/test_freeze/test_tracing_region.py b/Lib/test/test_freeze/test_tracing_region.py index ae1701519cf921..0422c837be5c7f 100644 --- a/Lib/test/test_freeze/test_tracing_region.py +++ b/Lib/test/test_freeze/test_tracing_region.py @@ -209,7 +209,7 @@ def test_bridge_refs_keep_region_closed(self): self.assertEqual( str(cm.exception), - "the cown couldn't be released, due to the bridge having incoming references") + "the region couldn't be detached, due to incoming references to the bridge") # The release should succeed once all refs have been killed del r1 diff --git a/Objects/cownobject.c b/Objects/cownobject.c index 7c6cfa652f16d3..b6e8f968b47077 100644 --- a/Objects/cownobject.c +++ b/Objects/cownobject.c @@ -215,8 +215,9 @@ static int cown_lock(_PyCownObject* self, PyTime_t timeout, _PyCown_ipid_t locki has_gil ? _PyCown_ThisThreadId() : UNSET_THREAD_ID); if (self->value && Region_Check(self->value)) { - assert(!PyObject_GC_IsTracked(self->value)); - PyObject_GC_Track(self->value); + if (_PyTracingRegion_AttachIgnoreRegionRefs(self->value)) { + return COWN_ACQUIRE_ERROR; + } } return COWN_ACQUIRE_SUCCESS; @@ -309,7 +310,7 @@ static int PyCown_clear(_PyCownObject *self) { /* Tears the cown down. Only the interpreter owning the cown may run this, see * `cown_handoff_dealloc`. */ static void cown_dealloc_owned(_PyCownObject *self) { - if (_PyCown_Owner(self) == RELEASED_IPID) { + if (_PyCown_Owner(_PyObject_CAST(self)) == RELEASED_IPID) { _PyCown_ipid_t this_ip = _PyCown_ThisInterpreterId(); // This should never fail, since we have the last remaining instance int res = cown_lock(self, -1, this_ip, true); @@ -499,34 +500,6 @@ static int cown_check_owner_before_release(_PyCownObject *self, _PyCown_ipid_t u return 0; } -/* This attempts to close the region - * - * It returns non-zero if the closing failed - */ -static int cown_close_region(_PyCownObject *self) { - assert(Region_Check(self->value)); - - // Close the region - int closing_res = _PyTracingRegion_Close(self->value); - if (closing_res < 0) { - return -1; - } - - // Make sure that the cown owns the only external reference to the bridge object. - if (Py_REFCNT(self->value) > 1) { - PyErr_Format( - PyExc_RuntimeError, - "the cown couldn't be released, due to the bridge having incoming references"); - return -1; - } - - // The region is closed and this is the only owner of the bridge. We untrack - // from the current GC list. - PyObject_GC_UnTrack(self->value); - - return 0; -} - static int cown_release(_PyCownObject *self, _PyCown_ipid_t unlocking_ip) { if (cown_check_owner_before_release(self, unlocking_ip) < 0) { return -1; @@ -539,7 +512,7 @@ static int cown_release(_PyCownObject *self, _PyCown_ipid_t unlocking_ip) { assert(Region_Check(self->value)); // The contained region needs to be closed, to allow the cown to release - if (cown_close_region(self)) { + if (_PyTracingRegion_DetachIgnoreRegionRefs(self->value)) { return -1; } diff --git a/Objects/tracingregionobject.c b/Objects/tracingregionobject.c index 7179b86775bf3d..716a85e35ce06a 100644 --- a/Objects/tracingregionobject.c +++ b/Objects/tracingregionobject.c @@ -17,7 +17,7 @@ * graph to. The graph is not written when the variable is unset or empty. */ #define REGION_GRAPH_ENV_VAR "PYTHON_REGION_GRAPH" -#define REGION_TRACING +// #define REGION_TRACING #ifdef REGION_TRACING #define dbg(msg, ...) \ @@ -1978,3 +1978,71 @@ PyTypeObject _PyTracingRegion_Type = { .tp_finalize = TracingRegion_finalize, .tp_reachable = _PyObject_ReachableVisitTypeAndTraverse, }; + +/// This attempts to detach the region from the current interpreter and thread. +/// +/// Raises an exception and returns -1 if it couldn't be detached. +int _PyTracingRegion_DetachIgnoreRegionRefs(PyObject* region) { + assert(Region_Check(region)); + + // Close the region + int closing_res = _PyTracingRegion_Close(region); + if (closing_res < 0) { + return -1; + } + + // Make sure that the cown owns the only external reference to the bridge object. + if (Py_REFCNT(region) > 1) { + PyErr_Format( + PyExc_RuntimeError, + "the region couldn't be detached, due to incoming references to the bridge"); + return -1; + } + + // The region is closed and this is the only owner of the bridge. We untrack + // from the current GC list. + PyObject_GC_UnTrack(region); + + return 0; +} + +/// This attempts to detach the region from the current interpreter and thread. +/// +/// Raises an exception and returns -1 if it couldn't be detached. +int _PyTracingRegion_Detach(PyObject* region) { + TracingRegionObject *self = (TracingRegionObject*)region; + + if (_PyTracingRegion_DetachIgnoreRegionRefs(region)) { + return -1; + } + + // This is safe, assuming the region references respect the thread ID, + // as that one prevents other threads and IPs from opening the chain under foot. + if (self->meta != NULL) { + _PyRegionRef_MetaSetReleased(self->meta); + } + + return 0; +} + +int _PyTracingRegion_AttachIgnoreRegionRefs(PyObject* region) { + assert(Region_Check(region)); + assert(!PyObject_GC_IsTracked(region)); + PyObject_GC_Track(region); + return 0; +} + +int _PyTracingRegion_Attach(PyObject* region, uint64_t ipid, uint64_t tid) { + TracingRegionObject *self = (TracingRegionObject*)region; + + if (_PyTracingRegion_AttachIgnoreRegionRefs(region)) { + return -1; + } + + if (self->meta != NULL) { + _PyRegionRef_MetaSetIpid(self->meta, ipid); + } + (void)tid; + + return 0; +} diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index b9e2a6fde8f4ea..20f91cf1a4bbb8 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -309,6 +309,14 @@ _PyRegionRef_MetaSetIpid(_PyRegionRefMetadata *meta, _PyCown_ipid_t ipid) UNLOCK_REGION_REF_META(); } +void +_PyRegionRef_MetaSetReleased(_PyRegionRefMetadata *meta) +{ + LOCK_REGION_REF_META(); + meta_set_closed_ipid_lock_held(meta, _PyCown_ReleasedIpid()); + UNLOCK_REGION_REF_META(); +} + void _PyRegionRef_MetaRegionOpened(_PyRegionRefMetadata *meta) {