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

  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(-)
  

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

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.