Re: SIGCHLD ignored

Re: SIGCHLD ignored

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 1938 bytes --]

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

-- 
Pedro Alves

[-- Attachment #2: sigchld.diff --]
[-- Type: text/x-diff, Size: 14477 bytes --]

gdb/
2008-06-12  Pedro Alves  <pedro@codesourcery.com>

	* linux-nat.c (enum sigchld_state): New.
	(linux_nat_async_events_state): Renamed from
	linux_nat_async_events_enabled.
	(linux_nat_event_pipe_push, my_waitpid): Adjust.
	(sigchld_default_action): New.
	(lin_lwp_attach_lwp): Adjust.  Call linux_nat_async_events
	unconditionally.
	(linux_nat_create_inferior): Set events state to sigchld_default
	state.
	(linux_nat_resume): Adjust.
	(linux_nat_wait): Call linux_nat_async_events unconditionally.
	(sigchld_handler): Adjust.
	(linux_nat_async_mask): Don't set SIGCHLD actions here.
	(get_pending_events): Adjust.
	(linux_nat_async_events): Rewrite to handle enum sigchld_state
	instead of a boolean.
	(linux_nat_async): Adjust.
	(_initialize_linux_nat): Capture default SIGCHLD action into
	sigchld_default_action.

gdb/testsuite/
2008-06-12  Pedro Alves  <pedro@codesourcery.com>

	* gdb.base/sigchld.c, gdb.base/sigchld.exp: New test.
	
---
 gdb/linux-nat.c                    |  154 +++++++++++++++++++++++--------------
 gdb/testsuite/gdb.base/sigchld.c   |   37 ++++++++
 gdb/testsuite/gdb.base/sigchld.exp |   45 ++++++++++
 3 files changed, 179 insertions(+), 57 deletions(-)

Index: src/gdb/linux-nat.c
===================================================================
--- src.orig/gdb/linux-nat.c	2008-06-11 23:37:14.000000000 +0100
+++ src/gdb/linux-nat.c	2008-06-12 00:12:14.000000000 +0100
@@ -196,11 +196,24 @@ static int linux_nat_event_pipe[2] = { -
 /* Number of queued events in the pipe.  */
 static volatile int linux_nat_num_queued_events;
 
-/* If async mode is on, true if we're listening for events; false if
-   target events are blocked.  */
-static int linux_nat_async_events_enabled;
+/* The possible SIGCHLD handling states.  */
 
-static int linux_nat_async_events (int enable);
+enum sigchld_state
+{
+  /* SIGCHLD disabled, with action set to sigchld_handler, for the
+     sigsuspend in linux_nat_wait.  */
+  sigchld_sync,
+  /* SIGCHLD enabled, with action set to async_sigchld_handler.  */
+  sigchld_async,
+  /* Set SIGCHLD to default action.  Used while creating an
+     inferior.  */
+  sigchld_default
+};
+
+/* The current SIGCHLD handling state.  */
+static enum sigchld_state linux_nat_async_events_state;
+
+static enum sigchld_state linux_nat_async_events (enum sigchld_state enable);
 static void pipe_to_local_event_queue (void);
 static void local_event_queue_to_pipe (void);
 static void linux_nat_event_pipe_push (int pid, int status, int options);
@@ -234,8 +247,8 @@ queued_waitpid (int pid, int *status, in
   if (debug_linux_nat_async)
     fprintf_unfiltered (gdb_stdlog,
 			"\
-QWPID: linux_nat_async_events_enabled(%d), linux_nat_num_queued_events(%d)\n",
-			linux_nat_async_events_enabled,
+QWPID: linux_nat_async_events_state(%d), linux_nat_num_queued_events(%d)\n",
+			linux_nat_async_events_state,
 			linux_nat_num_queued_events);
 
   if (flags & __WALL)
@@ -381,7 +394,7 @@ my_waitpid (int pid, int *status, int fl
   int ret;
 
   /* There should be no concurrent calls to waitpid.  */
-  gdb_assert (!linux_nat_async_events_enabled);
+  gdb_assert (linux_nat_async_events_state != sigchld_async);
 
   ret = queued_waitpid (pid, status, flags);
   if (ret != -1)
@@ -813,6 +826,9 @@ struct sigaction sync_sigchld_action;
 
 /* SIGCHLD action for asynchronous mode.  */
 static struct sigaction async_sigchld_action;
+
+/* SIGCHLD default action, to pass to new inferiors.  */
+static struct sigaction sigchld_default_action;
 \f
 
 /* Prototypes for local functions.  */
@@ -1136,12 +1152,11 @@ int
 lin_lwp_attach_lwp (ptid_t ptid)
 {
   struct lwp_info *lp;
-  int async_events_were_enabled = 0;
+  enum sigchld_state async_events_original_state;
 
   gdb_assert (is_lwp (ptid));
 
-  if (target_can_async_p ())
-    async_events_were_enabled = linux_nat_async_events (0);
+  async_events_original_state = linux_nat_async_events (sigchld_sync);
 
   lp = find_lwp_pid (ptid);
 
@@ -1206,9 +1221,7 @@ lin_lwp_attach_lwp (ptid_t ptid)
       lp->stopped = 1;
     }
 
-  if (async_events_were_enabled)
-    linux_nat_async_events (1);
-
+  linux_nat_async_events (async_events_original_state);
   return 0;
 }
 
@@ -1222,6 +1235,8 @@ linux_nat_create_inferior (char *exec_fi
      we have to mask the async mode.  */
 
   if (target_can_async_p ())
+    /* Mask async mode.  Creating a child requires a loop calling
+       wait_for_inferior currently.  */
     saved_async = linux_nat_async_mask (0);
   else
     {
@@ -1232,6 +1247,12 @@ linux_nat_create_inferior (char *exec_fi
       sigdelset (&suspend_mask, SIGCHLD);
     }
 
+  /* Set SIGCHLD to the default action, until after execing the child,
+     since the inferior inherits the superior's signal mask.  It will
+     be blocked again in linux_nat_wait, which is only reached after
+     the inferior execing.  */
+  linux_nat_async_events (sigchld_default);
+
   linux_ops->to_create_inferior (exec_file, allargs, env, from_tty);
 
   if (saved_async)
@@ -1463,7 +1484,7 @@ linux_nat_resume (ptid_t ptid, int step,
 
   if (target_can_async_p ())
     /* Block events while we're here.  */
-    linux_nat_async_events (0);
+    linux_nat_async_events (sigchld_sync);
 
   /* A specific PTID means `step only this process id'.  */
   resume_all = (PIDGET (ptid) == -1);
@@ -2525,9 +2546,8 @@ linux_nat_wait (ptid_t ptid, struct targ
 
   sigemptyset (&flush_mask);
 
-  if (target_can_async_p ())
-    /* Block events while we're here.  */
-    target_async (NULL, 0);
+  /* Block events while we're here.  */
+  linux_nat_async_events (sigchld_sync);
 
 retry:
 
@@ -2986,7 +3006,7 @@ static void
 sigchld_handler (int signo)
 {
   if (linux_nat_async_enabled
-      && linux_nat_async_events_enabled
+      && linux_nat_async_events_state != sigchld_sync
       && signo == SIGCHLD)
     /* It is *always* a bug to hit this.  */
     internal_error (__FILE__, __LINE__,
@@ -3845,15 +3865,9 @@ linux_nat_async_mask (int mask)
 	{
 	  linux_nat_async (NULL, 0);
 	  linux_nat_async_mask_value = mask;
-	  /* We're in sync mode.  Make sure SIGCHLD isn't handled by
-	     async_sigchld_handler when we come out of sigsuspend in
-	     linux_nat_wait.  */
-	  sigaction (SIGCHLD, &sync_sigchld_action, NULL);
 	}
       else
 	{
-	  /* Restore the async handler.  */
-	  sigaction (SIGCHLD, &async_sigchld_action, NULL);
 	  linux_nat_async_mask_value = mask;
 	  linux_nat_async (inferior_event_handler, 0);
 	}
@@ -3911,7 +3925,8 @@ get_pending_events (void)
 {
   int status, options, pid;
 
-  if (!linux_nat_async_enabled || !linux_nat_async_events_enabled)
+  if (!linux_nat_async_enabled
+      || linux_nat_async_events_state != sigchld_async)
     internal_error (__FILE__, __LINE__,
 		    "get_pending_events called with async masked");
 
@@ -3965,44 +3980,71 @@ async_sigchld_handler (int signo)
   get_pending_events ();
 }
 
-/* Enable or disable async SIGCHLD handling.  */
+/* Set SIGCHLD handling state to STATE.  Returns previous state.  */
 
-static int
-linux_nat_async_events (int enable)
+static enum sigchld_state
+linux_nat_async_events (enum sigchld_state state)
 {
-  int current_state = linux_nat_async_events_enabled;
+  enum sigchld_state current_state = linux_nat_async_events_state;
 
   if (debug_linux_nat_async)
     fprintf_unfiltered (gdb_stdlog,
-			"LNAE: enable(%d): linux_nat_async_events_enabled(%d), "
+			"LNAE: state(%d): linux_nat_async_events_state(%d), "
 			"linux_nat_num_queued_events(%d)\n",
-			enable, linux_nat_async_events_enabled,
+			state, linux_nat_async_events_state,
 			linux_nat_num_queued_events);
 
-  if (current_state != enable)
+  if (current_state != state)
     {
       sigset_t mask;
       sigemptyset (&mask);
       sigaddset (&mask, SIGCHLD);
-      if (enable)
-	{
-	  /* Unblock target events.  */
-	  linux_nat_async_events_enabled = 1;
 
-	  local_event_queue_to_pipe ();
-	  /* While in masked async, we may have not collected all the
-	     pending events.  Get them out now.  */
-	  get_pending_events ();
-	  sigprocmask (SIG_UNBLOCK, &mask, NULL);
-	}
-      else
+      linux_nat_async_events_state = state;
+
+      switch (state)
 	{
-	  /* Block target events.  */
-	  sigprocmask (SIG_BLOCK, &mask, NULL);
-	  linux_nat_async_events_enabled = 0;
-	  /* Get events out of queue, and make them available to
-	     queued_waitpid / my_waitpid.  */
-	  pipe_to_local_event_queue ();
+	case sigchld_sync:
+	  {
+	    /* Block target events.  */
+	    sigprocmask (SIG_BLOCK, &mask, NULL);
+	    sigaction (SIGCHLD, &sync_sigchld_action, NULL);
+	    /* Get events out of queue, and make them available to
+	       queued_waitpid / my_waitpid.  */
+	    pipe_to_local_event_queue ();
+	  }
+	  break;
+	case sigchld_async:
+	  {
+	    /* Unblock target events for async mode.  */
+
+	    sigprocmask (SIG_BLOCK, &mask, NULL);
+
+	    /* Put events we already waited on, in the pipe first, so
+	       events are FIFO.  */
+	    local_event_queue_to_pipe ();
+	    /* While in masked async, we may have not collected all
+	       the pending events.  Get them out now.  */
+	    get_pending_events ();
+
+	    /* Let'em come.   */
+	    sigaction (SIGCHLD, &async_sigchld_action, NULL);
+	    sigprocmask (SIG_UNBLOCK, &mask, NULL);
+	  }
+	  break;
+	case sigchld_default:
+	  {
+	    /* SIGCHLD default mode.  */
+	    sigaction (SIGCHLD, &sigchld_default_action, NULL);
+
+	    /* Get events out of queue, and make them available to
+	       queued_waitpid / my_waitpid.  */
+	    pipe_to_local_event_queue ();
+
+	    /* Unblock SIGCHLD.  */
+	    sigprocmask (SIG_UNBLOCK, &mask, NULL);
+	  }
+	  break;
 	}
     }
 
@@ -4094,14 +4136,14 @@ linux_nat_async (void (*callback) (enum 
       add_file_handler (linux_nat_event_pipe[0],
 			linux_nat_async_file_handler, NULL);
 
-      linux_nat_async_events (1);
+      linux_nat_async_events (sigchld_async);
     }
   else
     {
       async_client_callback = callback;
       async_client_context = context;
 
-      linux_nat_async_events (0);
+      linux_nat_async_events (sigchld_sync);
       delete_file_handler (linux_nat_event_pipe[0]);
     }
   return;
@@ -4117,21 +4159,15 @@ linux_nat_set_async_mode (int on)
       if (on)
 	{
 	  gdb_assert (waitpid_queue == NULL);
-	  sigaction (SIGCHLD, &async_sigchld_action, NULL);
-
 	  if (pipe (linux_nat_event_pipe) == -1)
 	    internal_error (__FILE__, __LINE__,
 			    "creating event pipe failed.");
-
 	  fcntl (linux_nat_event_pipe[0], F_SETFL, O_NONBLOCK);
 	  fcntl (linux_nat_event_pipe[1], F_SETFL, O_NONBLOCK);
 	}
       else
 	{
-	  sigaction (SIGCHLD, &sync_sigchld_action, NULL);
-
 	  drain_queued_events (-1);
-
 	  linux_nat_num_queued_events = 0;
 	  close (linux_nat_event_pipe[0]);
 	  close (linux_nat_event_pipe[1]);
@@ -4248,6 +4284,10 @@ Tells gdb whether to control the GNU/Lin
 			   &maintenance_set_cmdlist,
 			   &maintenance_show_cmdlist);
 
+  /* Get the default SIGCHLD action.  Used while forking an inferior
+     (see linux_nat_create_inferior/linux_nat_async_events).  */
+  sigaction (SIGCHLD, NULL, &sigchld_default_action);
+
   /* Block SIGCHLD by default.  Doing this early prevents it getting
      unblocked if an exception is thrown due to an error while the
      inferior is starting (sigsetjmp/siglongjmp).  */
Index: src/gdb/testsuite/gdb.base/sigchld.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.base/sigchld.c	2008-06-12 00:05:36.000000000 +0100
@@ -0,0 +1,37 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2008 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+/* Check that GDB isn't messing the SIGCHLD mask while creating an
+   inferior.  */
+
+#include <signal.h>
+#include <stdlib.h>
+
+int
+main ()
+{
+  sigset_t mask;
+
+  sigemptyset (&mask);
+  sigprocmask (SIG_BLOCK, NULL, &mask);
+
+  if (!sigismember (&mask, SIGCHLD))
+    return 0; /* good, not blocked */
+  else
+    return 1; /* bad, blocked */
+}
Index: src/gdb/testsuite/gdb.base/sigchld.exp
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.base/sigchld.exp	2008-06-12 00:10:54.000000000 +0100
@@ -0,0 +1,45 @@
+# Copyright (C) 2008 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Check that GDB isn't messing the SIGCHLD mask while creating an
+# inferior.
+
+if [target_info exists gdb,nosignals] {
+    verbose "Skipping sigchld.exp because of nosignals."
+    continue
+}
+
+set testfile "sigchld"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+if {[gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+    return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+runto_main
+
+gdb_test "b [gdb_get_line_number "good, not blocked"]" \
+         ".*Breakpoint .*sigchld.*" "set breakpoint at success exit"
+
+gdb_test "b [gdb_get_line_number "bad, blocked"]" \
+         ".*Breakpoint .*sigchld.*" "set breakpoint at failure exit"
+
+gdb_test "continue" ".*good, not blocked.*" "SIGCHLD blocked in inferior"

Re: SIGCHLD ignored

[Vladimir Prus]

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.

This patch works fine for me, and (with my limited understanding of the code),
makes sense. Can we get it in?

- Volodya

Re: SIGCHLD ignored

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 2481 bytes --]

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.

-- 
Pedro Alves

[-- Attachment #2: sigchld.diff --]
[-- Type: text/x-diff, Size: 14603 bytes --]

gdb/
2008-06-13  Pedro Alves  <pedro@codesourcery.com>

	* linux-nat.c (enum sigchld_state): New.
	(linux_nat_async_events_state): Renamed from
	linux_nat_async_events_enabled.
	(linux_nat_event_pipe_push, my_waitpid): Adjust.
	(sigchld_default_action): New.
	(lin_lwp_attach_lwp): Adjust.  Call linux_nat_async_events
	unconditionally.
	(linux_nat_create_inferior): Set events state to sigchld_default
	state.
	(linux_nat_resume): Adjust.
	(linux_nat_wait): Call linux_nat_async_events unconditionally.
	(sigchld_handler): Adjust.
	(linux_nat_async_mask): Don't set SIGCHLD actions here.
	(get_pending_events): Adjust.
	(linux_nat_async_events): Rewrite to handle enum sigchld_state
	instead of a boolean.
	(linux_nat_async): Adjust.
	(_initialize_linux_nat): Capture default SIGCHLD action into
	sigchld_default_action.

gdb/testsuite/
2008-06-13  Pedro Alves  <pedro@codesourcery.com>

	* gdb.base/sigchld.c, gdb.base/sigchld.exp: New test.
	
---
 gdb/linux-nat.c                    |  158 +++++++++++++++++++++++--------------
 gdb/testsuite/gdb.base/sigchld.c   |   37 ++++++++
 gdb/testsuite/gdb.base/sigchld.exp |   45 ++++++++++
 3 files changed, 183 insertions(+), 57 deletions(-)

Index: src/gdb/linux-nat.c
===================================================================
--- src.orig/gdb/linux-nat.c	2008-06-11 23:37:14.000000000 +0100
+++ src/gdb/linux-nat.c	2008-06-13 00:16:50.000000000 +0100
@@ -196,11 +196,24 @@ static int linux_nat_event_pipe[2] = { -
 /* Number of queued events in the pipe.  */
 static volatile int linux_nat_num_queued_events;
 
-/* If async mode is on, true if we're listening for events; false if
-   target events are blocked.  */
-static int linux_nat_async_events_enabled;
+/* The possible SIGCHLD handling states.  */
 
-static int linux_nat_async_events (int enable);
+enum sigchld_state
+{
+  /* SIGCHLD disabled, with action set to sigchld_handler, for the
+     sigsuspend in linux_nat_wait.  */
+  sigchld_sync,
+  /* SIGCHLD enabled, with action set to async_sigchld_handler.  */
+  sigchld_async,
+  /* Set SIGCHLD to default action.  Used while creating an
+     inferior.  */
+  sigchld_default
+};
+
+/* The current SIGCHLD handling state.  */
+static enum sigchld_state linux_nat_async_events_state;
+
+static enum sigchld_state linux_nat_async_events (enum sigchld_state enable);
 static void pipe_to_local_event_queue (void);
 static void local_event_queue_to_pipe (void);
 static void linux_nat_event_pipe_push (int pid, int status, int options);
@@ -234,8 +247,8 @@ queued_waitpid (int pid, int *status, in
   if (debug_linux_nat_async)
     fprintf_unfiltered (gdb_stdlog,
 			"\
-QWPID: linux_nat_async_events_enabled(%d), linux_nat_num_queued_events(%d)\n",
-			linux_nat_async_events_enabled,
+QWPID: linux_nat_async_events_state(%d), linux_nat_num_queued_events(%d)\n",
+			linux_nat_async_events_state,
 			linux_nat_num_queued_events);
 
   if (flags & __WALL)
@@ -381,7 +394,7 @@ my_waitpid (int pid, int *status, int fl
   int ret;
 
   /* There should be no concurrent calls to waitpid.  */
-  gdb_assert (!linux_nat_async_events_enabled);
+  gdb_assert (linux_nat_async_events_state != sigchld_async);
 
   ret = queued_waitpid (pid, status, flags);
   if (ret != -1)
@@ -813,6 +826,9 @@ struct sigaction sync_sigchld_action;
 
 /* SIGCHLD action for asynchronous mode.  */
 static struct sigaction async_sigchld_action;
+
+/* SIGCHLD default action, to pass to new inferiors.  */
+static struct sigaction sigchld_default_action;
 \f
 
 /* Prototypes for local functions.  */
@@ -1136,12 +1152,11 @@ int
 lin_lwp_attach_lwp (ptid_t ptid)
 {
   struct lwp_info *lp;
-  int async_events_were_enabled = 0;
+  enum sigchld_state async_events_original_state;
 
   gdb_assert (is_lwp (ptid));
 
-  if (target_can_async_p ())
-    async_events_were_enabled = linux_nat_async_events (0);
+  async_events_original_state = linux_nat_async_events (sigchld_sync);
 
   lp = find_lwp_pid (ptid);
 
@@ -1206,9 +1221,7 @@ lin_lwp_attach_lwp (ptid_t ptid)
       lp->stopped = 1;
     }
 
-  if (async_events_were_enabled)
-    linux_nat_async_events (1);
-
+  linux_nat_async_events (async_events_original_state);
   return 0;
 }
 
@@ -1222,6 +1235,8 @@ linux_nat_create_inferior (char *exec_fi
      we have to mask the async mode.  */
 
   if (target_can_async_p ())
+    /* Mask async mode.  Creating a child requires a loop calling
+       wait_for_inferior currently.  */
     saved_async = linux_nat_async_mask (0);
   else
     {
@@ -1232,6 +1247,12 @@ linux_nat_create_inferior (char *exec_fi
       sigdelset (&suspend_mask, SIGCHLD);
     }
 
+  /* Set SIGCHLD to the default action, until after execing the child,
+     since the inferior inherits the superior's signal mask.  It will
+     be blocked again in linux_nat_wait, which is only reached after
+     the inferior execing.  */
+  linux_nat_async_events (sigchld_default);
+
   linux_ops->to_create_inferior (exec_file, allargs, env, from_tty);
 
   if (saved_async)
@@ -1463,7 +1484,7 @@ linux_nat_resume (ptid_t ptid, int step,
 
   if (target_can_async_p ())
     /* Block events while we're here.  */
-    linux_nat_async_events (0);
+    linux_nat_async_events (sigchld_sync);
 
   /* A specific PTID means `step only this process id'.  */
   resume_all = (PIDGET (ptid) == -1);
@@ -2525,9 +2546,8 @@ linux_nat_wait (ptid_t ptid, struct targ
 
   sigemptyset (&flush_mask);
 
-  if (target_can_async_p ())
-    /* Block events while we're here.  */
-    target_async (NULL, 0);
+  /* Block events while we're here.  */
+  linux_nat_async_events (sigchld_sync);
 
 retry:
 
@@ -2986,7 +3006,7 @@ static void
 sigchld_handler (int signo)
 {
   if (linux_nat_async_enabled
-      && linux_nat_async_events_enabled
+      && linux_nat_async_events_state != sigchld_sync
       && signo == SIGCHLD)
     /* It is *always* a bug to hit this.  */
     internal_error (__FILE__, __LINE__,
@@ -3845,15 +3865,9 @@ linux_nat_async_mask (int mask)
 	{
 	  linux_nat_async (NULL, 0);
 	  linux_nat_async_mask_value = mask;
-	  /* We're in sync mode.  Make sure SIGCHLD isn't handled by
-	     async_sigchld_handler when we come out of sigsuspend in
-	     linux_nat_wait.  */
-	  sigaction (SIGCHLD, &sync_sigchld_action, NULL);
 	}
       else
 	{
-	  /* Restore the async handler.  */
-	  sigaction (SIGCHLD, &async_sigchld_action, NULL);
 	  linux_nat_async_mask_value = mask;
 	  linux_nat_async (inferior_event_handler, 0);
 	}
@@ -3911,7 +3925,8 @@ get_pending_events (void)
 {
   int status, options, pid;
 
-  if (!linux_nat_async_enabled || !linux_nat_async_events_enabled)
+  if (!linux_nat_async_enabled
+      || linux_nat_async_events_state != sigchld_async)
     internal_error (__FILE__, __LINE__,
 		    "get_pending_events called with async masked");
 
@@ -3965,44 +3980,75 @@ async_sigchld_handler (int signo)
   get_pending_events ();
 }
 
-/* Enable or disable async SIGCHLD handling.  */
+/* Set SIGCHLD handling state to STATE.  Returns previous state.  */
 
-static int
-linux_nat_async_events (int enable)
+static enum sigchld_state
+linux_nat_async_events (enum sigchld_state state)
 {
-  int current_state = linux_nat_async_events_enabled;
+  enum sigchld_state current_state = linux_nat_async_events_state;
 
   if (debug_linux_nat_async)
     fprintf_unfiltered (gdb_stdlog,
-			"LNAE: enable(%d): linux_nat_async_events_enabled(%d), "
+			"LNAE: state(%d): linux_nat_async_events_state(%d), "
 			"linux_nat_num_queued_events(%d)\n",
-			enable, linux_nat_async_events_enabled,
+			state, linux_nat_async_events_state,
 			linux_nat_num_queued_events);
 
-  if (current_state != enable)
+  if (current_state != state)
     {
       sigset_t mask;
       sigemptyset (&mask);
       sigaddset (&mask, SIGCHLD);
-      if (enable)
-	{
-	  /* Unblock target events.  */
-	  linux_nat_async_events_enabled = 1;
 
-	  local_event_queue_to_pipe ();
-	  /* While in masked async, we may have not collected all the
-	     pending events.  Get them out now.  */
-	  get_pending_events ();
-	  sigprocmask (SIG_UNBLOCK, &mask, NULL);
-	}
-      else
+      /* Always block before changing state.  */
+      sigprocmask (SIG_BLOCK, &mask, NULL);
+
+      /* Set new state.  */
+      linux_nat_async_events_state = state;
+
+      switch (state)
 	{
-	  /* Block target events.  */
-	  sigprocmask (SIG_BLOCK, &mask, NULL);
-	  linux_nat_async_events_enabled = 0;
-	  /* Get events out of queue, and make them available to
-	     queued_waitpid / my_waitpid.  */
-	  pipe_to_local_event_queue ();
+	case sigchld_sync:
+	  {
+	    /* Block target events.  */
+	    sigprocmask (SIG_BLOCK, &mask, NULL);
+	    sigaction (SIGCHLD, &sync_sigchld_action, NULL);
+	    /* Get events out of queue, and make them available to
+	       queued_waitpid / my_waitpid.  */
+	    pipe_to_local_event_queue ();
+	  }
+	  break;
+	case sigchld_async:
+	  {
+	    /* Unblock target events for async mode.  */
+
+	    sigprocmask (SIG_BLOCK, &mask, NULL);
+
+	    /* Put events we already waited on, in the pipe first, so
+	       events are FIFO.  */
+	    local_event_queue_to_pipe ();
+	    /* While in masked async, we may have not collected all
+	       the pending events.  Get them out now.  */
+	    get_pending_events ();
+
+	    /* Let'em come.   */
+	    sigaction (SIGCHLD, &async_sigchld_action, NULL);
+	    sigprocmask (SIG_UNBLOCK, &mask, NULL);
+	  }
+	  break;
+	case sigchld_default:
+	  {
+	    /* SIGCHLD default mode.  */
+	    sigaction (SIGCHLD, &sigchld_default_action, NULL);
+
+	    /* Get events out of queue, and make them available to
+	       queued_waitpid / my_waitpid.  */
+	    pipe_to_local_event_queue ();
+
+	    /* Unblock SIGCHLD.  */
+	    sigprocmask (SIG_UNBLOCK, &mask, NULL);
+	  }
+	  break;
 	}
     }
 
@@ -4094,14 +4140,14 @@ linux_nat_async (void (*callback) (enum 
       add_file_handler (linux_nat_event_pipe[0],
 			linux_nat_async_file_handler, NULL);
 
-      linux_nat_async_events (1);
+      linux_nat_async_events (sigchld_async);
     }
   else
     {
       async_client_callback = callback;
       async_client_context = context;
 
-      linux_nat_async_events (0);
+      linux_nat_async_events (sigchld_sync);
       delete_file_handler (linux_nat_event_pipe[0]);
     }
   return;
@@ -4117,21 +4163,15 @@ linux_nat_set_async_mode (int on)
       if (on)
 	{
 	  gdb_assert (waitpid_queue == NULL);
-	  sigaction (SIGCHLD, &async_sigchld_action, NULL);
-
 	  if (pipe (linux_nat_event_pipe) == -1)
 	    internal_error (__FILE__, __LINE__,
 			    "creating event pipe failed.");
-
 	  fcntl (linux_nat_event_pipe[0], F_SETFL, O_NONBLOCK);
 	  fcntl (linux_nat_event_pipe[1], F_SETFL, O_NONBLOCK);
 	}
       else
 	{
-	  sigaction (SIGCHLD, &sync_sigchld_action, NULL);
-
 	  drain_queued_events (-1);
-
 	  linux_nat_num_queued_events = 0;
 	  close (linux_nat_event_pipe[0]);
 	  close (linux_nat_event_pipe[1]);
@@ -4248,6 +4288,10 @@ Tells gdb whether to control the GNU/Lin
 			   &maintenance_set_cmdlist,
 			   &maintenance_show_cmdlist);
 
+  /* Get the default SIGCHLD action.  Used while forking an inferior
+     (see linux_nat_create_inferior/linux_nat_async_events).  */
+  sigaction (SIGCHLD, NULL, &sigchld_default_action);
+
   /* Block SIGCHLD by default.  Doing this early prevents it getting
      unblocked if an exception is thrown due to an error while the
      inferior is starting (sigsetjmp/siglongjmp).  */
Index: src/gdb/testsuite/gdb.base/sigchld.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.base/sigchld.c	2008-06-12 00:05:36.000000000 +0100
@@ -0,0 +1,37 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2008 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+/* Check that GDB isn't messing the SIGCHLD mask while creating an
+   inferior.  */
+
+#include <signal.h>
+#include <stdlib.h>
+
+int
+main ()
+{
+  sigset_t mask;
+
+  sigemptyset (&mask);
+  sigprocmask (SIG_BLOCK, NULL, &mask);
+
+  if (!sigismember (&mask, SIGCHLD))
+    return 0; /* good, not blocked */
+  else
+    return 1; /* bad, blocked */
+}
Index: src/gdb/testsuite/gdb.base/sigchld.exp
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.base/sigchld.exp	2008-06-12 00:10:54.000000000 +0100
@@ -0,0 +1,45 @@
+# Copyright (C) 2008 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Check that GDB isn't messing the SIGCHLD mask while creating an
+# inferior.
+
+if [target_info exists gdb,nosignals] {
+    verbose "Skipping sigchld.exp because of nosignals."
+    continue
+}
+
+set testfile "sigchld"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+if {[gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+    return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+runto_main
+
+gdb_test "b [gdb_get_line_number "good, not blocked"]" \
+         ".*Breakpoint .*sigchld.*" "set breakpoint at success exit"
+
+gdb_test "b [gdb_get_line_number "bad, blocked"]" \
+         ".*Breakpoint .*sigchld.*" "set breakpoint at failure exit"
+
+gdb_test "continue" ".*good, not blocked.*" "SIGCHLD blocked in inferior"

Re: SIGCHLD ignored

[Vladimir Prus]

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

Re: SIGCHLD ignored

[Pedro Alves]

A Friday 13 June 2008 07:07:52, Vladimir Prus wrote:
> On Friday 13 June 2008 03:23:45 Pedro Alves wrote:

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

In this case, yes, it's just a protective measure against hitting the
wrong signal handler.

-- 
Pedro Alves

Re: SIGCHLD ignored

[Daniel Jacobowitz]

On Fri, Jun 13, 2008 at 12:23:45AM +0100, Pedro Alves wrote:
> @@ -381,7 +394,7 @@ my_waitpid (int pid, int *status, int fl
>    int ret;
>  
>    /* There should be no concurrent calls to waitpid.  */
> -  gdb_assert (!linux_nat_async_events_enabled);
> +  gdb_assert (linux_nat_async_events_state != sigchld_async);
>  
>    ret = queued_waitpid (pid, status, flags);
>    if (ret != -1)

Should this be == sigchld_sync?  IIUC we have unspecified SIGCHLD
behavior during sigchld_default.

Otherwise OK.

-- 
Daniel Jacobowitz
CodeSourcery

Re: SIGCHLD ignored

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 642 bytes --]

A Thursday 26 June 2008 15:09:36, Daniel Jacobowitz wrote:
> On Fri, Jun 13, 2008 at 12:23:45AM +0100, Pedro Alves wrote:
> > @@ -381,7 +394,7 @@ my_waitpid (int pid, int *status, int fl
> >    int ret;
> >
> >    /* There should be no concurrent calls to waitpid.  */
> > -  gdb_assert (!linux_nat_async_events_enabled);
> > +  gdb_assert (linux_nat_async_events_state != sigchld_async);
> >
> >    ret = queued_waitpid (pid, status, flags);
> >    if (ret != -1)
>
> Should this be == sigchld_sync?  IIUC we have unspecified SIGCHLD
> behavior during sigchld_default.
>
> Otherwise OK.

Commited with that change.  Thanks.

-- 
Pedro Alves

[-- Attachment #2: sigchld.diff --]
[-- Type: text/x-diff, Size: 14602 bytes --]

gdb/
2008-06-28  Pedro Alves  <pedro@codesourcery.com>

	* linux-nat.c (enum sigchld_state): New.
	(linux_nat_async_events_state): Renamed from
	linux_nat_async_events_enabled.
	(linux_nat_event_pipe_push, my_waitpid): Adjust.
	(sigchld_default_action): New.
	(lin_lwp_attach_lwp): Adjust.  Call linux_nat_async_events
	unconditionally.
	(linux_nat_create_inferior): Set events state to sigchld_default
	state.
	(linux_nat_resume): Adjust.
	(linux_nat_wait): Call linux_nat_async_events unconditionally.
	(sigchld_handler): Adjust.
	(linux_nat_async_mask): Don't set SIGCHLD actions here.
	(get_pending_events): Adjust.
	(linux_nat_async_events): Rewrite to handle enum sigchld_state
	instead of a boolean.
	(linux_nat_async): Adjust.
	(_initialize_linux_nat): Capture default SIGCHLD action into
	sigchld_default_action.

gdb/testsuite/
2008-06-28  Pedro Alves  <pedro@codesourcery.com>

	* gdb.base/sigchld.c, gdb.base/sigchld.exp: New test.
	
---
 gdb/linux-nat.c                    |  158 +++++++++++++++++++++++--------------
 gdb/testsuite/gdb.base/sigchld.c   |   37 ++++++++
 gdb/testsuite/gdb.base/sigchld.exp |   45 ++++++++++
 3 files changed, 183 insertions(+), 57 deletions(-)

Index: src/gdb/linux-nat.c
===================================================================
--- src.orig/gdb/linux-nat.c	2008-06-28 12:09:08.000000000 +0100
+++ src/gdb/linux-nat.c	2008-06-28 12:10:05.000000000 +0100
@@ -256,11 +256,24 @@ static int linux_nat_event_pipe[2] = { -
 /* Number of queued events in the pipe.  */
 static volatile int linux_nat_num_queued_events;
 
-/* If async mode is on, true if we're listening for events; false if
-   target events are blocked.  */
-static int linux_nat_async_events_enabled;
+/* The possible SIGCHLD handling states.  */
 
-static int linux_nat_async_events (int enable);
+enum sigchld_state
+{
+  /* SIGCHLD disabled, with action set to sigchld_handler, for the
+     sigsuspend in linux_nat_wait.  */
+  sigchld_sync,
+  /* SIGCHLD enabled, with action set to async_sigchld_handler.  */
+  sigchld_async,
+  /* Set SIGCHLD to default action.  Used while creating an
+     inferior.  */
+  sigchld_default
+};
+
+/* The current SIGCHLD handling state.  */
+static enum sigchld_state linux_nat_async_events_state;
+
+static enum sigchld_state linux_nat_async_events (enum sigchld_state enable);
 static void pipe_to_local_event_queue (void);
 static void local_event_queue_to_pipe (void);
 static void linux_nat_event_pipe_push (int pid, int status, int options);
@@ -294,8 +307,8 @@ queued_waitpid (int pid, int *status, in
   if (debug_linux_nat_async)
     fprintf_unfiltered (gdb_stdlog,
 			"\
-QWPID: linux_nat_async_events_enabled(%d), linux_nat_num_queued_events(%d)\n",
-			linux_nat_async_events_enabled,
+QWPID: linux_nat_async_events_state(%d), linux_nat_num_queued_events(%d)\n",
+			linux_nat_async_events_state,
 			linux_nat_num_queued_events);
 
   if (flags & __WALL)
@@ -441,7 +454,7 @@ my_waitpid (int pid, int *status, int fl
   int ret;
 
   /* There should be no concurrent calls to waitpid.  */
-  gdb_assert (!linux_nat_async_events_enabled);
+  gdb_assert (linux_nat_async_events_state == sigchld_sync);
 
   ret = queued_waitpid (pid, status, flags);
   if (ret != -1)
@@ -862,6 +875,9 @@ struct sigaction sync_sigchld_action;
 
 /* SIGCHLD action for asynchronous mode.  */
 static struct sigaction async_sigchld_action;
+
+/* SIGCHLD default action, to pass to new inferiors.  */
+static struct sigaction sigchld_default_action;
 \f
 
 /* Prototypes for local functions.  */
@@ -1185,12 +1201,11 @@ int
 lin_lwp_attach_lwp (ptid_t ptid)
 {
   struct lwp_info *lp;
-  int async_events_were_enabled = 0;
+  enum sigchld_state async_events_original_state;
 
   gdb_assert (is_lwp (ptid));
 
-  if (target_can_async_p ())
-    async_events_were_enabled = linux_nat_async_events (0);
+  async_events_original_state = linux_nat_async_events (sigchld_sync);
 
   lp = find_lwp_pid (ptid);
 
@@ -1255,9 +1270,7 @@ lin_lwp_attach_lwp (ptid_t ptid)
       lp->stopped = 1;
     }
 
-  if (async_events_were_enabled)
-    linux_nat_async_events (1);
-
+  linux_nat_async_events (async_events_original_state);
   return 0;
 }
 
@@ -1271,6 +1284,8 @@ linux_nat_create_inferior (char *exec_fi
      we have to mask the async mode.  */
 
   if (target_can_async_p ())
+    /* Mask async mode.  Creating a child requires a loop calling
+       wait_for_inferior currently.  */
     saved_async = linux_nat_async_mask (0);
   else
     {
@@ -1281,6 +1296,12 @@ linux_nat_create_inferior (char *exec_fi
       sigdelset (&suspend_mask, SIGCHLD);
     }
 
+  /* Set SIGCHLD to the default action, until after execing the child,
+     since the inferior inherits the superior's signal mask.  It will
+     be blocked again in linux_nat_wait, which is only reached after
+     the inferior execing.  */
+  linux_nat_async_events (sigchld_default);
+
   linux_ops->to_create_inferior (exec_file, allargs, env, from_tty);
 
   if (saved_async)
@@ -1512,7 +1533,7 @@ linux_nat_resume (ptid_t ptid, int step,
 
   if (target_can_async_p ())
     /* Block events while we're here.  */
-    linux_nat_async_events (0);
+    linux_nat_async_events (sigchld_sync);
 
   /* A specific PTID means `step only this process id'.  */
   resume_all = (PIDGET (ptid) == -1);
@@ -2574,9 +2595,8 @@ linux_nat_wait (ptid_t ptid, struct targ
 
   sigemptyset (&flush_mask);
 
-  if (target_can_async_p ())
-    /* Block events while we're here.  */
-    target_async (NULL, 0);
+  /* Block events while we're here.  */
+  linux_nat_async_events (sigchld_sync);
 
 retry:
 
@@ -3035,7 +3055,7 @@ static void
 sigchld_handler (int signo)
 {
   if (linux_nat_async_enabled
-      && linux_nat_async_events_enabled
+      && linux_nat_async_events_state != sigchld_sync
       && signo == SIGCHLD)
     /* It is *always* a bug to hit this.  */
     internal_error (__FILE__, __LINE__,
@@ -3894,15 +3914,9 @@ linux_nat_async_mask (int mask)
 	{
 	  linux_nat_async (NULL, 0);
 	  linux_nat_async_mask_value = mask;
-	  /* We're in sync mode.  Make sure SIGCHLD isn't handled by
-	     async_sigchld_handler when we come out of sigsuspend in
-	     linux_nat_wait.  */
-	  sigaction (SIGCHLD, &sync_sigchld_action, NULL);
 	}
       else
 	{
-	  /* Restore the async handler.  */
-	  sigaction (SIGCHLD, &async_sigchld_action, NULL);
 	  linux_nat_async_mask_value = mask;
 	  linux_nat_async (inferior_event_handler, 0);
 	}
@@ -3960,7 +3974,8 @@ get_pending_events (void)
 {
   int status, options, pid;
 
-  if (!linux_nat_async_enabled || !linux_nat_async_events_enabled)
+  if (!linux_nat_async_enabled
+      || linux_nat_async_events_state != sigchld_async)
     internal_error (__FILE__, __LINE__,
 		    "get_pending_events called with async masked");
 
@@ -4014,44 +4029,75 @@ async_sigchld_handler (int signo)
   get_pending_events ();
 }
 
-/* Enable or disable async SIGCHLD handling.  */
+/* Set SIGCHLD handling state to STATE.  Returns previous state.  */
 
-static int
-linux_nat_async_events (int enable)
+static enum sigchld_state
+linux_nat_async_events (enum sigchld_state state)
 {
-  int current_state = linux_nat_async_events_enabled;
+  enum sigchld_state current_state = linux_nat_async_events_state;
 
   if (debug_linux_nat_async)
     fprintf_unfiltered (gdb_stdlog,
-			"LNAE: enable(%d): linux_nat_async_events_enabled(%d), "
+			"LNAE: state(%d): linux_nat_async_events_state(%d), "
 			"linux_nat_num_queued_events(%d)\n",
-			enable, linux_nat_async_events_enabled,
+			state, linux_nat_async_events_state,
 			linux_nat_num_queued_events);
 
-  if (current_state != enable)
+  if (current_state != state)
     {
       sigset_t mask;
       sigemptyset (&mask);
       sigaddset (&mask, SIGCHLD);
-      if (enable)
-	{
-	  /* Unblock target events.  */
-	  linux_nat_async_events_enabled = 1;
 
-	  local_event_queue_to_pipe ();
-	  /* While in masked async, we may have not collected all the
-	     pending events.  Get them out now.  */
-	  get_pending_events ();
-	  sigprocmask (SIG_UNBLOCK, &mask, NULL);
-	}
-      else
+      /* Always block before changing state.  */
+      sigprocmask (SIG_BLOCK, &mask, NULL);
+
+      /* Set new state.  */
+      linux_nat_async_events_state = state;
+
+      switch (state)
 	{
-	  /* Block target events.  */
-	  sigprocmask (SIG_BLOCK, &mask, NULL);
-	  linux_nat_async_events_enabled = 0;
-	  /* Get events out of queue, and make them available to
-	     queued_waitpid / my_waitpid.  */
-	  pipe_to_local_event_queue ();
+	case sigchld_sync:
+	  {
+	    /* Block target events.  */
+	    sigprocmask (SIG_BLOCK, &mask, NULL);
+	    sigaction (SIGCHLD, &sync_sigchld_action, NULL);
+	    /* Get events out of queue, and make them available to
+	       queued_waitpid / my_waitpid.  */
+	    pipe_to_local_event_queue ();
+	  }
+	  break;
+	case sigchld_async:
+	  {
+	    /* Unblock target events for async mode.  */
+
+	    sigprocmask (SIG_BLOCK, &mask, NULL);
+
+	    /* Put events we already waited on, in the pipe first, so
+	       events are FIFO.  */
+	    local_event_queue_to_pipe ();
+	    /* While in masked async, we may have not collected all
+	       the pending events.  Get them out now.  */
+	    get_pending_events ();
+
+	    /* Let'em come.   */
+	    sigaction (SIGCHLD, &async_sigchld_action, NULL);
+	    sigprocmask (SIG_UNBLOCK, &mask, NULL);
+	  }
+	  break;
+	case sigchld_default:
+	  {
+	    /* SIGCHLD default mode.  */
+	    sigaction (SIGCHLD, &sigchld_default_action, NULL);
+
+	    /* Get events out of queue, and make them available to
+	       queued_waitpid / my_waitpid.  */
+	    pipe_to_local_event_queue ();
+
+	    /* Unblock SIGCHLD.  */
+	    sigprocmask (SIG_UNBLOCK, &mask, NULL);
+	  }
+	  break;
 	}
     }
 
@@ -4143,14 +4189,14 @@ linux_nat_async (void (*callback) (enum 
       add_file_handler (linux_nat_event_pipe[0],
 			linux_nat_async_file_handler, NULL);
 
-      linux_nat_async_events (1);
+      linux_nat_async_events (sigchld_async);
     }
   else
     {
       async_client_callback = callback;
       async_client_context = context;
 
-      linux_nat_async_events (0);
+      linux_nat_async_events (sigchld_sync);
       delete_file_handler (linux_nat_event_pipe[0]);
     }
   return;
@@ -4166,21 +4212,15 @@ linux_nat_set_async_mode (int on)
       if (on)
 	{
 	  gdb_assert (waitpid_queue == NULL);
-	  sigaction (SIGCHLD, &async_sigchld_action, NULL);
-
 	  if (pipe (linux_nat_event_pipe) == -1)
 	    internal_error (__FILE__, __LINE__,
 			    "creating event pipe failed.");
-
 	  fcntl (linux_nat_event_pipe[0], F_SETFL, O_NONBLOCK);
 	  fcntl (linux_nat_event_pipe[1], F_SETFL, O_NONBLOCK);
 	}
       else
 	{
-	  sigaction (SIGCHLD, &sync_sigchld_action, NULL);
-
 	  drain_queued_events (-1);
-
 	  linux_nat_num_queued_events = 0;
 	  close (linux_nat_event_pipe[0]);
 	  close (linux_nat_event_pipe[1]);
@@ -4297,6 +4337,10 @@ Tells gdb whether to control the GNU/Lin
 			   &maintenance_set_cmdlist,
 			   &maintenance_show_cmdlist);
 
+  /* Get the default SIGCHLD action.  Used while forking an inferior
+     (see linux_nat_create_inferior/linux_nat_async_events).  */
+  sigaction (SIGCHLD, NULL, &sigchld_default_action);
+
   /* Block SIGCHLD by default.  Doing this early prevents it getting
      unblocked if an exception is thrown due to an error while the
      inferior is starting (sigsetjmp/siglongjmp).  */
Index: src/gdb/testsuite/gdb.base/sigchld.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.base/sigchld.c	2008-06-28 12:10:05.000000000 +0100
@@ -0,0 +1,37 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2008 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+/* Check that GDB isn't messing the SIGCHLD mask while creating an
+   inferior.  */
+
+#include <signal.h>
+#include <stdlib.h>
+
+int
+main ()
+{
+  sigset_t mask;
+
+  sigemptyset (&mask);
+  sigprocmask (SIG_BLOCK, NULL, &mask);
+
+  if (!sigismember (&mask, SIGCHLD))
+    return 0; /* good, not blocked */
+  else
+    return 1; /* bad, blocked */
+}
Index: src/gdb/testsuite/gdb.base/sigchld.exp
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.base/sigchld.exp	2008-06-28 12:10:05.000000000 +0100
@@ -0,0 +1,45 @@
+# Copyright (C) 2008 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Check that GDB isn't messing the SIGCHLD mask while creating an
+# inferior.
+
+if [target_info exists gdb,nosignals] {
+    verbose "Skipping sigchld.exp because of nosignals."
+    continue
+}
+
+set testfile "sigchld"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+if {[gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+    return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+runto_main
+
+gdb_test "b [gdb_get_line_number "good, not blocked"]" \
+         ".*Breakpoint .*sigchld.*" "set breakpoint at success exit"
+
+gdb_test "b [gdb_get_line_number "bad, blocked"]" \
+         ".*Breakpoint .*sigchld.*" "set breakpoint at failure exit"
+
+gdb_test "continue" ".*good, not blocked.*" "SIGCHLD blocked in inferior"