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.
This commit is contained in:
Sam Gross 2024-06-03 16:58:41 -04:00 committed by GitHub
parent b8fde5db86
commit 47fb4327b5
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
9 changed files with 30 additions and 44 deletions

View File

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

View File

@ -529,11 +529,11 @@ def suppress_immortalization(suppress=True):
yield yield
return return
old_values = _testinternalcapi.set_immortalize_deferred(False) _testinternalcapi.suppress_immortalization(True)
try: try:
yield yield
finally: finally:
_testinternalcapi.set_immortalize_deferred(*old_values) _testinternalcapi.suppress_immortalization(False)
def skip_if_suppress_immortalization(): def skip_if_suppress_immortalization():
try: try:

View File

@ -1966,24 +1966,18 @@ get_py_thread_id(PyObject *self, PyObject *Py_UNUSED(ignored))
#endif #endif
static PyObject * static PyObject *
set_immortalize_deferred(PyObject *self, PyObject *value) suppress_immortalization(PyObject *self, PyObject *value)
{ {
#ifdef Py_GIL_DISABLED #ifdef Py_GIL_DISABLED
PyInterpreterState *interp = PyInterpreterState_Get(); int suppress = PyObject_IsTrue(value);
int old_enabled = interp->gc.immortalize.enabled; if (suppress < 0) {
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))
{
return NULL; return NULL;
} }
interp->gc.immortalize.enable_on_thread_created = enabled_on_thread; PyInterpreterState *interp = PyInterpreterState_Get();
return Py_BuildValue("ii", old_enabled, old_enabled_on_thread); // Subtract two to suppress immortalization (so that 1 -> -1)
#else _Py_atomic_add_int(&interp->gc.immortalize, suppress ? -2 : 2);
return Py_BuildValue("OO", Py_False, Py_False);
#endif #endif
Py_RETURN_NONE;
} }
static PyObject * static PyObject *
@ -1991,7 +1985,7 @@ get_immortalize_deferred(PyObject *self, PyObject *Py_UNUSED(ignored))
{ {
#ifdef Py_GIL_DISABLED #ifdef Py_GIL_DISABLED
PyInterpreterState *interp = PyInterpreterState_Get(); 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 #else
Py_RETURN_FALSE; Py_RETURN_FALSE;
#endif #endif
@ -2111,7 +2105,7 @@ static PyMethodDef module_functions[] = {
#ifdef Py_GIL_DISABLED #ifdef Py_GIL_DISABLED
{"py_thread_id", get_py_thread_id, METH_NOARGS}, {"py_thread_id", get_py_thread_id, METH_NOARGS},
#endif #endif
{"set_immortalize_deferred", set_immortalize_deferred, METH_VARARGS}, {"suppress_immortalization", suppress_immortalization, METH_O},
{"get_immortalize_deferred", get_immortalize_deferred, METH_NOARGS}, {"get_immortalize_deferred", get_immortalize_deferred, METH_NOARGS},
#ifdef _Py_TIER2 #ifdef _Py_TIER2
{"uop_symbols_test", _Py_uop_symbols_test, METH_NOARGS}, {"uop_symbols_test", _Py_uop_symbols_test, METH_NOARGS},

View File

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

View File

@ -2429,7 +2429,7 @@ _PyObject_SetDeferredRefcount(PyObject *op)
assert(op->ob_ref_shared == 0); assert(op->ob_ref_shared == 0);
_PyObject_SET_GC_BITS(op, _PyGC_BITS_DEFERRED); _PyObject_SET_GC_BITS(op, _PyGC_BITS_DEFERRED);
PyInterpreterState *interp = _PyInterpreterState_GET(); 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 // gh-117696: immortalize objects instead of using deferred reference
// counting for now. // counting for now.
_Py_SetImmortal(op); _Py_SetImmortal(op);

View File

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

View File

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

View File

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

View File

@ -47,7 +47,6 @@ race_top:_PyImport_AcquireLock
race_top:_Py_dict_lookup_threadsafe race_top:_Py_dict_lookup_threadsafe
race_top:_imp_release_lock race_top:_imp_release_lock
race_top:_multiprocessing_SemLock_acquire_impl race_top:_multiprocessing_SemLock_acquire_impl
race_top:builtin_compile_impl
race_top:dictiter_new race_top:dictiter_new
race_top:dictresize race_top:dictresize
race_top:insert_to_emptydict race_top:insert_to_emptydict
@ -55,7 +54,6 @@ race_top:insertdict
race_top:list_get_item_ref race_top:list_get_item_ref
race_top:make_pending_calls race_top:make_pending_calls
race_top:set_add_entry race_top:set_add_entry
race_top:should_intern_string
race_top:_Py_slot_tp_getattr_hook race_top:_Py_slot_tp_getattr_hook
race_top:add_threadstate race_top:add_threadstate
race_top:dump_traceback race_top:dump_traceback