Review usage of atomic variables in signamodule.c

[vstinner]

BPO	42767
Nosy	@vstinner, @ammaraskar, @corona10, @shihai1991

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields: ```python assignee = None closed_at = None created_at = labels = ['interpreter-core', '3.10'] title = 'Review usage of atomic variables in signamodule.c' updated_at = user = 'https://github.com/vstinner' ``` bugs.python.org fields: ```python activity = actor = 'ammar2' assignee = 'none' closed = False closed_date = None closer = None components = ['Interpreter Core'] creation = creator = 'vstinner' dependencies = [] files = [] hgrepos = [] issue_num = 42767 keywords = [] message_count = 2.0 messages = ['383901', '383923'] nosy_count = 4.0 nosy_names = ['vstinner', 'ammar2', 'corona10', 'shihai1991'] pr_nums = [] priority = 'normal' resolution = None stage = None status = 'open' superseder = None type = None url = 'https://bugs.python.org/issue42767' versions = ['Python 3.10'] ```

[vstinner]

In bpo-41713, I ported the _signal module to the multi-phase initialization API. I tried to move the signal state into a structure to cleanup the code, but I was scared by the following atomic variables: ---------------

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

#ifdef MS_WINDOWS
#define INVALID_FD ((SOCKET_T)-1)

static volatile struct {
    SOCKET_T fd;
    int warn_on_full_buffer;
    int use_send;
} wakeup = {.fd = INVALID_FD, .warn_on_full_buffer = 1, .use_send = 0};
#else
#define INVALID_FD (-1)
static volatile struct {
#ifdef __VXWORKS__
    int fd;
#else
    sig_atomic_t fd;
#endif
    int warn_on_full_buffer;
} wakeup = {.fd = INVALID_FD, .warn_on_full_buffer = 1};
#endif

/ Speed up sigcheck() when none tripped \/ static _Py_atomic_int is_tripped; ---------------

For me, the most surprising part is Handlers[signum].tripped which is declared as volatile *and* an atomic variable.

I'm not sure if Handlers[signum].func must be volatile neither.

Also, wakeup.fd is declared as sig_atomic_t on Unix. Could we use an atomic variable instead, or is it important to use "sig_atomic_t"?

--

I recently added pycore_atomic_funcs.h which provides functions to access variables atomically. It uses atomic functions if available, or falls back on "volatile" otherwise. Maybe this approach would be interesting here, maybe for Handlers[signum].func?

[ammaraskar]

For me, the most surprising part is Handlers[signum].tripped which is declared as volatile *and* an atomic variable.

I think it's just following this part of the C spec (Some background in bpo-12060):

or refers to any object with static storage duration other than by assigning a value to a static storage duration variable of type volatile sig_atomic_t. Furthermore, if such a call fails, the value of errno is unspecified.

https://pubs.opengroup.org/onlinepubs/007904875/functions/sigaction.html / https://wiki.sei.cmu.edu/confluence/display/c/SIG31-C.+Do+not+access+shared+objects+in+signal+handlers

I'm not sure if Handlers[signum].func must be volatile neither.

I think your assessment here is right considering it's never used in the signal handler itself.

Also, wakeup.fd is declared as sig_atomic_t on Unix. Could we use an atomic variable instead, or is it important to use "sig_atomic_t"?

Again looking at the https://wiki.sei.cmu.edu/confluence/display/c/SIG31-C.+Do+not+access+shared+objects+in+signal+handlers recommendation it seems like using an atomic variable is fine but only if it's lock-free.

[vstinner]

I don't have the bandwidth to work on this issue, so I just close it.