Tregions: External References for bocpy

Include/internal/pycore_immutability.h

@@ -14,6 +14,10 @@ PyAPI_DATA(PyTypeObject) _PyTracingRegion_Type;
 PyAPI_FUNC(int) _PyTracingRegion_Close(PyObject* region);
 PyAPI_FUNC(int) _PyTracingRegion_IsClosed(PyObject* region);
 PyAPI_FUNC(void) _PyTracingRegion_Open(PyObject* region);
+PyAPI_FUNC(int) _PyTracingRegion_Detach(PyObject* region);
+PyAPI_FUNC(int) _PyTracingRegion_DetachIgnoreRegionRefs(PyObject* region);
+PyAPI_FUNC(int) _PyTracingRegion_Attach(PyObject* region, uint64_t ipid, uint64_t tid);
+PyAPI_FUNC(int) _PyTracingRegion_AttachIgnoreRegionRefs(PyObject* region);
 
 /* Returns the region's metadata node, allocating it if this is the first
  * region reference the current close has found. Borrowed, and only valid while

Include/internal/pycore_regionref.h

@@ -31,8 +31,11 @@ typedef enum {
     /* Terminal. The region is held by `value.cown`, on which the owner is
      * looked up dynamically. */
     _Py_REGION_REF_COWN,
+    /* Terminal, owned by one interpreter, but hasn't been opened.
+       this can be restamped */
+    _Py_REGION_REF_CLOSED_IPID,
     /* Terminal, owned by one interpreter. */
-    _Py_REGION_REF_IPID,
+    _Py_REGION_REF_OPEN_IPID,
 } _PyRegionRefKind;
 
 typedef struct _PyRegionRefMetadata {
@@ -68,6 +71,7 @@ extern void _PyRegionRef_MetaSetCown(_PyRegionRefMetadata *meta, PyObject *cown)
  * be `_PyCown_ReleasedIpid()` to mean nobody owns the region. */
 extern void _PyRegionRef_MetaSetIpid(_PyRegionRefMetadata *meta,
                                      _PyCown_ipid_t ipid);
+extern void _PyRegionRef_MetaSetReleased(_PyRegionRefMetadata *meta);
 extern void _PyRegionRef_MetaRegionOpened(_PyRegionRefMetadata *meta);
 extern void _PyRegionRef_MetaResolveWip(_PyRegionRefMetadata *meta);
 

Lib/test/test_freeze/test_prefreeze.py

@@ -152,5 +152,58 @@ def __pre_freeze__(self):
         self.assertTrue(is_frozen(a))
         self.assertFalse(is_frozen(b))
 
+    def test_nested_freeze_restarts_incomplete_scc(self):
+        class A:
+            pass
+
+        class Restart:
+            def __pre_freeze__(self):
+                freeze(self)
+
+        a = A()
+        a.l = [a, Restart()]
+        l = a.l
+
+        freeze(A)
+        freeze(a)
+
+        self.assertTrue(is_frozen(a))
+        self.assertTrue(is_frozen(l))
+        self.assertTrue(is_frozen(l[1]))
+
+    def test_nested_freeze_restart_clears_non_gc_visited(self):
+        class A:
+            pass
+
+        class Restart:
+            def __pre_freeze__(self):
+                freeze(self)
+
+        a = A()
+        a.leaf = "unique-nongc-string"
+        a.restart = Restart()
+
+        freeze(a)
+
+        self.assertTrue(is_frozen(a))
+        self.assertTrue(is_frozen(a.restart))
+
+    def test_failure_rolls_back_incomplete_scc(self):
+        class A:
+            pass
+
+        bad = {}
+        set_freezable(bad, FREEZABLE_NO)
+        a = A()
+        a.l = [a, bad]
+        l = a.l
+
+        with self.assertRaises(TypeError):
+            freeze(a)
+
+        self.assertFalse(is_frozen(a))
+        self.assertFalse(is_frozen(l))
+        self.assertFalse(is_frozen(bad))
+
 if __name__ == "__main__":
     unittest.main()

Lib/test/test_freeze/test_tracing_region.py

@@ -209,7 +209,7 @@ def test_bridge_refs_keep_region_closed(self):
 
         self.assertEqual(
             str(cm.exception),
-            "the cown couldn't be released, due to the bridge having incoming references")
+            "the region couldn't be detached, due to incoming references to the bridge")
 
         # The release should succeed once all refs have been killed
         del r1

Objects/cownobject.c

@@ -107,6 +107,8 @@
     // The region is moving out of the cown, so its region references answer to
     // the cown's owner from now on.
     if (self->value != value && Region_Check(self->value)) {
+        // FIXME(regions): If the cown is released this sets the released owner,
+        // not what we want
         _PyTracingRegion_SetMetaOwner(self->value, cown_get_owner(self));
     }
 
@@ -213,8 +215,9 @@
         has_gil ? _PyCown_ThisThreadId() : UNSET_THREAD_ID);
 
     if (self->value && Region_Check(self->value)) {
-        assert(!PyObject_GC_IsTracked(self->value));
-        PyObject_GC_Track(self->value);
+       if (_PyTracingRegion_AttachIgnoreRegionRefs(self->value)) {
+            return COWN_ACQUIRE_ERROR;
+       }
     }
 
     return COWN_ACQUIRE_SUCCESS;
@@ -307,6 +310,13 @@
 /* Tears the cown down. Only the interpreter owning the cown may run this, see
  * `cown_handoff_dealloc`. */
 static void cown_dealloc_owned(_PyCownObject *self) {
+    if (_PyCown_Owner(_PyObject_CAST(self)) == RELEASED_IPID) {
+        _PyCown_ipid_t this_ip = _PyCown_ThisInterpreterId();
+        // This should never fail, since we have the last remaining instance
+        int res = cown_lock(self, -1, this_ip, true);
+        assert(res >= 0);
+    }
+
     // Clearing hands the region off, so no region reference points here any more.
     PyCown_clear(self);
     PyObject_GC_Del(self);
@@ -490,34 +500,6 @@
     return 0;
 }
 
-/* This attempts to close the region
- *
- * It returns non-zero if the closing failed
- */
-static int cown_close_region(_PyCownObject *self) {
-    assert(Region_Check(self->value));
-
-    // Close the region
-    int closing_res = _PyTracingRegion_Close(self->value);
-    if (closing_res < 0) {
-        return -1;
-    }
-
-    // Make sure that the cown owns the only external reference to the bridge object.
-    if (Py_REFCNT(self->value) > 1) {
-        PyErr_Format(
-            PyExc_RuntimeError,
-            "the cown couldn't be released, due to the bridge having incoming references");
-        return -1;
-    }
-
-    // The region is closed and this is the only owner of the bridge. We untrack
-    // from the current GC list.
-    PyObject_GC_UnTrack(self->value);
-
-    return 0;
-}
-
 static int cown_release(_PyCownObject *self, _PyCown_ipid_t unlocking_ip) {
     if (cown_check_owner_before_release(self, unlocking_ip) < 0) {
         return -1;
@@ -530,7 +512,7 @@
     assert(Region_Check(self->value));
 
     // The contained region needs to be closed, to allow the cown to release
-    if (cown_close_region(self)) {
+    if (_PyTracingRegion_DetachIgnoreRegionRefs(self->value)) {
         return -1;
     }
 

Objects/tracingregionobject.c

@@ -17,7 +17,7 @@
  * graph to. The graph is not written when the variable is unset or empty. */
 #define REGION_GRAPH_ENV_VAR "PYTHON_REGION_GRAPH"
 
-#define REGION_TRACING
+// #define REGION_TRACING
 
 #ifdef REGION_TRACING
 #define dbg(msg, ...) \
@@ -215,6 +215,8 @@ static movable_status get_movable_status(PyObject *obj) {
 
     // Cowns are not movable, but the reference is explicitly allowed.
     if (Cown_Check(obj)) {
+        // Cowns are frozen on creation, so we just accept the reference.
+        assert(_Py_IsImmutable(obj));
         return Py_MOVABLE_COWN;
     }
 
@@ -1773,7 +1775,12 @@ static void _region_delete_contents(TracingRegionObject *self) {
 
 static int
 TracingRegion_traverse(TracingRegionObject *self, visitproc visit, void *arg) {
-    Py_VISIT(self->dict);
+    // If the region is closed, we know that everything inside the region is reachable.
+    // There is no advantage of opening the region to double check. This would also
+    // mess with the GC list of this region.
+    if (self->open) {
+        Py_VISIT(self->dict);
+    }
     return 0;
 }
 
@@ -1971,3 +1978,71 @@ PyTypeObject _PyTracingRegion_Type = {
     .tp_finalize = TracingRegion_finalize,
     .tp_reachable = _PyObject_ReachableVisitTypeAndTraverse,
 };
+
+/// This attempts to detach the region from the current interpreter and thread.
+///
+/// Raises an exception and returns -1 if it couldn't be detached.
+int _PyTracingRegion_DetachIgnoreRegionRefs(PyObject* region) {
+    assert(Region_Check(region));
+
+    // Close the region
+    int closing_res = _PyTracingRegion_Close(region);
+    if (closing_res < 0) {
+        return -1;
+    }
+
+    // Make sure that the cown owns the only external reference to the bridge object.
+    if (Py_REFCNT(region) > 1) {
+        PyErr_Format(
+            PyExc_RuntimeError,
+            "the region couldn't be detached, due to incoming references to the bridge");
+        return -1;
+    }
+
+    // The region is closed and this is the only owner of the bridge. We untrack
+    // from the current GC list.
+    PyObject_GC_UnTrack(region);
+
+    return 0;
+}
+
+/// This attempts to detach the region from the current interpreter and thread.
+///
+/// Raises an exception and returns -1 if it couldn't be detached.
+int _PyTracingRegion_Detach(PyObject* region) {
+    TracingRegionObject *self = (TracingRegionObject*)region;
+
+    if (_PyTracingRegion_DetachIgnoreRegionRefs(region)) {
+        return -1;
+    }
+
+    // This is safe, assuming the region references respect the thread ID,
+    // as that one prevents other threads and IPs from opening the chain under foot.
+    if (self->meta != NULL) {
+        _PyRegionRef_MetaSetReleased(self->meta);
+    }
+
+    return 0;
+}
+
+int _PyTracingRegion_AttachIgnoreRegionRefs(PyObject* region) {
+    assert(Region_Check(region));
+    assert(!PyObject_GC_IsTracked(region));
+    PyObject_GC_Track(region);
+    return 0;
+}
+
+int _PyTracingRegion_Attach(PyObject* region, uint64_t ipid, uint64_t tid) {
+    TracingRegionObject *self = (TracingRegionObject*)region;
+
+    if (_PyTracingRegion_AttachIgnoreRegionRefs(region)) {
+        return -1;
+    }
+
+    if (self->meta != NULL) {
+        _PyRegionRef_MetaSetIpid(self->meta, ipid);
+    }
+    (void)tid;
+
+    return 0;
+}

Objects/weakrefobject.c

@@ -189,7 +189,7 @@ meta_decref_lock_held(_PyRegionRefMetadata *meta)
 static _PyRegionRefMetadata *
 meta_new_local_lock_held(void)
 {
-    _PyRegionRefMetadata *meta = meta_new_lock_held(_Py_REGION_REF_IPID);
+    _PyRegionRefMetadata *meta = meta_new_lock_held(_Py_REGION_REF_OPEN_IPID);
     if (meta != NULL) {
         meta->value.ipid = _PyCown_ThisInterpreterId();
     }
@@ -230,10 +230,18 @@ meta_set_cown_lock_held(_PyRegionRefMetadata *meta, PyObject *cown)
 }
 
 static void
-meta_set_ipid_lock_held(_PyRegionRefMetadata *meta, _PyCown_ipid_t ipid)
+meta_set_open_ipid_lock_held(_PyRegionRefMetadata *meta, _PyCown_ipid_t ipid)
 {
     meta_clear_parent_lock_held(meta);
-    meta->kind = _Py_REGION_REF_IPID;
+    meta->kind = _Py_REGION_REF_OPEN_IPID;
+    meta->value.ipid = ipid;
+}
+
+static void
+meta_set_closed_ipid_lock_held(_PyRegionRefMetadata *meta, _PyCown_ipid_t ipid)
+{
+    meta_clear_parent_lock_held(meta);
+    meta->kind = _Py_REGION_REF_CLOSED_IPID;
     meta->value.ipid = ipid;
 }
 
@@ -295,20 +303,28 @@ _PyRegionRef_MetaSetCown(_PyRegionRefMetadata *meta, PyObject *cown)
 void
 _PyRegionRef_MetaSetIpid(_PyRegionRefMetadata *meta, _PyCown_ipid_t ipid)
 {
-    // FIXME(regions): `ipid` should always be the current interpreter. It isn't
-    // for a released cown, or when `PyCown_clear` runs on an interpreter that
-    // doesn't own the cown; once that is refactored this can assert it.
     LOCK_REGION_REF_META();
-    meta_set_ipid_lock_held(meta, ipid);
+    assert(ipid == _PyCown_ThisInterpreterId());
+    meta_set_closed_ipid_lock_held(meta, ipid);
+    UNLOCK_REGION_REF_META();
+}
+
+void
+_PyRegionRef_MetaSetReleased(_PyRegionRefMetadata *meta)
+{
+    LOCK_REGION_REF_META();
+    meta_set_closed_ipid_lock_held(meta, _PyCown_ReleasedIpid());
     UNLOCK_REGION_REF_META();
 }
 
 void
 _PyRegionRef_MetaRegionOpened(_PyRegionRefMetadata *meta)
 {
     LOCK_REGION_REF_META();
+    // FIXME(regions): The following assert fails since some metas have a parent meta IDK why
+    // assert(meta->kind == _Py_REGION_REF_CLOSED_IPID || meta->kind == _Py_REGION_REF_COWN);
     meta->region = NULL;
-    meta_set_ipid_lock_held(meta, _PyCown_ThisInterpreterId());
+    meta_set_open_ipid_lock_held(meta, _PyCown_ThisInterpreterId());
     UNLOCK_REGION_REF_META();
 }
 
@@ -317,7 +333,7 @@ _PyRegionRef_MetaResolveWip(_PyRegionRefMetadata *meta)
 {
     LOCK_REGION_REF_META();
     if (meta->kind == _Py_REGION_REF_WIP) {
-        meta_set_ipid_lock_held(meta, _PyCown_ThisInterpreterId());
+        meta_set_closed_ipid_lock_held(meta, _PyCown_ThisInterpreterId());
     }
     UNLOCK_REGION_REF_META();
 }
@@ -457,7 +473,21 @@ regionref_check_access(PyWeakReference *self, regionref_open_list_t *regions,
         case _Py_REGION_REF_WIP:
             verdict = REGIONREF_DENIED_WIP;
             break;
-        case _Py_REGION_REF_IPID:
+        case _Py_REGION_REF_CLOSED_IPID:
+            owner = meta->value.ipid;
+            if (owner != this_ip) {
+                verdict = REGIONREF_DENIED_COWN;
+            }
+            else {
+                // FIXME(regions): For this to work, we also need to track the TID
+                // inside meta. This can also be used for `_Py_REGION_REF_OPEN_IPID`
+                //
+                // locking_thread = _PyCown_LockingThread(meta->value.cown);
+                // wrong_thread = locking_thread != _PyCown_UnsetThreadId()
+                //                && locking_thread != _PyCown_ThisThreadId();
+            }
+            break;
+        case _Py_REGION_REF_OPEN_IPID:
             owner = meta->value.ipid;
             if (owner != this_ip) {
                 verdict = REGIONREF_DENIED_IPID;