diff --git a/Doc/c-api/typeobj.rst b/Doc/c-api/typeobj.rst index a6a2c437ea4..c9ef076c78c 100644 --- a/Doc/c-api/typeobj.rst +++ b/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 - not be used. It will be removed in a future version of CPython + This flag is present in header files, but is not be used. + It will be removed in a future version of CPython .. c:member:: const char* PyTypeObject.tp_doc diff --git a/Include/object.h b/Include/object.h index 7aaa8da8e8d..05d90567736 100644 --- a/Include/object.h +++ b/Include/object.h @@ -703,7 +703,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 */ diff --git a/Lib/test/test_type_cache.py b/Lib/test/test_type_cache.py index e90e315c808..09c68d408cc 100644 --- a/Lib/test/test_type_cache.py +++ b/Lib/test/test_type_cache.py @@ -92,6 +92,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 @@ -105,8 +120,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) @@ -135,7 +152,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 @@ -186,7 +203,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 @@ -205,7 +222,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 @@ -225,7 +242,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_() @@ -244,7 +261,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 diff --git a/Misc/NEWS.d/next/Core and Builtins/2024-06-19-11-10-50.gh-issue-119462.DpcqSe.rst b/Misc/NEWS.d/next/Core and Builtins/2024-06-19-11-10-50.gh-issue-119462.DpcqSe.rst new file mode 100644 index 00000000000..e07c2fd227c --- /dev/null +++ b/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-zero if the tag is valid. diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index b58c1726062..8b40821dc6a 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -2415,7 +2415,7 @@ type_assign_specific_version_unsafe(PyObject *self, PyObject *args) } assert(!PyType_HasFeature(type, Py_TPFLAGS_IMMUTABLETYPE)); type->tp_version_tag = version; - type->tp_flags |= Py_TPFLAGS_VALID_VERSION_TAG; + type->tp_versions_used++; Py_RETURN_NONE; } diff --git a/Modules/pyexpat.c b/Modules/pyexpat.c index f67d480f19d..fafb5ce4106 100644 --- a/Modules/pyexpat.c +++ b/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; diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 5bf2bc24f31..7c11d87b80f 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -973,43 +973,21 @@ 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(); - - /* Clear version tags on all types, but leave the valid - version tag intact. This prepares for a modification so that - any concurrent readers of the type cache will not see invalid - values. - */ - if (!_PyType_HasFeature(type, Py_TPFLAGS_VALID_VERSION_TAG)) { - return; +#ifndef Py_GIL_DISABLED + if (version) { + tp->tp_versions_used++; } - - PyObject *subclasses = lookup_tp_subclasses(type); - 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); - Py_DECREF(subclass); - } +#else + if (version) { + _Py_atomic_add_uint16(&tp->tp_versions_used, 1); } - - /* 0 is not a valid version tag */ - _Py_atomic_store_uint32_release(&type->tp_version_tag, 0); -} - #endif + FT_ATOMIC_STORE_UINT32_RELAXED(tp->tp_version_tag, version); +} static void type_modified_unlocked(PyTypeObject *type) @@ -1020,16 +998,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; } @@ -1069,8 +1047,7 @@ type_modified_unlocked(PyTypeObject *type) } } - type->tp_flags &= ~Py_TPFLAGS_VALID_VERSION_TAG; - FT_ATOMIC_STORE_UINT32_RELAXED(type->tp_version_tag, 0); /* 0 is not a valid version tag */ + set_version_unlocked(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): @@ -1082,7 +1059,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; } @@ -1146,8 +1123,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; - FT_ATOMIC_STORE_UINT32_RELAXED(type->tp_version_tag, 0); /* 0 is not a valid version tag */ + set_version_unlocked(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): @@ -1162,12 +1138,11 @@ assign_version_tag(PyInterpreterState *interp, PyTypeObject *type) { ASSERT_TYPE_LOCK_HELD(); - /* Ensure that the tp_version_tag is valid and set - Py_TPFLAGS_VALID_VERSION_TAG. To respect the invariant, this - must first be done on all super classes. Return 0 if this - cannot be done, 1 if Py_TPFLAGS_VALID_VERSION_TAG. + /* Ensure that the tp_version_tag is valid. + * To respect the invariant, this must first be done on all super classes. + * Return 0 if this cannot be done, 1 if tp_version_tag is set. */ - if (_PyType_HasFeature(type, Py_TPFLAGS_VALID_VERSION_TAG)) { + if (type->tp_version_tag != 0) { return 1; } if (!_PyType_HasFeature(type, Py_TPFLAGS_READY)) { @@ -1176,15 +1151,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; } - FT_ATOMIC_STORE_UINT32_RELAXED(type->tp_version_tag, - NEXT_GLOBAL_VERSION_TAG++); + set_version_unlocked(type, NEXT_GLOBAL_VERSION_TAG++); assert (type->tp_version_tag <= _Py_MAX_GLOBAL_TYPE_VERSION_TAG); } else { @@ -1193,19 +1175,9 @@ assign_version_tag(PyInterpreterState *interp, PyTypeObject *type) /* We have run out of version numbers */ return 0; } - FT_ATOMIC_STORE_UINT32_RELAXED(type->tp_version_tag, - 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; } @@ -3126,7 +3098,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) @@ -5275,7 +5247,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); @@ -5298,7 +5270,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() @@ -5582,16 +5553,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: @@ -5599,6 +5560,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); @@ -5710,7 +5673,6 @@ fini_static_type(PyInterpreterState *interp, PyTypeObject *type, if (final) { type->tp_flags &= ~Py_TPFLAGS_READY; - type->tp_flags &= ~Py_TPFLAGS_VALID_VERSION_TAG; type->tp_version_tag = 0; } @@ -8329,12 +8291,11 @@ init_static_type(PyInterpreterState *interp, PyTypeObject *self, assert(NEXT_GLOBAL_VERSION_TAG <= _Py_MAX_GLOBAL_TYPE_VERSION_TAG); self->tp_version_tag = 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);