pythongh-117657: Fix race involving immortalizing objects (python#119927

) The free-threaded build currently immortalizes objects that use deferred reference counting (see pythongh-117783). This typically happens once the first non-main thread is created, but the behavior can be suppressed for tests, in subinterpreters, or during a compile() call. This fixes a race condition involving the tracking of whether the behavior is suppressed.
hudson-trading · Jul 11, 2024 · 65cc5e0 · 65cc5e0
1 parent f38d493
commit 65cc5e0
Show file tree

Hide file tree

Showing 9 changed files with 30 additions and 44 deletions.
diff --git a/Include/internal/pycore_gc.h b/Include/internal/pycore_gc.h
@@ -347,15 +347,11 @@ struct _gc_runtime_state {
 
     /* gh-117783: Deferred reference counting is not fully implemented yet, so
        as a temporary measure we treat objects using deferred referenence
-       counting as immortal. */
-    struct {
-        /* Immortalize objects instead of marking them as using deferred
-           reference counting. */
-        int enabled;
-
-        /* Set enabled=1 when the first background thread is created. */
-        int enable_on_thread_created;
-    } immortalize;
+       counting as immortal. The value may be zero, one, or a negative number:
+        0: immortalize deferred RC objects once the first thread is created
+        1: immortalize all deferred RC objects immediately
+        <0: suppressed; don't immortalize objects */
+    int immortalize;
 #endif
 };
 

diff --git a/Lib/test/support/__init__.py b/Lib/test/support/__init__.py
@@ -529,11 +529,11 @@ def suppress_immortalization(suppress=True):
         yield
         return
 
-    old_values = _testinternalcapi.set_immortalize_deferred(False)
+    _testinternalcapi.suppress_immortalization(True)
     try:
         yield
     finally:
-        _testinternalcapi.set_immortalize_deferred(*old_values)
+        _testinternalcapi.suppress_immortalization(False)
 
 def skip_if_suppress_immortalization():
     try:

diff --git a/Modules/_testinternalcapi.c b/Modules/_testinternalcapi.c
@@ -1966,32 +1966,26 @@ get_py_thread_id(PyObject *self, PyObject *Py_UNUSED(ignored))
 #endif
 
 static PyObject *
-set_immortalize_deferred(PyObject *self, PyObject *value)
+suppress_immortalization(PyObject *self, PyObject *value)
 {
 #ifdef Py_GIL_DISABLED
-    PyInterpreterState *interp = PyInterpreterState_Get();
-    int old_enabled = interp->gc.immortalize.enabled;
-    int old_enabled_on_thread = interp->gc.immortalize.enable_on_thread_created;
-    int enabled_on_thread = 0;
-    if (!PyArg_ParseTuple(value, "i|i",
-                          &interp->gc.immortalize.enabled,
-                          &enabled_on_thread))
-    {
+    int suppress = PyObject_IsTrue(value);
+    if (suppress < 0) {
         return NULL;
     }
-    interp->gc.immortalize.enable_on_thread_created = enabled_on_thread;
-    return Py_BuildValue("ii", old_enabled, old_enabled_on_thread);
-#else
-    return Py_BuildValue("OO", Py_False, Py_False);
+    PyInterpreterState *interp = PyInterpreterState_Get();
+    // Subtract two to suppress immortalization (so that 1 -> -1)
+    _Py_atomic_add_int(&interp->gc.immortalize, suppress ? -2 : 2);
 #endif
+    Py_RETURN_NONE;
 }
 
 static PyObject *
 get_immortalize_deferred(PyObject *self, PyObject *Py_UNUSED(ignored))
 {
 #ifdef Py_GIL_DISABLED
     PyInterpreterState *interp = PyInterpreterState_Get();
-    return PyBool_FromLong(interp->gc.immortalize.enable_on_thread_created);
+    return PyBool_FromLong(_Py_atomic_load_int(&interp->gc.immortalize) >= 0);
 #else
     Py_RETURN_FALSE;
 #endif
@@ -2111,7 +2105,7 @@ static PyMethodDef module_functions[] = {
 #ifdef Py_GIL_DISABLED
     {"py_thread_id", get_py_thread_id, METH_NOARGS},
 #endif
-    {"set_immortalize_deferred", set_immortalize_deferred, METH_VARARGS},
+    {"suppress_immortalization", suppress_immortalization, METH_O},
     {"get_immortalize_deferred", get_immortalize_deferred, METH_NOARGS},
 #ifdef _Py_TIER2
     {"uop_symbols_test", _Py_uop_symbols_test, METH_NOARGS},

diff --git a/Objects/codeobject.c b/Objects/codeobject.c
@@ -110,7 +110,7 @@ should_intern_string(PyObject *o)
     // unless we've disabled immortalizing objects that use deferred reference
     // counting.
     PyInterpreterState *interp = _PyInterpreterState_GET();
-    if (interp->gc.immortalize.enable_on_thread_created) {
+    if (_Py_atomic_load_int(&interp->gc.immortalize) < 0) {
         return 1;
     }
 #endif
@@ -240,7 +240,7 @@ intern_constants(PyObject *tuple, int *modified)
         PyThreadState *tstate = PyThreadState_GET();
         if (!_Py_IsImmortal(v) && !PyCode_Check(v) &&
             !PyUnicode_CheckExact(v) &&
-            tstate->interp->gc.immortalize.enable_on_thread_created)
+            _Py_atomic_load_int(&tstate->interp->gc.immortalize) >= 0)
         {
             PyObject *interned = intern_one_constant(v);
             if (interned == NULL) {

diff --git a/Objects/object.c b/Objects/object.c
@@ -2429,7 +2429,7 @@ _PyObject_SetDeferredRefcount(PyObject *op)
     assert(op->ob_ref_shared == 0);
     _PyObject_SET_GC_BITS(op, _PyGC_BITS_DEFERRED);
     PyInterpreterState *interp = _PyInterpreterState_GET();
-    if (interp->gc.immortalize.enabled) {
+    if (_Py_atomic_load_int_relaxed(&interp->gc.immortalize) == 1) {
         // gh-117696: immortalize objects instead of using deferred reference
         // counting for now.
         _Py_SetImmortal(op);

diff --git a/Python/bltinmodule.c b/Python/bltinmodule.c
@@ -870,15 +870,15 @@ builtin_compile_impl(PyObject *module, PyObject *source, PyObject *filename,
     // gh-118527: Disable immortalization of code constants for explicit
     // compile() calls to get consistent frozen outputs between the default
     // and free-threaded builds.
+    // Subtract two to suppress immortalization (so that 1 -> -1)
     PyInterpreterState *interp = _PyInterpreterState_GET();
-    int old_value = interp->gc.immortalize.enable_on_thread_created;
-    interp->gc.immortalize.enable_on_thread_created = 0;
+    _Py_atomic_add_int(&interp->gc.immortalize, -2);
 #endif
 
     result = Py_CompileStringObject(str, filename, start[compile_mode], &cf, optimize);
 
 #ifdef Py_GIL_DISABLED
-    interp->gc.immortalize.enable_on_thread_created = old_value;
+    _Py_atomic_add_int(&interp->gc.immortalize, 2);
 #endif
 
     Py_XDECREF(source_copy);

diff --git a/Python/gc_free_threading.c b/Python/gc_free_threading.c
@@ -703,11 +703,9 @@ _PyGC_Init(PyInterpreterState *interp)
 {
     GCState *gcstate = &interp->gc;
 
-    if (_Py_IsMainInterpreter(interp)) {
-        // gh-117783: immortalize objects that would use deferred refcounting
-        // once the first non-main thread is created.
-        gcstate->immortalize.enable_on_thread_created = 1;
-    }
+    // gh-117783: immortalize objects that would use deferred refcounting
+    // once the first non-main thread is created (but not in subinterpreters).
+    gcstate->immortalize = _Py_IsMainInterpreter(interp) ? 0 : -1;
 
     gcstate->garbage = PyList_New(0);
     if (gcstate->garbage == NULL) {
@@ -1808,8 +1806,10 @@ _PyGC_ImmortalizeDeferredObjects(PyInterpreterState *interp)
 {
     struct visitor_args args;
     _PyEval_StopTheWorld(interp);
-    gc_visit_heaps(interp, &immortalize_visitor, &args);
-    interp->gc.immortalize.enabled = 1;
+    if (interp->gc.immortalize == 0) {
+        gc_visit_heaps(interp, &immortalize_visitor, &args);
+        interp->gc.immortalize = 1;
+    }
     _PyEval_StartTheWorld(interp);
 }
 

diff --git a/Python/pystate.c b/Python/pystate.c
@@ -1583,9 +1583,7 @@ new_threadstate(PyInterpreterState *interp, int whence)
     }
     else {
 #ifdef Py_GIL_DISABLED
-        if (interp->gc.immortalize.enable_on_thread_created &&
-            !interp->gc.immortalize.enabled)
-        {
+        if (_Py_atomic_load_int(&interp->gc.immortalize) == 0) {
             // Immortalize objects marked as using deferred reference counting
             // the first time a non-main thread is created.
             _PyGC_ImmortalizeDeferredObjects(interp);

diff --git a/Tools/tsan/suppressions_free_threading.txt b/Tools/tsan/suppressions_free_threading.txt
@@ -47,15 +47,13 @@ race_top:_PyImport_AcquireLock
 race_top:_Py_dict_lookup_threadsafe
 race_top:_imp_release_lock
 race_top:_multiprocessing_SemLock_acquire_impl
-race_top:builtin_compile_impl
 race_top:dictiter_new
 race_top:dictresize
 race_top:insert_to_emptydict
 race_top:insertdict
 race_top:list_get_item_ref
 race_top:make_pending_calls
 race_top:set_add_entry
-race_top:should_intern_string
 race_top:_Py_slot_tp_getattr_hook
 race_top:add_threadstate
 race_top:dump_traceback