Re: [PATCH 1/6] Move notif_queue to remote_state

[Pedro Alves <palves@redhat.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, &notif_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 (&notif_client_stop);
> +    remote_notif_process (rs->notif_state, &notif_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