GH-119462: Enforce invariants of type versioning (GH-120731)

* Remove uses of Py_TPFLAGS_VALID_VERSION_TAG

Doc/c-api/typeobj.rst

@@ -1328,8 +1328,8 @@ and :c:data:`PyType_Type` effectively act as defaults.)
       To indicate that a class has changed call :c:func:`PyType_Modified`
 
       .. warning::
-         This flag is present in header files, but is an internal feature and should
+         This flag is present in header files, but is not be used.
-         not be used. It will be removed in a future version of CPython
+         It will be removed in a future version of CPython
 
 
 .. c:member:: const char* PyTypeObject.tp_doc

Include/object.h

@@ -566,7 +566,7 @@ given type object has a specified feature.
 /* Objects behave like an unbound method */
 #define Py_TPFLAGS_METHOD_DESCRIPTOR (1UL << 17)
 
-/* Object has up-to-date type attribute cache */
+/* Unused. Legacy flag */
 #define Py_TPFLAGS_VALID_VERSION_TAG  (1UL << 19)
 
 /* Type is abstract and cannot be instantiated */

Lib/test/test_type_cache.py

@@ -93,6 +93,21 @@ class TypeCacheTests(unittest.TestCase):
         new_version = type_get_version(C)
         self.assertEqual(new_version, 0)
 
+    def test_119462(self):
+
+        class Holder:
+            value = None
+
+            @classmethod
+            def set_value(cls):
+                cls.value = object()
+
+        class HolderSub(Holder):
+            pass
+
+        for _ in range(1050):
+            Holder.set_value()
+            HolderSub.value
 
 @support.cpython_only
 @requires_specialization
@@ -106,8 +121,10 @@ class TypeCacheWithSpecializationTests(unittest.TestCase):
         if type_get_version(type_) == 0:
             self.skipTest("Could not assign valid type version")
 
-    def _assign_and_check_version_0(self, user_type):
+    def _no_more_versions(self, user_type):
         type_modified(user_type)
+        for _ in range(1001):
+            type_assign_specific_version_unsafe(user_type, 1000_000_000)
         type_assign_specific_version_unsafe(user_type, 0)
         self.assertEqual(type_get_version(user_type), 0)
 
@@ -136,7 +153,7 @@ class TypeCacheWithSpecializationTests(unittest.TestCase):
         self._check_specialization(load_foo_1, A, "LOAD_ATTR", should_specialize=True)
         del load_foo_1
 
-        self._assign_and_check_version_0(A)
+        self._no_more_versions(A)
 
         def load_foo_2(type_):
             return type_.foo
@@ -187,7 +204,7 @@ class TypeCacheWithSpecializationTests(unittest.TestCase):
         self._check_specialization(load_x_1, G(), "LOAD_ATTR", should_specialize=True)
         del load_x_1
 
-        self._assign_and_check_version_0(G)
+        self._no_more_versions(G)
 
         def load_x_2(instance):
             instance.x
@@ -206,7 +223,7 @@ class TypeCacheWithSpecializationTests(unittest.TestCase):
         self._check_specialization(store_bar_1, B(), "STORE_ATTR", should_specialize=True)
         del store_bar_1
 
-        self._assign_and_check_version_0(B)
+        self._no_more_versions(B)
 
         def store_bar_2(type_):
             type_.bar = 10
@@ -226,7 +243,7 @@ class TypeCacheWithSpecializationTests(unittest.TestCase):
         self._check_specialization(call_class_1, F, "CALL", should_specialize=True)
         del call_class_1
 
-        self._assign_and_check_version_0(F)
+        self._no_more_versions(F)
 
         def call_class_2(type_):
             type_()
@@ -245,7 +262,7 @@ class TypeCacheWithSpecializationTests(unittest.TestCase):
         self._check_specialization(to_bool_1, H(), "TO_BOOL", should_specialize=True)
         del to_bool_1
 
-        self._assign_and_check_version_0(H)
+        self._no_more_versions(H)
 
         def to_bool_2(instance):
             not instance

Misc/NEWS.d/next/Core and Builtins/2024-06-19-11-10-50.gh-issue-119462.DpcqSe.rst

@@ -0,0 +1,4 @@
+Make sure that invariants of type versioning are maintained:
+* Superclasses always have their version number assigned before subclasses
+* The version tag is always zero if the tag is not valid.
+* The version tag is always non-if the tag is valid.

Modules/_testinternalcapi.c

@@ -2014,7 +2014,6 @@ type_assign_specific_version_unsafe(PyObject *self, PyObject *args)
     }
     assert(!PyType_HasFeature(type, Py_TPFLAGS_IMMUTABLETYPE));
     _PyType_SetVersion(type, version);
-    type->tp_flags |= Py_TPFLAGS_VALID_VERSION_TAG;
     Py_RETURN_NONE;
 }
 

Modules/pyexpat.c

@@ -1651,7 +1651,7 @@ PyDoc_STRVAR(pyexpat_module_documentation,
 static int init_handler_descrs(pyexpat_state *state)
 {
     int i;
-    assert(!PyType_HasFeature(state->xml_parse_type, Py_TPFLAGS_VALID_VERSION_TAG));
+    assert(state->xml_parse_type->tp_version_tag == 0);
     for (i = 0; handler_info[i].name != NULL; i++) {
         struct HandlerInfo *hi = &handler_info[i];
         hi->getset.name = hi->name;

Objects/typeobject.c

@@ -974,43 +974,37 @@ PyType_Unwatch(int watcher_id, PyObject* obj)
     return 0;
 }
 
-#ifdef Py_GIL_DISABLED
-
 static void
-type_modification_starting_unlocked(PyTypeObject *type)
+set_version_unlocked(PyTypeObject *tp, unsigned int version)
 {
     ASSERT_TYPE_LOCK_HELD();
-
+#ifndef Py_GIL_DISABLED
-    /* Clear version tags on all types, but leave the valid
+    PyInterpreterState *interp = _PyInterpreterState_GET();
-       version tag intact.  This prepares for a modification so that
+    // lookup the old version and set to null
-       any concurrent readers of the type cache will not see invalid
+    if (tp->tp_version_tag != 0) {
-       values.
+        PyTypeObject **slot =
-     */
+            interp->types.type_version_cache
-    if (!_PyType_HasFeature(type, Py_TPFLAGS_VALID_VERSION_TAG)) {
+            + (tp->tp_version_tag % TYPE_VERSION_CACHE_SIZE);
-        return;
+        *slot = NULL;
     }
-
+    if (version) {
-    PyObject *subclasses = lookup_tp_subclasses(type);
+        tp->tp_versions_used++;
-    if (subclasses != NULL) {
-        assert(PyDict_CheckExact(subclasses));
-
-        Py_ssize_t i = 0;
-        PyObject *ref;
-        while (PyDict_Next(subclasses, &i, NULL, &ref)) {
-            PyTypeObject *subclass = type_from_ref(ref);
-            if (subclass == NULL) {
-                continue;
     }
-            type_modification_starting_unlocked(subclass);
+#else
-            Py_DECREF(subclass);
+    if (version) {
+        _Py_atomic_add_uint16(&tp->tp_versions_used, 1);
     }
-    }
-
-    /* 0 is not a valid version tag */
-    _PyType_SetVersion(type, 0);
-}
-
 #endif
+    FT_ATOMIC_STORE_UINT32_RELAXED(tp->tp_version_tag, version);
+#ifndef Py_GIL_DISABLED
+    if (version != 0) {
+        PyTypeObject **slot =
+            interp->types.type_version_cache
+            + (version % TYPE_VERSION_CACHE_SIZE);
+        *slot = tp;
+    }
+#endif
+}
 
 static void
 type_modified_unlocked(PyTypeObject *type)
@@ -1021,16 +1015,16 @@ type_modified_unlocked(PyTypeObject *type)
 
        Invariants:
 
-       - before Py_TPFLAGS_VALID_VERSION_TAG can be set on a type,
+       - before tp_version_tag can be set on a type,
          it must first be set on all super types.
 
-       This function clears the Py_TPFLAGS_VALID_VERSION_TAG of a
+       This function clears the tp_version_tag of a
        type (so it must first clear it on all subclasses).  The
-       tp_version_tag value is meaningless unless this flag is set.
+       tp_version_tag value is meaningless when equal to zero.
        We don't assign new version tags eagerly, but only as
        needed.
      */
-    if (!_PyType_HasFeature(type, Py_TPFLAGS_VALID_VERSION_TAG)) {
+    if (type->tp_version_tag == 0) {
         return;
     }
 
@@ -1070,8 +1064,7 @@ type_modified_unlocked(PyTypeObject *type)
         }
     }
 
-    type->tp_flags &= ~Py_TPFLAGS_VALID_VERSION_TAG;
+    set_version_unlocked(type, 0); /* 0 is not a valid version tag */
-    _PyType_SetVersion(type, 0); /* 0 is not a valid version tag */
     if (PyType_HasFeature(type, Py_TPFLAGS_HEAPTYPE)) {
         // This field *must* be invalidated if the type is modified (see the
         // comment on struct _specialization_cache):
@@ -1083,7 +1076,7 @@ void
 PyType_Modified(PyTypeObject *type)
 {
     // Quick check without the lock held
-    if (!_PyType_HasFeature(type, Py_TPFLAGS_VALID_VERSION_TAG)) {
+    if (type->tp_version_tag == 0) {
         return;
     }
 
@@ -1147,8 +1140,7 @@ type_mro_modified(PyTypeObject *type, PyObject *bases) {
 
  clear:
     assert(!(type->tp_flags & _Py_TPFLAGS_STATIC_BUILTIN));
-    type->tp_flags &= ~Py_TPFLAGS_VALID_VERSION_TAG;
+    set_version_unlocked(type, 0); /* 0 is not a valid version tag */
-    _PyType_SetVersion(type, 0); /* 0 is not a valid version tag */
     if (PyType_HasFeature(type, Py_TPFLAGS_HEAPTYPE)) {
         // This field *must* be invalidated if the type is modified (see the
         // comment on struct _specialization_cache):
@@ -1168,25 +1160,10 @@ This is similar to func_version_cache.
 void
 _PyType_SetVersion(PyTypeObject *tp, unsigned int version)
 {
-#ifndef Py_GIL_DISABLED
+
-    PyInterpreterState *interp = _PyInterpreterState_GET();
+    BEGIN_TYPE_LOCK()
-    // lookup the old version and set to null
+    set_version_unlocked(tp, version);
-    if (tp->tp_version_tag != 0) {
+    END_TYPE_LOCK()
-        PyTypeObject **slot =
-            interp->types.type_version_cache
-            + (tp->tp_version_tag % TYPE_VERSION_CACHE_SIZE);
-        *slot = NULL;
-    }
-#endif
-    FT_ATOMIC_STORE_UINT32_RELAXED(tp->tp_version_tag, version);
-#ifndef Py_GIL_DISABLED
-    if (version != 0) {
-        PyTypeObject **slot =
-            interp->types.type_version_cache
-            + (version % TYPE_VERSION_CACHE_SIZE);
-        *slot = tp;
-    }
-#endif
 }
 
 PyTypeObject *
@@ -1221,12 +1198,11 @@ assign_version_tag(PyInterpreterState *interp, PyTypeObject *type)
 {
     ASSERT_TYPE_LOCK_HELD();
 
-    /* Ensure that the tp_version_tag is valid and set
+    /* Ensure that the tp_version_tag is valid.
-       Py_TPFLAGS_VALID_VERSION_TAG.  To respect the invariant, this
+     * To respect the invariant, this must first be done on all super classes.
-       must first be done on all super classes.  Return 0 if this
+     * Return 0 if this cannot be done, 1 if tp_version_tag is set.
-       cannot be done, 1 if Py_TPFLAGS_VALID_VERSION_TAG.
     */
-    if (_PyType_HasFeature(type, Py_TPFLAGS_VALID_VERSION_TAG)) {
+    if (type->tp_version_tag != 0) {
         return 1;
     }
     if (!_PyType_HasFeature(type, Py_TPFLAGS_READY)) {
@@ -1235,14 +1211,22 @@ assign_version_tag(PyInterpreterState *interp, PyTypeObject *type)
     if (type->tp_versions_used >= MAX_VERSIONS_PER_CLASS) {
         return 0;
     }
-    type->tp_versions_used++;
+
+    PyObject *bases = lookup_tp_bases(type);
+    Py_ssize_t n = PyTuple_GET_SIZE(bases);
+    for (Py_ssize_t i = 0; i < n; i++) {
+        PyObject *b = PyTuple_GET_ITEM(bases, i);
+        if (!assign_version_tag(interp, _PyType_CAST(b))) {
+            return 0;
+        }
+    }
     if (type->tp_flags & Py_TPFLAGS_IMMUTABLETYPE) {
         /* static types */
         if (NEXT_GLOBAL_VERSION_TAG > _Py_MAX_GLOBAL_TYPE_VERSION_TAG) {
             /* We have run out of version numbers */
             return 0;
         }
-        _PyType_SetVersion(type, NEXT_GLOBAL_VERSION_TAG++);
+        set_version_unlocked(type, NEXT_GLOBAL_VERSION_TAG++);
         assert (type->tp_version_tag <= _Py_MAX_GLOBAL_TYPE_VERSION_TAG);
     }
     else {
@@ -1251,18 +1235,9 @@ assign_version_tag(PyInterpreterState *interp, PyTypeObject *type)
             /* We have run out of version numbers */
             return 0;
         }
-        _PyType_SetVersion(type, NEXT_VERSION_TAG(interp)++);
+        set_version_unlocked(type, NEXT_VERSION_TAG(interp)++);
         assert (type->tp_version_tag != 0);
     }
-
-    PyObject *bases = lookup_tp_bases(type);
-    Py_ssize_t n = PyTuple_GET_SIZE(bases);
-    for (Py_ssize_t i = 0; i < n; i++) {
-        PyObject *b = PyTuple_GET_ITEM(bases, i);
-        if (!assign_version_tag(interp, _PyType_CAST(b)))
-            return 0;
-    }
-    type->tp_flags |= Py_TPFLAGS_VALID_VERSION_TAG;
     return 1;
 }
 
@@ -3318,7 +3293,7 @@ mro_internal_unlocked(PyTypeObject *type, int initial, PyObject **p_old_mro)
     else {
         /* For static builtin types, this is only called during init
            before the method cache has been populated. */
-        assert(_PyType_HasFeature(type, Py_TPFLAGS_VALID_VERSION_TAG));
+        assert(type->tp_version_tag);
     }
 
     if (p_old_mro != NULL)
@@ -5463,7 +5438,7 @@ _PyType_LookupRef(PyTypeObject *type, PyObject *name)
 #else
     if (entry->version == type->tp_version_tag &&
         entry->name == name) {
-        assert(_PyType_HasFeature(type, Py_TPFLAGS_VALID_VERSION_TAG));
+        assert(type->tp_version_tag);
         OBJECT_STAT_INC_COND(type_cache_hits, !is_dunder_name(name));
         OBJECT_STAT_INC_COND(type_cache_dunder_hits, is_dunder_name(name));
         Py_XINCREF(entry->value);
@@ -5486,7 +5461,6 @@ _PyType_LookupRef(PyTypeObject *type, PyObject *name)
     if (MCACHE_CACHEABLE_NAME(name)) {
         has_version = assign_version_tag(interp, type);
         version = type->tp_version_tag;
-        assert(!has_version || _PyType_HasFeature(type, Py_TPFLAGS_VALID_VERSION_TAG));
     }
     END_TYPE_LOCK()
 
@@ -5770,16 +5744,6 @@ type_setattro(PyObject *self, PyObject *name, PyObject *value)
         return -1;
     }
 
-#ifdef Py_GIL_DISABLED
-    // In free-threaded builds readers can race with the lock-free portion
-    // of the type cache and the assignment into the dict.  We clear all of the
-    // versions initially so no readers will succeed in the lock-free case.
-    // They'll then block on the type lock until the update below is done.
-    type_modification_starting_unlocked(type);
-#endif
-
-    res = _PyDict_SetItem_LockHeld((PyDictObject *)dict, name, value);
-
     /* Clear the VALID_VERSION flag of 'type' and all its
         subclasses.  This could possibly be unified with the
         update_subclasses() recursion in update_slot(), but carefully:
@@ -5787,6 +5751,8 @@ type_setattro(PyObject *self, PyObject *name, PyObject *value)
         recursing into subclasses. */
     type_modified_unlocked(type);
 
+    res = _PyDict_SetItem_LockHeld((PyDictObject *)dict, name, value);
+
     if (res == 0) {
         if (is_dunder_name(name)) {
             res = update_slot(type, name);
@@ -5898,7 +5864,6 @@ fini_static_type(PyInterpreterState *interp, PyTypeObject *type,
 
     if (final) {
         type->tp_flags &= ~Py_TPFLAGS_READY;
-        type->tp_flags &= ~Py_TPFLAGS_VALID_VERSION_TAG;
         _PyType_SetVersion(type, 0);
     }
 
@@ -8516,12 +8481,11 @@ init_static_type(PyInterpreterState *interp, PyTypeObject *self,
 
         assert(NEXT_GLOBAL_VERSION_TAG <= _Py_MAX_GLOBAL_TYPE_VERSION_TAG);
         _PyType_SetVersion(self, NEXT_GLOBAL_VERSION_TAG++);
-        self->tp_flags |= Py_TPFLAGS_VALID_VERSION_TAG;
     }
     else {
         assert(!initial);
         assert(self->tp_flags & _Py_TPFLAGS_STATIC_BUILTIN);
-        assert(self->tp_flags & Py_TPFLAGS_VALID_VERSION_TAG);
+        assert(self->tp_version_tag != 0);
     }
 
     managed_static_type_state_init(interp, self, isbuiltin, initial);