[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAA2zVHrg15DFS5HC6-3nWbx+SdsphiB+GBz5t6NR1F3ZbN8ehA@mail.gmail.com>
Date: Thu, 1 Aug 2019 14:00:44 -0400
From: James Y Knight <jyknight@...gle.com>
To: musl@...ts.openwall.com
Subject: Re: Support SIGEV_THREAD_ID
On Thu, Aug 1, 2019 at 12:50 PM Rich Felker <dalias@...c.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@...gle.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@...c.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@...gle.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
>
Content of type "text/html" skipped
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
