Re: [PATCH 1/4] gdb: make async event handlers clear themselves

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2020-11-30 11:52:48 -0500]:

> The `ready` flag of async event handlers is cleared by the async event
> handler system right before invoking the associated callback, in
> check_async_event_handlers.
> 
> This is not ideal with how the infrun subsystem consumes events: all
> targets' async event handler callbacks essentially just invoke
> `inferior_event_handler`, which eventually calls `fetch_inferior_event`
> and `do_target_wait`.  `do_target_wait` picks an inferior at random,
> and thus a target at random (it could be the target whose `ready` flag
> was cleared, or not), and pulls one event from it.
> 
> So it's possible that:
> 
> - the async event handler for a target A is called
> - we end up consuming an event for target B
> - all threads of target B are stopped, target_async(0) is called on it,
>   so its async event handler is cleared (e.g.
>   record_btrace_target::async)
> 
> As a result, target A still has events to report while its async event
> handler is left unmarked, so these events are not consumed.  To counter
> this, at the end of their async event handler callbacks, targets check
> if they still have something to report and re-mark their async event
> handler (e.g. remote_async_inferior_event_handler).
> 
> The linux_nat target does not suffer from this because it doesn't use an
> async event handler at the moment.  It only uses a pipe registered with
> the event loop.  It is written to in the SIGCHLD handler (and in other
> spots that want to get target wait method called) and read from in
> the target's wait method.  So if linux_nat happened to be target A in
> the example above, the pipe would just stay readable, and the event loop
> would wake up again, until linux_nat's wait method is finally called and
> consumes the contents of the pipe.
> 
> I think it would be nicer if targets using async_event_handler worked in
> a similar way, where the flag would stay set until the target's wait
> method is actually called.  As a first step towards that, this patch
> moves the responsibility of clearing the ready flags of async event
> handlers to the invoked callback.
> 
> All async event handler callbacks are modified to clear their ready flag
> before doing anything else.  So in practice, nothing changes with this
> patch.  It's only the responsibility of clearing the flag that is
> shifted toward the callee.
> 
> gdb/ChangeLog:
> 
>         * async-event.h (async_event_handler_func):  Add documentation.
>         * async-event.c (check_async_event_handlers): Don't clear
> 	async_event_handler ready flag.
>         * infrun.c (infrun_async_inferior_event_handler): Clear ready
> 	flag.
>         * record-btrace.c (record_btrace_handle_async_inferior_event):
> 	Likewise.
>         * record-full.c (record_full_async_inferior_event_handler):
> 	Likewise.
>         * remote-notif.c (remote_async_get_pending_events_handler):
> 	Likewise.
>         * remote.c (remote_async_inferior_event_handler): Likewise.
>

LGTM.

Thanks,
Andrew

> Change-Id: I179ef8e99580eae642d332846fd13664dbddc0c1
> ---
>  gdb/async-event.c   | 1 -
>  gdb/async-event.h   | 9 +++++++++
>  gdb/infrun.c        | 1 +
>  gdb/record-btrace.c | 1 +
>  gdb/record-full.c   | 1 +
>  gdb/remote-notif.c  | 4 +++-
>  gdb/remote.c        | 5 +++--
>  7 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/async-event.c b/gdb/async-event.c
> index 4228dfb09e6..eb967b568c5 100644
> --- a/gdb/async-event.c
> +++ b/gdb/async-event.c
> @@ -322,7 +322,6 @@ check_async_event_handlers ()
>      {
>        if (async_handler_ptr->ready)
>  	{
> -	  async_handler_ptr->ready = 0;
>  	  event_loop_debug_printf ("invoking async event handler `%s`",
>  				   async_handler_ptr->name);
>  	  (*async_handler_ptr->proc) (async_handler_ptr->client_data);
> diff --git a/gdb/async-event.h b/gdb/async-event.h
> index 8f279d63d63..10b9ae85112 100644
> --- a/gdb/async-event.h
> +++ b/gdb/async-event.h
> @@ -24,6 +24,15 @@
>  struct async_signal_handler;
>  struct async_event_handler;
>  typedef void (sig_handler_func) (gdb_client_data);
> +
> +/* Type of async event handler callbacks.
> +
> +   DATA is the client data originally passed to create_async_event_handler.
> +
> +   The callback is called when the async event handler is marked.  The callback
> +   is responsible for clearing the async event handler if it no longer needs
> +   to be called.  */
> +
>  typedef void (async_event_handler_func) (gdb_client_data);
>  
>  extern struct async_signal_handler *
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 2e5e837452d..eee96816e3b 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -9158,6 +9158,7 @@ static const struct internalvar_funcs siginfo_funcs =
>  static void
>  infrun_async_inferior_event_handler (gdb_client_data data)
>  {
> +  clear_async_event_handler (infrun_async_inferior_event_token);
>    inferior_event_handler (INF_REG_EVENT);
>  }
>  
> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> index dece9b60778..d5338c74aed 100644
> --- a/gdb/record-btrace.c
> +++ b/gdb/record-btrace.c
> @@ -326,6 +326,7 @@ record_btrace_auto_disable (void)
>  static void
>  record_btrace_handle_async_inferior_event (gdb_client_data data)
>  {
> +  clear_async_event_handler (record_btrace_async_inferior_event_handler);
>    inferior_event_handler (INF_REG_EVENT);
>  }
>  
> diff --git a/gdb/record-full.c b/gdb/record-full.c
> index 3b5e6fee7cd..410028aed60 100644
> --- a/gdb/record-full.c
> +++ b/gdb/record-full.c
> @@ -904,6 +904,7 @@ static struct async_event_handler *record_full_async_inferior_event_token;
>  static void
>  record_full_async_inferior_event_handler (gdb_client_data data)
>  {
> +  clear_async_event_handler (record_full_async_inferior_event_token);
>    inferior_event_handler (INF_REG_EVENT);
>  }
>  
> diff --git a/gdb/remote-notif.c b/gdb/remote-notif.c
> index f18bc8678e3..3b81fd55557 100644
> --- a/gdb/remote-notif.c
> +++ b/gdb/remote-notif.c
> @@ -108,8 +108,10 @@ remote_notif_process (struct remote_notif_state *state,
>  static void
>  remote_async_get_pending_events_handler (gdb_client_data data)
>  {
> +  remote_notif_state *notif_state = (remote_notif_state *) data;
> +  clear_async_event_handler (notif_state->get_pending_events_token);
>    gdb_assert (target_is_non_stop_p ());
> -  remote_notif_process ((struct remote_notif_state *) data, NULL);
> +  remote_notif_process (notif_state, NULL);
>  }
>  
>  /* Remote notification handler.  Parse BUF, queue notification and
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 71f814efb36..23c1bab0b27 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -14174,10 +14174,11 @@ remote_async_serial_handler (struct serial *scb, void *context)
>  static void
>  remote_async_inferior_event_handler (gdb_client_data data)
>  {
> -  inferior_event_handler (INF_REG_EVENT);
> -
>    remote_target *remote = (remote_target *) data;
>    remote_state *rs = remote->get_remote_state ();
> +  clear_async_event_handler (rs->remote_async_inferior_event_token);
> +
> +  inferior_event_handler (INF_REG_EVENT);
>  
>    /* inferior_event_handler may have consumed an event pending on the
>       infrun side without calling target_wait on the REMOTE target, or
> -- 
> 2.29.2
>