Re: Support SIGEV_THREAD_ID

[James Y Knight <jyknight@google.com>]

[-- Attachment #1: Type: text/plain, Size: 9208 bytes --]

On Thu, Aug 1, 2019 at 12:50 PM Rich Felker <dalias@libc.org> wrote:
>
> On Thu, Aug 01, 2019 at 12:15:57PM -0400, James Y Knight wrote:
> > There seems to be some debate in glibc over whether this API should be
> > supported, due to the long-standing debate about "pthread_t" vs
> > "kernel tid" APIs. (And this API uses kernel tids, of course.)
> >
> > One proposal from previous glibc discussion was to add a
> > SIGEV_PTHREAD_ID, which takes a pthread_t, instead of a kernel tid.
> > Nobody has done this yet, and I'd note that if it is done, that is not
> > at all incompatible with also continuing to support SIGEV_THREAD_ID.
> > (Just like both sched_setaffinity and pthread_setaffinity_np exist
> > without issue).
> >
> > Despite that discussion, SIGEV_THREAD_ID functionality does in fact
> > work with glibc -- it provides the SIGEV_THREAD_ID define in its
> > headers, and it defines the same 'struct sigevent' as the kernel does,
> > including a _tid member. The only thing it's missing is the field name
> > "sigev_notify_thread_id" -- so users are simply doing "#define
> > sigev_notify_thread_id _sigev_un._tid" as a workaround (ugh).
> >
> > However, it does _not_ work today with musl, as musl's timer_create
> > function translates the user-facing struct to a separate kernel-facing
> > structure.
> >
> > Given long-standing usage of this feature, and given that potential
> > future directions are additive, not conflicting, ISTM reasonable to
> > just add support for this to musl.
>
> > From 0a0aef759f216444f558f427466b47f38d678052 Mon Sep 17 00:00:00 2001
> > From: James Y Knight <jyknight@google.com>
> > Date: Sun, 30 Jun 2019 21:55:20 -0400
> > Subject: [PATCH] Add support for SIGEV_THREAD_ID and
sigev_notify_thread_id.
> >
> > This is like SIGEV_SIGNAL, but targeted to a particular thread's
> > tid, rather than the process.
> > ---
> >  include/signal.h        | 16 +++++++++++++---
> >  src/time/timer_create.c |  8 ++++++--
> >  2 files changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/signal.h b/include/signal.h
> > index 5c48cb83..735e0ac7 100644
> > --- a/include/signal.h
> > +++ b/include/signal.h
> > @@ -180,14 +180,24 @@ struct sigevent {
> >       union sigval sigev_value;
> >       int sigev_signo;
> >       int sigev_notify;
> > -     void (*sigev_notify_function)(union sigval);
> > -     pthread_attr_t *sigev_notify_attributes;
> > -     char __pad[56-3*sizeof(long)];
> > +     union {
> > +             char __pad[64 - 2*sizeof(int) - sizeof(union sigval)];
> > +             pid_t sigev_notify_thread_id;
> > +             struct {
> > +                     void (*sigev_notify_function)(union sigval);
> > +                     pthread_attr_t *sigev_notify_attributes;
> > +             } __sev_thread;
> > +     } __sev_fields;
> >  };
> >
> > +#define sigev_notify_thread_id __sev_fields.sigev_notify_thread_id
> > +#define sigev_notify_function
__sev_fields.__sev_thread.sigev_notify_function
> > +#define sigev_notify_attributes
__sev_fields.__sev_thread.sigev_notify_attributes
> > +
>
> I really hate these macro hacks, and have been looking at getting rid
> of the ones we have (using anon unions). We don't mandate C11 to use
> the public headers, but it might make sense to mandate C11 || __GNUC__
> and stick __extension__ on the struct if __GNUC__ is defined.
> Unfortunately, cparser/firm prior to latest git master does not
> support designated initializers right with anon unions, and GCC 3.x
> doesn't either (yes, there are users of GCC 3 with musl, both
> full-time and just as a bootstrapping-from-plain-C stage for distros).
> So I'm not sure if we can fix this yet or just keep doing the same
> nasty macro hack for now...

100% agreed!

I had actually drafted the email below proposing to get rid of this in
favor of C11 anonymous struct/union. However, after writing the below text,
I then abandoned that idea, because _no_ version of the C++ standard
supports anonymous structs -- although every version has supported
anonymous unions. And I figured that because the musl headers must be
parseable in C++, and given musl's goals of standards adherence, the use of
a widely-supported-yet-nonstandard compiler extension would not be
acceptable, even if effectively every C++ compiler in use supports it.

If I was wrong about that, I'd be quite happy to use anonymous structs and
unions here (and to convert the other cases as well).

Unnamed struct/unions have been standardized as of C11, and was a
> very-long-standing compiler extension in at least GCC, Clang, and MSVC
> before that. (Looks to have been first introduced in GCC in version 3.0).
> Docs https://gcc.gnu.org/onlinedocs/gcc-4.7.2/gcc/Unnamed-Fields.html



The feature makes things a bit cleaner than having to introduce #defines to
> name the inner-fields, as is generally done now. Both gcc and clang accept
> these unnamed, even in "-std=c89". They even accept such structs without
> warning in musl headers with "-std=c89 -pedantic", since pedantic warnings
> are ignored in system headers (otherwise all sorts of things like "long
> long" would also warn!).



Given that this feature is standard now for 8 years, and was widely
> supported for decades before that, it seems that it may be a nice
> simplification.



On Thu, Aug 1, 2019 at 12:50 PM Rich Felker <dalias@libc.org> wrote:

> On Thu, Aug 01, 2019 at 12:15:57PM -0400, James Y Knight wrote:
> > There seems to be some debate in glibc over whether this API should be
> > supported, due to the long-standing debate about "pthread_t" vs
> > "kernel tid" APIs. (And this API uses kernel tids, of course.)
> >
> > One proposal from previous glibc discussion was to add a
> > SIGEV_PTHREAD_ID, which takes a pthread_t, instead of a kernel tid.
> > Nobody has done this yet, and I'd note that if it is done, that is not
> > at all incompatible with also continuing to support SIGEV_THREAD_ID.
> > (Just like both sched_setaffinity and pthread_setaffinity_np exist
> > without issue).
> >
> > Despite that discussion, SIGEV_THREAD_ID functionality does in fact
> > work with glibc -- it provides the SIGEV_THREAD_ID define in its
> > headers, and it defines the same 'struct sigevent' as the kernel does,
> > including a _tid member. The only thing it's missing is the field name
> > "sigev_notify_thread_id" -- so users are simply doing "#define
> > sigev_notify_thread_id _sigev_un._tid" as a workaround (ugh).
> >
> > However, it does _not_ work today with musl, as musl's timer_create
> > function translates the user-facing struct to a separate kernel-facing
> > structure.
> >
> > Given long-standing usage of this feature, and given that potential
> > future directions are additive, not conflicting, ISTM reasonable to
> > just add support for this to musl.
>
> > From 0a0aef759f216444f558f427466b47f38d678052 Mon Sep 17 00:00:00 2001
> > From: James Y Knight <jyknight@google.com>
> > Date: Sun, 30 Jun 2019 21:55:20 -0400
> > Subject: [PATCH] Add support for SIGEV_THREAD_ID and
> sigev_notify_thread_id.
> >
> > This is like SIGEV_SIGNAL, but targeted to a particular thread's
> > tid, rather than the process.
> > ---
> >  include/signal.h        | 16 +++++++++++++---
> >  src/time/timer_create.c |  8 ++++++--
> >  2 files changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/signal.h b/include/signal.h
> > index 5c48cb83..735e0ac7 100644
> > --- a/include/signal.h
> > +++ b/include/signal.h
> > @@ -180,14 +180,24 @@ struct sigevent {
> >       union sigval sigev_value;
> >       int sigev_signo;
> >       int sigev_notify;
> > -     void (*sigev_notify_function)(union sigval);
> > -     pthread_attr_t *sigev_notify_attributes;
> > -     char __pad[56-3*sizeof(long)];
> > +     union {
> > +             char __pad[64 - 2*sizeof(int) - sizeof(union sigval)];
> > +             pid_t sigev_notify_thread_id;
> > +             struct {
> > +                     void (*sigev_notify_function)(union sigval);
> > +                     pthread_attr_t *sigev_notify_attributes;
> > +             } __sev_thread;
> > +     } __sev_fields;
> >  };
> >
> > +#define sigev_notify_thread_id __sev_fields.sigev_notify_thread_id
> > +#define sigev_notify_function
> __sev_fields.__sev_thread.sigev_notify_function
> > +#define sigev_notify_attributes
> __sev_fields.__sev_thread.sigev_notify_attributes
> > +
>
> I really hate these macro hacks, and have been looking at getting rid
> of the ones we have (using anon unions). We don't mandate C11 to use
> the public headers, but it might make sense to mandate C11 || __GNUC__
> and stick __extension__ on the struct if __GNUC__ is defined.
> Unfortunately, cparser/firm prior to latest git master does not
> support designated initializers right with anon unions, and GCC 3.x
> doesn't either (yes, there are users of GCC 3 with musl, both
> full-time and just as a bootstrapping-from-plain-C stage for distros).
> So I'm not sure if we can fix this yet or just keep doing the same
> nasty macro hack for now...
>
> Alternatively we could just put the new member in the padding since
> we're translating the struct anyway, but that sacrifices glibc
> ABI-compat for the newly-added feature.
>
> Rich
>

[-- Attachment #2: Type: text/html, Size: 11876 bytes --]