diff --git a/Include/cpython/object.h b/Include/cpython/object.h index 70cf0b51f140a9..639959060537b7 100644 --- a/Include/cpython/object.h +++ b/Include/cpython/object.h @@ -284,6 +284,15 @@ typedef struct _heaptypeobject { struct _specialization_cache _spec_cache; // For use by the specializer. #ifdef Py_GIL_DISABLED Py_ssize_t unique_id; // ID used for per-thread refcounting + + // Used to store type flags that can be toggled after the type + // structure has been potentially exposed to other threads (i.e. after + // type_ready()). We need to use atomic loads and stores for these + // flags, unlike for tp_flags. The set of flags allowed to be toggled are + // POST_READY_FLAGS. They are toggled with type_set_flags_with_mask() and + // need to be tested with _PyType_HasFeatureSafe(), in order to avoid data + // races. + unsigned long ht_flags; #endif /* here are optional user slots, followed by the members. */ } PyHeapTypeObject; diff --git a/Include/internal/pycore_call.h b/Include/internal/pycore_call.h index 49f5c3322de267..d7c451e44ff3e1 100644 --- a/Include/internal/pycore_call.h +++ b/Include/internal/pycore_call.h @@ -9,6 +9,7 @@ extern "C" { #endif #include "pycore_pystate.h" // _PyThreadState_GET() +#include "pycore_object.h" // _PyType_HaveFeatureSafe() /* Suggested size (number of positional arguments) for arrays of PyObject* allocated on a C stack to avoid allocating memory on the heap memory. Such @@ -116,7 +117,7 @@ _PyVectorcall_FunctionInline(PyObject *callable) assert(callable != NULL); PyTypeObject *tp = Py_TYPE(callable); - if (!PyType_HasFeature(tp, Py_TPFLAGS_HAVE_VECTORCALL)) { + if (!_PyType_HasFeatureSafe(tp, Py_TPFLAGS_HAVE_VECTORCALL)) { return NULL; } assert(PyCallable_Check(callable)); diff --git a/Include/internal/pycore_object.h b/Include/internal/pycore_object.h index 53403ebcfc0043..94cc71850d9650 100644 --- a/Include/internal/pycore_object.h +++ b/Include/internal/pycore_object.h @@ -303,7 +303,20 @@ extern int _PyDict_CheckConsistency(PyObject *mp, int check_content); // Fast inlined version of PyType_HasFeature() static inline int _PyType_HasFeature(PyTypeObject *type, unsigned long feature) { - return ((FT_ATOMIC_LOAD_ULONG_RELAXED(type->tp_flags) & feature) != 0); + return (type->tp_flags & feature) != 0; +} + +// Variant of above function that uses safely reads type flags that can be +// toggled after the type is first created. +static inline int +_PyType_HasFeatureSafe(PyTypeObject *type, unsigned long feature) { +#ifdef Py_GIL_DISABLED + if (_PyType_HasFeature(type, Py_TPFLAGS_HEAPTYPE)) { + PyHeapTypeObject *ht = (PyHeapTypeObject*)type; + return (FT_ATOMIC_LOAD_ULONG_RELAXED(ht->ht_flags) & feature) != 0; + } +#endif + return (type->tp_flags & feature) != 0; } extern void _PyType_InitCache(PyInterpreterState *interp); diff --git a/Include/object.h b/Include/object.h index 7d1667d889cb43..649f300f7bad51 100644 --- a/Include/object.h +++ b/Include/object.h @@ -774,11 +774,7 @@ PyType_HasFeature(PyTypeObject *type, unsigned long feature) // PyTypeObject is opaque in the limited C API flags = PyType_GetFlags(type); #else -# ifdef Py_GIL_DISABLED - flags = _Py_atomic_load_ulong_relaxed(&type->tp_flags); -# else - flags = type->tp_flags; -# endif + flags = type->tp_flags; #endif return ((flags & feature) != 0); } diff --git a/Lib/test/test_free_threading/test_races.py b/Lib/test/test_free_threading/test_races.py index 85aa69c8cd494f..f3e6d91a91c127 100644 --- a/Lib/test/test_free_threading/test_races.py +++ b/Lib/test/test_free_threading/test_races.py @@ -130,6 +130,45 @@ def mutate(): # with the cell binding being changed). do_race(access, mutate) + def test_racing_tp_flags_is_abstract(self): + class C: + pass + + def access(): + obj = C() + + def mutate(): + C.__abstractmethods__ = set() + time.sleep(0) + del C.__abstractmethods__ + time.sleep(0) + + # The "mutate" method will set and clear the Py_TPFLAGS_IS_ABSTRACT type flag. + do_race(access, mutate) + + def test_racing_tp_flags_vectorcall(self): + class C: + pass + + def access(): + try: + C()() + except Exception: + pass + + def mutate(): + nonlocal C + # This will clear the Py_TPFLAGS_VECTORCALL flag. + C.__call__ = lambda self: None + time.sleep(0) + # There is no way to re-set the flag it so we create a new class. + class C: + pass + time.sleep(0) + + do_race(access, mutate) + + def test_racing_to_bool(self): seq = [1] diff --git a/Lib/test/test_sys.py b/Lib/test/test_sys.py index 87c0106ad30840..72299fd01a16e1 100644 --- a/Lib/test/test_sys.py +++ b/Lib/test/test_sys.py @@ -1750,6 +1750,7 @@ def delx(self): del self.__x s = vsize(fmt) check(int, s) typeid = 'n' if support.Py_GIL_DISABLED else '' + ht_flags = 'l' if support.Py_GIL_DISABLED else '' # class s = vsize(fmt + # PyTypeObject '4P' # PyAsyncMethods @@ -1760,6 +1761,7 @@ def delx(self): del self.__x '7P' '1PIP' # Specializer cache + typeid # heap type id (free-threaded only) + + ht_flags ) class newstyleclass(object): pass # Separate block for PyDictKeysObject with 8 keys and 5 entries diff --git a/Modules/_testcapi/vectorcall.c b/Modules/_testcapi/vectorcall.c index 03aaacb328e0b6..d9a8c20c8a890b 100644 --- a/Modules/_testcapi/vectorcall.c +++ b/Modules/_testcapi/vectorcall.c @@ -3,6 +3,7 @@ #include "parts.h" #include "clinic/vectorcall.c.h" +#include "pycore_object.h" // _PyType_HasFeatureSafe() #include // offsetof @@ -255,7 +256,7 @@ static int _testcapi_has_vectorcall_flag_impl(PyObject *module, PyTypeObject *type) /*[clinic end generated code: output=3ae8d1374388c671 input=8eee492ac548749e]*/ { - return PyType_HasFeature(type, Py_TPFLAGS_HAVE_VECTORCALL); + return _PyType_HasFeatureSafe(type, Py_TPFLAGS_HAVE_VECTORCALL); } static PyMethodDef TestMethods[] = { diff --git a/Objects/typeobject.c b/Objects/typeobject.c index bc840ed51ffe4c..b6086a5148195f 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -347,23 +347,43 @@ _PyStaticType_GetBuiltins(void) static void type_set_flags(PyTypeObject *tp, unsigned long flags) { - if (tp->tp_flags & Py_TPFLAGS_READY) { - // It's possible the type object has been exposed to other threads - // if it's been marked ready. In that case, the type lock should be - // held when flags are modified. - ASSERT_TYPE_LOCK_HELD(); - } - // Since PyType_HasFeature() reads the flags without holding the type - // lock, we need an atomic store here. - FT_ATOMIC_STORE_ULONG_RELAXED(tp->tp_flags, flags); + // In the free-threaded build, this can only be used on types that have + // not yet been exposed to other threads. Use type_set_flags_with_mask() + // for flags that need to be toggled afterwards. + tp->tp_flags = flags; } +// Flags used by pattern matching +#define COLLECTION_FLAGS (Py_TPFLAGS_SEQUENCE | Py_TPFLAGS_MAPPING) + +// This is the set of flags are allowed to be toggled after type_ready() +// is called (and the type potentially exposed to other threads). +#define POST_READY_FLAGS \ + (COLLECTION_FLAGS | Py_TPFLAGS_IS_ABSTRACT | Py_TPFLAGS_HAVE_VECTORCALL) + +// Used to toggle type flags after the type has been created. Only the flags +// in POST_READY_FLAGS are allowed to be toggled. static void type_set_flags_with_mask(PyTypeObject *tp, unsigned long mask, unsigned long flags) { ASSERT_TYPE_LOCK_HELD(); + // Non-heap types are immutable and so these flags can only be toggled + // after creation on heap types. + assert(_PyType_HasFeature(tp, Py_TPFLAGS_HEAPTYPE)); unsigned long new_flags = (tp->tp_flags & ~mask) | flags; - type_set_flags(tp, new_flags); +#ifdef Py_DEBUG + unsigned long changed_flags = tp->tp_flags ^ new_flags; + assert((changed_flags & ~POST_READY_FLAGS) == 0); +#endif +#ifdef Py_GIL_DISABLED + PyHeapTypeObject *ht = (PyHeapTypeObject*)tp; + FT_ATOMIC_STORE_ULONG_RELAXED(ht->ht_flags, new_flags); +#else + // In this case the GIL protects us and we don't need the separate ht_flags + // member and atomics to read and write. Instead we can just toggle the + // flags directly in tp_flags. + tp->tp_flags = new_flags; +#endif } static void @@ -1318,7 +1338,6 @@ int PyUnstable_Type_AssignVersionTag(PyTypeObject *type) static PyMemberDef type_members[] = { {"__basicsize__", Py_T_PYSSIZET, offsetof(PyTypeObject,tp_basicsize),Py_READONLY}, {"__itemsize__", Py_T_PYSSIZET, offsetof(PyTypeObject, tp_itemsize), Py_READONLY}, - {"__flags__", Py_T_ULONG, offsetof(PyTypeObject, tp_flags), Py_READONLY}, /* Note that this value is misleading for static builtin types, since the memory at this offset will always be NULL. */ {"__weakrefoffset__", Py_T_PYSSIZET, @@ -1584,9 +1603,9 @@ type_set_abstractmethods(PyObject *tp, PyObject *value, void *Py_UNUSED(closure) BEGIN_TYPE_LOCK(); type_modified_unlocked(type); if (abstract) - type_add_flags(type, Py_TPFLAGS_IS_ABSTRACT); + type_set_flags_with_mask(type, 0, Py_TPFLAGS_IS_ABSTRACT); else - type_clear_flags(type, Py_TPFLAGS_IS_ABSTRACT); + type_set_flags_with_mask(type, Py_TPFLAGS_IS_ABSTRACT, 0); END_TYPE_LOCK(); return 0; @@ -2109,6 +2128,24 @@ type_set_type_params(PyObject *tp, PyObject *value, void *Py_UNUSED(closure)) return result; } +static PyObject * +type_get_flags(PyObject *tp, void *Py_UNUSED(closure)) +{ + PyTypeObject *type = PyTypeObject_CAST(tp); + unsigned long flags; +#ifdef Py_GIL_DISABLED + if (_PyType_HasFeature(type, Py_TPFLAGS_HEAPTYPE)) { + PyHeapTypeObject *ht = (PyHeapTypeObject*)type; + flags = FT_ATOMIC_LOAD_ULONG_RELAXED(ht->ht_flags); + } + else { + flags = type->tp_flags; + } +#else + flags = type->tp_flags; +#endif + return PyLong_FromLong(flags); +} /*[clinic input] type.__instancecheck__ -> bool @@ -2157,6 +2194,7 @@ static PyGetSetDef type_getsets[] = { {"__annotations__", type_get_annotations, type_set_annotations, NULL}, {"__annotate__", type_get_annotate, type_set_annotate, NULL}, {"__type_params__", type_get_type_params, type_set_type_params, NULL}, + {"__flags__", type_get_flags, NULL, NULL}, {0} }; @@ -3687,7 +3725,13 @@ type_init(PyObject *cls, PyObject *args, PyObject *kwds) unsigned long PyType_GetFlags(PyTypeObject *type) { - return FT_ATOMIC_LOAD_ULONG_RELAXED(type->tp_flags); +#ifdef Py_GIL_DISABLED + if (_PyType_HasFeature(type, Py_TPFLAGS_HEAPTYPE)) { + PyHeapTypeObject *ht = (PyHeapTypeObject*)type; + return FT_ATOMIC_LOAD_ULONG_RELAXED(ht->ht_flags); + } +#endif + return type->tp_flags; } @@ -4005,6 +4049,7 @@ type_new_alloc(type_new_ctx *ctx) #ifdef Py_GIL_DISABLED et->unique_id = _PyObject_AssignUniqueId((PyObject *)et); + et->ht_flags = type->tp_flags; #endif return type; @@ -4428,6 +4473,7 @@ type_new_init(type_new_ctx *ctx) return NULL; } +static int type_ready(PyTypeObject *type, int initial); static PyObject* type_new_impl(type_new_ctx *ctx) @@ -4441,13 +4487,28 @@ type_new_impl(type_new_ctx *ctx) goto error; } + + int res; + + // This lock isn't strictly necessary because the type has not been + // exposed to anyone else yet, but update_one_slot calls find_name_in_mro + // where we'd like to assert that the type is locked. Also, type_ready() + // will assert that the type lock is held. + BEGIN_TYPE_LOCK(); + /* Initialize the rest */ - if (PyType_Ready(type) < 0) { - goto error; + res = type_ready(type, 1); + + if (res >= 0) { + // Put the proper slots in place + fixup_slot_dispatchers(type); } - // Put the proper slots in place - fixup_slot_dispatchers(type); + END_TYPE_LOCK(); + + if (res < 0) { + goto error; + } if (!_PyDict_HasOnlyStringKeys(type->tp_dict)) { if (PyErr_WarnFormat( @@ -6603,7 +6664,7 @@ object_new(PyTypeObject *type, PyObject *args, PyObject *kwds) } } - if (type->tp_flags & Py_TPFLAGS_IS_ABSTRACT) { + if (_PyType_HasFeatureSafe(type, Py_TPFLAGS_IS_ABSTRACT)) { PyObject *abstract_methods; PyObject *sorted_methods; PyObject *joined; @@ -8100,7 +8161,7 @@ inherit_slots(PyTypeObject *type, PyTypeObject *base) /* Inherit Py_TPFLAGS_HAVE_VECTORCALL if tp_call is not overridden */ if (!type->tp_call && - _PyType_HasFeature(base, Py_TPFLAGS_HAVE_VECTORCALL)) + _PyType_HasFeatureSafe(base, Py_TPFLAGS_HAVE_VECTORCALL)) { type_add_flags(type, Py_TPFLAGS_HAVE_VECTORCALL); } @@ -8169,8 +8230,6 @@ inherit_slots(PyTypeObject *type, PyTypeObject *base) static int add_operators(PyTypeObject *type); static int add_tp_new_wrapper(PyTypeObject *type); -#define COLLECTION_FLAGS (Py_TPFLAGS_SEQUENCE | Py_TPFLAGS_MAPPING) - static int type_ready_pre_checks(PyTypeObject *type) { @@ -8451,7 +8510,7 @@ type_ready_inherit_as_structs(PyTypeObject *type, PyTypeObject *base) static void inherit_patma_flags(PyTypeObject *type, PyTypeObject *base) { if ((type->tp_flags & COLLECTION_FLAGS) == 0) { - type_add_flags(type, base->tp_flags & COLLECTION_FLAGS); + type_add_flags(type, PyType_GetFlags(base) & COLLECTION_FLAGS); } } @@ -8718,6 +8777,13 @@ type_ready(PyTypeObject *type, int initial) } } +#ifdef Py_GIL_DISABLED + if (_PyType_HasFeature(type, Py_TPFLAGS_HEAPTYPE)) { + PyHeapTypeObject *ht = (PyHeapTypeObject*)type; + FT_ATOMIC_STORE_ULONG_RELAXED(ht->ht_flags, type->tp_flags); + } +#endif + /* All done -- set the ready flag */ type_add_flags(type, Py_TPFLAGS_READY); stop_readying(type); @@ -11159,7 +11225,7 @@ update_one_slot(PyTypeObject *type, pytype_slotdef *p) generic = p->function; if (p->function == slot_tp_call) { /* A generic __call__ is incompatible with vectorcall */ - type_clear_flags(type, Py_TPFLAGS_HAVE_VECTORCALL); + _PyType_SetFlags(type, Py_TPFLAGS_HAVE_VECTORCALL, 0); } } Py_DECREF(descr); @@ -11228,9 +11294,6 @@ update_slot(PyTypeObject *type, PyObject *name) static void fixup_slot_dispatchers(PyTypeObject *type) { - // This lock isn't strictly necessary because the type has not been - // exposed to anyone else yet, but update_ont_slot calls find_name_in_mro - // where we'd like to assert that the type is locked. BEGIN_TYPE_LOCK(); assert(!PyErr_Occurred()); diff --git a/Python/bytecodes.c b/Python/bytecodes.c index 24aa7bbb87c193..5ac51bb0cd9bd4 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2919,12 +2919,12 @@ dummy_func( } inst(MATCH_MAPPING, (subject -- subject, res)) { - int match = PyStackRef_TYPE(subject)->tp_flags & Py_TPFLAGS_MAPPING; + int match = _PyType_HasFeatureSafe(PyStackRef_TYPE(subject), Py_TPFLAGS_MAPPING); res = match ? PyStackRef_True : PyStackRef_False; } inst(MATCH_SEQUENCE, (subject -- subject, res)) { - int match = PyStackRef_TYPE(subject)->tp_flags & Py_TPFLAGS_SEQUENCE; + int match = _PyType_HasFeatureSafe(PyStackRef_TYPE(subject), Py_TPFLAGS_SEQUENCE); res = match ? PyStackRef_True : PyStackRef_False; } diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index 29160b9f6634c5..9696685c60b46b 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -3974,7 +3974,7 @@ _PyStackRef subject; _PyStackRef res; subject = stack_pointer[-1]; - int match = PyStackRef_TYPE(subject)->tp_flags & Py_TPFLAGS_MAPPING; + int match = _PyType_HasFeatureSafe(PyStackRef_TYPE(subject), Py_TPFLAGS_MAPPING); res = match ? PyStackRef_True : PyStackRef_False; stack_pointer[0] = res; stack_pointer += 1; @@ -3986,7 +3986,7 @@ _PyStackRef subject; _PyStackRef res; subject = stack_pointer[-1]; - int match = PyStackRef_TYPE(subject)->tp_flags & Py_TPFLAGS_SEQUENCE; + int match = _PyType_HasFeatureSafe(PyStackRef_TYPE(subject), Py_TPFLAGS_SEQUENCE); res = match ? PyStackRef_True : PyStackRef_False; stack_pointer[0] = res; stack_pointer += 1; diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 5216918560a487..b7edcc54afff8f 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -9739,7 +9739,7 @@ _PyStackRef subject; _PyStackRef res; subject = stack_pointer[-1]; - int match = PyStackRef_TYPE(subject)->tp_flags & Py_TPFLAGS_MAPPING; + int match = _PyType_HasFeatureSafe(PyStackRef_TYPE(subject), Py_TPFLAGS_MAPPING); res = match ? PyStackRef_True : PyStackRef_False; stack_pointer[0] = res; stack_pointer += 1; @@ -9758,7 +9758,7 @@ _PyStackRef subject; _PyStackRef res; subject = stack_pointer[-1]; - int match = PyStackRef_TYPE(subject)->tp_flags & Py_TPFLAGS_SEQUENCE; + int match = _PyType_HasFeatureSafe(PyStackRef_TYPE(subject), Py_TPFLAGS_SEQUENCE); res = match ? PyStackRef_True : PyStackRef_False; stack_pointer[0] = res; stack_pointer += 1; diff --git a/Tools/cases_generator/analyzer.py b/Tools/cases_generator/analyzer.py index cecfb6f3834d44..192c795e4082a7 100644 --- a/Tools/cases_generator/analyzer.py +++ b/Tools/cases_generator/analyzer.py @@ -650,6 +650,7 @@ def has_error_without_pop(op: parser.CodeDef) -> bool: "_PyTuple_FromStackRefStealOnSuccess", "_PyTuple_ITEMS", "_PyType_HasFeature", + "_PyType_HasFeatureSafe", "_PyType_NewManagedObject", "_PyUnicode_Equal", "_PyUnicode_JoinArray",