wip: thread safety for tp_flags

Use separate structure member in PyHeapTypeObject, `ht_flags`, to store
a copy of the type flags.  That allows safe toggling of some of the
flags after the type has been exposed.

Author: nascheme

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;

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));

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);

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);
 }

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]

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

Modules/_testcapi/vectorcall.c

@@ -3,6 +3,7 @@
 
 #include "parts.h"
 #include "clinic/vectorcall.c.h"
+#include "pycore_object.h" // _PyType_HasFeatureSafe()
 
 
 #include <stddef.h>                 // 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[] = {

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());