[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20190806020632.GS9017@brightrain.aerifal.cx>
Date: Mon, 5 Aug 2019 22:06:32 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: Support SIGEV_THREAD_ID
On Thu, Aug 01, 2019 at 02:00:44PM -0400, James Y Knight wrote:
> 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.
The C++ part is really disappointing, since poor interaction with C++
is one of the main reasons I wanted to eliminate the #defines... :(
I think we could probably get by with only anon unions, not anon
structs, but I haven't done any detailed analysis. We should probably
just put this aside for now, though, since it looks less productive
relative to the painfulness than I expected...
Rich
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.
