SIGCHLD ignored

SIGCHLD ignored

[Vladimir Prus]

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


A fellow KDevelop hacker has reported that when running kdevelop itself
under CVS HEAD of gdb, kdevelop hangs. What happens if that kdevelop spawns
subprocess, and then does not notice it has exited, because it never
receives SIGCHLD. I attach a much reduced project that requires only Qt4,
and probably an even more reduced project is possible.

If I apply the attached patch to GDB, things work fine -- but I suspect
this 'fix' will break something else. 

Pedro, I think this SIGCHLD magic is your doing -- do you have any ideas
how to fix it? 

Thanks,
Volodya

[-- Attachment #2: hang.tar --]
[-- Type: application/x-tar, Size: 20480 bytes --]

[-- Attachment #3: workaround.diff --]
[-- Type: text/x-diff, Size: 689 bytes --]

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 47dac59..8e62718 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -4248,12 +4248,14 @@ Tells gdb whether to control the GNU/Linux inferior in asynchronous mode."),
 			   &maintenance_set_cmdlist,
 			   &maintenance_show_cmdlist);
 
+#if 0
   /* 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).  */
   sigemptyset (&mask);
   sigaddset (&mask, SIGCHLD);
   sigprocmask (SIG_BLOCK, &mask, NULL);
+#endif
 
   /* Save this mask as the default.  */
   sigprocmask (SIG_SETMASK, NULL, &normal_mask);

Re: SIGCHLD ignored

[Daniel Jacobowitz]

On Wed, Jun 11, 2008 at 09:21:05PM +0400, Vladimir Prus wrote:
> 
> A fellow KDevelop hacker has reported that when running kdevelop itself
> under CVS HEAD of gdb, kdevelop hangs. What happens if that kdevelop spawns
> subprocess, and then does not notice it has exited, because it never
> receives SIGCHLD. I attach a much reduced project that requires only Qt4,
> and probably an even more reduced project is possible.
> 
> If I apply the attached patch to GDB, things work fine -- but I suspect
> this 'fix' will break something else. 
> 
> Pedro, I think this SIGCHLD magic is your doing -- do you have any ideas
> how to fix it? 

linux_nat_create_inferior is already unblocking signals before
creating.  Does normal_mask contain SIGCHLD?

The approach taken (search for "signal" in fork-child.c, first
comment) may not be compatible with async...

-- 
Daniel Jacobowitz
CodeSourcery

Re: SIGCHLD ignored

[Vladimir Prus]

Daniel Jacobowitz wrote:

> On Wed, Jun 11, 2008 at 09:21:05PM +0400, Vladimir Prus wrote:
>> 
>> A fellow KDevelop hacker has reported that when running kdevelop itself
>> under CVS HEAD of gdb, kdevelop hangs. What happens if that kdevelop spawns
>> subprocess, and then does not notice it has exited, because it never
>> receives SIGCHLD. I attach a much reduced project that requires only Qt4,
>> and probably an even more reduced project is possible.
>> 
>> If I apply the attached patch to GDB, things work fine -- but I suspect
>> this 'fix' will break something else.
>> 
>> Pedro, I think this SIGCHLD magic is your doing -- do you have any ideas
>> how to fix it?
> 
> linux_nat_create_inferior is already unblocking signals before
> creating.  Does normal_mask contain SIGCHLD? 

It does not, at least when the normal_mask is initially obtained.
linux_nat_create_inferior does restore the mask. Still, my test
case does not work.

> The approach taken (search for "signal" in fork-child.c, first
> comment) may not be compatible with async...

I actually would appreciate some comments about what we do with SIGCHLD
in linux-nat.c -- the code is fairly complex to understand this by
reading.

- Volodya

Re: SIGCHLD ignored

[Michael Snyder]

On Wed, 2008-06-11 at 22:08 +0400, Vladimir Prus wrote:

> I actually would appreciate some comments about what we do with SIGCHLD
> in linux-nat.c -- the code is fairly complex to understand this by
> reading.

Ha ha, yes.  Legacy of old linux threads model, I think.

If we change anything in that area, we should probably test
to make sure gdb still works on old linux thread model.

Unles we want to decide that we no longer support that --
but I believe there are still platforms that have not
converted over to NPTL threads (or have done so only
recently).

Re: SIGCHLD ignored

[Pedro Alves]

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

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

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

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;
 \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).  */

Re: SIGCHLD ignored

[Pedro Alves]

[Discussion moved to gdb-patches@]
http://sources.redhat.com/ml/gdb-patches/2008-06/msg00228.html

-- 
Pedro Alves