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 <j...@jk.fr.eu.org>
http://jk.fr.eu.org
