Re: SIGCHLD ignored

[Pedro Alves <pedro at codesourcery dot com>]

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.

-- 
Pedro Alves

2008-06-11  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/linux-nat.c |  154 +++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 97 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-11 23:43:49.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;
 
 
 /* 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).  */