[1/4,REPOST] Remote Linux ptrace exit events

  >>>>> "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
  

Hi Tom,
Thank you for looking at this.  Several responses below...

On 5/21/2014 8:15 AM, Tom Tromey wrote:
>>>>>> "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.

As you saw later in the "Patch 0" email:
Don> Use of
Don> this event was necessary due to a race condition in the two-thread
case.
Don> It is only used internally; exit events detected this way are not
exposed
Don> to the user, and exit processing was changed as little as possible.

I believe exit events are needed in GDB.  I'm fairly confident that the
same race condition I found in gdbserver exists in GDB.  It is a really
small window, though, so it might be difficult to concoct a test case
that demonstrates it reliably.

I totally agree that moving the two implementations closer together
should be the goal, but my position is that GDB needs to use this new
approach, as opposed to vice versa.  Since I started this work on the
gdbserver side, I'd like to get buy-in on the exit event approach before
spending effort implementing the same thing on the native GDB side.

> 
> 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.

Agreed, but I'm going to hold off making this change for now.  If we go
with the exit event approach GDB and gdbserver will converge anyway.

> 
> 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.

PTRACE_EVENT_EXIT is reported for each LWP.  Here is a snippet of
--debug output from debugging (with exit events) a test program that
creates four pthreads that each do some busywork and exit:

LHEW: Got exit event from LWP 10650
--snip--
LHEW: Got exit event from LWP 10648
--snip--
LHEW: Got exit event from LWP 10649
--snip--
LHEW: Got exit event from LWP 10647
--snip--
LHEW: Got exit event from LWP 10634
LWP 10634 is the last lwp of process.  Process 10634 exiting.
linux_wait_1 ret = LWP 10634.10634, exited with retcode 0

I agree that the ptrace man page is not really clear on this point.  In
the section on execve it does say that all non-execing threads stop in
PTRACE_EVENT_EXIT stop, which is ambiguous about reporting the event.

I'm working on a revised version of the patch series, and I'll include a
detailed description of the race condition.  Yao also asked about this.

Thanks!
--Don

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