diff mbox

[02/15,v2] Remove some GDBSERVER checks from linux-ptrace

Message ID 1405520243-17282-3-git-send-email-gbenson@redhat.com
State Changes Requested, archived
Headers show

Commit Message

Gary Benson July 16, 2014, 2:17 p.m. UTC
This patch removes some GDBSERVER checks from nat/linux-ptrace.c.
Currently the code uses a compile-time check to decide whether some
flags should be used.  This changes the code to instead let users of
the module specify an additional set of flags; and then changes gdb's
linux-nat.c to call this function.  At some later date, when the back
ends are fully merged, we will be able to remove this function again.

gdb/
2014-07-16  Tom Tromey  <tromey@redhat.com>
	    Gary Benson  <gbenson@redhat.com>

	* nat/linux-ptrace.c (additional_flags): New global.
	(linux_test_for_tracesysgood, linux_test_for_tracefork): Use
	additional_flags; don't check GDBSERVER.
	(linux_ptrace_set_additional_flags): New function.
	* nat/linux-ptrace.h (linux_ptrace_set_additional_flags):
	Declare.
	* linux-nat.c (_initialize_linux_nat): Call
	linux_ptrace_set_additional_flags.
---
 gdb/ChangeLog          |   12 +++++++++
 gdb/linux-nat.c        |    8 ++++++
 gdb/nat/linux-ptrace.c |   64 ++++++++++++++++++++++++-----------------------
 gdb/nat/linux-ptrace.h |    1 +
 4 files changed, 54 insertions(+), 31 deletions(-)

Comments

Doug Evans July 17, 2014, 8:35 a.m. UTC | #1
On Wed, Jul 16, 2014 at 7:17 AM, Gary Benson <gbenson@redhat.com> wrote:
> This patch removes some GDBSERVER checks from nat/linux-ptrace.c.
> Currently the code uses a compile-time check to decide whether some
> flags should be used.  This changes the code to instead let users of
> the module specify an additional set of flags; and then changes gdb's
> linux-nat.c to call this function.  At some later date, when the back
> ends are fully merged, we will be able to remove this function again.
>
> gdb/
> 2014-07-16  Tom Tromey  <tromey@redhat.com>
>             Gary Benson  <gbenson@redhat.com>
>
>         * nat/linux-ptrace.c (additional_flags): New global.
>         (linux_test_for_tracesysgood, linux_test_for_tracefork): Use
>         additional_flags; don't check GDBSERVER.
>         (linux_ptrace_set_additional_flags): New function.
>         * nat/linux-ptrace.h (linux_ptrace_set_additional_flags):
>         Declare.
>         * linux-nat.c (_initialize_linux_nat): Call
>         linux_ptrace_set_additional_flags.

LGTM.

I had a thought, and please don't feel compelled to have to submit yet
another revision.
I'm just writing this down for completeness' sake.
Since linux_ptrace_set_additional_flags is supposed to be called
before any other related function, one could add an assert to the
other functions to verify this.

> [...]
> +/* Set additional ptrace flags to use.  Some such flags may be checked
> +   by the implementation above.  This function must be called before
> +   any other function in this file; otherwise the flags may not take
> +   effect appropriately.  */
> +
> +void
> +linux_ptrace_set_additional_flags (int flags)
> +{
> +  additional_flags = flags;
> +}
Pedro Alves July 17, 2014, 4:35 p.m. UTC | #2
On 07/16/2014 03:17 PM, Gary Benson wrote:
> This patch removes some GDBSERVER checks from nat/linux-ptrace.c.
> Currently the code uses a compile-time check to decide whether some
> flags should be used.  This changes the code to instead let users of
> the module specify an additional set of flags; and then changes gdb's
> linux-nat.c to call this function. 

> At some later date, when the back
> ends are fully merged, we will be able to remove this function again.


> @@ -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 as strings to the already 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)
> +    {

Please avoid this indenting unless necessary, by
reversing the logic:

  if ((additional_flags & PTRACE_O_TRACESYSGOOD) == 0)
     return;

As this will end up being undone once additional_flags is
removed again later on, this way avoid re-indenting back,
and complicating history / git blame.

> +      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;

This isn't right.  The function should still update
current_ptrace_options (the effectively enabled options).

Otherwise ...

> +    }
>  }
>  
>  /* 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;

... note how PTRACE_O_TRACESYSGOOD is lost if PTRACE_O_TRACECLONE
isn't supported.

So don't clear additional_flags.  Instead add to current_ptrace_options
as options required in additional_flags are detected to be supported.
diff mbox

Patch

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index c738abf..de31229 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -5033,6 +5033,14 @@  Enables printf debugging output."),
   sigdelset (&suspend_mask, SIGCHLD);
 
   sigemptyset (&blocked_mask);
+
+  /* Do not enable PTRACE_O_TRACEEXIT until GDB is more prepared to
+     support read-only process state.  */
+  linux_ptrace_set_additional_flags (PTRACE_O_TRACESYSGOOD
+				     | PTRACE_O_TRACEVFORKDONE
+				     | PTRACE_O_TRACEVFORK
+				     | PTRACE_O_TRACEFORK
+				     | PTRACE_O_TRACEEXEC);
 }
 
 
diff --git a/gdb/nat/linux-ptrace.c b/gdb/nat/linux-ptrace.c
index 3ad2113..59c9bfa 100644
--- a/gdb/nat/linux-ptrace.c
+++ b/gdb/nat/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 as strings to the already 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);
@@ -551,3 +542,14 @@  linux_ptrace_init_warnings (void)
 
   linux_ptrace_test_ret_to_nx ();
 }
+
+/* Set additional ptrace flags to use.  Some such flags may be checked
+   by the implementation above.  This function must be called before
+   any other function in this file; otherwise the flags may not take
+   effect appropriately.  */
+
+void
+linux_ptrace_set_additional_flags (int flags)
+{
+  additional_flags = flags;
+}
diff --git a/gdb/nat/linux-ptrace.h b/gdb/nat/linux-ptrace.h
index cffb5ce..41b3198 100644
--- a/gdb/nat/linux-ptrace.h
+++ b/gdb/nat/linux-ptrace.h
@@ -91,5 +91,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 */