Re: [PATCH v7 3/7] Extended-remote Linux follow fork

[Pedro Alves <palves at redhat dot com>]

On 04/10/2015 06:09 PM, Don Breazeal wrote:

>>>  /* Do low-level handling of the event, and check if we should go on
>>>     and pass it to caller code.  Return the affected lwp if we are, or
>>>     NULL otherwise.  */
>>> @@ -1912,11 +1983,11 @@ linux_low_filter_event (int lwpid, int wstat)
>>>  	}
>>>      }
>>>  
>>> -  if (WIFSTOPPED (wstat) && child->must_set_ptrace_flags)
>>> +  if (WIFSTOPPED (wstat) && child->must_set_ptrace_flags && gdb_connected ())
>>>      {
>>
>> I don't really understand this.  If the flag is set, why would it matter
>> whether gdb is connected?
> 
> My thinking was that there was no point in setting the ptrace options 
> until GDB had connected, since the qSupported packet right after connection
> would cause a reset of the ptrace options. However, your point is taken, it
> doesn't really matter, so I have remove the call to gdb_connected.

It's not just that it doesn't matter; it's really wrong.  If we attach to
a multi-threaded program in non-stop mode, and set some tracepoints and
detach for disconnected tracing before all threads stop and set their
ptrace flags, we'll miss setting the ptrace options on some threads.
But we need to be able to at least follow clone events in that case.

> 
>>
>>>        struct process_info *proc = find_process_pid (pid_of (thread));
>>>  
>>> -      linux_enable_event_reporting (lwpid, proc->attached);
>>> +      linux_low_enable_events (lwpid, proc->attached);
>>>        child->must_set_ptrace_flags = 0;
>>>      }
>>>  



>>> +
>>> +/* Target hook for 'handle_new_gdb_connection'.  Causes a reset of the
>>> +   ptrace flags for all inferiors.  This is in case the new GDB connection
>>> +   doesn't support the same set of events that the previous one did.  */
>>> +
>>> +static void
>>> +linux_handle_new_gdb_connection (void)
>>> +{
>>> +  pid_t pid;
>>> +
>>> +  /* Reset the ptrace options to enable on the inferior(s).  */
>>> +  linux_reset_ptrace_options ();
>>> +
>>> +  /* Request that all the lwps reset their ptrace options.  */
>>> +  find_inferior (&all_threads, reset_lwp_ptrace_options_callback , &pid);
>>
>> Spurious space before ', &pid'.  But, you can't do this if threads
>> are already running, such as when you reconnect after you left the
>> target running for disconnected tracing.  Instead, you need to
>> force threads to momentarily pause and set their must_set_ptrace_flags
>> flag, much like when we need to change running thread's debug registers
>> for watchpoints.  See linux-x86-low.c:update_debug_registers_callback.
> 
> I've added the mechanism to stop the lwps as in that function.  In a
> comment below I believe you were proposing that this mechanism could be
> unnecessary, and that resetting the ptrace flags for a new connection
> could be handled another way.  I still think this mechanism is necessary.
> See the comment below after linux_enable_event_reporting for my rationale.

No, that must be a misunderstanding.  The mechanism to reset the ptrace
flags for a new connection is indeed necessary (I was the one that brought
it up); what I thought was unnecessary was the additional_flags juggling
in the previous iteration.

>> That'd cope with a different gdb reconnecting and requesting
>> different options too.
> 
> This is definitely more concise, thanks.
> 
> One outcome of my changes here is the elimination of the "additional_flags"
> mechanism. 

Right.

> This had a side-effect in the linux-ptrace functions like
> linux_test_for_tracefork, removing the checks that allowed the functions to
> bail out if additional_flags did not include the flags being tested.  I
> don't think that this is a problem, since the events will not be enabled
> unless they are supported and requested.

That's not really a problem.  We want gdbserver to support all those
features anyway.

> 
> As I noted above, I think we still need handle_new_gdb_connection to ensure
> that the ptrace options are set properly when GDB reconnects.
> 
> Rationale: suppose we have the following sequence of events:
> 1) GDB that supports fork event connects, and gdbserver sets
>    PTRACE_O_TRACEFORK
> 2) inferior stops at a breakpoint
> 3) GDB disconnects, inferior remains stopped
> 4) GDB that does not support fork events connects.  ptrace options are
>    unchanged without handle_new_gdb_connection.
> 5) inferior is resumed and executes a fork.  A fork event is
>    generated and sent to GDB, who doesn't recognize it because we
>    did not reset the options when the new GDB connected.

Right.


>>
>>> +static int
>>> +remote_follow_fork (struct target_ops *ops, int follow_child,
>>> +		    int detach_fork)
>>> +{
>>> +  struct remote_state *rs = get_remote_state ();
>>> +
>>> +  if (remote_fork_event_p (rs))
>>> +    {
>>> +      if (detach_fork && !follow_child)
>>
>> Aren't we missing the "detach_fork && follow_child" case?
> 
> That case is handled in the target-independent code in infrun.c by 
> calling target_detach.  

Could you add a one-liner comment mentioning that?

> @@ -449,6 +451,57 @@ handle_extended_wait (struct lwp_info *event_child, int wstat)
>  	    warning ("wait returned unexpected status 0x%x", status);
>  	}
>  
> +      if (event == PTRACE_EVENT_FORK)
> +	{
> +	  struct process_info *parent_proc;
> +	  struct process_info *child_proc;
> +	  struct lwp_info *child_lwp;
> +	  struct target_desc *tdesc;
> +
> +	  ptid = ptid_build (new_pid, new_pid, 0);
> +
> +	  if (debug_threads)
> +	    {
> +	      debug_printf ("HEW: Got fork event from LWP %ld, "
> +			    "new child is %d\n",
> +			    ptid_get_lwp (ptid_of (event_thr)),
> +			    ptid_get_pid (ptid));
> +	    }
> +
> +	  /* Add the new process to the tables and clone the breakpoint
> +	     lists of the parent.  We need to do this even if the new process
> +	     will be detached, since we will need the process object and the
> +	     breakpoints to remove any breakpoints from memory when we
> +	     detach, and the host side will access registers.  */

Say "client side" instead of "host side".


> +/* Callback for 'find_inferior'.  Set the (possibly changed) ptrace
> +   options for the specified lwp.  */
> +
> +static int
> +reset_lwp_ptrace_options_callback (struct inferior_list_entry *entry,
> +				   void *args)
> +{
> +  struct thread_info *thread = (struct thread_info *) entry;
> +  struct lwp_info *lwp = get_thread_lwp (thread);
> +  struct process_info *proc = find_process_pid (pid_of (thread));
> +  int options = linux_low_ptrace_options (proc->attached);
> +
> +  if (!lwp->stopped)
> +    {
> +      /* Stop the lwp so we can modify its ptrace options.  */
> +      linux_stop_lwp (lwp);
> +    }
> +
> +  linux_enable_event_reporting (lwpid_of (thread), options);
> +  lwp->must_set_ptrace_flags = 0;

This still has the same problem.  linux_stop_lwp does not
wait for the LWP to stop, it just sends it a SIGSTOP signal.
We can only set the ptrace options when the LWP ptrace-stops,
that is, after we see it stop with waitpid.  That is the whole
point of the must_set_ptrace_flags flag.  Do it like this:

  if (!lwp->stopped)
    {
      /* Stop the lwp so we can modify its ptrace options.  */
      lwp->must_set_ptrace_flags = 1;
      stop_lwp (lwp);
    }
  else
    {
       /* Already stopped; go ahead and set the ptrace options.  */
       struct process_info *proc = find_process_pid (pid_of (thread));
       int options = linux_low_ptrace_options (proc->attached);

       linux_enable_event_reporting (lwpid_of (thread), options);
    }



>  
> +static int
> +linux_nat_ptrace_options (int attached)

Missing intro comment.

> +{
> +  int options = 0;
> +
> +  if (!attached)
> +    options |= PTRACE_O_EXITKILL;
> +
> +  options |= (PTRACE_O_TRACESYSGOOD
> +	      | PTRACE_O_TRACEVFORKDONE
> +	      | PTRACE_O_TRACEVFORK
> +	      | PTRACE_O_TRACEFORK
> +	      | PTRACE_O_TRACEEXEC);
> +
> +  return options;
> +}
> +


> --- a/gdb/nat/linux-ptrace.c
> +++ b/gdb/nat/linux-ptrace.c
> @@ -25,14 +25,10 @@
>  
>  #include <stdint.h>
>  
> -/* Stores the currently supported ptrace options.  A value of
> -   -1 means we did not check for features yet.  A value of 0 means
> -   there are no supported features.  */
> -static int current_ptrace_options = -1;
> -
> -/* Additional flags to test.  */
> -
> -static int additional_flags;
> +/* Stores the ptrace options supported by the target.

s/by the target/by the running kernel/

> +   A value of -1 means we did not check for features yet.  A value
> +   of 0 means there are no supported features.  */
> +static int supported_ptrace_options = -1;
>  
>  /* Find all possible reasons we could fail to attach PID and append
>     these as strings to the already initialized BUFFER.  '\0'
> @@ -343,7 +339,7 @@ linux_check_ptrace_features (void)
>    int child_pid, ret, status;
>  
>    /* Initialize the options.  */
> -  current_ptrace_options = 0;
> +  supported_ptrace_options = 0;
>  
>    /* Fork a child so we can do some testing.  The child will call
>       linux_child_function and will get traced.  The child will
> @@ -387,14 +383,11 @@ linux_test_for_tracesysgood (int child_pid)
>  {
>    int ret;
>  
> -  if ((additional_flags & PTRACE_O_TRACESYSGOOD) == 0)
> -    return;
> -
>    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;
> +    supported_ptrace_options |= PTRACE_O_TRACESYSGOOD;
>  }
>  
>  /* Determine if PTRACE_O_TRACEFORK can be used to follow fork
> @@ -414,15 +407,12 @@ linux_test_for_tracefork (int child_pid)
>    if (ret != 0)
>      return;
>  
> -  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)
> -	current_ptrace_options |= PTRACE_O_TRACEVFORKDONE;
> -    }
> +  /* 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)
> +    supported_ptrace_options |= 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
> @@ -458,10 +448,13 @@ linux_test_for_tracefork (int child_pid)
>  
>  	  /* We got the PID from the grandchild, which means fork
>  	     tracing is supported.  */
> -	  current_ptrace_options |= PTRACE_O_TRACECLONE;
> -	  current_ptrace_options |= (additional_flags & (PTRACE_O_TRACEFORK
> -                                                         | PTRACE_O_TRACEVFORK
> -                                                         | PTRACE_O_TRACEEXEC));
> +	  supported_ptrace_options |= PTRACE_O_TRACECLONE;
> +
> +	  /* Save the "extended" options in case we need to reset
> +	     the options later for a connect from a different GDB.  */

Remove now stale comment.

> +	  supported_ptrace_options |= (PTRACE_O_TRACEFORK
> +				       | PTRACE_O_TRACEVFORK
> +				       | PTRACE_O_TRACEEXEC);
>  
>  	  /* Do some cleanup and kill the grandchild.  */
>  	  my_waitpid (second_pid, &second_status, 0);

>  
>  /* Enable reporting of all currently supported ptrace events.
>     ATTACHED should be nonzero if we have attached to the inferior.  */

Update comment.  I had suggested:

/* Enable reporting of all currently supported ptrace events.
   OPTIONS is a bit mask of extended features we want enabled,
   if supported by the kernel.  PTRACE_O_TRACECLONE is always
   enabled, if supported.  */

>  
>  void
> -linux_enable_event_reporting (pid_t pid, int attached)
> +linux_enable_event_reporting (pid_t pid, int options)
>  {
> -  int ptrace_options;
> -
>    /* Check if we have initialized the ptrace features for this
>       target.  If not, do it now.  */
> -  if (current_ptrace_options == -1)
> +  if (supported_ptrace_options == -1)
>      linux_check_ptrace_features ();
>  
> -  ptrace_options = current_ptrace_options;
> -  if (attached)
> -    {
> -      /* When attached to our inferior, we do not want the inferior
> -	 to die with us if we terminate unexpectedly.  */
> -      ptrace_options &= ~PTRACE_O_EXITKILL;
> -    }
> +  /* We always want clone events.  */
> +  options |= PTRACE_O_TRACECLONE;
> +
> +  /* Filter out unsupported options.  */
> +  options &= supported_ptrace_options;
>  
>    /* Set the options.  */
>    ptrace (PTRACE_SETOPTIONS, pid, (PTRACE_TYPE_ARG3) 0,
> -	  (PTRACE_TYPE_ARG4) (uintptr_t) ptrace_options);
> +	  (PTRACE_TYPE_ARG4) (uintptr_t) options);
>  }



> +
> +/* This detaches a program to which we previously attached, using
> +   inferior_ptid to identify the process.  After this is done, GDB
> +   can be used to debug some other program.  We better not have left
> +   any breakpoints in the target program or it'll die when it hits
> +   one.  If IS_FORK_CHILD is true, then inferior_ptid is the child
> +   of an unfollowed fork, and we need to avoid deleting breakpoints
> +   still needed by the parent.  */
>  
>  static void
> -remote_detach_1 (const char *args, int from_tty, int extended)
> +remote_detach_1 (const char *args, int from_tty, int is_fork_child)

Do we still need this "is_fork_child" change?  I don't see where
any caller has been updated in this version of the patch.  AFAICS
current callers still pass "extended":

static void
remote_detach (struct target_ops *ops, const char *args, int from_tty)
{
  remote_detach_1 (args, from_tty, 0);
}

static void
extended_remote_detach (struct target_ops *ops, const char *args, int from_tty)
{
  remote_detach_1 (args, from_tty, 1);
}

So something doesn't feel right here.

>  {
>    int pid = ptid_get_pid (inferior_ptid);
>    struct remote_state *rs = get_remote_state ();
> +  struct thread_info *tp = find_thread_ptid (inferior_ptid);
> +  int is_fork_parent;
>  
>    if (args)
>      error (_("Argument given to \"detach\" when remotely debugging."));
> @@ -4447,25 +4483,25 @@ remote_detach_1 (const char *args, int from_tty, int extended)
>      }
>  
>    /* Tell the remote target to detach.  */
> -  if (remote_multi_process_p (rs))
> -    xsnprintf (rs->buf, get_remote_packet_size (), "D;%x", pid);
> -  else
> -    strcpy (rs->buf, "D");
> +  remote_detach_pid (pid);
>  
> -  putpkt (rs->buf);
> -  getpkt (&rs->buf, &rs->buf_size, 0);
> -
> -  if (rs->buf[0] == 'O' && rs->buf[1] == 'K')
> -    ;
> -  else if (rs->buf[0] == '\0')
> -    error (_("Remote doesn't know how to detach"));
> -  else
> -    error (_("Can't detach process."));
> -
> -  if (from_tty && !extended)
> +  if (from_tty && !rs->extended)
>      puts_filtered (_("Ending remote debugging.\n"));
>  
> -  target_mourn_inferior ();
> +  /* Check to see if we are detaching a fork parent.  Note that if we
> +     are detaching a fork child, tp == NULL.  */
> +  if (tp != NULL)
> +    is_fork_parent = tp->pending_follow.kind == TARGET_WAITKIND_FORKED;
> +
> +  /* If doing detach-on-fork, we don't mourn, because that will delete
> +     breakpoints that should be available for the followed inferior.  */

How does this work in the native case then?

> +  if (!is_fork_child && !is_fork_parent)
> +    target_mourn_inferior ();
> +  else
> +    {
> +      inferior_ptid = null_ptid;
> +      detach_inferior (pid);
> +    }
>  }
>  

Thanks,
Pedro Alves