From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25641 invoked by alias); 11 Jun 2008 23:18:40 -0000 Received: (qmail 25630 invoked by uid 22791); 11 Jun 2008 23:18:38 -0000 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.31) with ESMTP; Wed, 11 Jun 2008 23:18:09 +0000 Received: (qmail 19517 invoked from network); 11 Jun 2008 23:18:06 -0000 Received: from unknown (HELO orlando.local) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 11 Jun 2008 23:18:06 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: SIGCHLD ignored Date: Thu, 12 Jun 2008 05:51:00 -0000 User-Agent: KMail/1.9.9 Cc: Vladimir Prus , Hamish Rodda , Daniel Jacobowitz References: <200806112121.06783.ghost@cs.msu.su> <200806112345.22321.pedro@codesourcery.com> In-Reply-To: <200806112345.22321.pedro@codesourcery.com> MIME-Version: 1.0 Content-Type: Multipart/Mixed; boundary="Boundary-00=_t0FUIgJ8MlAFj17" Message-Id: <200806120018.05719.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2008-06/txt/msg00230.txt.bz2 --Boundary-00=_t0FUIgJ8MlAFj17 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Content-Disposition: inline Content-length: 1938 [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 --Boundary-00=_t0FUIgJ8MlAFj17 Content-Type: text/x-diff; charset="utf-8"; name="sigchld.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="sigchld.diff" Content-length: 14477 gdb/ 2008-06-12 Pedro Alves * 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 * 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; /* 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 . +*/ + +/* Check that GDB isn't messing the SIGCHLD mask while creating an + inferior. */ + +#include +#include + +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 . + +# 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" --Boundary-00=_t0FUIgJ8MlAFj17--