From 00257c746c447a2e026b5a2a618f0e033fb90111 Mon Sep 17 00:00:00 2001 From: Mark Shannon Date: Wed, 19 Jun 2024 17:38:45 +0100 Subject: [PATCH] GH-119462: Enforce invariants of type versioning (GH-120731) * Remove uses of Py_TPFLAGS_VALID_VERSION_TAG --- Doc/c-api/typeobj.rst | 4 +- Include/object.h | 2 +- Lib/test/test_type_cache.py | 29 +++- ...-06-19-11-10-50.gh-issue-119462.DpcqSe.rst | 4 + Modules/_testinternalcapi.c | 1 - Modules/pyexpat.c | 2 +- Objects/typeobject.c | 148 +++++++----------- 7 files changed, 87 insertions(+), 103 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2024-06-19-11-10-50.gh-issue-119462.DpcqSe.rst 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 795d4fb2584..ed39e7dc877 100644 --- a/Include/object.h +++ b/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 */ diff --git a/Lib/test/test_type_cache.py b/Lib/test/test_type_cache.py index edaf076707a..89632a3abeb 100644 --- a/Lib/test/test_type_cache.py +++ b/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 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..7a3b74b63b2 --- /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-if the tag is valid. diff --git a/Modules/_testinternalcapi.c b/Modules/_testinternalcapi.c index 139a0509795..3dd40a66abc 100644 --- a/Modules/_testinternalcapi.c +++ b/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; } diff --git a/Modules/pyexpat.c b/Modules/pyexpat.c index 8495fe2dd4d..9733bc34f7c 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 0dcf1d399d9..1cc6ca79298 100644 --- a/Objects/typeobject.c +++ b/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(); - - /* 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 + PyInterpreterState *interp = _PyInterpreterState_GET(); + // lookup the old version and set to null + if (tp->tp_version_tag != 0) { + PyTypeObject **slot = + interp->types.type_version_cache + + (tp->tp_version_tag % TYPE_VERSION_CACHE_SIZE); + *slot = NULL; } - - 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); - } + if (version) { + tp->tp_versions_used++; + } +#else + 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; - _PyType_SetVersion(type, 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): @@ -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; - _PyType_SetVersion(type, 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): @@ -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(); - // lookup the old version and set to null - if (tp->tp_version_tag != 0) { - 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 + + BEGIN_TYPE_LOCK() + set_version_unlocked(tp, version); + END_TYPE_LOCK() } 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 - 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)) { @@ -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);