Phase3: Add write barrier to Cell and some bytecodes

.github/workflows/build.yml

@@ -299,7 +299,7 @@ jobs:
 
   build_ubuntu:
     name: 'Ubuntu'
-    runs-on: ubuntu-20.04
+    runs-on: ubuntu-22.04
     timeout-minutes: 60
     needs: check_source
     if: needs.check_source.outputs.run_tests == 'true'
@@ -366,7 +366,7 @@ jobs:
 
   build_ubuntu_ssltests:
     name: 'Ubuntu SSL tests with OpenSSL'
-    runs-on: ubuntu-20.04
+    runs-on: ubuntu-22.04
     timeout-minutes: 60
     needs: check_source
     if: needs.check_source.outputs.run_tests == 'true'
@@ -420,7 +420,7 @@ jobs:
 
   test_hypothesis:
     name: "Hypothesis tests on Ubuntu"
-    runs-on: ubuntu-20.04
+    runs-on: ubuntu-22.04
     timeout-minutes: 60
     needs: check_source
     if: needs.check_source.outputs.run_tests == 'true' && needs.check_source.outputs.run_hypothesis == 'true'
@@ -529,7 +529,7 @@ jobs:
 
   build_asan:
     name: 'Address sanitizer'
-    runs-on: ubuntu-20.04
+    runs-on: ubuntu-22.04
     timeout-minutes: 60
     needs: check_source
     if: needs.check_source.outputs.run_tests == 'true'

.github/workflows/build_min.yml

@@ -286,7 +286,7 @@ jobs:
 
   build_ubuntu:
     name: 'Ubuntu'
-    runs-on: ubuntu-20.04
+    runs-on: ubuntu-22.04
     timeout-minutes: 60
     needs: check_source
     if: needs.check_source.outputs.run_tests == 'true'
@@ -356,7 +356,7 @@ jobs:
   #
   # build_ubuntu_ssltests:
   #   name: 'Ubuntu SSL tests with OpenSSL'
-  #   runs-on: ubuntu-20.04
+  #   runs-on: ubuntu-22.04
   #   timeout-minutes: 60
   #   needs: check_source
   #   if: needs.check_source.outputs.run_tests == 'true'
@@ -410,7 +410,7 @@ jobs:
 
   test_hypothesis:
     name: "Hypothesis tests on Ubuntu"
-    runs-on: ubuntu-20.04
+    runs-on: ubuntu-22.04
     timeout-minutes: 60
     needs: check_source
     if: needs.check_source.outputs.run_tests == 'true' && needs.check_source.outputs.run_hypothesis == 'true'
@@ -520,7 +520,7 @@ jobs:
 
   build_asan:
     name: 'Address sanitizer'
-    runs-on: ubuntu-20.04
+    runs-on: ubuntu-22.04
     timeout-minutes: 60
     needs: check_source
     if: needs.check_source.outputs.run_tests == 'true'

Include/internal/pycore_regions.h

@@ -58,10 +58,10 @@ PyObject* _Py_ResetInvariant(void);
 
 // Invariant placeholder
 bool _Py_RegionAddReference(PyObject* src, PyObject* new_tgt);
-#define Py_REGIONADDREFERENCE(a, b) _Py_RegionAddReference(_PyObject_CAST(a), b)
+#define Py_REGIONADDREFERENCE(a, b) _Py_RegionAddReference(_PyObject_CAST(a), _PyObject_CAST(b))
 
 void _Py_RegionAddLocalReference(PyObject* new_tgt);
-#define Py_REGIONADDLOCALREFERENCE(b) _Py_RegionAddLocalReference(b)
+#define Py_REGIONADDLOCALREFERENCE(b) _Py_RegionAddLocalReference(_PyObject_CAST(b))
 
 // Helper macros to count the number of arguments
 #define _COUNT_ARGS(_1, _2, _3, _4, _5, _6, _7, _8, _9, _10, _11, _12, _13, _14, _15, _16, N, ...) N
@@ -70,8 +70,11 @@ void _Py_RegionAddLocalReference(PyObject* new_tgt);
 bool _Py_RegionAddReferences(PyObject* src, int new_tgtc, ...);
 #define Py_REGIONADDREFERENCES(a, ...) _Py_RegionAddReferences(_PyObject_CAST(a), COUNT_ARGS(__VA_ARGS__), __VA_ARGS__)
 
+bool _Py_RegionChangeReference(PyObject* src, PyObject* old_tgt, PyObject* new_tgt);
+#define Py_REGIONCHANGEREFERENCE(a, b, c) _Py_RegionChangeReference(_PyObject_CAST(a), _PyObject_CAST(b), _PyObject_CAST(c))
+
 void _Py_RegionRemoveReference(PyObject* src, PyObject* old_tgt);
-#define Py_REGIONREMOVEREFERENCE(a, b) _Py_RegionRemoveReference(_PyObject_CAST(a), b)
+#define Py_REGIONREMOVEREFERENCE(a, b) _Py_RegionRemoveReference(_PyObject_CAST(a), _PyObject_CAST(b))
 
 #ifdef NDEBUG
 #define _Py_VPYDBG(fmt, ...)
@@ -97,8 +100,8 @@ int _PyRegion_is_closed(PyObject* op);
 #ifdef _Py_TYPEOF
 #define Py_CLEAR_OBJECT_FIELD(op, field) \
     do { \
-        _Py_TYPEOF(op)* _tmp_field_ptr = &(field); \
-        _Py_TYPEOF(op) _tmp_old_field = (*_tmp_field_ptr); \
+        _Py_TYPEOF(field)* _tmp_field_ptr = &(field); \
+        _Py_TYPEOF(field) _tmp_old_field = (*_tmp_field_ptr); \
         if (_tmp_old_field != NULL) { \
             *_tmp_field_ptr = _Py_NULL; \
             Py_REGIONREMOVEREFERENCE(op, _tmp_old_field); \

Lib/test/test_veronapy_writebarrier.py

@@ -0,0 +1,238 @@
+import unittest
+import dis
+
+class TestCellWriteBarrier(unittest.TestCase):
+    class A:
+        pass
+
+    # Create a cell by capturing a variable in a closure
+    # The content of the cell is an instance of A which can be added to regions.
+    def make_cell(self):
+        x = self.A()
+        return (lambda: x).__closure__[0]
+
+    def setUp(self):
+        # This freezes A and super and meta types of A namely `type` and `object`
+        makeimmutable(self.A)
+
+    def test_cell_region_propagates(self):
+        c = self.make_cell()
+        r = Region()
+
+        # Move c into the region r
+        r.c = c
+
+        # Make sure region r now owns the cell and its content
+        self.assertTrue(r.owns_object(c))
+        self.assertTrue(r.owns_object(c.cell_contents))
+
+    def test_cell_add_ref(self):
+        c = self.make_cell()
+        r = Region()
+
+        # Move c into the region r
+        r.c = c
+
+        # Make sure region r takes ownership of new references from c
+        self.assertTrue(r.owns_object(c))
+        self.assertTrue(r.owns_object(c.cell_contents))
+
+    def test_cell_remove_ref(self):
+        c = self.make_cell()
+        r1 = Region()
+        child = Region()
+
+        # Move c into the region r1
+        r1.c = c
+        self.assertTrue(r1.owns_object(c))
+
+        # Make `child` a subregion of `r1`
+        c.cell_contents = child
+        self.assertEqual(c.cell_contents, child)
+
+        # Replaceing the value in `c` should unparent the child region
+        # We can test this by reparenting it to `r2` without an exception
+        r2 = Region()
+        c.cell_contents = None
+        r2.child = child
+
+    def test_cell_replace_same_bridge(self):
+        # This test makes sure that reassigning a pointer to a bridge object with
+        # the same value doesn't throw an exception, due the write barrier
+        # believing that there are two owning references to the same bridge object.
+        c = self.make_cell()
+        r = Region()
+        child = Region()
+
+        # Move c into the region r
+        r.c = c
+        self.assertTrue(r.owns_object(c))
+
+        # Make `child` a subregion of `r1`
+        c.cell_contents = child
+        self.assertEqual(c.cell_contents, child)
+
+        # Reassigning the owning pointer to the bridge should be fine
+        # since this writes replaces the previous owning reference
+        c.cell_contents = child
+
+# It's not possible to test the existance of all bytecodes. The dis only
+# shows the unoptimized version of the bytecode. For testing specilized or
+# optimized it's therefore not possible to check for the presence of that
+# bytecode in the `dis`. Instead we can only check for the effects of the
+# bytecodes.
+#
+# During test development it's also possible to verify that the expected
+# bytecode is triggered by setting a breakpoint in it. Note that it's also
+# run during the startup of the runtime and by other tests, so make sure
+# the test in question actually started, when the breakpoint is triggered.
+class TestBytecodeWriteBarrier(unittest.TestCase):
+    # Define a class with __slots__
+    class ClassWithSlots:
+        # Use `__slots__` to get the `STORE_ATTR_SLOT` bytecode
+        __slots__ = ('slot',)
+
+        def __init__(self):
+            self.slot = None
+
+    class ClassWithAttr:
+        def __init__(self):
+            self.attr = None
+
+    def setUp(self):
+        # This freezes A and super and meta types of A namely `type` and `object`
+        makeimmutable(self.ClassWithSlots)
+        makeimmutable(self.ClassWithAttr)
+
+    def test_delete_deref(self):
+        r = Region()
+        r.field = {}
+        x = r.field
+
+        # Make sure the region knows about local reference from x
+        self.assertFalse(r.try_close())
+
+        def del_x():
+            nonlocal x
+            del x # This triggers the DELETE_DEREF opcode
+
+        # Create the function and disassemble to confirm DELETE_DEREF is present
+        bytecode = dis.Bytecode(del_x)
+        self.assertIn("DELETE_DEREF", [instr.opname for instr in bytecode])
+
+        self.assertTrue(r.is_open())
+
+        # Call the function forcing the deletion of x which should
+        # close the region.
+        del_x()
+
+        self.assertFalse(r.is_open())
+
+    # This tests that references stored by `STORE_ATTR_SLOT` are known
+    # by the write barrier
+    def test_store_attr_slot_add_reference(self):
+        def set_value(obj, val):
+            obj.slot = val  # This triggers the STORE_ATTR_SLOT opcode
+
+        # Setup a region and a class with slots
+        r = Region()
+        r.slots = self.ClassWithSlots()
+        self.assertTrue(r.owns_object(r.slots))
+
+        # Run `set_value` multiple times to optimize it
+        set_value(r.slots, None)
+        set_value(r.slots, {})
+
+        # Create a new local object
+        new_object = {}
+        new_object["data"] = {}
+
+        # Store the object in slots
+        set_value(r.slots, new_object)
+
+        # Verify the region ownership
+        self.assertTrue(r.owns_object(new_object))
+        self.assertTrue(r.owns_object(new_object["data"]))
+
+    # This tests that references removed by `STORE_ATTR_SLOT` are known
+    # by the write barrier
+    def test_store_attr_slot_remove_reference(self):
+        def set_value(obj, val):
+            obj.slot = val  # This triggers the STORE_ATTR_SLOT opcode
+
+        # Setup a region and a class with slots
+        r = Region()
+        r.data = {}
+        slots = self.ClassWithSlots()
+
+        # Run `set_value` multiple times to optimize it
+        set_value(slots, None)
+        set_value(slots, {})
+
+        # Create local reference into the region
+        set_value(slots, r.data)
+
+        # Make sure the region knows about the reference from the slot
+        self.assertFalse(r.try_close())
+
+        # Clear the reference from slots
+        set_value(slots, None)
+
+        # Check that the region was closed.
+        self.assertFalse(r.is_open())
+
+    # This tests that references stored by `STORE_ATTR_INSTANCE_VALUE` are known
+    # by the write barrier
+    def test_store_attr_instance_value_add_reference(self):
+        def set_value(obj, val):
+            obj.attr = val  # This triggers the STORE_ATTR_INSTANCE_VALUE opcode
+
+        # Setup a region and a class with attributes
+        r = Region()
+        r.attr = self.ClassWithAttr()
+        self.assertTrue(r.owns_object(r.attr))
+
+        # Run `set_value` multiple times to optimize it
+        set_value(r.attr, None)
+        set_value(r.attr, {})
+
+        # Create a new local object
+        new_object = {}
+        new_object["data"] = {}
+
+        # Store the object in attributes
+        set_value(r.attr, new_object)
+
+        # Verify the region ownership
+        self.assertTrue(r.owns_object(new_object))
+        self.assertTrue(r.owns_object(new_object["data"]))
+
+    # This tests that references removed by `STORE_ATTR_INSTANCE_VALUE` are known
+    # by the write barrier
+    def test_store_attr_instance_value_remove_reference(self):
+        def set_value(obj, val):
+            obj.attr = val  # This triggers the STORE_ATTR_INSTANCE_VALUE opcode
+
+        # Setup a region and a class with attributes
+        r = Region()
+        r.data = {}
+        attr = self.ClassWithAttr()
+
+        # Run `set_value` multiple times to optimize it
+        set_value(attr, None)
+        set_value(attr, {})
+
+        # Create local reference into the region
+        set_value(attr, r.data)
+
+        # Make sure the region knows about the reference from the slot
+        self.assertFalse(r.try_close())
+
+        # Clear the reference from attributes
+        set_value(attr, None)
+
+        # Check that the region was closed.
+        self.assertFalse(r.is_open())
+
+if __name__ == "__main__":
+    unittest.main()

Objects/cellobject.c

@@ -74,6 +74,13 @@ PyCell_Set(PyObject *op, PyObject *value)
     }
 
     PyObject *old_value = PyCell_GET(op);
+
+    // Check that the reference can be created
+    if (!Py_REGIONCHANGEREFERENCE(op, old_value, value)) {
+        // Propagate the error from attempting to add the reference
+        return -1;
+    }
+
     PyCell_SET(op, Py_XNewRef(value));
     Py_XDECREF(old_value);
     return 0;
@@ -133,7 +140,7 @@ cell_clear(PyCellObject *op)
         return -1;
     }
 
-    Py_CLEAR(op->ob_ref);
+    Py_CLEAR_OBJECT_FIELD(op, op->ob_ref);
     return 0;
 }
 
@@ -156,6 +163,14 @@ cell_set_contents(PyCellObject *op, PyObject *obj, void *Py_UNUSED(ignored))
         return -1;
     }
 
+    PyObject *old_value = PyCell_GET(op);
+
+    // Check that the reference can be created
+    if (!Py_REGIONCHANGEREFERENCE(op, old_value, obj)) {
+        // Propagate the error from attempting to add the reference
+        return -1;
+    }
+
     Py_XSETREF(op->ob_ref, Py_XNewRef(obj));
     return 0;
 }
@@ -186,7 +201,7 @@ PyTypeObject PyCell_Type = {
     PyObject_GenericGetAttr,                    /* tp_getattro */
     0,                                          /* tp_setattro */
     0,                                          /* tp_as_buffer */
-    Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_GC,    /* tp_flags */
+    Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_GC | Py_TPFLAGS_REGION_AWARE,    /* tp_flags */
     cell_new_doc,                               /* tp_doc */
     (traverseproc)cell_traverse,                /* tp_traverse */
     (inquiry)cell_clear,                        /* tp_clear */