From f9a4eecba646d93dc8dfbdc53cdb4cff7e7a7651 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 12 Apr 2024 16:11:02 -0700 Subject: [PATCH] Quiet more TSAN warnings due to incorrect modeling of compare/exchange TSAN complains about data races between the non-atomic load of `tstate->state` in `detach_thread()` and `tstate_set_detached()` and the atomic compare/exchange of `tstate->tstate` in `park_detached_threads()`. I believe this is due to a bug with how TSAN models compare/exchange (treating it solely as a store, rather than a load/store). According to the standard, > If one evaluation modifies a memory location, and the other > reads or modifies the same memory location, and if at least one > of the evaluations is not an atomic operation, the behavior of > the program is undefined (the program has a data race) unless > there exists a happens-before relationship between these two > evaluations. However, according to the standard, A happens-before B if any of the following are true: > 1) A is sequenced-before B. > 2) A synchronizes-with B. > 3) A strongly happens-before X, and X strongly happens-before B. Using this we can establish a happens-before relationship between each non-atomic load and the compare/exchange: 1. Each non-atomic load is sequenced-before the seq-cst store to `tstate->state` that happens in `tstate_set_detached()`. Therefore, each non-atomic load happens-before the seq-cst store. 2. The seq-cst store to `tstate->state` in `tstate_set_detached()` synchronizes-with the compare/exchange to `tstate->state` in `park_detached_threads()`. Thus the seq-cst store happens-before the compare/exchange. 3. (1) and (2) establish the happens-before relationship between the non-atomic load and the compare/exchange on `tstate->state`, therefore it should not be flagged as a data race. --- Python/pystate.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Python/pystate.c b/Python/pystate.c index acec905484c21f..557450c6dfd09e 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -2003,7 +2003,7 @@ tstate_try_attach(PyThreadState *tstate) static void tstate_set_detached(PyThreadState *tstate, int detached_state) { - assert(tstate->state == _Py_THREAD_ATTACHED); + assert(_Py_atomic_load_int_relaxed(&tstate->state) == _Py_THREAD_ATTACHED); #ifdef Py_GIL_DISABLED _Py_atomic_store_int(&tstate->state, detached_state); #else @@ -2068,7 +2068,7 @@ static void detach_thread(PyThreadState *tstate, int detached_state) { // XXX assert(tstate_is_alive(tstate) && tstate_is_bound(tstate)); - assert(tstate->state == _Py_THREAD_ATTACHED); + assert(_Py_atomic_load_int_relaxed(&tstate->state) == _Py_THREAD_ATTACHED); assert(tstate == current_fast_get()); if (tstate->critical_section != 0) { _PyCriticalSection_SuspendAll(tstate);