gh-109693: Use pyatomic.h for signal module

[corona10]

• Issue: Clean-up pyatomic headers #109693

[corona10]

!buildbot nogil

[bedevere-bot]

The regex 'nogil' did not match any buildbot builder. Is the requested builder in the list of stable builders?

[colesbury]

@corona10, unfortunately you can't currently request the "nogil" buildbots on a PR because they're marked as unstable and the "!buildbot" command only works for stable buildbots. I think Itamar is working on getting a nogil GitHub actions or similar to run on PRs.

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @corona10 for commit a747442 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

[corona10]

I think Itamar is working on getting a nogil GitHub actions or similar to run on PRs.

Okay got it!

[colesbury]

LGTM

[vstinner]

In Python 2.7, signamodule.c used:

static struct {
    int tripped;
    PyObject *func;
} Handlers[NSIG];

static sig_atomic_t wakeup_fd = -1;
    
/* Speed up sigcheck() when none tripped */
static volatile sig_atomic_t is_tripped = 0;

Handlers[NSIG].tripped was not atomic.

---

The latest major change was commit 57e836c of issue gh-74991 in 2017 to use atomic types:

 static volatile struct {
-    sig_atomic_t tripped;
+    _Py_atomic_int tripped;
     PyObject *func;
 } Handlers[NSIG];
 
 /* Speed up sigcheck() when none tripped */
-static volatile sig_atomic_t is_tripped = 0;
+static _Py_atomic_int is_tripped;

and:

@@ -240,11 +240,13 @@ trip_signal(int sig_num)
     int fd;
     Py_ssize_t rc;
 
-    Handlers[sig_num].tripped = 1;
+    _Py_atomic_store_relaxed(&Handlers[sig_num].tripped, 1);
 
     /* Set is_tripped after setting .tripped, as it gets
        cleared in PyErr_CheckSignals() before .tripped. */
-    is_tripped = 1;
+    _Py_atomic_store(&is_tripped, 1);

[vstinner]

I expected trip_signal() and _PyErr_CheckSignalsTstate() to be the same about memory order, but trip_signal() is stricter (seq cst) when it sets "tripped" member, whereas _PyErr_CheckSignalsTstate() uses "relaxed" to get "tripped". Would you mind to explain a comment to explain why "seq cst" is needed to access "tripped" and "is_tripped"?

[corona10]

Would you mind to explain a comment to explain why "seq cst" is needed to access "tripped" and "is_tripped"?

cpython/Modules/signalmodule.c

Lines 1810 to 1815 in 7e30821

	for (int i = 1; i < Py_NSIG; i++) {
	if (!_Py_atomic_load_relaxed(&Handlers[i].tripped)) {
	continue;
	}
	_Py_atomic_store_relaxed(&Handlers[i].tripped, 0);

Relaxed order always has a possibility of changes for the efficiency of the CPU pipeline by the compiler if the compiler thinks that it has no dependency between multiple atomic operations, so it only guarantees the single atomic operation itself.
But _PyErr_CheckSignalsTstate case, as the attached code lines, two atomic operations have the dependency
load &Handlers[i].tripped to store &Handlers[i].tripped so in this case compiler will not change the order so it is safe.

[pitrou]

Since the _Py_atomic types are not used anymore, can you add comments mentioning that these fields must be accessed using atomic ops?

[corona10]

Done!

[vstinner]

Thanks @corona10 for additional comments :-)