From 3ee0e637009f047d8309414b573393d95a0b396f Mon Sep 17 00:00:00 2001 From: eduardo-elizondo Date: Sat, 17 Aug 2019 13:44:45 -0700 Subject: [PATCH 01/10] Suppress subtype_dealloc decref when base type is a C heap type The instance destructor for a type is responsible for preparing an instance for deallocation by decrementing the reference counts of its referents. If an instance belongs to a heap type, the type object of an instance has its reference count decremented while for static types, which are permanently allocated, the type object is unaffected by the instance destructor. Previously, the default instance destructor searched the class hierarchy for an inherited instance destructor and, if present, would invoke it. Then, if the instance type is a heap type, it would decrement the reference count of that heap type. However, this could result in the premature destruction of a type because the inherited instance destructor should have already decremented the reference count of the type object. This change avoids the premature destruction of the type object by suppressing the decrement of its reference count when an inherited, non-default instance destructor has been invoked. Finally, an assertion on the Py_SIZE of a type was deleted. Heap types have a non zero size, making this into an incorrect assertion. --- Lib/test/test_capi.py | 86 ++++++++++++++++ Modules/_testcapimodule.c | 209 ++++++++++++++++++++++++++++++++++++++ Objects/typeobject.c | 20 ++-- 3 files changed, 305 insertions(+), 10 deletions(-) diff --git a/Lib/test/test_capi.py b/Lib/test/test_capi.py index 7b35ba60b53a75..6f082d35aa5a9a 100644 --- a/Lib/test/test_capi.py +++ b/Lib/test/test_capi.py @@ -38,6 +38,92 @@ class InstanceMethod: class CAPITest(unittest.TestCase): + def test_subclass_of_heap_gc_ctype_with_tpdealloc_decrefs_once(self): + class HeapGcCTypeSubclass(_testcapi.HeapGcCType): + def __init__(self): + self.value2 = 20 + super().__init__() + + subclass_instance = HeapGcCTypeSubclass() + type_refcnt = sys.getrefcount(HeapGcCTypeSubclass) + + # Test that subclass instance was fully created + self.assertEqual(subclass_instance.value, 10) + self.assertEqual(subclass_instance.value2, 20) + + # Test that the type reference count is only decremented once + del subclass_instance + self.assertEqual(type_refcnt - 1, sys.getrefcount(HeapGcCTypeSubclass)) + + def test_subclass_of_heap_gc_ctype_with_del_modifying_dunder_class_only_decrefs_once(self): + class A(_testcapi.HeapGcCType): + def __init__(self): + self.value2 = 20 + super().__init__() + + class B(A): + def __init__(self): + super().__init__() + + def __del__(self): + self.__class__ = A + A.refcnt_in_del = sys.getrefcount(A) + B.refcnt_in_del = sys.getrefcount(B) + + subclass_instance = B() + type_refcnt = sys.getrefcount(B) + new_type_refcnt = sys.getrefcount(A) + + # Test that subclass instance was fully created + self.assertEqual(subclass_instance.value, 10) + self.assertEqual(subclass_instance.value2, 20) + + del subclass_instance + + # Test that setting __class__ modified the reference counts of the types + self.assertEqual(type_refcnt - 1, B.refcnt_in_del) + self.assertEqual(new_type_refcnt + 1, A.refcnt_in_del) + + # Test that the original type already has decreased its refcnt + self.assertEqual(type_refcnt - 1, sys.getrefcount(B)) + + # Test that subtype_dealloc decref the newly assigned __class__ only once + self.assertEqual(new_type_refcnt, sys.getrefcount(A)) + + def test_c_subclass_of_heap_ctype_with_tpdealloc_decrefs_once(self): + subclass_instance = _testcapi.HeapCTypeSubclass() + type_refcnt = sys.getrefcount(_testcapi.HeapCTypeSubclass) + + # Test that subclass instance was fully created + self.assertEqual(subclass_instance.value, 10) + self.assertEqual(subclass_instance.value2, 20) + + # Test that the type reference count is only decremented once + del subclass_instance + self.assertEqual(type_refcnt - 1, sys.getrefcount(_testcapi.HeapCTypeSubclass)) + + def test_c_subclass_of_heap_ctype_with_del_modifying_dunder_class_only_decrefs_once(self): + subclass_instance = _testcapi.HeapCTypeSubclassWithFinalizer() + type_refcnt = sys.getrefcount(_testcapi.HeapCTypeSubclassWithFinalizer) + new_type_refcnt = sys.getrefcount(_testcapi.HeapCTypeSubclass) + + # Test that subclass instance was fully created + self.assertEqual(subclass_instance.value, 10) + self.assertEqual(subclass_instance.value2, 20) + + # The tp_finalize slot will set __class__ to HeapCTypeSubclass + del subclass_instance + + # Test that setting __class__ modified the reference counts of the types + self.assertEqual(type_refcnt - 1, _testcapi.HeapCTypeSubclassWithFinalizer.refcnt_in_del) + self.assertEqual(new_type_refcnt + 1, _testcapi.HeapCTypeSubclass.refcnt_in_del) + + # Test that the original type already has decreased its refcnt + self.assertEqual(type_refcnt - 1, sys.getrefcount(_testcapi.HeapCTypeSubclassWithFinalizer)) + + # Test that subtype_dealloc decref the newly assigned __class__ only once + self.assertEqual(new_type_refcnt, sys.getrefcount(_testcapi.HeapCTypeSubclass)) + def test_instancemethod(self): inst = InstanceMethod() self.assertEqual(id(inst), inst.id()) diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 8a6e741d28100c..5a695ec572ccf8 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -6012,6 +6012,180 @@ static struct PyModuleDef _testcapimodule = { NULL }; +typedef struct { + PyObject_HEAD + int value; +} HeapCTypeObject; + +static struct PyMemberDef heapctype_members[] = { + {"value", T_INT, offsetof(HeapCTypeObject, value)}, + {NULL} /* Sentinel */ +}; + +static int +heapctype_init(PyObject *self, PyObject *args, PyObject *kwargs) +{ + ((HeapCTypeObject *)self)->value = 10; + return 0; +} + +static void +heapgcctype_dealloc(HeapCTypeObject *self) +{ + PyTypeObject *tp = Py_TYPE(self); + PyObject_GC_UnTrack(self); + PyObject_GC_Del(self); + Py_DECREF(tp); +} + +static PyType_Slot HeapGcCType_slots[] = { + {Py_tp_init, heapctype_init}, + {Py_tp_members, heapctype_members}, + {Py_tp_dealloc, heapgcctype_dealloc}, + {0, 0}, +}; + +static PyType_Spec HeapGcCType_spec = { + "_testcapi.HeapGcCType", + sizeof(HeapCTypeObject), + 0, + Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE | Py_TPFLAGS_HAVE_GC, + HeapGcCType_slots +}; + +static void +heapctype_dealloc(HeapCTypeObject *self) +{ + PyTypeObject *tp = Py_TYPE(self); + PyObject_Del(self); + Py_DECREF(tp); +} + +static PyType_Slot HeapCType_slots[] = { + {Py_tp_init, heapctype_init}, + {Py_tp_members, heapctype_members}, + {Py_tp_dealloc, heapctype_dealloc}, + {0, 0}, +}; + +static PyType_Spec HeapCType_spec = { + "_testcapi.HeapCType", + sizeof(HeapCTypeObject), + 0, + Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE, + HeapCType_slots +}; + +typedef struct { + HeapCTypeObject base; + int value2; +} HeapCTypeSubclassObject; + +static int +heapctypesubclass_init(PyObject *self, PyObject *args, PyObject *kwargs) +{ + /* Get HeapCTypeSubclass */ + PyObject *m = PyState_FindModule(&_testcapimodule); + if (m == NULL) { + return -1; + } + PyTypeObject *ctypesubclass = (PyTypeObject *)PyObject_GetAttrString(m, "HeapCTypeSubclass"); + if (ctypesubclass == NULL) { + return -1; + } + + PyTypeObject *base = (PyTypeObject *)PyType_GetSlot(ctypesubclass, Py_tp_base); + Py_DECREF(ctypesubclass); + initproc base_init = PyType_GetSlot(base, Py_tp_init); + if (base_init(self, args, kwargs) < 0) { + return -1; + } + ((HeapCTypeSubclassObject *)self)->value2 = 20; + return 0; +} + +static struct PyMemberDef heapctypesubclass_members[] = { + {"value", T_INT, offsetof(HeapCTypeObject, value)}, + {"value2", T_INT, offsetof(HeapCTypeSubclassObject, value2)}, + {NULL} /* Sentinel */ +}; + +static PyType_Slot HeapCTypeSubclass_slots[] = { + {Py_tp_init, heapctypesubclass_init}, + {Py_tp_members, heapctypesubclass_members}, + {0, 0}, +}; + +static PyType_Spec HeapCTypeSubclass_spec = { + "_testcapi.HeapCTypeSubclass", + sizeof(HeapCTypeSubclassObject), + 0, + Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE, + HeapCTypeSubclass_slots +}; + +static int +heapctypesubclasswithfinalizer_init(PyObject *self, PyObject *args, PyObject *kwargs) +{ + PyTypeObject *base = (PyTypeObject *)PyType_GetSlot(Py_TYPE(self), Py_tp_base); + initproc base_init = PyType_GetSlot(base, Py_tp_init); + base_init(self, args, kwargs); + return 0; +} + +static void +heapctypesubclasswithfinalizer_finalize(PyObject *self) +{ + PyObject *error_type, *error_value, *error_traceback, *m, *oldtype, *newtype; + + /* Save the current exception, if any. */ + PyErr_Fetch(&error_type, &error_value, &error_traceback); + + m = PyState_FindModule(&_testcapimodule); + if (m == NULL) { + return; + } + oldtype = PyObject_GetAttrString(m, "HeapCTypeSubclassWithFinalizer"); + newtype = PyObject_GetAttrString(m, "HeapCTypeSubclass"); + if (oldtype == NULL || newtype == NULL) { + goto cleanup_finalize; + } + + if (PyObject_SetAttrString(self, "__class__", newtype) < 0) { + goto cleanup_finalize; + } + if (PyObject_SetAttrString( + oldtype, "refcnt_in_del", PyLong_FromSsize_t(Py_REFCNT(oldtype))) < 0) { + goto cleanup_finalize; + } + if (PyObject_SetAttrString( + newtype, "refcnt_in_del", PyLong_FromSsize_t(Py_REFCNT(newtype))) < 0) { + goto cleanup_finalize; + } + +cleanup_finalize: + Py_XDECREF(oldtype); + Py_XDECREF(newtype); + + /* Restore the saved exception. */ + PyErr_Restore(error_type, error_value, error_traceback); +} + +static PyType_Slot HeapCTypeSubclassWithFinalizer_slots[] = { + {Py_tp_init, heapctypesubclasswithfinalizer_init}, + {Py_tp_members, heapctypesubclass_members}, + {Py_tp_finalize, heapctypesubclasswithfinalizer_finalize}, + {0, 0}, +}; + +static PyType_Spec HeapCTypeSubclassWithFinalizer_spec = { + "_testcapi.HeapCTypeSubclassWithFinalizer", + sizeof(HeapCTypeSubclassObject), + 0, + Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_FINALIZE, + HeapCTypeSubclassWithFinalizer_slots +}; + /* Per PEP 489, this module will not be converted to multi-phase initialization */ @@ -6129,5 +6303,40 @@ PyInit__testcapi(void) TestError = PyErr_NewException("_testcapi.error", NULL, NULL); Py_INCREF(TestError); PyModule_AddObject(m, "error", TestError); + + PyObject *HeapGcCType = PyType_FromSpec(&HeapGcCType_spec); + if (HeapGcCType == NULL) { + return NULL; + } + PyModule_AddObject(m, "HeapGcCType", HeapGcCType); + + PyObject *HeapCType = PyType_FromSpec(&HeapCType_spec); + if (HeapCType == NULL) { + return NULL; + } + PyObject *subclass_bases = PyTuple_Pack(1, HeapCType); + if (subclass_bases == NULL) { + return NULL; + } + PyObject *HeapCTypeSubclass = PyType_FromSpecWithBases(&HeapCTypeSubclass_spec, subclass_bases); + if (HeapCTypeSubclass == NULL) { + return NULL; + } + Py_DECREF(subclass_bases); + PyModule_AddObject(m, "HeapCTypeSubclass", HeapCTypeSubclass); + + PyObject *subclass_with_finalizer_bases = PyTuple_Pack(1, HeapCTypeSubclass); + if (subclass_with_finalizer_bases == NULL) { + return NULL; + } + PyObject *HeapCTypeSubclassWithFinalizer = PyType_FromSpecWithBases( + &HeapCTypeSubclassWithFinalizer_spec, subclass_with_finalizer_bases); + if (HeapCTypeSubclassWithFinalizer == NULL) { + return NULL; + } + Py_DECREF(subclass_with_finalizer_bases); + PyModule_AddObject(m, "HeapCTypeSubclassWithFinalizer", HeapCTypeSubclassWithFinalizer); + + PyState_AddModule(m, &_testcapimodule); return m; } diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 0ca7dcb695bb7f..67ac3d4c10fc35 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -1157,11 +1157,9 @@ subtype_dealloc(PyObject *self) /* Test whether the type has GC exactly once */ if (!PyType_IS_GC(type)) { - /* It's really rare to find a dynamic type that doesn't have - GC; it can only happen when deriving from 'object' and not - adding any slots or instance variables. This allows - certain simplifications: there's no need to call - clear_slots(), or DECREF the dict, or clear weakrefs. */ + /* A non GC dyanmic type allows certain simplifications: + there's no need to call clear_slots(), or DECREF the dict, + or clear weakrefs. */ /* Maybe call finalizer; exit early if resurrected */ if (type->tp_finalize) { @@ -1177,7 +1175,6 @@ subtype_dealloc(PyObject *self) /* Find the nearest base with a different tp_dealloc */ base = type; while ((basedealloc = base->tp_dealloc) == subtype_dealloc) { - assert(Py_SIZE(base) == 0); base = base->tp_base; assert(base); } @@ -1189,8 +1186,10 @@ subtype_dealloc(PyObject *self) assert(basedealloc); basedealloc(self); - /* Can't reference self beyond this point */ - Py_DECREF(type); + /* Only decref if the base type is not already a heap allocated type. + Otherwise, basedealloc should have decref'd it already */ + if (type->tp_flags & Py_TPFLAGS_HEAPTYPE && !(base->tp_flags & Py_TPFLAGS_HEAPTYPE)) + Py_DECREF(type); /* Done */ return; @@ -1289,8 +1288,9 @@ subtype_dealloc(PyObject *self) /* Can't reference self beyond this point. It's possible tp_del switched our type from a HEAPTYPE to a non-HEAPTYPE, so be careful about - reference counting. */ - if (type->tp_flags & Py_TPFLAGS_HEAPTYPE) + reference counting. Only decref if the base type is not already a heap + allocated type. Otherwise, basedealloc should have decref'd it already */ + if (type->tp_flags & Py_TPFLAGS_HEAPTYPE && !(base->tp_flags & Py_TPFLAGS_HEAPTYPE)) Py_DECREF(type); endlabel: From 67cef48390f9cb2d04edc7922a724ea511ec68cd Mon Sep 17 00:00:00 2001 From: eduardo-elizondo Date: Sat, 17 Aug 2019 13:50:32 -0700 Subject: [PATCH 02/10] added blurb --- Misc/NEWS.d/next/C API/2019-08-17-13-50-21.bpo-37879.CZeUem.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/C API/2019-08-17-13-50-21.bpo-37879.CZeUem.rst diff --git a/Misc/NEWS.d/next/C API/2019-08-17-13-50-21.bpo-37879.CZeUem.rst b/Misc/NEWS.d/next/C API/2019-08-17-13-50-21.bpo-37879.CZeUem.rst new file mode 100644 index 00000000000000..87322fbf5f239b --- /dev/null +++ b/Misc/NEWS.d/next/C API/2019-08-17-13-50-21.bpo-37879.CZeUem.rst @@ -0,0 +1,2 @@ +Fix subtype_dealloc to suppress the type decref when the base type is a C +heap type From d440e8ad8c520fc1ce25d129edf997031625d947 Mon Sep 17 00:00:00 2001 From: eduardo-elizondo Date: Sat, 17 Aug 2019 14:02:00 -0700 Subject: [PATCH 03/10] Fixed whitespace --- Objects/typeobject.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 67ac3d4c10fc35..ba311002493f04 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -1189,7 +1189,7 @@ subtype_dealloc(PyObject *self) /* Only decref if the base type is not already a heap allocated type. Otherwise, basedealloc should have decref'd it already */ if (type->tp_flags & Py_TPFLAGS_HEAPTYPE && !(base->tp_flags & Py_TPFLAGS_HEAPTYPE)) - Py_DECREF(type); + Py_DECREF(type); /* Done */ return; From 5bceecd58125a7157c9842df2d5f08fda2903fff Mon Sep 17 00:00:00 2001 From: eduardo-elizondo Date: Sun, 18 Aug 2019 15:33:59 -0700 Subject: [PATCH 04/10] Typo and indent --- Objects/typeobject.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index ba311002493f04..684c5619f29f0b 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -1157,7 +1157,7 @@ subtype_dealloc(PyObject *self) /* Test whether the type has GC exactly once */ if (!PyType_IS_GC(type)) { - /* A non GC dyanmic type allows certain simplifications: + /* A non GC dynamic type allows certain simplifications: there's no need to call clear_slots(), or DECREF the dict, or clear weakrefs. */ @@ -1189,7 +1189,7 @@ subtype_dealloc(PyObject *self) /* Only decref if the base type is not already a heap allocated type. Otherwise, basedealloc should have decref'd it already */ if (type->tp_flags & Py_TPFLAGS_HEAPTYPE && !(base->tp_flags & Py_TPFLAGS_HEAPTYPE)) - Py_DECREF(type); + Py_DECREF(type); /* Done */ return; From d227ab3f4e748c3d26d5d1c0377a028892379584 Mon Sep 17 00:00:00 2001 From: Eddie Elizondo Date: Mon, 9 Sep 2019 22:44:48 -0700 Subject: [PATCH 05/10] Fix leaks --- Modules/_testcapimodule.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 5a695ec572ccf8..d4d0e2129d2727 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -6143,7 +6143,7 @@ heapctypesubclasswithfinalizer_finalize(PyObject *self) m = PyState_FindModule(&_testcapimodule); if (m == NULL) { - return; + goto cleanup_finalize; } oldtype = PyObject_GetAttrString(m, "HeapCTypeSubclassWithFinalizer"); newtype = PyObject_GetAttrString(m, "HeapCTypeSubclass"); From eaea7be0f4ff8020fdc4ebbc9bb0733fe39e4bd8 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Mon, 9 Sep 2019 14:56:43 +0100 Subject: [PATCH 06/10] Avoid PyState_FindModule in heapctypesubclass_init --- Modules/_testcapimodule.c | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-) diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index d4d0e2129d2727..6a2ef6bb6ad877 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -6084,22 +6084,11 @@ typedef struct { static int heapctypesubclass_init(PyObject *self, PyObject *args, PyObject *kwargs) { - /* Get HeapCTypeSubclass */ - PyObject *m = PyState_FindModule(&_testcapimodule); - if (m == NULL) { - return -1; - } - PyTypeObject *ctypesubclass = (PyTypeObject *)PyObject_GetAttrString(m, "HeapCTypeSubclass"); - if (ctypesubclass == NULL) { - return -1; - } - - PyTypeObject *base = (PyTypeObject *)PyType_GetSlot(ctypesubclass, Py_tp_base); - Py_DECREF(ctypesubclass); - initproc base_init = PyType_GetSlot(base, Py_tp_init); - if (base_init(self, args, kwargs) < 0) { + /* Call __init__ of the superclass */ + if (heapctype_init(self, args, kwargs) < 0) { return -1; } + /* Initialize additional element */ ((HeapCTypeSubclassObject *)self)->value2 = 20; return 0; } From 935bc378a8e41a7f4afa622c45ebe7c2ce8eb6c9 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Mon, 9 Sep 2019 14:58:16 +0100 Subject: [PATCH 07/10] Move the added test code - Move CAPITest additions to the end (they're testing a fairly exotic edge case, so I wouldn't make them prominent). - Have the PyModuleDef for _testcapi close to the PyInit function again (this needs a forward declaration, but keeps the module init code logically together). --- Lib/test/test_capi.py | 172 +++++++++++++++++++------------------- Modules/_testcapimodule.c | 27 +++--- 2 files changed, 100 insertions(+), 99 deletions(-) diff --git a/Lib/test/test_capi.py b/Lib/test/test_capi.py index 6f082d35aa5a9a..4d6e2f21551a6c 100644 --- a/Lib/test/test_capi.py +++ b/Lib/test/test_capi.py @@ -38,92 +38,6 @@ class InstanceMethod: class CAPITest(unittest.TestCase): - def test_subclass_of_heap_gc_ctype_with_tpdealloc_decrefs_once(self): - class HeapGcCTypeSubclass(_testcapi.HeapGcCType): - def __init__(self): - self.value2 = 20 - super().__init__() - - subclass_instance = HeapGcCTypeSubclass() - type_refcnt = sys.getrefcount(HeapGcCTypeSubclass) - - # Test that subclass instance was fully created - self.assertEqual(subclass_instance.value, 10) - self.assertEqual(subclass_instance.value2, 20) - - # Test that the type reference count is only decremented once - del subclass_instance - self.assertEqual(type_refcnt - 1, sys.getrefcount(HeapGcCTypeSubclass)) - - def test_subclass_of_heap_gc_ctype_with_del_modifying_dunder_class_only_decrefs_once(self): - class A(_testcapi.HeapGcCType): - def __init__(self): - self.value2 = 20 - super().__init__() - - class B(A): - def __init__(self): - super().__init__() - - def __del__(self): - self.__class__ = A - A.refcnt_in_del = sys.getrefcount(A) - B.refcnt_in_del = sys.getrefcount(B) - - subclass_instance = B() - type_refcnt = sys.getrefcount(B) - new_type_refcnt = sys.getrefcount(A) - - # Test that subclass instance was fully created - self.assertEqual(subclass_instance.value, 10) - self.assertEqual(subclass_instance.value2, 20) - - del subclass_instance - - # Test that setting __class__ modified the reference counts of the types - self.assertEqual(type_refcnt - 1, B.refcnt_in_del) - self.assertEqual(new_type_refcnt + 1, A.refcnt_in_del) - - # Test that the original type already has decreased its refcnt - self.assertEqual(type_refcnt - 1, sys.getrefcount(B)) - - # Test that subtype_dealloc decref the newly assigned __class__ only once - self.assertEqual(new_type_refcnt, sys.getrefcount(A)) - - def test_c_subclass_of_heap_ctype_with_tpdealloc_decrefs_once(self): - subclass_instance = _testcapi.HeapCTypeSubclass() - type_refcnt = sys.getrefcount(_testcapi.HeapCTypeSubclass) - - # Test that subclass instance was fully created - self.assertEqual(subclass_instance.value, 10) - self.assertEqual(subclass_instance.value2, 20) - - # Test that the type reference count is only decremented once - del subclass_instance - self.assertEqual(type_refcnt - 1, sys.getrefcount(_testcapi.HeapCTypeSubclass)) - - def test_c_subclass_of_heap_ctype_with_del_modifying_dunder_class_only_decrefs_once(self): - subclass_instance = _testcapi.HeapCTypeSubclassWithFinalizer() - type_refcnt = sys.getrefcount(_testcapi.HeapCTypeSubclassWithFinalizer) - new_type_refcnt = sys.getrefcount(_testcapi.HeapCTypeSubclass) - - # Test that subclass instance was fully created - self.assertEqual(subclass_instance.value, 10) - self.assertEqual(subclass_instance.value2, 20) - - # The tp_finalize slot will set __class__ to HeapCTypeSubclass - del subclass_instance - - # Test that setting __class__ modified the reference counts of the types - self.assertEqual(type_refcnt - 1, _testcapi.HeapCTypeSubclassWithFinalizer.refcnt_in_del) - self.assertEqual(new_type_refcnt + 1, _testcapi.HeapCTypeSubclass.refcnt_in_del) - - # Test that the original type already has decreased its refcnt - self.assertEqual(type_refcnt - 1, sys.getrefcount(_testcapi.HeapCTypeSubclassWithFinalizer)) - - # Test that subtype_dealloc decref the newly assigned __class__ only once - self.assertEqual(new_type_refcnt, sys.getrefcount(_testcapi.HeapCTypeSubclass)) - def test_instancemethod(self): inst = InstanceMethod() self.assertEqual(id(inst), inst.id()) @@ -469,6 +383,92 @@ def __del__(self): del L self.assertEqual(PyList.num, 0) + def test_subclass_of_heap_gc_ctype_with_tpdealloc_decrefs_once(self): + class HeapGcCTypeSubclass(_testcapi.HeapGcCType): + def __init__(self): + self.value2 = 20 + super().__init__() + + subclass_instance = HeapGcCTypeSubclass() + type_refcnt = sys.getrefcount(HeapGcCTypeSubclass) + + # Test that subclass instance was fully created + self.assertEqual(subclass_instance.value, 10) + self.assertEqual(subclass_instance.value2, 20) + + # Test that the type reference count is only decremented once + del subclass_instance + self.assertEqual(type_refcnt - 1, sys.getrefcount(HeapGcCTypeSubclass)) + + def test_subclass_of_heap_gc_ctype_with_del_modifying_dunder_class_only_decrefs_once(self): + class A(_testcapi.HeapGcCType): + def __init__(self): + self.value2 = 20 + super().__init__() + + class B(A): + def __init__(self): + super().__init__() + + def __del__(self): + self.__class__ = A + A.refcnt_in_del = sys.getrefcount(A) + B.refcnt_in_del = sys.getrefcount(B) + + subclass_instance = B() + type_refcnt = sys.getrefcount(B) + new_type_refcnt = sys.getrefcount(A) + + # Test that subclass instance was fully created + self.assertEqual(subclass_instance.value, 10) + self.assertEqual(subclass_instance.value2, 20) + + del subclass_instance + + # Test that setting __class__ modified the reference counts of the types + self.assertEqual(type_refcnt - 1, B.refcnt_in_del) + self.assertEqual(new_type_refcnt + 1, A.refcnt_in_del) + + # Test that the original type already has decreased its refcnt + self.assertEqual(type_refcnt - 1, sys.getrefcount(B)) + + # Test that subtype_dealloc decref the newly assigned __class__ only once + self.assertEqual(new_type_refcnt, sys.getrefcount(A)) + + def test_c_subclass_of_heap_ctype_with_tpdealloc_decrefs_once(self): + subclass_instance = _testcapi.HeapCTypeSubclass() + type_refcnt = sys.getrefcount(_testcapi.HeapCTypeSubclass) + + # Test that subclass instance was fully created + self.assertEqual(subclass_instance.value, 10) + self.assertEqual(subclass_instance.value2, 20) + + # Test that the type reference count is only decremented once + del subclass_instance + self.assertEqual(type_refcnt - 1, sys.getrefcount(_testcapi.HeapCTypeSubclass)) + + def test_c_subclass_of_heap_ctype_with_del_modifying_dunder_class_only_decrefs_once(self): + subclass_instance = _testcapi.HeapCTypeSubclassWithFinalizer() + type_refcnt = sys.getrefcount(_testcapi.HeapCTypeSubclassWithFinalizer) + new_type_refcnt = sys.getrefcount(_testcapi.HeapCTypeSubclass) + + # Test that subclass instance was fully created + self.assertEqual(subclass_instance.value, 10) + self.assertEqual(subclass_instance.value2, 20) + + # The tp_finalize slot will set __class__ to HeapCTypeSubclass + del subclass_instance + + # Test that setting __class__ modified the reference counts of the types + self.assertEqual(type_refcnt - 1, _testcapi.HeapCTypeSubclassWithFinalizer.refcnt_in_del) + self.assertEqual(new_type_refcnt + 1, _testcapi.HeapCTypeSubclass.refcnt_in_del) + + # Test that the original type already has decreased its refcnt + self.assertEqual(type_refcnt - 1, sys.getrefcount(_testcapi.HeapCTypeSubclassWithFinalizer)) + + # Test that subtype_dealloc decref the newly assigned __class__ only once + self.assertEqual(new_type_refcnt, sys.getrefcount(_testcapi.HeapCTypeSubclass)) + class TestPendingCalls(unittest.TestCase): diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 6a2ef6bb6ad877..51dd21e63ebabd 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -32,6 +32,8 @@ # error "_testcapi must test the public Python C API, not CPython internal C API" #endif +static struct PyModuleDef _testcapimodule; + static PyObject *TestError; /* set to exception object in init */ /* Raise TestError with test_name + ": " + msg, and return NULL. */ @@ -5999,19 +6001,6 @@ static PyTypeObject MethodDescriptor2_Type = { .tp_flags = Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE | _Py_TPFLAGS_HAVE_VECTORCALL, }; - -static struct PyModuleDef _testcapimodule = { - PyModuleDef_HEAD_INIT, - "_testcapi", - NULL, - -1, - TestMethods, - NULL, - NULL, - NULL, - NULL -}; - typedef struct { PyObject_HEAD int value; @@ -6175,6 +6164,18 @@ static PyType_Spec HeapCTypeSubclassWithFinalizer_spec = { HeapCTypeSubclassWithFinalizer_slots }; +static struct PyModuleDef _testcapimodule = { + PyModuleDef_HEAD_INIT, + "_testcapi", + NULL, + -1, + TestMethods, + NULL, + NULL, + NULL, + NULL +}; + /* Per PEP 489, this module will not be converted to multi-phase initialization */ From 6a23625440fa5c8809cfcbd277547e2d5ccc9f04 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Mon, 9 Sep 2019 17:59:14 +0100 Subject: [PATCH 08/10] Remove unnecessary descriptor The same descriptor is in the superclass. --- Modules/_testcapimodule.c | 1 - 1 file changed, 1 deletion(-) diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 51dd21e63ebabd..3f6bbb20303ee3 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -6083,7 +6083,6 @@ heapctypesubclass_init(PyObject *self, PyObject *args, PyObject *kwargs) } static struct PyMemberDef heapctypesubclass_members[] = { - {"value", T_INT, offsetof(HeapCTypeObject, value)}, {"value2", T_INT, offsetof(HeapCTypeSubclassObject, value2)}, {NULL} /* Sentinel */ }; From f4258a1bf71951133a0a72e70fce9d21acb6f40d Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Mon, 9 Sep 2019 15:29:50 +0100 Subject: [PATCH 09/10] Add docstrings to C classes This is mainly to make the intention clearer to readers of the code: the docstrings serve as headings. --- Modules/_testcapimodule.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 3f6bbb20303ee3..9f5b84b95f6b42 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -6001,6 +6001,10 @@ static PyTypeObject MethodDescriptor2_Type = { .tp_flags = Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE | _Py_TPFLAGS_HAVE_VECTORCALL, }; +PyDoc_STRVAR(heapgctype__doc__, +"A heap type with GC, and with overridden dealloc.\n\n" +"The 'value' attribute is set to 10 in __init__."); + typedef struct { PyObject_HEAD int value; @@ -6031,6 +6035,7 @@ static PyType_Slot HeapGcCType_slots[] = { {Py_tp_init, heapctype_init}, {Py_tp_members, heapctype_members}, {Py_tp_dealloc, heapgcctype_dealloc}, + {Py_tp_doc, heapgctype__doc__}, {0, 0}, }; @@ -6042,6 +6047,10 @@ static PyType_Spec HeapGcCType_spec = { HeapGcCType_slots }; +PyDoc_STRVAR(heapctype__doc__, +"A heap type without GC, but with overridden dealloc.\n\n" +"The 'value' attribute is set to 10 in __init__."); + static void heapctype_dealloc(HeapCTypeObject *self) { @@ -6054,6 +6063,7 @@ static PyType_Slot HeapCType_slots[] = { {Py_tp_init, heapctype_init}, {Py_tp_members, heapctype_members}, {Py_tp_dealloc, heapctype_dealloc}, + {Py_tp_doc, heapctype__doc__}, {0, 0}, }; @@ -6065,6 +6075,10 @@ static PyType_Spec HeapCType_spec = { HeapCType_slots }; +PyDoc_STRVAR(heapctypesubclass__doc__, +"Subclass of HeapCType, without GC.\n\n" +"__init__ sets the 'value' attribute to 10 and 'value2' to 20."); + typedef struct { HeapCTypeObject base; int value2; @@ -6090,6 +6104,7 @@ static struct PyMemberDef heapctypesubclass_members[] = { static PyType_Slot HeapCTypeSubclass_slots[] = { {Py_tp_init, heapctypesubclass_init}, {Py_tp_members, heapctypesubclass_members}, + {Py_tp_doc, heapctypesubclass__doc__}, {0, 0}, }; @@ -6101,6 +6116,11 @@ static PyType_Spec HeapCTypeSubclass_spec = { HeapCTypeSubclass_slots }; +PyDoc_STRVAR(heapctypesubclasswithfinalizer__doc__, +"Subclass of HeapCType with a finalizer that reassigns __class__.\n\n" +"__class__ is set to plain HeapCTypeSubclass during finalization.\n" +"__init__ sets the 'value' attribute to 10 and 'value2' to 20."); + static int heapctypesubclasswithfinalizer_init(PyObject *self, PyObject *args, PyObject *kwargs) { @@ -6152,6 +6172,7 @@ static PyType_Slot HeapCTypeSubclassWithFinalizer_slots[] = { {Py_tp_init, heapctypesubclasswithfinalizer_init}, {Py_tp_members, heapctypesubclass_members}, {Py_tp_finalize, heapctypesubclasswithfinalizer_finalize}, + {Py_tp_doc, heapctypesubclasswithfinalizer__doc__}, {0, 0}, }; From cf82442e19dd491f7bd952e0c03df568b542c7c5 Mon Sep 17 00:00:00 2001 From: Eddie Elizondo Date: Wed, 11 Sep 2019 00:33:04 -0400 Subject: [PATCH 10/10] Remove merge artifacts --- Modules/_testcapimodule.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index bb24185d92e623..21061a2171a052 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -6353,7 +6353,7 @@ static PyType_Spec HeapCTypeSubclassWithFinalizer_spec = { Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_FINALIZE, HeapCTypeSubclassWithFinalizer_slots }; -======= + static PyMethodDef meth_instance_methods[] = { {"meth_varargs", meth_varargs, METH_VARARGS}, {"meth_varargs_keywords", (PyCFunction)(void(*)(void))meth_varargs_keywords, METH_VARARGS|METH_KEYWORDS},