From 4652f0dbec49d56f17d36365743e0a988a960113 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Mon, 14 Apr 2025 06:53:07 -0400 Subject: [PATCH 01/24] Convert the gilstate into a thread-local. --- Include/internal/pycore_runtime_init.h | 3 - Include/internal/pycore_runtime_structs.h | 6 - Python/pystate.c | 182 ++++------------------ 3 files changed, 31 insertions(+), 160 deletions(-) diff --git a/Include/internal/pycore_runtime_init.h b/Include/internal/pycore_runtime_init.h index 2b2e439681f459..e40698dba732fd 100644 --- a/Include/internal/pycore_runtime_init.h +++ b/Include/internal/pycore_runtime_init.h @@ -61,9 +61,6 @@ extern PyTypeObject _PyExc_MemoryError; }, \ }, \ }, \ - /* A TSS key must be initialized with Py_tss_NEEDS_INIT \ - in accordance with the specification. */ \ - .autoTSSkey = Py_tss_NEEDS_INIT, \ .parser = _parser_runtime_state_INIT, \ .ceval = { \ .pending_mainthread = { \ diff --git a/Include/internal/pycore_runtime_structs.h b/Include/internal/pycore_runtime_structs.h index 6bf3aae7175a97..d5e5531350af74 100644 --- a/Include/internal/pycore_runtime_structs.h +++ b/Include/internal/pycore_runtime_structs.h @@ -223,12 +223,6 @@ struct pyruntimestate { struct _pythread_runtime_state threads; struct _signals_runtime_state signals; - /* Used for the thread state bound to the current thread. */ - Py_tss_t autoTSSkey; - - /* Used instead of PyThreadState.trash when there is not current tstate. */ - Py_tss_t trashTSSkey; - PyWideStringList orig_argv; struct _parser_runtime_state parser; diff --git a/Python/pystate.c b/Python/pystate.c index ee35f0fa945f8b..525e27b4fa0d24 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -69,7 +69,10 @@ to avoid the expense of doing their own locking). #ifdef HAVE_THREAD_LOCAL +/* The attached thread state for the current thread. */ _Py_thread_local PyThreadState *_Py_tss_tstate = NULL; +/* The bound gilstate for the current thread. */ +_Py_thread_local PyThreadState *_Py_tss_gilstate = NULL; #endif static inline PyThreadState * @@ -118,78 +121,9 @@ _PyThreadState_GetCurrent(void) } -//------------------------------------------------ -// the thread state bound to the current OS thread -//------------------------------------------------ - -static inline int -tstate_tss_initialized(Py_tss_t *key) -{ - return PyThread_tss_is_created(key); -} - -static inline int -tstate_tss_init(Py_tss_t *key) -{ - assert(!tstate_tss_initialized(key)); - return PyThread_tss_create(key); -} - -static inline void -tstate_tss_fini(Py_tss_t *key) -{ - assert(tstate_tss_initialized(key)); - PyThread_tss_delete(key); -} - -static inline PyThreadState * -tstate_tss_get(Py_tss_t *key) -{ - assert(tstate_tss_initialized(key)); - return (PyThreadState *)PyThread_tss_get(key); -} - -static inline int -tstate_tss_set(Py_tss_t *key, PyThreadState *tstate) -{ - assert(tstate != NULL); - assert(tstate_tss_initialized(key)); - return PyThread_tss_set(key, (void *)tstate); -} - -static inline int -tstate_tss_clear(Py_tss_t *key) -{ - assert(tstate_tss_initialized(key)); - return PyThread_tss_set(key, (void *)NULL); -} - -#ifdef HAVE_FORK -/* Reset the TSS key - called by PyOS_AfterFork_Child(). - * This should not be necessary, but some - buggy - pthread implementations - * don't reset TSS upon fork(), see issue #10517. - */ -static PyStatus -tstate_tss_reinit(Py_tss_t *key) -{ - if (!tstate_tss_initialized(key)) { - return _PyStatus_OK(); - } - PyThreadState *tstate = tstate_tss_get(key); - - tstate_tss_fini(key); - if (tstate_tss_init(key) != 0) { - return _PyStatus_NO_MEMORY(); - } - - /* If the thread had an associated auto thread state, reassociate it with - * the new key. */ - if (tstate && tstate_tss_set(key, tstate) != 0) { - return _PyStatus_ERR("failed to re-set autoTSSkey"); - } - return _PyStatus_OK(); -} -#endif +//--------------------------------------------- +// the GIL state bound to the current OS thread +//--------------------------------------------- /* @@ -198,36 +132,23 @@ tstate_tss_reinit(Py_tss_t *key) The GIL does no need to be held for these. */ -#define gilstate_tss_initialized(runtime) \ - tstate_tss_initialized(&(runtime)->autoTSSkey) -#define gilstate_tss_init(runtime) \ - tstate_tss_init(&(runtime)->autoTSSkey) -#define gilstate_tss_fini(runtime) \ - tstate_tss_fini(&(runtime)->autoTSSkey) -#define gilstate_tss_get(runtime) \ - tstate_tss_get(&(runtime)->autoTSSkey) -#define _gilstate_tss_set(runtime, tstate) \ - tstate_tss_set(&(runtime)->autoTSSkey, tstate) -#define _gilstate_tss_clear(runtime) \ - tstate_tss_clear(&(runtime)->autoTSSkey) -#define gilstate_tss_reinit(runtime) \ - tstate_tss_reinit(&(runtime)->autoTSSkey) +static inline PyThreadState * +gilstate_get(void) +{ + return _Py_tss_gilstate; +} static inline void -gilstate_tss_set(_PyRuntimeState *runtime, PyThreadState *tstate) +gilstate_set(PyThreadState *tstate) { - assert(tstate != NULL && tstate->interp->runtime == runtime); - if (_gilstate_tss_set(runtime, tstate) != 0) { - Py_FatalError("failed to set current tstate (TSS)"); - } + assert(tstate != NULL); + _Py_tss_gilstate = tstate; } static inline void -gilstate_tss_clear(_PyRuntimeState *runtime) +gilstate_clear(void) { - if (_gilstate_tss_clear(runtime) != 0) { - Py_FatalError("failed to clear current tstate (TSS)"); - } + _Py_tss_gilstate = NULL; } @@ -253,7 +174,7 @@ bind_tstate(PyThreadState *tstate) assert(tstate_is_alive(tstate) && !tstate->_status.bound); assert(!tstate->_status.unbound); // just in case assert(!tstate->_status.bound_gilstate); - assert(tstate != gilstate_tss_get(tstate->interp->runtime)); + assert(tstate != gilstate_get()); assert(!tstate->_status.active); assert(tstate->thread_id == 0); assert(tstate->native_thread_id == 0); @@ -328,14 +249,13 @@ bind_gilstate_tstate(PyThreadState *tstate) // XXX assert(!tstate->_status.active); assert(!tstate->_status.bound_gilstate); - _PyRuntimeState *runtime = tstate->interp->runtime; - PyThreadState *tcur = gilstate_tss_get(runtime); + PyThreadState *tcur = gilstate_get(); assert(tstate != tcur); if (tcur != NULL) { tcur->_status.bound_gilstate = 0; } - gilstate_tss_set(runtime, tstate); + gilstate_set(tstate); tstate->_status.bound_gilstate = 1; } @@ -347,9 +267,9 @@ unbind_gilstate_tstate(PyThreadState *tstate) assert(tstate_is_bound(tstate)); // XXX assert(!tstate->_status.active); assert(tstate->_status.bound_gilstate); - assert(tstate == gilstate_tss_get(tstate->interp->runtime)); + assert(tstate == gilstate_get()); - gilstate_tss_clear(tstate->interp->runtime); + gilstate_clear(); tstate->_status.bound_gilstate = 0; } @@ -373,7 +293,7 @@ holds_gil(PyThreadState *tstate) // (and tstate->interp->runtime->ceval.gil.locked). assert(tstate != NULL); /* Must be the tstate for this thread */ - assert(tstate == gilstate_tss_get(tstate->interp->runtime)); + assert(tstate == gilstate_get()); return tstate == current_fast_get(); } @@ -469,16 +389,6 @@ _PyRuntimeState_Init(_PyRuntimeState *runtime) return status; } - if (gilstate_tss_init(runtime) != 0) { - _PyRuntimeState_Fini(runtime); - return _PyStatus_NO_MEMORY(); - } - - if (PyThread_tss_create(&runtime->trashTSSkey) != 0) { - _PyRuntimeState_Fini(runtime); - return _PyStatus_NO_MEMORY(); - } - init_runtime(runtime, open_code_hook, open_code_userdata, audit_hook_head, unicode_next_index); @@ -492,14 +402,6 @@ _PyRuntimeState_Fini(_PyRuntimeState *runtime) /* The count is cleared by _Py_FinalizeRefTotal(). */ assert(runtime->object_state.interpreter_leaks == 0); #endif - - if (gilstate_tss_initialized(runtime)) { - gilstate_tss_fini(runtime); - } - - if (PyThread_tss_is_created(&runtime->trashTSSkey)) { - PyThread_tss_delete(&runtime->trashTSSkey); - } } #ifdef HAVE_FORK @@ -532,18 +434,6 @@ _PyRuntimeState_ReInitThreads(_PyRuntimeState *runtime) _PyTypes_AfterFork(); - PyStatus status = gilstate_tss_reinit(runtime); - if (_PyStatus_EXCEPTION(status)) { - return status; - } - - if (PyThread_tss_is_created(&runtime->trashTSSkey)) { - PyThread_tss_delete(&runtime->trashTSSkey); - } - if (PyThread_tss_create(&runtime->trashTSSkey) != 0) { - return _PyStatus_NO_MEMORY(); - } - _PyThread_AfterFork(&runtime->threads); return _PyStatus_OK(); @@ -1629,7 +1519,7 @@ _PyThreadState_NewBound(PyInterpreterState *interp, int whence) bind_tstate(tstate); // This makes sure there's a gilstate tstate bound // as soon as possible. - if (gilstate_tss_get(tstate->interp->runtime) == NULL) { + if (gilstate_get() == NULL) { bind_gilstate_tstate(tstate); } } @@ -2050,7 +1940,7 @@ tstate_activate(PyThreadState *tstate) assert(!tstate->_status.active); assert(!tstate->_status.bound_gilstate || - tstate == gilstate_tss_get((tstate->interp->runtime))); + tstate == gilstate_get()); if (!tstate->_status.bound_gilstate) { bind_gilstate_tstate(tstate); } @@ -2515,7 +2405,7 @@ _PyThreadState_Bind(PyThreadState *tstate) bind_tstate(tstate); // This makes sure there's a gilstate tstate bound // as soon as possible. - if (gilstate_tss_get(tstate->interp->runtime) == NULL) { + if (gilstate_get() == NULL) { bind_gilstate_tstate(tstate); } } @@ -2717,7 +2607,7 @@ _PyGILState_Init(PyInterpreterState *interp) return _PyStatus_OK(); } _PyRuntimeState *runtime = interp->runtime; - assert(gilstate_tss_get(runtime) == NULL); + assert(gilstate_get() == NULL); assert(runtime->gilstate.autoInterpreterState == NULL); runtime->gilstate.autoInterpreterState = interp; return _PyStatus_OK(); @@ -2753,7 +2643,7 @@ _PyGILState_SetTstate(PyThreadState *tstate) _PyRuntimeState *runtime = tstate->interp->runtime; assert(runtime->gilstate.autoInterpreterState == tstate->interp); - assert(gilstate_tss_get(runtime) == tstate); + assert(gilstate_get() == tstate); assert(tstate->gilstate_counter == 1); #endif } @@ -2769,11 +2659,7 @@ _PyGILState_GetInterpreterStateUnsafe(void) PyThreadState * PyGILState_GetThisThreadState(void) { - _PyRuntimeState *runtime = &_PyRuntime; - if (!gilstate_tss_initialized(runtime)) { - return NULL; - } - return gilstate_tss_get(runtime); + return gilstate_get(); } int @@ -2784,16 +2670,12 @@ PyGILState_Check(void) return 1; } - if (!gilstate_tss_initialized(runtime)) { - return 1; - } - PyThreadState *tstate = current_fast_get(); if (tstate == NULL) { return 0; } - PyThreadState *tcur = gilstate_tss_get(runtime); + PyThreadState *tcur = gilstate_get(); return (tstate == tcur); } @@ -2810,10 +2692,9 @@ PyGILState_Ensure(void) /* Ensure that _PyEval_InitThreads() and _PyGILState_Init() have been called by Py_Initialize() */ assert(_PyEval_ThreadsInitialized()); - assert(gilstate_tss_initialized(runtime)); assert(runtime->gilstate.autoInterpreterState != NULL); - PyThreadState *tcur = gilstate_tss_get(runtime); + PyThreadState *tcur = gilstate_get(); int has_gil; if (tcur == NULL) { /* Create a new Python thread state for this thread */ @@ -2853,8 +2734,7 @@ PyGILState_Ensure(void) void PyGILState_Release(PyGILState_STATE oldstate) { - _PyRuntimeState *runtime = &_PyRuntime; - PyThreadState *tstate = gilstate_tss_get(runtime); + PyThreadState *tstate = gilstate_get(); if (tstate == NULL) { Py_FatalError("auto-releasing thread-state, " "but no thread-state for this thread"); From 0e93b0351995e9fd63e04d26f722e53252f32715 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Mon, 14 Apr 2025 06:54:44 -0400 Subject: [PATCH 02/24] Add a comment. --- Python/pystate.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Python/pystate.c b/Python/pystate.c index 525e27b4fa0d24..0c21a12079cff1 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -71,7 +71,11 @@ to avoid the expense of doing their own locking). #ifdef HAVE_THREAD_LOCAL /* The attached thread state for the current thread. */ _Py_thread_local PyThreadState *_Py_tss_tstate = NULL; -/* The bound gilstate for the current thread. */ + +/* The bound gilstate for the current thread. + Basically, this is used for caching the right interpreter + for subsequent PyGILState_Ensure() calls. Despite the name, + this doesn't have much to do with the actual GIL. */ _Py_thread_local PyThreadState *_Py_tss_gilstate = NULL; #endif From ba4c00ee48be4ff33e5082e386fbf4e53b60792f Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Mon, 14 Apr 2025 06:59:22 -0400 Subject: [PATCH 03/24] Add a test case. --- Programs/_testembed.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/Programs/_testembed.c b/Programs/_testembed.c index 6f6d0cae58010e..99cf7919cfa63f 100644 --- a/Programs/_testembed.c +++ b/Programs/_testembed.c @@ -1,5 +1,6 @@ #ifndef Py_BUILD_CORE_MODULE # define Py_BUILD_CORE_MODULE +#include "pystate.h" #endif /* Always enable assertion (even in release mode) */ @@ -2341,6 +2342,27 @@ test_get_incomplete_frame(void) return result; } +static void do_gilstate_ensure(void *unused) +{ + for (int i = 0; i < 50; ++i) { + PyGILState_STATE gstate = PyGILState_Ensure(); + assert(PyGILState_Check()); // Yuck + PyGILState_Release(gstate); + } +} + +static int +test_gilstate_after_finalization(void) +{ + PyThread_handle_t handle; + PyThread_ident_t ident; + _testembed_Py_Initialize(); + if (PyThread_start_joinable_thread(&do_gilstate_ensure, NULL, &ident, &handle) < 0) { + return -1; + } + Py_Finalize(); + return PyThread_join_thread(handle); +} /* ********************************************************* * List of test cases and the function that implements it. @@ -2431,7 +2453,7 @@ static struct TestCase TestCases[] = { {"test_frozenmain", test_frozenmain}, #endif {"test_get_incomplete_frame", test_get_incomplete_frame}, - + {"test_gilstate_after_finalization", test_gilstate_after_finalization}, {NULL, NULL} }; From 2e990928a03107473ddb8224290d7ee647dc868a Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Mon, 14 Apr 2025 07:05:55 -0400 Subject: [PATCH 04/24] Fix the test. --- Programs/_testembed.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Programs/_testembed.c b/Programs/_testembed.c index 99cf7919cfa63f..f3bd0489767596 100644 --- a/Programs/_testembed.c +++ b/Programs/_testembed.c @@ -1,6 +1,5 @@ #ifndef Py_BUILD_CORE_MODULE # define Py_BUILD_CORE_MODULE -#include "pystate.h" #endif /* Always enable assertion (even in release mode) */ @@ -2342,7 +2341,8 @@ test_get_incomplete_frame(void) return result; } -static void do_gilstate_ensure(void *unused) +static void +do_gilstate_ensure(void *unused) { for (int i = 0; i < 50; ++i) { PyGILState_STATE gstate = PyGILState_Ensure(); @@ -2354,9 +2354,9 @@ static void do_gilstate_ensure(void *unused) static int test_gilstate_after_finalization(void) { + _testembed_Py_Initialize(); PyThread_handle_t handle; PyThread_ident_t ident; - _testembed_Py_Initialize(); if (PyThread_start_joinable_thread(&do_gilstate_ensure, NULL, &ident, &handle) < 0) { return -1; } From 6425c3e31156f333b27b3cff0fcf37710323454f Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Mon, 14 Apr 2025 07:27:30 -0400 Subject: [PATCH 05/24] Fix the test case (again). --- Programs/_testembed.c | 25 ++++++++++++++++--------- Python/pystate.c | 11 ++++++----- 2 files changed, 22 insertions(+), 14 deletions(-) diff --git a/Programs/_testembed.c b/Programs/_testembed.c index f3bd0489767596..c1ac330c51ec32 100644 --- a/Programs/_testembed.c +++ b/Programs/_testembed.c @@ -8,6 +8,7 @@ #include #include "pycore_initconfig.h" // _PyConfig_InitCompatConfig() #include "pycore_runtime.h" // _PyRuntime +#include "pycore_semaphore.h" // _PySemaphore #include "pycore_pythread.h" // PyThread_start_joinable_thread() #include "pycore_import.h" // _PyImport_FrozenBootstrap #include @@ -2342,26 +2343,32 @@ test_get_incomplete_frame(void) } static void -do_gilstate_ensure(void *unused) +do_gilstate_ensure(void *semaphore_ptr) { - for (int i = 0; i < 50; ++i) { - PyGILState_STATE gstate = PyGILState_Ensure(); - assert(PyGILState_Check()); // Yuck - PyGILState_Release(gstate); - } + _PySemaphore *semaphore = (_PySemaphore *)semaphore_ptr; + // Signal to the calling thread that we've started + _PySemaphore_Wakeup(semaphore); + PyGILState_STATE gstate = PyGILState_Ensure(); // This should hang + assert(NULL); } static int test_gilstate_after_finalization(void) { _testembed_Py_Initialize(); + Py_Finalize(); PyThread_handle_t handle; PyThread_ident_t ident; - if (PyThread_start_joinable_thread(&do_gilstate_ensure, NULL, &ident, &handle) < 0) { + _PySemaphore semaphore; + _PySemaphore_Init(&semaphore); + if (PyThread_start_joinable_thread(&do_gilstate_ensure, &semaphore, &ident, &handle) < 0) { return -1; } - Py_Finalize(); - return PyThread_join_thread(handle); + _PySemaphore_Wait(&semaphore, /*timeout_ns=*/-1, /*detach=*/0); + // We're now pretty confident that the thread went for + // PyGILState_Ensure(), but that means it got hung. + _PySemaphore_Destroy(&semaphore); + return PyThread_detach_thread(handle); } /* ********************************************************* diff --git a/Python/pystate.c b/Python/pystate.c index 0c21a12079cff1..d4fa3792dfd6c9 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -271,9 +271,9 @@ unbind_gilstate_tstate(PyThreadState *tstate) assert(tstate_is_bound(tstate)); // XXX assert(!tstate->_status.active); assert(tstate->_status.bound_gilstate); - assert(tstate == gilstate_get()); - - gilstate_clear(); + if (tstate == gilstate_get()) { + gilstate_clear(); + } tstate->_status.bound_gilstate = 0; } @@ -2695,8 +2695,9 @@ PyGILState_Ensure(void) /* Ensure that _PyEval_InitThreads() and _PyGILState_Init() have been called by Py_Initialize() */ - assert(_PyEval_ThreadsInitialized()); - assert(runtime->gilstate.autoInterpreterState != NULL); + if (!_PyEval_ThreadsInitialized() || runtime->gilstate.autoInterpreterState == NULL) { + PyThread_hang_thread(); + } PyThreadState *tcur = gilstate_get(); int has_gil; From 78cd775a30c2ad99e5f5eca8919633475798b3eb Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Mon, 14 Apr 2025 07:27:59 -0400 Subject: [PATCH 06/24] Remove unused variable. --- Programs/_testembed.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Programs/_testembed.c b/Programs/_testembed.c index c1ac330c51ec32..a5e9e3d0e87f99 100644 --- a/Programs/_testembed.c +++ b/Programs/_testembed.c @@ -2348,7 +2348,7 @@ do_gilstate_ensure(void *semaphore_ptr) _PySemaphore *semaphore = (_PySemaphore *)semaphore_ptr; // Signal to the calling thread that we've started _PySemaphore_Wakeup(semaphore); - PyGILState_STATE gstate = PyGILState_Ensure(); // This should hang + PyGILState_Ensure(); // This should hang assert(NULL); } From c8421fc3725cbabba4b4f674748efe73db067ef6 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Mon, 14 Apr 2025 07:41:34 -0400 Subject: [PATCH 07/24] Add blurb. --- .../next/C_API/2025-04-14-07-41-28.gh-issue-131185.ZCjMHD.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/C_API/2025-04-14-07-41-28.gh-issue-131185.ZCjMHD.rst diff --git a/Misc/NEWS.d/next/C_API/2025-04-14-07-41-28.gh-issue-131185.ZCjMHD.rst b/Misc/NEWS.d/next/C_API/2025-04-14-07-41-28.gh-issue-131185.ZCjMHD.rst new file mode 100644 index 00000000000000..8a19251f88f5be --- /dev/null +++ b/Misc/NEWS.d/next/C_API/2025-04-14-07-41-28.gh-issue-131185.ZCjMHD.rst @@ -0,0 +1,2 @@ +:c:func:`PyGILState_Ensure` no longer crashes when calling after interpreter +finalization. From 0a875ff65d0ac9446ca050d50fc88612bb866723 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Mon, 14 Apr 2025 07:54:37 -0400 Subject: [PATCH 08/24] Make the C analyzer happy. --- Tools/c-analyzer/cpython/ignored.tsv | 1 + 1 file changed, 1 insertion(+) diff --git a/Tools/c-analyzer/cpython/ignored.tsv b/Tools/c-analyzer/cpython/ignored.tsv index 14dc5007b65861..11a6214822ac99 100644 --- a/Tools/c-analyzer/cpython/ignored.tsv +++ b/Tools/c-analyzer/cpython/ignored.tsv @@ -190,6 +190,7 @@ Python/pyfpe.c - PyFPE_counter - Python/import.c - pkgcontext - Python/pystate.c - _Py_tss_tstate - +Python/pystate.c - _Py_tss_gilstate - ##----------------------- ## should be const From dd330f8060a2139b08cd765e619bdbda8e5b4b80 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Mon, 14 Apr 2025 08:07:08 -0400 Subject: [PATCH 09/24] Fix the C analyzer (round 2). --- Tools/c-analyzer/cpython/ignored.tsv | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tools/c-analyzer/cpython/ignored.tsv b/Tools/c-analyzer/cpython/ignored.tsv index 11a6214822ac99..4576d4cc9ea5a9 100644 --- a/Tools/c-analyzer/cpython/ignored.tsv +++ b/Tools/c-analyzer/cpython/ignored.tsv @@ -190,7 +190,7 @@ Python/pyfpe.c - PyFPE_counter - Python/import.c - pkgcontext - Python/pystate.c - _Py_tss_tstate - -Python/pystate.c - _Py_tss_gilstate - +Python/pystate.c - _Py_tss_gilstate - ##----------------------- ## should be const From a3c229266565e238ec25227f135cfeba1dfd6990 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Tue, 15 Apr 2025 16:14:52 -0400 Subject: [PATCH 10/24] Add test to test_embed --- Lib/test/test_embed.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Lib/test/test_embed.py b/Lib/test/test_embed.py index e06e684408ca6b..757be365019749 100644 --- a/Lib/test/test_embed.py +++ b/Lib/test/test_embed.py @@ -1916,6 +1916,10 @@ def test_get_incomplete_frame(self): self.run_embedded_interpreter("test_get_incomplete_frame") + def test_gilstate_after_finalization(self): + self.run_embedded_interpreter("test_gilstate_after_finalization") + + class MiscTests(EmbeddingTestsMixin, unittest.TestCase): def test_unicode_id_init(self): # bpo-42882: Test that _PyUnicode_FromId() works From 86be7f316d12e2d7e009c3ea85f2cf156b28f6f7 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Tue, 15 Apr 2025 16:15:44 -0400 Subject: [PATCH 11/24] Update Misc/NEWS.d/next/C_API/2025-04-14-07-41-28.gh-issue-131185.ZCjMHD.rst Co-authored-by: Sam Gross --- .../next/C_API/2025-04-14-07-41-28.gh-issue-131185.ZCjMHD.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/C_API/2025-04-14-07-41-28.gh-issue-131185.ZCjMHD.rst b/Misc/NEWS.d/next/C_API/2025-04-14-07-41-28.gh-issue-131185.ZCjMHD.rst index 8a19251f88f5be..aa0e8bca93b46f 100644 --- a/Misc/NEWS.d/next/C_API/2025-04-14-07-41-28.gh-issue-131185.ZCjMHD.rst +++ b/Misc/NEWS.d/next/C_API/2025-04-14-07-41-28.gh-issue-131185.ZCjMHD.rst @@ -1,2 +1,2 @@ -:c:func:`PyGILState_Ensure` no longer crashes when calling after interpreter +:c:func:`PyGILState_Ensure` no longer crashes when called after interpreter finalization. From 453a46850fd9c1c5cfdcd1c8275e1d0ee6c4ad3f Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Tue, 15 Apr 2025 16:18:19 -0400 Subject: [PATCH 12/24] Fix some of the comments. --- Python/pystate.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/Python/pystate.c b/Python/pystate.c index d4fa3792dfd6c9..11213d1f2fcb74 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -72,10 +72,8 @@ to avoid the expense of doing their own locking). /* The attached thread state for the current thread. */ _Py_thread_local PyThreadState *_Py_tss_tstate = NULL; -/* The bound gilstate for the current thread. - Basically, this is used for caching the right interpreter - for subsequent PyGILState_Ensure() calls. Despite the name, - this doesn't have much to do with the actual GIL. */ +/* The "bound" thread state used by PyGILState_Ensure(), + also known as a "gilstate." */ _Py_thread_local PyThreadState *_Py_tss_gilstate = NULL; #endif @@ -126,10 +124,9 @@ _PyThreadState_GetCurrent(void) //--------------------------------------------- -// the GIL state bound to the current OS thread +// The thread state used by PyGILState_Ensure() //--------------------------------------------- - /* The stored thread state is set by bind_tstate() (AKA PyThreadState_Bind(). From da9b606c671e8c9ba95b672d8135629ef677fcb6 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Tue, 15 Apr 2025 16:22:06 -0400 Subject: [PATCH 13/24] Use PyEvent instead of _PySemaphore --- Programs/_testembed.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/Programs/_testembed.c b/Programs/_testembed.c index a5e9e3d0e87f99..0721db7b41afe3 100644 --- a/Programs/_testembed.c +++ b/Programs/_testembed.c @@ -2343,11 +2343,11 @@ test_get_incomplete_frame(void) } static void -do_gilstate_ensure(void *semaphore_ptr) +do_gilstate_ensure(void *event_ptr) { - _PySemaphore *semaphore = (_PySemaphore *)semaphore_ptr; + PyEvent *event = (PyEvent *)event_ptr; // Signal to the calling thread that we've started - _PySemaphore_Wakeup(semaphore); + _PyEvent_Notify(event); PyGILState_Ensure(); // This should hang assert(NULL); } @@ -2359,15 +2359,13 @@ test_gilstate_after_finalization(void) Py_Finalize(); PyThread_handle_t handle; PyThread_ident_t ident; - _PySemaphore semaphore; - _PySemaphore_Init(&semaphore); - if (PyThread_start_joinable_thread(&do_gilstate_ensure, &semaphore, &ident, &handle) < 0) { + PyEvent event = {}; + if (PyThread_start_joinable_thread(&do_gilstate_ensure, &event, &ident, &handle) < 0) { return -1; } - _PySemaphore_Wait(&semaphore, /*timeout_ns=*/-1, /*detach=*/0); + PyEvent_Wait(&event); // We're now pretty confident that the thread went for // PyGILState_Ensure(), but that means it got hung. - _PySemaphore_Destroy(&semaphore); return PyThread_detach_thread(handle); } From 2f9e499af4bd2202f495ffd4f0a5433f7d973457 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Fri, 18 Apr 2025 12:18:17 -0400 Subject: [PATCH 14/24] Fix scalar initializer. --- Programs/_testembed.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Programs/_testembed.c b/Programs/_testembed.c index 0721db7b41afe3..cb89486946946a 100644 --- a/Programs/_testembed.c +++ b/Programs/_testembed.c @@ -2359,7 +2359,7 @@ test_gilstate_after_finalization(void) Py_Finalize(); PyThread_handle_t handle; PyThread_ident_t ident; - PyEvent event = {}; + PyEvent event = {0}; if (PyThread_start_joinable_thread(&do_gilstate_ensure, &event, &ident, &handle) < 0) { return -1; } From 0dba178207f63e92a7566811e3122909bf448f84 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Fri, 18 Apr 2025 12:34:12 -0400 Subject: [PATCH 15/24] Add comment about thread-safety. --- Python/pystate.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/Python/pystate.c b/Python/pystate.c index 11213d1f2fcb74..8c8dad01115d0b 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -2691,7 +2691,12 @@ PyGILState_Ensure(void) called Py_Initialize(). */ /* Ensure that _PyEval_InitThreads() and _PyGILState_Init() have been - called by Py_Initialize() */ + called by Py_Initialize() + + TODO: This isn't thread-safe. There's no protection here against + concurrent finalization of the interpreter; it's simply a guard + for *after* the interpreter has finalized. + */ if (!_PyEval_ThreadsInitialized() || runtime->gilstate.autoInterpreterState == NULL) { PyThread_hang_thread(); } From 863d2bc89d454275f167df144275941f3dc102a6 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Sun, 18 May 2025 11:55:32 -0400 Subject: [PATCH 16/24] Fix header. --- Programs/_testembed.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Programs/_testembed.c b/Programs/_testembed.c index cb89486946946a..ec1be1304a1b98 100644 --- a/Programs/_testembed.c +++ b/Programs/_testembed.c @@ -8,7 +8,7 @@ #include #include "pycore_initconfig.h" // _PyConfig_InitCompatConfig() #include "pycore_runtime.h" // _PyRuntime -#include "pycore_semaphore.h" // _PySemaphore +#include "pycore_lock.h" // PyEvent #include "pycore_pythread.h" // PyThread_start_joinable_thread() #include "pycore_import.h" // _PyImport_FrozenBootstrap #include From cad04048a2f142784eeb1c2510f81524f728f67b Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Sun, 18 May 2025 12:18:05 -0400 Subject: [PATCH 17/24] Fix renaming. --- Programs/_testembed.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Programs/_testembed.c b/Programs/_testembed.c index 44bafe4468ef45..577da65c7cdafa 100644 --- a/Programs/_testembed.c +++ b/Programs/_testembed.c @@ -2326,7 +2326,7 @@ do_gilstate_ensure(void *event_ptr) static int test_gilstate_after_finalization(void) { - _testembed_Py_Initialize(); + _testembed_initialize(); Py_Finalize(); PyThread_handle_t handle; PyThread_ident_t ident; From 779be6efeaa75533bcfe8ea0c0b47e0247efc5c9 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Sun, 18 May 2025 12:18:35 -0400 Subject: [PATCH 18/24] Restore assertion. --- Python/pystate.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Python/pystate.c b/Python/pystate.c index ccd0ca2a52d954..dfb1ede5651528 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -268,9 +268,8 @@ unbind_gilstate_tstate(PyThreadState *tstate) assert(tstate_is_bound(tstate)); // XXX assert(!tstate->_status.active); assert(tstate->_status.bound_gilstate); - if (tstate == gilstate_get()) { - gilstate_clear(); - } + assert(tstate == gilstate_get()); + gilstate_clear(); tstate->_status.bound_gilstate = 0; } From 86144fcc78cd7644c3ed6c1654c66048d1128d70 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Mon, 19 May 2025 12:24:34 -0400 Subject: [PATCH 19/24] Clear the thread-local. --- Python/pystate.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Python/pystate.c b/Python/pystate.c index dfb1ede5651528..2f40380e4b016f 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -402,6 +402,7 @@ _PyRuntimeState_Fini(_PyRuntimeState *runtime) /* The count is cleared by _Py_FinalizeRefTotal(). */ assert(runtime->object_state.interpreter_leaks == 0); #endif + gilstate_clear(); } #ifdef HAVE_FORK @@ -433,6 +434,7 @@ _PyRuntimeState_ReInitThreads(_PyRuntimeState *runtime) #endif _PyTypes_AfterFork(); + gilstate_clear(); _PyThread_AfterFork(&runtime->threads); From 9f1d919b219048f7eb2caa74e690308f0a9bafb4 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Mon, 19 May 2025 12:28:47 -0400 Subject: [PATCH 20/24] Restore trashTSSkey silliness. --- Include/internal/pycore_runtime_init.h | 3 +++ Include/internal/pycore_runtime_structs.h | 3 +++ 2 files changed, 6 insertions(+) diff --git a/Include/internal/pycore_runtime_init.h b/Include/internal/pycore_runtime_init.h index 0837fcb54a3906..4200d91a2fcd9d 100644 --- a/Include/internal/pycore_runtime_init.h +++ b/Include/internal/pycore_runtime_init.h @@ -61,6 +61,9 @@ extern PyTypeObject _PyExc_MemoryError; }, \ }, \ }, \ + /* A TSS key must be initialized with Py_tss_NEEDS_INIT \ + in accordance with the specification. */ \ + .autoTSSkey = Py_tss_NEEDS_INIT, \ .parser = _parser_runtime_state_INIT, \ .ceval = { \ .pending_mainthread = { \ diff --git a/Include/internal/pycore_runtime_structs.h b/Include/internal/pycore_runtime_structs.h index d5e5531350af74..12164c7fdd9425 100644 --- a/Include/internal/pycore_runtime_structs.h +++ b/Include/internal/pycore_runtime_structs.h @@ -223,6 +223,9 @@ struct pyruntimestate { struct _pythread_runtime_state threads; struct _signals_runtime_state signals; + /* Used instead of PyThreadState.trash when there is not current tstate. */ + Py_tss_t trashTSSkey; + PyWideStringList orig_argv; struct _parser_runtime_state parser; From 0c16fea663a0cd5e41de489effd9d00c7cfc6b03 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Mon, 19 May 2025 12:37:01 -0400 Subject: [PATCH 21/24] Fix broken build. --- Include/internal/pycore_runtime_init.h | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/Include/internal/pycore_runtime_init.h b/Include/internal/pycore_runtime_init.h index 4200d91a2fcd9d..b182f7825a2326 100644 --- a/Include/internal/pycore_runtime_init.h +++ b/Include/internal/pycore_runtime_init.h @@ -61,9 +61,6 @@ extern PyTypeObject _PyExc_MemoryError; }, \ }, \ }, \ - /* A TSS key must be initialized with Py_tss_NEEDS_INIT \ - in accordance with the specification. */ \ - .autoTSSkey = Py_tss_NEEDS_INIT, \ .parser = _parser_runtime_state_INIT, \ .ceval = { \ .pending_mainthread = { \ @@ -236,4 +233,4 @@ extern PyTypeObject _PyExc_MemoryError; #ifdef __cplusplus } #endif -#endif /* !Py_INTERNAL_RUNTIME_INIT_H */ +#endif /* !Py_INTERNAL_RUNTIME_INIT_H */ \ No newline at end of file From 7c27c9fb416c7ab1b9b070df08982c630e07ce98 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Mon, 19 May 2025 16:56:33 -0400 Subject: [PATCH 22/24] Revert "Clear the thread-local." This reverts commit 86144fcc78cd7644c3ed6c1654c66048d1128d70. --- Python/pystate.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/Python/pystate.c b/Python/pystate.c index 2f40380e4b016f..dfb1ede5651528 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -402,7 +402,6 @@ _PyRuntimeState_Fini(_PyRuntimeState *runtime) /* The count is cleared by _Py_FinalizeRefTotal(). */ assert(runtime->object_state.interpreter_leaks == 0); #endif - gilstate_clear(); } #ifdef HAVE_FORK @@ -434,7 +433,6 @@ _PyRuntimeState_ReInitThreads(_PyRuntimeState *runtime) #endif _PyTypes_AfterFork(); - gilstate_clear(); _PyThread_AfterFork(&runtime->threads); From 8f18e95586c03f9e24f85cb1e18bef01af0f1289 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Mon, 19 May 2025 16:57:13 -0400 Subject: [PATCH 23/24] Reapply "Clear the thread-local." This reverts commit 7c27c9fb416c7ab1b9b070df08982c630e07ce98. --- Python/pystate.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Python/pystate.c b/Python/pystate.c index dfb1ede5651528..2f40380e4b016f 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -402,6 +402,7 @@ _PyRuntimeState_Fini(_PyRuntimeState *runtime) /* The count is cleared by _Py_FinalizeRefTotal(). */ assert(runtime->object_state.interpreter_leaks == 0); #endif + gilstate_clear(); } #ifdef HAVE_FORK @@ -433,6 +434,7 @@ _PyRuntimeState_ReInitThreads(_PyRuntimeState *runtime) #endif _PyTypes_AfterFork(); + gilstate_clear(); _PyThread_AfterFork(&runtime->threads); From f90bb119b1b5e6318a938fd5ebf5a90f238a6ed5 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Mon, 19 May 2025 23:44:26 -0400 Subject: [PATCH 24/24] Only remove the evil gilstate clear. --- Python/pystate.c | 1 - 1 file changed, 1 deletion(-) diff --git a/Python/pystate.c b/Python/pystate.c index 2f40380e4b016f..3657d958ce12d6 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -434,7 +434,6 @@ _PyRuntimeState_ReInitThreads(_PyRuntimeState *runtime) #endif _PyTypes_AfterFork(); - gilstate_clear(); _PyThread_AfterFork(&runtime->threads);