Re: [PATCH v4 1/7] Identify remote fork event support

[Pedro Alves <palves at redhat dot com>]

Hi Don,

On 01/25/2015 09:46 PM, Don Breazeal wrote:
> This patch implements a mechanism for GDB to determine whether fork
> events are supported in gdbserver.  This is a preparatory patch for
> remote fork and exec event support.
> 
> Two new RSP packets are defined to represent fork and vfork event
> support.  These packets are used just like PACKET_multiprocess_feature
> to denote whether the corresponding event is supported.  GDB sends
> fork-events+ and vfork-events+ to gdbserver to inquire about fork
> event support.  If the response enables these packets, then GDB
> knows that gdbserver supports the corresponding events and will
> enable them.
> 
> Target functions used to query for support are included along with
> each new packet.
> 
> In order for gdbserver to know whether the events are supported at the
> point where the qSupported packet arrives, the code in nat/linux-ptrace.c
> had to be reorganized.  Previously it would test for fork/exec event
> support, then enable the events using the pid of the inferior.  When the
> qSupported packet arrives there may not be an inferior.  So the mechanism
> was split into two parts: a function that checks whether the events are
> supported, called when gdbserver starts up, and another that enables the
> events when the inferior stops for the first time.
> 
> Another gdbserver change was to add some global variables similar to
> multi_process, one per new packet.  These are used to control whether
> the corresponsing fork events are enabled.  If GDB does not inquire
> about the event support in the qSupported packet, then gdbserver will
> not set these "report the event" flags.  If the flags are not set, the
> events are ignored like they were in the past.
> 
> There is an #if that has been added temporarily to allow the code for
> managing the events to be included in this patch without enabling the
> events.  See PTRACE_FORK_EVENTS in gdb/remote.c.  This #if will be
> removed later in the patch series as the events are enabled.

(Nit: I'd prefer that these code bits / hunks were simply moved
to the patches that make use of them.)

> 
> Tested on Ubuntu x64, native/remote/extended-remote, and as part of
> subsequent patches in the series.
> 
> Thanks,
> --Don
> 
> gdb/gdbserver/
> 2015-01-25  Don Breazeal  <donb@codesourcery.com>
> 
> 	* linux-low.c (linux_supports_fork_events): New function.
> 	(linux_supports_vfork_events): New function.
> 	(linux_target_ops): Initialize new structure members.
> 	(initialize_low): Call linux_ptrace_set_additional_flags
> 	and linux_test_for_event_reporting.
> 	* lynx-low.c (lynx_target_ops): Initialize new structure
> 	members.
> 	* server.c (report_fork_events, report_vfork_events):
> 	New global flags.
> 	(handle_query): Add new features to qSupported packet.
> 	(captured_main): Initialize new global variables.
> 	* target.h (struct target_ops) <supports_fork_events>:
> 	New member.
> 	<supports_vfork_events>: New member.
> 	(target_supports_fork_events): New macro.
> 	(target_supports_vfork_events): New macro.
> 	* win32-low.c (win32_target_ops): Initialize new structure
> 	members.
> 
> gdb/
> 2015-01-25  Don Breazeal  <donb@codesourcery.com>
> 
> 	* nat/linux-ptrace.c (linux_test_for_event_reporting): Rename
> 	from linux_check_ptrace_features and make it extern.
> 	(linux_test_for_tracefork): Reformat code.
> 	(linux_enable_event_reporting): Change name of called function
> 	to linux_check_ptrace_features.
> 	(ptrace_supports_feature): Call linux_test_for_event_reporting
> 	instead of linux_check_ptrace_features.
> 	* nat/linux-ptrace.h: Declare linux_test_for_event_reporting.
> 	* remote.c (anonymous enum) <PACKET_fork_event_feature,
> 	PACKET_vfork_event_feature>: New enumeration constants.
> 	(remote_fork_event_p): New function.
> 	(remote_vfork_event_p): New function.
> 	(remote_query_supported): Add new feature queries to qSupported
> 	packet.
> 	(remote_protocol_features): Add table entries for new packets.
> 	(_initialize_remote): Exempt new packets from the requirement
> 	to have 'set remote' commands.
> 
> ---
>  gdb/gdbserver/linux-low.c |   22 ++++++++++++++++++++++
>  gdb/gdbserver/lynx-low.c  |    2 ++
>  gdb/gdbserver/server.c    |   22 ++++++++++++++++++++++
>  gdb/gdbserver/target.h    |   14 ++++++++++++++
>  gdb/gdbserver/win32-low.c |    2 ++
>  gdb/nat/linux-ptrace.c    |   25 +++++++++++++------------
>  gdb/nat/linux-ptrace.h    |    1 +
>  gdb/remote.c              |   41 +++++++++++++++++++++++++++++++++++++++--
>  8 files changed, 115 insertions(+), 14 deletions(-)
> 
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index 5e37dd5..e31844c 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -5123,6 +5123,22 @@ linux_supports_multi_process (void)
>    return 1;
>  }
>  
> +/* Check if fork events are supported.  */
> +
> +static int
> +linux_supports_fork_events (void)
> +{
> +  return linux_supports_tracefork ();
> +}
> +
> +/* Check if vfork events are supported.  */
> +
> +static int
> +linux_supports_vfork_events (void)
> +{
> +  return linux_supports_tracefork ();
> +}
> +
>  static int
>  linux_supports_disable_randomization (void)
>  {
> @@ -6034,6 +6050,8 @@ static struct target_ops linux_target_ops = {
>    linux_async,
>    linux_start_non_stop,
>    linux_supports_multi_process,
> +  linux_supports_fork_events,
> +  linux_supports_vfork_events,
>  #ifdef USE_THREAD_DB
>    thread_db_handle_monitor_command,
>  #else
> @@ -6108,4 +6126,8 @@ initialize_low (void)
>    sigaction (SIGCHLD, &sigchld_action, NULL);
>  
>    initialize_low_arch ();
> +
> +  /* Enable any extended ptrace events that are supported.  */
> +  linux_ptrace_set_additional_flags (0);

Likewise, seems pointless add this call now.

> +  linux_test_for_event_reporting ();
>  }
> diff --git a/gdb/gdbserver/lynx-low.c b/gdb/gdbserver/lynx-low.c
> index 98797ba..1126103 100644
> --- a/gdb/gdbserver/lynx-low.c
> +++ b/gdb/gdbserver/lynx-low.c
> @@ -754,6 +754,8 @@ static struct target_ops lynx_target_ops = {
>    NULL,  /* async */
>    NULL,  /* start_non_stop */
>    NULL,  /* supports_multi_process */
> +  NULL,  /* supports_fork_events */
> +  NULL,  /* supports_vfork_events */
>    NULL,  /* handle_monitor_command */
>  };
>  
> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index 0e72cf1..48cc363 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -57,6 +57,8 @@ static int exit_requested;
>  int run_once;
>  
>  int multi_process;
> +int report_fork_events;
> +int report_vfork_events;
>  int non_stop;
>  
>  /* Whether we should attempt to disable the operating system's address
> @@ -1841,6 +1843,18 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
>  		  /* GDB supports relocate instruction requests.  */
>  		  gdb_supports_qRelocInsn = 1;
>  		}
> +	      if (strcmp (p, "fork-events+") == 0)
> +		{
> +		  /* GDB supports and wants fork events if possible.  */
> +		  if (target_supports_fork_events ())
> +		    report_fork_events = 1;
> +		}
> +	      if (strcmp (p, "vfork-events+") == 0)
> +		{
> +		  /* GDB supports and wants vfork events if possible.  */
> +		  if (target_supports_vfork_events ())
> +		    report_vfork_events = 1;
> +		}
>  	      else
>  		target_process_qsupported (p);
>  
> @@ -1891,6 +1905,12 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
>        if (target_supports_multi_process ())
>  	strcat (own_buf, ";multiprocess+");
>  
> +      if (target_supports_fork_events ())
> +	strcat (own_buf, ";fork-events+");
> +
> +      if (target_supports_vfork_events ())
> +	strcat (own_buf, ";vfork-events+");
> +
>        if (target_supports_non_stop ())
>  	strcat (own_buf, ";QNonStop+");
>  
> @@ -3242,6 +3262,8 @@ captured_main (int argc, char *argv[])
>  
>        noack_mode = 0;
>        multi_process = 0;
> +      report_fork_events = 0;
> +      report_vfork_events = 0;
>        /* Be sure we're out of tfind mode.  */
>        current_traceframe = -1;
>        cont_thread = null_ptid;
> diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
> index bbb0567..8af76d9 100644
> --- a/gdb/gdbserver/target.h
> +++ b/gdb/gdbserver/target.h
> @@ -262,6 +262,12 @@ struct target_ops
>    /* Returns true if the target supports multi-process debugging.  */
>    int (*supports_multi_process) (void);
>  
> +  /* Returns true if fork events are supported.  */
> +  int (*supports_fork_events) (void);
> +
> +  /* Returns true if vfork events are supported.  */
> +  int (*supports_vfork_events) (void);
> +
>    /* If not NULL, target-specific routine to process monitor command.
>       Returns 1 if handled, or 0 to perform default processing.  */
>    int (*handle_monitor_command) (char *);
> @@ -387,6 +393,14 @@ void set_target_ops (struct target_ops *);
>  
>  int kill_inferior (int);
>  
> +#define target_supports_fork_events() \
> +  (the_target->supports_fork_events ? \
> +   (*the_target->supports_fork_events) () : 0)
> +
> +#define target_supports_vfork_events() \
> +  (the_target->supports_vfork_events ? \
> +   (*the_target->supports_vfork_events) () : 0)
> +
>  #define detach_inferior(pid) \
>    (*the_target->detach) (pid)
>  
> diff --git a/gdb/gdbserver/win32-low.c b/gdb/gdbserver/win32-low.c
> index e3fb618..210a747 100644
> --- a/gdb/gdbserver/win32-low.c
> +++ b/gdb/gdbserver/win32-low.c
> @@ -1823,6 +1823,8 @@ static struct target_ops win32_target_ops = {
>    NULL, /* async */
>    NULL, /* start_non_stop */
>    NULL, /* supports_multi_process */
> +  NULL, /* supports_fork_events */
> +  NULL, /* supports_vfork_events */
>    NULL, /* handle_monitor_command */
>    NULL, /* core_of_thread */
>    NULL, /* read_loadmap */
> diff --git a/gdb/nat/linux-ptrace.c b/gdb/nat/linux-ptrace.c
> index 0ce258f..3e1fcd5 100644
> --- a/gdb/nat/linux-ptrace.c
> +++ b/gdb/nat/linux-ptrace.c
> @@ -299,7 +299,7 @@ linux_fork_to_function (gdb_byte *child_stack, void (*function) (gdb_byte *))
>    return child_pid;
>  }
>  
> -/* A helper function for linux_check_ptrace_features, called after
> +/* A helper function for linux_test_for_event_reporting, called after
>     the child forks a grandchild.  */
>  
>  static void
> @@ -313,7 +313,7 @@ linux_grandchild_function (gdb_byte *child_stack)
>    _exit (0);
>  }
>  
> -/* A helper function for linux_check_ptrace_features, called after
> +/* A helper function for linux_test_for_event_reporting, called after
>     the parent process forks a child.  The child allows itself to
>     be traced by its parent.  */
>  
> @@ -337,8 +337,8 @@ static void linux_test_for_exitkill (int child_pid);
>  
>  /* Determine ptrace features available on this target.  */
>  
> -static void
> -linux_check_ptrace_features (void)
> +void
> +linux_test_for_event_reporting (void)
>  {
>    int child_pid, ret, status;
>  
> @@ -355,10 +355,10 @@ linux_check_ptrace_features (void)
>    if (ret == -1)
>      perror_with_name (("waitpid"));
>    else if (ret != child_pid)
> -    error (_("linux_check_ptrace_features: waitpid: unexpected result %d."),
> +    error (_("linux_test_for_event_reporting: waitpid: unexpected result %d."),
>  	   ret);
>    if (! WIFSTOPPED (status))
> -    error (_("linux_check_ptrace_features: waitpid: unexpected status %d."),
> +    error (_("linux_test_for_event_reporting: waitpid: unexpected status %d."),
>  	   status);
>  
>    linux_test_for_tracesysgood (child_pid);
> @@ -373,7 +373,7 @@ linux_check_ptrace_features (void)
>        ret = ptrace (PTRACE_KILL, child_pid, (PTRACE_TYPE_ARG3) 0,
>  		    (PTRACE_TYPE_ARG4) 0);
>        if (ret != 0)
> -	warning (_("linux_check_ptrace_features: failed to kill child"));
> +	warning (_("linux_test_for_event_reporting: failed to kill child"));
>        my_waitpid (child_pid, &status, 0);
>      }
>    while (WIFSTOPPED (status));
> @@ -459,9 +459,10 @@ 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));
> +	  current_ptrace_options |= (additional_flags
> +				     & (PTRACE_O_TRACEFORK
> +					| PTRACE_O_TRACEVFORK
> +					| PTRACE_O_TRACEEXEC));

This looks like a spurious change?

>  
>  	  /* Do some cleanup and kill the grandchild.  */
>  	  my_waitpid (second_pid, &second_status, 0);
> @@ -503,7 +504,7 @@ linux_enable_event_reporting (pid_t pid, int attached)
>    /* Check if we have initialized the ptrace features for this
>       target.  If not, do it now.  */
>    if (current_ptrace_options == -1)
> -    linux_check_ptrace_features ();
> +    linux_test_for_event_reporting ();
>  
>    ptrace_options = current_ptrace_options;
>    if (attached)
> @@ -535,7 +536,7 @@ static int
>  ptrace_supports_feature (int ptrace_options)
>  {
>    if (current_ptrace_options == -1)
> -    linux_check_ptrace_features ();
> +    linux_test_for_event_reporting ();

I don't really understand the motivation for the renaming.
The function still tests all kinds of ptrace features, not just
event-related features. E.g., it tests PTRACE_O_EXITKILL.  And the
variable it controls is called "ptrace_options".  How about
leaving the function's name as is?

>  
>    return ((current_ptrace_options & ptrace_options) == ptrace_options);
>  }
> diff --git a/gdb/nat/linux-ptrace.h b/gdb/nat/linux-ptrace.h
> index 137b61a..d75b37c 100644
> --- a/gdb/nat/linux-ptrace.h
> +++ b/gdb/nat/linux-ptrace.h
> @@ -98,6 +98,7 @@ extern void linux_ptrace_attach_fail_reason (pid_t pid, struct buffer *buffer);
>  extern char *linux_ptrace_attach_fail_reason_string (ptid_t ptid, int err);
>  
>  extern void linux_ptrace_init_warnings (void);
> +extern void linux_test_for_event_reporting (void);
>  extern void linux_enable_event_reporting (pid_t pid, int attached);
>  extern void linux_disable_event_reporting (pid_t pid);
>  extern int linux_supports_tracefork (void);
> diff --git a/gdb/remote.c b/gdb/remote.c
> index c4d09ba..ec1f1d2 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -1327,6 +1327,12 @@ enum {
>    /* Support for qXfer:libraries-svr4:read with a non-empty annex.  */
>    PACKET_augmented_libraries_svr4_read_feature,
>  
> +  /* Support for fork events.  */
> +  PACKET_fork_event_feature,
> +
> +  /* Support for vfork events.  */
> +  PACKET_vfork_event_feature,
> +
>    PACKET_MAX
>  };
>  
> @@ -1432,6 +1438,24 @@ remote_multi_process_p (struct remote_state *rs)
>    return packet_support (PACKET_multiprocess_feature) == PACKET_ENABLE;
>  }
>  
> +#if PTRACE_FORK_EVENTS
> +/* Returns true if fork events are supported.  */
> +
> +static int
> +remote_fork_event_p (struct remote_state *rs)
> +{
> +  return packet_support (PACKET_fork_event_feature) == PACKET_ENABLE;
> +}
> +
> +/* Returns true if vfork events are supported.  */
> +
> +static int
> +remote_vfork_event_p (struct remote_state *rs)
> +{
> +  return packet_support (PACKET_vfork_event_feature) == PACKET_ENABLE;
> +}
> +#endif
> +
>  /* Tokens for use by the asynchronous signal handlers for SIGINT.  */
>  static struct async_signal_handler *async_sigint_remote_twice_token;
>  static struct async_signal_handler *async_sigint_remote_token;
> @@ -4010,7 +4034,11 @@ static const struct protocol_feature remote_protocol_features[] = {
>    { "Qbtrace:off", PACKET_DISABLE, remote_supported_packet, PACKET_Qbtrace_off },
>    { "Qbtrace:bts", PACKET_DISABLE, remote_supported_packet, PACKET_Qbtrace_bts },
>    { "qXfer:btrace:read", PACKET_DISABLE, remote_supported_packet,
> -    PACKET_qXfer_btrace }
> +    PACKET_qXfer_btrace },
> +  { "fork-events", PACKET_DISABLE, remote_supported_packet,
> +    PACKET_fork_event_feature },
> +  { "vfork-events", PACKET_DISABLE, remote_supported_packet,
> +    PACKET_vfork_event_feature },
>  };
>  
>  static char *remote_support_xml;
> @@ -4084,6 +4112,12 @@ remote_query_supported (void)
>  
>        q = remote_query_supported_append (q, "qRelocInsn+");
>  
> +      if (rs->extended)
> +	{
> +	  q = remote_query_supported_append (q, "fork-events+");
> +	  q = remote_query_supported_append (q, "vfork-events+");
> +	}
> +
>        q = reconcat (q, "qSupported:", q, (char *) NULL);
>        putpkt (q);
>  
> @@ -12191,7 +12225,8 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
>    add_packet_config_cmd (&remote_protocol_packets[PACKET_qXfer_btrace],
>         "qXfer:btrace", "read-btrace", 0);
>  
> -  /* Assert that we've registered commands for all packet configs.  */
> +  /* Assert that we've registered "set remote foo-packet" commands
> +     for all packet configs.  */
>    {
>      int i;
>  
> @@ -12210,6 +12245,8 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
>  	  case PACKET_DisconnectedTracing_feature:
>  	  case PACKET_augmented_libraries_svr4_read_feature:
>  	  case PACKET_qCRC:
> +	  case PACKET_fork_event_feature:
> +	  case PACKET_vfork_event_feature:

So what's the justification for not adding the commands?  :-)

>  	    /* Additions to this list need to be well justified:
>  	       pre-existing packets are OK; new packets are not.  */
>  	    excepted = 1;

The patch should also include a GDB manual change describing the
new RSP features, and a NEWS change mentioning them.

Thanks,
Pedro Alves