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 acdff81d6df39e..a8a33b87a84e15 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 { @@ -68,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_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/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 fc3304d78b82ce..b6e8f968b47077 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)); } @@ -213,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; @@ -307,6 +310,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(_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); + assert(res >= 0); + } + // Clearing hands the region off, so no region reference points here any more. PyCown_clear(self); PyObject_GC_Del(self); @@ -490,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; @@ -530,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 ee8c7c90028c39..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, ...) \ @@ -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; } @@ -1971,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 0ba2119ed92992..20f91cf1a4bbb8 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,17 @@ _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(); +} + +void +_PyRegionRef_MetaSetReleased(_PyRegionRefMetadata *meta) +{ + LOCK_REGION_REF_META(); + meta_set_closed_ipid_lock_held(meta, _PyCown_ReleasedIpid()); UNLOCK_REGION_REF_META(); } @@ -307,8 +321,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 +333,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 +473,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; 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: