Re: [PATCH 1/4][REPOST] Remote Linux ptrace exit events

[Tom Tromey <tromey@redhat.com>]

>>>>> "Don" == Don Breazeal <donb@codesourcery.com> writes:

Don> This patch implements support for the extended ptrace event
Don> PTRACE_EVENT_EXIT on Linux.  This is a preparatory patch for exec event
Don> support.

Thanks.

I had a few questions about this patch.

Don> 	* common/linux-ptrace.c (linux_test_for_tracefork)
Don> 	[GDBSERVER]: Add support for PTRACE_EVENT_EXIT if the OS
Don> 	supports it.

I'm curious why PTRACE_O_TRACEEXIT is needed by gdbserver to implement
this feature.  It isn't needed by gdb.  And, I think it's preferable to
try to push the two back ends closer together -- it's a long-term goal
-- so new divergences are subject to special scrutiny.

Don> +#ifdef GDBSERVER
Don> +  /* PTRACE_O_FORK is supported, so now test for PTRACE_O_TRACEEXIT.
Don> +     First try to set the option.  If this fails, we know for sure that
Don> +     it is not supported.  */
Don> +  ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
Don> +		(PTRACE_TYPE_ARG4) PTRACE_O_TRACEEXIT);
Don> +  if (ret != 0)
Don> +    return;
[...]

It would be much nicer to reduce the use of #ifdef GDBSERVER, rather
than to add to it.

I think this could be done a different way, say by having the clients of
this interface specify which flags they're interested in.  Then
gdbserver could ask for PTRACE_O_TRACEEXIT and gdb could refrain.

This would be just as simple but still be a good fit for the long-term
goal.

I've appended a patch I wrote along these lines -- perhaps hacky,
definitely untested -- from my experiment with moving gdbserver to
top-level.  Feel free to use or ignore it.

Don> +  else if (event == PTRACE_EVENT_EXIT)
Don> +    {
Don> +      unsigned long exit_status;
Don> +      unsigned long lwpid = lwpid_of (event_thr);
Don> +      int ret;
Don> +
Don> +      if (debug_threads)
Don> +        debug_printf ("LHEW: Got exit event from LWP %ld\n",
Don> +                      lwpid_of (event_thr));
Don> +
Don> +      ptrace (PTRACE_GETEVENTMSG, lwpid_of (event_thr), (PTRACE_TYPE_ARG3) 0,
Don> +	      &exit_status);
Don> +
Don> +      if (num_lwps (pid_of (event_thr)) > 1)
Don> +        {
Don> +	  /* If there is at least one more LWP, then the program still
Don> +	     exists and the exit should not be reported to GDB.  */

I thought, from the man page, that PTRACE_EVENT_EXIT was for a process
exit event only.  So checking num_lwps doesn't seem correct here.  But
after seeing your patch I am not sure; and I would like to know the
answer.

Tom


commit a1b6a7417e0e192c8f925ac94491a819c384c7e9
Author: Tom Tromey <tromey@redhat.com>
Date:   Fri Jan 3 10:55:52 2014 -0700

    remove some GDBSERVER checks from linux-ptrace

diff --git a/gdb/common/linux-ptrace.c b/gdb/common/linux-ptrace.c
index 7c1b78a..d1659e0 100644
--- a/gdb/common/linux-ptrace.c
+++ b/gdb/common/linux-ptrace.c
@@ -37,6 +37,10 @@
    there are no supported features.  */
 static int current_ptrace_options = -1;
 
+/* Additional flags to test.  */
+
+static int additional_flags;
+
 /* Find all possible reasons we could fail to attach PID and append these
    newline terminated reason strings to initialized BUFFER.  '\0' termination
    of BUFFER must be done by the caller.  */
@@ -359,16 +363,15 @@ linux_check_ptrace_features (void)
 static void
 linux_test_for_tracesysgood (int child_pid)
 {
-#ifdef GDBSERVER
-  /* gdbserver does not support PTRACE_O_TRACESYSGOOD.  */
-#else
-  int ret;
+  if ((additional_flags & PTRACE_O_TRACESYSGOOD) != 0)
+    {
+      int ret;
 
-  ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
-		(PTRACE_TYPE_ARG4) PTRACE_O_TRACESYSGOOD);
-  if (ret == 0)
-    current_ptrace_options |= PTRACE_O_TRACESYSGOOD;
-#endif
+      ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
+		    (PTRACE_TYPE_ARG4) PTRACE_O_TRACESYSGOOD);
+      if (ret != 0)
+	additional_flags &= ~PTRACE_O_TRACESYSGOOD;
+    }
 }
 
 /* Determine if PTRACE_O_TRACEFORK can be used to follow fork
@@ -388,16 +391,15 @@ linux_test_for_tracefork (int child_pid)
   if (ret != 0)
     return;
 
-#ifdef GDBSERVER
-  /* gdbserver does not support PTRACE_O_TRACEVFORKDONE yet.  */
-#else
-  /* Check if the target supports PTRACE_O_TRACEVFORKDONE.  */
-  ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
-		(PTRACE_TYPE_ARG4) (PTRACE_O_TRACEFORK
-				    | PTRACE_O_TRACEVFORKDONE));
-  if (ret == 0)
-    current_ptrace_options |= PTRACE_O_TRACEVFORKDONE;
-#endif
+  if ((additional_flags & PTRACE_O_TRACEVFORKDONE) != 0)
+    {
+      /* Check if the target supports PTRACE_O_TRACEVFORKDONE.  */
+      ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
+		    (PTRACE_TYPE_ARG4) (PTRACE_O_TRACEFORK
+					| PTRACE_O_TRACEVFORKDONE));
+      if (ret != 0)
+	additional_flags &= ~PTRACE_O_TRACEVFORKDONE;
+    }
 
   /* Setting PTRACE_O_TRACEFORK did not cause an error, however we
      don't know for sure that the feature is available; old
@@ -433,18 +435,7 @@ linux_test_for_tracefork (int child_pid)
 
 	  /* We got the PID from the grandchild, which means fork
 	     tracing is supported.  */
-#ifdef GDBSERVER
-	  /* Do not enable all the options for now since gdbserver does not
-	     properly support them.  This restriction will be lifted when
-	     gdbserver is augmented to support them.  */
-	  current_ptrace_options |= PTRACE_O_TRACECLONE;
-#else
-	  current_ptrace_options |= PTRACE_O_TRACEFORK | PTRACE_O_TRACEVFORK
-	    | PTRACE_O_TRACECLONE | PTRACE_O_TRACEEXEC;
-
-	  /* Do not enable PTRACE_O_TRACEEXIT until GDB is more prepared to
-	     support read-only process state.  */
-#endif
+	  current_ptrace_options |= PTRACE_O_TRACECLONE | additional_flags;
 
 	  /* Do some cleanup and kill the grandchild.  */
 	  my_waitpid (second_pid, &second_status, 0);
@@ -542,3 +533,9 @@ linux_ptrace_init_warnings (void)
 
   linux_ptrace_test_ret_to_nx ();
 }
+
+void
+linux_ptrace_set_additional_flags (int flags)
+{
+  additional_flags = flags;
+}
diff --git a/gdb/common/linux-ptrace.h b/gdb/common/linux-ptrace.h
index 38bb9ea..e5094df 100644
--- a/gdb/common/linux-ptrace.h
+++ b/gdb/common/linux-ptrace.h
@@ -90,5 +90,6 @@ extern int linux_supports_tracefork (void);
 extern int linux_supports_traceclone (void);
 extern int linux_supports_tracevforkdone (void);
 extern int linux_supports_tracesysgood (void);
+extern void linux_ptrace_set_additional_flags (int);
 
 #endif /* COMMON_LINUX_PTRACE_H */
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index bf6f586..289ae41 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -4980,6 +4980,12 @@ Enables printf debugging output."),
   sigdelset (&suspend_mask, SIGCHLD);
 
   sigemptyset (&blocked_mask);
+
+  linux_ptrace_set_additional_flags (PTRACE_O_TRACESYSGOOD
+				     | PTRACE_O_TRACEVFORKDONE
+				     | PTRACE_O_TRACEVFORK
+				     | PTRACE_O_TRACEFORK
+				     | PTRACE_O_TRACEEXEC);
 }
 \f