[PATCH] signal handlers: volatile sigatomic_t, not volatile OR sigatomic_t

[PATCH] signal handlers: volatile sigatomic_t, not volatile OR sigatomic_t

[Guilherme Janczak]

Variables shared with signal handlers must be of type `volatile
sigatomic_t`, not `volatile` or `sigatomic_t` as the current text says,
according to a C11 draft:

    When ... interrupted by ... a signal, values of objects that are
    neither lock-free atomic objects nor of type volatile sig_atomic_t
    are unspecified.

Ref: https://www.iso-9899.info/n1570.html#5.1.2.3p5
Signed-off-by: Guilherme Janczak <guilherme.janczak@yandex.com>
---
 memorder/memorder.tex | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/memorder/memorder.tex b/memorder/memorder.tex
index 5c50d42d..873c3424 100644
--- a/memorder/memorder.tex
+++ b/memorder/memorder.tex
@@ -1317,8 +1317,8 @@ from the viewpoint of the interrupted thread, at least at the
 assembly-language level.
 However, the C and C++ languages do not define the results of handlers
 and interrupted threads sharing plain variables.
-Instead, such shared variables must be \co{sig_atomic_t}, lock-free
-atomics, or \co{volatile}.
+Instead, such shared variables must be \co{volatile sig_atomic_t} or
+lock-free atomics.
 
 On the other hand, because the handler executes within the interrupted
 thread's context, the memory ordering used to synchronize communication
-- 
2.42.0

Re: [PATCH] signal handlers: volatile sigatomic_t, not volatile OR sigatomic_t

[Elad Lahav]

Do you really need volatile?
There are two cases to consider. Either your code synchronizes updates
to the shared value with the signal handler (e.g., by blocking and
then unblocking the signal), in which case I believe the compiler
cannot ignore updates to the value; or you don't, and you can't depend
on the variable having any specific value in the signal handler. The
only thing you want to prevent in the latter case is the handler
observing a partial update to the variable, which I presume is where
the other requirements originate. (In practice, there should be little
or no concern with any primitive type on modern hardware).

--Elad

On Wed, Mar 20, 2024 at 4:32 PM Guilherme Janczak
<guilherme.janczak@yandex.com> wrote:
>
> Variables shared with signal handlers must be of type `volatile
> sigatomic_t`, not `volatile` or `sigatomic_t` as the current text says,
> according to a C11 draft:
>
>     When ... interrupted by ... a signal, values of objects that are
>     neither lock-free atomic objects nor of type volatile sig_atomic_t
>     are unspecified.
>
> Ref: https://www.iso-9899.info/n1570.html#5.1.2.3p5
> Signed-off-by: Guilherme Janczak <guilherme.janczak@yandex.com>
> ---
>  memorder/memorder.tex | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/memorder/memorder.tex b/memorder/memorder.tex
> index 5c50d42d..873c3424 100644
> --- a/memorder/memorder.tex
> +++ b/memorder/memorder.tex
> @@ -1317,8 +1317,8 @@ from the viewpoint of the interrupted thread, at least at the
>  assembly-language level.
>  However, the C and C++ languages do not define the results of handlers
>  and interrupted threads sharing plain variables.
> -Instead, such shared variables must be \co{sig_atomic_t}, lock-free
> -atomics, or \co{volatile}.
> +Instead, such shared variables must be \co{volatile sig_atomic_t} or
> +lock-free atomics.
>
>  On the other hand, because the handler executes within the interrupted
>  thread's context, the memory ordering used to synchronize communication
> --
> 2.42.0
>
>

Re: [PATCH] signal handlers: volatile sigatomic_t, not volatile OR sigatomic_t

[Guilherme Janczak]

The standard says it's necessary.
If you have a `volatile long long` on a 32-bit architecture, the
compiler will have to compile it to some bignum code (meaning multiple
instructions), and the signal can come in between them.

I can quickly make it happen on a Nintendo Wii (32-bit powerpc) as
well as an i386 laptop using this test program:
```
#define _POSIX_C_SOURCE 200809L

#include <sys/time.h>

#include <stdio.h>
#include <stdlib.h>
#include <signal.h>
#include <string.h>

static void catch_sigalrm(int);

volatile unsigned long long longword;
volatile sig_atomic_t partial_update;

int
main(void)
{
	const struct sigaction act = { .sa_handler = catch_sigalrm };
	const struct itimerval timer = { 
		.it_value.tv_usec = 1, 
		.it_interval.tv_usec = 1,
	};
	unsigned long long n;

	sigaction(SIGALRM, &act, NULL);
	setitimer(ITIMER_REAL, &timer, NULL);
	for (unsigned char i = 0;; i++) {
		memset(&n, i, sizeof(n));
		longword = n;
		if (partial_update) {
			fprintf(stderr, "longword partially updated\n");
			exit(1);
		}
	}
}

static void
catch_sigalrm(int unused)
{
	union multibyte {
		long long n;
		unsigned char bytes[sizeof(long long)];
	} window;
	(void)unused;	
	window.n = longword;

	for (size_t i = 1; i < sizeof(window.bytes); i++) {
		if (window.bytes[i] != window.bytes[0])
			partial_update = 1;
	}
}
```

Output:
```
$ cc -O2 test.c && ./a.out
longword partially updated
```

The program runs (apparently) forever on my amd64 desktop.

If you look at the powerpc assembly version of the program in Godbolt:
https://godbolt.org/z/Pc8q7E5ej
Lines 69 and 70 of the assembly use 2 STW instructions to store each 
32-bit half of the bignum.

On Wed, Mar 20, 2024 at 04:44:33PM -0400, Elad Lahav wrote:
> Do you really need volatile?
> There are two cases to consider. Either your code synchronizes updates
> to the shared value with the signal handler (e.g., by blocking and
> then unblocking the signal), in which case I believe the compiler
> cannot ignore updates to the value; or you don't, and you can't depend
> on the variable having any specific value in the signal handler. The
> only thing you want to prevent in the latter case is the handler
> observing a partial update to the variable, which I presume is where
> the other requirements originate. (In practice, there should be little
> or no concern with any primitive type on modern hardware).
> 
> --Elad
> 
> On Wed, Mar 20, 2024 at 4:32 PM Guilherme Janczak
> <guilherme.janczak@yandex.com> wrote:
> >
> > Variables shared with signal handlers must be of type `volatile
> > sigatomic_t`, not `volatile` or `sigatomic_t` as the current text says,
> > according to a C11 draft:
> >
> >     When ... interrupted by ... a signal, values of objects that are
> >     neither lock-free atomic objects nor of type volatile sig_atomic_t
> >     are unspecified.
> >
> > Ref: https://www.iso-9899.info/n1570.html#5.1.2.3p5
> > Signed-off-by: Guilherme Janczak <guilherme.janczak@yandex.com>
> > ---
> >  memorder/memorder.tex | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/memorder/memorder.tex b/memorder/memorder.tex
> > index 5c50d42d..873c3424 100644
> > --- a/memorder/memorder.tex
> > +++ b/memorder/memorder.tex
> > @@ -1317,8 +1317,8 @@ from the viewpoint of the interrupted thread, at least at the
> >  assembly-language level.
> >  However, the C and C++ languages do not define the results of handlers
> >  and interrupted threads sharing plain variables.
> > -Instead, such shared variables must be \co{sig_atomic_t}, lock-free
> > -atomics, or \co{volatile}.
> > +Instead, such shared variables must be \co{volatile sig_atomic_t} or
> > +lock-free atomics.
> >
> >  On the other hand, because the handler executes within the interrupted
> >  thread's context, the memory ordering used to synchronize communication
> > --
> > 2.42.0
> >
> >

Re: [PATCH] signal handlers: volatile sigatomic_t, not volatile OR sigatomic_t

[Paul E. McKenney]

On Wed, Mar 20, 2024 at 08:25:23PM +0000, Guilherme Janczak wrote:
> Variables shared with signal handlers must be of type `volatile
> sigatomic_t`, not `volatile` or `sigatomic_t` as the current text says,
> according to a C11 draft:
> 
>     When ... interrupted by ... a signal, values of objects that are
>     neither lock-free atomic objects nor of type volatile sig_atomic_t
>     are unspecified.
> 
> Ref: https://www.iso-9899.info/n1570.html#5.1.2.3p5
> Signed-off-by: Guilherme Janczak <guilherme.janczak@yandex.com>
> ---
>  memorder/memorder.tex | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/memorder/memorder.tex b/memorder/memorder.tex
> index 5c50d42d..873c3424 100644
> --- a/memorder/memorder.tex
> +++ b/memorder/memorder.tex
> @@ -1317,8 +1317,8 @@ from the viewpoint of the interrupted thread, at least at the
>  assembly-language level.
>  However, the C and C++ languages do not define the results of handlers
>  and interrupted threads sharing plain variables.
> -Instead, such shared variables must be \co{sig_atomic_t}, lock-free
> -atomics, or \co{volatile}.
> +Instead, such shared variables must be \co{volatile sig_atomic_t} or
> +lock-free atomics.

You might be right about "sig_atomic_t" needing to also be "volatile"
(I will let you and Elad settle your differences on this one), but if
device drivers are to work correctly, then volatile variables that are
machine-word sized and aligned must necessarily work for communication
between process-level code and interrupt/signal handlers.

							Thanx, Paul

>  On the other hand, because the handler executes within the interrupted
>  thread's context, the memory ordering used to synchronize communication
> -- 
> 2.42.0
> 
> 

Re: [PATCH] signal handlers: volatile sigatomic_t, not volatile OR sigatomic_t

[Elad Lahav]

No problem...
Yes, in your example the issue is that the type is not atomic, and
thus subject to partial updates that can be interrupted.
Looking at the example given in
https://en.cppreference.com/w/c/program/sig_atomic_t the volatile is
needed to let the compiler know that the value can be updated in
between two reads by the main function. That makes sense, especially
if you have code that loops waiting for the value to change.

--Elad

On Wed, Mar 20, 2024 at 6:39 PM Guilherme Janczak
<guilherme.janczak@yandex.com> wrote:
>
> Actually, uh, I misread your reply, forget the previous reply I sent.
>
> You don't need the volatile with lock-free atomics, but the standard
> says you do need it with sig_atomic_t. I don't know of a case that would
> break a plain `sig_atomic_t` variable with no `volatile`, however.
>
> On Wed, Mar 20, 2024 at 04:44:33PM -0400, Elad Lahav wrote:
> > Do you really need volatile?
> > There are two cases to consider. Either your code synchronizes updates
> > to the shared value with the signal handler (e.g., by blocking and
> > then unblocking the signal), in which case I believe the compiler
> > cannot ignore updates to the value; or you don't, and you can't depend
> > on the variable having any specific value in the signal handler. The
> > only thing you want to prevent in the latter case is the handler
> > observing a partial update to the variable, which I presume is where
> > the other requirements originate. (In practice, there should be little
> > or no concern with any primitive type on modern hardware).
> >
> > --Elad
> >
> > On Wed, Mar 20, 2024 at 4:32 PM Guilherme Janczak
> > <guilherme.janczak@yandex.com> wrote:
> > >
> > > Variables shared with signal handlers must be of type `volatile
> > > sigatomic_t`, not `volatile` or `sigatomic_t` as the current text says,
> > > according to a C11 draft:
> > >
> > >     When ... interrupted by ... a signal, values of objects that are
> > >     neither lock-free atomic objects nor of type volatile sig_atomic_t
> > >     are unspecified.
> > >
> > > Ref: https://www.iso-9899.info/n1570.html#5.1.2.3p5
> > > Signed-off-by: Guilherme Janczak <guilherme.janczak@yandex.com>
> > > ---
> > >  memorder/memorder.tex | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/memorder/memorder.tex b/memorder/memorder.tex
> > > index 5c50d42d..873c3424 100644
> > > --- a/memorder/memorder.tex
> > > +++ b/memorder/memorder.tex
> > > @@ -1317,8 +1317,8 @@ from the viewpoint of the interrupted thread, at least at the
> > >  assembly-language level.
> > >  However, the C and C++ languages do not define the results of handlers
> > >  and interrupted threads sharing plain variables.
> > > -Instead, such shared variables must be \co{sig_atomic_t}, lock-free
> > > -atomics, or \co{volatile}.
> > > +Instead, such shared variables must be \co{volatile sig_atomic_t} or
> > > +lock-free atomics.
> > >
> > >  On the other hand, because the handler executes within the interrupted
> > >  thread's context, the memory ordering used to synchronize communication
> > > --
> > > 2.42.0
> > >
> > >
> >

Re: [PATCH] signal handlers: volatile sigatomic_t, not volatile OR sigatomic_t

[Paul E. McKenney]

On Wed, Mar 20, 2024 at 10:26:04PM +0000, Guilherme Janczak wrote:
> The standard says it's necessary.
> If you have a `volatile long long` on a 32-bit architecture, the
> compiler will have to compile it to some bignum code (meaning multiple
> instructions), and the signal can come in between them.

Agreed, because in this case the variable is not machine-word sized.

What happens if you instead use a 32-bit aligned volatile quantity?
(Or 16 or 8 bits, for that matter, at least on PowerPC [1].)

I would hope that these smaller sizes would work, otherwise, again,
device drivers become problematic on that platform.

							Thanx, Paul

[1] There have been some platforms without 16-bit or 8-bit loads, 
    and on such platforms, you could also see failures of one sort
    or another.

> I can quickly make it happen on a Nintendo Wii (32-bit powerpc) as
> well as an i386 laptop using this test program:
> ```
> #define _POSIX_C_SOURCE 200809L
> 
> #include <sys/time.h>
> 
> #include <stdio.h>
> #include <stdlib.h>
> #include <signal.h>
> #include <string.h>
> 
> static void catch_sigalrm(int);
> 
> volatile unsigned long long longword;
> volatile sig_atomic_t partial_update;
> 
> int
> main(void)
> {
> 	const struct sigaction act = { .sa_handler = catch_sigalrm };
> 	const struct itimerval timer = { 
> 		.it_value.tv_usec = 1, 
> 		.it_interval.tv_usec = 1,
> 	};
> 	unsigned long long n;
> 
> 	sigaction(SIGALRM, &act, NULL);
> 	setitimer(ITIMER_REAL, &timer, NULL);
> 	for (unsigned char i = 0;; i++) {
> 		memset(&n, i, sizeof(n));
> 		longword = n;
> 		if (partial_update) {
> 			fprintf(stderr, "longword partially updated\n");
> 			exit(1);
> 		}
> 	}
> }
> 
> static void
> catch_sigalrm(int unused)
> {
> 	union multibyte {
> 		long long n;
> 		unsigned char bytes[sizeof(long long)];
> 	} window;
> 	(void)unused;	
> 	window.n = longword;
> 
> 	for (size_t i = 1; i < sizeof(window.bytes); i++) {
> 		if (window.bytes[i] != window.bytes[0])
> 			partial_update = 1;
> 	}
> }
> ```
> 
> Output:
> ```
> $ cc -O2 test.c && ./a.out
> longword partially updated
> ```
> 
> The program runs (apparently) forever on my amd64 desktop.
> 
> If you look at the powerpc assembly version of the program in Godbolt:
> https://godbolt.org/z/Pc8q7E5ej
> Lines 69 and 70 of the assembly use 2 STW instructions to store each 
> 32-bit half of the bignum.
> 
> On Wed, Mar 20, 2024 at 04:44:33PM -0400, Elad Lahav wrote:
> > Do you really need volatile?
> > There are two cases to consider. Either your code synchronizes updates
> > to the shared value with the signal handler (e.g., by blocking and
> > then unblocking the signal), in which case I believe the compiler
> > cannot ignore updates to the value; or you don't, and you can't depend
> > on the variable having any specific value in the signal handler. The
> > only thing you want to prevent in the latter case is the handler
> > observing a partial update to the variable, which I presume is where
> > the other requirements originate. (In practice, there should be little
> > or no concern with any primitive type on modern hardware).
> > 
> > --Elad
> > 
> > On Wed, Mar 20, 2024 at 4:32 PM Guilherme Janczak
> > <guilherme.janczak@yandex.com> wrote:
> > >
> > > Variables shared with signal handlers must be of type `volatile
> > > sigatomic_t`, not `volatile` or `sigatomic_t` as the current text says,
> > > according to a C11 draft:
> > >
> > >     When ... interrupted by ... a signal, values of objects that are
> > >     neither lock-free atomic objects nor of type volatile sig_atomic_t
> > >     are unspecified.
> > >
> > > Ref: https://www.iso-9899.info/n1570.html#5.1.2.3p5
> > > Signed-off-by: Guilherme Janczak <guilherme.janczak@yandex.com>
> > > ---
> > >  memorder/memorder.tex | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/memorder/memorder.tex b/memorder/memorder.tex
> > > index 5c50d42d..873c3424 100644
> > > --- a/memorder/memorder.tex
> > > +++ b/memorder/memorder.tex
> > > @@ -1317,8 +1317,8 @@ from the viewpoint of the interrupted thread, at least at the
> > >  assembly-language level.
> > >  However, the C and C++ languages do not define the results of handlers
> > >  and interrupted threads sharing plain variables.
> > > -Instead, such shared variables must be \co{sig_atomic_t}, lock-free
> > > -atomics, or \co{volatile}.
> > > +Instead, such shared variables must be \co{volatile sig_atomic_t} or
> > > +lock-free atomics.
> > >
> > >  On the other hand, because the handler executes within the interrupted
> > >  thread's context, the memory ordering used to synchronize communication
> > > --
> > > 2.42.0
> > >
> > >
> 

Re: [PATCH] signal handlers: volatile sigatomic_t, not volatile OR sigatomic_t

[Elad Lahav]

Actually, in the example I cited, there are multiple function calls
(printf, raise) between the two reads. How can the compiler optimize
out reading the value a second time?

My view, which is subject to change at any moment and without notice,
is that you need volatile in the cases where you normally need
volatile, rather than inherently whenever you use sig_atomic_t. In the
following example I would not expect you to need it:

static sig_atomic_t value;

void
sig_handler(int signum)
{
    value++;
}

void
func(void)
{
    value = 0;
    sleep(1);
    printf("There have been %d signals while I was napping\n", value);
}

--Elad

On Wed, Mar 20, 2024 at 7:05 PM Elad Lahav <e2lahav@gmail.com> wrote:
>
> No problem...
> Yes, in your example the issue is that the type is not atomic, and
> thus subject to partial updates that can be interrupted.
> Looking at the example given in
> https://en.cppreference.com/w/c/program/sig_atomic_t the volatile is
> needed to let the compiler know that the value can be updated in
> between two reads by the main function. That makes sense, especially
> if you have code that loops waiting for the value to change.
>
> --Elad
>
> On Wed, Mar 20, 2024 at 6:39 PM Guilherme Janczak
> <guilherme.janczak@yandex.com> wrote:
> >
> > Actually, uh, I misread your reply, forget the previous reply I sent.
> >
> > You don't need the volatile with lock-free atomics, but the standard
> > says you do need it with sig_atomic_t. I don't know of a case that would
> > break a plain `sig_atomic_t` variable with no `volatile`, however.
> >
> > On Wed, Mar 20, 2024 at 04:44:33PM -0400, Elad Lahav wrote:
> > > Do you really need volatile?
> > > There are two cases to consider. Either your code synchronizes updates
> > > to the shared value with the signal handler (e.g., by blocking and
> > > then unblocking the signal), in which case I believe the compiler
> > > cannot ignore updates to the value; or you don't, and you can't depend
> > > on the variable having any specific value in the signal handler. The
> > > only thing you want to prevent in the latter case is the handler
> > > observing a partial update to the variable, which I presume is where
> > > the other requirements originate. (In practice, there should be little
> > > or no concern with any primitive type on modern hardware).
> > >
> > > --Elad
> > >
> > > On Wed, Mar 20, 2024 at 4:32 PM Guilherme Janczak
> > > <guilherme.janczak@yandex.com> wrote:
> > > >
> > > > Variables shared with signal handlers must be of type `volatile
> > > > sigatomic_t`, not `volatile` or `sigatomic_t` as the current text says,
> > > > according to a C11 draft:
> > > >
> > > >     When ... interrupted by ... a signal, values of objects that are
> > > >     neither lock-free atomic objects nor of type volatile sig_atomic_t
> > > >     are unspecified.
> > > >
> > > > Ref: https://www.iso-9899.info/n1570.html#5.1.2.3p5
> > > > Signed-off-by: Guilherme Janczak <guilherme.janczak@yandex.com>
> > > > ---
> > > >  memorder/memorder.tex | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/memorder/memorder.tex b/memorder/memorder.tex
> > > > index 5c50d42d..873c3424 100644
> > > > --- a/memorder/memorder.tex
> > > > +++ b/memorder/memorder.tex
> > > > @@ -1317,8 +1317,8 @@ from the viewpoint of the interrupted thread, at least at the
> > > >  assembly-language level.
> > > >  However, the C and C++ languages do not define the results of handlers
> > > >  and interrupted threads sharing plain variables.
> > > > -Instead, such shared variables must be \co{sig_atomic_t}, lock-free
> > > > -atomics, or \co{volatile}.
> > > > +Instead, such shared variables must be \co{volatile sig_atomic_t} or
> > > > +lock-free atomics.
> > > >
> > > >  On the other hand, because the handler executes within the interrupted
> > > >  thread's context, the memory ordering used to synchronize communication
> > > > --
> > > > 2.42.0
> > > >
> > > >
> > >

Re: [PATCH] signal handlers: volatile sigatomic_t, not volatile OR sigatomic_t

[Guilherme Janczak]

In the case in your example, sleep() doesn't need to be implemented
using SIGALRM. It isn't in glibc and OpenBSD that I know of:
https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/posix/sleep.c;h=3df79097c4464510f0197138a00a0c9772a5e83e;hb=3ab9b88e2ac91062b6d493fe32bd101a55006c6a
https://man.openbsd.org/sleep.3

I managed to make GCC and Clang optimize out a sig_atomic_t store:
```
#define _POSIX_C_SOURCE 200809L

#include <sys/time.h>

#include <stdio.h>
#include <stdlib.h>
#include <signal.h>
#include <string.h>

static void catch_sigalrm(int);

#ifdef VOLATILE
volatile sig_atomic_t msg;
static const char *decl = "volatile sig_atomic_t msg";
#else
sig_atomic_t msg;
static const char *decl = "sig_atomic_t msg";
#endif

volatile sig_atomic_t reply;
volatile sig_atomic_t replied;

int
main(void)
{
	const struct sigaction act = { .sa_handler = catch_sigalrm };
	const struct itimerval timer = {
		.it_value.tv_usec = 1,
		.it_interval.tv_usec = 1,
	};
	sigaction(SIGALRM, &act, NULL);
	setitimer(ITIMER_REAL, &timer, NULL);
	while (!replied) {
		msg = 1;
	}
	printf("%s == %d\n", decl, (int)reply);
}

static void
catch_sigalrm(int unused)
{
	int n = msg;
	if (!n)
		return;
	reply = n;
	replied = 1;
}
```

This runs forever:
```
$ cc -O2 test2.c && ./a.out 
```
This terminates:
```
$ clang -O2 -DVOLATILE test2.c && ./a.out
volatile sig_atomic_t msg == 1
```

Without volatile in its declaration, msg is never set, so the program 
spins forever because the signal handler never tells the loop to stop. 
There's probably a simpler way to do this.

On Wed, Mar 20, 2024 at 07:20:03PM -0400, Elad Lahav wrote:
> Actually, in the example I cited, there are multiple function calls
> (printf, raise) between the two reads. How can the compiler optimize
> out reading the value a second time?
> 
> My view, which is subject to change at any moment and without notice,
> is that you need volatile in the cases where you normally need
> volatile, rather than inherently whenever you use sig_atomic_t. In the
> following example I would not expect you to need it:
> 
> static sig_atomic_t value;
> 
> void
> sig_handler(int signum)
> {
>     value++;
> }
> 
> void
> func(void)
> {
>     value = 0;
>     sleep(1);
>     printf("There have been %d signals while I was napping\n", value);
> }
> 
> --Elad
> 
> On Wed, Mar 20, 2024 at 7:05 PM Elad Lahav <e2lahav@gmail.com> wrote:
> >
> > No problem...
> > Yes, in your example the issue is that the type is not atomic, and
> > thus subject to partial updates that can be interrupted.
> > Looking at the example given in
> > https://en.cppreference.com/w/c/program/sig_atomic_t the volatile is
> > needed to let the compiler know that the value can be updated in
> > between two reads by the main function. That makes sense, especially
> > if you have code that loops waiting for the value to change.
> >
> > --Elad
> >
> > On Wed, Mar 20, 2024 at 6:39 PM Guilherme Janczak
> > <guilherme.janczak@yandex.com> wrote:
> > >
> > > Actually, uh, I misread your reply, forget the previous reply I sent.
> > >
> > > You don't need the volatile with lock-free atomics, but the standard
> > > says you do need it with sig_atomic_t. I don't know of a case that would
> > > break a plain `sig_atomic_t` variable with no `volatile`, however.
> > >
> > > On Wed, Mar 20, 2024 at 04:44:33PM -0400, Elad Lahav wrote:
> > > > Do you really need volatile?
> > > > There are two cases to consider. Either your code synchronizes updates
> > > > to the shared value with the signal handler (e.g., by blocking and
> > > > then unblocking the signal), in which case I believe the compiler
> > > > cannot ignore updates to the value; or you don't, and you can't depend
> > > > on the variable having any specific value in the signal handler. The
> > > > only thing you want to prevent in the latter case is the handler
> > > > observing a partial update to the variable, which I presume is where
> > > > the other requirements originate. (In practice, there should be little
> > > > or no concern with any primitive type on modern hardware).
> > > >
> > > > --Elad
> > > >
> > > > On Wed, Mar 20, 2024 at 4:32 PM Guilherme Janczak
> > > > <guilherme.janczak@yandex.com> wrote:
> > > > >
> > > > > Variables shared with signal handlers must be of type `volatile
> > > > > sigatomic_t`, not `volatile` or `sigatomic_t` as the current text says,
> > > > > according to a C11 draft:
> > > > >
> > > > >     When ... interrupted by ... a signal, values of objects that are
> > > > >     neither lock-free atomic objects nor of type volatile sig_atomic_t
> > > > >     are unspecified.
> > > > >
> > > > > Ref: https://www.iso-9899.info/n1570.html#5.1.2.3p5
> > > > > Signed-off-by: Guilherme Janczak <guilherme.janczak@yandex.com>
> > > > > ---
> > > > >  memorder/memorder.tex | 4 ++--
> > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/memorder/memorder.tex b/memorder/memorder.tex
> > > > > index 5c50d42d..873c3424 100644
> > > > > --- a/memorder/memorder.tex
> > > > > +++ b/memorder/memorder.tex
> > > > > @@ -1317,8 +1317,8 @@ from the viewpoint of the interrupted thread, at least at the
> > > > >  assembly-language level.
> > > > >  However, the C and C++ languages do not define the results of handlers
> > > > >  and interrupted threads sharing plain variables.
> > > > > -Instead, such shared variables must be \co{sig_atomic_t}, lock-free
> > > > > -atomics, or \co{volatile}.
> > > > > +Instead, such shared variables must be \co{volatile sig_atomic_t} or
> > > > > +lock-free atomics.
> > > > >
> > > > >  On the other hand, because the handler executes within the interrupted
> > > > >  thread's context, the memory ordering used to synchronize communication
> > > > > --
> > > > > 2.42.0
> > > > >
> > > > >
> > > >

Re: [PATCH] signal handlers: volatile sigatomic_t, not volatile OR sigatomic_t

[Elad Lahav]

Sorry, that's not at all what I meant. I used sleep() just to simulate
the main function doing something while signals arrive asynchronously.
What you gave in your example is exactly the situation that does
require volatile, per my previous email, as the main function
constantly polls the shared value without any function calls in reads.

--Elad

On Wed, Mar 20, 2024 at 8:34 PM Guilherme Janczak
<guilherme.janczak@yandex.com> wrote:
>
> In the case in your example, sleep() doesn't need to be implemented
> using SIGALRM. It isn't in glibc and OpenBSD that I know of:
> https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/posix/sleep.c;h=3df79097c4464510f0197138a00a0c9772a5e83e;hb=3ab9b88e2ac91062b6d493fe32bd101a55006c6a
> https://man.openbsd.org/sleep.3
>
> I managed to make GCC and Clang optimize out a sig_atomic_t store:
> ```
> #define _POSIX_C_SOURCE 200809L
>
> #include <sys/time.h>
>
> #include <stdio.h>
> #include <stdlib.h>
> #include <signal.h>
> #include <string.h>
>
> static void catch_sigalrm(int);
>
> #ifdef VOLATILE
> volatile sig_atomic_t msg;
> static const char *decl = "volatile sig_atomic_t msg";
> #else
> sig_atomic_t msg;
> static const char *decl = "sig_atomic_t msg";
> #endif
>
> volatile sig_atomic_t reply;
> volatile sig_atomic_t replied;
>
> int
> main(void)
> {
>         const struct sigaction act = { .sa_handler = catch_sigalrm };
>         const struct itimerval timer = {
>                 .it_value.tv_usec = 1,
>                 .it_interval.tv_usec = 1,
>         };
>         sigaction(SIGALRM, &act, NULL);
>         setitimer(ITIMER_REAL, &timer, NULL);
>         while (!replied) {
>                 msg = 1;
>         }
>         printf("%s == %d\n", decl, (int)reply);
> }
>
> static void
> catch_sigalrm(int unused)
> {
>         int n = msg;
>         if (!n)
>                 return;
>         reply = n;
>         replied = 1;
> }
> ```
>
> This runs forever:
> ```
> $ cc -O2 test2.c && ./a.out
> ```
> This terminates:
> ```
> $ clang -O2 -DVOLATILE test2.c && ./a.out
> volatile sig_atomic_t msg == 1
> ```
>
> Without volatile in its declaration, msg is never set, so the program
> spins forever because the signal handler never tells the loop to stop.
> There's probably a simpler way to do this.
>
> On Wed, Mar 20, 2024 at 07:20:03PM -0400, Elad Lahav wrote:
> > Actually, in the example I cited, there are multiple function calls
> > (printf, raise) between the two reads. How can the compiler optimize
> > out reading the value a second time?
> >
> > My view, which is subject to change at any moment and without notice,
> > is that you need volatile in the cases where you normally need
> > volatile, rather than inherently whenever you use sig_atomic_t. In the
> > following example I would not expect you to need it:
> >
> > static sig_atomic_t value;
> >
> > void
> > sig_handler(int signum)
> > {
> >     value++;
> > }
> >
> > void
> > func(void)
> > {
> >     value = 0;
> >     sleep(1);
> >     printf("There have been %d signals while I was napping\n", value);
> > }
> >
> > --Elad
> >
> > On Wed, Mar 20, 2024 at 7:05 PM Elad Lahav <e2lahav@gmail.com> wrote:
> > >
> > > No problem...
> > > Yes, in your example the issue is that the type is not atomic, and
> > > thus subject to partial updates that can be interrupted.
> > > Looking at the example given in
> > > https://en.cppreference.com/w/c/program/sig_atomic_t the volatile is
> > > needed to let the compiler know that the value can be updated in
> > > between two reads by the main function. That makes sense, especially
> > > if you have code that loops waiting for the value to change.
> > >
> > > --Elad
> > >
> > > On Wed, Mar 20, 2024 at 6:39 PM Guilherme Janczak
> > > <guilherme.janczak@yandex.com> wrote:
> > > >
> > > > Actually, uh, I misread your reply, forget the previous reply I sent.
> > > >
> > > > You don't need the volatile with lock-free atomics, but the standard
> > > > says you do need it with sig_atomic_t. I don't know of a case that would
> > > > break a plain `sig_atomic_t` variable with no `volatile`, however.
> > > >
> > > > On Wed, Mar 20, 2024 at 04:44:33PM -0400, Elad Lahav wrote:
> > > > > Do you really need volatile?
> > > > > There are two cases to consider. Either your code synchronizes updates
> > > > > to the shared value with the signal handler (e.g., by blocking and
> > > > > then unblocking the signal), in which case I believe the compiler
> > > > > cannot ignore updates to the value; or you don't, and you can't depend
> > > > > on the variable having any specific value in the signal handler. The
> > > > > only thing you want to prevent in the latter case is the handler
> > > > > observing a partial update to the variable, which I presume is where
> > > > > the other requirements originate. (In practice, there should be little
> > > > > or no concern with any primitive type on modern hardware).
> > > > >
> > > > > --Elad
> > > > >
> > > > > On Wed, Mar 20, 2024 at 4:32 PM Guilherme Janczak
> > > > > <guilherme.janczak@yandex.com> wrote:
> > > > > >
> > > > > > Variables shared with signal handlers must be of type `volatile
> > > > > > sigatomic_t`, not `volatile` or `sigatomic_t` as the current text says,
> > > > > > according to a C11 draft:
> > > > > >
> > > > > >     When ... interrupted by ... a signal, values of objects that are
> > > > > >     neither lock-free atomic objects nor of type volatile sig_atomic_t
> > > > > >     are unspecified.
> > > > > >
> > > > > > Ref: https://www.iso-9899.info/n1570.html#5.1.2.3p5
> > > > > > Signed-off-by: Guilherme Janczak <guilherme.janczak@yandex.com>
> > > > > > ---
> > > > > >  memorder/memorder.tex | 4 ++--
> > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/memorder/memorder.tex b/memorder/memorder.tex
> > > > > > index 5c50d42d..873c3424 100644
> > > > > > --- a/memorder/memorder.tex
> > > > > > +++ b/memorder/memorder.tex
> > > > > > @@ -1317,8 +1317,8 @@ from the viewpoint of the interrupted thread, at least at the
> > > > > >  assembly-language level.
> > > > > >  However, the C and C++ languages do not define the results of handlers
> > > > > >  and interrupted threads sharing plain variables.
> > > > > > -Instead, such shared variables must be \co{sig_atomic_t}, lock-free
> > > > > > -atomics, or \co{volatile}.
> > > > > > +Instead, such shared variables must be \co{volatile sig_atomic_t} or
> > > > > > +lock-free atomics.
> > > > > >
> > > > > >  On the other hand, because the handler executes within the interrupted
> > > > > >  thread's context, the memory ordering used to synchronize communication
> > > > > > --
> > > > > > 2.42.0
> > > > > >
> > > > > >
> > > > >

Re: [PATCH] signal handlers: volatile sigatomic_t, not volatile OR sigatomic_t

[Elad Lahav]

Here's the actual code I had in mind:

#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>
#include <signal.h>
#include <time.h>
#include <unistd.h>

static sig_atomic_t value;

static void
sig_handler(int const signum)
{
    value++;
}

int
main(int argc, char **argv)
{
    // Install a signal handler for SIGUSR1.
    struct sigaction act = { .sa_handler = sig_handler };
    if (sigaction(SIGUSR1, &act, NULL) == -1) {
        perror("sigaction");
        return EXIT_FAILURE;
    }

    // Create a timer to dispatch signals.
    struct sigevent event;
    SIGEV_SIGNAL_INIT(&event, SIGUSR1);

    timer_t timerid;
    if (timer_create(CLOCK_MONOTONIC, &event, &timerid) == -1) {
        perror("timer_create");
        return EXIT_FAILURE;
    }

    // Start the timer to fire every 10ms.
    struct itimerspec ts = {
        .it_value.tv_nsec = 10000000UL,
        .it_interval.tv_nsec = 10000000UL
    };

    if (timer_settime(timerid, 0, &ts, NULL) == -1) {
        perror("timer_settime");
        return EXIT_FAILURE;
    }

    // Wait for one second.
    // Can't use sleep(), as that call will be interrupted.
    uint64_t const start = clock_gettime_mon_ns();
    for (;;) {
        uint64_t const now = clock_gettime_mon_ns();
        if ((now - start) > 1000000000UL) {
            break;
        }
    }

    printf("There were %d signals while I was napping\n", value);

    return EXIT_SUCCESS;
}

--Elad

On Wed, Mar 20, 2024 at 9:03 PM Elad Lahav <e2lahav@gmail.com> wrote:
>
> Sorry, that's not at all what I meant. I used sleep() just to simulate
> the main function doing something while signals arrive asynchronously.
> What you gave in your example is exactly the situation that does
> require volatile, per my previous email, as the main function
> constantly polls the shared value without any function calls in reads.
>
> --Elad
>
> On Wed, Mar 20, 2024 at 8:34 PM Guilherme Janczak
> <guilherme.janczak@yandex.com> wrote:
> >
> > In the case in your example, sleep() doesn't need to be implemented
> > using SIGALRM. It isn't in glibc and OpenBSD that I know of:
> > https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/posix/sleep.c;h=3df79097c4464510f0197138a00a0c9772a5e83e;hb=3ab9b88e2ac91062b6d493fe32bd101a55006c6a
> > https://man.openbsd.org/sleep.3
> >
> > I managed to make GCC and Clang optimize out a sig_atomic_t store:
> > ```
> > #define _POSIX_C_SOURCE 200809L
> >
> > #include <sys/time.h>
> >
> > #include <stdio.h>
> > #include <stdlib.h>
> > #include <signal.h>
> > #include <string.h>
> >
> > static void catch_sigalrm(int);
> >
> > #ifdef VOLATILE
> > volatile sig_atomic_t msg;
> > static const char *decl = "volatile sig_atomic_t msg";
> > #else
> > sig_atomic_t msg;
> > static const char *decl = "sig_atomic_t msg";
> > #endif
> >
> > volatile sig_atomic_t reply;
> > volatile sig_atomic_t replied;
> >
> > int
> > main(void)
> > {
> >         const struct sigaction act = { .sa_handler = catch_sigalrm };
> >         const struct itimerval timer = {
> >                 .it_value.tv_usec = 1,
> >                 .it_interval.tv_usec = 1,
> >         };
> >         sigaction(SIGALRM, &act, NULL);
> >         setitimer(ITIMER_REAL, &timer, NULL);
> >         while (!replied) {
> >                 msg = 1;
> >         }
> >         printf("%s == %d\n", decl, (int)reply);
> > }
> >
> > static void
> > catch_sigalrm(int unused)
> > {
> >         int n = msg;
> >         if (!n)
> >                 return;
> >         reply = n;
> >         replied = 1;
> > }
> > ```
> >
> > This runs forever:
> > ```
> > $ cc -O2 test2.c && ./a.out
> > ```
> > This terminates:
> > ```
> > $ clang -O2 -DVOLATILE test2.c && ./a.out
> > volatile sig_atomic_t msg == 1
> > ```
> >
> > Without volatile in its declaration, msg is never set, so the program
> > spins forever because the signal handler never tells the loop to stop.
> > There's probably a simpler way to do this.
> >
> > On Wed, Mar 20, 2024 at 07:20:03PM -0400, Elad Lahav wrote:
> > > Actually, in the example I cited, there are multiple function calls
> > > (printf, raise) between the two reads. How can the compiler optimize
> > > out reading the value a second time?
> > >
> > > My view, which is subject to change at any moment and without notice,
> > > is that you need volatile in the cases where you normally need
> > > volatile, rather than inherently whenever you use sig_atomic_t. In the
> > > following example I would not expect you to need it:
> > >
> > > static sig_atomic_t value;
> > >
> > > void
> > > sig_handler(int signum)
> > > {
> > >     value++;
> > > }
> > >
> > > void
> > > func(void)
> > > {
> > >     value = 0;
> > >     sleep(1);
> > >     printf("There have been %d signals while I was napping\n", value);
> > > }
> > >
> > > --Elad
> > >
> > > On Wed, Mar 20, 2024 at 7:05 PM Elad Lahav <e2lahav@gmail.com> wrote:
> > > >
> > > > No problem...
> > > > Yes, in your example the issue is that the type is not atomic, and
> > > > thus subject to partial updates that can be interrupted.
> > > > Looking at the example given in
> > > > https://en.cppreference.com/w/c/program/sig_atomic_t the volatile is
> > > > needed to let the compiler know that the value can be updated in
> > > > between two reads by the main function. That makes sense, especially
> > > > if you have code that loops waiting for the value to change.
> > > >
> > > > --Elad
> > > >
> > > > On Wed, Mar 20, 2024 at 6:39 PM Guilherme Janczak
> > > > <guilherme.janczak@yandex.com> wrote:
> > > > >
> > > > > Actually, uh, I misread your reply, forget the previous reply I sent.
> > > > >
> > > > > You don't need the volatile with lock-free atomics, but the standard
> > > > > says you do need it with sig_atomic_t. I don't know of a case that would
> > > > > break a plain `sig_atomic_t` variable with no `volatile`, however.
> > > > >
> > > > > On Wed, Mar 20, 2024 at 04:44:33PM -0400, Elad Lahav wrote:
> > > > > > Do you really need volatile?
> > > > > > There are two cases to consider. Either your code synchronizes updates
> > > > > > to the shared value with the signal handler (e.g., by blocking and
> > > > > > then unblocking the signal), in which case I believe the compiler
> > > > > > cannot ignore updates to the value; or you don't, and you can't depend
> > > > > > on the variable having any specific value in the signal handler. The
> > > > > > only thing you want to prevent in the latter case is the handler
> > > > > > observing a partial update to the variable, which I presume is where
> > > > > > the other requirements originate. (In practice, there should be little
> > > > > > or no concern with any primitive type on modern hardware).
> > > > > >
> > > > > > --Elad
> > > > > >
> > > > > > On Wed, Mar 20, 2024 at 4:32 PM Guilherme Janczak
> > > > > > <guilherme.janczak@yandex.com> wrote:
> > > > > > >
> > > > > > > Variables shared with signal handlers must be of type `volatile
> > > > > > > sigatomic_t`, not `volatile` or `sigatomic_t` as the current text says,
> > > > > > > according to a C11 draft:
> > > > > > >
> > > > > > >     When ... interrupted by ... a signal, values of objects that are
> > > > > > >     neither lock-free atomic objects nor of type volatile sig_atomic_t
> > > > > > >     are unspecified.
> > > > > > >
> > > > > > > Ref: https://www.iso-9899.info/n1570.html#5.1.2.3p5
> > > > > > > Signed-off-by: Guilherme Janczak <guilherme.janczak@yandex.com>
> > > > > > > ---
> > > > > > >  memorder/memorder.tex | 4 ++--
> > > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/memorder/memorder.tex b/memorder/memorder.tex
> > > > > > > index 5c50d42d..873c3424 100644
> > > > > > > --- a/memorder/memorder.tex
> > > > > > > +++ b/memorder/memorder.tex
> > > > > > > @@ -1317,8 +1317,8 @@ from the viewpoint of the interrupted thread, at least at the
> > > > > > >  assembly-language level.
> > > > > > >  However, the C and C++ languages do not define the results of handlers
> > > > > > >  and interrupted threads sharing plain variables.
> > > > > > > -Instead, such shared variables must be \co{sig_atomic_t}, lock-free
> > > > > > > -atomics, or \co{volatile}.
> > > > > > > +Instead, such shared variables must be \co{volatile sig_atomic_t} or
> > > > > > > +lock-free atomics.
> > > > > > >
> > > > > > >  On the other hand, because the handler executes within the interrupted
> > > > > > >  thread's context, the memory ordering used to synchronize communication
> > > > > > > --
> > > > > > > 2.42.0
> > > > > > >
> > > > > > >
> > > > > >

Re: [PATCH] signal handlers: volatile sigatomic_t, not volatile OR sigatomic_t

[Elad Lahav]

Looks like this patch wasn't merged. Regardless of my
questions/observations, Guilherme is absolutely right that volatile is
required for sig_atomic_t by the standard. It may also need to be
mentioned that, despite its name, the only operations that can be
safely done on sig_atomic_t are store and load - it does not provide
read/modify/write atomicity, unlike the lock-free atomic types. The
standard says so, but it's easy to miss:

"the signal handler refers to an object with static or thread storage
duration that is not a
lock-free atomic object other than by *assigning* a value to an object
declared as
volatile sig_atomic_t"

--Elad

On Wed, Mar 20, 2024 at 7:07 PM Paul E. McKenney <paulmck@kernel.org> wrote:
>
> On Wed, Mar 20, 2024 at 10:26:04PM +0000, Guilherme Janczak wrote:
> > The standard says it's necessary.
> > If you have a `volatile long long` on a 32-bit architecture, the
> > compiler will have to compile it to some bignum code (meaning multiple
> > instructions), and the signal can come in between them.
>
> Agreed, because in this case the variable is not machine-word sized.
>
> What happens if you instead use a 32-bit aligned volatile quantity?
> (Or 16 or 8 bits, for that matter, at least on PowerPC [1].)
>
> I would hope that these smaller sizes would work, otherwise, again,
> device drivers become problematic on that platform.
>
>                                                         Thanx, Paul
>
> [1] There have been some platforms without 16-bit or 8-bit loads,
>     and on such platforms, you could also see failures of one sort
>     or another.
>
> > I can quickly make it happen on a Nintendo Wii (32-bit powerpc) as
> > well as an i386 laptop using this test program:
> > ```
> > #define _POSIX_C_SOURCE 200809L
> >
> > #include <sys/time.h>
> >
> > #include <stdio.h>
> > #include <stdlib.h>
> > #include <signal.h>
> > #include <string.h>
> >
> > static void catch_sigalrm(int);
> >
> > volatile unsigned long long longword;
> > volatile sig_atomic_t partial_update;
> >
> > int
> > main(void)
> > {
> >       const struct sigaction act = { .sa_handler = catch_sigalrm };
> >       const struct itimerval timer = {
> >               .it_value.tv_usec = 1,
> >               .it_interval.tv_usec = 1,
> >       };
> >       unsigned long long n;
> >
> >       sigaction(SIGALRM, &act, NULL);
> >       setitimer(ITIMER_REAL, &timer, NULL);
> >       for (unsigned char i = 0;; i++) {
> >               memset(&n, i, sizeof(n));
> >               longword = n;
> >               if (partial_update) {
> >                       fprintf(stderr, "longword partially updated\n");
> >                       exit(1);
> >               }
> >       }
> > }
> >
> > static void
> > catch_sigalrm(int unused)
> > {
> >       union multibyte {
> >               long long n;
> >               unsigned char bytes[sizeof(long long)];
> >       } window;
> >       (void)unused;
> >       window.n = longword;
> >
> >       for (size_t i = 1; i < sizeof(window.bytes); i++) {
> >               if (window.bytes[i] != window.bytes[0])
> >                       partial_update = 1;
> >       }
> > }
> > ```
> >
> > Output:
> > ```
> > $ cc -O2 test.c && ./a.out
> > longword partially updated
> > ```
> >
> > The program runs (apparently) forever on my amd64 desktop.
> >
> > If you look at the powerpc assembly version of the program in Godbolt:
> > https://godbolt.org/z/Pc8q7E5ej
> > Lines 69 and 70 of the assembly use 2 STW instructions to store each
> > 32-bit half of the bignum.
> >
> > On Wed, Mar 20, 2024 at 04:44:33PM -0400, Elad Lahav wrote:
> > > Do you really need volatile?
> > > There are two cases to consider. Either your code synchronizes updates
> > > to the shared value with the signal handler (e.g., by blocking and
> > > then unblocking the signal), in which case I believe the compiler
> > > cannot ignore updates to the value; or you don't, and you can't depend
> > > on the variable having any specific value in the signal handler. The
> > > only thing you want to prevent in the latter case is the handler
> > > observing a partial update to the variable, which I presume is where
> > > the other requirements originate. (In practice, there should be little
> > > or no concern with any primitive type on modern hardware).
> > >
> > > --Elad
> > >
> > > On Wed, Mar 20, 2024 at 4:32 PM Guilherme Janczak
> > > <guilherme.janczak@yandex.com> wrote:
> > > >
> > > > Variables shared with signal handlers must be of type `volatile
> > > > sigatomic_t`, not `volatile` or `sigatomic_t` as the current text says,
> > > > according to a C11 draft:
> > > >
> > > >     When ... interrupted by ... a signal, values of objects that are
> > > >     neither lock-free atomic objects nor of type volatile sig_atomic_t
> > > >     are unspecified.
> > > >
> > > > Ref: https://www.iso-9899.info/n1570.html#5.1.2.3p5
> > > > Signed-off-by: Guilherme Janczak <guilherme.janczak@yandex.com>
> > > > ---
> > > >  memorder/memorder.tex | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/memorder/memorder.tex b/memorder/memorder.tex
> > > > index 5c50d42d..873c3424 100644
> > > > --- a/memorder/memorder.tex
> > > > +++ b/memorder/memorder.tex
> > > > @@ -1317,8 +1317,8 @@ from the viewpoint of the interrupted thread, at least at the
> > > >  assembly-language level.
> > > >  However, the C and C++ languages do not define the results of handlers
> > > >  and interrupted threads sharing plain variables.
> > > > -Instead, such shared variables must be \co{sig_atomic_t}, lock-free
> > > > -atomics, or \co{volatile}.
> > > > +Instead, such shared variables must be \co{volatile sig_atomic_t} or
> > > > +lock-free atomics.
> > > >
> > > >  On the other hand, because the handler executes within the interrupted
> > > >  thread's context, the memory ordering used to synchronize communication
> > > > --
> > > > 2.42.0
> > > >
> > > >
> >

Re: [PATCH] signal handlers: volatile sigatomic_t, not volatile OR sigatomic_t

[Akira Yokosawa]

Hi,

Somehow, all messages from Guilherme ended up in my spam folder
and I couldn't follow this thread properly earlier.  I think I now
see the context.

On Thu, 28 Mar 2024 12:30:26 -0400, Elad Lahav wrote:
> Looks like this patch wasn't merged. Regardless of my
> questions/observations, Guilherme is absolutely right that volatile is
> required for sig_atomic_t by the standard. It may also need to be
> mentioned that, despite its name, the only operations that can be
> safely done on sig_atomic_t are store and load - it does not provide
> read/modify/write atomicity, unlike the lock-free atomic types. The
> standard says so, but it's easy to miss:
> 
> "the signal handler refers to an object with static or thread storage
> duration that is not a
> lock-free atomic object other than by *assigning* a value to an object
> declared as
> volatile sig_atomic_t"

Hmm.

perfbook's (or LKMM's) style of *not* putting volatile at variable
declarations, but marking accesses to shared variable with
READ_ONCE()/WRITE_ONCE() when needed does not require the shared
object to be declared as "volatile sig_atomic_t".

As far as I can see, that is Paul's take throughout perfbook.

There are a couple of mentions to sig_atomic_t elsewhere.

Footnote 9 in Chapter 4:

    That said, the various standards committees would prefer that
    you use atomics or variables of type sig_atomic_t, instead of
    READ_ONCE() and WRITE_ONCE().

Answer to Quick Quiz 5.54:

    The theft variable must be of type sig_atomic_t to guarantee
    that it can be safely shared between the signal handler and
    the code interrupted by the signal.

If Guilherme's change is to be accepted, above sentences may need
some rewording as well for consistency, I guess.

My preference is to keep Listings 5.17 and 5.18 intact.

Sidenote:

You might be interested in Linus' recent remark on volatile
casting of LKMM vs C/C++ standards on volatile declaration at:

    https://lore.kernel.org/lkml/CAHk-=wjP1i014DGPKTsAC6TpByC3xeNHDjVA4E4gsnzUgJBYBQ@mail.gmail.com/

(Warning: lengthy thread)

I'm not saying I agree 100% with him, but I'm accustomed to LKMM's
way so much that I don't like to see variables declared volatile... :-/

        Thanks, Akira