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

[Jeremie Koenig <jk at jk dot fr dot eu dot org>]

On Sun, Jul 03, 2011 at 02:16:34AM +0200, Samuel Thibault wrote:
> Jeremie Koenig, le Wed 29 Jun 2011 18:30:21 +0200, a écrit :
> > +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.

I agree as far as internal libc usage is concerned. However,

  - sigstate_is_global_rcv and sigstate_clear_pending are static
    functions, which I think gcc will inline if appropriate;
  - _hurd_sigstate_actions is not actually exported by hurd/Versions.
  - _hurd_sigstate_lock/unlock use _hurd_global_sigstate, which is not
    exported either.

What is the status of <hurd/signal.h> from a user point of view?
Is it supposed to be used beyond libpthread and hurd?

> > 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?

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.

The only straightforward way to fix this that I can see would be to
reinstate _hurd_sigthread (maybe as "_hurd_mainthread"), so that we can
distinguish the main thread from other global signal receivers (in the
libpthread case).

Or maybe we could drop this and let them return EINVAL?

> > 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?

Since _hurd_thread_sigstate is now used to allocate _hurd_global_sigstate
as well as "thread sigstates", I felt it would be more accurate to
change the message to reflect that.

I'm not sure it's actually useful, though.

> > +         /* 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)

Fixed, thanks.

> >    /* 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.

However it would convey the impression that it's only used for global
receiver threads. I think I'll just drop the comment.

> > -  /* 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?

Yes. One way of putting it would be "a global receiver thread".
However, if the signal is marked pending in _hurd_global_sigstate, it is
not exactly delivered to any thread in particular.

I'll go with the following for now, suggestions are welcome.

  /* Post the signal to a global receiver thread (or mark it pending in
     the global sigstate).  This will reply when the signal can be
     considered delivered.  */

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

Fixed, thanks.

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

As an aside, could mach_notify_<something> be used to eliminate stale
sigstate structures? It would be much cleaner than relying on
libpthread's help, as the _hurd_sigstate_delete patch currently does.

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

I was worried about a case where the user would install a minuscule
stack, sufficient for their own handler, but not for a preemptor's one.
(But you're right that this is not a new problem).

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

I'll make a new patch.

Thanks,
-- 
Jeremie Koenig <jk@jk.fr.eu.org>
http://jk.fr.eu.org