Re: SIGCHLD ignored

[Vladimir Prus <ghost at cs dot msu dot su>]

On Friday 13 June 2008 03:23:45 Pedro Alves wrote:
> A Thursday 12 June 2008 17:36:17, Vladimir Prus escreveu:
> > On Thursday 12 June 2008 03:18:05 Pedro Alves wrote:
> > > [moved to gdb-patches@]
> > >
> > > Original report here:
> > >  SIGCHLD ignored
> > >  http://sourceware.org/ml/gdb/2008-06/msg00099.html
> > >
> > > A Wednesday 11 June 2008 23:45:22, Pedro Alves wrote:
> > > > A Wednesday 11 June 2008 18:21:05, Vladimir Prus wrote:
> > > > > Pedro, I think this SIGCHLD magic is your doing -- do you have any
> > > > > ideas how to fix it?
> > > >
> > > > Dang, I totally missed this could be a problem.
> > > >
> > > > Right, so, normal_mask has SIGCHLD blocked in _initialize_linux_nat,
> > > > and since forking a child inherits the signal mask, the child gets
> > > > SIGCHLD blocked by default.  This would have gone unnoticed
> > > > if Qt unblocked SIGCHLD in it's setup (one may argue it should).
> > > >
> > > > It's not safe to just remove that SIGCHLD blocking, we're not
> > > > taking care of blocking it in places we used to in sync mode
> > > > (we used to block it the first time we hit linux_nat_wait, which
> > > > is reached after forking an inferior).
> > > >
> > > > For async mode, we also need to do something about SIGCHLD
> > > > blocking.  We have:
> > > >
> > > > linux_nat_create_inferior
> > > >   -> linux_nat_async_mask
> > > >      -> linux_nat_async
> > > >         -> linux_nat_async_events
> > > >            -> block SIGCHLD.
> > > >
> > > >   -> linux_ops->to_create_inferior (forks a child)
> > > >
> > > > Attached is a patch to fix this.
> > > >
> > > > Basically, I turned linux_nat_async_events_enabled into a
> > > > 3-state instead of a bool, with the new state being
> > > > "SIGCHLD unblocked with action set to whatever we get
> > > > on startup".  Most of the SIGCHLD state management stays with
> > > > the same logic, except that linux_nat_create_inferior gets a
> > > > switch to the new state before forking a child, and
> > > > linux_nat_wait, gets an unconditional switch to the blocked
> > > > state.  The rest of the patch is mostly updating to the
> > > > new interface.
> > > >
> > > > Tested on x86-64-unknown-linux-gnu sync and async modes
> > > > without regressions.
> > >
> > > Same code patch, but updated with a new testcase.  I imagine
> > > we're going to touch these issues again with full
> > > multi-process support.
> >
> 
> I had a report that this patch actually fixed a race that triggered
> an assertion in combination with non-stop mode; and, I just found
> another similar race in linux_nat_async_events.
> 
> Here's an updated patch that fixes it.

For my education, can you clarify what the race was? This patch differs by this block:

+      /* Always block before changing state.  */
+      sigprocmask (SIG_BLOCK, &mask, NULL);
+
+      /* Set new state.  */
+      linux_nat_async_events_state = state;

What harm will the race cause, except for internal_error? I mean,
if we have sync sigchld handler installed, then if SIGCHLD arrives when we're
not inside sigsuspend, it's just ignored. If we have async sigchld handler installed,
then everything should be working, because even if some futher events arrive,
will put them in the pipe, and further in the function, call pipe_to_local_event_queue,
so no event will be lost.

I realize that the asserts in sigchld handlers are meant to catch problems, and we
can't remove them -- I just wonder if in this case we have a real problem due to race.

Thanks,
Volodya