[PATCH v1 0/8] Update to C11, fix signal undefined behavior

Ian Rogers posted 8 patches 3 years, 5 months ago
There is a newer version of this series
tools/perf/Makefile.config  | 2 +-
tools/perf/builtin-daemon.c | 3 ++-
tools/perf/builtin-ftrace.c | 4 ++--
tools/perf/builtin-record.c | 9 +++++----
tools/perf/builtin-stat.c   | 9 +++++----
tools/perf/builtin-top.c    | 4 ++--
tools/perf/builtin-trace.c  | 4 ++--
tools/perf/util/session.c   | 3 ++-
8 files changed, 21 insertions(+), 17 deletions(-)
[PATCH v1 0/8] Update to C11, fix signal undefined behavior
Posted by Ian Rogers 3 years, 5 months ago
The use of C11 is mainstream in the kernel [1]. There was some
confusion on volatile and signal handlers in [2]. Switch to using
stdatomic.h (requires C11) and sig_atomic_t as per [3]. Thanks to Leo
Yan <leo.yan@linaro.org> for the suggestions.

[1] https://lore.kernel.org/lkml/CAHk-=whWbENRz-vLY6vpESDLj6kGUTKO3khGtVfipHqwewh2HQ@mail.gmail.com/
[2] https://lore.kernel.org/lkml/20221024011024.462518-1-irogers@google.com/
[3] https://wiki.sei.cmu.edu/confluence/display/c/SIG31-C.+Do+not+access+shared+objects+in+signal+handlers


Ian Rogers (8):
  perf build: Update to C standard to gnu11
  perf record: Use sig_atomic_t for signal handlers
  perf daemon: Use sig_atomic_t to avoid UB
  perf ftrace: Use sig_atomic_t to avoid UB
  perf session: Change type to avoid UB
  perf stat: Use sig_atomic_t to avoid UB
  perf top: Use sig_atomic_t to avoid UB
  perf trace: Use sig_atomic_t to avoid UB

 tools/perf/Makefile.config  | 2 +-
 tools/perf/builtin-daemon.c | 3 ++-
 tools/perf/builtin-ftrace.c | 4 ++--
 tools/perf/builtin-record.c | 9 +++++----
 tools/perf/builtin-stat.c   | 9 +++++----
 tools/perf/builtin-top.c    | 4 ++--
 tools/perf/builtin-trace.c  | 4 ++--
 tools/perf/util/session.c   | 3 ++-
 8 files changed, 21 insertions(+), 17 deletions(-)

-- 
2.38.0.135.g90850a2211-goog
Re: [PATCH v1 0/8] Update to C11, fix signal undefined behavior
Posted by Arnaldo Carvalho de Melo 3 years, 5 months ago
Em Mon, Oct 24, 2022 at 10:35:15AM -0700, Ian Rogers escreveu:
> The use of C11 is mainstream in the kernel [1]. There was some
> confusion on volatile and signal handlers in [2]. Switch to using
> stdatomic.h (requires C11) and sig_atomic_t as per [3]. Thanks to Leo
> Yan <leo.yan@linaro.org> for the suggestions.
> 
> [1] https://lore.kernel.org/lkml/CAHk-=whWbENRz-vLY6vpESDLj6kGUTKO3khGtVfipHqwewh2HQ@mail.gmail.com/
> [2] https://lore.kernel.org/lkml/20221024011024.462518-1-irogers@google.com/
> [3] https://wiki.sei.cmu.edu/confluence/display/c/SIG31-C.+Do+not+access+shared+objects+in+signal+handlers

I think I'll apply this to perf/core, i.e. for 6.3, ok?

- Arnaldo
 
> 
> Ian Rogers (8):
>   perf build: Update to C standard to gnu11
>   perf record: Use sig_atomic_t for signal handlers
>   perf daemon: Use sig_atomic_t to avoid UB
>   perf ftrace: Use sig_atomic_t to avoid UB
>   perf session: Change type to avoid UB
>   perf stat: Use sig_atomic_t to avoid UB
>   perf top: Use sig_atomic_t to avoid UB
>   perf trace: Use sig_atomic_t to avoid UB
> 
>  tools/perf/Makefile.config  | 2 +-
>  tools/perf/builtin-daemon.c | 3 ++-
>  tools/perf/builtin-ftrace.c | 4 ++--
>  tools/perf/builtin-record.c | 9 +++++----
>  tools/perf/builtin-stat.c   | 9 +++++----
>  tools/perf/builtin-top.c    | 4 ++--
>  tools/perf/builtin-trace.c  | 4 ++--
>  tools/perf/util/session.c   | 3 ++-
>  8 files changed, 21 insertions(+), 17 deletions(-)
> 
> -- 
> 2.38.0.135.g90850a2211-goog

-- 

- Arnaldo
Re: [PATCH v1 0/8] Update to C11, fix signal undefined behavior
Posted by Ian Rogers 3 years, 5 months ago
On Mon, Oct 24, 2022 at 10:51 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Mon, Oct 24, 2022 at 10:35:15AM -0700, Ian Rogers escreveu:
> > The use of C11 is mainstream in the kernel [1]. There was some
> > confusion on volatile and signal handlers in [2]. Switch to using
> > stdatomic.h (requires C11) and sig_atomic_t as per [3]. Thanks to Leo
> > Yan <leo.yan@linaro.org> for the suggestions.
> >
> > [1] https://lore.kernel.org/lkml/CAHk-=whWbENRz-vLY6vpESDLj6kGUTKO3khGtVfipHqwewh2HQ@mail.gmail.com/
> > [2] https://lore.kernel.org/lkml/20221024011024.462518-1-irogers@google.com/
> > [3] https://wiki.sei.cmu.edu/confluence/display/c/SIG31-C.+Do+not+access+shared+objects+in+signal+handlers
>
> I think I'll apply this to perf/core, i.e. for 6.3, ok?

Sounds good to me. 6.3 or 6.2? I suspect there is more cleanup like
this and to the iterators (from C11) that can be done.

Thanks,
Ian

> - Arnaldo
>
> >
> > Ian Rogers (8):
> >   perf build: Update to C standard to gnu11
> >   perf record: Use sig_atomic_t for signal handlers
> >   perf daemon: Use sig_atomic_t to avoid UB
> >   perf ftrace: Use sig_atomic_t to avoid UB
> >   perf session: Change type to avoid UB
> >   perf stat: Use sig_atomic_t to avoid UB
> >   perf top: Use sig_atomic_t to avoid UB
> >   perf trace: Use sig_atomic_t to avoid UB
> >
> >  tools/perf/Makefile.config  | 2 +-
> >  tools/perf/builtin-daemon.c | 3 ++-
> >  tools/perf/builtin-ftrace.c | 4 ++--
> >  tools/perf/builtin-record.c | 9 +++++----
> >  tools/perf/builtin-stat.c   | 9 +++++----
> >  tools/perf/builtin-top.c    | 4 ++--
> >  tools/perf/builtin-trace.c  | 4 ++--
> >  tools/perf/util/session.c   | 3 ++-
> >  8 files changed, 21 insertions(+), 17 deletions(-)
> >
> > --
> > 2.38.0.135.g90850a2211-goog
>
> --
>
> - Arnaldo
Re: [PATCH v1 0/8] Update to C11, fix signal undefined behavior
Posted by Arnaldo Carvalho de Melo 3 years, 5 months ago
Em Mon, Oct 24, 2022 at 10:59:03AM -0700, Ian Rogers escreveu:
> On Mon, Oct 24, 2022 at 10:51 AM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > Em Mon, Oct 24, 2022 at 10:35:15AM -0700, Ian Rogers escreveu:
> > > The use of C11 is mainstream in the kernel [1]. There was some
> > > confusion on volatile and signal handlers in [2]. Switch to using
> > > stdatomic.h (requires C11) and sig_atomic_t as per [3]. Thanks to Leo
> > > Yan <leo.yan@linaro.org> for the suggestions.
> > >
> > > [1] https://lore.kernel.org/lkml/CAHk-=whWbENRz-vLY6vpESDLj6kGUTKO3khGtVfipHqwewh2HQ@mail.gmail.com/
> > > [2] https://lore.kernel.org/lkml/20221024011024.462518-1-irogers@google.com/
> > > [3] https://wiki.sei.cmu.edu/confluence/display/c/SIG31-C.+Do+not+access+shared+objects+in+signal+handlers
> >
> > I think I'll apply this to perf/core, i.e. for 6.3, ok?
> 
> Sounds good to me. 6.3 or 6.2? I suspect there is more cleanup like
> this and to the iterators (from C11) that can be done.

oops, 6.2, sure, 6.1 is the current one, merge window closed. :-)

- Arnaldo
Re: [PATCH v1 0/8] Update to C11, fix signal undefined behavior
Posted by Ian Rogers 3 years, 5 months ago
On Mon, Oct 24, 2022 at 10:59 AM Ian Rogers <irogers@google.com> wrote:
>
> On Mon, Oct 24, 2022 at 10:51 AM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > Em Mon, Oct 24, 2022 at 10:35:15AM -0700, Ian Rogers escreveu:
> > > The use of C11 is mainstream in the kernel [1]. There was some
> > > confusion on volatile and signal handlers in [2]. Switch to using
> > > stdatomic.h (requires C11) and sig_atomic_t as per [3]. Thanks to Leo
> > > Yan <leo.yan@linaro.org> for the suggestions.
> > >
> > > [1] https://lore.kernel.org/lkml/CAHk-=whWbENRz-vLY6vpESDLj6kGUTKO3khGtVfipHqwewh2HQ@mail.gmail.com/
> > > [2] https://lore.kernel.org/lkml/20221024011024.462518-1-irogers@google.com/
> > > [3] https://wiki.sei.cmu.edu/confluence/display/c/SIG31-C.+Do+not+access+shared+objects+in+signal+handlers
> >
> > I think I'll apply this to perf/core, i.e. for 6.3, ok?
>
> Sounds good to me. 6.3 or 6.2? I suspect there is more cleanup like
> this and to the iterators (from C11) that can be done.
>
> Thanks,
> Ian

So I noticed a few changes missing #include-ing stdatomic.h and
sig_atomic_t is actually in signal.h. I'm not sure we need the C11
change then, but it seems like the right thing to do anyway. I'll do a
v2 to drop the unneeded (currently) include of stdatomic.h.

Thanks,
Ian

> > - Arnaldo
> >
> > >
> > > Ian Rogers (8):
> > >   perf build: Update to C standard to gnu11
> > >   perf record: Use sig_atomic_t for signal handlers
> > >   perf daemon: Use sig_atomic_t to avoid UB
> > >   perf ftrace: Use sig_atomic_t to avoid UB
> > >   perf session: Change type to avoid UB
> > >   perf stat: Use sig_atomic_t to avoid UB
> > >   perf top: Use sig_atomic_t to avoid UB
> > >   perf trace: Use sig_atomic_t to avoid UB
> > >
> > >  tools/perf/Makefile.config  | 2 +-
> > >  tools/perf/builtin-daemon.c | 3 ++-
> > >  tools/perf/builtin-ftrace.c | 4 ++--
> > >  tools/perf/builtin-record.c | 9 +++++----
> > >  tools/perf/builtin-stat.c   | 9 +++++----
> > >  tools/perf/builtin-top.c    | 4 ++--
> > >  tools/perf/builtin-trace.c  | 4 ++--
> > >  tools/perf/util/session.c   | 3 ++-
> > >  8 files changed, 21 insertions(+), 17 deletions(-)
> > >
> > > --
> > > 2.38.0.135.g90850a2211-goog
> >
> > --
> >
> > - Arnaldo
RE: [PATCH v1 0/8] Update to C11, fix signal undefined behavior
Posted by David Laight 3 years, 5 months ago
From: Ian Rogers
> Sent: 24 October 2022 19:12
> 
> On Mon, Oct 24, 2022 at 10:59 AM Ian Rogers <irogers@google.com> wrote:
> >
> > On Mon, Oct 24, 2022 at 10:51 AM Arnaldo Carvalho de Melo
> > <acme@kernel.org> wrote:
> > >
> > > Em Mon, Oct 24, 2022 at 10:35:15AM -0700, Ian Rogers escreveu:
> > > > The use of C11 is mainstream in the kernel [1]. There was some
> > > > confusion on volatile and signal handlers in [2]. Switch to using
> > > > stdatomic.h (requires C11) and sig_atomic_t as per [3]. Thanks to Leo
> > > > Yan <leo.yan@linaro.org> for the suggestions.
> > > >
> > > > [1] https://lore.kernel.org/lkml/CAHk-=whWbENRz-
> vLY6vpESDLj6kGUTKO3khGtVfipHqwewh2HQ@mail.gmail.com/
> > > > [2] https://lore.kernel.org/lkml/20221024011024.462518-1-irogers@google.com/
> > > > [3] https://wiki.sei.cmu.edu/confluence/display/c/SIG31-
> C.+Do+not+access+shared+objects+in+signal+handlers
> > >
> > > I think I'll apply this to perf/core, i.e. for 6.3, ok?
> >
> > Sounds good to me. 6.3 or 6.2? I suspect there is more cleanup like
> > this and to the iterators (from C11) that can be done.
> >
> > Thanks,
> > Ian
> 
> So I noticed a few changes missing #include-ing stdatomic.h and
> sig_atomic_t is actually in signal.h. I'm not sure we need the C11
> change then, but it seems like the right thing to do anyway. I'll do a
> v2 to drop the unneeded (currently) include of stdatomic.h.

While the C standard requires you use sig_atomic_t (to avoid
wider RMW being done for writes) the kernel very much requires
that volatiles accesses are atomic.
So the compilers used will do that even when the C standard
would allow them to do otherwise.

Pretty much the last mainstream cpu that couldn't do atomic
byte writes was an old alpha.

So for anything that Linux is going to run on these changes
aren't really needed.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Re: [PATCH v1 0/8] Update to C11, fix signal undefined behavior
Posted by Leo Yan 3 years, 5 months ago
On Tue, Oct 25, 2022 at 10:36:16AM +0000, David Laight wrote:

[...]

> > So I noticed a few changes missing #include-ing stdatomic.h and
> > sig_atomic_t is actually in signal.h. I'm not sure we need the C11
> > change then, but it seems like the right thing to do anyway. I'll do a
> > v2 to drop the unneeded (currently) include of stdatomic.h.
> 
> While the C standard requires you use sig_atomic_t (to avoid
> wider RMW being done for writes) the kernel very much requires
> that volatiles accesses are atomic.
> So the compilers used will do that even when the C standard
> would allow them to do otherwise.
> 
> Pretty much the last mainstream cpu that couldn't do atomic
> byte writes was an old alpha.

One case that it might break atomicity for the data with int type,
I.e. on Arm CPU when CPU access data without alignment, then it
cannot promise atomicity.  I am not sure if the data with int type
will be always compiled with 4-byte alignment, if it's packed
without alignment then sig_atomic_t is still required.

Clarify that I don't have deep understanding for compiler, so sorry
if I introduce any noise.

Thanks,
Leo

> So for anything that Linux is going to run on these changes
> aren't really needed.

> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)