https://github.com/python/cpython/commit/47fb4327b5c405da6df066dcaa01b7c1aefab313
commit: 47fb4327b5c405da6df066dcaa01b7c1aefab313
branch: main
author: Sam Gross <[email protected]>
committer: colesbury <[email protected]>
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
_______________________________________________
Python-checkins mailing list -- [email protected]
To unsubscribe send an email to [email protected]
https://mail.python.org/mailman3/lists/python-checkins.python.org/
Member address: [email protected]
