This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 1/6] Move notif_queue to remote_state
- From: Pedro Alves <palves at redhat dot com>
- To: Yao Qi <yao at codesourcery dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Wed, 25 Sep 2013 17:12:25 +0100
- Subject: Re: [PATCH 1/6] Move notif_queue to remote_state
- Authentication-results: sourceware.org; auth=none
- References: <1376877311-4135-1-git-send-email-yao at codesourcery dot com> <1376877311-4135-2-git-send-email-yao at codesourcery dot com>
On 08/19/2013 02:55 AM, Yao Qi wrote:
> This patch is to move global 'notif_queue' to 'struct remote_state', as
> a follow up to Tom's patch series '[PATCH 00/16] clean up remote.c state'
> <http://sourceware.org/ml/gdb-patches/2013-06/msg00605.html>
This all looks like good forward progress. Thanks!
Though a couple things weren't migrated to the new struct, and I'm not
sure whether that was an oversight, given you didn't specify a plan
of action for those upfront. Comments below.
>
> gdb:
>
> * remote-notif.c (DECLARE_QUEUE_P): Remove.
> (notif_queue): Remove.
> (remote_notif_process): Add one parameter 'notif_queue'.
> Update comments. Callers update.
> (remote_notif_register_async_event_handler): Add parameter
> 'notif_queue'. Pass 'notif_queue' to
> create_async_event_handler.
> (handle_notification): Add parameter 'notif_queue'. Update
> comments. Callers update.
> (remote_notif_state): New function.
> (_initialize_notif): Remove code to allocate queue.
> * remote-notif.h (DECLARE_QUEUE_P): Moved from remote-notif.c.
> (struct remote_notif_state): New.
> (handle_notification): Update declaration.
> (remote_notif_register_async_event_handler): Likewise.
> (remote_notif_process): Likewise.
> (remote_notif_state): Declare.
> * remote.c (struct remote_state) <notif_state>: New field.
> (remote_open_1): Initialize rs->notif_state and pass it to
> remote_notif_register_async_event_handler.
> ---
> gdb/remote-notif.c | 40 ++++++++++++++++++++++++----------------
> gdb/remote-notif.h | 20 +++++++++++++++++---
> gdb/remote.c | 12 ++++++++----
> 3 files changed, 49 insertions(+), 23 deletions(-)
>
> diff --git a/gdb/remote-notif.c b/gdb/remote-notif.c
> index c4bfd8d..8b69f2f 100644
> --- a/gdb/remote-notif.c
> +++ b/gdb/remote-notif.c
> @@ -91,21 +91,19 @@ remote_notif_parse (struct notif_client *nc, char *buf)
> return event;
> }
>
> -DECLARE_QUEUE_P (notif_client_p);
> DEFINE_QUEUE_P (notif_client_p);
>
> -static QUEUE(notif_client_p) *notif_queue;
> -
> -/* Process notifications one by one. EXCEPT is not expected in
> - the queue. */
> +/* Process notifications in NOTIF_QUEUE one by one. EXCEPT is not expected
> + in the queue. */
Looks like stale comment from earlier revision:
s/NOTIF_QUEUE/in STATE's notification queue/ or something like that.
>
> void
> -remote_notif_process (struct notif_client *except)
> +remote_notif_process (struct remote_notif_state *state,
> + struct notif_client *except)
> {
> - while (!QUEUE_is_empty (notif_client_p, notif_queue))
> + while (!QUEUE_is_empty (notif_client_p, state->notif_queue))
> {
> struct notif_client *nc = QUEUE_deque (notif_client_p,
> - notif_queue);
> + state->notif_queue);
>
> gdb_assert (nc != except);
>
> @@ -118,7 +116,7 @@ static void
> remote_async_get_pending_events_handler (gdb_client_data data)
> {
> gdb_assert (non_stop);
> - remote_notif_process (NULL);
> + remote_notif_process (data, NULL);
> }
>
> /* Asynchronous signal handle registered as event loop source for when
> @@ -131,11 +129,11 @@ static struct async_event_handler *remote_async_get_pending_events_token;
> /* Register async_event_handler for notification. */
>
> void
> -remote_notif_register_async_event_handler (void)
> +remote_notif_register_async_event_handler (struct remote_notif_state *state)
> {
> remote_async_get_pending_events_token
> = create_async_event_handler (remote_async_get_pending_events_handler,
> - NULL);
> + state);
As soon as we call this a second time, we overwrite the previous token.
What's the plan here? Shouldn't we move the token to the remote_notif_state too?
> }
>
> /* Unregister async_event_handler for notification. */
> @@ -147,10 +145,11 @@ remote_notif_unregister_async_event_handler (void)
> delete_async_event_handler (&remote_async_get_pending_events_token);
> }
>
> -/* Remote notification handler. */
> +/* Remote notification handler. Parse BUF, queue notification and
> + update STATE. */
>
> void
> -handle_notification (char *buf)
> +handle_notification (struct remote_notif_state *state, char *buf)
> {
> struct notif_client *nc = NULL;
> int i;
> @@ -189,7 +188,7 @@ handle_notification (char *buf)
>
> /* Notify the event loop there's a stop reply to acknowledge
> and that there may be more events to fetch. */
> - QUEUE_enque (notif_client_p, notif_queue, nc);
> + QUEUE_enque (notif_client_p, state->notif_queue, nc);
> if (non_stop)
> {
> /* In non-stop, We mark REMOTE_ASYNC_GET_PENDING_EVENTS_TOKEN
> @@ -262,14 +261,23 @@ notif_xfree (struct notif_client *notif)
> xfree (notif);
> }
>
> +/* Return an allocated remote_notif_state. */
> +
> +struct remote_notif_state *
> +remote_notif_state (void)
(I'd suggest naming it new_remote_notif_state, avoiding the naming
conflict when compiling with a C++ compiler.)
> +{
> + struct remote_notif_state *notif_state = xmalloc (sizeof (*notif_state));
> +
> + notif_state->notif_queue = QUEUE_alloc (notif_client_p, notif_xfree);
> + return notif_state;
> +}
> +
> /* -Wmissing-prototypes */
> extern initialize_file_ftype _initialize_notif;
>
> void
> _initialize_notif (void)
> {
> - notif_queue = QUEUE_alloc (notif_client_p, notif_xfree);
> -
> add_setshow_boolean_cmd ("notification", no_class, ¬if_debug,
> _("\
> Set debugging of async remote notification."), _("\
> diff --git a/gdb/remote-notif.h b/gdb/remote-notif.h
> index da4fdea..05d2613 100644
> --- a/gdb/remote-notif.h
> +++ b/gdb/remote-notif.h
> @@ -68,16 +68,30 @@ typedef struct notif_client
> struct notif_event *pending_event;
> } *notif_client_p;
>
> +DECLARE_QUEUE_P (notif_client_p);
> +
> +/* State on remote async notification. */
> +
> +struct remote_notif_state
> +{
> + /* Notification queue. */
> + QUEUE(notif_client_p) *notif_queue;
> +};
> +
> void remote_notif_ack (struct notif_client *nc, char *buf);
> struct notif_event *remote_notif_parse (struct notif_client *nc,
> char *buf);
>
> -void handle_notification (char *buf);
> +void handle_notification (struct remote_notif_state *notif_state,
> + char *buf);
>
> -void remote_notif_register_async_event_handler (void);
> +void remote_notif_register_async_event_handler (struct remote_notif_state *);
> void remote_notif_unregister_async_event_handler (void);
>
> -void remote_notif_process (struct notif_client *except);
> +void remote_notif_process (struct remote_notif_state *state,
> + struct notif_client *except);
> +struct remote_notif_state *remote_notif_state (void);
> +
> extern struct notif_client notif_client_stop;
>
> extern int notif_debug;
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 5028451..e50623a 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -425,6 +425,9 @@ struct remote_state
> threadref echo_nextthread;
> threadref nextthread;
> threadref resultthreadlist[MAXTHREADLISTRESULTS];
> +
> + /* The state of remote notification. */
> + struct remote_notif_state *notif_state;
> };
>
> /* Private data that we'll store in (struct thread_info)->private. */
> @@ -4337,7 +4340,8 @@ remote_open_1 (char *name, int from_tty,
> remote_async_inferior_event_token
> = create_async_event_handler (remote_async_inferior_event_handler,
> NULL);
> - remote_notif_register_async_event_handler ();
> + rs->notif_state = remote_notif_state ();
> + remote_notif_register_async_event_handler (rs->notif_state);
>
> /* Reset the target state; these things will be queried either by
> remote_query_supported or as they are needed. */
> @@ -4931,7 +4935,7 @@ remote_resume (struct target_ops *ops,
> before resuming inferior, because inferior was stopped and no RSP
> traffic at that moment. */
> if (!non_stop)
> - remote_notif_process (¬if_client_stop);
> + remote_notif_process (rs->notif_state, ¬if_client_stop);
This means there's only one notif_client_stop.pending_event for all
rs's. That doesn't look right. If then intend was to still have only
one global pending_event for all connections, then we'd need to also
record which remote_state->notif_state does the pending event below to?
>
> rs->last_sent_signal = siggnal;
> rs->last_sent_step = step;
> @@ -7395,7 +7399,7 @@ putpkt_binary (char *buf, int cnt)
> str);
> do_cleanups (old_chain);
> }
> - handle_notification (rs->buf);
> + handle_notification (rs->notif_state, rs->buf);
> /* We're in sync now, rewait for the ack. */
> tcount = 0;
> }
> @@ -7781,7 +7785,7 @@ getpkt_or_notif_sane_1 (char **buf, long *sizeof_buf, int forever,
> if (is_notif != NULL)
> *is_notif = 1;
>
> - handle_notification (*buf);
> + handle_notification (rs->notif_state, *buf);
>
> /* Notifications require no acknowledgement. */
>
>
--
Pedro Alves