bpo-40217: Ensure Py_VISIT(Py_TYPE(self)) is always called for PyType_FromSpec types (reverts GH-19414) (GH-20264)

Heap types now always visit the type in tp_traverse. See added docs for details.

This reverts commit 0169d30.

Automerge-Triggered-By: @encukou

Author: pablogsal

Doc/c-api/typeobj.rst

@@ -1223,11 +1223,25 @@ and :c:type:`PyType_Type` effectively act as defaults.)
        but the instance has no strong reference to the elements inside it, as they
        are allowed to be removed even if the instance is still alive).
 
-
    Note that :c:func:`Py_VISIT` requires the *visit* and *arg* parameters to
    :c:func:`local_traverse` to have these specific names; don't name them just
    anything.
 
+   Heap-allocated types (:const:`Py_TPFLAGS_HEAPTYPE`, such as those created
+   with :c:func:`PyType_FromSpec` and similar APIs) hold a reference to their
+   type. Their traversal function must therefore either visit
+   :c:func:`Py_TYPE(self) <Py_TYPE>`, or delegate this responsibility by
+   calling ``tp_traverse`` of another heap-allocated type (such as a
+   heap-allocated superclass).
+   If they do not, the type object may not be garbage-collected.
+
+   .. versionchanged:: 3.9
+
+      Heap-allocated types are expected to visit ``Py_TYPE(self)`` in
+      ``tp_traverse``.  In earlier versions of Python, due to
+      `bug 40217 <https://bugs.python.org/issue40217>`_, doing this
+      may lead to crashes in subclasses.
+
    **Inheritance:**
 
    Group: :const:`Py_TPFLAGS_HAVE_GC`, :attr:`tp_traverse`, :attr:`tp_clear`

Doc/whatsnew/3.9.rst

@@ -933,6 +933,55 @@ Changes in the Python API
   (Contributed by Inada Naoki in :issue:`34538`.)
 
 
+Changes in the C API
+--------------------
+
+* Instances of heap-allocated types (such as those created with
+  :c:func:`PyType_FromSpec` and similar APIs) hold a reference to their type
+  object since Python 3.8. As indicated in the "Changes in the C API" of Python
+  3.8, for the vast majority of cases, there should be no side effect but for
+  types that have a custom :c:member:`~PyTypeObject.tp_traverse` function,
+  ensure that all custom ``tp_traverse`` functions of heap-allocated types
+  visit the object's type.
+
+    Example:
+
+    .. code-block:: c
+
+        int
+        foo_traverse(foo_struct *self, visitproc visit, void *arg) {
+        // Rest of the traverse function
+        #if PY_VERSION_HEX >= 0x03090000
+            // This was not needed before Python 3.9 (Python issue 35810 and 40217)
+            Py_VISIT(Py_TYPE(self));
+        #endif
+        }
+
+  If your traverse function delegates to ``tp_traverse`` of its base class
+  (or another type), ensure that ``Py_TYPE(self)`` is visited only once.
+  Note that only heap types are expected to visit the type in ``tp_traverse``.
+
+    For example, if your ``tp_traverse`` function includes:
+
+    .. code-block:: c
+
+        base->tp_traverse(self, visit, arg)
+
+    then add:
+
+    .. code-block:: c
+
+        #if PY_VERSION_HEX >= 0x03090000
+            // This was not needed before Python 3.9 (Python issue 35810 and 40217)
+            if (base->tp_flags & Py_TPFLAGS_HEAPTYPE) {
+                // a heap type's tp_traverse already visited Py_TYPE(self)
+            } else {
+                Py_VISIT(Py_TYPE(self));
+            }
+        #else
+
+  (See :issue:`35810` and :issue:`40217` for more information.)
+
 CPython bytecode changes
 ------------------------
 

Misc/NEWS.d/next/Core and Builtins/2020-05-23-01-15-51.bpo-40217.jZsHTc.rst

@@ -0,0 +1,4 @@
+Instances of types created with :c:func:`PyType_FromSpecWithBases` will no
+longer automatically visit their class object when traversing references in
+the garbage collector. The user is expected to manually visit the object's
+class. Patch by Pablo Galindo.

Modules/_abc.c

@@ -46,6 +46,7 @@ typedef struct {
 static int
 abc_data_traverse(_abc_data *self, visitproc visit, void *arg)
 {
+    Py_VISIT(Py_TYPE(self));
     Py_VISIT(self->_abc_registry);
     Py_VISIT(self->_abc_cache);
     Py_VISIT(self->_abc_negative_cache);

Modules/_curses_panel.c

@@ -39,6 +39,7 @@ _curses_panel_clear(PyObject *m)
 static int
 _curses_panel_traverse(PyObject *m, visitproc visit, void *arg)
 {
+    Py_VISIT(Py_TYPE(m));
     Py_VISIT(get_curses_panelstate(m)->PyCursesError);
     return 0;
 }

Modules/_json.c

@@ -647,6 +647,7 @@ scanner_dealloc(PyObject *self)
 static int
 scanner_traverse(PyScannerObject *self, visitproc visit, void *arg)
 {
+    Py_VISIT(Py_TYPE(self));
     Py_VISIT(self->object_hook);
     Py_VISIT(self->object_pairs_hook);
     Py_VISIT(self->parse_float);
@@ -1745,6 +1746,7 @@ encoder_dealloc(PyObject *self)
 static int
 encoder_traverse(PyEncoderObject *self, visitproc visit, void *arg)
 {
+    Py_VISIT(Py_TYPE(self));
     Py_VISIT(self->markers);
     Py_VISIT(self->defaultfn);
     Py_VISIT(self->encoder);

Modules/_struct.c

@@ -1646,6 +1646,7 @@ unpackiter_dealloc(unpackiterobject *self)
 static int
 unpackiter_traverse(unpackiterobject *self, visitproc visit, void *arg)
 {
+    Py_VISIT(Py_TYPE(self));
     Py_VISIT(self->so);
     Py_VISIT(self->buf.obj);
     return 0;

Modules/xxlimited.c

@@ -43,6 +43,7 @@ newXxoObject(PyObject *arg)
 static int
 Xxo_traverse(XxoObject *self, visitproc visit, void *arg)
 {
+    Py_VISIT(Py_TYPE(self));
     Py_VISIT(self->x_attr);
     return 0;
 }

Objects/structseq.c

@@ -70,6 +70,9 @@ PyStructSequence_GetItem(PyObject* op, Py_ssize_t i)
 static int
 structseq_traverse(PyStructSequence *obj, visitproc visit, void *arg)
 {
+    if (Py_TYPE(obj)->tp_flags & Py_TPFLAGS_HEAPTYPE) {
+        Py_VISIT(Py_TYPE(obj));
+    }
     Py_ssize_t i, size;
     size = REAL_SIZE(obj);
     for (i = 0; i < size; ++i) {

Objects/typeobject.c

@@ -1039,42 +1039,6 @@ type_call(PyTypeObject *type, PyObject *args, PyObject *kwds)
     return obj;
 }
 
-PyObject *
-PyType_FromSpec_Alloc(PyTypeObject *type, Py_ssize_t nitems)
-{
-    PyObject *obj;
-    const size_t size = _Py_SIZE_ROUND_UP(
-            _PyObject_VAR_SIZE(type, nitems+1) + sizeof(traverseproc),
-            SIZEOF_VOID_P);
-    /* note that we need to add one, for the sentinel and space for the
-       provided tp-traverse: See bpo-40217 for more details */
-
-    if (PyType_IS_GC(type)) {
-        obj = _PyObject_GC_Malloc(size);
-    }
-    else {
-        obj = (PyObject *)PyObject_MALLOC(size);
-    }
-
-    if (obj == NULL) {
-        return PyErr_NoMemory();
-    }
-
-    memset(obj, '\0', size);
-
-    if (type->tp_itemsize == 0) {
-        (void)PyObject_INIT(obj, type);
-    }
-    else {
-        (void) PyObject_INIT_VAR((PyVarObject *)obj, type, nitems);
-    }
-
-    if (PyType_IS_GC(type)) {
-        _PyObject_GC_TRACK(obj);
-    }
-    return obj;
-}
-
 PyObject *
 PyType_GenericAlloc(PyTypeObject *type, Py_ssize_t nitems)
 {
@@ -1164,11 +1128,16 @@ subtype_traverse(PyObject *self, visitproc visit, void *arg)
             Py_VISIT(*dictptr);
     }
 
-    if (type->tp_flags & Py_TPFLAGS_HEAPTYPE)
+    if (type->tp_flags & Py_TPFLAGS_HEAPTYPE
+        && (!basetraverse || !(base->tp_flags & Py_TPFLAGS_HEAPTYPE))) {
         /* For a heaptype, the instances count as references
            to the type.          Traverse the type so the collector
-           can find cycles involving this link. */
+           can find cycles involving this link.
+           Skip this visit if basetraverse belongs to a heap type: in that
+           case, basetraverse will visit the type when we call it later.
+           */
         Py_VISIT(type);
+    }
 
     if (basetraverse)
         return basetraverse(self, visit, arg);
@@ -2910,36 +2879,6 @@ static const short slotoffsets[] = {
 #include "typeslots.inc"
 };
 
-static int
-PyType_FromSpec_tp_traverse(PyObject *self, visitproc visit, void *arg)
-{
-    PyTypeObject *parent = Py_TYPE(self);
-
-    // Only a instance of a type that is directly created by
-    // PyType_FromSpec (not subclasses) must visit its parent.
-    if (parent->tp_traverse == PyType_FromSpec_tp_traverse) {
-        Py_VISIT(parent);
-    }
-
-    // Search for the original type that was created using PyType_FromSpec
-    PyTypeObject *base;
-    base = parent;
-    while (base->tp_traverse != PyType_FromSpec_tp_traverse) {
-        base = base->tp_base;
-        assert(base);
-    }
-
-    // Extract the user defined traverse function that we placed at the end
-    // of the type and call it.
-    size_t size = Py_SIZE(base);
-    size_t _offset = _PyObject_VAR_SIZE(&PyType_Type, size+1);
-    traverseproc fun = *(traverseproc*)((char*)base + _offset);
-    if (fun == NULL) {
-        return 0;
-    }
-    return fun(self, visit, arg);
-}
-
 PyObject *
 PyType_FromSpecWithBases(PyType_Spec *spec, PyObject *bases)
 {
@@ -2985,7 +2924,7 @@ PyType_FromModuleAndSpec(PyObject *module, PyType_Spec *spec, PyObject *bases)
         }
     }
 
-    res = (PyHeapTypeObject*)PyType_FromSpec_Alloc(&PyType_Type, nmembers);
+    res = (PyHeapTypeObject*)PyType_GenericAlloc(&PyType_Type, nmembers);
     if (res == NULL)
         return NULL;
     res_start = (char*)res;
@@ -3093,30 +3032,6 @@ PyType_FromModuleAndSpec(PyObject *module, PyType_Spec *spec, PyObject *bases)
             memcpy(PyHeapType_GET_MEMBERS(res), slot->pfunc, len);
             type->tp_members = PyHeapType_GET_MEMBERS(res);
         }
-        else if (slot->slot == Py_tp_traverse) {
-
-           /* Types created by PyType_FromSpec own a strong reference to their
-            * type, but this was added in Python 3.8. The tp_traverse function
-            * needs to call Py_VISIT on the type but all existing traverse
-            * functions cannot be updated (especially the ones from existing user
-            * functions) so we need to provide a tp_traverse that manually calls
-            * Py_VISIT(Py_TYPE(self)) and then call the provided tp_traverse. In
-            * this way, user functions do not need to be updated, preserve
-            * backwards compatibility.
-            *
-            * We store the user-provided traverse function at the end of the type
-            * (we have allocated space for it) so we can call it from our
-            * PyType_FromSpec_tp_traverse wrapper.
-            *
-            * Check bpo-40217 for more information and rationale about this issue.
-            *
-            * */
-
-            type->tp_traverse = PyType_FromSpec_tp_traverse;
-            size_t _offset = _PyObject_VAR_SIZE(&PyType_Type, nmembers+1);
-            traverseproc *user_traverse = (traverseproc*)((char*)type + _offset);
-            *user_traverse = slot->pfunc;
-        }
         else {
             /* Copy other slots directly */
             *(void**)(res_start + slotoffsets[slot->slot]) = slot->pfunc;