Re: [PATCH 09/15] Hurd signals: implement global signal dispositions

[Samuel Thibault <samuel dot thibault at gnu dot org>]

Jeremie Koenig, le Wed 29 Jun 2011 18:30:21 +0200, a écrit :
> Currently each thread has a full "sigstate" structure which keeps track
> of the signal dispositions, blocking mask and pending signals for this
> thread. Process-wide signals are delivered to the main thread.
> 
> However, the semantics for POSIX threads is that all of them share the
> same signal dispositions, although their blocking masks may differ.
> Signals sent to the process as a whole can be delivered to any thread
> which does not block them.
> 
> This is implemented here in addition to the current behavior: libpthread
> will call _hurd_sigstate_set_global_rcv to mark newly created threads as
> global signal receivers, while cthreads-based programs can continue to
> rely on the Hurd semantics.

Agreed, just a few details following.

Jeremie Koenig, le Wed 29 Jun 2011 18:30:21 +0200, a écrit :
> diff --git a/hurd/hurdsig.c b/hurd/hurdsig.c
> index 74a01a6..eaf5ac1 100644
> --- a/hurd/hurdsig.c
> +++ b/hurd/hurdsig.c
> +sigstate_is_global_rcv (const struct hurd_sigstate *ss)
> +_hurd_sigstate_lock (struct hurd_sigstate *ss)
> +_hurd_sigstate_unlock (struct hurd_sigstate *ss)
> +_hurd_sigstate_pending (const struct hurd_sigstate *ss)
> +sigstate_clear_pending (struct hurd_sigstate *ss, int signo)
> +_hurd_sigstate_actions (struct hurd_sigstate *ss)

These should probably be made extern inline.

> diff --git a/hurd/hurdmsg.c b/hurd/hurdmsg.c
> index ffcce61..85b87aa 100644
> --- a/hurd/hurdmsg.c
> +++ b/hurd/hurdmsg.c
> @@ -124,7 +125,7 @@ get_int (int which, int *value)
>        return 0;
>      case INIT_SIGMASK:
>        {
> -	struct hurd_sigstate *ss = _hurd_thread_sigstate (_hurd_sigthread);
> +	struct hurd_sigstate *ss = _hurd_global_sigstate;
>  	__spin_lock (&ss->lock);
>  	*value = ss->blocked;
>  	__spin_unlock (&ss->lock);

Mmm, this should be left as _hurd_thread_sigstate: the global sigstate's
blocked field does not make much sense, right?

> @@ -210,7 +211,7 @@ set_int (int which, int value)
>        /* These are pretty odd things to do.  But you asked for it.  */
>      case INIT_SIGMASK:
>        {
> -	struct hurd_sigstate *ss = _hurd_thread_sigstate (_hurd_sigthread);
> +	struct hurd_sigstate *ss = _hurd_global_sigstate;
>  	__spin_lock (&ss->lock);
>  	ss->blocked = value;
>  	__spin_unlock (&ss->lock);

Likewise.

> diff --git a/hurd/hurdsig.c b/hurd/hurdsig.c
> index 74a01a6..eaf5ac1 100644
> --- a/hurd/hurdsig.c
> +++ b/hurd/hurdsig.c
> @@ -83,7 +83,7 @@ _hurd_thread_sigstate (thread_t thread)
>      {
>        ss = malloc (sizeof (*ss));
>        if (ss == NULL)
> -	__libc_fatal ("hurd: Can't allocate thread sigstate\n");
> +	__libc_fatal ("hurd: Can't allocate sigstate\n");
>        ss->thread = thread;
>        __spin_lock_init (&ss->lock);
>  

This seems spurious?

> +         /* The global sigstate is not added to the _hurd_sigstates list,
> +            and while it is created using _hurd_sigthread (MACH_PORT_NULL),

_hurd_thread_sigstate (MACH_PORT_NULL)

>    /* Check for a preempted signal.  Preempted signals can arrive during
>       critical sections.  */
> +  /* XXX how does this mix with _hurd_global_sigstate?  Should its PREEMPTORS
> +   * field replace _hurdsig_preemptors?  */

It could be an idea, indeed.

> -  /* Post the signal to the designated signal-receiving thread.  This will
> +  /* Post the signal to XXX[the designated signal-receiving thread.]  This will

Are you looking for a way to explain that either the signal-receiving
thread, or any thread, will receiving, depending on pthread vs cthreads?

> @@ -1258,8 +1343,8 @@ extern void __mig_init (void *);
>  
>  #include <mach/task_special_ports.h>
>  
> -/* Initialize the message port and _hurd_sigthread and start the signal
> -   thread.  */
> +/* Initialize the message port, _hurd_sigthread and _hurd_global_sigstate
> +   and start the signal thread.  */

_hurd_sigthread does not exist any more with the patch.

> diff --git a/sysdeps/mach/hurd/fork.c b/sysdeps/mach/hurd/fork.c
> index 3288f18..a4f3055 100644
> --- a/sysdeps/mach/hurd/fork.c
> +++ b/sysdeps/mach/hurd/fork.c
> @@ -459,6 +459,7 @@ __fork (void)
>  	 function, accounted for by mach_port_names (and which will thus be
>  	 accounted for in the child below).  This extra right gets consumed
>  	 in the child by the store into _hurd_sigthread in the child fork.  */
> +      /* XXX consumed? (_hurd_sigthread is no more) */
>        if (thread_refs > 1 &&
>  	  (err = __mach_port_mod_refs (newtask, ss->thread,
>  				       MACH_PORT_RIGHT_SEND,

I don't understand either. I don't see port references around
the existing _hurd_sigthread management.

> diff --git a/sysdeps/mach/hurd/i386/trampoline.c b/sysdeps/mach/hurd/i386/trampoline.c
> index 99d9308..ec52847 100644
> --- a/sysdeps/mach/hurd/i386/trampoline.c
> +++ b/sysdeps/mach/hurd/i386/trampoline.c
> @@ -77,7 +77,11 @@ _hurd_setup_sighandler (struct hurd_sigstate *ss, __sighandler_t handler,
>       interrupted RPC frame.  */
>    state->basic.esp = state->basic.uesp;
>  
> -  if ((ss->actions[signo].sa_flags & SA_ONSTACK) &&
> +  /* XXX what if handler != action->handler (for instance, if a signal
> +   * preemptor took over) ? */
> +  action = & _hurd_sigstate_actions (ss) [signo];
> +
> +  if ((action->sa_flags & SA_ONSTACK) &&

The SA_ONSTACK logic remains the same.  I believe this is right.  In
the Hurd semantic case your patch does not change the behavior.  In the
POSIX semantic all threads share the SA_ONSTACK logic.  This is what
POSIX says for signals.  I don't think it is a problem for preemptors.

> diff --git a/sysdeps/mach/hurd/spawni.c b/sysdeps/mach/hurd/spawni.c
> index 244ca2d..373da8d 100644
> --- a/sysdeps/mach/hurd/spawni.c
> +++ b/sysdeps/mach/hurd/spawni.c
> @@ -239,26 +239,29 @@ __spawni (pid_t *pid, const char *file,
>    assert (! __spin_lock_locked (&ss->critical_section_lock));
>    __spin_lock (&ss->critical_section_lock);
>  
> -  __spin_lock (&ss->lock);
> +  _hurd_sigstate_lock (ss);
>    ints[INIT_SIGMASK] = ss->blocked;
> -  ints[INIT_SIGPENDING] = ss->pending;
> +  ints[INIT_SIGPENDING] = _hurd_sigstate_pending (ss); /* XXX really? */

Mmm. According to POSIX, fork() is supposed to clear pending signals,
but GNU/Hurd currently does not clear them. According to POSIX,
exec() is supposed to not clear pending signals. So at least, spawn()
inheriting pending signals is coherent in GNU/Hurd. Making fork() and
spwan() clear pending signals would be a separate fix.

Samuel