Hi,
This bit of the C spec is a bit of a mess. There was, I believe, a desire to
return volatile to its original use and make any use of volatile other than
MMIO discouraged. This broke too much legacy code and so now it’s a confusing
state.
The requirements for volatile are that the compiler must not elide loads or
stores and may not narrow them (I am not sure if it’s allowed to widen them).
Loads and stores to a volatile variable may not be reordered with respect to
other loads or stores to the same object but *may* be reordered with respect to
any other accesses.
The sig_atomic_t typedef just indicates an integer type that can be loaded and
stored with a single instruction and so is immune to tearing if updated from a
signal handler. There is no requirement to use this from signal handlers in
preference to int on FreeBSD (whether other types work is implementation
defined and int works on all supported architectures for us).
The weak ordering guarantees for volatile mean that any code using volatile for
detecting whether a signal has fired is probably wrong if if does not include a
call to automic_signal_fence(). This guarantees that the compiler will not
reorder the load of the volatile with respect to other accesses. In practice,
compilers tend to be fairly conservative about reordering volatile accesses and
so it probably won’t break until you upgrade your compiler in a few years time.
My general recommendation is to use _Atomic(int) (or ideally a enum type) for
this. If you just use it like a normal int, you will get sequentially
consistent atomics. On a weakly ordered platform like Arm this will include
some more atomic barrier instructions but it will do the right thing if you add
additional threads monitoring the same variable later. In something like
mountd, the extra performance overhead from the barriers is unlikely to be
measurable, if it is then you can weaken the atomicity (sequentially consistent
unless specified otherwise is a good default in C/C++, for once prioritising
correctness over performance).
David
> On 1 Aug 2023, at 06:14, Rick Macklem wrote:
>
> Hi,
>
> I just put D41265 up on phabricator. It is a trivial
> change to mountd.c that defines the variable set
> by got_sighup() (the SIGHUP handler) as
> static volatile sig_atomic_t
> instead of
>static int
>
> I did list a couple of reviewers, but if you are familiar
> with this C requirement, please take a look at it and
> review it.
>
> Thanks, rick
> ps: I was unaware of this C requirement until Peter Eriksson
> reported it to me yesterday. Several of the other NFS
> related daemons probably need to same fix, which I will
> do after this is reviewed.
>
