gdb: make async event handlers clear themselves

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.

Change-Id: I179ef8e99580eae642d332846fd13664dbddc0c1

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 4ac326be1639ea5e3f5c63781cd585bbb501a40e..137f3be6a7c6107c9384de1d4b194993cfe8960a 100644 (file)
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,18 @@
+2021-02-04  Simon Marchi  <simon.marchi@efficios.com>
+
+       * 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.
+
 2021-02-03  Simon Marchi  <simon.marchi@polymtl.ca>
 
        * infrun.c (handle_inferior_event): Move stop_soon variable to

diff --git a/gdb/async-event.c b/gdb/async-event.c
index 18dac7700ccf3c7b0327e8a4f8e88474d804d1ca..c4ab7318179e9b3952ec8c26cb20f075f9d1bf5d 100644 (file)
--- 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 0f4c8925298ce8f443346a5c8440f5f306b42036..9d96235b38dc967307e46cf7b9db73bcfa8a25f7 100644 (file)
--- 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 6ec269adaa8c5b127197ff85615f26ea15de9d15..a271220b2616dc8da9f1ee3645a01608afc2c477 100644 (file)
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -9212,6 +9212,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 81686ee867b7309fd48ea002036cf80fddc77572..ea339b865b35f41da68fc1206fdd7003d308c3c7 100644 (file)
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -325,6 +325,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 22eaaa4bb1bc35873f88f3617daea632855f4e32..247573cc5c6403fe39816506b12df770cd0d5fad 100644 (file)
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -903,6 +903,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 035d5f42f013d18fb8dd1e61aad5a090bab9b6e3..5a3e1395b76e63df0efd9c20b409a506228e16ed 100644 (file)
--- 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 c544fe7fcece9105fd4f4f47877df54e33be8268..47538fce1969845bc10d54b5ea0d7f3dec358e5e 100644 (file)
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -14259,10 +14259,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