On Thu, Aug 1, 2019 at 12:50 PM Rich Felker 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 > > 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 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 > > 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 >