[PATCH 1/4] Query supported notifications by qSupported

[PATCH 1/4] Query supported notifications by qSupported

[Yao Qi]

V7: remove annex.
V6: the supported flag of each notification is saved in
'struct remote_notif_state' in GDB.  In GDBserver, the supported flag
is still associated with each notification.

As we we adding more notifications, both GDB and GDBserver
has to know what notifications are supported in the other
side.  This is what this patch does.  When GDB connects to GDBserver,
it will happen:

  --> qSupported:XXX;notifications=N1,N2.N3
      (GDB supports notification N1, N2 and N3)
  <-- XXX;Notifications=N1,N2,N4
      (GDBsever supports notification N1, N2 and N4)

after this, GDB knows what notifications GDBserver is able to send,
and GDBservers knows what notifications GDB doesn't support.

gdb/gdbserver:

	* notif.c (notif_qsupported_record): New function.
	(notif_qsupported_reply): New function.
	* notif.h (struct notif_server) <supported>: New field.
	(notif_qsupported_reply): Declare.
	(notif_qsupported_record): Declare.
	* server.c (notif_annex_stop): Update.
	(handle_query): Call notif_qsupported_record and
	notif_qsupported_reply.

gdb:

	* remote-notif.c (remote_notif_ack): Add argument 'state'.
	Callers update.
	(remote_notif_parse): Likewise.
	(remote_notif_qsupported): New function.
	(remote_notif_qsupported_reply): New function.
	(remote_notif_state_allocate): Initialize field 'supported'.
	(remote_notif_state_xfree): Free field 'supported'.
	* remote-notif.h (struct remote_notif_state) <supported>: New
	field.
	(remote_notif_ack, remote_notif_parse): Update declarations.
	(remote_notif_qsupported): Declare.
	(remote_notif_qsupported_reply): Declare.
	* remote.c (PACKET_notifications): New enum.
	(remote_notifications_feature): New function.
	(remote_protocol_features): Add new element.
	(remote_query_supported): Call remote_notif_qsupported and
	append supported notifications to qSupported feature.

gdb/doc:

	* gdb.texinfo (General Query Packets): Document the new feature
	'notifications' of 'qSupported' packet.
	Document the new feature in qSupported reply.
---
 gdb/doc/gdb.texinfo    |   18 ++++++++
 gdb/gdbserver/notif.c  |   83 ++++++++++++++++++++++++++++++++++++
 gdb/gdbserver/notif.h  |    5 ++
 gdb/gdbserver/server.c |   16 +++++++-
 gdb/remote-notif.c     |  109 ++++++++++++++++++++++++++++++++++++++++++++++-
 gdb/remote-notif.h     |   14 ++++++-
 gdb/remote.c           |   29 +++++++++++--
 7 files changed, 265 insertions(+), 9 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 7dfa9f6..e76f73b 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -39383,6 +39383,14 @@ description.
 This feature indicates whether @value{GDBN} supports the
 @samp{qRelocInsn} packet (@pxref{Tracepoint Packets,,Relocate
 instruction reply packet}).
+
+@item notifications
+@anchor{notifications feature}
+This feature indicates that @value{GDBN} supports the async remote
+notifications (@pxref{Notification Packets}).  If the stub sees
+@samp{notifications=} with a string of name of supported
+notifications, separated by commas, it will report notifications
+supported by the stub.
 @end table
 
 Stubs should ignore any unknown values for
@@ -39601,6 +39609,11 @@ These are the currently defined stub features and their properties:
 @tab @samp{-}
 @tab No
 
+@item @samp{Notifications}
+@tab Yes
+@tab @samp{-}
+@tab No
+
 @end multitable
 
 These are the currently defined stub features, in more detail:
@@ -39768,6 +39781,11 @@ See @ref{Bytecode Descriptions} for details about the bytecode.
 The remote stub supports running a breakpoint's command list itself,
 rather than reporting the hit to @value{GDBN}.
 
+@item Notifications=@var{name}@r{[},@var{name}@r{]}@dots{}
+@cindex notifications, in remote protocol
+The remote stub supports a string of notifications.  @var{name} is
+the name of the notification.
+
 @item Qbtrace:off
 The remote stub understands the @samp{Qbtrace:off} packet.
 
diff --git a/gdb/gdbserver/notif.c b/gdb/gdbserver/notif.c
index 6da2c5c..4766bba 100644
--- a/gdb/gdbserver/notif.c
+++ b/gdb/gdbserver/notif.c
@@ -159,6 +159,89 @@ notif_event_xfree (struct notif_event *event)
   xfree (event);
 }
 
+/* Record the notifications supported by GDB.  GDB_NOTIFICATIONS is a
+   string about notifications GDB supports.  */
+
+void
+notif_qsupported_record (char *gdb_notifications)
+{
+  const char *p = gdb_notifications;
+
+  while (1)
+    {
+      char *end = strchr (p, ',');
+      struct notif_server *nc = NULL;
+
+      if (end == NULL)
+	{
+	  int i;
+
+	  for (i = 0; i < ARRAY_SIZE (notifs); i++)
+	    {
+	      if (strcmp (p, notifs[i]->notif_name) == 0)
+		{
+		  nc = notifs[i];
+		  break;
+		}
+	    }
+	}
+      else
+	{
+	  int i;
+
+	  for (i = 0; i < ARRAY_SIZE (notifs); i++)
+	    {
+	      if (strncmp (p, notifs[i]->notif_name, end - p) == 0
+		  && strlen (notifs[i]->notif_name) == (end - p))
+		{
+		  nc = notifs[i];
+		  break;
+		}
+	    }
+
+	  p = end + 1;
+	}
+
+      /* Notification NC is supported.  */
+      if (nc != NULL)
+	nc->supported = 1;
+
+      if (end == NULL)
+	break;
+    }
+}
+
+/* Return a string about notifications that GDBserver supports.
+   Return NULL if no notification is supported.  The caller is
+   responsible to free the returned string.  Suppose GDBserver
+   supports notifications N1, N2, and N3.  The returned string is
+   "N1,N2,N3".  */
+
+char *
+notif_qsupported_reply (void)
+{
+  int i;
+  char * p = NULL;
+
+#define BUF_LEN 128
+
+  for (i = 0; i < ARRAY_SIZE (notifs); i++)
+    {
+      struct notif_server *nb = notifs[i];
+
+      if (p == NULL)
+	{
+	  p = xmalloc (BUF_LEN);
+	  strcpy (p, nb->notif_name);
+	}
+      else
+	xsnprintf (p + strlen (p), BUF_LEN - strlen (p), ",%s",
+		   nb->notif_name);
+    }
+
+  return p;
+}
+
 void
 initialize_notif (void)
 {
diff --git a/gdb/gdbserver/notif.h b/gdb/gdbserver/notif.h
index c714e7b..80e084e 100644
--- a/gdb/gdbserver/notif.h
+++ b/gdb/gdbserver/notif.h
@@ -52,12 +52,17 @@ typedef struct notif_server
 
   /* Write event EVENT to OWN_BUF.  */
   void (*write) (struct notif_event *event, char *own_buf);
+
+  /* This notification is supported by GDB or not.  */
+  int supported;
 } *notif_server_p;
 
 extern struct notif_server notif_stop;
 
 int handle_notif_ack (char *own_buf, int packet_len);
 void notif_write_event (struct notif_server *notif, char *own_buf);
+char* notif_qsupported_reply (void);
+void notif_qsupported_record (char *gdb_notifications);
 
 void notif_push (struct notif_server *np, struct notif_event *event);
 void notif_event_enque (struct notif_server *notif,
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index e0af785..d79c8db 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -187,7 +187,7 @@ vstop_notif_reply (struct notif_event *event, char *own_buf)
 
 struct notif_server notif_stop =
 {
-  "vStopped", "Stop", NULL, vstop_notif_reply,
+  "vStopped", "Stop", NULL, vstop_notif_reply, 1,
 };
 
 static int
@@ -1731,6 +1731,11 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
 		  /* GDB supports relocate instruction requests.  */
 		  gdb_supports_qRelocInsn = 1;
 		}
+	      else if (strncmp (p, "notifications=", 14) == 0)
+		{
+		  /* Record what notifications GDB supports.  */
+		  notif_qsupported_record (&p[14]);
+		}
 	      else
 		target_process_qsupported (p);
 
@@ -1820,6 +1825,15 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
 	  strcat (own_buf, ";qXfer:btrace:read+");
 	}
 
+      p = notif_qsupported_reply ();
+
+      if (p != NULL)
+	{
+	  strcat (own_buf, ";Notifications=");
+	  strcat (own_buf, p);
+	  xfree (p);
+	}
+
       return;
     }
 
diff --git a/gdb/remote-notif.c b/gdb/remote-notif.c
index 163979d..4685d30 100644
--- a/gdb/remote-notif.c
+++ b/gdb/remote-notif.c
@@ -59,7 +59,8 @@ static void do_notif_event_xfree (void *arg);
    acknowledge.  */
 
 void
-remote_notif_ack (struct notif_client *nc, char *buf)
+remote_notif_ack (struct notif_client *nc,
+		  struct remote_notif_state *state, char *buf)
 {
   struct notif_event *event = nc->alloc_event ();
   struct cleanup *old_chain
@@ -78,7 +79,8 @@ remote_notif_ack (struct notif_client *nc, char *buf)
 /* Parse the BUF for the expected notification NC.  */
 
 struct notif_event *
-remote_notif_parse (struct notif_client *nc, char *buf)
+remote_notif_parse (struct notif_client *nc,
+		    struct remote_notif_state *state, char *buf)
 {
   struct notif_event *event = nc->alloc_event ();
   struct cleanup *old_chain
@@ -158,7 +160,7 @@ handle_notification (struct remote_notif_state *state, char *buf)
   else
     {
       struct notif_event *event
-	= remote_notif_parse (nc, buf + strlen (nc->name) + 1);
+	= remote_notif_parse (nc, state, buf + strlen (nc->name) + 1);
 
       /* Be careful to only set it after parsing, since an error
 	 may be thrown then.  */
@@ -234,6 +236,99 @@ do_notif_event_xfree (void *arg)
   notif_event_xfree (arg);
 }
 
+/* Return a string about notifications that GDB supports.  The caller
+   is responsible to free the returned string.  Suppose GDB supports
+   notifications N1, N2, and N3.  The returned string is
+   "N1,N2,N3".  */
+
+char *
+remote_notif_qsupported (void)
+{
+  int i;
+  char * p = NULL;
+
+#define BUF_LEN 128
+
+  for (i = 0; i < ARRAY_SIZE (notifs); i++)
+    {
+      struct notif_client *nb = notifs[i];
+
+      if (p == NULL)
+	{
+	  p = xmalloc (BUF_LEN);
+	  strcpy (p, nb->name);
+	}
+      else
+	xsnprintf (p + strlen (p), BUF_LEN - strlen (p), ",%s",
+		   nb->name);
+    }
+
+  return p;
+}
+
+/* Parse the qSupported reply REPLY from the remote stub and enable
+   notifications if the remote stub supports.  */
+
+void
+remote_notif_qsupported_reply (const char *reply,
+			       struct remote_notif_state *state)
+{
+  const char *p = reply;
+
+  if (notif_debug)
+    fprintf_unfiltered (gdb_stdlog,
+			"notif: stub supported notifications '%s'\n",
+			reply);
+
+  while (1)
+    {
+      char *end = strchr (p, ',');
+      struct notif_client *nc = NULL;
+
+      if (end == NULL)
+	{
+	  int i;
+
+	  for (i = 0; i < ARRAY_SIZE (notifs); i++)
+	    {
+	      if (strcmp (p, notifs[i]->name) == 0)
+		{
+		  nc = notifs[i];
+		  break;
+		}
+	    }
+	}
+      else
+	{
+	  int i;
+
+	  for (i = 0; i < ARRAY_SIZE (notifs); i++)
+	    {
+	      if (strncmp (p, notifs[i]->name, end - p) == 0
+		  && strlen (notifs[i]->name) == (end - p))
+		{
+		  nc = notifs[i];
+		  break;
+		}
+	    }
+
+	  p = end + 1;
+	}
+
+      if (nc != NULL)
+	{
+	  /* Notification NC is supported.  */
+	  state->supported[nc->id] = 1;
+	  if (notif_debug)
+	    fprintf_unfiltered (gdb_stdlog, "notif: '%s' is supported\n",
+				nc->name);
+	}
+
+      if (end == NULL)
+	break;
+    }
+}
+
 /* Return an allocated remote_notif_state.  */
 
 struct remote_notif_state *
@@ -249,6 +344,12 @@ remote_notif_state_allocate (void)
     = create_async_event_handler (remote_async_get_pending_events_handler,
 				  notif_state);
 
+  notif_state->supported = xcalloc (ARRAY_SIZE (notifs), sizeof (int));
+
+  /* Even the remote stub doesn't understand
+    'qSupported:notifications=', it may still support notification
+     stop if it supports non-stop.  */
+  notif_state->supported[notif_client_stop.id] = 1;
   return notif_state;
 }
 
@@ -268,6 +369,8 @@ remote_notif_state_xfree (struct remote_notif_state *state)
   for (i = 0; i < REMOTE_NOTIF_LAST; i++)
     notif_event_xfree (state->pending_event[i]);
 
+  xfree (state->supported);
+
   xfree (state);
 }
 
diff --git a/gdb/remote-notif.h b/gdb/remote-notif.h
index e5ebbc7..14984b4 100644
--- a/gdb/remote-notif.h
+++ b/gdb/remote-notif.h
@@ -96,10 +96,17 @@ struct remote_notif_state
    remote.c:remote_notif_pending_replies).  */
 
   struct notif_event *pending_event[REMOTE_NOTIF_LAST];
+
+  /* Each element indicates the notification is supported by the
+     remote  stub or not.  The index is the field 'id' in
+     'struct notif_client'.  */
+  int *supported;
 };
 
-void remote_notif_ack (struct notif_client *nc, char *buf);
+void remote_notif_ack (struct notif_client *nc,
+		       struct remote_notif_state *state, char *buf);
 struct notif_event *remote_notif_parse (struct notif_client *nc,
+					struct remote_notif_state *state,
 					char *buf);
 
 void notif_event_xfree (struct notif_event *event);
@@ -112,6 +119,11 @@ void remote_notif_process (struct remote_notif_state *state,
 struct remote_notif_state *remote_notif_state_allocate (void);
 void remote_notif_state_xfree (struct remote_notif_state *state);
 
+
+char * remote_notif_qsupported (void);
+void remote_notif_qsupported_reply (const char *reply,
+				    struct remote_notif_state *state);
+
 extern struct notif_client notif_client_stop;
 
 extern int notif_debug;
diff --git a/gdb/remote.c b/gdb/remote.c
index 2ac8c36..82dce9c 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1367,6 +1367,7 @@ enum {
   PACKET_Qbtrace_off,
   PACKET_Qbtrace_bts,
   PACKET_qXfer_btrace,
+  PACKET_notifications,
   PACKET_MAX
 };
 
@@ -3569,7 +3570,7 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
 	  /* remote_notif_get_pending_replies acks this one, and gets
 	     the rest out.  */
 	  rs->notif_state->pending_event[notif_client_stop.id]
-	    = remote_notif_parse (notif, rs->buf);
+	    = remote_notif_parse (notif, rs->notif_state, rs->buf);
 	  remote_notif_get_pending_events (notif);
 
 	  /* Make sure that threads that were stopped remain
@@ -3989,6 +3990,17 @@ remote_augmented_libraries_svr4_read_feature
   rs->augmented_libraries_svr4_read = (support == PACKET_ENABLE);
 }
 
+static void
+remote_notifications_feature (const struct protocol_feature *feature,
+			      enum packet_support support,
+			      const char *value)
+{
+  struct remote_state *rs = get_remote_state ();
+
+  if (support == PACKET_ENABLE)
+    remote_notif_qsupported_reply (value, rs->notif_state);
+}
+
 static const struct protocol_feature remote_protocol_features[] = {
   { "PacketSize", PACKET_DISABLE, remote_packet_size, -1 },
   { "qXfer:auxv:read", PACKET_DISABLE, remote_supported_packet,
@@ -4063,7 +4075,9 @@ 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 },
+  { "Notifications", PACKET_DISABLE, remote_notifications_feature,
+    -1 },
 };
 
 static char *remote_support_xml;
@@ -4128,6 +4142,7 @@ remote_query_supported (void)
   if (remote_protocol_packets[PACKET_qSupported].support != PACKET_DISABLE)
     {
       char *q = NULL;
+      char *notifications = remote_notif_qsupported ();
       struct cleanup *old_chain = make_cleanup (free_current_contents, &q);
 
       q = remote_query_supported_append (q, "multiprocess+");
@@ -4137,6 +4152,10 @@ remote_query_supported (void)
 
       q = remote_query_supported_append (q, "qRelocInsn+");
 
+      q = reconcat (q, q, ";notifications=", notifications,
+		    (char *) NULL);
+      xfree (notifications);
+
       q = reconcat (q, "qSupported:", q, (char *) NULL);
       putpkt (q);
 
@@ -4592,7 +4611,8 @@ extended_remote_attach_1 (struct target_ops *target, char *args, int from_tty)
       if (target_can_async_p ())
 	{
 	  struct notif_event *reply
-	    =  remote_notif_parse (&notif_client_stop, wait_status);
+	    =  remote_notif_parse (&notif_client_stop, rs->notif_state,
+				   wait_status);
 
 	  push_stop_reply ((struct stop_reply *) reply);
 
@@ -5854,7 +5874,7 @@ remote_notif_get_pending_events (struct notif_client *nc)
 	  if (strcmp (rs->buf, "OK") == 0)
 	    break;
 	  else
-	    remote_notif_ack (nc, rs->buf);
+	    remote_notif_ack (nc, rs->notif_state, rs->buf);
 	}
     }
   else
@@ -6058,6 +6078,7 @@ remote_wait_as (ptid_t ptid, struct target_waitstatus *status, int options)
       {
 	struct stop_reply *stop_reply
 	  = (struct stop_reply *) remote_notif_parse (&notif_client_stop,
+						      rs->notif_state,
 						      rs->buf);
 
 	event_ptid = process_stop_reply (stop_reply, status);
-- 
1.7.7.6

[PATCH 4/4] MI notification on trace stop: triggered by remote

[Yao Qi]

As a result of previous patch, GDB has a Trace remote notification.
In this patch, GDB starts to use Trace notification, and emits MI
notification '=trace-stopped' to front-end.  A test case is
added to see if MI trace-stopped notification is emitted when trace
buffer is full.

gdb:

	* remote-notif-trace.c: Include "observer.h".
	(remote_notif_trace_status_parse): Call
	observer_notify_trace_changed.

gdb/testsuite:

	* gdb.trace/mi-trace-changed.exp (test_trace_buffer_full): New.
---
 gdb/remote-notif-trace.c                     |   12 ++++
 gdb/testsuite/gdb.trace/mi-trace-changed.exp |   75 ++++++++++++++++++++++++++
 2 files changed, 87 insertions(+), 0 deletions(-)

diff --git a/gdb/remote-notif-trace.c b/gdb/remote-notif-trace.c
index ec7f46c..a409753 100644
--- a/gdb/remote-notif-trace.c
+++ b/gdb/remote-notif-trace.c
@@ -22,6 +22,7 @@
 #include "remote.h"
 #include "tracepoint.h"
 #include "remote-notif.h"
+#include "observer.h"
 
 static void
 remote_notif_trace_parse (struct notif_client *self, char *buf,
@@ -34,6 +35,17 @@ remote_notif_trace_parse (struct notif_client *self, char *buf,
       if (buf[7] != 'T')
 	error (_("Unknown trace status in trace notification."));
       parse_trace_status (buf + 7 + 1, ts);
+
+      /* When the tracing is stopped, there is no changes anymore in
+	 the trace, so the remote stub can't send another
+	 notification.  We don't have to worry about notifying
+	 'trace_changed' observer  with argument 1 twice.
+	 The remote stub can't request tracing start and the remote
+	 stub may send multiple trace notifications on various status
+	 changes, we don't notify 'trace_changed' observer with
+	 argument 0.  */
+      if (!ts->running)
+	observer_notify_trace_changed (0);
     }
   else
     error (_("Unknown trace notification."));
diff --git a/gdb/testsuite/gdb.trace/mi-trace-changed.exp b/gdb/testsuite/gdb.trace/mi-trace-changed.exp
index fbd6fe7..9d6cfff 100644
--- a/gdb/testsuite/gdb.trace/mi-trace-changed.exp
+++ b/gdb/testsuite/gdb.trace/mi-trace-changed.exp
@@ -71,4 +71,79 @@ proc test_normal_tstart_stop { } {
 
 test_normal_tstart_stop
 
+# Verify that MI notification '=trace-stopped' is emitted when trace
+# buffer is full.
+
+proc test_trace_buffer_full { } {
+    with_test_prefix "tracebuffer full" {
+	global mi_gdb_prompt
+
+	if [mi_gdb_start] {
+	    return
+	}
+	mi_run_to_main
+
+	mi_gdb_test "-break-insert -a func2" {.*\^done,bkpt=.*} \
+	    "insert tracepoint on func2"
+
+	send_gdb "actions\n"
+	gdb_expect {
+	    -re "End with" {
+	    }
+	}
+
+	send_gdb "collect buf\nend\n"
+	set test "define actions"
+	gdb_expect {
+	    -re ".*${mi_gdb_prompt}$" {
+		pass $test
+	    }
+	    timeout {
+		fail "$test (timeout)"
+	    }
+	}
+
+	# No =trace-started notification.
+	mi_gdb_test "-trace-start" "-trace-start\r\n=breakpoint-modified\[^\n\]+\r\n\\^done" \
+	    "start trace without notification"
+	mi_gdb_test "-break-insert end" {.*\^done,bkpt=.*} \
+	    "insert breakpoint on end"
+
+	mi_send_resuming_command "exec-continue" \
+	    "continuing execution to end"
+
+	set test "trace-stopped triggered by bufferfull"
+	gdb_expect {
+	    # We don't set stop-notes.
+	    -re "=trace-stopped\\\\n" {
+		pass "$test"
+	    }
+	    timeout {
+		fail "$test (timeout)"
+	    }
+	}
+
+	global async
+	# In sync mode, eat all the output.  Don't have to do so in
+	# async mode.
+	if {!$async} {
+	    gdb_expect {
+		-re ".*${mi_gdb_prompt}$" {
+		}
+	    }
+	}
+	# GDB has got the rsp notifcation from remote stub that trace
+	# is stopped.
+	mi_gdb_test "tstop" ".*Trace is not running.*" \
+	    "tstop on stopped"
+
+	mi_gdb_test "-trace-status" ".*\\^done.*stop-reason=\"overflow\".*" \
+	    "trace-status"
+
+	mi_gdb_exit
+    }
+}
+
+test_trace_buffer_full
+
 return 0
-- 
1.7.7.6

[PATCH 2/4] async remote notification 'Trace'.

[Yao Qi]

Hi,
This patch adds a new async remote notification 'Trace' to report the
trace-status-related changes in the stub.  So far, it is only used for
reporting 'tracing status change', like this,

%Trace:status:T0;tstop::0;tframes:0;tcreated:0;tfree:500000;tsize:500000;circular:0;disconn:0;starttime:001355728912543287;stoptime:001355728912543768;username::;notes:666f6f:\n

gdb/gdbserver:

	* notif.c (notifs): Add "notif_trace".
	* notif.h (ontif_trace): Declare.
	* tracepoint.c [!IN_PROCESS_AGENT]: Include "notif.h".
	[!IN_PROCESS_AGENT] (notif_reply_trace): New function.
	[!IN_PROCESS_AGENT] (notif_trace): New variable.
	(stop_tracing): Call notif_push.
gdb:

	* Makefile.in (REMOTE_OBS): Append remote-notif-trace.o
	(SFILES): Add remote-notif-trace.c
	* remote-notif.c (notifs): Add "notif_client_trace".
	* remote-notif.h (notif_client_trace): Declare.
	(REMOTE_NOTIF_TRACE): New enum
	* remote-notif-trace.c: New.

gdb/doc:

	* gdb.texinfo (Packets): Add "vTraced".
	(Notification Packets): Add doc about "Trace" notification
	and "vTraced" packet.
---
 gdb/Makefile.in            |    5 ++-
 gdb/doc/gdb.texinfo        |    9 +++++
 gdb/gdbserver/notif.c      |    1 +
 gdb/gdbserver/notif.h      |    1 +
 gdb/gdbserver/tracepoint.c |   27 +++++++++++++++
 gdb/remote-notif-trace.c   |   76 ++++++++++++++++++++++++++++++++++++++++++++
 gdb/remote-notif.c         |    1 +
 gdb/remote-notif.h         |    2 +
 8 files changed, 120 insertions(+), 2 deletions(-)
 create mode 100644 gdb/remote-notif-trace.c

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index be30dfd..2829aba 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -526,7 +526,7 @@ SER_HARDWIRE = @SER_HARDWIRE@
 # The `remote' debugging target is supported for most architectures,
 # but not all (e.g. 960)
 REMOTE_OBS = remote.o dcache.o tracepoint.o ax-general.o ax-gdb.o remote-fileio.o \
-	remote-notif.o ctf.o
+	remote-notif.o ctf.o remote-notif-trace.o
 
 # This is remote-sim.o if a simulator is to be linked in.
 SIM_OBS = @SIM_OBS@
@@ -756,7 +756,8 @@ SFILES = ada-exp.y ada-lang.c ada-typeprint.c ada-valprint.c ada-tasks.c \
 	p-exp.y p-lang.c p-typeprint.c p-valprint.c parse.c printcmd.c \
 	proc-service.list progspace.c \
 	prologue-value.c psymtab.c \
-	regcache.c reggroups.c remote.c remote-fileio.c remote-notif.c reverse.c \
+	regcache.c reggroups.c remote.c remote-fileio.c remote-notif.c \
+	remote-notif-trace.c reverse.c \
 	sentinel-frame.c \
 	serial.c ser-base.c ser-unix.c skip.c \
 	solib.c solib-target.c source.c \
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index e76f73b..e91c01e 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -38550,6 +38550,8 @@ for success (@pxref{Stop Reply Packets})
 
 @item vStopped
 @cindex @samp{vStopped} packet
+@itemx vTraced
+@cindex @samp{vTraced} packet
 @xref{Notification Packets}.
 
 @item X @var{addr},@var{length}:@var{XX@dots{}}
@@ -40570,6 +40572,7 @@ continue the tracing run, while 0 tells the target to stop tracing if
 @value{GDBN} is no longer in the picture.
 
 @item qTStatus
+@anchor{qTStatus packet}
 @cindex @samp{qTStatus} packet
 Ask the stub if there is a trace experiment running right now.
 
@@ -41100,6 +41103,12 @@ for information on how these notifications are acknowledged by
 @value{GDBN}.
 @tab Report an asynchronous stop event in non-stop mode.
 
+@item Trace
+@tab vTraced
+@tab @var{status}.  The @var{reply} has the form of the reply to
+packet @samp{qTStatus}, as described in @ref{qTStatus packet}.
+@tab Report an asynchronous event related to trace status.
+
 @end multitable
 
 @node Remote Non-Stop
diff --git a/gdb/gdbserver/notif.c b/gdb/gdbserver/notif.c
index 4766bba..123581d 100644
--- a/gdb/gdbserver/notif.c
+++ b/gdb/gdbserver/notif.c
@@ -52,6 +52,7 @@
 static struct notif_server *notifs[] =
 {
   &notif_stop,
+  &notif_trace,
 };
 
 /* Write another event or an OK, if there are no more left, to
diff --git a/gdb/gdbserver/notif.h b/gdb/gdbserver/notif.h
index 80e084e..c1abae0 100644
--- a/gdb/gdbserver/notif.h
+++ b/gdb/gdbserver/notif.h
@@ -58,6 +58,7 @@ typedef struct notif_server
 } *notif_server_p;
 
 extern struct notif_server notif_stop;
+extern struct notif_server notif_trace;
 
 int handle_notif_ack (char *own_buf, int packet_len);
 void notif_write_event (struct notif_server *notif, char *own_buf);
diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index ea1a8a1..c47dd88 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -3381,6 +3381,25 @@ cmd_qtstart (char *packet)
   write_ok (packet);
 }
 
+#ifndef IN_PROCESS_AGENT
+#include "notif.h"
+
+static void cmd_qtstatus (char *packet);
+
+static void
+notif_reply_trace (struct notif_event *event, char *own_buf)
+{
+  sprintf (own_buf, "status:");
+  cmd_qtstatus (own_buf + 7);
+}
+
+struct notif_server notif_trace =
+{
+  "vTraced", "Trace", NULL, notif_reply_trace, 0,
+};
+
+#endif
+
 /* End a tracing run, filling in a stop reason to report back to GDB,
    and removing the tracepoints from the code.  */
 
@@ -3481,6 +3500,14 @@ stop_tracing (void)
     }
 
   unpause_all (1);
+
+  if (notif_trace.supported)
+    {
+      struct notif_event *event
+	= xmalloc (sizeof (struct notif_event));
+
+      notif_push (&notif_trace, event);
+    }
 }
 
 static int
diff --git a/gdb/remote-notif-trace.c b/gdb/remote-notif-trace.c
new file mode 100644
index 0000000..ec7f46c
--- /dev/null
+++ b/gdb/remote-notif-trace.c
@@ -0,0 +1,76 @@
+/* Async remote notification on trace.
+
+   Copyright (C) 2013 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "defs.h"
+#include <string.h>
+#include "remote.h"
+#include "tracepoint.h"
+#include "remote-notif.h"
+
+static void
+remote_notif_trace_parse (struct notif_client *self, char *buf,
+			  struct notif_event *event)
+{
+  if (strncmp (buf, "status:", 7) == 0)
+    {
+      struct trace_status *ts = current_trace_status ();
+
+      if (buf[7] != 'T')
+	error (_("Unknown trace status in trace notification."));
+      parse_trace_status (buf + 7 + 1, ts);
+    }
+  else
+    error (_("Unknown trace notification."));
+}
+
+static void
+remote_notif_trace_ack (struct notif_client *self, char *buf,
+			struct notif_event *event)
+{
+  /* acknowledge */
+  putpkt ((char *) self->ack_command);
+}
+
+static int
+remote_notif_trace_can_get_pending_events (struct notif_client *self)
+{
+  return 1;
+}
+
+static struct notif_event *
+remote_notif_trace_alloc_event (void)
+{
+  struct notif_event *event = xmalloc (sizeof (struct notif_event));
+
+  event->dtr = NULL;
+
+  return event;
+}
+
+/* A client of notification 'Trace'.  */
+
+struct notif_client notif_client_trace =
+{
+  "Trace", "vTraced",
+  remote_notif_trace_parse,
+  remote_notif_trace_ack,
+  remote_notif_trace_can_get_pending_events,
+  remote_notif_trace_alloc_event,
+  REMOTE_NOTIF_TRACE,
+};
diff --git a/gdb/remote-notif.c b/gdb/remote-notif.c
index 4685d30..86a709a 100644
--- a/gdb/remote-notif.c
+++ b/gdb/remote-notif.c
@@ -49,6 +49,7 @@ int notif_debug = 0;
 static struct notif_client *notifs[] =
 {
   &notif_client_stop,
+  &notif_client_trace,
 };
 
 gdb_static_assert (ARRAY_SIZE (notifs) == REMOTE_NOTIF_LAST);
diff --git a/gdb/remote-notif.h b/gdb/remote-notif.h
index 14984b4..1a9e81c 100644
--- a/gdb/remote-notif.h
+++ b/gdb/remote-notif.h
@@ -36,6 +36,7 @@ struct notif_event
 enum REMOTE_NOTIF_ID
 {
   REMOTE_NOTIF_STOP = 0,
+  REMOTE_NOTIF_TRACE,
   REMOTE_NOTIF_LAST,
 };
 
@@ -125,6 +126,7 @@ void remote_notif_qsupported_reply (const char *reply,
 				    struct remote_notif_state *state);
 
 extern struct notif_client notif_client_stop;
+extern struct notif_client notif_client_trace;
 
 extern int notif_debug;
 
-- 
1.7.7.6

[PATCH 0/4 V7] MI notification on trace started/stopped

[Yao Qi]

Hi,
I examined all previous versions of this series and corresponding
reviews.  I find that V2 is the closet one to acceptance.  After
v2, I pursued "notification annex" and supported query on it,
which is a blind alley. (I don't recall why I choose this way then).

I update my patches to follow V2 and address comments I got during
the review.  The major comment raised by Pedro is that "GDB and
the remote negotiate the set of supported notifications".

  https://sourceware.org/ml/gdb-patches/2013-01/msg00245.html

Patch 1/4 does it.  Patch 2/4 is to add new notification on
trace.  Patch 3/4 and 4/4 are to use this notification and add
tests.

Patch series is regression tested on x86_64-linux with board file
{unix, native-gdbserver} x {sync, async}.

*** BLURB HERE ***

Yao Qi (4):
  Query supported notifications by qSupported
  async remote notification 'Trace'.
  MI notification on trace started/stopped:basic
  MI notification on trace stop: triggered by remote

 gdb/Makefile.in                              |    5 +-
 gdb/NEWS                                     |    2 +
 gdb/doc/gdb.texinfo                          |   31 ++++++
 gdb/doc/observer.texi                        |    6 +
 gdb/gdbserver/notif.c                        |   84 +++++++++++++++
 gdb/gdbserver/notif.h                        |    6 +
 gdb/gdbserver/server.c                       |   16 +++-
 gdb/gdbserver/tracepoint.c                   |   27 +++++
 gdb/mi/mi-cmds.c                             |    6 +-
 gdb/mi/mi-interp.c                           |   22 ++++
 gdb/mi/mi-main.h                             |    2 +
 gdb/remote-notif-trace.c                     |   88 +++++++++++++++
 gdb/remote-notif.c                           |  110 ++++++++++++++++++-
 gdb/remote-notif.h                           |   16 +++-
 gdb/remote.c                                 |   29 +++++-
 gdb/testsuite/gdb.trace/mi-trace-changed.exp |  149 ++++++++++++++++++++++++++
 gdb/tracepoint.c                             |    4 +
 17 files changed, 590 insertions(+), 13 deletions(-)
 create mode 100644 gdb/remote-notif-trace.c
 create mode 100644 gdb/testsuite/gdb.trace/mi-trace-changed.exp

-- 
1.7.7.6

[PATCH 3/4] MI notification on trace started/stopped:basic

[Yao Qi]

Hi,
This patch adds the notifications of 'trace-started' and
'trace-stopped', which are emitted when trace is started or stopped by
command 'tstart' and 'tstop', so that when trace is started or stopped
in console, MI frontend can be notified.

This patch doesn't handle the case the trace is stopped due to some
reasons in remote side, because we don't have any existing RSP
notification yet.  This issue will be addressed in another post.

Regression tested on x86_64-linux with native and gdbserver.  Is it
OK?

The documentation patch was approved by Eli here
<http://sourceware.org/ml/gdb-patches/2013-02/msg00441.html>

gdb/doc:

	* gdb.texinfo (GDB/MI Async Records): New MI notifications
	'trace-changed'.
	* observer.texi (GDB Observers): New observer 'trace-changed'

gdb:

	* mi/mi-cmds.c (mi_cmds): Adjust for commands 'trace-start'
	and 'trace-stop'.
	* mi/mi-interp.c: Declare mi_trace_changed.
	(mi_interpreter_init): Install mi_trace_changed to observer.
	(mi_trace_changed): New.
	* mi/mi-main.h (struct mi_suppress_notification) <trace>:
	New field.
	* tracepoint.c (start_tracing): Call
	observer_notify_trace_changed.
	(stop_tracing): Call observer_notify_trace_changed.

	* NEWS: Mention it.

gdb/testsuite/

	* gdb.mi/mi-trace-changed.exp: New.
---
 gdb/NEWS                                     |    2 +
 gdb/doc/gdb.texinfo                          |    4 ++
 gdb/doc/observer.texi                        |    6 ++
 gdb/mi/mi-cmds.c                             |    6 ++-
 gdb/mi/mi-interp.c                           |   22 ++++++++
 gdb/mi/mi-main.h                             |    2 +
 gdb/testsuite/gdb.trace/mi-trace-changed.exp |   74 ++++++++++++++++++++++++++
 gdb/tracepoint.c                             |    4 ++
 8 files changed, 118 insertions(+), 2 deletions(-)
 create mode 100644 gdb/testsuite/gdb.trace/mi-trace-changed.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index eff057f..0b9291d 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -174,6 +174,8 @@ show code-cache
 
   ** The -trace-save MI command can optionally save trace buffer in Common
      Trace Format now.
+  ** The start and stop of trace are now notified using new async records
+     "=trace-started" and "=trace-stopped".
 
   ** The new command -dprintf-insert sets a dynamic printf breakpoint.
 
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index e91c01e..0538b53 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -29638,6 +29638,10 @@ written in an inferior.  The @var{id} is the identifier of the
 thread group corresponding to the affected inferior.  The optional
 @code{type="code"} part is reported if the memory written to holds
 executable code.
+
+@item =trace-started
+@itemx =trace-stopped
+Reports that trace was started or stopped.
 @end table
 
 @node GDB/MI Breakpoint Information
diff --git a/gdb/doc/observer.texi b/gdb/doc/observer.texi
index f753965..450d732 100644
--- a/gdb/doc/observer.texi
+++ b/gdb/doc/observer.texi
@@ -253,6 +253,12 @@ The trace state variable @var{tsv} is deleted.  If @var{tsv} is
 The trace state value @var{tsv} is modified.
 @end deftypefun
 
+@deftypefun void trace_changed (int @var{started})
+The status of trace in @value{GDBN} has changed.  The trace is started
+if @var{started} is non-zero, and the trace is stopped if
+@var{started} is zero.
+@end deftypefun
+
 @deftypefun void test_notification (int @var{somearg})
 This observer is used for internal testing.  Do not use.  
 See testsuite/gdb.gdb/observer.exp.
diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index 58a8b89..eaf42ff 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -158,9 +158,11 @@ static struct mi_cmd mi_cmds[] =
 		 mi_cmd_trace_frame_collected),
   DEF_MI_CMD_MI ("trace-list-variables", mi_cmd_trace_list_variables),
   DEF_MI_CMD_MI ("trace-save", mi_cmd_trace_save),
-  DEF_MI_CMD_MI ("trace-start", mi_cmd_trace_start),
+  DEF_MI_CMD_MI_1 ("trace-start", mi_cmd_trace_start,
+		   &mi_suppress_notification.trace),
   DEF_MI_CMD_MI ("trace-status", mi_cmd_trace_status),
-  DEF_MI_CMD_MI ("trace-stop", mi_cmd_trace_stop),
+  DEF_MI_CMD_MI_1 ("trace-stop", mi_cmd_trace_stop,
+		   &mi_suppress_notification.trace),
   DEF_MI_CMD_MI ("var-assign", mi_cmd_var_assign),
   DEF_MI_CMD_MI ("var-create", mi_cmd_var_create),
   DEF_MI_CMD_MI ("var-delete", mi_cmd_var_delete),
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index 2ed1726..7a564d2 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -64,6 +64,7 @@ static void mi_inferior_appeared (struct inferior *inf);
 static void mi_inferior_exit (struct inferior *inf);
 static void mi_inferior_removed (struct inferior *inf);
 static void mi_on_resume (ptid_t ptid);
+static void mi_trace_changed (int started);
 static void mi_solib_loaded (struct so_list *solib);
 static void mi_solib_unloaded (struct so_list *solib);
 static void mi_about_to_proceed (void);
@@ -129,6 +130,7 @@ mi_interpreter_init (struct interp *interp, int top_level)
       observer_attach_record_changed (mi_record_changed);
       observer_attach_normal_stop (mi_on_normal_stop);
       observer_attach_target_resumed (mi_on_resume);
+      observer_attach_trace_changed (mi_trace_changed);
       observer_attach_solib_loaded (mi_solib_loaded);
       observer_attach_solib_unloaded (mi_solib_unloaded);
       observer_attach_about_to_proceed (mi_about_to_proceed);
@@ -582,6 +584,26 @@ mi_tsv_modified (const struct trace_state_variable *tsv)
   gdb_flush (mi->event_channel);
 }
 
+/* Emit notification on trace was started or stopped.  */
+
+static void
+mi_trace_changed (int started)
+{
+  struct mi_interp *mi = top_level_interpreter_data ();
+
+  if (mi_suppress_notification.trace)
+    return;
+
+  target_terminal_ours ();
+
+  if (started)
+    fprintf_unfiltered (mi->event_channel, "trace-started\n");
+  else
+    fprintf_unfiltered (mi->event_channel, "trace-stopped\n");
+
+  gdb_flush (mi->event_channel);
+}
+
 /* Emit notification about a created breakpoint.  */
 
 static void
diff --git a/gdb/mi/mi-main.h b/gdb/mi/mi-main.h
index d75526a..0903d60 100644
--- a/gdb/mi/mi-main.h
+++ b/gdb/mi/mi-main.h
@@ -43,6 +43,8 @@ struct mi_suppress_notification
   int traceframe;
   /* Memory changed notification suppressed?  */
   int memory;
+  /* Trace started/stopped notification suppressed?  */
+  int trace;
 };
 extern struct mi_suppress_notification mi_suppress_notification;
 
diff --git a/gdb/testsuite/gdb.trace/mi-trace-changed.exp b/gdb/testsuite/gdb.trace/mi-trace-changed.exp
new file mode 100644
index 0000000..fbd6fe7
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/mi-trace-changed.exp
@@ -0,0 +1,74 @@
+# Copyright 2013 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+load_lib mi-support.exp
+load_lib trace-support.exp
+
+standard_testfile status-stop.c
+
+if { [gdb_compile "$srcdir/$subdir/$srcfile" $binfile \
+	  executable {debug nowarnings}] != "" } {
+    untested mi-record-changed.exp
+    return -1
+}
+
+clean_restart $testfile
+
+if ![runto_main] {
+    fail "Can't run to main to check for trace support"
+    return -1
+}
+
+if ![gdb_target_supports_trace] {
+    unsupported "target does not support trace"
+    return -1;
+}
+
+gdb_exit
+
+# Verify that MI notification '=trace-started' and '=trace-stopped' are
+# emitted for normal 'tstart' and 'tstart' command.
+
+proc test_normal_tstart_stop { } {
+    with_test_prefix "tstart_tstop" {
+	global decimal hex
+
+	if [mi_gdb_start] {
+	    return
+	}
+	mi_run_to_main
+
+	mi_gdb_test "-break-insert -a main" {.*\^done,bkpt=.*} \
+	    "insert tracepoint on main"
+
+	# No =trace-started notification.
+	mi_gdb_test "-trace-start" "-trace-start\r\n=breakpoint-modified\[^\n\]+\r\n\\^done" \
+	    "start trace without notification"
+	mi_gdb_test "-trace-stop" \
+	    "-trace-stop\r\n\\^done,stop-reason=\"request\".*" \
+	    "stop trace without notification"
+
+	mi_gdb_test "tstart" \
+	    ".*=trace-started.*\\^done" "start trace notification"
+	mi_gdb_test "tstop" ".*=trace-stopped\\\\n\r\n\\^done" \
+	    "stop trace notification"
+
+	mi_gdb_exit
+    }
+}
+
+test_normal_tstart_stop
+
+return 0
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index f30282b..bf48038 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -1898,6 +1898,8 @@ start_tracing (char *notes)
   /* Now insert traps and begin collecting data.  */
   target_trace_start ();
 
+  observer_notify_trace_changed (1);
+
   /* Reset our local state.  */
   trace_reset_local_state ();
   current_trace_status()->running = 1;
@@ -1980,6 +1982,8 @@ stop_tracing (char *note)
 
   /* Should change in response to reply?  */
   current_trace_status ()->running = 0;
+
+  observer_notify_trace_changed (0);
 }
 
 /* tstatus command */
-- 
1.7.7.6

Re: [PATCH 0/4 V7] MI notification on trace started/stopped

[Pedro Alves]

On 12/12/2013 01:49 PM, Yao Qi wrote:
> Hi,
> I examined all previous versions of this series and corresponding
> reviews.  I find that V2 is the closet one to acceptance.  After
> v2, I pursued "notification annex" and supported query on it,
> which is a blind alley. (I don't recall why I choose this way then).
> 
> I update my patches to follow V2 and address comments I got during
> the review.  The major comment raised by Pedro is that "GDB and
> the remote negotiate the set of supported notifications".

Also, I still think ordering issues between trace start/stop and
stop events needs to be considered.  Let me expand.

With trace events sent on a separate channel, we can get things
like this, with a single-threaded program being traced:

 #1 - tstart, resume program
 #2 - trace stops because buffer full
 #3 - gdbserver queues trace stop notification.
 #4 - breakpoint at "next_session" is hit.
 #5 - gdb happens to process breakpoint notification first.
 #6 - The "next_session" breakpoint has a breakpoint
      command that does: "tstop; tsave; tstart; continue"
      (program iterates, and another collection sample begins)
 #7 - GDB emits MI =trace-stopped then =trace-started in response
      to tstop/tstart above.
 #8 - gdb processes the pending trace stop notification, emits
      MI =trace-stopped to frontend.
 #9 - The frontend is confused, thinking the trace session is
      stopped, when it is in fact running.

Now, even if we sent trace stops down the %Stop
notification, that wouldn't happen.  GDB would always process
the trace stop notification before the breakpoint.

But, considering multi-threaded programs, a different thread
can stop the trace immediately after the breakpoint at
next_session hits and triggers a %Stop notification,
and so sending trace notifications using %Stop wouldn't
fix the frontend confusion in this case, as the stop would
still be processed before the trace stop.

What I'm just now thinking would fix it would be if the
remote _also_ triggered trace _start_ notifications in
addition to trace stops:

 #1 - tstart, resume program
 #2 - trace stops because buffer full
 #3 - gdbserver queues trace stop notification.
 #4 - breakpoint at "next_session" is hit.
 #5 - gdb happens to process breakpoint notification first.
 #6 - The "next_session" breakpoint has a breakpoint
      command that does: "tstop; tsave; tstart; continue"
      (program iterates, and another collection sample begins)
 #7 - GDB emits MI =trace-stopped then =trace-started in response
      to tstop/tstart above.
 #8 - gdb processes the pending trace stop notification, emits
      MI =trace-stopped to frontend.
 #9 - gdb processes a trace start notification, emits
      MI =trace-start to frontend.
 #10 - frontend displays the trace session as running.


Now, looking at this, we see MI may get this trace event
sequence:

...
 =trace-stopped  (caused by tstop in bp command)
 =trace-start    (caused by tstart in bp command)
 =trace-stopped  (triggered by target)
 =trace-start    (triggered by target)

even though the tracing only stopped and started once.

To fully fix that, MI trace events triggered
by GDB actions could be distinguishable from MI trace
events triggered by the target (e.g, an attribute),
and the frontend could only look at target-triggered
events.  I'm not sure that's really necessary, and it can
always be added later, but it may be nice to have.

Another way to fix the ordering issue I just thought would
be to have a sequence number associated with each trace
session, and send those along trace start/stop notifications,
so that a delayed =trace-stopped generated from the target
would be older than the current trace session, so
it would be ignored.  Not sure how I feel about that.
Not sure how I feel about the other solution either.  :-)

Seems to me something needs to be done though.

-- 
Pedro Alves

Re: [PATCH 1/4] Query supported notifications by qSupported

[Pedro Alves]

On 12/12/2013 01:49 PM, Yao Qi wrote:
> 
>   --> qSupported:XXX;notifications=N1,N2.N3
>       (GDB supports notification N1, N2 and N3)
>   <-- XXX;Notifications=N1,N2,N4
>       (GDBsever supports notification N1, N2 and N4)

But is there any real benefit to the extra "Notifications="
indirection, rather than just treat notifications as regular
qSupported features?  IOW, why not simply:

   --> qSupported:XXX;N1+;N2+;N3+;N2ext+;ultimatefeature+

?

I.e., GDB supports XXX; N1 notifications; N2 notifications;
N3 notifications; foo extension on N2 notifications;
and the "ultimatefeature" feature, which actually
implies support for 3 different notifications.)

-- 
Pedro Alves

Re: [PATCH 0/4 V7] MI notification on trace started/stopped

[Yao Qi]

Pedro,
Thanks for giving such great example to facilitate the discussion.

On 12/18/2013 10:09 PM, Pedro Alves wrote:
> With trace events sent on a separate channel, we can get things
> like this, with a single-threaded program being traced:
> 
>  #1 - tstart, resume program
>  #2 - trace stops because buffer full
>  #3 - gdbserver queues trace stop notification.

Unlike stop notification, trace status notification is unlikely to be
queued in GDBserver.  Multiple threads may trigger multiple stops, and
stop notifications can be queued in GDBserver.  However, trace status
is a global state in GDBserver.  When tracing is stopped, trace status
can't be changed until GDB requests some changes.  That is to say,
GDBserver shouldn't get a new trace stop notification before the last
one is ack'ed by GDB.

If target is able to trigger trace start, trace notification may be
queued, like this sequence,

 #1 GDBserver sends %Trace on trace start, triggered by target,
 #2 trace stops because buffer full
 #3 gdbserver queues %Trace on trace stop (because notification in #1
    has not been ack'ed)

That is the _only_ case I can think of.

>  #4 - breakpoint at "next_session" is hit.
>  #5 - gdb happens to process breakpoint notification first.
>  #6 - The "next_session" breakpoint has a breakpoint
>       command that does: "tstop; tsave; tstart; continue"
>       (program iterates, and another collection sample begins)
>  #7 - GDB emits MI =trace-stopped then =trace-started in response
>       to tstop/tstart above.
>  #8 - gdb processes the pending trace stop notification, emits
>       MI =trace-stopped to frontend.
>  #9 - The frontend is confused, thinking the trace session is
>       stopped, when it is in fact running.
> 
> Now, even if we sent trace stops down the %Stop
> notification, that wouldn't happen.  GDB would always process
> the trace stop notification before the breakpoint.

Sorry, it is unclear to me.  What do you mean by "send trace stops down
the %Stop"?  Using %Stop to send trace stop notification?

> 
> But, considering multi-threaded programs, a different thread
> can stop the trace immediately after the breakpoint at
> next_session hits and triggers a %Stop notification,
> and so sending trace notifications using %Stop wouldn't
> fix the frontend confusion in this case, as the stop would
> still be processed before the trace stop.
> 
> What I'm just now thinking would fix it would be if the
> remote _also_ triggered trace _start_ notifications in
> addition to trace stops:
> 
>  #1 - tstart, resume program
>  #2 - trace stops because buffer full
>  #3 - gdbserver queues trace stop notification.
>  #4 - breakpoint at "next_session" is hit.
>  #5 - gdb happens to process breakpoint notification first.
>  #6 - The "next_session" breakpoint has a breakpoint
>       command that does: "tstop; tsave; tstart; continue"
>       (program iterates, and another collection sample begins)
>  #7 - GDB emits MI =trace-stopped then =trace-started in response
>       to tstop/tstart above.
>  #8 - gdb processes the pending trace stop notification, emits
>       MI =trace-stopped to frontend.
>  #9 - gdb processes a trace start notification, emits
>       MI =trace-start to frontend.
>  #10 - frontend displays the trace session as running.
> 

In order to get the queued trace notification, these steps can be
revised a little,

 #1 - trace is not started,
 #2 - trace is started triggered by target, %Trace is sent
 #3 - one thread hits breakpoint at "next_session", %Stop is sent
 #4 - the other thread triggers trace stops because buffer full,
 #5 - gdbserver queues trace stop notification (because notification in
      #2 hasn't been ack'ed),

When GDB starts process notification, GDB will process two %Trace first
(#2 and #4), then it processes %Stop, because in notif_queue, different
type of notifications are processed in an FIFO order.  In notif_queue,
we have Trace (1), Stop, Trace (2), but they are processed in this
order: Trace (1), Trace (2), Stop.

MI notifications are correct to MI front-end.

The follow steps really trigger the problem:

 #1 - thread A hit breakpoint, %Stop is send,
 #2 - trace is started triggered by target, %Trace is sent
 #3 - one thread hits breakpoint at "next_session", stop notification
      is queued in GDBserver,
 #4 - the other thread triggers trace stops because buffer full, trace
      notification is queued too.

In notif_queue, we have Stop (1), Trace (1), Stop (2), Trace (2), and
they are processed in order: Stop (1), Stop (2), Trace (1), Trace (2).
Then, MI notification are incorrect to MI front-end.

> To fully fix that, MI trace events triggered
> by GDB actions could be distinguishable from MI trace
> events triggered by the target (e.g, an attribute),
> and the frontend could only look at target-triggered
> events.  I'm not sure that's really necessary, and it can
> always be added later, but it may be nice to have.
> 
> Another way to fix the ordering issue I just thought would
> be to have a sequence number associated with each trace
> session, and send those along trace start/stop notifications,
> so that a delayed =trace-stopped generated from the target
> would be older than the current trace session, so
> it would be ignored.  Not sure how I feel about that.
> Not sure how I feel about the other solution either.  
> 
> Seems to me something needs to be done though.

These two solutions require MI front-end to have some extra logic
handling this ordering issue.  How about triggering MI trace events
_only_ by %Trace notification?  Even tracing is started or stopped
by commands in CLI, GDBserver still emits %Trace for started or stopped
trace.  In this way, the ordering issue can be resolved.

-- 
Yao (齐尧)

Re: [PATCH 1/4] Query supported notifications by qSupported

[Yao Qi]

On 12/18/2013 10:11 PM, Pedro Alves wrote:
> But is there any real benefit to the extra "Notifications="
> indirection, rather than just treat notifications as regular
> qSupported features?  IOW, why not simply:
> 
>    --> qSupported:XXX;N1+;N2+;N3+;N2ext+;ultimatefeature+
> 
> ?
> 
> I.e., GDB supports XXX; N1 notifications; N2 notifications;
> N3 notifications; foo extension on N2 notifications;
> and the "ultimatefeature" feature, which actually
> implies support for 3 different notifications.)

This will simplify the parsing and checking a given notification
is supported or not.  I didn't consider this approach when wrote
this patch.

Each notification doesn't look like a feature to me(, but some one
else may think notification is a feature).  The only
benefit I can think of now is to avoid name clashes.  A
third-party stub supports one notification Nfoo and GDB doesn't
have such feature or notification.  During the development, we add
a new feature Nfoo in GDB, so "N1+;Nfoo+;" confuses GDB, but
"Notifications=N1,Nfoo" doesn't.  On the other hand, it is better
to keep notification related code in remote-notif.c.

Again, I don't have a strong opinion on this.

-- 
Yao (齐尧)

Re: [PATCH 0/4 V7] MI notification on trace started/stopped

[Pedro Alves]

On 12/19/2013 06:48 AM, Yao Qi wrote:
> Pedro,
> Thanks for giving such great example to facilitate the discussion.
> 
> On 12/18/2013 10:09 PM, Pedro Alves wrote:
>> With trace events sent on a separate channel, we can get things
>> like this, with a single-threaded program being traced:
>>
>>  #1 - tstart, resume program
>>  #2 - trace stops because buffer full
>>  #3 - gdbserver queues trace stop notification.
> 
> Unlike stop notification, trace status notification is unlikely to be
> queued in GDBserver.  Multiple threads may trigger multiple stops, and
> stop notifications can be queued in GDBserver.  However, trace status
> is a global state in GDBserver.  When tracing is stopped, trace status
> can't be changed until GDB requests some changes.  That is to say,
> GDBserver shouldn't get a new trace stop notification before the last
> one is ack'ed by GDB.

I didn't mean queued behind another _trace_ event.  I meant queued
behind _other_ events GDB is handling.  Like, gdb handling the %Stop
or stdin before the %Trace.  Events on the GDB side are
ultimately handled serially.  So GDB can handle both stdin and
stop events before the trace event is handled.

Actually, we don't need stop events to show the problem.
async/non-stop/stdin work just as well:

#1 - (gdb) tstart
#2 - (gdb) c -a &
#3 - User types "tstop/tstart" (or runs a user defined command
     that does both with one command).
#4 - Meanwhile, as GDB is processing the "stdin" events, the
     trace stops on target, and a trace event is sent down the
     wire.  Let's say this event now "queued"
     until it is processed by GDB's event loop.
#5 - Due to #3, GDB outputs MI =trace-stop, followed by =trace-start.
#6 - The RSP trace-stop event is finally handled, and GDB outputs
     MI =trace-stop.
#7 - The frontend is now out of sync, because the the trace is actually
     running.

This is actually very much like GDB needing to be sure the frontend
doesn't get out of sync, with MI *running and *stopped
events/notifications.

> 
> If target is able to trigger trace start, trace notification may be
> queued, like this sequence,
> 
>  #1 GDBserver sends %Trace on trace start, triggered by target,
>  #2 trace stops because buffer full
>  #3 gdbserver queues %Trace on trace stop (because notification in #1
>     has not been ack'ed)
> 
> That is the _only_ case I can think of.
> 
>>  #4 - breakpoint at "next_session" is hit.
>>  #5 - gdb happens to process breakpoint notification first.
>>  #6 - The "next_session" breakpoint has a breakpoint
>>       command that does: "tstop; tsave; tstart; continue"
>>       (program iterates, and another collection sample begins)
>>  #7 - GDB emits MI =trace-stopped then =trace-started in response
>>       to tstop/tstart above.
>>  #8 - gdb processes the pending trace stop notification, emits
>>       MI =trace-stopped to frontend.
>>  #9 - The frontend is confused, thinking the trace session is
>>       stopped, when it is in fact running.
>>
>> Now, even if we sent trace stops down the %Stop
>> notification, that wouldn't happen.  GDB would always process
>> the trace stop notification before the breakpoint.
> 
> Sorry, it is unclear to me.  What do you mean by "send trace stops down
> the %Stop"?  Using %Stop to send trace stop notification?

Yes.  Sorry, the "even" shouldn't be there.  I meant:

"Now, if we sent trace stops down as a %Stop notification, that
wouldn't happen.  GDB would always process the trace stop
notification before the breakpoint."  But as both the
multi-threaded and the stdin examples show, that's not sufficient.

> 
>>
>> But, considering multi-threaded programs, a different thread
>> can stop the trace immediately after the breakpoint at
>> next_session hits and triggers a %Stop notification,
>> and so sending trace notifications using %Stop wouldn't
>> fix the frontend confusion in this case, as the stop would
>> still be processed before the trace stop.
>>
>> What I'm just now thinking would fix it would be if the
>> remote _also_ triggered trace _start_ notifications in
>> addition to trace stops:
>>
>>  #1 - tstart, resume program
>>  #2 - trace stops because buffer full
>>  #3 - gdbserver queues trace stop notification.
>>  #4 - breakpoint at "next_session" is hit.
>>  #5 - gdb happens to process breakpoint notification first.
>>  #6 - The "next_session" breakpoint has a breakpoint
>>       command that does: "tstop; tsave; tstart; continue"
>>       (program iterates, and another collection sample begins)
>>  #7 - GDB emits MI =trace-stopped then =trace-started in response
>>       to tstop/tstart above.
>>  #8 - gdb processes the pending trace stop notification, emits
>>       MI =trace-stopped to frontend.
>>  #9 - gdb processes a trace start notification, emits
>>       MI =trace-start to frontend.
>>  #10 - frontend displays the trace session as running.
>>
> 
> In order to get the queued trace notification, these steps can be
> revised a little,
> 
>  #1 - trace is not started,
>  #2 - trace is started triggered by target, %Trace is sent
>  #3 - one thread hits breakpoint at "next_session", %Stop is sent
>  #4 - the other thread triggers trace stops because buffer full,
>  #5 - gdbserver queues trace stop notification (because notification in
>       #2 hasn't been ack'ed),
> 
> When GDB starts process notification, GDB will process two %Trace first
> (#2 and #4), then it processes %Stop, because in notif_queue, different
> type of notifications are processed in an FIFO order.  In notif_queue,
> we have Trace (1), Stop, Trace (2), but they are processed in this
> order: Trace (1), Trace (2), Stop.
> 
> MI notifications are correct to MI front-end.
> 
> The follow steps really trigger the problem:
> 
>  #1 - thread A hit breakpoint, %Stop is send,
>  #2 - trace is started triggered by target, %Trace is sent
>  #3 - one thread hits breakpoint at "next_session", stop notification
>       is queued in GDBserver,
>  #4 - the other thread triggers trace stops because buffer full, trace
>       notification is queued too.
> 
> In notif_queue, we have Stop (1), Trace (1), Stop (2), Trace (2), and
> they are processed in order: Stop (1), Stop (2), Trace (1), Trace (2).
> Then, MI notification are incorrect to MI front-end.
> 
>> To fully fix that, MI trace events triggered
>> by GDB actions could be distinguishable from MI trace
>> events triggered by the target (e.g, an attribute),
>> and the frontend could only look at target-triggered
>> events.  I'm not sure that's really necessary, and it can
>> always be added later, but it may be nice to have.
>>
>> Another way to fix the ordering issue I just thought would
>> be to have a sequence number associated with each trace
>> session, and send those along trace start/stop notifications,
>> so that a delayed =trace-stopped generated from the target
>> would be older than the current trace session, so
>> it would be ignored.  Not sure how I feel about that.
>> Not sure how I feel about the other solution either.  
>>
>> Seems to me something needs to be done though.
> 
> These two solutions require MI front-end to have some extra logic
> handling this ordering issue.  

I was thinking that GDB would handle the sequence number
filtering on its own (the frontend wouldn't see them), but
I haven't thought that through.

> How about triggering MI trace events
> _only_ by %Trace notification?  Even tracing is started or stopped
> by commands in CLI, GDBserver still emits %Trace for started or stopped
> trace.  In this way, the ordering issue can be resolved.

Hmm.  I realized one more point to add to the discussion.

What GDB makes to sure the frontend doesn't get out
of sync wrt to thread's running state is that stops requests
are asynchronous, so GDB emits *running on its own (not when
the target said the resumption is effective), but *stopped is
only emitted when the target really reports it stopped,
not when the stop is requested with vCont;t.

But, unlike "vCont;t", tstop/QTStop requests are synchronous.
That is, when the target replies OK, the trace is stopped,
period.  It then worries me that the %Trace event will only
came later, after the "tstop" command returns.  Hmm^2.
So maybe we need to make sure that %Trace stop events
are flushed/consumed on "tstop"?
But If we'll ever support traces starting on their own on
the target, that may add to the complication.  I haven't
thought this part through, and I have to leave now for a
bit, but I thought I'd send out the email now anyway.

-- 
Pedro Alves

Re: [PATCH 0/4 V7] MI notification on trace started/stopped

[Yao Qi]

On 12/20/2013 09:57 PM, Pedro Alves wrote:
> I didn't mean queued behind another _trace_ event.  I meant queued
> behind _other_ events GDB is handling.  Like, gdb handling the %Stop
> or stdin before the %Trace.  Events on the GDB side are
> ultimately handled serially.  So GDB can handle both stdin and
> stop events before the trace event is handled.

You meant notification event is queued in GDB side, instead of
GDBserver, then I get your point.

> 
>> > How about triggering MI trace events
>> > _only_ by %Trace notification?  Even tracing is started or stopped
>> > by commands in CLI, GDBserver still emits %Trace for started or stopped
>> > trace.  In this way, the ordering issue can be resolved.
> Hmm.  I realized one more point to add to the discussion.
> 
> What GDB makes to sure the frontend doesn't get out
> of sync wrt to thread's running state is that stops requests
> are asynchronous, so GDB emits *running on its own (not when
> the target said the resumption is effective), but *stopped is
> only emitted when the target really reports it stopped,
> not when the stop is requested with vCont;t.
> 
> But, unlike "vCont;t", tstop/QTStop requests are synchronous.
> That is, when the target replies OK, the trace is stopped,
> period.  It then worries me that the %Trace event will only
> came later, after the "tstop" command returns.  Hmm^2.

As what we are doing in patch 2/4,

> @@ -3481,6 +3500,14 @@ stop_tracing (void)
>      }
>  
>    unpause_all (1);
> +
> +  if (notif_trace.supported)
> +    {
> +      struct notif_event *event
> +	= xmalloc (sizeof (struct notif_event));
> +
> +      notif_push (&notif_trace, event);
> +    }

%Trace is sent before OK, but is ack'ed after OK.

&"tstop\n"^M
&"Sending packet: $QTStop#4b..."^M
&"  Notification received:
Trace:status:T0;tstop::0;tframes:0;tcreated:0;tfree:500000;tsize:500000;circular:0;disconn:0;starttime:4ee06fece6180;stoptime:4ee06fece64a4;username:;notes::\n"^M
=trace-stopped\n^M
&"Packet received: OK\n"^M

  This "OK" is the reply to "QTStop".

&"Sending packet: $vTraced#c9..."^M
&"Packet received: OK\n"^M

  This "OK" is the reply to "vTraced".

> So maybe we need to make sure that %Trace stop events
> are flushed/consumed on "tstop"?

Yes, we need to add the following piece of code somewhere
to consume events,

  if (!non_stop)
    remote_notif_process (rs->notif_state, &notif_client_stop);

it is similar to what we did at the beginning of remote_resume.
Looks the end of remote_get_noisy_reply (when the actual reply is got)
is a good place to do so.

> But If we'll ever support traces starting on their own on
> the target, that may add to the complication.  I haven't
> thought this part through, and I have to leave now for a
> bit, but I thought I'd send out the email now anyway.

The ordering issue of MI events is caused by two sources triggering
MI events, CLI and remote target.  If MI notifications are only
triggered by one source, remote target, through %Trace, the ordering of
MI notifications should be consistent with the ordering of %Trace.
AFAICS, starting trace triggered by target doesn't make troubles here.

If all your concerns are cleared (I hope so) in this thread, I'll post
V8 to reflect the results of the discussion.

-- 
Yao (齐尧)