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

["Breazeal, Don" <donb@codesourcery.com>]

On 11/13/2014 10:48 AM, Pedro Alves wrote:
> On 11/13/2014 06:37 PM, Breazeal, Don wrote:
> 
>> Given your comment on patch 6 saying that there isn't any
>> reason not to support follow fork in remote as well as
>> extended-remote, the problem I was solving goes away.
>>
>> The problem I thought I had was that if there was no
>> agreement between GDB and gdbserver on whether the
>> features were supported, then gdbserver would send
>> events to GDB that it wasn't expecting in remote mode,
>> and handling those looked problematic.
>>
>> Those bad assumptions get me every time.  This change
>> should make things simpler as long as I don't run into
>> troubles with "remote".
> 
> The only thing I'm imagining at the moment, is that
> in "remote" mode, gdbserver always disconnects after
> a "D" / detach packet, even if there are other processes
> to debug.  But that seems silly anyway and we should
> probably fix it.
> 
> 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".  See:
> 
>   https://sourceware.org/ml/gdb-patches/2012-01/msg00490.html
> 
> 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.  :-)
> 
> 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.
> 
> Thanks,
> Pedro Alves
> 

Hi Pedro,
I have revised this patch to use the qSupported packet as you described,
and to eliminate the vFollowFork packet.  There is a mechanism to
prevent reporting the events unless GDB sends e.g. "fork-events+".

Is this more in line with what you were thinking of?
thanks!
--Don

This patch implements a mechanism for GDB to determine whether fork
and exec events are supported in gdbserver.  This is a preparatory
patch for remote fork and exec event support.

Three new RSP packets are defined, one each for fork, vfork, and exec
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 are a couple of #ifdefs that have been added to allow the code for
managing the events to be included in this patch without enabling the
events.  See PTRACE_EVENT_SUPPORT.  These #ifdefrs will be removed later in
the patch series as the events are enabled.

Tested on Ubuntu x64, native only, and as part of subsequent patches in the
series.

gdb/gdbserver/
2014-12-05  Don Breazeal  <donb@codesourcery.com>

	* linux-low.c (linux_supports_fork_events): New function.
	(linux_supports_vfork_events): New function.
	(linux_supports_exec_events): New function.
	(linux_target_ops): Initialize new structure members.
	(initialize_low): Call linux_ptrace_set_additional_flags,
	disabled for now, and linux_test_for_event_reporting.
	* lynx-low.c: Initialize new structure members.
	* server.c (report_fork_events, report_vfork_events,
	report_exec_events): New global flags.
	(handle_query): Add new feature queries to handling of
	qSupported packet.
	(captured_main): Initialize new global variables.
	* target.h (struct target_ops) <supports_fork_events>:
	New member.
	<supports_vfork_events>: New member.
	<supports_exec_events>: New member.
	(target_supports_fork_events): New macro.
	(target_supports_vfork_events): New macro.
	(target_supports_exec_events): New macro.
	* win32-low.c: Initialize new structure members.

gdb/
2014-12-05  Don Breazeal  <donb@codesourcery.com>

	* nat/linux-ptrace.c (linux_test_for_event_reporting): Rename
	from linux_check_ptrace_features.
	(linux_enable_event_reporting): Remove call to
	linux_check_ptrace_features.
	* nat/linux-ptrace.h (linux_test_for_event_reporting): Declare.
	* remote.c (anonymous enum) <PACKET_fork_event_feature,
	PACKET_vfork_event_feature, PACKET_exec_event_feature>: New
	enumeration constants.
	(remote_fork_event_p): New function.
	(remote_vfork_event_p): New function.
	(remote_exec_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 | 37 +++++++++++++++++++++++++++++++++
 gdb/gdbserver/lynx-low.c  |  3 +++
 gdb/gdbserver/server.c    | 33 +++++++++++++++++++++++++++++
 gdb/gdbserver/target.h    | 21 +++++++++++++++++++
 gdb/gdbserver/win32-low.c |  3 +++
 gdb/nat/linux-ptrace.c    | 13 ++++++------
 gdb/nat/linux-ptrace.h    |  1 +
 gdb/remote.c              | 53
+++++++++++++++++++++++++++++++++++++++++++++--
 8 files changed, 156 insertions(+), 8 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 01f11b7..a13631a 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -5133,6 +5133,30 @@ 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 ();
+}
+
+/* Check if exec events are supported.  */
+
+static int
+linux_supports_exec_events (void)
+{
+  return linux_supports_tracefork ();
+}
+
 static int
 linux_supports_disable_randomization (void)
 {
@@ -6044,6 +6068,9 @@ 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,
+  linux_supports_exec_events,
 #ifdef USE_THREAD_DB
   thread_db_handle_monitor_command,
 #else
@@ -6119,4 +6146,14 @@ initialize_low (void)
   sigaction (SIGCHLD, &sigchld_action, NULL);

   initialize_low_arch ();
+
+#if PTRACE_EVENT_SUPPORT
+  /* Always try to enable ptrace event extensions.  Leave
+     PTRACE_O_TRACESYSGOOD out until it is supported.  */
+  linux_ptrace_set_additional_flags (PTRACE_O_TRACEVFORKDONE
+				     | PTRACE_O_TRACEVFORK
+				     | PTRACE_O_TRACEFORK
+				     | PTRACE_O_TRACEEXEC);
+#endif
+  linux_test_for_event_reporting ();
 }
diff --git a/gdb/gdbserver/lynx-low.c b/gdb/gdbserver/lynx-low.c
index 6178e03..dc4ffb2 100644
--- a/gdb/gdbserver/lynx-low.c
+++ b/gdb/gdbserver/lynx-low.c
@@ -753,6 +753,9 @@ 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,  /* supports_exec_events */
   NULL,  /* handle_monitor_command */
 };

diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 173a22e..71c954f 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -57,6 +57,9 @@ static int exit_requested;
 int run_once;

 int multi_process;
+int report_fork_events;
+int report_vfork_events;
+int report_exec_events;
 int non_stop;

 /* Whether we should attempt to disable the operating system's address
@@ -1841,6 +1844,24 @@ 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;
+		}
+	      if (strcmp (p, "exec-events+") == 0)
+		{
+		  /* GDB supports and wants fork events if possible.  */
+		  if (target_supports_exec_events ())
+		    report_exec_events = 1;
+		}
 	      else
 		target_process_qsupported (p);

@@ -1891,6 +1912,15 @@ 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_exec_events ())
+	strcat (own_buf, ";exec-events+");
+
       if (target_supports_non_stop ())
 	strcat (own_buf, ";QNonStop+");

@@ -3242,6 +3272,9 @@ captured_main (int argc, char *argv[])

       noack_mode = 0;
       multi_process = 0;
+      report_fork_events = 0;
+      report_vfork_events = 0;
+      report_exec_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..a427840 100644
--- a/gdb/gdbserver/target.h
+++ b/gdb/gdbserver/target.h
@@ -262,6 +262,15 @@ 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);
+
+  /* Returns true if exec events are supported.  */
+  int (*supports_exec_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 +399,18 @@ 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 target_supports_exec_events() \
+  (the_target->supports_exec_events ? \
+   (*the_target->supports_exec_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..bf95db4 100644
--- a/gdb/gdbserver/win32-low.c
+++ b/gdb/gdbserver/win32-low.c
@@ -1823,6 +1823,9 @@ 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, /* supports_exec_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 8bc3f16..2760ddd 100644
--- a/gdb/nat/linux-ptrace.c
+++ b/gdb/nat/linux-ptrace.c
@@ -310,8 +310,8 @@ static void linux_test_for_tracefork (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;

@@ -430,9 +430,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);
@@ -457,7 +458,7 @@ linux_enable_event_reporting (pid_t pid)
   /* 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 ();

   /* Set the options.  */
   ptrace (PTRACE_SETOPTIONS, pid, (PTRACE_TYPE_ARG3) 0,
diff --git a/gdb/nat/linux-ptrace.h b/gdb/nat/linux-ptrace.h
index 31a77cd..437907e 100644
--- a/gdb/nat/linux-ptrace.h
+++ b/gdb/nat/linux-ptrace.h
@@ -85,6 +85,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);
 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..cb40955 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1327,6 +1327,15 @@ 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,
+
+  /* Support for exec events.  */
+  PACKET_exec_event_feature,
+
   PACKET_MAX
 };

@@ -1432,6 +1441,32 @@ remote_multi_process_p (struct remote_state *rs)
   return packet_support (PACKET_multiprocess_feature) == PACKET_ENABLE;
 }

+#if PTRACE_EVENT_SUPPORT
+/* 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;
+}
+
+/* Returns true if exec events are supported.  */
+
+static int
+remote_exec_event_p (struct remote_state *rs)
+{
+  return packet_support (PACKET_exec_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 +4045,13 @@ 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 },
+  { "exec-events", PACKET_DISABLE, remote_supported_packet,
+    PACKET_exec_event_feature }
 };

 static char *remote_support_xml;
@@ -4084,6 +4125,10 @@ remote_query_supported (void)

       q = remote_query_supported_append (q, "qRelocInsn+");

+      q = remote_query_supported_append (q, "fork-events+");
+      q = remote_query_supported_append (q, "vfork-events+");
+      q = remote_query_supported_append (q, "exec-events+");
+
       q = reconcat (q, "qSupported:", q, (char *) NULL);
       putpkt (q);

@@ -12191,7 +12236,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 +12256,9 @@ 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:
+	  case PACKET_exec_event_feature:
 	    /* Additions to this list need to be well justified:
 	       pre-existing packets are OK; new packets are not.  */
 	    excepted = 1;
-- 
1.9.1