From a7474421bb379e036dd131493bef08cb950a77ed Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Sat, 7 Oct 2023 00:03:02 +0900 Subject: [PATCH 01/10] gh-109693: Use pyatomic.h for signal module --- Include/internal/pycore_signal.h | 8 ++++---- Modules/signalmodule.c | 33 ++++++++++++++++---------------- Python/ceval_gil.c | 2 +- 3 files changed, 21 insertions(+), 22 deletions(-) diff --git a/Include/internal/pycore_signal.h b/Include/internal/pycore_signal.h index b38b1d30112b60..ffe49640392c8f 100644 --- a/Include/internal/pycore_signal.h +++ b/Include/internal/pycore_signal.h @@ -38,12 +38,12 @@ PyAPI_FUNC(void) _Py_RestoreSignals(void); #define INVALID_FD (-1) struct _signals_runtime_state { - volatile struct { - _Py_atomic_int tripped; + struct { + int tripped; /* func is atomic to ensure that PyErr_SetInterrupt is async-signal-safe * (even though it would probably be otherwise, anyway). */ - _Py_atomic_address func; + uintptr_t func; } handlers[Py_NSIG]; volatile struct { @@ -64,7 +64,7 @@ struct _signals_runtime_state { } wakeup; /* Speed up sigcheck() when none tripped */ - _Py_atomic_int is_tripped; + int is_tripped; /* These objects necessarily belong to the main interpreter. */ PyObject *default_handler; diff --git a/Modules/signalmodule.c b/Modules/signalmodule.c index ac3457003b0cb6..d6d94ba3df36c5 100644 --- a/Modules/signalmodule.c +++ b/Modules/signalmodule.c @@ -4,7 +4,6 @@ /* XXX Signals should be recorded per thread, now we have thread state. */ #include "Python.h" -#include "pycore_atomic.h" // _Py_atomic_int #include "pycore_call.h" // _PyObject_Call() #include "pycore_ceval.h" // _PyEval_SignalReceived() #include "pycore_emscripten_signal.h" // _Py_CHECK_EMSCRIPTEN_SIGNALS @@ -124,13 +123,13 @@ typedef struct { Py_LOCAL_INLINE(PyObject *) get_handler(int i) { - return (PyObject *)_Py_atomic_load(&Handlers[i].func); + return (PyObject *)_Py_atomic_load_uintptr(&Handlers[i].func); } Py_LOCAL_INLINE(void) set_handler(int i, PyObject* func) { - _Py_atomic_store(&Handlers[i].func, (uintptr_t)func); + _Py_atomic_store_uintptr(&Handlers[i].func, (uintptr_t)func); } @@ -267,11 +266,11 @@ report_wakeup_send_error(void* data) static void trip_signal(int sig_num) { - _Py_atomic_store_relaxed(&Handlers[sig_num].tripped, 1); + _Py_atomic_store_int_relaxed(&Handlers[sig_num].tripped, 1); /* Set is_tripped after setting .tripped, as it gets cleared in PyErr_CheckSignals() before .tripped. */ - _Py_atomic_store(&is_tripped, 1); + _Py_atomic_store_int(&is_tripped, 1); /* Signals are always handled by the main interpreter */ PyInterpreterState *interp = _PyInterpreterState_Main(); @@ -1731,7 +1730,7 @@ _PySignal_Fini(void) // Restore default signals and clear handlers for (int signum = 1; signum < Py_NSIG; signum++) { PyObject *func = get_handler(signum); - _Py_atomic_store_relaxed(&Handlers[signum].tripped, 0); + _Py_atomic_store_int_relaxed(&Handlers[signum].tripped, 0); set_handler(signum, NULL); if (func != NULL && func != Py_None @@ -1785,7 +1784,7 @@ int _PyErr_CheckSignalsTstate(PyThreadState *tstate) { _Py_CHECK_EMSCRIPTEN_SIGNALS(); - if (!_Py_atomic_load(&is_tripped)) { + if (!_Py_atomic_load_int(&is_tripped)) { return 0; } @@ -1803,15 +1802,15 @@ _PyErr_CheckSignalsTstate(PyThreadState *tstate) * we receive a signal i after we zero is_tripped and before we * check Handlers[i].tripped. */ - _Py_atomic_store(&is_tripped, 0); + _Py_atomic_store_int(&is_tripped, 0); _PyInterpreterFrame *frame = _PyThreadState_GetFrame(tstate); signal_state_t *state = &signal_global_state; for (int i = 1; i < Py_NSIG; i++) { - if (!_Py_atomic_load_relaxed(&Handlers[i].tripped)) { + if (!_Py_atomic_load_int_relaxed(&Handlers[i].tripped)) { continue; } - _Py_atomic_store_relaxed(&Handlers[i].tripped, 0); + _Py_atomic_store_int_relaxed(&Handlers[i].tripped, 0); /* Signal handlers can be modified while a signal is received, * and therefore the fact that trip_signal() or PyErr_SetInterrupt() @@ -1857,7 +1856,7 @@ _PyErr_CheckSignalsTstate(PyThreadState *tstate) } if (!result) { /* On error, re-schedule a call to _PyErr_CheckSignalsTstate() */ - _Py_atomic_store(&is_tripped, 1); + _Py_atomic_store_int(&is_tripped, 1); return -1; } @@ -1975,7 +1974,7 @@ _PySignal_Init(int install_signal_handlers) #endif for (int signum = 1; signum < Py_NSIG; signum++) { - _Py_atomic_store_relaxed(&Handlers[signum].tripped, 0); + _Py_atomic_store_int_relaxed(&Handlers[signum].tripped, 0); } if (install_signal_handlers) { @@ -1997,11 +1996,11 @@ _PyOS_InterruptOccurred(PyThreadState *tstate) return 0; } - if (!_Py_atomic_load_relaxed(&Handlers[SIGINT].tripped)) { + if (!_Py_atomic_load_int_relaxed(&Handlers[SIGINT].tripped)) { return 0; } - _Py_atomic_store_relaxed(&Handlers[SIGINT].tripped, 0); + _Py_atomic_store_int_relaxed(&Handlers[SIGINT].tripped, 0); return 1; } @@ -2019,13 +2018,13 @@ PyOS_InterruptOccurred(void) static void _clear_pending_signals(void) { - if (!_Py_atomic_load(&is_tripped)) { + if (!_Py_atomic_load_int(&is_tripped)) { return; } - _Py_atomic_store(&is_tripped, 0); + _Py_atomic_store_int(&is_tripped, 0); for (int i = 1; i < Py_NSIG; ++i) { - _Py_atomic_store_relaxed(&Handlers[i].tripped, 0); + _Py_atomic_store_int_relaxed(&Handlers[i].tripped, 0); } } diff --git a/Python/ceval_gil.c b/Python/ceval_gil.c index f237e384333e45..2201e70d0113e9 100644 --- a/Python/ceval_gil.c +++ b/Python/ceval_gil.c @@ -76,7 +76,7 @@ update_eval_breaker_from_thread(PyInterpreterState *interp, PyThreadState *tstat _Py_set_eval_breaker_bit(interp, _PY_CALLS_TO_DO_BIT, 1); } if (_Py_ThreadCanHandleSignals(interp)) { - if (_Py_atomic_load(&_PyRuntime.signals.is_tripped)) { + if (_Py_atomic_load_int(&_PyRuntime.signals.is_tripped)) { _Py_set_eval_breaker_bit(interp, _PY_SIGNALS_PENDING_BIT, 1); } } From 544fe847140aea1f42a1cd150776ff7baeecdbf5 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Sat, 7 Oct 2023 00:55:30 +0900 Subject: [PATCH 02/10] Remove pycore_atomic.h from pycore_signal.h too --- Include/internal/pycore_signal.h | 1 - 1 file changed, 1 deletion(-) diff --git a/Include/internal/pycore_signal.h b/Include/internal/pycore_signal.h index ffe49640392c8f..81fbbecd5cff90 100644 --- a/Include/internal/pycore_signal.h +++ b/Include/internal/pycore_signal.h @@ -10,7 +10,6 @@ extern "C" { # error "this header requires Py_BUILD_CORE define" #endif -#include "pycore_atomic.h" // _Py_atomic_address #include // NSIG From 86b97a5a857bdcbeaddf0edbcedcfd369c238f13 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Sat, 7 Oct 2023 01:10:42 +0900 Subject: [PATCH 03/10] Address code review --- Include/internal/pycore_signal.h | 2 +- Modules/signalmodule.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Include/internal/pycore_signal.h b/Include/internal/pycore_signal.h index 81fbbecd5cff90..d835cdc7d677b9 100644 --- a/Include/internal/pycore_signal.h +++ b/Include/internal/pycore_signal.h @@ -42,7 +42,7 @@ struct _signals_runtime_state { /* func is atomic to ensure that PyErr_SetInterrupt is async-signal-safe * (even though it would probably be otherwise, anyway). */ - uintptr_t func; + void* func; } handlers[Py_NSIG]; volatile struct { diff --git a/Modules/signalmodule.c b/Modules/signalmodule.c index d6d94ba3df36c5..351a937d8215e0 100644 --- a/Modules/signalmodule.c +++ b/Modules/signalmodule.c @@ -123,13 +123,13 @@ typedef struct { Py_LOCAL_INLINE(PyObject *) get_handler(int i) { - return (PyObject *)_Py_atomic_load_uintptr(&Handlers[i].func); + return (PyObject *)_Py_atomic_load_ptr(&Handlers[i].func); } Py_LOCAL_INLINE(void) set_handler(int i, PyObject* func) { - _Py_atomic_store_uintptr(&Handlers[i].func, (uintptr_t)func); + _Py_atomic_store_ptr(&Handlers[i].func, func); } From 4a8c0b3cec61f36b5358232857c063d20cfafb88 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Sat, 7 Oct 2023 01:13:01 +0900 Subject: [PATCH 04/10] nit --- Include/internal/pycore_signal.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Include/internal/pycore_signal.h b/Include/internal/pycore_signal.h index d835cdc7d677b9..e705249552d024 100644 --- a/Include/internal/pycore_signal.h +++ b/Include/internal/pycore_signal.h @@ -42,7 +42,7 @@ struct _signals_runtime_state { /* func is atomic to ensure that PyErr_SetInterrupt is async-signal-safe * (even though it would probably be otherwise, anyway). */ - void* func; + PyObject* func; } handlers[Py_NSIG]; volatile struct { From 3696f01993c6ced3fa84a7eebeea2ea0aab7451a Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Sun, 8 Oct 2023 02:45:15 +0900 Subject: [PATCH 05/10] Add comment --- Include/internal/pycore_signal.h | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/Include/internal/pycore_signal.h b/Include/internal/pycore_signal.h index e705249552d024..67989437b7da6e 100644 --- a/Include/internal/pycore_signal.h +++ b/Include/internal/pycore_signal.h @@ -38,6 +38,10 @@ PyAPI_FUNC(void) _Py_RestoreSignals(void); struct _signals_runtime_state { struct { + /* To access tripped / func with "relaxed" memory order + * _signals_runtime_state.is_tripped should be accessed by + * "sequentially consistent" order as the memory barrier. + */ int tripped; /* func is atomic to ensure that PyErr_SetInterrupt is async-signal-safe * (even though it would probably be otherwise, anyway). @@ -62,7 +66,9 @@ struct _signals_runtime_state { #endif } wakeup; - /* Speed up sigcheck() when none tripped */ + /* Speed up sigcheck() when none tripped and + * act as a memory barrier("sequentially consistent" order) + */ int is_tripped; /* These objects necessarily belong to the main interpreter. */ From e19a4378b4f572458940b65093b2d5e9d96d37e8 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Sun, 8 Oct 2023 13:00:17 +0900 Subject: [PATCH 06/10] Revert "Add comment" This reverts commit 3696f01993c6ced3fa84a7eebeea2ea0aab7451a. --- Include/internal/pycore_signal.h | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/Include/internal/pycore_signal.h b/Include/internal/pycore_signal.h index 67989437b7da6e..e705249552d024 100644 --- a/Include/internal/pycore_signal.h +++ b/Include/internal/pycore_signal.h @@ -38,10 +38,6 @@ PyAPI_FUNC(void) _Py_RestoreSignals(void); struct _signals_runtime_state { struct { - /* To access tripped / func with "relaxed" memory order - * _signals_runtime_state.is_tripped should be accessed by - * "sequentially consistent" order as the memory barrier. - */ int tripped; /* func is atomic to ensure that PyErr_SetInterrupt is async-signal-safe * (even though it would probably be otherwise, anyway). @@ -66,9 +62,7 @@ struct _signals_runtime_state { #endif } wakeup; - /* Speed up sigcheck() when none tripped and - * act as a memory barrier("sequentially consistent" order) - */ + /* Speed up sigcheck() when none tripped */ int is_tripped; /* These objects necessarily belong to the main interpreter. */ From bb3dae2003e4569e561814f310b01e62c740d9a2 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Sun, 8 Oct 2023 13:03:08 +0900 Subject: [PATCH 07/10] Use sequentially consistent for handlers tripped --- Modules/signalmodule.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/signalmodule.c b/Modules/signalmodule.c index 351a937d8215e0..13ce911f68c441 100644 --- a/Modules/signalmodule.c +++ b/Modules/signalmodule.c @@ -266,7 +266,7 @@ report_wakeup_send_error(void* data) static void trip_signal(int sig_num) { - _Py_atomic_store_int_relaxed(&Handlers[sig_num].tripped, 1); + _Py_atomic_store_int(&Handlers[sig_num].tripped, 1); /* Set is_tripped after setting .tripped, as it gets cleared in PyErr_CheckSignals() before .tripped. */ From 0750bbbbb6676a225f4037624b2fbd6504549f72 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Mon, 9 Oct 2023 09:33:52 +0900 Subject: [PATCH 08/10] Address code review --- Include/internal/pycore_signal.h | 3 --- Modules/signalmodule.c | 2 ++ 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/Include/internal/pycore_signal.h b/Include/internal/pycore_signal.h index e705249552d024..f197fb47b8143e 100644 --- a/Include/internal/pycore_signal.h +++ b/Include/internal/pycore_signal.h @@ -39,9 +39,6 @@ PyAPI_FUNC(void) _Py_RestoreSignals(void); struct _signals_runtime_state { struct { int tripped; - /* func is atomic to ensure that PyErr_SetInterrupt is async-signal-safe - * (even though it would probably be otherwise, anyway). - */ PyObject* func; } handlers[Py_NSIG]; diff --git a/Modules/signalmodule.c b/Modules/signalmodule.c index 13ce911f68c441..b8e9ff08833ddf 100644 --- a/Modules/signalmodule.c +++ b/Modules/signalmodule.c @@ -129,6 +129,8 @@ get_handler(int i) Py_LOCAL_INLINE(void) set_handler(int i, PyObject* func) { + /* Store func with atomic operation to ensure + that PyErr_SetInterrupt is async-signal-safe. */ _Py_atomic_store_ptr(&Handlers[i].func, func); } From 4d6e5ea85b36b123522d444add58cd9d39827c12 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Mon, 9 Oct 2023 18:41:25 +0900 Subject: [PATCH 09/10] Add comment --- Include/internal/pycore_signal.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Include/internal/pycore_signal.h b/Include/internal/pycore_signal.h index f197fb47b8143e..5a7e65fe9727eb 100644 --- a/Include/internal/pycore_signal.h +++ b/Include/internal/pycore_signal.h @@ -38,6 +38,7 @@ PyAPI_FUNC(void) _Py_RestoreSignals(void); struct _signals_runtime_state { struct { + // tripped and func should be accessed by using atomic ops. int tripped; PyObject* func; } handlers[Py_NSIG]; @@ -59,7 +60,8 @@ struct _signals_runtime_state { #endif } wakeup; - /* Speed up sigcheck() when none tripped */ + /* Speed up sigcheck() when none tripped + and is_tripped should be accessed by using atomic ops. */ int is_tripped; /* These objects necessarily belong to the main interpreter. */ From 3610ae674a5faa88183e31653e7d539891cc5bba Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Mon, 9 Oct 2023 21:28:32 +0900 Subject: [PATCH 10/10] Apply suggestions from code review Co-authored-by: Antoine Pitrou --- Include/internal/pycore_signal.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Include/internal/pycore_signal.h b/Include/internal/pycore_signal.h index 5a7e65fe9727eb..47213a34ab77b5 100644 --- a/Include/internal/pycore_signal.h +++ b/Include/internal/pycore_signal.h @@ -38,7 +38,7 @@ PyAPI_FUNC(void) _Py_RestoreSignals(void); struct _signals_runtime_state { struct { - // tripped and func should be accessed by using atomic ops. + // tripped and func should be accessed using atomic ops. int tripped; PyObject* func; } handlers[Py_NSIG]; @@ -60,8 +60,8 @@ struct _signals_runtime_state { #endif } wakeup; - /* Speed up sigcheck() when none tripped - and is_tripped should be accessed by using atomic ops. */ + /* Speed up sigcheck() when none tripped. + is_tripped should be accessed using atomic ops. */ int is_tripped; /* These objects necessarily belong to the main interpreter. */