mailing list of musl libc
 help / color / mirror / code / Atom feed
* Support SIGEV_THREAD_ID
@ 2019-08-01 16:15 James Y Knight
  2019-08-01 16:24 ` Florian Weimer
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: James Y Knight @ 2019-08-01 16:15 UTC (permalink / raw)
  To: musl

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

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.

[-- Attachment #2: 0001-Add-support-for-SIGEV_THREAD_ID-and-sigev_notify_thr.patch --]
[-- Type: text/x-patch, Size: 2647 bytes --]

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
+
 #define SIGEV_SIGNAL 0
 #define SIGEV_NONE 1
 #define SIGEV_THREAD 2
+#define SIGEV_THREAD_ID 4 /* Linux extension */
 
 int __libc_current_sigrtmin(void);
 int __libc_current_sigrtmax(void);
diff --git a/src/time/timer_create.c b/src/time/timer_create.c
index c5e40a19..5a3f4d3a 100644
--- a/src/time/timer_create.c
+++ b/src/time/timer_create.c
@@ -83,11 +83,15 @@ int timer_create(clockid_t clk, struct sigevent *restrict evp, timer_t *restrict
 	switch (evp ? evp->sigev_notify : SIGEV_SIGNAL) {
 	case SIGEV_NONE:
 	case SIGEV_SIGNAL:
+	case SIGEV_THREAD_ID:
 		if (evp) {
 			ksev.sigev_value = evp->sigev_value;
 			ksev.sigev_signo = evp->sigev_signo;
 			ksev.sigev_notify = evp->sigev_notify;
-			ksev.sigev_tid = 0;
+			if (evp->sigev_notify == SIGEV_THREAD_ID)
+				ksev.sigev_tid = evp->sigev_notify_thread_id;
+			else
+				ksev.sigev_tid = 0;
 			ksevp = &ksev;
 		}
 		if (syscall(SYS_timer_create, clk, ksevp, &timerid) < 0)
@@ -115,7 +119,7 @@ int timer_create(clockid_t clk, struct sigevent *restrict evp, timer_t *restrict
 
 		ksev.sigev_value.sival_ptr = 0;
 		ksev.sigev_signo = SIGTIMER;
-		ksev.sigev_notify = 4; /* SIGEV_THREAD_ID */
+		ksev.sigev_notify = SIGEV_THREAD_ID;
 		ksev.sigev_tid = td->tid;
 		if (syscall(SYS_timer_create, clk, &ksev, &timerid) < 0)
 			timerid = -1;
-- 
2.22.0.709.g102302147b-goog


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Support SIGEV_THREAD_ID
  2019-08-01 16:15 Support SIGEV_THREAD_ID James Y Knight
@ 2019-08-01 16:24 ` Florian Weimer
  2019-08-01 16:45   ` Rich Felker
  2020-10-30 21:22   ` [musl] " Michael Forney
  2019-08-01 16:49 ` Rich Felker
  2020-08-18  0:32 ` [musl] " Rich Felker
  2 siblings, 2 replies; 10+ messages in thread
From: Florian Weimer @ 2019-08-01 16:24 UTC (permalink / raw)
  To: James Y Knight; +Cc: musl

* James Y. Knight:

> 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.)

The debate is over and has been decided in favor of supporting TIDs.  We
just have a backlog of interfaces for which we need to add support.

Fortunately, C++ is standardizing stackless coroutines, so this has not
been proven to be a mistake.

Thanks,
Florian


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Support SIGEV_THREAD_ID
  2019-08-01 16:24 ` Florian Weimer
@ 2019-08-01 16:45   ` Rich Felker
  2020-10-30 21:22   ` [musl] " Michael Forney
  1 sibling, 0 replies; 10+ messages in thread
From: Rich Felker @ 2019-08-01 16:45 UTC (permalink / raw)
  To: musl

On Thu, Aug 01, 2019 at 06:24:31PM +0200, Florian Weimer wrote:
> * James Y. Knight:
> 
> > 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.)
> 
> The debate is over and has been decided in favor of supporting TIDs.  We
> just have a backlog of interfaces for which we need to add support.
> 
> Fortunately, C++ is standardizing stackless coroutines, so this has not
> been proven to be a mistake.

Good to know. I think we can go forward with doing the same then,
providing the interfaces to get and use tids.

Rich


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Support SIGEV_THREAD_ID
  2019-08-01 16:15 Support SIGEV_THREAD_ID James Y Knight
  2019-08-01 16:24 ` Florian Weimer
@ 2019-08-01 16:49 ` Rich Felker
  2019-08-01 18:00   ` James Y Knight
  2020-08-18  0:32 ` [musl] " Rich Felker
  2 siblings, 1 reply; 10+ messages in thread
From: Rich Felker @ 2019-08-01 16:49 UTC (permalink / raw)
  To: musl

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Support SIGEV_THREAD_ID
  2019-08-01 16:49 ` Rich Felker
@ 2019-08-01 18:00   ` James Y Knight
  2019-08-06  2:06     ` Rich Felker
  0 siblings, 1 reply; 10+ messages in thread
From: James Y Knight @ 2019-08-01 18:00 UTC (permalink / raw)
  To: musl

[-- 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 --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Support SIGEV_THREAD_ID
  2019-08-01 18:00   ` James Y Knight
@ 2019-08-06  2:06     ` Rich Felker
  0 siblings, 0 replies; 10+ messages in thread
From: Rich Felker @ 2019-08-06  2:06 UTC (permalink / raw)
  To: musl

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@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.

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [musl] Support SIGEV_THREAD_ID
  2019-08-01 16:15 Support SIGEV_THREAD_ID James Y Knight
  2019-08-01 16:24 ` Florian Weimer
  2019-08-01 16:49 ` Rich Felker
@ 2020-08-18  0:32 ` Rich Felker
  2 siblings, 0 replies; 10+ messages in thread
From: Rich Felker @ 2020-08-18  0:32 UTC (permalink / raw)
  To: musl

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).

I'd like to go forward with this, along with adding gettid() which I'm
doing now.

> 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;
>  };

I was concerned about the padding size computation, but it seems to
check out:

Before the patch, if you adjust to make the sigev_notify_* members
overlap with the padding, you have 56-sizeof(long) == 56-sizeof(union
sigval) bytes of padding.

After the patch, it's 64-2*sizeof(int)-sizeof(union sigval) bytes of
padding, where 64-2*sizeof(int)==56.

> +#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'm still ok with using the macros unless/until we make a global
change to anon unions and structs. (And I'm aware of the C++ problem
raised before... :/)

>  #define SIGEV_SIGNAL 0
>  #define SIGEV_NONE 1
>  #define SIGEV_THREAD 2
> +#define SIGEV_THREAD_ID 4 /* Linux extension */

Comment not needed/wanted in header but I can drop it when merging.

>  int __libc_current_sigrtmin(void);
>  int __libc_current_sigrtmax(void);
> diff --git a/src/time/timer_create.c b/src/time/timer_create.c
> index c5e40a19..5a3f4d3a 100644
> --- a/src/time/timer_create.c
> +++ b/src/time/timer_create.c
> @@ -83,11 +83,15 @@ int timer_create(clockid_t clk, struct sigevent *restrict evp, timer_t *restrict
>  	switch (evp ? evp->sigev_notify : SIGEV_SIGNAL) {
>  	case SIGEV_NONE:
>  	case SIGEV_SIGNAL:
> +	case SIGEV_THREAD_ID:
>  		if (evp) {
>  			ksev.sigev_value = evp->sigev_value;
>  			ksev.sigev_signo = evp->sigev_signo;
>  			ksev.sigev_notify = evp->sigev_notify;
> -			ksev.sigev_tid = 0;
> +			if (evp->sigev_notify == SIGEV_THREAD_ID)
> +				ksev.sigev_tid = evp->sigev_notify_thread_id;
> +			else
> +				ksev.sigev_tid = 0;
>  			ksevp = &ksev;
>  		}
>  		if (syscall(SYS_timer_create, clk, ksevp, &timerid) < 0)
> @@ -115,7 +119,7 @@ int timer_create(clockid_t clk, struct sigevent *restrict evp, timer_t *restrict
>  
>  		ksev.sigev_value.sival_ptr = 0;
>  		ksev.sigev_signo = SIGTIMER;
> -		ksev.sigev_notify = 4; /* SIGEV_THREAD_ID */
> +		ksev.sigev_notify = SIGEV_THREAD_ID;
>  		ksev.sigev_tid = td->tid;
>  		if (syscall(SYS_timer_create, clk, &ksev, &timerid) < 0)
>  			timerid = -1;
> -- 
> 2.22.0.709.g102302147b-goog

This looks ok, but I think the same SIGEV_THREAD_ID support should be
added to aio. That can be done as a separate patch.

So I think this is OK in its current form.

Rich

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [musl] Support SIGEV_THREAD_ID
  2019-08-01 16:24 ` Florian Weimer
  2019-08-01 16:45   ` Rich Felker
@ 2020-10-30 21:22   ` Michael Forney
  2020-11-01  5:46     ` Khem Raj
  2020-11-02 12:55     ` Florian Weimer
  1 sibling, 2 replies; 10+ messages in thread
From: Michael Forney @ 2020-10-30 21:22 UTC (permalink / raw)
  To: Florian Weimer; +Cc: musl

On 2019-08-01, Florian Weimer <fweimer@redhat.com> wrote:
> * James Y. Knight:
>
>> 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.)
>
> The debate is over and has been decided in favor of supporting TIDs.  We
> just have a backlog of interfaces for which we need to add support.

Hi Florian,

Am I interpreting you correctly that glibc intends to add a
sigev_notify_thread_id define to access this field in struct sigevent?
I plan to send a patch to qemu to use sigev_notify_thread_id over
_sigev_un.tid if it is available, and I just want to confirm that
glibc intends to follow suit before I mention that in the commit
message.

Thanks

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [musl] Support SIGEV_THREAD_ID
  2020-10-30 21:22   ` [musl] " Michael Forney
@ 2020-11-01  5:46     ` Khem Raj
  2020-11-02 12:55     ` Florian Weimer
  1 sibling, 0 replies; 10+ messages in thread
From: Khem Raj @ 2020-11-01  5:46 UTC (permalink / raw)
  To: musl, Michael Forney, Florian Weimer

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



On 10/30/20 2:22 PM, Michael Forney wrote:
> On 2019-08-01, Florian Weimer <fweimer@redhat.com> wrote:
>> * James Y. Knight:
>>
>>> 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.)
>>
>> The debate is over and has been decided in favor of supporting TIDs.  We
>> just have a backlog of interfaces for which we need to add support.
> 
> Hi Florian,
> 
> Am I interpreting you correctly that glibc intends to add a
> sigev_notify_thread_id define to access this field in struct sigevent?
> I plan to send a patch to qemu to use sigev_notify_thread_id over
> _sigev_un.tid if it is available, and I just want to confirm that
> glibc intends to follow suit before I mention that in the commit
> message.

we will need something similar for gperftools as well.

> 
> Thanks
> 

[-- Attachment #2: pEpkey.asc --]
[-- Type: application/pgp-keys, Size: 2373 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [musl] Support SIGEV_THREAD_ID
  2020-10-30 21:22   ` [musl] " Michael Forney
  2020-11-01  5:46     ` Khem Raj
@ 2020-11-02 12:55     ` Florian Weimer
  1 sibling, 0 replies; 10+ messages in thread
From: Florian Weimer @ 2020-11-02 12:55 UTC (permalink / raw)
  To: Michael Forney; +Cc: musl

* Michael Forney:

> On 2019-08-01, Florian Weimer <fweimer@redhat.com> wrote:
>> * James Y. Knight:
>>
>>> 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.)
>>
>> The debate is over and has been decided in favor of supporting TIDs.  We
>> just have a backlog of interfaces for which we need to add support.
>
> Hi Florian,
>
> Am I interpreting you correctly that glibc intends to add a
> sigev_notify_thread_id define to access this field in struct sigevent?
> I plan to send a patch to qemu to use sigev_notify_thread_id over
> _sigev_un.tid if it is available, and I just want to confirm that
> glibc intends to follow suit before I mention that in the commit
> message.

I do not expect that we are going to add a sigev_notify_thread_id member
once glibc implements the struct member.  On common configurations, it's
likely that we are going to use a different approach, using anonymous
structs and unions.

I think you will have to use a different mechanism for detecting the
presence of the struct member.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-11-02 12:55 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-01 16:15 Support SIGEV_THREAD_ID James Y Knight
2019-08-01 16:24 ` Florian Weimer
2019-08-01 16:45   ` Rich Felker
2020-10-30 21:22   ` [musl] " Michael Forney
2020-11-01  5:46     ` Khem Raj
2020-11-02 12:55     ` Florian Weimer
2019-08-01 16:49 ` Rich Felker
2019-08-01 18:00   ` James Y Knight
2019-08-06  2:06     ` Rich Felker
2020-08-18  0:32 ` [musl] " Rich Felker

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).