Re: [PATCH 1/3] Target remote mode fork and exec events

[Pedro Alves <palves at redhat dot com>]

Hi Don,

Thanks for doing this.  Starting to look at the series.

On 11/06/2015 11:56 PM, Don Breazeal wrote:
> This patch implements support for fork and exec events with target remote
> mode Linux targets.  For such targets with Linux kernels 2.5.46 and later,
> this enables follow-fork-mode, detach-on-fork and fork and exec
> catchpoints.  

> Note that follow-exec-mode is not supported, because target
> remote mode does not support the 'run' command.

Not sure I don't understand this part/comment.

> 
> The changes required to implement this included:
> 
>  * Don't exit from gdbserver if there are still active inferiors.
> 
>  * Allow changing the active process in remote mode.
> 
>  * Enable fork and exec events in remote mode.
> 
>  * Print "Ending remote debugging" when detaching only if in remote
>    mode and there is only one inferior left.
> 
>    (As I write this up I'm concluding that this is incorrect.  We
>    don't care how many active inferiors there are, yes?

Not sure I understand the question.  But I'd say what matters is
whether we're disconnecting/closing the connection.

> Perhaps we
>    need a test that detaches when there are multiple inferiors. I've
>    added that to my list.)
> 
>  * Combine remote_kill and extended_remote_kill into a single function
>    that can handle the multiple inferior case for target remote.  Also,
>    the same thing for remote_mourn and extended_remote_mourn.
> 
>  * Enable process-style ptids in target remote.
> 
>  * Remove restriction on multiprocess mode in target remote.
> 
> Thanks
> --Don
> 
> gdb/gdbserver/
> 2015-11-06  Don Breazeal  <donb@sourceware.org>
> 
> 	* server.c (process_serial_event): Don't exit from gdbserver
> 	in remote mode if there are still active inferiors.
> 
> gdb/
> 2015-11-06  Don Breazeal  <donb@sourceware.org>
> 
> 	* remote.c (set_general_process): Remove restriction on target
> 	remote mode.
> 	(remote_query_supported): Likewise.
> 	(remote_detach_1): Change restriction to target remote mode to
> 	restriction to last inferior left.
> 	(remote_disconnect): Unpush the target directly instead of 
> 	calling remote_mourn.
> 	(remote_kill, extended_remote_kill): Combine functions into one,
> 	remote_kill, and enable extended functionality for target remote.
> 	(remote_mourn, extended_remote_mourn): Combine functions into
> 	one, remote_mourn, and enable extended functionality for target
> 	remote.
> 	(remote_pid_to_str): Enable "process" style ptid string for
> 	target remote.
> 	(remote_supports_multi_process): Remove restriction on target
> 	remote mode.
> 
> ---
>  gdb/gdbserver/server.c |   6 +-
>  gdb/remote.c           | 166 ++++++++++++++++++++++++-------------------------
>  2 files changed, 84 insertions(+), 88 deletions(-)
> 
> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index fd2804b..0f57237 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -3865,9 +3865,11 @@ process_serial_event (void)
>  	  discard_queued_stop_replies (pid_to_ptid (pid));
>  	  write_ok (own_buf);
>  
> -	  if (extended_protocol)
> +	  if (extended_protocol || get_first_inferior (&all_threads) != NULL)
>  	    {
> -	      /* Treat this like a normal program exit.  */
> +	      /* There is still at least one inferior remaining, so
> +		 don't terminate gdbserver and treat this like a
> +		 normal program exit.  */
>  	      last_status.kind = TARGET_WAITKIND_EXITED;
>  	      last_status.value.integer = 0;
>  	      last_ptid = pid_to_ptid (pid);
> diff --git a/gdb/remote.c b/gdb/remote.c
> index fed397a..60da26c 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -123,8 +123,6 @@ static void remote_mourn (struct target_ops *ops);
>  
>  static void extended_remote_restart (void);
>  
> -static void extended_remote_mourn (struct target_ops *);
> -
>  static void remote_send (char **buf, long *sizeof_buf_p);
>  
>  static int readchar (int timeout);
> @@ -2084,7 +2082,7 @@ set_general_process (void)
>    struct remote_state *rs = get_remote_state ();
>  
>    /* If the remote can't handle multiple processes, don't bother.  */
> -  if (!rs->extended || !remote_multi_process_p (rs))
> +  if (!remote_multi_process_p (rs))
>      return;
>  
>    /* We only need to change the remote current thread if it's pointing
> @@ -4472,18 +4470,15 @@ remote_query_supported (void)
>  
>        q = remote_query_supported_append (q, "qRelocInsn+");
>  
> -      if (rs->extended)
> -	{
> -	  if (packet_set_cmd_state (PACKET_fork_event_feature)
> -	      != AUTO_BOOLEAN_FALSE)
> -	    q = remote_query_supported_append (q, "fork-events+");
> -	  if (packet_set_cmd_state (PACKET_vfork_event_feature)
> -	      != AUTO_BOOLEAN_FALSE)
> -	    q = remote_query_supported_append (q, "vfork-events+");
> -	  if (packet_set_cmd_state (PACKET_exec_event_feature)
> -	      != AUTO_BOOLEAN_FALSE)
> -	    q = remote_query_supported_append (q, "exec-events+");
> -	}
> +      if (packet_set_cmd_state (PACKET_fork_event_feature)
> +	  != AUTO_BOOLEAN_FALSE)
> +	q = remote_query_supported_append (q, "fork-events+");
> +      if (packet_set_cmd_state (PACKET_vfork_event_feature)
> +	  != AUTO_BOOLEAN_FALSE)
> +	q = remote_query_supported_append (q, "vfork-events+");
> +      if (packet_set_cmd_state (PACKET_exec_event_feature)
> +	  != AUTO_BOOLEAN_FALSE)
> +	q = remote_query_supported_append (q, "exec-events+");
>  
>        if (packet_set_cmd_state (PACKET_vContSupported) != AUTO_BOOLEAN_FALSE)
>  	q = remote_query_supported_append (q, "vContSupported+");
> @@ -4827,7 +4822,8 @@ remote_detach_1 (const char *args, int from_tty)
>    /* Tell the remote target to detach.  */
>    remote_detach_pid (pid);
>  
> -  if (from_tty && !rs->extended)
> +  /* Exit only if this is the only active inferior.  */
> +  if (from_tty && !rs->extended && number_of_inferiors () == 1)
>      puts_filtered (_("Ending remote debugging.\n"));
>  
>    /* Check to see if we are detaching a fork parent.  Note that if we
> @@ -4923,10 +4919,11 @@ remote_disconnect (struct target_ops *target, const char *args, int from_tty)
>    if (args)
>      error (_("Argument given to \"disconnect\" when remotely debugging."));
>  
> -  /* Make sure we unpush even the extended remote targets; mourn
> -     won't do it.  So call remote_mourn directly instead of
> -     target_mourn_inferior.  */
> -  remote_mourn (target);
> +  /* Make sure we unpush even the extended remote targets.  Calling
> +     target_mourn_inferior won't unpush, and remote_mourn won't
> +     unpush if there is more than one inferior left.  */
> +  unpush_target (target);
> +  generic_mourn_inferior ();

Note: this generic_mourn_inferior call here looks wrong, because we may be
debugging more than one inferior.  But remote_mourn was already wrong
for the same reason, so, not a problem added by this patch.

>  
>    if (from_tty)
>      puts_filtered ("Ending remote debugging.\n");
> @@ -8562,42 +8559,6 @@ kill_new_fork_children (int pid, struct remote_state *rs)
>  }
>  
>  
> -static void
> -remote_kill (struct target_ops *ops)
> -{

Please rename this (making it a helper function) instead of inlining
it in the new remote_kill.  E.g., remote_kill_k.

> -
> -  /* Catch errors so the user can quit from gdb even when we
> -     aren't on speaking terms with the remote system.  */
> -  TRY
> -    {
> -      putpkt ("k");
> -    }
> -  CATCH (ex, RETURN_MASK_ERROR)
> -    {
> -      if (ex.error == TARGET_CLOSE_ERROR)
> -	{
> -	  /* If we got an (EOF) error that caused the target
> -	     to go away, then we're done, that's what we wanted.
> -	     "k" is susceptible to cause a premature EOF, given
> -	     that the remote server isn't actually required to
> -	     reply to "k", and it can happen that it doesn't
> -	     even get to reply ACK to the "k".  */
> -	  return;
> -	}
> -
> -	/* Otherwise, something went wrong.  We didn't actually kill
> -	   the target.  Just propagate the exception, and let the
> -	   user or higher layers decide what to do.  */
> -	throw_exception (ex);
> -    }
> -  END_CATCH
> -
> -  /* We've killed the remote end, we get to mourn it.  Since this is
> -     target remote, single-process, mourning the inferior also
> -     unpushes remote_ops.  */
> -  target_mourn_inferior ();

Maybe do still move this mourn out into new remote_kill.

> -}
> -
>  static int
>  remote_vkill (int pid, struct remote_state *rs)
>  {
> @@ -8624,19 +8585,58 @@ remote_vkill (int pid, struct remote_state *rs)
>  }
>  
>  static void
> -extended_remote_kill (struct target_ops *ops)
> +remote_kill (struct target_ops *ops)
>  {
>    int res;
>    int pid = ptid_get_pid (inferior_ptid);
>    struct remote_state *rs = get_remote_state ();
>  
> +  /* If we are in 'target remote' mode and we are killing the only
> +     inferior, then we will tell gdbserver to exit and unpush the
> +     target.  */
> +  if (!rs->extended && number_of_inferiors () <= 1)

It's number of inferiors, or number of _live_ inferiors that matters?
I'd think the latter.  Likewise all other new number_of_inferiors
calls should be audited.

> +    {
> +      /* Catch errors so the user can quit from gdb even when we
> +	 aren't on speaking terms with the remote system.  */
> +      TRY
> +	{
> +	  putpkt ("k");
> +	}
> +      CATCH (ex, RETURN_MASK_ERROR)
> +	{
> +	  if (ex.error == TARGET_CLOSE_ERROR)
> +	    {
> +	      /* If we got an (EOF) error that caused the target
> +		 to go away, then we're done, that's what we wanted.
> +		 "k" is susceptible to cause a premature EOF, given
> +		 that the remote server isn't actually required to
> +		 reply to "k", and it can happen that it doesn't
> +		 even get to reply ACK to the "k".  */
> +	      return;
> +	    }
> +
> +	  /* Otherwise, something went wrong.  We didn't actually kill
> +	     the target.  Just propagate the exception, and let the
> +	     user or higher layers decide what to do.  */
> +	  throw_exception (ex);
> +	}
> +      END_CATCH
> +
> +      /* We've killed the remote end, we get to mourn it.  Since this is
> +	 target remote, single-process, mourning the inferior also
> +	 unpushes remote_ops.  */

Mentioning "single-process," here no longer looks right to me.


> +      target_mourn_inferior ();
> +
> +      return;
> +    }
> +
>    /* If we're stopped while forking and we haven't followed yet, kill the
>       child task.  We need to do this before killing the parent task
>       because if this is a vfork then the parent will be sleeping.  */
>    kill_new_fork_children (pid, rs);
>  
>    res = remote_vkill (pid, rs);
> -  if (res == -1 && !(rs->extended && remote_multi_process_p (rs)))
> +  if (res == -1 && !(remote_multi_process_p (rs)))

Drop the now-unnecessary parens.

>      {
>        /* Don't try 'k' on a multi-process aware stub -- it has no way
>  	 to specify the pid.  */


I wonder about re-writing this function in top-down approach?

 #0 - if vkill is supported:

     - kill the new fork children first.

     - then kill the current process with remote_vkill.

 #1 - then if not multi-process, and there are still
      live processes, call the new remote_kill_k helper, to kill
      with "k".

 #2 - then, if !extended, and there are no live processes
      left, disconnect.


Note this also gets rid of the putpkt("k") call, which currently misses
handling the EOF issue already handled by the (new) remote_kill_k.

> @@ -8662,16 +8662,17 @@ extended_remote_kill (struct target_ops *ops)
>  static void
>  remote_mourn (struct target_ops *target)
>  {
> -  unpush_target (target);
> +  struct remote_state *rs = get_remote_state ();
>  
> -  /* remote_close takes care of doing most of the clean up.  */
> -  generic_mourn_inferior ();
> -}
> +  /* In 'target remote' mode with one inferior, we shut down gdbserver.  */

I'd rather say:

     /* In 'target remote' mode with one inferior, we close the connection.  */


> +  if (!rs->extended && number_of_inferiors () <= 1)
> +    {
> +      unpush_target (target);
>  
> -static void
> -extended_remote_mourn (struct target_ops *target)
> -{
> -  struct remote_state *rs = get_remote_state ();
> +      /* remote_close takes care of doing most of the clean up.  */
> +      generic_mourn_inferior ();
> +      return;
> +    }
>  
>    /* In case we got here due to an error, but we're going to stay
>       connected.  */
> @@ -8702,8 +8703,9 @@ extended_remote_mourn (struct target_ops *target)
>       current thread.  */
>    record_currthread (rs, minus_one_ptid);
>  
> -  /* Unlike "target remote", we do not want to unpush the target; then
> -     the next time the user says "run", we won't be connected.  */
> +  /* Unlike 'target remote' with no more inferiors, we do not want to
> +     unpush the target.  If we do then the next time the user says
> +     "run", we won't be connected.  */
>  
>    /* Call common code to mark the inferior as not running.	*/
>    generic_mourn_inferior ();
> @@ -10224,7 +10226,7 @@ remote_pid_to_str (struct target_ops *ops, ptid_t ptid)
>      {
>        if (ptid_equal (magic_null_ptid, ptid))
>  	xsnprintf (buf, sizeof buf, "Thread <main>");
> -      else if (rs->extended && remote_multi_process_p (rs))
> +      else if (remote_multi_process_p (rs))
>  	if (ptid_get_lwp (ptid) == 0)
>  	  return normal_pid_to_str (ptid);
>  	else
> @@ -11398,7 +11400,7 @@ remote_supports_multi_process (struct target_ops *self)
>       processes, even though plain remote can use the multi-process
>       thread id extensions, so that GDB knows the target process's
>       PID.  */

Comment above needs updating.

> -  return rs->extended && remote_multi_process_p (rs);
> +  return remote_multi_process_p (rs);
>  }
>  

Thanks,
Pedro Alves