https://github.com/python/cpython/commit/00257c746c447a2e026b5a2a618f0e033fb90111
commit: 00257c746c447a2e026b5a2a618f0e033fb90111
branch: main
author: Mark Shannon <[email protected]>
committer: markshannon <[email protected]>
date: 2024-06-19T17:38:45+01:00
summary:
GH-119462: Enforce invariants of type versioning (GH-120731)
* Remove uses of Py_TPFLAGS_VALID_VERSION_TAG
files:
A Misc/NEWS.d/next/Core and
Builtins/2024-06-19-11-10-50.gh-issue-119462.DpcqSe.rst
M Doc/c-api/typeobj.rst
M Include/object.h
M Lib/test/test_type_cache.py
M Modules/_testinternalcapi.c
M Modules/pyexpat.c
M Objects/typeobject.c
diff --git a/Doc/c-api/typeobj.rst b/Doc/c-api/typeobj.rst
index a6a2c437ea4e16..c9ef076c78c66a 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 795d4fb2584df4..ed39e7dc877810 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 edaf076707ad8b..89632a3abebfb5 100644
--- a/Lib/test/test_type_cache.py
+++ b/Lib/test/test_type_cache.py
@@ -93,6 +93,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
@@ -106,8 +121,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)
@@ -136,7 +153,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
@@ -187,7 +204,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
@@ -206,7 +223,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
@@ -226,7 +243,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_()
@@ -245,7 +262,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 00000000000000..7a3b74b63b2e40
--- /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 139a0509795de9..3dd40a66abc1a4 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 8495fe2dd4dd2b..9733bc34f7c80a 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 0dcf1d399d91be..1cc6ca79298108 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);
_______________________________________________
Python-checkins mailing list -- [email protected]
To unsubscribe send an email to [email protected]
https://mail.python.org/mailman3/lists/python-checkins.python.org/
Member address: [email protected]
