Interesting discussion regarding sig_atomic_t, volatile & stuff. It seems I opened a can of worms. Sorry :-)
Anyway here are the references I had read when suggesting this change: C89: - http://port70.net/~nsz/c/c99/n1256.html#7.14p2 CERT C Coding Standard: - https://wiki.sei.cmu.edu/confluence/display/c/SIG31-C.+Do+not+access+shared+objects+in+signal+handlers I’ve also been using "static int foo;” in my own code forever before and like you say it works but then I got curious and started searching to see if there was a more “correct” (and portable) way of doing this) If I read you right then just dropping “volatile” and use “sig_atomic_t” should be better (and “portable” for some value of portable, not a problem in this case with mountd) but using atomic_int from stdatomic.h from C11 is even better? (volatile sig_atomic_t is from C89 I think). Something like this: > #if __STDC_NO_ATOMICS__ != 1 > /* C11 */ > #include <stdatomic.h> > static atomic_int got_sighup = ATOMIC_VAR_INIT(0); > > #else > /* C89 */ > static volatile sig_atomic_t got_sighup = 0; > #endif (for portability, not needed for mountd though). https://www.informit.com/articles/article.aspx?p=2204014 Ah well, time to relearn C again :-) (K&R C was simpler :-) - Peter > On 2 Aug 2023, at 10:12, David Chisnall <thera...@freebsd.org> wrote: > > On 2 Aug 2023, at 00:33, Rick Macklem <rick.mack...@gmail.com> wrote: >> >> Just trying to understand what you are suggesting... >> 1 - Declare the variable _Atomic(int) OR atomic_int (is there a preference) >> and >> not volatile. > > Either is fine (the latter is a typedef for the former). I am not a huge fan > of the typedefs, some people like them, as far as I can tell it’s purely > personal preference. > >> 2 - Is there a need for signal_atomic_fence(memory_order_acquire); before the >> assignment of the variable in the signal handler. (This exists in >> one place in >> the source tree (bin/dd/misc,c), although for this example, >> neither volatile nor >> _Atomic() are used for the variable's declaration. > > You don’t need a fence if you use an atomic variable. The fence prevents the > compiler reordering things across it, using atomic operations also prevents > this. You might be able to use a fence and not use an atomic but I’d have to > reread the spec very carefully to convince myself that this didn’t trigger > undefined behaviour. > >> 3 - Is there any need for other atomic_XXX() calls where the variable is used >> outside of the signal handler? > > No. By default, _Atomic variables use sequentially consistent semantics. > You need to use the `atomic_` functions only for explicit memory orderings, > which you might want to do for optimisation (very unlikely in this case). > Reading it outside the signal handler is the equivalent of doing > `atomic_load` with a sequentially consistent memory order. This is a > stronger guarantee than you need, but it’s unlikely to cause performance > problems if you’re doing more than a few dozen instructions worth of work > between checks. > >> In general, it is looking like FreeBSD needs to have a standard way of >> dealing >> with this and there will be assorted places that need to be fixed? > > If we used a language that let you build abstractions, that would be easy (I > have a C++ class that provides a static epoch counter that’s incremented in a > signal handler and a local copy for each instance, so you can check if the > signal handler has fired since it was last used. It’s trivial to reuse in > C++ projects but C doesn’t give you tools for doing this. > > David > >
