Re: Support SIGEV_THREAD_ID

[Rich Felker <dalias@libc.org>]

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