diff --git a/Include/internal/pycore_cown.h b/Include/internal/pycore_cown.h index 48f133bc38f75b..d294d452a80329 100644 --- a/Include/internal/pycore_cown.h +++ b/Include/internal/pycore_cown.h @@ -26,7 +26,9 @@ typedef uint64_t _PyCown_thread_id_t; PyAPI_FUNC(_PyCown_ipid_t) _PyCown_ThisInterpreterId(void); PyAPI_FUNC(_PyCown_thread_id_t) _PyCown_ThisThreadId(void); PyAPI_FUNC(int) _PyCown_RegionOpen(_PyCownObject *self, _PyRegionObject* region, _PyCown_ipid_t ip); -PyAPI_FUNC(int) _PyCown_RegionClose(_PyCownObject *self, _PyRegionObject* region, _PyCown_ipid_t ip); +PyAPI_FUNC(int) _PyCown_AcquireGC(_PyCownObject *self, Py_region_t *region); +PyAPI_FUNC(int) _PyCown_ReleaseGC(_PyCownObject *self); + #ifdef __cplusplus } diff --git a/Include/internal/pycore_ownership.h b/Include/internal/pycore_ownership.h index bc7ef0cc1349ea..6998961d738d85 100644 --- a/Include/internal/pycore_ownership.h +++ b/Include/internal/pycore_ownership.h @@ -44,6 +44,13 @@ typedef struct _Py_ownership_state { // unfreezable fields or thread local wrappers. PyObject *module_locks; PyObject *blocking_on; +#ifdef Py_DEBUG + /* The name of the last type that marked all open regions as dirty. + * + * This is only intended for debugging + */ + PyObject* last_dirty_reason; +#endif #ifdef Py_OWNERSHIP_INVARIANT /* Tracks the state of the ownership invariant. Some ownership-related * operations may temporarily violate the invariant. To handle this safely, @@ -79,7 +86,7 @@ typedef struct _Py_ownership_state { PyAPI_FUNC(Py_ssize_t) _PyOwnership_get_current_tick(void); /* Returns the tick which should be used for `region.open_tick` or 0 if the -* ownerstate is currently unavialble. +* ownerstate is currently unavailble. */ PyAPI_FUNC(Py_ssize_t) _PyOwnership_get_open_region_tick(void); @@ -88,8 +95,8 @@ PyAPI_FUNC(Py_ssize_t) _PyOwnership_get_open_region_tick(void); * * It can fail, if the ownership state is currently unavailable */ -PyAPI_FUNC(int) _PyOwnership_notify_untrusted_code(void); - +PyAPI_FUNC(int) _PyOwnership_notify_untrusted_code(const char* reason); +PyAPI_FUNC(PyObject*) _PyOwnership_get_last_dirty_region(void); PyAPI_FUNC(int) _PyOwnership_is_c_wrapper(PyObject *obj); diff --git a/Include/internal/pycore_region.h b/Include/internal/pycore_region.h index 6ae2cb4daeec45..cbe4913e535005 100644 --- a/Include/internal/pycore_region.h +++ b/Include/internal/pycore_region.h @@ -144,6 +144,7 @@ PyAPI_FUNC(int) _PyRegion_ClosesWithLrc(Py_region_t region, Py_ssize_t lrc); PyAPI_FUNC(Py_region_t) _PyRegion_GetParent(Py_region_t child); PyAPI_FUNC(int) _PyRegion_Clean(Py_region_t region); PyAPI_FUNC(void) _PyRegion_MakeDirty(Py_region_t region); +PyAPI_FUNC(PyObject*) _PyRegion_GetSubregions(Py_region_t region); PyAPI_FUNC(int) _PyRegion_IsBridge(PyObject *obj); PyAPI_FUNC(PyObject*) _PyRegion_GetBridge(Py_region_t region); diff --git a/Include/internal/pycore_stackref.h b/Include/internal/pycore_stackref.h index 7f800176db22c0..4671eb83bb5ba0 100644 --- a/Include/internal/pycore_stackref.h +++ b/Include/internal/pycore_stackref.h @@ -687,6 +687,11 @@ PyStackRef_MakeHeapSafe(_PyStackRef ref) return ref; } PyObject *obj = BITS_TO_PTR_MASKED(ref); + + // This should always succeed, since we already have a reference on the stack + int res = PyRegion_AddLocalRef(obj); + assert(res == 0); + (void)res; Py_INCREF(obj); ref.bits = (uintptr_t)obj; PyStackRef_CheckValid(ref); diff --git a/Include/region.h b/Include/region.h index 0ce3d48fba5dfe..19126937f38bb0 100644 --- a/Include/region.h +++ b/Include/region.h @@ -31,7 +31,7 @@ PyAPI_FUNC(int) _PyRegion_AddRefs(PyObject *src, int tgt_count, ...); #define PyRegion_AddRef(src, tgt) _PyRegion_AddRef(_PyObject_CAST(src), _PyObject_CAST(tgt)) #define PyRegion_AddRefs(src, ...) _PyRegion_AddRefs(_PyObject_CAST(src), _PyRegion_COUNT_ARGS(__VA_ARGS__), __VA_ARGS__) -PyAPI_FUNC(int) _PyRegion_RemoveRef(PyObject *src, PyObject *tgt); +PyAPI_FUNC(void) _PyRegion_RemoveRef(PyObject *src, PyObject *tgt); #define PyRegion_RemoveRef(src, tgt) _PyRegion_RemoveRef(_PyObject_CAST(src), _PyObject_CAST(tgt)) PyAPI_FUNC(int) _PyRegion_AddLocalRef(PyObject *tgt); @@ -39,7 +39,7 @@ PyAPI_FUNC(int) _PyRegion_AddLocalRefs(int tgt_count, ...); #define PyRegion_AddLocalRef(tgt) _PyRegion_AddLocalRef(_PyObject_CAST(tgt)) #define PyRegion_AddLocalRefs(...) _PyRegion_AddLocalRefs(_PyRegion_COUNT_ARGS(__VA_ARGS__), __VA_ARGS__) -PyAPI_FUNC(int) _PyRegion_RemoveLocalRef(PyObject *tgt); +PyAPI_FUNC(void) _PyRegion_RemoveLocalRef(PyObject *tgt); #define PyRegion_RemoveLocalRef(tgt) _PyRegion_RemoveLocalRef(_PyObject_CAST(tgt)) static inline PyObject* _PyRegion_NewRef(PyObject* tgt) { @@ -60,7 +60,7 @@ static inline PyObject* _PyRegion_XNewRef(PyObject* tgt) { static inline int _PyRegion_TakeRef(PyObject *src, PyObject *tgt) { int res = _PyRegion_AddRef(src, tgt); - if (res) { + if (res != 0) { return res; } @@ -79,9 +79,12 @@ static inline int _PyRegion_TakeRef(PyObject *src, PyObject *tgt) { // modify the OSC of X but not close X. This ensures that no cown is // released or send off, while we still have remaining references into // X and Y. - return _PyRegion_RemoveLocalRef(tgt); + _PyRegion_RemoveLocalRef(tgt); + return 0; } +PyAPI_FUNC(int) _PyRegion_TakeRefs(PyObject *src, int tgt_count, ...); #define PyRegion_TakeRef(src, tgt) _PyRegion_TakeRef(_PyObject_CAST(src), _PyObject_CAST(tgt)) +#define PyRegion_TakeRefs(src, ...) _PyRegion_TakeRefs(_PyObject_CAST(src), _PyRegion_COUNT_ARGS(__VA_ARGS__), __VA_ARGS__) static inline int _PyRegion_XSetRef(PyObject *src, PyObject **field, PyObject *val) { PyObject *old = *field; @@ -118,6 +121,7 @@ static inline void _PyRegion_Clear(PyObject *src, PyObject **field) { } #define PyRegion_CLEAR(src, dst) _PyRegion_Clear(_PyObject_CAST(src), (PyObject **)&(dst)) +PyAPI_FUNC(void) PyRegion_NotifyTypeUse(PyTypeObject* type); #ifdef __cplusplus } diff --git a/Lib/test/test_regions/test_clean.py b/Lib/test/test_regions/test_clean.py index 05969693787c62..7c5dfaae6f368e 100644 --- a/Lib/test/test_regions/test_clean.py +++ b/Lib/test/test_regions/test_clean.py @@ -1,7 +1,5 @@ import unittest from regions import Region, is_local -import sys - class TestCleanRegion(unittest.TestCase): def mark_region_as_dirty(self, region: Region): @@ -9,15 +7,36 @@ def mark_region_as_dirty(self, region: Region): self.assertTrue(region.is_dirty, "Region should be dirty here") - def test_try_close_dirty_with_local_ref(self): + def test_clean_marks_region_as_clean(self): region = Region() self.mark_region_as_dirty(region) - region.clean() + cleaned = region.clean() self.assertFalse(region.is_dirty) + self.assertEqual(cleaned, 1) + + def test_clean_ignores_clean_subregions(self): + region = Region() + self.mark_region_as_dirty(region) + region.sub = Region() + detached_object = {} + region.sub.obj = detached_object + region.sub.obj = None - def test_try_close_sub_region(self): + # Precondition + self.assertFalse(region.sub.is_dirty, "The subregion should be clean") + self.assertTrue(region.sub.owns(detached_object)) + + # Action - Only dirty regions should be effected by this clean call + cleaned = region.clean() + + # Postcondition + self.assertEqual(cleaned, 1) + self.assertFalse(region.is_dirty, "The parent region should be cleaned") + self.assertTrue(region.sub.owns(detached_object), "The subregion should remain uncleaned") + + def test_cleaning_also_cleans_dirty_subregion(self): region = Region() region.sub = Region() self.mark_region_as_dirty(region) @@ -26,11 +45,12 @@ def test_try_close_sub_region(self): sub = region.sub # Cleaning a dirty parent region should clean the child as well - region.clean() + cleaned = region.clean() - # The regions should now be clean + # The regions should now be clean and the LRC should be correct + self.assertEqual(cleaned, 2) self.assertFalse(sub.is_dirty) - self.assertEqual(sub._lrc, 1, "The local should be the only known reference") + self.assertEqual(sub._lrc, 1, "The sub region should only have an LRC of 1") self.assertEqual(sub._osc, 0, "No subregions should be present") self.assertEqual(region._osc, 1) @@ -38,6 +58,27 @@ def test_try_close_sub_region(self): sub = None self.assertEqual(region._osc, 0) + def test_cleaning_finds_dirty_subregion(self): + region = Region() + region.sub = Region() + region.sub.sub = Region() + detached_object = {} + region.sub.sub.obj = detached_object + region.sub.sub.obj = None + self.mark_region_as_dirty(region.sub.sub) + + # Precondition + self.assertFalse(region.is_dirty, "The region should be clean") + self.assertFalse(region.sub.is_dirty, "The subregion should be clean") + self.assertTrue(region.sub.sub.owns(detached_object)) + + # Action: Clean should find the dirty subsubregion + cleaned = region.clean() + + # Postcondition + self.assertEqual(cleaned, 1) + self.assertTrue(is_local(detached_object)) + self.assertFalse(region.sub.sub.is_dirty, "The subsubregion should be clean") def test_clean_removes_unreachable(self): region = Region() diff --git a/Lib/test/test_regions/test_core.py b/Lib/test/test_regions/test_core.py index a372506635a29a..372fe791a15203 100644 --- a/Lib/test/test_regions/test_core.py +++ b/Lib/test/test_regions/test_core.py @@ -15,15 +15,15 @@ def test_region_construction(self): # The region should be open since r points into it self.assertTrue(r.is_open) - # FIXME(regions): xFrednet: Regions currently default to being dirty - # while most write barriers are missing - # # A new region should be clean - # self.assertFalse(r.is_dirty) + self.assertFalse(r.is_dirty) # A new region has no parent self.assertIsNone(r.parent) + # A new region should have not subregions + self.assertListEqual(r._subregions, []) + def test_fields_read_only(self): r = Region() @@ -37,6 +37,15 @@ def test_fields_read_only(self): with self.assertRaises(AttributeError): r.parent = None + with self.assertRaises(AttributeError): + r._lrc = None + + with self.assertRaises(AttributeError): + r._osc = None + + with self.assertRaises(AttributeError): + r._subregions = None + def test_instance_dict_is_owned(self): r = Region() @@ -202,3 +211,56 @@ def test_get_parent(self): # Check that r2 has no parent self.assertIsNone(r2.parent) + + def test_subregions(self): + r1 = Region() + r2 = Region() + r3 = Region() + r4 = Region() + + # r1 starts with no children + self.assertEqual(len(r1._subregions), 0) + + # This should add r2 as a subregion + r1.r2 = r2 + self.assertEqual(len(r1._subregions), 1) + self.assertIn(r2, r1._subregions) + + # This should add r3 as a subregion + r1.r3 = r3 + self.assertEqual(len(r1._subregions), 2) + self.assertIn(r2, r1._subregions) + self.assertIn(r3, r1._subregions) + + # This should replace r3 as a subregion + r1.r3 = r4 + self.assertEqual(len(r1._subregions), 2) + self.assertIn(r2, r1._subregions) + self.assertIn(r4, r1._subregions) + self.assertNotIn(r3, r1._subregions) + + # The subregions list should be temporary and clear the LRC after + self.assertEqual(r2._lrc, 1) + self.assertEqual(r3._lrc, 1) + self.assertEqual(r4._lrc, 1) + + def test_region_dissolve_bumps_subregion_lrc(self): + r1 = Region() + r2 = Region() + obj = self.A() + + # Make r2 a subregion of 1 + r1.obj = obj + obj.r2 = r2 + + # Precondition + self.assertEqual(r2.parent, r1) + r2_lrc = r2._lrc + + # Dissolve parent region + r1 = None + + # Postcondition + self.assertEqual(r2._lrc, r2_lrc + 1) + self.assertIsNone(r2.parent) + self.assertTrue(is_local(obj)) diff --git a/Lib/test/test_regions/test_dict.py b/Lib/test/test_regions/test_dict.py index b86487fc78a279..a5ffecec038906 100644 --- a/Lib/test/test_regions/test_dict.py +++ b/Lib/test/test_regions/test_dict.py @@ -1,5 +1,4 @@ import unittest -import sys from regions import Region, is_local from immutable import freeze, isfrozen @@ -183,23 +182,26 @@ class SomeObject: # Post-condition self.assertEqual(r._lrc, lrc + 1) - def check_loop_lrc_change(self, region, iter_src, loop_lrc_effect, iter_lrc_cost = 1): + def check_loop_lrc_change(self, region, iter_src, loop_lrc_effect, iter_lrc_cost = 1, check_lrc_reset=True): # Check loop iterations change the LRC lrc = region._lrc i = 0 for v in iter_src: self.assertEqual(region._lrc, lrc + iter_lrc_cost + loop_lrc_effect, f"Fail in iteration: {i} base LRC {lrc} + {iter_lrc_cost} for iter") - # Reassigning v should reset the LRC - v = None - self.assertEqual(region._lrc, lrc + iter_lrc_cost, - f"LRC didn't reset in iteration: {i} base LRC {lrc} + {iter_lrc_cost} for iter") + if check_lrc_reset: + v = None + self.assertEqual(region._lrc, lrc + iter_lrc_cost, + f"LRC didn't reset in iteration: {i} base LRC {lrc} + {iter_lrc_cost} for iter") i += 1 + # Setting v to none should reset the LRC to pre-loop levels + v = None + # Check LRC is back to pre-loop levels self.assertEqual(region._lrc, lrc) - def check_dict_view(self, key1, key2, create_view, loop_lrc_effect, iter_lrc_cost = 1): + def check_dict_view(self, key1, key2, create_view, loop_lrc_effect, iter_lrc_cost = 1, check_lrc_reset=True): class SomeObject: pass freeze(SomeObject()) @@ -213,7 +215,130 @@ class SomeObject: # Create the view view = create_view(r.dict) - self.check_loop_lrc_change(r, view, loop_lrc_effect, iter_lrc_cost) + self.check_loop_lrc_change(r, view, loop_lrc_effect, iter_lrc_cost, check_lrc_reset) + + def check_pop(self, key): + class SomeObject: + pass + freeze(SomeObject()) + + # Setup + r = Region() + r.obj = SomeObject() + d = {} + + # Precondition + d[key] = r.obj + base_lrc = r._lrc + self.assertGreaterEqual(base_lrc, 1) + + # Action + d.pop(key) + self.assertEqual(r._lrc, base_lrc - 1) + + # Check pop(key, default) with a new dictionary + d = {} + base_lrc = r._lrc + local_ref = d.pop(key, r.obj) + self.assertEqual(r._lrc, base_lrc + 1) + local_ref = None + self.assertEqual(r._lrc, base_lrc) + + def check_popitem(self, key1, key2, lrc_for_key): + class Value: + pass + freeze(Value()) + + # Setup + r = Region() + r.obj1 = Value() + r.obj2 = Value() + r.key1 = key1 + r.key2 = key2 + d = {} + + # Pre-condition + d[key1] = r.obj1 + d[key2] = r.obj2 + + # Pop 1. item + base_lrc = r._lrc + local_ref = d.popitem() + self.assertEqual(r._lrc, base_lrc, "The LRC should remain unchanged due to `local_ref`") + local_ref = None + self.assertEqual(r._lrc, base_lrc - lrc_for_key - 1) + + # Pop 2. item + base_lrc = r._lrc + local_ref = d.popitem() + self.assertEqual(r._lrc, base_lrc, "The LRC should remain unchanged due to `local_ref`") + local_ref = None + self.assertEqual(r._lrc, base_lrc - lrc_for_key - 1) + + # Make sure the dict is empty + with self.assertRaises(KeyError): + d.popitem() + + def check_setdefault_new_key(self, key, lrc_for_key): + """ + Checks that the `setdefault()` method of the dictionary correctly adjusts the LRC. + This tests the case case then the key is not present in the set + """ + class SomeObject: + pass + freeze(SomeObject()) + + # Setup + r = Region() + r.obj = SomeObject() + r.key = key + d = {} + base_lrc = r._lrc + + # Pre-condition + self.assertGreater(base_lrc, 0) + + # Action: setdefault with non-existing key + local_ref = d.setdefault(key, r.obj) + + # Post-condition: LRC should increase for the inserted key, value pair + # plus the returned reference + self.assertEqual(r._lrc, base_lrc + 1 + lrc_for_key + 1, + f"LRC should increase when setdefault inserts a new value") + + # Removing the local reference should only remove one LRC + local_ref = None + self.assertEqual(r._lrc, base_lrc + 1 + lrc_for_key) + + def check_setdefault_present_key(self, key, lrc_for_key): + """ + Checks that the `setdefault()` method of the dictionary correctly adjusts the LRC. + This tests the case case then the key is not present in the set + """ + class SomeObject: + pass + freeze(SomeObject()) + + # Setup + r1 = Region() + r1.obj = SomeObject() + r1.key = key + d = {} + d[key] = r1.obj + r1_base_lrc = r1._lrc + + r2 = Region() + r2.unused_default = SomeObject() + r2_base_lrc = r2._lrc + + # Action: setdefault with non-existing key + local_ref = d.setdefault(key, r2.unused_default) + + # Post-condition + self.assertEqual(r2._lrc, r2_base_lrc, "r2.unused_default should be unused") + self.assertEqual(r1._lrc, r1_base_lrc + 1, "the new local ref should be tracked") + local_ref = None + self.assertEqual(r1._lrc, r1_base_lrc, "LRC should be reset") class TestRegionDictUnicodeKeys(BaseTestRegionDictKeys): @unittest.expectedFailure # FIXME(regions): xFrednet: Broken until WBs in zip have been added @@ -266,15 +391,23 @@ def test_wb_get_default(self): def test_wb_copy(self): self.check_dict_item_access("another key", lambda dict, key: dict.copy()) + def test_wb_pop(self): + self.check_pop("Cool Key") + + def test_wb_popitem(self): + self.check_popitem("Cool Key", "Best Key", lrc_for_key=0) + def test_wb_keys_view(self): self.check_dict_view("K1", "K2", lambda d: d.keys(), loop_lrc_effect=0) def test_wb_values_view(self): self.check_dict_view("K1", "K2", lambda d: d.values(), loop_lrc_effect=1) - @unittest.expectedFailure # FIXME(regions): xFrednet: Broken until WBs in tuples have been added def test_wb_items_view(self): - self.check_dict_view("K1", "K2", lambda d: d.items(), loop_lrc_effect=1) + # Python's dictionary iterator caches the tuple used during iteration. + # This is good for performance, but means that the LRC doesn't + # reset if we clear the loop variable. + self.check_dict_view("K1", "K2", lambda d: d.items(), loop_lrc_effect=1, check_lrc_reset=False) def test_wb_iter_dict(self): self.check_dict_view("K1", "K2", lambda d: d, loop_lrc_effect=0) @@ -283,6 +416,10 @@ def test_wb_iter_dict_reversed(self): # The iterator doesn't effect the LRC, since we create it with the `reversed` self.check_dict_view("K1", "K2", lambda d: reversed(d), loop_lrc_effect=0, iter_lrc_cost=0) + def test_wb_setdefault(self): + self.check_setdefault_new_key("setdefault-key", lrc_for_key=0) + self.check_setdefault_present_key("Meow", lrc_for_key=0) + class TestRegionDictObjectKeys(BaseTestRegionDictKeys): class Key: pass @@ -362,6 +499,12 @@ def test_wb_copy(self): # LRC increase of 2: 1x for the key 1x for the item self.check_dict_item_access(self.Key(), lambda dict, key: dict.copy(), lrc_offset = 2) + def test_wb_pop(self): + self.check_pop(self.Key()) + + def test_wb_popitem(self): + self.check_popitem(self.Key(), self.Key(), lrc_for_key=1) + def test_wb_key_list(self): self.check_dict_item_access(self.Key(), lambda dict, key: list(dict)) @@ -371,9 +514,11 @@ def test_wb_keys_view(self): def test_wb_values_view(self): self.check_dict_view(self.Key(), self.Key(), lambda d: d.values(), 1) - @unittest.expectedFailure # FIXME(regions): xFrednet: Broken until WBs in tuples have been added def test_wb_items_view(self): - self.check_dict_view(self.Key(), self.Key(), lambda d: d.items(), 2) + # Python's dictionary iterator caches the tuple used during iteration. + # This is good for performance, but means that the LRC doesn't + # reset if we clear the loop variable. + self.check_dict_view(self.Key(), self.Key(), lambda d: d.items(), loop_lrc_effect=2, check_lrc_reset=False) def test_wb_iter_dict(self): self.check_dict_view(self.Key(), self.Key(), lambda d: d, loop_lrc_effect=1) @@ -382,17 +527,15 @@ def test_wb_iter_reversed_dict(self): # The iterator doesn't effect the LRC, since we create it with the `reversed` self.check_dict_view(self.Key(), self.Key(), lambda d: reversed(d), loop_lrc_effect=1, iter_lrc_cost=0) + def test_wb_setdefault(self): + self.check_setdefault_new_key(self.Key(), lrc_for_key=1) + self.check_setdefault_present_key(self.Key(), lrc_for_key=1) + # FIXME(regions): xFrednet: Set operations on views, like `&`, `^` and `|` # are currently not tested and probably don't work. # TODO: classmethod fromkeys(iterable, value=None, /) -# TODO: dict.pop(key) -# TODO: dict.pop(key, default) -# TODO: dict.popitem(key) -# TODO: reversed(dict) -# TODO: dict.setdefault(key) -# TODO: dict.setdefault(key, default) # TODO: dict.update(???) # TODO: dict1 | dict2 # TODO: dict1 |= dict2 diff --git a/Lib/test/test_regions/test_gc.py b/Lib/test/test_regions/test_gc.py index eca1cbed89e90a..40f57600ed31ad 100644 --- a/Lib/test/test_regions/test_gc.py +++ b/Lib/test/test_regions/test_gc.py @@ -31,4 +31,4 @@ def test_owned_cycles_are_ignored(self): # Dissolving a region should allow cycles to be collected again r = None - self.assertGreaterEqual(gc.collect(), 2) \ No newline at end of file + self.assertGreaterEqual(gc.collect(), 2) diff --git a/Modules/clinic/regionsmodule.c.h b/Modules/clinic/regionsmodule.c.h index bb4a7e6791a9da..9cf1398f60ad43 100644 --- a/Modules/clinic/regionsmodule.c.h +++ b/Modules/clinic/regionsmodule.c.h @@ -29,4 +29,24 @@ regions_is_local(PyObject *module, PyObject *obj) exit: return return_value; } -/*[clinic end generated code: output=4d408aceaaaa05ff input=a9049054013a1b77]*/ + +PyDoc_STRVAR(regions_get_last_dirty_reason__doc__, +"get_last_dirty_reason($module, /)\n" +"--\n" +"\n" +"Returns the last reason for marking open regions as dirty.\n" +"\n" +"Return value: str"); + +#define REGIONS_GET_LAST_DIRTY_REASON_METHODDEF \ + {"get_last_dirty_reason", (PyCFunction)regions_get_last_dirty_reason, METH_NOARGS, regions_get_last_dirty_reason__doc__}, + +static PyObject * +regions_get_last_dirty_reason_impl(PyObject *module); + +static PyObject * +regions_get_last_dirty_reason(PyObject *module, PyObject *Py_UNUSED(ignored)) +{ + return regions_get_last_dirty_reason_impl(module); +} +/*[clinic end generated code: output=42af3f0e45d27e2f input=a9049054013a1b77]*/ diff --git a/Modules/regionsmodule.c b/Modules/regionsmodule.c index ebe27b2a9f3185..f2f32e8ccce379 100644 --- a/Modules/regionsmodule.c +++ b/Modules/regionsmodule.c @@ -104,13 +104,29 @@ Return True the object is in the local region. static int regions_is_local_impl(PyObject *module, PyObject *obj) -/*[clinic end generated code: output=e113b6b045da92b4 input=17b3dedc5693f308]*/ +/*[clinic end generated code: output=e113b6b045da92b4 input=9e5338e938093877]*/ { return PyRegion_IsLocal(obj); } +/*[clinic input] +regions.get_last_dirty_reason + +Returns the last reason for marking open regions as dirty. + +Return value: str +[clinic start generated code]*/ + +static PyObject * +regions_get_last_dirty_reason_impl(PyObject *module) +/*[clinic end generated code: output=7fa56844889d85b8 input=56996052e520f95d]*/ +{ + return _PyOwnership_get_last_dirty_region(); +} + static struct PyMethodDef regions_methods[] = { REGIONS_IS_LOCAL_METHODDEF + REGIONS_GET_LAST_DIRTY_REASON_METHODDEF { NULL, NULL } }; diff --git a/Objects/abstract.c b/Objects/abstract.c index f137b28d71e27a..ca3d013593388e 100644 --- a/Objects/abstract.c +++ b/Objects/abstract.c @@ -1919,9 +1919,7 @@ PySequence_GetItem(PyObject *s, Py_ssize_t i) // Check if the type is Pyrona aware, otherwise, mark all open // regions as dirty - if ((Py_TYPE(s)->tp_flags2 & Py_TPFLAGS2_REGION_AWARE) == 0) { - _PyOwnership_notify_untrusted_code(); - } + PyRegion_NotifyTypeUse(Py_TYPE(s)); PyObject *res = m->sq_item(s, i); assert(_Py_CheckSlotResult(s, "__getitem__", res != NULL)); return res; @@ -2962,9 +2960,7 @@ iternext(PyObject *iter, PyObject **item) // Check if the type is Pyrona aware, otherwise, mark all open // regions as dirty // FIXME(regions): Enable this check, which currently almost always triggers - // if ((Py_TYPE(iter)->tp_flags2 & Py_TPFLAGS2_REGION_AWARE) == 0) { - // _PyOwnership_notify_untrusted_code(); - // } + // PyRegion_NotifyTypeUse(Py_TYPE(iter)); if ((*item = tp_iternext(iter))) { return 1; @@ -3031,11 +3027,7 @@ PyIter_Send(PyObject *iter, PyObject *arg, PyObject **result) return res; } if (arg == Py_None && PyIter_Check(iter)) { - // Check if the type is Pyrona aware, otherwise, mark all open - // regions as dirty - if ((Py_TYPE(iter)->tp_flags2 & Py_TPFLAGS2_REGION_AWARE) == 0) { - _PyOwnership_notify_untrusted_code(); - } + PyRegion_NotifyTypeUse(Py_TYPE(iter)); *result = Py_TYPE(iter)->tp_iternext(iter); } else { diff --git a/Objects/cownobject.c b/Objects/cownobject.c index b60001b804bcb1..c8e11502b01c08 100644 --- a/Objects/cownobject.c +++ b/Objects/cownobject.c @@ -12,8 +12,10 @@ // 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 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, @@ -140,7 +142,10 @@ int cown_set_value(_PyCownObject* self, PyObject* value) { * (0) => Block with no timeout * (n) => Blocking with timeout */ -static int cown_lock(_PyCownObject* self, PyTime_t 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); @@ -174,11 +179,10 @@ static int cown_lock(_PyCownObject* self, PyTime_t timeout) { // Set the owning_ip to the current interpreter, thereby taking ownership _PyCown_ipid_t released_value = RELEASED_IPID; - _PyCown_ipid_t this_ip = _PyCown_ThisInterpreterId(); if (!_Py_atomic_compare_exchange_uint64( &self->owning_ip, &released_value, - this_ip) + locking_ip) ) { // Failed to set owning_ip, this should never happen and points // to a deeper issue. @@ -194,7 +198,11 @@ static int cown_lock(_PyCownObject* self, PyTime_t timeout) { } // Set the locking thread. - self->locking_thread = _PyCown_ThisThreadId(); + if (has_gil) { + self->locking_thread = _PyCown_ThisThreadId(); + } else { + self->locking_thread = UNSET_THREAD_ID; + } return COWN_ACQUIRE_SUCCESS; } @@ -203,7 +211,7 @@ static int cown_lock(_PyCownObject* self, PyTime_t timeout) { * * The caller must hold the GIL. */ -_PyCown_ipid_t _PyCown_ThisInterpreterId(void ) { +_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); @@ -214,7 +222,7 @@ _PyCown_ipid_t _PyCown_ThisInterpreterId(void ) { * * The caller must hold the GIL. */ -_PyCown_thread_id_t _PyCown_ThisThreadId(void ) { +_PyCown_thread_id_t _PyCown_ThisThreadId(void) { _PyCown_thread_id_t id = PyThreadState_GetID(PyThreadState_Get()); return id; } @@ -246,9 +254,9 @@ static int PyCown_init(_PyCownObject *self, PyObject *args, PyObject *kwds) { } // 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) != COWN_ACQUIRE_SUCCESS) { - _PyCown_ipid_t this_ip = _PyCown_ThisInterpreterId(); + 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)", @@ -264,7 +272,7 @@ static int PyCown_init(_PyCownObject *self, PyObject *args, PyObject *kwds) { return -1; } -static int PyCown_traverse(_PyCownObject *self, visitproc, void*) { +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 @@ -343,7 +351,8 @@ CownObject_acquire(_PyCownObject *self, PyObject *args, PyObject *kwds) } // Attempt to lock the cown - int res = cown_lock(self, timeout); + _PyCown_ipid_t this_ip = _PyCown_ThisInterpreterId(); + int res = cown_lock(self, timeout, this_ip, true); if (res == COWN_ACQUIRE_ERROR) { return NULL; } @@ -361,16 +370,15 @@ 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 PyObject* cown_release_unchecked(_PyCownObject* self) { +static int cown_release_unchecked(_PyCownObject* self, _PyCown_ipid_t unlocking_ip) { // Set owning_ip to indicate the released state - _PyCown_ipid_t this_ip = _PyCown_ThisInterpreterId(); - if (!_Py_atomic_compare_exchange_uint64(&self->owning_ip, &this_ip, RELEASED_IPID)) { + 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", - this_ip, self); - return NULL; + unlocking_ip, self); + return -1; } // Unlocking should always succeed @@ -378,57 +386,53 @@ static PyObject* cown_release_unchecked(_PyCownObject* self) { assert(res == 0); (void)res; - Py_RETURN_NONE; + return 0; } -static PyObject* cown_release_region(_PyCownObject* self) { +static int cown_release_region(_PyCownObject* self, _PyCown_ipid_t unlocking_ip) { assert(_PyRegion_IsBridge(self->value)); - // Fetch the region handle - Py_region_t region = _PyRegion_Get(self->value); - - // Dirty regions may close after cleaning - if (_PyRegion_IsDirty(region)) { - SUCCEEDS(_PyRegion_Clean(region)); - - // Update the region handle - region = _PyRegion_Get(self->value); + // If the region is open attempt to close it by cleaning it. + if (!_PyRegion_IsOpen(_PyRegion_Get(self->value))) { + int cleaning_res = _PyRegion_Clean(_PyRegion_Get(self->value)); + if (cleaning_res < 0) { + goto error; + } } // A closed region is safe to release - if (_PyRegion_IsOpen(region) == false) { - return cown_release_unchecked(self); + if (!_PyRegion_IsOpen(_PyRegion_Get(self->value))) { + return cown_release_unchecked(self, unlocking_ip); } PyErr_Format( PyExc_RuntimeError, "the cown can't be released, since the contained region is still open"); error: - return NULL; + return -1; } -static PyObject* CownObject_release(_PyCownObject *self, PyObject *ignored) { +static int cown_release(_PyCownObject *self, _PyCown_ipid_t unlocking_ip) { _PyCown_ipid_t owning_ip = cown_get_owner(self); - _PyCown_ipid_t this_ip = _PyCown_ThisInterpreterId(); // Error if the cown is already released if (owning_ip == RELEASED_IPID) { PyErr_Format( PyExc_RuntimeError, "interpreter %lld attempted to release a released cown", - this_ip + unlocking_ip ); - return NULL; + return -1; } // Error if the cown is owned by a different interpreter - if (owning_ip != this_ip) { + if (owning_ip != unlocking_ip) { PyErr_Format( PyExc_RuntimeError, "interpreter %lld attempted to release a cown owned by %lld", - this_ip, owning_ip + unlocking_ip, owning_ip ); - return NULL; + return -1; } PyObject *value = self->value; @@ -437,11 +441,19 @@ static PyObject* CownObject_release(_PyCownObject *self, PyObject *ignored) { // restrictions Py_region_t value_region = _PyRegion_Get(value); if (value_region == _Py_COWN_REGION || value_region == _Py_IMMUTABLE_REGION) { - return cown_release_unchecked(self); + return cown_release_unchecked(self, unlocking_ip); } assert(value_region != _Py_LOCAL_REGION); - return cown_release_region(self); + return cown_release_region(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, @@ -594,3 +606,36 @@ PyTypeObject _PyCown_Type = { PyType_GenericNew, /* tp_new */ .tp_flags2 = Py_TPFLAGS2_REGION_AWARE }; + +/* This acquires the current cown for the GC. The cown returns a borrowed + * reference to the contained region via the `region` argument. + * + * Possible returns: + * (-1): Indicates a error state. (This should never happen). + * (0): the acquisition failed, probably because a different thread + * acquired the cown first. + * (1): The cown was acquired and the `region` argument was updated. The + * cown needs to be manually released via `_PyCown_ReleaseGC`. + */ +int _PyCown_AcquireGC(_PyCownObject *self, Py_region_t *region) { + // Attempt to lock the cown + int res = cown_lock(self, NO_BLOCKING_TIMEOUT, GC_IPID, false); + if (res == COWN_ACQUIRE_ERROR) { + return -1; + } + + // The cown was snatched up by something else. This is fine for + // the GC + if (res != COWN_ACQUIRE_FAIL) { + return 0; + } + assert(res == COWN_ACQUIRE_SUCCESS); + + // This accesses the value directly, to keep a potential region closed + *region = _PyRegion_Get(self->value); + return 1; +} + +int _PyCown_ReleaseGC(_PyCownObject *self) { + return cown_release(self, GC_IPID); +} diff --git a/Objects/dictobject.c b/Objects/dictobject.c index fceddebfe7be67..63b8d3db5a5d13 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -1827,7 +1827,6 @@ insert_split_key(PyDictKeysObject *keys, PyObject *key, Py_hash_t hash) assert(PyUnicode_CheckExact(key)); Py_ssize_t ix; - #ifdef Py_GIL_DISABLED ix = unicodekeys_lookup_unicode_threadsafe(keys, key, hash); if (ix >= 0) { @@ -1835,6 +1834,10 @@ insert_split_key(PyDictKeysObject *keys, PyObject *key, Py_hash_t hash) } #endif + // Regions: Since the key is a PyUnicode object we know + // that it will be frozen when its added to a region. This + // should always succeed. + LOCK_KEYS(keys); ix = unicodekeys_lookup_unicode(keys, key, hash); if (ix == DKIX_EMPTY && keys->dk_usable > 0) { @@ -1852,11 +1855,14 @@ insert_split_key(PyDictKeysObject *keys, PyObject *key, Py_hash_t hash) return ix; } -static void +static int insert_split_value(PyInterpreterState *interp, PyDictObject *mp, PyObject *key, PyObject *value, Py_ssize_t ix) { assert(PyUnicode_CheckExact(key)); ASSERT_DICT_LOCKED(mp); + if (PyRegion_AddRef(mp, value)) { + return -1; + } PyObject *old_value = mp->ma_values->values[ix]; if (old_value == NULL) { _PyDict_NotifyEvent(interp, PyDict_EVENT_ADDED, mp, key, value); @@ -1873,6 +1879,7 @@ insert_split_value(PyInterpreterState *interp, PyDictObject *mp, PyObject *key, Py_DECREF(old_value); } ASSERT_CONSISTENT(mp); + return 0; } /* @@ -1903,7 +1910,9 @@ insertdict(PyInterpreterState *interp, PyDictObject *mp, if (_PyDict_HasSplitTable(mp)) { Py_ssize_t ix = insert_split_key(mp->ma_keys, key, hash); if (ix != DKIX_EMPTY) { - insert_split_value(interp, mp, key, value, ix); + if (insert_split_value(interp, mp, key, value, ix)) { + goto Fail; + } PyRegion_RemoveLocalRef(key); PyRegion_RemoveLocalRef(value); Py_DECREF(key); @@ -1935,12 +1944,9 @@ insertdict(PyInterpreterState *interp, PyDictObject *mp, } if (old_value != value) { - // FIXME(regions): xFrednet: PyRegion_TakeRefs - if (PyRegion_AddRefs(mp, key, value)) { + if (PyRegion_TakeRefs(mp, key, value)) { goto Fail; } - PyRegion_RemoveLocalRef(key); - PyRegion_RemoveLocalRef(value); _PyDict_NotifyEvent(interp, PyDict_EVENT_MODIFIED, mp, key, value); assert(old_value != NULL); @@ -1992,16 +1998,13 @@ insert_to_emptydict(PyInterpreterState *interp, PyDictObject *mp, return -1; } - // FIXME(regions): xFrednet: PyRegion_TakeRefs - if (PyRegion_AddRefs(mp, key, value) != 0) { + if (PyRegion_TakeRefs(mp, key, value) != 0) { PyRegion_RemoveLocalRef(key); PyRegion_RemoveLocalRef(value); Py_DECREF(key); Py_DECREF(value); return -1; } - PyRegion_RemoveLocalRef(key); - PyRegion_RemoveLocalRef(value); _PyDict_NotifyEvent(interp, PyDict_EVENT_ADDED, mp, key, value); @@ -3216,6 +3219,14 @@ _PyDict_Pop_KnownHash(PyDictObject *mp, PyObject *key, Py_hash_t hash, return 0; } + // This should always succeed, since we have a reference to mp + if (PyRegion_AddLocalRef(old_value)) { + if (result) { + *result = NULL; + } + return -1; + } + assert(old_value != NULL); PyInterpreterState *interp = _PyInterpreterState_GET(); _PyDict_NotifyEvent(interp, PyDict_EVENT_DELETED, mp, key, NULL); @@ -3307,7 +3318,7 @@ dict_pop_default(PyObject *dict, PyObject *key, PyObject *default_value) PyObject *result; if (PyDict_Pop(dict, key, &result) == 0) { if (default_value != NULL) { - return Py_NewRef(default_value); + return PyRegion_NewRef(default_value); } _PyErr_SetKeyError(key); return NULL; @@ -3583,17 +3594,9 @@ dict_repr_lock_held(PyObject *self) goto error; } - if (PyRegion_RemoveLocalRef(key)) { - // Clear the key to prevent a second remove ref call during - // error handling - Py_CLEAR(key); - goto error; - } + PyRegion_RemoveLocalRef(key); + PyRegion_RemoveLocalRef(value); Py_CLEAR(key); - if (PyRegion_RemoveLocalRef(value)) { - Py_CLEAR(value); - goto error; - } Py_CLEAR(value); } @@ -4625,19 +4628,13 @@ dict_setdefault_ref_lock_held(PyObject *d, PyObject *key, PyObject *default_valu if (!PyDict_Check(d)) { PyErr_BadInternalCall(); - if (result) { - *result = NULL; - } - return -1; + goto error; } hash = _PyObject_HashFast(key); if (hash == -1) { dict_unhashable_type(key); - if (result) { - *result = NULL; - } - return -1; + goto error; } if(!Py_CHECKWRITE(d)){ @@ -4646,25 +4643,30 @@ dict_setdefault_ref_lock_held(PyObject *d, PyObject *key, PyObject *default_valu } if (mp->ma_keys == Py_EMPTY_KEYS) { + // Regions: Ensure that we can create these referemces + if (PyRegion_AddRefs(mp, key, default_value)) { + goto error; + } if (insert_to_emptydict(interp, mp, Py_NewRef(key), hash, Py_NewRef(default_value)) < 0) { - if (result) { - *result = NULL; - } - return -1; + goto error; } if (result) { - *result = incref_result ? Py_NewRef(default_value) : default_value; + if (incref_result) { + // This should always succeed, since we have a local refernce to mp + if (PyRegion_AddLocalRef(default_value)) { + goto error; + } + Py_INCREF(default_value); + } + *result = default_value; } return 0; } if (!PyUnicode_CheckExact(key) && DK_IS_UNICODE(mp->ma_keys)) { if (insertion_resize(mp, 0) < 0) { - if (result) { - *result = NULL; - } - return -1; + goto error; } } @@ -4674,11 +4676,20 @@ dict_setdefault_ref_lock_held(PyObject *d, PyObject *key, PyObject *default_valu PyObject *value = mp->ma_values->values[ix]; int already_present = value != NULL; if (!already_present) { - insert_split_value(interp, mp, key, default_value, ix); + if (insert_split_value(interp, mp, key, default_value, ix)) { + goto error; + } value = default_value; } if (result) { - *result = incref_result ? Py_NewRef(value) : value; + if (incref_result) { + // This should always succeed, since we have a local refernce to mp + if (PyRegion_AddLocalRef(value)) { + goto error; + } + Py_INCREF(value); + } + *result = value; } return already_present; } @@ -4693,30 +4704,37 @@ dict_setdefault_ref_lock_held(PyObject *d, PyObject *key, PyObject *default_valu Py_ssize_t ix = _Py_dict_lookup(mp, key, hash, &value); if (ix == DKIX_ERROR) { - if (result) { - *result = NULL; - } - return -1; + goto error; } if (ix == DKIX_EMPTY) { assert(!_PyDict_HasSplitTable(mp)); value = default_value; + // Regions: This should always succeed, since we have local references + if (PyRegion_AddLocalRefs(key, value)) { + goto error; + } if (insert_combined_dict(interp, mp, hash, Py_NewRef(key), Py_NewRef(value)) < 0) { + PyRegion_RemoveLocalRef(key); + PyRegion_RemoveLocalRef(value); Py_DECREF(key); Py_DECREF(value); - if (result) { - *result = NULL; - } - return -1; + goto error; } STORE_USED(mp, mp->ma_used + 1); assert(mp->ma_keys->dk_usable >= 0); ASSERT_CONSISTENT(mp); if (result) { - *result = incref_result ? Py_NewRef(value) : value; + if (incref_result) { + // This should always succeed, since we have a local refernce to mp + if (PyRegion_AddLocalRef(value)) { + goto error; + } + Py_INCREF(value); + } + *result = value; } return 0; } @@ -4724,7 +4742,14 @@ dict_setdefault_ref_lock_held(PyObject *d, PyObject *key, PyObject *default_valu assert(value != NULL); ASSERT_CONSISTENT(mp); if (result) { - *result = incref_result ? Py_NewRef(value) : value; + if (incref_result) { + // This should always succeed, since we have a local refernce to mp + if (PyRegion_AddLocalRef(value)) { + return -1; + } + Py_INCREF(value); + } + *result = value; } return 1; @@ -4913,6 +4938,13 @@ dict_popitem_impl(PyDictObject *self) assert(dictkeys_get_index(self->ma_keys, j) == i); dictkeys_set_index(self->ma_keys, j, DKIX_DUMMY); + // This should always succeed + if (PyRegion_AddRefs(res, key, value)) { + Py_DECREF(res); + return NULL; + } + PyRegion_RemoveRef(self, key); + PyRegion_RemoveRef(self, value); PyTuple_SET_ITEM(res, 0, key); PyTuple_SET_ITEM(res, 1, value); /* We can't dk_usable++ since there is DKIX_DUMMY in indices */ @@ -5974,11 +6006,14 @@ dictiter_iternextitem(PyObject *self) #ifdef Py_GIL_DISABLED if (dictiter_iternext_threadsafe(d, self, &key, &value) == 0) { #else + // Regions: This function returns two new local references, these + // are then moved into a local object. Therefore we don't + // need more write barriers here. if (dictiter_iternextitem_lock_held(d, self, &key, &value) == 0) { #endif PyObject *result = di->di_result; - if (acquire_iter_result(result) && PyRegion_IsLocal(result)) { + if (result && acquire_iter_result(result) && PyRegion_IsLocal(result)) { PyObject *oldkey = PyTuple_GET_ITEM(result, 0); PyObject *oldvalue = PyTuple_GET_ITEM(result, 1); PyTuple_SET_ITEM(result, 0, key); @@ -5992,6 +6027,11 @@ dictiter_iternextitem(PyObject *self) _PyTuple_Recycle(result); } else { + // Something prevented the tuple from being reused. Clear it to + // make sure that this cache doesn't interfear with regions. + if (di->di_result) { + PyRegion_CLEAR(di, di->di_result); + } result = PyTuple_New(2); if (result == NULL) return NULL; @@ -7280,8 +7320,11 @@ store_instance_attr_lock_held(PyObject *obj, PyDictValues *values, // Make the dict but don't publish it in the object // so that no one else will see it. dict = make_dict_from_instance_attributes(keys, values); - if (dict == NULL || - _PyDict_SetItem_LockHeld(dict, name, value) < 0) { + if (dict == NULL + || _PyDict_SetItem_LockHeld(dict, name, value) < 0 + || PyRegion_TakeRef(obj, dict) != 0 + ) { + PyRegion_RemoveLocalRef(dict); Py_XDECREF(dict); return -1; } @@ -7306,6 +7349,10 @@ store_instance_attr_lock_held(PyObject *obj, PyDictValues *values, return -1; } + if (PyRegion_AddRefs(obj, name, value)) { + return -1; + } + if (dict) { PyInterpreterState *interp = _PyInterpreterState_GET(); PyDict_WatchEvent event = (old_value == NULL ? PyDict_EVENT_ADDED : @@ -7331,6 +7378,7 @@ store_instance_attr_lock_held(PyObject *obj, PyDictValues *values, STORE_USED(dict, dict->ma_used - 1); } } + PyRegion_RemoveRef(dict, old_value); Py_DECREF(old_value); } return 0; diff --git a/Objects/listobject.c b/Objects/listobject.c index e2a2e1305c3884..a24457913d6045 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -745,11 +745,20 @@ list_slice_lock_held(PyListObject *a, Py_ssize_t ilow, Py_ssize_t ihigh) src = a->ob_item + ilow; dest = np->ob_item; + // Pyrona: Normally removing a reference does't undo the add reference + // since objects are not moved out of the region. However, in this + // case we know that np is new and therefore local. Since these are + // just LRC updates they can be safely undone + assert(PyRegion_IsLocal(np)); for (i = 0; i < len; i++) { PyObject *v = src[i]; - // FIXME(regions): This doesn't undo the previous loops - if (PyRegion_AddRef(np, v)) + if (PyRegion_AddLocalRef(v)) { + /* Undo previous additions on error */ + for (Py_ssize_t j = 0; j < i; j++) { + PyRegion_RemoveLocalRef(src[j]); + } goto fail; + } dest[i] = Py_NewRef(v); } Py_SET_SIZE(np, len); @@ -804,20 +813,36 @@ list_concat_lock_held(PyListObject *a, PyListObject *b) } src = a->ob_item; dest = np->ob_item; + // Pyrona: Normally removing a reference does't undo the add reference + // since objects are not moved out of the region. However, in this + // case we know that np is new and therefore local. Since these are + // just LRC updates they can be safely undone + assert(PyRegion_IsLocal(np)); for (i = 0; i < Py_SIZE(a); i++) { PyObject *v = src[i]; - // FIXME(regions): This doesn't undo the previous loops - if (PyRegion_AddRef(np, v)) + if (PyRegion_AddLocalRef(v)) { + /* Undo previous additions on error */ + for (Py_ssize_t j = 0; j < i; j++) { + PyRegion_RemoveLocalRef(a->ob_item[j]); + } goto fail; + } dest[i] = Py_NewRef(v); } src = b->ob_item; dest = np->ob_item + Py_SIZE(a); for (i = 0; i < Py_SIZE(b); i++) { PyObject *v = src[i]; - // FIXME(regions): This doesn't undo the previous loops - if (PyRegion_AddRef(np, v)) + if (PyRegion_AddLocalRef(v)) { + /* Undo previous additions on error from both loops */ + for (Py_ssize_t j = 0; j < Py_SIZE(a); j++) { + PyRegion_RemoveLocalRef(a->ob_item[j]); + } + for (Py_ssize_t j = 0; j < i; j++) { + PyRegion_RemoveLocalRef(b->ob_item[j]); + } goto fail; + } dest[i] = Py_NewRef(v); } Py_SET_SIZE(np, size); @@ -4023,7 +4048,7 @@ list_ass_subscript_lock_held(PyObject *_self, PyObject *item, PyObject *value) PyRegion_RemoveLocalRef(seq); Py_DECREF(seq); - return 0; + return res; } } else { @@ -4318,7 +4343,7 @@ PyTypeObject PyListRevIter_Type = { listreviter_next, /* tp_iternext */ listreviter_methods, /* tp_methods */ 0, - .tp_flags2 = Py_TPFLAGS2_REGION_AWARE, + .tp_flags2 = Py_TPFLAGS2_REGION_AWARE, }; /*[clinic input] diff --git a/Objects/object.c b/Objects/object.c index c97e20c033f93a..9018a1a4f3fb39 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -1489,10 +1489,8 @@ PyObject_SetAttr(PyObject *v, PyObject *name, PyObject *value) // Check if the type is Pyrona aware, otherwise, mark all open // regions as dirty - if (tp->tp_setattro != PyObject_GenericSetAttr - && (tp->tp_flags2 & Py_TPFLAGS2_REGION_AWARE) == 0 - ) { - _PyOwnership_notify_untrusted_code(); + if (tp->tp_setattro != PyObject_GenericSetAttr) { + PyRegion_NotifyTypeUse(tp); } // Call the setattro function of the type diff --git a/Objects/regionobject.c b/Objects/regionobject.c index f72a0281d927b3..5124cf074e9b0b 100644 --- a/Objects/regionobject.c +++ b/Objects/regionobject.c @@ -33,6 +33,10 @@ static int Region_init(_PyRegionObject *self, PyObject *args, PyObject *kwds) { return -1; } + // Regions should not be tracked in normal GC, those fields will be + // used to track subregions + PyObject_GC_UnTrack(self); + self->region = NULL_REGION; self->name = NULL; @@ -93,11 +97,12 @@ static PyObject* Region_owns(PyObject *self, PyObject *other) { static PyObject* Region_clean(PyObject *op) { CHECK_BRIDGE(op); - if (_PyRegion_Clean(_PyRegion_Get(op))) { + int cleaning_res = _PyRegion_Clean(_PyRegion_Get(op)); + if (cleaning_res < 0) { return NULL; } - Py_RETURN_NONE; + return PyLong_FromInt32(cleaning_res); } static PyObject* Region__make_dirty(PyObject *op) { @@ -136,13 +141,13 @@ static PyObject* Region_get_parent(PyObject *self, void *closure) { CHECK_BRIDGE(self); Py_region_t parent_region = _PyRegion_GetParent(_PyRegion_Get(self)); - return _Py_NewRef(_PyRegion_GetBridge(parent_region)); + return _PyRegion_NewRef(_PyRegion_GetBridge(parent_region)); } static PyObject* Region_get_name(PyObject *self, void *closure) { CHECK_BRIDGE(self); - return Py_NewRef(_PyRegionObject_CAST(self)->name); + return PyRegion_NewRef(_PyRegionObject_CAST(self)->name); } static PyObject* Region_get__lrc(PyObject* self, void* closure) { @@ -159,6 +164,12 @@ static PyObject* Region_get__osc(PyObject* self, void* closure) { return PyLong_FromSize_t(osc); } +static PyObject* Region_get__subregions(PyObject* self, void* closure) { + CHECK_BRIDGE(self); + + return _PyRegion_GetSubregions(_PyRegion_Get(self)); +} + static PyGetSetDef Region_getset[] = { {"is_open", (getter)Region_is_open, NULL, "indicates if the region is currently open or closed", NULL}, @@ -168,10 +179,12 @@ static PyGetSetDef Region_getset[] = { "the parent of the region", NULL}, {"name", (getter)Region_get_name, NULL, "the name of the region", NULL}, - {"_lrc", (getter)Region_get__lrc, NULL, + {"_lrc", (getter)Region_get__lrc, NULL, "the local-reference count, mainly intended for debugging", NULL}, - {"_osc", (getter)Region_get__osc, NULL, + {"_osc", (getter)Region_get__osc, NULL, "the open-subregion count, mainly intended for debugging", NULL}, + {"_subregions", (getter)Region_get__subregions, NULL, + "returns a list of all subregions, mainly intended for debugging", NULL}, {NULL, NULL, NULL, NULL, NULL} }; @@ -221,7 +234,8 @@ Region_clear(PyObject *op) self->region = NULL_REGION; } - // Clear members + // Clear members. This doesn't need write barriers since both are owned by + // this region Py_CLEAR(self->name); Py_CLEAR(self->dict); return 0; diff --git a/Objects/tupleobject.c b/Objects/tupleobject.c index f52e15864eda8c..b9ceaa8474e5c7 100644 --- a/Objects/tupleobject.c +++ b/Objects/tupleobject.c @@ -139,14 +139,13 @@ PyTuple_SetItem(PyObject *op, Py_ssize_t i, PyObject *newitem) "tuple assignment index out of range"); return -1; } - if (PyRegion_TakeRef(op, newitem)) { + + p = ((PyTupleObject *)op) -> ob_item + i; + if (PyRegion_XSETREF(op, *p, newitem)) { PyRegion_RemoveLocalRef(newitem); Py_XDECREF(newitem); return -1; } - - p = ((PyTupleObject *)op) -> ob_item + i; - Py_XSETREF(*p, newitem); return 0; } diff --git a/Python/ownership.c b/Python/ownership.c index 166e4f78eb1fe3..4f2dec77a4b9d5 100644 --- a/Python/ownership.c +++ b/Python/ownership.c @@ -200,7 +200,7 @@ Py_ssize_t _PyOwnership_get_open_region_tick(void) { return state->tick; } -int _PyOwnership_notify_untrusted_code(void) { +int _PyOwnership_notify_untrusted_code(const char* reason) { _Py_ownership_state* state = get_ownership_state(); if (state == NULL) { return 1; @@ -212,10 +212,32 @@ int _PyOwnership_notify_untrusted_code(void) { } assert(!IS_OPEN_REGION_TICK(state->tick)); +#ifdef Py_DEBUG + PyObject* name = PyUnicode_InternFromString(reason); + if (name != NULL) { + Py_XSETREF(state->last_dirty_reason, name); + } +#endif // Everything is alright return 0; } +PyObject* _PyOwnership_get_last_dirty_region(void) { +#ifdef Py_DEBUG + _Py_ownership_state* state = get_ownership_state(); + if (state == NULL) { + return NULL; + } + + PyObject* reason = PyRegion_XNewRef(state->last_dirty_reason); + if (reason) { + return reason; + } +#endif + + Py_RETURN_NONE; +} + /* This function returns true for C wrappers around functions, types and * all kinds of wrappers around C with immutable state. For ownership these * can be seen as immutable, meaning they can be referenced from immutable @@ -891,7 +913,7 @@ static int check_invariant_visit_owned(PyObject* tgt, _check_invariant_state* st Py_None); return -1; } - + // This is the owning reference to the target region, but target doesn't know about it if (_PyRegion_IsBridge(tgt) && !_PyRegion_IsParent(tgt_region, src_region)) { throw_invariant_error( diff --git a/Python/region.c b/Python/region.c index 2d04a1f6cf218e..6d71ef088480e3 100644 --- a/Python/region.c +++ b/Python/region.c @@ -15,7 +15,7 @@ #include /* 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 SUCCEEDS(x) do { int r = (x); if (r != 0) goto error; } while (0) /* Checks for predefined static regions without data */ #define IS_LOCAL_REGION(r) ((Py_region_t)(r) == _Py_LOCAL_REGION) @@ -56,7 +56,7 @@ // Prototypes static int regiondata_inc_osc(Py_region_t region); -static int regiondata_dec_osc(Py_region_t region); +static void regiondata_dec_osc(Py_region_t region); static int regiondata_is_open(Py_region_t data); static Py_region_t regiondata_get_parent(Py_region_t region); static Py_region_t regiondata_get_parent_follow_pending(Py_region_t region); @@ -76,7 +76,9 @@ static PyObject* list_pop(PyObject* s){ 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; @@ -111,6 +113,21 @@ gc_list_is_empty(PyGC_Head *list) return (list->_gc_next == (uintptr_t)list); } +/* Remove `node` from the gc list it's currently in. */ +static inline void +gc_list_remove(PyGC_Head *node) +{ + PyGC_Head *prev = GC_PREV(node); + PyGC_Head *next = GC_NEXT(node); + + _PyGCHead_SET_NEXT(prev, next); + _PyGCHead_SET_PREV(next, prev); + + // Clear the node pointers + node->_gc_prev = node->_gc_prev & _PyGC_PREV_MASK_FINALIZED; + node->_gc_next = 0; +} + /* Move `node` from the gc list it's currently in (which is not explicitly * named here) to the end of `list`. This is semantically the same as * gc_list_remove(node) followed by gc_list_append(node, list). @@ -161,6 +178,115 @@ get_gc_state(void) return &interp->gc; } #endif // Py_GIL_DISABLED +// ********************************************************************** +// Modified from the GC functions above +// ********************************************************************** + +/* Prepend `node` to `list`. */ +static inline void +gc_list_prepend(PyGC_Head *node, PyGC_Head *list) +{ + assert((list->_gc_prev & ~_PyGC_PREV_MASK) == 0); + PyGC_Head *first = GC_NEXT(list); + + // first <-> node + _PyGCHead_SET_NEXT(node, first); + _PyGCHead_SET_PREV(first, node); + + // node <-> list + _PyGCHead_SET_NEXT(list, node); + _PyGCHead_SET_PREV(node, list); +} + +/* This merges two region lists, this keeps bridge objects of subregions + * at the beginning of the list and other contained objects at the end. + */ +static void +gc_region_list_merge(PyGC_Head *from, PyGC_Head *to) +{ + assert(from != to); + if (gc_list_is_empty(from)) { + return; + } + + // Move sub-regions to the start of the `to` list + PyGC_Head *from_bridges = GC_NEXT(from); + while (from_bridges != from) { + PyObject* item = _Py_FROM_GC(from_bridges); + // Break if this is not a bride + if (Py_TYPE(item) != &_PyRegion_Type) { + break; + } + from_bridges = GC_NEXT(from_bridges); + } + if (from_bridges != GC_NEXT(from)) { + // We have bridges which should be moved: + PyGC_Head *bridges_start = GC_NEXT(from); + PyGC_Head *bridges_end = GC_PREV(from_bridges); + PyGC_Head *to_head = GC_NEXT(to); + + // Remove bridges from the `from` list + _PyGCHead_SET_NEXT(from, from_bridges); + _PyGCHead_SET_PREV(from_bridges, from); + + // Insert bridges into the `to` list + _PyGCHead_SET_NEXT(to, bridges_start); + _PyGCHead_SET_NEXT(bridges_end, to_head); + _PyGCHead_SET_PREV(to_head, bridges_end); + _PyGCHead_SET_PREV(bridges_start, to); + } + + // Move all other contained objects + gc_list_merge(from, to); +} + +typedef int (*gc_list_callback_t)(Py_region_t region, void *data); +/* Calls the given callback with the bridge of each subregion */ +static int gc_list_for_each_subregion(PyGC_Head *list, gc_list_callback_t callback, void* data) { + PyGC_Head *node = GC_NEXT(list); + while (node != list) { + // Grab the next node here, since the callback may modify the list + PyGC_Head *next = GC_NEXT(node); + + // Stop looping if this is not a bridge + PyObject *obj = _Py_FROM_GC(node); + if (Py_TYPE(obj) != &_PyRegion_Type) { + break; + } + + // Call the callback + int res = callback(_PyRegion_Get(obj), data); + if (res != 0) { + return res; + } + + node = next; + } + + return 0; +} + +static int _gc_region_list_dissolve_callback(Py_region_t region, void* _ignore) { + PyObject* obj = _PyRegion_GetBridge(region); + // Bump LRC for the reference which was previously owning this + // region and made it a sub-region. This should also update the + // parent pointer + PyRegion_AddLocalRef(obj); + if (PyObject_GC_IsTracked(obj)) { + gc_list_remove(_Py_AS_GC(obj)); + } + + return 0; +} + +static void gc_region_list_dissolve(PyGC_Head *list) { + gc_list_for_each_subregion(list, (gc_list_callback_t)_gc_region_list_dissolve_callback, NULL); + + 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)); +} + // ********************************************************************** // This uses the given arguments to create and throw a `RegionError` @@ -324,7 +450,7 @@ static int regiondata_union_merge( _Py_region_data *source_data = (_Py_region_data*) source; if (HAS_OWNER_TAG(source, OWNER_TAG_MERGE_PENDING)) { Py_region_t pending_target = GET_OWNER_PTR(source); - + // Validate, that we either merge in the pending target or // into the local region on failure. // @@ -347,9 +473,6 @@ static int regiondata_union_merge( return -1; } - // TODO: insert COWN callbacks where needed - // TODO: insert specific cown handling were needed (clean???) - // Increase the RC of `target` to make sure none of the following // operations deallocates it by accident. regiondata_inc_rc(target); @@ -367,7 +490,7 @@ static int regiondata_union_merge( // If `target` is the parent of `source` it can be merged. This unsets // the parent of `source` to correctly update the OSC and RC. Py_region_t source_parent = regiondata_get_parent_follow_pending(source); - if (source_parent == target) { + if (source_parent == target && source_parent != NULL_REGION) { // Set parent can't fail here, since this function has increased the // OSC, thereby keeping the region open if it was previously open. regiondata_set_parent(source, NULL_REGION); @@ -406,7 +529,9 @@ static int regiondata_union_merge( _Py_region_data *target_data = (_Py_region_data*)target; target_data->lrc += source_data->lrc; target_data->osc += source_data->osc; - gc_list_merge(&source_data->gc_list, &target_data->gc_list); + // Do a region merge, which keeps the bridge objects at the start + // of the list and the contained objects at the end + gc_region_list_merge(&source_data->gc_list, &target_data->gc_list); // Check how the `open_tick` should be updated if (target_data->open_tick == OPEN_TICK_CLOSED) { @@ -430,9 +555,9 @@ static int regiondata_union_merge( // Check if the region can be opened or closed. regiondata_check_status(target); } else if (IS_LOCAL_REGION(target)) { - 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(&(source_data->gc_list), &(gc_state->old[0].head)); + // The function below also bumps the LRC of the sub-regions + // meaning this should be all covered now. + gc_region_list_dissolve(&(source_data->gc_list)); } // Remove information from `source` @@ -453,7 +578,7 @@ static int regiondata_union_merge( // This returns the OSC which was acquired earlier to keep it open during // this merge. if (cleanup_inc_osc) { - result |= regiondata_dec_osc(target); + regiondata_dec_osc(target); } // Decrement the `target` RC again @@ -521,33 +646,6 @@ static int regiondata_open(Py_region_t region) { return 1; } -static int regiondata_mark_as_clean(Py_region_t region) { - // Invariant: - ASSERT_IS_UNION_ROOT(region); - assert(regiondata_is_open(region)); - - // Regions without metadata are always clean - if (!HAS_DATA(region)) { - return 0; - } - - // Mark the region as open. - _Py_region_data *data = (_Py_region_data*)region; - Py_ssize_t old_open_tick = data->open_tick; - data->open_tick = _PyOwnership_get_open_region_tick(); - - // Check if an error occured - if (data->open_tick == OPEN_TICK_CLOSED) { - data->open_tick = old_open_tick; - return -1; - } - - // The open tick should always be even, see invariant - assert((data->open_tick % 2) == 0); - - return 0; -} - static int regiondata_is_open(Py_region_t region) { // Invariant: ASSERT_IS_UNION_ROOT(region); @@ -614,7 +712,6 @@ static int regiondata_is_dirty(Py_region_t region) { * This operation may fail if: * - The region is dirty (potentially caused by `_Py_ownership_state` being unavailable) * - Closing a parent region failed - * - TODO: xFrednet: If the owing cown is released. * * The region might still be closed, if the error came from an owner. */ @@ -639,14 +736,12 @@ static int regiondata_close(Py_region_t region) { // Notify the owner if (HAS_OWNER_TAG(region, OWNER_TAG_COWN)) { - SUCCEEDS(_PyCown_RegionClose( - _PyCownObject_CAST(GET_OWNER_PTR(region)), - _Py_region_data_CAST(region)->bridge, - _PyCown_ThisInterpreterId() - )); + // We don't notify the owning cown, mainly because this would add + // a potential failure state to this function which may be called + // from error paths. } else if (regiondata_get_parent(region) != 0) { Py_region_t parent = regiondata_get_parent(region); - SUCCEEDS(regiondata_dec_osc(parent)); + regiondata_dec_osc(parent); SUCCEEDS(regiondata_check_status(parent)); } @@ -766,24 +861,32 @@ static int regiondata_inc_lrc(Py_region_t region) { /* This decreases the local reference count. * - * This might close this and parent regions, which can fail. See - * `regiondata_close` for possible failures. * */ -static int regiondata_dec_lrc(Py_region_t region) { +static void regiondata_dec_lrc(Py_region_t region) { // Invariant: ASSERT_IS_UNION_ROOT(region); // Static regions don't need to be updated if (!HAS_DATA(region)) { - return 0; + return; } // Update the LRC _Py_region_data *data = (_Py_region_data*)region; if (data->lrc == 0) { - // Open the region, to mark it as dirty - SUCCEEDS(regiondata_open(region)); - regiondata_mark_as_dirty(region); + // Try to open the region to mark it as dirty + // + // This can fail if the region is owned by a cown which + // is currently not owned by the current interpreter + if (regiondata_open(region) == 0) { + regiondata_mark_as_dirty(region); + } else { + // Check if opening the failed attempt to open the region + // set an exception, and if so clear it. + if (PyErr_Occurred()) { + PyErr_Clear(); + } + } } else { data->lrc -= 1; @@ -791,15 +894,13 @@ static int regiondata_dec_lrc(Py_region_t region) { SUCCEEDS(regiondata_check_close(region)); } - // Return 0 on success - return 0; + return; error: // Undo the LRC decrement data->lrc += 1; - // Propagate the failure information - return 1; + assert(false && "Decrementing the LRC should never error"); } /* This increases the open-subregion count. (This does not update RC) @@ -833,13 +934,13 @@ static int regiondata_inc_osc(Py_region_t region) { * This might close this and parent regions, which can fail. See * `regiondata_close` for possible failures. * */ -static int regiondata_dec_osc(Py_region_t region) { +static void regiondata_dec_osc(Py_region_t region) { // Invariant: ASSERT_IS_UNION_ROOT(region); // Static regions don't need to be updated if (!HAS_DATA(region)) { - return 0; + return; } // Update the OSC @@ -850,14 +951,13 @@ static int regiondata_dec_osc(Py_region_t region) { SUCCEEDS(regiondata_check_close(region)); // Return 0 on success - return 0; + return; error: // Undo the OSC decrement data->osc += 1; - // Propagate the failure information - return 1; + assert(false && "Decrementing the OSC should never error"); } /* Setting the parent of an open region, might open the new parent region @@ -885,13 +985,19 @@ static int regiondata_set_parent(Py_region_t region, Py_region_t new_parent) { return 1; } - // TODO(regions): xFrednet: This is probably wrong? At least cleaning - // (a region and sub region) seems to have a off-by-one counting a OSC of 2 - if (regiondata_dec_osc(old_parent)) { - // Undo the inc_osc from above. - regiondata_dec_osc(new_parent); - return 1; - } + regiondata_dec_osc(old_parent); + } + + // Make sure the sub-region is removed from the old parent and added to the + // GC list of the new parent + if (HAS_DATA(old_parent)) { + assert(PyObject_GC_IsTracked(_PyObject_CAST(data->bridge))); + gc_list_remove(_Py_AS_GC(_PyObject_CAST(data->bridge))); + } + assert(!PyObject_GC_IsTracked(_PyObject_CAST(data->bridge))); + if (HAS_DATA(new_parent)) { + _Py_region_data *parent_data = _Py_region_data_CAST(new_parent); + gc_list_prepend(_Py_AS_GC(_PyObject_CAST(data->bridge)), &parent_data->gc_list); } // Only set the parent here, once all the failable operations are done @@ -1078,7 +1184,7 @@ static bool regiondata_is_bridge(Py_region_t region, PyObject *obj) { * from the GC list. * * This will just update the RC of the old and new region, all other state, - * like the LRC, has to be updated separatly. + * like the LRC, has to be updated separately. */ static void _PyRegion_Set(PyObject* obj, Py_region_t new_region) { // Invariant: @@ -1086,9 +1192,14 @@ static void _PyRegion_Set(PyObject* obj, Py_region_t new_region) { ASSERT_IS_UNION_ROOT(new_region); ASSERT_REGION_HAS_NO_TAG(new_region); - // Remove the object from its GC list. This has to be done before the - // updating the region RC to make sure that the list head remains allocated - if (PyObject_IS_GC(obj) && PyObject_GC_IsTracked(obj)) { + // Set the region first, this is important for the bridge check + Py_region_t old_region = obj->ob_region; + obj->ob_region = new_region; + + // Remove the object from its GC list. + if (Py_TYPE(obj) == &_PyRegion_Type) { + // Nothing to do here, bridges are moved by `set_parent` + } else if (PyObject_IS_GC(obj) && PyObject_GC_IsTracked(obj)) { if (HAS_DATA(new_region)) { _Py_region_data *data = (_Py_region_data *)new_region; gc_set_old_space(_Py_AS_GC(obj), 0); @@ -1103,9 +1214,7 @@ static void _PyRegion_Set(PyObject* obj, Py_region_t new_region) { } } - // Update the region and region rc - Py_region_t old_region = obj->ob_region; - obj->ob_region = new_region; + // Update the RC last to make sure the used GC lists stay allocated regiondata_inc_rc(new_region); regiondata_dec_rc(old_region); } @@ -1144,7 +1253,7 @@ int _add_to_region_visit(PyObject *src, PyObject *tgt, void *state_void) { if (IS_IMMUTABLE_REGION(tgt_region) || IS_COWN_REGION(tgt_region)) { return Py_OWNERSHIP_TRAVERSE_SKIP; } - + if (PyUnicode_CheckExact(tgt)) { if (_PyImmutability_Freeze(tgt)) { return Py_OWNERSHIP_TRAVERSE_ERR; @@ -1318,7 +1427,7 @@ PyRegion_staged_ref_t regiondata_stage_objects( add_state.add_ref_target = true; result = _add_to_region_visit(src, tgt, (void*)&add_state); add_state.add_ref_target = false; - + switch (result) { case Py_OWNERSHIP_TRAVERSE_VISIT: @@ -1376,8 +1485,7 @@ void staged_ref_reset(PyRegion_staged_ref_t staged_ref) { // and the one below is not the root of the union root? There is an // assert for this in `regiondata_dec_lrc` Py_region_t region = STAGED_AS_PTR(staged_ref); - int res = regiondata_dec_lrc(region); - assert(res == 0); + regiondata_dec_lrc(region); regiondata_dec_rc(region); return; } @@ -1392,8 +1500,7 @@ void staged_ref_reset(PyRegion_staged_ref_t staged_ref) { Py_region_t region = regions[i]; // Decrement the LRC - int res = regiondata_dec_lrc(region); - assert(res == 0); + regiondata_dec_lrc(region); regiondata_dec_rc(region); i += 1; @@ -1471,6 +1578,18 @@ PyRegion_staged_ref_t regiondata_add_object(Py_region_t subject_region, PyObject return 0; } +static int _clean_collect_subregions(Py_region_t region, PyObject *pending_list) { + assert(HAS_DATA(region)); + + // Ignore the region if it's currently closed + if (!regiondata_is_open(region)) { + return 0; + } + + // Enqueue the region to be cleaned + _Py_region_data *data = _Py_region_data_CAST(region); + return PyList_Append(pending_list, _PyObject_CAST(data->bridge)); +} int regiondata_clean(PyObject* bridge) { // Invariant assert(HAS_DATA(_PyRegion_GetFollowPending(bridge))); @@ -1499,18 +1618,39 @@ int regiondata_clean(PyObject* bridge) { if (pending_list == NULL) { goto error; } - PyList_SET_ITEM(_PyList_CAST(pending_list), 0, Py_NewRef(bridge)); + PyList_SET_ITEM(_PyList_CAST(pending_list), 0, PyRegion_NewRef(bridge)); while(PyList_Size(pending_list) != 0){ PyObject* item = list_pop(pending_list); Py_region_t item_region = _PyRegion_Get(item); + _Py_region_data* dirty_region_data = _Py_region_data_CAST(item_region); + // Invariant + ASSERT_REGION_OWNER_HAS_NO_TAG(item_region); assert(regiondata_is_bridge(item_region, item)); + assert(HAS_DATA(item_region)); + + // Clean path: If the region is clean, we collect the subregions to + // check if they need cleaning + if (!regiondata_is_dirty(item_region)) { + // Only collect subregions, if any of them is actually open. + // It should be impossible to have a dirty subregion without + // the parent knowing about them. + if (dirty_region_data->osc > 0) { + SUCCEEDS(gc_list_for_each_subregion( + &dirty_region_data->gc_list, + (gc_list_callback_t)_clean_collect_subregions, + pending_list + )); + } + + // The region doesn't need cleaning, proceed to the next one + continue; + } // Store metadata for the new region - assert(HAS_DATA(item_region)); - Py_region_t owner = ((_Py_region_data*)item_region)->owner; - ((_Py_region_data*)item_region)->owner = 0; + Py_region_t owner = dirty_region_data->owner; + dirty_region_data->owner = 0; bool was_open = regiondata_is_open(item_region); // Merge the region into local @@ -1534,16 +1674,15 @@ int regiondata_clean(PyObject* bridge) { } staged_ref_commit(staged_ref); - // TODO: Account in LRC for reference from owner, if present. - - - // TODO(regions): xFrednet: This doesn't account for region union... - // // `stage_objects` doesn't know about the reference from the owner and // counts it as a local reference. Meaning the LRC counts one more // reference than present. + // + // Owner may reference a cown, region, or (pending) merged region. Each of + // these would add a reference, expect cases when the region has been merged + // into the local region. But then we should never have a reference to it. if (owner != 0) { - SUCCEEDS(regiondata_dec_lrc(clean_region)); + regiondata_dec_lrc(clean_region); } // The region should now be marked as clean @@ -1557,6 +1696,9 @@ int regiondata_clean(PyObject* bridge) { if (!was_open && regiondata_is_open(clean_region)) { regiondata_inc_osc(clean_region); } + + // Increase the number of regions which have been cleaned + result += 1; } goto finally; @@ -1735,8 +1877,8 @@ Py_region_t _PyRegion_GetParent(Py_region_t child) { /* This cleans the region by reconstructing it from the bridge object. * - * FIXME(regions): xFrednet: This could be smarter, by only cleaning - * the region if it's dirty (or a subregion) is dirty. + * This returns the number of regions which have been cleaned or a negative + * number on failure. */ int _PyRegion_Clean(Py_region_t region) { if (!HAS_DATA(region)) { @@ -1751,6 +1893,31 @@ void _PyRegion_MakeDirty(Py_region_t region) { regiondata_mark_as_dirty(region); } +static int _get_subregion_callback(Py_region_t region, PyObject* list) { + assert(HAS_DATA(region)); + + _Py_region_data *data = _Py_region_data_CAST(region); + return PyList_Append(list, _PyObject_CAST(data->bridge)); +} +PyObject* _PyRegion_GetSubregions(Py_region_t region) { + PyObject* list = PyList_New(0); + if (!HAS_DATA(region)) { + return list; + } + + _Py_region_data *data = _Py_region_data_CAST(region); + int res = gc_list_for_each_subregion( + &data->gc_list, + (gc_list_callback_t)_get_subregion_callback, + (void*)list); + if (res != 0) { + Py_DECREF(list); + return NULL; + } + + return list; +} + int _PyRegion_IsBridge(PyObject *obj) { // _PyRegion_GetBridge will return None, if the region has no bridge, // this would result in a false positive for the None object @@ -1885,7 +2052,7 @@ static int _add_local_refs(PyObject *src, int tgt_count, PyObject **targets) { error: for (int undo_i = 0; undo_i < arg_i; undo_i += 1) { PyObject* tgt = targets[undo_i]; - result |= regiondata_dec_lrc(_PyRegion_Get(tgt)); + regiondata_dec_lrc(_PyRegion_Get(tgt)); } return result; } @@ -1992,6 +2159,18 @@ void PyRegion_CommitStagedRef(PyRegion_staged_ref_t staged_ref) { staged_ref_commit(staged_ref); } +static int add_refs_va_list(PyObject *src, int argc, va_list *args) { + PyRegion_staged_ref_t staged_ref = stage_va_list(src, argc, args); + + if (staged_ref == PyRegion_staged_ref_ERR) { + return -1; + } + + // Should always succeed + PyRegion_CommitStagedRef(staged_ref); + return 0; +} + /* Checks if the references from `src` to the targets are allowed and * updates the internal region state accordingly. * @@ -2004,16 +2183,10 @@ void PyRegion_CommitStagedRef(PyRegion_staged_ref_t staged_ref) { int _PyRegion_AddRefs(PyObject *src, int argc, ...) { va_list args; va_start(args, argc); - PyRegion_staged_ref_t staged_ref = stage_va_list(src, argc, &args); + int res = add_refs_va_list(src, argc, &args); va_end(args); - if (staged_ref == PyRegion_staged_ref_ERR) { - return -1; - } - - // Should always succeed - PyRegion_CommitStagedRef(staged_ref); - return 0; + return res; } /* Removes the reference from `src` to `tgt` and updates the internal state of @@ -2021,9 +2194,9 @@ int _PyRegion_AddRefs(PyObject *src, int argc, ...) { * * Returns 0 on success. */ -int _PyRegion_RemoveRef(PyObject *src, PyObject *tgt) { +void _PyRegion_RemoveRef(PyObject *src, PyObject *tgt) { if (tgt == NULL) { - return 0; + return; } Py_region_t src_region = _PyRegion_GetFollowPending(src); @@ -2031,12 +2204,12 @@ int _PyRegion_RemoveRef(PyObject *src, PyObject *tgt) { if (src_region == tgt_region) { // Intra-region references are always permitted and not tracket - return 0; + return; } if (IS_IMMUTABLE_REGION(tgt_region) || IS_COWN_REGION(tgt_region)) { // References to immutable objects or cowns are always permitted - return 0; + return; } // Mark the target region as dirty, if the source wasn't passed in. @@ -2044,13 +2217,14 @@ int _PyRegion_RemoveRef(PyObject *src, PyObject *tgt) { // include the dict object if (src == NULL) { regiondata_mark_as_dirty(_PyRegion_Get(tgt)); - return 0; + return; } if (IS_LOCAL_REGION(src_region)) { // Decrease the target region LRC since this reference came from // the local region - return regiondata_dec_lrc(_PyRegion_Get(tgt)); + regiondata_dec_lrc(_PyRegion_Get(tgt)); + return; } if (regiondata_is_bridge(tgt_region, tgt) @@ -2059,7 +2233,11 @@ int _PyRegion_RemoveRef(PyObject *src, PyObject *tgt) { assert(tgt_region == _PyRegion_Get(tgt)); // The removed reference was the owning references. The target region // gets unparented and is now free. - return regiondata_set_parent(tgt_region, NULL_REGION); + // + // This should always succeed, since this action may only close regions + // but not open any. + int res = regiondata_set_parent(tgt_region, NULL_REGION); + assert(res == 0); } else { // The reference came from `src` to `tgt` while the target region // already had a parent. This is not allowed but can happen in @@ -2071,9 +2249,6 @@ int _PyRegion_RemoveRef(PyObject *src, PyObject *tgt) { // for builds without asserts. regiondata_mark_as_dirty(src_region); regiondata_mark_as_dirty(tgt_region); - - // Still return 0, since the reference could be should be removed. - return 0; } } @@ -2112,8 +2287,34 @@ int _PyRegion_AddLocalRefs(int argc, ...) { return _add_local_refs(NULL, list_size, list); } -int _PyRegion_RemoveLocalRef(PyObject *tgt) { - return regiondata_dec_lrc(_PyRegion_Get(tgt)); +void _PyRegion_RemoveLocalRef(PyObject *tgt) { + regiondata_dec_lrc(_PyRegion_Get(tgt)); +} + +int _PyRegion_TakeRefs(PyObject *src, int argc, ...) { + // See comment in `_PyRegion_TakeRef` to explain how this + // function works and why it should be safe. + va_list args; + va_start(args, argc); + + // Add references + va_list add_ref_args; + va_copy(add_ref_args, args); + int res = add_refs_va_list(src, argc, &add_ref_args); + va_end(add_ref_args); + if (res != 0) { + va_end(args); + return res; + } + + // Remove the local references + for (int arg_i = 0; arg_i < argc; arg_i += 1) { + PyObject* tgt = va_arg(args, PyObject*); + PyRegion_RemoveLocalRef(tgt); + } + + va_end(args); + return res; } int _PyRegion_SetCownRegion(_PyCownObject *cown) { @@ -2176,6 +2377,28 @@ int _PyRegion_RemoveCown(_PyRegionObject* bridge, _PyCownObject *cown) { return regiondata_set_cown(region, NULL); } +/* This function should be called when a function of the given type is +* called from C. This will check if the type is marked as Pyrona aware +* meaning that it has the needed write barriers. +* +* If the type is not aware of regions, we'll assume that the code is +* untrusted and mark all currently open regions as dirty. This ensures +* that the region invariant can be trusted for clean regions. +* +* This operation requires the GIL. +* +* This function will also store the name of the type, to be accessible +* on demand to help with debugging. +*/ +void PyRegion_NotifyTypeUse(PyTypeObject* tp) { + if ((tp->tp_flags2 & Py_TPFLAGS2_REGION_AWARE) != 0) { + return; + } + + _PyOwnership_notify_untrusted_code(tp->tp_name); +} + + // TODO(regions): xFrednet: Cleanup // - Move region error into core and emit it instead of runtime errors // TODO(regions): xFrednet: Regions with pending merges can still be closed and send off. @@ -2187,12 +2410,6 @@ int _PyRegion_RemoveCown(_PyRegionObject* bridge, _PyCownObject *cown) { // TODO(regions): xFrednet: Weak Reference into regions // TODO(regions): xFrednet: Add new `MergedRegion` so that the Region type // correlates with it being the bridge. -// TODO(regions): xFrednet: Merging a region into the local region should open -// subregions, if the merge didn't happened for error handling -// (Make sure subregions are always at the start of the region CG list) -// (This might need a custom list, since bridges are currently part of -// their regions list) -// (Can this opening be done by checking if the parent is local?) // TODO(regions): xFrednet: Add notion of movability. // - Cowns (Should be in cown region) // - Immutable (Should be in immutable region)