From 9c4c42746fd8e6f6ec3518fa84f0706c9e1a989f Mon Sep 17 00:00:00 2001 From: xFrednet Date: Wed, 8 Apr 2026 17:22:55 +0200 Subject: [PATCH 01/24] TRegion: Very basic TRegion type --- Include/internal/pycore_immutability.h | 2 + Lib/immutable.py | 1 + Makefile.pre.in | 1 + Modules/_immutablemodule.c | 8 ++++ Objects/tracingregionobject.c | 57 ++++++++++++++++++++++++++ PCbuild/_freeze_module.vcxproj | 1 + PCbuild/_freeze_module.vcxproj.filters | 3 ++ PCbuild/pythoncore.vcxproj | 1 + PCbuild/pythoncore.vcxproj.filters | 3 ++ 9 files changed, 77 insertions(+) create mode 100644 Objects/tracingregionobject.c diff --git a/Include/internal/pycore_immutability.h b/Include/internal/pycore_immutability.h index 8e4d32b78527a63..4e79803b50fd121 100644 --- a/Include/internal/pycore_immutability.h +++ b/Include/internal/pycore_immutability.h @@ -8,6 +8,8 @@ extern "C" { # error "Py_BUILD_CORE must be defined to include this header" #endif +PyAPI_DATA(PyTypeObject) _PyTracingRegion_Type; + struct _Py_immutability_state { int late_init_done; struct _Py_hashtable_t *shallow_immutable_types; diff --git a/Lib/immutable.py b/Lib/immutable.py index e1c00152f94bbd8..40cce9c93cbefbf 100644 --- a/Lib/immutable.py +++ b/Lib/immutable.py @@ -21,6 +21,7 @@ FREEZABLE_PROXY = _c.FREEZABLE_PROXY InterpreterLocal = _c.InterpreterLocal SharedField = _c.SharedField +TracingRegion = _c.TracingRegion # FIXME(immutable): For the longest time we used the name `isfrozen` # without the underscore. This keeps the function name for now, but diff --git a/Makefile.pre.in b/Makefile.pre.in index 572a784546b60fb..1b55ebe01a2a852 100644 --- a/Makefile.pre.in +++ b/Makefile.pre.in @@ -555,6 +555,7 @@ OBJECT_OBJS= \ Objects/sliceobject.o \ Objects/structseq.o \ Objects/templateobject.o \ + Objects/tracingregionobject.o \ Objects/tupleobject.o \ Objects/typeobject.o \ Objects/typevarobject.o \ diff --git a/Modules/_immutablemodule.c b/Modules/_immutablemodule.c index 94ea460c340526e..7a237c6e1a7a8cd 100644 --- a/Modules/_immutablemodule.c +++ b/Modules/_immutablemodule.c @@ -650,6 +650,14 @@ immutable_exec(PyObject *module) { return -1; } + if (PyModule_AddType(module, &_PyTracingRegion_Type) != 0) { + return -1; + } + if (_PyImmutability_SetFreezable( + (PyObject*)&_PyTracingRegion_Type, _Py_FREEZABLE_YES) < 0) { + return -1; + } + if (PyModule_AddIntConstant(module, "FREEZABLE_YES", _Py_FREEZABLE_YES) != 0) { return -1; diff --git a/Objects/tracingregionobject.c b/Objects/tracingregionobject.c new file mode 100644 index 000000000000000..251cf5a0cb3250c --- /dev/null +++ b/Objects/tracingregionobject.c @@ -0,0 +1,57 @@ +#include "Python.h" +#include "pycore_gc.h" // _PyObject_GC_IS_TRACKED() +#include "pycore_object.h" // _PyObject_GC_TRACK(), _PyDebugAllocatorStats() + +typedef struct { + PyObject_HEAD + PyObject *dict; +} TracingRegionObject; + + +static int +TracingRegion_traverse(TracingRegionObject *self, visitproc visit, void *arg) +{ + Py_VISIT(self->dict); + return 0; +} + +static int +TracingRegion_clear(TracingRegionObject *self) +{ + Py_CLEAR(self->dict); + return 0; +} + +static void +TracingRegion_dealloc(TracingRegionObject *self) +{ + PyObject_GC_UnTrack(self); + TracingRegion_clear(self); + Py_TYPE(self)->tp_free((PyObject *)self); +} + +static int +TracingRegion_init(TracingRegionObject *self, PyObject *args, PyObject *kwds) +{ + return 0; +} + +static PyMemberDef TracingRegion_members[] = { + {"__dict__", _Py_T_OBJECT, offsetof(TracingRegionObject, dict), Py_READONLY}, + {NULL} +}; + +PyTypeObject _PyTracingRegion_Type = { + PyVarObject_HEAD_INIT(NULL, 0) + .tp_name = "TracingRegion", + .tp_basicsize = sizeof(TracingRegionObject), + .tp_dealloc = (destructor)TracingRegion_dealloc, + .tp_flags = Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_GC | Py_TPFLAGS_IMMUTABLETYPE, + .tp_traverse = (traverseproc)TracingRegion_traverse, + .tp_clear = (inquiry)TracingRegion_clear, + .tp_members = TracingRegion_members, + .tp_dictoffset = offsetof(TracingRegionObject, dict), + .tp_init = (initproc)TracingRegion_init, + .tp_new = PyType_GenericNew, + .tp_reachable = _PyObject_ReachableVisitTypeAndTraverse, +}; diff --git a/PCbuild/_freeze_module.vcxproj b/PCbuild/_freeze_module.vcxproj index 3702e4e99987183..c2514ce954c9024 100644 --- a/PCbuild/_freeze_module.vcxproj +++ b/PCbuild/_freeze_module.vcxproj @@ -161,6 +161,7 @@ + diff --git a/PCbuild/_freeze_module.vcxproj.filters b/PCbuild/_freeze_module.vcxproj.filters index 0b968eba5b977bf..ebb3c7a469b443c 100644 --- a/PCbuild/_freeze_module.vcxproj.filters +++ b/PCbuild/_freeze_module.vcxproj.filters @@ -478,6 +478,9 @@ Source Files + + Source Files + Source Files diff --git a/PCbuild/pythoncore.vcxproj b/PCbuild/pythoncore.vcxproj index d6ce53bbea28245..b61f2669d4974d9 100644 --- a/PCbuild/pythoncore.vcxproj +++ b/PCbuild/pythoncore.vcxproj @@ -559,6 +559,7 @@ + diff --git a/PCbuild/pythoncore.vcxproj.filters b/PCbuild/pythoncore.vcxproj.filters index d5351a82741a0fe..61fe05d775a7b58 100644 --- a/PCbuild/pythoncore.vcxproj.filters +++ b/PCbuild/pythoncore.vcxproj.filters @@ -1273,6 +1273,9 @@ Objects + + Objects + Objects From 78e5fe4795ee64b106809034e4eea856f52873d3 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Wed, 8 Apr 2026 18:08:38 +0200 Subject: [PATCH 02/24] Copy and pasta the needed bits --- Objects/tracingregionobject.c | 240 +++++++++++++++++++++++++++++++++- 1 file changed, 234 insertions(+), 6 deletions(-) diff --git a/Objects/tracingregionobject.c b/Objects/tracingregionobject.c index 251cf5a0cb3250c..4b50276bef7214e 100644 --- a/Objects/tracingregionobject.c +++ b/Objects/tracingregionobject.c @@ -1,13 +1,228 @@ #include "Python.h" +#include "pycore_interp.h" #include "pycore_gc.h" // _PyObject_GC_IS_TRACKED() #include "pycore_object.h" // _PyObject_GC_TRACK(), _PyDebugAllocatorStats() +#include "pycore_descrobject.h" + +// #define REGION_TRACING + +#ifdef REGION_TRACING +#define if_trace(...) __VA_ARGS__ +#define trace_arg(arg) , (Py_uintptr_t)(arg) +#define trace(msg, region, ...) \ + do { \ + printf(msg "\n", (Py_region_t)(region) __VA_OPT__(,) __VA_ARGS__); \ + } while(0) +#define trace_lrc(...) trace(__VA_ARGS__) +#else +#define if_trace(...) +#define trace_arg(...) +#define trace(...) +#define trace_lrc(...) +#endif + +/* Macro that jumps to error, if the expression `x` does not succeed. */ +#define SUCCEEDS(x) do { int r = (x); if (r != 0) goto error; } while (0) + +// ################################################################### +// Copied from gc.c +// ################################################################### + +#ifndef Py_GIL_DISABLED +#define GC_NEXT _PyGCHead_NEXT +#define GC_PREV _PyGCHead_PREV + +static inline void +gc_set_old_space(PyGC_Head *g, int space) +{ + assert(space == 0 || space == _PyGC_NEXT_MASK_OLD_SPACE_1); + g->_gc_next &= ~_PyGC_NEXT_MASK_OLD_SPACE_1; + g->_gc_next |= space; +} + +static inline void +gc_list_init(PyGC_Head *list) +{ + // List header must not have flags. + // We can assign pointer by simple cast. + list->_gc_prev = (uintptr_t)list; + list->_gc_next = (uintptr_t)list; +} + +static void +gc_list_move(PyGC_Head *node, PyGC_Head *list) +{ + /* Unlink from current list. */ + PyGC_Head *from_prev = GC_PREV(node); + PyGC_Head *from_next = GC_NEXT(node); + _PyGCHead_SET_NEXT(from_prev, from_next); + _PyGCHead_SET_PREV(from_next, from_prev); + + /* Relink at end of new list. */ + // list must not have flags. So we can skip macros. + PyGC_Head *to_prev = (PyGC_Head*)list->_gc_prev; + _PyGCHead_SET_PREV(node, to_prev); + _PyGCHead_SET_NEXT(to_prev, node); + list->_gc_prev = (uintptr_t)node; + _PyGCHead_SET_NEXT(node, list); +} + +static inline int +gc_list_is_empty(PyGC_Head *list) +{ + return (list->_gc_next == (uintptr_t)list); +} + +static void +gc_list_merge(PyGC_Head *from, PyGC_Head *to) +{ + assert(from != to); + if (!gc_list_is_empty(from)) { + PyGC_Head *to_tail = GC_PREV(to); + PyGC_Head *from_head = GC_NEXT(from); + PyGC_Head *from_tail = GC_PREV(from); + assert(from_head != from); + assert(from_tail != from); + + _PyGCHead_SET_NEXT(to_tail, from_head); + _PyGCHead_SET_PREV(from_head, to_tail); + + _PyGCHead_SET_NEXT(from_tail, to); + _PyGCHead_SET_PREV(to, from_tail); + } + gc_list_init(from); +} + +static struct _gc_runtime_state* +get_gc_state(void) +{ + PyInterpreterState *interp = _PyInterpreterState_GET(); + return &interp->gc; +} + +static inline void +gc_clear_collecting(PyGC_Head *g) +{ + g->_gc_prev &= ~_PyGC_PREV_MASK_COLLECTING; +} + +#elif // Py_GIL_DISABLED +#error "We need GIL" +#endif + +// ################################################################### +// Copied from regions-main +// ################################################################### + +typedef enum { + Py_MOVABLE_YES = 0, + Py_MOVABLE_NO = 1, + Py_MOVABLE_FREEZE = 2, +} movable_status; + +movable_status get_movable_status(PyObject *obj) { + // FIXME(regions): xFrednet: Currently it's not possible to set + // the movability per object. This instead returns the default + // movability for objects. Note that some shallow immutable objects + // will not return freeze as their movability. + + // Immortal object have no real RC, this makes it infeasible to have them + // in a region and dynamically track their ownership. Immortal objects are + // intended to be immutable in Python, so it should be safe to implicitly + // freeze them. + if (_Py_IsImmortal(obj)) { + return Py_MOVABLE_FREEZE; + } + + // Immutable objects don't need to be moved + if (_Py_IsImmutable(obj)) { + return Py_MOVABLE_FREEZE; + } + + // Types are a pain for regions since it's likely that objects of one type may + // end up in multiple regions, requiring the type to be frozen. Types also + // have a lot of reference pointing to them. Let's hope there is no need to + // keep them freezable + if (PyType_Check(obj)) { + return Py_MOVABLE_FREEZE; + } + + // Module objects are also complicated. Freezing them should turn most modules + // into proxys which should make them mostly usable. + if (PyModule_Check(obj)) { + return Py_MOVABLE_FREEZE; + } + + // Functions are a mess as well, making the entire system reachable. Freezing + // them should again just magically make most things work + if (PyFunction_Check(obj)) { + return Py_MOVABLE_FREEZE; + } + + // CWrappers can't really be owned, but need some special handling since + // interpreters could still race on their RC. Solution, throw them in the + // freezer + if (PyCFunction_Check(obj) + || Py_IS_TYPE(obj, &_PyMethodWrapper_Type) + || Py_IS_TYPE(obj, &PyWrapperDescr_Type) + ) { + return Py_MOVABLE_FREEZE; + } + + // Freezing or moving these objects is... complicated. In some cases it is + // possible but more hassle than it's probably worth. For not we mark them + // all as unmovable. + if (PyFrame_Check(obj) + || PyGen_CheckExact(obj) + || PyCoro_CheckExact(obj) + || PyAsyncGen_CheckExact(obj) + || PyAsyncGenASend_CheckExact(obj) + ) { + return Py_MOVABLE_NO; + } + + // Exceptions don't hold anything obviously problematic preventing them + // from being moved into a region. The actual problem is that the runtime + // stores references to them and that these are already emitted on an + // error path. Moving them into a region could add more problems. + // We should discuss how to handle these, maybe freezing is the correct + // approach? + if (PyExceptionInstance_Check(obj)) { + return Py_MOVABLE_NO; + } + + // For now, we define all other objects as movable by default. (Surely + // this will not backfire) + return Py_MOVABLE_YES; +} + +// ################################################################### +// Tracing Impl +// ################################################################### + +typedef struct { + Py_ssize_t objs; + Py_ssize_t incoming_refs; +} trace_res; + +static trace_res trace_object(PyObject* obj) { + trace_res res = { + .objs = 1, + .incoming_refs = 2, + }; + + return res; +} + +// ################################################################### +// Region Object +// ################################################################### typedef struct { PyObject_HEAD PyObject *dict; } TracingRegionObject; - static int TracingRegion_traverse(TracingRegionObject *self, visitproc visit, void *arg) { @@ -30,12 +245,23 @@ TracingRegion_dealloc(TracingRegionObject *self) Py_TYPE(self)->tp_free((PyObject *)self); } -static int -TracingRegion_init(TracingRegionObject *self, PyObject *args, PyObject *kwds) -{ - return 0; +static PyObject* TracingRegion_trace(PyObject *op) { + trace_res res = trace_object(op); + + PyObject *t = Py_BuildValue("(ii)", res.objs, res.incoming_refs); + if (t == NULL) { + return NULL; // propagate Python exception + } + + return t; } +static PyMethodDef TracingRegion_methods[] = { + {"trace", _PyCFunction_CAST(TracingRegion_trace), METH_NOARGS, + "This traces the region and returns the number of incoming references"}, + {NULL, NULL} /* sentinel */ +}; + static PyMemberDef TracingRegion_members[] = { {"__dict__", _Py_T_OBJECT, offsetof(TracingRegionObject, dict), Py_READONLY}, {NULL} @@ -50,8 +276,10 @@ PyTypeObject _PyTracingRegion_Type = { .tp_traverse = (traverseproc)TracingRegion_traverse, .tp_clear = (inquiry)TracingRegion_clear, .tp_members = TracingRegion_members, + .tp_methods = TracingRegion_methods, .tp_dictoffset = offsetof(TracingRegionObject, dict), - .tp_init = (initproc)TracingRegion_init, .tp_new = PyType_GenericNew, .tp_reachable = _PyObject_ReachableVisitTypeAndTraverse, }; + + From 8f11ad1e22dd692c4283ec809016ea170ed41148 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Wed, 15 Apr 2026 14:22:00 +0200 Subject: [PATCH 03/24] TracingRegions a working prototype --- Objects/tracingregionobject.c | 335 ++++++++++++++++++++++++++++++++-- 1 file changed, 317 insertions(+), 18 deletions(-) diff --git a/Objects/tracingregionobject.c b/Objects/tracingregionobject.c index 4b50276bef7214e..a0602f6bb5b60df 100644 --- a/Objects/tracingregionobject.c +++ b/Objects/tracingregionobject.c @@ -9,16 +9,14 @@ #ifdef REGION_TRACING #define if_trace(...) __VA_ARGS__ #define trace_arg(arg) , (Py_uintptr_t)(arg) -#define trace(msg, region, ...) \ +#define trace(msg, ...) \ do { \ - printf(msg "\n", (Py_region_t)(region) __VA_OPT__(,) __VA_ARGS__); \ + printf(msg "\n" __VA_OPT__(,) __VA_ARGS__); \ } while(0) -#define trace_lrc(...) trace(__VA_ARGS__) #else #define if_trace(...) #define trace_arg(...) #define trace(...) -#define trace_lrc(...) #endif /* Macro that jumps to error, if the expression `x` does not succeed. */ @@ -114,6 +112,24 @@ gc_clear_collecting(PyGC_Head *g) // Copied from regions-main // ################################################################### +static PyObject* list_pop(PyObject* s){ + PyObject* item; + Py_ssize_t size = PyList_Size(s); + if(size == 0){ + return NULL; + } + item = PyList_GetItem(s, size - 1); + if(item == NULL){ + return NULL; + } + // This should never fail, since we shrink the size + if(PyList_SetSlice(s, size - 1, size, NULL)){ + Py_DECREF(item); + return NULL; + } + return item; +} + typedef enum { Py_MOVABLE_YES = 0, Py_MOVABLE_NO = 1, @@ -196,24 +212,298 @@ movable_status get_movable_status(PyObject *obj) { return Py_MOVABLE_YES; } +// This uses the given arguments to create and throw a `RegionError` +static void throw_region_error( + const char *format_str, const char *tp_name, + PyObject* src, PyObject* tgt) +{ + // Don't stomp existing exception + PyThreadState *tstate = PyThreadState_Get(); + if (_PyErr_Occurred(tstate)) { + return; + } + + PyErr_Format(PyExc_RuntimeError, format_str, tp_name); + + // Set source and target fields + // Get the current exception (should be a RuntimeError) + PyObject *exc = PyErr_GetRaisedException(); + assert(exc && PyObject_TypeCheck(exc, (PyTypeObject *)PyExc_RuntimeError)); + + // Add 'source' and 'target' attributes to the exception + PyObject_SetAttr(exc, &_Py_ID(source), src ? src : Py_None); + PyObject_SetAttr(exc, &_Py_ID(target), tgt ? tgt : Py_None); + + PyErr_SetRaisedException((PyObject*)exc); +} + +// Wrapper around tp_traverse that also visits the type object. +static int +traverse_via_tp_traverse(PyObject *obj, visitproc visit, void *state) +{ + PyTypeObject *tp = Py_TYPE(obj); + + // Visit the type with traverse + traverseproc traverse = tp->tp_traverse; + if (traverse != NULL) { + int err = traverse(obj, visit, state); + if (err) { + return err; + } + } + + + // Most `tp_traverse` don't visit the type even though they should. + // Here it won't hurt to potentially visit it twice, since types + // are non-movable but will be frozen. + return visit((PyObject *)Py_TYPE(obj), state); +} + +// Returns the appropriate traversal function for reaching all references +// from an object. Prefers tp_reachable, falls back to tp_traverse wrapped +// to also visit the type. Emits a warning once per type on fallback. +static traverseproc +get_reachable_proc(PyTypeObject *tp) +{ + if (tp->tp_reachable != NULL) { + return tp->tp_reachable; + } + + if (tp->tp_traverse != NULL) { + PySys_FormatStderr( + "regions: type '%.100s' has tp_traverse but no tp_reachable\n", + tp->tp_name); + } else { + PySys_FormatStderr( + "regions: type '%.100s' has no tp_traverse and no tp_reachable\n", + tp->tp_name); + } + + // Always return the wrapper; even when tp_traverse is NULL, the wrapper + // will still visit the type object which tp_reachable is expected to do. + return traverse_via_tp_traverse; +} + // ################################################################### // Tracing Impl // ################################################################### +static void +gc_list_dissolve(PyGC_Head *list) { + struct _gc_runtime_state* gc_state = get_gc_state(); + // Use `old[0]` here, we are setting the visited space to 0 in add_visited_set(). + gc_list_merge(list, &(gc_state->old[0].head)); +} + +typedef struct { + /// A list of all visited objects + _Py_hashtable_t *visited; + /// The number of refs coming into this object graph + Py_ssize_t external_rc; + // This is set if an object was frozen and the trace needs to restart to be valid + bool restart; + // The GC list used for this trace + PyGC_Head* gc_list; + // The source of the reference, this is used for error reporting + PyObject *src; + // List of pending objects that are not GC + PyObject *pending; +} trace_state; + +static void trace_state_destroy(trace_state* state, bool dissolve_gc) { + if (state->visited) { + _Py_hashtable_destroy(state->visited); + state->visited = NULL; + } + if (state->pending) { + Py_DECREF(state->pending); + state->pending = NULL; + } + + if (dissolve_gc) { + gc_list_dissolve(state->gc_list); + } +} +static int trace_state_init(trace_state* state, PyGC_Head *gc_list) { + assert(gc_list_is_empty(gc_list)); + + state->visited = NULL; + state->pending = NULL; + + state->visited = _Py_hashtable_new( + _Py_hashtable_hash_ptr, + _Py_hashtable_compare_direct); + if (state->visited == NULL) { + goto error; + } + + state->pending = PyList_New(0); + if (state->pending == NULL) { + goto error; + } + + state->external_rc = 0; + state->restart = false; + state->gc_list = gc_list; + state->src = NULL; + + return 0; +error: + trace_state_destroy(state, false); + return -1; +} + typedef struct { Py_ssize_t objs; Py_ssize_t incoming_refs; -} trace_res; +} trace_result; + +const int TRACE_RES_ERR = -1; +const int TRACE_RES_DONE = 0; +const int TRACE_RES_RESTART = 1; + +static int _move_obj(PyObject* obj, trace_state* state) { + // Check the movability of the object: + movable_status status = get_movable_status(obj); + switch (status) { + case Py_MOVABLE_YES: + break; + case Py_MOVABLE_NO: + trace(" - %p is not movable", obj); + throw_region_error( + "Instances of type '%s' are not movable", Py_TYPE(obj)->tp_name, + state->src, obj); + return TRACE_RES_ERR; + case Py_MOVABLE_FREEZE: + // Freeze the object, this can invalidate our `external_rc`, we restart after this trace + trace(" - freezing %p", obj); + if (_PyImmutability_Freeze(obj)) { + return TRACE_RES_ERR; + } + + state->restart = true; + return 0; + default: + assert(false); + break; + } + + // Move the object + Py_ssize_t lrc_change = Py_REFCNT(obj); + if (state->src != NULL) { + // -1 for the reference we just followed + lrc_change -= 1; + } + trace(" - moving %p; LRC += %zd", obj, lrc_change); + state->external_rc += lrc_change; + + if (_Py_hashtable_set(state->visited, obj, obj) == -1) { + return -1; + } + + // This makes sure the object is removed from the local GC list. + if (PyObject_IS_GC(obj) && PyObject_GC_IsTracked(obj)) { + // This flag may be set if the region is constructed as part of + // a finalizer. If the flag remains set, for an object removed + // from its GC list bad things can happen. + gc_clear_collecting(_Py_AS_GC(obj)); + // Clearing the space flag makes it easy to merge this list back + // into the local GC lists + gc_set_old_space(_Py_AS_GC(obj), 0); + gc_list_move(_Py_AS_GC(obj), state->gc_list); + } + + if (PyList_Append(state->pending, obj)) { + return -1; + } + + return 0; +} + +static int _trace_visit(PyObject* obj, trace_state* state) { + // References to immutable objects are allowed + if (_PyImmutability_CanViewAsImmutable(obj)) { + assert(_Py_IsImmutable(obj)); + return 0; + } -static trace_res trace_object(PyObject* obj) { - trace_res res = { - .objs = 1, - .incoming_refs = 2, - }; + // Check if the object is already part of the region + if (_Py_hashtable_get(state->visited, (void*)obj)) { + trace(" - Internal reference to %p; LRC -= 1", obj); + state->external_rc -= 1; + return 0; + } + + return _move_obj(obj, state); +} + +static int _trace_once(PyObject* obj, trace_result* result, PyGC_Head *gc_list) { + trace(" - starting trace from %p", obj); + int res = TRACE_RES_DONE; + + // Make sure gc_list is valid + PyGC_Head local_gc_list; + bool dissolve_gc = false; + if (gc_list == NULL) { + gc_list_init(&local_gc_list); + gc_list = &local_gc_list; + dissolve_gc = true; + } + // init the trace state + trace_state state; + if (trace_state_init(&state, gc_list)) { + return TRACE_RES_ERR; + } + + SUCCEEDS(_move_obj(obj, &state)); + + while (PyList_GET_SIZE(state.pending) > 0) { + // Find the next pending item: + PyObject *item = list_pop(state.pending); + + // Traverse item + state.src = item; + trace(" - traversing %p", item); + traverseproc proc = get_reachable_proc(Py_TYPE(item)); + SUCCEEDS(proc(item, (visitproc)_trace_visit, (void*)&state)); + } + + if (state.restart) { + res = TRACE_RES_RESTART; + } + + goto finally; +error: + dissolve_gc = true; + res = TRACE_RES_ERR; +finally: + result->incoming_refs = state.external_rc; + result->objs = _Py_hashtable_len(state.visited); + trace_state_destroy(&state, dissolve_gc); return res; } +static int trace_object(PyObject* obj, trace_result* result, PyGC_Head *gc_list) { + const int TRIES = 2; + trace("Starting trace for %p", obj); + for (int i = 0; i < TRIES; i++) { + // Reset trace + result->objs = 0; + result->incoming_refs = 0; + + // Trace object + int res = _trace_once(obj, result, gc_list); + if (res == TRACE_RES_RESTART) { + trace("- restarting trace for %p", obj); + continue; + } + return res; + } + + return TRACE_RES_DONE; +} + // ################################################################### // Region Object // ################################################################### @@ -221,34 +511,42 @@ static trace_res trace_object(PyObject* obj) { typedef struct { PyObject_HEAD PyObject *dict; + // The GC list containing all objects, used during transfer + PyGC_Head gc_list; } TracingRegionObject; static int -TracingRegion_traverse(TracingRegionObject *self, visitproc visit, void *arg) -{ +TracingRegion_init(TracingRegionObject *self, PyObject *args, PyObject *kwargs) { + gc_list_init(&self->gc_list); + return 0; +} + +static int +TracingRegion_traverse(TracingRegionObject *self, visitproc visit, void *arg) { Py_VISIT(self->dict); return 0; } static int -TracingRegion_clear(TracingRegionObject *self) -{ +TracingRegion_clear(TracingRegionObject *self) { Py_CLEAR(self->dict); return 0; } static void -TracingRegion_dealloc(TracingRegionObject *self) -{ +TracingRegion_dealloc(TracingRegionObject *self) { PyObject_GC_UnTrack(self); TracingRegion_clear(self); Py_TYPE(self)->tp_free((PyObject *)self); } static PyObject* TracingRegion_trace(PyObject *op) { - trace_res res = trace_object(op); + trace_result result; + if (trace_object(op, &result, NULL)) { + return NULL; // propagate Python exception + } - PyObject *t = Py_BuildValue("(ii)", res.objs, res.incoming_refs); + PyObject *t = Py_BuildValue("(ii)", result.objs, result.incoming_refs); if (t == NULL) { return NULL; // propagate Python exception } @@ -278,6 +576,7 @@ PyTypeObject _PyTracingRegion_Type = { .tp_members = TracingRegion_members, .tp_methods = TracingRegion_methods, .tp_dictoffset = offsetof(TracingRegionObject, dict), + .tp_init = (initproc)TracingRegion_init, .tp_new = PyType_GenericNew, .tp_reachable = _PyObject_ReachableVisitTypeAndTraverse, }; From cf0cacb0b383bbedaa0e9155487fd0b9dea9d79c Mon Sep 17 00:00:00 2001 From: xFrednet Date: Thu, 16 Apr 2026 11:53:33 +0200 Subject: [PATCH 04/24] TracingRegions C-interface for closing --- Include/internal/pycore_immutability.h | 2 + Lib/test/test_freeze/test_tracing_region.py | 76 ++++++++++++++++++ Objects/tracingregionobject.c | 88 +++++++++++++++------ 3 files changed, 143 insertions(+), 23 deletions(-) create mode 100644 Lib/test/test_freeze/test_tracing_region.py diff --git a/Include/internal/pycore_immutability.h b/Include/internal/pycore_immutability.h index 4e79803b50fd121..e696fe0542598ec 100644 --- a/Include/internal/pycore_immutability.h +++ b/Include/internal/pycore_immutability.h @@ -9,6 +9,8 @@ extern "C" { #endif PyAPI_DATA(PyTypeObject) _PyTracingRegion_Type; +PyAPI_FUNC(int) _PyTracingRegion_Close(PyObject* region); +PyAPI_FUNC(int) _PyTracingRegion_Open(PyObject* region); struct _Py_immutability_state { int late_init_done; diff --git a/Lib/test/test_freeze/test_tracing_region.py b/Lib/test/test_freeze/test_tracing_region.py new file mode 100644 index 000000000000000..3c430122e91aeca --- /dev/null +++ b/Lib/test/test_freeze/test_tracing_region.py @@ -0,0 +1,76 @@ +import sys +import unittest +from immutable import freeze, is_frozen, freezable +from immutable import TracingRegion as Region + +class TestTraceRefs(unittest.TestCase): + def test_trace(self): + @freezable + class A: + pass + + r = Region() + r.a = A() + r.b = A() + r.c = A() + + _, base_refs = r.trace() + + a = r.a + _, ref_count = r.trace() + self.assertEqual(ref_count, base_refs + 1) + + b = r.b + c = r.c + _, ref_count = r.trace() + self.assertEqual(ref_count, base_refs + 3) + +class TestImplicitFreeze(unittest.TestCase): + def test_implicit_freeze_func(self): + @freezable + def some_func(): + pass + r = Region() + + r.obj = some_func + self.assertFalse(is_frozen(r.obj)) + r.trace() + self.assertTrue(is_frozen(r.obj)) + + def test_implicit_freeze_type(self): + @freezable + class A: + pass + r = Region() + + r.obj = A + self.assertFalse(is_frozen(r.obj)) + r.trace() + self.assertTrue(is_frozen(r.obj)) + + def test_implicit_freeze_module(self): + import random; + r = Region() + + r.obj = random + self.assertFalse(is_frozen(r.obj)) + r.trace() + self.assertTrue(is_frozen(r.obj)) + + # Unimport module + sys.modules.pop("random", None) + sys.mut_modules.pop("random", None) + + def test_implicit_freeze_str(self): + r = Region() + + r.obj = "Ducks are cool" + r.trace() + self.assertTrue(is_frozen(r.obj)) + + def test_implicit_freeze_int(self): + r = Region() + + r.obj = 17 + r.trace() + self.assertTrue(is_frozen(r.obj)) diff --git a/Objects/tracingregionobject.c b/Objects/tracingregionobject.c index a0602f6bb5b60df..1d28de214535a7f 100644 --- a/Objects/tracingregionobject.c +++ b/Objects/tracingregionobject.c @@ -291,8 +291,7 @@ get_reachable_proc(PyTypeObject *tp) static void gc_list_dissolve(PyGC_Head *list) { struct _gc_runtime_state* gc_state = get_gc_state(); - // Use `old[0]` here, we are setting the visited space to 0 in add_visited_set(). - gc_list_merge(list, &(gc_state->old[0].head)); + gc_list_merge(list, &(gc_state->young.head)); } typedef struct { @@ -310,7 +309,7 @@ typedef struct { PyObject *pending; } trace_state; -static void trace_state_destroy(trace_state* state, bool dissolve_gc) { +static void trace_state_destroy(trace_state* state) { if (state->visited) { _Py_hashtable_destroy(state->visited); state->visited = NULL; @@ -319,13 +318,9 @@ static void trace_state_destroy(trace_state* state, bool dissolve_gc) { Py_DECREF(state->pending); state->pending = NULL; } - - if (dissolve_gc) { - gc_list_dissolve(state->gc_list); - } } static int trace_state_init(trace_state* state, PyGC_Head *gc_list) { - assert(gc_list_is_empty(gc_list)); + assert(gc_list == NULL || gc_list_is_empty(gc_list)); state->visited = NULL; state->pending = NULL; @@ -349,7 +344,7 @@ static int trace_state_init(trace_state* state, PyGC_Head *gc_list) { return 0; error: - trace_state_destroy(state, false); + trace_state_destroy(state); return -1; } @@ -401,8 +396,8 @@ static int _move_obj(PyObject* obj, trace_state* state) { return -1; } - // This makes sure the object is removed from the local GC list. - if (PyObject_IS_GC(obj) && PyObject_GC_IsTracked(obj)) { + // This moves the object into the region list, if provided. + if (state->gc_list && PyObject_IS_GC(obj) && PyObject_GC_IsTracked(obj)) { // This flag may be set if the region is constructed as part of // a finalizer. If the flag remains set, for an object removed // from its GC list bad things can happen. @@ -441,15 +436,6 @@ static int _trace_once(PyObject* obj, trace_result* result, PyGC_Head *gc_list) trace(" - starting trace from %p", obj); int res = TRACE_RES_DONE; - // Make sure gc_list is valid - PyGC_Head local_gc_list; - bool dissolve_gc = false; - if (gc_list == NULL) { - gc_list_init(&local_gc_list); - gc_list = &local_gc_list; - dissolve_gc = true; - } - // init the trace state trace_state state; if (trace_state_init(&state, gc_list)) { @@ -475,12 +461,11 @@ static int _trace_once(PyObject* obj, trace_result* result, PyGC_Head *gc_list) goto finally; error: - dissolve_gc = true; res = TRACE_RES_ERR; finally: result->incoming_refs = state.external_rc; result->objs = _Py_hashtable_len(state.visited); - trace_state_destroy(&state, dissolve_gc); + trace_state_destroy(&state); return res; } @@ -494,8 +479,14 @@ static int trace_object(PyObject* obj, trace_result* result, PyGC_Head *gc_list) // Trace object int res = _trace_once(obj, result, gc_list); + + // Restart trace on demand if (res == TRACE_RES_RESTART) { trace("- restarting trace for %p", obj); + if (gc_list != NULL) { + gc_list_dissolve(gc_list); + assert(gc_list_is_empty(gc_list)); + } continue; } return res; @@ -530,6 +521,10 @@ TracingRegion_traverse(TracingRegionObject *self, visitproc visit, void *arg) { static int TracingRegion_clear(TracingRegionObject *self) { Py_CLEAR(self->dict); + // This is deallocating a closed region, we just dissolve it + if (!gc_list_is_empty(&self->gc_list)) { + gc_list_dissolve(&self->gc_list); + } return 0; } @@ -554,6 +549,51 @@ static PyObject* TracingRegion_trace(PyObject *op) { return t; } +/* This method traces the region and closes it if the caller has the only + * owning reference into the graph. The reference passed into this function + * needs to be borrowed. + * + * This function requires the GIL to be held. + * + * Returns -1 if an exception was raised. 0 if the region couldn't be closed + * and 1 if the region was closed. + */ +int _PyTracingRegion_Close(PyObject* op) { + TracingRegionObject *self = (TracingRegionObject*)op; + assert(gc_list_is_empty(&self->gc_list)); + + trace_result result; + if (trace_object(op, &result, &self->gc_list)) { + return -1; // propagate Python exception + } + + // Keep the region open, if the there are more incoming references + // besides the expected owning one + if (result.incoming_refs > 1) { + trace("- Failed to close region %p, there are %zd incoming references", self, result.incoming_refs); + gc_list_dissolve(&self->gc_list); + assert(gc_list_is_empty(&self->gc_list)); + return 1; + } + + trace("- Closed region %p", self); + assert(!gc_list_is_empty(&self->gc_list)); + return 0; +} + +/* This method opens the region by dissolving it and all objects into the + * local GC list. + * + * This function requires the GIL to be held. + */ +int _PyTracingRegion_Open(PyObject* op) { + TracingRegionObject *self = (TracingRegionObject*)op; + assert(!gc_list_is_empty(&self->gc_list)); + gc_list_dissolve(&self->gc_list); + assert(gc_list_is_empty(&self->gc_list)); + return 0; +} + static PyMethodDef TracingRegion_methods[] = { {"trace", _PyCFunction_CAST(TracingRegion_trace), METH_NOARGS, "This traces the region and returns the number of incoming references"}, @@ -581,4 +621,6 @@ PyTypeObject _PyTracingRegion_Type = { .tp_reachable = _PyObject_ReachableVisitTypeAndTraverse, }; - +// TODO: Weak-references pointing into the trace are not handled +// TODO: Weak-references part of the trace are not handled +// From f112d8bf430647b387f0cd7c09a0a46f5d9b139e Mon Sep 17 00:00:00 2001 From: xFrednet Date: Thu, 16 Apr 2026 16:11:24 +0200 Subject: [PATCH 05/24] Weakrefs again --- Objects/tracingregionobject.c | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/Objects/tracingregionobject.c b/Objects/tracingregionobject.c index 1d28de214535a7f..6033ac17b9c5790 100644 --- a/Objects/tracingregionobject.c +++ b/Objects/tracingregionobject.c @@ -3,6 +3,7 @@ #include "pycore_gc.h" // _PyObject_GC_IS_TRACKED() #include "pycore_object.h" // _PyObject_GC_TRACK(), _PyDebugAllocatorStats() #include "pycore_descrobject.h" +#include "pycore_weakref.h" // #define REGION_TRACING @@ -370,13 +371,18 @@ static int _move_obj(PyObject* obj, trace_state* state) { state->src, obj); return TRACE_RES_ERR; case Py_MOVABLE_FREEZE: - // Freeze the object, this can invalidate our `external_rc`, we restart after this trace + // Freeze the object, this can invalidate our `external_rc`, + // we restart after this trace trace(" - freezing %p", obj); if (_PyImmutability_Freeze(obj)) { return TRACE_RES_ERR; } state->restart = true; + // Setting the gc_list to NULL will stop objects from being moved + // between GC lists. Just a small thing we can avoid. The next (full) + // trace will have this set again. + state->gc_list = NULL; return 0; default: assert(false); @@ -453,6 +459,9 @@ static int _trace_once(PyObject* obj, trace_result* result, PyGC_Head *gc_list) trace(" - traversing %p", item); traverseproc proc = get_reachable_proc(Py_TYPE(item)); SUCCEEDS(proc(item, (visitproc)_trace_visit, (void*)&state)); + + // Weak refs need special handling + assert(!PyWeakref_Check(item)); } if (state.restart) { @@ -495,6 +504,20 @@ static int trace_object(PyObject* obj, trace_result* result, PyGC_Head *gc_list) return TRACE_RES_DONE; } +static void detach_weak_refs(PyGC_Head *gc_list) { + PyGC_Head *current = GC_NEXT(gc_list); + while (current != gc_list) { + PyObject *item = _Py_FROM_GC(current); +#ifdef PY_DEBUG + Py_ssize_t weak_ctn = _PyWeakref_GetWeakrefCount(item); + if (weak_ctn) { + trace("- Clearing %zd weak references to %p", weak_ctn, item); + } +#endif + _PyWeakref_ClearWeakRefsNoCallbacks(item); + } +} + // ################################################################### // Region Object // ################################################################### @@ -576,6 +599,10 @@ int _PyTracingRegion_Close(PyObject* op) { return 1; } + // FIXME: This can be optimized, for example by inserting all objects + // with weak refs in the beginning. + detach_weak_refs(&self->gc_list); + trace("- Closed region %p", self); assert(!gc_list_is_empty(&self->gc_list)); return 0; @@ -621,6 +648,4 @@ PyTypeObject _PyTracingRegion_Type = { .tp_reachable = _PyObject_ReachableVisitTypeAndTraverse, }; -// TODO: Weak-references pointing into the trace are not handled // TODO: Weak-references part of the trace are not handled -// From 247f0cb0a77b5a5031616f3d3d46052b83526860 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Thu, 16 Apr 2026 17:13:04 +0200 Subject: [PATCH 06/24] Cowns are working? --- Include/internal/pycore_cown.h | 29 ++ Lib/immutable.py | 1 + Makefile.pre.in | 1 + Modules/_immutablemodule.c | 8 + Objects/cownobject.c | 554 +++++++++++++++++++++++++ Objects/tracingregionobject.c | 10 +- PCbuild/_freeze_module.vcxproj | 1 + PCbuild/_freeze_module.vcxproj.filters | 3 + PCbuild/pythoncore.vcxproj | 2 + PCbuild/pythoncore.vcxproj.filters | 8 + 10 files changed, 614 insertions(+), 3 deletions(-) create mode 100644 Include/internal/pycore_cown.h create mode 100644 Objects/cownobject.c diff --git a/Include/internal/pycore_cown.h b/Include/internal/pycore_cown.h new file mode 100644 index 000000000000000..0345690cc015ab6 --- /dev/null +++ b/Include/internal/pycore_cown.h @@ -0,0 +1,29 @@ +#ifndef Py_INTERNAL_COWN_H +#define Py_INTERNAL_COWN_H +#ifdef __cplusplus +extern "C" { +#endif + +#ifndef Py_BUILD_CORE +# error "Py_BUILD_CORE must be defined to include this header" +#endif + +#include "object.h" +#include "exports.h" + +typedef struct _PyCownObject _PyCownObject; +#define _PyCownObject_CAST(op) _Py_CAST(_PyCownObject*, op) + +PyAPI_DATA(PyTypeObject) _PyCown_Type; + +typedef uint64_t _PyCown_ipid_t; +typedef uint64_t _PyCown_thread_id_t; + +PyAPI_FUNC(_PyCown_ipid_t) _PyCown_ThisInterpreterId(void); +PyAPI_FUNC(_PyCown_thread_id_t) _PyCown_ThisThreadId(void); + + +#ifdef __cplusplus +} +#endif +#endif /* !Py_INTERNAL_COWN_H */ \ No newline at end of file diff --git a/Lib/immutable.py b/Lib/immutable.py index 40cce9c93cbefbf..167273bc31cdbd6 100644 --- a/Lib/immutable.py +++ b/Lib/immutable.py @@ -22,6 +22,7 @@ InterpreterLocal = _c.InterpreterLocal SharedField = _c.SharedField TracingRegion = _c.TracingRegion +Cown = _c.Cown # FIXME(immutable): For the longest time we used the name `isfrozen` # without the underscore. This keeps the function name for now, but diff --git a/Makefile.pre.in b/Makefile.pre.in index 1b55ebe01a2a852..d8ae75ed97237e0 100644 --- a/Makefile.pre.in +++ b/Makefile.pre.in @@ -528,6 +528,7 @@ OBJECT_OBJS= \ Objects/classobject.o \ Objects/codeobject.o \ Objects/complexobject.o \ + Objects/cownobject.o \ Objects/descrobject.o \ Objects/enumobject.o \ Objects/exceptions.o \ diff --git a/Modules/_immutablemodule.c b/Modules/_immutablemodule.c index 7a237c6e1a7a8cd..2c5a5ac665a5108 100644 --- a/Modules/_immutablemodule.c +++ b/Modules/_immutablemodule.c @@ -8,6 +8,7 @@ #include "Python.h" #include +#include "pycore_cown.h" #include "pycore_object.h" #include "pycore_immutability.h" #include "pycore_critical_section.h" @@ -658,6 +659,13 @@ immutable_exec(PyObject *module) { return -1; } + if (PyModule_AddType(module, &_PyCown_Type) != 0) { + return -1; + } + if (_PyImmutability_SetFreezable((PyObject*)&_PyCown_Type, _Py_FREEZABLE_YES) < 0) { + return -1; + } + if (PyModule_AddIntConstant(module, "FREEZABLE_YES", _Py_FREEZABLE_YES) != 0) { return -1; diff --git a/Objects/cownobject.c b/Objects/cownobject.c new file mode 100644 index 000000000000000..5e91a2f6f8a3fb4 --- /dev/null +++ b/Objects/cownobject.c @@ -0,0 +1,554 @@ +#include "Python.h" +#include "pymacro.h" + +#include "pycore_cown.h" +#include "pycore_lock.h" +#include "pycore_time.h" // _PyTime_FromSeconds() + +/* Macro that jumps to error, if the expression `x` does not succeed. */ +#define SUCCEEDS(x) { do { int r = (x); if (r != 0) goto error; } while (0); } + +#define Region_Check(x) Py_IS_TYPE((x), &_PyTracingRegion_Type) + +// The interpreter id 0 is used. This value will be used to indicate that +// no interpreter owns the cown. +#define RELEASED_IPID ((_PyCown_ipid_t)0xff00ff00ff00ff00LL) +#define GC_IPID ((_PyCown_ipid_t)0xffff00ff00ff00ffLL) +#define NO_BLOCKING_TIMEOUT -1 +#define UNSET_THREAD_ID ((_PyCown_ipid_t)0xff00000000000000LL) + +typedef enum CownLockStatus { + COWN_ACQUIRE_ERROR = -1, + COWN_ACQUIRE_FAIL = 0, + COWN_ACQUIRE_SUCCESS = 1 +} CownLockStatus; + +struct _PyCownObject { + PyObject_HEAD + /* The id of the interpreter that currently owns this cown. + * + * This value may be read from and written to from different threads. + * Only use atomic operations to access this field. + */ + // FIXME(cowns): xFrednet: Make sure that an interpreter releases all + // cowns on destruction. + _PyCown_ipid_t owning_ip; + + /* The id of the thread that unlocked this cown. + * + * This is provided as additional information to users, it is not validated + * or used by this cown implementation. + */ + _PyCown_thread_id_t locking_thread; + + /* The value stored in the cown. This value may be immutable, another cown + * or a region object. + */ + PyObject* value; + + /* A lock used, mainly to support timeouts and queueing for locking. + * All other functions should use `owning_ip` to determine if they can + * access the data or not. + * + * Python's mutexes already implement queueing and timeouts in a good way. + * Later we can role our own, if we need but for not this is better. Note + * that the optional GIL release from the lock should not be used, as it + * doesn't seem to account for waiting threads from different interpreters. + * Therefore, we are responsible for releasing and acquireing the GIL. + */ + PyMutex lock; +}; + +static _PyCown_ipid_t cown_get_owner(_PyCownObject *obj) { + return _Py_atomic_load_uint64(&obj->owning_ip); +} + +#define BAIL_UNLESS_OWNED_BY(o, owned_by, result) \ + do {\ + _PyCown_ipid_t owning_ip = cown_get_owner(_PyCownObject_CAST(o)); \ + if (owning_ip != owned_by) { \ + PyErr_Format( \ + PyExc_RuntimeError, \ + "attempted to access a cown owned by %llu from %llu", \ + owning_ip, owned_by); \ + return result; \ + } \ + } while (0); +#define BAIL_UNLESS_OWNED(o, result) BAIL_UNLESS_OWNED_BY(o, _PyCown_ThisInterpreterId(), result) +#define BAIL_UNLESS_OWNED_NULL(o) BAIL_UNLESS_OWNED(o, NULL) + +static int cown_set_value_unchecked(_PyCownObject* self, PyObject* value) { + // Update the value + Py_XSETREF(self->value, Py_NewRef(value)); + + return 0; +} + +static int cown_set_value(_PyCownObject* self, PyObject* value) { + BAIL_UNLESS_OWNED(self, -1); + + // Bridge objects are allowed + if (Region_Check(value)) { + return cown_set_value_unchecked(self, value); + } + + // Immutable objects are allowed + if (_Py_IsImmutable(value)) { + return cown_set_value_unchecked(self, value); + } + + // Local objects are forbidden + PyErr_Format( + PyExc_RuntimeError, + "attempted to store a local mutable object in a cown.\n" + "Only regions, cown, and immutable objects are allowed"); + + return -1; +} + +/* Attempt to lock the cown. + * + * Timeout values: + * (-1) => Non-blocking locking + * (0) => Block with no timeout + * (n) => Blocking with timeout + */ +static int cown_lock(_PyCownObject* self, PyTime_t timeout, _PyCown_ipid_t locking_ip, bool has_gil) { + // A blocking time should only be set, if this call holds the GIL + assert(has_gil || timeout == NO_BLOCKING_TIMEOUT); + + // Try to lock the mutex directly, without releasing the GIL first + PyLockStatus r = _PyMutex_LockTimed(&self->lock, 0, _Py_LOCK_DONT_DETACH); + + // The cown is currently owned by something else. Release the GIL and + // wait for the timeout. + if (r != PY_LOCK_ACQUIRED && timeout != NO_BLOCKING_TIMEOUT) { + // Release the GIL + Py_BEGIN_ALLOW_THREADS; + + // Attempt to lock the mutex. This uses a PyMutex for the locking, + // timeout and signal handling. + r = _PyMutex_LockTimed( + &self->lock, + timeout, + _Py_LOCK_DONT_DETACH | _PY_LOCK_HANDLE_SIGNALS + ); + + // Acquire the GIL + Py_END_ALLOW_THREADS; + } + + // The lock was interrupted + if (r == PY_LOCK_INTR) { + return COWN_ACQUIRE_ERROR; + } + + // The lock acquisition failed + if (r == PY_LOCK_FAILURE) { + return COWN_ACQUIRE_FAIL; + } + + // Set the owning_ip to the current interpreter, thereby taking ownership + _PyCown_ipid_t released_value = RELEASED_IPID; + if (!_Py_atomic_compare_exchange_uint64( + &self->owning_ip, + &released_value, + locking_ip) + ) { + // Failed to set owning_ip, this should never happen and points + // to a deeper issue. + PyErr_Format( + PyExc_RuntimeError, + "[BUG] failed to set owner on a locked cown\n" + "Cown: %U", + self + ); + + _PyMutex_Unlock(&self->lock); + return COWN_ACQUIRE_ERROR; + } + + // Set the locking thread. + if (has_gil) { + self->locking_thread = _PyCown_ThisThreadId(); + } else { + self->locking_thread = UNSET_THREAD_ID; + } + + if (self->value && Region_Check(self->value)) { + _PyTracingRegion_Open(self->value); + } + + return COWN_ACQUIRE_SUCCESS; +} + +/* Returns the interpreter id used by cowns. + * + * The caller must hold the GIL. + */ +_PyCown_ipid_t _PyCown_ThisInterpreterId(void) { + _PyCown_ipid_t ip = PyInterpreterState_GetID(PyInterpreterState_Get()); + // This should never happen... if it does... we have a problem... + assert(ip != RELEASED_IPID); + return ip; +} + +/* Returns the thread id used by cowns. + * + * The caller must hold the GIL. + */ +_PyCown_thread_id_t _PyCown_ThisThreadId(void) { + _PyCown_thread_id_t id = PyThreadState_GetID(PyThreadState_Get()); + return id; +} + +static int PyCown_init(_PyCownObject *self, PyObject *args, PyObject *kwds) { + // See if we got a value as a keyword argument + static char *kwlist[] = {"value", NULL}; + PyObject *value = Py_None; + if (!PyArg_ParseTupleAndKeywords(args, kwds, "|O", kwlist, &value)) { + return -1; + } + + // Init the cown as being acquired by the current interpreter + _PyCown_ipid_t this_ip = _PyCown_ThisInterpreterId(); + _Py_atomic_store_uint64(&self->owning_ip, RELEASED_IPID); + if (cown_lock(self, NO_BLOCKING_TIMEOUT, this_ip, true) != COWN_ACQUIRE_SUCCESS) { + PyErr_Format( + PyExc_RuntimeError, + "Newly created cown couldn't be acquired by interpreter %lld (this)", + this_ip); + return -1; + } + + // Set the cown value using the internal function for full validation + SUCCEEDS(cown_set_value(self, value)); + + // Freeze the cown to enable atomic reference counting for it. + PyObject_GC_UnTrack(self); + SUCCEEDS(_PyImmutability_Freeze(_PyObject_CAST(self))); + + return 0; +error: + return -1; +} + +static int PyCown_traverse(_PyCownObject *self, visitproc _ignore1, void* _ignore2) { + // tp_traverse should never be called on cowns since they're not + // tracked by the GC or in any other GC list. The cown type + // still defines `tp_traverse` to ensure that this is never + // accidentally called. Later we may want to simple remove it + // from the type. + assert(false); + return -1; +} + +static int PyCown_reachable(_PyCownObject *self, visitproc visit, void *arg) { + Py_VISIT(Py_TYPE(self)); + + // The value is explicitly not visited. Freezing or moving cowns should + // not propagate to the value. + // Py_VISIT(self->value); + + return 0; +} + +static int PyCown_clear(_PyCownObject *self) { + cown_set_value_unchecked(self, Py_None); + Py_CLEAR(self->value); + return 0; +} + +static void PyCown_dealloc(_PyCownObject *self) { + PyObject_GC_UnTrack(self); + PyCown_clear(self); + PyObject_GC_Del(self); +} + +static int +lock_acquire_parse_args(PyObject *args, PyObject *kwds, + PyTime_t *timeout) +{ + // Taken from `Modules/_threadmodule.c` + + char *kwlist[] = {"blocking", "timeout", NULL}; + int blocking = 1; + PyObject *timeout_obj = NULL; + if (!PyArg_ParseTupleAndKeywords(args, kwds, "|pO:acquire", kwlist, + &blocking, &timeout_obj)) + return -1; + + const PyTime_t unset_timeout = _PyTime_FromSeconds(NO_BLOCKING_TIMEOUT); + *timeout = unset_timeout; + + if (timeout_obj + && _PyTime_FromSecondsObject(timeout, + timeout_obj, _PyTime_ROUND_TIMEOUT) < 0) + return -1; + + if (!blocking && *timeout != unset_timeout ) { + PyErr_SetString(PyExc_ValueError, + "can't specify a timeout for a non-blocking call"); + return -1; + } + if (*timeout < 0 && *timeout != unset_timeout) { + PyErr_SetString(PyExc_ValueError, + "timeout value must be a non-negative number"); + return -1; + } + if (!blocking) + *timeout = 0; + else if (*timeout != unset_timeout) { + PyTime_t microseconds; + + microseconds = _PyTime_AsMicroseconds(*timeout, _PyTime_ROUND_TIMEOUT); + if (microseconds > PY_TIMEOUT_MAX) { + PyErr_SetString(PyExc_OverflowError, + "timeout value is too large"); + return -1; + } + } + return 0; +} + +static PyObject * +CownObject_acquire(_PyCownObject *self, PyObject *args, PyObject *kwds) +{ + // Parse the arguments + PyTime_t timeout; + if (lock_acquire_parse_args(args, kwds, &timeout) < 0) { + return NULL; + } + + // Attempt to lock the cown + _PyCown_ipid_t this_ip = _PyCown_ThisInterpreterId(); + int res = cown_lock(self, timeout, this_ip, true); + if (res == COWN_ACQUIRE_ERROR) { + return NULL; + } + + // Return the result + return PyBool_FromLong(res == COWN_ACQUIRE_SUCCESS); +} + +PyDoc_STRVAR(CownObject_acquire_doc, +"acquire($self, /, blocking=True, timeout=-1)\n\ +--\n\ +\n\ +Attempts to acquires the cown. With default arguments this will block\n\ +until the cown can be aquired, even when acquire is called from the same\n\ +interpreter. The return indicates if the cown was\n\ +was acquired. The blocking operation is interruptible."); + +static int cown_release_unchecked(_PyCownObject* self, _PyCown_ipid_t unlocking_ip) { + // Set owning_ip to indicate the released state + if (!_Py_atomic_compare_exchange_uint64(&self->owning_ip, &unlocking_ip, RELEASED_IPID)) { + PyErr_Format( + PyExc_RuntimeError, + "interpreter %lld (this) attempted to release a cown owned by someone else\n" + "Cown: %U", + unlocking_ip, self); + return -1; + } + + // Unlocking should always succeed + int res = _PyMutex_TryUnlock(&self->lock); + assert(res == 0); + (void)res; + + return 0; +} + +/* Checks that the cown is not released, and that the owner is as the current interpreter. */ +static int cown_check_owner_before_release(_PyCownObject *self, _PyCown_ipid_t unlocking_ip) { + _PyCown_ipid_t owning_ip = cown_get_owner(self); + if (owning_ip == RELEASED_IPID) { + PyErr_Format( + PyExc_RuntimeError, + "interpreter %lld attempted to release/switch a released cown", + unlocking_ip + ); + return -1; + } + if (owning_ip != unlocking_ip) { + PyErr_Format( + PyExc_RuntimeError, + "interpreter %lld attempted to release/switch a cown owned by %lld", + unlocking_ip, owning_ip + ); + return -1; + } + return 0; +} + +/* Try closing the region by cleaning it. + * Returns: + * (-1) If an error occurred while trying to clean the region. + * (0) If the region is still open after this call. + * (1) If the region is closed after this call. + */ +static int cown_try_closing_region(_PyCownObject *self) { + assert(Region_Check(self->value)); + + return _PyTracingRegion_Close(self->value); +} + +static int cown_release(_PyCownObject *self, _PyCown_ipid_t unlocking_ip) { + if (cown_check_owner_before_release(self, unlocking_ip) < 0) { + return -1; + } + + if (_Py_IsImmutable(self->value)) { + // Can be released without any restrictions + return cown_release_unchecked(self, unlocking_ip); + } + assert(Region_Check(self->value)); + + int cleaning_res = cown_try_closing_region(self); + if (cleaning_res < 0) { + return -1; + } + if (cleaning_res == 0) { + PyErr_Format( + PyExc_RuntimeError, + "the cown can't be released, since the contained region is still open"); + return -1; + } + // Region is closed, safe to release + return cown_release_unchecked(self, unlocking_ip); +} + +static PyObject* CownObject_release(_PyCownObject *self, PyObject *ignored) { + _PyCown_ipid_t this_ip = _PyCown_ThisInterpreterId(); + if (cown_release(self, this_ip) < 0) { + return NULL; + } + + Py_RETURN_NONE; +} + +PyDoc_STRVAR(CownObject_release_doc, +"release($self, /)\n\ +--\n\ +\n\ +Release the cown, allowing another interpreter that is blocked waiting for\n\ +the cown to acquire the cown. The cown must be in the locked state\n\ +and must be unlocked from the owning interpreter. It may be unlocked \n\ +by any thread on the owning interpreter."); + +static PyObject * +CownObject_locked(_PyCownObject *op, PyObject *Py_UNUSED(dummy)) +{ + return PyBool_FromLong(cown_get_owner(op) != RELEASED_IPID); +} + +PyDoc_STRVAR(CownObject_locked_doc, +"locked($self, /)\n\ +--\n\ +\n\ +Return whether the cown currently released or aquired. \n\ +Use `owned()` to check if the cown is aquired by the current interpreter."); + +static PyObject * +CownObject_owned(_PyCownObject *op, PyObject *Py_UNUSED(dummy)) +{ + return PyBool_FromLong(cown_get_owner(op) == _PyCown_ThisInterpreterId()); +} + +PyDoc_STRVAR(CownObject_owned_doc, +"owned($self, /)\n\ +--\n\ +\n\ +Return true if the cown is currently aquired by this interpreter, false otherwise."); + +static PyObject * +CownObject_owned_by_thread(_PyCownObject *op, PyObject *Py_UNUSED(dummy)) +{ + if (cown_get_owner(op) != _PyCown_ThisInterpreterId()) { + Py_RETURN_FALSE; + } + + return PyBool_FromLong(op->locking_thread == _PyCown_ThisThreadId()); +} + +PyDoc_STRVAR(CownObject_owned_by_thread_doc, +"owned($self, /)\n\ +--\n\ +\n\ +Return true if the cown is currently aquired by this interpreter and was \n\ +locked by the current thread, false otherwise. \n\ +Ownership on the thread level is not enforced, any thread on the owning\n\ +interpreter can access and release the cown. This is information is only\n\ +provided to give more control for those who seek it."); + + +// Define the CownType with methods +static PyMethodDef PyCown_methods[] = { + {"acquire", _PyCFunction_CAST(CownObject_acquire), METH_VARARGS | METH_KEYWORDS, CownObject_acquire_doc}, + {"release", _PyCFunction_CAST(CownObject_release), METH_NOARGS, CownObject_release_doc}, + {"locked", _PyCFunction_CAST(CownObject_locked), METH_NOARGS, CownObject_locked_doc}, + {"owned", _PyCFunction_CAST(CownObject_owned), METH_NOARGS, CownObject_owned_doc}, + {"owned_by_thread", _PyCFunction_CAST(CownObject_owned_by_thread), METH_NOARGS, CownObject_owned_by_thread_doc}, + {NULL} // Sentinel +}; + +static PyObject *CownObject_get_value(_PyCownObject *self, void *closure) { + BAIL_UNLESS_OWNED_NULL(self); + + return Py_NewRef(self->value); +} + +static int CownObject_set_value(_PyCownObject *self, PyObject *value, void *closure) { + BAIL_UNLESS_OWNED(self, -1); + + return cown_set_value(self, value); +} + +static PyGetSetDef PyCownObject_getset[] = { + {"value", (getter)CownObject_get_value, (setter)CownObject_set_value, + "", NULL}, + {NULL, NULL, NULL, NULL, NULL} +}; + +static PyObject *PyCown_repr(_PyCownObject *self) { + _PyCown_ipid_t owner = cown_get_owner(self); + // On this interpreter we can access the cown and content + // safely since we hold the GIL + if (owner == _PyCown_ThisInterpreterId()) { + return PyUnicode_FromFormat( + "Cown(interpreter=%llu (this), value=%S)", + owner, + PyObject_Repr(self->value) + ); + } + + // The cown is released and can be acquired + if (owner == RELEASED_IPID) { + return PyUnicode_FromFormat( + "Cown(interpreter=None, status=Released)" + ); + } + + // The cown is owned by a different interpreter + return PyUnicode_FromFormat( + "Cown(interpreter=%llu (other))", + owner + ); +} + +PyTypeObject _PyCown_Type = { + PyVarObject_HEAD_INIT(&PyType_Type, 0) + .tp_name = "Cown", + .tp_basicsize = sizeof(_PyCownObject), + .tp_dealloc = (destructor)PyCown_dealloc, + .tp_repr = (reprfunc)PyCown_repr, + .tp_flags = Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_GC | Py_TPFLAGS_IMMUTABLETYPE, + .tp_traverse = (traverseproc)PyCown_traverse, + .tp_reachable = (traverseproc)PyCown_reachable, + .tp_clear = (inquiry)PyCown_clear, + .tp_methods = PyCown_methods, + .tp_getset = PyCownObject_getset, + .tp_init = (initproc)PyCown_init, + .tp_new = PyType_GenericNew, +}; + diff --git a/Objects/tracingregionobject.c b/Objects/tracingregionobject.c index 6033ac17b9c5790..ddc16e1a1fb01e3 100644 --- a/Objects/tracingregionobject.c +++ b/Objects/tracingregionobject.c @@ -514,7 +514,11 @@ static void detach_weak_refs(PyGC_Head *gc_list) { trace("- Clearing %zd weak references to %p", weak_ctn, item); } #endif - _PyWeakref_ClearWeakRefsNoCallbacks(item); + if (_PyType_SUPPORTS_WEAKREFS(Py_TYPE(item))) { + _PyWeakref_ClearWeakRefsNoCallbacks(item); + } + + current = GC_NEXT(current); } } @@ -596,7 +600,7 @@ int _PyTracingRegion_Close(PyObject* op) { trace("- Failed to close region %p, there are %zd incoming references", self, result.incoming_refs); gc_list_dissolve(&self->gc_list); assert(gc_list_is_empty(&self->gc_list)); - return 1; + return 0; } // FIXME: This can be optimized, for example by inserting all objects @@ -605,7 +609,7 @@ int _PyTracingRegion_Close(PyObject* op) { trace("- Closed region %p", self); assert(!gc_list_is_empty(&self->gc_list)); - return 0; + return 1; } /* This method opens the region by dissolving it and all objects into the diff --git a/PCbuild/_freeze_module.vcxproj b/PCbuild/_freeze_module.vcxproj index c2514ce954c9024..c19b7efdd181537 100644 --- a/PCbuild/_freeze_module.vcxproj +++ b/PCbuild/_freeze_module.vcxproj @@ -134,6 +134,7 @@ + diff --git a/PCbuild/_freeze_module.vcxproj.filters b/PCbuild/_freeze_module.vcxproj.filters index ebb3c7a469b443c..0a33235d9dc855c 100644 --- a/PCbuild/_freeze_module.vcxproj.filters +++ b/PCbuild/_freeze_module.vcxproj.filters @@ -106,6 +106,9 @@ Source Files + + Source Files + Source Files diff --git a/PCbuild/pythoncore.vcxproj b/PCbuild/pythoncore.vcxproj index b61f2669d4974d9..32d5877122e4948 100644 --- a/PCbuild/pythoncore.vcxproj +++ b/PCbuild/pythoncore.vcxproj @@ -238,6 +238,7 @@ + @@ -532,6 +533,7 @@ + diff --git a/PCbuild/pythoncore.vcxproj.filters b/PCbuild/pythoncore.vcxproj.filters index 61fe05d775a7b58..0106e8290c20fa5 100644 --- a/PCbuild/pythoncore.vcxproj.filters +++ b/PCbuild/pythoncore.vcxproj.filters @@ -618,6 +618,9 @@ Include\internal + + Include\internal + Include\internal @@ -697,6 +700,8 @@ Include\internal + Include\internal + Include\internal @@ -1207,6 +1212,9 @@ Objects + + Objects + Objects From 6271692724d30db6f843de9a23c5319950f6bef1 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Tue, 21 Apr 2026 11:00:52 +0200 Subject: [PATCH 07/24] IDK --- Objects/tracingregionobject.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Objects/tracingregionobject.c b/Objects/tracingregionobject.c index ddc16e1a1fb01e3..a9cda30698f7764 100644 --- a/Objects/tracingregionobject.c +++ b/Objects/tracingregionobject.c @@ -292,7 +292,8 @@ get_reachable_proc(PyTypeObject *tp) static void gc_list_dissolve(PyGC_Head *list) { struct _gc_runtime_state* gc_state = get_gc_state(); - gc_list_merge(list, &(gc_state->young.head)); + //gc_list_merge(list, &(gc_state->young.head)); + gc_list_merge(list, &(gc_state->old[0].head)); } typedef struct { From bf4b7b0391b4a46cf5a1054b2f2ec9960374a17e Mon Sep 17 00:00:00 2001 From: xFrednet Date: Tue, 28 Apr 2026 09:52:38 +0200 Subject: [PATCH 08/24] Memory fun --- Objects/tracingregionobject.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Objects/tracingregionobject.c b/Objects/tracingregionobject.c index a9cda30698f7764..50053331634d59b 100644 --- a/Objects/tracingregionobject.c +++ b/Objects/tracingregionobject.c @@ -548,11 +548,11 @@ TracingRegion_traverse(TracingRegionObject *self, visitproc visit, void *arg) { static int TracingRegion_clear(TracingRegionObject *self) { - Py_CLEAR(self->dict); // This is deallocating a closed region, we just dissolve it if (!gc_list_is_empty(&self->gc_list)) { gc_list_dissolve(&self->gc_list); } + Py_CLEAR(self->dict); return 0; } From 31566c6e6007afb5501d78f0cb847005204314fb Mon Sep 17 00:00:00 2001 From: xFrednet Date: Wed, 15 Jul 2026 09:57:54 +0200 Subject: [PATCH 09/24] Pyrona: Log what objects have incoming refs --- Lib/test/test_freeze/test_tracing_region.py | 60 +++++ Objects/tracingregionobject.c | 253 +++++++++++++++++--- 2 files changed, 278 insertions(+), 35 deletions(-) diff --git a/Lib/test/test_freeze/test_tracing_region.py b/Lib/test/test_freeze/test_tracing_region.py index 3c430122e91aeca..79f8227445ba8fa 100644 --- a/Lib/test/test_freeze/test_tracing_region.py +++ b/Lib/test/test_freeze/test_tracing_region.py @@ -2,8 +2,67 @@ import unittest from immutable import freeze, is_frozen, freezable from immutable import TracingRegion as Region +from immutable import Cown + +def sort_region_error(msg): + """Normalize a 'region could not be closed' message by sorting its + per-object lines. Useful for deterministic test assertions, since the + object order comes from hashtable iteration and isn't stable.""" + header, *lines = msg.splitlines() + return [header, *sorted(lines)] class TestTraceRefs(unittest.TestCase): + + def test_release_error(self): + x = [1] + y = [2] + + c = Cown(Region()) + c.value.x = x + c.value.y = y + + with self.assertRaises(RuntimeError) as cm: + c.release() + + self.assertEqual( + sort_region_error(str(cm.exception)), + [ + "The region could not be closed due to:", + "- 1 incoming reference to '[1]'", + "- 1 incoming reference to '[2]'" + ]) + + def test_release_error(self): + # The object order in the error message is based on the address + # and therefore fairly random. All elements look the same of + # make testing stable. + l = [[1], [1], [1], [1], [1], [1], [1], [1]] + + c = Cown(Region()) + c.value.x = [] + + for i in range(len(l)): + c.value.x.append(l[i]) + + with self.assertRaises(RuntimeError) as cm: + c.release() + + self.assertEqual( + sort_region_error(str(cm.exception)), + [ + "The region could not be closed due to:", + "- 1 incoming reference to '[1]'", + "- 1 incoming reference to '[1]'", + "- 1 incoming reference to '[1]'", + "- 1 incoming reference to '[1]'", + "- 1 incoming reference to '[1]'", + "- 3 references to other objects", + ]) + + # The cown should now be released + l = None + c.release() + def test_trace(self): @freezable class A: @@ -74,3 +133,4 @@ def test_implicit_freeze_int(self): r.obj = 17 r.trace() self.assertTrue(is_frozen(r.obj)) + diff --git a/Objects/tracingregionobject.c b/Objects/tracingregionobject.c index 50053331634d59b..a0e638376f252bf 100644 --- a/Objects/tracingregionobject.c +++ b/Objects/tracingregionobject.c @@ -5,6 +5,8 @@ #include "pycore_descrobject.h" #include "pycore_weakref.h" +#define ERROR_OBJECT_REPORT_COUNT 5 + // #define REGION_TRACING #ifdef REGION_TRACING @@ -349,8 +351,24 @@ static int trace_state_init(trace_state* state, PyGC_Head *gc_list) { trace_state_destroy(state); return -1; } +static int trace_state_reset(trace_state* state, PyGC_Head *gc_list) { + _Py_hashtable_clear(state->visited); + SUCCEEDS(PyList_Clear(state->pending)); + + state->external_rc = 0; + state->restart = false; + state->gc_list = gc_list; + state->src = NULL; + + return 0; +error: + trace_state_destroy(state); + return -1; +} + typedef struct { + _Py_hashtable_t *obj_table; Py_ssize_t objs; Py_ssize_t incoming_refs; } trace_result; @@ -399,7 +417,8 @@ static int _move_obj(PyObject* obj, trace_state* state) { trace(" - moving %p; LRC += %zd", obj, lrc_change); state->external_rc += lrc_change; - if (_Py_hashtable_set(state->visited, obj, obj) == -1) { + // Mark the object as visited, this stores the lrc_change for better error reporting + if (_Py_hashtable_set(state->visited, obj, (void*)lrc_change) == -1) { return -1; } @@ -430,7 +449,9 @@ static int _trace_visit(PyObject* obj, trace_state* state) { } // Check if the object is already part of the region - if (_Py_hashtable_get(state->visited, (void*)obj)) { + _Py_hashtable_entry_t *entry = _Py_hashtable_get_entry(state->visited, (void*)obj); + if (entry != NULL) { + entry->value -= 1; trace(" - Internal reference to %p; LRC -= 1", obj); state->external_rc -= 1; return 0; @@ -439,56 +460,72 @@ static int _trace_visit(PyObject* obj, trace_state* state) { return _move_obj(obj, state); } -static int _trace_once(PyObject* obj, trace_result* result, PyGC_Head *gc_list) { +static int _filter_visited(_Py_hashtable_t *ht, const void *key, const void *value, void *target_void) { + _Py_hashtable_t *target = (_Py_hashtable_t *)target_void; + // Only take objects with incoming references + if (value == 0) { + return 0; + } + if (_Py_hashtable_set(target, key, value)) { + return -1; + } + if (_Py_hashtable_len(target) >= ERROR_OBJECT_REPORT_COUNT) { + return 1; + } + return 0; +} + +static int _trace_once(PyObject* obj, trace_state* state) { trace(" - starting trace from %p", obj); int res = TRACE_RES_DONE; - // init the trace state - trace_state state; - if (trace_state_init(&state, gc_list)) { - return TRACE_RES_ERR; - } - - SUCCEEDS(_move_obj(obj, &state)); + SUCCEEDS(_move_obj(obj, state)); - while (PyList_GET_SIZE(state.pending) > 0) { + while (PyList_GET_SIZE(state->pending) > 0) { // Find the next pending item: - PyObject *item = list_pop(state.pending); + PyObject *item = list_pop(state->pending); // Traverse item - state.src = item; + state->src = item; trace(" - traversing %p", item); traverseproc proc = get_reachable_proc(Py_TYPE(item)); - SUCCEEDS(proc(item, (visitproc)_trace_visit, (void*)&state)); + SUCCEEDS(proc(item, (visitproc)_trace_visit, (void*)state)); // Weak refs need special handling assert(!PyWeakref_Check(item)); } - if (state.restart) { + if (state->restart) { res = TRACE_RES_RESTART; } - goto finally; -error: - res = TRACE_RES_ERR; -finally: - result->incoming_refs = state.external_rc; - result->objs = _Py_hashtable_len(state.visited); - trace_state_destroy(&state); return res; +error: + return TRACE_RES_ERR; } static int trace_object(PyObject* obj, trace_result* result, PyGC_Head *gc_list) { + // We do two tracing attempts, the first one may freeze classes and objects + // and require a retrace. The second attempt should pass since all objects + // should now be frozen. Pre-freeze hooks can mess with this, but consenting + // adults and such. const int TRIES = 2; trace("Starting trace for %p", obj); + + // Init trace state. + trace_state state; + if (trace_state_init(&state, gc_list)) { + return TRACE_RES_ERR; + } + + result->obj_table = NULL; + + int res = 0; for (int i = 0; i < TRIES; i++) { - // Reset trace - result->objs = 0; - result->incoming_refs = 0; + SUCCEEDS(trace_state_reset(&state, gc_list)); // Trace object - int res = _trace_once(obj, result, gc_list); + res = _trace_once(obj, &state); // Restart trace on demand if (res == TRACE_RES_RESTART) { @@ -499,10 +536,43 @@ static int trace_object(PyObject* obj, trace_result* result, PyGC_Head *gc_list) } continue; } - return res; + + break; + } + + // The region can't be closed, we'll collect some extra meta data for + // a better error message. + if (state.external_rc > 1) { + // + result->obj_table = _Py_hashtable_new( + _Py_hashtable_hash_ptr, + _Py_hashtable_compare_direct); + if (result->obj_table == NULL) { + goto error; + } + int for_res = _Py_hashtable_foreach(state.visited, _filter_visited, (void*)result->obj_table); + if (for_res < -1) { + _Py_hashtable_destroy(result->obj_table); + result->obj_table = NULL; + goto error; + } + // If the region is "small" enough for a mermaid diagram we do the trace again. + // + // TODO: We probably want a flag to enable/disable this + // if (result->objs >= 500) { + // // pass + // } } - return TRACE_RES_DONE; + goto finally; +error: + res = TRACE_RES_RESTART; +finally: + result->incoming_refs = state.external_rc; + result->objs = _Py_hashtable_len(state.visited); + trace_state_destroy(&state); + + return res; } static void detach_weak_refs(PyGC_Head *gc_list) { @@ -569,6 +639,10 @@ static PyObject* TracingRegion_trace(PyObject *op) { return NULL; // propagate Python exception } + if (result.obj_table != NULL) { + _Py_hashtable_destroy(result.obj_table); + } + PyObject *t = Py_BuildValue("(ii)", result.objs, result.incoming_refs); if (t == NULL) { return NULL; // propagate Python exception @@ -577,6 +651,90 @@ static PyObject* TracingRegion_trace(PyObject *op) { return t; } +// State threaded through `_report_incoming_ref` while building the +// "region could not be closed" error message. +typedef struct { + PyUnicodeWriter *writer; + // The region object being closed. One incoming reference to it is the + // expected owning reference and is not reported as a problem. + PyObject *region; + // Sum of the (problematic) incoming references reported so far. + Py_ssize_t accounted; +} incoming_ref_report; + +// `_Py_hashtable_foreach` callback over `trace_result.obj_table`. Appends one +// "- N incoming reference(s) to 'obj'" line per object to the writer. +static int +_report_incoming_ref(_Py_hashtable_t *ht, const void *key, const void *value, void *user_data) { + incoming_ref_report *report = (incoming_ref_report *)user_data; + PyObject *obj = (PyObject *)key; + Py_ssize_t refs = (Py_ssize_t)value; + + // The caller holds one owning reference to the region object itself, which + // is expected and not a reason the region couldn't be closed. Don't report + // it, but do report any additional references to the region. + if (obj == report->region) { + refs -= 1; + if (refs <= 0) { + return 0; + } + } + + report->accounted += refs; + + // `%S` calls `str()` on the object. + if (PyUnicodeWriter_Format(report->writer, + "- %zd incoming reference%s to '%S'\n", + refs, (refs == 1) ? "" : "s", obj) < 0) { + return -1; + } + return 0; +} + +// Builds the error message describing why a region could not be closed, listing +// the objects that still have incoming references. Returns a new reference to +// the message string, or NULL with an exception set. +static PyObject * +build_close_error_message(trace_result *trace_info, PyObject *region) { + PyUnicodeWriter *writer = PyUnicodeWriter_Create(0); + if (writer == NULL) { + return NULL; + } + + incoming_ref_report report = { writer, region, 0 }; + + if (PyUnicodeWriter_WriteUTF8(writer, + "The region could not be closed due to:\n", -1) < 0) { + goto error; + } + + // `obj_table` maps each object with incoming references to the number of + // such references. Emit one line per object. + if (_Py_hashtable_foreach(trace_info->obj_table, _report_incoming_ref, &report) < 0) { + goto error; + } + + // One incoming reference is the expected owning reference to the region + // itself; everything beyond that is a reason the region stayed open. The + // `obj_table` is also capped at `ERROR_OBJECT_REPORT_COUNT` entries, so it + // may not list every object. Summarise whatever wasn't reported above. + Py_ssize_t problem_refs = trace_info->incoming_refs - 1; + if (report.accounted < problem_refs) { + Py_ssize_t others = problem_refs - report.accounted; + if (PyUnicodeWriter_Format(writer, + "- %zd reference%s to other objects\n", + others, (others == 1) ? "" : "s") < 0) { + goto error; + } + } + + return PyUnicodeWriter_Finish(writer); + +error: + PyUnicodeWriter_Discard(writer); + return NULL; +} + /* This method traces the region and closes it if the caller has the only * owning reference into the graph. The reference passed into this function * needs to be borrowed. @@ -590,18 +748,34 @@ int _PyTracingRegion_Close(PyObject* op) { TracingRegionObject *self = (TracingRegionObject*)op; assert(gc_list_is_empty(&self->gc_list)); - trace_result result; - if (trace_object(op, &result, &self->gc_list)) { - return -1; // propagate Python exception + int res = 0; + trace_result trace_info; + if (trace_object(op, &trace_info, &self->gc_list)) { + goto error; // propagate Python exception } // Keep the region open, if the there are more incoming references // besides the expected owning one - if (result.incoming_refs > 1) { - trace("- Failed to close region %p, there are %zd incoming references", self, result.incoming_refs); + if (trace_info.incoming_refs > 1) { + trace("- Failed to close region %p, there are %zd incoming references", self, trace_info.incoming_refs); gc_list_dissolve(&self->gc_list); assert(gc_list_is_empty(&self->gc_list)); - return 0; + + // Report which objects still have incoming references as a + // `RuntimeError`, e.g.: + // + // RuntimeError: The region could not be closed due to: + // - 1 incoming reference to '[1, 2, 3]' + // - 2 incoming references to '(6, 7)' + PyObject *msg = build_close_error_message(&trace_info, op); + if (msg != NULL) { + PyErr_SetObject(PyExc_RuntimeError, msg); + Py_DECREF(msg); + } + // If `msg` is NULL, building the message failed and an exception + // (e.g. MemoryError) is already set; either way we propagate it. + + goto error; } // FIXME: This can be optimized, for example by inserting all objects @@ -610,7 +784,16 @@ int _PyTracingRegion_Close(PyObject* op) { trace("- Closed region %p", self); assert(!gc_list_is_empty(&self->gc_list)); - return 1; + res = 1; + goto finally; +error: + res = -1; +finally: + if (trace_info.obj_table != NULL) { + _Py_hashtable_destroy(trace_info.obj_table); + } + + return res; } /* This method opens the region by dissolving it and all objects into the From 41687713dd040f596bd8df3a22f620bc7a3100ce Mon Sep 17 00:00:00 2001 From: xFrednet Date: Wed, 15 Jul 2026 12:42:02 +0200 Subject: [PATCH 10/24] TRegion: Mermaid plan --- Objects/tracingregionobject.c | 108 ++++++++++++++++++++++++++-------- 1 file changed, 82 insertions(+), 26 deletions(-) diff --git a/Objects/tracingregionobject.c b/Objects/tracingregionobject.c index a0e638376f252bf..e49e771271d71ee 100644 --- a/Objects/tracingregionobject.c +++ b/Objects/tracingregionobject.c @@ -6,6 +6,7 @@ #include "pycore_weakref.h" #define ERROR_OBJECT_REPORT_COUNT 5 +#define ERROR_MERMAID_REPORT_LIMIT 50 // #define REGION_TRACING @@ -298,22 +299,52 @@ gc_list_dissolve(PyGC_Head *list) { gc_list_merge(list, &(gc_state->old[0].head)); } + +typedef struct { + // These are the objects with incoming references, that + // should be highlighted in the graph. + _Py_hashtable_t *error_objs; + // TODO(mermaid): PyUnicodeWriter *writer; +} mermaid_builder_t; + typedef struct { /// A list of all visited objects _Py_hashtable_t *visited; /// The number of refs coming into this object graph Py_ssize_t external_rc; - // This is set if an object was frozen and the trace needs to restart to be valid - bool restart; - // The GC list used for this trace + // The GC list used for this trace, it may be null if the trace + // should not move the objects from their current list. PyGC_Head* gc_list; // The source of the reference, this is used for error reporting PyObject *src; // List of pending objects that are not GC PyObject *pending; -} trace_state; + // Used to build a mermaid diagram for error reporting if + // the field is not NULL. + mermaid_builder_t *mermaid; + // This is set if an object was frozen and the trace needs + // to restart to be valid + bool restart; +} trace_state_t; + +static int mermaid_visit(PyObject* obj, trace_state_t* state) { + // TODO(mermaid): + // - Draw a reference from `state->src` to `obj` + // - if the object is immutable, give it the `immutable` class + // - if an object is inside `_Py_hashtable_t *error_objs` give it the `error` class + // + // An object node should look roughly like this: + // 0x12344321 + // RC = 10 + // [Type] + // + // In mermaid, it may look like this: + // %p[%p
rc=%ld%s], obj, obj, Py_REFCTN(obj), ("[%s]", Py_TYPE(obj)->name) -static void trace_state_destroy(trace_state* state) { + return 0; +} + +static void trace_state_destroy(trace_state_t* state) { if (state->visited) { _Py_hashtable_destroy(state->visited); state->visited = NULL; @@ -323,7 +354,7 @@ static void trace_state_destroy(trace_state* state) { state->pending = NULL; } } -static int trace_state_init(trace_state* state, PyGC_Head *gc_list) { +static int trace_state_init(trace_state_t* state, PyGC_Head *gc_list) { assert(gc_list == NULL || gc_list_is_empty(gc_list)); state->visited = NULL; @@ -351,7 +382,7 @@ static int trace_state_init(trace_state* state, PyGC_Head *gc_list) { trace_state_destroy(state); return -1; } -static int trace_state_reset(trace_state* state, PyGC_Head *gc_list) { +static int trace_state_reset(trace_state_t* state, PyGC_Head *gc_list) { _Py_hashtable_clear(state->visited); SUCCEEDS(PyList_Clear(state->pending)); @@ -371,13 +402,13 @@ typedef struct { _Py_hashtable_t *obj_table; Py_ssize_t objs; Py_ssize_t incoming_refs; -} trace_result; +} trace_info_t; const int TRACE_RES_ERR = -1; const int TRACE_RES_DONE = 0; const int TRACE_RES_RESTART = 1; -static int _move_obj(PyObject* obj, trace_state* state) { +static int _move_obj(PyObject* obj, trace_state_t* state) { // Check the movability of the object: movable_status status = get_movable_status(obj); switch (status) { @@ -441,7 +472,13 @@ static int _move_obj(PyObject* obj, trace_state* state) { return 0; } -static int _trace_visit(PyObject* obj, trace_state* state) { +static int _trace_visit(PyObject* obj, trace_state_t* state) { + if (state->mermaid) { + if (mermaid_visit(obj, state)) { + return -1; + } + } + // References to immutable objects are allowed if (_PyImmutability_CanViewAsImmutable(obj)) { assert(_Py_IsImmutable(obj)); @@ -475,7 +512,7 @@ static int _filter_visited(_Py_hashtable_t *ht, const void *key, const void *val return 0; } -static int _trace_once(PyObject* obj, trace_state* state) { +static int _trace_once(PyObject* obj, trace_state_t* state) { trace(" - starting trace from %p", obj); int res = TRACE_RES_DONE; @@ -504,7 +541,7 @@ static int _trace_once(PyObject* obj, trace_state* state) { return TRACE_RES_ERR; } -static int trace_object(PyObject* obj, trace_result* result, PyGC_Head *gc_list) { +static int trace_object(PyObject* obj, trace_info_t* result, PyGC_Head *gc_list) { // We do two tracing attempts, the first one may freeze classes and objects // and require a retrace. The second attempt should pass since all objects // should now be frozen. Pre-freeze hooks can mess with this, but consenting @@ -513,7 +550,7 @@ static int trace_object(PyObject* obj, trace_result* result, PyGC_Head *gc_list) trace("Starting trace for %p", obj); // Init trace state. - trace_state state; + trace_state_t state; if (trace_state_init(&state, gc_list)) { return TRACE_RES_ERR; } @@ -543,7 +580,6 @@ static int trace_object(PyObject* obj, trace_result* result, PyGC_Head *gc_list) // The region can't be closed, we'll collect some extra meta data for // a better error message. if (state.external_rc > 1) { - // result->obj_table = _Py_hashtable_new( _Py_hashtable_hash_ptr, _Py_hashtable_compare_direct); @@ -556,12 +592,34 @@ static int trace_object(PyObject* obj, trace_result* result, PyGC_Head *gc_list) result->obj_table = NULL; goto error; } - // If the region is "small" enough for a mermaid diagram we do the trace again. - // - // TODO: We probably want a flag to enable/disable this - // if (result->objs >= 500) { - // // pass - // } + + // If the number of objects is below the limit we + // can create and dump a mermaid diagram of the graph. + if (trace_info.objs < ERROR_MERMAID_REPORT_LIMIT) { + // TODO(mermaid): Populate there mermaid state + // Then run the trace which should write the connections + + // Here are is the mermaid template: + // --- + //
+ // + // ```mermaid + // %%%%{init: {'theme': 'neutral', 'themeVariables': { 'fontSize': '16px' }}}%%%% + // + // // TODO Tracing + // + // "classDef immutable fill:#94f7ff" + // "classDef error stroke-width:4px,stroke:red" + // ``` + //
+ // --- + + if (trace_object(op, &trace_info, NULL)) { + goto error; // propagate Python exception + } + + // TODO(mermaid): Write the diagram to `region-graph.md` + } } goto finally; @@ -634,7 +692,7 @@ TracingRegion_dealloc(TracingRegionObject *self) { } static PyObject* TracingRegion_trace(PyObject *op) { - trace_result result; + trace_info_t result; if (trace_object(op, &result, NULL)) { return NULL; // propagate Python exception } @@ -662,7 +720,7 @@ typedef struct { Py_ssize_t accounted; } incoming_ref_report; -// `_Py_hashtable_foreach` callback over `trace_result.obj_table`. Appends one +// `_Py_hashtable_foreach` callback over `trace_info.obj_table`. Appends one // "- N incoming reference(s) to 'obj'" line per object to the writer. static int _report_incoming_ref(_Py_hashtable_t *ht, const void *key, const void *value, void *user_data) { @@ -695,7 +753,7 @@ _report_incoming_ref(_Py_hashtable_t *ht, const void *key, const void *value, vo // the objects that still have incoming references. Returns a new reference to // the message string, or NULL with an exception set. static PyObject * -build_close_error_message(trace_result *trace_info, PyObject *region) { +build_close_error_message(trace_info_t *trace_info, PyObject *region) { PyUnicodeWriter *writer = PyUnicodeWriter_Create(0); if (writer == NULL) { return NULL; @@ -749,7 +807,7 @@ int _PyTracingRegion_Close(PyObject* op) { assert(gc_list_is_empty(&self->gc_list)); int res = 0; - trace_result trace_info; + trace_info_t trace_info; if (trace_object(op, &trace_info, &self->gc_list)) { goto error; // propagate Python exception } @@ -772,8 +830,6 @@ int _PyTracingRegion_Close(PyObject* op) { PyErr_SetObject(PyExc_RuntimeError, msg); Py_DECREF(msg); } - // If `msg` is NULL, building the message failed and an exception - // (e.g. MemoryError) is already set; either way we propagate it. goto error; } From 545bc5d83309cb30a8993b3833dd23771065fdd1 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Wed, 15 Jul 2026 16:55:05 +0200 Subject: [PATCH 11/24] TRegion: Add mermaid output --- Objects/tracingregionobject.c | 177 ++++++++++++++++++++++++++-------- 1 file changed, 139 insertions(+), 38 deletions(-) diff --git a/Objects/tracingregionobject.c b/Objects/tracingregionobject.c index e49e771271d71ee..771f17f2dcafe6a 100644 --- a/Objects/tracingregionobject.c +++ b/Objects/tracingregionobject.c @@ -7,6 +7,7 @@ #define ERROR_OBJECT_REPORT_COUNT 5 #define ERROR_MERMAID_REPORT_LIMIT 50 +#define ERROR_MERMAID_HIDE_IMMUTABLE true // #define REGION_TRACING @@ -304,7 +305,8 @@ typedef struct { // These are the objects with incoming references, that // should be highlighted in the graph. _Py_hashtable_t *error_objs; - // TODO(mermaid): PyUnicodeWriter *writer; + // Accumulates the mermaid edge/node definitions as the graph is traversed. + PyUnicodeWriter *writer; } mermaid_builder_t; typedef struct { @@ -328,18 +330,53 @@ typedef struct { } trace_state_t; static int mermaid_visit(PyObject* obj, trace_state_t* state) { - // TODO(mermaid): - // - Draw a reference from `state->src` to `obj` - // - if the object is immutable, give it the `immutable` class - // - if an object is inside `_Py_hashtable_t *error_objs` give it the `error` class - // - // An object node should look roughly like this: - // 0x12344321 - // RC = 10 - // [Type] - // - // In mermaid, it may look like this: - // %p[%p
rc=%ld%s], obj, obj, Py_REFCTN(obj), ("[%s]", Py_TYPE(obj)->name) + if (_Py_IsImmutable(obj) && ERROR_MERMAID_HIDE_IMMUTABLE) { + return 0; + } + + // Emit one mermaid edge `src --> obj` per reference, labelling both + // endpoints with a node of the form: + // 0x + // rc= + // [] + // Node ids are prefixed with 'n' so they always start with a letter, and + // the label is quoted so the `
` and `[...]` are not parsed as mermaid + // syntax. Mermaid dedupes repeated node definitions, so re-emitting a + // node's label on every incoming edge is harmless. + mermaid_builder_t *mermaid = state->mermaid; + PyUnicodeWriter *writer = mermaid->writer; + PyObject *src = state->src; + + if (src != NULL) { + if (PyUnicodeWriter_Format(writer, + " n%p[\"%p
rc=%zd
[%s]\"] --> ", + src, src, Py_REFCNT(src), Py_TYPE(src)->tp_name) < 0) { + return -1; + } + } + + if (PyUnicodeWriter_Format(writer, + "n%p[\"%p
rc=%zd
[%s]\"]", + obj, obj, Py_REFCNT(obj), Py_TYPE(obj)->tp_name) < 0) { + return -1; + } + + // Highlight immutable objects and the objects with outstanding incoming + // references. These two sets never overlap: immutable objects are never + // added to the trace's visited set that `error_objs` is derived from. + if (_Py_IsImmutable(obj)) { + if (PyUnicodeWriter_WriteUTF8(writer, ":::immutable", -1) < 0) { + return -1; + } + } else if (_Py_hashtable_get_entry(mermaid->error_objs, (void*)obj) != NULL) { + if (PyUnicodeWriter_WriteUTF8(writer, ":::error", -1) < 0) { + return -1; + } + } + + if (PyUnicodeWriter_WriteUTF8(writer, "\n", -1) < 0) { + return -1; + } return 0; } @@ -376,6 +413,7 @@ static int trace_state_init(trace_state_t* state, PyGC_Head *gc_list) { state->restart = false; state->gc_list = gc_list; state->src = NULL; + state->mermaid = NULL; return 0; error: @@ -390,6 +428,7 @@ static int trace_state_reset(trace_state_t* state, PyGC_Head *gc_list) { state->restart = false; state->gc_list = gc_list; state->src = NULL; + state->mermaid = NULL; return 0; error: @@ -503,7 +542,7 @@ static int _filter_visited(_Py_hashtable_t *ht, const void *key, const void *val if (value == 0) { return 0; } - if (_Py_hashtable_set(target, key, value)) { + if (_Py_hashtable_set(target, key, (void*)value)) { return -1; } if (_Py_hashtable_len(target) >= ERROR_OBJECT_REPORT_COUNT) { @@ -541,6 +580,88 @@ static int _trace_once(PyObject* obj, trace_state_t* state) { return TRACE_RES_ERR; } +// Builds a mermaid diagram of the object graph reachable from `obj` and dumps +// it to `region-graph.md`. `error_objs` holds the objects with outstanding +// incoming references, which are highlighted in the diagram. +// +// The diagram is produced by re-tracing the graph with a mermaid builder +// attached to the trace state; `mermaid_visit` then appends one edge per +// reference. No `gc_list` is passed, so no objects are moved, and by this +// point every freezable object is already frozen. +// +// Writing the file is best-effort and silently skipped if it can't be opened. +// Returns 0 on success and -1 with a Python exception set on error. +static int dump_mermaid_diagram(PyObject* obj, _Py_hashtable_t *error_objs) { + int res = -1; + mermaid_builder_t mermaid = { error_objs, NULL }; + trace_state_t state; + bool state_ready = false; + PyObject *diagram = NULL; + + mermaid.writer = PyUnicodeWriter_Create(0); + if (mermaid.writer == NULL) { + goto finally; + } + + // Top-down flowchart; `mermaid_visit` appends the edges as we traverse. + if (PyUnicodeWriter_WriteUTF8(mermaid.writer, "flowchart TD\n", -1) < 0) { + goto finally; + } + + if (trace_state_init(&state, NULL)) { + goto finally; + } + state_ready = true; + state.mermaid = &mermaid; + + if (_trace_once(obj, &state) == TRACE_RES_ERR) { + goto finally; + } + + // `PyUnicodeWriter_Finish` consumes the writer regardless of outcome. + diagram = PyUnicodeWriter_Finish(mermaid.writer); + mermaid.writer = NULL; + if (diagram == NULL) { + goto finally; + } + + const char *body = PyUnicode_AsUTF8(diagram); + if (body == NULL) { + goto finally; + } + + FILE *f = fopen("region-graph.md", "w"); + if (f != NULL) { + fputs( + "
\n" + "\n" + "```mermaid\n" + "%%{init: {'theme': 'neutral', 'themeVariables': { 'fontSize': '16px' }}}%%\n" + "\n", + f); + fputs(body, f); + fputs( + "\n" + "classDef immutable fill:#94f7ff\n" + "classDef error stroke-width:4px,stroke:red\n" + "```\n" + "
\n", + f); + fclose(f); + } + + res = 0; +finally: + if (mermaid.writer != NULL) { + PyUnicodeWriter_Discard(mermaid.writer); + } + if (state_ready) { + trace_state_destroy(&state); + } + Py_XDECREF(diagram); + return res; +} + static int trace_object(PyObject* obj, trace_info_t* result, PyGC_Head *gc_list) { // We do two tracing attempts, the first one may freeze classes and objects // and require a retrace. The second attempt should pass since all objects @@ -593,32 +714,12 @@ static int trace_object(PyObject* obj, trace_info_t* result, PyGC_Head *gc_list) goto error; } - // If the number of objects is below the limit we - // can create and dump a mermaid diagram of the graph. - if (trace_info.objs < ERROR_MERMAID_REPORT_LIMIT) { - // TODO(mermaid): Populate there mermaid state - // Then run the trace which should write the connections - - // Here are is the mermaid template: - // --- - //
- // - // ```mermaid - // %%%%{init: {'theme': 'neutral', 'themeVariables': { 'fontSize': '16px' }}}%%%% - // - // // TODO Tracing - // - // "classDef immutable fill:#94f7ff" - // "classDef error stroke-width:4px,stroke:red" - // ``` - //
- // --- - - if (trace_object(op, &trace_info, NULL)) { + // If the number of objects is below the limit we can build and dump + // a mermaid diagram of the graph to `region-graph.md` for debugging. + if (_Py_hashtable_len(state.visited) < ERROR_MERMAID_REPORT_LIMIT) { + if (dump_mermaid_diagram(obj, result->obj_table)) { goto error; // propagate Python exception } - - // TODO(mermaid): Write the diagram to `region-graph.md` } } From 8e4a04526465d400cbdcf9000a70e69e4c438dc6 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Tue, 18 Aug 2026 10:17:45 +0200 Subject: [PATCH 12/24] TRegions: Disable LRU cache for `sqlite3` Example Code: ``` import sqlite3 from immutable import TracingRegion as Region from immutable import set_freezable, FREEZABLE_YES, Cown import collections set_freezable(collections._tuplegetter, FREEZABLE_YES) c = Cown(Region()) c.value.db = sqlite3.connect("dummy.db") c.release() c.acquire() c.value.db.close() print("Done") ``` --- Modules/_sqlite/connection.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index c73e79eec243fd6..81f93c8332485bb 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -155,6 +155,11 @@ static PyObject * new_statement_cache(pysqlite_Connection *self, pysqlite_state *state, int maxsize) { + // FIXME(regions): statement cache disabled for testing. Return the connection + // itself (its tp_call creates a fresh statement) so callers of + // statement_cache(sql) bypass the functools.lru_cache wrapper. + return Py_NewRef((PyObject *)self); + PyObject *args[] = { NULL, PyLong_FromLong(maxsize), }; if (args[1] == NULL) { return NULL; From 6200c353a36cbc59421d81e958d1c27eea5b7a2f Mon Sep 17 00:00:00 2001 From: xFrednet Date: Tue, 18 Aug 2026 14:21:11 +0200 Subject: [PATCH 13/24] TRegions: Only open on attribute access --- Include/internal/pycore_immutability.h | 2 +- Lib/test/test_freeze/test_tracing_region.py | 10 ++ Objects/cownobject.c | 23 ++- Objects/tracingregionobject.c | 161 +++++++++++++++----- 4 files changed, 151 insertions(+), 45 deletions(-) diff --git a/Include/internal/pycore_immutability.h b/Include/internal/pycore_immutability.h index e696fe0542598ec..32b49580c56d775 100644 --- a/Include/internal/pycore_immutability.h +++ b/Include/internal/pycore_immutability.h @@ -10,7 +10,7 @@ extern "C" { PyAPI_DATA(PyTypeObject) _PyTracingRegion_Type; PyAPI_FUNC(int) _PyTracingRegion_Close(PyObject* region); -PyAPI_FUNC(int) _PyTracingRegion_Open(PyObject* region); +PyAPI_FUNC(int) _PyTracingRegion_IsClosed(PyObject* region); struct _Py_immutability_state { int late_init_done; diff --git a/Lib/test/test_freeze/test_tracing_region.py b/Lib/test/test_freeze/test_tracing_region.py index 79f8227445ba8fa..f15c241de4a3c70 100644 --- a/Lib/test/test_freeze/test_tracing_region.py +++ b/Lib/test/test_freeze/test_tracing_region.py @@ -84,6 +84,16 @@ class A: _, ref_count = r.trace() self.assertEqual(ref_count, base_refs + 3) +class TestRegionOpening(unittest.TestCase): + def test_open_after_acquire(self): + c = Cown(Region()) + c.value.x = [] + + c.release() + c.acquire() + + c.value.x = None + class TestImplicitFreeze(unittest.TestCase): def test_implicit_freeze_func(self): @freezable diff --git a/Objects/cownobject.c b/Objects/cownobject.c index 5e91a2f6f8a3fb4..7141b69f76ba550 100644 --- a/Objects/cownobject.c +++ b/Objects/cownobject.c @@ -2,6 +2,7 @@ #include "pymacro.h" #include "pycore_cown.h" +#include "pycore_immutability.h" #include "pycore_lock.h" #include "pycore_time.h" // _PyTime_FromSeconds() @@ -175,10 +176,6 @@ static int cown_lock(_PyCownObject* self, PyTime_t timeout, _PyCown_ipid_t locki self->locking_thread = UNSET_THREAD_ID; } - if (self->value && Region_Check(self->value)) { - _PyTracingRegion_Open(self->value); - } - return COWN_ACQUIRE_SUCCESS; } @@ -481,6 +478,23 @@ Ownership on the thread level is not enforced, any thread on the owning\n\ interpreter can access and release the cown. This is information is only\n\ provided to give more control for those who seek it."); +static PyObject * +CownObject_is_closed(_PyCownObject *self, PyObject *Py_UNUSED(dummy)) +{ + if (!Region_Check(self->value)) { + PyErr_SetString(PyExc_TypeError, "cown value is not a tracing region"); + return NULL; + } + + return PyBool_FromLong(_PyTracingRegion_IsClosed(self->value)); +} + +PyDoc_STRVAR(CownObject_is_closed_doc, +"_is_closed($self, /)\n\ +--\n\ +\n\ +Return true if the cown's tracing region value is closed."); + // Define the CownType with methods static PyMethodDef PyCown_methods[] = { @@ -489,6 +503,7 @@ static PyMethodDef PyCown_methods[] = { {"locked", _PyCFunction_CAST(CownObject_locked), METH_NOARGS, CownObject_locked_doc}, {"owned", _PyCFunction_CAST(CownObject_owned), METH_NOARGS, CownObject_owned_doc}, {"owned_by_thread", _PyCFunction_CAST(CownObject_owned_by_thread), METH_NOARGS, CownObject_owned_by_thread_doc}, + {"_is_closed", _PyCFunction_CAST(CownObject_is_closed), METH_NOARGS, CownObject_is_closed_doc}, {NULL} // Sentinel }; diff --git a/Objects/tracingregionobject.c b/Objects/tracingregionobject.c index 771f17f2dcafe6a..d65f6fb0339d42e 100644 --- a/Objects/tracingregionobject.c +++ b/Objects/tracingregionobject.c @@ -9,7 +9,7 @@ #define ERROR_MERMAID_REPORT_LIMIT 50 #define ERROR_MERMAID_HIDE_IMMUTABLE true -// #define REGION_TRACING +#define REGION_TRACING #ifdef REGION_TRACING #define if_trace(...) __VA_ARGS__ @@ -536,16 +536,30 @@ static int _trace_visit(PyObject* obj, trace_state_t* state) { return _move_obj(obj, state); } -static int _filter_visited(_Py_hashtable_t *ht, const void *key, const void *value, void *target_void) { - _Py_hashtable_t *target = (_Py_hashtable_t *)target_void; - // Only take objects with incoming references - if (value == 0) { +typedef struct { + _Py_hashtable_t *target; + PyObject *region; +} error_ref_filter; + +static int _filter_visited(_Py_hashtable_t *ht, const void *key, const void *value, void *filter_void) { + error_ref_filter *filter = (error_ref_filter *)filter_void; + Py_ssize_t refs = (Py_ssize_t)value; + + // The caller holds one owning reference to the region object itself, which + // is expected and not a reason the region couldn't be closed. Don't let it + // consume one of the limited error-report slots. + if ((PyObject *)key == filter->region) { + refs -= 1; + } + + // Only take objects with problematic incoming references. + if (refs <= 0) { return 0; } - if (_Py_hashtable_set(target, key, (void*)value)) { + if (_Py_hashtable_set(filter->target, key, (void*)refs)) { return -1; } - if (_Py_hashtable_len(target) >= ERROR_OBJECT_REPORT_COUNT) { + if (_Py_hashtable_len(filter->target) >= ERROR_OBJECT_REPORT_COUNT) { return 1; } return 0; @@ -707,7 +721,8 @@ static int trace_object(PyObject* obj, trace_info_t* result, PyGC_Head *gc_list) if (result->obj_table == NULL) { goto error; } - int for_res = _Py_hashtable_foreach(state.visited, _filter_visited, (void*)result->obj_table); + error_ref_filter filter = { result->obj_table, obj }; + int for_res = _Py_hashtable_foreach(state.visited, _filter_visited, (void*)&filter); if (for_res < -1) { _Py_hashtable_destroy(result->obj_table); result->obj_table = NULL; @@ -759,13 +774,23 @@ static void detach_weak_refs(PyGC_Head *gc_list) { typedef struct { PyObject_HEAD PyObject *dict; - // The GC list containing all objects, used during transfer + // The GC list containing all objects while the region is closed. The bridge + // object is not in this GC list but in the list of the owning region or in no + // list if it's owned by a released cown. PyGC_Head gc_list; + // FIXME(regions): This can be inferred from the status of the gc_list + // or stored in the lower bits of the GC list. For now we keep it separate + // for the prototype + bool open; } TracingRegionObject; static int TracingRegion_init(TracingRegionObject *self, PyObject *args, PyObject *kwargs) { gc_list_init(&self->gc_list); + // We make the region open by default, this ensures that the first close + // will handle the region type correctly. Alternatively, we could make them + // closed in the beginning, but then handle the cases specifically. + self->open = true; return 0; } @@ -777,6 +802,8 @@ TracingRegion_traverse(TracingRegionObject *self, visitproc visit, void *arg) { static int TracingRegion_clear(TracingRegionObject *self) { + // FIXME(regions): Special branch when closed to dealloc all + // This is deallocating a closed region, we just dissolve it if (!gc_list_is_empty(&self->gc_list)) { gc_list_dissolve(&self->gc_list); @@ -787,11 +814,84 @@ TracingRegion_clear(TracingRegionObject *self) { static void TracingRegion_dealloc(TracingRegionObject *self) { + // FIXME(regions): Special branch when closed to dealloc all + PyObject_GC_UnTrack(self); TracingRegion_clear(self); Py_TYPE(self)->tp_free((PyObject *)self); } +static void _open_region(TracingRegionObject *self) { + if (self->open) { + return; + } + + trace("Opening region %p", self); + + // This only dissolves this region, all sub-regions remain closed. + gc_list_dissolve(&self->gc_list); + assert(gc_list_is_empty(&self->gc_list)); + + self->open = true; +} + +static PyObject * +TracingRegion_getattro(PyObject *op, PyObject *name) { + TracingRegionObject *self = (TracingRegionObject*)op; + _open_region(self); + + return _PyObject_GenericGetAttrWithDict(op, name, self->dict, 0); +} + +static int +TracingRegion_setattro(PyObject *op, PyObject *name, PyObject *value) { + TracingRegionObject *self = (TracingRegionObject*)op; + _open_region(self); + + // Allocate lazily because the generic helper only stores into a provided dict. + if (self->dict == NULL) { + self->dict = PyDict_New(); + if (self->dict == NULL) { + return -1; + } + } + + return _PyObject_GenericSetAttrWithDict(op, name, value, self->dict); +} + +static PyObject * +TracingRegion_get_dict(PyObject *op, void *Py_UNUSED(context)) { + TracingRegionObject *self = (TracingRegionObject*)op; + _open_region(self); + + if (self->dict == NULL) { + self->dict = PyDict_New(); + if (self->dict == NULL) { + return NULL; + } + } + return Py_NewRef(self->dict); +} + +static int +TracingRegion_set_dict(PyObject *op, PyObject *value, void *Py_UNUSED(context)) { + TracingRegionObject *self = (TracingRegionObject*)op; + _open_region(self); + + if (value == NULL) { + PyErr_SetString(PyExc_TypeError, "cannot delete __dict__"); + return -1; + } + if (!PyDict_Check(value)) { + PyErr_Format(PyExc_TypeError, + "__dict__ must be set to a dictionary, not a '%.200s'", + Py_TYPE(value)->tp_name); + return -1; + } + Py_XSETREF(self->dict, Py_NewRef(value)); + return 0; +} + static PyObject* TracingRegion_trace(PyObject *op) { trace_info_t result; if (trace_object(op, &result, NULL)) { @@ -814,9 +914,6 @@ static PyObject* TracingRegion_trace(PyObject *op) { // "region could not be closed" error message. typedef struct { PyUnicodeWriter *writer; - // The region object being closed. One incoming reference to it is the - // expected owning reference and is not reported as a problem. - PyObject *region; // Sum of the (problematic) incoming references reported so far. Py_ssize_t accounted; } incoming_ref_report; @@ -829,16 +926,6 @@ _report_incoming_ref(_Py_hashtable_t *ht, const void *key, const void *value, vo PyObject *obj = (PyObject *)key; Py_ssize_t refs = (Py_ssize_t)value; - // The caller holds one owning reference to the region object itself, which - // is expected and not a reason the region couldn't be closed. Don't report - // it, but do report any additional references to the region. - if (obj == report->region) { - refs -= 1; - if (refs <= 0) { - return 0; - } - } - report->accounted += refs; // `%S` calls `str()` on the object. @@ -854,13 +941,13 @@ _report_incoming_ref(_Py_hashtable_t *ht, const void *key, const void *value, vo // the objects that still have incoming references. Returns a new reference to // the message string, or NULL with an exception set. static PyObject * -build_close_error_message(trace_info_t *trace_info, PyObject *region) { +build_close_error_message(trace_info_t *trace_info) { PyUnicodeWriter *writer = PyUnicodeWriter_Create(0); if (writer == NULL) { return NULL; } - incoming_ref_report report = { writer, region, 0 }; + incoming_ref_report report = { writer, 0 }; if (PyUnicodeWriter_WriteUTF8(writer, "The region could not be closed due to:\n", -1) < 0) { @@ -926,7 +1013,7 @@ int _PyTracingRegion_Close(PyObject* op) { // RuntimeError: The region could not be closed due to: // - 1 incoming reference to '[1, 2, 3]' // - 2 incoming references to '(6, 7)' - PyObject *msg = build_close_error_message(&trace_info, op); + PyObject *msg = build_close_error_message(&trace_info); if (msg != NULL) { PyErr_SetObject(PyExc_RuntimeError, msg); Py_DECREF(msg); @@ -941,6 +1028,7 @@ int _PyTracingRegion_Close(PyObject* op) { trace("- Closed region %p", self); assert(!gc_list_is_empty(&self->gc_list)); + self->open = false; res = 1; goto finally; error: @@ -953,17 +1041,9 @@ int _PyTracingRegion_Close(PyObject* op) { return res; } -/* This method opens the region by dissolving it and all objects into the - * local GC list. - * - * This function requires the GIL to be held. - */ -int _PyTracingRegion_Open(PyObject* op) { - TracingRegionObject *self = (TracingRegionObject*)op; - assert(!gc_list_is_empty(&self->gc_list)); - gc_list_dissolve(&self->gc_list); - assert(gc_list_is_empty(&self->gc_list)); - return 0; +int _PyTracingRegion_IsClosed(PyObject* region) { + TracingRegionObject *self = (TracingRegionObject*)region; + return !self->open; } static PyMethodDef TracingRegion_methods[] = { @@ -972,8 +1052,8 @@ static PyMethodDef TracingRegion_methods[] = { {NULL, NULL} /* sentinel */ }; -static PyMemberDef TracingRegion_members[] = { - {"__dict__", _Py_T_OBJECT, offsetof(TracingRegionObject, dict), Py_READONLY}, +static PyGetSetDef TracingRegion_getset[] = { + {"__dict__", TracingRegion_get_dict, TracingRegion_set_dict}, {NULL} }; @@ -985,9 +1065,10 @@ PyTypeObject _PyTracingRegion_Type = { .tp_flags = Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_GC | Py_TPFLAGS_IMMUTABLETYPE, .tp_traverse = (traverseproc)TracingRegion_traverse, .tp_clear = (inquiry)TracingRegion_clear, - .tp_members = TracingRegion_members, + .tp_getset = TracingRegion_getset, .tp_methods = TracingRegion_methods, - .tp_dictoffset = offsetof(TracingRegionObject, dict), + .tp_getattro = TracingRegion_getattro, + .tp_setattro = TracingRegion_setattro, .tp_init = (initproc)TracingRegion_init, .tp_new = PyType_GenericNew, .tp_reachable = _PyObject_ReachableVisitTypeAndTraverse, From 10af569f47d98db1a7aec31813e543f673fbb88a Mon Sep 17 00:00:00 2001 From: xFrednet Date: Tue, 18 Aug 2026 14:52:48 +0200 Subject: [PATCH 14/24] TRegions: Keep the bridge object in the GC list of the owning region --- Objects/cownobject.c | 54 +++++++++++++++++++++++++++++++------------- 1 file changed, 38 insertions(+), 16 deletions(-) diff --git a/Objects/cownobject.c b/Objects/cownobject.c index 7141b69f76ba550..0e1e263ee5da7d4 100644 --- a/Objects/cownobject.c +++ b/Objects/cownobject.c @@ -176,6 +176,11 @@ static int cown_lock(_PyCownObject* self, PyTime_t timeout, _PyCown_ipid_t locki self->locking_thread = UNSET_THREAD_ID; } + if (self->value && Region_Check(self->value)) { + assert(!PyObject_GC_IsTracked(self->value)); + PyObject_GC_Track(self->value); + } + return COWN_ACQUIRE_SUCCESS; } @@ -378,16 +383,38 @@ static int cown_check_owner_before_release(_PyCownObject *self, _PyCown_ipid_t u return 0; } -/* Try closing the region by cleaning it. - * Returns: - * (-1) If an error occurred while trying to clean the region. - * (0) If the region is still open after this call. - * (1) If the region is closed after this call. +/* This attempts to close the region + * + * It returns non-zero if the closing failed */ -static int cown_try_closing_region(_PyCownObject *self) { +static int cown_close_region(_PyCownObject *self) { assert(Region_Check(self->value)); - return _PyTracingRegion_Close(self->value); + // Close the region + int closing_res = _PyTracingRegion_Close(self->value); + if (closing_res < 0) { + return -1; + } + if (closing_res == 0) { + PyErr_Format( + PyExc_RuntimeError, + "the region in the cown couldn't be closed due to incoming references"); + 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) { @@ -395,22 +422,17 @@ static int cown_release(_PyCownObject *self, _PyCown_ipid_t unlocking_ip) { return -1; } + // Immutable objects are safe to share, the cown can be release directly if (_Py_IsImmutable(self->value)) { - // Can be released without any restrictions return cown_release_unchecked(self, unlocking_ip); } assert(Region_Check(self->value)); - int cleaning_res = cown_try_closing_region(self); - if (cleaning_res < 0) { - return -1; - } - if (cleaning_res == 0) { - PyErr_Format( - PyExc_RuntimeError, - "the cown can't be released, since the contained region is still open"); + // The contained region needs to be closed, to allow the cown to release + if (cown_close_region(self)) { return -1; } + // Region is closed, safe to release return cown_release_unchecked(self, unlocking_ip); } From e06a4996ee16f169dea59ef9aee12158f5252931 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Tue, 18 Aug 2026 15:21:03 +0200 Subject: [PATCH 15/24] F: Bugfix and doc updates --- Lib/test/test_freeze/test_tracing_region.py | 14 ++++++++++++++ Objects/tracingregionobject.c | 10 +++++++--- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_freeze/test_tracing_region.py b/Lib/test/test_freeze/test_tracing_region.py index f15c241de4a3c70..497ebe902b775d9 100644 --- a/Lib/test/test_freeze/test_tracing_region.py +++ b/Lib/test/test_freeze/test_tracing_region.py @@ -92,7 +92,21 @@ def test_open_after_acquire(self): c.release() c.acquire() + self.assertTrue(c._is_closed()) c.value.x = None + self.assertFalse(c._is_closed()) + + def test_release_closed_region(self): + c = Cown(Region()) + c.value.x = [] + + c.release() + c.acquire() + + self.assertTrue(c._is_closed()) + + c.release() + class TestImplicitFreeze(unittest.TestCase): def test_implicit_freeze_func(self): diff --git a/Objects/tracingregionobject.c b/Objects/tracingregionobject.c index d65f6fb0339d42e..30c77ef2f8a916f 100644 --- a/Objects/tracingregionobject.c +++ b/Objects/tracingregionobject.c @@ -681,6 +681,8 @@ static int trace_object(PyObject* obj, trace_info_t* result, PyGC_Head *gc_list) // and require a retrace. The second attempt should pass since all objects // should now be frozen. Pre-freeze hooks can mess with this, but consenting // adults and such. + // + // The first trace also finds sub-regions that needed to be closed before this one can. const int TRIES = 2; trace("Starting trace for %p", obj); @@ -981,9 +983,8 @@ build_close_error_message(trace_info_t *trace_info) { return NULL; } -/* This method traces the region and closes it if the caller has the only - * owning reference into the graph. The reference passed into this function - * needs to be borrowed. +/* This method traces the region and closes it, if there are no references + * pointing into the region. References to the bridge are allowed. * * This function requires the GIL to be held. * @@ -992,6 +993,9 @@ build_close_error_message(trace_info_t *trace_info) { */ int _PyTracingRegion_Close(PyObject* op) { TracingRegionObject *self = (TracingRegionObject*)op; + if (!self->open) { + return 1; + } assert(gc_list_is_empty(&self->gc_list)); int res = 0; From b0bf8fe7e525615d81df633ffd5804bb5f767cb7 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Wed, 19 Aug 2026 11:20:17 +0200 Subject: [PATCH 16/24] A compiling version --- Objects/tracingregionobject.c | 323 +++++++++++++++++++++++++++++----- 1 file changed, 276 insertions(+), 47 deletions(-) diff --git a/Objects/tracingregionobject.c b/Objects/tracingregionobject.c index 30c77ef2f8a916f..79b4ee970979c2c 100644 --- a/Objects/tracingregionobject.c +++ b/Objects/tracingregionobject.c @@ -18,15 +18,28 @@ do { \ printf(msg "\n" __VA_OPT__(,) __VA_ARGS__); \ } while(0) + +#define if_dbg(...) __VA_ARGS__ +#define dbg_arg(arg) , (Py_uintptr_t)(arg) +#define dbg(msg, ...) \ + do { \ + printf(msg "\n" __VA_OPT__(,) __VA_ARGS__); \ + } while(0) #else #define if_trace(...) #define trace_arg(...) #define trace(...) + +#define if_dbg(...) +#define dbg_arg(...) +#define dbg(...) #endif /* Macro that jumps to error, if the expression `x` does not succeed. */ #define SUCCEEDS(x) do { int r = (x); if (r != 0) goto error; } while (0) +#define Region_Check(x) Py_IS_TYPE((x), &_PyTracingRegion_Type) + // ################################################################### // Copied from gc.c // ################################################################### @@ -296,10 +309,184 @@ get_reachable_proc(PyTypeObject *tp) static void gc_list_dissolve(PyGC_Head *list) { struct _gc_runtime_state* gc_state = get_gc_state(); - //gc_list_merge(list, &(gc_state->young.head)); gc_list_merge(list, &(gc_state->old[0].head)); } +static void detach_weak_refs(PyGC_Head *gc_list) { + PyGC_Head *current = GC_NEXT(gc_list); + while (current != gc_list) { + PyObject *item = _Py_FROM_GC(current); +#ifdef PY_DEBUG + Py_ssize_t weak_ctn = _PyWeakref_GetWeakrefCount(item); + if (weak_ctn) { + trace("- Clearing %zd weak references to %p", weak_ctn, item); + } +#endif + if (_PyType_SUPPORTS_WEAKREFS(Py_TYPE(item))) { + _PyWeakref_ClearWeakRefsNoCallbacks(item); + } + + current = GC_NEXT(current); + } +} + +typedef struct { + PyObject_HEAD + PyObject *dict; + // The GC list containing all objects while the region is closed. The bridge + // object is not in this GC list but in the list of the owning region or in no + // list if it's owned by a released cown. + PyGC_Head gc_list; + // FIXME(regions): This can be inferred from the status of the gc_list + // or stored in the lower bits of the GC list. For now we keep it separate + // for the prototype + bool open; + // This is the number of references from inside the region that reference + // this bridge object. + Py_ssize_t internal_bridge_refs; +} TracingRegionObject; + +const int PER_REGION_TRACE_LIMIT = 2; + +typedef struct { + // This is the stack of pending regions needing to be closed to close + // this region tree. Objects will be inqueued `PER_REGION_TRACE_LIMIT` + // times. It the region is not closed when it hits the limit, the closing + // will fail. + PyObject *pending; + // This tracks per region in the tree how often it has been traversed. + // Some things require the trace to be redone, namely freezing an object + // as that may create references and finding an open sub-region, as that + // one needs to be traced and closed first. + // + // We limit the number of times we restart the trace per region. + // Theoretically, this may reject some programs that would eventually + // reach a fixed point, but if somebody wants to do dark magic, that's + // really not our problem. + _Py_hashtable_t *traceing_counts; +} tree_trace_state_t; + +static void tree_trace_state_destroy(tree_trace_state_t* state) { + if (state->traceing_counts) { + _Py_hashtable_destroy(state->traceing_counts); + state->traceing_counts = NULL; + } + if (state->pending) { + Py_CLEAR(state->pending); + } +} + +static int tree_trace_state_init(tree_trace_state_t* state) { + state->traceing_counts = _Py_hashtable_new( + _Py_hashtable_hash_ptr, + _Py_hashtable_compare_direct); + if (state->traceing_counts == NULL) { + goto error; + } + + state->pending = PyList_New(0); + if (state->pending == NULL) { + goto error; + } + + return 0; +error: + tree_trace_state_destroy(state); + return -1; +} + +typedef struct { + // List of pending objects that are not GC + PyObject *pending; + // A list of all visited objects + _Py_hashtable_t *visited; + + // The trace state belonging to the region tree that this region + // is a part of. + tree_trace_state_t *tree_trace_state; + // The bridge object of the region that is currently being traced. + PyObject* bridge; + // The source of the reference, this is used for error reporting + PyObject *src; + + // The number of refs coming into this object graph + Py_ssize_t external_rc; + // The number of refs coming from inside the region to the bridge object + Py_ssize_t bridge_rc; + + // The GC list used for this trace, it may be null if the trace + // should not move the objects from their current list. + PyGC_Head* gc_list; + + + // This is set if an object was frozen and the trace needs + // to restart to be valid + bool restart; +} region_trace_state_t; + +static void region_trace_state_destroy(region_trace_state_t* state) { + if (state->pending) { + Py_CLEAR(state->pending); + } + if (state->visited) { + _Py_hashtable_destroy(state->visited); + state->visited = NULL; + } +} + +static int region_trace_state_reset(region_trace_state_t* state, PyGC_Head *gc_list) { + assert(gc_list == NULL || gc_list_is_empty(gc_list)); + + SUCCEEDS(PyList_Clear(state->pending)); + _Py_hashtable_clear(state->visited); + + // state->tree_trace_state stays unchanged + // state->bridge stays unchanged + state->src = NULL; + + state->external_rc = 0; + state->bridge_rc = 0; + state->gc_list = gc_list; + state->restart = false; + + return 0; +error: + region_trace_state_destroy(state); + return -1; +} + +static int region_trace_state_init( + region_trace_state_t* state, + PyObject* bridge, + PyGC_Head* gc_list, + tree_trace_state_t *tree_trace_state +) { + state->pending = NULL; + state->visited = NULL; + + state->pending = PyList_New(0); + if (state->pending == NULL) { + goto error; + } + + state->visited = _Py_hashtable_new( + _Py_hashtable_hash_ptr, + _Py_hashtable_compare_direct); + if (state->visited == NULL) { + goto error; + } + + + state->bridge = bridge; + state->tree_trace_state = tree_trace_state; + + return region_trace_state_reset(state, gc_list); +error: + region_trace_state_destroy(state); + return -1; +} + +// TODO: Continue Migration typedef struct { // These are the objects with incoming references, that @@ -581,7 +768,7 @@ static int _trace_once(PyObject* obj, trace_state_t* state) { traverseproc proc = get_reachable_proc(Py_TYPE(item)); SUCCEEDS(proc(item, (visitproc)_trace_visit, (void*)state)); - // Weak refs need special handling + // TODO(regions): Handle weakrefs assert(!PyWeakref_Check(item)); } @@ -751,41 +938,103 @@ static int trace_object(PyObject* obj, trace_info_t* result, PyGC_Head *gc_list) return res; } -static void detach_weak_refs(PyGC_Head *gc_list) { - PyGC_Head *current = GC_NEXT(gc_list); - while (current != gc_list) { - PyObject *item = _Py_FROM_GC(current); -#ifdef PY_DEBUG - Py_ssize_t weak_ctn = _PyWeakref_GetWeakrefCount(item); - if (weak_ctn) { - trace("- Clearing %zd weak references to %p", weak_ctn, item); +static int _try_close_region(PyObject *region_obj, tree_trace_state_t *tree_trace_state) { + assert(Region_Check(region_obj)); + TracingRegionObject* region = (TracingRegionObject*)region_obj; + + // Init trace state. + region_trace_state_t state; + if (region_trace_state_init(&state, _PyObject_CAST(region), ®ion->gc_list, tree_trace_state)) { + return TRACE_RES_ERR; + } + int region_trace_res = TRACE_RES_DONE; + + while (PyList_GET_SIZE(state.pending) > 0) { + // Find the next pending item: + PyObject *item = list_pop(state.pending); + + // Traverse item + state.src = item; + trace(" - traversing %p", item); + traverseproc proc = get_reachable_proc(Py_TYPE(item)); + SUCCEEDS(proc(item, (visitproc)_trace_visit, (void*)&state)); + + // TODO(regions): Handle weakrefs + assert(!PyWeakref_Check(item)); + } + + goto finally; +error: + region_trace_res = TRACE_RES_RESTART; +finally: + region_trace_state_destroy(&state); + + return region_trace_res; +} + +static int try_close_region_tree(PyObject *root) { + dbg("Starting region tree trace from %p", root); + + tree_trace_state_t state; + if (tree_trace_state_init(&state)) { + return -1; + } + + SUCCEEDS(PyList_Append(state.pending, root)); + + int tree_trace_res = TRACE_RES_DONE; + while (PyList_GET_SIZE(state.pending) > 0) { + // Find the next pending item: + PyObject *region = list_pop(state.pending); + assert(Region_Check(region)); + + // If the region is closed we can safely skip it. Regions can be enqueued + // multiple times, this handles all safe cases. + if (_PyTracingRegion_IsClosed(region)) { + continue; } -#endif - if (_PyType_SUPPORTS_WEAKREFS(Py_TYPE(item))) { - _PyWeakref_ClearWeakRefsNoCallbacks(item); + + dbg("- tracing region %p", region); + int res = _try_close_region(region, &state); + if (res == TRACE_RES_ERR) { + goto error; } - current = GC_NEXT(current); + _Py_hashtable_entry_t *entry = _Py_hashtable_get_entry(state.traceing_counts, (void*)region); + if (entry != NULL) { + if ((Py_uintptr_t)entry->value < PER_REGION_TRACE_LIMIT) { + entry->value = (void*)(((Py_uintptr_t)entry->value) + 1); + } else { + // FIXME(regions): This should maybe be turned into a trace that creates a + // error, the problem is, that this retrace may then close the region. This + // means that this increase the tracing limit by one. There is also a question + // how often this actually happens. This case is pretty specific for sub-regions + // that can't be closed and pre-freeze hooks + PyErr_Format( + PyExc_RuntimeError, + "the region %p could not be closed after %d tracing attempts", + (void *)region, + PER_REGION_TRACE_LIMIT); + goto error; + } + } else { + SUCCEEDS(_Py_hashtable_set(state.traceing_counts, (void*)region, (void*)1)); + } } + + goto finally; +error: + tree_trace_res = TRACE_RES_ERR; +finally: + tree_trace_state_destroy(&state); + + return tree_trace_res; } // ################################################################### // Region Object // ################################################################### -typedef struct { - PyObject_HEAD - PyObject *dict; - // The GC list containing all objects while the region is closed. The bridge - // object is not in this GC list but in the list of the owning region or in no - // list if it's owned by a released cown. - PyGC_Head gc_list; - // FIXME(regions): This can be inferred from the status of the gc_list - // or stored in the lower bits of the GC list. For now we keep it separate - // for the prototype - bool open; -} TracingRegionObject; - static int TracingRegion_init(TracingRegionObject *self, PyObject *args, PyObject *kwargs) { gc_list_init(&self->gc_list); @@ -894,24 +1143,6 @@ TracingRegion_set_dict(PyObject *op, PyObject *value, void *Py_UNUSED(context)) return 0; } -static PyObject* TracingRegion_trace(PyObject *op) { - trace_info_t result; - if (trace_object(op, &result, NULL)) { - return NULL; // propagate Python exception - } - - if (result.obj_table != NULL) { - _Py_hashtable_destroy(result.obj_table); - } - - PyObject *t = Py_BuildValue("(ii)", result.objs, result.incoming_refs); - if (t == NULL) { - return NULL; // propagate Python exception - } - - return t; -} - // State threaded through `_report_incoming_ref` while building the // "region could not be closed" error message. typedef struct { @@ -1051,8 +1282,6 @@ int _PyTracingRegion_IsClosed(PyObject* region) { } static PyMethodDef TracingRegion_methods[] = { - {"trace", _PyCFunction_CAST(TracingRegion_trace), METH_NOARGS, - "This traces the region and returns the number of incoming references"}, {NULL, NULL} /* sentinel */ }; From ebb95051fb228037cba310a193a29162b235f658 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Wed, 19 Aug 2026 13:44:31 +0200 Subject: [PATCH 17/24] Seemingly a working rewrite --- Objects/cownobject.c | 8 +- Objects/tracingregionobject.c | 188 +++++++++++++++++++++++++++------- 2 files changed, 151 insertions(+), 45 deletions(-) diff --git a/Objects/cownobject.c b/Objects/cownobject.c index 0e1e263ee5da7d4..ada9239995e35b3 100644 --- a/Objects/cownobject.c +++ b/Objects/cownobject.c @@ -24,6 +24,8 @@ typedef enum CownLockStatus { COWN_ACQUIRE_SUCCESS = 1 } CownLockStatus; +// Cowns rely on the immutability machinery for atomic reference counting: +// PyCown_init() freezes each instance once its initial value is installed. struct _PyCownObject { PyObject_HEAD /* The id of the interpreter that currently owns this cown. @@ -395,12 +397,6 @@ static int cown_close_region(_PyCownObject *self) { if (closing_res < 0) { return -1; } - if (closing_res == 0) { - PyErr_Format( - PyExc_RuntimeError, - "the region in the cown couldn't be closed due to incoming references"); - return -1; - } // Make sure that the cown owns the only external reference to the bridge object. if (Py_REFCNT(self->value) > 1) { diff --git a/Objects/tracingregionobject.c b/Objects/tracingregionobject.c index 79b4ee970979c2c..8eb21b55d33b84a 100644 --- a/Objects/tracingregionobject.c +++ b/Objects/tracingregionobject.c @@ -4,6 +4,7 @@ #include "pycore_object.h" // _PyObject_GC_TRACK(), _PyDebugAllocatorStats() #include "pycore_descrobject.h" #include "pycore_weakref.h" +#include "pycore_cown.h" #define ERROR_OBJECT_REPORT_COUNT 5 #define ERROR_MERMAID_REPORT_LIMIT 50 @@ -39,6 +40,7 @@ #define SUCCEEDS(x) do { int r = (x); if (r != 0) goto error; } while (0) #define Region_Check(x) Py_IS_TYPE((x), &_PyTracingRegion_Type) +#define Cown_Check(x) Py_IS_TYPE((x), &_PyCown_Type) // ################################################################### // Copied from gc.c @@ -151,7 +153,11 @@ static PyObject* list_pop(PyObject* s){ typedef enum { Py_MOVABLE_YES = 0, Py_MOVABLE_NO = 1, + // The object should be frozen Py_MOVABLE_FREEZE = 2, + // The object is not movable, but the reference is allowed. The object + // should be skipped + Py_MOVABLE_COWN = 3, } movable_status; movable_status get_movable_status(PyObject *obj) { @@ -203,6 +209,11 @@ movable_status get_movable_status(PyObject *obj) { return Py_MOVABLE_FREEZE; } + // Cowns are not movable, but the reference is explicitly allowed. + if (Cown_Check(obj)) { + return Py_MOVABLE_COWN; + } + // Freezing or moving these objects is... complicated. In some cases it is // possible but more hassle than it's probably worth. For not we mark them // all as unmovable. @@ -225,6 +236,9 @@ movable_status get_movable_status(PyObject *obj) { return Py_MOVABLE_NO; } + // Regions are theoretically only movable, if they're closed. The traversal + // checks this manually. + // For now, we define all other objects as movable by default. (Surely // this will not backfire) return Py_MOVABLE_YES; @@ -346,6 +360,52 @@ typedef struct { Py_ssize_t internal_bridge_refs; } TracingRegionObject; +static void _region_close(TracingRegionObject *self, Py_ssize_t bridge_rc) { + if (!self->open) { + return; + } + + dbg("Closing region %p", self); + + // FIXME: This can be optimized, for example by inserting all objects + // with weak refs in the beginning. + detach_weak_refs(&self->gc_list); + + // TODO(regions): explain RC magic + if (bridge_rc != 0) { + assert(bridge_rc >= 0); + dbg("- subtracting %ld internal references from the bridge object %p", bridge_rc, self); + _Py_RefcntAdd(self, -bridge_rc); + self->internal_bridge_refs = bridge_rc; + } else { + assert(self->internal_bridge_refs == 0); + } + + self->open = false; +} + +static void _open_region(TracingRegionObject *self) { + if (self->open) { + return; + } + + dbg("Opening region %p", self); + + // We re-add the internal references to the RC that have been subtracted during closing. + if (self->internal_bridge_refs != 0) { + assert(self->internal_bridge_refs >= 0); + dbg("- adding %ld internal references from the bridge object %p", self->internal_bridge_refs, self); + _Py_RefcntAdd(self, self->internal_bridge_refs); + self->internal_bridge_refs = 0; + } + + // This only dissolves this region, all sub-regions remain closed. + gc_list_dissolve(&self->gc_list); + assert(gc_list_is_empty(&self->gc_list)); + + self->open = true; +} + const int PER_REGION_TRACE_LIMIT = 2; typedef struct { @@ -486,6 +546,14 @@ static int region_trace_state_init( return -1; } +static void region_trace_state_set_restart(region_trace_state_t* state) { + state->restart = true; + // Setting the gc_list to NULL will stop objects from being moved + // between GC lists. Just a small thing we can avoid. The next (full) + // trace will have this set again. + state->gc_list = NULL; +} + // TODO: Continue Migration typedef struct { @@ -634,14 +702,14 @@ const int TRACE_RES_ERR = -1; const int TRACE_RES_DONE = 0; const int TRACE_RES_RESTART = 1; -static int _move_obj(PyObject* obj, trace_state_t* state) { +static int _move_obj(PyObject* obj, region_trace_state_t* state) { // Check the movability of the object: movable_status status = get_movable_status(obj); switch (status) { case Py_MOVABLE_YES: break; case Py_MOVABLE_NO: - trace(" - %p is not movable", obj); + dbg(" - %p is not movable", obj); throw_region_error( "Instances of type '%s' are not movable", Py_TYPE(obj)->tp_name, state->src, obj); @@ -649,29 +717,23 @@ static int _move_obj(PyObject* obj, trace_state_t* state) { case Py_MOVABLE_FREEZE: // Freeze the object, this can invalidate our `external_rc`, // we restart after this trace - trace(" - freezing %p", obj); + dbg(" - freezing %p", obj); if (_PyImmutability_Freeze(obj)) { return TRACE_RES_ERR; } - state->restart = true; - // Setting the gc_list to NULL will stop objects from being moved - // between GC lists. Just a small thing we can avoid. The next (full) - // trace will have this set again. - state->gc_list = NULL; + region_trace_state_set_restart(state); + return 0; + case Py_MOVABLE_COWN: return 0; default: assert(false); break; } - // Move the object - Py_ssize_t lrc_change = Py_REFCNT(obj); - if (state->src != NULL) { - // -1 for the reference we just followed - lrc_change -= 1; - } - trace(" - moving %p; LRC += %zd", obj, lrc_change); + // Update the LRC, -1 for the reference we just followed + Py_ssize_t lrc_change = Py_REFCNT(obj) - 1; + dbg(" - moving %p; LRC += %zd", obj, lrc_change); state->external_rc += lrc_change; // Mark the object as visited, this stores the lrc_change for better error reporting @@ -691,31 +753,64 @@ static int _move_obj(PyObject* obj, trace_state_t* state) { gc_list_move(_Py_AS_GC(obj), state->gc_list); } - if (PyList_Append(state->pending, obj)) { - return -1; + // Bridge objects of sub-regions are moved, but shouldn't be traversed. + if (!Region_Check(obj)) { + if (PyList_Append(state->pending, obj)) { + return -1; + } } return 0; } -static int _trace_visit(PyObject* obj, trace_state_t* state) { - if (state->mermaid) { - if (mermaid_visit(obj, state)) { +static int +_enqueue_region_for_closing(tree_trace_state_t *state, PyObject *region) +{ + for (int i = 0; i < PER_REGION_TRACE_LIMIT; i++) { + if (PyList_Append(state->pending, region) < 0) { return -1; } } + return 0; +} +static int _trace_visit(PyObject* obj, region_trace_state_t* state) { // References to immutable objects are allowed if (_PyImmutability_CanViewAsImmutable(obj)) { assert(_Py_IsImmutable(obj)); return 0; } + // References to the bridge are tracked separately + if (obj == state->bridge) { + // This branch also accounts for references from the bridge object to itself. + dbg(" - Internal reference to bridge from %p; bridge_rc += 1", state->src); + state->bridge_rc += 1; + return 0; + } + + // References external regions turns them into sub-regions. These + // need to be traversed and closed separately + if (Region_Check(obj)) { + if (_PyTracingRegion_IsClosed(obj)) { + // If the child region is closed we can move it directly + return _move_obj(obj, state); + } else { + // The child region is open, we need to traverse it first and then + // retry closing this. + if (_enqueue_region_for_closing(state->tree_trace_state, obj) < 0) { + return -1; + } + region_trace_state_set_restart(state); + } + return 0; + } + // Check if the object is already part of the region _Py_hashtable_entry_t *entry = _Py_hashtable_get_entry(state->visited, (void*)obj); if (entry != NULL) { entry->value -= 1; - trace(" - Internal reference to %p; LRC -= 1", obj); + dbg(" - Internal reference to %p; LRC -= 1", obj); state->external_rc -= 1; return 0; } @@ -949,6 +1044,8 @@ static int _try_close_region(PyObject *region_obj, tree_trace_state_t *tree_trac } int region_trace_res = TRACE_RES_DONE; + SUCCEEDS(PyList_Append(state.pending, _PyObject_CAST(region))); + while (PyList_GET_SIZE(state.pending) > 0) { // Find the next pending item: PyObject *item = list_pop(state.pending); @@ -963,9 +1060,28 @@ static int _try_close_region(PyObject *region_obj, tree_trace_state_t *tree_trac assert(!PyWeakref_Check(item)); } + if (state.restart) { + goto finally; + } + + if (state.external_rc == 0) { + _region_close(region, state.bridge_rc); + } else { + dbg("- Failed to close region %p, there are %zd incoming references", region, state.external_rc); + PyErr_Format( + PyExc_RuntimeError, + "Failed to close region %p, there are %zd incoming references", + region, + state.external_rc); + gc_list_dissolve(®ion->gc_list); + assert(gc_list_is_empty(®ion->gc_list)); + + goto error; + } + goto finally; error: - region_trace_res = TRACE_RES_RESTART; + region_trace_res = TRACE_RES_ERR; finally: region_trace_state_destroy(&state); @@ -980,7 +1096,7 @@ static int try_close_region_tree(PyObject *root) { return -1; } - SUCCEEDS(PyList_Append(state.pending, root)); + _enqueue_region_for_closing(&state, root); int tree_trace_res = TRACE_RES_DONE; while (PyList_GET_SIZE(state.pending) > 0) { @@ -1072,20 +1188,6 @@ TracingRegion_dealloc(TracingRegionObject *self) { Py_TYPE(self)->tp_free((PyObject *)self); } -static void _open_region(TracingRegionObject *self) { - if (self->open) { - return; - } - - trace("Opening region %p", self); - - // This only dissolves this region, all sub-regions remain closed. - gc_list_dissolve(&self->gc_list); - assert(gc_list_is_empty(&self->gc_list)); - - self->open = true; -} - static PyObject * TracingRegion_getattro(PyObject *op, PyObject *name) { TracingRegionObject *self = (TracingRegionObject*)op; @@ -1219,8 +1321,7 @@ build_close_error_message(trace_info_t *trace_info) { * * This function requires the GIL to be held. * - * Returns -1 if an exception was raised. 0 if the region couldn't be closed - * and 1 if the region was closed. + * Returns -1 if an exception was raised. 0 if the region could be closed. */ int _PyTracingRegion_Close(PyObject* op) { TracingRegionObject *self = (TracingRegionObject*)op; @@ -1229,6 +1330,15 @@ int _PyTracingRegion_Close(PyObject* op) { } assert(gc_list_is_empty(&self->gc_list)); + return try_close_region_tree(self); +} + +int __old_PyTracingRegion_Close(PyObject* op) { + TracingRegionObject *self = (TracingRegionObject*)op; + if (!self->open) { + return 1; + } + assert(gc_list_is_empty(&self->gc_list)); int res = 0; trace_info_t trace_info; if (trace_object(op, &trace_info, &self->gc_list)) { From 16cf7c1ccfad1585a766bd2e3b9e39b4aa1e6ca7 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Wed, 19 Aug 2026 14:00:00 +0200 Subject: [PATCH 18/24] TRegions --- Lib/test/test_freeze/test_tracing_region.py | 71 ++- Objects/tracingregionobject.c | 505 +------------------- 2 files changed, 37 insertions(+), 539 deletions(-) diff --git a/Lib/test/test_freeze/test_tracing_region.py b/Lib/test/test_freeze/test_tracing_region.py index 497ebe902b775d9..ec386c9a1d4b4b5 100644 --- a/Lib/test/test_freeze/test_tracing_region.py +++ b/Lib/test/test_freeze/test_tracing_region.py @@ -63,26 +63,6 @@ def test_release_error(self): l = None c.release() - def test_trace(self): - @freezable - class A: - pass - - r = Region() - r.a = A() - r.b = A() - r.c = A() - - _, base_refs = r.trace() - - a = r.a - _, ref_count = r.trace() - self.assertEqual(ref_count, base_refs + 1) - - b = r.b - c = r.c - _, ref_count = r.trace() - self.assertEqual(ref_count, base_refs + 3) class TestRegionOpening(unittest.TestCase): def test_open_after_acquire(self): @@ -113,48 +93,53 @@ def test_implicit_freeze_func(self): @freezable def some_func(): pass - r = Region() + c = Cown(Region()) - r.obj = some_func - self.assertFalse(is_frozen(r.obj)) - r.trace() - self.assertTrue(is_frozen(r.obj)) + c.value.obj = some_func + self.assertFalse(is_frozen(c.value.obj)) + c.release() + c.acquire() + self.assertTrue(is_frozen(c.value.obj)) def test_implicit_freeze_type(self): @freezable class A: pass - r = Region() + c = Cown(Region()) - r.obj = A - self.assertFalse(is_frozen(r.obj)) - r.trace() - self.assertTrue(is_frozen(r.obj)) + c.value.obj = A + self.assertFalse(is_frozen(c.value.obj)) + c.release() + c.acquire() + self.assertTrue(is_frozen(c.value.obj)) def test_implicit_freeze_module(self): import random; - r = Region() + c = Cown(Region()) - r.obj = random - self.assertFalse(is_frozen(r.obj)) - r.trace() - self.assertTrue(is_frozen(r.obj)) + c.value.obj = random + self.assertFalse(is_frozen(c.value.obj)) + c.release() + c.acquire() + self.assertTrue(is_frozen(c.value.obj)) # Unimport module sys.modules.pop("random", None) sys.mut_modules.pop("random", None) def test_implicit_freeze_str(self): - r = Region() + c = Cown(Region()) - r.obj = "Ducks are cool" - r.trace() - self.assertTrue(is_frozen(r.obj)) + c.value.obj = "Ducks are cool" + c.release() + c.acquire() + self.assertTrue(is_frozen(c.value.obj)) def test_implicit_freeze_int(self): - r = Region() + c = Cown(Region()) - r.obj = 17 - r.trace() - self.assertTrue(is_frozen(r.obj)) + c.value.obj = 17 + c.release() + c.acquire() + self.assertTrue(is_frozen(c.value.obj)) diff --git a/Objects/tracingregionobject.c b/Objects/tracingregionobject.c index 8eb21b55d33b84a..40a4e6a5b02bece 100644 --- a/Objects/tracingregionobject.c +++ b/Objects/tracingregionobject.c @@ -13,13 +13,6 @@ #define REGION_TRACING #ifdef REGION_TRACING -#define if_trace(...) __VA_ARGS__ -#define trace_arg(arg) , (Py_uintptr_t)(arg) -#define trace(msg, ...) \ - do { \ - printf(msg "\n" __VA_OPT__(,) __VA_ARGS__); \ - } while(0) - #define if_dbg(...) __VA_ARGS__ #define dbg_arg(arg) , (Py_uintptr_t)(arg) #define dbg(msg, ...) \ @@ -27,10 +20,6 @@ printf(msg "\n" __VA_OPT__(,) __VA_ARGS__); \ } while(0) #else -#define if_trace(...) -#define trace_arg(...) -#define trace(...) - #define if_dbg(...) #define dbg_arg(...) #define dbg(...) @@ -333,7 +322,7 @@ static void detach_weak_refs(PyGC_Head *gc_list) { #ifdef PY_DEBUG Py_ssize_t weak_ctn = _PyWeakref_GetWeakrefCount(item); if (weak_ctn) { - trace("- Clearing %zd weak references to %p", weak_ctn, item); + dbg("- Clearing %zd weak references to %p", weak_ctn, item); } #endif if (_PyType_SUPPORTS_WEAKREFS(Py_TYPE(item))) { @@ -554,144 +543,6 @@ static void region_trace_state_set_restart(region_trace_state_t* state) { state->gc_list = NULL; } -// TODO: Continue Migration - -typedef struct { - // These are the objects with incoming references, that - // should be highlighted in the graph. - _Py_hashtable_t *error_objs; - // Accumulates the mermaid edge/node definitions as the graph is traversed. - PyUnicodeWriter *writer; -} mermaid_builder_t; - -typedef struct { - /// A list of all visited objects - _Py_hashtable_t *visited; - /// The number of refs coming into this object graph - Py_ssize_t external_rc; - // The GC list used for this trace, it may be null if the trace - // should not move the objects from their current list. - PyGC_Head* gc_list; - // The source of the reference, this is used for error reporting - PyObject *src; - // List of pending objects that are not GC - PyObject *pending; - // Used to build a mermaid diagram for error reporting if - // the field is not NULL. - mermaid_builder_t *mermaid; - // This is set if an object was frozen and the trace needs - // to restart to be valid - bool restart; -} trace_state_t; - -static int mermaid_visit(PyObject* obj, trace_state_t* state) { - if (_Py_IsImmutable(obj) && ERROR_MERMAID_HIDE_IMMUTABLE) { - return 0; - } - - // Emit one mermaid edge `src --> obj` per reference, labelling both - // endpoints with a node of the form: - // 0x - // rc= - // [] - // Node ids are prefixed with 'n' so they always start with a letter, and - // the label is quoted so the `
` and `[...]` are not parsed as mermaid - // syntax. Mermaid dedupes repeated node definitions, so re-emitting a - // node's label on every incoming edge is harmless. - mermaid_builder_t *mermaid = state->mermaid; - PyUnicodeWriter *writer = mermaid->writer; - PyObject *src = state->src; - - if (src != NULL) { - if (PyUnicodeWriter_Format(writer, - " n%p[\"%p
rc=%zd
[%s]\"] --> ", - src, src, Py_REFCNT(src), Py_TYPE(src)->tp_name) < 0) { - return -1; - } - } - - if (PyUnicodeWriter_Format(writer, - "n%p[\"%p
rc=%zd
[%s]\"]", - obj, obj, Py_REFCNT(obj), Py_TYPE(obj)->tp_name) < 0) { - return -1; - } - - // Highlight immutable objects and the objects with outstanding incoming - // references. These two sets never overlap: immutable objects are never - // added to the trace's visited set that `error_objs` is derived from. - if (_Py_IsImmutable(obj)) { - if (PyUnicodeWriter_WriteUTF8(writer, ":::immutable", -1) < 0) { - return -1; - } - } else if (_Py_hashtable_get_entry(mermaid->error_objs, (void*)obj) != NULL) { - if (PyUnicodeWriter_WriteUTF8(writer, ":::error", -1) < 0) { - return -1; - } - } - - if (PyUnicodeWriter_WriteUTF8(writer, "\n", -1) < 0) { - return -1; - } - - return 0; -} - -static void trace_state_destroy(trace_state_t* state) { - if (state->visited) { - _Py_hashtable_destroy(state->visited); - state->visited = NULL; - } - if (state->pending) { - Py_DECREF(state->pending); - state->pending = NULL; - } -} -static int trace_state_init(trace_state_t* state, PyGC_Head *gc_list) { - assert(gc_list == NULL || gc_list_is_empty(gc_list)); - - state->visited = NULL; - state->pending = NULL; - - state->visited = _Py_hashtable_new( - _Py_hashtable_hash_ptr, - _Py_hashtable_compare_direct); - if (state->visited == NULL) { - goto error; - } - - state->pending = PyList_New(0); - if (state->pending == NULL) { - goto error; - } - - state->external_rc = 0; - state->restart = false; - state->gc_list = gc_list; - state->src = NULL; - state->mermaid = NULL; - - return 0; -error: - trace_state_destroy(state); - return -1; -} -static int trace_state_reset(trace_state_t* state, PyGC_Head *gc_list) { - _Py_hashtable_clear(state->visited); - SUCCEEDS(PyList_Clear(state->pending)); - - state->external_rc = 0; - state->restart = false; - state->gc_list = gc_list; - state->src = NULL; - state->mermaid = NULL; - - return 0; -error: - trace_state_destroy(state); - return -1; -} - - typedef struct { _Py_hashtable_t *obj_table; Py_ssize_t objs; @@ -700,7 +551,6 @@ typedef struct { const int TRACE_RES_ERR = -1; const int TRACE_RES_DONE = 0; -const int TRACE_RES_RESTART = 1; static int _move_obj(PyObject* obj, region_trace_state_t* state) { // Check the movability of the object: @@ -818,220 +668,6 @@ static int _trace_visit(PyObject* obj, region_trace_state_t* state) { return _move_obj(obj, state); } -typedef struct { - _Py_hashtable_t *target; - PyObject *region; -} error_ref_filter; - -static int _filter_visited(_Py_hashtable_t *ht, const void *key, const void *value, void *filter_void) { - error_ref_filter *filter = (error_ref_filter *)filter_void; - Py_ssize_t refs = (Py_ssize_t)value; - - // The caller holds one owning reference to the region object itself, which - // is expected and not a reason the region couldn't be closed. Don't let it - // consume one of the limited error-report slots. - if ((PyObject *)key == filter->region) { - refs -= 1; - } - - // Only take objects with problematic incoming references. - if (refs <= 0) { - return 0; - } - if (_Py_hashtable_set(filter->target, key, (void*)refs)) { - return -1; - } - if (_Py_hashtable_len(filter->target) >= ERROR_OBJECT_REPORT_COUNT) { - return 1; - } - return 0; -} - -static int _trace_once(PyObject* obj, trace_state_t* state) { - trace(" - starting trace from %p", obj); - int res = TRACE_RES_DONE; - - SUCCEEDS(_move_obj(obj, state)); - - while (PyList_GET_SIZE(state->pending) > 0) { - // Find the next pending item: - PyObject *item = list_pop(state->pending); - - // Traverse item - state->src = item; - trace(" - traversing %p", item); - traverseproc proc = get_reachable_proc(Py_TYPE(item)); - SUCCEEDS(proc(item, (visitproc)_trace_visit, (void*)state)); - - // TODO(regions): Handle weakrefs - assert(!PyWeakref_Check(item)); - } - - if (state->restart) { - res = TRACE_RES_RESTART; - } - - return res; -error: - return TRACE_RES_ERR; -} - -// Builds a mermaid diagram of the object graph reachable from `obj` and dumps -// it to `region-graph.md`. `error_objs` holds the objects with outstanding -// incoming references, which are highlighted in the diagram. -// -// The diagram is produced by re-tracing the graph with a mermaid builder -// attached to the trace state; `mermaid_visit` then appends one edge per -// reference. No `gc_list` is passed, so no objects are moved, and by this -// point every freezable object is already frozen. -// -// Writing the file is best-effort and silently skipped if it can't be opened. -// Returns 0 on success and -1 with a Python exception set on error. -static int dump_mermaid_diagram(PyObject* obj, _Py_hashtable_t *error_objs) { - int res = -1; - mermaid_builder_t mermaid = { error_objs, NULL }; - trace_state_t state; - bool state_ready = false; - PyObject *diagram = NULL; - - mermaid.writer = PyUnicodeWriter_Create(0); - if (mermaid.writer == NULL) { - goto finally; - } - - // Top-down flowchart; `mermaid_visit` appends the edges as we traverse. - if (PyUnicodeWriter_WriteUTF8(mermaid.writer, "flowchart TD\n", -1) < 0) { - goto finally; - } - - if (trace_state_init(&state, NULL)) { - goto finally; - } - state_ready = true; - state.mermaid = &mermaid; - - if (_trace_once(obj, &state) == TRACE_RES_ERR) { - goto finally; - } - - // `PyUnicodeWriter_Finish` consumes the writer regardless of outcome. - diagram = PyUnicodeWriter_Finish(mermaid.writer); - mermaid.writer = NULL; - if (diagram == NULL) { - goto finally; - } - - const char *body = PyUnicode_AsUTF8(diagram); - if (body == NULL) { - goto finally; - } - - FILE *f = fopen("region-graph.md", "w"); - if (f != NULL) { - fputs( - "
\n" - "\n" - "```mermaid\n" - "%%{init: {'theme': 'neutral', 'themeVariables': { 'fontSize': '16px' }}}%%\n" - "\n", - f); - fputs(body, f); - fputs( - "\n" - "classDef immutable fill:#94f7ff\n" - "classDef error stroke-width:4px,stroke:red\n" - "```\n" - "
\n", - f); - fclose(f); - } - - res = 0; -finally: - if (mermaid.writer != NULL) { - PyUnicodeWriter_Discard(mermaid.writer); - } - if (state_ready) { - trace_state_destroy(&state); - } - Py_XDECREF(diagram); - return res; -} - -static int trace_object(PyObject* obj, trace_info_t* result, PyGC_Head *gc_list) { - // We do two tracing attempts, the first one may freeze classes and objects - // and require a retrace. The second attempt should pass since all objects - // should now be frozen. Pre-freeze hooks can mess with this, but consenting - // adults and such. - // - // The first trace also finds sub-regions that needed to be closed before this one can. - const int TRIES = 2; - trace("Starting trace for %p", obj); - - // Init trace state. - trace_state_t state; - if (trace_state_init(&state, gc_list)) { - return TRACE_RES_ERR; - } - - result->obj_table = NULL; - - int res = 0; - for (int i = 0; i < TRIES; i++) { - SUCCEEDS(trace_state_reset(&state, gc_list)); - - // Trace object - res = _trace_once(obj, &state); - - // Restart trace on demand - if (res == TRACE_RES_RESTART) { - trace("- restarting trace for %p", obj); - if (gc_list != NULL) { - gc_list_dissolve(gc_list); - assert(gc_list_is_empty(gc_list)); - } - continue; - } - - break; - } - - // The region can't be closed, we'll collect some extra meta data for - // a better error message. - if (state.external_rc > 1) { - result->obj_table = _Py_hashtable_new( - _Py_hashtable_hash_ptr, - _Py_hashtable_compare_direct); - if (result->obj_table == NULL) { - goto error; - } - error_ref_filter filter = { result->obj_table, obj }; - int for_res = _Py_hashtable_foreach(state.visited, _filter_visited, (void*)&filter); - if (for_res < -1) { - _Py_hashtable_destroy(result->obj_table); - result->obj_table = NULL; - goto error; - } - - // If the number of objects is below the limit we can build and dump - // a mermaid diagram of the graph to `region-graph.md` for debugging. - if (_Py_hashtable_len(state.visited) < ERROR_MERMAID_REPORT_LIMIT) { - if (dump_mermaid_diagram(obj, result->obj_table)) { - goto error; // propagate Python exception - } - } - } - - goto finally; -error: - res = TRACE_RES_RESTART; -finally: - result->incoming_refs = state.external_rc; - result->objs = _Py_hashtable_len(state.visited); - trace_state_destroy(&state); - - return res; -} static int _try_close_region(PyObject *region_obj, tree_trace_state_t *tree_trace_state) { assert(Region_Check(region_obj)); @@ -1052,7 +688,7 @@ static int _try_close_region(PyObject *region_obj, tree_trace_state_t *tree_trac // Traverse item state.src = item; - trace(" - traversing %p", item); + dbg(" - traversing %p", item); traverseproc proc = get_reachable_proc(Py_TYPE(item)); SUCCEEDS(proc(item, (visitproc)_trace_visit, (void*)&state)); @@ -1060,22 +696,22 @@ static int _try_close_region(PyObject *region_obj, tree_trace_state_t *tree_trac assert(!PyWeakref_Check(item)); } - if (state.restart) { - goto finally; - } - if (state.external_rc == 0) { _region_close(region, state.bridge_rc); } else { + gc_list_dissolve(®ion->gc_list); + assert(gc_list_is_empty(®ion->gc_list)); + + if (state.restart) { + goto finally; + } + dbg("- Failed to close region %p, there are %zd incoming references", region, state.external_rc); PyErr_Format( PyExc_RuntimeError, "Failed to close region %p, there are %zd incoming references", region, state.external_rc); - gc_list_dissolve(®ion->gc_list); - assert(gc_list_is_empty(®ion->gc_list)); - goto error; } @@ -1245,76 +881,6 @@ TracingRegion_set_dict(PyObject *op, PyObject *value, void *Py_UNUSED(context)) return 0; } -// State threaded through `_report_incoming_ref` while building the -// "region could not be closed" error message. -typedef struct { - PyUnicodeWriter *writer; - // Sum of the (problematic) incoming references reported so far. - Py_ssize_t accounted; -} incoming_ref_report; - -// `_Py_hashtable_foreach` callback over `trace_info.obj_table`. Appends one -// "- N incoming reference(s) to 'obj'" line per object to the writer. -static int -_report_incoming_ref(_Py_hashtable_t *ht, const void *key, const void *value, void *user_data) { - incoming_ref_report *report = (incoming_ref_report *)user_data; - PyObject *obj = (PyObject *)key; - Py_ssize_t refs = (Py_ssize_t)value; - - report->accounted += refs; - - // `%S` calls `str()` on the object. - if (PyUnicodeWriter_Format(report->writer, - "- %zd incoming reference%s to '%S'\n", - refs, (refs == 1) ? "" : "s", obj) < 0) { - return -1; - } - return 0; -} - -// Builds the error message describing why a region could not be closed, listing -// the objects that still have incoming references. Returns a new reference to -// the message string, or NULL with an exception set. -static PyObject * -build_close_error_message(trace_info_t *trace_info) { - PyUnicodeWriter *writer = PyUnicodeWriter_Create(0); - if (writer == NULL) { - return NULL; - } - - incoming_ref_report report = { writer, 0 }; - - if (PyUnicodeWriter_WriteUTF8(writer, - "The region could not be closed due to:\n", -1) < 0) { - goto error; - } - - // `obj_table` maps each object with incoming references to the number of - // such references. Emit one line per object. - if (_Py_hashtable_foreach(trace_info->obj_table, _report_incoming_ref, &report) < 0) { - goto error; - } - - // One incoming reference is the expected owning reference to the region - // itself; everything beyond that is a reason the region stayed open. The - // `obj_table` is also capped at `ERROR_OBJECT_REPORT_COUNT` entries, so it - // may not list every object. Summarise whatever wasn't reported above. - Py_ssize_t problem_refs = trace_info->incoming_refs - 1; - if (report.accounted < problem_refs) { - Py_ssize_t others = problem_refs - report.accounted; - if (PyUnicodeWriter_Format(writer, - "- %zd reference%s to other objects\n", - others, (others == 1) ? "" : "s") < 0) { - goto error; - } - } - - return PyUnicodeWriter_Finish(writer); - -error: - PyUnicodeWriter_Discard(writer); - return NULL; -} /* This method traces the region and closes it, if there are no references * pointing into the region. References to the bridge are allowed. @@ -1333,59 +899,6 @@ int _PyTracingRegion_Close(PyObject* op) { return try_close_region_tree(self); } -int __old_PyTracingRegion_Close(PyObject* op) { - TracingRegionObject *self = (TracingRegionObject*)op; - if (!self->open) { - return 1; - } - assert(gc_list_is_empty(&self->gc_list)); - int res = 0; - trace_info_t trace_info; - if (trace_object(op, &trace_info, &self->gc_list)) { - goto error; // propagate Python exception - } - - // Keep the region open, if the there are more incoming references - // besides the expected owning one - if (trace_info.incoming_refs > 1) { - trace("- Failed to close region %p, there are %zd incoming references", self, trace_info.incoming_refs); - gc_list_dissolve(&self->gc_list); - assert(gc_list_is_empty(&self->gc_list)); - - // Report which objects still have incoming references as a - // `RuntimeError`, e.g.: - // - // RuntimeError: The region could not be closed due to: - // - 1 incoming reference to '[1, 2, 3]' - // - 2 incoming references to '(6, 7)' - PyObject *msg = build_close_error_message(&trace_info); - if (msg != NULL) { - PyErr_SetObject(PyExc_RuntimeError, msg); - Py_DECREF(msg); - } - - goto error; - } - - // FIXME: This can be optimized, for example by inserting all objects - // with weak refs in the beginning. - detach_weak_refs(&self->gc_list); - - trace("- Closed region %p", self); - assert(!gc_list_is_empty(&self->gc_list)); - self->open = false; - res = 1; - goto finally; -error: - res = -1; -finally: - if (trace_info.obj_table != NULL) { - _Py_hashtable_destroy(trace_info.obj_table); - } - - return res; -} - int _PyTracingRegion_IsClosed(PyObject* region) { TracingRegionObject *self = (TracingRegionObject*)region; return !self->open; From c9500ec1e4738b81c2fdc0bc158f2d73ba5d6365 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Wed, 19 Aug 2026 14:44:25 +0200 Subject: [PATCH 19/24] Again nice error reporting and mermaid --- Lib/test/test_freeze/test_tracing_region.py | 54 ++- Objects/tracingregionobject.c | 351 +++++++++++++++++++- region-error-plan.md | 158 +++++++++ 3 files changed, 556 insertions(+), 7 deletions(-) create mode 100644 region-error-plan.md diff --git a/Lib/test/test_freeze/test_tracing_region.py b/Lib/test/test_freeze/test_tracing_region.py index ec386c9a1d4b4b5..5e120bbd61f2ec7 100644 --- a/Lib/test/test_freeze/test_tracing_region.py +++ b/Lib/test/test_freeze/test_tracing_region.py @@ -1,9 +1,12 @@ import sys import unittest +from test.support import os_helper from immutable import freeze, is_frozen, freezable from immutable import TracingRegion as Region from immutable import Cown +REGION_GRAPH = "region-graph.md" + def sort_region_error(msg): """Normalize a 'region could not be closed' message by sorting its per-object lines. Useful for deterministic test assertions, since the @@ -12,6 +15,9 @@ def sort_region_error(msg): return [header, *sorted(lines)] class TestTraceRefs(unittest.TestCase): + def setUp(self): + self.addCleanup(os_helper.unlink, REGION_GRAPH) + os_helper.unlink(REGION_GRAPH) def test_release_error(self): x = [1] @@ -32,7 +38,7 @@ def test_release_error(self): "- 1 incoming reference to '[2]'" ]) - def test_release_error(self): + def test_release_error_capped_output(self): # The object order in the error message is based on the address # and therefore fairly random. All elements look the same of # make testing stable. @@ -63,11 +69,30 @@ def test_release_error(self): l = None c.release() + def test_release_error_in_subregion(self): + x = [1] + + c = Cown(Region()) + child = Region() + child.x = x + c.value.child = child + + with self.assertRaises(RuntimeError) as cm: + c.release() + + self.assertEqual( + sort_region_error(str(cm.exception)), + [ + "The region could not be closed due to:", + "- 1 incoming reference to '[1]'", + ]) + class TestRegionOpening(unittest.TestCase): def test_open_after_acquire(self): c = Cown(Region()) c.value.x = [] + self.assertFalse(c._is_closed()) c.release() c.acquire() @@ -79,15 +104,42 @@ def test_open_after_acquire(self): def test_release_closed_region(self): c = Cown(Region()) c.value.x = [] + self.assertFalse(c._is_closed()) + + c.release() + c.acquire() + + self.assertTrue(c._is_closed()) c.release() + + def test_bridge_refs_keep_region_closed(self): + c = Cown(Region()) + c.release() c.acquire() + self.assertTrue(c._is_closed()) + # Adding new references to the bridge object should keep it closed. + # only attribute accesses should open it. + r1 = c.value + r2 = c.value self.assertTrue(c._is_closed()) + # However, these references should prevent the cown from being released + with self.assertRaises(RuntimeError) as cm: + c.release() + + self.assertEqual( + str(cm.exception), + "the cown couldn't be released, due to the bridge having incoming references") + + # The release should succeed once all refs have been killed + del r1 + del r2 c.release() + class TestImplicitFreeze(unittest.TestCase): def test_implicit_freeze_func(self): @freezable diff --git a/Objects/tracingregionobject.c b/Objects/tracingregionobject.c index 40a4e6a5b02bece..cbd70b2c960352f 100644 --- a/Objects/tracingregionobject.c +++ b/Objects/tracingregionobject.c @@ -549,9 +549,335 @@ typedef struct { Py_ssize_t incoming_refs; } trace_info_t; +typedef struct { + _Py_hashtable_t *obj_table; + Py_ssize_t incoming_refs; +} close_error_info_t; + +typedef struct { + _Py_hashtable_t *target; + PyObject *bridge; + Py_ssize_t ignored_refs; +} close_error_filter_t; + +typedef struct { + PyUnicodeWriter *writer; + Py_ssize_t accounted; +} incoming_ref_report_t; + +typedef struct { + PyUnicodeWriter *writer; + _Py_hashtable_t *visited; + _Py_hashtable_t *error_objs; + PyObject *pending; + PyObject *src; +} mermaid_dump_state_t; + const int TRACE_RES_ERR = -1; const int TRACE_RES_DONE = 0; +static int +collect_close_error_obj(_Py_hashtable_t *ht, const void *key, const void *value, void *user_data) +{ + close_error_filter_t *filter = (close_error_filter_t *)user_data; + Py_ssize_t refs = (Py_ssize_t)value; + + if ((PyObject *)key == filter->bridge) { + refs -= 1; + filter->ignored_refs += 1; + } + + if (refs <= 0) { + return 0; + } + if (_Py_hashtable_set(filter->target, key, (void *)refs) < 0) { + return -1; + } + if (_Py_hashtable_len(filter->target) >= ERROR_OBJECT_REPORT_COUNT) { + return 1; + } + return 0; +} + +static int +close_error_info_init(close_error_info_t *info, region_trace_state_t *state) +{ + info->incoming_refs = state->external_rc; + info->obj_table = _Py_hashtable_new( + _Py_hashtable_hash_ptr, + _Py_hashtable_compare_direct); + if (info->obj_table == NULL) { + return -1; + } + + close_error_filter_t filter = {info->obj_table, state->bridge, 0}; + int res = _Py_hashtable_foreach(state->visited, collect_close_error_obj, &filter); + if (res < 0) { + _Py_hashtable_destroy(info->obj_table); + info->obj_table = NULL; + return -1; + } + info->incoming_refs -= filter.ignored_refs; + return 0; +} + +static void +close_error_info_destroy(close_error_info_t *info) +{ + if (info->obj_table != NULL) { + _Py_hashtable_destroy(info->obj_table); + info->obj_table = NULL; + } +} + +static int +report_incoming_ref(_Py_hashtable_t *ht, const void *key, const void *value, void *user_data) +{ + incoming_ref_report_t *report = (incoming_ref_report_t *)user_data; + PyObject *obj = (PyObject *)key; + Py_ssize_t refs = (Py_ssize_t)value; + + report->accounted += refs; + + if (PyUnicodeWriter_Format(report->writer, + "- %zd incoming reference%s to '%S'\n", + refs, (refs == 1) ? "" : "s", obj) < 0) { + return -1; + } + return 0; +} + +static PyObject * +build_close_error_message(close_error_info_t *info) +{ + PyUnicodeWriter *writer = PyUnicodeWriter_Create(0); + if (writer == NULL) { + return NULL; + } + + incoming_ref_report_t report = {writer, 0}; + + if (PyUnicodeWriter_WriteUTF8(writer, + "The region could not be closed due to:\n", -1) < 0) { + goto error; + } + + if (_Py_hashtable_foreach(info->obj_table, report_incoming_ref, &report) < 0) { + goto error; + } + + if (report.accounted < info->incoming_refs) { + Py_ssize_t others = info->incoming_refs - report.accounted; + if (PyUnicodeWriter_Format(writer, + "- %zd reference%s to other objects\n", + others, (others == 1) ? "" : "s") < 0) { + goto error; + } + } + + return PyUnicodeWriter_Finish(writer); + +error: + if (!PyErr_Occurred()) { + PyErr_SetString(PyExc_RuntimeError, "failed to build region close error message"); + } + PyUnicodeWriter_Discard(writer); + return NULL; +} + +static int +mermaid_write_node(PyUnicodeWriter *writer, PyObject *obj) +{ + if (Region_Check(obj)) { + const char *status = ((TracingRegionObject *)obj)->open ? "open" : "closed"; + return PyUnicodeWriter_Format(writer, + "n%p[[\"Region %p
rc=%zd
%s\"]]", + obj, obj, Py_REFCNT(obj), status); + } + if (Cown_Check(obj)) { + return PyUnicodeWriter_Format(writer, + "n%p([\"Cown %p
rc=%zd\"])", + obj, obj, Py_REFCNT(obj)); + } + return PyUnicodeWriter_Format(writer, + "n%p[\"%p
rc=%zd
[%s]\"]", + obj, obj, Py_REFCNT(obj), Py_TYPE(obj)->tp_name); +} + +static int +mermaid_write_class(PyUnicodeWriter *writer, PyObject *obj, _Py_hashtable_t *error_objs) +{ + if (_Py_IsImmutable(obj)) { + return PyUnicodeWriter_WriteUTF8(writer, ":::immutable", -1); + } + if (_Py_hashtable_get_entry(error_objs, obj) != NULL) { + return PyUnicodeWriter_WriteUTF8(writer, ":::error", -1); + } + return 0; +} + +static int +mermaid_enqueue_if_needed(mermaid_dump_state_t *state, PyObject *obj) +{ + if (_Py_IsImmutable(obj) || Cown_Check(obj)) { + return 0; + } + if (Region_Check(obj) && state->src != NULL) { + return 0; + } + if (_Py_hashtable_get_entry(state->visited, obj) != NULL) { + return 0; + } + if (_Py_hashtable_set(state->visited, obj, obj) < 0) { + return -1; + } + return PyList_Append(state->pending, obj); +} + +static int +mermaid_visit(PyObject *obj, mermaid_dump_state_t *state) +{ + if (_Py_IsImmutable(obj) && ERROR_MERMAID_HIDE_IMMUTABLE) { + return 0; + } + + if (state->src != NULL) { + if (PyUnicodeWriter_WriteUTF8(state->writer, " ", -1) < 0) { + return -1; + } + if (mermaid_write_node(state->writer, state->src) < 0) { + return -1; + } + if (PyUnicodeWriter_WriteUTF8(state->writer, " --> ", -1) < 0) { + return -1; + } + } else if (PyUnicodeWriter_WriteUTF8(state->writer, " ", -1) < 0) { + return -1; + } + + if (mermaid_write_node(state->writer, obj) < 0) { + return -1; + } + if (mermaid_write_class(state->writer, obj, state->error_objs) < 0) { + return -1; + } + if (PyUnicodeWriter_WriteUTF8(state->writer, "\n", -1) < 0) { + return -1; + } + + return mermaid_enqueue_if_needed(state, obj); +} + +static void +mermaid_dump_state_destroy(mermaid_dump_state_t *state) +{ + if (state->writer != NULL) { + PyUnicodeWriter_Discard(state->writer); + state->writer = NULL; + } + Py_CLEAR(state->pending); + if (state->visited != NULL) { + _Py_hashtable_destroy(state->visited); + state->visited = NULL; + } +} + +static int +mermaid_dump_state_init(mermaid_dump_state_t *state, _Py_hashtable_t *error_objs) +{ + state->writer = NULL; + state->visited = NULL; + state->pending = NULL; + state->src = NULL; + state->error_objs = error_objs; + + state->writer = PyUnicodeWriter_Create(0); + if (state->writer == NULL) { + goto error; + } + state->visited = _Py_hashtable_new( + _Py_hashtable_hash_ptr, + _Py_hashtable_compare_direct); + if (state->visited == NULL) { + goto error; + } + state->pending = PyList_New(0); + if (state->pending == NULL) { + goto error; + } + return 0; + +error: + mermaid_dump_state_destroy(state); + return -1; +} + +static int +dump_mermaid_diagram(PyObject *root, _Py_hashtable_t *error_objs) +{ + int res = -1; + mermaid_dump_state_t state; + PyObject *diagram = NULL; + + if (mermaid_dump_state_init(&state, error_objs) < 0) { + return -1; + } + + if (PyUnicodeWriter_WriteUTF8(state.writer, "flowchart TD\n", -1) < 0) { + goto finally; + } + if (mermaid_visit(root, &state) < 0) { + goto finally; + } + + while (PyList_GET_SIZE(state.pending) > 0) { + PyObject *item = list_pop(state.pending); + state.src = item; + traverseproc proc = get_reachable_proc(Py_TYPE(item)); + SUCCEEDS(proc(item, (visitproc)mermaid_visit, &state)); + } + + diagram = PyUnicodeWriter_Finish(state.writer); + state.writer = NULL; + if (diagram == NULL) { + goto finally; + } + + const char *body = PyUnicode_AsUTF8(diagram); + if (body == NULL) { + goto finally; + } + + FILE *f = fopen("region-graph.md", "w"); + if (f != NULL) { + fputs( + "
\n" + "\n" + "```mermaid\n" + "%%{init: {'theme': 'neutral', 'themeVariables': { 'fontSize': '16px' }}}%%\n" + "\n", + f); + fputs(body, f); + fputs( + "\n" + "classDef immutable fill:#94f7ff\n" + "classDef error stroke-width:4px,stroke:red\n" + "```\n" + "
\n", + f); + fclose(f); + } + + res = 0; + +finally: + mermaid_dump_state_destroy(&state); + Py_XDECREF(diagram); + return res; +error: + goto finally; +} + static int _move_obj(PyObject* obj, region_trace_state_t* state) { // Check the movability of the object: movable_status status = get_movable_status(obj); @@ -707,11 +1033,24 @@ static int _try_close_region(PyObject *region_obj, tree_trace_state_t *tree_trac } dbg("- Failed to close region %p, there are %zd incoming references", region, state.external_rc); - PyErr_Format( - PyExc_RuntimeError, - "Failed to close region %p, there are %zd incoming references", - region, - state.external_rc); + close_error_info_t error_info = {NULL, 0}; + if (close_error_info_init(&error_info, &state) < 0) { + goto error; + } + if (_Py_hashtable_len(state.visited) < ERROR_MERMAID_REPORT_LIMIT) { + // Borrowed error table; dump_mermaid_diagram() does not take ownership. + if (dump_mermaid_diagram(region_obj, error_info.obj_table) < 0) { + PyErr_Clear(); + } + } + + PyObject *msg = build_close_error_message(&error_info); + close_error_info_destroy(&error_info); + if (msg == NULL) { + goto error; + } + PyErr_SetObject(PyExc_RuntimeError, msg); + Py_DECREF(msg); goto error; } @@ -896,7 +1235,7 @@ int _PyTracingRegion_Close(PyObject* op) { } assert(gc_list_is_empty(&self->gc_list)); - return try_close_region_tree(self); + return try_close_region_tree(op); } int _PyTracingRegion_IsClosed(PyObject* region) { diff --git a/region-error-plan.md b/region-error-plan.md new file mode 100644 index 000000000000000..01b1e1874beed7d --- /dev/null +++ b/region-error-plan.md @@ -0,0 +1,158 @@ +## Plan: restore region close diagnostics + +### Current state + +The tree-region rewrite moved the close algorithm into `try_close_region_tree()` and `_try_close_region()` in `Objects/tracingregionobject.c`. The good news is that the important accounting still exists locally: + +- `region_trace_state_t.visited` still maps each moved object to its local refcount delta. +- `region_trace_state_t.external_rc` still carries the total outstanding incoming references for the region being traced. +- `region_trace_state_t.src` still identifies the source object during traversal, which is enough to rebuild graph edges. +- `_try_close_region()` still has the exact failure point where diagnostics should be produced, after dissolving the tentative GC list and after ignoring restart traces. + +The regression is mostly that `_try_close_region()` now formats only: + +```text +Failed to close region %p, there are %zd incoming references +``` + +and then destroys `state.visited`, so the object-level detail and Mermaid graph are lost. The old implementation had these pieces before the rewrite: + +- `error_ref_filter` / `_filter_visited()` to select the first `ERROR_OBJECT_REPORT_COUNT` objects with positive incoming references. +- `build_close_error_message()` to emit: + - `The region could not be closed due to:` + - `- N incoming reference(s) to 'obj'` + - `- N reference(s) to other objects` +- `mermaid_builder_t`, `mermaid_visit()`, and `dump_mermaid_diagram()` to write `region-graph.md` with red-highlighted leaking objects and cyan immutable objects. + +### Desired behavior + +When closing a region tree fails because a particular open region has lingering references into it, the exception should again identify the problematic objects instead of only reporting a total count. For small graphs, the failed close should also regenerate `region-graph.md` so the reference path can be inspected visually. + +The diagnostics should be scoped to the region that actually failed during `try_close_region_tree()`, not to the whole tree unless a later failure aggregation is explicitly added. That preserves the current close algorithm: child regions are closed first; the parent is retried; whichever region still has external refs reports its own graph. + +### Implementation steps + +1. Reintroduce a diagnostic result type. + + Add a small struct near the trace state types, for example: + + ```c + typedef struct { + _Py_hashtable_t *obj_table; + Py_ssize_t incoming_refs; + } close_error_info_t; + ``` + + Keep it separate from `region_trace_state_t` so the tracing state can remain reusable and the caller owns the filtered error table lifetime. + +2. Re-add the filtering helpers, adjusted for bridge semantics. + + Restore the old `error_ref_filter` idea, but make it explicit that the bridge object has one expected external owning reference. In the old code this was handled by subtracting one from the root region object; in the new code the bridge is `state.bridge`. + + Rules for `_filter_visited()`: + + - Start with the stored ref delta from `state.visited`. + - If `key == state.bridge`, subtract the expected owning reference. + - Keep only entries with `refs > 0`. + - Cap the table at `ERROR_OBJECT_REPORT_COUNT` entries. + - Treat `_Py_hashtable_foreach()` return `1` as intentional early stop, not an error. + + This preserves the old message shape while matching the current close model, where references to the bridge from inside the region are tracked separately as `bridge_rc` and should not be reported as external leaks. + +3. Build diagnostics inside `_try_close_region()` before destroying `state`. + + In the `state.external_rc != 0` failure branch, after `gc_list_dissolve(®ion->gc_list)` and after the `state.restart` check: + + - Allocate the filtered `close_error_info_t.obj_table` from `state.visited`. + - Store `close_error_info_t.incoming_refs = state.external_rc`. + - Build the Python exception with the restored `build_close_error_message()`. + - Fall back to the existing summary string only if message construction fails without a more specific exception. + - Destroy the filtered table on all exits. + + Important: do not build the nice error on restart traces. Restart traces are intentionally incomplete because freezing or open child-region discovery invalidated the current accounting. + +4. Restore `build_close_error_message()`. + + Port the old `incoming_ref_report`, `_report_incoming_ref()`, and `build_close_error_message()` almost directly. The main adjustment is replacing `trace_info_t` with `close_error_info_t` and making the expected-reference subtraction happen during filtering, not during final summarization. + + The summary calculation should therefore be: + + ```c + Py_ssize_t problem_refs = error_info->incoming_refs; + ``` + + not `incoming_refs - 1`, because the bridge's expected reference has already been removed from the filtered object counts and should also be excluded from `external_rc` if needed. If `external_rc` still includes the expected bridge reference for the region currently being closed, subtract it once at diagnostic collection time and document that invariant next to the code. + + Cheap check: the existing `test_release_error` expectations in `Lib/test/test_freeze/test_tracing_region.py` should pass with the old exact message lines. + +5. Re-add Mermaid generation as a read-only diagnostic trace. + + Restore `mermaid_builder_t` and `mermaid_visit()`, but adapt it to `region_trace_state_t`: + + - Add `mermaid_builder_t *mermaid;` to `region_trace_state_t`, initialized to `NULL` in `region_trace_state_reset()`. + - At the start of `_trace_visit()`, call `mermaid_visit(obj, state)` when `state->mermaid != NULL`. + - In `mermaid_visit()`, keep the old node format: pointer, refcount, and type name. + - Preserve the old special node shapes for ownership objects: + - Regions use Mermaid's subroutine shape: `id[[Region 0x...]]`. + - Cowns use Mermaid's stadium shape: `id([Cown 0x...])`. + - Continue hiding immutable nodes behind `ERROR_MERMAID_HIDE_IMMUTABLE`. + - Highlight objects present in the filtered error table with `:::error`. + + For the diagnostic trace, initialize `region_trace_state_t` with `gc_list == NULL` so no objects are moved. Use the same `tree_trace_state_t` shape only if required by `_trace_visit()` for region references; otherwise, split a read-only Mermaid visitor path from closing behavior so dumping the graph cannot enqueue or close subregions. + +6. Decide how Mermaid handles sub-regions. + + The tree rewrite adds a case the old graph did not have: references to region bridge objects can represent nested ownership rather than ordinary objects. + + Recommended first version: + + - Show closed sub-region bridge objects as region-shaped boundary nodes and do not traverse into them, matching `_move_obj()`'s current `if (!Region_Check(obj))` behavior. + - Treat open sub-regions as boundary nodes in the graph and label them as `[TracingRegion open]` or `[TracingRegion closed]` if that can be done without allocating risky strings. + - Do not let Mermaid dumping trigger `_enqueue_region_for_closing()` or `region_trace_state_set_restart()`. + - For now, dump only the graph for the single region that failed. Do not attempt to show the whole region tree yet. + + This keeps the diagnostic graph side-effect-free and aligned with the current failure point. A later enhancement can add dashed edges from parent to child region graphs if whole-tree visualization becomes useful. + +7. Write `region-graph.md` only when the graph is small. + + Reuse the old limit: + + ```c + if (_Py_hashtable_len(state.visited) < ERROR_MERMAID_REPORT_LIMIT) { + dump_mermaid_diagram(region_obj, error_info.obj_table); + } + ``` + + Keep the graph dump strictly best-effort: failure to open `region-graph.md` must not replace the close error. Actual Python exceptions from building the diagram should either be cleared and ignored, or avoided by making the dump path best-effort all the way through. For diagnostics, losing the graph is less important than preserving the close failure message. + +8. Add focused tests. + + Update or add tests in `Lib/test/test_freeze/test_tracing_region.py`: + + - Keep the existing simple leak test for exact message shape. + - Add a capped-output test with more than `ERROR_OBJECT_REPORT_COUNT` leaked objects and an `other objects` summary. + - Add a tree-region case where a child region fails to close and the error names an object inside the child, not just the parent total. + - Add a tree-region case where the child closes successfully but the parent fails due to a reference into the parent. + - For Mermaid, either assert that `region-graph.md` exists and contains `flowchart TD` plus `:::error`, or add a small C-visible/private Python hook if file-system assertions are too brittle. + + Also fix the duplicate Python test method name currently present in `TestTraceRefs`; the second `test_release_error` overrides the first. + +9. Validation commands. + + After implementation, run the narrow test file first: + + ```sh + ./python.exe -m test test_freeze.test_tracing_region + ``` + + Then run a build if C changes were made: + + ```sh + make -j + ``` + +### Decisions for this pass + +- `region-graph.md` should show only the single failing region for now. +- Graph dumping is strictly best-effort; diagnostic file failures should not mask the ownership violation. +- Structured exception attributes like `source` and `target` are a follow-up, not part of this restoration pass. From e282562a39f417245f965494d1fca25b1ef94df2 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Wed, 19 Aug 2026 16:42:48 +0200 Subject: [PATCH 20/24] Immutability: Fix bug in shallow immutability check --- Lib/test/test_freeze/test_implicit.py | 21 +++++++++++++++++++++ Python/immutability.c | 8 ++++++++ 2 files changed, 29 insertions(+) diff --git a/Lib/test/test_freeze/test_implicit.py b/Lib/test/test_freeze/test_implicit.py index b710b787fe5bdaf..35e46036e66d75f 100644 --- a/Lib/test/test_freeze/test_implicit.py +++ b/Lib/test/test_freeze/test_implicit.py @@ -1,3 +1,4 @@ +import sys import unittest from immutable import freeze, is_frozen @@ -139,6 +140,26 @@ def test_deeply_nested_no_stack_overflow(self): obj = (obj,) self.assertTrue(is_frozen(obj)) + def test_abandoned_walk_keeps_references(self): + """An aborted walk must not drop references it never took. + + The walk pushes objects onto a worklist without increfing them, so + anything still on the worklist when a mutable object aborts the walk + used to be decrefed when the worklist was released. That freed the + object while its real owners were still pointing at it, which showed + up much later as a negative refcount. + """ + # Built at runtime so it is neither interned nor immortal, which makes + # its reference count fully accounted for by this test. + item = "".join(["abandoned", "-", "worklist", "-", "entry"]) + # Tuples are traversed back to front, so `item` reaches the worklist + # before the dict aborts the walk. + obj = ({"mutable": 1}, item) + + before = sys.getrefcount(item) + self.assertFalse(is_frozen(obj)) + self.assertEqual(sys.getrefcount(item), before) + if __name__ == '__main__': unittest.main() diff --git a/Python/immutability.c b/Python/immutability.c index c4feb45d0511c7b..9ca0901159beada 100644 --- a/Python/immutability.c +++ b/Python/immutability.c @@ -1828,6 +1828,14 @@ int _PyImmutability_CanViewAsImmutable(PyObject *obj) } _Py_hashtable_destroy(state.visited); + + // We can't call the destructor directly as we didn't newref the objects + // on push. Breaking out of the loop above leaves the remaining objects + // on the worklist, so drain it here. 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.worklist) > 0) { + pop(state.worklist); + } Py_DECREF(state.worklist); if (result < 0) { From 56948e9f50d46da01768adced39b4e3310d656aa Mon Sep 17 00:00:00 2001 From: xFrednet Date: Wed, 19 Aug 2026 17:11:33 +0200 Subject: [PATCH 21/24] TRegions: Fixes and niceties --- Lib/test/test_freeze/test_tracing_region.py | 56 ++++- Objects/tracingregionobject.c | 259 +++++++++++++++++--- 2 files changed, 275 insertions(+), 40 deletions(-) diff --git a/Lib/test/test_freeze/test_tracing_region.py b/Lib/test/test_freeze/test_tracing_region.py index 5e120bbd61f2ec7..85333353c6e8279 100644 --- a/Lib/test/test_freeze/test_tracing_region.py +++ b/Lib/test/test_freeze/test_tracing_region.py @@ -112,7 +112,7 @@ def test_release_closed_region(self): self.assertTrue(c._is_closed()) c.release() - + def test_bridge_refs_keep_region_closed(self): c = Cown(Region()) c.release() @@ -138,6 +138,60 @@ def test_bridge_refs_keep_region_closed(self): del r2 c.release() + def test_sub_region_closing(self): + @freezable + class A: + pass + c = Cown(Region()) + c.value.a = A() + c.value.a.child = Region() + c.value.a.child.b = A() + + c.release() + c.acquire() + + r2 = c.value.a.child + c2 = Cown(r2) + + self.assertTrue(c2._is_closed()) + + def test_sub_region_multiple_refs(self): + @freezable + class A: + pass + c = Cown(Region()) + c.value.a = A() + sub = Region() + c.value.a.child_a = sub + c.value.a.child_b = sub + + c.release() + c.acquire() + + r2 = c.value.a.child_a + c2 = Cown(r2) + + self.assertTrue(c2._is_closed()) + + def test_ref_to_sub_region_bridge_keeps_parent_open(self): + c1 = Cown(Region()) + c2 = Cown(Region()) + c1.value.child = c2.value + + self.assertFalse(c2._is_closed()) + + with self.assertRaises(RuntimeError) as cm: + c1.release() + + # Attempting to close the region c1 should have closed c2 and then + # failed due to the incoming reference to the bridge stored in c2 + self.assertTrue(c2._is_closed()) + + + error = sort_region_error(str(cm.exception)) + self.assertEqual(error[0], "The region could not be closed due to:") + self.assertTrue(error[1].startswith("- 1 incoming reference to 'target, key, (void *)refs) < 0) { + if (_Py_hashtable_set(filter->problem_target, key, (void *)refs) < 0) { return -1; } - if (_Py_hashtable_len(filter->target) >= ERROR_OBJECT_REPORT_COUNT) { - return 1; + if (_Py_hashtable_len(filter->reported_target) < ERROR_OBJECT_REPORT_COUNT) { + if (_Py_hashtable_set(filter->reported_target, key, (void *)refs) < 0) { + return -1; + } } return 0; } @@ -603,18 +609,29 @@ static int close_error_info_init(close_error_info_t *info, region_trace_state_t *state) { info->incoming_refs = state->external_rc; + info->problem_obj_table = NULL; info->obj_table = _Py_hashtable_new( _Py_hashtable_hash_ptr, _Py_hashtable_compare_direct); if (info->obj_table == NULL) { return -1; } + info->problem_obj_table = _Py_hashtable_new( + _Py_hashtable_hash_ptr, + _Py_hashtable_compare_direct); + if (info->problem_obj_table == NULL) { + _Py_hashtable_destroy(info->obj_table); + info->obj_table = NULL; + return -1; + } - close_error_filter_t filter = {info->obj_table, state->bridge, 0}; + close_error_filter_t filter = {info->obj_table, info->problem_obj_table, state->bridge, 0}; int res = _Py_hashtable_foreach(state->visited, collect_close_error_obj, &filter); if (res < 0) { _Py_hashtable_destroy(info->obj_table); info->obj_table = NULL; + _Py_hashtable_destroy(info->problem_obj_table); + info->problem_obj_table = NULL; return -1; } info->incoming_refs -= filter.ignored_refs; @@ -628,6 +645,10 @@ close_error_info_destroy(close_error_info_t *info) _Py_hashtable_destroy(info->obj_table); info->obj_table = NULL; } + if (info->problem_obj_table != NULL) { + _Py_hashtable_destroy(info->problem_obj_table); + info->problem_obj_table = NULL; + } } static int @@ -689,29 +710,104 @@ static int mermaid_write_node(PyUnicodeWriter *writer, PyObject *obj) { if (Region_Check(obj)) { - const char *status = ((TracingRegionObject *)obj)->open ? "open" : "closed"; + bool open = ((TracingRegionObject *)obj)->open; + const char *status = open ? "open" : "closed"; return PyUnicodeWriter_Format(writer, - "n%p[[\"Region %p
rc=%zd
%s\"]]", - obj, obj, Py_REFCNT(obj), status); + "n%p[\\Region
%s
rc=%zd
%p/]", + obj, status, Py_REFCNT(obj), obj); } if (Cown_Check(obj)) { return PyUnicodeWriter_Format(writer, - "n%p([\"Cown %p
rc=%zd\"])", - obj, obj, Py_REFCNT(obj)); + "n%p([\"Cown
rc=%zd
%p\"])", + obj, Py_REFCNT(obj), obj); } return PyUnicodeWriter_Format(writer, - "n%p[\"%p
rc=%zd
[%s]\"]", - obj, obj, Py_REFCNT(obj), Py_TYPE(obj)->tp_name); + "n%p[\"[%s]
rc=%zd
%p\"]", + obj, Py_TYPE(obj)->tp_name, Py_REFCNT(obj), obj); } static int -mermaid_write_class(PyUnicodeWriter *writer, PyObject *obj, _Py_hashtable_t *error_objs) +mermaid_write_class( + PyUnicodeWriter *writer, + PyObject *obj, + _Py_hashtable_t *error_objs, + _Py_hashtable_t *reported_objs) { if (_Py_IsImmutable(obj)) { - return PyUnicodeWriter_WriteUTF8(writer, ":::immutable", -1); + return PyUnicodeWriter_Format(writer, " class n%p immutable\n", obj); + } + if (_Py_hashtable_get_entry(reported_objs, obj) != NULL) { + return PyUnicodeWriter_Format(writer, " class n%p error\n", obj); } if (_Py_hashtable_get_entry(error_objs, obj) != NULL) { - return PyUnicodeWriter_WriteUTF8(writer, ":::error", -1); + return PyUnicodeWriter_Format(writer, " class n%p problem\n", obj); + } + return 0; +} + +static int +mermaid_write_escaped_label(PyUnicodeWriter *writer, const char *label) +{ + for (const char *p = label; *p != '\0'; p++) { + switch (*p) { + case '|': + if (PyUnicodeWriter_WriteChar(writer, '/') < 0) { + return -1; + } + break; + case '\n': + case '\r': + if (PyUnicodeWriter_WriteChar(writer, ' ') < 0) { + return -1; + } + break; + default: + if (PyUnicodeWriter_WriteChar(writer, (Py_UCS4)(unsigned char)*p) < 0) { + return -1; + } + break; + } + } + return 0; +} + +static int +mermaid_write_escaped_unicode_label(PyUnicodeWriter *writer, PyObject *label) +{ + Py_ssize_t size; + const char *utf8 = PyUnicode_AsUTF8AndSize(label, &size); + if (utf8 == NULL) { + return -1; + } + + Py_ssize_t start = 0; + for (Py_ssize_t i = 0; i < size; i++) { + switch (utf8[i]) { + case '|': + if (i > start && PyUnicodeWriter_WriteUTF8(writer, utf8 + start, i - start) < 0) { + return -1; + } + if (PyUnicodeWriter_WriteChar(writer, '/') < 0) { + return -1; + } + start = i + 1; + break; + case '\n': + case '\r': + if (i > start && PyUnicodeWriter_WriteUTF8(writer, utf8 + start, i - start) < 0) { + return -1; + } + if (PyUnicodeWriter_WriteChar(writer, ' ') < 0) { + return -1; + } + start = i + 1; + break; + default: + break; + } + } + if (size > start && PyUnicodeWriter_WriteUTF8(writer, utf8 + start, size - start) < 0) { + return -1; } return 0; } @@ -735,9 +831,13 @@ mermaid_enqueue_if_needed(mermaid_dump_state_t *state, PyObject *obj) } static int -mermaid_visit(PyObject *obj, mermaid_dump_state_t *state) +mermaid_visit_labeled( + PyObject *obj, + mermaid_dump_state_t *state, + const char *ascii_label, + PyObject *unicode_label) { - if (_Py_IsImmutable(obj) && ERROR_MERMAID_HIDE_IMMUTABLE) { + if (_Py_IsImmutable(obj) && ERROR_MERMAID_HIDE_IMMUTABLE && !Cown_Check(obj)) { return 0; } @@ -748,7 +848,23 @@ mermaid_visit(PyObject *obj, mermaid_dump_state_t *state) if (mermaid_write_node(state->writer, state->src) < 0) { return -1; } - if (PyUnicodeWriter_WriteUTF8(state->writer, " --> ", -1) < 0) { + if (ascii_label != NULL || unicode_label != NULL) { + if (PyUnicodeWriter_WriteUTF8(state->writer, " -->|", -1) < 0) { + return -1; + } + if (ascii_label != NULL) { + if (mermaid_write_escaped_label(state->writer, ascii_label) < 0) { + return -1; + } + } + if (unicode_label != NULL && mermaid_write_escaped_unicode_label(state->writer, unicode_label) < 0) { + return -1; + } + if (PyUnicodeWriter_WriteUTF8(state->writer, "| ", -1) < 0) { + return -1; + } + } + else if (PyUnicodeWriter_WriteUTF8(state->writer, " --> ", -1) < 0) { return -1; } } else if (PyUnicodeWriter_WriteUTF8(state->writer, " ", -1) < 0) { @@ -758,16 +874,76 @@ mermaid_visit(PyObject *obj, mermaid_dump_state_t *state) if (mermaid_write_node(state->writer, obj) < 0) { return -1; } - if (mermaid_write_class(state->writer, obj, state->error_objs) < 0) { + if (PyUnicodeWriter_WriteUTF8(state->writer, "\n", -1) < 0) { return -1; } - if (PyUnicodeWriter_WriteUTF8(state->writer, "\n", -1) < 0) { + if (mermaid_write_class(state->writer, obj, state->error_objs, state->reported_objs) < 0) { return -1; } return mermaid_enqueue_if_needed(state, obj); } +static int +mermaid_visit(PyObject *obj, mermaid_dump_state_t *state) +{ + return mermaid_visit_labeled(obj, state, NULL, NULL); +} + +static int +mermaid_visit_dict(PyObject *obj, mermaid_dump_state_t *state) +{ + Py_ssize_t pos = 0; + PyObject *key; + PyObject *value; + + while (PyDict_Next(obj, &pos, &key, &value)) { + if (!_PyImmutability_CanViewAsImmutable(key) + && !Cown_Check(key) + && !Region_Check(key) + ) { + if (mermaid_visit_labeled(key, state, "", NULL) < 0) { + return -1; + } + } + + PyObject *label = PyUnicode_Check(key) ? key : NULL; + if (mermaid_visit_labeled(value, state, NULL, label) < 0) { + return -1; + } + } + return 0; +} + +static int +mermaid_visit_sequence(PyObject *obj, mermaid_dump_state_t *state) +{ + Py_ssize_t size = PyList_CheckExact(obj) ? PyList_GET_SIZE(obj) : PyTuple_GET_SIZE(obj); + for (Py_ssize_t i = 0; i < size; i++) { + char label[32]; + PyOS_snprintf(label, sizeof(label), "#91;%zd#93;", i); + PyObject *item = PyList_CheckExact(obj) ? PyList_GET_ITEM(obj, i) : PyTuple_GET_ITEM(obj, i); + if (mermaid_visit_labeled(item, state, label, NULL) < 0) { + return -1; + } + } + return 0; +} + +static int +mermaid_traverse(PyObject *obj, mermaid_dump_state_t *state) +{ + if (PyDict_CheckExact(obj)) { + return mermaid_visit_dict(obj, state); + } + if (PyList_CheckExact(obj) || PyTuple_CheckExact(obj)) { + return mermaid_visit_sequence(obj, state); + } + + traverseproc proc = get_reachable_proc(Py_TYPE(obj)); + return proc(obj, (visitproc)mermaid_visit, (void *)state); +} + static void mermaid_dump_state_destroy(mermaid_dump_state_t *state) { @@ -783,13 +959,17 @@ mermaid_dump_state_destroy(mermaid_dump_state_t *state) } static int -mermaid_dump_state_init(mermaid_dump_state_t *state, _Py_hashtable_t *error_objs) +mermaid_dump_state_init( + mermaid_dump_state_t *state, + _Py_hashtable_t *error_objs, + _Py_hashtable_t *reported_objs) { state->writer = NULL; state->visited = NULL; state->pending = NULL; state->src = NULL; state->error_objs = error_objs; + state->reported_objs = reported_objs; state->writer = PyUnicodeWriter_Create(0); if (state->writer == NULL) { @@ -813,13 +993,16 @@ mermaid_dump_state_init(mermaid_dump_state_t *state, _Py_hashtable_t *error_objs } static int -dump_mermaid_diagram(PyObject *root, _Py_hashtable_t *error_objs) +dump_mermaid_diagram( + PyObject *root, + _Py_hashtable_t *error_objs, + _Py_hashtable_t *reported_objs) { int res = -1; mermaid_dump_state_t state; PyObject *diagram = NULL; - if (mermaid_dump_state_init(&state, error_objs) < 0) { + if (mermaid_dump_state_init(&state, error_objs, reported_objs) < 0) { return -1; } @@ -833,8 +1016,7 @@ dump_mermaid_diagram(PyObject *root, _Py_hashtable_t *error_objs) while (PyList_GET_SIZE(state.pending) > 0) { PyObject *item = list_pop(state.pending); state.src = item; - traverseproc proc = get_reachable_proc(Py_TYPE(item)); - SUCCEEDS(proc(item, (visitproc)mermaid_visit, &state)); + SUCCEEDS(mermaid_traverse(item, &state)); } diagram = PyUnicodeWriter_Finish(state.writer); @@ -861,7 +1043,8 @@ dump_mermaid_diagram(PyObject *root, _Py_hashtable_t *error_objs) fputs( "\n" "classDef immutable fill:#94f7ff\n" - "classDef error stroke-width:4px,stroke:red\n" + "classDef problem fill:#ffe8d6,stroke:#f08c00,stroke-width:2px\n" + "classDef error fill:#ffe8d6,stroke:red,stroke-width:4px\n" "```\n" "\n", f); @@ -1022,15 +1205,15 @@ static int _try_close_region(PyObject *region_obj, tree_trace_state_t *tree_trac assert(!PyWeakref_Check(item)); } + if (state.restart) { + gc_list_dissolve(®ion->gc_list); + goto finally; + } + if (state.external_rc == 0) { _region_close(region, state.bridge_rc); } else { gc_list_dissolve(®ion->gc_list); - assert(gc_list_is_empty(®ion->gc_list)); - - if (state.restart) { - goto finally; - } dbg("- Failed to close region %p, there are %zd incoming references", region, state.external_rc); close_error_info_t error_info = {NULL, 0}; @@ -1038,8 +1221,11 @@ static int _try_close_region(PyObject *region_obj, tree_trace_state_t *tree_trac goto error; } if (_Py_hashtable_len(state.visited) < ERROR_MERMAID_REPORT_LIMIT) { - // Borrowed error table; dump_mermaid_diagram() does not take ownership. - if (dump_mermaid_diagram(region_obj, error_info.obj_table) < 0) { + // Borrowed error tables; dump_mermaid_diagram() does not take ownership. + if (dump_mermaid_diagram( + region_obj, + error_info.problem_obj_table, + error_info.obj_table) < 0) { PyErr_Clear(); } } @@ -1146,18 +1332,13 @@ static int TracingRegion_clear(TracingRegionObject *self) { // FIXME(regions): Special branch when closed to dealloc all - // This is deallocating a closed region, we just dissolve it - if (!gc_list_is_empty(&self->gc_list)) { - gc_list_dissolve(&self->gc_list); - } + _open_region(self); Py_CLEAR(self->dict); return 0; } static void TracingRegion_dealloc(TracingRegionObject *self) { - // FIXME(regions): Special branch when closed to dealloc all - PyObject_GC_UnTrack(self); TracingRegion_clear(self); Py_TYPE(self)->tp_free((PyObject *)self); From 821b1d5fb0885b0b4c0c7a82be0cd407306d44b2 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Thu, 20 Aug 2026 16:09:12 +0200 Subject: [PATCH 22/24] TRegions: A lot of bug fixes --- Lib/test/test_freeze/test_tracing_region.py | 46 +- Objects/tracingregionobject.c | 710 +++++++++++++------- 2 files changed, 494 insertions(+), 262 deletions(-) diff --git a/Lib/test/test_freeze/test_tracing_region.py b/Lib/test/test_freeze/test_tracing_region.py index 85333353c6e8279..f661bf0fa8b83cd 100644 --- a/Lib/test/test_freeze/test_tracing_region.py +++ b/Lib/test/test_freeze/test_tracing_region.py @@ -1,24 +1,19 @@ +import re import sys import unittest -from test.support import os_helper from immutable import freeze, is_frozen, freezable from immutable import TracingRegion as Region from immutable import Cown -REGION_GRAPH = "region-graph.md" - def sort_region_error(msg): - """Normalize a 'region could not be closed' message by sorting its - per-object lines. Useful for deterministic test assertions, since the - object order comes from hashtable iteration and isn't stable.""" - header, *lines = msg.splitlines() + """Normalize a 'region could not be closed' message by masking the object + addresses and sorting its per-object lines. Useful for deterministic test + assertions, since the addresses differ per run and the object order comes + from hashtable iteration and isn't stable.""" + header, *lines = re.sub(r"0x[0-9a-fA-F]+", "0x...", msg).splitlines() return [header, *sorted(lines)] class TestTraceRefs(unittest.TestCase): - def setUp(self): - self.addCleanup(os_helper.unlink, REGION_GRAPH) - os_helper.unlink(REGION_GRAPH) - def test_release_error(self): x = [1] y = [2] @@ -34,8 +29,8 @@ def test_release_error(self): sort_region_error(str(cm.exception)), [ "The region could not be closed due to:", - "- 1 incoming reference to '[1]'", - "- 1 incoming reference to '[2]'" + "- 1 incoming reference to list '[1]'", + "- 1 incoming reference to list '[2]'" ]) def test_release_error_capped_output(self): @@ -57,11 +52,11 @@ def test_release_error_capped_output(self): sort_region_error(str(cm.exception)), [ "The region could not be closed due to:", - "- 1 incoming reference to '[1]'", - "- 1 incoming reference to '[1]'", - "- 1 incoming reference to '[1]'", - "- 1 incoming reference to '[1]'", - "- 1 incoming reference to '[1]'", + "- 1 incoming reference to list '[1]'", + "- 1 incoming reference to list '[1]'", + "- 1 incoming reference to list '[1]'", + "- 1 incoming reference to list '[1]'", + "- 1 incoming reference to list '[1]'", "- 3 references to other objects", ]) @@ -84,7 +79,7 @@ def test_release_error_in_subregion(self): sort_region_error(str(cm.exception)), [ "The region could not be closed due to:", - "- 1 incoming reference to '[1]'", + "- 1 incoming reference to list '[1]'", ]) @@ -164,6 +159,10 @@ class A: sub = Region() c.value.a.child_a = sub c.value.a.child_b = sub + # A reference to the bridge of a sub-region counts as an incoming + # reference into the parent region, see + # test_ref_to_sub_region_bridge_keeps_parent_open. + del sub c.release() c.acquire() @@ -188,9 +187,12 @@ def test_ref_to_sub_region_bridge_keeps_parent_open(self): self.assertTrue(c2._is_closed()) - error = sort_region_error(str(cm.exception)) - self.assertEqual(error[0], "The region could not be closed due to:") - self.assertTrue(error[1].startswith("- 1 incoming reference to ''", + ]) diff --git a/Objects/tracingregionobject.c b/Objects/tracingregionobject.c index 07ddb668081032b..911045d03cf0b06 100644 --- a/Objects/tracingregionobject.c +++ b/Objects/tracingregionobject.c @@ -4,6 +4,7 @@ #include "pycore_dict.h" // _PyObject_MaterializeManagedDict() #include "pycore_object.h" // _PyObject_GC_TRACK(), _PyDebugAllocatorStats() #include "pycore_descrobject.h" +#include "pycore_modsupport.h" // _PyArg_NoPositional() #include "pycore_weakref.h" #include "pycore_cown.h" @@ -11,18 +12,18 @@ #define ERROR_MERMAID_REPORT_LIMIT 50 #define ERROR_MERMAID_HIDE_IMMUTABLE true +/* Set this to the path of the file that a failed close should write its mermaid + * 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 #ifdef REGION_TRACING -#define if_dbg(...) __VA_ARGS__ -#define dbg_arg(arg) , (Py_uintptr_t)(arg) #define dbg(msg, ...) \ do { \ printf(msg "\n" __VA_OPT__(,) __VA_ARGS__); \ } while(0) #else -#define if_dbg(...) -#define dbg_arg(...) #define dbg(...) #endif @@ -40,6 +41,12 @@ #define GC_NEXT _PyGCHead_NEXT #define GC_PREV _PyGCHead_PREV +static inline int +gc_old_space(PyGC_Head *g) +{ + return g->_gc_next & _PyGC_NEXT_MASK_OLD_SPACE_1; +} + static inline void gc_set_old_space(PyGC_Head *g, int space) { @@ -91,6 +98,8 @@ gc_list_merge(PyGC_Head *from, PyGC_Head *to) PyGC_Head *from_tail = GC_PREV(from); assert(from_head != from); assert(from_tail != from); + assert(gc_list_is_empty(to) || + gc_old_space(to_tail) == gc_old_space(from_tail)); _PyGCHead_SET_NEXT(to_tail, from_head); _PyGCHead_SET_PREV(from_head, to_tail); @@ -114,7 +123,7 @@ gc_clear_collecting(PyGC_Head *g) g->_gc_prev &= ~_PyGC_PREV_MASK_COLLECTING; } -#elif // Py_GIL_DISABLED +#else // Py_GIL_DISABLED #error "We need GIL" #endif @@ -122,18 +131,22 @@ gc_clear_collecting(PyGC_Head *g) // Copied from regions-main // ################################################################### +/* Removes the last item of the list and returns it as a new reference. + * + * The caller needs a reference of its own, since the list was the only thing + * keeping the item alive. Traversing the item can run arbitrary code, for + * example through `_PyImmutability_Freeze()`, which could otherwise deallocate + * it while it is being traversed. + * + * Returns NULL with an exception set on failure. The list must not be empty. + */ static PyObject* list_pop(PyObject* s){ - PyObject* item; - Py_ssize_t size = PyList_Size(s); - if(size == 0){ - return NULL; - } - item = PyList_GetItem(s, size - 1); - if(item == NULL){ - return NULL; - } + Py_ssize_t size = PyList_GET_SIZE(s); + assert(size > 0); + + PyObject *item = Py_NewRef(PyList_GET_ITEM(s, size - 1)); // This should never fail, since we shrink the size - if(PyList_SetSlice(s, size - 1, size, NULL)){ + if (PyList_SetSlice(s, size - 1, size, NULL)) { Py_DECREF(item); return NULL; } @@ -150,7 +163,7 @@ typedef enum { Py_MOVABLE_COWN = 3, } movable_status; -movable_status get_movable_status(PyObject *obj) { +static movable_status get_movable_status(PyObject *obj) { // FIXME(regions): xFrednet: Currently it's not possible to set // the movability per object. This instead returns the default // movability for objects. Note that some shallow immutable objects @@ -178,7 +191,7 @@ movable_status get_movable_status(PyObject *obj) { } // Module objects are also complicated. Freezing them should turn most modules - // into proxys which should make them mostly usable. + // into proxies which should make them mostly usable. if (PyModule_Check(obj)) { return Py_MOVABLE_FREEZE; } @@ -205,7 +218,7 @@ movable_status get_movable_status(PyObject *obj) { } // Freezing or moving these objects is... complicated. In some cases it is - // possible but more hassle than it's probably worth. For not we mark them + // possible but more hassle than it's probably worth. For now we mark them // all as unmovable. if (PyFrame_Check(obj) || PyGen_CheckExact(obj) @@ -234,7 +247,7 @@ movable_status get_movable_status(PyObject *obj) { return Py_MOVABLE_YES; } -// This uses the given arguments to create and throw a `RegionError` +// This uses the given arguments to create and throw a `RuntimeError` static void throw_region_error( const char *format_str, const char *tp_name, PyObject* src, PyObject* tgt) @@ -247,16 +260,17 @@ static void throw_region_error( PyErr_Format(PyExc_RuntimeError, format_str, tp_name); - // Set source and target fields - // Get the current exception (should be a RuntimeError) PyObject *exc = PyErr_GetRaisedException(); - assert(exc && PyObject_TypeCheck(exc, (PyTypeObject *)PyExc_RuntimeError)); + assert(exc != NULL); - // Add 'source' and 'target' attributes to the exception - PyObject_SetAttr(exc, &_Py_ID(source), src ? src : Py_None); - PyObject_SetAttr(exc, &_Py_ID(target), tgt ? tgt : Py_None); + // Failing to attach it must not replace the error raised above. + if (PyObject_SetAttr(exc, &_Py_ID(source), src ? src : Py_None) < 0 + || PyObject_SetAttr(exc, &_Py_ID(target), tgt ? tgt : Py_None) < 0) + { + PyErr_Clear(); + } - PyErr_SetRaisedException((PyObject*)exc); + PyErr_SetRaisedException(exc); } // Wrapper around tp_traverse that also visits the type object. @@ -274,36 +288,74 @@ traverse_via_tp_traverse(PyObject *obj, visitproc visit, void *state) } } - // Most `tp_traverse` don't visit the type even though they should. // Here it won't hurt to potentially visit it twice, since types // are non-movable but will be frozen. - return visit((PyObject *)Py_TYPE(obj), state); + return visit((PyObject *)tp, state); } -// Returns the appropriate traversal function for reaching all references -// from an object. Prefers tp_reachable, falls back to tp_traverse wrapped -// to also visit the type. Emits a warning once per type on fallback. +/* Returns the appropriate traversal function for reaching all references from + * an object. Prefers tp_reachable, falls back to tp_traverse wrapped to also + * visit the type. + * + * Falling back means the trace can miss references that only tp_reachable + * reports, so every type it happens for is recorded in `missing_reachable` and + * reported by `report_missing_reachable()` once the trace is over. Warning here + * would write to `sys.stderr` in the middle of the traversal, which can run + * arbitrary Python code and invalidate the reference counts already sampled. + * + * `missing_reachable` may be NULL to skip the recording. + */ static traverseproc -get_reachable_proc(PyTypeObject *tp) +get_reachable_proc(PyTypeObject *tp, _Py_hashtable_t *missing_reachable) { if (tp->tp_reachable != NULL) { return tp->tp_reachable; } - if (tp->tp_traverse != NULL) { + if (missing_reachable != NULL + && _Py_hashtable_get_entry(missing_reachable, tp) == NULL) + { + // Types are frozen rather than moved, so `_move_obj()` returns before it + // samples their reference count. Holding one here can therefore not + // disturb the LRC of any region. + if (_Py_hashtable_set(missing_reachable, Py_NewRef(tp), + (void *)(Py_uintptr_t)(tp->tp_traverse != NULL)) < 0) { + Py_DECREF(tp); + // A failed warning must not fail the close. + PyErr_Clear(); + } + } + + // Always return the wrapper; even when tp_traverse is NULL, the wrapper + // will still visit the type object which tp_reachable is expected to do. + return traverse_via_tp_traverse; +} + +static int +report_missing_reachable_type( + _Py_hashtable_t *ht, const void *key, const void *value, void *user_data) +{ + PyTypeObject *tp = (PyTypeObject *)key; + if (value) { PySys_FormatStderr( "regions: type '%.100s' has tp_traverse but no tp_reachable\n", tp->tp_name); - } else { + } + else { PySys_FormatStderr( "regions: type '%.100s' has no tp_traverse and no tp_reachable\n", tp->tp_name); } + return 0; +} - // Always return the wrapper; even when tp_traverse is NULL, the wrapper - // will still visit the type object which tp_reachable is expected to do. - return traverse_via_tp_traverse; +static int +release_missing_reachable_type( + _Py_hashtable_t *ht, const void *key, const void *value, void *user_data) +{ + Py_DECREF((PyObject *)key); + return 0; } // ################################################################### @@ -316,22 +368,34 @@ gc_list_dissolve(PyGC_Head *list) { gc_list_merge(list, &(gc_state->old[0].head)); } -static void detach_weak_refs(PyGC_Head *gc_list) { - PyGC_Head *current = GC_NEXT(gc_list); - while (current != gc_list) { - PyObject *item = _Py_FROM_GC(current); -#ifdef PY_DEBUG - Py_ssize_t weak_ctn = _PyWeakref_GetWeakrefCount(item); - if (weak_ctn) { - dbg("- Clearing %zd weak references to %p", weak_ctn, item); - } -#endif - if (_PyType_SUPPORTS_WEAKREFS(Py_TYPE(item))) { - _PyWeakref_ClearWeakRefsNoCallbacks(item); - } +static int +detach_weak_refs_visit(_Py_hashtable_t *ht, const void *key, const void *value, void *user_data) +{ + PyObject *item = (PyObject *)key; + if (!_PyType_SUPPORTS_WEAKREFS(Py_TYPE(item))) { + return 0; + } - current = GC_NEXT(current); +#ifdef Py_DEBUG + Py_ssize_t weak_ctn = _PyWeakref_GetWeakrefCount(item); + if (weak_ctn) { + dbg("- Clearing %zd weak references to %p", weak_ctn, item); } +#endif + _PyWeakref_ClearWeakRefsNoCallbacks(item); + return 0; +} + +/* Detaches all weak references pointing to objects inside the region. + * + * This walks the set of traced objects instead of the region's GC list, since + * objects that are not tracked by the GC never enter that list. Missing one + * would leave a live weak reference pointing into the closed region, which is + * enough for external code to read and mutate its contents. + */ +static void detach_weak_refs(_Py_hashtable_t *visited) { + // `detach_weak_refs_visit()` never fails, so the result can be ignored. + (void)_Py_hashtable_foreach(visited, detach_weak_refs_visit, NULL); } typedef struct { @@ -350,21 +414,23 @@ typedef struct { Py_ssize_t internal_bridge_refs; } TracingRegionObject; -static void _region_close(TracingRegionObject *self, Py_ssize_t bridge_rc) { +static void _region_close( + TracingRegionObject *self, + Py_ssize_t bridge_rc, + _Py_hashtable_t *visited +) { if (!self->open) { return; } dbg("Closing region %p", self); - // FIXME: This can be optimized, for example by inserting all objects - // with weak refs in the beginning. - detach_weak_refs(&self->gc_list); + detach_weak_refs(visited); // TODO(regions): explain RC magic if (bridge_rc != 0) { assert(bridge_rc >= 0); - dbg("- subtracting %ld internal references from the bridge object %p", bridge_rc, self); + dbg("- subtracting %zd internal references from the bridge object %p", bridge_rc, self); _Py_RefcntAdd(self, -bridge_rc); self->internal_bridge_refs = bridge_rc; } else { @@ -384,7 +450,7 @@ static void _open_region(TracingRegionObject *self) { // We re-add the internal references to the RC that have been subtracted during closing. if (self->internal_bridge_refs != 0) { assert(self->internal_bridge_refs >= 0); - dbg("- adding %ld internal references from the bridge object %p", self->internal_bridge_refs, self); + dbg("- adding %zd internal references from the bridge object %p", self->internal_bridge_refs, self); _Py_RefcntAdd(self, self->internal_bridge_refs); self->internal_bridge_refs = 0; } @@ -396,13 +462,15 @@ static void _open_region(TracingRegionObject *self) { self->open = true; } -const int PER_REGION_TRACE_LIMIT = 2; +#define PER_REGION_TRACE_LIMIT 2 typedef struct { - // This is the stack of pending regions needing to be closed to close - // this region tree. Objects will be inqueued `PER_REGION_TRACE_LIMIT` - // times. It the region is not closed when it hits the limit, the closing - // will fail. + // This is the stack of regions that still need to be closed to close this + // region tree. A region stays on the stack until it is closed, so anything + // its trace discovers is pushed on top of it and handled first. The loop can + // therefore only drain once every region in the tree is closed. + // + // How many attempts a region gets is tracked by `tracing_counts`. PyObject *pending; // This tracks per region in the tree how often it has been traversed. // Some things require the trace to be redone, namely freezing an object @@ -413,24 +481,67 @@ typedef struct { // Theoretically, this may reject some programs that would eventually // reach a fixed point, but if somebody wants to do dark magic, that's // really not our problem. - _Py_hashtable_t *traceing_counts; + _Py_hashtable_t *tracing_counts; + // The types that had to be traversed via tp_traverse because they have no + // tp_reachable. Used to report each of them once per trace, see + // `get_reachable_proc()`. + _Py_hashtable_t *missing_reachable; } tree_trace_state_t; static void tree_trace_state_destroy(tree_trace_state_t* state) { - if (state->traceing_counts) { - _Py_hashtable_destroy(state->traceing_counts); - state->traceing_counts = NULL; + if (state->tracing_counts) { + _Py_hashtable_destroy(state->tracing_counts); + state->tracing_counts = NULL; + } + if (state->missing_reachable) { + (void)_Py_hashtable_foreach( + state->missing_reachable, release_missing_reachable_type, NULL); + _Py_hashtable_destroy(state->missing_reachable); + state->missing_reachable = NULL; } if (state->pending) { Py_CLEAR(state->pending); } } +/* Reports the types that `get_reachable_proc()` had to fall back for. + * + * This has to run after the traversal is over, since writing to `sys.stderr` + * can execute arbitrary Python code. + */ +static void report_missing_reachable(tree_trace_state_t* state) { + if (state->missing_reachable == NULL + || _Py_hashtable_len(state->missing_reachable) == 0) + { + return; + } + + // Keep whatever the trace is raising; a failed warning is not worth + // replacing a region error with. + PyObject *exc = PyErr_GetRaisedException(); + (void)_Py_hashtable_foreach( + state->missing_reachable, report_missing_reachable_type, NULL); + PyErr_SetRaisedException(exc); +} + static int tree_trace_state_init(tree_trace_state_t* state) { - state->traceing_counts = _Py_hashtable_new( + // Both fields have to be cleared up front, so that the error path below can + // call `tree_trace_state_destroy()` before they have all been assigned. + state->tracing_counts = NULL; + state->missing_reachable = NULL; + state->pending = NULL; + + state->tracing_counts = _Py_hashtable_new( + _Py_hashtable_hash_ptr, + _Py_hashtable_compare_direct); + if (state->tracing_counts == NULL) { + goto error; + } + + state->missing_reachable = _Py_hashtable_new( _Py_hashtable_hash_ptr, _Py_hashtable_compare_direct); - if (state->traceing_counts == NULL) { + if (state->missing_reachable == NULL) { goto error; } @@ -484,33 +595,14 @@ static void region_trace_state_destroy(region_trace_state_t* state) { } } -static int region_trace_state_reset(region_trace_state_t* state, PyGC_Head *gc_list) { - assert(gc_list == NULL || gc_list_is_empty(gc_list)); - - SUCCEEDS(PyList_Clear(state->pending)); - _Py_hashtable_clear(state->visited); - - // state->tree_trace_state stays unchanged - // state->bridge stays unchanged - state->src = NULL; - - state->external_rc = 0; - state->bridge_rc = 0; - state->gc_list = gc_list; - state->restart = false; - - return 0; -error: - region_trace_state_destroy(state); - return -1; -} - static int region_trace_state_init( region_trace_state_t* state, PyObject* bridge, PyGC_Head* gc_list, tree_trace_state_t *tree_trace_state ) { + assert(gc_list == NULL || gc_list_is_empty(gc_list)); + state->pending = NULL; state->visited = NULL; @@ -526,11 +618,16 @@ static int region_trace_state_init( goto error; } - - state->bridge = bridge; state->tree_trace_state = tree_trace_state; + state->bridge = bridge; + state->src = NULL; - return region_trace_state_reset(state, gc_list); + state->external_rc = 0; + state->bridge_rc = 0; + state->gc_list = gc_list; + state->restart = false; + + return 0; error: region_trace_state_destroy(state); return -1; @@ -545,40 +642,47 @@ static void region_trace_state_set_restart(region_trace_state_t* state) { } typedef struct { - _Py_hashtable_t *obj_table; - Py_ssize_t objs; - Py_ssize_t incoming_refs; -} trace_info_t; - -typedef struct { - _Py_hashtable_t *obj_table; - _Py_hashtable_t *problem_obj_table; + // Every object with incoming references, used to mark up the mermaid graph. + _Py_hashtable_t *problem_objs; + // The subset of `problem_objs` that the error message lists, capped at + // `ERROR_OBJECT_REPORT_COUNT` entries. + _Py_hashtable_t *reported_objs; Py_ssize_t incoming_refs; } close_error_info_t; typedef struct { - _Py_hashtable_t *reported_target; - _Py_hashtable_t *problem_target; - PyObject *bridge; - Py_ssize_t ignored_refs; + _Py_hashtable_t *problem_objs; + _Py_hashtable_t *reported_objs; } close_error_filter_t; typedef struct { - PyUnicodeWriter *writer; - Py_ssize_t accounted; + // A strong reference, see `collect_incoming_ref()`. + PyObject *obj; + Py_ssize_t refs; +} incoming_ref_entry_t; + +typedef struct { + // `collect_close_error_obj()` caps the reported set at this size. + incoming_ref_entry_t entries[ERROR_OBJECT_REPORT_COUNT]; + Py_ssize_t count; } incoming_ref_report_t; typedef struct { PyUnicodeWriter *writer; _Py_hashtable_t *visited; - _Py_hashtable_t *error_objs; + _Py_hashtable_t *problem_objs; _Py_hashtable_t *reported_objs; PyObject *pending; PyObject *src; } mermaid_dump_state_t; -const int TRACE_RES_ERR = -1; -const int TRACE_RES_DONE = 0; +enum { + TRACE_RES_ERR = -1, + TRACE_RES_DONE = 0, + // The trace itself succeeded, but it was based on information that changed + // while it ran, so the region is still open and needs another attempt. + TRACE_RES_RESTART = 1, +}; static int collect_close_error_obj(_Py_hashtable_t *ht, const void *key, const void *value, void *user_data) @@ -586,109 +690,131 @@ collect_close_error_obj(_Py_hashtable_t *ht, const void *key, const void *value, close_error_filter_t *filter = (close_error_filter_t *)user_data; Py_ssize_t refs = (Py_ssize_t)value; - if ((PyObject *)key == filter->bridge) { - refs -= 1; - filter->ignored_refs += 1; - } - + // Objects whose every reference came from inside the region are not part of + // the problem. if (refs <= 0) { return 0; } - if (_Py_hashtable_set(filter->problem_target, key, (void *)refs) < 0) { + if (_Py_hashtable_set(filter->problem_objs, key, (void *)refs) < 0) { return -1; } - if (_Py_hashtable_len(filter->reported_target) < ERROR_OBJECT_REPORT_COUNT) { - if (_Py_hashtable_set(filter->reported_target, key, (void *)refs) < 0) { + if (_Py_hashtable_len(filter->reported_objs) < ERROR_OBJECT_REPORT_COUNT) { + if (_Py_hashtable_set(filter->reported_objs, key, (void *)refs) < 0) { return -1; } } return 0; } +static void +close_error_info_destroy(close_error_info_t *info) +{ + if (info->problem_objs != NULL) { + _Py_hashtable_destroy(info->problem_objs); + info->problem_objs = NULL; + } + if (info->reported_objs != NULL) { + _Py_hashtable_destroy(info->reported_objs); + info->reported_objs = NULL; + } +} + static int close_error_info_init(close_error_info_t *info, region_trace_state_t *state) { info->incoming_refs = state->external_rc; - info->problem_obj_table = NULL; - info->obj_table = _Py_hashtable_new( + info->problem_objs = NULL; + info->reported_objs = NULL; + info->problem_objs = _Py_hashtable_new( _Py_hashtable_hash_ptr, _Py_hashtable_compare_direct); - if (info->obj_table == NULL) { + if (info->problem_objs == NULL) { return -1; } - info->problem_obj_table = _Py_hashtable_new( + info->reported_objs = _Py_hashtable_new( _Py_hashtable_hash_ptr, _Py_hashtable_compare_direct); - if (info->problem_obj_table == NULL) { - _Py_hashtable_destroy(info->obj_table); - info->obj_table = NULL; + if (info->reported_objs == NULL) { + close_error_info_destroy(info); return -1; } - close_error_filter_t filter = {info->obj_table, info->problem_obj_table, state->bridge, 0}; + close_error_filter_t filter = {info->problem_objs, info->reported_objs}; int res = _Py_hashtable_foreach(state->visited, collect_close_error_obj, &filter); if (res < 0) { - _Py_hashtable_destroy(info->obj_table); - info->obj_table = NULL; - _Py_hashtable_destroy(info->problem_obj_table); - info->problem_obj_table = NULL; + close_error_info_destroy(info); return -1; } - info->incoming_refs -= filter.ignored_refs; return 0; } -static void -close_error_info_destroy(close_error_info_t *info) -{ - if (info->obj_table != NULL) { - _Py_hashtable_destroy(info->obj_table); - info->obj_table = NULL; - } - if (info->problem_obj_table != NULL) { - _Py_hashtable_destroy(info->problem_obj_table); - info->problem_obj_table = NULL; - } -} - static int -report_incoming_ref(_Py_hashtable_t *ht, const void *key, const void *value, void *user_data) +collect_incoming_ref(_Py_hashtable_t *ht, const void *key, const void *value, void *user_data) { incoming_ref_report_t *report = (incoming_ref_report_t *)user_data; - PyObject *obj = (PyObject *)key; - Py_ssize_t refs = (Py_ssize_t)value; - - report->accounted += refs; - if (PyUnicodeWriter_Format(report->writer, - "- %zd incoming reference%s to '%S'\n", - refs, (refs == 1) ? "" : "s", obj) < 0) { - return -1; + assert(report->count < ERROR_OBJECT_REPORT_COUNT); + if (report->count >= ERROR_OBJECT_REPORT_COUNT) { + return 0; } + + incoming_ref_entry_t *entry = &report->entries[report->count]; + // The hashtable stores raw pointers without owning a reference. Taking one + // here keeps every reported object alive while `__str__` runs on the others, + // since that can execute arbitrary code and drop the last reference to any + // of them. + entry->obj = Py_NewRef((PyObject *)key); + entry->refs = (Py_ssize_t)value; + report->count += 1; return 0; } +static void +incoming_ref_report_clear(incoming_ref_report_t *report) +{ + for (Py_ssize_t i = 0; i < report->count; i++) { + Py_CLEAR(report->entries[i].obj); + } + report->count = 0; +} + static PyObject * build_close_error_message(close_error_info_t *info) { - PyUnicodeWriter *writer = PyUnicodeWriter_Create(0); - if (writer == NULL) { - return NULL; + incoming_ref_report_t report = {{{NULL, 0}}, 0}; + PyUnicodeWriter *writer = NULL; + + // Collect the reported objects, and with them their references, before any + // of them is formatted below. + if (_Py_hashtable_foreach(info->reported_objs, collect_incoming_ref, &report) < 0) { + goto error; } - incoming_ref_report_t report = {writer, 0}; + writer = PyUnicodeWriter_Create(0); + if (writer == NULL) { + goto error; + } if (PyUnicodeWriter_WriteUTF8(writer, "The region could not be closed due to:\n", -1) < 0) { goto error; } - if (_Py_hashtable_foreach(info->obj_table, report_incoming_ref, &report) < 0) { - goto error; + Py_ssize_t accounted = 0; + for (Py_ssize_t i = 0; i < report.count; i++) { + PyObject *obj = report.entries[i].obj; + Py_ssize_t refs = report.entries[i].refs; + accounted += refs; + + if (PyUnicodeWriter_Format(writer, + "- %zd incoming reference%s to %s '%S'\n", + refs, (refs == 1) ? "" : "s", Py_TYPE(obj)->tp_name, obj) < 0) { + goto error; + } } - if (report.accounted < info->incoming_refs) { - Py_ssize_t others = info->incoming_refs - report.accounted; + if (accounted < info->incoming_refs) { + Py_ssize_t others = info->incoming_refs - accounted; if (PyUnicodeWriter_Format(writer, "- %zd reference%s to other objects\n", others, (others == 1) ? "" : "s") < 0) { @@ -696,12 +822,14 @@ build_close_error_message(close_error_info_t *info) } } + incoming_ref_report_clear(&report); return PyUnicodeWriter_Finish(writer); error: if (!PyErr_Occurred()) { PyErr_SetString(PyExc_RuntimeError, "failed to build region close error message"); } + incoming_ref_report_clear(&report); PyUnicodeWriter_Discard(writer); return NULL; } @@ -730,7 +858,7 @@ static int mermaid_write_class( PyUnicodeWriter *writer, PyObject *obj, - _Py_hashtable_t *error_objs, + _Py_hashtable_t *problem_objs, _Py_hashtable_t *reported_objs) { if (_Py_IsImmutable(obj)) { @@ -739,7 +867,7 @@ mermaid_write_class( if (_Py_hashtable_get_entry(reported_objs, obj) != NULL) { return PyUnicodeWriter_Format(writer, " class n%p error\n", obj); } - if (_Py_hashtable_get_entry(error_objs, obj) != NULL) { + if (_Py_hashtable_get_entry(problem_objs, obj) != NULL) { return PyUnicodeWriter_Format(writer, " class n%p problem\n", obj); } return 0; @@ -877,7 +1005,7 @@ mermaid_visit_labeled( if (PyUnicodeWriter_WriteUTF8(state->writer, "\n", -1) < 0) { return -1; } - if (mermaid_write_class(state->writer, obj, state->error_objs, state->reported_objs) < 0) { + if (mermaid_write_class(state->writer, obj, state->problem_objs, state->reported_objs) < 0) { return -1; } @@ -940,7 +1068,9 @@ mermaid_traverse(PyObject *obj, mermaid_dump_state_t *state) return mermaid_visit_sequence(obj, state); } - traverseproc proc = get_reachable_proc(Py_TYPE(obj)); + // The trace already reports the types without tp_reachable; the graph dump + // walks the same objects and would only repeat it. + traverseproc proc = get_reachable_proc(Py_TYPE(obj), NULL); return proc(obj, (visitproc)mermaid_visit, (void *)state); } @@ -961,14 +1091,14 @@ mermaid_dump_state_destroy(mermaid_dump_state_t *state) static int mermaid_dump_state_init( mermaid_dump_state_t *state, - _Py_hashtable_t *error_objs, + _Py_hashtable_t *problem_objs, _Py_hashtable_t *reported_objs) { state->writer = NULL; state->visited = NULL; state->pending = NULL; state->src = NULL; - state->error_objs = error_objs; + state->problem_objs = problem_objs; state->reported_objs = reported_objs; state->writer = PyUnicodeWriter_Create(0); @@ -995,14 +1125,24 @@ mermaid_dump_state_init( static int dump_mermaid_diagram( PyObject *root, - _Py_hashtable_t *error_objs, + _Py_hashtable_t *problem_objs, _Py_hashtable_t *reported_objs) { int res = -1; mermaid_dump_state_t state; PyObject *diagram = NULL; + // Owns the item currently being traversed, released at `finally`. + PyObject *item = NULL; + + // Writing a file into the working directory is too surprising to do by + // default, so the graph is only dumped when it has been asked for. The + // value of the variable is the path to write to. + const char *path = Py_GETENV(REGION_GRAPH_ENV_VAR); + if (path == NULL || *path == '\0') { + return 0; + } - if (mermaid_dump_state_init(&state, error_objs, reported_objs) < 0) { + if (mermaid_dump_state_init(&state, problem_objs, reported_objs) < 0) { return -1; } @@ -1014,10 +1154,14 @@ dump_mermaid_diagram( } while (PyList_GET_SIZE(state.pending) > 0) { - PyObject *item = list_pop(state.pending); + Py_XSETREF(item, list_pop(state.pending)); + if (item == NULL) { + goto finally; + } state.src = item; SUCCEEDS(mermaid_traverse(item, &state)); } + Py_CLEAR(item); diagram = PyUnicodeWriter_Finish(state.writer); state.writer = NULL; @@ -1030,30 +1174,42 @@ dump_mermaid_diagram( goto finally; } - FILE *f = fopen("region-graph.md", "w"); - if (f != NULL) { - fputs( + FILE *f = fopen(path, "w"); + if (f == NULL) { + PyErr_SetFromErrnoWithFilename(PyExc_OSError, path); + goto finally; + } + if (fputs( "
\n" "\n" "```mermaid\n" "%%{init: {'theme': 'neutral', 'themeVariables': { 'fontSize': '16px' }}}%%\n" "\n", - f); - fputs(body, f); - fputs( + f) < 0 + || fputs(body, f) < 0 + || fputs( "\n" "classDef immutable fill:#94f7ff\n" "classDef problem fill:#ffe8d6,stroke:#f08c00,stroke-width:2px\n" "classDef error fill:#ffe8d6,stroke:red,stroke-width:4px\n" "```\n" "
\n", - f); + f) < 0) + { + PyErr_SetFromErrnoWithFilename(PyExc_OSError, path); fclose(f); + goto finally; + } + // Buffered writes can still fail here, so this result matters too. + if (fclose(f) != 0) { + PyErr_SetFromErrnoWithFilename(PyExc_OSError, path); + goto finally; } res = 0; finally: + Py_XDECREF(item); mermaid_dump_state_destroy(&state); Py_XDECREF(diagram); return res; @@ -1086,10 +1242,14 @@ static int _move_obj(PyObject* obj, region_trace_state_t* state) { case Py_MOVABLE_COWN: return 0; default: - assert(false); - break; + Py_UNREACHABLE(); } + // References to the bridge object are allowed and counted by + // `state->bridge_rc` instead. `_trace_visit()` intercepts them, so the + // bridge must never end up in `visited` or in the LRC below. + assert(obj != state->bridge); + // Update the LRC, -1 for the reference we just followed Py_ssize_t lrc_change = Py_REFCNT(obj) - 1; dbg(" - moving %p; LRC += %zd", obj, lrc_change); @@ -1122,17 +1282,6 @@ static int _move_obj(PyObject* obj, region_trace_state_t* state) { return 0; } -static int -_enqueue_region_for_closing(tree_trace_state_t *state, PyObject *region) -{ - for (int i = 0; i < PER_REGION_TRACE_LIMIT; i++) { - if (PyList_Append(state->pending, region) < 0) { - return -1; - } - } - return 0; -} - static int _trace_visit(PyObject* obj, region_trace_state_t* state) { // References to immutable objects are allowed if (_PyImmutability_CanViewAsImmutable(obj)) { @@ -1148,6 +1297,15 @@ static int _trace_visit(PyObject* obj, region_trace_state_t* state) { return 0; } + // Check if the object is already part of the region + _Py_hashtable_entry_t *entry = _Py_hashtable_get_entry(state->visited, (void*)obj); + if (entry != NULL) { + entry->value = (void*)(((Py_ssize_t)entry->value) - 1); + dbg(" - Internal reference to %p; LRC -= 1", obj); + state->external_rc -= 1; + return 0; + } + // References external regions turns them into sub-regions. These // need to be traversed and closed separately if (Region_Check(obj)) { @@ -1157,7 +1315,7 @@ static int _trace_visit(PyObject* obj, region_trace_state_t* state) { } else { // The child region is open, we need to traverse it first and then // retry closing this. - if (_enqueue_region_for_closing(state->tree_trace_state, obj) < 0) { + if (PyList_Append(state->tree_trace_state->pending, obj) < 0) { return -1; } region_trace_state_set_restart(state); @@ -1165,15 +1323,6 @@ static int _trace_visit(PyObject* obj, region_trace_state_t* state) { return 0; } - // Check if the object is already part of the region - _Py_hashtable_entry_t *entry = _Py_hashtable_get_entry(state->visited, (void*)obj); - if (entry != NULL) { - entry->value -= 1; - dbg(" - Internal reference to %p; LRC -= 1", obj); - state->external_rc -= 1; - return 0; - } - return _move_obj(obj, state); } @@ -1181,42 +1330,49 @@ static int _trace_visit(PyObject* obj, region_trace_state_t* state) { static int _try_close_region(PyObject *region_obj, tree_trace_state_t *tree_trace_state) { assert(Region_Check(region_obj)); TracingRegionObject* region = (TracingRegionObject*)region_obj; - + // Init trace state. region_trace_state_t state; if (region_trace_state_init(&state, _PyObject_CAST(region), ®ion->gc_list, tree_trace_state)) { return TRACE_RES_ERR; } int region_trace_res = TRACE_RES_DONE; + // Owns the item currently being traversed, released at `finally`. + PyObject *item = NULL; SUCCEEDS(PyList_Append(state.pending, _PyObject_CAST(region))); while (PyList_GET_SIZE(state.pending) > 0) { // Find the next pending item: - PyObject *item = list_pop(state.pending); + Py_XSETREF(item, list_pop(state.pending)); + if (item == NULL) { + goto error; + } // Traverse item state.src = item; dbg(" - traversing %p", item); - traverseproc proc = get_reachable_proc(Py_TYPE(item)); + traverseproc proc = get_reachable_proc(Py_TYPE(item), tree_trace_state->missing_reachable); SUCCEEDS(proc(item, (visitproc)_trace_visit, (void*)&state)); // TODO(regions): Handle weakrefs assert(!PyWeakref_Check(item)); } + Py_CLEAR(item); if (state.restart) { gc_list_dissolve(®ion->gc_list); + region_trace_res = TRACE_RES_RESTART; goto finally; } if (state.external_rc == 0) { - _region_close(region, state.bridge_rc); + _region_close(region, state.bridge_rc, state.visited); } else { gc_list_dissolve(®ion->gc_list); dbg("- Failed to close region %p, there are %zd incoming references", region, state.external_rc); - close_error_info_t error_info = {NULL, 0}; + close_error_info_t error_info = {0}; if (close_error_info_init(&error_info, &state) < 0) { goto error; } @@ -1224,9 +1380,12 @@ static int _try_close_region(PyObject *region_obj, tree_trace_state_t *tree_trac // Borrowed error tables; dump_mermaid_diagram() does not take ownership. if (dump_mermaid_diagram( region_obj, - error_info.problem_obj_table, - error_info.obj_table) < 0) { - PyErr_Clear(); + error_info.problem_objs, + error_info.reported_objs) < 0) { + // The graph is a diagnostic aid. Report why it is missing, but + // don't let that replace the region error being built here. + PyErr_FormatUnraisable( + "Exception ignored while writing the region graph"); } } @@ -1244,6 +1403,7 @@ static int _try_close_region(PyObject *region_obj, tree_trace_state_t *tree_trac error: region_trace_res = TRACE_RES_ERR; finally: + Py_CLEAR(item); region_trace_state_destroy(&state); return region_trace_res; @@ -1257,52 +1417,65 @@ static int try_close_region_tree(PyObject *root) { return -1; } - _enqueue_region_for_closing(&state, root); - int tree_trace_res = TRACE_RES_DONE; + + SUCCEEDS(PyList_Append(state.pending, root)); + while (PyList_GET_SIZE(state.pending) > 0) { - // Find the next pending item: - PyObject *region = list_pop(state.pending); + // Look at the region on top of the stack without removing it. A region + // stays queued until it is closed, so the sub-regions that its trace + // discovers end up above it and are closed first. Draining the stack + // therefore means every region in the tree is closed, which is what lets + // this function report success. + Py_ssize_t top = PyList_GET_SIZE(state.pending) - 1; + PyObject *region = PyList_GET_ITEM(state.pending, top); assert(Region_Check(region)); - // If the region is closed we can safely skip it. Regions can be enqueued - // multiple times, this handles all safe cases. + // A closed region has nothing left to do. Regions can be queued more + // than once, this handles all safe cases. if (_PyTracingRegion_IsClosed(region)) { + SUCCEEDS(PyList_SetSlice(state.pending, top, top + 1, NULL)); continue; } + // Account for this attempt before running it. Counting afterwards would + // report a region that was closed by its last attempt as a failure, and + // would grant `PER_REGION_TRACE_LIMIT + 1` attempts. + _Py_hashtable_entry_t *entry = _Py_hashtable_get_entry(state.tracing_counts, (void*)region); + if (entry == NULL) { + SUCCEEDS(_Py_hashtable_set(state.tracing_counts, (void*)region, (void*)1)); + } else if ((Py_uintptr_t)entry->value < PER_REGION_TRACE_LIMIT) { + entry->value = (void*)(((Py_uintptr_t)entry->value) + 1); + } else { + // FIXME(regions): It would be nicer to spend the last attempt on a + // trace that reports the objects keeping the region open, like the + // `external_rc != 0` path in `_try_close_region()` does, instead of + // this bare message. The catch is that such a trace may close the + // region after all, which is why it can't simply be run here. + PyErr_Format( + PyExc_RuntimeError, + "the region %p could not be closed after %d tracing attempts", + (void *)region, + PER_REGION_TRACE_LIMIT); + goto error; + } + dbg("- tracing region %p", region); int res = _try_close_region(region, &state); if (res == TRACE_RES_ERR) { goto error; } - - _Py_hashtable_entry_t *entry = _Py_hashtable_get_entry(state.traceing_counts, (void*)region); - if (entry != NULL) { - if ((Py_uintptr_t)entry->value < PER_REGION_TRACE_LIMIT) { - entry->value = (void*)(((Py_uintptr_t)entry->value) + 1); - } else { - // FIXME(regions): This should maybe be turned into a trace that creates a - // error, the problem is, that this retrace may then close the region. This - // means that this increase the tracing limit by one. There is also a question - // how often this actually happens. This case is pretty specific for sub-regions - // that can't be closed and pre-freeze hooks - PyErr_Format( - PyExc_RuntimeError, - "the region %p could not be closed after %d tracing attempts", - (void *)region, - PER_REGION_TRACE_LIMIT); - goto error; - } - } else { - SUCCEEDS(_Py_hashtable_set(state.traceing_counts, (void*)region, (void*)1)); - } + // A restarted trace leaves the region open on purpose. It keeps its slot + // on the stack and is retried once the sub-regions that its trace pushed + // on top of it have been closed. + assert(res == TRACE_RES_RESTART || _PyTracingRegion_IsClosed(region)); } goto finally; error: tree_trace_res = TRACE_RES_ERR; finally: + report_missing_reachable(&state); tree_trace_state_destroy(&state); return tree_trace_res; @@ -1312,13 +1485,35 @@ static int try_close_region_tree(PyObject *root) { // Region Object // ################################################################### -static int -TracingRegion_init(TracingRegionObject *self, PyObject *args, PyObject *kwargs) { +static PyObject * +TracingRegion_new(PyTypeObject *type, PyObject *args, PyObject *kwargs) { + TracingRegionObject *self = (TracingRegionObject *)type->tp_alloc(type, 0); + if (self == NULL) { + return NULL; + } + + // The region is set up here rather than in `tp_init()`, so that a region + // can never be observed in an uninitialized state. gc_list_init(&self->gc_list); // We make the region open by default, this ensures that the first close // will handle the region type correctly. Alternatively, we could make them // closed in the beginning, but then handle the cases specifically. self->open = true; + + return (PyObject *)self; +} + +static int +TracingRegion_init(TracingRegionObject *self, PyObject *args, PyObject *kwargs) { + // `tp_new()` already set the region up. Re-running the initialization here + // would reset the GC list holding the contents of a closed region and drop + // the reference count that `_region_close()` subtracted from the bridge + // object, so this only validates the arguments. + if (!_PyArg_NoPositional("TracingRegion", args) + || !_PyArg_NoKeywords("TracingRegion", kwargs)) + { + return -1; + } return 0; } @@ -1337,11 +1532,44 @@ TracingRegion_clear(TracingRegionObject *self) { return 0; } +static void +TracingRegion_finalize(PyObject *op) { + // Reopening the region restores the references that the objects inside it + // hold to the bridge object, which can resurrect `op`. This has to run as a + // finalizer so that `PyObject_CallFinalizerFromDealloc()` notices the + // resurrection, instead of freeing an object that is still referenced. + (void)TracingRegion_clear((TracingRegionObject *)op); +} + static void TracingRegion_dealloc(TracingRegionObject *self) { + PyObject *op = (PyObject *)self; + + // `PyObject_CallFinalizerFromDealloc()` requires a GC type to be tracked + // while the finalizer runs, but the bridge object of a closed region may + // get untracked by an owning cown. + if (!_PyObject_GC_IS_TRACKED(op)) { + _PyObject_GC_TRACK(op); + } + if (PyObject_CallFinalizerFromDealloc(op) < 0) { + // The bridge object was resurrected by the references from inside the + // region. It is deallocated again once those are gone. + return; + } + PyObject_GC_UnTrack(self); - TracingRegion_clear(self); - Py_TYPE(self)->tp_free((PyObject *)self); + Py_TYPE(self)->tp_free(op); +} + +static PyObject * +TracingRegion_repr(PyObject *op) { + TracingRegionObject *self = (TracingRegionObject*)op; + + // Deliberately reads `open` instead of going through the attribute access + // below, so that reporting on a region does not open it. Deliberately + // address free as well, so that error messages are reproducible. + return PyUnicode_FromFormat( + "", self->open ? "open" : "closed"); } static PyObject * @@ -1412,7 +1640,7 @@ TracingRegion_set_dict(PyObject *op, PyObject *value, void *Py_UNUSED(context)) int _PyTracingRegion_Close(PyObject* op) { TracingRegionObject *self = (TracingRegionObject*)op; if (!self->open) { - return 1; + return 0; } assert(gc_list_is_empty(&self->gc_list)); @@ -1438,6 +1666,7 @@ PyTypeObject _PyTracingRegion_Type = { .tp_name = "TracingRegion", .tp_basicsize = sizeof(TracingRegionObject), .tp_dealloc = (destructor)TracingRegion_dealloc, + .tp_repr = TracingRegion_repr, .tp_flags = Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_GC | Py_TPFLAGS_IMMUTABLETYPE, .tp_traverse = (traverseproc)TracingRegion_traverse, .tp_clear = (inquiry)TracingRegion_clear, @@ -1446,7 +1675,8 @@ PyTypeObject _PyTracingRegion_Type = { .tp_getattro = TracingRegion_getattro, .tp_setattro = TracingRegion_setattro, .tp_init = (initproc)TracingRegion_init, - .tp_new = PyType_GenericNew, + .tp_new = TracingRegion_new, + .tp_finalize = TracingRegion_finalize, .tp_reachable = _PyObject_ReachableVisitTypeAndTraverse, }; From 451149cb3f04d4ebbb791e444d21054096f9c977 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Thu, 20 Aug 2026 17:16:32 +0200 Subject: [PATCH 23/24] TRegions: Delete close region content --- Include/internal/pycore_gc.h | 19 ++++++ Objects/tracingregionobject.c | 105 +++++++++++++++++++++++++++++----- Python/gc.c | 19 +++--- 3 files changed, 121 insertions(+), 22 deletions(-) diff --git a/Include/internal/pycore_gc.h b/Include/internal/pycore_gc.h index 2dfce32237a83c3..6a1f91d2bad7cde 100644 --- a/Include/internal/pycore_gc.h +++ b/Include/internal/pycore_gc.h @@ -352,6 +352,25 @@ extern PyObject *_PyGC_GetObjects(PyInterpreterState *interp, int generation); extern PyObject *_PyGC_GetReferrers(PyInterpreterState *interp, PyObject *objs); // Functions to clear types free lists +/* Disposal of a list of objects that are known to be unreachable. Used by the + * collector itself and by anything else that owns a set of objects it has + * established to be garbage, such as a closed tracing region. + * + * `_PyGC_FinalizeGarbage()` runs the finalizer of every object in `collectable`, + * before anything is cleared, so that a `__del__` still sees its object intact. + * + * `_PyGC_DeleteGarbage()` then breaks the references between them, deallocating + * every object whose reference count reaches zero. Objects that a finalizer kept + * alive are moved to `old` instead. + * + * Neither may be called with an exception set. Only available in the default + * build; the free-threaded collector has its own implementation. + */ +#ifndef Py_GIL_DISABLED +extern void _PyGC_FinalizeGarbage(PyGC_Head *collectable); +extern void _PyGC_DeleteGarbage(PyGC_Head *collectable, PyGC_Head *old); +#endif + extern void _PyGC_ClearAllFreeLists(PyInterpreterState *interp); extern void _Py_ScheduleGC(PyThreadState *tstate); extern void _Py_RunGC(PyThreadState *tstate); diff --git a/Objects/tracingregionobject.c b/Objects/tracingregionobject.c index 911045d03cf0b06..6cc182ce84ef4ce 100644 --- a/Objects/tracingregionobject.c +++ b/Objects/tracingregionobject.c @@ -440,6 +440,19 @@ static void _region_close( self->open = false; } +/* Re-adds the references to the bridge object that `_region_close()` subtracted. + * + * Note that this may resurrect the bridge object. Callers may need to handle this case. + */ +static void _restore_internal_bridge_refs(TracingRegionObject *self) { + if (self->internal_bridge_refs != 0) { + assert(self->internal_bridge_refs >= 0); + dbg("- adding %zd internal references from the bridge object %p", self->internal_bridge_refs, self); + _Py_RefcntAdd(self, self->internal_bridge_refs); + self->internal_bridge_refs = 0; + } +} + static void _open_region(TracingRegionObject *self) { if (self->open) { return; @@ -447,13 +460,7 @@ static void _open_region(TracingRegionObject *self) { dbg("Opening region %p", self); - // We re-add the internal references to the RC that have been subtracted during closing. - if (self->internal_bridge_refs != 0) { - assert(self->internal_bridge_refs >= 0); - dbg("- adding %zd internal references from the bridge object %p", self->internal_bridge_refs, self); - _Py_RefcntAdd(self, self->internal_bridge_refs); - self->internal_bridge_refs = 0; - } + _restore_internal_bridge_refs(self); // This only dissolves this region, all sub-regions remain closed. gc_list_dissolve(&self->gc_list); @@ -1331,6 +1338,16 @@ static int _try_close_region(PyObject *region_obj, tree_trace_state_t *tree_trac assert(Region_Check(region_obj)); TracingRegionObject* region = (TracingRegionObject*)region_obj; + // Finalized regions can't be closed since they're deletion would not call the + // finalizer and therefore leak the owned nodes. + if (_PyGC_FINALIZED(region_obj)) { + PyErr_Format( + PyExc_RuntimeError, + "the region %p has been finalized and cannot be closed again", + (void *)region_obj); + return TRACE_RES_ERR; + } + // Init trace state. region_trace_state_t state; if (region_trace_state_init(&state, _PyObject_CAST(region), ®ion->gc_list, tree_trace_state)) { @@ -1517,6 +1534,57 @@ TracingRegion_init(TracingRegionObject *self, PyObject *args, PyObject *kwargs) return 0; } +/* Disposes of everything a closed region owns. + * + * Closing a region establishes that no object inside it has incoming references + * from the outside; only the bridge object may have those. So once the bridge + * object dies, every member of the region is garbage too, however the references + * between them happen to be arranged. + * + * That lets the region clean up after itself instead of handing the objects back + * to the GC. + * + * This can resurrect the bridge object, so it has to run as a finalizer. + */ +static void _region_delete_contents(TracingRegionObject *self) { + assert(!self->open); + + dbg("Deleting the contents of region %p", self); + + PyGC_Head members; + PyGC_Head survivors; + gc_list_init(&members); + gc_list_init(&survivors); + + // Steal the members and open the region first. A finalizer reaching the + // bridge calls `_open_region()`, which would otherwise dissolve the very + // list being disposed of here. + gc_list_merge(&self->gc_list, &members); + assert(gc_list_is_empty(&self->gc_list)); + // Has to happen before anything is released, the members still hold these. + _restore_internal_bridge_refs(self); + self->open = true; + + // The disposal needs a clean error state; a dealloc can happen mid-raise. + PyObject *exc = PyErr_GetRaisedException(); + + // Cleaning the dict should deallocate most things. + Py_CLEAR(self->dict); + + // Deallocate remaining cyclic garbage + _PyGC_FinalizeGarbage(&members); + _PyGC_DeleteGarbage(&members, &survivors); + PyErr_SetRaisedException(exc); + + // Anything a finalizer kept alive is not owned by the region any more. + if (!gc_list_is_empty(&survivors)) { + gc_list_dissolve(&survivors); + } + // Nothing may still point at these stack allocated list heads. + assert(gc_list_is_empty(&members)); + assert(gc_list_is_empty(&survivors)); +} + static int TracingRegion_traverse(TracingRegionObject *self, visitproc visit, void *arg) { Py_VISIT(self->dict); @@ -1525,8 +1593,6 @@ TracingRegion_traverse(TracingRegionObject *self, visitproc visit, void *arg) { static int TracingRegion_clear(TracingRegionObject *self) { - // FIXME(regions): Special branch when closed to dealloc all - _open_region(self); Py_CLEAR(self->dict); return 0; @@ -1534,11 +1600,19 @@ TracingRegion_clear(TracingRegionObject *self) { static void TracingRegion_finalize(PyObject *op) { - // Reopening the region restores the references that the objects inside it - // hold to the bridge object, which can resurrect `op`. This has to run as a - // finalizer so that `PyObject_CallFinalizerFromDealloc()` notices the - // resurrection, instead of freeing an object that is still referenced. - (void)TracingRegion_clear((TracingRegionObject *)op); + TracingRegionObject *self = (TracingRegionObject *)op; + + if (self->open) { + assert(gc_list_is_empty(&self->gc_list)); + // An open region does not own its members. They live in the GC + // generations and the usual reference counting disposes of them. + Py_CLEAR(self->dict); + } else { + // Objects in a closed region have no incoming references besides the + // one from the bridge. We can therefore delete all objects directly + // instead of returning them to the GC. + _region_delete_contents(self); + } } static void @@ -1557,6 +1631,9 @@ TracingRegion_dealloc(TracingRegionObject *self) { return; } + // Make sure any objects added after/during finalization are freed + Py_CLEAR(self->dict); + PyObject_GC_UnTrack(self); Py_TYPE(self)->tp_free(op); } diff --git a/Python/gc.c b/Python/gc.c index 91f50486cda01ce..67d2a6fcb01262c 100644 --- a/Python/gc.c +++ b/Python/gc.c @@ -968,7 +968,7 @@ handle_weakref_callbacks(PyGC_Head *unreachable, PyGC_Head *old) * Since the callback is never needed and may be unsafe in this * case, wr is simply left in the unreachable set. Note that * clear_weakrefs() will ensure its callback will not trigger - * inside delete_garbage(). + * inside _PyGC_DeleteGarbage(). * * OTOH, if wr isn't part of CT, we should invoke the callback: the * weakref outlived the trash. Note that since wr isn't CT in this @@ -1136,9 +1136,10 @@ handle_legacy_finalizers(PyThreadState *tstate, * Note that this may remove some (or even all) of the objects from the * list, due to refcounts falling to 0. */ -static void -finalize_garbage(PyThreadState *tstate, PyGC_Head *collectable) +void +_PyGC_FinalizeGarbage(PyGC_Head *collectable) { + PyThreadState *tstate = _PyThreadState_GET(); destructor finalize; PyGC_Head seen; @@ -1173,10 +1174,12 @@ finalize_garbage(PyThreadState *tstate, PyGC_Head *collectable) * tricky business as the lists can be changing and we don't know which * objects may be freed. It is possible I screwed something up here. */ -static void -delete_garbage(PyThreadState *tstate, GCState *gcstate, - PyGC_Head *collectable, PyGC_Head *old) +void +_PyGC_DeleteGarbage(PyGC_Head *collectable, PyGC_Head *old) { + PyThreadState *tstate = _PyThreadState_GET(); + GCState *gcstate = &tstate->interp->gc; + assert(!_PyErr_Occurred(tstate)); while (!gc_list_is_empty(collectable)) { @@ -1796,7 +1799,7 @@ gc_collect_region(PyThreadState *tstate, validate_list(&unreachable, collecting_set_unreachable_clear); /* Call tp_finalize on objects which have one. */ - finalize_garbage(tstate, &unreachable); + _PyGC_FinalizeGarbage(&unreachable); /* Handle any objects that may have resurrected after the call * to 'finalize_garbage' and continue the collection with the * objects that are still unreachable */ @@ -1814,7 +1817,7 @@ gc_collect_region(PyThreadState *tstate, * in finalizers to be freed. */ stats->collected += gc_list_size(&final_unreachable); - delete_garbage(tstate, gcstate, &final_unreachable, to); + _PyGC_DeleteGarbage(&final_unreachable, to); /* Collect statistics on uncollectable objects found and print * debugging information. */ From d5a5b6b6ed8d5e66a0d8170a525191fbcff1ba4c Mon Sep 17 00:00:00 2001 From: xFrednet Date: Sat, 22 Aug 2026 23:58:50 +0200 Subject: [PATCH 24/24] TRegions: Delection tests --- Lib/test/test_freeze/test_tracing_region.py | 116 +++++++++++++++++++- Objects/cownobject.c | 2 + Objects/tracingregionobject.c | 9 +- 3 files changed, 123 insertions(+), 4 deletions(-) diff --git a/Lib/test/test_freeze/test_tracing_region.py b/Lib/test/test_freeze/test_tracing_region.py index f661bf0fa8b83cd..b605fad011baee2 100644 --- a/Lib/test/test_freeze/test_tracing_region.py +++ b/Lib/test/test_freeze/test_tracing_region.py @@ -1,9 +1,10 @@ +import gc import re import sys import unittest from immutable import freeze, is_frozen, freezable from immutable import TracingRegion as Region -from immutable import Cown +from immutable import Cown, InterpreterLocal def sort_region_error(msg): """Normalize a 'region could not be closed' message by masking the object @@ -251,3 +252,116 @@ def test_implicit_freeze_int(self): c.acquire() self.assertTrue(is_frozen(c.value.obj)) + +class TestClosedRegionTeardown(unittest.TestCase): + """A closed region disposes of its own contents. + + Closing proves nothing outside the region references its members, so the + death of the bridge object makes all of them garbage. The region finalizes + and clears them itself rather than handing them to the GC. + """ + + def test_cycles_reclaimed_without_the_collector(self): + """Check that cycles in closed regions are reclaimed without the collector""" + + @freezable + class Node: + pass + + def _live_nodes(): + """The number of nodes the collector can see.""" + return sum(1 for o in gc.get_objects() if type(o) is Node) + + gc.disable() + try: + before = _live_nodes() + + # Create a cycle + a = Node() + b = Node() + a.b = b + b.a = a + + # Move the cycle into a cown + c = Cown(Region()) + c.value.cycle = a + del a + del b + mid = _live_nodes() + + # Close the region + c.release() + del c + + leaked = _live_nodes() - before + finally: + gc.enable() + + self.assertEqual(mid, 2, "the cycle wasn't detected while the region is open") + self.assertEqual(leaked, 0, "closed region contents were not reclaimed") + + def test_finalizers_run(self): + local = InterpreterLocal(0) + + @freezable + class Recorder: + def __del__(self, local=local): + local.set(local.get() + 1) + + c = Cown(Region()) + for i in range(5): + setattr(c.value, "r%d" % i, Recorder()) + c.release() + + self.assertEqual(local.get(), 0) + del c + self.assertEqual(local.get(), 5) + + + def test_finalizer_can_modify_the_bridge(self): + local = InterpreterLocal(False) + + @freezable + class Reenter: + def __del__(self, local=local): + # This will open the region and also prove that the finalizer ran + local.set(self.bridge.reenter == self) + self.bridge.__dict__ = {} + + # Create a cycle, and allow Reenter to modify the bridge + c = Cown(Region()) + c.value.reenter = Reenter() + c.value.reenter.bridge = c.value + c.release() + del c + + self.assertTrue(local.get(), "the finalizer did not run or got the wrong object") + + def test_finalizer_revivial(self): + local_bridge = InterpreterLocal(None) + local_medic = InterpreterLocal(None) + + @freezable + class Medic: + def __del__(self, loca_bridge=local_bridge, local_medic=local_medic): + local_bridge.set(self.bridge) + local_medic.set(self) + + c1 = Cown(Region()) + c1.value.reviver = Medic() + c1.value.reviver.bridge = c1.value + c1.release() + del c1 + + # Retrieve the revived bridge + self.assertIsInstance(local_bridge.get(), Region); + c2 = Cown(local_bridge.get()) + local_bridge.set(None) + + with self.assertRaises(RuntimeError) as e: + c2.release() + self.assertTrue(str(e.exception).endswith("has been finalized and cannot be closed again")) + + # Check that the revived medic object is valid + self.assertIn("Medic object at 0x", str(local_medic.get())) + diff --git a/Objects/cownobject.c b/Objects/cownobject.c index ada9239995e35b3..c4dc73843f01e49 100644 --- a/Objects/cownobject.c +++ b/Objects/cownobject.c @@ -406,6 +406,8 @@ static int cown_close_region(_PyCownObject *self) { return -1; } + // TODO(regions): Test that we can't create weak refs to the bridge object. Otherwise, we also need to clear them. + // 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); diff --git a/Objects/tracingregionobject.c b/Objects/tracingregionobject.c index 6cc182ce84ef4ce..61820a6f73abadc 100644 --- a/Objects/tracingregionobject.c +++ b/Objects/tracingregionobject.c @@ -16,7 +16,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, ...) \ @@ -1568,11 +1568,14 @@ static void _region_delete_contents(TracingRegionObject *self) { // The disposal needs a clean error state; a dealloc can happen mid-raise. PyObject *exc = PyErr_GetRaisedException(); + // Finalize everything before anything is released, so that no `__del__` + // observes a member that is already gone. + _PyGC_FinalizeGarbage(&members); + // Cleaning the dict should deallocate most things. Py_CLEAR(self->dict); - + // Deallocate remaining cyclic garbage - _PyGC_FinalizeGarbage(&members); _PyGC_DeleteGarbage(&members, &survivors); PyErr_SetRaisedException(exc);