This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] Workaround for 10970, 12702, avoid calling waitpid


On Wed, 18 May 2011 20:16:59 +0200, Pedro Alves wrote:
> On Wednesday 18 May 2011 18:13:11, Jan Kratochvil wrote:
> > +      if (is_lwp (lp->ptid) && GET_PID (lp->ptid) == GET_LWP (lp->ptid)
> > +         && linux_lwp_is_zombie (GET_LWP (lp->ptid)))
> 
> When can is_lwp(lp->ptid) be false?

Removed.  IIRC it could be false before (2007) which I agree is irrelevant
now.


> > +      if (pid != 0 && (pid == -1 && errno == ECHILD))
> 
> pid != 0 check looks redundant as is.  Is it there to 
> try to make this more readable?

No, more a coding thinko, removed.


> FWIW, this looks good to me, but I wonder whether
> checking for zombieness after the waitpids wouldn't
> avoid a few /proc/ reads in the common case.

OK, changed the order which does not matter there; still the syscalls
efficiency of linux-nat is far from perfect.


> The inferior exit code reported to the core/user will be the
> exit code of the last LWP that exits, instead of the
> leader's exit code.  We've stepped out of C realm when
> the leader exited, so I'm not sure that matters (or how
> to make it be != 0, even).

I do not understand this comment much.  That != 0 is here for PID from waitpid
which is unrelated to the exit code (present in STATUS).


No regressions on {x86_64,x86_64-m32,i686}-fedora15-linux-gnu.

I will check it in if no futher comments appear.


Thanks for the review,
Jan


gdb/
2011-05-18  Jan Kratochvil  <jan.kratochvil@redhat.com>
	    Doug Evans  <dje@google.com>

	Fix PR 10970, PR 12702.
	* linux-nat.c (linux_lwp_is_zombie): New function.
	(wait_lwp): Initialize status.  New variable prev_mask.  Block signals.
	Check for linux_lwp_is_zombie.  Use WNOHANG and sigsuspend.

testsuite/
2011-05-18  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.threads/leader-exit.c: New file.
	* gdb.threads/leader-exit.exp: New file.

--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -2356,6 +2356,33 @@ linux_handle_extended_wait (struct lwp_info *lp, int status,
 		  _("unknown ptrace event %d"), event);
 }
 
+/* Return non-zero if LWP is a zombie.  */
+
+static int
+linux_lwp_is_zombie (long lwp)
+{
+  char buffer[MAXPATHLEN];
+  FILE *procfile;
+  int retval = 0;
+
+  sprintf (buffer, "/proc/%ld/status", lwp);
+  procfile = fopen (buffer, "r");
+  if (procfile == NULL)
+    {
+      warning (_("unable to open /proc file '%s'"), buffer);
+      return 0;
+    }
+  while (fgets (buffer, sizeof (buffer), procfile) != NULL)
+    if (strcmp (buffer, "State:\tZ (zombie)\n") == 0)
+      {
+	retval = 1;
+	break;
+      }
+  fclose (procfile);
+
+  return retval;
+}
+
 /* Wait for LP to stop.  Returns the wait status, or 0 if the LWP has
    exited.  */
 
@@ -2363,28 +2390,76 @@ static int
 wait_lwp (struct lwp_info *lp)
 {
   pid_t pid;
-  int status;
+  int status = 0;
   int thread_dead = 0;
+  sigset_t prev_mask;
 
   gdb_assert (!lp->stopped);
   gdb_assert (lp->status == 0);
 
-  pid = my_waitpid (GET_LWP (lp->ptid), &status, 0);
-  if (pid == -1 && errno == ECHILD)
+  /* Make sure SIGCHLD is blocked for sigsuspend avoiding a race below.  */
+  block_child_signals (&prev_mask);
+
+  for (;;)
     {
-      pid = my_waitpid (GET_LWP (lp->ptid), &status, __WCLONE);
+      /* If my_waitpid returns 0 it means the __WCLONE vs. non-__WCLONE kind
+	 was right and we should just call sigsuspend.  */
+
+      pid = my_waitpid (GET_LWP (lp->ptid), &status, WNOHANG);
       if (pid == -1 && errno == ECHILD)
+	pid = my_waitpid (GET_LWP (lp->ptid), &status, __WCLONE | WNOHANG);
+      if (pid != 0)
+	break;
+
+      /* Bugs 10970, 12702.
+	 Thread group leader may have exited in which case we'll lock up in
+	 waitpid if there are other threads, even if they are all zombies too.
+	 Basically, we're not supposed to use waitpid this way.
+	 __WCLONE is not applicable for the leader so we can't use that.
+	 LINUX_NAT_THREAD_ALIVE cannot be used here as it requires a STOPPED
+	 process; it gets ESRCH both for the zombie and for running processes.
+
+	 As a workaround, check if we're waiting for the thread group leader and
+	 if it's a zombie, and avoid calling waitpid if it is.
+
+	 This is racy, what if the tgl becomes a zombie right after we check?
+	 Therefore always use WNOHANG with sigsuspend - it is equivalent to
+	 waiting waitpid but the linux_lwp_is_zombie is safe this way.  */
+
+      if (GET_PID (lp->ptid) == GET_LWP (lp->ptid)
+	  && linux_lwp_is_zombie (GET_LWP (lp->ptid)))
 	{
-	  /* The thread has previously exited.  We need to delete it
-	     now because, for some vendor 2.4 kernels with NPTL
-	     support backported, there won't be an exit event unless
-	     it is the main thread.  2.6 kernels will report an exit
-	     event for each thread that exits, as expected.  */
 	  thread_dead = 1;
 	  if (debug_linux_nat)
-	    fprintf_unfiltered (gdb_stdlog, "WL: %s vanished.\n",
+	    fprintf_unfiltered (gdb_stdlog,
+				"WL: Thread group leader %s vanished.\n",
 				target_pid_to_str (lp->ptid));
+	  break;
 	}
+
+      /* Wait for next SIGCHLD and try again.  This may let SIGCHLD handlers
+	 get invoked despite our caller had them intentionally blocked by
+	 block_child_signals.  This is sensitive only to the loop of
+	 linux_nat_wait_1 and there if we get called my_waitpid gets called
+	 again before it gets to sigsuspend so we can safely let the handlers
+	 get executed here.  */
+
+      sigsuspend (&suspend_mask);
+    }
+
+  restore_child_signals_mask (&prev_mask);
+
+  if (pid == -1 && errno == ECHILD)
+    {
+      /* The thread has previously exited.  We need to delete it
+	 now because, for some vendor 2.4 kernels with NPTL
+	 support backported, there won't be an exit event unless
+	 it is the main thread.  2.6 kernels will report an exit
+	 event for each thread that exits, as expected.  */
+      thread_dead = 1;
+      if (debug_linux_nat)
+	fprintf_unfiltered (gdb_stdlog, "WL: %s vanished.\n",
+			    target_pid_to_str (lp->ptid));
     }
 
   if (!thread_dead)
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/leader-exit.c
@@ -0,0 +1,49 @@
+/* Clean exit of the thread group leader should not break GDB.
+
+   Copyright 2007, 2011 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/>.  */
+
+#include <pthread.h>
+#include <assert.h>
+#include <unistd.h>
+
+static volatile pthread_t main_thread;
+
+static void *
+start (void *arg)
+{
+  int i;
+
+  i = pthread_join (main_thread, NULL);
+  assert (i == 0);
+
+  return arg;	/* break-here */
+}
+
+int
+main (void)
+{
+  pthread_t thread;
+  int i;
+
+  main_thread = pthread_self ();
+
+  i = pthread_create (&thread, NULL, start, NULL);
+  assert (i == 0);
+
+  pthread_exit (NULL);
+  /* NOTREACHED */
+  return 0;
+}
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/leader-exit.exp
@@ -0,0 +1,38 @@
+# Copyright (C) 2007, 2011 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/>.
+
+# Exit of the thread group leader should not break GDB.
+
+set testfile "leader-exit"
+set srcfile ${testfile}.c
+set executable ${testfile}
+set binfile ${objdir}/${subdir}/${executable}
+
+if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+    return -1
+}
+
+clean_restart ${executable}
+
+if ![runto_main] {
+    return -1
+}
+
+gdb_breakpoint [gdb_get_line_number "break-here"]
+gdb_continue_to_breakpoint "break-here" ".* break-here .*"
+
+gdb_test "info threads" \
+	 "\r\n\[ \t\]*Id\[ \t\]+Target\[ \t\]+Id\[ \t\]+Frame\[ \t\]*\r\n\\* 2 *Thread \[^\r\n\]* at \[^\r\n\]*" \
+	 "Single thread has been left"


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]