* [PATCH 3/6] Query supported notifications by qSupported
2013-08-19 1:56 [PATCH 0/6 V5] MI notification on trace started/stopped Yao Qi
@ 2013-08-19 1:56 ` Yao Qi
2013-08-19 1:56 ` [PATCH 6/6] MI notification on trace stop: triggered by remote Yao Qi
` (6 subsequent siblings)
7 siblings, 0 replies; 23+ messages in thread
From: Yao Qi @ 2013-08-19 1:56 UTC (permalink / raw)
To: gdb-patches
V5: the supported flag of each annex of notification is saved in
'struct remote_notif_state' in GDB. In GDBserver, the supported flag
is still associated with each annex.
As we we adding more notifications and annexes, both GDB and GDBserver
has to know what notifications and annexes 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.A1.A2,N3
(GDB supports notification N1, N2 with annexes A1,A2, and N3)
<-- XXX;Notifications=N1,N2.A1,N4
(GDBsever supports notification N1, N2 with annex A1 and N4)
after this, GDB knows what notifications GDBserver is able to send,
and GDBservers knows what notifications GDB doesn't support.
The documentation patch was approved by Eli here
<http://sourceware.org/ml/gdb-patches/2013-03/msg00474.html>
gdb/gdbserver:
* Makefile.in (SFILES): Add "notif-base.c".
(OBS): Add notif-base.o.
(notif-base.o): New rule.
* notif.c (notif_qsupported_record): New.
(notif_qsupported_reply): New.
* notif.h (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:
* common/notif-base.c: New.
* common/notif-base.h (struct notif_annex) [GDBSERVER] <supported>:
New field.
(struct notif_annex) [!GDBSERVER] <id>: New field.
(NOTIF_ANNEX_SUPPORTED_P): New macro.
(notif_supported, notif_parse_supported): Declare.
* Makefile.in (REMOTE_OBS): Append "notif-base.o".
(SFILES): Add "notif-base.c".
* remote-notif.c (remote_notif_parse_1): Call the parser of
annex if it is supported.
(remote_notif_qsupported): New.
(remote_notif_qsupported_reply): New.
* remote-notif.h (remote_notif_qsupported): Declare.
(remote_notif_qsupported_reply): Declare.
* remote.c (PACKET_notifications): New enum.
(remote_notifications_feature): New.
(remote_protocol_features): Add new element.
(remote_query_supported): Call remote_notif_qsupported and
append supported notifications to qSupported feature.
(notif_client_annex_stop): Update.
gdb/doc:
* gdb.texinfo (General Query Packets): Document the new feature
'notifications' of 'qSupported' packet.
Document the new feature in qSupported reply.
---
gdb/Makefile.in | 9 ++-
gdb/common/notif-base.c | 198 +++++++++++++++++++++++++++++++++++++++++++++
gdb/common/notif-base.h | 32 +++++++
gdb/doc/gdb.texinfo | 22 +++++
gdb/gdbserver/Makefile.in | 7 +-
gdb/gdbserver/notif.c | 21 +++++
gdb/gdbserver/notif.h | 2 +
gdb/gdbserver/server.c | 15 +++-
gdb/remote-notif.c | 108 +++++++++++++++++++++++--
gdb/remote-notif.h | 14 +++-
gdb/remote.c | 31 ++++++-
11 files changed, 440 insertions(+), 19 deletions(-)
create mode 100644 gdb/common/notif-base.c
diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 2dc3ac0..3f83122 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -521,7 +521,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 notif-base.o
# This is remote-sim.o if a simulator is to be linked in.
SIM_OBS = @SIM_OBS@
@@ -751,7 +751,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 \
+ notif-base.c reverse.c \
sentinel-frame.c \
serial.c ser-base.c ser-unix.c skip.c \
solib.c solib-target.c source.c \
@@ -1975,6 +1976,10 @@ common-utils.o: ${srcdir}/common/common-utils.c
$(COMPILE) $(srcdir)/common/common-utils.c
$(POSTCOMPILE)
+notif-base.o: ${srcdir}/common/notif-base.c
+ $(COMPILE) $(srcdir)/common/notif-base.c
+ $(POSTCOMPILE)
+
gdb_vecs.o: ${srcdir}/common/gdb_vecs.c
$(COMPILE) $(srcdir)/common/gdb_vecs.c
$(POSTCOMPILE)
diff --git a/gdb/common/notif-base.c b/gdb/common/notif-base.c
new file mode 100644
index 0000000..3d0a5f7
--- /dev/null
+++ b/gdb/common/notif-base.c
@@ -0,0 +1,198 @@
+/* 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/>. */
+
+#ifdef GDBSERVER
+#include "server.h"
+#else
+#include "defs.h"
+#include "remote-notif.h"
+#endif
+#include <string.h>
+#include "notif-base.h"
+#include "gdb_assert.h"
+
+/* Return a string about the notifications in array NOTIFS. NUM is
+ the number of elements in array NOTIFS. The caller is responsible
+ to free the returned string. Suppose array NOTIFS has
+ notifications N1, N2, and N3. Only N2 has annexes A1 and A2. The
+ returned string is "N1,N2.A2.A2,N3". */
+
+char *
+notif_supported (struct notif_base *notifs[], int num)
+{
+ int i;
+ char * p = NULL;
+
+#define BUF_LEN 128
+
+ for (i = 0; i < num; i++)
+ {
+ struct notif_base *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);
+
+ if (NOTIF_HAS_ANNEX (nb))
+ {
+ int j;
+ struct notif_annex *annex;
+
+ NOTIF_ITER_ANNEX (nb, j, annex)
+ {
+ xsnprintf (p + strlen (p), BUF_LEN - strlen (p),
+ ".%s", annex->name);
+ }
+ }
+ }
+
+ return p;
+}
+
+/* Find annex in notification NB by name NAME and length LEN.
+ If found, return its index in NB, otherwise return -1. */
+
+static int
+notif_find_annex (struct notif_base *nb, const char *name, int len)
+{
+ if (NOTIF_HAS_ANNEX (nb))
+ {
+ int j;
+ struct notif_annex *annex;
+
+ NOTIF_ITER_ANNEX (nb, j, annex)
+ if (strncmp (name, annex->name, len) == 0
+ && len == strlen (annex->name))
+ return j;
+ }
+ return -1;
+}
+
+/* Parse the REPLY, which is about supported annexes and
+ notifications from the peer, and disable some annexes of
+ notification in array NOTIFS if the peer doesn't support. NUM is
+ the number of elements in array NOTIFS. */
+
+void
+notif_parse_supported (const char *reply, struct notif_base *notifs[],
+#ifdef GDBSERVER
+ int num)
+#else
+ int num, struct remote_notif_state *state)
+#endif
+{
+ const char *p = reply;
+ int notif_num = 1;
+ char **notif_str;
+ int i;
+
+ /* Count how many notifications in REPLY. */
+ for (i = 0; reply[i] != '\0'; i++)
+ if (reply[i] == ',')
+ notif_num++;
+
+ /* Copy contents of each notification in REPLY to each slot of
+ NOTIF_STR. */
+ notif_str = xmalloc (notif_num * sizeof (char *));
+ for (i = 0; i < notif_num; i++)
+ {
+ char *end = strchr (p, ',');
+
+ if (end == NULL)
+ notif_str[i] = xstrdup (p);
+ else
+ {
+ /* Can't use xstrndup in GDBserver. */
+ notif_str[i] = strndup (p, end - p);
+ p = end + 1;
+ }
+ }
+
+ /* Iterate each element in NOTIF_STR and parse annex in it. */
+ for (i = 0; i < notif_num; i++)
+ {
+ int j;
+ struct notif_base *nb = NULL;
+
+ p = notif_str[i];
+
+ for (j = 0; j < num; j++)
+ {
+ int name_len = strlen (notifs[j]->notif_name);
+
+ if (0 == strncmp (notifs[j]->notif_name, p, name_len)
+ && (p[name_len] == '.' || p[name_len] == 0))
+ {
+ nb = notifs[j];
+ p += name_len;
+ break;
+ }
+ }
+
+ if (nb != NULL)
+ {
+ if (p[0] == 0)
+ {
+ /* No annex. */
+ gdb_assert (!NOTIF_HAS_ANNEX (nb));
+ NOTIF_ANNEX_SUPPORTED (state, nb, 0) = 1;
+ }
+ else if (p[0] == '.')
+ {
+ gdb_assert (NOTIF_HAS_ANNEX (nb));
+
+ p++;
+
+ /* Parse the rest of P and look for annexes. */
+ while (p != NULL)
+ {
+ char *end = strchr (p, '.');
+ int index;
+
+ if (end != NULL)
+ {
+ index = notif_find_annex (nb, p, end - p);
+ p = end + 1;
+ }
+ else
+ {
+ index = notif_find_annex (nb, p, strlen (p));
+ p = end;
+ }
+
+ /* If annex is known, mark it supported, otherwise
+ skip it because the peer knows the annex but we
+ don't know. */
+ if (index >= 0)
+ NOTIF_ANNEX_SUPPORTED (state, nb, index) = 1;
+ }
+ }
+ else
+ warning (_("Unknown supported notification"));
+ }
+ }
+
+ for (i = 0; i < notif_num; i++)
+ xfree (notif_str[i]);
+
+ xfree (notif_str);
+}
diff --git a/gdb/common/notif-base.h b/gdb/common/notif-base.h
index 0a3a4ed..ddc9293 100644
--- a/gdb/common/notif-base.h
+++ b/gdb/common/notif-base.h
@@ -37,9 +37,18 @@ struct notif_annex
const char *name;
#ifdef GDBSERVER
+ /* This annex is supported by GDB or not. A notification may have
+ multiple annexes and some of them are supported. Annex is the
+ smallest unit of support. */
+ int supported;
+
/* Write event EVENT to OWN_BUF. */
void (*write) (struct notif_event *event, char *own_buf);
#else
+ /* The id of the annex. In GDB, we use this field as an index to
+ access the state of this annex. */
+ int id;
+
/* Parse BUF to get the expected event and update EVENT. This
function may throw exception if contents in BUF is not the
expected event. */
@@ -57,6 +66,15 @@ struct notif_annex
#define NOTIF_HAS_ANNEX(NOTIF) ((NOTIF)->annexes[0].name != NULL)
+/* Whether the annex of notification N is supported. */
+
+#ifdef GDBSERVER
+#define NOTIF_ANNEX_SUPPORTED(STATE, N, INDEX) \
+ ((N)->annexes[INDEX].supported)
+#else
+#define NOTIF_ANNEX_SUPPORTED(STATE, N, INDEX) \
+ ((STATE)->supported[(N)->annexes[INDEX].id])
+#endif
/* "Base class" of a notification. It can be extended in both GDB
and GDBserver to represent a type of notification. */
@@ -76,4 +94,18 @@ struct notif_base
struct notif_annex *annexes;
};
+char *notif_supported (struct notif_base *notifs[], int num);
+
+#ifndef GDBSERVER
+struct remote_notif_state;
+#endif
+
+void notif_parse_supported (const char *reply,
+ struct notif_base *notifs[],
+#ifdef GDBSERVER
+ int num);
+#else
+ int num, struct remote_notif_state *state);
+#endif
+
#endif /* NOTIF_BASE_H */
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 375f988..946e132 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -38655,6 +38655,15 @@ 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 supported notifications,
+separated by commas, it will report notifications supported by the
+stub. Each notification in the string is composed by the name of the
+notification and the annexes, if any, which are separated by periods.
@end table
Stubs should ignore any unknown values for
@@ -38873,6 +38882,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:
@@ -39040,6 +39054,14 @@ 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{annex}@r{]}@dots{}@r{[},@var{name}@r{[}.@var{annex}@r{]}@dots{}@r{]}@dots{}
+@cindex notifications, in remote protocol
+The remote stub supports a string of notifications. @var{name} is
+the name of the notification and @var{annex} is the name of the annex,
+if the notification has the annex. Note that the notification may
+have several annexes, so here could be several @var{annex} parts
+separated by periods.
+
@item Qbtrace:off
The remote stub understands the @samp{Qbtrace:off} packet.
diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
index e541497..7c6db6f 100644
--- a/gdb/gdbserver/Makefile.in
+++ b/gdb/gdbserver/Makefile.in
@@ -158,7 +158,7 @@ SFILES= $(srcdir)/gdbreplay.c $(srcdir)/inferiors.c $(srcdir)/dll.c \
$(srcdir)/common/linux-osdata.c $(srcdir)/common/ptid.c \
$(srcdir)/common/buffer.c $(srcdir)/common/linux-btrace.c \
$(srcdir)/common/filestuff.c $(srcdir)/common/target-common.c \
- $(srcdir)/common/mips-linux-watch.c
+ $(srcdir)/common/mips-linux-watch.c $(srcdir)/common/notif-base.c
DEPFILES = @GDBSERVER_DEPFILES@
@@ -168,7 +168,7 @@ SOURCES = $(SFILES)
TAGFILES = $(SOURCES) ${HFILES} ${ALLPARAM} ${POSSLIBS}
OBS = agent.o ax.o inferiors.o regcache.o remote-utils.o server.o signals.o \
- target.o target-common.o utils.o version.o vec.o gdb_vecs.o \
+ target.o target-common.o utils.o version.o vec.o gdb_vecs.o notif-base.o \
mem-break.o hostio.o event-loop.o tracepoint.o xml-utils.o \
common-utils.o ptid.o buffer.o format.o filestuff.o dll.o notif.o \
tdesc.o $(XML_BUILTIN) $(DEPFILES) $(LIBOBJS)
@@ -523,6 +523,9 @@ linux-ptrace.o: ../common/linux-ptrace.c
common-utils.o: ../common/common-utils.c
$(COMPILE) $<
$(POSTCOMPILE)
+notif-base.o: ../common/notif-base.c
+ $(COMPILE) $<
+ $(POSTCOMPILE)
vec.o: ../common/vec.c
$(COMPILE) $<
$(POSTCOMPILE)
diff --git a/gdb/gdbserver/notif.c b/gdb/gdbserver/notif.c
index 9300697..2b09078 100644
--- a/gdb/gdbserver/notif.c
+++ b/gdb/gdbserver/notif.c
@@ -183,6 +183,27 @@ 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)
+{
+ return notif_parse_supported (gdb_notifications,
+ (struct notif_base **) notifs,
+ ARRAY_SIZE (notifs));
+}
+
+/* Return a string about notifications that GDBserver supports.
+ Return NULL if no notification is supported. */
+
+char *
+notif_qsupported_reply (void)
+{
+ return notif_supported ((struct notif_base **) notifs,
+ ARRAY_SIZE (notifs));
+}
+
void
initialize_notif (void)
{
diff --git a/gdb/gdbserver/notif.h b/gdb/gdbserver/notif.h
index 037f1b0..f33fd04 100644
--- a/gdb/gdbserver/notif.h
+++ b/gdb/gdbserver/notif.h
@@ -60,6 +60,8 @@ 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 71b860d..ce221fc 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -183,7 +183,7 @@ vstop_notif_reply (struct notif_event *event, char *own_buf)
static struct notif_annex notif_annex_stop[] =
{
- { NULL, vstop_notif_reply, },
+ { NULL, 1, vstop_notif_reply, },
};
struct notif_server notif_stop =
@@ -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);
@@ -1819,6 +1824,14 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
strcat (own_buf, ";Qbtrace:off+");
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 d901a51..b83ea7e 100644
--- a/gdb/remote-notif.c
+++ b/gdb/remote-notif.c
@@ -57,6 +57,7 @@ static void do_notif_event_xfree (void *arg);
static void
remote_notif_parse_1 (struct notif_client *nc,
+ struct remote_notif_state *state,
struct notif_event *event, char *buf)
{
const struct notif_annex *m = NULL;
@@ -73,9 +74,16 @@ remote_notif_parse_1 (struct notif_client *nc,
contents in BUF. */
&& buf[strlen (m->name)] == ':')
{
- /* Pass BUF without annex and ':'. */
- m->parse (nc, buf + strlen (m->name) + 1, event);
- break;
+ if (state->supported[m->id])
+ {
+ /* Pass BUF without annex and ':'. */
+ m->parse (nc, buf + strlen (m->name) + 1, event);
+ break;
+ }
+ else
+ warning (_("GDB gets annex '%s' of notification '%s'"
+ "but remote stub doesn't support"),
+ base->notif_name, m->name);
}
m = NULL;
}
@@ -95,7 +103,8 @@ remote_notif_parse_1 (struct notif_client *nc,
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
@@ -105,7 +114,7 @@ remote_notif_ack (struct notif_client *nc, char *buf)
fprintf_unfiltered (gdb_stdlog, "notif: ack '%s'\n",
nc->base.ack_name);
- remote_notif_parse_1 (nc, event, buf);
+ remote_notif_parse_1 (nc, state, event, buf);
nc->ack (nc, buf, event);
discard_cleanups (old_chain);
@@ -114,7 +123,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
@@ -124,7 +134,7 @@ remote_notif_parse (struct notif_client *nc, char *buf)
fprintf_unfiltered (gdb_stdlog, "notif: parse '%s'\n",
nc->base.notif_name);
- remote_notif_parse_1 (nc, event, buf);
+ remote_notif_parse_1 (nc, state, event, buf);
discard_cleanups (old_chain);
return event;
@@ -220,7 +230,7 @@ handle_notification (struct remote_notif_state *state, char *buf)
else
{
struct notif_event *event
- = remote_notif_parse (nc,
+ = remote_notif_parse (nc, state,
buf + strlen (nc->base.notif_name) + 1);
/* Be careful to only set it after parsing, since an error
@@ -302,6 +312,60 @@ notif_xfree (struct notif_client *notif)
xfree (notif);
}
+/* Return a string of GDB supported features. */
+
+char *
+remote_notif_qsupported (void)
+{
+ return notif_supported ((struct notif_base **) notifs,
+ ARRAY_SIZE (notifs));
+}
+
+/* Parse the qSupported reply REPLY from the remote stub and disable
+ some notifications if the remote stub doesn't support. */
+
+void
+remote_notif_qsupported_reply (const char *reply,
+ struct remote_notif_state *state)
+{
+ notif_parse_supported (reply, (struct notif_base **) notifs,
+ ARRAY_SIZE (notifs), state);
+
+ if (notif_debug)
+ {
+ int i;
+
+ fprintf_unfiltered (gdb_stdlog,
+ "notif: stub supported notifications '%s'\n",
+ reply);
+ fprintf_unfiltered (gdb_stdlog, "notif annexes:\n");
+ for (i = 0; i < ARRAY_SIZE (notifs); i++)
+ {
+ struct notif_base *nb = (struct notif_base *) notifs[i];
+
+ fprintf_unfiltered (gdb_stdlog, "%-10s ", nb->notif_name);
+ if (NOTIF_HAS_ANNEX (nb))
+ {
+ int j;
+ struct notif_annex *annex;
+
+ NOTIF_ITER_ANNEX (nb, j, annex)
+ fprintf_unfiltered (gdb_stdlog, "\n %-10s %d", annex->name,
+ NOTIF_ANNEX_SUPPORTED (state, nb, j));
+ }
+ else
+ fprintf_unfiltered (gdb_stdlog, " %d",
+ NOTIF_ANNEX_SUPPORTED (state, nb, 0));
+
+ fprintf_unfiltered (gdb_stdlog, "\n");
+ }
+ }
+}
+
+/* Total number of annexes. */
+
+static int notif_annex_number = 0;
+
/* Return an allocated remote_notif_state. */
struct remote_notif_state *
@@ -310,6 +374,14 @@ remote_notif_state (void)
struct remote_notif_state *notif_state = xmalloc (sizeof (*notif_state));
notif_state->notif_queue = QUEUE_alloc (notif_client_p, notif_xfree);
+ notif_state->supported = xmalloc (notif_annex_number * sizeof (int));
+ memset (notif_state->supported, 0, notif_annex_number * sizeof (int));
+
+ /* Even the remote stub doesn't understand
+ 'qSupported:notifications=', it may still support notification
+ stop if it supports non-stop. */
+ NOTIF_ANNEX_SUPPORTED (notif_state, ¬if_client_stop.base, 0) = 1;
+
return notif_state;
}
@@ -319,6 +391,26 @@ extern initialize_file_ftype _initialize_notif;
void
_initialize_notif (void)
{
+ int i;
+
+ /* Initialize the field of 'struct notif_annex' which is the index
+ of array which is about whether the annex is supported or not. */
+ for (i = 0; i < ARRAY_SIZE (notifs); i++)
+ {
+ struct notif_base *nb = (struct notif_base *) notifs[i];
+
+ if (NOTIF_HAS_ANNEX (nb))
+ {
+ int j;
+ struct notif_annex *annex;
+
+ NOTIF_ITER_ANNEX (nb, j, annex)
+ annex->id = notif_annex_number++;
+ }
+ else
+ nb->annexes[0].id = notif_annex_number++;
+ }
+
add_setshow_boolean_cmd ("notification", no_class, ¬if_debug,
_("\
Set debugging of async remote notification."), _("\
diff --git a/gdb/remote-notif.h b/gdb/remote-notif.h
index 454dce8..9a68779 100644
--- a/gdb/remote-notif.h
+++ b/gdb/remote-notif.h
@@ -67,10 +67,17 @@ struct remote_notif_state
{
/* Notification queue. */
QUEUE(notif_client_p) *notif_queue;
+
+ /* Each element indicates the annex is supported by the remote
+ stub or not. The index is the field 'id' in
+ 'struct notif_annex'. */
+ 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 handle_notification (struct remote_notif_state *notif_state,
@@ -83,6 +90,11 @@ void remote_notif_process (struct remote_notif_state *state,
struct notif_client *except);
struct remote_notif_state *remote_notif_state (void);
+
+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 9700a8e..2877c7c6 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1369,6 +1369,7 @@ enum {
PACKET_Qbtrace_off,
PACKET_Qbtrace_bts,
PACKET_qXfer_btrace,
+ PACKET_notifications,
PACKET_MAX
};
@@ -3573,7 +3574,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. */
notif_client_stop.pending_event
- = 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
@@ -3993,6 +3994,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,
@@ -4067,7 +4079,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;
@@ -4132,6 +4146,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+");
@@ -4141,6 +4156,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);
@@ -4597,7 +4616,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 (¬if_client_stop, wait_status);
+ = remote_notif_parse (¬if_client_stop, rs->notif_state,
+ wait_status);
push_stop_reply ((struct stop_reply *) reply);
@@ -5371,7 +5391,7 @@ remote_notif_stop_alloc_reply (void)
static struct notif_annex notif_client_annex_stop[] =
{
- { NULL, remote_notif_stop_parse, },
+ { NULL, -1, remote_notif_stop_parse, },
};
/* A client of notification Stop. */
@@ -5817,7 +5837,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
@@ -6021,6 +6041,7 @@ remote_wait_as (ptid_t ptid, struct target_waitstatus *status, int options)
{
struct stop_reply *stop_reply
= (struct stop_reply *) remote_notif_parse (¬if_client_stop,
+ rs->notif_state,
rs->buf);
event_ptid = process_stop_reply (stop_reply, status);
--
1.7.7.6
^ permalink raw reply [flat|nested] 23+ messages in thread* [PATCH 6/6] MI notification on trace stop: triggered by remote
2013-08-19 1:56 [PATCH 0/6 V5] MI notification on trace started/stopped Yao Qi
2013-08-19 1:56 ` [PATCH 3/6] Query supported notifications by qSupported Yao Qi
@ 2013-08-19 1:56 ` Yao Qi
2013-08-19 1:56 ` [PATCH 1/6] Move notif_queue to remote_state Yao Qi
` (5 subsequent siblings)
7 siblings, 0 replies; 23+ messages in thread
From: Yao Qi @ 2013-08-19 1:56 UTC (permalink / raw)
To: gdb-patches
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 | 11 ++++
gdb/testsuite/gdb.trace/mi-trace-changed.exp | 75 ++++++++++++++++++++++++++
2 files changed, 86 insertions(+), 0 deletions(-)
diff --git a/gdb/remote-notif-trace.c b/gdb/remote-notif-trace.c
index d9ff1c1..65c1748 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_status_parse (struct notif_client *self, char *buf,
@@ -31,6 +32,16 @@ remote_notif_trace_status_parse (struct notif_client *self, char *buf,
gdb_assert (buf[0] == 'T');
parse_trace_status (buf + 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);
}
static void
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
^ permalink raw reply [flat|nested] 23+ messages in thread* [PATCH 1/6] Move notif_queue to remote_state
2013-08-19 1:56 [PATCH 0/6 V5] MI notification on trace started/stopped Yao Qi
2013-08-19 1:56 ` [PATCH 3/6] Query supported notifications by qSupported Yao Qi
2013-08-19 1:56 ` [PATCH 6/6] MI notification on trace stop: triggered by remote Yao Qi
@ 2013-08-19 1:56 ` Yao Qi
2013-09-25 16:12 ` Pedro Alves
2013-08-19 1:56 ` [PATCH 4/6] async remote notification 'Trace' Yao Qi
` (4 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Yao Qi @ 2013-08-19 1:56 UTC (permalink / raw)
To: gdb-patches
This patch is to move global 'notif_queue' to 'struct remote_state', as
a follow up to Tom's patch series '[PATCH 00/16] clean up remote.c state'
<http://sourceware.org/ml/gdb-patches/2013-06/msg00605.html>
gdb:
* remote-notif.c (DECLARE_QUEUE_P): Remove.
(notif_queue): Remove.
(remote_notif_process): Add one parameter 'notif_queue'.
Update comments. Callers update.
(remote_notif_register_async_event_handler): Add parameter
'notif_queue'. Pass 'notif_queue' to
create_async_event_handler.
(handle_notification): Add parameter 'notif_queue'. Update
comments. Callers update.
(remote_notif_state): New function.
(_initialize_notif): Remove code to allocate queue.
* remote-notif.h (DECLARE_QUEUE_P): Moved from remote-notif.c.
(struct remote_notif_state): New.
(handle_notification): Update declaration.
(remote_notif_register_async_event_handler): Likewise.
(remote_notif_process): Likewise.
(remote_notif_state): Declare.
* remote.c (struct remote_state) <notif_state>: New field.
(remote_open_1): Initialize rs->notif_state and pass it to
remote_notif_register_async_event_handler.
---
gdb/remote-notif.c | 40 ++++++++++++++++++++++++----------------
gdb/remote-notif.h | 20 +++++++++++++++++---
gdb/remote.c | 12 ++++++++----
3 files changed, 49 insertions(+), 23 deletions(-)
diff --git a/gdb/remote-notif.c b/gdb/remote-notif.c
index c4bfd8d..8b69f2f 100644
--- a/gdb/remote-notif.c
+++ b/gdb/remote-notif.c
@@ -91,21 +91,19 @@ remote_notif_parse (struct notif_client *nc, char *buf)
return event;
}
-DECLARE_QUEUE_P (notif_client_p);
DEFINE_QUEUE_P (notif_client_p);
-static QUEUE(notif_client_p) *notif_queue;
-
-/* Process notifications one by one. EXCEPT is not expected in
- the queue. */
+/* Process notifications in NOTIF_QUEUE one by one. EXCEPT is not expected
+ in the queue. */
void
-remote_notif_process (struct notif_client *except)
+remote_notif_process (struct remote_notif_state *state,
+ struct notif_client *except)
{
- while (!QUEUE_is_empty (notif_client_p, notif_queue))
+ while (!QUEUE_is_empty (notif_client_p, state->notif_queue))
{
struct notif_client *nc = QUEUE_deque (notif_client_p,
- notif_queue);
+ state->notif_queue);
gdb_assert (nc != except);
@@ -118,7 +116,7 @@ static void
remote_async_get_pending_events_handler (gdb_client_data data)
{
gdb_assert (non_stop);
- remote_notif_process (NULL);
+ remote_notif_process (data, NULL);
}
/* Asynchronous signal handle registered as event loop source for when
@@ -131,11 +129,11 @@ static struct async_event_handler *remote_async_get_pending_events_token;
/* Register async_event_handler for notification. */
void
-remote_notif_register_async_event_handler (void)
+remote_notif_register_async_event_handler (struct remote_notif_state *state)
{
remote_async_get_pending_events_token
= create_async_event_handler (remote_async_get_pending_events_handler,
- NULL);
+ state);
}
/* Unregister async_event_handler for notification. */
@@ -147,10 +145,11 @@ remote_notif_unregister_async_event_handler (void)
delete_async_event_handler (&remote_async_get_pending_events_token);
}
-/* Remote notification handler. */
+/* Remote notification handler. Parse BUF, queue notification and
+ update STATE. */
void
-handle_notification (char *buf)
+handle_notification (struct remote_notif_state *state, char *buf)
{
struct notif_client *nc = NULL;
int i;
@@ -189,7 +188,7 @@ handle_notification (char *buf)
/* Notify the event loop there's a stop reply to acknowledge
and that there may be more events to fetch. */
- QUEUE_enque (notif_client_p, notif_queue, nc);
+ QUEUE_enque (notif_client_p, state->notif_queue, nc);
if (non_stop)
{
/* In non-stop, We mark REMOTE_ASYNC_GET_PENDING_EVENTS_TOKEN
@@ -262,14 +261,23 @@ notif_xfree (struct notif_client *notif)
xfree (notif);
}
+/* Return an allocated remote_notif_state. */
+
+struct remote_notif_state *
+remote_notif_state (void)
+{
+ struct remote_notif_state *notif_state = xmalloc (sizeof (*notif_state));
+
+ notif_state->notif_queue = QUEUE_alloc (notif_client_p, notif_xfree);
+ return notif_state;
+}
+
/* -Wmissing-prototypes */
extern initialize_file_ftype _initialize_notif;
void
_initialize_notif (void)
{
- notif_queue = QUEUE_alloc (notif_client_p, notif_xfree);
-
add_setshow_boolean_cmd ("notification", no_class, ¬if_debug,
_("\
Set debugging of async remote notification."), _("\
diff --git a/gdb/remote-notif.h b/gdb/remote-notif.h
index da4fdea..05d2613 100644
--- a/gdb/remote-notif.h
+++ b/gdb/remote-notif.h
@@ -68,16 +68,30 @@ typedef struct notif_client
struct notif_event *pending_event;
} *notif_client_p;
+DECLARE_QUEUE_P (notif_client_p);
+
+/* State on remote async notification. */
+
+struct remote_notif_state
+{
+ /* Notification queue. */
+ QUEUE(notif_client_p) *notif_queue;
+};
+
void remote_notif_ack (struct notif_client *nc, char *buf);
struct notif_event *remote_notif_parse (struct notif_client *nc,
char *buf);
-void handle_notification (char *buf);
+void handle_notification (struct remote_notif_state *notif_state,
+ char *buf);
-void remote_notif_register_async_event_handler (void);
+void remote_notif_register_async_event_handler (struct remote_notif_state *);
void remote_notif_unregister_async_event_handler (void);
-void remote_notif_process (struct notif_client *except);
+void remote_notif_process (struct remote_notif_state *state,
+ struct notif_client *except);
+struct remote_notif_state *remote_notif_state (void);
+
extern struct notif_client notif_client_stop;
extern int notif_debug;
diff --git a/gdb/remote.c b/gdb/remote.c
index 5028451..e50623a 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -425,6 +425,9 @@ struct remote_state
threadref echo_nextthread;
threadref nextthread;
threadref resultthreadlist[MAXTHREADLISTRESULTS];
+
+ /* The state of remote notification. */
+ struct remote_notif_state *notif_state;
};
/* Private data that we'll store in (struct thread_info)->private. */
@@ -4337,7 +4340,8 @@ remote_open_1 (char *name, int from_tty,
remote_async_inferior_event_token
= create_async_event_handler (remote_async_inferior_event_handler,
NULL);
- remote_notif_register_async_event_handler ();
+ rs->notif_state = remote_notif_state ();
+ remote_notif_register_async_event_handler (rs->notif_state);
/* Reset the target state; these things will be queried either by
remote_query_supported or as they are needed. */
@@ -4931,7 +4935,7 @@ remote_resume (struct target_ops *ops,
before resuming inferior, because inferior was stopped and no RSP
traffic at that moment. */
if (!non_stop)
- remote_notif_process (¬if_client_stop);
+ remote_notif_process (rs->notif_state, ¬if_client_stop);
rs->last_sent_signal = siggnal;
rs->last_sent_step = step;
@@ -7395,7 +7399,7 @@ putpkt_binary (char *buf, int cnt)
str);
do_cleanups (old_chain);
}
- handle_notification (rs->buf);
+ handle_notification (rs->notif_state, rs->buf);
/* We're in sync now, rewait for the ack. */
tcount = 0;
}
@@ -7781,7 +7785,7 @@ getpkt_or_notif_sane_1 (char **buf, long *sizeof_buf, int forever,
if (is_notif != NULL)
*is_notif = 1;
- handle_notification (*buf);
+ handle_notification (rs->notif_state, *buf);
/* Notifications require no acknowledgement. */
--
1.7.7.6
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 1/6] Move notif_queue to remote_state
2013-08-19 1:56 ` [PATCH 1/6] Move notif_queue to remote_state Yao Qi
@ 2013-09-25 16:12 ` Pedro Alves
2013-09-30 7:34 ` Yao Qi
0 siblings, 1 reply; 23+ messages in thread
From: Pedro Alves @ 2013-09-25 16:12 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On 08/19/2013 02:55 AM, Yao Qi wrote:
> This patch is to move global 'notif_queue' to 'struct remote_state', as
> a follow up to Tom's patch series '[PATCH 00/16] clean up remote.c state'
> <http://sourceware.org/ml/gdb-patches/2013-06/msg00605.html>
This all looks like good forward progress. Thanks!
Though a couple things weren't migrated to the new struct, and I'm not
sure whether that was an oversight, given you didn't specify a plan
of action for those upfront. Comments below.
>
> gdb:
>
> * remote-notif.c (DECLARE_QUEUE_P): Remove.
> (notif_queue): Remove.
> (remote_notif_process): Add one parameter 'notif_queue'.
> Update comments. Callers update.
> (remote_notif_register_async_event_handler): Add parameter
> 'notif_queue'. Pass 'notif_queue' to
> create_async_event_handler.
> (handle_notification): Add parameter 'notif_queue'. Update
> comments. Callers update.
> (remote_notif_state): New function.
> (_initialize_notif): Remove code to allocate queue.
> * remote-notif.h (DECLARE_QUEUE_P): Moved from remote-notif.c.
> (struct remote_notif_state): New.
> (handle_notification): Update declaration.
> (remote_notif_register_async_event_handler): Likewise.
> (remote_notif_process): Likewise.
> (remote_notif_state): Declare.
> * remote.c (struct remote_state) <notif_state>: New field.
> (remote_open_1): Initialize rs->notif_state and pass it to
> remote_notif_register_async_event_handler.
> ---
> gdb/remote-notif.c | 40 ++++++++++++++++++++++++----------------
> gdb/remote-notif.h | 20 +++++++++++++++++---
> gdb/remote.c | 12 ++++++++----
> 3 files changed, 49 insertions(+), 23 deletions(-)
>
> diff --git a/gdb/remote-notif.c b/gdb/remote-notif.c
> index c4bfd8d..8b69f2f 100644
> --- a/gdb/remote-notif.c
> +++ b/gdb/remote-notif.c
> @@ -91,21 +91,19 @@ remote_notif_parse (struct notif_client *nc, char *buf)
> return event;
> }
>
> -DECLARE_QUEUE_P (notif_client_p);
> DEFINE_QUEUE_P (notif_client_p);
>
> -static QUEUE(notif_client_p) *notif_queue;
> -
> -/* Process notifications one by one. EXCEPT is not expected in
> - the queue. */
> +/* Process notifications in NOTIF_QUEUE one by one. EXCEPT is not expected
> + in the queue. */
Looks like stale comment from earlier revision:
s/NOTIF_QUEUE/in STATE's notification queue/ or something like that.
>
> void
> -remote_notif_process (struct notif_client *except)
> +remote_notif_process (struct remote_notif_state *state,
> + struct notif_client *except)
> {
> - while (!QUEUE_is_empty (notif_client_p, notif_queue))
> + while (!QUEUE_is_empty (notif_client_p, state->notif_queue))
> {
> struct notif_client *nc = QUEUE_deque (notif_client_p,
> - notif_queue);
> + state->notif_queue);
>
> gdb_assert (nc != except);
>
> @@ -118,7 +116,7 @@ static void
> remote_async_get_pending_events_handler (gdb_client_data data)
> {
> gdb_assert (non_stop);
> - remote_notif_process (NULL);
> + remote_notif_process (data, NULL);
> }
>
> /* Asynchronous signal handle registered as event loop source for when
> @@ -131,11 +129,11 @@ static struct async_event_handler *remote_async_get_pending_events_token;
> /* Register async_event_handler for notification. */
>
> void
> -remote_notif_register_async_event_handler (void)
> +remote_notif_register_async_event_handler (struct remote_notif_state *state)
> {
> remote_async_get_pending_events_token
> = create_async_event_handler (remote_async_get_pending_events_handler,
> - NULL);
> + state);
As soon as we call this a second time, we overwrite the previous token.
What's the plan here? Shouldn't we move the token to the remote_notif_state too?
> }
>
> /* Unregister async_event_handler for notification. */
> @@ -147,10 +145,11 @@ remote_notif_unregister_async_event_handler (void)
> delete_async_event_handler (&remote_async_get_pending_events_token);
> }
>
> -/* Remote notification handler. */
> +/* Remote notification handler. Parse BUF, queue notification and
> + update STATE. */
>
> void
> -handle_notification (char *buf)
> +handle_notification (struct remote_notif_state *state, char *buf)
> {
> struct notif_client *nc = NULL;
> int i;
> @@ -189,7 +188,7 @@ handle_notification (char *buf)
>
> /* Notify the event loop there's a stop reply to acknowledge
> and that there may be more events to fetch. */
> - QUEUE_enque (notif_client_p, notif_queue, nc);
> + QUEUE_enque (notif_client_p, state->notif_queue, nc);
> if (non_stop)
> {
> /* In non-stop, We mark REMOTE_ASYNC_GET_PENDING_EVENTS_TOKEN
> @@ -262,14 +261,23 @@ notif_xfree (struct notif_client *notif)
> xfree (notif);
> }
>
> +/* Return an allocated remote_notif_state. */
> +
> +struct remote_notif_state *
> +remote_notif_state (void)
(I'd suggest naming it new_remote_notif_state, avoiding the naming
conflict when compiling with a C++ compiler.)
> +{
> + struct remote_notif_state *notif_state = xmalloc (sizeof (*notif_state));
> +
> + notif_state->notif_queue = QUEUE_alloc (notif_client_p, notif_xfree);
> + return notif_state;
> +}
> +
> /* -Wmissing-prototypes */
> extern initialize_file_ftype _initialize_notif;
>
> void
> _initialize_notif (void)
> {
> - notif_queue = QUEUE_alloc (notif_client_p, notif_xfree);
> -
> add_setshow_boolean_cmd ("notification", no_class, ¬if_debug,
> _("\
> Set debugging of async remote notification."), _("\
> diff --git a/gdb/remote-notif.h b/gdb/remote-notif.h
> index da4fdea..05d2613 100644
> --- a/gdb/remote-notif.h
> +++ b/gdb/remote-notif.h
> @@ -68,16 +68,30 @@ typedef struct notif_client
> struct notif_event *pending_event;
> } *notif_client_p;
>
> +DECLARE_QUEUE_P (notif_client_p);
> +
> +/* State on remote async notification. */
> +
> +struct remote_notif_state
> +{
> + /* Notification queue. */
> + QUEUE(notif_client_p) *notif_queue;
> +};
> +
> void remote_notif_ack (struct notif_client *nc, char *buf);
> struct notif_event *remote_notif_parse (struct notif_client *nc,
> char *buf);
>
> -void handle_notification (char *buf);
> +void handle_notification (struct remote_notif_state *notif_state,
> + char *buf);
>
> -void remote_notif_register_async_event_handler (void);
> +void remote_notif_register_async_event_handler (struct remote_notif_state *);
> void remote_notif_unregister_async_event_handler (void);
>
> -void remote_notif_process (struct notif_client *except);
> +void remote_notif_process (struct remote_notif_state *state,
> + struct notif_client *except);
> +struct remote_notif_state *remote_notif_state (void);
> +
> extern struct notif_client notif_client_stop;
>
> extern int notif_debug;
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 5028451..e50623a 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -425,6 +425,9 @@ struct remote_state
> threadref echo_nextthread;
> threadref nextthread;
> threadref resultthreadlist[MAXTHREADLISTRESULTS];
> +
> + /* The state of remote notification. */
> + struct remote_notif_state *notif_state;
> };
>
> /* Private data that we'll store in (struct thread_info)->private. */
> @@ -4337,7 +4340,8 @@ remote_open_1 (char *name, int from_tty,
> remote_async_inferior_event_token
> = create_async_event_handler (remote_async_inferior_event_handler,
> NULL);
> - remote_notif_register_async_event_handler ();
> + rs->notif_state = remote_notif_state ();
> + remote_notif_register_async_event_handler (rs->notif_state);
>
> /* Reset the target state; these things will be queried either by
> remote_query_supported or as they are needed. */
> @@ -4931,7 +4935,7 @@ remote_resume (struct target_ops *ops,
> before resuming inferior, because inferior was stopped and no RSP
> traffic at that moment. */
> if (!non_stop)
> - remote_notif_process (¬if_client_stop);
> + remote_notif_process (rs->notif_state, ¬if_client_stop);
This means there's only one notif_client_stop.pending_event for all
rs's. That doesn't look right. If then intend was to still have only
one global pending_event for all connections, then we'd need to also
record which remote_state->notif_state does the pending event below to?
>
> rs->last_sent_signal = siggnal;
> rs->last_sent_step = step;
> @@ -7395,7 +7399,7 @@ putpkt_binary (char *buf, int cnt)
> str);
> do_cleanups (old_chain);
> }
> - handle_notification (rs->buf);
> + handle_notification (rs->notif_state, rs->buf);
> /* We're in sync now, rewait for the ack. */
> tcount = 0;
> }
> @@ -7781,7 +7785,7 @@ getpkt_or_notif_sane_1 (char **buf, long *sizeof_buf, int forever,
> if (is_notif != NULL)
> *is_notif = 1;
>
> - handle_notification (*buf);
> + handle_notification (rs->notif_state, *buf);
>
> /* Notifications require no acknowledgement. */
>
>
--
Pedro Alves
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 1/6] Move notif_queue to remote_state
2013-09-25 16:12 ` Pedro Alves
@ 2013-09-30 7:34 ` Yao Qi
2013-09-30 7:58 ` Move pending_event to remote_notif_state ([PATCH 1/6] Move notif_queue to remote_state) Yao Qi
2013-09-30 17:08 ` [PATCH 1/6] Move notif_queue to remote_state Pedro Alves
0 siblings, 2 replies; 23+ messages in thread
From: Yao Qi @ 2013-09-30 7:34 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
On 09/26/2013 12:12 AM, Pedro Alves wrote:
>> void
>> >-remote_notif_register_async_event_handler (void)
>> >+remote_notif_register_async_event_handler (struct remote_notif_state *state)
>> > {
>> > remote_async_get_pending_events_token
>> > = create_async_event_handler (remote_async_get_pending_events_handler,
>> >- NULL);
>> >+ state);
> As soon as we call this a second time, we overwrite the previous token.
> What's the plan here? Shouldn't we move the token to the remote_notif_state too?
>
I thought of this, but can't figure the situation clearly that GDB has
multiple remote connections in the future and each of them has a token.
Probably, GDB works. Let us move it to remote_notif_state too. The
patch below is to move notif_queue and
remote_async_get_pending_events_token to remote_notif_state.
>> >@@ -4931,7 +4935,7 @@ remote_resume (struct target_ops *ops,
>> > before resuming inferior, because inferior was stopped and no RSP
>> > traffic at that moment. */
>> > if (!non_stop)
>> >- remote_notif_process (¬if_client_stop);
>> >+ remote_notif_process (rs->notif_state, ¬if_client_stop);
> This means there's only one notif_client_stop.pending_event for all
> rs's. That doesn't look right. If then intend was to still have only
> one global pending_event for all connections, then we'd need to also
> record which remote_state->notif_state does the pending event below to?
>
The pending event should be connection-local. I'll post a patch to move
pending event to remote_notif_state too.
Note that there is also a global stop_reply_queue, which is about the
stop replies pulled from the remote target. It should be a global,
because multiple remote connections can contribute to this queue, and GDB
consumes events from this queue. However, GDB discards events in it when
a connection is closed. It is right in single connection, but wrong in
multiple connections. I'll fix it in next step.
--
Yao (é½å°§)
gdb:
2013-09-30 Yao Qi <yao@codesourcery.com>
* remote-notif.c (DECLARE_QUEUE_P): Remove.
(notif_queue): Remove.
(remote_notif_process): Add one parameter 'notif_queue'.
Update comments. Callers update.
(remote_async_get_pending_events_token): Remove.
(remote_notif_register_async_event_handler): Remove.
(remote_notif_unregister_async_event_handler): Remove.
(handle_notification): Add parameter 'notif_queue'. Update
comments. Callers update.
(notif_xfree): Remove.
(remote_notif_state_allocate): New function.
(remote_notif_state_xfree): New function.
(_initialize_notif): Remove code to allocate queue.
* remote-notif.h (DECLARE_QUEUE_P): Moved from remote-notif.c.
(struct remote_notif_state): New.
(handle_notification): Update declaration.
(remote_notif_process): Likewise.
(remote_notif_register_async_event_handler): Remove.
(remote_notif_unregister_async_event_handler): Remove.
(remote_notif_state_allocate): Declare.
(remote_notif_state_xfree): Declare.
* remote.c (struct remote_state) <notif_state>: New field.
(remote_close): Don't call
remote_notif_unregister_async_event_handler. Call
remote_notif_state_xfree.
(remote_open_1): Don't call
remote_notif_register_async_event_handler. Call
remote_notif_state_allocate.
---
gdb/remote-notif.c | 88 +++++++++++++++++++++++----------------------------
gdb/remote-notif.h | 27 +++++++++++++--
gdb/remote.c | 13 +++++---
3 files changed, 71 insertions(+), 57 deletions(-)
diff --git a/gdb/remote-notif.c b/gdb/remote-notif.c
index 0f73a52..0bec097 100644
--- a/gdb/remote-notif.c
+++ b/gdb/remote-notif.c
@@ -91,21 +91,19 @@ remote_notif_parse (struct notif_client *nc, char *buf)
return event;
}
-DECLARE_QUEUE_P (notif_client_p);
DEFINE_QUEUE_P (notif_client_p);
-static QUEUE(notif_client_p) *notif_queue;
-
-/* Process notifications one by one. EXCEPT is not expected in
- the queue. */
+/* Process notifications in in STATE's notification queue one by one.
+ EXCEPT is not expected in the queue. */
void
-remote_notif_process (struct notif_client *except)
+remote_notif_process (struct remote_notif_state *state,
+ struct notif_client *except)
{
- while (!QUEUE_is_empty (notif_client_p, notif_queue))
+ while (!QUEUE_is_empty (notif_client_p, state->notif_queue))
{
struct notif_client *nc = QUEUE_deque (notif_client_p,
- notif_queue);
+ state->notif_queue);
gdb_assert (nc != except);
@@ -118,39 +116,14 @@ static void
remote_async_get_pending_events_handler (gdb_client_data data)
{
gdb_assert (non_stop);
- remote_notif_process (NULL);
-}
-
-/* Asynchronous signal handle registered as event loop source for when
- the remote sent us a notification. The registered callback
- will do a ACK sequence to pull the rest of the events out of
- the remote side into our event queue. */
-
-static struct async_event_handler *remote_async_get_pending_events_token;
-
-/* Register async_event_handler for notification. */
-
-void
-remote_notif_register_async_event_handler (void)
-{
- remote_async_get_pending_events_token
- = create_async_event_handler (remote_async_get_pending_events_handler,
- NULL);
-}
-
-/* Unregister async_event_handler for notification. */
-
-void
-remote_notif_unregister_async_event_handler (void)
-{
- if (remote_async_get_pending_events_token)
- delete_async_event_handler (&remote_async_get_pending_events_token);
+ remote_notif_process (data, NULL);
}
-/* Remote notification handler. */
+/* Remote notification handler. Parse BUF, queue notification and
+ update STATE. */
void
-handle_notification (char *buf)
+handle_notification (struct remote_notif_state *state, char *buf)
{
struct notif_client *nc = NULL;
int i;
@@ -188,7 +161,7 @@ handle_notification (char *buf)
/* Notify the event loop there's a stop reply to acknowledge
and that there may be more events to fetch. */
- QUEUE_enque (notif_client_p, notif_queue, nc);
+ QUEUE_enque (notif_client_p, state->notif_queue, nc);
if (non_stop)
{
/* In non-stop, We mark REMOTE_ASYNC_GET_PENDING_EVENTS_TOKEN
@@ -227,7 +200,7 @@ handle_notification (char *buf)
2.3) <-- T05 thread:2
These pending notifications can be processed later. */
- mark_async_event_handler (remote_async_get_pending_events_token);
+ mark_async_event_handler (state->remote_async_get_pending_events_token);
}
if (notif_debug)
@@ -250,15 +223,36 @@ do_notif_event_xfree (void *arg)
xfree (event);
}
-static void
-notif_xfree (struct notif_client *notif)
+/* Return an allocated remote_notif_state. */
+
+struct remote_notif_state *
+remote_notif_state_allocate (void)
+{
+ struct remote_notif_state *notif_state = xmalloc (sizeof (*notif_state));
+
+ notif_state->notif_queue = QUEUE_alloc (notif_client_p, NULL);
+
+ /* Register async_event_handler for notification. */
+
+ notif_state->remote_async_get_pending_events_token
+ = create_async_event_handler (remote_async_get_pending_events_handler,
+ notif_state);
+
+ return notif_state;
+}
+
+/* Free STATE and its fields. */
+
+void
+remote_notif_state_xfree (struct remote_notif_state *state)
{
- if (notif->pending_event != NULL
- && notif->pending_event->dtr != NULL)
- notif->pending_event->dtr (notif->pending_event);
+ QUEUE_free (notif_client_p, state->notif_queue);
+
+ /* Unregister async_event_handler for notification. */
+ if (state->remote_async_get_pending_events_token != NULL)
+ delete_async_event_handler (&state->remote_async_get_pending_events_token);
- xfree (notif->pending_event);
- xfree (notif);
+ xfree (state);
}
/* -Wmissing-prototypes */
@@ -267,8 +261,6 @@ extern initialize_file_ftype _initialize_notif;
void
_initialize_notif (void)
{
- notif_queue = QUEUE_alloc (notif_client_p, notif_xfree);
-
add_setshow_boolean_cmd ("notification", no_class, ¬if_debug,
_("\
Set debugging of async remote notification."), _("\
diff --git a/gdb/remote-notif.h b/gdb/remote-notif.h
index da4fdea..fbb1b3b 100644
--- a/gdb/remote-notif.h
+++ b/gdb/remote-notif.h
@@ -68,16 +68,35 @@ typedef struct notif_client
struct notif_event *pending_event;
} *notif_client_p;
+DECLARE_QUEUE_P (notif_client_p);
+
+/* State on remote async notification. */
+
+struct remote_notif_state
+{
+ /* Notification queue. */
+ QUEUE(notif_client_p) *notif_queue;
+
+ /* Asynchronous signal handle registered as event loop source for when
+ the remote sent us a notification. The registered callback
+ will do a ACK sequence to pull the rest of the events out of
+ the remote side into our event queue. */
+
+ struct async_event_handler *remote_async_get_pending_events_token;
+};
+
void remote_notif_ack (struct notif_client *nc, char *buf);
struct notif_event *remote_notif_parse (struct notif_client *nc,
char *buf);
-void handle_notification (char *buf);
+void handle_notification (struct remote_notif_state *notif_state,
+ char *buf);
-void remote_notif_register_async_event_handler (void);
-void remote_notif_unregister_async_event_handler (void);
+void remote_notif_process (struct remote_notif_state *state,
+ struct notif_client *except);
+struct remote_notif_state *remote_notif_state_allocate (void);
+void remote_notif_state_xfree (struct remote_notif_state *state);
-void remote_notif_process (struct notif_client *except);
extern struct notif_client notif_client_stop;
extern int notif_debug;
diff --git a/gdb/remote.c b/gdb/remote.c
index 2e116d9..88e1c41 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -425,6 +425,9 @@ struct remote_state
threadref echo_nextthread;
threadref nextthread;
threadref resultthreadlist[MAXTHREADLISTRESULTS];
+
+ /* The state of remote notification. */
+ struct remote_notif_state *notif_state;
};
/* Private data that we'll store in (struct thread_info)->private. */
@@ -3073,7 +3076,7 @@ remote_close (void)
if (remote_async_inferior_event_token)
delete_async_event_handler (&remote_async_inferior_event_token);
- remote_notif_unregister_async_event_handler ();
+ remote_notif_state_xfree (rs->notif_state);
trace_reset_local_state ();
}
@@ -4337,7 +4340,7 @@ remote_open_1 (char *name, int from_tty,
remote_async_inferior_event_token
= create_async_event_handler (remote_async_inferior_event_handler,
NULL);
- remote_notif_register_async_event_handler ();
+ rs->notif_state = remote_notif_state_allocate ();
/* Reset the target state; these things will be queried either by
remote_query_supported or as they are needed. */
@@ -4931,7 +4934,7 @@ remote_resume (struct target_ops *ops,
before resuming inferior, because inferior was stopped and no RSP
traffic at that moment. */
if (!non_stop)
- remote_notif_process (¬if_client_stop);
+ remote_notif_process (rs->notif_state, ¬if_client_stop);
rs->last_sent_signal = siggnal;
rs->last_sent_step = step;
@@ -7361,7 +7364,7 @@ putpkt_binary (char *buf, int cnt)
str);
do_cleanups (old_chain);
}
- handle_notification (rs->buf);
+ handle_notification (rs->notif_state, rs->buf);
/* We're in sync now, rewait for the ack. */
tcount = 0;
}
@@ -7747,7 +7750,7 @@ getpkt_or_notif_sane_1 (char **buf, long *sizeof_buf, int forever,
if (is_notif != NULL)
*is_notif = 1;
- handle_notification (*buf);
+ handle_notification (rs->notif_state, *buf);
/* Notifications require no acknowledgement. */
--
1.7.7.6
^ permalink raw reply [flat|nested] 23+ messages in thread* Move pending_event to remote_notif_state ([PATCH 1/6] Move notif_queue to remote_state)
2013-09-30 7:34 ` Yao Qi
@ 2013-09-30 7:58 ` Yao Qi
2013-09-30 19:34 ` Pedro Alves
2013-09-30 17:08 ` [PATCH 1/6] Move notif_queue to remote_state Pedro Alves
1 sibling, 1 reply; 23+ messages in thread
From: Yao Qi @ 2013-09-30 7:58 UTC (permalink / raw)
To: gdb-patches
On 09/30/2013 03:33 PM, Yao Qi wrote:
> The pending event should be connection-local. I'll post a patch to move
> pending event to remote_notif_state too.
This patch moves pending_event to remote_notif_state. Each remote_notif_state
has an array of pending_event for all notif_client. All pending
events are destroyed in remote_notif_state_xfree. However,
discard_pending_stop_replies release pending event too, so the pending
event of stop notification is released twice, we need some refactor
here. We add a new function discard_pending_stop_replies_in_queue
which only discard events in stop_reply_queue, and let
remote_notif_state_xfree release pending event for all notif_client.
After this change, discard_pending_stop_replies is only used to be
attached to ifnerior_exit observer, so the INF can't be NULL any more.
The NULL checking is removed too.
--
Yao (é½å°§)
gdb:
2013-09-30 Yao Qi <yao@codesourcery.com>
* remote-notif.h (REMOTE_NOTIF_ID): New enum.
(struct notif_client) <pending_event>: Moved
to struct remote_notif_state.
<id>: New field.
(struct remote_notif_state) <pending_event>: New field.
(notif_event_xfree): Declare.
* remote-notif.c (handle_notification): Adjust.
(notif_event_xfree): New function.
(do_notif_event_xfree): Call notif_event_xfree.
(remote_notif_state_allocate): Clear pending_event.
(remote_notif_state_xfree): Call notif_event_xfree to free
each element in field pending_event.
* remote.c (discard_pending_stop_replies): Remove declaration.
(discard_pending_stop_replies_in_queue): Declare.
(remote_close): Call discard_pending_stop_replies_in_queue
instead of discard_pending_stop_replies.
(remote_start_remote): Adjust.
(stop_reply_xfree): Call notif_event_xfree.
(notif_client_stop): Adjust initialization.
(remote_notif_remove_all): Rename it to ...
(remove_stop_reply_for_inferior): ... this. Update comments.
Don't check INF is NULL.
(discard_pending_stop_replies): Return early if notif_state is
NULL. Adjust. Don't check INF is NULL.
(remote_notif_get_pending_events): Adjust.
(discard_pending_stop_replies_in_queue): New function.
(remote_wait_ns): Likewise.
---
gdb/remote-notif.c | 31 +++++++++++++++-----
gdb/remote-notif.h | 28 ++++++++++++++-----
gdb/remote.c | 76 +++++++++++++++++++++++++++++++--------------------
3 files changed, 90 insertions(+), 45 deletions(-)
diff --git a/gdb/remote-notif.c b/gdb/remote-notif.c
index 0bec097..a0225b7 100644
--- a/gdb/remote-notif.c
+++ b/gdb/remote-notif.c
@@ -51,6 +51,8 @@ static struct notif_client *notifs[] =
¬if_client_stop,
};
+gdb_static_assert (ARRAY_SIZE (notifs) == REMOTE_NOTIF_LAST);
+
static void do_notif_event_xfree (void *arg);
/* Parse the BUF for the expected notification NC, and send packet to
@@ -141,7 +143,7 @@ handle_notification (struct remote_notif_state *state, char *buf)
if (nc == NULL)
return;
- if (nc->pending_event)
+ if (state->pending_event[nc->id] != NULL)
{
/* We've already parsed the in-flight reply, but the stub for some
reason thought we didn't, possibly due to timeout on its side.
@@ -157,7 +159,7 @@ handle_notification (struct remote_notif_state *state, char *buf)
/* Be careful to only set it after parsing, since an error
may be thrown then. */
- nc->pending_event = event;
+ state->pending_event[nc->id] = event;
/* Notify the event loop there's a stop reply to acknowledge
and that there may be more events to fetch. */
@@ -210,19 +212,25 @@ handle_notification (struct remote_notif_state *state, char *buf)
}
}
-/* Cleanup wrapper. */
+/* Invoke destructor of EVENT and xfree it. */
-static void
-do_notif_event_xfree (void *arg)
+void
+notif_event_xfree (struct notif_event *event)
{
- struct notif_event *event = arg;
-
- if (event && event->dtr)
+ if (event != NULL && event->dtr != NULL)
event->dtr (event);
xfree (event);
}
+/* Cleanup wrapper. */
+
+static void
+do_notif_event_xfree (void *arg)
+{
+ notif_event_xfree (arg);
+}
+
/* Return an allocated remote_notif_state. */
struct remote_notif_state *
@@ -238,6 +246,8 @@ remote_notif_state_allocate (void)
= create_async_event_handler (remote_async_get_pending_events_handler,
notif_state);
+ memset (notif_state->pending_event, 0, sizeof (notif_state->pending_event));
+
return notif_state;
}
@@ -246,12 +256,17 @@ remote_notif_state_allocate (void)
void
remote_notif_state_xfree (struct remote_notif_state *state)
{
+ int i;
+
QUEUE_free (notif_client_p, state->notif_queue);
/* Unregister async_event_handler for notification. */
if (state->remote_async_get_pending_events_token != NULL)
delete_async_event_handler (&state->remote_async_get_pending_events_token);
+ for (i = 0; i < REMOTE_NOTIF_LAST; i++)
+ notif_event_xfree (state->pending_event[i]);
+
xfree (state);
}
diff --git a/gdb/remote-notif.h b/gdb/remote-notif.h
index fbb1b3b..6c8f60b 100644
--- a/gdb/remote-notif.h
+++ b/gdb/remote-notif.h
@@ -31,6 +31,14 @@ struct notif_event
void (*dtr) (struct notif_event *self);
};
+/* ID of the notif_client. */
+
+enum REMOTE_NOTIF_ID
+{
+ REMOTE_NOTIF_STOP = 0,
+ REMOTE_NOTIF_LAST,
+};
+
/* A client to a sort of async remote notification. */
typedef struct notif_client
@@ -59,13 +67,8 @@ typedef struct notif_client
/* Allocate an event. */
struct notif_event *(*alloc_event) (void);
- /* One pending event. This is where we keep it until it is
- acknowledged. When there is a notification packet, parse it,
- and create an object of 'struct notif_event' to assign to
- it. This field is unchanged until GDB starts to ack this
- notification (which is done by
- remote.c:remote_notif_pending_replies). */
- struct notif_event *pending_event;
+ /* Id of this notif_client. */
+ const enum REMOTE_NOTIF_ID id;
} *notif_client_p;
DECLARE_QUEUE_P (notif_client_p);
@@ -83,12 +86,23 @@ struct remote_notif_state
the remote side into our event queue. */
struct async_event_handler *remote_async_get_pending_events_token;
+
+/* One pending event for each notification client. This is where we
+ keep it until it is acknowledged. When there is a notification
+ packet, parse it, and create an object of 'struct notif_event' to
+ assign to it. This field is unchanged until GDB starts to ack
+ this notification (which is done by
+ remote.c:remote_notif_pending_replies). */
+
+ struct notif_event *pending_event[REMOTE_NOTIF_LAST];
};
void remote_notif_ack (struct notif_client *nc, char *buf);
struct notif_event *remote_notif_parse (struct notif_client *nc,
char *buf);
+void notif_event_xfree (struct notif_event *event);
+
void handle_notification (struct remote_notif_state *notif_state,
char *buf);
diff --git a/gdb/remote.c b/gdb/remote.c
index 6ac3f51..8a07c97 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -220,7 +220,7 @@ struct stop_reply;
static void stop_reply_xfree (struct stop_reply *);
static void remote_parse_stop_reply (char *, struct stop_reply *);
static void push_stop_reply (struct stop_reply *);
-static void discard_pending_stop_replies (struct inferior *);
+static void discard_pending_stop_replies_in_queue (void);
static int peek_stop_reply (ptid_t ptid);
static void remote_async_inferior_event_handler (gdb_client_data);
@@ -3067,11 +3067,9 @@ remote_close (void)
inferior_ptid = null_ptid;
discard_all_inferiors ();
- /* Stop replies may from inferiors which are still unknown to GDB.
- We are closing the remote target, so we should discard
- everything, including the stop replies from GDB-unknown
- inferiors. */
- discard_pending_stop_replies (NULL);
+ /* We are closing the remote target, so we should discard
+ everything of this target. */
+ discard_pending_stop_replies_in_queue ();
if (remote_async_inferior_event_token)
delete_async_event_handler (&remote_async_inferior_event_token);
@@ -3572,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. */
- notif_client_stop.pending_event
+ rs->notif_state->pending_event[notif_client_stop.id]
= remote_notif_parse (notif, rs->buf);
remote_notif_get_pending_events (notif);
@@ -5304,11 +5302,7 @@ static QUEUE (stop_reply_p) *stop_reply_queue;
static void
stop_reply_xfree (struct stop_reply *r)
{
- if (r != NULL)
- {
- VEC_free (cached_reg_t, r->regcache);
- xfree (r);
- }
+ notif_event_xfree ((struct notif_event *) r);
}
static void
@@ -5375,7 +5369,7 @@ struct notif_client notif_client_stop =
remote_notif_stop_ack,
remote_notif_stop_can_get_pending_events,
remote_notif_stop_alloc_reply,
- NULL,
+ REMOTE_NOTIF_STOP,
};
/* A parameter to pass data in and out. */
@@ -5386,18 +5380,19 @@ struct queue_iter_param
struct stop_reply *output;
};
-/* Remove all queue elements meet the condition it checks. */
+/* Remove stop replies in the queue if its pid is equal to the given
+ inferior's pid. */
static int
-remote_notif_remove_all (QUEUE (stop_reply_p) *q,
- QUEUE_ITER (stop_reply_p) *iter,
- stop_reply_p event,
- void *data)
+remove_stop_reply_for_inferior (QUEUE (stop_reply_p) *q,
+ QUEUE_ITER (stop_reply_p) *iter,
+ stop_reply_p event,
+ void *data)
{
struct queue_iter_param *param = data;
struct inferior *inf = param->input;
- if (inf == NULL || ptid_get_pid (event->ptid) == inf->pid)
+ if (ptid_get_pid (event->ptid) == inf->pid)
{
stop_reply_xfree (event);
QUEUE_remove_elem (stop_reply_p, q, iter);
@@ -5414,16 +5409,22 @@ discard_pending_stop_replies (struct inferior *inf)
{
int i;
struct queue_iter_param param;
- struct stop_reply *reply
- = (struct stop_reply *) notif_client_stop.pending_event;
+ struct stop_reply *reply;
+ struct remote_state *rs = get_remote_state ();
+ struct remote_notif_state *rns = rs->notif_state;
+
+ /* This function can be notified when an inferior exists. When the
+ target is not remote, the notification state is NULL. */
+ if (rns == NULL)
+ return;
+
+ reply = (struct stop_reply *) rns->pending_event[notif_client_stop.id];
/* Discard the in-flight notification. */
- if (reply != NULL
- && (inf == NULL
- || ptid_get_pid (reply->ptid) == inf->pid))
+ if (reply != NULL && ptid_get_pid (reply->ptid) == inf->pid)
{
stop_reply_xfree (reply);
- notif_client_stop.pending_event = NULL;
+ rns->pending_event[notif_client_stop.id] = NULL;
}
param.input = inf;
@@ -5431,7 +5432,22 @@ discard_pending_stop_replies (struct inferior *inf)
/* Discard the stop replies we have already pulled with
vStopped. */
QUEUE_iterate (stop_reply_p, stop_reply_queue,
- remote_notif_remove_all, ¶m);
+ remove_stop_reply_for_inferior, ¶m);
+}
+
+/* Discard the stop replies in stop_reply_queue. */
+
+static void
+discard_pending_stop_replies_in_queue (void)
+{
+ struct queue_iter_param param;
+
+ param.input = NULL;
+ param.output = NULL;
+ /* Discard the stop replies we have already pulled with
+ vStopped. */
+ QUEUE_iterate (stop_reply_p, stop_reply_queue,
+ remove_stop_reply_for_inferior, ¶m);
}
/* A parameter to pass data in and out. */
@@ -5787,7 +5803,7 @@ remote_notif_get_pending_events (struct notif_client *nc)
{
struct remote_state *rs = get_remote_state ();
- if (nc->pending_event)
+ if (rs->notif_state->pending_event[nc->id] != NULL)
{
if (notif_debug)
fprintf_unfiltered (gdb_stdlog,
@@ -5795,8 +5811,8 @@ remote_notif_get_pending_events (struct notif_client *nc)
nc->name);
/* acknowledge */
- nc->ack (nc, rs->buf, nc->pending_event);
- nc->pending_event = NULL;
+ nc->ack (nc, rs->buf, rs->notif_state->pending_event[nc->id]);
+ rs->notif_state->pending_event[nc->id] = NULL;
while (1)
{
@@ -5901,7 +5917,7 @@ remote_wait_ns (ptid_t ptid, struct target_waitstatus *status, int options)
/* Acknowledge a pending stop reply that may have arrived in the
mean time. */
- if (notif_client_stop.pending_event != NULL)
+ if (rs->notif_state->pending_event[notif_client_stop.id] != NULL)
remote_notif_get_pending_events (¬if_client_stop);
/* If indeed we noticed a stop reply, we're done. */
--
1.7.7.6
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: Move pending_event to remote_notif_state ([PATCH 1/6] Move notif_queue to remote_state)
2013-09-30 7:58 ` Move pending_event to remote_notif_state ([PATCH 1/6] Move notif_queue to remote_state) Yao Qi
@ 2013-09-30 19:34 ` Pedro Alves
2013-10-04 7:42 ` Yao Qi
0 siblings, 1 reply; 23+ messages in thread
From: Pedro Alves @ 2013-09-30 19:34 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
Eh, sorry, somehow I only noticed this patch now.
(Still, I'd have preferred if the previous patch didn't
create an intermediate state that leaks.)
Anyway...
On 09/30/2013 08:56 AM, Yao Qi wrote:
> /* Return an allocated remote_notif_state. */
>
> struct remote_notif_state *
> @@ -238,6 +246,8 @@ remote_notif_state_allocate (void)
> = create_async_event_handler (remote_async_get_pending_events_handler,
> notif_state);
>
> + memset (notif_state->pending_event, 0, sizeof (notif_state->pending_event));
Could use xzalloc to begin with. Might be more future proof.
> @@ -5414,16 +5409,22 @@ discard_pending_stop_replies (struct inferior *inf)
> {
> int i;
> struct queue_iter_param param;
> - struct stop_reply *reply
> - = (struct stop_reply *) notif_client_stop.pending_event;
> + struct stop_reply *reply;
> + struct remote_state *rs = get_remote_state ();
> + struct remote_notif_state *rns = rs->notif_state;
> +
> + /* This function can be notified when an inferior exists. When the
> + target is not remote, the notification state is NULL. */
> + if (rns == NULL)
> + return;
I'd rather we use the same "target is remote" predicate we use everywhere else:
if (rs->remote_desc == NULL)
return;
Otherwise this looks fine.
Thanks,
--
Pedro Alves
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: Move pending_event to remote_notif_state ([PATCH 1/6] Move notif_queue to remote_state)
2013-09-30 19:34 ` Pedro Alves
@ 2013-10-04 7:42 ` Yao Qi
0 siblings, 0 replies; 23+ messages in thread
From: Yao Qi @ 2013-10-04 7:42 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
On 10/01/2013 03:34 AM, Pedro Alves wrote:
>> >@@ -5414,16 +5409,22 @@ discard_pending_stop_replies (struct inferior *inf)
>> > {
>> > int i;
>> > struct queue_iter_param param;
>> >- struct stop_reply *reply
>> >- = (struct stop_reply *) notif_client_stop.pending_event;
>> >+ struct stop_reply *reply;
>> >+ struct remote_state *rs = get_remote_state ();
>> >+ struct remote_notif_state *rns = rs->notif_state;
>> >+
>> >+ /* This function can be notified when an inferior exists. When the
>> >+ target is not remote, the notification state is NULL. */
>> >+ if (rns == NULL)
>> >+ return;
> I'd rather we use the same "target is remote" predicate we use everywhere else:
>
> if (rs->remote_desc == NULL)
> return;
Fixed.
>
> Otherwise this looks fine.
Patch below is committed. Thanks for the review, Pedro.
--
Yao (é½å°§)
gdb:
2013-10-04 Yao Qi <yao@codesourcery.com>
* remote-notif.h (REMOTE_NOTIF_ID): New enum.
(struct notif_client) <pending_event>: Moved
to struct remote_notif_state.
<id>: New field.
(struct remote_notif_state) <pending_event>: New field.
(notif_event_xfree): Declare.
* remote-notif.c (handle_notification): Adjust.
(notif_event_xfree): New function.
(do_notif_event_xfree): Call notif_event_xfree.
(remote_notif_state_xfree): Call notif_event_xfree to free
each element in field pending_event.
* remote.c (discard_pending_stop_replies): Remove declaration.
(discard_pending_stop_replies_in_queue): Declare.
(remote_close): Call discard_pending_stop_replies_in_queue
instead of discard_pending_stop_replies.
(remote_start_remote): Adjust.
(stop_reply_xfree): Call notif_event_xfree.
(notif_client_stop): Adjust initialization.
(remote_notif_remove_all): Rename it to ...
(remove_stop_reply_for_inferior): ... this. Update comments.
Don't check INF is NULL.
(discard_pending_stop_replies): Return early if notif_state is
NULL. Adjust. Don't check INF is NULL.
(remote_notif_get_pending_events): Adjust.
(discard_pending_stop_replies_in_queue): New function.
(remote_wait_ns): Likewise.
---
gdb/remote-notif.c | 29 ++++++++++++++-----
gdb/remote-notif.h | 28 ++++++++++++++----
gdb/remote.c | 79 +++++++++++++++++++++++++++++++---------------------
3 files changed, 89 insertions(+), 47 deletions(-)
diff --git a/gdb/remote-notif.c b/gdb/remote-notif.c
index 905b8a1..0d59279 100644
--- a/gdb/remote-notif.c
+++ b/gdb/remote-notif.c
@@ -51,6 +51,8 @@ static struct notif_client *notifs[] =
¬if_client_stop,
};
+gdb_static_assert (ARRAY_SIZE (notifs) == REMOTE_NOTIF_LAST);
+
static void do_notif_event_xfree (void *arg);
/* Parse the BUF for the expected notification NC, and send packet to
@@ -141,7 +143,7 @@ handle_notification (struct remote_notif_state *state, char *buf)
if (nc == NULL)
return;
- if (nc->pending_event)
+ if (state->pending_event[nc->id] != NULL)
{
/* We've already parsed the in-flight reply, but the stub for some
reason thought we didn't, possibly due to timeout on its side.
@@ -157,7 +159,7 @@ handle_notification (struct remote_notif_state *state, char *buf)
/* Be careful to only set it after parsing, since an error
may be thrown then. */
- nc->pending_event = event;
+ state->pending_event[nc->id] = event;
/* Notify the event loop there's a stop reply to acknowledge
and that there may be more events to fetch. */
@@ -210,19 +212,25 @@ handle_notification (struct remote_notif_state *state, char *buf)
}
}
-/* Cleanup wrapper. */
+/* Invoke destructor of EVENT and xfree it. */
-static void
-do_notif_event_xfree (void *arg)
+void
+notif_event_xfree (struct notif_event *event)
{
- struct notif_event *event = arg;
-
- if (event && event->dtr)
+ if (event != NULL && event->dtr != NULL)
event->dtr (event);
xfree (event);
}
+/* Cleanup wrapper. */
+
+static void
+do_notif_event_xfree (void *arg)
+{
+ notif_event_xfree (arg);
+}
+
/* Return an allocated remote_notif_state. */
struct remote_notif_state *
@@ -246,12 +254,17 @@ remote_notif_state_allocate (void)
void
remote_notif_state_xfree (struct remote_notif_state *state)
{
+ int i;
+
QUEUE_free (notif_client_p, state->notif_queue);
/* Unregister async_event_handler for notification. */
if (state->get_pending_events_token != NULL)
delete_async_event_handler (&state->get_pending_events_token);
+ for (i = 0; i < REMOTE_NOTIF_LAST; i++)
+ notif_event_xfree (state->pending_event[i]);
+
xfree (state);
}
diff --git a/gdb/remote-notif.h b/gdb/remote-notif.h
index 4c3617e..e5ebbc7 100644
--- a/gdb/remote-notif.h
+++ b/gdb/remote-notif.h
@@ -31,6 +31,14 @@ struct notif_event
void (*dtr) (struct notif_event *self);
};
+/* ID of the notif_client. */
+
+enum REMOTE_NOTIF_ID
+{
+ REMOTE_NOTIF_STOP = 0,
+ REMOTE_NOTIF_LAST,
+};
+
/* A client to a sort of async remote notification. */
typedef struct notif_client
@@ -59,13 +67,8 @@ typedef struct notif_client
/* Allocate an event. */
struct notif_event *(*alloc_event) (void);
- /* One pending event. This is where we keep it until it is
- acknowledged. When there is a notification packet, parse it,
- and create an object of 'struct notif_event' to assign to
- it. This field is unchanged until GDB starts to ack this
- notification (which is done by
- remote.c:remote_notif_pending_replies). */
- struct notif_event *pending_event;
+ /* Id of this notif_client. */
+ const enum REMOTE_NOTIF_ID id;
} *notif_client_p;
DECLARE_QUEUE_P (notif_client_p);
@@ -84,12 +87,23 @@ struct remote_notif_state
the remote side into our event queue. */
struct async_event_handler *get_pending_events_token;
+
+/* One pending event for each notification client. This is where we
+ keep it until it is acknowledged. When there is a notification
+ packet, parse it, and create an object of 'struct notif_event' to
+ assign to it. This field is unchanged until GDB starts to ack
+ this notification (which is done by
+ remote.c:remote_notif_pending_replies). */
+
+ struct notif_event *pending_event[REMOTE_NOTIF_LAST];
};
void remote_notif_ack (struct notif_client *nc, char *buf);
struct notif_event *remote_notif_parse (struct notif_client *nc,
char *buf);
+void notif_event_xfree (struct notif_event *event);
+
void handle_notification (struct remote_notif_state *notif_state,
char *buf);
diff --git a/gdb/remote.c b/gdb/remote.c
index 6ac3f51..60086d2 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -220,7 +220,7 @@ struct stop_reply;
static void stop_reply_xfree (struct stop_reply *);
static void remote_parse_stop_reply (char *, struct stop_reply *);
static void push_stop_reply (struct stop_reply *);
-static void discard_pending_stop_replies (struct inferior *);
+static void discard_pending_stop_replies_in_queue (void);
static int peek_stop_reply (ptid_t ptid);
static void remote_async_inferior_event_handler (gdb_client_data);
@@ -3067,11 +3067,9 @@ remote_close (void)
inferior_ptid = null_ptid;
discard_all_inferiors ();
- /* Stop replies may from inferiors which are still unknown to GDB.
- We are closing the remote target, so we should discard
- everything, including the stop replies from GDB-unknown
- inferiors. */
- discard_pending_stop_replies (NULL);
+ /* We are closing the remote target, so we should discard
+ everything of this target. */
+ discard_pending_stop_replies_in_queue ();
if (remote_async_inferior_event_token)
delete_async_event_handler (&remote_async_inferior_event_token);
@@ -3572,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. */
- notif_client_stop.pending_event
+ rs->notif_state->pending_event[notif_client_stop.id]
= remote_notif_parse (notif, rs->buf);
remote_notif_get_pending_events (notif);
@@ -5304,11 +5302,7 @@ static QUEUE (stop_reply_p) *stop_reply_queue;
static void
stop_reply_xfree (struct stop_reply *r)
{
- if (r != NULL)
- {
- VEC_free (cached_reg_t, r->regcache);
- xfree (r);
- }
+ notif_event_xfree ((struct notif_event *) r);
}
static void
@@ -5375,7 +5369,7 @@ struct notif_client notif_client_stop =
remote_notif_stop_ack,
remote_notif_stop_can_get_pending_events,
remote_notif_stop_alloc_reply,
- NULL,
+ REMOTE_NOTIF_STOP,
};
/* A parameter to pass data in and out. */
@@ -5386,18 +5380,19 @@ struct queue_iter_param
struct stop_reply *output;
};
-/* Remove all queue elements meet the condition it checks. */
+/* Remove stop replies in the queue if its pid is equal to the given
+ inferior's pid. */
static int
-remote_notif_remove_all (QUEUE (stop_reply_p) *q,
- QUEUE_ITER (stop_reply_p) *iter,
- stop_reply_p event,
- void *data)
+remove_stop_reply_for_inferior (QUEUE (stop_reply_p) *q,
+ QUEUE_ITER (stop_reply_p) *iter,
+ stop_reply_p event,
+ void *data)
{
struct queue_iter_param *param = data;
struct inferior *inf = param->input;
- if (inf == NULL || ptid_get_pid (event->ptid) == inf->pid)
+ if (ptid_get_pid (event->ptid) == inf->pid)
{
stop_reply_xfree (event);
QUEUE_remove_elem (stop_reply_p, q, iter);
@@ -5406,24 +5401,29 @@ remote_notif_remove_all (QUEUE (stop_reply_p) *q,
return 1;
}
-/* Discard all pending stop replies of inferior INF. If INF is NULL,
- discard everything. */
+/* Discard all pending stop replies of inferior INF. */
static void
discard_pending_stop_replies (struct inferior *inf)
{
int i;
struct queue_iter_param param;
- struct stop_reply *reply
- = (struct stop_reply *) notif_client_stop.pending_event;
+ struct stop_reply *reply;
+ struct remote_state *rs = get_remote_state ();
+ struct remote_notif_state *rns = rs->notif_state;
+
+ /* This function can be notified when an inferior exists. When the
+ target is not remote, the notification state is NULL. */
+ if (rs->remote_desc == NULL)
+ return;
+
+ reply = (struct stop_reply *) rns->pending_event[notif_client_stop.id];
/* Discard the in-flight notification. */
- if (reply != NULL
- && (inf == NULL
- || ptid_get_pid (reply->ptid) == inf->pid))
+ if (reply != NULL && ptid_get_pid (reply->ptid) == inf->pid)
{
stop_reply_xfree (reply);
- notif_client_stop.pending_event = NULL;
+ rns->pending_event[notif_client_stop.id] = NULL;
}
param.input = inf;
@@ -5431,7 +5431,22 @@ discard_pending_stop_replies (struct inferior *inf)
/* Discard the stop replies we have already pulled with
vStopped. */
QUEUE_iterate (stop_reply_p, stop_reply_queue,
- remote_notif_remove_all, ¶m);
+ remove_stop_reply_for_inferior, ¶m);
+}
+
+/* Discard the stop replies in stop_reply_queue. */
+
+static void
+discard_pending_stop_replies_in_queue (void)
+{
+ struct queue_iter_param param;
+
+ param.input = NULL;
+ param.output = NULL;
+ /* Discard the stop replies we have already pulled with
+ vStopped. */
+ QUEUE_iterate (stop_reply_p, stop_reply_queue,
+ remove_stop_reply_for_inferior, ¶m);
}
/* A parameter to pass data in and out. */
@@ -5787,7 +5802,7 @@ remote_notif_get_pending_events (struct notif_client *nc)
{
struct remote_state *rs = get_remote_state ();
- if (nc->pending_event)
+ if (rs->notif_state->pending_event[nc->id] != NULL)
{
if (notif_debug)
fprintf_unfiltered (gdb_stdlog,
@@ -5795,8 +5810,8 @@ remote_notif_get_pending_events (struct notif_client *nc)
nc->name);
/* acknowledge */
- nc->ack (nc, rs->buf, nc->pending_event);
- nc->pending_event = NULL;
+ nc->ack (nc, rs->buf, rs->notif_state->pending_event[nc->id]);
+ rs->notif_state->pending_event[nc->id] = NULL;
while (1)
{
@@ -5901,7 +5916,7 @@ remote_wait_ns (ptid_t ptid, struct target_waitstatus *status, int options)
/* Acknowledge a pending stop reply that may have arrived in the
mean time. */
- if (notif_client_stop.pending_event != NULL)
+ if (rs->notif_state->pending_event[notif_client_stop.id] != NULL)
remote_notif_get_pending_events (¬if_client_stop);
/* If indeed we noticed a stop reply, we're done. */
--
1.7.7.6
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/6] Move notif_queue to remote_state
2013-09-30 7:34 ` Yao Qi
2013-09-30 7:58 ` Move pending_event to remote_notif_state ([PATCH 1/6] Move notif_queue to remote_state) Yao Qi
@ 2013-09-30 17:08 ` Pedro Alves
2013-10-01 14:08 ` Yao Qi
2013-10-02 1:54 ` Yao Qi
1 sibling, 2 replies; 23+ messages in thread
From: Pedro Alves @ 2013-09-30 17:08 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On 09/30/2013 08:33 AM, Yao Qi wrote:
> -/* Process notifications one by one. EXCEPT is not expected in
> - the queue. */
> +/* Process notifications in in STATE's notification queue one by one.
double "in"
> + EXCEPT is not expected in the queue. */
> void
> -remote_notif_process (struct notif_client *except)
> +remote_notif_process (struct remote_notif_state *state,
> + struct notif_client *except)
> {
> +DECLARE_QUEUE_P (notif_client_p);
> +
> +/* State on remote async notification. */
> +
> +struct remote_notif_state
> +{
> + /* Notification queue. */
> + QUEUE(notif_client_p) *notif_queue;
> +
> + /* Asynchronous signal handle registered as event loop source for when
> + the remote sent us a notification. The registered callback
> + will do a ACK sequence to pull the rest of the events out of
> + the remote side into our event queue. */
> +
> + struct async_event_handler *remote_async_get_pending_events_token;
Inconsistent "empty line between describing comment and field".
> -static void
> -notif_xfree (struct notif_client *notif)
...
> - if (notif->pending_event != NULL
> - && notif->pending_event->dtr != NULL)
> - notif->pending_event->dtr (notif->pending_event);
Hmm, it's not clear to me from reading the patch where is
pending_event released after this patch. Could you help
me understand that?
--
Pedro Alves
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 1/6] Move notif_queue to remote_state
2013-09-30 17:08 ` [PATCH 1/6] Move notif_queue to remote_state Pedro Alves
@ 2013-10-01 14:08 ` Yao Qi
2013-10-02 1:54 ` Yao Qi
1 sibling, 0 replies; 23+ messages in thread
From: Yao Qi @ 2013-10-01 14:08 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
On 10/01/2013 01:08 AM, Pedro Alves wrote:
>> -static void
>> >-notif_xfree (struct notif_client *notif)
> ...
>> >- if (notif->pending_event != NULL
>> >- && notif->pending_event->dtr != NULL)
>> >- notif->pending_event->dtr (notif->pending_event);
> Hmm, it's not clear to me from reading the patch where is
> pending_event released after this patch. Could you help
> me understand that?
Since notif_queue is a global variable, we've never used QUEUE_free to
free notif_queue, so notif_xfree is never called, and pending_event on
each notif_client is not released.
When we move notif_queue to 'remote_notif_state', we have to explicitly
use QUEUE_free to free notif_queue and elements in it. We put the
pointer of each notif_client to notif_queue, so we don't need a function
to free the notif_client when notif_queue is destroyed. So the
notif_queue is allocated like this,
notif_state->notif_queue = QUEUE_alloc (notif_client_p, NULL);
and notif_xfree is not needed any more. The leak on pending_event is
still there, but it is fixed by the next patch, which moves
pending_event to remote_notif_state, and pending_event is freed in
remote_notif_state_xfree.
--
Yao (é½å°§)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/6] Move notif_queue to remote_state
2013-09-30 17:08 ` [PATCH 1/6] Move notif_queue to remote_state Pedro Alves
2013-10-01 14:08 ` Yao Qi
@ 2013-10-02 1:54 ` Yao Qi
2013-10-02 10:48 ` Pedro Alves
1 sibling, 1 reply; 23+ messages in thread
From: Yao Qi @ 2013-10-02 1:54 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
On 10/01/2013 01:08 AM, Pedro Alves wrote:
>> -static void
>> >-notif_xfree (struct notif_client *notif)
> ...
>> >- if (notif->pending_event != NULL
>> >- && notif->pending_event->dtr != NULL)
>> >- notif->pending_event->dtr (notif->pending_event);
> Hmm, it's not clear to me from reading the patch where is
> pending_event released after this patch. Could you help
> me understand that?
Since notif_queue is a global variable, we've never used QUEUE_free to
free notif_queue, so notif_xfree is never called, and pending_event on
each notif_client is not released.
When we move notif_queue to 'remote_notif_state', we have to explicitly
use QUEUE_free to free notif_queue and elements in it. We put the
pointer of each notif_client to notif_queue, so we don't need a function
to free the notif_client when notif_queue is destroyed. So the
notif_queue is allocated like this,
notif_state->notif_queue = QUEUE_alloc (notif_client_p, NULL);
and notif_xfree is not needed any more. The leak on pending_event is
still there, but it is fixed by the next patch, which moves
pending_event to remote_notif_state, and pending_event is freed in
remote_notif_state_xfree.
--
Yao (é½å°§)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/6] Move notif_queue to remote_state
2013-10-02 1:54 ` Yao Qi
@ 2013-10-02 10:48 ` Pedro Alves
2013-10-04 7:36 ` Yao Qi
0 siblings, 1 reply; 23+ messages in thread
From: Pedro Alves @ 2013-10-02 10:48 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On 10/02/2013 02:52 AM, Yao Qi wrote:
> On 10/01/2013 01:08 AM, Pedro Alves wrote:
>>> -static void
>>>> -notif_xfree (struct notif_client *notif)
>> ...
>>>> - if (notif->pending_event != NULL
>>>> - && notif->pending_event->dtr != NULL)
>>>> - notif->pending_event->dtr (notif->pending_event);
>> Hmm, it's not clear to me from reading the patch where is
>> pending_event released after this patch. Could you help
>> me understand that?
>
> Since notif_queue is a global variable, we've never used QUEUE_free to
> free notif_queue, so notif_xfree is never called, and pending_event on
> each notif_client is not released.
>
> When we move notif_queue to 'remote_notif_state', we have to explicitly
> use QUEUE_free to free notif_queue and elements in it. We put the
> pointer of each notif_client to notif_queue, so we don't need a function
> to free the notif_client when notif_queue is destroyed. So the
> notif_queue is allocated like this,
>
> notif_state->notif_queue = QUEUE_alloc (notif_client_p, NULL);
>
> and notif_xfree is not needed any more. The leak on pending_event is
> still there, but it is fixed by the next patch, which moves
> pending_event to remote_notif_state, and pending_event is freed in
> remote_notif_state_xfree.
I see now, thanks.
BTW:
+struct remote_notif_state
+{
+ /* Notification queue. */
+ QUEUE(notif_client_p) *notif_queue;
+
+ /* Asynchronous signal handle registered as event loop source for when
+ the remote sent us a notification. The registered callback
+ will do a ACK sequence to pull the rest of the events out of
+ the remote side into our event queue. */
+
+ struct async_event_handler *remote_async_get_pending_events_token;
Since remote_async_get_pending_events_token is no longer a global,
it doesn't need the "remote_" prefix (or even remote_async_?) anymore.
WDYT?
The patch is OK with or without that change.
Thanks,
--
Pedro Alves
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 1/6] Move notif_queue to remote_state
2013-10-02 10:48 ` Pedro Alves
@ 2013-10-04 7:36 ` Yao Qi
0 siblings, 0 replies; 23+ messages in thread
From: Yao Qi @ 2013-10-04 7:36 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
On 10/02/2013 06:48 PM, Pedro Alves wrote:
> Since remote_async_get_pending_events_token is no longer a global,
> it doesn't need the "remote_" prefix (or even remote_async_?) anymore.
> WDYT?
Yeah, I agree. I thought of this renaming during writing the patch,
but I didn't do that to keep the patch simple.
>
> The patch is OK with or without that change.
Patch below is what I committed, with the renaming.
--
Yao (é½å°§)
gdb:
2013-10-04 Yao Qi <yao@codesourcery.com>
* remote-notif.c (DECLARE_QUEUE_P): Remove.
(notif_queue): Remove.
(remote_notif_process): Add one parameter 'notif_queue'.
Update comments. Callers update.
(remote_async_get_pending_events_token): Remove.
(remote_notif_register_async_event_handler): Remove.
(remote_notif_unregister_async_event_handler): Remove.
(handle_notification): Add parameter 'notif_queue'. Update
comments. Callers update.
(notif_xfree): Remove.
(remote_notif_state_allocate): New function.
(remote_notif_state_xfree): New function.
(_initialize_notif): Remove code to allocate queue.
* remote-notif.h (DECLARE_QUEUE_P): Moved from remote-notif.c.
(struct remote_notif_state): New.
(handle_notification): Update declaration.
(remote_notif_process): Likewise.
(remote_notif_register_async_event_handler): Remove.
(remote_notif_unregister_async_event_handler): Remove.
(remote_notif_state_allocate): Declare.
(remote_notif_state_xfree): Declare.
* remote.c (struct remote_state) <notif_state>: New field.
(remote_close): Don't call
remote_notif_unregister_async_event_handler. Call
remote_notif_state_xfree.
(remote_open_1): Don't call
remote_notif_register_async_event_handler. Call
remote_notif_state_allocate.
---
gdb/remote-notif.c | 88 +++++++++++++++++++++++----------------------------
gdb/remote-notif.h | 28 ++++++++++++++--
gdb/remote.c | 13 +++++---
3 files changed, 72 insertions(+), 57 deletions(-)
diff --git a/gdb/remote-notif.c b/gdb/remote-notif.c
index 0f73a52..905b8a1 100644
--- a/gdb/remote-notif.c
+++ b/gdb/remote-notif.c
@@ -91,21 +91,19 @@ remote_notif_parse (struct notif_client *nc, char *buf)
return event;
}
-DECLARE_QUEUE_P (notif_client_p);
DEFINE_QUEUE_P (notif_client_p);
-static QUEUE(notif_client_p) *notif_queue;
-
-/* Process notifications one by one. EXCEPT is not expected in
- the queue. */
+/* Process notifications in STATE's notification queue one by one.
+ EXCEPT is not expected in the queue. */
void
-remote_notif_process (struct notif_client *except)
+remote_notif_process (struct remote_notif_state *state,
+ struct notif_client *except)
{
- while (!QUEUE_is_empty (notif_client_p, notif_queue))
+ while (!QUEUE_is_empty (notif_client_p, state->notif_queue))
{
struct notif_client *nc = QUEUE_deque (notif_client_p,
- notif_queue);
+ state->notif_queue);
gdb_assert (nc != except);
@@ -118,39 +116,14 @@ static void
remote_async_get_pending_events_handler (gdb_client_data data)
{
gdb_assert (non_stop);
- remote_notif_process (NULL);
-}
-
-/* Asynchronous signal handle registered as event loop source for when
- the remote sent us a notification. The registered callback
- will do a ACK sequence to pull the rest of the events out of
- the remote side into our event queue. */
-
-static struct async_event_handler *remote_async_get_pending_events_token;
-
-/* Register async_event_handler for notification. */
-
-void
-remote_notif_register_async_event_handler (void)
-{
- remote_async_get_pending_events_token
- = create_async_event_handler (remote_async_get_pending_events_handler,
- NULL);
-}
-
-/* Unregister async_event_handler for notification. */
-
-void
-remote_notif_unregister_async_event_handler (void)
-{
- if (remote_async_get_pending_events_token)
- delete_async_event_handler (&remote_async_get_pending_events_token);
+ remote_notif_process (data, NULL);
}
-/* Remote notification handler. */
+/* Remote notification handler. Parse BUF, queue notification and
+ update STATE. */
void
-handle_notification (char *buf)
+handle_notification (struct remote_notif_state *state, char *buf)
{
struct notif_client *nc = NULL;
int i;
@@ -188,7 +161,7 @@ handle_notification (char *buf)
/* Notify the event loop there's a stop reply to acknowledge
and that there may be more events to fetch. */
- QUEUE_enque (notif_client_p, notif_queue, nc);
+ QUEUE_enque (notif_client_p, state->notif_queue, nc);
if (non_stop)
{
/* In non-stop, We mark REMOTE_ASYNC_GET_PENDING_EVENTS_TOKEN
@@ -227,7 +200,7 @@ handle_notification (char *buf)
2.3) <-- T05 thread:2
These pending notifications can be processed later. */
- mark_async_event_handler (remote_async_get_pending_events_token);
+ mark_async_event_handler (state->get_pending_events_token);
}
if (notif_debug)
@@ -250,15 +223,36 @@ do_notif_event_xfree (void *arg)
xfree (event);
}
-static void
-notif_xfree (struct notif_client *notif)
+/* Return an allocated remote_notif_state. */
+
+struct remote_notif_state *
+remote_notif_state_allocate (void)
+{
+ struct remote_notif_state *notif_state = xzalloc (sizeof (*notif_state));
+
+ notif_state->notif_queue = QUEUE_alloc (notif_client_p, NULL);
+
+ /* Register async_event_handler for notification. */
+
+ notif_state->get_pending_events_token
+ = create_async_event_handler (remote_async_get_pending_events_handler,
+ notif_state);
+
+ return notif_state;
+}
+
+/* Free STATE and its fields. */
+
+void
+remote_notif_state_xfree (struct remote_notif_state *state)
{
- if (notif->pending_event != NULL
- && notif->pending_event->dtr != NULL)
- notif->pending_event->dtr (notif->pending_event);
+ QUEUE_free (notif_client_p, state->notif_queue);
+
+ /* Unregister async_event_handler for notification. */
+ if (state->get_pending_events_token != NULL)
+ delete_async_event_handler (&state->get_pending_events_token);
- xfree (notif->pending_event);
- xfree (notif);
+ xfree (state);
}
/* -Wmissing-prototypes */
@@ -267,8 +261,6 @@ extern initialize_file_ftype _initialize_notif;
void
_initialize_notif (void)
{
- notif_queue = QUEUE_alloc (notif_client_p, notif_xfree);
-
add_setshow_boolean_cmd ("notification", no_class, ¬if_debug,
_("\
Set debugging of async remote notification."), _("\
diff --git a/gdb/remote-notif.h b/gdb/remote-notif.h
index da4fdea..4c3617e 100644
--- a/gdb/remote-notif.h
+++ b/gdb/remote-notif.h
@@ -68,16 +68,36 @@ typedef struct notif_client
struct notif_event *pending_event;
} *notif_client_p;
+DECLARE_QUEUE_P (notif_client_p);
+
+/* State on remote async notification. */
+
+struct remote_notif_state
+{
+ /* Notification queue. */
+
+ QUEUE(notif_client_p) *notif_queue;
+
+ /* Asynchronous signal handle registered as event loop source for when
+ the remote sent us a notification. The registered callback
+ will do a ACK sequence to pull the rest of the events out of
+ the remote side into our event queue. */
+
+ struct async_event_handler *get_pending_events_token;
+};
+
void remote_notif_ack (struct notif_client *nc, char *buf);
struct notif_event *remote_notif_parse (struct notif_client *nc,
char *buf);
-void handle_notification (char *buf);
+void handle_notification (struct remote_notif_state *notif_state,
+ char *buf);
-void remote_notif_register_async_event_handler (void);
-void remote_notif_unregister_async_event_handler (void);
+void remote_notif_process (struct remote_notif_state *state,
+ struct notif_client *except);
+struct remote_notif_state *remote_notif_state_allocate (void);
+void remote_notif_state_xfree (struct remote_notif_state *state);
-void remote_notif_process (struct notif_client *except);
extern struct notif_client notif_client_stop;
extern int notif_debug;
diff --git a/gdb/remote.c b/gdb/remote.c
index a9ef297..6ac3f51 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -425,6 +425,9 @@ struct remote_state
threadref echo_nextthread;
threadref nextthread;
threadref resultthreadlist[MAXTHREADLISTRESULTS];
+
+ /* The state of remote notification. */
+ struct remote_notif_state *notif_state;
};
/* Private data that we'll store in (struct thread_info)->private. */
@@ -3073,7 +3076,7 @@ remote_close (void)
if (remote_async_inferior_event_token)
delete_async_event_handler (&remote_async_inferior_event_token);
- remote_notif_unregister_async_event_handler ();
+ remote_notif_state_xfree (rs->notif_state);
trace_reset_local_state ();
}
@@ -4337,7 +4340,7 @@ remote_open_1 (char *name, int from_tty,
remote_async_inferior_event_token
= create_async_event_handler (remote_async_inferior_event_handler,
NULL);
- remote_notif_register_async_event_handler ();
+ rs->notif_state = remote_notif_state_allocate ();
/* Reset the target state; these things will be queried either by
remote_query_supported or as they are needed. */
@@ -4931,7 +4934,7 @@ remote_resume (struct target_ops *ops,
before resuming inferior, because inferior was stopped and no RSP
traffic at that moment. */
if (!non_stop)
- remote_notif_process (¬if_client_stop);
+ remote_notif_process (rs->notif_state, ¬if_client_stop);
rs->last_sent_signal = siggnal;
rs->last_sent_step = step;
@@ -7352,7 +7355,7 @@ putpkt_binary (char *buf, int cnt)
str);
do_cleanups (old_chain);
}
- handle_notification (rs->buf);
+ handle_notification (rs->notif_state, rs->buf);
/* We're in sync now, rewait for the ack. */
tcount = 0;
}
@@ -7738,7 +7741,7 @@ getpkt_or_notif_sane_1 (char **buf, long *sizeof_buf, int forever,
if (is_notif != NULL)
*is_notif = 1;
- handle_notification (*buf);
+ handle_notification (rs->notif_state, *buf);
/* Notifications require no acknowledgement. */
--
1.7.7.6
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 4/6] async remote notification 'Trace'.
2013-08-19 1:56 [PATCH 0/6 V5] MI notification on trace started/stopped Yao Qi
` (2 preceding siblings ...)
2013-08-19 1:56 ` [PATCH 1/6] Move notif_queue to remote_state Yao Qi
@ 2013-08-19 1:56 ` Yao Qi
2013-08-19 1:56 ` [PATCH 2/6] Add annex in an async remote notification Yao Qi
` (3 subsequent siblings)
7 siblings, 0 replies; 23+ messages in thread
From: Yao Qi @ 2013-08-19 1:56 UTC (permalink / raw)
To: gdb-patches
Hi,
This patch adds a new async remote notification 'Trace' to report the
trace-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
and of course, it can be used for other trace-related changes, with
different annex.
The documentation patch was approved by Eli
here <http://sourceware.org/ml/gdb-patches/2013-02/msg00440.html>
gdb/gdbserver:
* notif.c (notifs): Add "notif_trace".
* notif.h (ontif_trace): Declare.
* tracepoint.c [!IN_PROCESS_AGENT]: Include "notif.h".
(notif_reply_trace): New.
(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.
(NOTIF_ANNEX_TRACE_STATUS): New macro.
* 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 | 4 +-
gdb/doc/gdb.texinfo | 10 ++++++
gdb/gdbserver/notif.c | 1 +
gdb/gdbserver/notif.h | 1 +
gdb/gdbserver/tracepoint.c | 36 +++++++++++++++++++++
gdb/remote-notif-trace.c | 75 ++++++++++++++++++++++++++++++++++++++++++++
gdb/remote-notif.c | 1 +
gdb/remote-notif.h | 1 +
8 files changed, 127 insertions(+), 2 deletions(-)
create mode 100644 gdb/remote-notif-trace.c
diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 3f83122..749ea95 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -521,7 +521,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 notif-base.o
+ remote-notif.o ctf.o notif-base.o remote-notif-trace.o
# This is remote-sim.o if a simulator is to be linked in.
SIM_OBS = @SIM_OBS@
@@ -752,7 +752,7 @@ SFILES = ada-exp.y ada-lang.c ada-typeprint.c ada-valprint.c ada-tasks.c \
proc-service.list progspace.c \
prologue-value.c psymtab.c \
regcache.c reggroups.c remote.c remote-fileio.c remote-notif.c \
- notif-base.c reverse.c \
+ notif-base.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 946e132..755946e 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -37822,6 +37822,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{}}
@@ -39846,6 +39848,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.
@@ -40380,6 +40383,13 @@ 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 @code{status}
+@tab @var{reply}. The @var{reply} has the form of the reply to packet
+@samp{qTStatus}, as described in @ref{qTStatus packet}.
+@tab Report an asynchronous trace-related event.
+
@end multitable
@node Remote Non-Stop
diff --git a/gdb/gdbserver/notif.c b/gdb/gdbserver/notif.c
index 2b09078..8921ee7 100644
--- a/gdb/gdbserver/notif.c
+++ b/gdb/gdbserver/notif.c
@@ -52,6 +52,7 @@
static struct notif_server *notifs[] =
{
¬if_stop,
+ ¬if_trace,
};
/* Helper function to write EVENT of NOTIF to buffer OWN_BUF. */
diff --git a/gdb/gdbserver/notif.h b/gdb/gdbserver/notif.h
index f33fd04..4c16a45 100644
--- a/gdb/gdbserver/notif.h
+++ b/gdb/gdbserver/notif.h
@@ -57,6 +57,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 5c0dec7..284358f 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -3371,6 +3371,32 @@ 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)
+{
+ cmd_qtstatus (own_buf);
+}
+
+#define NOTIF_ANNEX_TRACE_STATUS 0
+
+static struct notif_annex notif_annex_trace[] =
+{
+ { "status", 0, notif_reply_trace, },
+ { NULL, 0, NULL, },
+};
+
+struct notif_server notif_trace =
+{
+ { "Trace", "vTraced", notif_annex_trace, }, NULL,
+};
+
+#endif
+
/* End a tracing run, filling in a stop reason to report back to GDB,
and removing the tracepoints from the code. */
@@ -3471,6 +3497,16 @@ stop_tracing (void)
}
unpause_all (1);
+
+ if (NOTIF_ANNEX_SUPPORTED (UNUSED, ¬if_trace.base,
+ NOTIF_ANNEX_TRACE_STATUS))
+ {
+ struct notif_annex_event *event
+ = xmalloc (sizeof (struct notif_annex_event));
+
+ event->annex_index = NOTIF_ANNEX_TRACE_STATUS;
+ notif_push (¬if_trace, (struct notif_event *) event);
+ }
}
static int
diff --git a/gdb/remote-notif-trace.c b/gdb/remote-notif-trace.c
new file mode 100644
index 0000000..d9ff1c1
--- /dev/null
+++ b/gdb/remote-notif-trace.c
@@ -0,0 +1,75 @@
+/* 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_status_parse (struct notif_client *self, char *buf,
+ struct notif_event *event)
+{
+ struct trace_status *ts = current_trace_status ();
+
+ gdb_assert (buf[0] == 'T');
+ parse_trace_status (buf + 1, ts);
+}
+
+static void
+remote_notif_trace_ack (struct notif_client *self, char *buf,
+ struct notif_event *event)
+{
+ /* acknowledge */
+ putpkt ((char *) self->base.ack_name);
+}
+
+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;
+}
+
+static struct notif_annex notif_client_trace_annex[] =
+{
+ { "status", -1, remote_notif_trace_status_parse, },
+ { NULL, -1, NULL, },
+};
+
+/* A client of notification 'Trace'. */
+
+struct notif_client notif_client_trace =
+{
+ { "Trace", "vTraced", notif_client_trace_annex, },
+ remote_notif_trace_ack,
+ remote_notif_trace_can_get_pending_events,
+ remote_notif_trace_alloc_event,
+ NULL,
+};
diff --git a/gdb/remote-notif.c b/gdb/remote-notif.c
index b83ea7e..885e3b8 100644
--- a/gdb/remote-notif.c
+++ b/gdb/remote-notif.c
@@ -49,6 +49,7 @@ int notif_debug = 0;
static struct notif_client *notifs[] =
{
¬if_client_stop,
+ ¬if_client_trace,
};
static void do_notif_event_xfree (void *arg);
diff --git a/gdb/remote-notif.h b/gdb/remote-notif.h
index 9a68779..8ccc741 100644
--- a/gdb/remote-notif.h
+++ b/gdb/remote-notif.h
@@ -96,6 +96,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
^ permalink raw reply [flat|nested] 23+ messages in thread* [PATCH 2/6] Add annex in an async remote notification.
2013-08-19 1:56 [PATCH 0/6 V5] MI notification on trace started/stopped Yao Qi
` (3 preceding siblings ...)
2013-08-19 1:56 ` [PATCH 4/6] async remote notification 'Trace' Yao Qi
@ 2013-08-19 1:56 ` Yao Qi
2013-09-26 18:43 ` Pedro Alves
2013-08-19 1:56 ` [PATCH 5/6] MI notification on trace started/stopped:basic Yao Qi
` (2 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Yao Qi @ 2013-08-19 1:56 UTC (permalink / raw)
To: gdb-patches
In order to support "Trace:status" notification and other similar
usage (such as "Point:modified", about a breakpoint is modified), we
introduce "annex" in the async remote notification, which is helpful
to give us more details what the contents about in the async remote
notification. The annex in each notification is optional. In the
RSP:
<-- %name:annex1:event
--> vAck
<-- annex2:event
--> vAck
<-- annex1:event
--> OK
As we can see above, three events of two annexes (annex1 and annex2)
are sent.
The parsing (receiving rsp packet) and writing (sending rsp packet)
can be done on the annex level. Each annex has its own routines to
send and parse packet, which is more OO.
Also, annex, instead of notification, is the unit of support and they
(GDB and GDBserver) can exchange their supported annexes by means of
qSupported mechanism, which is done in the next patch.
This patch adds "annex" to both GDB and GDBserver. I find 'struct
notif_client' and 'struct notif_server' have three fields which can be
shared, so additionally, we move some shared struct to
common/common-notif.h.
The documentation patch was approved by Eli here
<http://sourceware.org/ml/gdb-patches/2013-02/msg00438.html>
gdb:
* common/notif-base.h: New.
* Makefile.in (HFILES_NO_SRCDIR): Add "common/notif-base.h".
* remote-notif.h: Include "notif-base.h".
(struct notif_client) <name, ack_command, parse>: Remove.
<base>: New.
Caller update.
* remote-notif.c (remote_notif_parse_1): New.
(remote_notif_ack): Call remote_notif_parse_1.
(remote_notif_parse): Likewise.
(struct notif_client_annex): New.
* remote.c (notif_client_annex_stop): New.
gdb/gdbserver:
* notif.h: Include "notif-base.h".
(struct notif_server) <write, ack_name, notif_name>: Remove.
<base>: New field.
Caller update.
(struct notif_annex_event): New.
* notif.c (notif_write_event_1): New.
(notif_write_event): Call notif_write_event_1 to write event.
(notif_push): Likewise.
* server.c (notif_annex_stop): New.
(notif_stop): Update.
gdb/doc:
* gdb.texinfo (Notification Packets): Document 'annex'.
---
gdb/Makefile.in | 3 +-
gdb/common/notif-base.h | 79 +++++++++++++++++++++++++++++++++++++++++++++++
gdb/doc/gdb.texinfo | 12 +++++--
gdb/gdbserver/notif.c | 41 ++++++++++++++++++++----
gdb/gdbserver/notif.h | 20 ++++++-----
gdb/gdbserver/server.c | 7 +++-
gdb/remote-notif.c | 57 +++++++++++++++++++++++++++++-----
gdb/remote-notif.h | 13 +------
gdb/remote.c | 15 +++++---
9 files changed, 200 insertions(+), 47 deletions(-)
create mode 100644 gdb/common/notif-base.h
diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 9171940..2dc3ac0 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -854,7 +854,8 @@ common/common-utils.h common/xml-utils.h common/buffer.h common/ptid.h \
common/format.h common/host-defs.h utils.h common/queue.h common/gdb_string.h \
common/linux-osdata.h gdb-dlfcn.h auto-load.h probe.h stap-probe.h \
gdb_bfd.h sparc-ravenscar-thread.h ppc-ravenscar-thread.h common/linux-btrace.h \
-ctf.h common/i386-cpuid.h common/i386-gcc-cpuid.h common/target-common.h
+ctf.h common/i386-cpuid.h common/i386-gcc-cpuid.h common/target-common.h \
+common/notif-base.h
# Header files that already have srcdir in them, or which are in objdir.
diff --git a/gdb/common/notif-base.h b/gdb/common/notif-base.h
new file mode 100644
index 0000000..0a3a4ed
--- /dev/null
+++ b/gdb/common/notif-base.h
@@ -0,0 +1,79 @@
+/* Shared structs of asynchronous remote notification.
+
+ 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/>. */
+#ifndef NOTIF_BASE_H
+#define NOTIF_BASE_H 1
+
+struct notif_event;
+#ifndef GDBSERVER
+struct notif_client;
+#endif
+
+/* An annex of a notification. A notification may or may not have
+ annexes. */
+
+struct notif_annex
+{
+ /* Name of this annex. If it is NULL, the notification doesn't have
+ annex, and the field PARSE is about the notification this annex
+ belongs to. For example, notification event "N1:event" doesn't
+ have an annex in the packet, but the notification object still has
+ one instance of 'notif_annex', and its field <name> is NULL. */
+ const char *name;
+
+#ifdef GDBSERVER
+ /* Write event EVENT to OWN_BUF. */
+ void (*write) (struct notif_event *event, char *own_buf);
+#else
+ /* Parse BUF to get the expected event and update EVENT. This
+ function may throw exception if contents in BUF is not the
+ expected event. */
+ void (*parse) (struct notif_client *self, char *buf,
+ struct notif_event *event);
+#endif
+};
+
+/* Iterate over annexes in *NOTIF by increasing INDEX from zero. */
+
+#define NOTIF_ITER_ANNEX(NOTIF, INDEX, ANNEX) \
+ for (INDEX = 0; (ANNEX) = &(NOTIF)->annexes[INDEX], (ANNEX)->name != NULL; INDEX++)
+
+/* Notification *NOTIF has annex or not. */
+
+#define NOTIF_HAS_ANNEX(NOTIF) ((NOTIF)->annexes[0].name != NULL)
+
+
+/* "Base class" of a notification. It can be extended in both GDB
+ and GDBserver to represent a type of notification. */
+
+struct notif_base
+{
+ /* The notification packet, for example, '%Stop'. Note that '%' is
+ not in 'notif_name'. */
+ const char *notif_name;
+ /* The name of ack packet, for example, 'vStopped'. */
+ const char *ack_name;
+
+ /* Annexes the notification has. The notification may or not have
+ annexes. Macro 'NOTIF_HAS_ANNEX' is to check notification has
+ annexes, and macro 'NOTIF_ITER_ANNEX' is to iterate over annexes
+ of the notification. */
+ struct notif_annex *annexes;
+};
+
+#endif /* NOTIF_BASE_H */
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 6d5dec4..375f988 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -40270,13 +40270,15 @@ transmit notifications without fear of confusing older clients. There
are no notifications defined for @value{GDBN} to send at the moment, but we
assume that most older stubs would ignore them, as well.)
-Each notification is comprised of three parts:
+Each notification is comprised of four parts:
@table @samp
-@item @var{name}:@var{event}
+@item @var{name}[:@var{annex}]:@var{event}
The notification packet is sent by the side that initiates the
exchange (currently, only the stub does that), with @var{event}
carrying the specific information about the notification.
-@var{name} is the name of the notification.
+@var{name} is the name of the notification. The @var{annex} is
+specific to @var{name}; it can supply additional details about
+@var{event}.
@item @var{ack}
The acknowledge sent by the other side, usually @value{GDBN}, to
acknowledge the exchange and request the event.
@@ -40339,15 +40341,17 @@ following example:
@end smallexample
The following notifications are defined:
-@multitable @columnfractions 0.12 0.12 0.38 0.38
+@multitable @columnfractions 0.10 0.10 0.10 0.35 0.35
@item Notification
@tab Ack
+@tab Annex
@tab Event
@tab Description
@item Stop
@tab vStopped
+@tab
@tab @var{reply}. The @var{reply} has the form of a stop reply, as
described in @ref{Stop Reply Packets}. Refer to @ref{Remote Non-Stop},
for information on how these notifications are acknowledged by
diff --git a/gdb/gdbserver/notif.c b/gdb/gdbserver/notif.c
index e27746e..9300697 100644
--- a/gdb/gdbserver/notif.c
+++ b/gdb/gdbserver/notif.c
@@ -54,6 +54,30 @@ static struct notif_server *notifs[] =
¬if_stop,
};
+/* Helper function to write EVENT of NOTIF to buffer OWN_BUF. */
+
+static void
+notif_write_event_1 (struct notif_base *notif,
+ struct notif_event *event,
+ char *own_buf)
+{
+ int annex_index = 0;
+
+ /* If the NOTIF has annexes, extract the annex index from the
+ EVENT and append annex name to OWN_BUF; otherwise, NOTIF
+ doesn't have annex and use slot 0 of annexes for the
+ notification itself. */
+ if (NOTIF_HAS_ANNEX (notif))
+ {
+ annex_index
+ = ((struct notif_annex_event *) event)->annex_index;
+ sprintf (own_buf, "%s:", notif->annexes[annex_index].name);
+ own_buf += strlen (notif->annexes[annex_index].name) + 1;
+ }
+
+ notif->annexes[annex_index].write (event, own_buf);
+}
+
/* Write another event or an OK, if there are no more left, to
OWN_BUF. */
@@ -65,7 +89,8 @@ notif_write_event (struct notif_server *notif, char *own_buf)
struct notif_event *event
= QUEUE_peek (notif_event_p, notif->queue);
- notif->write (event, own_buf);
+ notif_write_event_1 ((struct notif_base *) notif, event,
+ own_buf);
}
else
write_ok (own_buf);
@@ -84,8 +109,9 @@ handle_notif_ack (char *own_buf, int packet_len)
for (i = 0; i < ARRAY_SIZE (notifs); i++)
{
np = notifs[i];
- if (strncmp (own_buf, np->ack_name, strlen (np->ack_name)) == 0
- && packet_len == strlen (np->ack_name))
+ if (0 == strncmp (own_buf, np->base.ack_name,
+ strlen (np->base.ack_name))
+ && packet_len == strlen (np->base.ack_name))
break;
}
@@ -100,7 +126,7 @@ handle_notif_ack (char *own_buf, int packet_len)
= QUEUE_deque (notif_event_p, np->queue);
if (remote_debug)
- fprintf (stderr, "%s: acking %d\n", np->ack_name,
+ fprintf (stderr, "%s: acking %d\n", np->base.ack_name,
QUEUE_length (notif_event_p, np->queue));
xfree (head);
@@ -120,7 +146,8 @@ notif_event_enque (struct notif_server *notif,
QUEUE_enque (notif_event_p, notif->queue, event);
if (remote_debug)
- fprintf (stderr, "pending events: %s %d\n", notif->notif_name,
+ fprintf (stderr, "pending events: %s %d\n",
+ notif->base.notif_name,
QUEUE_length (notif_event_p, notif->queue));
}
@@ -142,10 +169,10 @@ notif_push (struct notif_server *np, struct notif_event *new_event)
char buf[PBUFSIZ];
char *p = buf;
- xsnprintf (p, PBUFSIZ, "%s:", np->notif_name);
+ xsnprintf (p, PBUFSIZ, "%s:", np->base.notif_name);
p += strlen (p);
- np->write (new_event, p);
+ notif_write_event_1 ((struct notif_base *) np, new_event, p);
putpkt_notif (buf);
}
}
diff --git a/gdb/gdbserver/notif.h b/gdb/gdbserver/notif.h
index c714e7b..037f1b0 100644
--- a/gdb/gdbserver/notif.h
+++ b/gdb/gdbserver/notif.h
@@ -20,6 +20,7 @@
#include "server.h"
#include "target.h"
#include "queue.h"
+#include "notif-base.h"
/* Structure holding information related to a single event. We
keep a queue of these to push to GDB. It can be extended if
@@ -33,25 +34,26 @@ typedef struct notif_event
DECLARE_QUEUE_P (notif_event_p);
+/* An event of a notification which has an annex. */
+
+struct notif_annex_event
+{
+ struct notif_event base;
+ /* The index of the annex in field 'annexes' in 'notif_server'. */
+ int annex_index;
+};
+
/* A type notification to GDB. An object of 'struct notif_server'
represents a type of notification. */
typedef struct notif_server
{
- /* The name of ack packet, for example, 'vStopped'. */
- const char *ack_name;
-
- /* The notification packet, for example, '%Stop'. Note that '%' is
- not in 'notif_name'. */
- const char *notif_name;
+ struct notif_base base;
/* A queue of events to GDB. A new notif_event can be enque'ed
into QUEUE at any appropriate time, and the notif_reply is
deque'ed only when the ack from GDB arrives. */
QUEUE (notif_event_p) *queue;
-
- /* Write event EVENT to OWN_BUF. */
- void (*write) (struct notif_event *event, char *own_buf);
} *notif_server_p;
extern struct notif_server notif_stop;
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index a4b9129..71b860d 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -181,9 +181,14 @@ vstop_notif_reply (struct notif_event *event, char *own_buf)
prepare_resume_reply (own_buf, vstop->ptid, &vstop->status);
}
+static struct notif_annex notif_annex_stop[] =
+{
+ { NULL, vstop_notif_reply, },
+};
+
struct notif_server notif_stop =
{
- "vStopped", "Stop", NULL, vstop_notif_reply,
+ { "Stop", "vStopped", notif_annex_stop, }, NULL,
};
static int
diff --git a/gdb/remote-notif.c b/gdb/remote-notif.c
index 8b69f2f..d901a51 100644
--- a/gdb/remote-notif.c
+++ b/gdb/remote-notif.c
@@ -53,6 +53,44 @@ static struct notif_client *notifs[] =
static void do_notif_event_xfree (void *arg);
+/* Iterate over annexes in NC to match annex in BUF. */
+
+static void
+remote_notif_parse_1 (struct notif_client *nc,
+ struct notif_event *event, char *buf)
+{
+ const struct notif_annex *m = NULL;
+ const struct notif_base *base = (struct notif_base *) nc;
+
+ if (NOTIF_HAS_ANNEX (base))
+ {
+ int i;
+
+ NOTIF_ITER_ANNEX (base, i, m)
+ {
+ if (strncmp (m->name, buf, strlen (base->notif_name)) == 0
+ /* The annex is separated by ':' for the rest of
+ contents in BUF. */
+ && buf[strlen (m->name)] == ':')
+ {
+ /* Pass BUF without annex and ':'. */
+ m->parse (nc, buf + strlen (m->name) + 1, event);
+ break;
+ }
+ m = NULL;
+ }
+ }
+ else
+ {
+ m = &base->annexes[0];
+ m->parse (nc, buf, event);
+ }
+
+ if (m == NULL)
+ error (_("Can't parse '%s' for notif '%s'"), buf,
+ base->notif_name);
+}
+
/* Parse the BUF for the expected notification NC, and send packet to
acknowledge. */
@@ -65,9 +103,9 @@ remote_notif_ack (struct notif_client *nc, char *buf)
if (notif_debug)
fprintf_unfiltered (gdb_stdlog, "notif: ack '%s'\n",
- nc->ack_command);
+ nc->base.ack_name);
- nc->parse (nc, buf, event);
+ remote_notif_parse_1 (nc, event, buf);
nc->ack (nc, buf, event);
discard_cleanups (old_chain);
@@ -83,9 +121,10 @@ remote_notif_parse (struct notif_client *nc, char *buf)
= make_cleanup (do_notif_event_xfree, event);
if (notif_debug)
- fprintf_unfiltered (gdb_stdlog, "notif: parse '%s'\n", nc->name);
+ fprintf_unfiltered (gdb_stdlog, "notif: parse '%s'\n",
+ nc->base.notif_name);
- nc->parse (nc, buf, event);
+ remote_notif_parse_1 (nc, event, buf);
discard_cleanups (old_chain);
return event;
@@ -157,8 +196,9 @@ handle_notification (struct remote_notif_state *state, char *buf)
for (i = 0; i < ARRAY_SIZE (notifs); i++)
{
nc = notifs[i];
- if (strncmp (buf, nc->name, strlen (nc->name)) == 0
- && buf[strlen (nc->name)] == ':')
+ if (0 == strncmp (buf, nc->base.notif_name,
+ strlen (nc->base.notif_name))
+ && buf[strlen (nc->base.notif_name)] == ':')
break;
nc = NULL;
}
@@ -180,7 +220,8 @@ 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,
+ buf + strlen (nc->base.notif_name) + 1);
/* Be careful to only set it after parsing, since an error
may be thrown then. */
@@ -233,7 +274,7 @@ handle_notification (struct remote_notif_state *state, char *buf)
if (notif_debug)
fprintf_unfiltered (gdb_stdlog,
"notif: Notification '%s' captured\n",
- nc->name);
+ nc->base.notif_name);
}
}
diff --git a/gdb/remote-notif.h b/gdb/remote-notif.h
index 05d2613..454dce8 100644
--- a/gdb/remote-notif.h
+++ b/gdb/remote-notif.h
@@ -21,6 +21,7 @@
#define REMOTE_NOTIF_H
#include "queue.h"
+#include "notif-base.h"
/* An event of a type of async remote notification. */
@@ -35,17 +36,7 @@ struct notif_event
typedef struct notif_client
{
- /* The name of notification packet. */
- const char *name;
-
- /* The packet to acknowledge a previous reply. */
- const char *ack_command;
-
- /* Parse BUF to get the expected event and update EVENT. This
- function may throw exception if contents in BUF is not the
- expected event. */
- void (*parse) (struct notif_client *self, char *buf,
- struct notif_event *event);
+ struct notif_base base;
/* Send field <ack_command> to remote, and do some checking. If
something wrong, throw an exception. */
diff --git a/gdb/remote.c b/gdb/remote.c
index e50623a..9700a8e 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -5329,7 +5329,7 @@ remote_notif_stop_ack (struct notif_client *self, char *buf,
struct stop_reply *stop_reply = (struct stop_reply *) event;
/* acknowledge */
- putpkt ((char *) self->ack_command);
+ putpkt ((char *) self->base.ack_name);
if (stop_reply->ws.kind == TARGET_WAITKIND_IGNORE)
/* We got an unknown stop reply. */
@@ -5369,13 +5369,16 @@ remote_notif_stop_alloc_reply (void)
return r;
}
+static struct notif_annex notif_client_annex_stop[] =
+{
+ { NULL, remote_notif_stop_parse, },
+};
+
/* A client of notification Stop. */
struct notif_client notif_client_stop =
{
- "Stop",
- "vStopped",
- remote_notif_stop_parse,
+ { "Stop", "vStopped", notif_client_annex_stop, },
remote_notif_stop_ack,
remote_notif_stop_can_get_pending_events,
remote_notif_stop_alloc_reply,
@@ -5802,7 +5805,7 @@ remote_notif_get_pending_events (struct notif_client *nc)
if (notif_debug)
fprintf_unfiltered (gdb_stdlog,
"notif: process: '%s' ack pending event\n",
- nc->name);
+ nc->base.notif_name);
/* acknowledge */
nc->ack (nc, rs->buf, nc->pending_event);
@@ -5822,7 +5825,7 @@ remote_notif_get_pending_events (struct notif_client *nc)
if (notif_debug)
fprintf_unfiltered (gdb_stdlog,
"notif: process: '%s' no pending reply\n",
- nc->name);
+ nc->base.notif_name);
}
}
--
1.7.7.6
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 2/6] Add annex in an async remote notification.
2013-08-19 1:56 ` [PATCH 2/6] Add annex in an async remote notification Yao Qi
@ 2013-09-26 18:43 ` Pedro Alves
2013-09-27 1:44 ` Yao Qi
0 siblings, 1 reply; 23+ messages in thread
From: Pedro Alves @ 2013-09-26 18:43 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
Hi Yao,
On 08/19/2013 02:55 AM, Yao Qi wrote:
> In order to support "Trace:status" notification and other similar
> usage (such as "Point:modified", about a breakpoint is modified), we
> introduce "annex" in the async remote notification, which is helpful
> to give us more details what the contents about in the async remote
> notification. The annex in each notification is optional. In the
> RSP:
Please help me understand this, as it's not obvious to me.
This mechanism adds a bunch of new code. I've read the series and the
descriptions a few times, and I still haven't managed to find where
the rationale behind these annexes is (or figure it out myself). :-(
_Why_ are they necessary? What problem do they solve, that simply
calling these notifications "Trace-status", "Point-modified", and
later other new notifications "Trace-whatever-else", "Point-whatnot",
etc. wouldn't solve? If it's just neat grouping, than it doesn't
look like worthwhile.
The only thing that comes to mind is ordering -- that is, all
event with the same base notification handled on a FIFO basis,
even if they have different annexes. But that doesn't look like
to be the reason -- given that the other annex
hinted -- Point:modified -- belongs to a different notification.
--
Pedro Alves
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/6] Add annex in an async remote notification.
2013-09-26 18:43 ` Pedro Alves
@ 2013-09-27 1:44 ` Yao Qi
2013-10-18 1:05 ` Yao Qi
0 siblings, 1 reply; 23+ messages in thread
From: Yao Qi @ 2013-09-27 1:44 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
On 09/27/2013 02:43 AM, Pedro Alves wrote:
> This mechanism adds a bunch of new code. I've read the series and the
> descriptions a few times, and I still haven't managed to find where
> the rationale behind these annexes is (or figure it out myself).:-(
>
> _Why_ are they necessary? What problem do they solve, that simply
> calling these notifications "Trace-status", "Point-modified", and
> later other new notifications "Trace-whatever-else", "Point-whatnot",
> etc. wouldn't solve? If it's just neat grouping, than it doesn't
> look like worthwhile.
>
> The only thing that comes to mind is ordering -- that is, all
> event with the same base notification handled on a FIFO basis,
> even if they have different annexes. But that doesn't look like
> to be the reason -- given that the other annex
> hinted -- Point:modified -- belongs to a different notification.
The ordering is the reason we add annex. As you said, events
with the same basic notification are handled in a FIFO manner. Events
with Point:created, Point:modified, and Point:deleted should be
processed in a FIFO order, while Trace:status and Trace:foo should be
processed in FIFO too. When this series was written, 2013 Jan, hui was
proposing "target-defined breakpoint", which requires some asnyc
notifications on breakpoints, so we added annex in V3.
[PATCH 1/5] Add annex in a async remote notification.
https://sourceware.org/ml/gdb-patches/2013-01/msg00506.html
Nowadays, we have few notifications (Stop and Trace) and each of them
are not related. As more notification added, we'll need annex in the
infrastructure, IMO.
A lot of code is added for annex, but the code is more modulized. It is
flexible to add a new notification or add a new annex for an
existing notification.
--
Yao (é½å°§)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/6] Add annex in an async remote notification.
2013-09-27 1:44 ` Yao Qi
@ 2013-10-18 1:05 ` Yao Qi
0 siblings, 0 replies; 23+ messages in thread
From: Yao Qi @ 2013-10-18 1:05 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
On 09/27/2013 09:44 AM, Yao Qi wrote:
> The ordering is the reason we add annex. As you said, events
> with the same basic notification are handled in a FIFO manner. Events
> with Point:created, Point:modified, and Point:deleted should be
> processed in a FIFO order, while Trace:status and Trace:foo should be
> processed in FIFO too. When this series was written, 2013 Jan, hui was
> proposing "target-defined breakpoint", which requires some asnyc
> notifications on breakpoints, so we added annex in V3.
>
> [PATCH 1/5] Add annex in a async remote notification.
> https://sourceware.org/ml/gdb-patches/2013-01/msg00506.html
>
> Nowadays, we have few notifications (Stop and Trace) and each of them
> are not related. As more notification added, we'll need annex in the
> infrastructure, IMO.
Pedro,
If you think annex is unnecessary, I can remove it and recreate the
patches on top of V2.
--
Yao (é½å°§)
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 5/6] MI notification on trace started/stopped:basic
2013-08-19 1:56 [PATCH 0/6 V5] MI notification on trace started/stopped Yao Qi
` (4 preceding siblings ...)
2013-08-19 1:56 ` [PATCH 2/6] Add annex in an async remote notification Yao Qi
@ 2013-08-19 1:56 ` Yao Qi
2013-09-02 0:14 ` [PATCH 0/6 V5] MI notification on trace started/stopped Yao Qi
2013-09-18 13:24 ` [ping 2]: " Yao Qi
7 siblings, 0 replies; 23+ messages in thread
From: Yao Qi @ 2013-08-19 1:56 UTC (permalink / raw)
To: gdb-patches
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 new MI notifications =trace-started and
=trace-stopped.
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 6ee82f7..b143c0e 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -79,6 +79,8 @@ show range-stepping
** 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 755946e..4319953 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -29154,6 +29154,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 adb7085..17e3588 100644
--- a/gdb/doc/observer.texi
+++ b/gdb/doc/observer.texi
@@ -249,6 +249,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 0768b2a..4735589 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -152,9 +152,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 e370a57..15e33f1 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -68,6 +68,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);
@@ -133,6 +134,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);
@@ -619,6 +621,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 7dbe54b..fa7518f 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -1901,6 +1901,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;
@@ -1983,6 +1985,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
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 0/6 V5] MI notification on trace started/stopped
2013-08-19 1:56 [PATCH 0/6 V5] MI notification on trace started/stopped Yao Qi
` (5 preceding siblings ...)
2013-08-19 1:56 ` [PATCH 5/6] MI notification on trace started/stopped:basic Yao Qi
@ 2013-09-02 0:14 ` Yao Qi
2013-09-18 13:24 ` [ping 2]: " Yao Qi
7 siblings, 0 replies; 23+ messages in thread
From: Yao Qi @ 2013-09-02 0:14 UTC (permalink / raw)
To: gdb-patches
On 08/19/2013 09:55 AM, Yao Qi wrote:
> This is the V5 of this patch series, and the changes in V5 are:
>
> - Move 'notif_queue' to remote_state, which is done by patch 1/6.
> - Store the supported status of each annex in remote_state too. It
> is the major change in patch 3/6 compared with V4.
> - some small fixes in code and comments.
>
> Patch 1/6 can be regarded as a refactor patch, so it can be approved
> separately. Patch 5/6 adds MI notifications when command 'tstart'
> and 'tstop' is typed in MI, it is a typical MI patch, and can be
> approved separately too.
>
> Regression tested them on x86_64-linux with
> {unix, native-gdbserver} x {sync, async}. Is it OK?
>
> Below is the introduction of this series. People who are
> familiar with this series, please skip it.
>
> This patch series adds the MI notifications of 'trace-started' and
> 'trace-stopped', which are emitted when
>
> 1) trace is started or stopped by commands in GDB,
> 2) trace is stopped due to some reasons in the remote stub, such as
> trace buffer full.
>
> With these notifications, MI front-end can show the status of trace
> up to date.
>
> V4 can be found here
>
> [PATCH v4 0/5] MI notification on trace started/stopped
> http://sourceware.org/ml/gdb-patches/2013-04/msg00019.html
Ping. https://sourceware.org/ml/gdb-patches/2013-08/msg00484.html
--
Yao (é½å°§)
^ permalink raw reply [flat|nested] 23+ messages in thread* [ping 2]: [PATCH 0/6 V5] MI notification on trace started/stopped
2013-08-19 1:56 [PATCH 0/6 V5] MI notification on trace started/stopped Yao Qi
` (6 preceding siblings ...)
2013-09-02 0:14 ` [PATCH 0/6 V5] MI notification on trace started/stopped Yao Qi
@ 2013-09-18 13:24 ` Yao Qi
2013-09-18 13:25 ` Pedro Alves
7 siblings, 1 reply; 23+ messages in thread
From: Yao Qi @ 2013-09-18 13:24 UTC (permalink / raw)
To: gdb-patches
On 08/19/2013 09:55 AM, Yao Qi wrote:
> This is the V5 of this patch series, and the changes in V5 are:
>
> - Move 'notif_queue' to remote_state, which is done by patch 1/6.
> - Store the supported status of each annex in remote_state too. It
> is the major change in patch 3/6 compared with V4.
> - some small fixes in code and comments.
>
> Patch 1/6 can be regarded as a refactor patch, so it can be approved
> separately. Patch 5/6 adds MI notifications when command 'tstart'
> and 'tstop' is typed in MI, it is a typical MI patch, and can be
> approved separately too.
>
> Regression tested them on x86_64-linux with
> {unix, native-gdbserver} x {sync, async}. Is it OK?
>
> Below is the introduction of this series. People who are
> familiar with this series, please skip it.
>
> This patch series adds the MI notifications of 'trace-started' and
> 'trace-stopped', which are emitted when
>
> 1) trace is started or stopped by commands in GDB,
> 2) trace is stopped due to some reasons in the remote stub, such as
> trace buffer full.
>
> With these notifications, MI front-end can show the status of trace
> up to date.
>
> V4 can be found here
>
> [PATCH v4 0/5] MI notification on trace started/stopped
> http://sourceware.org/ml/gdb-patches/2013-04/msg00019.html
Ping. https://sourceware.org/ml/gdb-patches/2013-08/msg00484.html
--
Yao (é½å°§)
^ permalink raw reply [flat|nested] 23+ messages in thread