From b2d7a4424add2fc70fd9a9d5e45517e68d75a060 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Thu, 1 Nov 2018 04:17:35 +0100 Subject: [PATCH] WIP: bpo-35059: _PyThreadState_GET() checks that the GIL is hold Convert _PyThreadState_GET() and _PyInterpreterState_GET_UNSAFE() macros to static inline functions, and the functions now check that the GIL is hold. Add _PyThreadState_GET_UNSAFE(): similar to _PyThreadState_GET(), but don't check that the GIL is hold. --- Include/internal/pycore_state.h | 23 ++++++++++++++++++++--- Lib/test/test_capi.py | 7 +++++-- Python/ceval.c | 3 ++- Python/pylifecycle.c | 2 +- Python/pystate.c | 12 +++++++----- 5 files changed, 35 insertions(+), 12 deletions(-) diff --git a/Include/internal/pycore_state.h b/Include/internal/pycore_state.h index 6285ecf5f5dc42..540322628aa8a3 100644 --- a/Include/internal/pycore_state.h +++ b/Include/internal/pycore_state.h @@ -176,6 +176,12 @@ PyAPI_FUNC(_PyInitError) _PyRuntime_Initialize(void); /* Variable and macro for in-line access to current thread and interpreter state */ +/* Similar to _PyThreadState_GET(), but don't check that the GIL is hold. */ +static inline PyThreadState* _PyThreadState_GET_UNSAFE(void) +{ + return (PyThreadState*)_Py_atomic_load_relaxed(&_PyRuntime.gilstate.tstate_current); +} + /* Get the current Python thread state. Efficient macro reading directly the 'gilstate.tstate_current' atomic @@ -185,8 +191,11 @@ PyAPI_FUNC(_PyInitError) _PyRuntime_Initialize(void); The caller must hold the GIL. See also PyThreadState_Get() and PyThreadState_GET(). */ -#define _PyThreadState_GET() \ - ((PyThreadState*)_Py_atomic_load_relaxed(&_PyRuntime.gilstate.tstate_current)) +static inline PyThreadState* _PyThreadState_GET(void) +{ + assert(PyGILState_Check()); + return _PyThreadState_GET_UNSAFE(); +} /* Redefine PyThreadState_GET() as an alias to _PyThreadState_GET() */ #undef PyThreadState_GET @@ -200,7 +209,15 @@ PyAPI_FUNC(_PyInitError) _PyRuntime_Initialize(void); See also _PyInterpreterState_Get() and _PyGILState_GetInterpreterStateUnsafe(). */ -#define _PyInterpreterState_GET_UNSAFE() (_PyThreadState_GET()->interp) +static inline PyInterpreterState* _PyInterpreterState_GET_UNSAFE(void) +{ + assert(PyGILState_Check()); + PyThreadState *tstate = _PyThreadState_GET_UNSAFE(); + assert(tstate != NULL); + PyInterpreterState *interp = tstate->interp; + assert(interp != NULL); + return interp; +} /* Other */ diff --git a/Lib/test/test_capi.py b/Lib/test/test_capi.py index b3600ebe993de5..1c1128dff980ed 100644 --- a/Lib/test/test_capi.py +++ b/Lib/test/test_capi.py @@ -59,9 +59,12 @@ def test_no_FatalError_infinite_loop(self): (out, err) = p.communicate() self.assertEqual(out, b'') # This used to cause an infinite loop. - self.assertTrue(err.rstrip().startswith( + err = err.rstrip() + cond = err.startswith( b'Fatal Python error:' - b' PyThreadState_Get: no current thread')) + b' PyThreadState_Get: no current thread') + cond |= bool(re.search(b"Assertion.*PyGILState_Check.*failed", err)) + self.assertTrue(cond, err.decode(errors="replace")) def test_memoryview_from_NULL_pointer(self): self.assertRaises(ValueError, _testcapi.make_memoryview_from_NULL_pointer) diff --git a/Python/ceval.c b/Python/ceval.c index ac9db1533bf3ef..5838f29b153ea8 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -188,7 +188,8 @@ PyEval_ReleaseLock(void) We therefore avoid PyThreadState_Get() which dumps a fatal error in debug mode. */ - drop_gil(_PyThreadState_GET()); + PyThreadState *tstate = _PyThreadState_GET_UNSAFE(); + drop_gil(tstate); } void diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 4c5cb53429172a..4e78d0be9bc809 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -1916,7 +1916,7 @@ fatal_error(const char *prefix, const char *msg, int status) and holds the GIL */ PyThreadState *tss_tstate = PyGILState_GetThisThreadState(); if (tss_tstate != NULL) { - PyThreadState *tstate = _PyThreadState_GET(); + PyThreadState *tstate = _PyThreadState_GET_UNSAFE(); if (tss_tstate != tstate) { /* The Python thread does not hold the GIL */ tss_tstate = NULL; diff --git a/Python/pystate.c b/Python/pystate.c index c193a10c8cb290..3f0d7057208cb4 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -703,7 +703,7 @@ PyThreadState_Delete(PyThreadState *tstate) void PyThreadState_DeleteCurrent() { - PyThreadState *tstate = _PyThreadState_GET(); + PyThreadState *tstate = _PyThreadState_GET_UNSAFE(); if (tstate == NULL) Py_FatalError( "PyThreadState_DeleteCurrent: no current tstate"); @@ -776,7 +776,7 @@ PyThreadState_Get(void) PyThreadState * PyThreadState_Swap(PyThreadState *newts) { - PyThreadState *oldts = _PyThreadState_GET(); + PyThreadState *oldts = _PyThreadState_GET_UNSAFE(); _PyThreadState_SET(newts); /* It should not be possible for more than one thread state @@ -957,8 +957,10 @@ static int PyThreadState_IsCurrent(PyThreadState *tstate) { /* Must be the tstate for this thread */ - assert(PyGILState_GetThisThreadState()==tstate); - return tstate == _PyThreadState_GET(); + assert(PyGILState_GetThisThreadState() == tstate); + + PyThreadState *current = _PyThreadState_GET_UNSAFE(); + return (tstate == current); } /* Internal initialization/finalization functions called by @@ -1085,7 +1087,7 @@ PyGILState_Check(void) return 1; } - tstate = _PyThreadState_GET(); + tstate = _PyThreadState_GET_UNSAFE(); if (tstate == NULL) return 0;