[3.13] GH-119462: Enforce invariants of type versioning. Backport of GH-120731. (#120748)
* Remove uses of Py_TPFLAGS_VALID_VERSION_TAG
diff --git a/Doc/c-api/typeobj.rst b/Doc/c-api/typeobj.rst
index a6a2c43..c9ef076 100644
--- a/Doc/c-api/typeobj.rst
+++ b/Doc/c-api/typeobj.rst
@@ -1328,8 +1328,8 @@
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 7aaa8da..05d9056 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 e90e315..09c68d4 100644
--- a/Lib/test/test_type_cache.py
+++ b/Lib/test/test_type_cache.py
@@ -92,6 +92,21 @@ class C:
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 @@ def _assign_valid_version_or_skip(self, type_):
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 @@ def load_foo_1(type_):
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 @@ def load_x_1(instance):
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 @@ def store_bar_1(type_):
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 @@ def call_class_1(type_):
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 @@ def to_bool_1(instance):
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 0000000..e07c2fd
--- /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 b58c172..8b40821 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 f67d480..fafb5ce 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 5bf2bc2..7c11d87 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);
