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

[Pedro Alves <pedro@palves.net>]

On 30/11/20 16:52, Simon Marchi via Gdb-patches wrote:
> 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.

(Watch out for spaces/tabs in the ChangeLog.)

I like this.  OK.