From 8eeb4feb4fdae95e86f8cc3bf622efd82a9381e5 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Tue, 10 Dec 2024 13:51:22 -0800 Subject: [PATCH 01/67] Add _PyDictKeys_StringLookupAndVersion Look up a unicode key in an all unicode keys object along with the keys version, assigning one if not present. We need a keys version that is consistent with presence of the key for use in the guards. --- Include/internal/pycore_dict.h | 11 +++++++++ Objects/dictobject.c | 43 +++++++++++++++++++++++++++------- 2 files changed, 45 insertions(+), 9 deletions(-) diff --git a/Include/internal/pycore_dict.h b/Include/internal/pycore_dict.h index 6e4a308226f3fe..98da06efd636dd 100644 --- a/Include/internal/pycore_dict.h +++ b/Include/internal/pycore_dict.h @@ -113,7 +113,18 @@ extern Py_ssize_t _Py_dict_lookup_threadsafe(PyDictObject *mp, PyObject *key, Py extern Py_ssize_t _Py_dict_lookup_threadsafe_stackref(PyDictObject *mp, PyObject *key, Py_hash_t hash, _PyStackRef *value_addr); extern Py_ssize_t _PyDict_LookupIndex(PyDictObject *, PyObject *); + extern Py_ssize_t _PyDictKeys_StringLookup(PyDictKeysObject* dictkeys, PyObject *key); + +/* Look up a string key in an all unicode dict keys, assign the keys object a version, and + * stored it in version. + * + * Returns DKIX_ERROR if key is not a string or if the keys object is not all + * strings. + * + * Returns DKIX_EMPTY if the key is not present. + */ +extern Py_ssize_t _PyDictKeys_StringLookupAndVersion(PyDictKeysObject* dictkeys, PyObject *key, uint32_t *version); PyAPI_FUNC(PyObject *)_PyDict_LoadGlobal(PyDictObject *, PyDictObject *, PyObject *); PyAPI_FUNC(void) _PyDict_LoadGlobalStackRef(PyDictObject *, PyDictObject *, PyObject *, _PyStackRef *); diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 05c93a3e448181..4256c584e9083a 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -1129,6 +1129,23 @@ dictkeys_generic_lookup(PyDictObject *mp, PyDictKeysObject* dk, PyObject *key, P return do_lookup(mp, dk, key, hash, compare_generic); } +static Py_hash_t +check_keys_and_hash(PyDictKeysObject *dk, PyObject *key) { + DictKeysKind kind = dk->dk_kind; + if (!PyUnicode_CheckExact(key) || kind == DICT_KEYS_GENERAL) { + return -1; + } + Py_hash_t hash = unicode_get_hash(key); + if (hash == -1) { + hash = PyUnicode_Type.tp_hash(key); + if (hash == -1) { + PyErr_Clear(); + return -1; + } + } + return hash; +} + /* Lookup a string in a (all unicode) dict keys. * Returns DKIX_ERROR if key is not a string, * or if the dict keys is not all strings. @@ -1138,19 +1155,27 @@ dictkeys_generic_lookup(PyDictObject *mp, PyDictKeysObject* dk, PyObject *key, P Py_ssize_t _PyDictKeys_StringLookup(PyDictKeysObject* dk, PyObject *key) { - DictKeysKind kind = dk->dk_kind; - if (!PyUnicode_CheckExact(key) || kind == DICT_KEYS_GENERAL) { + Py_hash_t hash = check_keys_and_hash(dk, key); + if (hash == -1) { return DKIX_ERROR; } - Py_hash_t hash = unicode_get_hash(key); + return unicodekeys_lookup_unicode(dk, key, hash); +} + +Py_ssize_t +_PyDictKeys_StringLookupAndVersion(PyDictKeysObject* dk, PyObject *key, uint32_t *version) +{ + Py_hash_t hash = check_keys_and_hash(dk, key); if (hash == -1) { - hash = PyUnicode_Type.tp_hash(key); - if (hash == -1) { - PyErr_Clear(); - return DKIX_ERROR; - } + return DKIX_ERROR; } - return unicodekeys_lookup_unicode(dk, key, hash); + + Py_ssize_t ix; + LOCK_KEYS(dk); + ix = unicodekeys_lookup_unicode(dk, key, hash); + *version = _PyDictKeys_GetVersionForCurrentState(_PyInterpreterState_GET(), dk); + UNLOCK_KEYS(dk); + return ix; } #ifdef Py_GIL_DISABLED From fcd05a0c692d1ba21929ef008f959411d27dcd5d Mon Sep 17 00:00:00 2001 From: Matt Page Date: Tue, 10 Dec 2024 13:58:09 -0800 Subject: [PATCH 02/67] Pass shared keys version to specialization Reading the shared keys version and looking up the key need to be performed atomically. Otherwise, a key inserted after the lookup could race with reading the version, causing us to incorrectly specialize that no key shadows the descriptor. --- Python/specialize.c | 45 +++++++++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 20 deletions(-) diff --git a/Python/specialize.c b/Python/specialize.c index 6eb298217ec2d3..8d601ff63bb0cb 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -1007,9 +1007,11 @@ specialize_dict_access( return 1; } -#ifndef Py_GIL_DISABLED -static int specialize_attr_loadclassattr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* name, - PyObject* descr, DescriptorClassification kind, bool is_method); +static int +specialize_attr_loadclassattr(PyObject *owner, _Py_CODEUNIT *instr, + PyObject *name, PyObject *descr, + DescriptorClassification kind, bool is_method, + uint32_t shared_keys_version); static int specialize_class_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* name); /* Returns true if instances of obj's class are @@ -1018,7 +1020,7 @@ static int specialize_class_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyOb * For other objects, we check their actual dictionary. */ static bool -instance_has_key(PyObject *obj, PyObject* name) +instance_has_key(PyObject *obj, PyObject *name, uint32_t *shared_keys_version) { PyTypeObject *cls = Py_TYPE(obj); if ((cls->tp_flags & Py_TPFLAGS_MANAGED_DICT) == 0) { @@ -1026,7 +1028,8 @@ instance_has_key(PyObject *obj, PyObject* name) } if (cls->tp_flags & Py_TPFLAGS_INLINE_VALUES) { PyDictKeysObject *keys = ((PyHeapTypeObject *)cls)->ht_cached_keys; - Py_ssize_t index = _PyDictKeys_StringLookup(keys, name); + Py_ssize_t index = + _PyDictKeys_StringLookupAndVersion(keys, name, shared_keys_version); return index >= 0; } PyDictObject *dict = _PyObject_GetManagedDict(obj); @@ -1048,7 +1051,9 @@ specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* na { _PyAttrCache *cache = (_PyAttrCache *)(instr + 1); PyTypeObject *type = Py_TYPE(owner); - bool shadow = instance_has_key(owner, name); + // 0 is not a valid version + uint32_t shared_keys_version = 0; + bool shadow = instance_has_key(owner, name, &shared_keys_version); PyObject *descr = NULL; DescriptorClassification kind = analyze_descriptor(type, name, &descr, 0); assert(descr != NULL || kind == ABSENT || kind == GETSET_OVERRIDDEN); @@ -1066,7 +1071,9 @@ specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* na } int oparg = instr->op.arg; if (oparg & 1) { - if (specialize_attr_loadclassattr(owner, instr, name, descr, kind, true)) { + if (specialize_attr_loadclassattr(owner, instr, name, descr, + kind, true, + shared_keys_version)) { return 0; } else { @@ -1188,7 +1195,9 @@ specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* na goto try_instance; } if ((instr->op.arg & 1) == 0) { - if (specialize_attr_loadclassattr(owner, instr, name, descr, kind, false)) { + if (specialize_attr_loadclassattr(owner, instr, name, descr, + kind, false, + shared_keys_version)) { return 0; } } @@ -1209,7 +1218,6 @@ specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* na } return -1; } -#endif // Py_GIL_DISABLED void _Py_Specialize_LoadAttr(_PyStackRef owner_st, _Py_CODEUNIT *instr, PyObject *name) @@ -1239,12 +1247,7 @@ _Py_Specialize_LoadAttr(_PyStackRef owner_st, _Py_CODEUNIT *instr, PyObject *nam #endif } else { - #ifdef Py_GIL_DISABLED - SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_EXPECTED_ERROR); - fail = true; - #else fail = specialize_instance_load_attr(owner, instr, name); - #endif } if (fail) { @@ -1458,8 +1461,10 @@ specialize_class_load_attr(PyObject *owner, _Py_CODEUNIT *instr, // can cause a significant drop in cache hits. A possible test is // python.exe -m test_typing test_re test_dis test_zlib. static int -specialize_attr_loadclassattr(PyObject *owner, _Py_CODEUNIT *instr, PyObject *name, -PyObject *descr, DescriptorClassification kind, bool is_method) +specialize_attr_loadclassattr(PyObject *owner, _Py_CODEUNIT *instr, + PyObject *name, PyObject *descr, + DescriptorClassification kind, bool is_method, + uint32_t shared_keys_version) { _PyLoadMethodCache *cache = (_PyLoadMethodCache *)(instr + 1); PyTypeObject *owner_cls = Py_TYPE(owner); @@ -1467,15 +1472,15 @@ PyObject *descr, DescriptorClassification kind, bool is_method) assert(descr != NULL); assert((is_method && kind == METHOD) || (!is_method && kind == NON_DESCRIPTOR)); if (owner_cls->tp_flags & Py_TPFLAGS_INLINE_VALUES) { +#ifndef Py_GIL_DISABLED PyDictKeysObject *keys = ((PyHeapTypeObject *)owner_cls)->ht_cached_keys; assert(_PyDictKeys_StringLookup(keys, name) < 0); - uint32_t keys_version = _PyDictKeys_GetVersionForCurrentState( - _PyInterpreterState_GET(), keys); - if (keys_version == 0) { +#endif + if (shared_keys_version == 0) { SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_OUT_OF_VERSIONS); return 0; } - write_u32(cache->keys_version, keys_version); + write_u32(cache->keys_version, shared_keys_version); specialize(instr, is_method ? LOAD_ATTR_METHOD_WITH_VALUES : LOAD_ATTR_NONDESCRIPTOR_WITH_VALUES); } else { From 5c03db067013cfa6bc9e2683f776ad8c8cdda28b Mon Sep 17 00:00:00 2001 From: Matt Page Date: Tue, 10 Dec 2024 15:21:25 -0800 Subject: [PATCH 03/67] Add support for enabling each of the instance attribute kinds Everything starts out disabled --- Python/specialize.c | 35 +++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/Python/specialize.c b/Python/specialize.c index 8d601ff63bb0cb..fa811f0aeff582 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -738,10 +738,8 @@ unspecialize(_Py_CODEUNIT *instr) } static int function_kind(PyCodeObject *code); -#ifndef Py_GIL_DISABLED static bool function_check_args(PyObject *o, int expected_argcount, int opcode); static uint32_t function_get_version(PyObject *o, int opcode); -#endif static uint32_t type_get_version(PyTypeObject *t, int opcode); static int @@ -1012,7 +1010,9 @@ specialize_attr_loadclassattr(PyObject *owner, _Py_CODEUNIT *instr, PyObject *name, PyObject *descr, DescriptorClassification kind, bool is_method, uint32_t shared_keys_version); +#ifndef Py_GIL_DISABLED static int specialize_class_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* name); +#endif /* Returns true if instances of obj's class are * likely to have `name` in their __dict__. @@ -1046,6 +1046,20 @@ instance_has_key(PyObject *obj, PyObject *name, uint32_t *shared_keys_version) return true; } +#ifdef Py_GIL_DISABLED + +#define FT_UNIMPLEMENTED() \ + do { \ + SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_EXPECTED_ERROR); \ + return -1; \ + } while (0) + +#else + +#define FT_UNIMPLEMENTED() + +#endif + static int specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* name) { @@ -1066,6 +1080,7 @@ specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* na return -1; case METHOD: { + FT_UNIMPLEMENTED(); if (shadow) { goto try_instance; } @@ -1085,6 +1100,7 @@ specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* na } case PROPERTY: { + FT_UNIMPLEMENTED(); _PyLoadMethodCache *lm_cache = (_PyLoadMethodCache *)(instr + 1); assert(Py_TYPE(descr) == &PyProperty_Type); PyObject *fget = ((_PyPropertyObject *)descr)->prop_get; @@ -1116,6 +1132,7 @@ specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* na } case OBJECT_SLOT: { + FT_UNIMPLEMENTED(); PyMemberDescrObject *member = (PyMemberDescrObject *)descr; struct PyMemberDef *dmem = member->d_member; Py_ssize_t offset = dmem->offset; @@ -1140,6 +1157,7 @@ specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* na } case DUNDER_CLASS: { + FT_UNIMPLEMENTED(); Py_ssize_t offset = offsetof(PyObject, ob_type); assert(offset == (uint16_t)offset); cache->index = (uint16_t)offset; @@ -1160,6 +1178,7 @@ specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* na { assert(type->tp_getattro == _Py_slot_tp_getattro); assert(Py_IS_TYPE(descr, &PyFunction_Type)); + FT_UNIMPLEMENTED(); _PyLoadMethodCache *lm_cache = (_PyLoadMethodCache *)(instr + 1); if (!function_check_args(descr, 2, LOAD_ATTR)) { return -1; @@ -1191,6 +1210,8 @@ specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* na } return -1; case NON_DESCRIPTOR: + FT_UNIMPLEMENTED(); + if (shadow) { goto try_instance; } @@ -1211,6 +1232,8 @@ specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* na } Py_UNREACHABLE(); try_instance: + FT_UNIMPLEMENTED(); + if (specialize_dict_access(owner, instr, type, kind, name, LOAD_ATTR, LOAD_ATTR_INSTANCE_VALUE, LOAD_ATTR_WITH_HINT)) { @@ -1219,6 +1242,8 @@ specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* na return -1; } +#undef FT_UNIMPLEMENTED + void _Py_Specialize_LoadAttr(_PyStackRef owner_st, _Py_CODEUNIT *instr, PyObject *name) { @@ -1457,6 +1482,8 @@ specialize_class_load_attr(PyObject *owner, _Py_CODEUNIT *instr, } } +#endif // Py_GIL_DISABLED + // Please collect stats carefully before and after modifying. A subtle change // can cause a significant drop in cache hits. A possible test is // python.exe -m test_typing test_re test_dis test_zlib. @@ -1536,8 +1563,6 @@ specialize_attr_loadclassattr(PyObject *owner, _Py_CODEUNIT *instr, return 1; } -#endif // Py_GIL_DISABLED - static void specialize_load_global_lock_held( @@ -1684,7 +1709,6 @@ function_kind(PyCodeObject *code) { return SIMPLE_FUNCTION; } -#ifndef Py_GIL_DISABLED /* Returning false indicates a failure. */ static bool function_check_args(PyObject *o, int expected_argcount, int opcode) @@ -1717,7 +1741,6 @@ function_get_version(PyObject *o, int opcode) } return version; } -#endif // Py_GIL_DISABLED /* Returning 0 indicates a failure. */ static uint32_t From a576748efbc80de4117278d25b815a6b7576c39c Mon Sep 17 00:00:00 2001 From: Matt Page Date: Tue, 10 Dec 2024 15:26:22 -0800 Subject: [PATCH 04/67] Only cache deferred descriptors --- Python/specialize.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/Python/specialize.c b/Python/specialize.c index fa811f0aeff582..a5aca4a774e0fb 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -546,6 +546,7 @@ _PyCode_Quicken(_Py_CODEUNIT *instructions, Py_ssize_t size, PyObject *consts, #define SPEC_FAIL_ATTR_BUILTIN_CLASS_METHOD_OBJ 33 #define SPEC_FAIL_ATTR_METACLASS_OVERRIDDEN 34 #define SPEC_FAIL_ATTR_SPLIT_DICT 35 +#define SPEC_FAIL_ATTR_DESCR_NOT_DEFERRED 36 /* Binary subscr and store subscr */ @@ -1498,6 +1499,14 @@ specialize_attr_loadclassattr(PyObject *owner, _Py_CODEUNIT *instr, assert(descr != NULL); assert((is_method && kind == METHOD) || (!is_method && kind == NON_DESCRIPTOR)); + +#ifdef Py_GIL_DISABLED + if (!_PyObject_HasDeferredRefcount(descr)) { + SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_ATTR_DESCR_NOT_DEFERRED); + return 0; + } +#endif + if (owner_cls->tp_flags & Py_TPFLAGS_INLINE_VALUES) { #ifndef Py_GIL_DISABLED PyDictKeysObject *keys = ((PyHeapTypeObject *)owner_cls)->ht_cached_keys; From 8475fd6822f5cc7eca37bb75e24d3a47f836c42f Mon Sep 17 00:00:00 2001 From: Matt Page Date: Tue, 10 Dec 2024 17:50:58 -0800 Subject: [PATCH 05/67] Make analyze_descriptor thread-safe * Check that the type hasn't changed across lookups * Descr is now an owned ref --- Python/specialize.c | 89 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 72 insertions(+), 17 deletions(-) diff --git a/Python/specialize.c b/Python/specialize.c index a5aca4a774e0fb..1f666650bc6179 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -547,6 +547,7 @@ _PyCode_Quicken(_Py_CODEUNIT *instructions, Py_ssize_t size, PyObject *consts, #define SPEC_FAIL_ATTR_METACLASS_OVERRIDDEN 34 #define SPEC_FAIL_ATTR_SPLIT_DICT 35 #define SPEC_FAIL_ATTR_DESCR_NOT_DEFERRED 36 +#define SPEC_FAIL_ATTR_TYPE_CHANGED 37 /* Binary subscr and store subscr */ @@ -834,7 +835,8 @@ typedef enum { ABSENT, /* Attribute is not present on the class */ DUNDER_CLASS, /* __class__ attribute */ GETSET_OVERRIDDEN, /* __getattribute__ or __setattr__ has been overridden */ - GETATTRIBUTE_IS_PYTHON_FUNCTION /* Descriptor requires calling a Python __getattribute__ */ + GETATTRIBUTE_IS_PYTHON_FUNCTION, /* Descriptor requires calling a Python __getattribute__ */ + TYPE_CHANGED, /* Error: the type changed during classification */ } DescriptorClassification; @@ -881,9 +883,11 @@ classify_descriptor(PyObject *descriptor, bool has_getattr) } static DescriptorClassification -analyze_descriptor(PyTypeObject *type, PyObject *name, PyObject **descr, int store) +analyze_descriptor(PyTypeObject *type, PyObject *name, PyObject **descr, unsigned int *tp_version, int store) { bool has_getattr = false; + bool have_getattribute_ver = false; + unsigned int getattribute_ver; if (store) { if (type->tp_setattro != PyObject_GenericSetAttr) { *descr = NULL; @@ -900,22 +904,34 @@ analyze_descriptor(PyTypeObject *type, PyObject *name, PyObject **descr, int sto getattro_slot == _Py_slot_tp_getattro) { /* One or both of __getattribute__ or __getattr__ may have been overridden See typeobject.c for why these functions are special. */ - PyObject *getattribute = _PyType_Lookup(type, - &_Py_ID(__getattribute__)); + PyObject *getattribute = _PyType_LookupRefAndVersion(type, + &_Py_ID(__getattribute__), &getattribute_ver); + have_getattribute_ver = true; PyInterpreterState *interp = _PyInterpreterState_GET(); bool has_custom_getattribute = getattribute != NULL && getattribute != interp->callable_cache.object__getattribute__; - has_getattr = _PyType_Lookup(type, &_Py_ID(__getattr__)) != NULL; + unsigned int getattr_ver; + PyObject *getattr = _PyType_LookupRefAndVersion(type, &_Py_ID(__getattr__), &getattr_ver); + has_getattr = getattr != NULL; + if (getattr_ver != getattribute_ver) { + Py_XDECREF(getattribute); + Py_XDECREF(getattr); + return TYPE_CHANGED; + } if (has_custom_getattribute) { if (getattro_slot == _Py_slot_tp_getattro && !has_getattr && Py_IS_TYPE(getattribute, &PyFunction_Type)) { *descr = getattribute; + assert(getattr == NULL); + *tp_version = getattribute_ver; return GETATTRIBUTE_IS_PYTHON_FUNCTION; } /* Potentially both __getattr__ and __getattribute__ are set. Too complicated */ *descr = NULL; + Py_XDECREF(getattribute); + Py_XDECREF(getattr); return GETSET_OVERRIDDEN; } /* Potentially has __getattr__ but no custom __getattribute__. @@ -925,14 +941,22 @@ analyze_descriptor(PyTypeObject *type, PyObject *name, PyObject **descr, int sto raised. This means some specializations, e.g. specializing for property() isn't safe. */ + Py_XDECREF(getattr); + Py_XDECREF(getattribute); } else { *descr = NULL; return GETSET_OVERRIDDEN; } } - PyObject *descriptor = _PyType_Lookup(type, name); + unsigned int descr_version; + PyObject *descriptor = _PyType_LookupRefAndVersion(type, name, &descr_version); + if (have_getattribute_ver && (descr_version != getattribute_ver)) { + Py_XDECREF(descriptor); + return TYPE_CHANGED; + } *descr = descriptor; + *tp_version = descr_version; if (PyUnicode_CompareWithASCIIString(name, "__class__") == 0) { if (descriptor == _PyType_Lookup(&PyBaseObject_Type, name)) { return DUNDER_CLASS; @@ -1062,20 +1086,20 @@ instance_has_key(PyObject *obj, PyObject *name, uint32_t *shared_keys_version) #endif static int -specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* name) +do_specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* name, + bool shadow, uint32_t shared_keys_version, + DescriptorClassification kind, PyObject *descr, unsigned int tp_version) { _PyAttrCache *cache = (_PyAttrCache *)(instr + 1); PyTypeObject *type = Py_TYPE(owner); - // 0 is not a valid version - uint32_t shared_keys_version = 0; - bool shadow = instance_has_key(owner, name, &shared_keys_version); - PyObject *descr = NULL; - DescriptorClassification kind = analyze_descriptor(type, name, &descr, 0); - assert(descr != NULL || kind == ABSENT || kind == GETSET_OVERRIDDEN); - if (type_get_version(type, LOAD_ATTR) == 0) { + if (tp_version == 0) { + SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_OUT_OF_VERSIONS); return -1; } switch(kind) { + case TYPE_CHANGED: + SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_ATTR_TYPE_CHANGED); + return -1; case OVERRIDING: SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_ATTR_OVERRIDING_DESCRIPTOR); return -1; @@ -1245,6 +1269,21 @@ specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* na #undef FT_UNIMPLEMENTED +static int +specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* name) +{ + // 0 is not a valid version + uint32_t shared_keys_version = 0; + bool shadow = instance_has_key(owner, name, &shared_keys_version); + PyObject *descr = NULL; + unsigned int tp_version; + PyTypeObject *type = Py_TYPE(owner); + DescriptorClassification kind = analyze_descriptor(type, name, &descr, &tp_version, 0); + int result = do_specialize_instance_load_attr(owner, instr, name, shadow, shared_keys_version, kind, descr, tp_version); + Py_XDECREF(descr); + return result; +} + void _Py_Specialize_LoadAttr(_PyStackRef owner_st, _Py_CODEUNIT *instr, PyObject *name) { @@ -1290,6 +1329,7 @@ _Py_Specialize_StoreAttr(_PyStackRef owner_st, _Py_CODEUNIT *instr, PyObject *na assert(_PyOpcode_Caches[STORE_ATTR] == INLINE_CACHE_ENTRIES_STORE_ATTR); _PyAttrCache *cache = (_PyAttrCache *)(instr + 1); PyTypeObject *type = Py_TYPE(owner); + PyObject *descr = NULL; if (!_PyType_IsReady(type)) { // We *might* not really need this check, but we inherited it from // PyObject_GenericSetAttr and friends... and this way we still do the @@ -1301,12 +1341,15 @@ _Py_Specialize_StoreAttr(_PyStackRef owner_st, _Py_CODEUNIT *instr, PyObject *na SPECIALIZATION_FAIL(STORE_ATTR, SPEC_FAIL_OVERRIDDEN); goto fail; } - PyObject *descr; - DescriptorClassification kind = analyze_descriptor(type, name, &descr, 1); + unsigned int tp_version; + DescriptorClassification kind = analyze_descriptor(type, name, &descr, &tp_version, 1); if (type_get_version(type, STORE_ATTR) == 0) { goto fail; } switch(kind) { + case TYPE_CHANGED: + SPECIALIZATION_FAIL(STORE_ATTR, SPEC_FAIL_ATTR_TYPE_CHANGED); + goto fail; case OVERRIDING: SPECIALIZATION_FAIL(STORE_ATTR, SPEC_FAIL_ATTR_OVERRIDING_DESCRIPTOR); goto fail; @@ -1375,11 +1418,13 @@ _Py_Specialize_StoreAttr(_PyStackRef owner_st, _Py_CODEUNIT *instr, PyObject *na assert(!PyErr_Occurred()); instr->op.code = STORE_ATTR; cache->counter = adaptive_counter_backoff(cache->counter); + Py_XDECREF(descr); return; success: STAT_INC(STORE_ATTR, success); assert(!PyErr_Occurred()); cache->counter = adaptive_counter_cooldown(); + Py_XDECREF(descr); } #ifndef Py_GIL_DISABLED @@ -1448,18 +1493,25 @@ specialize_class_load_attr(PyObject *owner, _Py_CODEUNIT *instr, } PyObject *descr = NULL; DescriptorClassification kind = 0; - kind = analyze_descriptor(cls, name, &descr, 0); + unsigned int tp_version; + kind = analyze_descriptor(cls, name, &descr, &tp_version, 0); if (type_get_version(cls, LOAD_ATTR) == 0) { + Py_XDECREF(descr); return -1; } bool metaclass_check = false; if ((Py_TYPE(cls)->tp_flags & Py_TPFLAGS_IMMUTABLETYPE) == 0) { metaclass_check = true; if (type_get_version(Py_TYPE(cls), LOAD_ATTR) == 0) { + Py_XDECREF(descr); return -1; } } switch (kind) { + case TYPE_CHANGED: + SPECIALIZATION_FAIL(STORE_ATTR, SPEC_FAIL_ATTR_TYPE_CHANGED); + Py_XDECREF(descr); + return -1; case METHOD: case NON_DESCRIPTOR: write_u32(cache->type_version, cls->tp_version_tag); @@ -1471,14 +1523,17 @@ specialize_class_load_attr(PyObject *owner, _Py_CODEUNIT *instr, else { specialize(instr, LOAD_ATTR_CLASS); } + Py_XDECREF(descr); return 0; #ifdef Py_STATS case ABSENT: SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_EXPECTED_ERROR); + Py_XDECREF(descr); return -1; #endif default: SPECIALIZATION_FAIL(LOAD_ATTR, load_attr_fail_kind(kind)); + Py_XDECREF(descr); return -1; } } From 29c33564b4672a59e6b2321a3ebfc9e664252b5c Mon Sep 17 00:00:00 2001 From: Matt Page Date: Tue, 10 Dec 2024 17:56:31 -0800 Subject: [PATCH 06/67] Use an atomic load for GUARD_TYPE_VERSION --- Python/bytecodes.c | 2 +- Python/executor_cases.c.h | 2 +- Python/generated_cases.c.h | 28 ++++++++++++++-------------- 3 files changed, 16 insertions(+), 16 deletions(-) diff --git a/Python/bytecodes.c b/Python/bytecodes.c index 772b46d17ec198..86c49e38912397 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2129,7 +2129,7 @@ dummy_func( op(_GUARD_TYPE_VERSION, (type_version/2, owner -- owner)) { PyTypeObject *tp = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); assert(type_version != 0); - EXIT_IF(tp->tp_version_tag != type_version); + EXIT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(tp->tp_version_tag) != type_version); } op(_CHECK_MANAGED_OBJECT_HAS_VALUES, (owner -- owner)) { diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index 55e9c3aa2db64d..40e442bdc6b5ee 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -2570,7 +2570,7 @@ uint32_t type_version = (uint32_t)CURRENT_OPERAND0(); PyTypeObject *tp = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); assert(type_version != 0); - if (tp->tp_version_tag != type_version) { + if (FT_ATOMIC_LOAD_UINT32_RELAXED(tp->tp_version_tag) != type_version) { UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); } diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 94343f953221eb..84452d8722100f 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -5321,7 +5321,7 @@ uint32_t type_version = read_u32(&this_instr[4].cache); PyTypeObject *tp = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); assert(type_version != 0); - DEOPT_IF(tp->tp_version_tag != type_version, LOAD_ATTR); + DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(tp->tp_version_tag) != type_version, LOAD_ATTR); } // _LOAD_ATTR_CLASS { @@ -5390,7 +5390,7 @@ uint32_t type_version = read_u32(&this_instr[2].cache); PyTypeObject *tp = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); assert(type_version != 0); - DEOPT_IF(tp->tp_version_tag != type_version, LOAD_ATTR); + DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(tp->tp_version_tag) != type_version, LOAD_ATTR); } // _CHECK_MANAGED_OBJECT_HAS_VALUES { @@ -5435,7 +5435,7 @@ uint32_t type_version = read_u32(&this_instr[2].cache); PyTypeObject *tp = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); assert(type_version != 0); - DEOPT_IF(tp->tp_version_tag != type_version, LOAD_ATTR); + DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(tp->tp_version_tag) != type_version, LOAD_ATTR); } // _CHECK_ATTR_METHOD_LAZY_DICT { @@ -5478,7 +5478,7 @@ uint32_t type_version = read_u32(&this_instr[2].cache); PyTypeObject *tp = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); assert(type_version != 0); - DEOPT_IF(tp->tp_version_tag != type_version, LOAD_ATTR); + DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(tp->tp_version_tag) != type_version, LOAD_ATTR); } /* Skip 2 cache entries */ // _LOAD_ATTR_METHOD_NO_DICT @@ -5514,7 +5514,7 @@ uint32_t type_version = read_u32(&this_instr[2].cache); PyTypeObject *tp = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); assert(type_version != 0); - DEOPT_IF(tp->tp_version_tag != type_version, LOAD_ATTR); + DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(tp->tp_version_tag) != type_version, LOAD_ATTR); } // _GUARD_DORV_VALUES_INST_ATTR_FROM_DICT { @@ -5613,7 +5613,7 @@ uint32_t type_version = read_u32(&this_instr[2].cache); PyTypeObject *tp = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); assert(type_version != 0); - DEOPT_IF(tp->tp_version_tag != type_version, LOAD_ATTR); + DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(tp->tp_version_tag) != type_version, LOAD_ATTR); } /* Skip 2 cache entries */ // _LOAD_ATTR_NONDESCRIPTOR_NO_DICT @@ -5644,7 +5644,7 @@ uint32_t type_version = read_u32(&this_instr[2].cache); PyTypeObject *tp = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); assert(type_version != 0); - DEOPT_IF(tp->tp_version_tag != type_version, LOAD_ATTR); + DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(tp->tp_version_tag) != type_version, LOAD_ATTR); } // _GUARD_DORV_VALUES_INST_ATTR_FROM_DICT { @@ -5690,7 +5690,7 @@ uint32_t type_version = read_u32(&this_instr[2].cache); PyTypeObject *tp = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); assert(type_version != 0); - DEOPT_IF(tp->tp_version_tag != type_version, LOAD_ATTR); + DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(tp->tp_version_tag) != type_version, LOAD_ATTR); } /* Skip 2 cache entries */ // _LOAD_ATTR_PROPERTY_FRAME @@ -5752,7 +5752,7 @@ uint32_t type_version = read_u32(&this_instr[2].cache); PyTypeObject *tp = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); assert(type_version != 0); - DEOPT_IF(tp->tp_version_tag != type_version, LOAD_ATTR); + DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(tp->tp_version_tag) != type_version, LOAD_ATTR); } // _LOAD_ATTR_SLOT { @@ -5789,7 +5789,7 @@ uint32_t type_version = read_u32(&this_instr[2].cache); PyTypeObject *tp = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); assert(type_version != 0); - DEOPT_IF(tp->tp_version_tag != type_version, LOAD_ATTR); + DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(tp->tp_version_tag) != type_version, LOAD_ATTR); } // _CHECK_ATTR_WITH_HINT { @@ -7354,7 +7354,7 @@ uint32_t type_version = read_u32(&this_instr[2].cache); PyTypeObject *tp = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); assert(type_version != 0); - DEOPT_IF(tp->tp_version_tag != type_version, STORE_ATTR); + DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(tp->tp_version_tag) != type_version, STORE_ATTR); } // _GUARD_DORV_NO_DICT { @@ -7403,7 +7403,7 @@ uint32_t type_version = read_u32(&this_instr[2].cache); PyTypeObject *tp = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); assert(type_version != 0); - DEOPT_IF(tp->tp_version_tag != type_version, STORE_ATTR); + DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(tp->tp_version_tag) != type_version, STORE_ATTR); } // _STORE_ATTR_SLOT { @@ -7436,7 +7436,7 @@ uint32_t type_version = read_u32(&this_instr[2].cache); PyTypeObject *tp = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); assert(type_version != 0); - DEOPT_IF(tp->tp_version_tag != type_version, STORE_ATTR); + DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(tp->tp_version_tag) != type_version, STORE_ATTR); } // _STORE_ATTR_WITH_HINT { @@ -7810,7 +7810,7 @@ uint32_t type_version = read_u32(&this_instr[2].cache); PyTypeObject *tp = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); assert(type_version != 0); - DEOPT_IF(tp->tp_version_tag != type_version, TO_BOOL); + DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(tp->tp_version_tag) != type_version, TO_BOOL); } // _REPLACE_WITH_TRUE { From 4f9eeb3b10dfddaacfd9cbfc9d8ee3ece32def93 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Tue, 10 Dec 2024 18:05:59 -0800 Subject: [PATCH 07/67] Use atomics to load valid bit for inline values in _GUARD_DORV_VALUES_INST_ATTR_FROM_DICT --- Python/bytecodes.c | 3 ++- Python/executor_cases.c.h | 3 ++- Python/generated_cases.c.h | 6 ++++-- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/Python/bytecodes.c b/Python/bytecodes.c index 86c49e38912397..d3da5dda2c4990 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -3216,7 +3216,8 @@ dummy_func( op(_GUARD_DORV_VALUES_INST_ATTR_FROM_DICT, (owner -- owner)) { PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); assert(Py_TYPE(owner_o)->tp_flags & Py_TPFLAGS_INLINE_VALUES); - DEOPT_IF(!_PyObject_InlineValues(owner_o)->valid); + PyDictValues *ivs = _PyObject_InlineValues(owner_o); + DEOPT_IF(!FT_ATOMIC_LOAD_UINT8(ivs->valid)); } op(_GUARD_KEYS_VERSION, (keys_version/2, owner -- owner)) { diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index 40e442bdc6b5ee..c4d9ecb77f8d03 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -3823,7 +3823,8 @@ owner = stack_pointer[-1]; PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); assert(Py_TYPE(owner_o)->tp_flags & Py_TPFLAGS_INLINE_VALUES); - if (!_PyObject_InlineValues(owner_o)->valid) { + PyDictValues *ivs = _PyObject_InlineValues(owner_o); + if (!FT_ATOMIC_LOAD_UINT8(ivs->valid)) { UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); } diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 84452d8722100f..fca98c70f44a3b 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -5520,7 +5520,8 @@ { PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); assert(Py_TYPE(owner_o)->tp_flags & Py_TPFLAGS_INLINE_VALUES); - DEOPT_IF(!_PyObject_InlineValues(owner_o)->valid, LOAD_ATTR); + PyDictValues *ivs = _PyObject_InlineValues(owner_o); + DEOPT_IF(!FT_ATOMIC_LOAD_UINT8(ivs->valid), LOAD_ATTR); } // _GUARD_KEYS_VERSION { @@ -5650,7 +5651,8 @@ { PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); assert(Py_TYPE(owner_o)->tp_flags & Py_TPFLAGS_INLINE_VALUES); - DEOPT_IF(!_PyObject_InlineValues(owner_o)->valid, LOAD_ATTR); + PyDictValues *ivs = _PyObject_InlineValues(owner_o); + DEOPT_IF(!FT_ATOMIC_LOAD_UINT8(ivs->valid), LOAD_ATTR); } // _GUARD_KEYS_VERSION { From e9050fc925dadd0cb7eb69e225bc7d4e8b05800a Mon Sep 17 00:00:00 2001 From: Matt Page Date: Tue, 10 Dec 2024 18:08:26 -0800 Subject: [PATCH 08/67] Use atomic to load keys version in _GUARD_KEYS_VERSION --- Python/bytecodes.c | 3 ++- Python/executor_cases.c.h | 3 ++- Python/generated_cases.c.h | 6 ++++-- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/Python/bytecodes.c b/Python/bytecodes.c index d3da5dda2c4990..aff75d4010d486 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -3223,7 +3223,8 @@ dummy_func( op(_GUARD_KEYS_VERSION, (keys_version/2, owner -- owner)) { PyTypeObject *owner_cls = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); PyHeapTypeObject *owner_heap_type = (PyHeapTypeObject *)owner_cls; - DEOPT_IF(owner_heap_type->ht_cached_keys->dk_version != keys_version); + PyDictKeysObject *keys = owner_heap_type->ht_cached_keys; + DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(keys->dk_version) != keys_version); } split op(_LOAD_ATTR_METHOD_WITH_VALUES, (descr/4, owner -- attr, self if (1))) { diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index c4d9ecb77f8d03..119269257686b0 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -3837,7 +3837,8 @@ uint32_t keys_version = (uint32_t)CURRENT_OPERAND0(); PyTypeObject *owner_cls = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); PyHeapTypeObject *owner_heap_type = (PyHeapTypeObject *)owner_cls; - if (owner_heap_type->ht_cached_keys->dk_version != keys_version) { + PyDictKeysObject *keys = owner_heap_type->ht_cached_keys; + if (FT_ATOMIC_LOAD_UINT32_RELAXED(keys->dk_version) != keys_version) { UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); } diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index fca98c70f44a3b..4bb04459bc52f9 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -5528,7 +5528,8 @@ uint32_t keys_version = read_u32(&this_instr[4].cache); PyTypeObject *owner_cls = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); PyHeapTypeObject *owner_heap_type = (PyHeapTypeObject *)owner_cls; - DEOPT_IF(owner_heap_type->ht_cached_keys->dk_version != keys_version, LOAD_ATTR); + PyDictKeysObject *keys = owner_heap_type->ht_cached_keys; + DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(keys->dk_version) != keys_version, LOAD_ATTR); } // _LOAD_ATTR_METHOD_WITH_VALUES { @@ -5659,7 +5660,8 @@ uint32_t keys_version = read_u32(&this_instr[4].cache); PyTypeObject *owner_cls = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); PyHeapTypeObject *owner_heap_type = (PyHeapTypeObject *)owner_cls; - DEOPT_IF(owner_heap_type->ht_cached_keys->dk_version != keys_version, LOAD_ATTR); + PyDictKeysObject *keys = owner_heap_type->ht_cached_keys; + DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(keys->dk_version) != keys_version, LOAD_ATTR); } // _LOAD_ATTR_NONDESCRIPTOR_WITH_VALUES { From ade57f233d44fa8ce5b97e4dca184ac6e8151ce1 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Wed, 11 Dec 2024 14:12:48 -0800 Subject: [PATCH 09/67] Use an atomic load for managed dict in `_CHECK_ATTR_METHOD_LAZY_DICT` --- Python/bytecodes.c | 2 +- Python/executor_cases.c.h | 2 +- Python/generated_cases.c.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Python/bytecodes.c b/Python/bytecodes.c index aff75d4010d486..cc4b39d30a86a0 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -3294,7 +3294,7 @@ dummy_func( op(_CHECK_ATTR_METHOD_LAZY_DICT, (dictoffset/1, owner -- owner)) { char *ptr = ((char *)PyStackRef_AsPyObjectBorrow(owner)) + MANAGED_DICT_OFFSET + dictoffset; - PyObject *dict = *(PyObject **)ptr; + PyObject *dict = FT_ATOMIC_LOAD_PTR_ACQUIRE(*(PyObject **)ptr); /* This object has a __dict__, just not yet created */ DEOPT_IF(dict != NULL); } diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index 119269257686b0..9d96f531b7137e 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -3923,7 +3923,7 @@ owner = stack_pointer[-1]; uint16_t dictoffset = (uint16_t)CURRENT_OPERAND0(); char *ptr = ((char *)PyStackRef_AsPyObjectBorrow(owner)) + MANAGED_DICT_OFFSET + dictoffset; - PyObject *dict = *(PyObject **)ptr; + PyObject *dict = FT_ATOMIC_LOAD_PTR_ACQUIRE(*(PyObject **)ptr); /* This object has a __dict__, just not yet created */ if (dict != NULL) { UOP_STAT_INC(uopcode, miss); diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 4bb04459bc52f9..8b55f89ba3ba48 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -5441,7 +5441,7 @@ { uint16_t dictoffset = read_u16(&this_instr[4].cache); char *ptr = ((char *)PyStackRef_AsPyObjectBorrow(owner)) + MANAGED_DICT_OFFSET + dictoffset; - PyObject *dict = *(PyObject **)ptr; + PyObject *dict = FT_ATOMIC_LOAD_PTR_ACQUIRE(*(PyObject **)ptr); /* This object has a __dict__, just not yet created */ DEOPT_IF(dict != NULL, LOAD_ATTR); } From 0a87264a5f87917b3892cde0548672fcc433a9df Mon Sep 17 00:00:00 2001 From: Matt Page Date: Wed, 11 Dec 2024 14:22:00 -0800 Subject: [PATCH 10/67] Enable specialization of method loads --- Python/specialize.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/Python/specialize.c b/Python/specialize.c index 1f666650bc6179..103f0d0b78dcf4 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -1033,6 +1033,7 @@ specialize_dict_access( static int specialize_attr_loadclassattr(PyObject *owner, _Py_CODEUNIT *instr, PyObject *name, PyObject *descr, + unsigned int tp_version, DescriptorClassification kind, bool is_method, uint32_t shared_keys_version); #ifndef Py_GIL_DISABLED @@ -1105,14 +1106,14 @@ do_specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* return -1; case METHOD: { - FT_UNIMPLEMENTED(); if (shadow) { + FT_UNIMPLEMENTED(); goto try_instance; } int oparg = instr->op.arg; if (oparg & 1) { if (specialize_attr_loadclassattr(owner, instr, name, descr, - kind, true, + tp_version, kind, true, shared_keys_version)) { return 0; } @@ -1242,7 +1243,7 @@ do_specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* } if ((instr->op.arg & 1) == 0) { if (specialize_attr_loadclassattr(owner, instr, name, descr, - kind, false, + tp_version, kind, false, shared_keys_version)) { return 0; } @@ -1546,6 +1547,7 @@ specialize_class_load_attr(PyObject *owner, _Py_CODEUNIT *instr, static int specialize_attr_loadclassattr(PyObject *owner, _Py_CODEUNIT *instr, PyObject *name, PyObject *descr, + unsigned int tp_version, DescriptorClassification kind, bool is_method, uint32_t shared_keys_version) { @@ -1622,7 +1624,7 @@ specialize_attr_loadclassattr(PyObject *owner, _Py_CODEUNIT *instr, * PyType_Modified usages in typeobject.c). The MCACHE has been * working since Python 2.6 and it's battle-tested. */ - write_u32(cache->type_version, owner_cls->tp_version_tag); + write_u32(cache->type_version, tp_version); write_obj(cache->descr, descr); return 1; } From 816e22f17cc150e3e48b624122b74de40577fb12 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Wed, 11 Dec 2024 14:22:15 -0800 Subject: [PATCH 11/67] Use atomics for fetching type flags --- Python/specialize.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Python/specialize.c b/Python/specialize.c index 103f0d0b78dcf4..fcc7db9ecd2d42 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -1564,7 +1564,8 @@ specialize_attr_loadclassattr(PyObject *owner, _Py_CODEUNIT *instr, } #endif - if (owner_cls->tp_flags & Py_TPFLAGS_INLINE_VALUES) { + unsigned long tp_flags = PyType_GetFlags(owner_cls); + if (tp_flags & Py_TPFLAGS_INLINE_VALUES) { #ifndef Py_GIL_DISABLED PyDictKeysObject *keys = ((PyHeapTypeObject *)owner_cls)->ht_cached_keys; assert(_PyDictKeys_StringLookup(keys, name) < 0); @@ -1578,7 +1579,7 @@ specialize_attr_loadclassattr(PyObject *owner, _Py_CODEUNIT *instr, } else { Py_ssize_t dictoffset; - if (owner_cls->tp_flags & Py_TPFLAGS_MANAGED_DICT) { + if (tp_flags & Py_TPFLAGS_MANAGED_DICT) { dictoffset = MANAGED_DICT_OFFSET; } else { From ca1e232d1a8ae47782f7dfac3eb79d2d92efb3d8 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Wed, 11 Dec 2024 14:59:20 -0800 Subject: [PATCH 12/67] Get a strong reference to dict in instance_has_key --- Python/specialize.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/Python/specialize.c b/Python/specialize.c index fcc7db9ecd2d42..b8f192d2519ef4 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -1040,6 +1040,17 @@ specialize_attr_loadclassattr(PyObject *owner, _Py_CODEUNIT *instr, static int specialize_class_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* name); #endif +static PyDictObject * +get_managed_dict_ref(PyObject *obj) +{ + PyDictObject *dict; + Py_BEGIN_CRITICAL_SECTION(obj); + dict = _PyObject_GetManagedDict(obj); + Py_XINCREF(dict); + Py_END_CRITICAL_SECTION(); + return dict; +} + /* Returns true if instances of obj's class are * likely to have `name` in their __dict__. * For objects with inline values, we check in the shared keys. @@ -1058,14 +1069,17 @@ instance_has_key(PyObject *obj, PyObject *name, uint32_t *shared_keys_version) _PyDictKeys_StringLookupAndVersion(keys, name, shared_keys_version); return index >= 0; } - PyDictObject *dict = _PyObject_GetManagedDict(obj); + PyDictObject *dict = get_managed_dict_ref(obj); if (dict == NULL || !PyDict_CheckExact(dict)) { + Py_XDECREF(dict); return false; } if (dict->ma_values) { + Py_DECREF(dict); return false; } Py_ssize_t index = _PyDict_LookupIndex(dict, name); + Py_DECREF(dict); if (index < 0) { return false; } From 945b61c2d9d10275aea9156d9ca8057cb9c00e5c Mon Sep 17 00:00:00 2001 From: Matt Page Date: Wed, 11 Dec 2024 15:47:17 -0800 Subject: [PATCH 13/67] Split specialize_dict_access --- Python/specialize.c | 104 ++++++++++++++++++++++++++------------------ 1 file changed, 62 insertions(+), 42 deletions(-) diff --git a/Python/specialize.c b/Python/specialize.c index b8f192d2519ef4..86055dff25d251 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -965,6 +965,64 @@ analyze_descriptor(PyTypeObject *type, PyObject *name, PyObject **descr, unsigne return classify_descriptor(descriptor, has_getattr); } +static int +specialize_inline_values_access( + PyObject *owner, _Py_CODEUNIT *instr, PyTypeObject *type, + PyObject *name, int base_op, int values_op) +{ + PyDictKeysObject *keys = ((PyHeapTypeObject *)type)->ht_cached_keys; + _PyAttrCache *cache = (_PyAttrCache *)(instr + 1); + assert(PyUnicode_CheckExact(name)); + Py_ssize_t index = _PyDictKeys_StringLookup(keys, name); + assert (index != DKIX_ERROR); + if (index == DKIX_EMPTY) { + SPECIALIZATION_FAIL(base_op, SPEC_FAIL_ATTR_NOT_IN_KEYS); + return 0; + } + assert(index >= 0); + char *value_addr = (char *)&_PyObject_InlineValues(owner)->values[index]; + Py_ssize_t offset = value_addr - (char *)owner; + if (offset != (uint16_t)offset) { + SPECIALIZATION_FAIL(base_op, SPEC_FAIL_OUT_OF_RANGE); + return 0; + } + write_u32(cache->version, type->tp_version_tag); + cache->index = (uint16_t)offset; + specialize(instr, values_op); + return 1; +} + +static int +specialize_managed_dict_access( + PyObject *owner, _Py_CODEUNIT *instr, PyTypeObject *type, + PyObject *name, int base_op, int hint_op) +{ + PyDictObject *dict = _PyObject_GetManagedDict(owner); + _PyAttrCache *cache = (_PyAttrCache *)(instr + 1); + if (dict == NULL || !PyDict_CheckExact(dict)) { + SPECIALIZATION_FAIL(base_op, SPEC_FAIL_NO_DICT); + return 0; + } + // We found an instance with a __dict__. + if (dict->ma_values) { + SPECIALIZATION_FAIL(base_op, SPEC_FAIL_ATTR_SPLIT_DICT); + return 0; + } + Py_ssize_t index = + _PyDict_LookupIndex(dict, name); + if (index != (uint16_t)index) { + SPECIALIZATION_FAIL(base_op, + index == DKIX_EMPTY ? + SPEC_FAIL_ATTR_NOT_IN_DICT : + SPEC_FAIL_OUT_OF_RANGE); + return 0; + } + cache->index = (uint16_t)index; + write_u32(cache->version, type->tp_version_tag); + specialize(instr, hint_op); + return 1; +} + static int specialize_dict_access( PyObject *owner, _Py_CODEUNIT *instr, PyTypeObject *type, @@ -979,55 +1037,17 @@ specialize_dict_access( SPECIALIZATION_FAIL(base_op, SPEC_FAIL_ATTR_NOT_MANAGED_DICT); return 0; } - _PyAttrCache *cache = (_PyAttrCache *)(instr + 1); if (type->tp_flags & Py_TPFLAGS_INLINE_VALUES && _PyObject_InlineValues(owner)->valid && !(base_op == STORE_ATTR && _PyObject_GetManagedDict(owner) != NULL)) { - PyDictKeysObject *keys = ((PyHeapTypeObject *)type)->ht_cached_keys; - assert(PyUnicode_CheckExact(name)); - Py_ssize_t index = _PyDictKeys_StringLookup(keys, name); - assert (index != DKIX_ERROR); - if (index == DKIX_EMPTY) { - SPECIALIZATION_FAIL(base_op, SPEC_FAIL_ATTR_NOT_IN_KEYS); - return 0; - } - assert(index >= 0); - char *value_addr = (char *)&_PyObject_InlineValues(owner)->values[index]; - Py_ssize_t offset = value_addr - (char *)owner; - if (offset != (uint16_t)offset) { - SPECIALIZATION_FAIL(base_op, SPEC_FAIL_OUT_OF_RANGE); - return 0; - } - write_u32(cache->version, type->tp_version_tag); - cache->index = (uint16_t)offset; - specialize(instr, values_op); + return specialize_inline_values_access( + owner, instr, type, name, base_op, values_op); } else { - PyDictObject *dict = _PyObject_GetManagedDict(owner); - if (dict == NULL || !PyDict_CheckExact(dict)) { - SPECIALIZATION_FAIL(base_op, SPEC_FAIL_NO_DICT); - return 0; - } - // We found an instance with a __dict__. - if (dict->ma_values) { - SPECIALIZATION_FAIL(base_op, SPEC_FAIL_ATTR_SPLIT_DICT); - return 0; - } - Py_ssize_t index = - _PyDict_LookupIndex(dict, name); - if (index != (uint16_t)index) { - SPECIALIZATION_FAIL(base_op, - index == DKIX_EMPTY ? - SPEC_FAIL_ATTR_NOT_IN_DICT : - SPEC_FAIL_OUT_OF_RANGE); - return 0; - } - cache->index = (uint16_t)index; - write_u32(cache->version, type->tp_version_tag); - specialize(instr, hint_op); + return specialize_managed_dict_access( + owner, instr, type, name, base_op, hint_op); } - return 1; } static int From feb7d340f25908b3b91640b76f92ca30c44bc0ef Mon Sep 17 00:00:00 2001 From: Matt Page Date: Wed, 11 Dec 2024 16:05:03 -0800 Subject: [PATCH 14/67] Pass type version to specialize_dict_access --- Python/specialize.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/Python/specialize.c b/Python/specialize.c index 86055dff25d251..c7d7bfc929cba0 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -968,6 +968,7 @@ analyze_descriptor(PyTypeObject *type, PyObject *name, PyObject **descr, unsigne static int specialize_inline_values_access( PyObject *owner, _Py_CODEUNIT *instr, PyTypeObject *type, + unsigned int tp_version, PyObject *name, int base_op, int values_op) { PyDictKeysObject *keys = ((PyHeapTypeObject *)type)->ht_cached_keys; @@ -986,7 +987,7 @@ specialize_inline_values_access( SPECIALIZATION_FAIL(base_op, SPEC_FAIL_OUT_OF_RANGE); return 0; } - write_u32(cache->version, type->tp_version_tag); + write_u32(cache->version, tp_version); cache->index = (uint16_t)offset; specialize(instr, values_op); return 1; @@ -995,6 +996,7 @@ specialize_inline_values_access( static int specialize_managed_dict_access( PyObject *owner, _Py_CODEUNIT *instr, PyTypeObject *type, + unsigned int tp_version, PyObject *name, int base_op, int hint_op) { PyDictObject *dict = _PyObject_GetManagedDict(owner); @@ -1018,7 +1020,7 @@ specialize_managed_dict_access( return 0; } cache->index = (uint16_t)index; - write_u32(cache->version, type->tp_version_tag); + write_u32(cache->version, tp_version); specialize(instr, hint_op); return 1; } @@ -1026,7 +1028,7 @@ specialize_managed_dict_access( static int specialize_dict_access( PyObject *owner, _Py_CODEUNIT *instr, PyTypeObject *type, - DescriptorClassification kind, PyObject *name, + unsigned int tp_version, DescriptorClassification kind, PyObject *name, int base_op, int values_op, int hint_op) { assert(kind == NON_OVERRIDING || kind == NON_DESCRIPTOR || kind == ABSENT || @@ -1042,11 +1044,11 @@ specialize_dict_access( !(base_op == STORE_ATTR && _PyObject_GetManagedDict(owner) != NULL)) { return specialize_inline_values_access( - owner, instr, type, name, base_op, values_op); + owner, instr, type, tp_version, name, base_op, values_op); } else { return specialize_managed_dict_access( - owner, instr, type, name, base_op, hint_op); + owner, instr, type, tp_version, name, base_op, hint_op); } } @@ -1294,7 +1296,7 @@ do_specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* try_instance: FT_UNIMPLEMENTED(); - if (specialize_dict_access(owner, instr, type, kind, name, LOAD_ATTR, + if (specialize_dict_access(owner, instr, type, tp_version, kind, name, LOAD_ATTR, LOAD_ATTR_INSTANCE_VALUE, LOAD_ATTR_WITH_HINT)) { return 0; @@ -1442,8 +1444,8 @@ _Py_Specialize_StoreAttr(_PyStackRef owner_st, _Py_CODEUNIT *instr, PyObject *na SPECIALIZATION_FAIL(STORE_ATTR, SPEC_FAIL_ATTR_CLASS_ATTR_SIMPLE); goto fail; case ABSENT: - if (specialize_dict_access(owner, instr, type, kind, name, STORE_ATTR, - STORE_ATTR_INSTANCE_VALUE, STORE_ATTR_WITH_HINT)) + if (specialize_dict_access(owner, instr, type, tp_version, kind, name, STORE_ATTR, + STORE_ATTR_INSTANCE_VALUE, STORE_ATTR_WITH_HINT)) { goto success; } From ecfd199b81282b8d47a44ab296139cc4dcc93dc2 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Wed, 11 Dec 2024 16:13:18 -0800 Subject: [PATCH 15/67] Take a critical section around dict specialization --- Python/specialize.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/Python/specialize.c b/Python/specialize.c index c7d7bfc929cba0..51b43e24e489d1 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -3,6 +3,7 @@ #include "opcode.h" #include "pycore_code.h" +#include "pycore_critical_section.h" #include "pycore_descrobject.h" // _PyMethodWrapper_Type #include "pycore_dict.h" // DICT_KEYS_UNICODE #include "pycore_function.h" // _PyFunction_GetVersionForCurrentState() @@ -966,7 +967,7 @@ analyze_descriptor(PyTypeObject *type, PyObject *name, PyObject **descr, unsigne } static int -specialize_inline_values_access( +specialize_inline_values_access_lock_held( PyObject *owner, _Py_CODEUNIT *instr, PyTypeObject *type, unsigned int tp_version, PyObject *name, int base_op, int values_op) @@ -974,6 +975,7 @@ specialize_inline_values_access( PyDictKeysObject *keys = ((PyHeapTypeObject *)type)->ht_cached_keys; _PyAttrCache *cache = (_PyAttrCache *)(instr + 1); assert(PyUnicode_CheckExact(name)); + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(owner); Py_ssize_t index = _PyDictKeys_StringLookup(keys, name); assert (index != DKIX_ERROR); if (index == DKIX_EMPTY) { @@ -994,13 +996,16 @@ specialize_inline_values_access( } static int -specialize_managed_dict_access( +specialize_managed_dict_access_lock_held( PyObject *owner, _Py_CODEUNIT *instr, PyTypeObject *type, unsigned int tp_version, PyObject *name, int base_op, int hint_op) { PyDictObject *dict = _PyObject_GetManagedDict(owner); _PyAttrCache *cache = (_PyAttrCache *)(instr + 1); + + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(owner); + if (dict == NULL || !PyDict_CheckExact(dict)) { SPECIALIZATION_FAIL(base_op, SPEC_FAIL_NO_DICT); return 0; @@ -1039,17 +1044,21 @@ specialize_dict_access( SPECIALIZATION_FAIL(base_op, SPEC_FAIL_ATTR_NOT_MANAGED_DICT); return 0; } + int result; + Py_BEGIN_CRITICAL_SECTION(owner); if (type->tp_flags & Py_TPFLAGS_INLINE_VALUES && _PyObject_InlineValues(owner)->valid && !(base_op == STORE_ATTR && _PyObject_GetManagedDict(owner) != NULL)) { - return specialize_inline_values_access( + result = specialize_inline_values_access_lock_held( owner, instr, type, tp_version, name, base_op, values_op); } else { - return specialize_managed_dict_access( + result = specialize_managed_dict_access_lock_held( owner, instr, type, tp_version, name, base_op, hint_op); } + Py_END_CRITICAL_SECTION(); + return result; } static int From 9fda5dbc6dad4a515f72c89b761eae82fc9cd526 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Wed, 11 Dec 2024 16:14:22 -0800 Subject: [PATCH 16/67] Use thread-safe version of _PyDictKeys_StringLookup --- Python/specialize.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Python/specialize.c b/Python/specialize.c index 51b43e24e489d1..4bc13f4a97ad50 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -976,7 +976,8 @@ specialize_inline_values_access_lock_held( _PyAttrCache *cache = (_PyAttrCache *)(instr + 1); assert(PyUnicode_CheckExact(name)); _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(owner); - Py_ssize_t index = _PyDictKeys_StringLookup(keys, name); + uint32_t version; + Py_ssize_t index = _PyDictKeys_StringLookupAndVersion(keys, name, &version); assert (index != DKIX_ERROR); if (index == DKIX_EMPTY) { SPECIALIZATION_FAIL(base_op, SPEC_FAIL_ATTR_NOT_IN_KEYS); From 3b2c22035788c9cdd272347d0fc38f39f779d491 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Wed, 11 Dec 2024 16:18:33 -0800 Subject: [PATCH 17/67] Use atomic load in _CHECK_MANAGED_OBJECT_HAS_VALUES --- Python/bytecodes.c | 2 +- Python/executor_cases.c.h | 2 +- Python/generated_cases.c.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Python/bytecodes.c b/Python/bytecodes.c index cc4b39d30a86a0..e75cdf9d1d1ce1 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2136,7 +2136,7 @@ dummy_func( PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); assert(Py_TYPE(owner_o)->tp_dictoffset < 0); assert(Py_TYPE(owner_o)->tp_flags & Py_TPFLAGS_INLINE_VALUES); - DEOPT_IF(!_PyObject_InlineValues(owner_o)->valid); + DEOPT_IF(!FT_ATOMIC_LOAD_UINT8(_PyObject_InlineValues(owner_o)->valid)); } split op(_LOAD_ATTR_INSTANCE_VALUE, (offset/1, owner -- attr, null if (oparg & 1))) { diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index 9d96f531b7137e..4746aa09d6458d 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -2583,7 +2583,7 @@ PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); assert(Py_TYPE(owner_o)->tp_dictoffset < 0); assert(Py_TYPE(owner_o)->tp_flags & Py_TPFLAGS_INLINE_VALUES); - if (!_PyObject_InlineValues(owner_o)->valid) { + if (!FT_ATOMIC_LOAD_UINT8(_PyObject_InlineValues(owner_o)->valid)) { UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); } diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 8b55f89ba3ba48..8381ca5d03934e 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -5397,7 +5397,7 @@ PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); assert(Py_TYPE(owner_o)->tp_dictoffset < 0); assert(Py_TYPE(owner_o)->tp_flags & Py_TPFLAGS_INLINE_VALUES); - DEOPT_IF(!_PyObject_InlineValues(owner_o)->valid, LOAD_ATTR); + DEOPT_IF(!FT_ATOMIC_LOAD_UINT8(_PyObject_InlineValues(owner_o)->valid), LOAD_ATTR); } // _LOAD_ATTR_INSTANCE_VALUE { From 408e44b23b1c3dcb9586400e71adc09ac345522e Mon Sep 17 00:00:00 2001 From: Matt Page Date: Wed, 11 Dec 2024 16:29:27 -0800 Subject: [PATCH 18/67] Make _LOAD_ATTR_INSTANCE_VALUE thread-safe - Use atomic load for value - Use _Py_TryIncrefCompareStackRef for incref --- Python/bytecodes.c | 12 +++++++++--- Python/executor_cases.c.h | 30 ++++++++++++++++++++++++------ Python/generated_cases.c.h | 12 +++++++++--- 3 files changed, 42 insertions(+), 12 deletions(-) diff --git a/Python/bytecodes.c b/Python/bytecodes.c index e75cdf9d1d1ce1..e7e09be6df1bc9 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2142,12 +2142,18 @@ dummy_func( split op(_LOAD_ATTR_INSTANCE_VALUE, (offset/1, owner -- attr, null if (oparg & 1))) { PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); PyObject **value_ptr = (PyObject**)(((char *)owner_o) + offset); - PyObject *attr_o = *value_ptr; + PyObject *attr_o = FT_ATOMIC_LOAD_PTR_ACQUIRE(*value_ptr); DEOPT_IF(attr_o == NULL); - STAT_INC(LOAD_ATTR, hit); + #ifdef Py_GIL_DISABLED + if (!_Py_TryIncrefCompareStackRef(value_ptr, attr_o, &attr)) { + DEOPT_IF(true); + } + #else Py_INCREF(attr_o); - null = PyStackRef_NULL; attr = PyStackRef_FromPyObjectSteal(attr_o); + #endif + STAT_INC(LOAD_ATTR, hit); + null = PyStackRef_NULL; DECREF_INPUTS(); } diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index 4746aa09d6458d..8e69a33cad801c 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -2599,15 +2599,24 @@ uint16_t offset = (uint16_t)CURRENT_OPERAND0(); PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); PyObject **value_ptr = (PyObject**)(((char *)owner_o) + offset); - PyObject *attr_o = *value_ptr; + PyObject *attr_o = FT_ATOMIC_LOAD_PTR_ACQUIRE(*value_ptr); if (attr_o == NULL) { UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); } - STAT_INC(LOAD_ATTR, hit); + #ifdef Py_GIL_DISABLED + if (!_Py_TryIncrefCompareStackRef(value_ptr, attr_o, &attr)) { + if (true) { + UOP_STAT_INC(uopcode, miss); + JUMP_TO_JUMP_TARGET(); + } + } + #else Py_INCREF(attr_o); - null = PyStackRef_NULL; attr = PyStackRef_FromPyObjectSteal(attr_o); + #endif + STAT_INC(LOAD_ATTR, hit); + null = PyStackRef_NULL; PyStackRef_CLOSE(owner); stack_pointer[-1] = attr; break; @@ -2622,15 +2631,24 @@ uint16_t offset = (uint16_t)CURRENT_OPERAND0(); PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); PyObject **value_ptr = (PyObject**)(((char *)owner_o) + offset); - PyObject *attr_o = *value_ptr; + PyObject *attr_o = FT_ATOMIC_LOAD_PTR_ACQUIRE(*value_ptr); if (attr_o == NULL) { UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); } - STAT_INC(LOAD_ATTR, hit); + #ifdef Py_GIL_DISABLED + if (!_Py_TryIncrefCompareStackRef(value_ptr, attr_o, &attr)) { + if (true) { + UOP_STAT_INC(uopcode, miss); + JUMP_TO_JUMP_TARGET(); + } + } + #else Py_INCREF(attr_o); - null = PyStackRef_NULL; attr = PyStackRef_FromPyObjectSteal(attr_o); + #endif + STAT_INC(LOAD_ATTR, hit); + null = PyStackRef_NULL; PyStackRef_CLOSE(owner); stack_pointer[-1] = attr; stack_pointer[0] = null; diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 8381ca5d03934e..568a846663fe75 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -5404,12 +5404,18 @@ uint16_t offset = read_u16(&this_instr[4].cache); PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); PyObject **value_ptr = (PyObject**)(((char *)owner_o) + offset); - PyObject *attr_o = *value_ptr; + PyObject *attr_o = FT_ATOMIC_LOAD_PTR_ACQUIRE(*value_ptr); DEOPT_IF(attr_o == NULL, LOAD_ATTR); - STAT_INC(LOAD_ATTR, hit); + #ifdef Py_GIL_DISABLED + if (!_Py_TryIncrefCompareStackRef(value_ptr, attr_o, &attr)) { + DEOPT_IF(true, LOAD_ATTR); + } + #else Py_INCREF(attr_o); - null = PyStackRef_NULL; attr = PyStackRef_FromPyObjectSteal(attr_o); + #endif + STAT_INC(LOAD_ATTR, hit); + null = PyStackRef_NULL; PyStackRef_CLOSE(owner); } /* Skip 5 cache entries */ From 16aab70d79ca5f54ec6b289826ec90cbbd815414 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 12 Dec 2024 13:22:50 -0800 Subject: [PATCH 19/67] Make _LOAD_ATTR_WITH_HINT thread-safe --- Python/bytecodes.c | 22 ++++++++++++++++++---- Python/executor_cases.c.h | 31 ++++++++++++++++++++++++------- Python/generated_cases.c.h | 22 ++++++++++++++++++---- 3 files changed, 60 insertions(+), 15 deletions(-) diff --git a/Python/bytecodes.c b/Python/bytecodes.c index e7e09be6df1bc9..52ed2ba2847445 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2217,16 +2217,30 @@ dummy_func( PyObject *attr_o; PyDictObject *dict = _PyObject_GetManagedDict(owner_o); - DEOPT_IF(hint >= (size_t)dict->ma_keys->dk_nentries); + DEOPT_IF(!LOCK_OBJECT(dict)); + if (hint >= (size_t)dict->ma_keys->dk_nentries) { + UNLOCK_OBJECT(dict); + DEOPT_IF(true); + } PyObject *name = GETITEM(FRAME_CO_NAMES, oparg>>1); - DEOPT_IF(!DK_IS_UNICODE(dict->ma_keys)); + if (!DK_IS_UNICODE(dict->ma_keys)) { + UNLOCK_OBJECT(dict); + DEOPT_IF(true); + } PyDictUnicodeEntry *ep = DK_UNICODE_ENTRIES(dict->ma_keys) + hint; - DEOPT_IF(ep->me_key != name); + if (ep->me_key != name) { + UNLOCK_OBJECT(dict); + DEOPT_IF(true); + } attr_o = ep->me_value; - DEOPT_IF(attr_o == NULL); + if (attr_o == NULL) { + UNLOCK_OBJECT(dict); + DEOPT_IF(true); + } STAT_INC(LOAD_ATTR, hit); Py_INCREF(attr_o); attr = PyStackRef_FromPyObjectSteal(attr_o); + UNLOCK_OBJECT(dict); null = PyStackRef_NULL; DECREF_INPUTS(); } diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index 8e69a33cad801c..b8b2395cfa031b 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -2749,28 +2749,45 @@ PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); PyObject *attr_o; PyDictObject *dict = _PyObject_GetManagedDict(owner_o); - if (hint >= (size_t)dict->ma_keys->dk_nentries) { + if (!LOCK_OBJECT(dict)) { UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); } + if (hint >= (size_t)dict->ma_keys->dk_nentries) { + UNLOCK_OBJECT(dict); + if (true) { + UOP_STAT_INC(uopcode, miss); + JUMP_TO_JUMP_TARGET(); + } + } PyObject *name = GETITEM(FRAME_CO_NAMES, oparg>>1); if (!DK_IS_UNICODE(dict->ma_keys)) { - UOP_STAT_INC(uopcode, miss); - JUMP_TO_JUMP_TARGET(); + UNLOCK_OBJECT(dict); + if (true) { + UOP_STAT_INC(uopcode, miss); + JUMP_TO_JUMP_TARGET(); + } } PyDictUnicodeEntry *ep = DK_UNICODE_ENTRIES(dict->ma_keys) + hint; if (ep->me_key != name) { - UOP_STAT_INC(uopcode, miss); - JUMP_TO_JUMP_TARGET(); + UNLOCK_OBJECT(dict); + if (true) { + UOP_STAT_INC(uopcode, miss); + JUMP_TO_JUMP_TARGET(); + } } attr_o = ep->me_value; if (attr_o == NULL) { - UOP_STAT_INC(uopcode, miss); - JUMP_TO_JUMP_TARGET(); + UNLOCK_OBJECT(dict); + if (true) { + UOP_STAT_INC(uopcode, miss); + JUMP_TO_JUMP_TARGET(); + } } STAT_INC(LOAD_ATTR, hit); Py_INCREF(attr_o); attr = PyStackRef_FromPyObjectSteal(attr_o); + UNLOCK_OBJECT(dict); null = PyStackRef_NULL; PyStackRef_CLOSE(owner); stack_pointer[-1] = attr; diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 568a846663fe75..7a6e58a30b47f7 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -5815,16 +5815,30 @@ PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); PyObject *attr_o; PyDictObject *dict = _PyObject_GetManagedDict(owner_o); - DEOPT_IF(hint >= (size_t)dict->ma_keys->dk_nentries, LOAD_ATTR); + DEOPT_IF(!LOCK_OBJECT(dict), LOAD_ATTR); + if (hint >= (size_t)dict->ma_keys->dk_nentries) { + UNLOCK_OBJECT(dict); + DEOPT_IF(true, LOAD_ATTR); + } PyObject *name = GETITEM(FRAME_CO_NAMES, oparg>>1); - DEOPT_IF(!DK_IS_UNICODE(dict->ma_keys), LOAD_ATTR); + if (!DK_IS_UNICODE(dict->ma_keys)) { + UNLOCK_OBJECT(dict); + DEOPT_IF(true, LOAD_ATTR); + } PyDictUnicodeEntry *ep = DK_UNICODE_ENTRIES(dict->ma_keys) + hint; - DEOPT_IF(ep->me_key != name, LOAD_ATTR); + if (ep->me_key != name) { + UNLOCK_OBJECT(dict); + DEOPT_IF(true, LOAD_ATTR); + } attr_o = ep->me_value; - DEOPT_IF(attr_o == NULL, LOAD_ATTR); + if (attr_o == NULL) { + UNLOCK_OBJECT(dict); + DEOPT_IF(true, LOAD_ATTR); + } STAT_INC(LOAD_ATTR, hit); Py_INCREF(attr_o); attr = PyStackRef_FromPyObjectSteal(attr_o); + UNLOCK_OBJECT(dict); null = PyStackRef_NULL; PyStackRef_CLOSE(owner); } From d0920ea8241eede3f7a1fb676354cf0871a09242 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 12 Dec 2024 13:43:08 -0800 Subject: [PATCH 20/67] Specialize instance accesses --- Lib/test/test_opcache.py | 4 ++-- Python/specialize.c | 5 ----- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/Lib/test/test_opcache.py b/Lib/test/test_opcache.py index 0a7557adc4763b..830299e48e849c 100644 --- a/Lib/test/test_opcache.py +++ b/Lib/test/test_opcache.py @@ -777,7 +777,7 @@ def write(items): opname = "LOAD_ATTR_GETATTRIBUTE_OVERRIDDEN" self.assert_races_do_not_crash(opname, get_items, read, write) - @requires_specialization + @requires_specialization_ft def test_load_attr_instance_value(self): def get_items(): class C: @@ -947,7 +947,7 @@ def write(items): opname = "LOAD_ATTR_PROPERTY" self.assert_races_do_not_crash(opname, get_items, read, write) - @requires_specialization + @requires_specialization_ft def test_load_attr_with_hint(self): def get_items(): class C: diff --git a/Python/specialize.c b/Python/specialize.c index 4bc13f4a97ad50..b3c86167864ad5 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -1153,7 +1153,6 @@ do_specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* case METHOD: { if (shadow) { - FT_UNIMPLEMENTED(); goto try_instance; } int oparg = instr->op.arg; @@ -1282,8 +1281,6 @@ do_specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* } return -1; case NON_DESCRIPTOR: - FT_UNIMPLEMENTED(); - if (shadow) { goto try_instance; } @@ -1304,8 +1301,6 @@ do_specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* } Py_UNREACHABLE(); try_instance: - FT_UNIMPLEMENTED(); - if (specialize_dict_access(owner, instr, type, tp_version, kind, name, LOAD_ATTR, LOAD_ATTR_INSTANCE_VALUE, LOAD_ATTR_WITH_HINT)) { From e7cea822b355e3b7eef29ef80ac97497546bdf06 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 12 Dec 2024 15:55:10 -0800 Subject: [PATCH 21/67] Specialize LOAD_ATTR_SLOT - Use atomics and _Py_TryIncrefCompareStackRef in _LOAD_ATTR_SLOT - Pass type version during specialization --- Lib/test/test_opcache.py | 27 +++++++++++++++++++++++++++ Python/bytecodes.c | 14 +++++++++++--- Python/executor_cases.c.h | 32 ++++++++++++++++++++++++++------ Python/generated_cases.c.h | 13 ++++++++++--- Python/specialize.c | 6 ++---- 5 files changed, 76 insertions(+), 16 deletions(-) diff --git a/Lib/test/test_opcache.py b/Lib/test/test_opcache.py index 830299e48e849c..a1b747e7a8d34e 100644 --- a/Lib/test/test_opcache.py +++ b/Lib/test/test_opcache.py @@ -947,6 +947,33 @@ def write(items): opname = "LOAD_ATTR_PROPERTY" self.assert_races_do_not_crash(opname, get_items, read, write) + @requires_specialization_ft + def test_load_attr_slot(self): + def get_items(): + class C: + __slots__ = ["a", "b"] + + items = [] + for i in range(self.ITEMS): + item = C() + item.a = i + item.b = i + self.ITEMS + items.append(item) + return items + + def read(items): + for item in items: + item.a + item.b + + def write(items): + for item in items: + item.a = 100 + item.b = 200 + + opname = "LOAD_ATTR_SLOT" + self.assert_races_do_not_crash(opname, get_items, read, write) + @requires_specialization_ft def test_load_attr_with_hint(self): def get_items(): diff --git a/Python/bytecodes.c b/Python/bytecodes.c index 52ed2ba2847445..c7bb4a4928ec62 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2255,12 +2255,20 @@ dummy_func( split op(_LOAD_ATTR_SLOT, (index/1, owner -- attr, null if (oparg & 1))) { PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); - char *addr = (char *)owner_o + index; - PyObject *attr_o = *(PyObject **)addr; + PyObject **addr = (PyObject **)((char *)owner_o + index); + PyObject *attr_o = FT_ATOMIC_LOAD_PTR(*addr); DEOPT_IF(attr_o == NULL); + #ifdef Py_GIL_DISABLED + int increfed = _Py_TryIncrefCompareStackRef(addr, attr_o, &attr); + DEOPT_IF(!increfed); + #else + // XXX - Bug in cases generator + Py_INCREF(attr_o); + attr = PyStackRef_FromPyObjectSteal(attr_o); + #endif STAT_INC(LOAD_ATTR, hit); + null = PyStackRef_NULL; - attr = PyStackRef_FromPyObjectNew(attr_o); DECREF_INPUTS(); } diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index b8b2395cfa031b..6c3e654ebc17f8 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -2805,15 +2805,25 @@ owner = stack_pointer[-1]; uint16_t index = (uint16_t)CURRENT_OPERAND0(); PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); - char *addr = (char *)owner_o + index; - PyObject *attr_o = *(PyObject **)addr; + PyObject **addr = (PyObject **)((char *)owner_o + index); + PyObject *attr_o = FT_ATOMIC_LOAD_PTR(*addr); if (attr_o == NULL) { UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); } + #ifdef Py_GIL_DISABLED + int increfed = _Py_TryIncrefCompareStackRef(addr, attr_o, &attr); + if (!increfed) { + UOP_STAT_INC(uopcode, miss); + JUMP_TO_JUMP_TARGET(); + } + #else + // XXX - Bug in cases generator + Py_INCREF(attr_o); + attr = PyStackRef_FromPyObjectSteal(attr_o); + #endif STAT_INC(LOAD_ATTR, hit); null = PyStackRef_NULL; - attr = PyStackRef_FromPyObjectNew(attr_o); PyStackRef_CLOSE(owner); stack_pointer[-1] = attr; break; @@ -2827,15 +2837,25 @@ owner = stack_pointer[-1]; uint16_t index = (uint16_t)CURRENT_OPERAND0(); PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); - char *addr = (char *)owner_o + index; - PyObject *attr_o = *(PyObject **)addr; + PyObject **addr = (PyObject **)((char *)owner_o + index); + PyObject *attr_o = FT_ATOMIC_LOAD_PTR(*addr); if (attr_o == NULL) { UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); } + #ifdef Py_GIL_DISABLED + int increfed = _Py_TryIncrefCompareStackRef(addr, attr_o, &attr); + if (!increfed) { + UOP_STAT_INC(uopcode, miss); + JUMP_TO_JUMP_TARGET(); + } + #else + // XXX - Bug in cases generator + Py_INCREF(attr_o); + attr = PyStackRef_FromPyObjectSteal(attr_o); + #endif STAT_INC(LOAD_ATTR, hit); null = PyStackRef_NULL; - attr = PyStackRef_FromPyObjectNew(attr_o); PyStackRef_CLOSE(owner); stack_pointer[-1] = attr; stack_pointer[0] = null; diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 7a6e58a30b47f7..1990bab5bfcc39 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -5768,12 +5768,19 @@ { uint16_t index = read_u16(&this_instr[4].cache); PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); - char *addr = (char *)owner_o + index; - PyObject *attr_o = *(PyObject **)addr; + PyObject **addr = (PyObject **)((char *)owner_o + index); + PyObject *attr_o = FT_ATOMIC_LOAD_PTR(*addr); DEOPT_IF(attr_o == NULL, LOAD_ATTR); + #ifdef Py_GIL_DISABLED + int increfed = _Py_TryIncrefCompareStackRef(addr, attr_o, &attr); + DEOPT_IF(!increfed, LOAD_ATTR); + #else + // XXX - Bug in cases generator + Py_INCREF(attr_o); + attr = PyStackRef_FromPyObjectSteal(attr_o); + #endif STAT_INC(LOAD_ATTR, hit); null = PyStackRef_NULL; - attr = PyStackRef_FromPyObjectNew(attr_o); PyStackRef_CLOSE(owner); } /* Skip 5 cache entries */ diff --git a/Python/specialize.c b/Python/specialize.c index b3c86167864ad5..8bab84cca546cf 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -1203,7 +1203,6 @@ do_specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* } case OBJECT_SLOT: { - FT_UNIMPLEMENTED(); PyMemberDescrObject *member = (PyMemberDescrObject *)descr; struct PyMemberDef *dmem = member->d_member; Py_ssize_t offset = dmem->offset; @@ -1222,17 +1221,16 @@ do_specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* assert(dmem->type == Py_T_OBJECT_EX || dmem->type == _Py_T_OBJECT); assert(offset > 0); cache->index = (uint16_t)offset; - write_u32(cache->version, type->tp_version_tag); + write_u32(cache->version, tp_version); specialize(instr, LOAD_ATTR_SLOT); return 0; } case DUNDER_CLASS: { - FT_UNIMPLEMENTED(); Py_ssize_t offset = offsetof(PyObject, ob_type); assert(offset == (uint16_t)offset); cache->index = (uint16_t)offset; - write_u32(cache->version, type->tp_version_tag); + write_u32(cache->version, tp_version); specialize(instr, LOAD_ATTR_SLOT); return 0; } From 8dac8c4149dc2f9d3269406dcc599b65b32a0268 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 12 Dec 2024 16:56:14 -0800 Subject: [PATCH 22/67] Enable LOAD_ATTR_PROPERTY - Check that fget is deferred - Pass tp_version --- Python/specialize.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/Python/specialize.c b/Python/specialize.c index 8bab84cca546cf..3c62945a6be576 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -1171,7 +1171,6 @@ do_specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* } case PROPERTY: { - FT_UNIMPLEMENTED(); _PyLoadMethodCache *lm_cache = (_PyLoadMethodCache *)(instr + 1); assert(Py_TYPE(descr) == &PyProperty_Type); PyObject *fget = ((_PyPropertyObject *)descr)->prop_get; @@ -1194,8 +1193,14 @@ do_specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_OTHER); return -1; } - assert(type->tp_version_tag != 0); - write_u32(lm_cache->type_version, type->tp_version_tag); + #ifdef Py_GIL_DISABLED + if (!_PyObject_HasDeferredRefcount(fget)) { + SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_ATTR_DESCR_NOT_DEFERRED); + return -1; + } + #endif + assert(tp_version != 0); + write_u32(lm_cache->type_version, tp_version); /* borrowed */ write_obj(lm_cache->descr, fget); specialize(instr, LOAD_ATTR_PROPERTY); From d29b3aaf0ce378e92a21498271bd65993a7efc84 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 12 Dec 2024 17:01:46 -0800 Subject: [PATCH 23/67] Checkpoint LOAD_ATTR_GETATTRIBUTE_OVERRIDEN --- Python/bytecodes.c | 2 +- Python/generated_cases.c.h | 2 +- Python/specialize.c | 9 +++++++-- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/Python/bytecodes.c b/Python/bytecodes.c index c7bb4a4928ec62..2c7a9d0cde81e2 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2337,7 +2337,7 @@ dummy_func( DEOPT_IF(tstate->interp->eval_frame); PyTypeObject *cls = Py_TYPE(owner_o); assert(type_version != 0); - DEOPT_IF(cls->tp_version_tag != type_version); + DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(cls->tp_version_tag) != type_version); assert(Py_IS_TYPE(getattribute, &PyFunction_Type)); PyFunctionObject *f = (PyFunctionObject *)getattribute; assert(func_version != 0); diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 1990bab5bfcc39..c858461d8fdf77 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -5355,7 +5355,7 @@ DEOPT_IF(tstate->interp->eval_frame, LOAD_ATTR); PyTypeObject *cls = Py_TYPE(owner_o); assert(type_version != 0); - DEOPT_IF(cls->tp_version_tag != type_version, LOAD_ATTR); + DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(cls->tp_version_tag) != type_version, LOAD_ATTR); assert(Py_IS_TYPE(getattribute, &PyFunction_Type)); PyFunctionObject *f = (PyFunctionObject *)getattribute; assert(func_version != 0); diff --git a/Python/specialize.c b/Python/specialize.c index 3c62945a6be576..f46f1fda8f4c17 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -1252,7 +1252,6 @@ do_specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* { assert(type->tp_getattro == _Py_slot_tp_getattro); assert(Py_IS_TYPE(descr, &PyFunction_Type)); - FT_UNIMPLEMENTED(); _PyLoadMethodCache *lm_cache = (_PyLoadMethodCache *)(instr + 1); if (!function_check_args(descr, 2, LOAD_ATTR)) { return -1; @@ -1269,10 +1268,16 @@ do_specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_OTHER); return -1; } + #ifdef Py_GIL_DISABLED + if (!_PyObject_HasDeferredRefcount(descr)) { + SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_ATTR_DESCR_NOT_DEFERRED); + return -1; + } + #endif write_u32(lm_cache->keys_version, version); /* borrowed */ write_obj(lm_cache->descr, descr); - write_u32(lm_cache->type_version, type->tp_version_tag); + write_u32(lm_cache->type_version, tp_version); specialize(instr, LOAD_ATTR_GETATTRIBUTE_OVERRIDDEN); return 0; } From b0b8102fc1f13a63b2200b3dc151804f19fcd789 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 13 Dec 2024 10:22:22 -0800 Subject: [PATCH 24/67] Lock dict in instance_has_key --- Python/specialize.c | 23 ++++++----------------- 1 file changed, 6 insertions(+), 17 deletions(-) diff --git a/Python/specialize.c b/Python/specialize.c index f46f1fda8f4c17..7332dfca5c5673 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -1072,17 +1072,6 @@ specialize_attr_loadclassattr(PyObject *owner, _Py_CODEUNIT *instr, static int specialize_class_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* name); #endif -static PyDictObject * -get_managed_dict_ref(PyObject *obj) -{ - PyDictObject *dict; - Py_BEGIN_CRITICAL_SECTION(obj); - dict = _PyObject_GetManagedDict(obj); - Py_XINCREF(dict); - Py_END_CRITICAL_SECTION(); - return dict; -} - /* Returns true if instances of obj's class are * likely to have `name` in their __dict__. * For objects with inline values, we check in the shared keys. @@ -1101,17 +1090,17 @@ instance_has_key(PyObject *obj, PyObject *name, uint32_t *shared_keys_version) _PyDictKeys_StringLookupAndVersion(keys, name, shared_keys_version); return index >= 0; } - PyDictObject *dict = get_managed_dict_ref(obj); + PyDictObject *dict = _PyObject_GetManagedDict(obj); if (dict == NULL || !PyDict_CheckExact(dict)) { - Py_XDECREF(dict); return false; } - if (dict->ma_values) { - Py_DECREF(dict); + if (FT_ATOMIC_LOAD_PTR(dict->ma_values)) { return false; } - Py_ssize_t index = _PyDict_LookupIndex(dict, name); - Py_DECREF(dict); + Py_ssize_t index; + Py_BEGIN_CRITICAL_SECTION(dict); + index = _PyDict_LookupIndex(dict, name); + Py_END_CRITICAL_SECTION(); if (index < 0) { return false; } From 85dab0db71a7a0281589afc38a6e0f1a89caee0f Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 13 Dec 2024 10:41:32 -0800 Subject: [PATCH 25/67] Lock dict instead of owner when specializing for dict access --- Python/specialize.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/Python/specialize.c b/Python/specialize.c index 7332dfca5c5673..8de62ca617b5ac 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -998,19 +998,14 @@ specialize_inline_values_access_lock_held( static int specialize_managed_dict_access_lock_held( - PyObject *owner, _Py_CODEUNIT *instr, PyTypeObject *type, + PyDictObject *dict, _Py_CODEUNIT *instr, PyTypeObject *type, unsigned int tp_version, PyObject *name, int base_op, int hint_op) { - PyDictObject *dict = _PyObject_GetManagedDict(owner); _PyAttrCache *cache = (_PyAttrCache *)(instr + 1); - _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(owner); + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(dict); - if (dict == NULL || !PyDict_CheckExact(dict)) { - SPECIALIZATION_FAIL(base_op, SPEC_FAIL_NO_DICT); - return 0; - } // We found an instance with a __dict__. if (dict->ma_values) { SPECIALIZATION_FAIL(base_op, SPEC_FAIL_ATTR_SPLIT_DICT); @@ -1046,19 +1041,26 @@ specialize_dict_access( return 0; } int result; - Py_BEGIN_CRITICAL_SECTION(owner); if (type->tp_flags & Py_TPFLAGS_INLINE_VALUES && _PyObject_InlineValues(owner)->valid && !(base_op == STORE_ATTR && _PyObject_GetManagedDict(owner) != NULL)) { + Py_BEGIN_CRITICAL_SECTION(owner); result = specialize_inline_values_access_lock_held( owner, instr, type, tp_version, name, base_op, values_op); + Py_END_CRITICAL_SECTION(); } else { + PyDictObject *dict = _PyObject_GetManagedDict(owner); + if (dict == NULL || !PyDict_CheckExact(dict)) { + SPECIALIZATION_FAIL(base_op, SPEC_FAIL_NO_DICT); + return 0; + } + Py_BEGIN_CRITICAL_SECTION(dict); result = specialize_managed_dict_access_lock_held( - owner, instr, type, tp_version, name, base_op, hint_op); + dict, instr, type, tp_version, name, base_op, hint_op); + Py_END_CRITICAL_SECTION(); } - Py_END_CRITICAL_SECTION(); return result; } From 581869e78225e3d3723d47c7837d0263316a3c80 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 13 Dec 2024 10:42:13 -0800 Subject: [PATCH 26/67] Use atomic load for valid bit --- Python/specialize.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Python/specialize.c b/Python/specialize.c index 8de62ca617b5ac..00a0d52f40cef1 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -1042,7 +1042,7 @@ specialize_dict_access( } int result; if (type->tp_flags & Py_TPFLAGS_INLINE_VALUES && - _PyObject_InlineValues(owner)->valid && + FT_ATOMIC_LOAD_UINT8(_PyObject_InlineValues(owner)->valid) && !(base_op == STORE_ATTR && _PyObject_GetManagedDict(owner) != NULL)) { Py_BEGIN_CRITICAL_SECTION(owner); From 96be738b2a9d868b115819815dda9ad6b3eee1e3 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 13 Dec 2024 10:49:34 -0800 Subject: [PATCH 27/67] Use _PyDictKeys_StringLookup and lock around it, rather than wasting out param --- Python/specialize.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/Python/specialize.c b/Python/specialize.c index 00a0d52f40cef1..0af189e71e22b1 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -976,8 +976,13 @@ specialize_inline_values_access_lock_held( _PyAttrCache *cache = (_PyAttrCache *)(instr + 1); assert(PyUnicode_CheckExact(name)); _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(owner); - uint32_t version; - Py_ssize_t index = _PyDictKeys_StringLookupAndVersion(keys, name, &version); + #ifdef Py_GIL_DISABLED + PyMutex_LockFlags(&keys->dk_mutex, _Py_LOCK_DONT_DETACH); + #endif + Py_ssize_t index = _PyDictKeys_StringLookup(keys, name); + #ifdef Py_GIL_DISABLED + PyMutex_Unlock(&keys->dk_mutex); + #endif assert (index != DKIX_ERROR); if (index == DKIX_EMPTY) { SPECIALIZATION_FAIL(base_op, SPEC_FAIL_ATTR_NOT_IN_KEYS); From 9afe05243fc0e9f120481ade4cfa2812d0aa962e Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 13 Dec 2024 11:01:28 -0800 Subject: [PATCH 28/67] Fix cases_generator bug Macros should be treated as terminators when searching for the assignment target of an expression involving PyStackRef_FromPyObjectNew --- Lib/test/test_generated_cases.py | 35 +++++++++++++++++++++++++++++++ Python/bytecodes.c | 3 +-- Python/executor_cases.c.h | 6 ++---- Python/generated_cases.c.h | 3 +-- Tools/cases_generator/analyzer.py | 2 +- 5 files changed, 40 insertions(+), 9 deletions(-) diff --git a/Lib/test/test_generated_cases.py b/Lib/test/test_generated_cases.py index 9c65e81dfe4be1..6eed14314e5d4b 100644 --- a/Lib/test/test_generated_cases.py +++ b/Lib/test/test_generated_cases.py @@ -1639,6 +1639,41 @@ def test_escaping_call_next_to_cmacro(self): """ self.run_cases_test(input, output) + def test_pystackref_frompyobject_new_next_to_cmacro(self): + input = """ + inst(OP, (-- out1, out2)) { + PyObject *obj = SPAM(); + #ifdef Py_GIL_DISABLED + out1 = PyStackRef_FromPyObjectNew(obj); + #else + out1 = PyStackRef_FromPyObjectNew(obj); + #endif + out2 = PyStackRef_FromPyObjectNew(obj); + } + """ + output = """ + TARGET(OP) { + frame->instr_ptr = next_instr; + next_instr += 1; + INSTRUCTION_STATS(OP); + _PyStackRef out1; + _PyStackRef out2; + PyObject *obj = SPAM(); + #ifdef Py_GIL_DISABLED + out1 = PyStackRef_FromPyObjectNew(obj); + #else + out1 = PyStackRef_FromPyObjectNew(obj); + #endif + out2 = PyStackRef_FromPyObjectNew(obj); + stack_pointer[0] = out1; + stack_pointer[1] = out2; + stack_pointer += 2; + assert(WITHIN_STACK_BOUNDS()); + DISPATCH(); + } + """ + self.run_cases_test(input, output) + def test_pop_dead_inputs_all_live(self): input = """ inst(OP, (a, b --)) { diff --git a/Python/bytecodes.c b/Python/bytecodes.c index 2c7a9d0cde81e2..800ac6bd52de62 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2149,8 +2149,7 @@ dummy_func( DEOPT_IF(true); } #else - Py_INCREF(attr_o); - attr = PyStackRef_FromPyObjectSteal(attr_o); + attr = PyStackRef_FromPyObjectNew(attr_o); #endif STAT_INC(LOAD_ATTR, hit); null = PyStackRef_NULL; diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index 6c3e654ebc17f8..f0f462278f1a1c 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -2612,8 +2612,7 @@ } } #else - Py_INCREF(attr_o); - attr = PyStackRef_FromPyObjectSteal(attr_o); + attr = PyStackRef_FromPyObjectNew(attr_o); #endif STAT_INC(LOAD_ATTR, hit); null = PyStackRef_NULL; @@ -2644,8 +2643,7 @@ } } #else - Py_INCREF(attr_o); - attr = PyStackRef_FromPyObjectSteal(attr_o); + attr = PyStackRef_FromPyObjectNew(attr_o); #endif STAT_INC(LOAD_ATTR, hit); null = PyStackRef_NULL; diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index c858461d8fdf77..8d7100a8884556 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -5411,8 +5411,7 @@ DEOPT_IF(true, LOAD_ATTR); } #else - Py_INCREF(attr_o); - attr = PyStackRef_FromPyObjectSteal(attr_o); + attr = PyStackRef_FromPyObjectNew(attr_o); #endif STAT_INC(LOAD_ATTR, hit); null = PyStackRef_NULL; diff --git a/Tools/cases_generator/analyzer.py b/Tools/cases_generator/analyzer.py index eca851e6de87ae..d0496b8d777955 100644 --- a/Tools/cases_generator/analyzer.py +++ b/Tools/cases_generator/analyzer.py @@ -384,7 +384,7 @@ def find_assignment_target(node: parser.InstDef, idx: int) -> list[lexer.Token]: """Find the tokens that make up the left-hand side of an assignment""" offset = 0 for tkn in reversed(node.block.tokens[: idx]): - if tkn.kind in {"SEMI", "LBRACE", "RBRACE"}: + if tkn.kind in {"SEMI", "LBRACE", "RBRACE", "CMACRO"}: return node.block.tokens[idx - offset : idx] offset += 1 return [] From e190a0d5f25f35444fb19f6fd11092ce81d6c891 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 13 Dec 2024 11:24:18 -0800 Subject: [PATCH 29/67] Fix load --- Python/bytecodes.c | 2 +- Python/executor_cases.c.h | 2 +- Python/generated_cases.c.h | 28 ++++++++++++++-------------- 3 files changed, 16 insertions(+), 16 deletions(-) diff --git a/Python/bytecodes.c b/Python/bytecodes.c index 800ac6bd52de62..2229e2ef3c803d 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2129,7 +2129,7 @@ dummy_func( op(_GUARD_TYPE_VERSION, (type_version/2, owner -- owner)) { PyTypeObject *tp = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); assert(type_version != 0); - EXIT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(tp->tp_version_tag) != type_version); + EXIT_IF(FT_ATOMIC_LOAD_UINT_RELAXED(tp->tp_version_tag) != type_version); } op(_CHECK_MANAGED_OBJECT_HAS_VALUES, (owner -- owner)) { diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index f0f462278f1a1c..aefaaf29b2db7e 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -2570,7 +2570,7 @@ uint32_t type_version = (uint32_t)CURRENT_OPERAND0(); PyTypeObject *tp = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); assert(type_version != 0); - if (FT_ATOMIC_LOAD_UINT32_RELAXED(tp->tp_version_tag) != type_version) { + if (FT_ATOMIC_LOAD_UINT_RELAXED(tp->tp_version_tag) != type_version) { UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); } diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 8d7100a8884556..c99c55dd9dc9fe 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -5321,7 +5321,7 @@ uint32_t type_version = read_u32(&this_instr[4].cache); PyTypeObject *tp = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); assert(type_version != 0); - DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(tp->tp_version_tag) != type_version, LOAD_ATTR); + DEOPT_IF(FT_ATOMIC_LOAD_UINT_RELAXED(tp->tp_version_tag) != type_version, LOAD_ATTR); } // _LOAD_ATTR_CLASS { @@ -5390,7 +5390,7 @@ uint32_t type_version = read_u32(&this_instr[2].cache); PyTypeObject *tp = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); assert(type_version != 0); - DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(tp->tp_version_tag) != type_version, LOAD_ATTR); + DEOPT_IF(FT_ATOMIC_LOAD_UINT_RELAXED(tp->tp_version_tag) != type_version, LOAD_ATTR); } // _CHECK_MANAGED_OBJECT_HAS_VALUES { @@ -5440,7 +5440,7 @@ uint32_t type_version = read_u32(&this_instr[2].cache); PyTypeObject *tp = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); assert(type_version != 0); - DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(tp->tp_version_tag) != type_version, LOAD_ATTR); + DEOPT_IF(FT_ATOMIC_LOAD_UINT_RELAXED(tp->tp_version_tag) != type_version, LOAD_ATTR); } // _CHECK_ATTR_METHOD_LAZY_DICT { @@ -5483,7 +5483,7 @@ uint32_t type_version = read_u32(&this_instr[2].cache); PyTypeObject *tp = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); assert(type_version != 0); - DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(tp->tp_version_tag) != type_version, LOAD_ATTR); + DEOPT_IF(FT_ATOMIC_LOAD_UINT_RELAXED(tp->tp_version_tag) != type_version, LOAD_ATTR); } /* Skip 2 cache entries */ // _LOAD_ATTR_METHOD_NO_DICT @@ -5519,7 +5519,7 @@ uint32_t type_version = read_u32(&this_instr[2].cache); PyTypeObject *tp = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); assert(type_version != 0); - DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(tp->tp_version_tag) != type_version, LOAD_ATTR); + DEOPT_IF(FT_ATOMIC_LOAD_UINT_RELAXED(tp->tp_version_tag) != type_version, LOAD_ATTR); } // _GUARD_DORV_VALUES_INST_ATTR_FROM_DICT { @@ -5620,7 +5620,7 @@ uint32_t type_version = read_u32(&this_instr[2].cache); PyTypeObject *tp = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); assert(type_version != 0); - DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(tp->tp_version_tag) != type_version, LOAD_ATTR); + DEOPT_IF(FT_ATOMIC_LOAD_UINT_RELAXED(tp->tp_version_tag) != type_version, LOAD_ATTR); } /* Skip 2 cache entries */ // _LOAD_ATTR_NONDESCRIPTOR_NO_DICT @@ -5651,7 +5651,7 @@ uint32_t type_version = read_u32(&this_instr[2].cache); PyTypeObject *tp = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); assert(type_version != 0); - DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(tp->tp_version_tag) != type_version, LOAD_ATTR); + DEOPT_IF(FT_ATOMIC_LOAD_UINT_RELAXED(tp->tp_version_tag) != type_version, LOAD_ATTR); } // _GUARD_DORV_VALUES_INST_ATTR_FROM_DICT { @@ -5699,7 +5699,7 @@ uint32_t type_version = read_u32(&this_instr[2].cache); PyTypeObject *tp = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); assert(type_version != 0); - DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(tp->tp_version_tag) != type_version, LOAD_ATTR); + DEOPT_IF(FT_ATOMIC_LOAD_UINT_RELAXED(tp->tp_version_tag) != type_version, LOAD_ATTR); } /* Skip 2 cache entries */ // _LOAD_ATTR_PROPERTY_FRAME @@ -5761,7 +5761,7 @@ uint32_t type_version = read_u32(&this_instr[2].cache); PyTypeObject *tp = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); assert(type_version != 0); - DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(tp->tp_version_tag) != type_version, LOAD_ATTR); + DEOPT_IF(FT_ATOMIC_LOAD_UINT_RELAXED(tp->tp_version_tag) != type_version, LOAD_ATTR); } // _LOAD_ATTR_SLOT { @@ -5805,7 +5805,7 @@ uint32_t type_version = read_u32(&this_instr[2].cache); PyTypeObject *tp = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); assert(type_version != 0); - DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(tp->tp_version_tag) != type_version, LOAD_ATTR); + DEOPT_IF(FT_ATOMIC_LOAD_UINT_RELAXED(tp->tp_version_tag) != type_version, LOAD_ATTR); } // _CHECK_ATTR_WITH_HINT { @@ -7384,7 +7384,7 @@ uint32_t type_version = read_u32(&this_instr[2].cache); PyTypeObject *tp = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); assert(type_version != 0); - DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(tp->tp_version_tag) != type_version, STORE_ATTR); + DEOPT_IF(FT_ATOMIC_LOAD_UINT_RELAXED(tp->tp_version_tag) != type_version, STORE_ATTR); } // _GUARD_DORV_NO_DICT { @@ -7433,7 +7433,7 @@ uint32_t type_version = read_u32(&this_instr[2].cache); PyTypeObject *tp = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); assert(type_version != 0); - DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(tp->tp_version_tag) != type_version, STORE_ATTR); + DEOPT_IF(FT_ATOMIC_LOAD_UINT_RELAXED(tp->tp_version_tag) != type_version, STORE_ATTR); } // _STORE_ATTR_SLOT { @@ -7466,7 +7466,7 @@ uint32_t type_version = read_u32(&this_instr[2].cache); PyTypeObject *tp = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); assert(type_version != 0); - DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(tp->tp_version_tag) != type_version, STORE_ATTR); + DEOPT_IF(FT_ATOMIC_LOAD_UINT_RELAXED(tp->tp_version_tag) != type_version, STORE_ATTR); } // _STORE_ATTR_WITH_HINT { @@ -7840,7 +7840,7 @@ uint32_t type_version = read_u32(&this_instr[2].cache); PyTypeObject *tp = Py_TYPE(PyStackRef_AsPyObjectBorrow(owner)); assert(type_version != 0); - DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(tp->tp_version_tag) != type_version, TO_BOOL); + DEOPT_IF(FT_ATOMIC_LOAD_UINT_RELAXED(tp->tp_version_tag) != type_version, TO_BOOL); } // _REPLACE_WITH_TRUE { From 8c7836955917ca1de0b5ed093b537b069ed9fa48 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 13 Dec 2024 11:26:47 -0800 Subject: [PATCH 30/67] Remove FT_UNIMPLEMENTED All instance loads are complete! --- Python/specialize.c | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/Python/specialize.c b/Python/specialize.c index 0af189e71e22b1..93d1b4168f76f9 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -1114,20 +1114,6 @@ instance_has_key(PyObject *obj, PyObject *name, uint32_t *shared_keys_version) return true; } -#ifdef Py_GIL_DISABLED - -#define FT_UNIMPLEMENTED() \ - do { \ - SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_EXPECTED_ERROR); \ - return -1; \ - } while (0) - -#else - -#define FT_UNIMPLEMENTED() - -#endif - static int do_specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* name, bool shadow, uint32_t shared_keys_version, @@ -1313,8 +1299,6 @@ do_specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* return -1; } -#undef FT_UNIMPLEMENTED - static int specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* name) { From cdf8eb554f9955e78b87cdc3e89dea6878583cee Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 13 Dec 2024 12:07:40 -0800 Subject: [PATCH 31/67] Specialize class attribute loads - Use atomics where appropriate - Only cache deferred values --- Lib/test/test_opcache.py | 9 +++++++-- Python/bytecodes.c | 2 +- Python/executor_cases.c.h | 2 +- Python/generated_cases.c.h | 4 ++-- Python/specialize.c | 32 ++++++++++++++++---------------- 5 files changed, 27 insertions(+), 22 deletions(-) diff --git a/Lib/test/test_opcache.py b/Lib/test/test_opcache.py index a1b747e7a8d34e..38b8f08ffb32e1 100644 --- a/Lib/test/test_opcache.py +++ b/Lib/test/test_opcache.py @@ -717,11 +717,16 @@ def write(items): opname = "FOR_ITER_LIST" self.assert_races_do_not_crash(opname, get_items, read, write) - @requires_specialization + @requires_specialization_ft def test_load_attr_class(self): def get_items(): + class B: + pass + class C: - a = object() + # a must be set to an instance that uses deferred reference + # counting + a = B items = [] for _ in range(self.ITEMS): diff --git a/Python/bytecodes.c b/Python/bytecodes.c index 2229e2ef3c803d..b9b2d976d35591 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2282,7 +2282,7 @@ dummy_func( EXIT_IF(!PyType_Check(owner_o)); assert(type_version != 0); - EXIT_IF(((PyTypeObject *)owner_o)->tp_version_tag != type_version); + EXIT_IF(FT_ATOMIC_LOAD_UINT_RELAXED(((PyTypeObject *)owner_o)->tp_version_tag) != type_version); } split op(_LOAD_ATTR_CLASS, (descr/4, owner -- attr, null if (oparg & 1))) { diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index aefaaf29b2db7e..3dea6a1fe67d69 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -2874,7 +2874,7 @@ JUMP_TO_JUMP_TARGET(); } assert(type_version != 0); - if (((PyTypeObject *)owner_o)->tp_version_tag != type_version) { + if (FT_ATOMIC_LOAD_UINT_RELAXED(((PyTypeObject *)owner_o)->tp_version_tag) != type_version) { UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); } diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index c99c55dd9dc9fe..680e6441528193 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -5279,7 +5279,7 @@ PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); DEOPT_IF(!PyType_Check(owner_o), LOAD_ATTR); assert(type_version != 0); - DEOPT_IF(((PyTypeObject *)owner_o)->tp_version_tag != type_version, LOAD_ATTR); + DEOPT_IF(FT_ATOMIC_LOAD_UINT_RELAXED(((PyTypeObject *)owner_o)->tp_version_tag) != type_version, LOAD_ATTR); } /* Skip 2 cache entries */ // _LOAD_ATTR_CLASS @@ -5314,7 +5314,7 @@ PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); DEOPT_IF(!PyType_Check(owner_o), LOAD_ATTR); assert(type_version != 0); - DEOPT_IF(((PyTypeObject *)owner_o)->tp_version_tag != type_version, LOAD_ATTR); + DEOPT_IF(FT_ATOMIC_LOAD_UINT_RELAXED(((PyTypeObject *)owner_o)->tp_version_tag) != type_version, LOAD_ATTR); } // _GUARD_TYPE_VERSION { diff --git a/Python/specialize.c b/Python/specialize.c index 93d1b4168f76f9..1e3eef32a1abf2 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -1075,9 +1075,7 @@ specialize_attr_loadclassattr(PyObject *owner, _Py_CODEUNIT *instr, unsigned int tp_version, DescriptorClassification kind, bool is_method, uint32_t shared_keys_version); -#ifndef Py_GIL_DISABLED static int specialize_class_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* name); -#endif /* Returns true if instances of obj's class are * likely to have `name` in their __dict__. @@ -1334,12 +1332,7 @@ _Py_Specialize_LoadAttr(_PyStackRef owner_st, _Py_CODEUNIT *instr, PyObject *nam fail = specialize_module_load_attr(owner, instr, name); } else if (PyType_Check(owner)) { - #ifdef Py_GIL_DISABLED - SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_EXPECTED_ERROR); - fail = true; - #else fail = specialize_class_load_attr(owner, instr, name); - #endif } else { fail = specialize_instance_load_attr(owner, instr, name); @@ -1457,8 +1450,6 @@ _Py_Specialize_StoreAttr(_PyStackRef owner_st, _Py_CODEUNIT *instr, PyObject *na Py_XDECREF(descr); } -#ifndef Py_GIL_DISABLED - #ifdef Py_STATS static int load_attr_fail_kind(DescriptorClassification kind) @@ -1507,8 +1498,10 @@ specialize_class_load_attr(PyObject *owner, _Py_CODEUNIT *instr, SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_ATTR_METACLASS_OVERRIDDEN); return -1; } - PyObject *metadescriptor = _PyType_Lookup(Py_TYPE(cls), name); + unsigned int meta_version; + PyObject *metadescriptor = _PyType_LookupRefAndVersion(Py_TYPE(cls), name, &meta_version); DescriptorClassification metakind = classify_descriptor(metadescriptor, false); + Py_XDECREF(metadescriptor); switch (metakind) { case METHOD: case NON_DESCRIPTOR: @@ -1525,14 +1518,16 @@ specialize_class_load_attr(PyObject *owner, _Py_CODEUNIT *instr, DescriptorClassification kind = 0; unsigned int tp_version; kind = analyze_descriptor(cls, name, &descr, &tp_version, 0); - if (type_get_version(cls, LOAD_ATTR) == 0) { + if (tp_version == 0) { + SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_OUT_OF_VERSIONS); Py_XDECREF(descr); return -1; } bool metaclass_check = false; if ((Py_TYPE(cls)->tp_flags & Py_TPFLAGS_IMMUTABLETYPE) == 0) { metaclass_check = true; - if (type_get_version(Py_TYPE(cls), LOAD_ATTR) == 0) { + if (meta_version == 0) { + SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_OUT_OF_VERSIONS); Py_XDECREF(descr); return -1; } @@ -1544,10 +1539,17 @@ specialize_class_load_attr(PyObject *owner, _Py_CODEUNIT *instr, return -1; case METHOD: case NON_DESCRIPTOR: - write_u32(cache->type_version, cls->tp_version_tag); + #ifdef Py_GIL_DISABLED + if (!_PyObject_HasDeferredRefcount(descr)) { + SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_ATTR_DESCR_NOT_DEFERRED); + Py_XDECREF(descr); + return -1; + } + #endif + write_u32(cache->type_version, tp_version); write_obj(cache->descr, descr); if (metaclass_check) { - write_u32(cache->keys_version, Py_TYPE(cls)->tp_version_tag); + write_u32(cache->keys_version, meta_version); specialize(instr, LOAD_ATTR_CLASS_WITH_METACLASS_CHECK); } else { @@ -1568,8 +1570,6 @@ specialize_class_load_attr(PyObject *owner, _Py_CODEUNIT *instr, } } -#endif // Py_GIL_DISABLED - // Please collect stats carefully before and after modifying. A subtle change // can cause a significant drop in cache hits. A possible test is // python.exe -m test_typing test_re test_dis test_zlib. From 1398699b2ede82473ca04927daebd0b53271fe3d Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 13 Dec 2024 17:46:21 -0800 Subject: [PATCH 32/67] Fix test_type_lookup_mro_reference If specialization fails, `X.mykey` will be performed again using the new mro set by __eq__. Check that the process doesn't crash, since that's what we care about. --- Lib/test/test_descr.py | 32 ++++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/Lib/test/test_descr.py b/Lib/test/test_descr.py index 168b78a477ee9c..67ab225b9fa7cc 100644 --- a/Lib/test/test_descr.py +++ b/Lib/test/test_descr.py @@ -7,6 +7,7 @@ import random import string import sys +import textwrap import types import unittest import warnings @@ -15,6 +16,7 @@ from copy import deepcopy from contextlib import redirect_stdout from test import support +from test.support.script_helper import assert_python_ok try: import _testcapi @@ -5230,6 +5232,7 @@ def test_type_lookup_mro_reference(self): # Issue #14199: _PyType_Lookup() has to keep a strong reference to # the type MRO because it may be modified during the lookup, if # __bases__ is set during the lookup for example. + code = textwrap.dedent(""" class MyKey(object): def __hash__(self): return hash('mykey') @@ -5245,12 +5248,29 @@ class Base2(object): mykey = 'from Base2' mykey2 = 'from Base2' - with self.assertWarnsRegex(RuntimeWarning, 'X'): - X = type('X', (Base,), {MyKey(): 5}) - # mykey is read from Base - self.assertEqual(X.mykey, 'from Base') - # mykey2 is read from Base2 because MyKey.__eq__ has set __bases__ - self.assertEqual(X.mykey2, 'from Base2') + X = type('X', (Base,), {MyKey(): 5}) + + bases_before = ",".join([c.__name__ for c in X.__bases__]) + print(f"before={bases_before}") + + # mykey is initially read from Base, however, the lookup will be perfomed + # again if specialization fails. The second lookup will use the new + # mro set by __eq__. + print(X.mykey) + + bases_after = ",".join([c.__name__ for c in X.__bases__]) + print(f"after={bases_after}") + + # mykey2 is read from Base2 because MyKey.__eq__ has set __bases_ + print(f"mykey2={X.mykey2}") + """) + _, out, err = assert_python_ok("-c", code) + err = err.decode() + self.assertRegex(err, "RuntimeWarning: .*X") + out = out.decode() + self.assertRegex(out, "before=Base") + self.assertRegex(out, "after=Base2") + self.assertRegex(out, "mykey2=from Base2") class PicklingTests(unittest.TestCase): From 3fbe18b67b8d27532e901cbf07cb03d25bb79edd Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 13 Dec 2024 19:06:28 -0800 Subject: [PATCH 33/67] Remove TYPE_CHANGED Always use the type version from the __getattribute__ lookup (even if its zero). The guard will fail if the type changes afterwards. --- Python/specialize.c | 36 +++++++----------------------------- 1 file changed, 7 insertions(+), 29 deletions(-) diff --git a/Python/specialize.c b/Python/specialize.c index 1e3eef32a1abf2..e2224d2f7632a1 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -548,7 +548,6 @@ _PyCode_Quicken(_Py_CODEUNIT *instructions, Py_ssize_t size, PyObject *consts, #define SPEC_FAIL_ATTR_METACLASS_OVERRIDDEN 34 #define SPEC_FAIL_ATTR_SPLIT_DICT 35 #define SPEC_FAIL_ATTR_DESCR_NOT_DEFERRED 36 -#define SPEC_FAIL_ATTR_TYPE_CHANGED 37 /* Binary subscr and store subscr */ @@ -837,7 +836,6 @@ typedef enum { DUNDER_CLASS, /* __class__ attribute */ GETSET_OVERRIDDEN, /* __getattribute__ or __setattr__ has been overridden */ GETATTRIBUTE_IS_PYTHON_FUNCTION, /* Descriptor requires calling a Python __getattribute__ */ - TYPE_CHANGED, /* Error: the type changed during classification */ } DescriptorClassification; @@ -887,8 +885,8 @@ static DescriptorClassification analyze_descriptor(PyTypeObject *type, PyObject *name, PyObject **descr, unsigned int *tp_version, int store) { bool has_getattr = false; - bool have_getattribute_ver = false; - unsigned int getattribute_ver; + bool have_ga_version = false; + unsigned int ga_version; if (store) { if (type->tp_setattro != PyObject_GenericSetAttr) { *descr = NULL; @@ -906,26 +904,20 @@ analyze_descriptor(PyTypeObject *type, PyObject *name, PyObject **descr, unsigne /* One or both of __getattribute__ or __getattr__ may have been overridden See typeobject.c for why these functions are special. */ PyObject *getattribute = _PyType_LookupRefAndVersion(type, - &_Py_ID(__getattribute__), &getattribute_ver); - have_getattribute_ver = true; + &_Py_ID(__getattribute__), &ga_version); + have_ga_version = true; PyInterpreterState *interp = _PyInterpreterState_GET(); bool has_custom_getattribute = getattribute != NULL && getattribute != interp->callable_cache.object__getattribute__; - unsigned int getattr_ver; - PyObject *getattr = _PyType_LookupRefAndVersion(type, &_Py_ID(__getattr__), &getattr_ver); + PyObject *getattr = _PyType_LookupRef(type, &_Py_ID(__getattr__)); has_getattr = getattr != NULL; - if (getattr_ver != getattribute_ver) { - Py_XDECREF(getattribute); - Py_XDECREF(getattr); - return TYPE_CHANGED; - } if (has_custom_getattribute) { if (getattro_slot == _Py_slot_tp_getattro && !has_getattr && Py_IS_TYPE(getattribute, &PyFunction_Type)) { *descr = getattribute; assert(getattr == NULL); - *tp_version = getattribute_ver; + *tp_version = ga_version; return GETATTRIBUTE_IS_PYTHON_FUNCTION; } /* Potentially both __getattr__ and __getattribute__ are set. @@ -952,12 +944,8 @@ analyze_descriptor(PyTypeObject *type, PyObject *name, PyObject **descr, unsigne } unsigned int descr_version; PyObject *descriptor = _PyType_LookupRefAndVersion(type, name, &descr_version); - if (have_getattribute_ver && (descr_version != getattribute_ver)) { - Py_XDECREF(descriptor); - return TYPE_CHANGED; - } *descr = descriptor; - *tp_version = descr_version; + *tp_version = have_ga_version ? ga_version : descr_version; if (PyUnicode_CompareWithASCIIString(name, "__class__") == 0) { if (descriptor == _PyType_Lookup(&PyBaseObject_Type, name)) { return DUNDER_CLASS; @@ -1124,9 +1112,6 @@ do_specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* return -1; } switch(kind) { - case TYPE_CHANGED: - SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_ATTR_TYPE_CHANGED); - return -1; case OVERRIDING: SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_ATTR_OVERRIDING_DESCRIPTOR); return -1; @@ -1370,9 +1355,6 @@ _Py_Specialize_StoreAttr(_PyStackRef owner_st, _Py_CODEUNIT *instr, PyObject *na goto fail; } switch(kind) { - case TYPE_CHANGED: - SPECIALIZATION_FAIL(STORE_ATTR, SPEC_FAIL_ATTR_TYPE_CHANGED); - goto fail; case OVERRIDING: SPECIALIZATION_FAIL(STORE_ATTR, SPEC_FAIL_ATTR_OVERRIDING_DESCRIPTOR); goto fail; @@ -1533,10 +1515,6 @@ specialize_class_load_attr(PyObject *owner, _Py_CODEUNIT *instr, } } switch (kind) { - case TYPE_CHANGED: - SPECIALIZATION_FAIL(STORE_ATTR, SPEC_FAIL_ATTR_TYPE_CHANGED); - Py_XDECREF(descr); - return -1; case METHOD: case NON_DESCRIPTOR: #ifdef Py_GIL_DISABLED From 5aa68ccfc6fd37b8fa1e640ab70f6892f4fa2f0a Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 13 Dec 2024 20:57:52 -0800 Subject: [PATCH 34/67] Misc clean ups --- Python/specialize.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/Python/specialize.c b/Python/specialize.c index e2224d2f7632a1..5de2b6d7ff37ea 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -909,14 +909,13 @@ analyze_descriptor(PyTypeObject *type, PyObject *name, PyObject **descr, unsigne PyInterpreterState *interp = _PyInterpreterState_GET(); bool has_custom_getattribute = getattribute != NULL && getattribute != interp->callable_cache.object__getattribute__; - PyObject *getattr = _PyType_LookupRef(type, &_Py_ID(__getattr__)); + PyObject *getattr = _PyType_Lookup(type, &_Py_ID(__getattr__)); has_getattr = getattr != NULL; if (has_custom_getattribute) { if (getattro_slot == _Py_slot_tp_getattro && !has_getattr && Py_IS_TYPE(getattribute, &PyFunction_Type)) { *descr = getattribute; - assert(getattr == NULL); *tp_version = ga_version; return GETATTRIBUTE_IS_PYTHON_FUNCTION; } @@ -924,7 +923,6 @@ analyze_descriptor(PyTypeObject *type, PyObject *name, PyObject **descr, unsigne Too complicated */ *descr = NULL; Py_XDECREF(getattribute); - Py_XDECREF(getattr); return GETSET_OVERRIDDEN; } /* Potentially has __getattr__ but no custom __getattribute__. @@ -934,7 +932,6 @@ analyze_descriptor(PyTypeObject *type, PyObject *name, PyObject **descr, unsigne raised. This means some specializations, e.g. specializing for property() isn't safe. */ - Py_XDECREF(getattr); Py_XDECREF(getattribute); } else { @@ -1289,7 +1286,7 @@ specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* na uint32_t shared_keys_version = 0; bool shadow = instance_has_key(owner, name, &shared_keys_version); PyObject *descr = NULL; - unsigned int tp_version; + unsigned int tp_version = 0; PyTypeObject *type = Py_TYPE(owner); DescriptorClassification kind = analyze_descriptor(type, name, &descr, &tp_version, 0); int result = do_specialize_instance_load_attr(owner, instr, name, shadow, shared_keys_version, kind, descr, tp_version); @@ -1480,7 +1477,7 @@ specialize_class_load_attr(PyObject *owner, _Py_CODEUNIT *instr, SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_ATTR_METACLASS_OVERRIDDEN); return -1; } - unsigned int meta_version; + unsigned int meta_version = 0; PyObject *metadescriptor = _PyType_LookupRefAndVersion(Py_TYPE(cls), name, &meta_version); DescriptorClassification metakind = classify_descriptor(metadescriptor, false); Py_XDECREF(metadescriptor); @@ -1498,7 +1495,7 @@ specialize_class_load_attr(PyObject *owner, _Py_CODEUNIT *instr, } PyObject *descr = NULL; DescriptorClassification kind = 0; - unsigned int tp_version; + unsigned int tp_version = 0; kind = analyze_descriptor(cls, name, &descr, &tp_version, 0); if (tp_version == 0) { SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_OUT_OF_VERSIONS); From e1bf1f4ddc036f475df3edf2d8b67c9f7f613a59 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 13 Dec 2024 21:07:57 -0800 Subject: [PATCH 35/67] Check that inline values still valid after taking CS --- Python/specialize.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/Python/specialize.c b/Python/specialize.c index 5de2b6d7ff37ea..2aab719f229951 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -974,6 +974,7 @@ specialize_inline_values_access_lock_held( return 0; } assert(index >= 0); + assert(_PyObject_InlineValues(owner)->valid); char *value_addr = (char *)&_PyObject_InlineValues(owner)->values[index]; Py_ssize_t offset = value_addr - (char *)owner; if (offset != (uint16_t)offset) { @@ -1036,8 +1037,15 @@ specialize_dict_access( !(base_op == STORE_ATTR && _PyObject_GetManagedDict(owner) != NULL)) { Py_BEGIN_CRITICAL_SECTION(owner); - result = specialize_inline_values_access_lock_held( - owner, instr, type, tp_version, name, base_op, values_op); + if (_PyObject_GetManagedDict(owner)) { + // Somebody materialized the dict + SPECIALIZATION_FAIL(base_op, SPEC_FAIL_OTHER); + result = 0; + } + else { + result = specialize_inline_values_access_lock_held( + owner, instr, type, tp_version, name, base_op, values_op); + } Py_END_CRITICAL_SECTION(); } else { From 396edb1a0525f1654173a226374bb458bf9c4a3b Mon Sep 17 00:00:00 2001 From: Matt Page Date: Wed, 18 Dec 2024 15:29:11 -0800 Subject: [PATCH 36/67] Enable test_opcache tests --- Lib/test/test_opcache.py | 11 ++++++----- Python/specialize.c | 5 +++++ 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/Lib/test/test_opcache.py b/Lib/test/test_opcache.py index 38b8f08ffb32e1..6b16989ba644e4 100644 --- a/Lib/test/test_opcache.py +++ b/Lib/test/test_opcache.py @@ -752,7 +752,7 @@ def write(items): opname = "LOAD_ATTR_CLASS" self.assert_races_do_not_crash(opname, get_items, read, write) - @requires_specialization + @requires_specialization_ft def test_load_attr_getattribute_overridden(self): def get_items(): class C: @@ -779,6 +779,7 @@ def write(items): pass type(item).__getattribute__ = lambda self, name: None + opname = "LOAD_ATTR_GETATTRIBUTE_OVERRIDDEN" self.assert_races_do_not_crash(opname, get_items, read, write) @@ -806,7 +807,7 @@ def write(items): opname = "LOAD_ATTR_INSTANCE_VALUE" self.assert_races_do_not_crash(opname, get_items, read, write) - @requires_specialization + @requires_specialization_ft def test_load_attr_method_lazy_dict(self): def get_items(): class C(Exception): @@ -836,7 +837,7 @@ def write(items): opname = "LOAD_ATTR_METHOD_LAZY_DICT" self.assert_races_do_not_crash(opname, get_items, read, write) - @requires_specialization + @requires_specialization_ft def test_load_attr_method_no_dict(self): def get_items(): class C: @@ -867,7 +868,7 @@ def write(items): opname = "LOAD_ATTR_METHOD_NO_DICT" self.assert_races_do_not_crash(opname, get_items, read, write) - @requires_specialization + @requires_specialization_ft def test_load_attr_method_with_values(self): def get_items(): class C: @@ -922,7 +923,7 @@ def write(items): opname = "LOAD_ATTR_MODULE" self.assert_races_do_not_crash(opname, get_items, read, write) - @requires_specialization + @requires_specialization_ft def test_load_attr_property(self): def get_items(): class C: diff --git a/Python/specialize.c b/Python/specialize.c index 2aab719f229951..0bfc50f71f1608 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -1220,7 +1220,12 @@ do_specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* return -1; case GETATTRIBUTE_IS_PYTHON_FUNCTION: { + #ifndef Py_GIL_DISABLED + // In free-threaded builds it's possible for tp_getattro to change + // after the call to analyze_descriptor. That is fine: the version + // guard will fail. assert(type->tp_getattro == _Py_slot_tp_getattro); + #endif assert(Py_IS_TYPE(descr, &PyFunction_Type)); _PyLoadMethodCache *lm_cache = (_PyLoadMethodCache *)(instr + 1); if (!function_check_args(descr, 2, LOAD_ATTR)) { From e5e2508624f103f8995d2f057f8afe4fcec699d7 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 19 Dec 2024 17:00:56 -0800 Subject: [PATCH 37/67] Skip test that triggers gh-127773 in refleak tests --- Lib/test/test_capi/test_type.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_capi/test_type.py b/Lib/test/test_capi/test_type.py index 54c83e09f892a0..92d056e802eeed 100644 --- a/Lib/test/test_capi/test_type.py +++ b/Lib/test/test_capi/test_type.py @@ -1,4 +1,4 @@ -from test.support import import_helper +from test.support import import_helper, Py_GIL_DISABLED, refleak_helper import unittest _testcapi = import_helper.import_module('_testcapi') @@ -37,6 +37,9 @@ class D(A, C): pass # as well type_freeze(D) + @unittest.skipIf( + Py_GIL_DISABLED and refleak_helper.hunting_for_refleaks(), + "Specialization failure triggers gh-127773") def test_freeze_meta(self): """test PyType_Freeze() with overridden MRO""" type_freeze = _testcapi.type_freeze From 47c794f7589bf9c5f2a0906400cbe3f8fa2a0fe3 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 19 Dec 2024 22:59:49 -0800 Subject: [PATCH 38/67] Add test for LOAD_ATTR_CLASS_WITH_METACLASS_CHECK --- Lib/test/test_opcache.py | 44 ++++++++++++++++++++++++++++++++++------ 1 file changed, 38 insertions(+), 6 deletions(-) diff --git a/Lib/test/test_opcache.py b/Lib/test/test_opcache.py index 5b42995e6a0bb8..d7c034365357b7 100644 --- a/Lib/test/test_opcache.py +++ b/Lib/test/test_opcache.py @@ -717,13 +717,10 @@ def write(items): @requires_specialization_ft def test_load_attr_class(self): def get_items(): - class B: - pass - class C: # a must be set to an instance that uses deferred reference - # counting - a = B + # counting in free-threaded builds + a = type("Foo", (object,), {}) items = [] for _ in range(self.ITEMS): @@ -744,11 +741,46 @@ def write(items): del item.a except AttributeError: pass - item.a = object() + item.a = type("Foo", (object,), {}) opname = "LOAD_ATTR_CLASS" self.assert_races_do_not_crash(opname, get_items, read, write) + @requires_specialization_ft + def test_load_attr_class_with_metaclass_check(self): + def get_items(): + class Meta(type): + pass + + class C(metaclass=Meta): + # a must be set to an instance that uses deferred reference + # counting in free-threaded builds + a = type("Foo", (object,), {}) + + items = [] + for _ in range(self.ITEMS): + item = C + items.append(item) + return items + + def read(items): + for item in items: + try: + item.a + except AttributeError: + pass + + def write(items): + for item in items: + try: + del item.a + except AttributeError: + pass + item.a = type("Foo", (object,), {}) + + opname = "LOAD_ATTR_CLASS_WITH_METACLASS_CHECK" + self.assert_races_do_not_crash(opname, get_items, read, write) + @requires_specialization_ft def test_load_attr_getattribute_overridden(self): def get_items(): From 3876bc7e862be7694f4d7f95e8681755192adbbc Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 19 Dec 2024 23:18:42 -0800 Subject: [PATCH 39/67] Undo workaround for cases generator bug --- Python/bytecodes.c | 4 +--- Python/executor_cases.c.h | 8 ++------ Python/generated_cases.c.h | 4 +--- 3 files changed, 4 insertions(+), 12 deletions(-) diff --git a/Python/bytecodes.c b/Python/bytecodes.c index bf88e4d01a4f0b..883ab8d65bbf6a 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2272,9 +2272,7 @@ dummy_func( int increfed = _Py_TryIncrefCompareStackRef(addr, attr_o, &attr); DEOPT_IF(!increfed); #else - // XXX - Bug in cases generator - Py_INCREF(attr_o); - attr = PyStackRef_FromPyObjectSteal(attr_o); + attr = PyStackRef_FromPyObjectNew(attr_o); #endif STAT_INC(LOAD_ATTR, hit); diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index 40e6bd7ec6d742..41c4443a448724 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -2841,9 +2841,7 @@ JUMP_TO_JUMP_TARGET(); } #else - // XXX - Bug in cases generator - Py_INCREF(attr_o); - attr = PyStackRef_FromPyObjectSteal(attr_o); + attr = PyStackRef_FromPyObjectNew(attr_o); #endif STAT_INC(LOAD_ATTR, hit); null = PyStackRef_NULL; @@ -2873,9 +2871,7 @@ JUMP_TO_JUMP_TARGET(); } #else - // XXX - Bug in cases generator - Py_INCREF(attr_o); - attr = PyStackRef_FromPyObjectSteal(attr_o); + attr = PyStackRef_FromPyObjectNew(attr_o); #endif STAT_INC(LOAD_ATTR, hit); null = PyStackRef_NULL; diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 9d061cbb4ba522..be250cd939f356 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -5772,9 +5772,7 @@ int increfed = _Py_TryIncrefCompareStackRef(addr, attr_o, &attr); DEOPT_IF(!increfed, LOAD_ATTR); #else - // XXX - Bug in cases generator - Py_INCREF(attr_o); - attr = PyStackRef_FromPyObjectSteal(attr_o); + attr = PyStackRef_FromPyObjectNew(attr_o); #endif STAT_INC(LOAD_ATTR, hit); null = PyStackRef_NULL; From 08d14d02580c7acae7e1a612c06f01fc08545ed8 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 20 Dec 2024 08:47:26 -0800 Subject: [PATCH 40/67] Fix whitespace after merge --- Include/internal/pycore_dict.h | 1 - Lib/test/test_opcache.py | 1 - Python/bytecodes.c | 1 - Python/specialize.c | 1 + 4 files changed, 1 insertion(+), 3 deletions(-) diff --git a/Include/internal/pycore_dict.h b/Include/internal/pycore_dict.h index e3981503e9a417..74ac8f2148174c 100644 --- a/Include/internal/pycore_dict.h +++ b/Include/internal/pycore_dict.h @@ -113,7 +113,6 @@ extern Py_ssize_t _Py_dict_lookup_threadsafe(PyDictObject *mp, PyObject *key, Py extern Py_ssize_t _Py_dict_lookup_threadsafe_stackref(PyDictObject *mp, PyObject *key, Py_hash_t hash, _PyStackRef *value_addr); extern Py_ssize_t _PyDict_LookupIndex(PyDictObject *, PyObject *); - extern Py_ssize_t _PyDictKeys_StringLookup(PyDictKeysObject* dictkeys, PyObject *key); /* Look up a string key in an all unicode dict keys, assign the keys object a version, and diff --git a/Lib/test/test_opcache.py b/Lib/test/test_opcache.py index d7c034365357b7..d0afd6528b0473 100644 --- a/Lib/test/test_opcache.py +++ b/Lib/test/test_opcache.py @@ -808,7 +808,6 @@ def write(items): pass type(item).__getattribute__ = lambda self, name: None - opname = "LOAD_ATTR_GETATTRIBUTE_OVERRIDDEN" self.assert_races_do_not_crash(opname, get_items, read, write) diff --git a/Python/bytecodes.c b/Python/bytecodes.c index 883ab8d65bbf6a..7218a15d1543ba 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2275,7 +2275,6 @@ dummy_func( attr = PyStackRef_FromPyObjectNew(attr_o); #endif STAT_INC(LOAD_ATTR, hit); - null = PyStackRef_NULL; DECREF_INPUTS(); } diff --git a/Python/specialize.c b/Python/specialize.c index cc4208a7e96ca3..8f71be6b494185 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -1022,6 +1022,7 @@ specialize_dict_access_hint( return 1; } + static int specialize_dict_access( PyObject *owner, _Py_CODEUNIT *instr, PyTypeObject *type, From 56e9a8a049319c064b618ca5aae8a3267a202f0c Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 20 Dec 2024 09:06:03 -0800 Subject: [PATCH 41/67] Double check that dict hasn't changed after locking it --- Python/bytecodes.c | 6 ++++++ Python/executor_cases.c.h | 9 +++++++++ Python/generated_cases.c.h | 6 ++++++ 3 files changed, 21 insertions(+) diff --git a/Python/bytecodes.c b/Python/bytecodes.c index 7218a15d1543ba..2a38baf6e9db41 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2228,6 +2228,12 @@ dummy_func( PyDictObject *dict = _PyObject_GetManagedDict(owner_o); DEOPT_IF(!LOCK_OBJECT(dict)); + #ifdef Py_GIL_DISABLED + if (dict != _PyObject_GetManagedDict(owner_o)) { + UNLOCK_OBJECT(dict); + DEOPT_IF(true); + } + #endif if (hint >= (size_t)dict->ma_keys->dk_nentries) { UNLOCK_OBJECT(dict); DEOPT_IF(true); diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index 41c4443a448724..fa628767d5f828 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -2776,6 +2776,15 @@ UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); } + #ifdef Py_GIL_DISABLED + if (dict != _PyObject_GetManagedDict(owner_o)) { + UNLOCK_OBJECT(dict); + if (true) { + UOP_STAT_INC(uopcode, miss); + JUMP_TO_JUMP_TARGET(); + } + } + #endif if (hint >= (size_t)dict->ma_keys->dk_nentries) { UNLOCK_OBJECT(dict); if (true) { diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index be250cd939f356..f90967e12b2874 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -5818,6 +5818,12 @@ PyObject *attr_o; PyDictObject *dict = _PyObject_GetManagedDict(owner_o); DEOPT_IF(!LOCK_OBJECT(dict), LOAD_ATTR); + #ifdef Py_GIL_DISABLED + if (dict != _PyObject_GetManagedDict(owner_o)) { + UNLOCK_OBJECT(dict); + DEOPT_IF(true, LOAD_ATTR); + } + #endif if (hint >= (size_t)dict->ma_keys->dk_nentries) { UNLOCK_OBJECT(dict); DEOPT_IF(true, LOAD_ATTR); From 8ca405ba22831a1dde29df4761f173b45258e996 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 20 Dec 2024 09:09:29 -0800 Subject: [PATCH 42/67] Use correct type for loading type version --- Python/bytecodes.c | 2 +- Python/generated_cases.c.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Python/bytecodes.c b/Python/bytecodes.c index 2a38baf6e9db41..92b633b51fc76e 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2350,7 +2350,7 @@ dummy_func( DEOPT_IF(tstate->interp->eval_frame); PyTypeObject *cls = Py_TYPE(owner_o); assert(type_version != 0); - DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(cls->tp_version_tag) != type_version); + DEOPT_IF(FT_ATOMIC_LOAD_UINT_RELAXED(cls->tp_version_tag) != type_version); assert(Py_IS_TYPE(getattribute, &PyFunction_Type)); PyFunctionObject *f = (PyFunctionObject *)getattribute; assert(func_version != 0); diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index f90967e12b2874..b7042e0523ac80 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -5353,7 +5353,7 @@ DEOPT_IF(tstate->interp->eval_frame, LOAD_ATTR); PyTypeObject *cls = Py_TYPE(owner_o); assert(type_version != 0); - DEOPT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(cls->tp_version_tag) != type_version, LOAD_ATTR); + DEOPT_IF(FT_ATOMIC_LOAD_UINT_RELAXED(cls->tp_version_tag) != type_version, LOAD_ATTR); assert(Py_IS_TYPE(getattribute, &PyFunction_Type)); PyFunctionObject *f = (PyFunctionObject *)getattribute; assert(func_version != 0); From 4c7a4b98299b0fe42a08b2367785383e993df4a2 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 20 Dec 2024 09:16:48 -0800 Subject: [PATCH 43/67] Remove unnecessary comma --- Python/specialize.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Python/specialize.c b/Python/specialize.c index 8f71be6b494185..34b6fd9a954b2a 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -834,7 +834,7 @@ typedef enum { ABSENT, /* Attribute is not present on the class */ DUNDER_CLASS, /* __class__ attribute */ GETSET_OVERRIDDEN, /* __getattribute__ or __setattr__ has been overridden */ - GETATTRIBUTE_IS_PYTHON_FUNCTION, /* Descriptor requires calling a Python __getattribute__ */ + GETATTRIBUTE_IS_PYTHON_FUNCTION /* Descriptor requires calling a Python __getattribute__ */ } DescriptorClassification; From 1b787b3f733938de76aafb93de67dc2fceca3bca Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 20 Dec 2024 09:25:59 -0800 Subject: [PATCH 44/67] Use atomics when loading oparg --- Python/specialize.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Python/specialize.c b/Python/specialize.c index 34b6fd9a954b2a..6d5b7d34dc2cc9 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -1127,6 +1127,7 @@ do_specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_OUT_OF_VERSIONS); return -1; } + uint8_t oparg = FT_ATOMIC_LOAD_UINT8_RELAXED(instr->op.arg); switch(kind) { case OVERRIDING: SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_ATTR_OVERRIDING_DESCRIPTOR); @@ -1136,7 +1137,6 @@ do_specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* if (shadow) { goto try_instance; } - int oparg = instr->op.arg; if (oparg & 1) { if (specialize_attr_loadclassattr(owner, instr, name, descr, tp_version, kind, true, @@ -1166,7 +1166,7 @@ do_specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* if (!function_check_args(fget, 1, LOAD_ATTR)) { return -1; } - if (instr->op.arg & 1) { + if (oparg & 1) { SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_ATTR_METHOD); return -1; } @@ -1243,7 +1243,7 @@ do_specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* if (!function_check_args(descr, 2, LOAD_ATTR)) { return -1; } - if (instr->op.arg & 1) { + if (oparg & 1) { SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_ATTR_METHOD); return -1; } @@ -1280,7 +1280,7 @@ do_specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* if (shadow) { goto try_instance; } - if ((instr->op.arg & 1) == 0) { + if ((oparg & 1) == 0) { if (specialize_attr_loadclassattr(owner, instr, name, descr, tp_version, kind, false, shared_keys_version)) { From b86836307e9918a975af706e4ddaf377a4bcefc7 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 20 Dec 2024 12:38:37 -0800 Subject: [PATCH 45/67] Fix formatting --- Python/specialize.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Python/specialize.c b/Python/specialize.c index 6d5b7d34dc2cc9..31268604c13305 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -1583,19 +1583,19 @@ specialize_attr_loadclassattr(PyObject *owner, _Py_CODEUNIT *instr, assert(descr != NULL); assert((is_method && kind == METHOD) || (!is_method && kind == NON_DESCRIPTOR)); -#ifdef Py_GIL_DISABLED + #ifdef Py_GIL_DISABLED if (!_PyObject_HasDeferredRefcount(descr)) { SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_ATTR_DESCR_NOT_DEFERRED); return 0; } -#endif + #endif unsigned long tp_flags = PyType_GetFlags(owner_cls); if (tp_flags & Py_TPFLAGS_INLINE_VALUES) { -#ifndef Py_GIL_DISABLED + #ifndef Py_GIL_DISABLED PyDictKeysObject *keys = ((PyHeapTypeObject *)owner_cls)->ht_cached_keys; assert(_PyDictKeys_StringLookup(keys, name) < 0); -#endif + #endif if (shared_keys_version == 0) { SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_OUT_OF_VERSIONS); return 0; From d6d4c73065fc3e1fe5746b5c10309fad14a89a7f Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 20 Dec 2024 15:05:47 -0800 Subject: [PATCH 46/67] Always return type version from analyze_descriptor_load This matches the previous implementation and causes failures to specialize due to the presence of both __getattr__ and __getattribute__ to be classified correctly (rather than being classified as being out of type versions). --- Python/specialize.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Python/specialize.c b/Python/specialize.c index 31268604c13305..d18c8887c06f41 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -921,6 +921,7 @@ analyze_descriptor_load(PyTypeObject *type, PyObject *name, PyObject **descr, un Too complicated */ Py_DECREF(getattribute); *descr = NULL; + *tp_version = ga_version; return GETSET_OVERRIDDEN; } /* Potentially has __getattr__ but no custom __getattribute__. @@ -934,6 +935,7 @@ analyze_descriptor_load(PyTypeObject *type, PyObject *name, PyObject **descr, un } else { *descr = NULL; + *tp_version = FT_ATOMIC_LOAD_UINT_RELAXED(type->tp_version_tag); return GETSET_OVERRIDDEN; } unsigned int descr_version; From 9673f78320fca55654f8fb0a2b40d5aee744880f Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 23 Dec 2024 10:33:27 -0800 Subject: [PATCH 47/67] Combine incref/steal into new stackref --- Python/bytecodes.c | 3 +-- Python/executor_cases.c.h | 3 +-- Python/generated_cases.c.h | 3 +-- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/Python/bytecodes.c b/Python/bytecodes.c index fa00614424415f..3fdc4717390ff1 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2254,8 +2254,7 @@ dummy_func( DEOPT_IF(true); } STAT_INC(LOAD_ATTR, hit); - Py_INCREF(attr_o); - attr = PyStackRef_FromPyObjectSteal(attr_o); + attr = PyStackRef_FromPyObjectNew(attr_o); UNLOCK_OBJECT(dict); null = PyStackRef_NULL; DECREF_INPUTS(); diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index 61be4cb36bbb2b..57fe7cd1c671cc 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -2817,8 +2817,7 @@ } } STAT_INC(LOAD_ATTR, hit); - Py_INCREF(attr_o); - attr = PyStackRef_FromPyObjectSteal(attr_o); + attr = PyStackRef_FromPyObjectNew(attr_o); UNLOCK_OBJECT(dict); null = PyStackRef_NULL; PyStackRef_CLOSE(owner); diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 7eb39d66bb6640..251e51242f9bd9 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -5851,8 +5851,7 @@ DEOPT_IF(true, LOAD_ATTR); } STAT_INC(LOAD_ATTR, hit); - Py_INCREF(attr_o); - attr = PyStackRef_FromPyObjectSteal(attr_o); + attr = PyStackRef_FromPyObjectNew(attr_o); UNLOCK_OBJECT(dict); null = PyStackRef_NULL; PyStackRef_CLOSE(owner); From 6c3041f55d2210c7b40986c6731ac5f58caa3ff9 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 23 Dec 2024 10:34:53 -0800 Subject: [PATCH 48/67] Fix formatting --- Objects/dictobject.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Objects/dictobject.c b/Objects/dictobject.c index cc8896332b2d16..42c2ebfc652fb9 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -1130,7 +1130,8 @@ dictkeys_generic_lookup(PyDictObject *mp, PyDictKeysObject* dk, PyObject *key, P } static Py_hash_t -check_keys_and_hash(PyDictKeysObject *dk, PyObject *key) { +check_keys_and_hash(PyDictKeysObject *dk, PyObject *key) +{ DictKeysKind kind = dk->dk_kind; if (!PyUnicode_CheckExact(key) || kind == DICT_KEYS_GENERAL) { return -1; @@ -1192,7 +1193,7 @@ _PyDictKeys_StringLookup(PyDictKeysObject* dk, PyObject *key) } Py_ssize_t -_PyDictKeys_StringLookupAndVersion(PyDictKeysObject* dk, PyObject *key, uint32_t *version) +_PyDictKeys_StringLookupAndVersion(PyDictKeysObject *dk, PyObject *key, uint32_t *version) { Py_hash_t hash = check_keys_and_hash(dk, key); if (hash == -1) { From a20a4a4f82e54f693b363e4764971fe28438c96b Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 23 Dec 2024 10:41:40 -0800 Subject: [PATCH 49/67] Remove unnecessary error check for PyUnicode_Type.tp_hash --- Objects/dictobject.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 42c2ebfc652fb9..14f22dc8ed2af9 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -1139,10 +1139,7 @@ check_keys_and_hash(PyDictKeysObject *dk, PyObject *key) Py_hash_t hash = unicode_get_hash(key); if (hash == -1) { hash = PyUnicode_Type.tp_hash(key); - if (hash == -1) { - PyErr_Clear(); - return -1; - } + assert(hash != -1); } return hash; } From 35b31c69399a3a741eee6a41c1d0719f28fd0add Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 23 Dec 2024 10:48:50 -0800 Subject: [PATCH 50/67] Check keys kind explicitly --- Python/bytecodes.c | 2 +- Python/executor_cases.c.h | 2 +- Python/generated_cases.c.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Python/bytecodes.c b/Python/bytecodes.c index 3fdc4717390ff1..1a67171aa24cfb 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2239,7 +2239,7 @@ dummy_func( DEOPT_IF(true); } PyObject *name = GETITEM(FRAME_CO_NAMES, oparg>>1); - if (!DK_IS_UNICODE(dict->ma_keys)) { + if (dict->ma_keys->dk_kind != DICT_KEYS_UNICODE) { UNLOCK_OBJECT(dict); DEOPT_IF(true); } diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index 57fe7cd1c671cc..75c5790af08a76 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -2793,7 +2793,7 @@ } } PyObject *name = GETITEM(FRAME_CO_NAMES, oparg>>1); - if (!DK_IS_UNICODE(dict->ma_keys)) { + if (dict->ma_keys->dk_kind != DICT_KEYS_UNICODE) { UNLOCK_OBJECT(dict); if (true) { UOP_STAT_INC(uopcode, miss); diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 251e51242f9bd9..0b0640c664f2e5 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -5836,7 +5836,7 @@ DEOPT_IF(true, LOAD_ATTR); } PyObject *name = GETITEM(FRAME_CO_NAMES, oparg>>1); - if (!DK_IS_UNICODE(dict->ma_keys)) { + if (dict->ma_keys->dk_kind != DICT_KEYS_UNICODE) { UNLOCK_OBJECT(dict); DEOPT_IF(true, LOAD_ATTR); } From e5a7ae93256e9412ac1fdbcc2ee32f7cb73417bb Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 23 Dec 2024 11:29:39 -0800 Subject: [PATCH 51/67] Pass dict from `_CHECK_ATTR_WITH_HINT` to `_LOAD_ATTR_WITH_HINT` --- Include/internal/pycore_opcode_metadata.h | 2 +- Include/internal/pycore_uop_metadata.h | 2 +- Python/bytecodes.c | 33 ++++++++++++------- Python/executor_cases.c.h | 39 ++++++++++++++--------- Python/generated_cases.c.h | 15 ++++----- Python/optimizer_bytecodes.c | 8 ++++- Python/optimizer_cases.c.h | 19 ++++++++--- 7 files changed, 75 insertions(+), 43 deletions(-) diff --git a/Include/internal/pycore_opcode_metadata.h b/Include/internal/pycore_opcode_metadata.h index 5fb236836dccd9..ade79e735e3829 100644 --- a/Include/internal/pycore_opcode_metadata.h +++ b/Include/internal/pycore_opcode_metadata.h @@ -1524,7 +1524,7 @@ int _PyOpcode_max_stack_effect(int opcode, int oparg, int *effect) { return 0; } case LOAD_ATTR_WITH_HINT: { - *effect = Py_MAX(0, (oparg & 1)); + *effect = Py_MAX(1, (oparg & 1)); return 0; } case LOAD_BUILD_CLASS: { diff --git a/Include/internal/pycore_uop_metadata.h b/Include/internal/pycore_uop_metadata.h index e71194b116e020..bc4cdbdfb20b4f 100644 --- a/Include/internal/pycore_uop_metadata.h +++ b/Include/internal/pycore_uop_metadata.h @@ -858,7 +858,7 @@ int _PyUop_num_popped(int opcode, int oparg) case _CHECK_ATTR_WITH_HINT: return 0; case _LOAD_ATTR_WITH_HINT: - return 1; + return 2; case _LOAD_ATTR_SLOT_0: return 1; case _LOAD_ATTR_SLOT_1: diff --git a/Python/bytecodes.c b/Python/bytecodes.c index 1a67171aa24cfb..3dbed066940ebd 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2213,51 +2213,60 @@ dummy_func( _LOAD_ATTR_MODULE_FROM_KEYS + unused/5; - op(_CHECK_ATTR_WITH_HINT, (owner -- owner)) { + op(_CHECK_ATTR_WITH_HINT, (owner -- owner, dict: PyDictObject *)) { PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); assert(Py_TYPE(owner_o)->tp_flags & Py_TPFLAGS_MANAGED_DICT); - PyDictObject *dict = _PyObject_GetManagedDict(owner_o); - EXIT_IF(dict == NULL); - assert(PyDict_CheckExact((PyObject *)dict)); + PyDictObject *dict_o = _PyObject_GetManagedDict(owner_o); + EXIT_IF(dict_o == NULL); + assert(PyDict_CheckExact((PyObject *)dict_o)); + dict = dict_o; } - op(_LOAD_ATTR_WITH_HINT, (hint/1, owner -- attr, null if (oparg & 1))) { + op(_LOAD_ATTR_WITH_HINT, (hint/1, owner, dict: PyDictObject * -- attr, null if (oparg & 1))) { PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); PyObject *attr_o; - PyDictObject *dict = _PyObject_GetManagedDict(owner_o); - DEOPT_IF(!LOCK_OBJECT(dict)); - #ifdef Py_GIL_DISABLED - if (dict != _PyObject_GetManagedDict(owner_o)) { - UNLOCK_OBJECT(dict); + if (!LOCK_OBJECT(dict)) { + DEAD(dict); + POP_DEAD_INPUTS(); DEOPT_IF(true); } - #endif + if (hint >= (size_t)dict->ma_keys->dk_nentries) { UNLOCK_OBJECT(dict); + DEAD(dict); + POP_DEAD_INPUTS(); DEOPT_IF(true); } PyObject *name = GETITEM(FRAME_CO_NAMES, oparg>>1); if (dict->ma_keys->dk_kind != DICT_KEYS_UNICODE) { UNLOCK_OBJECT(dict); + DEAD(dict); + POP_DEAD_INPUTS(); DEOPT_IF(true); } PyDictUnicodeEntry *ep = DK_UNICODE_ENTRIES(dict->ma_keys) + hint; if (ep->me_key != name) { UNLOCK_OBJECT(dict); + DEAD(dict); + POP_DEAD_INPUTS(); DEOPT_IF(true); } attr_o = ep->me_value; if (attr_o == NULL) { UNLOCK_OBJECT(dict); + DEAD(dict); + POP_DEAD_INPUTS(); DEOPT_IF(true); } STAT_INC(LOAD_ATTR, hit); attr = PyStackRef_FromPyObjectNew(attr_o); UNLOCK_OBJECT(dict); + DEAD(dict); null = PyStackRef_NULL; - DECREF_INPUTS(); + PyStackRef_CLOSE(owner); + INPUTS_DEAD(); } macro(LOAD_ATTR_WITH_HINT) = diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index 75c5790af08a76..6ac05d7d546182 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -2750,43 +2750,46 @@ case _CHECK_ATTR_WITH_HINT: { _PyStackRef owner; + PyDictObject *dict; owner = stack_pointer[-1]; PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); assert(Py_TYPE(owner_o)->tp_flags & Py_TPFLAGS_MANAGED_DICT); - PyDictObject *dict = _PyObject_GetManagedDict(owner_o); - if (dict == NULL) { + PyDictObject *dict_o = _PyObject_GetManagedDict(owner_o); + if (dict_o == NULL) { UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); } - assert(PyDict_CheckExact((PyObject *)dict)); + assert(PyDict_CheckExact((PyObject *)dict_o)); + dict = dict_o; + stack_pointer[0].bits = (uintptr_t)dict; + stack_pointer += 1; + assert(WITHIN_STACK_BOUNDS()); break; } case _LOAD_ATTR_WITH_HINT: { + PyDictObject *dict; _PyStackRef owner; _PyStackRef attr; _PyStackRef null = PyStackRef_NULL; oparg = CURRENT_OPARG(); - owner = stack_pointer[-1]; + dict = (PyDictObject *)stack_pointer[-1].bits; + owner = stack_pointer[-2]; uint16_t hint = (uint16_t)CURRENT_OPERAND0(); PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); PyObject *attr_o; - PyDictObject *dict = _PyObject_GetManagedDict(owner_o); if (!LOCK_OBJECT(dict)) { - UOP_STAT_INC(uopcode, miss); - JUMP_TO_JUMP_TARGET(); - } - #ifdef Py_GIL_DISABLED - if (dict != _PyObject_GetManagedDict(owner_o)) { - UNLOCK_OBJECT(dict); + stack_pointer += -1; + assert(WITHIN_STACK_BOUNDS()); if (true) { UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); } } - #endif if (hint >= (size_t)dict->ma_keys->dk_nentries) { UNLOCK_OBJECT(dict); + stack_pointer += -1; + assert(WITHIN_STACK_BOUNDS()); if (true) { UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); @@ -2795,6 +2798,8 @@ PyObject *name = GETITEM(FRAME_CO_NAMES, oparg>>1); if (dict->ma_keys->dk_kind != DICT_KEYS_UNICODE) { UNLOCK_OBJECT(dict); + stack_pointer += -1; + assert(WITHIN_STACK_BOUNDS()); if (true) { UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); @@ -2803,6 +2808,8 @@ PyDictUnicodeEntry *ep = DK_UNICODE_ENTRIES(dict->ma_keys) + hint; if (ep->me_key != name) { UNLOCK_OBJECT(dict); + stack_pointer += -1; + assert(WITHIN_STACK_BOUNDS()); if (true) { UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); @@ -2811,6 +2818,8 @@ attr_o = ep->me_value; if (attr_o == NULL) { UNLOCK_OBJECT(dict); + stack_pointer += -1; + assert(WITHIN_STACK_BOUNDS()); if (true) { UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); @@ -2821,9 +2830,9 @@ UNLOCK_OBJECT(dict); null = PyStackRef_NULL; PyStackRef_CLOSE(owner); - stack_pointer[-1] = attr; - if (oparg & 1) stack_pointer[0] = null; - stack_pointer += (oparg & 1); + stack_pointer[-2] = attr; + if (oparg & 1) stack_pointer[-1] = null; + stack_pointer += -1 + (oparg & 1); assert(WITHIN_STACK_BOUNDS()); break; } diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 0b0640c664f2e5..3a380d0d808e5e 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -5799,6 +5799,7 @@ INSTRUCTION_STATS(LOAD_ATTR_WITH_HINT); static_assert(INLINE_CACHE_ENTRIES_LOAD_ATTR == 9, "incorrect cache size"); _PyStackRef owner; + PyDictObject *dict; _PyStackRef attr; _PyStackRef null = PyStackRef_NULL; /* Skip 1 cache entry */ @@ -5814,23 +5815,19 @@ { PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); assert(Py_TYPE(owner_o)->tp_flags & Py_TPFLAGS_MANAGED_DICT); - PyDictObject *dict = _PyObject_GetManagedDict(owner_o); - DEOPT_IF(dict == NULL, LOAD_ATTR); - assert(PyDict_CheckExact((PyObject *)dict)); + PyDictObject *dict_o = _PyObject_GetManagedDict(owner_o); + DEOPT_IF(dict_o == NULL, LOAD_ATTR); + assert(PyDict_CheckExact((PyObject *)dict_o)); + dict = dict_o; } // _LOAD_ATTR_WITH_HINT { uint16_t hint = read_u16(&this_instr[4].cache); PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); PyObject *attr_o; - PyDictObject *dict = _PyObject_GetManagedDict(owner_o); - DEOPT_IF(!LOCK_OBJECT(dict), LOAD_ATTR); - #ifdef Py_GIL_DISABLED - if (dict != _PyObject_GetManagedDict(owner_o)) { - UNLOCK_OBJECT(dict); + if (!LOCK_OBJECT(dict)) { DEOPT_IF(true, LOAD_ATTR); } - #endif if (hint >= (size_t)dict->ma_keys->dk_nentries) { UNLOCK_OBJECT(dict); DEOPT_IF(true, LOAD_ATTR); diff --git a/Python/optimizer_bytecodes.c b/Python/optimizer_bytecodes.c index e60c0d38425bfe..c04016ca35cd03 100644 --- a/Python/optimizer_bytecodes.c +++ b/Python/optimizer_bytecodes.c @@ -542,11 +542,17 @@ dummy_func(void) { } } - op(_LOAD_ATTR_WITH_HINT, (hint/1, owner -- attr, null if (oparg & 1))) { + op(_CHECK_ATTR_WITH_HINT, (owner -- owner, dict)) { + dict = sym_new_not_null(ctx); + (void)owner; + } + + op(_LOAD_ATTR_WITH_HINT, (hint/1, owner, dict -- attr, null if (oparg & 1))) { attr = sym_new_not_null(ctx); null = sym_new_null(ctx); (void)hint; (void)owner; + (void)dict; } op(_LOAD_ATTR_SLOT, (index/1, owner -- attr, null if (oparg & 1))) { diff --git a/Python/optimizer_cases.c.h b/Python/optimizer_cases.c.h index b46079ec8a1992..076e54342fd796 100644 --- a/Python/optimizer_cases.c.h +++ b/Python/optimizer_cases.c.h @@ -1219,22 +1219,33 @@ } case _CHECK_ATTR_WITH_HINT: { + _Py_UopsSymbol *owner; + _Py_UopsSymbol *dict; + owner = stack_pointer[-1]; + dict = sym_new_not_null(ctx); + (void)owner; + stack_pointer[0] = dict; + stack_pointer += 1; + assert(WITHIN_STACK_BOUNDS()); break; } case _LOAD_ATTR_WITH_HINT: { + _Py_UopsSymbol *dict; _Py_UopsSymbol *owner; _Py_UopsSymbol *attr; _Py_UopsSymbol *null = NULL; - owner = stack_pointer[-1]; + dict = stack_pointer[-1]; + owner = stack_pointer[-2]; uint16_t hint = (uint16_t)this_instr->operand0; attr = sym_new_not_null(ctx); null = sym_new_null(ctx); (void)hint; (void)owner; - stack_pointer[-1] = attr; - if (oparg & 1) stack_pointer[0] = null; - stack_pointer += (oparg & 1); + (void)dict; + stack_pointer[-2] = attr; + if (oparg & 1) stack_pointer[-1] = null; + stack_pointer += -1 + (oparg & 1); assert(WITHIN_STACK_BOUNDS()); break; } From bb00f5ac82ef7038f35715c43f227edbee1e947f Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 23 Dec 2024 12:08:49 -0800 Subject: [PATCH 52/67] Fix unused variable warning --- Python/bytecodes.c | 2 -- Python/executor_cases.c.h | 1 - Python/generated_cases.c.h | 1 - 3 files changed, 4 deletions(-) diff --git a/Python/bytecodes.c b/Python/bytecodes.c index 3dbed066940ebd..08925b70319e64 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2224,9 +2224,7 @@ dummy_func( } op(_LOAD_ATTR_WITH_HINT, (hint/1, owner, dict: PyDictObject * -- attr, null if (oparg & 1))) { - PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); PyObject *attr_o; - if (!LOCK_OBJECT(dict)) { DEAD(dict); POP_DEAD_INPUTS(); diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index 6ac05d7d546182..b2a80c3f079536 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -2776,7 +2776,6 @@ dict = (PyDictObject *)stack_pointer[-1].bits; owner = stack_pointer[-2]; uint16_t hint = (uint16_t)CURRENT_OPERAND0(); - PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); PyObject *attr_o; if (!LOCK_OBJECT(dict)) { stack_pointer += -1; diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 3a380d0d808e5e..19b87dbeb428f6 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -5823,7 +5823,6 @@ // _LOAD_ATTR_WITH_HINT { uint16_t hint = read_u16(&this_instr[4].cache); - PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); PyObject *attr_o; if (!LOCK_OBJECT(dict)) { DEOPT_IF(true, LOAD_ATTR); From b6ae487d6b61a3225e6e3c3bc23127b965e0c604 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 23 Dec 2024 17:43:17 -0800 Subject: [PATCH 53/67] Update number of specialization failure kinds --- Include/cpython/pystats.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Include/cpython/pystats.h b/Include/cpython/pystats.h index 29ef0c0e4d4e72..ee8885cda7b60d 100644 --- a/Include/cpython/pystats.h +++ b/Include/cpython/pystats.h @@ -31,7 +31,7 @@ #define PYSTATS_MAX_UOP_ID 512 -#define SPECIALIZATION_FAILURE_KINDS 36 +#define SPECIALIZATION_FAILURE_KINDS 37 /* Stats for determining who is calling PyEval_EvalFrame */ #define EVAL_CALL_TOTAL 0 From 11a351da3a0edb616dc5acf77e47b54a794cec22 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 23 Dec 2024 19:40:46 -0800 Subject: [PATCH 54/67] Clarify construction of deferred object --- Lib/test/test_opcache.py | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/Lib/test/test_opcache.py b/Lib/test/test_opcache.py index 1dee1e5c605a10..a11183ad7667ea 100644 --- a/Lib/test/test_opcache.py +++ b/Lib/test/test_opcache.py @@ -564,6 +564,16 @@ def instantiate(): instantiate() +def make_deferred_ref_count_obj(): + """Create an object that uses deferred reference counting. + + Only objects that use deferred refence counting may be stored in inline + caches in free-threaded builds. This constructs a new class named Foo, + which uses deferred reference counting. + """ + return type("Foo", (object,), {}) + + @threading_helper.requires_working_threading() class TestRacesDoNotCrash(TestBase): # Careful with these. Bigger numbers have a higher chance of catching bugs, @@ -718,9 +728,7 @@ def write(items): def test_load_attr_class(self): def get_items(): class C: - # a must be set to an instance that uses deferred reference - # counting in free-threaded builds - a = type("Foo", (object,), {}) + a = make_deferred_ref_count_obj() items = [] for _ in range(self.ITEMS): @@ -741,7 +749,7 @@ def write(items): del item.a except AttributeError: pass - item.a = type("Foo", (object,), {}) + item.a = make_deferred_ref_count_obj() opname = "LOAD_ATTR_CLASS" self.assert_races_do_not_crash(opname, get_items, read, write) @@ -753,9 +761,7 @@ class Meta(type): pass class C(metaclass=Meta): - # a must be set to an instance that uses deferred reference - # counting in free-threaded builds - a = type("Foo", (object,), {}) + a = make_deferred_ref_count_obj() items = [] for _ in range(self.ITEMS): @@ -776,7 +782,7 @@ def write(items): del item.a except AttributeError: pass - item.a = type("Foo", (object,), {}) + item.a = make_deferred_ref_count_obj() opname = "LOAD_ATTR_CLASS_WITH_METACLASS_CHECK" self.assert_races_do_not_crash(opname, get_items, read, write) From 6f8aebf58e1302b9ae740d45b720ec5168ee30c7 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 23 Dec 2024 22:05:47 -0800 Subject: [PATCH 55/67] Add suppression for _PyUnicode_CheckConsistency --- Tools/tsan/suppressions_free_threading.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Tools/tsan/suppressions_free_threading.txt b/Tools/tsan/suppressions_free_threading.txt index e5eb665ae212de..09dd2ae3042ff6 100644 --- a/Tools/tsan/suppressions_free_threading.txt +++ b/Tools/tsan/suppressions_free_threading.txt @@ -43,6 +43,8 @@ race_top:write_thread_id race_top:PyThreadState_Clear # Only seen on macOS, sample: https://gist.github.com/aisk/dda53f5d494a4556c35dde1fce03259c race_top:set_default_allocator_unlocked +# gh-128212 +race_top:_PyUnicode_CheckConsistency # https://gist.github.com/mpage/6962e8870606cfc960e159b407a0cb40 thread:pthread_create From 187088519be53a800700c3971c59b86dd637f77e Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 2 Jan 2025 10:12:39 -0800 Subject: [PATCH 56/67] Fix compiler warning --- Python/specialize.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Python/specialize.c b/Python/specialize.c index 2f4adabeeda2a1..47c7a4cf49fd93 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -1595,8 +1595,8 @@ specialize_attr_loadclassattr(PyObject *owner, _Py_CODEUNIT *instr, unsigned long tp_flags = PyType_GetFlags(owner_cls); if (tp_flags & Py_TPFLAGS_INLINE_VALUES) { #ifndef Py_GIL_DISABLED - PyDictKeysObject *keys = ((PyHeapTypeObject *)owner_cls)->ht_cached_keys; - assert(_PyDictKeys_StringLookup(keys, name) < 0); + assert(_PyDictKeys_StringLookup( + ((PyHeapTypeObject *)owner_cls)->ht_cached_keys, name) < 0); #endif if (shared_keys_version == 0) { SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_OUT_OF_VERSIONS); From f1fdcc608ca5414e9f56491f96ee34fb6da8938c Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 2 Jan 2025 10:26:29 -0800 Subject: [PATCH 57/67] Make it easier to reason about thread-safety of instance_has_key --- Python/specialize.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/Python/specialize.c b/Python/specialize.c index 47c7a4cf49fd93..ae869fe1b32170 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -1105,17 +1105,16 @@ instance_has_key(PyObject *obj, PyObject *name, uint32_t *shared_keys_version) if (dict == NULL || !PyDict_CheckExact(dict)) { return false; } - if (FT_ATOMIC_LOAD_PTR(dict->ma_values)) { - return false; - } - Py_ssize_t index; + bool result; Py_BEGIN_CRITICAL_SECTION(dict); - index = _PyDict_LookupIndex(dict, name); - Py_END_CRITICAL_SECTION(); - if (index < 0) { - return false; + if (dict->ma_values) { + result = false; } - return true; + else { + result = (_PyDict_LookupIndex(dict, name) >= 0); + } + Py_END_CRITICAL_SECTION(); + return result; } static int From 8b71951df67f38c95076c7236d8e71c5337875a9 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 2 Jan 2025 10:39:44 -0800 Subject: [PATCH 58/67] Restore refactor lost in merge Use descriptor_is_class --- Python/specialize.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/Python/specialize.c b/Python/specialize.c index ae869fe1b32170..f3a3d2919c6a36 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -942,10 +942,8 @@ analyze_descriptor_load(PyTypeObject *type, PyObject *name, PyObject **descr, un PyObject *descriptor = _PyType_LookupRefAndVersion(type, name, &descr_version); *descr = descriptor; *tp_version = have_ga_version ? ga_version : descr_version; - if (PyUnicode_CompareWithASCIIString(name, "__class__") == 0) { - if (descriptor == _PyType_Lookup(&PyBaseObject_Type, name)) { - return DUNDER_CLASS; - } + if (descriptor_is_class(descriptor, name)) { + return DUNDER_CLASS; } return classify_descriptor(descriptor, has_getattr); } From 8b96368e6604ec3c0a47526230fe8ab8eebbc403 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 2 Jan 2025 11:28:47 -0800 Subject: [PATCH 59/67] Revert "Add suppression for _PyUnicode_CheckConsistency" This reverts commit 6f8aebf58e1302b9ae740d45b720ec5168ee30c7. --- Tools/tsan/suppressions_free_threading.txt | 2 -- 1 file changed, 2 deletions(-) diff --git a/Tools/tsan/suppressions_free_threading.txt b/Tools/tsan/suppressions_free_threading.txt index 09dd2ae3042ff6..e5eb665ae212de 100644 --- a/Tools/tsan/suppressions_free_threading.txt +++ b/Tools/tsan/suppressions_free_threading.txt @@ -43,8 +43,6 @@ race_top:write_thread_id race_top:PyThreadState_Clear # Only seen on macOS, sample: https://gist.github.com/aisk/dda53f5d494a4556c35dde1fce03259c race_top:set_default_allocator_unlocked -# gh-128212 -race_top:_PyUnicode_CheckConsistency # https://gist.github.com/mpage/6962e8870606cfc960e159b407a0cb40 thread:pthread_create From e07fb62a62213f303a065f719ddd03fd580223ab Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 9 Jan 2025 10:21:23 -0800 Subject: [PATCH 60/67] Split check_keys_and_hash These are two logically separate operations --- Objects/dictobject.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 14f22dc8ed2af9..0f9542da65b93d 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -1129,13 +1129,16 @@ dictkeys_generic_lookup(PyDictObject *mp, PyDictKeysObject* dk, PyObject *key, P return do_lookup(mp, dk, key, hash, compare_generic); } -static Py_hash_t -check_keys_and_hash(PyDictKeysObject *dk, PyObject *key) +static bool +check_keys_unicode(PyDictKeysObject *dk, PyObject *key) { - DictKeysKind kind = dk->dk_kind; - if (!PyUnicode_CheckExact(key) || kind == DICT_KEYS_GENERAL) { - return -1; - } + return PyUnicode_CheckExact(key) && (dk->dk_kind != DICT_KEYS_GENERAL); +} + +Py_ssize_t +hash_unicode_key(PyObject *key) +{ + assert(PyUnicode_CheckExact(key)); Py_hash_t hash = unicode_get_hash(key); if (hash == -1) { hash = PyUnicode_Type.tp_hash(key); @@ -1182,22 +1185,21 @@ unicodekeys_lookup_split(PyDictKeysObject* dk, PyObject *key, Py_hash_t hash) Py_ssize_t _PyDictKeys_StringLookup(PyDictKeysObject* dk, PyObject *key) { - Py_hash_t hash = check_keys_and_hash(dk, key); - if (hash == -1) { + if (!check_keys_unicode(dk, key)) { return DKIX_ERROR; } + Py_hash_t hash = hash_unicode_key(key); return unicodekeys_lookup_unicode(dk, key, hash); } Py_ssize_t _PyDictKeys_StringLookupAndVersion(PyDictKeysObject *dk, PyObject *key, uint32_t *version) { - Py_hash_t hash = check_keys_and_hash(dk, key); - if (hash == -1) { + if (!check_keys_unicode(dk, key)) { return DKIX_ERROR; } - Py_ssize_t ix; + Py_hash_t hash = hash_unicode_key(key); LOCK_KEYS(dk); ix = unicodekeys_lookup_unicode(dk, key, hash); *version = _PyDictKeys_GetVersionForCurrentState(_PyInterpreterState_GET(), dk); From a3f89b7d13d2074fbc02b4eedd81be174bfef1c7 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 9 Jan 2025 13:04:18 -0800 Subject: [PATCH 61/67] Clear inline values upon invalidation This avoids a race where another thread invalidates the values, overwrites an attribute stored in the values, and allocates a new object at the address present in the values. --- Objects/dictobject.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 0f9542da65b93d..97939d7b5eff28 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -1951,6 +1951,17 @@ build_indices_unicode(PyDictKeysObject *keys, PyDictUnicodeEntry *ep, Py_ssize_t } } + +static void +invalidate_and_clear_inline_values(PyDictValues *values) +{ + assert(values->embedded); + FT_ATOMIC_STORE_UINT8(values->valid, 0); + for (int i = 0; i < values->capacity; i++) { + FT_ATOMIC_STORE_PTR(values->values[i], NULL); + } +} + /* Restructure the table by allocating a new table and reinserting all items again. When entries have been deleted, the new table may @@ -2042,7 +2053,7 @@ dictresize(PyInterpreterState *interp, PyDictObject *mp, if (oldvalues->embedded) { assert(oldvalues->embedded == 1); assert(oldvalues->valid == 1); - FT_ATOMIC_STORE_UINT8(oldvalues->valid, 0); + invalidate_and_clear_inline_values(oldvalues); } else { free_values(oldvalues, IS_DICT_SHARED(mp)); @@ -7032,7 +7043,13 @@ _PyObject_TryGetInstanceAttribute(PyObject *obj, PyObject *name, PyObject **attr #ifdef Py_GIL_DISABLED PyObject *value = _Py_atomic_load_ptr_acquire(&values->values[ix]); - if (value == NULL || _Py_TryIncrefCompare(&values->values[ix], value)) { + if (value == NULL) { + if (FT_ATOMIC_LOAD_UINT8(values->valid)) { + *attr = NULL; + return true; + } + } + else if (_Py_TryIncrefCompare(&values->values[ix], value)) { *attr = value; return true; } @@ -7370,7 +7387,7 @@ _PyDict_DetachFromObject(PyDictObject *mp, PyObject *obj) } mp->ma_values = values; - FT_ATOMIC_STORE_UINT8(_PyObject_InlineValues(obj)->valid, 0); + invalidate_and_clear_inline_values(_PyObject_InlineValues(obj)); assert(_PyObject_InlineValuesConsistencyCheck(obj)); ASSERT_CONSISTENT(mp); From 3baa8400a53a4237b7fe2ca83468d7e6e34345fe Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 9 Jan 2025 16:53:42 -0800 Subject: [PATCH 62/67] Replace kill/pop_dead_inputs with pop_input --- Lib/test/test_generated_cases.py | 58 ++++------------------ Python/bytecodes.c | 18 +++---- Tools/cases_generator/generators_common.py | 10 ++-- Tools/cases_generator/stack.py | 4 -- 4 files changed, 22 insertions(+), 68 deletions(-) diff --git a/Lib/test/test_generated_cases.py b/Lib/test/test_generated_cases.py index 390659f50c0cd1..ae1dc90b18efef 100644 --- a/Lib/test/test_generated_cases.py +++ b/Lib/test/test_generated_cases.py @@ -1674,35 +1674,10 @@ def test_pystackref_frompyobject_new_next_to_cmacro(self): """ self.run_cases_test(input, output) - def test_pop_dead_inputs_all_live(self): + def test_pop_input(self): input = """ inst(OP, (a, b --)) { - POP_DEAD_INPUTS(); - HAM(a, b); - INPUTS_DEAD(); - } - """ - output = """ - TARGET(OP) { - frame->instr_ptr = next_instr; - next_instr += 1; - INSTRUCTION_STATS(OP); - _PyStackRef a; - _PyStackRef b; - b = stack_pointer[-1]; - a = stack_pointer[-2]; - HAM(a, b); - stack_pointer += -2; - assert(WITHIN_STACK_BOUNDS()); - DISPATCH(); - } - """ - self.run_cases_test(input, output) - - def test_pop_dead_inputs_some_live(self): - input = """ - inst(OP, (a, b, c --)) { - POP_DEAD_INPUTS(); + POP_INPUT(); HAM(a); INPUTS_DEAD(); } @@ -1713,8 +1688,8 @@ def test_pop_dead_inputs_some_live(self): next_instr += 1; INSTRUCTION_STATS(OP); _PyStackRef a; - a = stack_pointer[-3]; - stack_pointer += -2; + a = stack_pointer[-2]; + stack_pointer += -1; assert(WITHIN_STACK_BOUNDS()); HAM(a); stack_pointer += -1; @@ -1724,29 +1699,14 @@ def test_pop_dead_inputs_some_live(self): """ self.run_cases_test(input, output) - def test_pop_dead_inputs_with_output(self): + def test_pop_input_with_empty_stack(self): input = """ - inst(OP, (a, b -- c)) { - POP_DEAD_INPUTS(); - c = SPAM(); - } - """ - output = """ - TARGET(OP) { - frame->instr_ptr = next_instr; - next_instr += 1; - INSTRUCTION_STATS(OP); - _PyStackRef c; - stack_pointer += -2; - assert(WITHIN_STACK_BOUNDS()); - c = SPAM(); - stack_pointer[0] = c; - stack_pointer += 1; - assert(WITHIN_STACK_BOUNDS()); - DISPATCH(); + inst(OP, (--)) { + POP_INPUT(); } """ - self.run_cases_test(input, output) + with self.assertRaises(SyntaxError): + self.run_cases_test(input, "") def test_no_escaping_calls_in_branching_macros(self): diff --git a/Python/bytecodes.c b/Python/bytecodes.c index 877dabdbe37a05..79b659131fcd9b 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2221,9 +2221,8 @@ dummy_func( assert(index < FT_ATOMIC_LOAD_SSIZE_RELAXED(mod_keys->dk_nentries)); PyDictUnicodeEntry *ep = DK_UNICODE_ENTRIES(mod_keys) + index; PyObject *attr_o = FT_ATOMIC_LOAD_PTR_RELAXED(ep->me_value); - DEAD(mod_keys); // Clear mod_keys from stack in case we need to deopt - POP_DEAD_INPUTS(); + POP_INPUT(); DEOPT_IF(attr_o == NULL); #ifdef Py_GIL_DISABLED int increfed = _Py_TryIncrefCompareStackRef(&ep->me_value, attr_o, &attr); @@ -2258,36 +2257,31 @@ dummy_func( op(_LOAD_ATTR_WITH_HINT, (hint/1, owner, dict: PyDictObject * -- attr, null if (oparg & 1))) { PyObject *attr_o; if (!LOCK_OBJECT(dict)) { - DEAD(dict); - POP_DEAD_INPUTS(); + POP_INPUT(); DEOPT_IF(true); } if (hint >= (size_t)dict->ma_keys->dk_nentries) { UNLOCK_OBJECT(dict); - DEAD(dict); - POP_DEAD_INPUTS(); + POP_INPUT(); DEOPT_IF(true); } PyObject *name = GETITEM(FRAME_CO_NAMES, oparg>>1); if (dict->ma_keys->dk_kind != DICT_KEYS_UNICODE) { UNLOCK_OBJECT(dict); - DEAD(dict); - POP_DEAD_INPUTS(); + POP_INPUT(); DEOPT_IF(true); } PyDictUnicodeEntry *ep = DK_UNICODE_ENTRIES(dict->ma_keys) + hint; if (ep->me_key != name) { UNLOCK_OBJECT(dict); - DEAD(dict); - POP_DEAD_INPUTS(); + POP_INPUT(); DEOPT_IF(true); } attr_o = ep->me_value; if (attr_o == NULL) { UNLOCK_OBJECT(dict); - DEAD(dict); - POP_DEAD_INPUTS(); + POP_INPUT(); DEOPT_IF(true); } STAT_INC(LOAD_ATTR, hit); diff --git a/Tools/cases_generator/generators_common.py b/Tools/cases_generator/generators_common.py index 8df9a9cada92ae..59199c870eacf4 100644 --- a/Tools/cases_generator/generators_common.py +++ b/Tools/cases_generator/generators_common.py @@ -120,7 +120,7 @@ def __init__(self, out: CWriter): "PyStackRef_AsPyObjectSteal": self.stackref_steal, "DISPATCH": self.dispatch, "INSTRUCTION_SIZE": self.instruction_size, - "POP_DEAD_INPUTS": self.pop_dead_inputs, + "POP_INPUT": self.pop_input, } self.out = out @@ -349,7 +349,7 @@ def save_stack( self.emit_save(storage) return True - def pop_dead_inputs( + def pop_input( self, tkn: Token, tkn_iter: TokenIterator, @@ -360,7 +360,11 @@ def pop_dead_inputs( next(tkn_iter) next(tkn_iter) next(tkn_iter) - storage.pop_dead_inputs(self.out) + if not storage.inputs: + raise analysis_error("stack is empty", tkn) + storage.inputs[-1].defined = False + storage.clear_dead_inputs() + storage.flush(self.out) return True def emit_reload(self, storage: Storage) -> None: diff --git a/Tools/cases_generator/stack.py b/Tools/cases_generator/stack.py index 9471fe0e56f7d8..286f47d0cfb11b 100644 --- a/Tools/cases_generator/stack.py +++ b/Tools/cases_generator/stack.py @@ -512,10 +512,6 @@ def flush(self, out: CWriter, cast_type: str = "uintptr_t", extract_bits: bool = self._push_defined_outputs() self.stack.flush(out, cast_type, extract_bits) - def pop_dead_inputs(self, out: CWriter, cast_type: str = "uintptr_t", extract_bits: bool = True) -> None: - self.clear_dead_inputs() - self.stack.flush(out, cast_type, extract_bits) - def save(self, out: CWriter) -> None: assert self.spilled >= 0 if self.spilled == 0: From 9250e4e67a78c556ff4a73fe61e2cbba1166592f Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 9 Jan 2025 17:29:53 -0800 Subject: [PATCH 63/67] Fix formatting --- Objects/dictobject.c | 1 - 1 file changed, 1 deletion(-) diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 97939d7b5eff28..3715cac1fa6d1d 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -1951,7 +1951,6 @@ build_indices_unicode(PyDictKeysObject *keys, PyDictUnicodeEntry *ep, Py_ssize_t } } - static void invalidate_and_clear_inline_values(PyDictValues *values) { From 675eb8291fa775697e1263905ab5177ed01c1a41 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 9 Jan 2025 17:38:44 -0800 Subject: [PATCH 64/67] Fix visibility --- Objects/dictobject.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 3715cac1fa6d1d..6ce116fd037477 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -1135,7 +1135,7 @@ check_keys_unicode(PyDictKeysObject *dk, PyObject *key) return PyUnicode_CheckExact(key) && (dk->dk_kind != DICT_KEYS_GENERAL); } -Py_ssize_t +static Py_ssize_t hash_unicode_key(PyObject *key) { assert(PyUnicode_CheckExact(key)); From 10d693d19a523cb7ef00cca34c8c325e8b12ee4c Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 10 Jan 2025 12:28:27 -0800 Subject: [PATCH 65/67] Use release instead of seq_cst --- Objects/dictobject.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 6ce116fd037477..82789d5e56f523 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -1957,7 +1957,7 @@ invalidate_and_clear_inline_values(PyDictValues *values) assert(values->embedded); FT_ATOMIC_STORE_UINT8(values->valid, 0); for (int i = 0; i < values->capacity; i++) { - FT_ATOMIC_STORE_PTR(values->values[i], NULL); + FT_ATOMIC_STORE_PTR_RELEASE(values->values[i], NULL); } } From 7ab3ec65ed4947af23b13df8827fef2e9aecee93 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 10 Jan 2025 12:42:47 -0800 Subject: [PATCH 66/67] Make POP_INPUT take the name of the TOS --- Lib/test/test_generated_cases.py | 15 +++++++++++++-- Python/bytecodes.c | 12 ++++++------ Tools/cases_generator/generators_common.py | 7 ++++++- 3 files changed, 25 insertions(+), 9 deletions(-) diff --git a/Lib/test/test_generated_cases.py b/Lib/test/test_generated_cases.py index ae1dc90b18efef..ced4e0c99f3d7f 100644 --- a/Lib/test/test_generated_cases.py +++ b/Lib/test/test_generated_cases.py @@ -1677,7 +1677,7 @@ def test_pystackref_frompyobject_new_next_to_cmacro(self): def test_pop_input(self): input = """ inst(OP, (a, b --)) { - POP_INPUT(); + POP_INPUT(b); HAM(a); INPUTS_DEAD(); } @@ -1688,6 +1688,8 @@ def test_pop_input(self): next_instr += 1; INSTRUCTION_STATS(OP); _PyStackRef a; + _PyStackRef b; + b = stack_pointer[-1]; a = stack_pointer[-2]; stack_pointer += -1; assert(WITHIN_STACK_BOUNDS()); @@ -1702,7 +1704,16 @@ def test_pop_input(self): def test_pop_input_with_empty_stack(self): input = """ inst(OP, (--)) { - POP_INPUT(); + POP_INPUT(foo); + } + """ + with self.assertRaises(SyntaxError): + self.run_cases_test(input, "") + + def test_pop_input_with_non_tos(self): + input = """ + inst(OP, (a, b --)) { + POP_INPUT(a); } """ with self.assertRaises(SyntaxError): diff --git a/Python/bytecodes.c b/Python/bytecodes.c index 79b659131fcd9b..e7b1255cf5233c 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2222,7 +2222,7 @@ dummy_func( PyDictUnicodeEntry *ep = DK_UNICODE_ENTRIES(mod_keys) + index; PyObject *attr_o = FT_ATOMIC_LOAD_PTR_RELAXED(ep->me_value); // Clear mod_keys from stack in case we need to deopt - POP_INPUT(); + POP_INPUT(mod_keys); DEOPT_IF(attr_o == NULL); #ifdef Py_GIL_DISABLED int increfed = _Py_TryIncrefCompareStackRef(&ep->me_value, attr_o, &attr); @@ -2257,31 +2257,31 @@ dummy_func( op(_LOAD_ATTR_WITH_HINT, (hint/1, owner, dict: PyDictObject * -- attr, null if (oparg & 1))) { PyObject *attr_o; if (!LOCK_OBJECT(dict)) { - POP_INPUT(); + POP_INPUT(dict); DEOPT_IF(true); } if (hint >= (size_t)dict->ma_keys->dk_nentries) { UNLOCK_OBJECT(dict); - POP_INPUT(); + POP_INPUT(dict); DEOPT_IF(true); } PyObject *name = GETITEM(FRAME_CO_NAMES, oparg>>1); if (dict->ma_keys->dk_kind != DICT_KEYS_UNICODE) { UNLOCK_OBJECT(dict); - POP_INPUT(); + POP_INPUT(dict); DEOPT_IF(true); } PyDictUnicodeEntry *ep = DK_UNICODE_ENTRIES(dict->ma_keys) + hint; if (ep->me_key != name) { UNLOCK_OBJECT(dict); - POP_INPUT(); + POP_INPUT(dict); DEOPT_IF(true); } attr_o = ep->me_value; if (attr_o == NULL) { UNLOCK_OBJECT(dict); - POP_INPUT(); + POP_INPUT(dict); DEOPT_IF(true); } STAT_INC(LOAD_ATTR, hit); diff --git a/Tools/cases_generator/generators_common.py b/Tools/cases_generator/generators_common.py index 59199c870eacf4..01127f81e34cec 100644 --- a/Tools/cases_generator/generators_common.py +++ b/Tools/cases_generator/generators_common.py @@ -358,11 +358,16 @@ def pop_input( inst: Instruction | None, ) -> bool: next(tkn_iter) + name_tkn = next(tkn_iter) + name = name_tkn.text next(tkn_iter) next(tkn_iter) if not storage.inputs: raise analysis_error("stack is empty", tkn) - storage.inputs[-1].defined = False + tos = storage.inputs[-1] + if tos.name != name: + raise analysis_error(f"'{name} is not top of stack", name_tkn) + tos.defined = False storage.clear_dead_inputs() storage.flush(self.out) return True From 3616eab97d05db99b013321a2db19a60fdbc1b98 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Tue, 14 Jan 2025 09:58:28 -0800 Subject: [PATCH 67/67] Use DECREF_INPUTS() instead of manually closing inputs --- Python/bytecodes.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Python/bytecodes.c b/Python/bytecodes.c index b8ac33a98a305c..a906ded365650c 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2300,8 +2300,7 @@ dummy_func( UNLOCK_OBJECT(dict); DEAD(dict); null = PyStackRef_NULL; - PyStackRef_CLOSE(owner); - INPUTS_DEAD(); + DECREF_INPUTS(); } macro(LOAD_ATTR_WITH_HINT) =