[rfc] Fix linux_test_for_tracefork for newer kernels

[rfc] Fix linux_test_for_tracefork for newer kernels

[Daniel Jacobowitz]

This patch makes the run-time test for fork tracing a little more robust.
In particular, it fixes two things:
  - If the test failed in an unexpected way, then two stopped processes
    could be left around.
  - Waitpid can be interrupted by EINTR.  Kernel 2.6.10-rc1 has some
    changes to wait and signal processing, which cause the SIGCHLD
    to be received before wait returns.  I don't think there's any
    actual bug here, since reissuing the wait works fine.

These changes let my testsuite runs finish without leaving hordes of stopped
processes around.  I won't check it in yet, since I haven't tested it on a
system where this runtime check should fail; in the meantime, I'm just
posting it for comments.  Hopefully I got it right this time!

-- 
Daniel Jacobowitz

2004-11-12  Daniel Jacobowitz  <dan@debian.org>

	* linux-nat.c (my_waitpid): New function.
	(linux_test_for_tracefork): Make more robust and verbose.

Index: gdb-6.3/gdb/linux-nat.c
===================================================================
--- gdb-6.3.orig/gdb/linux-nat.c	2004-10-08 16:29:47.000000000 -0400
+++ gdb-6.3/gdb/linux-nat.c	2004-11-12 17:05:30.000000000 -0500
@@ -150,6 +150,21 @@ linux_tracefork_child (void)
   exit (0);
 }
 
+/* Wrapper function for waitpid which handles EINTR.  */
+
+static int
+my_waitpid (int pid, int *status, int flags)
+{
+  int ret;
+  do
+    {
+      ret = waitpid (pid, status, flags);
+    }
+  while (ret == -1 && errno == EINTR);
+
+  return ret;
+}
+
 /* Determine if PTRACE_O_TRACEFORK can be used to follow fork events.  We
    create a child process, attach to it, use PTRACE_SETOPTIONS to enable
    fork tracing, and let it fork.  If the process exits, we assume that
@@ -169,7 +184,7 @@ linux_test_for_tracefork (void)
   if (child_pid == 0)
     linux_tracefork_child ();
 
-  ret = waitpid (child_pid, &status, 0);
+  ret = my_waitpid (child_pid, &status, 0);
   if (ret == -1)
     perror_with_name ("linux_test_for_tracefork: waitpid");
   else if (ret != child_pid)
@@ -182,8 +197,17 @@ linux_test_for_tracefork (void)
   ret = ptrace (PTRACE_SETOPTIONS, child_pid, 0, PTRACE_O_TRACEFORK);
   if (ret != 0)
     {
-      ptrace (PTRACE_KILL, child_pid, 0, 0);
-      waitpid (child_pid, &status, 0);
+      ret = ptrace (PTRACE_KILL, child_pid, 0, 0);
+      if (ret != 0)
+	{
+	  warning ("linux_test_for_tracefork: failed to kill child");
+	  return;
+	}
+
+      ret = my_waitpid (child_pid, &status, 0);
+      if (ret != 0)
+	warning ("linux_test_for_tracefork: failed to wait for killed child");
+
       return;
     }
 
@@ -192,8 +216,12 @@ linux_test_for_tracefork (void)
 		PTRACE_O_TRACEFORK | PTRACE_O_TRACEVFORKDONE);
   linux_supports_tracevforkdone_flag = (ret == 0);
 
-  ptrace (PTRACE_CONT, child_pid, 0, 0);
-  ret = waitpid (child_pid, &status, 0);
+  ret = ptrace (PTRACE_CONT, child_pid, 0, 0);
+  if (ret != 0)
+    warning ("linux_test_for_tracefork: failed to resume child");
+
+  ret = my_waitpid (child_pid, &status, 0);
+
   if (ret == child_pid && WIFSTOPPED (status)
       && status >> 16 == PTRACE_EVENT_FORK)
     {
@@ -204,16 +232,20 @@ linux_test_for_tracefork (void)
 	  int second_status;
 
 	  linux_supports_tracefork_flag = 1;
-	  waitpid (second_pid, &second_status, 0);
-	  ptrace (PTRACE_DETACH, second_pid, 0, 0);
+	  my_waitpid (second_pid, &second_status, 0);
+	  ret = ptrace (PTRACE_KILL, second_pid, 0, 0);
+	  if (ret != 0)
+	    warning ("linux_test_for_tracefork: failed to kill second child");
 	}
     }
+  else
+    warning ("linux_test_for_tracefork: unexpected result from waitpid "
+	     "(%d, status 0x%x)", ret, status);
 
-  if (WIFSTOPPED (status))
-    {
-      ptrace (PTRACE_DETACH, child_pid, 0, 0);
-      waitpid (child_pid, &status, 0);
-    }
+  ret = ptrace (PTRACE_KILL, child_pid, 0, 0);
+  if (ret != 0)
+    warning ("linux_test_for_tracefork: failed to kill child");
+  my_waitpid (child_pid, &status, 0);
 }
 
 /* Return non-zero iff we have tracefork functionality available.

Re: [rfc] Fix linux_test_for_tracefork for newer kernels

[Daniel Jacobowitz]

On Fri, Nov 12, 2004 at 05:59:41PM -0500, Daniel Jacobowitz wrote:
> This patch makes the run-time test for fork tracing a little more robust.
> In particular, it fixes two things:
>   - If the test failed in an unexpected way, then two stopped processes
>     could be left around.
>   - Waitpid can be interrupted by EINTR.  Kernel 2.6.10-rc1 has some
>     changes to wait and signal processing, which cause the SIGCHLD
>     to be received before wait returns.  I don't think there's any
>     actual bug here, since reissuing the wait works fine.
> 
> These changes let my testsuite runs finish without leaving hordes of stopped
> processes around.  I won't check it in yet, since I haven't tested it on a
> system where this runtime check should fail; in the meantime, I'm just
> posting it for comments.  Hopefully I got it right this time!

Now I've tested it in a couple of other systems.  Here's a revised
version, with a spurious warning fixed, and one additional improvement:
on most systems lacking the support, we don't need to fork for the
runtime test.  This fixes some problems in user-mode-linux, which does
the most disgusting things with ptrace I have ever encountered.  Wow!

Barring comments, I'll check this in in a few days.

-- 
Daniel Jacobowitz

2004-11-12  Daniel Jacobowitz  <dan@debian.org>

	* linux-nat.c (my_waitpid): New function.
	(linux_test_for_tracefork): Make more robust and verbose.  Take
	an ORIGINAL_PID argument and test for PTRACE_SETOPTIONS first.
	(linux_supports_tracefork, linux_supports_tracevforkdone): Take a PID
	argument.  Update calls to linux_test_for_tracefork.
	(linux_enable_event_reporting, child_follow_fork)
	(child_insert_fork_catchpoint, child_insert_vfork_catchpoint)
	(child_insert_exec_catchpoint): Update calls to
	linux_supports_tracefork and linux_supports_tracevforkdone.

Index: gdb-6.3/gdb/linux-nat.c
===================================================================
--- gdb-6.3.orig/gdb/linux-nat.c	2004-10-08 16:29:47.000000000 -0400
+++ gdb-6.3/gdb/linux-nat.c	2004-11-13 16:41:51.368720845 -0500
@@ -150,18 +150,47 @@ linux_tracefork_child (void)
   exit (0);
 }
 
-/* Determine if PTRACE_O_TRACEFORK can be used to follow fork events.  We
+/* Wrapper function for waitpid which handles EINTR.  */
+
+static int
+my_waitpid (int pid, int *status, int flags)
+{
+  int ret;
+  do
+    {
+      ret = waitpid (pid, status, flags);
+    }
+  while (ret == -1 && errno == EINTR);
+
+  return ret;
+}
+
+/* Determine if PTRACE_O_TRACEFORK can be used to follow fork events.
+
+   First, we try to enable fork tracing on ORIGINAL_PID.  If this fails,
+   we know that the feature is not available.  This may change the tracing
+   options for ORIGINAL_PID, but we'll be setting them shortly anyway.
+
+   However, if it succeeds, we don't know for sure that the feature is
+   available; old versions of PTRACE_SETOPTIONS ignored unknown options.  We
    create a child process, attach to it, use PTRACE_SETOPTIONS to enable
-   fork tracing, and let it fork.  If the process exits, we assume that
-   we can't use TRACEFORK; if we get the fork notification, and we can
-   extract the new child's PID, then we assume that we can.  */
+   fork tracing, and let it fork.  If the process exits, we assume that we
+   can't use TRACEFORK; if we get the fork notification, and we can extract
+   the new child's PID, then we assume that we can.  */
 
 static void
-linux_test_for_tracefork (void)
+linux_test_for_tracefork (int original_pid)
 {
   int child_pid, ret, status;
   long second_pid;
 
+  linux_supports_tracefork_flag = 0;
+  linux_supports_tracevforkdone_flag = 0;
+
+  ret = ptrace (PTRACE_SETOPTIONS, original_pid, 0, PTRACE_O_TRACEFORK);
+  if (ret != 0)
+    return;
+
   child_pid = fork ();
   if (child_pid == -1)
     perror_with_name ("linux_test_for_tracefork: fork");
@@ -169,7 +198,7 @@ linux_test_for_tracefork (void)
   if (child_pid == 0)
     linux_tracefork_child ();
 
-  ret = waitpid (child_pid, &status, 0);
+  ret = my_waitpid (child_pid, &status, 0);
   if (ret == -1)
     perror_with_name ("linux_test_for_tracefork: waitpid");
   else if (ret != child_pid)
@@ -177,13 +206,23 @@ linux_test_for_tracefork (void)
   if (! WIFSTOPPED (status))
     error ("linux_test_for_tracefork: waitpid: unexpected status %d.", status);
 
-  linux_supports_tracefork_flag = 0;
-
   ret = ptrace (PTRACE_SETOPTIONS, child_pid, 0, PTRACE_O_TRACEFORK);
   if (ret != 0)
     {
-      ptrace (PTRACE_KILL, child_pid, 0, 0);
-      waitpid (child_pid, &status, 0);
+      ret = ptrace (PTRACE_KILL, child_pid, 0, 0);
+      if (ret != 0)
+	{
+	  warning ("linux_test_for_tracefork: failed to kill child");
+	  return;
+	}
+
+      ret = my_waitpid (child_pid, &status, 0);
+      if (ret != child_pid)
+	warning ("linux_test_for_tracefork: failed to wait for killed child");
+      else if (!WIFSIGNALED (status))
+	warning ("linux_test_for_tracefork: unexpected wait status 0x%x from "
+		 "killed child", status);
+
       return;
     }
 
@@ -192,8 +231,12 @@ linux_test_for_tracefork (void)
 		PTRACE_O_TRACEFORK | PTRACE_O_TRACEVFORKDONE);
   linux_supports_tracevforkdone_flag = (ret == 0);
 
-  ptrace (PTRACE_CONT, child_pid, 0, 0);
-  ret = waitpid (child_pid, &status, 0);
+  ret = ptrace (PTRACE_CONT, child_pid, 0, 0);
+  if (ret != 0)
+    warning ("linux_test_for_tracefork: failed to resume child");
+
+  ret = my_waitpid (child_pid, &status, 0);
+
   if (ret == child_pid && WIFSTOPPED (status)
       && status >> 16 == PTRACE_EVENT_FORK)
     {
@@ -204,34 +247,38 @@ linux_test_for_tracefork (void)
 	  int second_status;
 
 	  linux_supports_tracefork_flag = 1;
-	  waitpid (second_pid, &second_status, 0);
-	  ptrace (PTRACE_DETACH, second_pid, 0, 0);
+	  my_waitpid (second_pid, &second_status, 0);
+	  ret = ptrace (PTRACE_KILL, second_pid, 0, 0);
+	  if (ret != 0)
+	    warning ("linux_test_for_tracefork: failed to kill second child");
 	}
     }
+  else
+    warning ("linux_test_for_tracefork: unexpected result from waitpid "
+	     "(%d, status 0x%x)", ret, status);
 
-  if (WIFSTOPPED (status))
-    {
-      ptrace (PTRACE_DETACH, child_pid, 0, 0);
-      waitpid (child_pid, &status, 0);
-    }
+  ret = ptrace (PTRACE_KILL, child_pid, 0, 0);
+  if (ret != 0)
+    warning ("linux_test_for_tracefork: failed to kill child");
+  my_waitpid (child_pid, &status, 0);
 }
 
 /* Return non-zero iff we have tracefork functionality available.
    This function also sets linux_supports_tracefork_flag.  */
 
 static int
-linux_supports_tracefork (void)
+linux_supports_tracefork (int pid)
 {
   if (linux_supports_tracefork_flag == -1)
-    linux_test_for_tracefork ();
+    linux_test_for_tracefork (pid);
   return linux_supports_tracefork_flag;
 }
 
 static int
-linux_supports_tracevforkdone (void)
+linux_supports_tracevforkdone (int pid)
 {
   if (linux_supports_tracefork_flag == -1)
-    linux_test_for_tracefork ();
+    linux_test_for_tracefork (pid);
   return linux_supports_tracevforkdone_flag;
 }
 
@@ -242,12 +289,12 @@ linux_enable_event_reporting (ptid_t pti
   int pid = ptid_get_pid (ptid);
   int options;
 
-  if (! linux_supports_tracefork ())
+  if (! linux_supports_tracefork (pid))
     return;
 
   options = PTRACE_O_TRACEFORK | PTRACE_O_TRACEVFORK | PTRACE_O_TRACEEXEC
     | PTRACE_O_TRACECLONE;
-  if (linux_supports_tracevforkdone ())
+  if (linux_supports_tracevforkdone (pid))
     options |= PTRACE_O_TRACEVFORKDONE;
 
   /* Do not enable PTRACE_O_TRACEEXIT until GDB is more prepared to support
@@ -308,7 +355,8 @@ child_follow_fork (int follow_child)
 
       if (has_vforked)
 	{
-	  if (linux_supports_tracevforkdone ())
+	  gdb_assert (linux_supports_tracefork_flag >= 0);
+	  if (linux_supports_tracevforkdone (0))
 	    {
 	      int status;
 
@@ -476,7 +524,7 @@ linux_handle_extended_wait (int pid, int
 int
 child_insert_fork_catchpoint (int pid)
 {
-  if (! linux_supports_tracefork ())
+  if (! linux_supports_tracefork (pid))
     error ("Your system does not support fork catchpoints.");
 
   return 0;
@@ -485,7 +533,7 @@ child_insert_fork_catchpoint (int pid)
 int
 child_insert_vfork_catchpoint (int pid)
 {
-  if (!linux_supports_tracefork ())
+  if (!linux_supports_tracefork (pid))
     error ("Your system does not support vfork catchpoints.");
 
   return 0;
@@ -494,7 +542,7 @@ child_insert_vfork_catchpoint (int pid)
 int
 child_insert_exec_catchpoint (int pid)
 {
-  if (!linux_supports_tracefork ())
+  if (!linux_supports_tracefork (pid))
     error ("Your system does not support exec catchpoints.");
 
   return 0;

Re: [rfc] Fix linux_test_for_tracefork for newer kernels

[Daniel Jacobowitz]

On Sat, Nov 13, 2004 at 05:25:06PM -0500, Daniel Jacobowitz wrote:
> On Fri, Nov 12, 2004 at 05:59:41PM -0500, Daniel Jacobowitz wrote:
> > This patch makes the run-time test for fork tracing a little more robust.
> > In particular, it fixes two things:
> >   - If the test failed in an unexpected way, then two stopped processes
> >     could be left around.
> >   - Waitpid can be interrupted by EINTR.  Kernel 2.6.10-rc1 has some
> >     changes to wait and signal processing, which cause the SIGCHLD
> >     to be received before wait returns.  I don't think there's any
> >     actual bug here, since reissuing the wait works fine.
> > 
> > These changes let my testsuite runs finish without leaving hordes of stopped
> > processes around.  I won't check it in yet, since I haven't tested it on a
> > system where this runtime check should fail; in the meantime, I'm just
> > posting it for comments.  Hopefully I got it right this time!
> 
> Now I've tested it in a couple of other systems.  Here's a revised
> version, with a spurious warning fixed, and one additional improvement:
> on most systems lacking the support, we don't need to fork for the
> runtime test.  This fixes some problems in user-mode-linux, which does
> the most disgusting things with ptrace I have ever encountered.  Wow!
> 
> Barring comments, I'll check this in in a few days.

> 2004-11-12  Daniel Jacobowitz  <dan@debian.org>
> 
> 	* linux-nat.c (my_waitpid): New function.
> 	(linux_test_for_tracefork): Make more robust and verbose.  Take
> 	an ORIGINAL_PID argument and test for PTRACE_SETOPTIONS first.
> 	(linux_supports_tracefork, linux_supports_tracevforkdone): Take a PID
> 	argument.  Update calls to linux_test_for_tracefork.
> 	(linux_enable_event_reporting, child_follow_fork)
> 	(child_insert_fork_catchpoint, child_insert_vfork_catchpoint)
> 	(child_insert_exec_catchpoint): Update calls to
> 	linux_supports_tracefork and linux_supports_tracevforkdone.

I have checked this in.  It fixes a couple of hangs when running the
testsuite, for tests which expect GDB to exit, and it fixes the huge
number of zombies and stopped processes that my system has been
collecting every test run.

-- 
Daniel Jacobowitz