Re: [PATCH 04/16 v3] Determine supported extended-remote features

["Breazeal, Don" <donb at codesourcery dot com>]

Ping.
The two patches from this series that are currently under review are:

https://sourceware.org/ml/gdb-patches/2015-01/msg00320.html
https://sourceware.org/ml/gdb-patches/2015-01/msg00321.html

The second of these depends on this:

https://sourceware.org/ml/gdb-patches/2014-10/msg00870.html

Also, I have made some progress on follow fork for target 'remote'.
I'll post that as a separate patch once I have it all working and
cleaned up.
thanks
--Don

On 1/12/2015 2:36 PM, Don Breazeal wrote:
>> As I mentioned, you'll need to make old GDB against new
>> GDBserver work correctly, that is, make gdbserver not send these new
>> events if debugging with old GDB, which is not expecting them.
>> That can be handled by making GDB send a "fork-events+" in its
>> own qSupported packet, so that GDBserver knows its talking to a
>> new GDB.  See how the "multiprocess+" feature is handled in both
>> GDB and GDBserver.  For a while, GDB didn't
>> broadcast "multiprocess+" when connecting with "target remote".
> 
> Hi Pedro,
> This patch implements fork-events+ and vfork-events+ in the 
> qSupported packet as you described.
> 
>> One of the reasons I wanted that on "remote" was exactly to
>> make it possible to have fork events without caring for
>> remote vs extended-remote.  
> 
> I share this goal.
> 
>>
>> If there are bigger problems, we can make GDB not broadcast
>> the support with plain remote for starters.  But I'd be curious
>> to hear what the problems are.
> 
> My previous response included two patches that "almost worked" for
> target remote.  Please disregard those.  As I went through the remaining
> issues, it became clear to me that there are enough underlying changes
> needed in the target remote multiprocess support that they deserve a
> separate patch.  I would like to proceed with the extended-remote support
> and address target remote issue in a subsequent patch.
> 
> The issues that I ran into with target remote and follow fork
> were mostly related to detach and kill in making decisions about
> when and where to mourn an inferior and keeping things in sync
> between GDB and gdbserver.  The issues aren't insurmountable, but
> I'm proposing that try to make progress by taking things in smaller
> bites: first the extended-remote support, then the underlying target
> remote multiprocess additions, then target remote follow fork.
> 
> Note that the remote vs. extended-remote issue is moot for this
> patch except in the construction of the qSupported packet.
> 
> Let me know what you think.
> Thanks,
> --Don
> 
> 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 for fork and vfork events.  Other
> events like vfork-done may be added later in the patch series if
> needed.
> 
> These packets are used just like PACKET_multiprocess_feature to denote
> whether the corresponding event is supported by inquiring about packet
> support in the qSupported packet, then enabling or disabling the packet
> based on the qSupported response.  Target functions used to query for
> support are included for 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 events are reported.  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 #ifdef that have been added temporarily to allow the code for
> managing the events to be included in this patch without enabling the
> events.  See PTRACE_EVENT_SUPPORT.  This #ifdef 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.
> 
> gdb/gdbserver/
> 2015-01-12  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,
> 	report_exec_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-12  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.
> 	* 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.c (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    |   13 +++++++------
>  gdb/nat/linux-ptrace.h    |    1 +
>  gdb/remote.c              |   41 +++++++++++++++++++++++++++++++++++++++--
>  8 files changed, 109 insertions(+), 8 deletions(-)
> 
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index 65f72a2..b1201b3 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -5129,6 +5129,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)
>  {
> @@ -6040,6 +6056,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
> @@ -6115,4 +6133,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);
> +  linux_test_for_event_reporting ();
>  }
> diff --git a/gdb/gdbserver/lynx-low.c b/gdb/gdbserver/lynx-low.c
> index 3b83669..e3fb788 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 173a22e..b092020 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 5e29b7f..e25e5f8 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 *);
> @@ -390,6 +396,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 e714933..a35ff91 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 a0e0c32..542e762 100644
> --- a/gdb/nat/linux-ptrace.c
> +++ b/gdb/nat/linux-ptrace.c
> @@ -311,8 +311,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;
>  
> @@ -433,9 +433,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));
>  
>  	  /* Do some cleanup and kill the grandchild.  */
>  	  my_waitpid (second_pid, &second_status, 0);
> @@ -477,7 +478,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)
> diff --git a/gdb/nat/linux-ptrace.h b/gdb/nat/linux-ptrace.h
> index 588d38a..edbacfd 100644
> --- a/gdb/nat/linux-ptrace.h
> +++ b/gdb/nat/linux-ptrace.h
> @@ -90,6 +90,7 @@ struct buffer;
>  
>  extern void linux_ptrace_attach_fail_reason (pid_t pid, struct buffer *buffer);
>  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 4b9b099..891329a 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:
>  	    /* Additions to this list need to be well justified:
>  	       pre-existing packets are OK; new packets are not.  */
>  	    excepted = 1;
>