From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25836 invoked by alias); 25 Sep 2013 16:12:58 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 25822 invoked by uid 89); 25 Sep 2013 16:12:57 -0000 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 25 Sep 2013 16:12:57 +0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r8PGCran002903 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 25 Sep 2013 12:12:54 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r8PGCQSJ015080; Wed, 25 Sep 2013 12:12:27 -0400 Message-ID: <52430B69.4090301@redhat.com> Date: Wed, 25 Sep 2013 16:12:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: Yao Qi CC: gdb-patches@sourceware.org Subject: Re: [PATCH 1/6] Move notif_queue to remote_state References: <1376877311-4135-1-git-send-email-yao@codesourcery.com> <1376877311-4135-2-git-send-email-yao@codesourcery.com> In-Reply-To: <1376877311-4135-2-git-send-email-yao@codesourcery.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-09/txt/msg00899.txt.bz2 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' > 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) : 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