This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 3/7] de-couple %Stop from notification: gdb
- From: Pedro Alves <palves at redhat dot com>
- To: Yao Qi <yao at codesourcery dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Fri, 30 Nov 2012 17:36:19 +0000
- Subject: Re: [PATCH 3/7] de-couple %Stop from notification: gdb
- References: <1350991620-12950-1-git-send-email-yao@codesourcery.com> <1350991620-12950-4-git-send-email-yao@codesourcery.com>
On 10/23/2012 12:26 PM, Yao Qi wrote:
> This patch is to de-couple vStopped/%Stop from notification in gdb side.
> I just put the necessary explanations into the comments in
> remote-notif.c, so don't write down too much here.
>
> gdb:
>
> 2012-10-23 Yao Qi <yao@codesourcery.com>
>
> * Makefile.in (REMOTE_OBS): Add 'remote-notif.o'.
> (SFILES): Add 'remote-notif.c'.
> (HFILES_NO_SRCDIR): Add 'remote-notif.h' and 'common/queue.h'.
> * remote-notif.c: New. Moved from remote.c.
s/Moved/Factored out/
> * remote-notif.h: New.
> * remote.c: Include 'remote-notif.h'.
> Remove declarations of stop_reply_xmalloc, stop_reply_xfree,
> do_stop_reply_xfree, remote_parse_stop_reply, push_stop_reply,
> remote_get_pending_stop_replies,
> remote_async_get_pending_events_handler, and
> discard_pending_stop_replies.
> Declare remote_parse_stop_reply and notif_queued_reply_push.
> Remove variable pending_stop_reply.
This is not standard GNU formatting.
> (remote_async_inferior_event_token): Remove 'static'.
> (remote_async_get_pending_events_token): Move to
> remote-notif.c.
>
> (remote_close): Replace 'delete_async_event_handler' with
> remote_notif_unregister_async_event_handler.
> (remote_start_remote): Replace code with remote_notif_parse
> and remote_notif_pending_replies.
> (remote_open_1): Replace 'create_async_event_handler' with
> remote_notif_register_async_event_handler.
> (extended_remote_attach_1): Call remote_notif_parse and
> notif_reply_push.
>
> (struct stop_reply) <next>: Remove.
> <base>: New field.
> <ptid>: Move it to 'struct notif_reply'.
> Callers update.
> (stop_reply_queue): Remove.
> (stop_reply_xmalloc, stop_reply_xfree, do_stop_reply_xfree): Remove
Missing period.
> (remote_notif_ack_stop, stop_reply_dtr): New.
> (remote_notif_alloc_reply_stop): New.
> (notif_packet_stop): New variable.
> (stop_reply_match_ptid, stop_reply_match_ptid_and_ws: New.
> (queued_stop_reply, peek_stop_reply): Adjust.
> (push_stop_reply): Add one more parameter 'queue', and rename to
> notif_queued_reply_push.
>
> (remote_get_pending_stop_replies): Move to remote-notif.c.
> (handle_notification): Likewise.
> (remote_async_get_pending_events_handler): Likewise.
> (remote_wait_as): Adjust to call remote_notif_parse.
> (remote_wait): Call QUEUE_is_empty (notif_reply_p).
> * remote.h: Include "target.h" and "remote-notif.h".
> Declare remote_notif_pending_replies and
> remote_async_inferior_event_token
Missing period. But that's not standard format:
(remote_notif_pending_replies, remote_async_inferior_event_token): Declare.
> * remote.h: Include "target.h" and "remote-notif.h".
> * remote.c: Include 'remote-notif.h'.
Quoting inconsistency. </nit> :-)
> ---
> gdb/Makefile.in | 9 +-
> gdb/remote-notif.c | 298 +++++++++++++++++++++++++++++++++++++
> gdb/remote-notif.h | 100 +++++++++++++
> gdb/remote.c | 413 +++++++++++++++++++++------------------------------
> gdb/remote.h | 5 +
> 5 files changed, 578 insertions(+), 247 deletions(-)
> create mode 100644 gdb/remote-notif.c
> create mode 100644 gdb/remote-notif.h
>
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 9e7702d..c3bbbb1 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -503,7 +503,8 @@ 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_OBS = remote.o dcache.o tracepoint.o ax-general.o ax-gdb.o remote-fileio.o \
> + remote-notif.o
>
> # This is remote-sim.o if a simulator is to be linked in.
> SIM_OBS = @SIM_OBS@
> @@ -725,7 +726,7 @@ 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 reverse.c \
> + regcache.c reggroups.c remote.c remote-fileio.c reverse.c remote-notif.c \
Might as well put it before reverse.c.
> sentinel-frame.c \
> serial.c ser-base.c ser-unix.c skip.c \
> solib.c solib-target.c source.c \
> @@ -778,7 +779,7 @@ c-lang.h d-lang.h go-lang.h frame.h event-loop.h block.h cli/cli-setshow.h \
> cli/cli-decode.h cli/cli-cmds.h cli/cli-dump.h cli/cli-utils.h \
> cli/cli-script.h macrotab.h symtab.h version.h \
> gnulib/import/string.in.h gnulib/import/str-two-way.h \
> -gnulib/import/stdint.in.h remote.h gdb.h sparc-nat.h \
> +gnulib/import/stdint.in.h remote.h remote-notif.h gdb.h sparc-nat.h \
> gdbthread.h dwarf2-frame.h dwarf2-frame-tailcall.h nbsd-nat.h dcache.h \
> amd64-nat.h s390-tdep.h arm-linux-tdep.h exceptions.h macroscope.h \
> gdbarch.h bsd-uthread.h gdb_stat.h memory-map.h memrange.h \
> @@ -830,7 +831,7 @@ gnulib/import/extra/snippet/arg-nonnull.h gnulib/import/extra/snippet/c++defs.h
> gnulib/import/extra/snippet/warn-on-use.h \
> gnulib/import/stddef.in.h gnulib/import/inttypes.in.h inline-frame.h skip.h \
> common/common-utils.h common/xml-utils.h common/buffer.h common/ptid.h \
> -common/format.h common/host-defs.h utils.h \
> +common/format.h common/host-defs.h utils.h common/queue.h \
> common/linux-osdata.h gdb-dlfcn.h auto-load.h probe.h stap-probe.h gdb_bfd.h
>
> # Header files that already have srcdir in them, or which are in objdir.
> diff --git a/gdb/remote-notif.c b/gdb/remote-notif.c
> new file mode 100644
> index 0000000..1032051
> --- /dev/null
> +++ b/gdb/remote-notif.c
> @@ -0,0 +1,298 @@
> +/* Remote notification in GDB protocol
> +
> + Copyright (C) 1988-2012 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/>. */
> +
> +/* Remote async notification is sent from remote target over RSP.
> + Each type of notification is represented by an object of
> + 'struct notif', which has a field 'pending_reply'. It is not
> + NULL when GDB receives a notification from GDBserver, but hasn't
> + acknowledge yet. Before GDB acknowledges the notification,
> + GDBserver shouldn't send notification again (see the header comments
> + in gdbserver/notif.c).
> +
> + Notifications are processed in an almost-unified approach for both
> + all-stop mode and non-stop mode, except the timing to process them.
> + In non-stop mode, notifications are processed in
> + remote_async_get_pending_events_handler, while in all-stop mode,
> + they are processed in remote_resume. */
> +
> +#include "defs.h"
> +#include "remote.h"
> +#include "remote-notif.h"
> +#include "event-loop.h"
> +#include "target.h"
> +#include "inferior.h"
> +
> +#include <string.h>
> +
> +unsigned int notif_debug = 0;
> +
> +/* Supported notifications. */
> +
> +static struct notif *notifs[] =
> +{
> + (struct notif *) ¬if_packet_stop,
> +};
> +
> +static void do_notif_reply_xfree (void *arg);
> +
> +/* Parse the BUF for the expected notification NP, and send packet to
> + acknowledge. */
> +
> +void
> +remote_notif_ack (struct notif *np, char *buf)
> +{
> + struct notif_reply *reply = np->alloc_reply ();
> + struct cleanup *old_chain
> + = make_cleanup (do_notif_reply_xfree, reply);
> +
> + DEBUG_NOTIF ("ack '%s'", np->ack_command);
> +
> + np->parse (np, buf, reply);
> + np->ack (np, buf, reply);
> +
> + discard_cleanups (old_chain);
> +}
> +
> +/* Parse the BUF for the expected notification NP. */
> +
> +struct notif_reply *
> +remote_notif_parse (struct notif *np, char *buf)
> +{
> + struct notif_reply *reply = np->alloc_reply ();
> + struct cleanup *old_chain
> + = make_cleanup (do_notif_reply_xfree, reply);
> +
> + DEBUG_NOTIF ("parse '%s'", np->name);
> +
> + np->parse (np, buf, reply);
> +
> + discard_cleanups (old_chain);
> + return reply;
> +}
> +
> +DEFINE_QUEUE_P (notif_p);
> +
> +QUEUE(notif_p) *notif_queue;
> +
> +/* Process notifications one by one. EXCEPT is not expected in
> + the queue. */
> +
> +void
> +remote_notif_process (struct notif *except)
> +{
> + while (!QUEUE_is_empty (notif_p, notif_queue))
> + {
> + struct notif *np = QUEUE_deque (notif_p, notif_queue);
> +
> + gdb_assert (np != except);
> +
> + if (np == (struct notif *) ¬if_packet_stop)
> + {
> + /* If we get a Stop notification, handle it in
> + remote_wait_ns. */
> + mark_async_event_handler (remote_async_inferior_event_token);
> + break;
> + }
Not much decoupling here... Can we do something better?
> +
> + remote_notif_pending_replies (np);
> + }
> +}
> +
> +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;
Hmm? So there's only one token for all notification types?
What if two notifications of different types are pending?
> +
> +/* 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 notification handler. */
> +
> +void
> +handle_notification (char *buf)
> +{
> + struct notif *np = NULL;
> + int i;
> +
> + for (i = 0; i < sizeof (notifs) / sizeof (notifs[0]); i++)
size_t. Use ARRAY_SIZE.
> + {
> + np = notifs[i];
> + if (strncmp (buf, np->name, strlen (np->name)) == 0
> + && buf[strlen (np->name)] == ':')
> + break;
> + }
> +
> + /* We ignore notifications we don't recognize, for compatibility
> + with newer stubs. */
> + if (np == NULL)
> + return;
> +
> + if (np->pending_reply)
> + {
> + /* 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.
> + Just ignore it. */
> + DEBUG_NOTIF ("ignoring resent notification");
> + }
> + else
> + {
> + struct notif_reply *reply
> + = remote_notif_parse (np, buf + strlen (np->name) + 1);
> +
> + /* Be careful to only set it after parsing, since an error
> + may be thrown then. */
> + np->pending_reply = reply;
> +
> + /* Notify the event loop there's a stop reply to acknowledge
> + and that there may be more events to fetch. */
> + QUEUE_enque (notif_p, notif_queue, np);
> + if (non_stop)
> + mark_async_event_handler (remote_async_get_pending_events_token);
> +
> + DEBUG_NOTIF ("Notification '%s' captured", np->name);
> + }
> +}
> +
> +/* Free REPLY. */
> +
> +void
> +notif_reply_xfree (struct notif_reply *reply)
> +{
> + if (reply && reply->dtr)
> + reply->dtr (reply);
> +
> + xfree (reply);
> +}
> +
> +/* Cleanup wrapper. */
> +
> +static void
> +do_notif_reply_xfree (void *arg)
> +{
> + struct notif_reply *reply = arg;
> +
> + notif_reply_xfree (reply);
> +}
> +
> +DEFINE_QUEUE_P (notif_reply_p);
> +
> +/* A parameter to pass data in and out. */
> +
> +struct queue_iter_param
> +{
> + void *input;
> + struct notif_reply *output;
> +};
> +
> +static int
> +remote_notif_remove_always (QUEUE (notif_reply_p) *q,
> + QUEUE_ITER (notif_reply_p) *iter,
> + notif_reply_p reply,
> + void *data)
This is missing a comment. What's "always" here? I see some
filtering in the function.
> +{
> + struct queue_iter_param *param = data;
> + int *pid = (int *) param->input;
> +
> + if (*pid == -1 || ptid_get_pid (reply->ptid) == *pid)
> + {
> + notif_reply_xfree (reply);
> + QUEUE_remove_elem (notif_reply_p, q, iter);
> + }
> +
> + return 1;
> +}
> +
> +/* Discard all pending replies of inferior PID. If PID is -1,
> + discard everything. */
> +
> +void
> +remote_notif_discard_replies (int pid)
> +{
> + int i;
> + struct queue_iter_param param;
> +
> + for (i = 0; i < sizeof (notifs) / sizeof (notifs[0]); i++)
ARRAY_SIZE.
> + {
> + struct notif *np = notifs[i];
> +
> + /* Discard the in-flight notification. */
> + if (np->pending_reply != NULL
> + && (pid == -1 || ptid_get_pid (np->pending_reply->ptid) == pid))
> + {
> + notif_reply_xfree (np->pending_reply);
> + np->pending_reply = NULL;
> + }
> +
> + DEBUG_NOTIF ("discard all replies: '%s' in pid %d", np->name, pid);
> + }
> +
> + param.input = &pid;
> + param.output = NULL;
> + /* Discard the replies we have already pulled with ack. */
> + QUEUE_iterate (notif_reply_p, notif_packet_stop.ack_queue,
> + remote_notif_remove_always, ¶m);
> +}
> +
> +static void
> +notif_xfree (struct notif *notif)
> +{
> + if (notif->pending_reply != NULL)
> + notif_reply_xfree (notif->pending_reply);
> +
> + if (notif->dtr != NULL)
> + notif->dtr (notif);
> +
> + xfree (notif);
> +}
> +
> +/* -Wmissing-prototypes */
> +extern initialize_file_ftype _initialize_notif;
> +
> +void
> +_initialize_notif (void)
> +{
> + notif_packet_stop.ack_queue = QUEUE_alloc (notif_reply_p, notif_reply_xfree);
> +
> + notif_queue = QUEUE_alloc (notif_p, notif_xfree);
> +}
> diff --git a/gdb/remote-notif.h b/gdb/remote-notif.h
> new file mode 100644
> index 0000000..ac291e6
> --- /dev/null
> +++ b/gdb/remote-notif.h
> @@ -0,0 +1,100 @@
> +/* Remote notification in GDB protocol
> +
> + Copyright (C) 1988-2012 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 REMOTE_NOTIF_H
> +#define REMOTE_NOTIF_H
> +#include "queue.h"
Put an empty line between double-inclusion guards, and includes.
> +
> +/* A reply for a remote notification. */
> +
> +typedef struct notif_reply
I find calling these things "replies" a bit confusing. I wonder if we
could come up with better terminology. The reason the replies to vStopped
were called "replies", is that what is responded to vStopped are the
same packets that are replies to the resumption packets (c/s/vCont),
which indicate a stop, hence, "stop replies". Generalize this, and
"replies" no longer sounds clear to me. All sorts of packets have their
own replies, but "reply for a remote notification" is strange, because
the real notifications ("%Stop", etc.) by design don't _have_ any sort of
reply back to the stub. In a way, you could consider vStopped the reply
to a notification. See my confusion?
What do you think of something like renaming struct notif_reply
to struct notif, or struct notif_info (drawing a parallel to signals,
with "Stop"==signo, notification payload==siginfo) and then,
struct notif into struct notif_handler/notif_manager/notif_whatnot.
For the latter, maybe, since we have both client and server sides,
we could go with struct notif => struct notif_client in gdb, and
in gdbserver struct notif => struct notif_server.
> +{
> + ptid_t ptid;
Same comment as in gdbserver. Seems odd to me that this is part of the "base class".
> + /* Destructor. Release everything from SELF, but not SELF itself. */
> + void (*dtr) (struct notif_reply *self);
> +} *notif_reply_p;
> +
> +DECLARE_QUEUE_P (notif_reply_p);
> +
> +/* Data and operations related to async notification. */
> +
> +typedef struct notif
> +{
> + /* 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 reply. This function may throw
> + exception if contents in BUF is not the expected reply. */
> + void (*parse) (struct notif *self, char *buf, void *data);
> +
> + /* Send field <ack_command> to remote, and do some checking. If
> + something wrong, throw exception. */
> + void (*ack) (struct notif *self, char *buf, void *data);
> +
> + /* Allocate a reply. */
> + struct notif_reply *(*alloc_reply) (void);
> +
> + /* Release memory for sub-class. */
> + void (*dtr) (struct notif *self);
> +
> + /* One pending reply. 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_reply' 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_reply *pending_reply;
> +} *notif_p;
> +
> +/* A stop notification. */
> +
> +struct notif_stop
> +{
> + struct notif base;
> + /* The list of already fetched and acknowledged events. */
> + QUEUE (notif_reply_p) *ack_queue;
> +};
I'm confused on the abstraction layers here. So other notifications
won't need a queue? How do they work then? And then, if only stops
need it, why do we need to stuff it in a new type, and come up with
"struct notif_stop" at all? This seems to be related to the comment
in notif.c that talks about stop replies being different. Then, it seems
we're just adding layers for no no real generalization. I'd rather
just leave the queue global as it was, and make notif_packet_stop a plain
struct notif. Make struct notif|notif.c as simple as possible, a layer
to build uppon, with no knowlege of the sub types of notifications.
> +
> +DECLARE_QUEUE_P (notif_p);
> +
> +void remote_notif_ack (struct notif *np, char *buf);
> +struct notif_reply *remote_notif_parse (struct notif *np, char *buf);
> +
> +void handle_notification (char *buf);
> +
> +void remote_notif_register_async_event_handler (void);
> +void remote_notif_unregister_async_event_handler (void);
> +
> +void notif_reply_xfree (struct notif_reply *reply);
> +
> +void remote_notif_discard_replies (int pid);
> +void remote_notif_process (struct notif *except);
> +
> +extern struct notif_stop notif_packet_stop;
> +
> +extern unsigned int notif_debug;
> +
> +#define DEBUG_NOTIF(fmt, args...) \
> + if (notif_debug) \
> + fprintf_unfiltered (gdb_stdlog, "notif: " fmt "\n", ##args);
I think this is a gcc extension, and as such we can't use it. Are
there uses in the tree already?
> +
> +#endif /* REMOTE_NOTIF_H */
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 2239298..a72b364 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -34,6 +34,7 @@
> #include "gdb-stabs.h"
> #include "gdbthread.h"
> #include "remote.h"
> +#include "remote-notif.h"
> #include "regcache.h"
> #include "value.h"
> #include "gdb_assert.h"
> @@ -223,17 +224,13 @@ static void remote_check_symbols (struct objfile *objfile);
> void _initialize_remote (void);
>
> struct stop_reply;
> -static struct stop_reply *stop_reply_xmalloc (void);
> -static void stop_reply_xfree (struct stop_reply *);
> -static void do_stop_reply_xfree (void *arg);
> -static void remote_parse_stop_reply (char *buf, struct stop_reply *);
> -static void push_stop_reply (struct stop_reply *);
> -static void remote_get_pending_stop_replies (void);
> -static void discard_pending_stop_replies (int pid);
> +
> static int peek_stop_reply (ptid_t ptid);
> +static void remote_parse_stop_reply (char *buf, struct stop_reply *event);
> +static void notif_stop_reply_push (struct notif_stop *notif,
> + struct notif_reply *new_event);
>
> static void remote_async_inferior_event_handler (gdb_client_data);
> -static void remote_async_get_pending_events_handler (gdb_client_data);
>
> static void remote_terminal_ours (void);
>
> @@ -245,11 +242,6 @@ static int remote_supports_cond_breakpoints (void);
>
> static int remote_can_run_breakpoint_commands (void);
>
> -/* The non-stop remote protocol provisions for one pending stop reply.
> - This is where we keep it until it is acknowledged. */
> -
> -static struct stop_reply *pending_stop_reply = NULL;
> -
> /* For "remote". */
>
> static struct cmd_list_element *remote_cmdlist;
> @@ -1400,14 +1392,8 @@ static struct async_signal_handler *sigint_remote_token;
> /* Asynchronous signal handle registered as event loop source for
> when we have pending events ready to be passed to the core. */
>
> -static struct async_event_handler *remote_async_inferior_event_token;
> -
> -/* Asynchronous signal handle registered as event loop source for when
> - the remote sent us a %Stop notification. The registered callback
> - will do a vStopped sequence to pull the rest of the events out of
> - the remote side into our event queue. */
> +struct async_event_handler *remote_async_inferior_event_token;
>
> -static struct async_event_handler *remote_async_get_pending_events_token;
>
>
> static ptid_t magic_null_ptid;
> @@ -3023,12 +3009,12 @@ remote_close (int quitting)
> discard_all_inferiors ();
>
> /* We're no longer interested in any of these events. */
> - discard_pending_stop_replies (-1);
> + remote_notif_discard_replies (-1);
>
> if (remote_async_inferior_event_token)
> delete_async_event_handler (&remote_async_inferior_event_token);
> - if (remote_async_get_pending_events_token)
> - delete_async_event_handler (&remote_async_get_pending_events_token);
> +
> + remote_notif_unregister_async_event_handler ();
> }
>
> /* Query the remote side for the text, data and bss offsets. */
> @@ -3450,19 +3436,12 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
> mechanism. */
> if (strcmp (rs->buf, "OK") != 0)
> {
> - struct stop_reply *stop_reply;
> - struct cleanup *old_chain;
> -
> - stop_reply = stop_reply_xmalloc ();
> - old_chain = make_cleanup (do_stop_reply_xfree, stop_reply);
> -
> - remote_parse_stop_reply (rs->buf, stop_reply);
> - discard_cleanups (old_chain);
> -
> - /* get_pending_stop_replies acks this one, and gets the rest
> + /* remote_notif_pending_replies acks this one, and gets the rest
> out. */
> - pending_stop_reply = stop_reply;
> - remote_get_pending_stop_replies ();
> + notif_packet_stop.base.pending_reply
> + = remote_notif_parse ((struct notif *) ¬if_packet_stop,
> + rs->buf);
> + remote_notif_pending_replies ((struct notif *) ¬if_packet_stop);
IMO, losing the "get" makes the code a little less clear.
>
> /* Make sure that threads that were stopped remain
> stopped. */
> @@ -4222,9 +4201,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_async_get_pending_events_token
> - = create_async_event_handler (remote_async_get_pending_events_handler,
> - NULL);
> + remote_notif_register_async_event_handler ();
>
> /* Reset the target state; these things will be queried either by
> remote_query_supported or as they are needed. */
> @@ -4351,7 +4328,7 @@ remote_detach_1 (char *args, int from_tty, int extended)
> if (from_tty && !extended)
> puts_filtered (_("Ending remote debugging.\n"));
>
> - discard_pending_stop_replies (pid);
> + remote_notif_discard_replies (pid);
> target_mourn_inferior ();
> }
>
> @@ -4480,14 +4457,11 @@ extended_remote_attach_1 (struct target_ops *target, char *args, int from_tty)
>
> if (target_can_async_p ())
> {
> - struct stop_reply *stop_reply;
> - struct cleanup *old_chain;
> + struct notif_reply *reply
> + = remote_notif_parse ((struct notif *) ¬if_packet_stop,
> + wait_status);
>
> - stop_reply = stop_reply_xmalloc ();
> - old_chain = make_cleanup (do_stop_reply_xfree, stop_reply);
> - remote_parse_stop_reply (wait_status, stop_reply);
> - discard_cleanups (old_chain);
> - push_stop_reply (stop_reply);
> + notif_stop_reply_push (¬if_packet_stop, reply);
>
> target_async (inferior_event_handler, 0);
> }
> @@ -5116,9 +5090,7 @@ DEF_VEC_O(cached_reg_t);
>
> struct stop_reply
> {
> - struct stop_reply *next;
> -
> - ptid_t ptid;
> + struct notif_reply base;
>
> struct target_waitstatus ws;
>
> @@ -5137,73 +5109,117 @@ struct stop_reply
> int core;
> };
>
> -/* The list of already fetched and acknowledged stop events. */
> -static struct stop_reply *stop_reply_queue;
> +static void
> +remote_notif_parse_stop (struct notif *self, char *buf, void *data)
> +{
> + remote_parse_stop_reply (buf, (struct stop_reply *) data);
> +}
>
> -static struct stop_reply *
> -stop_reply_xmalloc (void)
> +static void
> +remote_notif_ack_stop (struct notif *self, char *buf, void *data)
> {
> - struct stop_reply *r = XMALLOC (struct stop_reply);
> + struct stop_reply *stop_reply = (struct stop_reply *) data;
>
> - r->next = NULL;
> - return r;
> + /* acknowledge */
> + putpkt ((char *) self->ack_command);
> +
> + if (stop_reply->ws.kind == TARGET_WAITKIND_IGNORE)
> + /* We got an unknown stop reply. */
> + error (_("Unknown stop reply"));
> +
> + notif_stop_reply_push ((struct notif_stop *) self,
> + (struct notif_reply *) stop_reply);
> }
>
> static void
> -stop_reply_xfree (struct stop_reply *r)
> +stop_reply_dtr (struct notif_reply *reply)
> {
> + struct stop_reply *r = (struct stop_reply *) reply;
> +
> if (r != NULL)
It makes sense for an xfree function to accept a NULL self,
but not so much a destructor.
> - {
> - VEC_free (cached_reg_t, r->regcache);
> - xfree (r);
> - }
> + VEC_free (cached_reg_t, r->regcache);
> }
>
> -/* Discard all pending stop replies of inferior PID. If PID is -1,
> - discard everything. */
> +static struct notif_reply *
> +remote_notif_alloc_reply_stop (void)
> +{
> + struct notif_reply *r = (struct notif_reply *) XMALLOC (struct stop_reply);
No need for the cast. See XMALLOC's definition.
> +
> + r->dtr = stop_reply_dtr;
> +
> + return r;
> +}
> +
> +/* Destructor of 'notif_stop' SELF. */
>
> static void
> -discard_pending_stop_replies (int pid)
> +remote_notif_dtr_stop (struct notif *self)
> {
> - struct stop_reply *prev = NULL, *reply, *next;
> + struct notif_stop *notif = (struct notif_stop *) self;
>
> - /* Discard the in-flight notification. */
> - if (pending_stop_reply != NULL
> - && (pid == -1
> - || ptid_get_pid (pending_stop_reply->ptid) == pid))
> - {
> - stop_reply_xfree (pending_stop_reply);
> - pending_stop_reply = NULL;
> - }
> + if (notif->ack_queue != NULL)
> + QUEUE_free (notif_reply_p, notif->ack_queue);
> +}
>
> - /* Discard the stop replies we have already pulled with
> - vStopped. */
> - for (reply = stop_reply_queue; reply; reply = next)
> - {
> - next = reply->next;
> - if (pid == -1
> - || ptid_get_pid (reply->ptid) == pid)
> - {
> - if (reply == stop_reply_queue)
> - stop_reply_queue = reply->next;
> - else
> - prev->next = reply->next;
> +struct notif_stop notif_packet_stop =
> +{
> + {
> + "Stop",
> + "vStopped",
> + remote_notif_parse_stop,
> + remote_notif_ack_stop,
> + remote_notif_alloc_reply_stop,
> + remote_notif_dtr_stop,
> + NULL,
> + },
> + NULL,
> +};
>
> - stop_reply_xfree (reply);
> - }
> - else
> - prev = reply;
> +/* A parameter to pass data in and out. */
> +
> +struct queue_iter_param
> +{
> + void *input;
> + struct notif_reply *output;
> +};
> +
> +static int
> +remote_notif_remove_once_on_match (QUEUE (notif_reply_p) *q,
> + QUEUE_ITER (notif_reply_p) *iter,
> + notif_reply_p reply,
> + void *data)
> +{
> + struct queue_iter_param *param = data;
> + ptid_t *ptid = param->input;
> +
> + if (ptid_match (reply->ptid, *ptid))
> + {
> + param->output = reply;
> + QUEUE_remove_elem (notif_reply_p, q, iter);
> + return 0;
> }
> +
> + return 1;
> }
>
> -/* Cleanup wrapper. */
> +/* Remove the first reply in NP->ack_queue if function MATCH returns
> + true. */
>
> -static void
> -do_stop_reply_xfree (void *arg)
> +static struct notif_reply *
> +remote_notif_discard_queued_reply (struct notif_stop *np, ptid_t ptid)
> {
> - struct stop_reply *r = arg;
> + struct queue_iter_param param;
> +
> + param.input = &ptid;
> + param.output = NULL;
> +
> + QUEUE_iterate (notif_reply_p, np->ack_queue,
> + remote_notif_remove_once_on_match, ¶m);
> +
> + DEBUG_NOTIF ("discard queued reply: '%s' in %s", np->base.name,
> + target_pid_to_str (ptid));
>
> - stop_reply_xfree (r);
> + return param.output;
> }
>
> /* Look for a queued stop reply belonging to PTID. If one is found,
> @@ -5214,70 +5230,54 @@ do_stop_reply_xfree (void *arg)
> static struct stop_reply *
> queued_stop_reply (ptid_t ptid)
> {
> - struct stop_reply *it;
> - struct stop_reply **it_link;
> -
> - it = stop_reply_queue;
> - it_link = &stop_reply_queue;
> - while (it)
> - {
> - if (ptid_match (it->ptid, ptid))
> - {
> - *it_link = it->next;
> - it->next = NULL;
> - break;
> - }
> -
> - it_link = &it->next;
> - it = *it_link;
> - }
> + struct notif_reply *r
> + = remote_notif_discard_queued_reply (¬if_packet_stop, ptid);
Hmm, the end result is confusing. The "discard" method returns
a reply, but doesn't actually destroy/discard it? It would be better
renamed to something else.
>
> - if (stop_reply_queue)
> + if (!QUEUE_is_empty (notif_reply_p, notif_packet_stop.ack_queue))
> /* There's still at least an event left. */
> mark_async_event_handler (remote_async_inferior_event_token);
>
> - return it;
> + return (struct stop_reply *) r;
> }
>
> -/* Push a fully parsed stop reply in the stop reply queue. Since we
> +/* Push a fully parsed reply in the reply queue. Since we
> know that we now have at least one queued event left to pass to the
> - core side, tell the event loop to get back to target_wait soon. */
> + core side, tell the event loop to get back to target_wait soon if
> + the event is about stop, otherwise don't have to back to target_wait. */
>
> static void
> -push_stop_reply (struct stop_reply *new_event)
> +notif_stop_reply_push (struct notif_stop *notif,
> + struct notif_reply *new_event)
> {
> - struct stop_reply *event;
> + QUEUE_enque (notif_reply_p, notif->ack_queue, new_event);
>
> - if (stop_reply_queue)
> - {
> - for (event = stop_reply_queue;
> - event && event->next;
> - event = event->next)
> - ;
> -
> - event->next = new_event;
> - }
> - else
> - stop_reply_queue = new_event;
> + DEBUG_NOTIF ("push '%s' %s to queue %d", notif->base.name,
> + target_pid_to_str (new_event->ptid),
> + QUEUE_length (notif_reply_p, notif->ack_queue));
>
> mark_async_event_handler (remote_async_inferior_event_token);
> }
>
> +static int
> +stop_reply_match_ptid_and_ws (QUEUE (notif_reply_p) *q,
> + QUEUE_ITER (notif_reply_p) *iter,
> + struct notif_reply *reply,
> + void *data)
> +{
> + ptid_t *ptid = data;
> + struct stop_reply *r = (struct stop_reply *) reply;
> +
> + return !(ptid_equal (*ptid, r->base.ptid)
> + && r->ws.kind == TARGET_WAITKIND_STOPPED);
> +}
> +
> /* Returns true if we have a stop reply for PTID. */
>
> static int
> peek_stop_reply (ptid_t ptid)
> {
> - struct stop_reply *it;
> -
> - for (it = stop_reply_queue; it; it = it->next)
> - if (ptid_equal (ptid, it->ptid))
> - {
> - if (it->ws.kind == TARGET_WAITKIND_STOPPED)
> - return 1;
> - }
> -
> - return 0;
> + return !QUEUE_iterate (notif_reply_p, notif_packet_stop.ack_queue,
> + stop_reply_match_ptid_and_ws, &ptid);
> }
>
> /* Parse the stop reply in BUF. Either the function succeeds, and the
> @@ -5290,7 +5290,7 @@ remote_parse_stop_reply (char *buf, struct stop_reply *event)
> ULONGEST addr;
> char *p;
>
> - event->ptid = null_ptid;
> + event->base.ptid = null_ptid;
> event->ws.kind = TARGET_WAITKIND_IGNORE;
> event->ws.value.integer = 0;
> event->solibs_changed = 0;
> @@ -5342,7 +5342,7 @@ remote_parse_stop_reply (char *buf, struct stop_reply *event)
> Packet: '%s'\n"),
> p, buf);
> if (strncmp (p, "thread", p1 - p) == 0)
> - event->ptid = read_ptid (++p1, &p);
> + event->base.ptid = read_ptid (++p1, &p);
> else if ((strncmp (p, "watch", p1 - p) == 0)
> || (strncmp (p, "rwatch", p1 - p) == 0)
> || (strncmp (p, "awatch", p1 - p) == 0))
> @@ -5484,21 +5484,21 @@ Packet: '%s'\n"),
> }
> else
> error (_("unknown stop reply packet: %s"), buf);
> - event->ptid = pid_to_ptid (pid);
> + event->base.ptid = pid_to_ptid (pid);
> }
> break;
> }
>
> - if (non_stop && ptid_equal (event->ptid, null_ptid))
> + if (non_stop && ptid_equal (event->base.ptid, null_ptid))
> error (_("No process or thread specified in stop reply: %s"), buf);
> }
>
> -/* When the stub wants to tell GDB about a new stop reply, it sends a
> - stop notification (%Stop). Those can come it at any time, hence,
> - we have to make sure that any pending putpkt/getpkt sequence we're
> - making is finished, before querying the stub for more events with
> - vStopped. E.g., if we started a vStopped sequence immediatelly
> - upon receiving the %Stop notification, something like this could
> +/* When the stub wants to tell GDB about a new notification reply, it sends a
> + notification (%Stop, for example). Those can come it at any time, hence,
> + we have to make sure that any pending putpkt/getpkt sequence we're making
> + is finished, before querying the stub for more events with the corresponding
> + ack command (vStopped, for example). E.g., if we started a vStopped sequence
> + immediately upon receiving the notification, something like this could
> happen:
>
> 1.1) --> Hg 1
> @@ -5534,19 +5534,17 @@ Packet: '%s'\n"),
> 2.9) --> OK
> */
>
> -static void
> -remote_get_pending_stop_replies (void)
> +void
> +remote_notif_pending_replies (struct notif *np)
> {
> struct remote_state *rs = get_remote_state ();
>
> - if (pending_stop_reply)
> + if (np->pending_reply)
> {
> + DEBUG_NOTIF ("process: '%s' ack pending reply", np->name);
> /* acknowledge */
> - putpkt ("vStopped");
> -
> - /* Now we can rely on it. */
> - push_stop_reply (pending_stop_reply);
> - pending_stop_reply = NULL;
> + np->ack (np, rs->buf, np->pending_reply);
> + np->pending_reply = NULL;
>
> while (1)
> {
> @@ -5554,31 +5552,15 @@ remote_get_pending_stop_replies (void)
> if (strcmp (rs->buf, "OK") == 0)
> break;
> else
> - {
> - struct cleanup *old_chain;
> - struct stop_reply *stop_reply = stop_reply_xmalloc ();
> -
> - old_chain = make_cleanup (do_stop_reply_xfree, stop_reply);
> - remote_parse_stop_reply (rs->buf, stop_reply);
> -
> - /* acknowledge */
> - putpkt ("vStopped");
> -
> - if (stop_reply->ws.kind != TARGET_WAITKIND_IGNORE)
> - {
> - /* Now we can rely on it. */
> - discard_cleanups (old_chain);
> - push_stop_reply (stop_reply);
> - }
> - else
> - /* We got an unknown stop reply. */
> - do_cleanups (old_chain);
> - }
> + remote_notif_ack (np, rs->buf);
> }
> }
> + else
> + {
> + DEBUG_NOTIF ("process: '%s' no pending reply", np->name);
> + }
> }
>
> -
> /* Called when it is decided that STOP_REPLY holds the info of the
> event that is to be returned to the core. This function always
> destroys STOP_REPLY. */
> @@ -5590,7 +5572,7 @@ process_stop_reply (struct stop_reply *stop_reply,
> ptid_t ptid;
>
> *status = stop_reply->ws;
> - ptid = stop_reply->ptid;
> + ptid = stop_reply->base.ptid;
>
> /* If no thread/process was reported by the stub, assume the current
> inferior. */
> @@ -5622,7 +5604,7 @@ process_stop_reply (struct stop_reply *stop_reply,
> demand_private_info (ptid)->core = stop_reply->core;
> }
>
> - stop_reply_xfree (stop_reply);
> + notif_reply_xfree ((struct notif_reply *) stop_reply);
> return ptid;
> }
>
> @@ -5661,8 +5643,8 @@ 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 (pending_stop_reply != NULL)
> - remote_get_pending_stop_replies ();
> + if (notif_packet_stop.base.pending_reply != NULL)
> + remote_notif_pending_replies ((struct notif *) ¬if_packet_stop);
>
> /* If indeed we noticed a stop reply, we're done. */
> stop_reply = queued_stop_reply (ptid);
> @@ -5758,14 +5740,11 @@ remote_wait_as (ptid_t ptid, struct target_waitstatus *status, int options)
> break;
> case 'T': case 'S': case 'X': case 'W':
> {
> - struct stop_reply *stop_reply;
> - struct cleanup *old_chain;
> -
> - stop_reply = stop_reply_xmalloc ();
> - old_chain = make_cleanup (do_stop_reply_xfree, stop_reply);
> - remote_parse_stop_reply (buf, stop_reply);
> - discard_cleanups (old_chain);
> - event_ptid = process_stop_reply (stop_reply, status);
> + struct notif_reply *reply
> + = remote_notif_parse ((struct notif *) ¬if_packet_stop,
> + rs->buf);
> +
> + event_ptid = process_stop_reply ((struct stop_reply *) reply, status);
> break;
> }
> case 'O': /* Console output. */
> @@ -5845,7 +5824,7 @@ remote_wait (struct target_ops *ops,
> {
> /* If there are are events left in the queue tell the event loop
> to return here. */
> - if (stop_reply_queue)
> + if (!QUEUE_is_empty (notif_reply_p, notif_packet_stop.ack_queue))
> mark_async_event_handler (remote_async_inferior_event_token);
> }
>
> @@ -6741,52 +6720,6 @@ remote_read_bytes (CORE_ADDR memaddr, gdb_byte *myaddr, int len)
> return i;
> }
>
> -
> -/* Remote notification handler. */
> -
> -static void
> -handle_notification (char *buf)
> -{
> - if (strncmp (buf, "Stop:", 5) == 0)
> - {
> - if (pending_stop_reply)
> - {
> - /* We've already parsed the in-flight stop-reply, but the
> - stub for some reason thought we didn't, possibly due to
> - timeout on its side. Just ignore it. */
> - if (remote_debug)
> - fprintf_unfiltered (gdb_stdlog, "ignoring resent notification\n");
> - }
> - else
> - {
> - struct cleanup *old_chain;
> - struct stop_reply *reply = stop_reply_xmalloc ();
> -
> - old_chain = make_cleanup (do_stop_reply_xfree, reply);
> -
> - remote_parse_stop_reply (buf + 5, reply);
> -
> - discard_cleanups (old_chain);
> -
> - /* Be careful to only set it after parsing, since an error
> - may be thrown then. */
> - pending_stop_reply = reply;
> -
> - /* Notify the event loop there's a stop reply to acknowledge
> - and that there may be more events to fetch. */
> - mark_async_event_handler (remote_async_get_pending_events_token);
> -
> - if (remote_debug)
> - fprintf_unfiltered (gdb_stdlog, "stop notification captured\n");
> - }
> - }
> - else
> - /* We ignore notifications we don't recognize, for compatibility
> - with newer stubs. */
> - ;
> -}
> -
> -
> /* Read or write LEN bytes from inferior memory at MEMADDR,
> transferring to or from debugger address BUFFER. Write to inferior
> if SHOULD_WRITE is nonzero. Returns length of data written or
> @@ -7656,7 +7589,7 @@ extended_remote_mourn_1 (struct target_ops *target)
> rs->waiting_for_stop_reply = 0;
>
> /* We're no longer interested in these events. */
> - discard_pending_stop_replies (ptid_get_pid (inferior_ptid));
> + remote_notif_discard_replies (ptid_get_pid (inferior_ptid));
>
> /* If the current general thread belonged to the process we just
> detached from or has exited, the remote side current general
> @@ -11188,12 +11121,6 @@ remote_async_inferior_event_handler (gdb_client_data data)
> }
>
> static void
> -remote_async_get_pending_events_handler (gdb_client_data data)
> -{
> - remote_get_pending_stop_replies ();
> -}
> -
> -static void
> remote_async (void (*callback) (enum inferior_event_type event_type,
> void *context), void *context)
> {
> diff --git a/gdb/remote.h b/gdb/remote.h
> index 3adc54e..fa896c2 100644
> --- a/gdb/remote.h
> +++ b/gdb/remote.h
> @@ -19,6 +19,9 @@
> #ifndef REMOTE_H
> #define REMOTE_H
>
> +#include "target.h"
Why did you need this one?
> +#include "remote-notif.h"
> +
> struct target_desc;
>
> /* Read a packet from the remote machine, with error checking, and
> @@ -40,6 +43,7 @@ extern int putpkt (char *buf);
> extern char *unpack_varlen_hex (char *buff, ULONGEST *result);
>
> extern void async_remote_interrupt_twice (void *arg);
> +extern struct async_event_handler *remote_async_inferior_event_token;
>
> void register_remote_g_packet_guess (struct gdbarch *gdbarch, int bytes,
> const struct target_desc *tdesc);
> @@ -59,4 +63,5 @@ extern int remote_register_number_and_offset (struct gdbarch *gdbarch,
> int regnum, int *pnum,
> int *poffset);
>
> +extern void remote_notif_pending_replies (struct notif *np);;
> #endif
> -- 1.7.7.6
>
--
Pedro Alves