[v4,1/7] Identify remote fork event support

  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.

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

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