Re: [PATCH 21/31] stop_all_threads: (re-)enable async before waiting for stops

Andrew Burgess via Gdb-patches
2023-06-08T15:50:02Z

Pedro Alves <pedro@palves.net> writes:

> Running the
> gdb.threads/step-over-thread-exit-while-stop-all-threads.exp testcase
> added later in the series against gdbserver, after the
> TARGET_WAITKIND_NO_RESUMED fix from the following patch, would run
> into an infinite loop in stop_all_threads, leading to a timeout:
>
> FAIL: gdb.threads/step-over-thread-exit-while-stop-all-threads.exp: displaced-stepping=off: target-non-stop=on: iter 0: continue (timeout)
>
> The is really a latent bug, and it is about the fact that
> stop_all_threads stops listening to events from a target as soon as it
> sees a TARGET_WAITKIND_NO_RESUMED, ignoring that
> TARGET_WAITKIND_NO_RESUMED may be delayed. handle_no_resumed knows
> how to handle delayed no-resumed events, but stop_all_threads was
> never taught to.
>
> In more detail, here's what happens with that testcase:
>
> #1 - Multiple threads report breakpoint hits to gdb.
>
> #2 - gdb picks one events, and it's for thread 1. All other stops are
> left pending. thread 1 needs to move past a breakpoint, so gdb
> stops all threads to start an inline step over for thread 1.
> While stopping threads, some of the threads that were still
> running report events that are also left pending.
>
> #2 - gdb steps thread 1
>
> #3 - Thread 1 exits while stepping (it steps over an exit syscall),
> gdbserver reports thread exit for thread 1
>
> #4 - Thread 1 was the last resumed thread, so gdbserver also reports
> no-resumed:
>
> [remote] Notification received: Stop:w0;p3445d0.3445d3
> [remote] Sending packet: $vStopped#55
> [remote] Packet received: N
> [remote] Sending packet: $vStopped#55
> [remote] Packet received: OK
>
> #5 - gdb processes the thread exit for thread 1, finishes the step
> over and restarts threads.
>
> #6 - gdb picks the next event to process out of one of the resumed
> threads with pending events:
>
> [infrun] random_resumed_with_pending_wait_status: Found 32 events, selecting #11
>
> #7 - This is again a breakpoint hit and the breakpoint needs to be
> stepped over too, so gdb starts a step-over dance again.
>
> #8 - We reach stop_all_threads, which finds that some threads need to
> be stopped.
>
> #9 - wait_one finally consumes the no-resumed event queue by #4.
> Seeing this, wait_one disable target async, to stop listening for
> events out of the remote target.
>
> #10 - We still haven't seen all the stops expected, so
> stop_all_threads tries another iteration.
>
> #11 - Because the remote target is no longer async, and there are no
> other targets, wait_one return no-resumed immediately without
> polling the remote target.
>
> #12 - We still haven't seen all the stops expected, so
> stop_all_threads tries another iteration. goto #11, looping
> forever.
>
> Fix this by explicitly enabling/re-enabling target async on targets
> that can async, before waiting for stops.
>
> Change-Id: Ie3ffb0df89635585a6631aa842689cecc989e33f
> ---
> gdb/infrun.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 81 insertions(+)
>
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 2866962d2dc..31321d758da 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -5011,6 +5011,8 @@ wait_one ()
> if (nfds == 0)
> {
> /* No waitable targets left. All must be stopped. */
> + infrun_debug_printf ("no waitable targets left");
> +
> target_waitstatus ws;
> ws.set_no_resumed ();
> return {nullptr, minus_one_ptid, std::move (ws)};
> @@ -5269,6 +5271,83 @@ handle_one (const wait_one_event &event)
> return false;
> }
>
> +/* Helper for stop_all_threads. wait_one waits for events until it
> + sees a TARGET_WAITKIND_NO_RESUMED event. When it sees one, it
> + disables target_async for the target to stop waiting for events
> + from it. TARGET_WAITKIND_NO_RESUMED can be delayed though,
> + consider, debugging against gdbserver:
> +
> + #1 - Threads 1-5 are running, and thread 1 hits a breakpoint.
> +
> + #2 - gdb processes the breakpoint hit for thread 1, stops all
> + threads, and steps thread 1 over the breakpoint. while
> + stopping threads, some other threads reported interesting
> + events, which were left pending in the thread's objects
> + (infrun's queue).
> +
> + #2 - Thread 1 exits (it stepped an exit syscall), and gdbserver
> + reports the thread exit for thread 1. The event ends up in
> + remote's stop reply queue.
> +
> + #3 - That was the last resumed thread, so gdbserver reports
> + no-resumed, and that event also ends up in remote's stop
> + reply queue, queued after the thread exit from #2.
> +
> + #4 - gdb processes the thread exit event, which finishes the
> + step-over, and so gdb restarts all threads (threads with
> + pending events are left marked resumed, but aren't set
> + executing). The no-resumed event is still left pending in
> + the remote stop reply queue.
> +
> + #5 - Since there are now resumed threads with pending breakpoint
> + hits, gdb picks one at random to process next.
> +
> + #5 - gdb picks the breakpoint hit for thread 2 this time, and that
> + breakpoint also needs to be stepped over, so gdb stops all
> + threads again.
> +
> + #6 - stop_all_threads counts number of expected stops and calls
> + wait_one once for each.
> +
> + #7 - The first wait_one call collects the no-resumed event from #3
> + above.
> +
> + #9 - Seeing the no-resumed event, wait_one disables target async
> + for the remote target, to stop waiting for events from it.
> + wait_one from here on always return no-resumed directly
> + without reaching the target.
> +
> + #10 - stop_all_threads still hasn't seen all the stops it expects,
> + so it does another pass.
> +
> + #11 - Since the remote target is not async (disabled in #9),
> + wait_one doesn't wait on it, so it won't see the expected
> + stops, and instead returns no-resumed directly.
> +
> + #12 - stop_all_threads still haven't seen all the stops, so it
> + does another pass. goto #b, looping forever.

s/#b/#11/

Otherwise, LGTM.

Reviewed-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

> +
> + To handle this, we explicitly (re-)enable target async on all
> + targets that can async every time stop_all_threads goes wait for
> + the expected stops. */
> +
> +static void
> +reenable_target_async ()
> +{
> + for (inferior *inf : all_inferiors ())
> + {
> + process_stratum_target *target = inf->process_target ();
> + if (target != nullptr
> + && target->threads_executing
> + && target->can_async_p ()
> + && !target->is_async_p ())
> + {
> + switch_to_inferior_no_thread (inf);
> + target_async (1);
> + }
> + }
> +}
> +
> /* See infrun.h. */
>
> void
> @@ -5395,6 +5474,8 @@ stop_all_threads (const char *reason, inferior *inf)
> if (pass > 0)
> pass = -1;
>
> + reenable_target_async ();
> +
> for (int i = 0; i < waits_needed; i++)
> {
> wait_one_event event = wait_one ();
> --
> 2.36.0