Re: Review of patch that uses "volatile sig_atomic_t"

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 <theraven@FreeBSD.org> wrote:
> 
> On 2 Aug 2023, at 00:33, Rick Macklem <rick.macklem@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
> 
>