https://github.com/python/cpython/commit/47fb4327b5c405da6df066dcaa01b7c1aef... commit: 47fb4327b5c405da6df066dcaa01b7c1aefab313 branch: main author: Sam Gross <colesbury@gmail.com> committer: colesbury <colesbury@gmail.com> date: 2024-06-03T20:58:41Z summary: gh-117657: Fix race involving immortalizing objects (#119927) The free-threaded build currently immortalizes objects that use deferred reference counting (see gh-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. files: M Include/internal/pycore_gc.h M Lib/test/support/__init__.py M Modules/_testinternalcapi.c M Objects/codeobject.c M Objects/object.c M Python/bltinmodule.c M Python/gc_free_threading.c M Python/pystate.c M Tools/tsan/suppressions_free_threading.txt diff --git a/Include/internal/pycore_gc.h b/Include/internal/pycore_gc.h index 60582521db5bd7..ba8b8e1903f307 100644 --- 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 index 5825efadffcb29..4b320b494bb8dd 100644 --- 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 index d9b9c999603d5a..6d4a00c06ca9de 100644 --- a/Modules/_testinternalcapi.c +++ b/Modules/_testinternalcapi.c @@ -1966,24 +1966,18 @@ 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 * @@ -1991,7 +1985,7 @@ 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 index 2ffda8dee07c90..e3e306bfe810c4 100644 --- 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 index d4fe14c5b3d1aa..5d53e9e5eaba4e 100644 --- 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 index 2a02d8161591c6..c4d3ecbeeff0e6 100644 --- 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 index e6bd012c40ee82..d005b79ff40dbf 100644 --- 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 index 36e4206b4a282e..d0293915db7689 100644 --- 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 index 39cc5b080c8703..d5fcac61f0db04 100644 --- a/Tools/tsan/suppressions_free_threading.txt +++ b/Tools/tsan/suppressions_free_threading.txt @@ -47,7 +47,6 @@ 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 @@ -55,7 +54,6 @@ 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