* [PATCH 3/4] de-couple %Stop from notification: gdbserver
2012-08-24 2:26 [RCF 0/4] A general notification in GDB RSP Yao Qi
2012-08-24 2:26 ` [PATCH 4/4] new notification "TEST" Yao Qi
2012-08-24 2:26 ` [PATCH 1/4] new gdb_queue.h in common/ Yao Qi
@ 2012-08-24 2:26 ` Yao Qi
2012-08-24 2:26 ` [PATCH 2/4] de-couple %Stop from notification: gdb Yao Qi
2012-08-24 17:53 ` [RCF 0/4] A general notification in GDB RSP Pedro Alves
4 siblings, 0 replies; 19+ messages in thread
From: Yao Qi @ 2012-08-24 2:26 UTC (permalink / raw)
To: gdb-patches
gdb/gdbserver:
2012-08-24 Yao Qi <yao@codesourcery.com>
* Makefile.in (OBS): Add notif.o.
(notif_h, gdb_queue_h): New.
(notif.o): New rule.
* server.c: Include 'notif.h'.
(struct vstop_notif): Move it to notif.h.
(notif_process_stop): New.
(notif_stop): New variable.
(push_event, notif_queue): Remove.
(queue_stop_reply): Remove parameters. Add two new parameters
'new_notif' and 'queue'.
Caller update.
(vstop_notif_match_pid, notif_queued_replies_discard): New.
(notif_queued_replies_discard_all): New.
(discard_queued_stop_replies, send_next_stop_reply): Remove.
(handle_v_kill): Don't call discard_queued_stop_replies. Call
notif_queued_replies_discard_all instead.
(handle_v_stopped): Remove.
(handle_v_requests): Don't call handle_v_stopped. Call
handle_ack_notif instead.
(queue_stop_reply_callback): Call notif_reply_enqueue instead
of queue_stop_reply.
(handle_status): Don't call discard_queued_stop_replies. Call
notif_queued_replies_discard instead.
(kill_inferior_callback): Likewise.
(detach_or_kill_inferior_callback): Likewise.
(process_serial_event): Likewise. Call
notif_queued_replies_empty_p.
(handle_target_event): Process queued replies.
* server.h: Remove declaration of push_event.
---
gdb/gdbserver/Makefile.in | 5 +-
gdb/gdbserver/linux-low.c | 9 ++-
gdb/gdbserver/notif.c | 152 +++++++++++++++++++++++++++++++
gdb/gdbserver/notif.h | 79 ++++++++++++++++
gdb/gdbserver/server.c | 218 ++++++++++++++++-----------------------------
gdb/gdbserver/server.h | 2 -
6 files changed, 318 insertions(+), 147 deletions(-)
create mode 100644 gdb/gdbserver/notif.c
create mode 100644 gdb/gdbserver/notif.h
diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
index f62799e..68f6b23 100644
--- a/gdb/gdbserver/Makefile.in
+++ b/gdb/gdbserver/Makefile.in
@@ -156,7 +156,7 @@ OBS = agent.o ax.o inferiors.o regcache.o remote-utils.o server.o signals.o targ
utils.o version.o vec.o gdb_vecs.o \
mem-break.o hostio.o event-loop.o tracepoint.o \
xml-utils.o common-utils.o ptid.o buffer.o format.o \
- dll.o \
+ dll.o notif.o \
$(XML_BUILTIN) \
$(DEPFILES) $(LIBOBJS)
GDBREPLAY_OBS = gdbreplay.o version.o
@@ -407,6 +407,7 @@ gdb_proc_service_h = $(srcdir)/gdb_proc_service.h
regdat_sh = $(srcdir)/../regformats/regdat.sh
regdef_h = $(srcdir)/../regformats/regdef.h
regcache_h = $(srcdir)/regcache.h
+gdb_queue_h = $(srcdir)/../common/gdb_queue.h
signals_def = $(srcdir)/../../include/gdb/signals.def
signals_h = $(srcdir)/../../include/gdb/signals.h $(signals_def)
ptid_h = $(srcdir)/../common/ptid.h
@@ -428,6 +429,7 @@ server_h = $(srcdir)/server.h $(regcache_h) $(srcdir)/target.h \
$(ptid_h) \
$(signals_h) \
$(generated_files)
+notif_h = ${srcdir}/notif.h
gdbthread_h = $(srcdir)/gdbthread.h $(target_h) $(srcdir)/server.h
linux_low_h = $(srcdir)/linux-low.h $(gdbthread_h)
@@ -485,6 +487,7 @@ proc-service.o: proc-service.c $(server_h) $(gdb_proc_service_h)
regcache.o: regcache.c $(server_h) $(regdef_h) $(gdbthread_h)
remote-utils.o: remote-utils.c terminal.h $(server_h) $(gdbthread_h)
server.o: server.c $(server_h) $(agent_h) $(gdbthread_h)
+notif.o: notif.c $(notif_h) $(gdb_queue_h)
target.o: target.c $(server_h)
thread-db.o: thread-db.c $(server_h) $(linux_low_h) $(gdb_proc_service_h) \
$(gdb_thread_db_h) $(gdb_vecs_h)
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index a476031..e9752b0 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -20,6 +20,7 @@
#include "linux-low.h"
#include "linux-osdata.h"
#include "agent.h"
+#include "notif.h"
#include <sys/wait.h>
#include <stdio.h>
@@ -2787,6 +2788,8 @@ async_file_mark (void)
be awakened anyway. */
}
+extern struct notif notif_stop;
+
static ptid_t
linux_wait (ptid_t ptid,
struct target_waitstatus *ourstatus, int target_options)
@@ -2807,7 +2810,11 @@ linux_wait (ptid_t ptid,
if (target_is_async_p ()
&& (target_options & TARGET_WNOHANG) != 0
&& !ptid_equal (event_ptid, null_ptid))
- async_file_mark ();
+ {
+ gdb_queue_notif_enque (¬if_stop, ¬if_queue);
+
+ async_file_mark ();
+ }
return event_ptid;
}
diff --git a/gdb/gdbserver/notif.c b/gdb/gdbserver/notif.c
new file mode 100644
index 0000000..838f391
--- /dev/null
+++ b/gdb/gdbserver/notif.c
@@ -0,0 +1,152 @@
+/* Notification to GDB.
+ Copyright (C) 1989, 1993-1995, 1997-2000, 2002-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/>. */
+
+#include "notif.h"
+
+extern struct notif notif_stop;
+
+static struct notif *notif_packets [] =
+{
+ ¬if_stop,
+ NULL,
+};
+
+void
+gdb_queue_ele_notif_xfree (struct notif *notif)
+{
+ /* It is a placeholder. Never be called. */
+ gdb_assert (0);
+}
+
+QUEUE_DEFINE_TYPE(notif);
+
+QUEUE_DEFINE_VAR(notif, notif_queue);
+
+void
+gdb_queue_ele_notif_reply_xfree (struct notif_reply *reply)
+{
+ /* Nothing to release for 'struct notif_reply' and its sub-classes. */
+}
+
+QUEUE_DEFINE_TYPE(notif_reply);
+
+extern int remote_debug;
+
+/* Push another reply, or if there are no more left, an OK. */
+
+void
+notif_send_reply (struct notif *notif, char *own_buf)
+{
+ struct notif_reply *reply = gdb_queue_notif_reply_peek (notif->queue);
+
+ if (reply != NULL)
+ notif->reply (reply, own_buf);
+ else
+ write_ok (own_buf);
+}
+
+/* Handle the ack in buffer OWN_BUF. Return 1 if the ack is handled, and
+ return 0 if the contents in OWN_BUF is not a ack. */
+
+int
+notif_ack_handle (char *own_buf)
+{
+ int i = 0;
+ struct notif *np = NULL;
+
+ for (i = 0; notif_packets[i] != NULL; i++)
+ {
+ np = notif_packets[i];
+ if (strncmp (own_buf, np->name, strlen (np->name)) == 0)
+ break;
+ }
+
+ if (np == NULL)
+ return 0;
+
+ /* If we're waiting for GDB to acknowledge a pending reply,
+ consider that done. */
+ if (!gdb_queue_notif_reply_is_empty (np->queue))
+ {
+ struct notif_reply *head = gdb_queue_notif_reply_deque (np->queue);
+
+ if (remote_debug)
+ fprintf (stderr, "%s: acking\n", np->name);
+
+ xfree (head);
+ }
+
+ notif_send_reply (np, own_buf);
+
+ return 1;
+}
+
+/* Return true if the queues of all notifications are empty. If the queue of
+ one of notification is not empty, return false. */
+
+int
+notif_queued_replies_empty_p (void)
+{
+ int i;
+
+ for (i = 0; notif_packets[i] != NULL; i++)
+ if (!gdb_queue_notif_reply_is_empty (notif_packets[i]->queue))
+ return 0;
+
+ return 1;
+}
+
+static int
+notif_reply_match_pid (struct notif_reply *reply, void *data)
+{
+ int *pid = data;
+
+ return (*pid == -1 || ptid_get_pid (reply->ptid) == *pid);
+}
+
+/* Get rid of the currently pending replies of NOTIF for PID. If PID is
+ -1, then apply to all processes. */
+
+void
+notif_queued_replies_discard (int pid, struct notif *notif)
+{
+ gdb_queue_notif_reply_remove_all (notif->queue, notif_reply_match_pid,
+ &pid);
+}
+
+/* Get rid of pending replies of all notifications. */
+
+void
+notif_queued_replies_discard_all (int pid)
+{
+ int i;
+
+ for (i = 0; notif_packets[i] != NULL; i++)
+ notif_queued_replies_discard (pid, notif_packets[i]);
+}
+
+void
+initialize_notif (void)
+{
+ int i = 0;
+
+ for (i = 0; notif_packets[i] != NULL; i++)
+ notif_packets[i]->queue = gdb_queue_notif_reply_alloc ();
+
+}
diff --git a/gdb/gdbserver/notif.h b/gdb/gdbserver/notif.h
new file mode 100644
index 0000000..ac62eab
--- /dev/null
+++ b/gdb/gdbserver/notif.h
@@ -0,0 +1,79 @@
+/* Notification to GDB.
+ Copyright (C) 1989, 1993-1995, 1997-2000, 2002-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/>. */
+
+#include "ptid.h"
+#include "server.h"
+#include "target.h"
+#include "gdb_queue.h"
+
+/* Structure holding information relative to a single reply. We
+ keep a queue of these (really a singly-linked list) to push to GDB
+ in non-stop mode. */
+
+struct notif_reply
+{
+ /* Thread or process that got the event. */
+ ptid_t ptid;
+};
+
+/* A sub-class of 'struct ack_notif' for stop reply. */
+
+struct vstop_notif
+{
+ struct notif_reply base;
+
+ /* Event info. */
+ struct target_waitstatus status;
+};
+
+enum notif_type { NOTIF_STOP };
+
+/* A notification to GDB. */
+
+struct notif
+{
+ /* The name of ack packet, for example, 'vStopped'. */
+ const char *name;
+
+ /* The notification packet, for example, '%Stop'. Note that '%' is not in
+ 'notif_name'. */
+ const char *notif_name;
+
+ const enum notif_type type;
+
+ /* A queue of reply to GDB. */
+ struct gdb_queue_notif_reply *queue;
+
+ /* Reply to GDB. */
+ void (*reply) (struct notif_reply *reply, char *own_buf);
+};
+
+int notif_ack_handle (char *own_buf);
+void notif_send_reply (struct notif *notif, char *own_buf);
+
+int notif_queued_replies_empty_p (void);
+void notif_queued_replies_discard (int pid, struct notif *notif);
+void notif_queued_replies_discard_all (int pid);
+
+extern struct gdb_queue_notif notif_queue;
+
+QUEUE_DECLARE (notif);
+QUEUE_DECLARE (notif_reply);
+
+void initialize_notif (void);
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 547552f..0b756cd 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -20,6 +20,7 @@
#include "server.h"
#include "gdbthread.h"
#include "agent.h"
+#include "notif.h"
#if HAVE_UNISTD_H
#include <unistd.h>
@@ -117,121 +118,28 @@ static ptid_t last_ptid;
static char *own_buf;
static unsigned char *mem_buf;
-/* Structure holding information relative to a single stop reply. We
- keep a queue of these (really a singly-linked list) to push to GDB
- in non-stop mode. */
-struct vstop_notif
-{
- /* Pointer to next in list. */
- struct vstop_notif *next;
-
- /* Thread or process that got the event. */
- ptid_t ptid;
-
- /* Event info. */
- struct target_waitstatus status;
-};
-
-/* The pending stop replies list head. */
-static struct vstop_notif *notif_queue = NULL;
-
-/* Put a stop reply to the stop reply queue. */
-
static void
-queue_stop_reply (ptid_t ptid, struct target_waitstatus *status)
+notif_reply_stop (struct notif_reply *reply, char *own_buf)
{
- struct vstop_notif *new_notif;
-
- new_notif = xmalloc (sizeof (*new_notif));
- new_notif->next = NULL;
- new_notif->ptid = ptid;
- new_notif->status = *status;
+ struct vstop_notif *vstop = (struct vstop_notif *) reply;
- if (notif_queue)
- {
- struct vstop_notif *tail;
- for (tail = notif_queue;
- tail && tail->next;
- tail = tail->next)
- ;
- tail->next = new_notif;
- }
- else
- notif_queue = new_notif;
-
- if (remote_debug)
- {
- int i = 0;
- struct vstop_notif *n;
-
- for (n = notif_queue; n; n = n->next)
- i++;
-
- fprintf (stderr, "pending stop replies: %d\n", i);
- }
+ prepare_resume_reply (own_buf, reply->ptid, &vstop->status);
}
-/* Place an event in the stop reply queue, and push a notification if
- we aren't sending one yet. */
-
-void
-push_event (ptid_t ptid, struct target_waitstatus *status)
+struct notif notif_stop =
{
- gdb_assert (status->kind != TARGET_WAITKIND_IGNORE);
-
- queue_stop_reply (ptid, status);
-
- /* If this is the first stop reply in the queue, then inform GDB
- about it, by sending a Stop notification. */
- if (notif_queue->next == NULL)
- {
- char *p = own_buf;
- strcpy (p, "Stop:");
- p += strlen (p);
- prepare_resume_reply (p,
- notif_queue->ptid, ¬if_queue->status);
- putpkt_notif (own_buf);
- }
-}
-
-/* Get rid of the currently pending stop replies for PID. If PID is
- -1, then apply to all processes. */
+ "vStopped", "Stop", NOTIF_STOP, NULL, notif_reply_stop,
+};
static void
-discard_queued_stop_replies (int pid)
+notif_reply_enque (struct notif_reply *reply, struct notif *notif)
{
- struct vstop_notif *prev = NULL, *reply, *next;
+ gdb_queue_notif_reply_enque (reply, notif->queue);
- for (reply = notif_queue; reply; reply = next)
- {
- next = reply->next;
-
- if (pid == -1
- || ptid_get_pid (reply->ptid) == pid)
- {
- if (reply == notif_queue)
- notif_queue = next;
- else
- prev->next = reply->next;
-
- free (reply);
- }
- else
- prev = reply;
- }
-}
-
-/* If there are more stop replies to push, push one now. */
+ if (remote_debug)
+ fprintf (stderr, "pending replies: %s %d\n", notif->notif_name,
+ gdb_queue_notif_reply_length (notif->queue));
-static void
-send_next_stop_reply (char *own_buf)
-{
- if (notif_queue)
- prepare_resume_reply (own_buf,
- notif_queue->ptid,
- ¬if_queue->status);
- else
- write_ok (own_buf);
}
static int
@@ -2159,7 +2067,8 @@ handle_v_kill (char *own_buf)
last_status.kind = TARGET_WAITKIND_SIGNALLED;
last_status.value.sig = GDB_SIGNAL_KILL;
last_ptid = pid_to_ptid (pid);
- discard_queued_stop_replies (pid);
+
+ notif_queued_replies_discard_all (pid);
write_ok (own_buf);
return 1;
}
@@ -2170,29 +2079,6 @@ handle_v_kill (char *own_buf)
}
}
-/* Handle a 'vStopped' packet. */
-static void
-handle_v_stopped (char *own_buf)
-{
- /* If we're waiting for GDB to acknowledge a pending stop reply,
- consider that done. */
- if (notif_queue)
- {
- struct vstop_notif *head;
-
- if (remote_debug)
- fprintf (stderr, "vStopped: acking %s\n",
- target_pid_to_str (notif_queue->ptid));
-
- head = notif_queue;
- notif_queue = notif_queue->next;
- free (head);
- }
-
- /* Push another stop reply, or if there are no more left, an OK. */
- send_next_stop_reply (own_buf);
-}
-
/* Handle all of the extended 'v' packets. */
void
handle_v_requests (char *own_buf, int packet_len, int *new_packet_len)
@@ -2253,11 +2139,8 @@ handle_v_requests (char *own_buf, int packet_len, int *new_packet_len)
return;
}
- if (strncmp (own_buf, "vStopped", 8) == 0)
- {
- handle_v_stopped (own_buf);
- return;
- }
+ if (notif_ack_handle (own_buf))
+ return;
/* Otherwise we didn't know what packet it was. Say we didn't
understand it. */
@@ -2335,18 +2218,25 @@ queue_stop_reply_callback (struct inferior_list_entry *entry, void *arg)
{
struct thread_info *thread = (struct thread_info *) entry;
+
/* For now, assume targets that don't have this callback also don't
manage the thread's last_status field. */
if (the_target->thread_stopped == NULL)
{
+ struct vstop_notif *new_notif = xmalloc (sizeof (*new_notif));
+
+ new_notif->base.ptid = entry->id;
+ new_notif->status = thread->last_status;
/* Pass the last stop reply back to GDB, but don't notify
yet. */
- queue_stop_reply (entry->id, &thread->last_status);
+ notif_reply_enque ((struct notif_reply *) new_notif, ¬if_stop);
}
else
{
if (thread_stopped (thread))
{
+ struct vstop_notif *new_notif = xmalloc (sizeof (*new_notif));
+
if (debug_threads)
fprintf (stderr,
"Reporting thread %s as already stopped with %s\n",
@@ -2355,9 +2245,11 @@ queue_stop_reply_callback (struct inferior_list_entry *entry, void *arg)
gdb_assert (thread->last_status.kind != TARGET_WAITKIND_IGNORE);
+ new_notif->base.ptid = entry->id;
+ new_notif->status = thread->last_status;
/* Pass the last stop reply back to GDB, but don't notify
yet. */
- queue_stop_reply (entry->id, &thread->last_status);
+ notif_reply_enque ((struct notif_reply *) new_notif, ¬if_stop);
}
}
@@ -2416,13 +2308,13 @@ handle_status (char *own_buf)
if (non_stop)
{
- discard_queued_stop_replies (-1);
+ notif_queued_replies_discard (-1, ¬if_stop);
find_inferior (&all_threads, queue_stop_reply_callback, NULL);
/* The first is sent immediatly. OK is sent if there is no
stopped thread, which is the same handling of the vStopped
packet (by design). */
- send_next_stop_reply (own_buf);
+ notif_send_reply (¬if_stop, own_buf);
}
else
{
@@ -2515,7 +2407,7 @@ kill_inferior_callback (struct inferior_list_entry *entry)
int pid = ptid_get_pid (process->head.id);
kill_inferior (pid);
- discard_queued_stop_replies (pid);
+ notif_queued_replies_discard_all (pid);
}
/* Callback for for_each_inferior to detach or kill the inferior,
@@ -2534,7 +2426,7 @@ detach_or_kill_inferior_callback (struct inferior_list_entry *entry)
else
kill_inferior (pid);
- discard_queued_stop_replies (pid);
+ notif_queued_replies_discard_all (pid);
}
/* for_each_inferior callback for detach_or_kill_for_exit to print
@@ -2791,6 +2683,8 @@ main (int argc, char *argv[])
last_ptid = minus_one_ptid;
}
+ initialize_notif ();
+
/* Don't report shared library events on the initial connection,
even if some libraries are preloaded. Avoids the "stopped by
shared library event" notice on gdb side. */
@@ -3061,7 +2955,7 @@ process_serial_event (void)
write_enn (own_buf);
else
{
- discard_queued_stop_replies (pid);
+ notif_queued_replies_discard_all (pid);
write_ok (own_buf);
if (extended_protocol)
@@ -3403,7 +3297,7 @@ process_serial_event (void)
{
/* In non-stop, defer exiting until GDB had a chance to query
the whole vStopped list (until it gets an OK). */
- if (!notif_queue)
+ if (notif_queued_replies_empty_p ())
{
fprintf (stderr, "GDBserver exiting\n");
remote_close ();
@@ -3501,8 +3395,46 @@ handle_target_event (int err, gdb_client_data client_data)
}
else
{
- /* Something interesting. Tell GDB about it. */
- push_event (last_ptid, &last_status);
+ while (!gdb_queue_notif_is_empty (¬if_queue))
+ {
+ struct notif *np = gdb_queue_notif_deque (¬if_queue);
+ struct notif_reply *new_notif = NULL;
+ int is_first_event = 0;
+
+ switch (np->type)
+ {
+ case NOTIF_STOP:
+ {
+ struct vstop_notif *vstop_notif
+ = xmalloc (sizeof (struct vstop_notif));
+
+ vstop_notif->status = last_status;
+ new_notif = (struct notif_reply *) vstop_notif;
+ }
+ break;
+ default:
+ error ("Unknown notification type");
+ }
+
+ new_notif->ptid = last_ptid;
+ is_first_event = gdb_queue_notif_reply_is_empty (np->queue);
+
+ /* Something interesting. Tell GDB about it. */
+ notif_reply_enque (new_notif, np);
+
+ /* If this is the first stop reply in the queue, then inform GDB
+ about it, by sending a corresponding notification. */
+ if (is_first_event)
+ {
+ char *p = own_buf;
+
+ xsnprintf (p, PBUFSIZ, "%s:", np->notif_name);
+ p += strlen (p);
+
+ np->reply (new_notif, p);
+ putpkt_notif (own_buf);
+ }
+ }
}
}
diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
index e93ad00..3bf72dc 100644
--- a/gdb/gdbserver/server.h
+++ b/gdb/gdbserver/server.h
@@ -265,8 +265,6 @@ extern void start_event_loop (void);
extern int handle_serial_event (int err, gdb_client_data client_data);
extern int handle_target_event (int err, gdb_client_data client_data);
-extern void push_event (ptid_t ptid, struct target_waitstatus *status);
-
/* Functions from hostio.c. */
extern int handle_vFile (char *, int, int *);
--
1.7.7.6
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 4/4] new notification "TEST".
2012-08-24 2:26 [RCF 0/4] A general notification in GDB RSP Yao Qi
@ 2012-08-24 2:26 ` Yao Qi
2012-08-24 2:26 ` [PATCH 1/4] new gdb_queue.h in common/ Yao Qi
` (3 subsequent siblings)
4 siblings, 0 replies; 19+ messages in thread
From: Yao Qi @ 2012-08-24 2:26 UTC (permalink / raw)
To: gdb-patches
This patch is only to present how do we add a new type of notifcation
in the future, and to test two types of notifications coexist together
for testing purpose. In each time GDB fetches registers, a new event
is pushed, and GDBserver will send a notification %Test to GDB.
This patch is not for committing to CVS trunk.
gdb/gdbserver:
2012-08-24 Yao Qi <yao@codesourcery.com>
* linux-low.c (linux_fetch_registers): Call circular_queue_enque
and async_file_mark.
* notif.c (notif_reply_test): New.
(notif_test): New variable.
(notfi_packets): Add new element 'notif_test'.
* notif.h (notif_type): New enum 'NOTIF_TEST'.
* server.c (handle_target_event): Handle 'NOTIF_TEST'.
gdb:
2012-08-24 Yao Qi <yao@codesourcery.com>
* remote-notif.c (struct test_reply): New.
(remote_notif_parse_test): New.
(remote_notif_ack_test): New.
(remote_notif_alloc_reply_test): New.
(notif_packet_test): New variable.
(notifs): New element 'notif_packet_test'.
---
gdb/gdbserver/linux-low.c | 10 ++++++++++
gdb/gdbserver/notif.c | 12 ++++++++++++
gdb/gdbserver/notif.h | 2 +-
gdb/gdbserver/server.c | 3 +++
gdb/remote-notif.c | 41 +++++++++++++++++++++++++++++++++++++++++
5 files changed, 67 insertions(+), 1 deletions(-)
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index e9752b0..2045717 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -4341,6 +4341,16 @@ linux_fetch_registers (struct regcache *regcache, int regno)
int use_regsets;
int all = 0;
+ if (target_is_async_p ())
+ {
+ /* Only for test. */
+
+ extern struct notif notif_test;
+
+ gdb_queue_notif_enque (¬if_test, ¬if_queue);
+ async_file_mark ();
+ }
+
if (regno == -1)
{
if (the_low_target.fetch_register != NULL)
diff --git a/gdb/gdbserver/notif.c b/gdb/gdbserver/notif.c
index 838f391..b94a196 100644
--- a/gdb/gdbserver/notif.c
+++ b/gdb/gdbserver/notif.c
@@ -19,11 +19,23 @@
#include "notif.h"
+static void
+notif_reply_test (struct notif_reply *reply, char *own_buf)
+{
+ strcpy (own_buf, "CESHI");
+}
+
+struct notif notif_test =
+{
+ "vTested", "Test", NOTIF_TEST, NULL, notif_reply_test
+};
+
extern struct notif notif_stop;
static struct notif *notif_packets [] =
{
¬if_stop,
+ ¬if_test,
NULL,
};
diff --git a/gdb/gdbserver/notif.h b/gdb/gdbserver/notif.h
index ac62eab..f929616 100644
--- a/gdb/gdbserver/notif.h
+++ b/gdb/gdbserver/notif.h
@@ -42,7 +42,7 @@ struct vstop_notif
struct target_waitstatus status;
};
-enum notif_type { NOTIF_STOP };
+enum notif_type { NOTIF_STOP, NOTIF_TEST };
/* A notification to GDB. */
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 0b756cd..01db55f 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -3412,6 +3412,9 @@ handle_target_event (int err, gdb_client_data client_data)
new_notif = (struct notif_reply *) vstop_notif;
}
break;
+ case NOTIF_TEST:
+ new_notif = xmalloc (sizeof (struct notif_reply));
+ break;
default:
error ("Unknown notification type");
}
diff --git a/gdb/remote-notif.c b/gdb/remote-notif.c
index ca7f821..3c1634a 100644
--- a/gdb/remote-notif.c
+++ b/gdb/remote-notif.c
@@ -27,11 +27,52 @@
extern struct remote_state *get_remote_state (void);
+struct test_reply
+{
+ struct notif_reply base;
+};
+
+static void
+remote_notif_parse_test (struct notif *self, char *buf, void *data)
+{
+ if (strncmp (buf, "CESHI", 5) != 0)
+ error (_("'CESHI' is expected"));
+}
+
+static void
+remote_notif_ack_test (struct notif *self, char *buf, void *data)
+{
+ struct notif_reply *reply = (struct notif_reply *) data;
+
+ /* acknowledge */
+ putpkt ((char *) self->ack_command);
+}
+
+static struct notif_reply *
+remote_notif_alloc_reply_test (void)
+{
+ struct notif_reply *reply = xmalloc (sizeof (struct test_reply));
+
+ reply->dtr = NULL;
+
+ return reply;
+}
+
+static struct notif notif_packet_test =
+{
+ "Test", "vTested",
+ remote_notif_parse_test,
+ remote_notif_ack_test,
+ remote_notif_alloc_reply_test,
+ NULL, NULL,
+};
+
/* Supported notifications. */
static struct notif *notifs[] =
{
¬if_packet_stop,
+ ¬if_packet_test,
};
/* Parse the BUF for the expected notification NP, and send packet to
--
1.7.7.6
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/4] de-couple %Stop from notification: gdb
2012-08-24 2:26 [RCF 0/4] A general notification in GDB RSP Yao Qi
` (2 preceding siblings ...)
2012-08-24 2:26 ` [PATCH 3/4] de-couple %Stop from notification: gdbserver Yao Qi
@ 2012-08-24 2:26 ` Yao Qi
2012-08-24 16:52 ` Yao Qi
2012-08-24 19:00 ` dje
2012-08-24 17:53 ` [RCF 0/4] A general notification in GDB RSP Pedro Alves
4 siblings, 2 replies; 19+ messages in thread
From: Yao Qi @ 2012-08-24 2:26 UTC (permalink / raw)
To: gdb-patches
This patch is to de-couple vStopped/%Stop from notification in gdb side.
gdb:
2012-08-24 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'.
* remote-notif.c: New. Moved from remote.c.
* 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, and
remote_async_get_pending_events_handler.
Declare remote_parse_stop_reply.
Remove variable pending_stop_reply.
(struct remote_state): Move to remote.h.
(get_remote_state): 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): Call 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.
Remove stop_reply_queue.
(stop_reply_xmalloc, stop_reply_xfree, do_stop_reply_xfree): Remove
(remote_notif_ack_stop, stop_reply_dtr): New.
(remote_notif_alloc_reply_stop): New.
(notif_packet_stop): New variable.
(stop_reply_match_pid, stop_reply_match_ptid): New.
(discard_pending_stop_replies): Adjust to call
remote_notif_discard_pending_replies only.
(queued_stop_reply, peek_stop_reply): Adjust.
(push_stop_reply): Add one more parameter 'queue', and rename to
notif_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 gdb_queue_notif_reply_is_empty.
* remote.h (struct remote_state): Moved from remote.c.
Declare notif_reply_push.
Include "target.h" and "remote-notif.h".
---
gdb/Makefile.in | 9 +-
gdb/remote-notif.c | 285 ++++++++++++++++++++++++++++++
gdb/remote-notif.h | 83 +++++++++
gdb/remote.c | 484 ++++++++++++----------------------------------------
gdb/remote.h | 103 +++++++++++
5 files changed, 585 insertions(+), 379 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 5d5574e..26e12ae 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 \
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/gdb_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..ca7f821
--- /dev/null
+++ b/gdb/remote-notif.c
@@ -0,0 +1,285 @@
+/* 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/>. */
+
+#include "defs.h"
+#include "remote.h"
+#include "remote-notif.h"
+#include "event-loop.h"
+#include "target.h"
+
+#include <string.h>
+
+extern struct remote_state *get_remote_state (void);
+
+/* Supported notifications. */
+
+static struct notif *notifs[] =
+{
+ ¬if_packet_stop,
+};
+
+/* Parse the BUF for the expected notification NP, and send packet to
+ acknowledge. */
+
+struct notif_reply *
+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);
+
+ np->parse (np, buf, reply);
+ np->ack (np, buf, reply);
+
+ discard_cleanups (old_chain);
+
+ return reply;
+}
+
+/* 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);
+
+ np->parse (np, buf, reply);
+
+ discard_cleanups (old_chain);
+
+ return reply;
+}
+
+/* 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
+ 1.2) <-- OK
+ 1.3) --> g
+ 1.4) <-- %Stop
+ 1.5) --> vStopped
+ 1.6) <-- (registers reply to step #1.3)
+
+ Obviously, the reply in step #1.6 would be unexpected to a vStopped
+ query.
+
+ To solve this, whenever we parse a %Stop notification successfully,
+ we mark the REMOTE_ASYNC_GET_PENDING_EVENTS_TOKEN, and carry on
+ doing whatever we were doing:
+
+ 2.1) --> Hg 1
+ 2.2) <-- OK
+ 2.3) --> g
+ 2.4) <-- %Stop
+ <GDB marks the REMOTE_ASYNC_GET_PENDING_EVENTS_TOKEN>
+ 2.5) <-- (registers reply to step #2.3)
+
+ Eventualy after step #2.5, we return to the event loop, which
+ notices there's an event on the
+ REMOTE_ASYNC_GET_PENDING_EVENTS_TOKEN event and calls the
+ associated callback --- the function below. At this point, we're
+ always safe to start a vStopped sequence. :
+
+ 2.6) --> vStopped
+ 2.7) <-- T05 thread:2
+ 2.8) --> vStopped
+ 2.9) --> OK
+*/
+
+void
+remote_notif_pending_replies (struct notif *np)
+{
+ struct remote_state *rs = get_remote_state ();
+
+ if (np->pending_reply)
+ {
+ /* acknowledge */
+ putpkt ((char *) np->ack_command);
+
+ /* Now we can rely on it. */
+ notif_reply_push (np->pending_reply, np->ack_queue);
+ np->pending_reply = NULL;
+
+ while (1)
+ {
+ getpkt (&rs->buf, &rs->buf_size, 0);
+ if (strcmp (rs->buf, "OK") == 0)
+ break;
+ else
+ {
+ struct notif_reply *reply = remote_notif_ack (np, rs->buf);
+
+ notif_reply_push (reply, np->ack_queue);
+ }
+ }
+ }
+}
+
+static void
+remote_async_get_pending_events_handler (gdb_client_data data)
+{
+ remote_notif_pending_replies (*((struct notif **) data));
+}
+
+/* The current notif_packet we are processing. */
+
+static struct notif *current_notif;
+
+/* 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. */
+
+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,
+ ¤t_notif);
+}
+
+/* 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);
+}
+
+extern int remote_debug;
+
+/* Remote notification handler. */
+
+void
+handle_notification (char *buf)
+{
+ struct notif *np = NULL;
+ int i;
+
+ for (i = 0; i < sizeof (notifs) / sizeof (notifs[0]); i++)
+ {
+ 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. */
+ if (remote_debug)
+ fprintf_unfiltered (gdb_stdlog, "ignoring resent notification\n");
+ }
+ 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. */
+ current_notif = np;
+ mark_async_event_handler (remote_async_get_pending_events_token);
+
+ if (remote_debug)
+ fprintf_unfiltered (gdb_stdlog, "Notification '%s' captured\n",
+ np->name);
+ }
+}
+
+/* Free REPLY. */
+
+void
+notif_reply_xfree (struct notif_reply *reply)
+{
+ if (reply && reply->dtr)
+ reply->dtr (reply);
+
+ xfree (reply);
+}
+
+/* Cleanup wrapper. */
+
+void
+do_notif_reply_xfree (void *arg)
+{
+ struct notif_reply *reply = arg;
+
+ notif_reply_xfree (reply);
+}
+
+void
+gdb_queue_ele_notif_reply_xfree (struct notif_reply *reply)
+{
+ /* A placehoder, nothing to release for 'struct notif_reply', and its sub
+ classes. */
+}
+
+QUEUE_DEFINE_TYPE(notif_reply);
+
+/* Discard the pending replies in NP if function MATCH returns true. */
+
+void
+remote_notif_discard_pending_replies (struct notif *np,
+ int (*match) (struct notif_reply *,
+ void *),
+ void *data)
+{
+ struct notif_reply *reply = NULL;
+
+ /* Discard the in-flight notification. */
+ if (np->pending_reply != NULL && match (np->pending_reply, data))
+ {
+ notif_reply_xfree (np->pending_reply);
+ np->pending_reply = NULL;
+ }
+
+ /* Discard the replies we have already pulled with ack. */
+ reply = gdb_queue_notif_reply_remove (np->ack_queue, match, data);
+ notif_reply_xfree (reply);
+}
+
+void
+initialize_notif (void)
+{
+ int i;
+
+ for (i = 0; i < sizeof (notifs) / sizeof (notifs[0]); i++)
+ notifs[i]->ack_queue = gdb_queue_notif_reply_alloc ();
+}
diff --git a/gdb/remote-notif.h b/gdb/remote-notif.h
new file mode 100644
index 0000000..b9a753b
--- /dev/null
+++ b/gdb/remote-notif.h
@@ -0,0 +1,83 @@
+/* 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 "gdb_queue.h"
+
+/* A reply for a remote notification. */
+
+struct notif_reply
+{
+ /* Destructor. Release everything from SELF, but not SELF itself. */
+ void (*dtr) (struct notif_reply *self);
+};
+
+QUEUE_DECLARE (notif_reply);
+
+/* Data and operations related to notification packet. */
+
+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);
+
+ /* One pending reply. This is where we keep it until it is acknowledge. */
+ struct notif_reply *pending_reply;
+
+ /* The list of already fetched and acknowledged events. */
+ struct gdb_queue_notif_reply *ack_queue;
+};
+
+struct notif_reply *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 do_notif_reply_xfree (void *arg);
+
+void remote_notif_pending_replies (struct notif *np);
+void remote_notif_discard_pending_replies (struct notif *np,
+ int (*match) (struct notif_reply *,
+ void *),
+ void *data);
+
+extern struct notif notif_packet_stop;
+
+void initialize_notif (void);
+
+#endif /* REMOTE_NOTIF_H */
diff --git a/gdb/remote.c b/gdb/remote.c
index 2db2c9d..278d4bd 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,12 @@ 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 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 +241,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;
@@ -259,103 +250,6 @@ static struct cmd_list_element *remote_cmdlist;
static struct cmd_list_element *remote_set_cmdlist;
static struct cmd_list_element *remote_show_cmdlist;
-/* Description of the remote protocol state for the currently
- connected target. This is per-target state, and independent of the
- selected architecture. */
-
-struct remote_state
-{
- /* A buffer to use for incoming packets, and its current size. The
- buffer is grown dynamically for larger incoming packets.
- Outgoing packets may also be constructed in this buffer.
- BUF_SIZE is always at least REMOTE_PACKET_SIZE;
- REMOTE_PACKET_SIZE should be used to limit the length of outgoing
- packets. */
- char *buf;
- long buf_size;
-
- /* True if we're going through initial connection setup (finding out
- about the remote side's threads, relocating symbols, etc.). */
- int starting_up;
-
- /* If we negotiated packet size explicitly (and thus can bypass
- heuristics for the largest packet size that will not overflow
- a buffer in the stub), this will be set to that packet size.
- Otherwise zero, meaning to use the guessed size. */
- long explicit_packet_size;
-
- /* remote_wait is normally called when the target is running and
- waits for a stop reply packet. But sometimes we need to call it
- when the target is already stopped. We can send a "?" packet
- and have remote_wait read the response. Or, if we already have
- the response, we can stash it in BUF and tell remote_wait to
- skip calling getpkt. This flag is set when BUF contains a
- stop reply packet and the target is not waiting. */
- int cached_wait_status;
-
- /* True, if in no ack mode. That is, neither GDB nor the stub will
- expect acks from each other. The connection is assumed to be
- reliable. */
- int noack_mode;
-
- /* True if we're connected in extended remote mode. */
- int extended;
-
- /* True if the stub reported support for multi-process
- extensions. */
- int multi_process_aware;
-
- /* True if we resumed the target and we're waiting for the target to
- stop. In the mean time, we can't start another command/query.
- The remote server wouldn't be ready to process it, so we'd
- timeout waiting for a reply that would never come and eventually
- we'd close the connection. This can happen in asynchronous mode
- because we allow GDB commands while the target is running. */
- int waiting_for_stop_reply;
-
- /* True if the stub reports support for non-stop mode. */
- int non_stop_aware;
-
- /* True if the stub reports support for vCont;t. */
- int support_vCont_t;
-
- /* True if the stub reports support for conditional tracepoints. */
- int cond_tracepoints;
-
- /* True if the stub reports support for target-side breakpoint
- conditions. */
- int cond_breakpoints;
-
- /* True if the stub reports support for target-side breakpoint
- commands. */
- int breakpoint_commands;
-
- /* True if the stub reports support for fast tracepoints. */
- int fast_tracepoints;
-
- /* True if the stub reports support for static tracepoints. */
- int static_tracepoints;
-
- /* True if the stub reports support for installing tracepoint while
- tracing. */
- int install_in_trace;
-
- /* True if the stub can continue running a trace while GDB is
- disconnected. */
- int disconnected_tracing;
-
- /* True if the stub reports support for enabling and disabling
- tracepoints while a trace experiment is running. */
- int enable_disable_tracepoints;
-
- /* True if the stub can collect strings using tracenz bytecode. */
- int string_tracing;
-
- /* Nonzero if the user has pressed Ctrl-C, but the target hasn't
- responded to that. */
- int ctrlc_pending_p;
-};
-
/* Private data that we'll store in (struct thread_info)->private. */
struct private_thread_info
{
@@ -528,9 +422,11 @@ get_remote_arch_state (void)
return gdbarch_data (target_gdbarch, remote_gdbarch_data_handle);
}
+extern struct remote_state *get_remote_state (void);
+
/* Fetch the global remote target state. */
-static struct remote_state *
+struct remote_state *
get_remote_state (void)
{
/* Make sure that the remote architecture state has been
@@ -1402,12 +1298,6 @@ static struct async_signal_handler *sigint_remote_token;
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. */
-
-static struct async_event_handler *remote_async_get_pending_events_token;
\f
static ptid_t magic_null_ptid;
@@ -3027,8 +2917,8 @@ remote_close (int quitting)
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 +3340,15 @@ 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);
+ struct stop_reply *stop_reply
+ = (struct stop_reply *) remote_notif_parse (¬if_packet_stop,
+ rs->buf);
- 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.pending_reply = (struct notif_reply *) stop_reply;
+ remote_notif_pending_replies (¬if_packet_stop);
/* Make sure that threads that were stopped remain
stopped. */
@@ -4222,9 +4108,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. */
@@ -4384,6 +4268,8 @@ remote_disconnect (struct target_ops *target, char *args, int from_tty)
puts_filtered ("Ending remote debugging.\n");
}
+struct stop_reply;
+
/* Attach to the process specified by ARGS. If FROM_TTY is non-zero,
be chatty about it. */
@@ -4480,14 +4366,12 @@ 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 stop_reply *stop_reply
+ = (struct stop_reply *) remote_notif_parse (¬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_reply_push ((struct notif_reply *) stop_reply,
+ notif_packet_stop.ack_queue);
target_async (inferior_event_handler, 0);
}
@@ -5116,7 +5000,7 @@ DEF_VEC_O(cached_reg_t);
struct stop_reply
{
- struct stop_reply *next;
+ struct notif_reply base;
ptid_t ptid;
@@ -5137,26 +5021,62 @@ 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"));
}
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)
- {
- VEC_free (cached_reg_t, r->regcache);
- xfree (r);
- }
+ VEC_free (cached_reg_t, r->regcache);
+}
+
+static struct notif_reply *
+remote_notif_alloc_reply_stop (void)
+{
+ struct notif_reply *r = (struct notif_reply *) XMALLOC (struct stop_reply);
+
+ r->dtr = stop_reply_dtr;
+
+ return r;
+}
+
+struct notif notif_packet_stop =
+{
+ "Stop",
+ "vStopped",
+ remote_notif_parse_stop,
+ remote_notif_ack_stop,
+ remote_notif_alloc_reply_stop,
+ NULL,
+ NULL,
+};
+
+static int
+stop_reply_match_pid (struct notif_reply *reply, void *data)
+{
+ int *pid = (int *) data;
+ struct stop_reply *r = (struct stop_reply *) reply;
+
+ return (*pid == -1 || ptid_get_pid (r->ptid) == *pid);
}
/* Discard all pending stop replies of inferior PID. If PID is -1,
@@ -5165,45 +5085,18 @@ stop_reply_xfree (struct stop_reply *r)
static void
discard_pending_stop_replies (int pid)
{
- struct stop_reply *prev = NULL, *reply, *next;
-
- /* 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;
- }
-
- /* 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;
+ remote_notif_discard_pending_replies (¬if_packet_stop,
+ stop_reply_match_pid, &pid);
- stop_reply_xfree (reply);
- }
- else
- prev = reply;
- }
}
-/* Cleanup wrapper. */
-
-static void
-do_stop_reply_xfree (void *arg)
+static int
+stop_reply_match_ptid (struct notif_reply *reply, void *data)
{
- struct stop_reply *r = arg;
+ ptid_t *ptid = (ptid_t *) data;
+ struct stop_reply *r = (struct stop_reply *) reply;
- stop_reply_xfree (r);
+ return ptid_match (r->ptid, *ptid);
}
/* Look for a queued stop reply belonging to PTID. If one is found,
@@ -5214,53 +5107,37 @@ 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;
+ struct notif_reply *r;
- 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;
- }
+ r = gdb_queue_notif_reply_remove (notif_packet_stop.ack_queue,
+ stop_reply_match_ptid, &ptid);
- if (stop_reply_queue)
+ if (!gdb_queue_notif_reply_is_empty (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. */
-static void
-push_stop_reply (struct stop_reply *new_event)
+void
+notif_reply_push (struct notif_reply *new_event,
+ struct gdb_queue_notif_reply *queue)
{
- struct stop_reply *event;
-
- if (stop_reply_queue)
- {
- for (event = stop_reply_queue;
- event && event->next;
- event = event->next)
- ;
+ gdb_queue_notif_reply_enque (new_event, queue);
+ mark_async_event_handler (remote_async_inferior_event_token);
+}
- event->next = new_event;
- }
- else
- stop_reply_queue = new_event;
+static int
+stop_reply_match_ptid_and_ws (struct notif_reply *reply, void *data)
+{
+ struct stop_reply *r = (struct stop_reply *) reply;
+ ptid_t *ptid = data;
- mark_async_event_handler (remote_async_inferior_event_token);
+ return (ptid_equal (*ptid, r->ptid) && r->ws.kind == TARGET_WAITKIND_STOPPED);
}
/* Returns true if we have a stop reply for PTID. */
@@ -5268,16 +5145,12 @@ push_stop_reply (struct stop_reply *new_event)
static int
peek_stop_reply (ptid_t ptid)
{
- struct stop_reply *it;
+ struct notif_reply *reply;
- for (it = stop_reply_queue; it; it = it->next)
- if (ptid_equal (ptid, it->ptid))
- {
- if (it->ws.kind == TARGET_WAITKIND_STOPPED)
- return 1;
- }
+ reply = gdb_queue_notif_reply_find (notif_packet_stop.ack_queue,
+ stop_reply_match_ptid_and_ws, &ptid);
- return 0;
+ return (reply != NULL);
}
/* Parse the stop reply in BUF. Either the function succeeds, and the
@@ -5493,92 +5366,6 @@ Packet: '%s'\n"),
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
- happen:
-
- 1.1) --> Hg 1
- 1.2) <-- OK
- 1.3) --> g
- 1.4) <-- %Stop
- 1.5) --> vStopped
- 1.6) <-- (registers reply to step #1.3)
-
- Obviously, the reply in step #1.6 would be unexpected to a vStopped
- query.
-
- To solve this, whenever we parse a %Stop notification sucessfully,
- we mark the REMOTE_ASYNC_GET_PENDING_EVENTS_TOKEN, and carry on
- doing whatever we were doing:
-
- 2.1) --> Hg 1
- 2.2) <-- OK
- 2.3) --> g
- 2.4) <-- %Stop
- <GDB marks the REMOTE_ASYNC_GET_PENDING_EVENTS_TOKEN>
- 2.5) <-- (registers reply to step #2.3)
-
- Eventualy after step #2.5, we return to the event loop, which
- notices there's an event on the
- REMOTE_ASYNC_GET_PENDING_EVENTS_TOKEN event and calls the
- associated callback --- the function below. At this point, we're
- always safe to start a vStopped sequence. :
-
- 2.6) --> vStopped
- 2.7) <-- T05 thread:2
- 2.8) --> vStopped
- 2.9) --> OK
-*/
-
-static void
-remote_get_pending_stop_replies (void)
-{
- struct remote_state *rs = get_remote_state ();
-
- if (pending_stop_reply)
- {
- /* acknowledge */
- putpkt ("vStopped");
-
- /* Now we can rely on it. */
- push_stop_reply (pending_stop_reply);
- pending_stop_reply = NULL;
-
- while (1)
- {
- getpkt (&rs->buf, &rs->buf_size, 0);
- 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);
- }
- }
- }
-}
-
-
/* 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. */
@@ -5622,7 +5409,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 +5448,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.pending_reply != NULL)
+ remote_notif_pending_replies (¬if_packet_stop);
/* If indeed we noticed a stop reply, we're done. */
stop_reply = queued_stop_reply (ptid);
@@ -5758,13 +5545,10 @@ 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;
+ struct stop_reply *stop_reply
+ = (struct stop_reply *) remote_notif_parse (¬if_packet_stop,
+ rs->buf);
- 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);
break;
}
@@ -5845,7 +5629,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 (!gdb_queue_notif_reply_is_empty (notif_packet_stop.ack_queue))
mark_async_event_handler (remote_async_inferior_event_token);
}
@@ -6741,52 +6525,6 @@ remote_read_bytes (CORE_ADDR memaddr, gdb_byte *myaddr, int len)
return i;
}
\f
-
-/* 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. */
- ;
-}
-
-\f
/* 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
@@ -11183,12 +10921,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)
{
@@ -11689,5 +11421,7 @@ Show the remote pathname for \"run\""), NULL, NULL, NULL,
target_buf_size = 2048;
target_buf = xmalloc (target_buf_size);
+
+ initialize_notif ();
}
diff --git a/gdb/remote.h b/gdb/remote.h
index 3adc54e..579a92f 100644
--- a/gdb/remote.h
+++ b/gdb/remote.h
@@ -19,8 +19,108 @@
#ifndef REMOTE_H
#define REMOTE_H
+#include "target.h"
+#include "remote-notif.h"
+
struct target_desc;
+/* Description of the remote protocol state for the currently
+ connected target. This is per-target state, and independent of the
+ selected architecture. */
+
+struct remote_state
+{
+ /* A buffer to use for incoming packets, and its current size. The
+ buffer is grown dynamically for larger incoming packets.
+ Outgoing packets may also be constructed in this buffer.
+ BUF_SIZE is always at least REMOTE_PACKET_SIZE;
+ REMOTE_PACKET_SIZE should be used to limit the length of outgoing
+ packets. */
+ char *buf;
+ long buf_size;
+
+ /* True if we're going through initial connection setup (finding out
+ about the remote side's threads, relocating symbols, etc.). */
+ int starting_up;
+
+ /* If we negotiated packet size explicitly (and thus can bypass
+ heuristics for the largest packet size that will not overflow
+ a buffer in the stub), this will be set to that packet size.
+ Otherwise zero, meaning to use the guessed size. */
+ long explicit_packet_size;
+
+ /* remote_wait is normally called when the target is running and
+ waits for a stop reply packet. But sometimes we need to call it
+ when the target is already stopped. We can send a "?" packet
+ and have remote_wait read the response. Or, if we already have
+ the response, we can stash it in BUF and tell remote_wait to
+ skip calling getpkt. This flag is set when BUF contains a
+ stop reply packet and the target is not waiting. */
+ int cached_wait_status;
+
+ /* True, if in no ack mode. That is, neither GDB nor the stub will
+ expect acks from each other. The connection is assumed to be
+ reliable. */
+ int noack_mode;
+
+ /* True if we're connected in extended remote mode. */
+ int extended;
+
+ /* True if the stub reported support for multi-process
+ extensions. */
+ int multi_process_aware;
+
+ /* True if we resumed the target and we're waiting for the target to
+ stop. In the mean time, we can't start another command/query.
+ The remote server wouldn't be ready to process it, so we'd
+ timeout waiting for a reply that would never come and eventually
+ we'd close the connection. This can happen in asynchronous mode
+ because we allow GDB commands while the target is running. */
+ int waiting_for_stop_reply;
+
+ /* True if the stub reports support for non-stop mode. */
+ int non_stop_aware;
+
+ /* True if the stub reports support for vCont;t. */
+ int support_vCont_t;
+
+ /* True if the stub reports support for conditional tracepoints. */
+ int cond_tracepoints;
+
+ /* True if the stub reports support for target-side breakpoint
+ conditions. */
+ int cond_breakpoints;
+
+ /* True if the stub reports support for target-side breakpoint
+ commands. */
+ int breakpoint_commands;
+
+ /* True if the stub reports support for fast tracepoints. */
+ int fast_tracepoints;
+
+ /* True if the stub reports support for static tracepoints. */
+ int static_tracepoints;
+
+ /* True if the stub reports support for installing tracepoint while
+ tracing. */
+ int install_in_trace;
+
+ /* True if the stub can continue running a trace while GDB is
+ disconnected. */
+ int disconnected_tracing;
+
+ /* True if the stub reports support for enabling and disabling
+ tracepoints while a trace experiment is running. */
+ int enable_disable_tracepoints;
+
+ /* True if the stub can collect strings using tracenz bytecode. */
+ int string_tracing;
+
+ /* Nonzero if the user has pressed Ctrl-C, but the target hasn't
+ responded to that. */
+ int ctrlc_pending_p;
+};
+
/* Read a packet from the remote machine, with error checking, and
store it in *BUF. Resize *BUF using xrealloc if necessary to hold
the result, and update *SIZEOF_BUF. If FOREVER, wait forever
@@ -59,4 +159,7 @@ extern int remote_register_number_and_offset (struct gdbarch *gdbarch,
int regnum, int *pnum,
int *poffset);
+extern void notif_reply_push (struct notif_reply *,
+ struct gdb_queue_notif_reply *);
+
#endif
--
1.7.7.6
^ permalink raw reply [flat|nested] 19+ messages in thread
* [RCF 0/4] A general notification in GDB RSP
@ 2012-08-24 2:26 Yao Qi
2012-08-24 2:26 ` [PATCH 4/4] new notification "TEST" Yao Qi
` (4 more replies)
0 siblings, 5 replies; 19+ messages in thread
From: Yao Qi @ 2012-08-24 2:26 UTC (permalink / raw)
To: gdb-patches
Hi,
We have more and more requirements that remote target wants to notify
GDB via RSP at any time during connection when some states are changed
in remote target. However, GDB doesn't have such general notification
infrastructure. This is what this patch set tries to add.
Nowadays, notification mechanism exists in GDB for '%Stop' only,
and works pretty good. My rationale of this work is 'decouple
%Stop/vStopped from existing notification mechanism so that
notification mechanism can handle other types of notification.
First of all, let us look at how '%Stop' and 'vStopped' works,
gdb gdbserver
<- %Stop // [1] Send notification
[after process other packets]
-> vStopped // [2] Ack this notification
<- T05 thread:2 // [3] Reply
-> vStopped // [4] Continue to query
<- OK // [5] Done
We can generalize this protocol like this,
gdb gdbserver
<- %NOTIF // [1] Send notification
[after process other packets]
-> vACK // [2] Ack this notification
<- REPLY // [3] Reply
-> vACK // [4] Continue to query
<- OK // [5] Done
For each type of notification, we only have to define three things,
1) NOTIF, the key word of notification, for example, "Stop"
2) ACK, the command GDB ack to this notification, "vStopped" for
example,
3) REPLY, the format of reply to GDB. GDBserver should be able to
send packet to comply with REPLY, and GDB is able parse the packet,
and identify the packet is REPLY or not.
Patch 1/4 is to create a general queue data structure which will not
only be used in patch 2/4 and 3/4, but also can be used in elsewhere
in GDB.
The patch 2/4 and 3/4 are to decouple %Stop and vStopped out of
existing notification, in order to make notification more general.
Patch 4/4 is to demonstrate how do we add a new type of notification
in the future, and test two types of notification work good
together. As you can see, it is relatively simple. Originally, I
intended to convert qTstatus to a new type of notification in patch
4/4 as an example (when tracing status is changed, GDBserver sends
notification), but it takes time to think about it, so I post this
version here to get feedbacks first.
All three patches are tested on x86_64-linux {native, gdbserver} x
{sync, async}. No regression.
Note that the behaviour of notification in RSP is unchanged, that is
to say, patched GDB (w/ patch 2/4) works well with unpatched
GDBserver, and patched GDBserver (w/ patch 3/4) works well with
unpatched GDB.
it is different from Hui's patch 3/9 in 'autload breakpoint'
patch series,
[RFC] Autoload-breakpoints new version [3/9] notification async
http://sourceware.org/ml/gdb-patches/2012-08/msg00214.html
His patch is to teach GDB to handle notification at anytime, while
mine is not, but to allow to add other types of notification similar
to existing %Stop/vStopped on the same infrastructure.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/4] new gdb_queue.h in common/.
2012-08-24 2:26 [RCF 0/4] A general notification in GDB RSP Yao Qi
2012-08-24 2:26 ` [PATCH 4/4] new notification "TEST" Yao Qi
@ 2012-08-24 2:26 ` Yao Qi
2012-08-24 18:44 ` dje
2012-08-24 2:26 ` [PATCH 3/4] de-couple %Stop from notification: gdbserver Yao Qi
` (2 subsequent siblings)
4 siblings, 1 reply; 19+ messages in thread
From: Yao Qi @ 2012-08-24 2:26 UTC (permalink / raw)
To: gdb-patches
Hi,
When writing the patches for 'general notification', I find queue is
used in many places, so I think we may need a general queue. This
queue not only have typical operations enqueue and dequeue, but also
has some have some operations like remove and remove_all.
gdb/
2012-08-24 Yao Qi <yao@codesourcery.com>
* common/gdb_queue.h: New.
---
gdb/common/gdb_queue.h | 283 ++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 283 insertions(+), 0 deletions(-)
create mode 100644 gdb/common/gdb_queue.h
diff --git a/gdb/common/gdb_queue.h b/gdb/common/gdb_queue.h
new file mode 100644
index 0000000..fa582c1
--- /dev/null
+++ b/gdb/common/gdb_queue.h
@@ -0,0 +1,283 @@
+/* General queue data structure for GDB, the GNU debugger.
+
+ Copyright (C) 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/>. */
+
+/* These macros implement functions and structs for a general queue. Macro
+ 'QUEUE_DEFINE_TYPE(TYPE)' is to define the new queue type for
+ 'struct TYPE', and macro 'QUEUE_DECLARE' *is to declare these external
+ APIs. When define a queue type for 'struct FOO', an extra helper function
+ 'gdb_queue_ele_FOO_xfree (struct FOO *foo)' has to be defined to release
+ the contents in foo, but not foo itself. */
+
+#ifndef GDB_QUEUE_H
+#define GDB_QUEUE_H
+
+#include <stdio.h>
+
+#include "libiberty.h" /* xmalloc */
+#include "gdb_assert.h"
+
+/* Define a new queue implementation. */
+
+#define QUEUE_DEFINE_TYPE(TYPE) \
+struct gdb_queue_ele_ ## TYPE \
+{ \
+ struct gdb_queue_ele_ ## TYPE *next; \
+ \
+ struct TYPE *data; \
+}; \
+ \
+struct gdb_queue_ ## TYPE \
+{ \
+ struct gdb_queue_ele_ ## TYPE *head; \
+ struct gdb_queue_ele_ ## TYPE *tail; \
+}; \
+ \
+/* Typical enqueue operation. Put V into queue Q. */ \
+ \
+void \
+gdb_queue_ ## TYPE ## _enque (struct TYPE *v, \
+ struct gdb_queue_ ## TYPE *q) \
+{ \
+ struct gdb_queue_ele_ ## TYPE *p \
+ = xmalloc (sizeof (struct gdb_queue_ele_ ## TYPE)); \
+ \
+ gdb_assert (q != NULL); \
+ p->data = v; \
+ p->next = NULL; \
+ if (q->tail == NULL) \
+ { \
+ q->tail = p; \
+ q->head = p; \
+ } \
+ else \
+ { \
+ q->tail->next = p; \
+ q->tail = p; \
+ } \
+} \
+ \
+/* Typical dequeue operation. Return one element queue Q. Return NULL \
+ if queue Q is empty. */ \
+ \
+struct TYPE * \
+gdb_queue_ ## TYPE ## _deque (struct gdb_queue_ ## TYPE *q) \
+{ \
+ struct gdb_queue_ele_ ## TYPE *p = NULL; \
+ struct TYPE *v = NULL; \
+ \
+ if (q == NULL) \
+ return NULL; \
+ p = q->head; \
+ \
+ if (q->head == q->tail) \
+ { \
+ q->head = NULL; \
+ q->tail = NULL; \
+ } \
+ else \
+ q->head = q->head->next; \
+ \
+ if (p != NULL) \
+ v = p->data; \
+ \
+ xfree (p); \
+ return v; \
+} \
+ \
+/* Return the element on tail, but don't remove it from queue. Return \
+ NULL if queue is empty. */ \
+ \
+struct TYPE * \
+gdb_queue_ ## TYPE ## _peek (struct gdb_queue_ ## TYPE *q) \
+{ \
+ if (q== NULL || q->head == NULL) \
+ return NULL; \
+ return q->head->data; \
+} \
+ \
+/* Return true if queue Q is empty. */ \
+ \
+int \
+gdb_queue_ ## TYPE ## _is_empty (struct gdb_queue_ ## TYPE *q) \
+{ \
+ return (q == NULL || q->head == NULL); \
+} \
+ \
+/* Iterate over elements in the queue Q, and remove all of them for \
+ which function MATCH returns true. */ \
+ \
+void \
+gdb_queue_ ## TYPE ## _remove_all (struct gdb_queue_ ## TYPE *q, \
+ int (*match) (struct TYPE *, void *), \
+ void *data) \
+{ \
+ struct gdb_queue_ele_ ## TYPE *p = NULL; \
+ struct gdb_queue_ele_ ## TYPE *prev = NULL, *next = NULL; \
+ \
+ if (q == NULL) \
+ return; \
+ \
+ for (p = q->head; p != NULL; p = next) \
+ { \
+ next = p->next; \
+ if (match (p->data, data)) \
+ { \
+ if (p == q->head || p == q->tail) \
+ { \
+ if (p == q->head) \
+ q->head = p->next; \
+ if (p == q->tail) \
+ q->tail = prev; \
+ } \
+ else \
+ prev->next = p->next; \
+ \
+ gdb_queue_ele_ ## TYPE ## _xfree (p->data); \
+ xfree (p->data); \
+ xfree (p); \
+ } \
+ else \
+ prev = p; \
+ } \
+} \
+ \
+/* Iterate over queue Q and remove one element for which function \
+ MATCH returns true. */ \
+ \
+struct TYPE * \
+gdb_queue_ ## TYPE ## _remove (struct gdb_queue_ ## TYPE *q, \
+ int (*match) (struct TYPE *, void *), \
+ void *data) \
+{ \
+ struct gdb_queue_ele_ ## TYPE *p = NULL; \
+ struct gdb_queue_ele_ ## TYPE *prev = NULL; \
+ struct TYPE *t = NULL; \
+ \
+ if (q == NULL) \
+ return NULL; \
+ \
+ for (p = q->head; p != NULL; p = p->next) \
+ { \
+ if (match (p->data, data)) \
+ { \
+ if (p == q->head || p == q->tail) \
+ { \
+ if (p == q->head) \
+ q->head = p->next; \
+ if (p == q->tail) \
+ q->tail = prev; \
+ } \
+ else \
+ prev->next = p->next; \
+ \
+ t = p->data; \
+ xfree (p); \
+ return t; \
+ } \
+ else \
+ prev = p; \
+ } \
+ return NULL; \
+} \
+ \
+/* Find an element in queue Q for which function MATCH returns true. \
+ Return NULL if not found. */ \
+ \
+struct TYPE * \
+gdb_queue_ ## TYPE ## _find (struct gdb_queue_ ## TYPE *q, \
+ int (*match) (struct TYPE *, void *), \
+ void *data) \
+{ \
+ struct gdb_queue_ele_ ## TYPE *p = NULL; \
+ \
+ if (q == NULL) \
+ return NULL; \
+ \
+ for (p = q->head; p != NULL; p = p->next) \
+ { \
+ if (match (p->data, data)) \
+ return p->data; \
+ } \
+ return NULL; \
+} \
+ \
+/* Allocate memory for queue. */ \
+ \
+struct gdb_queue_ ## TYPE * \
+gdb_queue_ ## TYPE ## _alloc (void) \
+{ \
+ struct gdb_queue_ ## TYPE *p; \
+ \
+ p = (struct gdb_queue_ ## TYPE *) xmalloc (sizeof (struct gdb_queue_ ## TYPE));\
+ p->head = NULL; \
+ p->tail = NULL; \
+ return p; \
+} \
+ \
+/* Length of queue Q. */ \
+ \
+int \
+gdb_queue_ ## TYPE ## _length (struct gdb_queue_ ## TYPE *q) \
+{ \
+ struct gdb_queue_ele_ ## TYPE *p = NULL; \
+ int len = 0; \
+ \
+ if (q == NULL) \
+ return 0; \
+ \
+ for (p = q->head; p != NULL; p = p->next) \
+ len++; \
+ return len; \
+} \
+
+
+/* Define a variable of type gdb_queue_ ## TYPE. */
+
+#define QUEUE_DEFINE_VAR(TYPE, VAR) \
+ struct gdb_queue_ ## TYPE VAR = { NULL, NULL }
+
+/* External declarations for these functions. */
+#define QUEUE_DECLARE(TYPE) \
+struct gdb_queue_ ## TYPE; \
+extern void gdb_queue_ ## TYPE ## _enque (struct TYPE *v, \
+ struct gdb_queue_ ## TYPE *q); \
+extern struct TYPE * \
+ gdb_queue_ ## TYPE ## _deque (struct gdb_queue_ ## TYPE *q); \
+extern int \
+ gdb_queue_ ## TYPE ## _is_empty (struct gdb_queue_ ## TYPE *q); \
+extern void \
+ gdb_queue_ ## TYPE ## _remove_all (struct gdb_queue_ ## TYPE *q, \
+ int (*match) (struct TYPE *, void *), \
+ void *data); \
+extern struct TYPE * \
+ gdb_queue_ ## TYPE ## _remove (struct gdb_queue_ ## TYPE *q, \
+ int (*match) (struct TYPE *, void *), \
+ void *data); \
+extern struct TYPE * \
+ gdb_queue_ ## TYPE ## _find (struct gdb_queue_ ## TYPE *q, \
+ int (*match) (struct TYPE *, void *), \
+ void *data); \
+extern struct gdb_queue_ ## TYPE * \
+ gdb_queue_ ## TYPE ## _alloc (void); \
+extern void gdb_queue_ele_ ## TYPE ## _xfree (struct TYPE *); \
+extern int gdb_queue_ ## TYPE ## _length (struct gdb_queue_ ## TYPE *q);\
+extern struct TYPE * \
+ gdb_queue_ ## TYPE ## _peek (struct gdb_queue_ ## TYPE *q); \
+
+#endif /* GDB_QUEUE_H */
--
1.7.7.6
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/4] de-couple %Stop from notification: gdb
2012-08-24 2:26 ` [PATCH 2/4] de-couple %Stop from notification: gdb Yao Qi
@ 2012-08-24 16:52 ` Yao Qi
2012-08-24 19:00 ` dje
1 sibling, 0 replies; 19+ messages in thread
From: Yao Qi @ 2012-08-24 16:52 UTC (permalink / raw)
To: gdb-patches
On 08/24/2012 10:25 AM, Yao Qi wrote:
> This patch is to de-couple vStopped/%Stop from notification in gdb side.
When writing new notification on top it, I find the reply queues are not cleared properly during quit. This new version get this problem fixed.
--
Yao
2012-08-24 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'.
* remote-notif.c: New. Moved from remote.c.
* 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, and
remote_async_get_pending_events_handler.
Remove declaration of 'discard_pending_stop_replies'.
Declare remote_parse_stop_reply.
Remove variable pending_stop_reply.
(struct remote_state): Move to remote.h.
(get_remote_state): 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): Call 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.
Remove stop_reply_queue.
(stop_reply_xmalloc, stop_reply_xfree, do_stop_reply_xfree): Remove
(remote_notif_ack_stop, stop_reply_dtr): New.
(remote_notif_alloc_reply_stop): New.
(notif_packet_stop): New variable.
(stop_reply_match_ptid): New.
(queued_stop_reply, peek_stop_reply): Adjust.
(push_stop_reply): Add one more parameter 'queue', and rename to
notif_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 gdb_queue_notif_reply_is_empty.
* remote.h (struct remote_state): Moved from remote.c.
Declare notif_reply_push.
Include "target.h" and "remote-notif.h".
---
gdb/Makefile.in | 9 +-
gdb/remote-notif.c | 317 ++++++++++++++++++++++++++++++++++
gdb/remote-notif.h | 87 +++++++++
gdb/remote.c | 489 +++++++++++-----------------------------------------
gdb/remote.h | 103 +++++++++++
5 files changed, 612 insertions(+), 393 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 5d5574e..26e12ae 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 \
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/gdb_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..5b1d532
--- /dev/null
+++ b/gdb/remote-notif.c
@@ -0,0 +1,317 @@
+/* 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/>. */
+
+#include "defs.h"
+#include "remote.h"
+#include "remote-notif.h"
+#include "event-loop.h"
+#include "target.h"
+
+#include <string.h>
+
+extern struct remote_state *get_remote_state (void);
+
+/* Supported notifications. */
+
+static struct notif *notifs[] =
+{
+ ¬if_packet_stop,
+};
+
+/* Parse the BUF for the expected notification NP, and send packet to
+ acknowledge. */
+
+struct notif_reply *
+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);
+
+ np->parse (np, buf, reply);
+ np->ack (np, buf, reply);
+
+ discard_cleanups (old_chain);
+
+ return reply;
+}
+
+/* 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);
+
+ np->parse (np, buf, reply);
+
+ discard_cleanups (old_chain);
+
+ return reply;
+}
+
+/* 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
+ 1.2) <-- OK
+ 1.3) --> g
+ 1.4) <-- %Stop
+ 1.5) --> vStopped
+ 1.6) <-- (registers reply to step #1.3)
+
+ Obviously, the reply in step #1.6 would be unexpected to a vStopped
+ query.
+
+ To solve this, whenever we parse a %Stop notification successfully,
+ we mark the REMOTE_ASYNC_GET_PENDING_EVENTS_TOKEN, and carry on
+ doing whatever we were doing:
+
+ 2.1) --> Hg 1
+ 2.2) <-- OK
+ 2.3) --> g
+ 2.4) <-- %Stop
+ <GDB marks the REMOTE_ASYNC_GET_PENDING_EVENTS_TOKEN>
+ 2.5) <-- (registers reply to step #2.3)
+
+ Eventualy after step #2.5, we return to the event loop, which
+ notices there's an event on the
+ REMOTE_ASYNC_GET_PENDING_EVENTS_TOKEN event and calls the
+ associated callback --- the function below. At this point, we're
+ always safe to start a vStopped sequence. :
+
+ 2.6) --> vStopped
+ 2.7) <-- T05 thread:2
+ 2.8) --> vStopped
+ 2.9) --> OK
+*/
+
+void
+remote_notif_pending_replies (struct notif *np)
+{
+ struct remote_state *rs = get_remote_state ();
+
+ if (np->pending_reply)
+ {
+ /* acknowledge */
+ putpkt ((char *) np->ack_command);
+
+ /* Now we can rely on it. */
+ notif_reply_push (np->pending_reply, np->ack_queue);
+ np->pending_reply = NULL;
+
+ while (1)
+ {
+ getpkt (&rs->buf, &rs->buf_size, 0);
+ if (strcmp (rs->buf, "OK") == 0)
+ break;
+ else
+ {
+ struct notif_reply *reply = remote_notif_ack (np, rs->buf);
+
+ notif_reply_push (reply, np->ack_queue);
+ }
+ }
+ }
+}
+
+QUEUE_DEFINE_TYPE(notif);
+void
+gdb_queue_ele_notif_xfree (struct notif *notif)
+{
+ /* It is a placeholder. Never be called. */
+ gdb_assert (0);
+}
+
+QUEUE_DEFINE_VAR(notif, notif_queue);
+
+static void
+remote_async_get_pending_events_handler (gdb_client_data data)
+{
+ while (!gdb_queue_notif_is_empty (¬if_queue))
+ {
+ struct notif *np = gdb_queue_notif_deque (¬if_queue);
+
+ remote_notif_pending_replies (np);
+ }
+}
+
+/* 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. */
+
+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);
+}
+
+extern int remote_debug;
+
+/* Remote notification handler. */
+
+void
+handle_notification (char *buf)
+{
+ struct notif *np = NULL;
+ int i;
+
+ for (i = 0; i < sizeof (notifs) / sizeof (notifs[0]); i++)
+ {
+ 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. */
+ if (remote_debug)
+ fprintf_unfiltered (gdb_stdlog, "ignoring resent notification\n");
+ }
+ 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. */
+ gdb_queue_notif_enque (np, ¬if_queue);
+ mark_async_event_handler (remote_async_get_pending_events_token);
+
+ if (remote_debug)
+ fprintf_unfiltered (gdb_stdlog, "Notification '%s' captured\n",
+ np->name);
+ }
+}
+
+/* Free REPLY. */
+
+void
+notif_reply_xfree (struct notif_reply *reply)
+{
+ if (reply && reply->dtr)
+ reply->dtr (reply);
+
+ xfree (reply);
+}
+
+/* Cleanup wrapper. */
+
+void
+do_notif_reply_xfree (void *arg)
+{
+ struct notif_reply *reply = arg;
+
+ notif_reply_xfree (reply);
+}
+
+void
+gdb_queue_ele_notif_reply_xfree (struct notif_reply *reply)
+{
+ /* A placehoder, nothing to release for 'struct notif_reply', and its sub
+ classes. */
+}
+
+QUEUE_DEFINE_TYPE(notif_reply);
+
+/* Discard the pending replies in NP if function MATCH returns true. */
+
+void
+remote_notif_discard_pending_replies (struct notif *np,
+ int (*match) (struct notif_reply *,
+ void *),
+ void *data)
+{
+ struct notif_reply *reply = NULL;
+
+ /* Discard the in-flight notification. */
+ if (np->pending_reply != NULL && match (np->pending_reply, data))
+ {
+ notif_reply_xfree (np->pending_reply);
+ np->pending_reply = NULL;
+ }
+
+ /* Discard the replies we have already pulled with ack. */
+ reply = gdb_queue_notif_reply_remove (np->ack_queue, match, data);
+ notif_reply_xfree (reply);
+}
+
+static int
+notif_reply_match_pid (struct notif_reply *reply, void *data)
+{
+ int *pid = (int *) data;
+
+ return (*pid == -1 || ptid_get_pid (reply->ptid) == *pid);
+}
+
+/* Discard all pending replies of inferior PID. If PID is -1,
+ discard everything. */
+
+void
+discard_pending_replies (int pid)
+{
+ int i;
+
+ for (i = 0; i < sizeof (notifs) / sizeof (notifs[0]); i++)
+ remote_notif_discard_pending_replies (notifs[i], notif_reply_match_pid,
+ &pid);
+}
+
+void
+initialize_notif (void)
+{
+ int i;
+
+ for (i = 0; i < sizeof (notifs) / sizeof (notifs[0]); i++)
+ notifs[i]->ack_queue = gdb_queue_notif_reply_alloc ();
+}
diff --git a/gdb/remote-notif.h b/gdb/remote-notif.h
new file mode 100644
index 0000000..4fbe7cf
--- /dev/null
+++ b/gdb/remote-notif.h
@@ -0,0 +1,87 @@
+/* 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 "gdb_queue.h"
+
+/* A reply for a remote notification. */
+
+struct notif_reply
+{
+ ptid_t ptid;
+ /* Destructor. Release everything from SELF, but not SELF itself. */
+ void (*dtr) (struct notif_reply *self);
+};
+
+QUEUE_DECLARE (notif_reply);
+
+/* Data and operations related to notification packet. */
+
+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);
+
+ /* One pending reply. This is where we keep it until it is acknowledge. */
+ struct notif_reply *pending_reply;
+
+ /* The list of already fetched and acknowledged events. */
+ struct gdb_queue_notif_reply *ack_queue;
+};
+
+QUEUE_DECLARE (notif);
+
+struct notif_reply *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 do_notif_reply_xfree (void *arg);
+
+void remote_notif_pending_replies (struct notif *np);
+void remote_notif_discard_pending_replies (struct notif *np,
+ int (*match) (struct notif_reply *,
+ void *),
+ void *data);
+void discard_pending_replies (int pid);
+
+extern struct notif notif_packet_stop;
+
+void initialize_notif (void);
+
+#endif /* REMOTE_NOTIF_H */
diff --git a/gdb/remote.c b/gdb/remote.c
index 2db2c9d..31189f4 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,11 @@ 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 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 +240,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;
@@ -259,103 +249,6 @@ static struct cmd_list_element *remote_cmdlist;
static struct cmd_list_element *remote_set_cmdlist;
static struct cmd_list_element *remote_show_cmdlist;
-/* Description of the remote protocol state for the currently
- connected target. This is per-target state, and independent of the
- selected architecture. */
-
-struct remote_state
-{
- /* A buffer to use for incoming packets, and its current size. The
- buffer is grown dynamically for larger incoming packets.
- Outgoing packets may also be constructed in this buffer.
- BUF_SIZE is always at least REMOTE_PACKET_SIZE;
- REMOTE_PACKET_SIZE should be used to limit the length of outgoing
- packets. */
- char *buf;
- long buf_size;
-
- /* True if we're going through initial connection setup (finding out
- about the remote side's threads, relocating symbols, etc.). */
- int starting_up;
-
- /* If we negotiated packet size explicitly (and thus can bypass
- heuristics for the largest packet size that will not overflow
- a buffer in the stub), this will be set to that packet size.
- Otherwise zero, meaning to use the guessed size. */
- long explicit_packet_size;
-
- /* remote_wait is normally called when the target is running and
- waits for a stop reply packet. But sometimes we need to call it
- when the target is already stopped. We can send a "?" packet
- and have remote_wait read the response. Or, if we already have
- the response, we can stash it in BUF and tell remote_wait to
- skip calling getpkt. This flag is set when BUF contains a
- stop reply packet and the target is not waiting. */
- int cached_wait_status;
-
- /* True, if in no ack mode. That is, neither GDB nor the stub will
- expect acks from each other. The connection is assumed to be
- reliable. */
- int noack_mode;
-
- /* True if we're connected in extended remote mode. */
- int extended;
-
- /* True if the stub reported support for multi-process
- extensions. */
- int multi_process_aware;
-
- /* True if we resumed the target and we're waiting for the target to
- stop. In the mean time, we can't start another command/query.
- The remote server wouldn't be ready to process it, so we'd
- timeout waiting for a reply that would never come and eventually
- we'd close the connection. This can happen in asynchronous mode
- because we allow GDB commands while the target is running. */
- int waiting_for_stop_reply;
-
- /* True if the stub reports support for non-stop mode. */
- int non_stop_aware;
-
- /* True if the stub reports support for vCont;t. */
- int support_vCont_t;
-
- /* True if the stub reports support for conditional tracepoints. */
- int cond_tracepoints;
-
- /* True if the stub reports support for target-side breakpoint
- conditions. */
- int cond_breakpoints;
-
- /* True if the stub reports support for target-side breakpoint
- commands. */
- int breakpoint_commands;
-
- /* True if the stub reports support for fast tracepoints. */
- int fast_tracepoints;
-
- /* True if the stub reports support for static tracepoints. */
- int static_tracepoints;
-
- /* True if the stub reports support for installing tracepoint while
- tracing. */
- int install_in_trace;
-
- /* True if the stub can continue running a trace while GDB is
- disconnected. */
- int disconnected_tracing;
-
- /* True if the stub reports support for enabling and disabling
- tracepoints while a trace experiment is running. */
- int enable_disable_tracepoints;
-
- /* True if the stub can collect strings using tracenz bytecode. */
- int string_tracing;
-
- /* Nonzero if the user has pressed Ctrl-C, but the target hasn't
- responded to that. */
- int ctrlc_pending_p;
-};
-
/* Private data that we'll store in (struct thread_info)->private. */
struct private_thread_info
{
@@ -528,9 +421,11 @@ get_remote_arch_state (void)
return gdbarch_data (target_gdbarch, remote_gdbarch_data_handle);
}
+extern struct remote_state *get_remote_state (void);
+
/* Fetch the global remote target state. */
-static struct remote_state *
+struct remote_state *
get_remote_state (void)
{
/* Make sure that the remote architecture state has been
@@ -1402,12 +1297,6 @@ static struct async_signal_handler *sigint_remote_token;
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. */
-
-static struct async_event_handler *remote_async_get_pending_events_token;
\f
static ptid_t magic_null_ptid;
@@ -3023,12 +2912,12 @@ remote_close (int quitting)
discard_all_inferiors ();
/* We're no longer interested in any of these events. */
- discard_pending_stop_replies (-1);
+ discard_pending_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 +3339,15 @@ 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;
+ struct stop_reply *stop_reply
+ = (struct stop_reply *) remote_notif_parse (¬if_packet_stop,
+ rs->buf);
- 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.pending_reply = (struct notif_reply *) stop_reply;
+ remote_notif_pending_replies (¬if_packet_stop);
/* Make sure that threads that were stopped remain
stopped. */
@@ -4222,9 +4107,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 +4234,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);
+ discard_pending_replies (pid);
target_mourn_inferior ();
}
@@ -4384,6 +4267,8 @@ remote_disconnect (struct target_ops *target, char *args, int from_tty)
puts_filtered ("Ending remote debugging.\n");
}
+struct stop_reply;
+
/* Attach to the process specified by ARGS. If FROM_TTY is non-zero,
be chatty about it. */
@@ -4480,14 +4365,12 @@ 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 stop_reply *stop_reply
+ = (struct stop_reply *) remote_notif_parse (¬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_reply_push ((struct notif_reply *) stop_reply,
+ notif_packet_stop.ack_queue);
target_async (inferior_event_handler, 0);
}
@@ -5116,9 +4999,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 +5018,61 @@ 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"));
}
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)
- {
- 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 void
-discard_pending_stop_replies (int pid)
+static struct notif_reply *
+remote_notif_alloc_reply_stop (void)
{
- struct stop_reply *prev = NULL, *reply, *next;
-
- /* 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;
- }
+ struct notif_reply *r = (struct notif_reply *) XMALLOC (struct stop_reply);
- /* 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;
+ r->dtr = stop_reply_dtr;
- stop_reply_xfree (reply);
- }
- else
- prev = reply;
- }
+ return r;
}
-/* Cleanup wrapper. */
+struct notif notif_packet_stop =
+{
+ "Stop",
+ "vStopped",
+ remote_notif_parse_stop,
+ remote_notif_ack_stop,
+ remote_notif_alloc_reply_stop,
+ NULL,
+ NULL,
+};
-static void
-do_stop_reply_xfree (void *arg)
+static int
+stop_reply_match_ptid (struct notif_reply *reply, void *data)
{
- struct stop_reply *r = arg;
+ ptid_t *ptid = (ptid_t *) data;
- stop_reply_xfree (r);
+ return ptid_match (reply->ptid, *ptid);
}
/* Look for a queued stop reply belonging to PTID. If one is found,
@@ -5214,53 +5083,38 @@ 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;
+ struct notif_reply *r;
- 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;
- }
+ r = gdb_queue_notif_reply_remove (notif_packet_stop.ack_queue,
+ stop_reply_match_ptid, &ptid);
- it_link = &it->next;
- it = *it_link;
- }
-
- if (stop_reply_queue)
+ if (!gdb_queue_notif_reply_is_empty (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. */
-static void
-push_stop_reply (struct stop_reply *new_event)
+void
+notif_reply_push (struct notif_reply *new_event,
+ struct gdb_queue_notif_reply *queue)
{
- struct stop_reply *event;
-
- if (stop_reply_queue)
- {
- for (event = stop_reply_queue;
- event && event->next;
- event = event->next)
- ;
+ gdb_queue_notif_reply_enque (new_event, queue);
+ mark_async_event_handler (remote_async_inferior_event_token);
+}
- event->next = new_event;
- }
- else
- stop_reply_queue = new_event;
+static int
+stop_reply_match_ptid_and_ws (struct notif_reply *reply, void *data)
+{
+ struct stop_reply *r = (struct stop_reply *) reply;
+ ptid_t *ptid = data;
- mark_async_event_handler (remote_async_inferior_event_token);
+ return (ptid_equal (*ptid, r->base.ptid)
+ && r->ws.kind == TARGET_WAITKIND_STOPPED);
}
/* Returns true if we have a stop reply for PTID. */
@@ -5268,16 +5122,12 @@ push_stop_reply (struct stop_reply *new_event)
static int
peek_stop_reply (ptid_t ptid)
{
- struct stop_reply *it;
+ struct notif_reply *reply;
- for (it = stop_reply_queue; it; it = it->next)
- if (ptid_equal (ptid, it->ptid))
- {
- if (it->ws.kind == TARGET_WAITKIND_STOPPED)
- return 1;
- }
+ reply = gdb_queue_notif_reply_find (notif_packet_stop.ack_queue,
+ stop_reply_match_ptid_and_ws, &ptid);
- return 0;
+ return (reply != NULL);
}
/* Parse the stop reply in BUF. Either the function succeeds, and the
@@ -5290,7 +5140,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 +5192,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,101 +5334,15 @@ 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
- happen:
-
- 1.1) --> Hg 1
- 1.2) <-- OK
- 1.3) --> g
- 1.4) <-- %Stop
- 1.5) --> vStopped
- 1.6) <-- (registers reply to step #1.3)
-
- Obviously, the reply in step #1.6 would be unexpected to a vStopped
- query.
-
- To solve this, whenever we parse a %Stop notification sucessfully,
- we mark the REMOTE_ASYNC_GET_PENDING_EVENTS_TOKEN, and carry on
- doing whatever we were doing:
-
- 2.1) --> Hg 1
- 2.2) <-- OK
- 2.3) --> g
- 2.4) <-- %Stop
- <GDB marks the REMOTE_ASYNC_GET_PENDING_EVENTS_TOKEN>
- 2.5) <-- (registers reply to step #2.3)
-
- Eventualy after step #2.5, we return to the event loop, which
- notices there's an event on the
- REMOTE_ASYNC_GET_PENDING_EVENTS_TOKEN event and calls the
- associated callback --- the function below. At this point, we're
- always safe to start a vStopped sequence. :
-
- 2.6) --> vStopped
- 2.7) <-- T05 thread:2
- 2.8) --> vStopped
- 2.9) --> OK
-*/
-
-static void
-remote_get_pending_stop_replies (void)
-{
- struct remote_state *rs = get_remote_state ();
-
- if (pending_stop_reply)
- {
- /* acknowledge */
- putpkt ("vStopped");
-
- /* Now we can rely on it. */
- push_stop_reply (pending_stop_reply);
- pending_stop_reply = NULL;
-
- while (1)
- {
- getpkt (&rs->buf, &rs->buf_size, 0);
- 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);
- }
- }
- }
-}
-
-
/* 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 +5354,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 +5386,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 +5425,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.pending_reply != NULL)
+ remote_notif_pending_replies (¬if_packet_stop);
/* If indeed we noticed a stop reply, we're done. */
stop_reply = queued_stop_reply (ptid);
@@ -5758,13 +5522,10 @@ 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;
+ struct stop_reply *stop_reply
+ = (struct stop_reply *) remote_notif_parse (¬if_packet_stop,
+ rs->buf);
- 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);
break;
}
@@ -5845,7 +5606,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 (!gdb_queue_notif_reply_is_empty (notif_packet_stop.ack_queue))
mark_async_event_handler (remote_async_inferior_event_token);
}
@@ -6741,52 +6502,6 @@ remote_read_bytes (CORE_ADDR memaddr, gdb_byte *myaddr, int len)
return i;
}
\f
-
-/* 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. */
- ;
-}
-
-\f
/* 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
@@ -7655,7 +7370,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));
+ discard_pending_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
@@ -11183,12 +10898,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)
{
@@ -11689,5 +11398,7 @@ Show the remote pathname for \"run\""), NULL, NULL, NULL,
target_buf_size = 2048;
target_buf = xmalloc (target_buf_size);
+
+ initialize_notif ();
}
diff --git a/gdb/remote.h b/gdb/remote.h
index 3adc54e..579a92f 100644
--- a/gdb/remote.h
+++ b/gdb/remote.h
@@ -19,8 +19,108 @@
#ifndef REMOTE_H
#define REMOTE_H
+#include "target.h"
+#include "remote-notif.h"
+
struct target_desc;
+/* Description of the remote protocol state for the currently
+ connected target. This is per-target state, and independent of the
+ selected architecture. */
+
+struct remote_state
+{
+ /* A buffer to use for incoming packets, and its current size. The
+ buffer is grown dynamically for larger incoming packets.
+ Outgoing packets may also be constructed in this buffer.
+ BUF_SIZE is always at least REMOTE_PACKET_SIZE;
+ REMOTE_PACKET_SIZE should be used to limit the length of outgoing
+ packets. */
+ char *buf;
+ long buf_size;
+
+ /* True if we're going through initial connection setup (finding out
+ about the remote side's threads, relocating symbols, etc.). */
+ int starting_up;
+
+ /* If we negotiated packet size explicitly (and thus can bypass
+ heuristics for the largest packet size that will not overflow
+ a buffer in the stub), this will be set to that packet size.
+ Otherwise zero, meaning to use the guessed size. */
+ long explicit_packet_size;
+
+ /* remote_wait is normally called when the target is running and
+ waits for a stop reply packet. But sometimes we need to call it
+ when the target is already stopped. We can send a "?" packet
+ and have remote_wait read the response. Or, if we already have
+ the response, we can stash it in BUF and tell remote_wait to
+ skip calling getpkt. This flag is set when BUF contains a
+ stop reply packet and the target is not waiting. */
+ int cached_wait_status;
+
+ /* True, if in no ack mode. That is, neither GDB nor the stub will
+ expect acks from each other. The connection is assumed to be
+ reliable. */
+ int noack_mode;
+
+ /* True if we're connected in extended remote mode. */
+ int extended;
+
+ /* True if the stub reported support for multi-process
+ extensions. */
+ int multi_process_aware;
+
+ /* True if we resumed the target and we're waiting for the target to
+ stop. In the mean time, we can't start another command/query.
+ The remote server wouldn't be ready to process it, so we'd
+ timeout waiting for a reply that would never come and eventually
+ we'd close the connection. This can happen in asynchronous mode
+ because we allow GDB commands while the target is running. */
+ int waiting_for_stop_reply;
+
+ /* True if the stub reports support for non-stop mode. */
+ int non_stop_aware;
+
+ /* True if the stub reports support for vCont;t. */
+ int support_vCont_t;
+
+ /* True if the stub reports support for conditional tracepoints. */
+ int cond_tracepoints;
+
+ /* True if the stub reports support for target-side breakpoint
+ conditions. */
+ int cond_breakpoints;
+
+ /* True if the stub reports support for target-side breakpoint
+ commands. */
+ int breakpoint_commands;
+
+ /* True if the stub reports support for fast tracepoints. */
+ int fast_tracepoints;
+
+ /* True if the stub reports support for static tracepoints. */
+ int static_tracepoints;
+
+ /* True if the stub reports support for installing tracepoint while
+ tracing. */
+ int install_in_trace;
+
+ /* True if the stub can continue running a trace while GDB is
+ disconnected. */
+ int disconnected_tracing;
+
+ /* True if the stub reports support for enabling and disabling
+ tracepoints while a trace experiment is running. */
+ int enable_disable_tracepoints;
+
+ /* True if the stub can collect strings using tracenz bytecode. */
+ int string_tracing;
+
+ /* Nonzero if the user has pressed Ctrl-C, but the target hasn't
+ responded to that. */
+ int ctrlc_pending_p;
+};
+
/* Read a packet from the remote machine, with error checking, and
store it in *BUF. Resize *BUF using xrealloc if necessary to hold
the result, and update *SIZEOF_BUF. If FOREVER, wait forever
@@ -59,4 +159,7 @@ extern int remote_register_number_and_offset (struct gdbarch *gdbarch,
int regnum, int *pnum,
int *poffset);
+extern void notif_reply_push (struct notif_reply *,
+ struct gdb_queue_notif_reply *);
+
#endif
--
1.7.7.6
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RCF 0/4] A general notification in GDB RSP
2012-08-24 2:26 [RCF 0/4] A general notification in GDB RSP Yao Qi
` (3 preceding siblings ...)
2012-08-24 2:26 ` [PATCH 2/4] de-couple %Stop from notification: gdb Yao Qi
@ 2012-08-24 17:53 ` Pedro Alves
4 siblings, 0 replies; 19+ messages in thread
From: Pedro Alves @ 2012-08-24 17:53 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On 08/24/2012 03:25 AM, Yao Qi wrote:
> Hi,
> We have more and more requirements that remote target wants to notify
> GDB via RSP at any time during connection when some states are changed
> in remote target. However, GDB doesn't have such general notification
> infrastructure. This is what this patch set tries to add.
>
> Nowadays, notification mechanism exists in GDB for '%Stop' only,
> and works pretty good. My rationale of this work is 'decouple
> %Stop/vStopped from existing notification mechanism so that
> notification mechanism can handle other types of notification.
>
> First of all, let us look at how '%Stop' and 'vStopped' works,
>
> gdb gdbserver
> <- %Stop // [1] Send notification
> [after process other packets]
> -> vStopped // [2] Ack this notification
> <- T05 thread:2 // [3] Reply
> -> vStopped // [4] Continue to query
> <- OK // [5] Done
>
> We can generalize this protocol like this,
>
> gdb gdbserver
> <- %NOTIF // [1] Send notification
> [after process other packets]
> -> vACK // [2] Ack this notification
> <- REPLY // [3] Reply
> -> vACK // [4] Continue to query
> <- OK // [5] Done
>
> For each type of notification, we only have to define three things,
>
> 1) NOTIF, the key word of notification, for example, "Stop"
> 2) ACK, the command GDB ack to this notification, "vStopped" for
> example,
> 3) REPLY, the format of reply to GDB. GDBserver should be able to
> send packet to comply with REPLY, and GDB is able parse the packet,
> and identify the packet is REPLY or not.
>
> Patch 1/4 is to create a general queue data structure which will not
> only be used in patch 2/4 and 3/4, but also can be used in elsewhere
> in GDB.
> The patch 2/4 and 3/4 are to decouple %Stop and vStopped out of
> existing notification, in order to make notification more general.
> Patch 4/4 is to demonstrate how do we add a new type of notification
> in the future, and test two types of notification work good
> together. As you can see, it is relatively simple. Originally, I
> intended to convert qTstatus to a new type of notification in patch
> 4/4 as an example (when tracing status is changed, GDBserver sends
> notification), but it takes time to think about it, so I post this
> version here to get feedbacks first.
>
> All three patches are tested on x86_64-linux {native, gdbserver} x
> {sync, async}. No regression.
>
> Note that the behaviour of notification in RSP is unchanged, that is
> to say, patched GDB (w/ patch 2/4) works well with unpatched
> GDBserver, and patched GDBserver (w/ patch 3/4) works well with
> unpatched GDB.
>
> it is different from Hui's patch 3/9 in 'autload breakpoint'
> patch series,
>
> [RFC] Autoload-breakpoints new version [3/9] notification async
> http://sourceware.org/ml/gdb-patches/2012-08/msg00214.html
>
> His patch is to teach GDB to handle notification at anytime, while
> mine is not, but to allow to add other types of notification similar
> to existing %Stop/vStopped on the same infrastructure.
I haven't read the patches in detail yet, but I'd like to say
upfront that I like this very much.
--
Pedro Alves
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] new gdb_queue.h in common/.
2012-08-24 2:26 ` [PATCH 1/4] new gdb_queue.h in common/ Yao Qi
@ 2012-08-24 18:44 ` dje
2012-08-24 18:49 ` Doug Evans
2012-08-29 9:42 ` Yao Qi
0 siblings, 2 replies; 19+ messages in thread
From: dje @ 2012-08-24 18:44 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
Yao Qi writes:
> Hi,
> When writing the patches for 'general notification', I find queue is
> used in many places, so I think we may need a general queue. This
> queue not only have typical operations enqueue and dequeue, but also
> has some have some operations like remove and remove_all.
>
> gdb/
>
> 2012-08-24 Yao Qi <yao@codesourcery.com>
>
> * common/gdb_queue.h: New.
> ---
> gdb/common/gdb_queue.h | 283 ++++++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 283 insertions(+), 0 deletions(-)
> create mode 100644 gdb/common/gdb_queue.h
>
> diff --git a/gdb/common/gdb_queue.h b/gdb/common/gdb_queue.h
> new file mode 100644
> index 0000000..fa582c1
> --- /dev/null
> +++ b/gdb/common/gdb_queue.h
> @@ -0,0 +1,283 @@
> +/* General queue data structure for GDB, the GNU debugger.
> +
> + Copyright (C) 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/>. */
> +
> +/* These macros implement functions and structs for a general queue. Macro
> + 'QUEUE_DEFINE_TYPE(TYPE)' is to define the new queue type for
> + 'struct TYPE', and macro 'QUEUE_DECLARE' *is to declare these external
> + APIs. When define a queue type for 'struct FOO', an extra helper function
> + 'gdb_queue_ele_FOO_xfree (struct FOO *foo)' has to be defined to release
> + the contents in foo, but not foo itself. */
> +
> +#ifndef GDB_QUEUE_H
> +#define GDB_QUEUE_H
> +
> +#include <stdio.h>
> +
> +#include "libiberty.h" /* xmalloc */
> +#include "gdb_assert.h"
> +
> +/* Define a new queue implementation. */
> +
> +#define QUEUE_DEFINE_TYPE(TYPE) \
DEFINE_QUEUE_TYPE
Plus, seems like we should go all the way and provide I(integer),P(pointer),O(object_) support that vec.h does.
I'm ok with, e.g., just support pointers for now, leaving integers and objects for later.
Also, one can imagine wanting stacks and deques too.
[I only mention it so that we think about it. It would be unfortunate if this proliferated and only later, when it's much harder, do we decide we want some unification.]
> +struct gdb_queue_ele_ ## TYPE \
> +{ \
> + struct gdb_queue_ele_ ## TYPE *next; \
> + \
> + struct TYPE *data; \
> +}; \
> + \
> +struct gdb_queue_ ## TYPE \
> +{ \
> + struct gdb_queue_ele_ ## TYPE *head; \
> + struct gdb_queue_ele_ ## TYPE *tail; \
> +}; \
> + \
> +/* Typical enqueue operation. Put V into queue Q. */ \
> + \
> +void \
> +gdb_queue_ ## TYPE ## _enque (struct TYPE *v, \
> + struct gdb_queue_ ## TYPE *q) \
I think it'd be better to keep the queue argument as the first parameter.
[general rule for every such function]
> +{ \
> + struct gdb_queue_ele_ ## TYPE *p \
> + = xmalloc (sizeof (struct gdb_queue_ele_ ## TYPE)); \
> + \
> + gdb_assert (q != NULL); \
> + p->data = v; \
> + p->next = NULL; \
> + if (q->tail == NULL) \
> + { \
> + q->tail = p; \
> + q->head = p; \
> + } \
> + else \
> + { \
> + q->tail->next = p; \
> + q->tail = p; \
> + } \
> +} \
> + \
> +/* Typical dequeue operation. Return one element queue Q. Return NULL \
> + if queue Q is empty. */ \
Returning NULL means one can't queue it as a value.
[mixed feelings on whether that's important enough]
assert fail if queue is empty?
Plus if one went with vec's IPO support, should this return the int-like type?
[in which case deciding whether to return NULL becomes moot]
> + \
> +struct TYPE * \
> +gdb_queue_ ## TYPE ## _deque (struct gdb_queue_ ## TYPE *q) \
> +{ \
> + struct gdb_queue_ele_ ## TYPE *p = NULL; \
> + struct TYPE *v = NULL; \
> + \
> + if (q == NULL) \
> + return NULL; \
> + p = q->head; \
> + \
> + if (q->head == q->tail) \
> + { \
> + q->head = NULL; \
> + q->tail = NULL; \
> + } \
> + else \
> + q->head = q->head->next; \
> + \
> + if (p != NULL) \
> + v = p->data; \
> + \
> + xfree (p); \
> + return v; \
> +} \
> + \
> +/* Return the element on tail, but don't remove it from queue. Return \
> + NULL if queue is empty. */ \
> + \
> +struct TYPE * \
> +gdb_queue_ ## TYPE ## _peek (struct gdb_queue_ ## TYPE *q) \
> +{ \
> + if (q== NULL || q->head == NULL) \
> + return NULL; \
assert fail if q == NULL?
[missing space in q==]
I wouldn't use "peek" to name a function that returns the tail.
"peek" should return the head.
> + return q->head->data; \
Oh, heh, the function comment is wrong, it returns head. :-)
> +} \
> + \
> +/* Return true if queue Q is empty. */ \
> + \
> +int \
> +gdb_queue_ ## TYPE ## _is_empty (struct gdb_queue_ ## TYPE *q) \
> +{ \
> + return (q == NULL || q->head == NULL); \
assert q != NULL ?
The vec functions handle vec == NULL because it's actually a pointer to the vector (so it can be resized which involves an xrealloc).
We don't need that here, so I'm thinking it'd be better to not be lax, and require q != NULL.
> +} \
> + \
> +/* Iterate over elements in the queue Q, and remove all of them for \
> + which function MATCH returns true. */ \
> + \
> +void \
> +gdb_queue_ ## TYPE ## _remove_all (struct gdb_queue_ ## TYPE *q, \
> + int (*match) (struct TYPE *, void *), \
> + void *data) \
> +{ \
The "all" in "remove_all" is confusing (I'd expected it to empty the queue completely).
How about "remove_matching".
> + struct gdb_queue_ele_ ## TYPE *p = NULL; \
> + struct gdb_queue_ele_ ## TYPE *prev = NULL, *next = NULL; \
> + \
> + if (q == NULL) \
> + return; \
> + \
> + for (p = q->head; p != NULL; p = next) \
> + { \
> + next = p->next; \
> + if (match (p->data, data)) \
> + { \
> + if (p == q->head || p == q->tail) \
> + { \
> + if (p == q->head) \
> + q->head = p->next; \
> + if (p == q->tail) \
> + q->tail = prev; \
> + } \
> + else \
> + prev->next = p->next; \
> + \
> + gdb_queue_ele_ ## TYPE ## _xfree (p->data); \
> + xfree (p->data); \
Why call both ele_TYPE_xfree and xfree?
Maybe the API should include the ability to specify an element destructor, passed as an argument to the queue constructor, akin to the htab API (ref: libiberty/hashtab.c). And perhaps also a copy-constructor for the object version of the API (to copy structs as values in the case where a simple memcpy is insufficient).
> + xfree (p); \
> + } \
> + else \
> + prev = p; \
> + } \
> +} \
> + \
> +/* Iterate over queue Q and remove one element for which function \
> + MATCH returns true. */ \
I'd rather this be clearer and specify that the *first* matching element is returned.
Here things get muddy as it's normal to return NULL if the value wasn't found, but that conflicts with the discussion above. I'm not sure what to do except return a bool indicating success, and pass in a pointer to the value to be returned.
> + \
> +struct TYPE * \
> +gdb_queue_ ## TYPE ## _remove (struct gdb_queue_ ## TYPE *q, \
> + int (*match) (struct TYPE *, void *), \
> + void *data) \
> +{ \
> + struct gdb_queue_ele_ ## TYPE *p = NULL; \
> + struct gdb_queue_ele_ ## TYPE *prev = NULL; \
> + struct TYPE *t = NULL; \
> + \
> + if (q == NULL) \
> + return NULL; \
> + \
> + for (p = q->head; p != NULL; p = p->next) \
> + { \
> + if (match (p->data, data)) \
> + { \
> + if (p == q->head || p == q->tail) \
> + { \
> + if (p == q->head) \
> + q->head = p->next; \
> + if (p == q->tail) \
> + q->tail = prev; \
> + } \
> + else \
> + prev->next = p->next; \
> + \
> + t = p->data; \
> + xfree (p); \
> + return t; \
> + } \
> + else \
> + prev = p; \
> + } \
> + return NULL; \
> +} \
> + \
> +/* Find an element in queue Q for which function MATCH returns true. \
> + Return NULL if not found. */ \
"Find the first element in queue ..."
Return bool, and pass in a pointer into which to store the found value?
[these notes are just for discussion btw]
> + \
> +struct TYPE * \
> +gdb_queue_ ## TYPE ## _find (struct gdb_queue_ ## TYPE *q, \
> + int (*match) (struct TYPE *, void *), \
> + void *data) \
> +{ \
> + struct gdb_queue_ele_ ## TYPE *p = NULL; \
> + \
> + if (q == NULL) \
> + return NULL; \
> + \
> + for (p = q->head; p != NULL; p = p->next) \
> + { \
> + if (match (p->data, data)) \
> + return p->data; \
> + } \
> + return NULL; \
> +} \
> + \
> +/* Allocate memory for queue. */ \
> + \
> +struct gdb_queue_ ## TYPE * \
> +gdb_queue_ ## TYPE ## _alloc (void) \
> +{ \
> + struct gdb_queue_ ## TYPE *p; \
> + \
> + p = (struct gdb_queue_ ## TYPE *) xmalloc (sizeof (struct gdb_queue_ ## TYPE));\
> + p->head = NULL; \
> + p->tail = NULL; \
> + return p; \
> +} \
The queue destructor is missing.
> + \
> +/* Length of queue Q. */ \
> + \
> +int \
> +gdb_queue_ ## TYPE ## _length (struct gdb_queue_ ## TYPE *q) \
> +{ \
> + struct gdb_queue_ele_ ## TYPE *p = NULL; \
> + int len = 0; \
> + \
> + if (q == NULL) \
> + return 0; \
> + \
> + for (p = q->head; p != NULL; p = p->next) \
> + len++; \
> + return len; \
> +} \
> +
> +
> +/* Define a variable of type gdb_queue_ ## TYPE. */
> +
> +#define QUEUE_DEFINE_VAR(TYPE, VAR) \
> + struct gdb_queue_ ## TYPE VAR = { NULL, NULL }
DEFINE_QUEUE_VAR
OTOH, I think I would delete this, and require the user to use gdb_queue_##TYPE##_alloc.
> +
> +/* External declarations for these functions. */
> +#define QUEUE_DECLARE(TYPE) \
DECLARE_QUEUE_TYPE (akin to DEFINE_QUEUE_TYPE).
> +struct gdb_queue_ ## TYPE; \
> +extern void gdb_queue_ ## TYPE ## _enque (struct TYPE *v, \
> + struct gdb_queue_ ## TYPE *q); \
> +extern struct TYPE * \
> + gdb_queue_ ## TYPE ## _deque (struct gdb_queue_ ## TYPE *q); \
> +extern int \
> + gdb_queue_ ## TYPE ## _is_empty (struct gdb_queue_ ## TYPE *q); \
> +extern void \
> + gdb_queue_ ## TYPE ## _remove_all (struct gdb_queue_ ## TYPE *q, \
> + int (*match) (struct TYPE *, void *), \
> + void *data); \
> +extern struct TYPE * \
> + gdb_queue_ ## TYPE ## _remove (struct gdb_queue_ ## TYPE *q, \
> + int (*match) (struct TYPE *, void *), \
> + void *data); \
> +extern struct TYPE * \
> + gdb_queue_ ## TYPE ## _find (struct gdb_queue_ ## TYPE *q, \
> + int (*match) (struct TYPE *, void *), \
> + void *data); \
> +extern struct gdb_queue_ ## TYPE * \
> + gdb_queue_ ## TYPE ## _alloc (void); \
> +extern void gdb_queue_ele_ ## TYPE ## _xfree (struct TYPE *); \
> +extern int gdb_queue_ ## TYPE ## _length (struct gdb_queue_ ## TYPE *q);\
> +extern struct TYPE * \
> + gdb_queue_ ## TYPE ## _peek (struct gdb_queue_ ## TYPE *q); \
> +
> +#endif /* GDB_QUEUE_H */
> --
> 1.7.7.6
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] new gdb_queue.h in common/.
2012-08-24 18:44 ` dje
@ 2012-08-24 18:49 ` Doug Evans
2012-08-29 9:42 ` Yao Qi
1 sibling, 0 replies; 19+ messages in thread
From: Doug Evans @ 2012-08-24 18:49 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On Fri, Aug 24, 2012 at 11:43 AM, <dje@google.com> wrote:
> Yao Qi writes:
> > Hi,
> > When writing the patches for 'general notification', I find queue is
> > used in many places, so I think we may need a general queue. This
> > queue not only have typical operations enqueue and dequeue, but also
> > has some have some operations like remove and remove_all.
Hi. Another thing that occurs to me is that there isn't anything
gdb-specific here.
We have vec.h and gdb_vecs.h.
So "Consistency is good." says we should have queue.h and gdb_queues.h.
[queue.h can be renamed if it collides with something, the point is
that gdb doesn't appear in the name, nor in any symbol defined by it]
Igdb_vecs.h is for very generic vectors, and we don't have any very
generic queues yet, but that's what I'd use gdb_queues.h for.
I think this is a good thing to have btw. :-)
[given that we can't use c++ ...]
> >
> > gdb/
> >
> > 2012-08-24 Yao Qi <yao@codesourcery.com>
> >
> > * common/gdb_queue.h: New.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/4] de-couple %Stop from notification: gdb
2012-08-24 2:26 ` [PATCH 2/4] de-couple %Stop from notification: gdb Yao Qi
2012-08-24 16:52 ` Yao Qi
@ 2012-08-24 19:00 ` dje
2012-08-30 6:40 ` Yao Qi
1 sibling, 1 reply; 19+ messages in thread
From: dje @ 2012-08-24 19:00 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
Yao Qi writes:
> This patch is to de-couple vStopped/%Stop from notification in gdb side.
Hi. Just a few nits, maybe more to follow.
> gdb:
>
> 2012-08-24 Yao Qi <yao@codesourcery.com>
>
> (struct remote_state): Move to remote.h.
What's the reason for moving struct remote_state to remote.h?
[I did a simple grep and couldn't find a reason, I could have missing something of course.]
Having it live in remote.c means the implementation is "private" to just that file.
> diff --git a/gdb/remote-notif.c b/gdb/remote-notif.c
> new file mode 100644
> index 0000000..ca7f821
> --- /dev/null
> +++ b/gdb/remote-notif.c
> @@ -0,0 +1,285 @@
> [...]
> +
> +extern struct remote_state *get_remote_state (void);
Don't declare external functions in .c files, it should live in a header.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] new gdb_queue.h in common/.
2012-08-24 18:44 ` dje
2012-08-24 18:49 ` Doug Evans
@ 2012-08-29 9:42 ` Yao Qi
2012-09-04 21:03 ` dje
1 sibling, 1 reply; 19+ messages in thread
From: Yao Qi @ 2012-08-29 9:42 UTC (permalink / raw)
To: dje; +Cc: gdb-patches
On 08/25/2012 02:43 AM, dje@google.com wrote:
> > +
> > +/* Define a new queue implementation. */
> > +
> > +#define QUEUE_DEFINE_TYPE(TYPE) \
>
> DEFINE_QUEUE_TYPE
>
Personally, I prefer operation prefixed with data type, such
as queue_define_type or queue_enque, etc. However, I am OK
with your suggestion.
> Plus, seems like we should go all the way and provide I(integer),P(pointer),O(object_) support that vec.h does.
> I'm ok with, e.g., just support pointers for now, leaving integers and objects for later.
>
OK.
> Also, one can imagine wanting stacks and deques too.
> [I only mention it so that we think about it. It would be unfortunate if this proliferated and only later, when it's much harder, do we decide we want some unification.]
>
> > +struct gdb_queue_ele_ ## TYPE \
> > +{ \
> > + struct gdb_queue_ele_ ## TYPE *next; \
> > + \
> > + struct TYPE *data; \
> > +}; \
> > + \
> > +struct gdb_queue_ ## TYPE \
> > +{ \
> > + struct gdb_queue_ele_ ## TYPE *head; \
> > + struct gdb_queue_ele_ ## TYPE *tail; \
> > +}; \
> > + \
> > +/* Typical enqueue operation. Put V into queue Q. */ \
> > + \
> > +void \
> > +gdb_queue_ ## TYPE ## _enque (struct TYPE *v, \
> > + struct gdb_queue_ ## TYPE *q) \
>
> I think it'd be better to keep the queue argument as the first parameter.
> [general rule for every such function]
Sure, fixed.
>
> > +{ \
> > + struct gdb_queue_ele_ ## TYPE *p \
> > + = xmalloc (sizeof (struct gdb_queue_ele_ ## TYPE)); \
> > + \
> > + gdb_assert (q != NULL); \
> > + p->data = v; \
> > + p->next = NULL; \
> > + if (q->tail == NULL) \
> > + { \
> > + q->tail = p; \
> > + q->head = p; \
> > + } \
> > + else \
> > + { \
> > + q->tail->next = p; \
> > + q->tail = p; \
> > + } \
> > +} \
> > + \
> > +/* Typical dequeue operation. Return one element queue Q. Return NULL \
> > + if queue Q is empty. */ \
>
> Returning NULL means one can't queue it as a value.
> [mixed feelings on whether that's important enough]
> assert fail if queue is empty?
>
> Plus if one went with vec's IPO support, should this return the int-like type?
> [in which case deciding whether to return NULL becomes moot]
>
In version 1, I focused on Pointer. Considering Object and Int, deque should
return TYPE here, and give an assert fail if something is unexpected.
> > + \
> > +/* Return the element on tail, but don't remove it from queue. Return \
> > + NULL if queue is empty. */ \
> > + \
> > +struct TYPE * \
> > +gdb_queue_ ## TYPE ## _peek (struct gdb_queue_ ## TYPE *q) \
> > +{ \
> > + if (q== NULL || q->head == NULL) \
> > + return NULL; \
>
> assert fail if q == NULL?
> [missing space in q==]
Add an assertion here.
>
> I wouldn't use "peek" to name a function that returns the tail.
> "peek" should return the head.
>
> > + return q->head->data; \
>
> Oh, heh, the function comment is wrong, it returns head. :-)
>
Oh, right. Get comment fixed.
> > +} \
> > + \
> > +/* Return true if queue Q is empty. */ \
> > + \
> > +int \
> > +gdb_queue_ ## TYPE ## _is_empty (struct gdb_queue_ ## TYPE *q) \
> > +{ \
> > + return (q == NULL || q->head == NULL); \
>
> assert q != NULL ?
>
> The vec functions handle vec == NULL because it's actually a pointer to the vector (so it can be resized which involves an xrealloc).
> We don't need that here, so I'm thinking it'd be better to not be lax, and require q != NULL.
>
Agreed. gdb_assert is added here.
> > +} \
> > + \
> > +/* Iterate over elements in the queue Q, and remove all of them for \
> > + which function MATCH returns true. */ \
> > + \
> > +void \
> > +gdb_queue_ ## TYPE ## _remove_all (struct gdb_queue_ ## TYPE *q, \
> > + int (*match) (struct TYPE *, void *), \
> > + void *data) \
> > +{ \
>
> The "all" in "remove_all" is confusing (I'd expected it to empty the queue completely).
> How about "remove_matching".
>
That sounds good.
> > + struct gdb_queue_ele_ ## TYPE *p = NULL; \
> > + struct gdb_queue_ele_ ## TYPE *prev = NULL, *next = NULL; \
> > + \
> > + if (q == NULL) \
> > + return; \
> > + \
> > + for (p = q->head; p != NULL; p = next) \
> > + { \
> > + next = p->next; \
> > + if (match (p->data, data)) \
> > + { \
> > + if (p == q->head || p == q->tail) \
> > + { \
> > + if (p == q->head) \
> > + q->head = p->next; \
> > + if (p == q->tail) \
> > + q->tail = prev; \
> > + } \
> > + else \
> > + prev->next = p->next; \
> > + \
> > + gdb_queue_ele_ ## TYPE ## _xfree (p->data); \
> > + xfree (p->data); \
>
> Why call both ele_TYPE_xfree and xfree?
> Maybe the API should include the ability to specify an element destructor, passed as an argument to the queue constructor, akin to the htab API (ref: libiberty/hashtab.c). And perhaps also a copy-constructor for the object version of the API (to copy structs as values in the case where a simple memcpy is insufficient).
>
In V2, I add a function pointer 'free_func' in 'struct queue_ ##type',
as you suggested. A copy-constructor can be postponed until we need it.
> > + xfree (p); \
> > + } \
> > + else \
> > + prev = p; \
> > + } \
> > +} \
> > + \
> > +/* Iterate over queue Q and remove one element for which function \
> > + MATCH returns true. */ \
>
> I'd rather this be clearer and specify that the *first* matching element is returned.
>
> Here things get muddy as it's normal to return NULL if the value wasn't found, but that conflicts with the discussion above. I'm not sure what to do except return a bool indicating success, and pass in a pointer to the value to be returned.
>
Fixed it by returning boolean indicating success and pass a pointer
of TYPE to the value returned.
> > + \
> > +struct TYPE * \
> > +gdb_queue_ ## TYPE ## _remove (struct gdb_queue_ ## TYPE *q, \
> > + int (*match) (struct TYPE *, void *), \
> > + void *data) \
> > +{ \
> > + struct gdb_queue_ele_ ## TYPE *p = NULL; \
> > + struct gdb_queue_ele_ ## TYPE *prev = NULL; \
> > + struct TYPE *t = NULL; \
> > + \
> > + if (q == NULL) \
> > + return NULL; \
> > + \
> > + for (p = q->head; p != NULL; p = p->next) \
> > + { \
> > + if (match (p->data, data)) \
> > + { \
> > + if (p == q->head || p == q->tail) \
> > + { \
> > + if (p == q->head) \
> > + q->head = p->next; \
> > + if (p == q->tail) \
> > + q->tail = prev; \
> > + } \
> > + else \
> > + prev->next = p->next; \
> > + \
> > + t = p->data; \
> > + xfree (p); \
> > + return t; \
> > + } \
> > + else \
> > + prev = p; \
> > + } \
> > + return NULL; \
> > +} \
> > + \
> > +/* Find an element in queue Q for which function MATCH returns true. \
> > + Return NULL if not found. */ \
>
> "Find the first element in queue ..."
>
> Return bool, and pass in a pointer into which to store the found value?
> [these notes are just for discussion btw]
... likewise here.
>
> > + \
> > +struct TYPE * \
> > +gdb_queue_ ## TYPE ## _find (struct gdb_queue_ ## TYPE *q, \
> > + int (*match) (struct TYPE *, void *), \
> > + void *data) \
> > +{ \
> > + struct gdb_queue_ele_ ## TYPE *p = NULL; \
> > + \
> > + if (q == NULL) \
> > + return NULL; \
> > + \
> > + for (p = q->head; p != NULL; p = p->next) \
> > + { \
> > + if (match (p->data, data)) \
> > + return p->data; \
> > + } \
> > + return NULL; \
> > +} \
> > + \
> > +/* Allocate memory for queue. */ \
> > + \
> > +struct gdb_queue_ ## TYPE * \
> > +gdb_queue_ ## TYPE ## _alloc (void) \
> > +{ \
> > + struct gdb_queue_ ## TYPE *p; \
> > + \
> > + p = (struct gdb_queue_ ## TYPE *) xmalloc (sizeof (struct gdb_queue_ ## TYPE));\
> > + p->head = NULL; \
> > + p->tail = NULL; \
> > + return p; \
> > +} \
>
> The queue destructor is missing.
>
destructor is added.
> > + \
> > +/* Length of queue Q. */ \
> > + \
> > +int \
> > +gdb_queue_ ## TYPE ## _length (struct gdb_queue_ ## TYPE *q) \
> > +{ \
> > + struct gdb_queue_ele_ ## TYPE *p = NULL; \
> > + int len = 0; \
> > + \
> > + if (q == NULL) \
> > + return 0; \
> > + \
> > + for (p = q->head; p != NULL; p = p->next) \
> > + len++; \
> > + return len; \
> > +} \
> > +
> > +
> > +/* Define a variable of type gdb_queue_ ## TYPE. */
> > +
> > +#define QUEUE_DEFINE_VAR(TYPE, VAR) \
> > + struct gdb_queue_ ## TYPE VAR = { NULL, NULL }
>
> DEFINE_QUEUE_VAR
>
> OTOH, I think I would delete this, and require the user to use gdb_queue_##TYPE##_alloc.
>
This macro is removed.
> > +
> > +/* External declarations for these functions. */
> > +#define QUEUE_DECLARE(TYPE) \
>
> DECLARE_QUEUE_TYPE (akin to DEFINE_QUEUE_TYPE).
Fixed.
gdb_queue.h is renamed to queue.h, and all symbols gdb_queue_XXX
are renamed to queue_XXX.
The V2 of this patch obsoletes the rest of patches in this series.
Once it is OK, I'll post updated patch 2/4 and 3/4.
--
Yao
gdb/
2012-08-29 Yao Qi <yao@codesourcery.com>
* common/queue.h: New.
---
gdb/common/queue.h | 312 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 312 insertions(+), 0 deletions(-)
create mode 100644 gdb/common/queue.h
diff --git a/gdb/common/queue.h b/gdb/common/queue.h
new file mode 100644
index 0000000..14e260c
--- /dev/null
+++ b/gdb/common/queue.h
@@ -0,0 +1,312 @@
+/* General queue data structure for GDB, the GNU debugger.
+
+ Copyright (C) 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/>. */
+
+/* These macros implement functions and structs for a general queue. Macro
+ 'DEFINE_QUEUE_TYPE(TYPE)' is to define the new queue type for
+ 'TYPE', and macro 'DECLARE_QUEUE' *is to declare these external
+ APIs. */
+
+#ifndef QUEUE_H
+#define QUEUE_H
+
+#include <stdio.h>
+
+#include "libiberty.h" /* xmalloc */
+#include "gdb_assert.h"
+
+/* Define a new queue implementation. */
+
+#define QUEUE(TYPE) struct queue_ ## TYPE
+
+#define DEFINE_QUEUE_TYPE(TYPE) \
+struct queue_ele_ ## TYPE \
+{ \
+ struct queue_ele_ ## TYPE *next; \
+ \
+ TYPE data; \
+}; \
+ \
+struct queue_ ## TYPE \
+{ \
+ struct queue_ele_ ## TYPE *head; \
+ struct queue_ele_ ## TYPE *tail; \
+ void (*free_func) (void *); \
+}; \
+ \
+/* Typical enqueue operation. Put V into queue Q. */ \
+ \
+void \
+queue_ ## TYPE ## _enque (struct queue_ ## TYPE *q, TYPE v) \
+{ \
+ struct queue_ele_ ## TYPE *p \
+ = xmalloc (sizeof (struct queue_ele_ ## TYPE)); \
+ \
+ gdb_assert (q != NULL); \
+ p->data = v; \
+ p->next = NULL; \
+ if (q->tail == NULL) \
+ { \
+ q->tail = p; \
+ q->head = p; \
+ } \
+ else \
+ { \
+ q->tail->next = p; \
+ q->tail = p; \
+ } \
+} \
+ \
+/* Typical dequeue operation. Return one element queue Q. Assert \
+ fail if Q is NULL or Q is empty. */ \
+ \
+TYPE \
+queue_ ## TYPE ## _deque (struct queue_ ## TYPE *q) \
+{ \
+ struct queue_ele_ ## TYPE *p = NULL; \
+ TYPE v; \
+ \
+ gdb_assert (q != NULL); \
+ p = q->head; \
+ \
+ if (q->head == q->tail) \
+ { \
+ q->head = NULL; \
+ q->tail = NULL; \
+ } \
+ else \
+ q->head = q->head->next; \
+ \
+ gdb_assert (p != NULL); \
+ v = p->data; \
+ \
+ xfree (p); \
+ return v; \
+} \
+ \
+/* Return the element on head, but don't remove it from queue. Assert \
+ fail is Q is NULL or Q is empty. */ \
+ \
+TYPE \
+queue_ ## TYPE ## _peek (struct queue_ ## TYPE *q) \
+{ \
+ gdb_assert (q != NULL); \
+ gdb_assert (q->head != NULL); \
+ return q->head->data; \
+} \
+ \
+/* Return true if queue Q is empty. */ \
+ \
+int \
+queue_ ## TYPE ## _is_empty (struct queue_ ## TYPE *q) \
+{ \
+ gdb_assert (q != NULL); \
+ return (q->head == NULL); \
+} \
+ \
+/* Iterate over elements in the queue Q, and remove all of them for \
+ which function MATCH returns true. */ \
+ \
+void \
+queue_ ## TYPE ## _remove_matching (struct queue_ ## TYPE *q, \
+ int (*match) (TYPE, void *), \
+ void *data) \
+{ \
+ struct queue_ele_ ## TYPE *p = NULL; \
+ struct queue_ele_ ## TYPE *prev = NULL, *next = NULL; \
+ \
+ if (q == NULL) \
+ return; \
+ \
+ for (p = q->head; p != NULL; p = next) \
+ { \
+ next = p->next; \
+ if (match (p->data, data)) \
+ { \
+ if (p == q->head || p == q->tail) \
+ { \
+ if (p == q->head) \
+ q->head = p->next; \
+ if (p == q->tail) \
+ q->tail = prev; \
+ } \
+ else \
+ prev->next = p->next; \
+ \
+ if (q->free_func != NULL) \
+ q->free_func (p->data); \
+ xfree (p); \
+ } \
+ else \
+ prev = p; \
+ } \
+} \
+ \
+/* Iterate over queue Q and remove the first element for which \
+ function MATCH returns true. Return true if element is removed, \
+ and set data to V. Otherwise return false. */ \
+ \
+int \
+queue_ ## TYPE ## _remove (struct queue_ ## TYPE *q, TYPE *v, \
+ int (*match) (TYPE, void *), \
+ void *data) \
+{ \
+ struct queue_ele_ ## TYPE *p = NULL; \
+ struct queue_ele_ ## TYPE *prev = NULL; \
+ \
+ if (q == NULL) \
+ return 0; \
+ \
+ for (p = q->head; p != NULL; p = p->next) \
+ { \
+ if (match (p->data, data)) \
+ { \
+ if (p == q->head || p == q->tail) \
+ { \
+ if (p == q->head) \
+ q->head = p->next; \
+ if (p == q->tail) \
+ q->tail = prev; \
+ } \
+ else \
+ prev->next = p->next; \
+ \
+ *v = p->data; \
+ xfree (p); \
+ return 1; \
+ } \
+ else \
+ prev = p; \
+ } \
+ return 0; \
+} \
+ \
+/* Find the first element in queue Q for which function MATCH returns \
+ true. Return true if found, and set found element to V. Otherwise \
+ return false.. */ \
+ \
+int \
+queue_ ## TYPE ## _find (struct queue_ ## TYPE *q, TYPE *v, \
+ int (*match) (TYPE, void *), \
+ void *data) \
+{ \
+ struct queue_ele_ ## TYPE *p = NULL; \
+ \
+ if (q == NULL) \
+ return 0; \
+ \
+ for (p = q->head; p != NULL; p = p->next) \
+ if (match (p->data, data)) \
+ { \
+ *v = p->data; \
+ return 1; \
+ } \
+ return 0; \
+} \
+ \
+/* Allocate memory for queue. */ \
+ \
+struct queue_ ## TYPE * \
+queue_ ## TYPE ## _alloc (void (*free_func) (void *)) \
+{ \
+ struct queue_ ## TYPE *p; \
+ \
+ p = (struct queue_ ## TYPE *) xmalloc (sizeof (struct queue_ ## TYPE));\
+ p->head = NULL; \
+ p->tail = NULL; \
+ p->free_func = free_func; \
+ return p; \
+} \
+ \
+/* Length of queue Q. */ \
+ \
+int \
+queue_ ## TYPE ## _length (struct queue_ ## TYPE *q) \
+{ \
+ struct queue_ele_ ## TYPE *p = NULL; \
+ int len = 0; \
+ \
+ if (q == NULL) \
+ return 0; \
+ \
+ for (p = q->head; p != NULL; p = p->next) \
+ len++; \
+ return len; \
+} \
+/* Free memory for queue Q. */ \
+ \
+void \
+queue_ ## TYPE ## _free (struct queue_ ## TYPE *q) \
+{ \
+ struct queue_ele_ ## TYPE *p, *next; \
+ \
+ if (q == NULL) \
+ return; \
+ \
+ for (p = q->head; p != NULL; p = next) \
+ { \
+ next = p->next; \
+ if (q->free_func) \
+ q->free_func (p->data); \
+ xfree (p); \
+ } \
+} \
+
+/* External declarations for these functions. */
+#define DECLARE_QUEUE_TYPE(TYPE) \
+struct queue_ ## TYPE; \
+extern void \
+ queue_ ## TYPE ## _enque (struct queue_ ## TYPE *q, TYPE v); \
+extern TYPE \
+ queue_ ## TYPE ## _deque (struct queue_ ## TYPE *q); \
+extern int queue_ ## TYPE ## _is_empty (struct queue_ ## TYPE *q); \
+extern void \
+ queue_ ## TYPE ## _remove_matching (struct queue_ ## TYPE *q, \
+ int (*match) (TYPE, void *), \
+ void *data); \
+extern int \
+ queue_ ## TYPE ## _remove (struct queue_ ## TYPE *q, TYPE *v, \
+ int (*match) (TYPE, void *), \
+ void *data); \
+extern int \
+ queue_ ## TYPE ## _find (struct queue_ ## TYPE *q, TYPE *v, \
+ int (*match) (TYPE, void *), \
+ void *data); \
+extern struct queue_ ## TYPE * \
+ queue_ ## TYPE ## _alloc (void (*free_func) (void *)); \
+extern int queue_ ## TYPE ## _length (struct queue_ ## TYPE *q); \
+extern TYPE \
+ queue_ ## TYPE ## _peek (struct queue_ ## TYPE *q); \
+extern void queue_ ## TYPE ## _free (struct queue_ ## TYPE *q); \
+
+
+#define QUEUE_enque(TYPE, QUEUE, V) queue_ ## TYPE ## _enque (QUEUE, V)
+#define QUEUE_deque(TYPE, QUEUE) queue_ ## TYPE ## _deque (QUEUE)
+#define QUEUE_peek(TYPE, QUEUE) queue_ ## TYPE ## _peek (QUEUE)
+#define QUEUE_is_empty(TYPE, QUEUE) queue_ ## TYPE ## _is_empty (QUEUE)
+#define QUEUE_remove(TYPE, QUEUE, V, MATCH, DATA) \
+ queue_ ## TYPE ## _remove (QUEUE, V, MATCH, DATA)
+#define QUEUE_remove_matching(TYPE, QUEUE, MATCH, DATA) \
+ queue_ ## TYPE ## _remove_matching (QUEUE, MATCH, DATA)
+#define QUEUE_find(TYPE, QUEUE, V, MATCH, DATA) \
+ queue_ ## TYPE ## _find (QUEUE, V, MATCH, DATA)
+#define QUEUE_alloc(TYPE, FUNC) queue_ ## TYPE ## _alloc (FUNC)
+#define QUEUE_length(TYPE, QUEUE) queue_ ## TYPE ## _length (QUEUE)
+#define QUEUE_free(TYPE, QUEUE) queue_ ## TYPE ## _free (QUEUE)
+
+#endif /* QUEUE_H */
--
1.7.7.6
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/4] de-couple %Stop from notification: gdb
2012-08-24 19:00 ` dje
@ 2012-08-30 6:40 ` Yao Qi
0 siblings, 0 replies; 19+ messages in thread
From: Yao Qi @ 2012-08-30 6:40 UTC (permalink / raw)
To: dje; +Cc: gdb-patches
On 08/25/2012 02:59 AM, dje@google.com wrote:
>> (struct remote_state): Move to remote.h.
>
> What's the reason for moving struct remote_state to remote.h?
> [I did a simple grep and couldn't find a reason, I could have missing something of course.]
> Having it live in remote.c means the implementation is "private" to just that file.
I hesitated to move 'struct remote_state' out of remote.c, and agree
with you we may keep it private.
The reason I move it out is remote-notif.c:remote_notif_pending_replies
uses 'remote_state' in this way,
void
remote_notif_pending_replies (struct notif *np)
{
struct remote_state *rs = get_remote_state ();
....
....
getpkt (&rs->buf, &rs->buf_size, 0);
If we really don't want to expose 'struct remote_state', we can keep it
living in remote.c, and move function remote_notif_pending_replies to
remote.c accordingly.
--
Yao
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] new gdb_queue.h in common/.
2012-08-29 9:42 ` Yao Qi
@ 2012-09-04 21:03 ` dje
2012-09-07 2:47 ` Yao Qi
2012-09-11 16:50 ` Tom Tromey
0 siblings, 2 replies; 19+ messages in thread
From: dje @ 2012-09-04 21:03 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
Yao Qi writes:
> On 08/25/2012 02:43 AM, dje@google.com wrote:
> [...]
> > Why call both ele_TYPE_xfree and xfree?
> > Maybe the API should include the ability to specify an element destructor, passed as an argument to the queue constructor, akin to the htab API (ref: libiberty/hashtab.c). And perhaps also a copy-constructor for the object version of the API (to copy structs as values in the case where a simple memcpy is insufficient).
> >
>
> In V2, I add a function pointer 'free_func' in 'struct queue_ ##type',
> as you suggested. A copy-constructor can be postponed until we need it.
Yeah, OTOH this is how "extended" versions of API functions come into being.
They're a wart in the API so I like to avoid them.
E.g. consider htab_create_alloc vs htab_create_alloc_ex in the hashtab API.
Some might not think this is a wart, alas I do, so this is one situation
where I don't like to lazily add stuff (when I'm aware of it at
the time ... :-)).
OTOOH, since we're lazily adding the object version of the API anyway,
I don't feel too strongly about it here.
> The V2 of this patch obsoletes the rest of patches in this series.
> Once it is OK, I'll post updated patch 2/4 and 3/4.
Thanks btw!
Note that part of the support for pointers, ints, and objects in vec.h
involves naming. That's missing here.
So while we don't need to implement the _I (integral) and _O (object)
versions of this, we still need the _P (pointer) naming (IMO).
[Also, we don't have to implement deques and stacks here.
I just wanted thought put into what it might look like, and see if that
would influence the design here. I think(!) the API won't change,
so I'm happy to leave it at that for now.]
> 2012-08-29 Yao Qi <yao@codesourcery.com>
>
> * common/queue.h: New.
> ---
> gdb/common/queue.h | 312 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 312 insertions(+), 0 deletions(-)
> create mode 100644 gdb/common/queue.h
>
> diff --git a/gdb/common/queue.h b/gdb/common/queue.h
> new file mode 100644
> index 0000000..14e260c
> --- /dev/null
> +++ b/gdb/common/queue.h
> @@ -0,0 +1,312 @@
> +/* General queue data structure for GDB, the GNU debugger.
> +
> + Copyright (C) 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/>. */
> +
> +/* These macros implement functions and structs for a general queue. Macro
> + 'DEFINE_QUEUE_TYPE(TYPE)' is to define the new queue type for
> + 'TYPE', and macro 'DECLARE_QUEUE' *is to declare these external
> + APIs. */
> +
> +#ifndef QUEUE_H
> +#define QUEUE_H
> +
> +#include <stdio.h>
> +
> +#include "libiberty.h" /* xmalloc */
> +#include "gdb_assert.h"
> +
> +/* Define a new queue implementation. */
> +
> +#define QUEUE(TYPE) struct queue_ ## TYPE
There should also be a similar macro for queue_ele_. QUEUE_ELE?
[I like ELM instead of ELE, but I don't feel strongly enough. :-)]
And both should be used *throughout* the implementation here.
vec.h also has VEC_OP but I'm not sure it's useful enough yet here
(vec.h API functions can call other API functions so it's more useful there).
> +
> +#define DEFINE_QUEUE_TYPE(TYPE) \
Maybe this should be DEF_QUEUE_P (_P for the pointer version,
akin to DEF_VEC_P), but IIRC vec.h doesn't have a "DECLARE_FOO" macro,
which this has. So I'm happy with DEFINE_QUEUE_P (better matches
DECLARE_QUEUE_P).
> +struct queue_ele_ ## TYPE \
> +{ \
> + struct queue_ele_ ## TYPE *next; \
> + \
> + TYPE data; \
> +}; \
> + \
> +struct queue_ ## TYPE \
> +{ \
> + struct queue_ele_ ## TYPE *head; \
> + struct queue_ele_ ## TYPE *tail; \
> + void (*free_func) (void *); \
Can free_func be passed TYPE instead of void *?
> +}; \
> + \
> +/* Typical enqueue operation. Put V into queue Q. */ \
> + \
> +void \
> +queue_ ## TYPE ## _enque (struct queue_ ## TYPE *q, TYPE v) \
> +{ \
> + struct queue_ele_ ## TYPE *p \
> + = xmalloc (sizeof (struct queue_ele_ ## TYPE)); \
> + \
> + gdb_assert (q != NULL); \
> + p->data = v; \
> + p->next = NULL; \
> + if (q->tail == NULL) \
> + { \
> + q->tail = p; \
> + q->head = p; \
> + } \
> + else \
> + { \
> + q->tail->next = p; \
> + q->tail = p; \
> + } \
> +} \
> + \
> +/* Typical dequeue operation. Return one element queue Q. Assert \
> + fail if Q is NULL or Q is empty. */ \
> + \
> +TYPE \
> +queue_ ## TYPE ## _deque (struct queue_ ## TYPE *q) \
> +{ \
> + struct queue_ele_ ## TYPE *p = NULL; \
> + TYPE v; \
> + \
> + gdb_assert (q != NULL); \
> + p = q->head; \
> + \
> + if (q->head == q->tail) \
> + { \
> + q->head = NULL; \
> + q->tail = NULL; \
> + } \
> + else \
> + q->head = q->head->next; \
> + \
> + gdb_assert (p != NULL); \
It'd be better to move this assert above to right after assigning to p.
> + v = p->data; \
> + \
> + xfree (p); \
> + return v; \
> +} \
> + \
> +/* Return the element on head, but don't remove it from queue. Assert \
> + fail is Q is NULL or Q is empty. */ \
> + \
> +TYPE \
> +queue_ ## TYPE ## _peek (struct queue_ ## TYPE *q) \
> +{ \
> + gdb_assert (q != NULL); \
> + gdb_assert (q->head != NULL); \
> + return q->head->data; \
> +} \
> + \
> +/* Return true if queue Q is empty. */ \
> + \
> +int \
> +queue_ ## TYPE ## _is_empty (struct queue_ ## TYPE *q) \
> +{ \
> + gdb_assert (q != NULL); \
> + return (q->head == NULL); \
No need to put q->head == NULL in parens.
> +} \
> + \
> +/* Iterate over elements in the queue Q, and remove all of them for \
> + which function MATCH returns true. */ \
> + \
> +void \
> +queue_ ## TYPE ## _remove_matching (struct queue_ ## TYPE *q, \
> + int (*match) (TYPE, void *), \
> + void *data) \
> +{ \
> + struct queue_ele_ ## TYPE *p = NULL; \
> + struct queue_ele_ ## TYPE *prev = NULL, *next = NULL; \
I'd remove the initialization of p = NULL.
> + \
> + if (q == NULL) \
> + return; \
assert q != NULL.
> + \
> + for (p = q->head; p != NULL; p = next) \
> + { \
> + next = p->next; \
> + if (match (p->data, data)) \
> + { \
> + if (p == q->head || p == q->tail) \
> + { \
> + if (p == q->head) \
> + q->head = p->next; \
> + if (p == q->tail) \
> + q->tail = prev; \
> + } \
> + else \
> + prev->next = p->next; \
> + \
> + if (q->free_func != NULL) \
> + q->free_func (p->data); \
> + xfree (p); \
> + } \
> + else \
> + prev = p; \
> + } \
> +} \
> + \
> +/* Iterate over queue Q and remove the first element for which \
> + function MATCH returns true. Return true if element is removed, \
> + and set data to V. Otherwise return false. */ \
"and set data to V" is a bit confusing (since "data" is also a parameter).
How about "and store the queue element in V" ?
> + \
> +int \
> +queue_ ## TYPE ## _remove (struct queue_ ## TYPE *q, TYPE *v, \
> + int (*match) (TYPE, void *), \
> + void *data) \
> +{ \
> + struct queue_ele_ ## TYPE *p = NULL; \
Remove assignment of p = NULL.
> + struct queue_ele_ ## TYPE *prev = NULL; \
> + \
> + if (q == NULL) \
> + return 0; \
assert q != NULL.
> + \
> + for (p = q->head; p != NULL; p = p->next) \
> + { \
> + if (match (p->data, data)) \
> + { \
> + if (p == q->head || p == q->tail) \
> + { \
> + if (p == q->head) \
> + q->head = p->next; \
> + if (p == q->tail) \
> + q->tail = prev; \
> + } \
> + else \
> + prev->next = p->next; \
> + \
> + *v = p->data; \
> + xfree (p); \
> + return 1; \
> + } \
> + else \
> + prev = p; \
> + } \
> + return 0; \
> +} \
> + \
> +/* Find the first element in queue Q for which function MATCH returns \
> + true. Return true if found, and set found element to V. Otherwise \
> + return false.. */ \
Apologies for not bringing this up earlier.
_remove_matching, _remove, and _find are quite similar, makes me think
an iterator could replace them.
If the iterator "traverse" function returned a boolean of whether to continue
or not, and there was a new API function that deleted a queue element given
an iterator, both the above _remove_matching and _remove functions could be
subsumed by them, and the result would be more useful.
To implement the _find function, the caller could store the found entry in
space allocated for it in the data parameter.
This would also allow finding multiple matching entries.
I like thinner APIs than fatter ones, but in this case I'm not sure
this is better (or worth it).
Thoughts?
> + \
> +int \
> +queue_ ## TYPE ## _find (struct queue_ ## TYPE *q, TYPE *v, \
> + int (*match) (TYPE, void *), \
> + void *data) \
> +{ \
> + struct queue_ele_ ## TYPE *p = NULL; \
> + \
> + if (q == NULL) \
> + return 0; \
> + \
> + for (p = q->head; p != NULL; p = p->next) \
> + if (match (p->data, data)) \
> + { \
> + *v = p->data; \
> + return 1; \
> + } \
> + return 0; \
> +} \
> + \
> +/* Allocate memory for queue. */ \
> + \
> +struct queue_ ## TYPE * \
> +queue_ ## TYPE ## _alloc (void (*free_func) (void *)) \
> +{ \
> + struct queue_ ## TYPE *p; \
For consistency's sake, use q instead of p here.
> + \
> + p = (struct queue_ ## TYPE *) xmalloc (sizeof (struct queue_ ## TYPE));\
> + p->head = NULL; \
> + p->tail = NULL; \
> + p->free_func = free_func; \
> + return p; \
> +} \
> + \
> +/* Length of queue Q. */ \
> + \
> +int \
> +queue_ ## TYPE ## _length (struct queue_ ## TYPE *q) \
> +{ \
> + struct queue_ele_ ## TYPE *p = NULL; \
> + int len = 0; \
> + \
> + if (q == NULL) \
> + return 0; \
assert q != NULL.
> + \
> + for (p = q->head; p != NULL; p = p->next) \
> + len++; \
> + return len; \
> +} \
blank line needed here
> +/* Free memory for queue Q. */ \
> + \
> +void \
> +queue_ ## TYPE ## _free (struct queue_ ## TYPE *q) \
> +{ \
> + struct queue_ele_ ## TYPE *p, *next; \
> + \
> + if (q == NULL) \
> + return; \
assert q != NULL?
> + \
> + for (p = q->head; p != NULL; p = next) \
> + { \
> + next = p->next; \
> + if (q->free_func) \
> + q->free_func (p->data); \
> + xfree (p); \
> + } \
xfree (q) before returning.
> +} \
> +
> +/* External declarations for these functions. */
> +#define DECLARE_QUEUE_TYPE(TYPE) \
DECLARE_QUEUE_P
> +struct queue_ ## TYPE; \
> +extern void \
> + queue_ ## TYPE ## _enque (struct queue_ ## TYPE *q, TYPE v); \
> +extern TYPE \
> + queue_ ## TYPE ## _deque (struct queue_ ## TYPE *q); \
> +extern int queue_ ## TYPE ## _is_empty (struct queue_ ## TYPE *q); \
> +extern void \
> + queue_ ## TYPE ## _remove_matching (struct queue_ ## TYPE *q, \
> + int (*match) (TYPE, void *), \
> + void *data); \
> +extern int \
> + queue_ ## TYPE ## _remove (struct queue_ ## TYPE *q, TYPE *v, \
> + int (*match) (TYPE, void *), \
> + void *data); \
> +extern int \
> + queue_ ## TYPE ## _find (struct queue_ ## TYPE *q, TYPE *v, \
> + int (*match) (TYPE, void *), \
> + void *data); \
> +extern struct queue_ ## TYPE * \
> + queue_ ## TYPE ## _alloc (void (*free_func) (void *)); \
> +extern int queue_ ## TYPE ## _length (struct queue_ ## TYPE *q); \
> +extern TYPE \
> + queue_ ## TYPE ## _peek (struct queue_ ## TYPE *q); \
> +extern void queue_ ## TYPE ## _free (struct queue_ ## TYPE *q); \
> +
> +
In the following, where the macro takes multiple parameters,
put each parameter in parens.
> +#define QUEUE_enque(TYPE, QUEUE, V) queue_ ## TYPE ## _enque (QUEUE, V)
> +#define QUEUE_deque(TYPE, QUEUE) queue_ ## TYPE ## _deque (QUEUE)
> +#define QUEUE_peek(TYPE, QUEUE) queue_ ## TYPE ## _peek (QUEUE)
> +#define QUEUE_is_empty(TYPE, QUEUE) queue_ ## TYPE ## _is_empty (QUEUE)
> +#define QUEUE_remove(TYPE, QUEUE, V, MATCH, DATA) \
> + queue_ ## TYPE ## _remove (QUEUE, V, MATCH, DATA)
> +#define QUEUE_remove_matching(TYPE, QUEUE, MATCH, DATA) \
> + queue_ ## TYPE ## _remove_matching (QUEUE, MATCH, DATA)
> +#define QUEUE_find(TYPE, QUEUE, V, MATCH, DATA) \
> + queue_ ## TYPE ## _find (QUEUE, V, MATCH, DATA)
> +#define QUEUE_alloc(TYPE, FUNC) queue_ ## TYPE ## _alloc (FUNC)
I'd rename FUNC to FREE_FUNC.
> +#define QUEUE_length(TYPE, QUEUE) queue_ ## TYPE ## _length (QUEUE)
> +#define QUEUE_free(TYPE, QUEUE) queue_ ## TYPE ## _free (QUEUE)
> +
> +#endif /* QUEUE_H */
> --
> 1.7.7.6
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] new gdb_queue.h in common/.
2012-09-04 21:03 ` dje
@ 2012-09-07 2:47 ` Yao Qi
2012-09-10 20:47 ` dje
2012-09-11 16:50 ` Tom Tromey
1 sibling, 1 reply; 19+ messages in thread
From: Yao Qi @ 2012-09-07 2:47 UTC (permalink / raw)
To: dje; +Cc: gdb-patches
On 09/05/2012 05:02 AM, dje@google.com wrote:
> There should also be a similar macro for queue_ele_. QUEUE_ELE?
> [I like ELM instead of ELE, but I don't feel strongly enough. :-)]
> And both should be used*throughout* the implementation here.
>
I am using QUEUE_ELEM in this new version.
> vec.h also has VEC_OP but I'm not sure it's useful enough yet here
> (vec.h API functions can call other API functions so it's more useful there).
>
> > +
> > +#define DEFINE_QUEUE_TYPE(TYPE) \
>
> Maybe this should be DEF_QUEUE_P (_P for the pointer version,
> akin to DEF_VEC_P), but IIRC vec.h doesn't have a "DECLARE_FOO" macro,
> which this has. So I'm happy with DEFINE_QUEUE_P (better matches
> DECLARE_QUEUE_P).
>
OK, let us use 'DEFINE_QUEUE_P' and 'DECLARE_QUEUE_P'.
> > +struct queue_ele_ ## TYPE \
> > +{ \
> > + struct queue_ele_ ## TYPE *next; \
> > + \
> > + TYPE data; \
> > +}; \
> > + \
> > +struct queue_ ## TYPE \
> > +{ \
> > + struct queue_ele_ ## TYPE *head; \
> > + struct queue_ele_ ## TYPE *tail; \
> > + void (*free_func) (void *); \
>
> Can free_func be passed TYPE instead of void *?
>
Sure. Fixed.
> > + \
> > +/* Typical dequeue operation. Return one element queue Q. Assert \
> > + fail if Q is NULL or Q is empty. */ \
> > + \
> > +TYPE \
> > +queue_ ## TYPE ##_deque (struct queue_ ## TYPE *q) \
> > +{ \
> > + struct queue_ele_ ## TYPE *p = NULL; \
> > + TYPE v; \
> > + \
> > + gdb_assert (q != NULL); \
> > + p = q->head; \
> > + \
> > + if (q->head == q->tail) \
> > + { \
> > + q->head = NULL; \
> > + q->tail = NULL; \
> > + } \
> > + else \
> > + q->head = q->head->next; \
> > + \
> > + gdb_assert (p != NULL); \
>
> It'd be better to move this assert above to right after assigning to p.
OK, done.
> > + \
> > +int \
> > +queue_ ## TYPE ##_is_empty (struct queue_ ## TYPE *q) \
> > +{ \
> > + gdb_assert (q != NULL); \
> > + return (q->head == NULL); \
>
> No need to put q->head == NULL in parens.
Removed.
>
> > +} \
> > + \
> > +/* Iterate over elements in the queue Q, and remove all of them for \
> > + which function MATCH returns true. */ \
> > + \
> > +void \
> > +queue_ ## TYPE ##_remove_matching (struct queue_ ## TYPE *q, \
> > + int (*match) (TYPE, void *), \
> > + void *data) \
> > +{ \
> > + struct queue_ele_ ## TYPE *p = NULL; \
> > + struct queue_ele_ ## TYPE *prev = NULL, *next = NULL; \
>
> I'd remove the initialization of p = NULL.
OK.
>
> > + \
> > + if (q == NULL) \
> > + return; \
>
> assert q != NULL.
>
I replaced all null pointer checking with assert.
> > + \
> > +/* Iterate over queue Q and remove the first element for which \
> > + function MATCH returns true. Return true if element is removed, \
> > + and set data to V. Otherwise return false. */ \
>
> "and set data to V" is a bit confusing (since "data" is also a parameter).
> How about "and store the queue element in V" ?
>
OK.
> > + \
> > +int \
> > +queue_ ## TYPE ##_remove (struct queue_ ## TYPE *q, TYPE *v, \
> > + int (*match) (TYPE, void *), \
> > + void *data) \
> > +{ \
> > + struct queue_ele_ ## TYPE *p = NULL; \
>
> Remove assignment of p = NULL.
Fixed.
> > + \
> > +/* Find the first element in queue Q for which function MATCH returns \
> > + true. Return true if found, and set found element to V. Otherwise \
> > + return false.. */ \
>
> Apologies for not bringing this up earlier.
Never mind :)
> _remove_matching, _remove, and _find are quite similar, makes me think
> an iterator could replace them.
> If the iterator "traverse" function returned a boolean of whether to continue
> or not, and there was a new API function that deleted a queue element given
> an iterator, both the above _remove_matching and _remove functions could be
> subsumed by them, and the result would be more useful.
>
> To implement the _find function, the caller could store the found entry in
> space allocated for it in the data parameter.
> This would also allow finding multiple matching entries.
I tried to write an iterator, and build _remove_matching, _remove and _find
on top of it. However, it doesn't allow finding multiple matching results.
>
> I like thinner APIs than fatter ones, but in this case I'm not sure
> this is better (or worth it).
> Thoughts?
>
Yeah, I agree. However, 'remove/remove_matching/find' are clear, but
'iterate' with different callbacks to achieve the same task are hard to
read. I post an implementation of _iterate method, and either way is OK
to me.
/* Iterate over elements in the queue Q. If function MATCH returns \
true, call function OPERATE_ON_MATCH. If function OPERATE_ON_MATCH \
returns true, return with true driectly, otherwise, continue to
traverse elements in queue. If none of elements matches, return \
false. */ \
\
int \
queue_ ## TYPE ## _iterate (QUEUE (TYPE) *q, TYPE *v, \
int (*match) (TYPE, void *), \
int (*operate_on_match) (QUEUE (TYPE) *, \
QUEUE_ELEM (TYPE) *, \
QUEUE_ELEM (TYPE) **), \
void *data) \
{ \
int matches = 0; \
\
QUEUE_ELEM (TYPE) *p; \
QUEUE_ELEM (TYPE) *prev = NULL, *next = NULL; \
\
gdb_assert (q != NULL); \
\
for (p = q->head; p != NULL; p = next) \
{ \
next = p->next; \
if (match (p->data, data)) \
{ \
matches = 1; \
if (v != NULL) \
*v = p->data; \
\
if (operate_on_match (q, p, &prev)) \
return 1; \
} \
else \
prev = p; \
} \
return matches; \
} \
You can see the implementation of _find on top of it in this patch.
We can implement '_remove_matching' like this,
static int
always_remove_ele_on_match (QUEUE (notif_reply_p) *q,
QUEUE_ELEM (notif_reply_p) *p,
QUEUE_ELEM (notif_reply_p) ** prev)
{
if (q->free_func != NULL)
q->free_func (p->data);
QUEUE_remove_ele (notif_reply_p, q, p, *prev);
return 0;
}
QUEUE_iterate (notif_reply_p, notif->queue, NULL, notif_reply_match_pid,
always_remove_ele_on_match, &pid);
We can implement '_remove' like this:
static int
remote_notif_remove_once_on_match (QUEUE (notif_reply_p) *q,
QUEUE_ELEM (notif_reply_p) *p,
QUEUE_ELEM (notif_reply_p) **prev)
{
QUEUE_remove_ele (notif_reply_p, q, p, *prev);
return 1;
}
QUEUE_iterate (notif_reply_p, np->ack_queue, &reply, match,
remote_notif_remove_once_on_match, data);
> > + \
> > +int \
> > +queue_ ## TYPE ##_find (struct queue_ ## TYPE *q, TYPE *v, \
> > + int (*match) (TYPE, void *), \
> > + void *data) \
> > +{ \
> > + struct queue_ele_ ## TYPE *p = NULL; \
> > + \
> > + if (q == NULL) \
> > + return 0; \
> > + \
> > + for (p = q->head; p != NULL; p = p->next) \
> > + if (match (p->data, data)) \
> > + { \
> > + *v = p->data; \
> > + return 1; \
> > + } \
> > + return 0; \
> > +} \
> > + \
> > +/* Allocate memory for queue. */ \
> > + \
> > +struct queue_ ## TYPE * \
> > +queue_ ## TYPE ## _alloc (void (*free_func) (void *)) \
> > +{ \
> > + struct queue_ ## TYPE *p; \
>
> For consistency's sake, use q instead of p here.
OK, fixed.
> > + \
> > + for (p = q->head; p != NULL; p = p->next) \
> > + len++; \
> > + return len; \
> > +} \
>
> blank line needed here
Done.
>
> > +/* Free memory for queue Q. */ \
> > + \
> > +void \
> > +queue_ ## TYPE ##_free (struct queue_ ## TYPE *q) \
> > +{ \
> > + struct queue_ele_ ## TYPE *p, *next; \
> > + \
> > + if (q == NULL) \
> > + return; \
>
> assert q != NULL?
>
> > + \
> > + for (p = q->head; p != NULL; p = next) \
> > + { \
> > + next = p->next; \
> > + if (q->free_func) \
> > + q->free_func (p->data); \
> > + xfree (p); \
> > + } \
>
> xfree (q) before returning.
>
Add xfree here.
> > +} \
> > +
> > +/* External declarations for these functions. */
> > +#define DECLARE_QUEUE_TYPE(TYPE) \
>
> DECLARE_QUEUE_P
>
Fixed.
--
Yao
gdb/
2012-09-06 Yao Qi <yao@codesourcery.com>
* common/queue.h: New.
---
gdb/common/queue.h | 292 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 292 insertions(+), 0 deletions(-)
create mode 100644 gdb/common/queue.h
diff --git a/gdb/common/queue.h b/gdb/common/queue.h
new file mode 100644
index 0000000..fff6778
--- /dev/null
+++ b/gdb/common/queue.h
@@ -0,0 +1,292 @@
+/* General queue data structure for GDB, the GNU debugger.
+
+ Copyright (C) 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/>. */
+
+/* These macros implement functions and structs for a general queue. Macro
+ 'DEFINE_QUEUE_TYPE(TYPE)' is to define the new queue type for
+ 'TYPE', and macro 'DECLARE_QUEUE' *is to declare these external
+ APIs. */
+
+#ifndef QUEUE_H
+#define QUEUE_H
+
+#include <stdio.h>
+
+#include "libiberty.h" /* xmalloc */
+#include "gdb_assert.h"
+
+/* Define a new queue implementation. */
+
+#define QUEUE(TYPE) struct queue_ ## TYPE
+#define QUEUE_ELEM(TYPE) struct queue_ele_ ## TYPE
+
+#define DEFINE_QUEUE_P(TYPE) \
+QUEUE_ELEM (TYPE) \
+{ \
+ QUEUE_ELEM (TYPE) *next; \
+ \
+ TYPE data; \
+}; \
+ \
+QUEUE(TYPE) \
+{ \
+ QUEUE_ELEM (TYPE) *head; \
+ QUEUE_ELEM (TYPE) *tail; \
+ void (*free_func) (TYPE); \
+}; \
+ \
+/* Typical enqueue operation. Put V into queue Q. */ \
+ \
+void \
+queue_ ## TYPE ## _enque (QUEUE (TYPE) *q, TYPE v) \
+{ \
+ QUEUE_ELEM (TYPE) *p \
+ = xmalloc (sizeof (QUEUE_ELEM (TYPE))); \
+ \
+ gdb_assert (q != NULL); \
+ p->data = v; \
+ p->next = NULL; \
+ if (q->tail == NULL) \
+ { \
+ q->tail = p; \
+ q->head = p; \
+ } \
+ else \
+ { \
+ q->tail->next = p; \
+ q->tail = p; \
+ } \
+} \
+ \
+/* Typical dequeue operation. Return one element queue Q. Assert \
+ fail if Q is NULL or Q is empty. */ \
+ \
+TYPE \
+queue_ ## TYPE ## _deque (QUEUE (TYPE) *q) \
+{ \
+ QUEUE_ELEM (TYPE) *p; \
+ TYPE v; \
+ \
+ gdb_assert (q != NULL); \
+ p = q->head; \
+ gdb_assert (p != NULL); \
+ \
+ if (q->head == q->tail) \
+ { \
+ q->head = NULL; \
+ q->tail = NULL; \
+ } \
+ else \
+ q->head = q->head->next; \
+ \
+ v = p->data; \
+ \
+ xfree (p); \
+ return v; \
+} \
+ \
+/* Return the element on head, but don't remove it from queue. Assert \
+ fail is Q is NULL or Q is empty. */ \
+ \
+TYPE \
+queue_ ## TYPE ## _peek (QUEUE (TYPE) *q) \
+{ \
+ gdb_assert (q != NULL); \
+ gdb_assert (q->head != NULL); \
+ return q->head->data; \
+} \
+ \
+/* Return true if queue Q is empty. */ \
+ \
+int \
+queue_ ## TYPE ## _is_empty (QUEUE (TYPE) *q) \
+{ \
+ gdb_assert (q != NULL); \
+ return q->head == NULL; \
+} \
+ \
+void \
+queue_ ## TYPE ## _remove_ele (QUEUE (TYPE) *q, \
+ QUEUE_ELEM (TYPE) *p, \
+ QUEUE_ELEM (TYPE) *prev) \
+{ \
+ if (p == q->head || p == q->tail) \
+ { \
+ if (p == q->head) \
+ q->head = p->next; \
+ if (p == q->tail) \
+ q->tail = prev; \
+ } \
+ else \
+ prev->next = p->next; \
+ \
+ xfree (p); \
+} \
+ \
+/* Iterate over elements in the queue Q. If function MATCH returns \
+ true, call function OPERATE_ON_MATCH. If function OPERATE_ON_MATCH \
+ returns true, return with true directly, otherwise, continue to \
+ traverse elements in queue. If none of elements matches, return \
+ false. */ \
+ \
+int \
+queue_ ## TYPE ## _iterate (QUEUE (TYPE) *q, TYPE *v, \
+ int (*match) (TYPE, void *), \
+ int (*operate_on_match) (QUEUE (TYPE) *, \
+ QUEUE_ELEM (TYPE) *, \
+ QUEUE_ELEM (TYPE) **), \
+ void *data) \
+{ \
+ int matches = 0; \
+ \
+ QUEUE_ELEM (TYPE) *p; \
+ QUEUE_ELEM (TYPE) *prev = NULL, *next = NULL; \
+ \
+ gdb_assert (q != NULL); \
+ \
+ for (p = q->head; p != NULL; p = next) \
+ { \
+ next = p->next; \
+ if (match (p->data, data)) \
+ { \
+ matches = 1; \
+ if (v != NULL) \
+ *v = p->data; \
+ \
+ if (operate_on_match (q, p, &prev)) \
+ return 1; \
+ } \
+ else \
+ prev = p; \
+ } \
+ return matches; \
+} \
+ \
+static int \
+queue_ ## TYPE ##_on_match_once (QUEUE (TYPE) *q, QUEUE_ELEM (TYPE) *p, \
+ QUEUE_ELEM (TYPE) **prev) \
+{ \
+ return 1; \
+} \
+ \
+/* Find the first element in queue Q for which function MATCH returns \
+ true. Return true if found, and store the found element in V. \
+ Otherwise return false.. */ \
+ \
+int \
+queue_ ## TYPE ## _find (QUEUE (TYPE) *q, TYPE *v, \
+ int (*match) (TYPE, void *), \
+ void *data) \
+{ \
+ return queue_ ## TYPE ## _iterate (q, v, match, \
+ queue_ ## TYPE ##_on_match_once, data); \
+} \
+ \
+/* Allocate memory for queue. */ \
+ \
+QUEUE (TYPE) * \
+queue_ ## TYPE ## _alloc (void (*free_func) (TYPE)) \
+{ \
+ QUEUE (TYPE) *q; \
+ \
+ q = (QUEUE (TYPE) *) xmalloc (sizeof (QUEUE (TYPE)));\
+ q->head = NULL; \
+ q->tail = NULL; \
+ q->free_func = free_func; \
+ return q; \
+} \
+ \
+/* Length of queue Q. */ \
+ \
+int \
+queue_ ## TYPE ## _length (QUEUE (TYPE) *q) \
+{ \
+ QUEUE_ELEM (TYPE) *p; \
+ int len = 0; \
+ \
+ gdb_assert (q != NULL); \
+ \
+ for (p = q->head; p != NULL; p = p->next) \
+ len++; \
+ \
+ return len; \
+} \
+/* Free memory for queue Q. */ \
+ \
+void \
+queue_ ## TYPE ## _free (QUEUE (TYPE) *q) \
+{ \
+ QUEUE_ELEM (TYPE) *p, *next; \
+ \
+ gdb_assert (q != NULL); \
+ \
+ for (p = q->head; p != NULL; p = next) \
+ { \
+ next = p->next; \
+ if (q->free_func) \
+ q->free_func (p->data); \
+ xfree (p); \
+ } \
+ xfree (q); \
+} \
+
+/* External declarations for these functions. */
+#define DECLARE_QUEUE_P(TYPE) \
+QUEUE (TYPE); \
+QUEUE_ELEM (TYPE); \
+extern void \
+ queue_ ## TYPE ## _enque (QUEUE (TYPE) *q, TYPE v); \
+extern TYPE \
+ queue_ ## TYPE ## _deque (QUEUE (TYPE) *q); \
+extern int queue_ ## TYPE ## _is_empty (QUEUE (TYPE) *q); \
+extern int \
+ queue_ ## TYPE ## _find (QUEUE (TYPE) *, TYPE *v, \
+ int (*match) (TYPE, void *), \
+ void *data); \
+extern QUEUE (TYPE) * \
+ queue_ ## TYPE ## _alloc (void (*free_func) (TYPE)); \
+extern int queue_ ## TYPE ## _length (QUEUE (TYPE) *q); \
+extern TYPE \
+ queue_ ## TYPE ## _peek (QUEUE (TYPE) *q); \
+extern void queue_ ## TYPE ## _free (QUEUE (TYPE) *q); \
+extern int \
+ queue_ ## TYPE ## _iterate (QUEUE (TYPE) *q, TYPE *v, \
+ int (*match) (TYPE, void *), \
+ int (*operate_on_match) (QUEUE (TYPE) *, \
+ QUEUE_ELEM (TYPE) *, \
+ QUEUE_ELEM (TYPE) **), \
+ void *data); \
+extern void queue_ ## TYPE ## _remove_ele (QUEUE (TYPE) *q, \
+ QUEUE_ELEM (TYPE) *p, \
+ QUEUE_ELEM (TYPE) *prev); \
+
+#define QUEUE_enque(TYPE, QUEUE, V) queue_ ## TYPE ## _enque ((QUEUE), (V))
+#define QUEUE_deque(TYPE, QUEUE) queue_ ## TYPE ## _deque (QUEUE)
+#define QUEUE_peek(TYPE, QUEUE) queue_ ## TYPE ## _peek (QUEUE)
+#define QUEUE_is_empty(TYPE, QUEUE) queue_ ## TYPE ## _is_empty (QUEUE)
+#define QUEUE_find(TYPE, QUEUE, V, MATCH, DATA) \
+ queue_ ## TYPE ## _find ((QUEUE), (V), (MATCH), (DATA))
+#define QUEUE_alloc(TYPE, FREE_FUNC) queue_ ## TYPE ## _alloc (FREE_FUNC)
+#define QUEUE_length(TYPE, QUEUE) queue_ ## TYPE ## _length (QUEUE)
+#define QUEUE_free(TYPE, QUEUE) queue_ ## TYPE ## _free (QUEUE)
+#define QUEUE_iterate(TYPE, QUEUE, V, MATCH, OP_ON_MATCH, DATA) \
+ queue_ ## TYPE ## _iterate ((QUEUE), (V), (MATCH), (OP_ON_MATCH), (DATA))
+#define QUEUE_remove_ele(TYPE, QUEUE, P, PREV) \
+ queue_ ## TYPE ## _remove_ele ((QUEUE), (P), (PREV))
+
+#endif /* QUEUE_H */
--
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] new gdb_queue.h in common/.
2012-09-07 2:47 ` Yao Qi
@ 2012-09-10 20:47 ` dje
2012-09-12 14:24 ` Yao Qi
0 siblings, 1 reply; 19+ messages in thread
From: dje @ 2012-09-10 20:47 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
Yao Qi writes:
> On 09/05/2012 05:02 AM, dje@google.com wrote:
> > [...]
> > I like thinner APIs than fatter ones, but in this case I'm not sure
> > this is better (or worth it).
> > Thoughts?
> >
>
> Yeah, I agree. However, 'remove/remove_matching/find' are clear, but
> 'iterate' with different callbacks to achieve the same task are hard to
> read. I post an implementation of _iterate method, and either way is OK
> to me.
They're ok for me to read. It might be more a matter of frequency of use.
> 2012-09-06 Yao Qi <yao@codesourcery.com>
>
> * common/queue.h: New.
> ---
> gdb/common/queue.h | 292 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 292 insertions(+), 0 deletions(-)
> create mode 100644 gdb/common/queue.h
>
> diff --git a/gdb/common/queue.h b/gdb/common/queue.h
> new file mode 100644
> index 0000000..fff6778
> --- /dev/null
> +++ b/gdb/common/queue.h
> @@ -0,0 +1,292 @@
> +/* General queue data structure for GDB, the GNU debugger.
> +
> + Copyright (C) 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/>. */
> +
> +/* These macros implement functions and structs for a general queue. Macro
> + 'DEFINE_QUEUE_TYPE(TYPE)' is to define the new queue type for
> + 'TYPE', and macro 'DECLARE_QUEUE' *is to declare these external
> + APIs. */
> +
> +#ifndef QUEUE_H
> +#define QUEUE_H
> +
> +#include <stdio.h>
> +
> +#include "libiberty.h" /* xmalloc */
> +#include "gdb_assert.h"
> +
> +/* Define a new queue implementation. */
> +
> +#define QUEUE(TYPE) struct queue_ ## TYPE
> +#define QUEUE_ELEM(TYPE) struct queue_ele_ ## TYPE
queue_elem_ ## TYPE
> +/* Return true if queue Q is empty. */ \
> + \
> +int \
> +queue_ ## TYPE ## _is_empty (QUEUE (TYPE) *q) \
> +{ \
> + gdb_assert (q != NULL); \
> + return q->head == NULL; \
> +} \
> + \
> +void \
> +queue_ ## TYPE ## _remove_ele (QUEUE (TYPE) *q, \
_remove_elem
Plus this function needs a comment.
> + QUEUE_ELEM (TYPE) *p, \
> + QUEUE_ELEM (TYPE) *prev) \
> +{ \
> + if (p == q->head || p == q->tail) \
> + { \
> + if (p == q->head) \
> + q->head = p->next; \
> + if (p == q->tail) \
> + q->tail = prev; \
> + } \
> + else \
> + prev->next = p->next; \
> + \
> + xfree (p); \
> +} \
> + \
> +/* Iterate over elements in the queue Q. If function MATCH returns \
> + true, call function OPERATE_ON_MATCH. If function OPERATE_ON_MATCH \
> + returns true, return with true directly, otherwise, continue to \
> + traverse elements in queue. If none of elements matches, return \
> + false. */ \
> + \
> +int \
> +queue_ ## TYPE ## _iterate (QUEUE (TYPE) *q, TYPE *v, \
> + int (*match) (TYPE, void *), \
> + int (*operate_on_match) (QUEUE (TYPE) *, \
> + QUEUE_ELEM (TYPE) *, \
> + QUEUE_ELEM (TYPE) **), \
> + void *data) \
The iterator I had in mind wouldn't pass v, leaving it for the function
that is passed in to record the element found (either in *data directly,
or as part of the struct that data points to).
Also, I had envisioned just passing in one function, say "operate".
Thus:
int
queue_ ## TYPE ## _iterate (QUEUE (TYPE) *q,
int (*operate) (QUEUE (TYPE) *,
QUEUE_ITER (TYPE) *,
QUEUE_ELEM (TYPE) *,
void *),
void *data)
operate would return 0 to terminate the iteration
and 1 to continue the iteration
(akin to hashtab.h:htab_trav - Consistency Is Good!).
QUEUE_ITER abstracts away the implementation details,
and is passed to _remove_elem.
I wasn't thinking of including _find, _remove, _remove_matching
in the API if we have an iterator - let the user write one if
desired for his/her particular type.
I was expecting users to write "operate" and then just call the
_iterate.
If you want _find,_remove,_remove_matching in the API,
it's not that big a deal. You *could* leave _iterate for later then.
I just want to make sure you've thought about it.
> +{ \
> + int matches = 0; \
> + \
> + QUEUE_ELEM (TYPE) *p; \
> + QUEUE_ELEM (TYPE) *prev = NULL, *next = NULL; \
> + \
> + gdb_assert (q != NULL); \
> + \
> + for (p = q->head; p != NULL; p = next) \
> + { \
> + next = p->next; \
> + if (match (p->data, data)) \
> + { \
> + matches = 1; \
> + if (v != NULL) \
> + *v = p->data; \
> + \
> + if (operate_on_match (q, p, &prev)) \
> + return 1; \
> + } \
> + else \
> + prev = p; \
> + } \
> + return matches; \
> +} \
> + \
> +static int \
> +queue_ ## TYPE ##_on_match_once (QUEUE (TYPE) *q, QUEUE_ELEM (TYPE) *p, \
> + QUEUE_ELEM (TYPE) **prev) \
> +{ \
> + return 1; \
> +} \
> + \
> +/* Find the first element in queue Q for which function MATCH returns \
> + true. Return true if found, and store the found element in V. \
> + Otherwise return false.. */ \
> + \
> +int \
> +queue_ ## TYPE ## _find (QUEUE (TYPE) *q, TYPE *v, \
> + int (*match) (TYPE, void *), \
> + void *data) \
> +{ \
> + return queue_ ## TYPE ## _iterate (q, v, match, \
> + queue_ ## TYPE ##_on_match_once, data); \
> +} \
> + \
> +/* Allocate memory for queue. */ \
> + \
> +QUEUE (TYPE) * \
> +queue_ ## TYPE ## _alloc (void (*free_func) (TYPE)) \
> +{ \
> + QUEUE (TYPE) *q; \
> + \
> + q = (QUEUE (TYPE) *) xmalloc (sizeof (QUEUE (TYPE)));\
> + q->head = NULL; \
> + q->tail = NULL; \
> + q->free_func = free_func; \
> + return q; \
> +} \
> + \
> +/* Length of queue Q. */ \
> + \
> +int \
> +queue_ ## TYPE ## _length (QUEUE (TYPE) *q) \
> +{ \
> + QUEUE_ELEM (TYPE) *p; \
> + int len = 0; \
> + \
> + gdb_assert (q != NULL); \
> + \
> + for (p = q->head; p != NULL; p = p->next) \
> + len++; \
> + \
> + return len; \
> +} \
blank line here
> +/* Free memory for queue Q. */ \
> + \
> +void \
> +queue_ ## TYPE ## _free (QUEUE (TYPE) *q) \
> +{ \
> + QUEUE_ELEM (TYPE) *p, *next; \
> + \
> + gdb_assert (q != NULL); \
> + \
> + for (p = q->head; p != NULL; p = next) \
> + { \
> + next = p->next; \
> + if (q->free_func) \
> + q->free_func (p->data); \
> + xfree (p); \
> + } \
> + xfree (q); \
> +} \
> +
> +/* External declarations for these functions. */
> +#define DECLARE_QUEUE_P(TYPE) \
> +QUEUE (TYPE); \
> +QUEUE_ELEM (TYPE); \
> +extern void \
> + queue_ ## TYPE ## _enque (QUEUE (TYPE) *q, TYPE v); \
> +extern TYPE \
> + queue_ ## TYPE ## _deque (QUEUE (TYPE) *q); \
> +extern int queue_ ## TYPE ## _is_empty (QUEUE (TYPE) *q); \
> +extern int \
> + queue_ ## TYPE ## _find (QUEUE (TYPE) *, TYPE *v, \
> + int (*match) (TYPE, void *), \
> + void *data); \
> +extern QUEUE (TYPE) * \
> + queue_ ## TYPE ## _alloc (void (*free_func) (TYPE)); \
> +extern int queue_ ## TYPE ## _length (QUEUE (TYPE) *q); \
> +extern TYPE \
> + queue_ ## TYPE ## _peek (QUEUE (TYPE) *q); \
> +extern void queue_ ## TYPE ## _free (QUEUE (TYPE) *q); \
> +extern int \
> + queue_ ## TYPE ## _iterate (QUEUE (TYPE) *q, TYPE *v, \
> + int (*match) (TYPE, void *), \
> + int (*operate_on_match) (QUEUE (TYPE) *, \
> + QUEUE_ELEM (TYPE) *, \
> + QUEUE_ELEM (TYPE) **), \
> + void *data); \
> +extern void queue_ ## TYPE ## _remove_ele (QUEUE (TYPE) *q, \
> + QUEUE_ELEM (TYPE) *p, \
> + QUEUE_ELEM (TYPE) *prev); \
> +
> +#define QUEUE_enque(TYPE, QUEUE, V) queue_ ## TYPE ## _enque ((QUEUE), (V))
> +#define QUEUE_deque(TYPE, QUEUE) queue_ ## TYPE ## _deque (QUEUE)
> +#define QUEUE_peek(TYPE, QUEUE) queue_ ## TYPE ## _peek (QUEUE)
> +#define QUEUE_is_empty(TYPE, QUEUE) queue_ ## TYPE ## _is_empty (QUEUE)
> +#define QUEUE_find(TYPE, QUEUE, V, MATCH, DATA) \
> + queue_ ## TYPE ## _find ((QUEUE), (V), (MATCH), (DATA))
> +#define QUEUE_alloc(TYPE, FREE_FUNC) queue_ ## TYPE ## _alloc (FREE_FUNC)
> +#define QUEUE_length(TYPE, QUEUE) queue_ ## TYPE ## _length (QUEUE)
> +#define QUEUE_free(TYPE, QUEUE) queue_ ## TYPE ## _free (QUEUE)
> +#define QUEUE_iterate(TYPE, QUEUE, V, MATCH, OP_ON_MATCH, DATA) \
> + queue_ ## TYPE ## _iterate ((QUEUE), (V), (MATCH), (OP_ON_MATCH), (DATA))
> +#define QUEUE_remove_ele(TYPE, QUEUE, P, PREV) \
> + queue_ ## TYPE ## _remove_ele ((QUEUE), (P), (PREV))
> +
> +#endif /* QUEUE_H */
> --
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] new gdb_queue.h in common/.
2012-09-04 21:03 ` dje
2012-09-07 2:47 ` Yao Qi
@ 2012-09-11 16:50 ` Tom Tromey
1 sibling, 0 replies; 19+ messages in thread
From: Tom Tromey @ 2012-09-11 16:50 UTC (permalink / raw)
To: dje; +Cc: Yao Qi, gdb-patches
>>>>> "Doug" == Douglas Evans <dje@google.com> writes:
Doug> Yeah, OTOH this is how "extended" versions of API functions come into being.
Doug> They're a wart in the API so I like to avoid them.
Doug> E.g. consider htab_create_alloc vs htab_create_alloc_ex in the hashtab API.
Doug> Some might not think this is a wart, alas I do, so this is one situation
Doug> where I don't like to lazily add stuff (when I'm aware of it at
Doug> the time ... :-)).
Doug> OTOOH, since we're lazily adding the object version of the API anyway,
Doug> I don't feel too strongly about it here.
I think the difference is that if the code is all in gdb, then a change
can also easily update all uses. This isn't true for hashtab.
Tom
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] new gdb_queue.h in common/.
2012-09-10 20:47 ` dje
@ 2012-09-12 14:24 ` Yao Qi
2012-09-13 15:55 ` dje
0 siblings, 1 reply; 19+ messages in thread
From: Yao Qi @ 2012-09-12 14:24 UTC (permalink / raw)
To: dje; +Cc: gdb-patches
On 09/11/2012 04:47 AM, dje@google.com wrote:
> The iterator I had in mind wouldn't pass v, leaving it for the function
> that is passed in to record the element found (either in *data directly,
> or as part of the struct that data points to).
> Also, I had envisioned just passing in one function, say "operate".
>
> Thus:
>
> int
> queue_ ## TYPE ## _iterate (QUEUE (TYPE) *q,
> int (*operate) (QUEUE (TYPE) *,
> QUEUE_ITER (TYPE) *,
> QUEUE_ELEM (TYPE) *,
> void *),
> void *data)
>
Doug,
I almost followed this design in the new patch, except moving parameter
3 (QUEUE_ELEM (TYPE) *) of '*operate' into QUEUE_ITER (TYPE), so that we
can save one parameter, and don't have to expose details of QUEUE_ELEM.
> operate would return 0 to terminate the iteration
> and 1 to continue the iteration
> (akin to hashtab.h:htab_trav - Consistency Is Good!).
>
> QUEUE_ITER abstracts away the implementation details,
> and is passed to _remove_elem.
>
> I wasn't thinking of including _find, _remove, _remove_matching
> in the API if we have an iterator - let the user write one if
> desired for his/her particular type.
> I was expecting users to write "operate" and then just call the
> _iterate.
Agreed.
>
> If you want _find,_remove,_remove_matching in the API,
> it's not that big a deal. You*could* leave _iterate for later then.
> I just want to make sure you've thought about it.
I really like the iterator, and I'd like to try again to implement
_iterate here. :) The queue is tested with the rest of patches ('general
async notification'), and no regressions.
--
Yao
gdb/
2012-09-12 Yao Qi <yao@codesourcery.com>
Doug Evans <dje@google.com>
* common/queue.h: New.
---
gdb/common/queue.h | 267 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 267 insertions(+), 0 deletions(-)
create mode 100644 gdb/common/queue.h
diff --git a/gdb/common/queue.h b/gdb/common/queue.h
new file mode 100644
index 0000000..47157a3
--- /dev/null
+++ b/gdb/common/queue.h
@@ -0,0 +1,267 @@
+/* General queue data structure for GDB, the GNU debugger.
+
+ Copyright (C) 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/>. */
+
+/* These macros implement functions and structs for a general queue. Macro
+ 'DEFINE_QUEUE_P(TYPE)' is to define the new queue type for 'TYPE', and
+ macro 'DECLARE_QUEUE_P' is to declare these external APIs. */
+
+#ifndef QUEUE_H
+#define QUEUE_H
+
+#include <stdio.h>
+
+#include "libiberty.h" /* xmalloc */
+#include "gdb_assert.h"
+
+/* Define a new queue implementation. */
+
+#define QUEUE(TYPE) struct queue_ ## TYPE
+#define QUEUE_ELEM(TYPE) struct queue_elem_ ## TYPE
+#define QUEUE_ITER(TYPE) struct queue_iter_ ## TYPE
+
+#define DEFINE_QUEUE_P(TYPE) \
+QUEUE_ELEM (TYPE) \
+{ \
+ QUEUE_ELEM (TYPE) *next; \
+ \
+ TYPE data; \
+}; \
+ \
+/* Queue iterator. */ \
+QUEUE_ITER (TYPE) \
+{ \
+ /* The current element during traverse. */ \
+ QUEUE_ELEM (TYPE) *p; \
+ /* The previous element of P. */ \
+ QUEUE_ELEM (TYPE) *prev; \
+}; \
+ \
+QUEUE(TYPE) \
+{ \
+ QUEUE_ELEM (TYPE) *head; \
+ QUEUE_ELEM (TYPE) *tail; \
+ void (*free_func) (TYPE); \
+}; \
+ \
+/* Typical enqueue operation. Put V into queue Q. */ \
+ \
+void \
+queue_ ## TYPE ## _enque (QUEUE (TYPE) *q, TYPE v) \
+{ \
+ QUEUE_ELEM (TYPE) *p \
+ = xmalloc (sizeof (QUEUE_ELEM (TYPE))); \
+ \
+ gdb_assert (q != NULL); \
+ p->data = v; \
+ p->next = NULL; \
+ if (q->tail == NULL) \
+ { \
+ q->tail = p; \
+ q->head = p; \
+ } \
+ else \
+ { \
+ q->tail->next = p; \
+ q->tail = p; \
+ } \
+} \
+ \
+/* Typical dequeue operation. Return one element queue Q. Assert \
+ fail if Q is NULL or Q is empty. */ \
+ \
+TYPE \
+queue_ ## TYPE ## _deque (QUEUE (TYPE) *q) \
+{ \
+ QUEUE_ELEM (TYPE) *p; \
+ TYPE v; \
+ \
+ gdb_assert (q != NULL); \
+ p = q->head; \
+ gdb_assert (p != NULL); \
+ \
+ if (q->head == q->tail) \
+ { \
+ q->head = NULL; \
+ q->tail = NULL; \
+ } \
+ else \
+ q->head = q->head->next; \
+ \
+ v = p->data; \
+ \
+ xfree (p); \
+ return v; \
+} \
+ \
+/* Return the element on head, but don't remove it from queue. Assert \
+ fail is Q is NULL or Q is empty. */ \
+ \
+TYPE \
+queue_ ## TYPE ## _peek (QUEUE (TYPE) *q) \
+{ \
+ gdb_assert (q != NULL); \
+ gdb_assert (q->head != NULL); \
+ return q->head->data; \
+} \
+ \
+/* Return true if queue Q is empty. */ \
+ \
+int \
+queue_ ## TYPE ## _is_empty (QUEUE (TYPE) *q) \
+{ \
+ gdb_assert (q != NULL); \
+ return q->head == NULL; \
+} \
+ \
+/* Remove the element per the state of iterator ITER from queue Q. */ \
+ \
+void \
+queue_ ## TYPE ## _remove_elem (QUEUE (TYPE) *q, \
+ QUEUE_ITER (TYPE) *iter) \
+{ \
+ if (iter->p == q->head || iter->p == q->tail) \
+ { \
+ if (iter->p == q->head) \
+ q->head = iter->p->next; \
+ if (iter->p == q->tail) \
+ q->tail = iter->prev; \
+ } \
+ else \
+ iter->prev->next = iter->p->next; \
+ \
+ xfree (iter->p); \
+} \
+ \
+/* Iterate over elements in the queue Q and call function OPERATE on \
+ each element. OPERATE would return false to terminate the \
+ iteration and true to continue the iteration. Return false if \
+ iteration is terminated by function OPERATE, otherwise return \
+ true. */ \
+ \
+int \
+queue_ ## TYPE ## _iterate (QUEUE (TYPE) *q, \
+ int (*operate) (QUEUE (TYPE) *, \
+ QUEUE_ITER (TYPE) *, \
+ TYPE, \
+ void *), \
+ void *data) \
+{ \
+ QUEUE_ELEM (TYPE) *next = NULL; \
+ QUEUE_ITER (TYPE) iter = { NULL, NULL }; \
+ \
+ gdb_assert (q != NULL); \
+ \
+ for (iter.p = q->head; iter.p != NULL; iter.p = next) \
+ { \
+ next = iter.p->next; \
+ if (!operate (q, &iter, iter.p->data, data)) \
+ return 0; \
+ } \
+ return 1; \
+} \
+ \
+/* Allocate memory for queue. */ \
+ \
+QUEUE (TYPE) * \
+queue_ ## TYPE ## _alloc (void (*free_func) (TYPE)) \
+{ \
+ QUEUE (TYPE) *q; \
+ \
+ q = (QUEUE (TYPE) *) xmalloc (sizeof (QUEUE (TYPE))); \
+ q->head = NULL; \
+ q->tail = NULL; \
+ q->free_func = free_func; \
+ return q; \
+} \
+ \
+/* Length of queue Q. */ \
+ \
+int \
+queue_ ## TYPE ## _length (QUEUE (TYPE) *q) \
+{ \
+ QUEUE_ELEM (TYPE) *p; \
+ int len = 0; \
+ \
+ gdb_assert (q != NULL); \
+ \
+ for (p = q->head; p != NULL; p = p->next) \
+ len++; \
+ \
+ return len; \
+} \
+ \
+/* Free memory for queue Q. */ \
+ \
+void \
+queue_ ## TYPE ## _free (QUEUE (TYPE) *q) \
+{ \
+ QUEUE_ELEM (TYPE) *p, *next; \
+ \
+ gdb_assert (q != NULL); \
+ \
+ for (p = q->head; p != NULL; p = next) \
+ { \
+ next = p->next; \
+ if (q->free_func) \
+ q->free_func (p->data); \
+ xfree (p); \
+ } \
+ xfree (q); \
+} \
+
+/* External declarations for these functions. */
+#define DECLARE_QUEUE_P(TYPE) \
+QUEUE (TYPE); \
+QUEUE_ELEM (TYPE); \
+QUEUE_ITER (TYPE); \
+extern void \
+ queue_ ## TYPE ## _enque (QUEUE (TYPE) *q, TYPE v); \
+extern TYPE \
+ queue_ ## TYPE ## _deque (QUEUE (TYPE) *q); \
+extern int queue_ ## TYPE ## _is_empty (QUEUE (TYPE) *q); \
+extern QUEUE (TYPE) * \
+ queue_ ## TYPE ## _alloc (void (*free_func) (TYPE)); \
+extern int queue_ ## TYPE ## _length (QUEUE (TYPE) *q); \
+extern TYPE \
+ queue_ ## TYPE ## _peek (QUEUE (TYPE) *q); \
+extern void queue_ ## TYPE ## _free (QUEUE (TYPE) *q); \
+extern int \
+ queue_ ## TYPE ## _iterate (QUEUE (TYPE) *q, \
+ int (*operate) (QUEUE (TYPE) *, \
+ QUEUE_ITER (TYPE) *, \
+ TYPE, \
+ void *), \
+ void *); \
+extern void \
+ queue_ ## TYPE ## _remove_elem (QUEUE (TYPE) *q, \
+ QUEUE_ITER (TYPE) *iter); \
+
+#define QUEUE_enque(TYPE, QUEUE, V) queue_ ## TYPE ## _enque ((QUEUE), (V))
+#define QUEUE_deque(TYPE, QUEUE) queue_ ## TYPE ## _deque (QUEUE)
+#define QUEUE_peek(TYPE, QUEUE) queue_ ## TYPE ## _peek (QUEUE)
+#define QUEUE_is_empty(TYPE, QUEUE) queue_ ## TYPE ## _is_empty (QUEUE)
+#define QUEUE_alloc(TYPE, FREE_FUNC) queue_ ## TYPE ## _alloc (FREE_FUNC)
+#define QUEUE_length(TYPE, QUEUE) queue_ ## TYPE ## _length (QUEUE)
+#define QUEUE_free(TYPE, QUEUE) queue_ ## TYPE ## _free (QUEUE)
+#define QUEUE_iterate(TYPE, QUEUE, OPERATE, PARAM) \
+ queue_ ## TYPE ## _iterate ((QUEUE), (OPERATE), (PARAM))
+#define QUEUE_remove_elem(TYPE, QUEUE, ITER) \
+ queue_ ## TYPE ## _remove_elem ((QUEUE), (ITER))
+
+#endif /* QUEUE_H */
--
1.7.7.6
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] new gdb_queue.h in common/.
2012-09-12 14:24 ` Yao Qi
@ 2012-09-13 15:55 ` dje
2012-09-14 2:38 ` Yao Qi
0 siblings, 1 reply; 19+ messages in thread
From: dje @ 2012-09-13 15:55 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
Yao Qi writes:
> On 09/11/2012 04:47 AM, dje@google.com wrote:
> > The iterator I had in mind wouldn't pass v, leaving it for the function
> > that is passed in to record the element found (either in *data directly,
> > or as part of the struct that data points to).
> > Also, I had envisioned just passing in one function, say "operate".
> >
> > Thus:
> >
> > int
> > queue_ ## TYPE ## _iterate (QUEUE (TYPE) *q,
> > int (*operate) (QUEUE (TYPE) *,
> > QUEUE_ITER (TYPE) *,
> > QUEUE_ELEM (TYPE) *,
> > void *),
> > void *data)
> >
>
> Doug,
> I almost followed this design in the new patch, except moving parameter
> 3 (QUEUE_ELEM (TYPE) *) of '*operate' into QUEUE_ITER (TYPE), so that we
> can save one parameter, and don't have to expose details of QUEUE_ELEM.
Oops, I didn't intend to expose the details of QUEUE_ELEM.
Pretend I wrote TYPE instead. :-)
[The intent being to provide operate with the element,
but not expose the details of QUEUE_ITER.]
> > operate would return 0 to terminate the iteration
> > and 1 to continue the iteration
> > (akin to hashtab.h:htab_trav - Consistency Is Good!).
> >
> > QUEUE_ITER abstracts away the implementation details,
> > and is passed to _remove_elem.
> >
> > I wasn't thinking of including _find, _remove, _remove_matching
> > in the API if we have an iterator - let the user write one if
> > desired for his/her particular type.
> > I was expecting users to write "operate" and then just call the
> > _iterate.
>
> Agreed.
>
> >
> > If you want _find,_remove,_remove_matching in the API,
> > it's not that big a deal. You*could* leave _iterate for later then.
> > I just want to make sure you've thought about it.
>
> I really like the iterator, and I'd like to try again to implement
> _iterate here. :) The queue is tested with the rest of patches ('general
> async notification'), and no regressions.
>
> --
> Yao
>
> gdb/
>
> 2012-09-12 Yao Qi <yao@codesourcery.com>
> Doug Evans <dje@google.com>
>
> * common/queue.h: New.
> ---
> gdb/common/queue.h | 267 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 267 insertions(+), 0 deletions(-)
> create mode 100644 gdb/common/queue.h
>
> diff --git a/gdb/common/queue.h b/gdb/common/queue.h
> new file mode 100644
> index 0000000..47157a3
> --- /dev/null
> +++ b/gdb/common/queue.h
> @@ -0,0 +1,267 @@
> +/* General queue data structure for GDB, the GNU debugger.
> +
> + Copyright (C) 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/>. */
> +
> +/* These macros implement functions and structs for a general queue. Macro
> + 'DEFINE_QUEUE_P(TYPE)' is to define the new queue type for 'TYPE', and
> + macro 'DECLARE_QUEUE_P' is to declare these external APIs. */
> +
> +#ifndef QUEUE_H
> +#define QUEUE_H
> +
> +#include <stdio.h>
> +
> +#include "libiberty.h" /* xmalloc */
> +#include "gdb_assert.h"
> +
> +/* Define a new queue implementation. */
> +
> +#define QUEUE(TYPE) struct queue_ ## TYPE
> +#define QUEUE_ELEM(TYPE) struct queue_elem_ ## TYPE
> +#define QUEUE_ITER(TYPE) struct queue_iter_ ## TYPE
> +
> +#define DEFINE_QUEUE_P(TYPE) \
> +QUEUE_ELEM (TYPE) \
> +{ \
> + QUEUE_ELEM (TYPE) *next; \
> + \
> + TYPE data; \
> +}; \
> + \
> +/* Queue iterator. */ \
> +QUEUE_ITER (TYPE) \
> +{ \
> + /* The current element during traverse. */ \
> + QUEUE_ELEM (TYPE) *p; \
> + /* The previous element of P. */ \
> + QUEUE_ELEM (TYPE) *prev; \
> +}; \
> + \
> +QUEUE(TYPE) \
> +{ \
> + QUEUE_ELEM (TYPE) *head; \
> + QUEUE_ELEM (TYPE) *tail; \
> + void (*free_func) (TYPE); \
> +}; \
> + \
> +/* Typical enqueue operation. Put V into queue Q. */ \
> + \
> +void \
> +queue_ ## TYPE ## _enque (QUEUE (TYPE) *q, TYPE v) \
> +{ \
> + QUEUE_ELEM (TYPE) *p \
> + = xmalloc (sizeof (QUEUE_ELEM (TYPE))); \
> + \
> + gdb_assert (q != NULL); \
> + p->data = v; \
> + p->next = NULL; \
> + if (q->tail == NULL) \
> + { \
> + q->tail = p; \
> + q->head = p; \
> + } \
> + else \
> + { \
> + q->tail->next = p; \
> + q->tail = p; \
> + } \
> +} \
> + \
> +/* Typical dequeue operation. Return one element queue Q. Assert \
> + fail if Q is NULL or Q is empty. */ \
> + \
> +TYPE \
> +queue_ ## TYPE ## _deque (QUEUE (TYPE) *q) \
> +{ \
> + QUEUE_ELEM (TYPE) *p; \
> + TYPE v; \
> + \
> + gdb_assert (q != NULL); \
> + p = q->head; \
> + gdb_assert (p != NULL); \
> + \
> + if (q->head == q->tail) \
> + { \
> + q->head = NULL; \
> + q->tail = NULL; \
> + } \
> + else \
> + q->head = q->head->next; \
> + \
> + v = p->data; \
> + \
> + xfree (p); \
> + return v; \
> +} \
> + \
> +/* Return the element on head, but don't remove it from queue. Assert \
> + fail is Q is NULL or Q is empty. */ \
> + \
> +TYPE \
> +queue_ ## TYPE ## _peek (QUEUE (TYPE) *q) \
> +{ \
> + gdb_assert (q != NULL); \
> + gdb_assert (q->head != NULL); \
> + return q->head->data; \
> +} \
> + \
> +/* Return true if queue Q is empty. */ \
> + \
> +int \
> +queue_ ## TYPE ## _is_empty (QUEUE (TYPE) *q) \
> +{ \
> + gdb_assert (q != NULL); \
> + return q->head == NULL; \
> +} \
> + \
> +/* Remove the element per the state of iterator ITER from queue Q. */ \
> + \
> +void \
> +queue_ ## TYPE ## _remove_elem (QUEUE (TYPE) *q, \
> + QUEUE_ITER (TYPE) *iter) \
> +{ \
gdb_assert (q != NULL);
gdb_assert (iter != NULL && iter->p != NULL);
> + if (iter->p == q->head || iter->p == q->tail) \
> + { \
> + if (iter->p == q->head) \
> + q->head = iter->p->next; \
> + if (iter->p == q->tail) \
> + q->tail = iter->prev; \
> + } \
> + else \
> + iter->prev->next = iter->p->next; \
> + \
Need to check if q->free_func != NULL and call it on p->data.
> + xfree (iter->p); \
> +} \
> + \
> +/* Iterate over elements in the queue Q and call function OPERATE on \
> + each element. OPERATE would return false to terminate the \
> + iteration and true to continue the iteration. Return false if \
> + iteration is terminated by function OPERATE, otherwise return \
> + true. */ \
> + \
> +int \
> +queue_ ## TYPE ## _iterate (QUEUE (TYPE) *q, \
> + int (*operate) (QUEUE (TYPE) *, \
> + QUEUE_ITER (TYPE) *, \
> + TYPE, \
> + void *), \
IWBN to have a typedef of operate.
typedef int queue_ ## TYPE ## _operate_func (QUEUE (TYPE) *,
QUEUE_ITER (TYPE) *,
TYPE,
void *);
or some such.
> + void *data) \
> +{ \
> + QUEUE_ELEM (TYPE) *next = NULL; \
> + QUEUE_ITER (TYPE) iter = { NULL, NULL }; \
> + \
> + gdb_assert (q != NULL); \
> + \
> + for (iter.p = q->head; iter.p != NULL; iter.p = next) \
Need to update iter.prev somewhere here
(and handle the case that iter.p has been deleted).
> + { \
> + next = iter.p->next; \
> + if (!operate (q, &iter, iter.p->data, data)) \
> + return 0; \
> + } \
> + return 1; \
> +} \
> + \
> +/* Allocate memory for queue. */ \
> + \
> +QUEUE (TYPE) * \
> +queue_ ## TYPE ## _alloc (void (*free_func) (TYPE)) \
> +{ \
> + QUEUE (TYPE) *q; \
> + \
> + q = (QUEUE (TYPE) *) xmalloc (sizeof (QUEUE (TYPE))); \
> + q->head = NULL; \
> + q->tail = NULL; \
> + q->free_func = free_func; \
> + return q; \
> +} \
> + \
> +/* Length of queue Q. */ \
> + \
> +int \
> +queue_ ## TYPE ## _length (QUEUE (TYPE) *q) \
> +{ \
> + QUEUE_ELEM (TYPE) *p; \
> + int len = 0; \
> + \
> + gdb_assert (q != NULL); \
> + \
> + for (p = q->head; p != NULL; p = p->next) \
> + len++; \
> + \
> + return len; \
> +} \
> + \
> +/* Free memory for queue Q. */ \
> + \
> +void \
> +queue_ ## TYPE ## _free (QUEUE (TYPE) *q) \
> +{ \
> + QUEUE_ELEM (TYPE) *p, *next; \
> + \
> + gdb_assert (q != NULL); \
> + \
> + for (p = q->head; p != NULL; p = next) \
> + { \
> + next = p->next; \
> + if (q->free_func) \
> + q->free_func (p->data); \
> + xfree (p); \
> + } \
> + xfree (q); \
> +} \
> +
> +/* External declarations for these functions. */
> +#define DECLARE_QUEUE_P(TYPE) \
> +QUEUE (TYPE); \
> +QUEUE_ELEM (TYPE); \
> +QUEUE_ITER (TYPE); \
> +extern void \
> + queue_ ## TYPE ## _enque (QUEUE (TYPE) *q, TYPE v); \
> +extern TYPE \
> + queue_ ## TYPE ## _deque (QUEUE (TYPE) *q); \
> +extern int queue_ ## TYPE ## _is_empty (QUEUE (TYPE) *q); \
> +extern QUEUE (TYPE) * \
> + queue_ ## TYPE ## _alloc (void (*free_func) (TYPE)); \
> +extern int queue_ ## TYPE ## _length (QUEUE (TYPE) *q); \
> +extern TYPE \
> + queue_ ## TYPE ## _peek (QUEUE (TYPE) *q); \
> +extern void queue_ ## TYPE ## _free (QUEUE (TYPE) *q); \
> +extern int \
> + queue_ ## TYPE ## _iterate (QUEUE (TYPE) *q, \
> + int (*operate) (QUEUE (TYPE) *, \
> + QUEUE_ITER (TYPE) *, \
> + TYPE, \
> + void *), \
> + void *); \
> +extern void \
> + queue_ ## TYPE ## _remove_elem (QUEUE (TYPE) *q, \
> + QUEUE_ITER (TYPE) *iter); \
> +
> +#define QUEUE_enque(TYPE, QUEUE, V) queue_ ## TYPE ## _enque ((QUEUE), (V))
> +#define QUEUE_deque(TYPE, QUEUE) queue_ ## TYPE ## _deque (QUEUE)
> +#define QUEUE_peek(TYPE, QUEUE) queue_ ## TYPE ## _peek (QUEUE)
> +#define QUEUE_is_empty(TYPE, QUEUE) queue_ ## TYPE ## _is_empty (QUEUE)
> +#define QUEUE_alloc(TYPE, FREE_FUNC) queue_ ## TYPE ## _alloc (FREE_FUNC)
> +#define QUEUE_length(TYPE, QUEUE) queue_ ## TYPE ## _length (QUEUE)
> +#define QUEUE_free(TYPE, QUEUE) queue_ ## TYPE ## _free (QUEUE)
> +#define QUEUE_iterate(TYPE, QUEUE, OPERATE, PARAM) \
> + queue_ ## TYPE ## _iterate ((QUEUE), (OPERATE), (PARAM))
> +#define QUEUE_remove_elem(TYPE, QUEUE, ITER) \
> + queue_ ## TYPE ## _remove_elem ((QUEUE), (ITER))
> +
> +#endif /* QUEUE_H */
> --
> 1.7.7.6
I'm happy with the above nits fixed.
Thanks!
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] new gdb_queue.h in common/.
2012-09-13 15:55 ` dje
@ 2012-09-14 2:38 ` Yao Qi
0 siblings, 0 replies; 19+ messages in thread
From: Yao Qi @ 2012-09-14 2:38 UTC (permalink / raw)
To: dje; +Cc: gdb-patches
On 09/13/2012 11:54 PM, dje@google.com wrote:
> > +void \
> > +queue_ ## TYPE ## _remove_elem (QUEUE (TYPE) *q, \
> > + QUEUE_ITER (TYPE) *iter) \
> > +{ \
>
> gdb_assert (q != NULL);
> gdb_assert (iter != NULL && iter->p != NULL);
>
> > + if (iter->p == q->head || iter->p == q->tail) \
> > + { \
> > + if (iter->p == q->head) \
> > + q->head = iter->p->next; \
> > + if (iter->p == q->tail) \
> > + q->tail = iter->prev; \
> > + } \
> > + else \
> > + iter->prev->next = iter->p->next; \
> > + \
>
> Need to check if q->free_func != NULL and call it on p->data.
We don't have to delete p->data here, because we just remove element,
and p->data may be used by user later, like function _remove, for
example. User has to take care of deleting p->data in his 'operate'
function.
> I'm happy with the above nits fixed.
Doug, thanks a lot for your patient review. Looks queue.h is in shape,
and I'll post a V2 of this series.
--
Yao
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2012-09-14 2:38 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-08-24 2:26 [RCF 0/4] A general notification in GDB RSP Yao Qi
2012-08-24 2:26 ` [PATCH 4/4] new notification "TEST" Yao Qi
2012-08-24 2:26 ` [PATCH 1/4] new gdb_queue.h in common/ Yao Qi
2012-08-24 18:44 ` dje
2012-08-24 18:49 ` Doug Evans
2012-08-29 9:42 ` Yao Qi
2012-09-04 21:03 ` dje
2012-09-07 2:47 ` Yao Qi
2012-09-10 20:47 ` dje
2012-09-12 14:24 ` Yao Qi
2012-09-13 15:55 ` dje
2012-09-14 2:38 ` Yao Qi
2012-09-11 16:50 ` Tom Tromey
2012-08-24 2:26 ` [PATCH 3/4] de-couple %Stop from notification: gdbserver Yao Qi
2012-08-24 2:26 ` [PATCH 2/4] de-couple %Stop from notification: gdb Yao Qi
2012-08-24 16:52 ` Yao Qi
2012-08-24 19:00 ` dje
2012-08-30 6:40 ` Yao Qi
2012-08-24 17:53 ` [RCF 0/4] A general notification in GDB RSP Pedro Alves
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).