From 0cb1d0dccf91dd59e5e084f6c6c7145c4416854f Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 11 May 2023 15:38:13 -0600 Subject: [PATCH 1/2] Add _PyEval_HoldsLock(). --- Include/internal/pycore_ceval.h | 1 + Python/ceval_gil.c | 7 +++++++ 2 files changed, 8 insertions(+) diff --git a/Include/internal/pycore_ceval.h b/Include/internal/pycore_ceval.h index 3c8b368bd2af4e..95133176008ec2 100644 --- a/Include/internal/pycore_ceval.h +++ b/Include/internal/pycore_ceval.h @@ -101,6 +101,7 @@ extern void _PyEval_FiniGIL(PyInterpreterState *interp); extern void _PyEval_AcquireLock(PyThreadState *tstate); extern void _PyEval_ReleaseLock(PyThreadState *tstate); +extern int _PyEval_HoldsLock(PyThreadState *tstate); extern PyThreadState * _PyThreadState_SwapNoGIL(PyThreadState *); extern void _PyEval_DeactivateOpCache(void); diff --git a/Python/ceval_gil.c b/Python/ceval_gil.c index 42e1436bc9130d..4857b0618d8966 100644 --- a/Python/ceval_gil.c +++ b/Python/ceval_gil.c @@ -629,6 +629,13 @@ _PyEval_ReleaseLock(PyThreadState *tstate) drop_gil(ceval, tstate); } +int +_PyEval_HoldsLock(PyThreadState *tstate) +{ + _Py_EnsureTstateNotNULL(tstate); + return current_thread_holds_gil(tstate->interp->ceval.gil, tstate); +} + void PyEval_AcquireThread(PyThreadState *tstate) { From 31b0d5cf11ae7e5a341d14fd0e077a7e2095d2da Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 11 May 2023 17:26:47 -0600 Subject: [PATCH 2/2] Ensure the current tstate is set whenever the GIL is held. --- Modules/_threadmodule.c | 1 + Python/ceval_gil.c | 34 +++++++++++++++++++++------------- Python/pylifecycle.c | 10 ++++++++-- Python/pystate.c | 4 +++- 4 files changed, 33 insertions(+), 16 deletions(-) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 5d753b4a0ebc5e..93e23b1b45e857 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -3,6 +3,7 @@ /* Interface to Sjoerd's portable C thread library */ #include "Python.h" +#include "pycore_ceval.h" // _PyEval_HoldsLock() #include "pycore_interp.h" // _PyInterpreterState.threads.count #include "pycore_moduleobject.h" // _PyModule_GetState() #include "pycore_pylifecycle.h" diff --git a/Python/ceval_gil.c b/Python/ceval_gil.c index 4857b0618d8966..08d3ce2b84165e 100644 --- a/Python/ceval_gil.c +++ b/Python/ceval_gil.c @@ -282,7 +282,10 @@ drop_gil(struct _ceval_state *ceval, PyThreadState *tstate) if (!_Py_atomic_load_relaxed(&gil->locked)) { Py_FatalError("drop_gil: GIL is not locked"); } + assert(_Py_tss_tstate == tstate); + assert(((PyThreadState *)_Py_atomic_load_relaxed(&gil->last_holder)) == tstate); + // XXX Does this still hold? /* tstate is allowed to be NULL (early interpreter init) */ if (tstate != NULL) { /* Sub-interpreter support: threads might have been switched @@ -348,6 +351,7 @@ take_gil(PyThreadState *tstate) assert(tstate != NULL); + assert(_Py_tss_tstate == tstate); if (tstate_must_exit(tstate)) { /* bpo-39877: If Py_Finalize() has been called and tstate is not the thread which called Py_Finalize(), exit immediately the thread. @@ -641,11 +645,11 @@ PyEval_AcquireThread(PyThreadState *tstate) { _Py_EnsureTstateNotNULL(tstate); - take_gil(tstate); - if (_PyThreadState_SwapNoGIL(tstate) != NULL) { Py_FatalError("non-NULL old thread state"); } + + take_gil(tstate); } void @@ -653,12 +657,13 @@ PyEval_ReleaseThread(PyThreadState *tstate) { assert(is_tstate_valid(tstate)); + struct _ceval_state *ceval = &tstate->interp->ceval; + drop_gil(ceval, tstate); + PyThreadState *new_tstate = _PyThreadState_SwapNoGIL(NULL); if (new_tstate != tstate) { Py_FatalError("wrong thread state"); } - struct _ceval_state *ceval = &tstate->interp->ceval; - drop_gil(ceval, tstate); } #ifdef HAVE_FORK @@ -701,12 +706,15 @@ _PyEval_SignalAsyncExc(PyInterpreterState *interp) PyThreadState * PyEval_SaveThread(void) { + PyThreadState *current_tstate = _PyThreadState_GET(); + _Py_EnsureTstateNotNULL(current_tstate); + struct _ceval_state *ceval = ¤t_tstate->interp->ceval; + assert(gil_created(ceval->gil)); + drop_gil(ceval, current_tstate); + PyThreadState *tstate = _PyThreadState_SwapNoGIL(NULL); - _Py_EnsureTstateNotNULL(tstate); + assert(tstate == current_tstate); - struct _ceval_state *ceval = &tstate->interp->ceval; - assert(gil_created(ceval->gil)); - drop_gil(ceval, tstate); return tstate; } @@ -715,9 +723,9 @@ PyEval_RestoreThread(PyThreadState *tstate) { _Py_EnsureTstateNotNULL(tstate); - take_gil(tstate); - _PyThreadState_SwapNoGIL(tstate); + + take_gil(tstate); } @@ -1013,18 +1021,18 @@ _Py_HandlePending(PyThreadState *tstate) /* GIL drop request */ if (_Py_atomic_load_relaxed_int32(&interp_ceval_state->gil_drop_request)) { /* Give another thread a chance */ + assert(tstate == _PyThreadState_GET()); + drop_gil(interp_ceval_state, tstate); if (_PyThreadState_SwapNoGIL(NULL) != tstate) { Py_FatalError("tstate mix-up"); } - drop_gil(interp_ceval_state, tstate); /* Other threads may run now */ - take_gil(tstate); - if (_PyThreadState_SwapNoGIL(tstate) != NULL) { Py_FatalError("orphan tstate"); } + take_gil(tstate); } /* Check for asynchronous exception. */ diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 61f87c5eba60ed..a483f874977c76 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -2027,8 +2027,16 @@ new_interpreter(PyThreadState **tstate_p, const PyInterpreterConfig *config) } _PyThreadState_Bind(tstate); + // We must release the GIL before swapping. + PyThreadState *current_tstate = _PyThreadState_GET(); + if (current_tstate != NULL) { + // XXX Might new_interpreter() have been called without the GIL held? + _PyEval_ReleaseLock(current_tstate); + } + // XXX For now we do this before the GIL is created. PyThreadState *save_tstate = _PyThreadState_SwapNoGIL(tstate); + assert(save_tstate == current_tstate); int has_gil = 0; /* From this point until the init_interp_create_gil() call, @@ -2039,8 +2047,6 @@ new_interpreter(PyThreadState **tstate_p, const PyInterpreterConfig *config) /* Copy the current interpreter config into the new interpreter */ const PyConfig *src_config; if (save_tstate != NULL) { - // XXX Might new_interpreter() have been called without the GIL held? - _PyEval_ReleaseLock(save_tstate); src_config = _PyInterpreterState_GetConfig(save_tstate->interp); } else diff --git a/Python/pystate.c b/Python/pystate.c index 26debf1f88b94a..750f48475c15ad 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -1564,8 +1564,10 @@ _PyThreadState_DeleteCurrent(PyThreadState *tstate) { _Py_EnsureTstateNotNULL(tstate); tstate_delete_common(tstate); - current_fast_clear(tstate->interp->runtime); + assert(_Py_tss_tstate == tstate); + assert(_PyEval_HoldsLock(tstate)); _PyEval_ReleaseLock(tstate); + current_fast_clear(tstate->interp->runtime); free_threadstate(tstate); }