Re: [PATCH 24/31] Report thread exit event for leader if reporting thread exit events

Andrew Burgess via Gdb-patches
2023-06-09T13:11:36Z

Pedro Alves <pedro@palves.net> writes:

> If GDB sets the GDB_THREAD_OPTION_EXIT option on a thread, then if the
> thread disappears from the thread list, GDB expects to shortly see a
> thread exit event for it. See e.g., here, in
> remote_target::update_thread_list():
>
> /* Do not remove the thread if we've requested to be
> notified of its exit. For example, the thread may be
> displaced stepping, infrun will need to handle the
> exit event, and displaced stepping info is recorded
> in the thread object. If we deleted the thread now,
> we'd lose that info. */
> if ((tp->thread_options () & GDB_THREAD_OPTION_EXIT) != 0)
> continue;
>
> There's one scenario that is deleting a thread from the
> remote/gdbserver thread list without ever reporting a corresponding
> thread exit event though -- check_zombie_leaders. This can lead to
> GDB getting confused. For example, with a following patch that
> enables GDB_THREAD_OPTION_EXIT whenever schedlock is enabled, we'd see
> this regression:
>
> $ make check RUNTESTFLAGS="--target_board=native-extended-gdbserver" TESTS="gdb.threads/no-unwaited-for-left.exp"
> ...
> Running src/gdb/testsuite/gdb.threads/no-unwaited-for-left.exp ...
> FAIL: gdb.threads/no-unwaited-for-left.exp: continue stops when the main thread exits (timeout)
> ... some more cascading FAILs ...
>
> gdb.log shows:
>
> (gdb) continue
> Continuing.
> FAIL: gdb.threads/no-unwaited-for-left.exp: continue stops when the main thread exits (timeout)
>
> A passing run would have resulted in:
>
> (gdb) continue
> Continuing.
> No unwaited-for children left.
> (gdb) PASS: gdb.threads/no-unwaited-for-left.exp: continue stops when the main thread exits
>
> note how the leader thread is not listed in the remote-reported XML
> thread list below:
>
> (gdb) set debug remote 1
> (gdb) set debug infrun 1
> (gdb) info threads
> Id Target Id Frame
> * 1 Thread 1163850.1163850 "no-unwaited-for" main () at /home/pedro/rocm/gdb/build/gdb/testsuite/../../../src/gdb/testsuite/gdb.threads/no-unwaited-for-left.c:65
> 3 Thread 1163850.1164130 "no-unwaited-for" [remote] Sending packet: $Hgp11c24a.11c362#39
> (gdb) c
> Continuing.
> [infrun] clear_proceed_status_thread: 1163850.1163850.0
> ...
> [infrun] resume_1: step=1, signal=GDB_SIGNAL_0, trap_expected=1, current thread [1163850.1163850.0] at 0x55555555534f
> [remote] Sending packet: $QPassSignals:#f3
> [remote] Packet received: OK
> [remote] Sending packet: $QThreadOptions;3:p11c24a.11c24a#f3
> [remote] Packet received: OK
> ...
> [infrun] target_set_thread_options: [options for Thread 1163850.1163850 are now 0x3]
> ...
> [infrun] do_target_resume: resume_ptid=1163850.1163850.0, step=0, sig=GDB_SIGNAL_0
> [remote] Sending packet: $vCont;c:p11c24a.11c24a#98
> [infrun] prepare_to_wait: prepare_to_wait
> [infrun] reset: reason=handling event
> [infrun] maybe_set_commit_resumed_all_targets: enabling commit-resumed for target extended-remote
> [infrun] maybe_call_commit_resumed_all_targets: calling commit_resumed for target extended-remote
> [infrun] fetch_inferior_event: exit
> [infrun] fetch_inferior_event: enter
> [infrun] scoped_disable_commit_resumed: reason=handling event
> [infrun] random_pending_event_thread: None found.
> [remote] wait: enter
> [remote] Packet received: N
> [remote] wait: exit
> [infrun] print_target_wait_results: target_wait (-1.0.0 [process -1], status) =
> [infrun] print_target_wait_results: -1.0.0 [process -1],
> [infrun] print_target_wait_results: status->kind = NO_RESUMED
> [infrun] handle_inferior_event: status->kind = NO_RESUMED
> [remote] Sending packet: $Hgp0.0#ad
> [remote] Packet received: OK
> [remote] Sending packet: $qXfer:threads:read::0,1000#92
> [remote] Packet received: l<threads>\n<thread id="p11c24a.11c362" core="0" name="no-unwaited-for" handle="0097d8f7ff7f0000"/>\n</threads>\n
> [infrun] handle_no_resumed: TARGET_WAITKIND_NO_RESUMED (ignoring: found resumed)
> ...
>
> ... however, infrun decided there was a resumed thread still, so
> ignored the TARGET_WAITKIND_NO_RESUMED event. Debugging GDB, we see
> that the "found resumed" thread that GDB finds, is the leader thread.
> Even though that thread is not on the remote-reported thread list, it
> is still on the GDB thread list, due to the special case in remote.c
> mentioned above.
>
> This commit addresses the issue by fixing GDBserver to report a thread
> exit event for the zombie leader too, i.e., making GDBserver respect
> the "if thread has GDB_THREAD_OPTION_EXIT set, report a thread exit"
> invariant. To do that, it takes a bit more code than one would
> imagine off hand, due to the fact that we currently always report LWP
> exit pending events as TARGET_WAITKIND_EXITED, and then decide whether
> to convert it to TARGET_WAITKIND_THREAD_EXITED just before reporting
> the event to GDBserver core. For the zombie leader scenario
> described, we need to record early on that we want to report a
> THREAD_EXITED event, and then make sure that decision isn't lost along
> the way to reporting the event to GDBserver core.

LGTM.

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

Thanks,
Andrew

>
> Change-Id: I1e68fccdbc9534434dee07163d3fd19744c8403b
> ---
> gdbserver/linux-low.cc | 75 ++++++++++++++++++++++++++++++++++++------
> gdbserver/linux-low.h | 5 +--
> 2 files changed, 68 insertions(+), 12 deletions(-)
>
> diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
> index bf6dc1d995a..c197846810c 100644
> --- a/gdbserver/linux-low.cc
> +++ b/gdbserver/linux-low.cc
> @@ -279,7 +279,8 @@ int using_threads = 1;
> static int stabilizing_threads;
>
> static void unsuspend_all_lwps (struct lwp_info *except);
> -static void mark_lwp_dead (struct lwp_info *lwp, int wstat);
> +static void mark_lwp_dead (struct lwp_info *lwp, int wstat,
> + bool thread_event);
> static int lwp_is_marked_dead (struct lwp_info *lwp);
> static int kill_lwp (unsigned long lwpid, int signo);
> static void enqueue_pending_signal (struct lwp_info *lwp, int signal, siginfo_t *info);
> @@ -1800,10 +1801,12 @@ iterate_over_lwps (ptid_t filter,
> return get_thread_lwp (thread);
> }
>
> -void
> +bool
> linux_process_target::check_zombie_leaders ()
> {
> - for_each_process ([this] (process_info *proc)
> + bool new_pending_event = false;
> +
> + for_each_process ([&] (process_info *proc)
> {
> pid_t leader_pid = pid_of (proc);
> lwp_info *leader_lp = find_lwp_pid (ptid_t (leader_pid));
> @@ -1872,9 +1875,19 @@ linux_process_target::check_zombie_leaders ()
> "(it exited, or another thread execd), "
> "deleting it.",
> leader_pid);
> - delete_lwp (leader_lp);
> +
> + thread_info *leader_thread = get_lwp_thread (leader_lp);
> + if (report_exit_events_for (leader_thread))
> + {
> + mark_lwp_dead (leader_lp, W_EXITCODE (0, 0), true);
> + new_pending_event = true;
> + }
> + else
> + delete_lwp (leader_lp);
> }
> });
> +
> + return new_pending_event;
> }
>
> /* Callback for `find_thread'. Returns the first LWP that is not
> @@ -2333,7 +2346,7 @@ linux_process_target::filter_event (int lwpid, int wstat)
> /* Since events are serialized to GDB core, and we can't
> report this one right now. Leave the status pending for
> the next time we're able to report it. */
> - mark_lwp_dead (child, wstat);
> + mark_lwp_dead (child, wstat, false);
> return;
> }
> else
> @@ -2646,7 +2659,8 @@ linux_process_target::wait_for_event_filtered (ptid_t wait_ptid,
>
> /* Check for zombie thread group leaders. Those can't be reaped
> until all other threads in the thread group are. */
> - check_zombie_leaders ();
> + if (check_zombie_leaders ())
> + goto retry;
>
> auto not_stopped = [&] (thread_info *thread)
> {
> @@ -2893,6 +2907,17 @@ linux_process_target::filter_exit_event (lwp_info *event_child,
> struct thread_info *thread = get_lwp_thread (event_child);
> ptid_t ptid = ptid_of (thread);
>
> + if (ourstatus->kind () == TARGET_WAITKIND_THREAD_EXITED)
> + {
> + /* We're reporting a thread exit for the leader. The exit was
> + detected by check_zombie_leaders. */
> + gdb_assert (is_leader (thread));
> + gdb_assert (report_exit_events_for (thread));
> +
> + delete_lwp (event_child);
> + return ptid;
> + }
> +
> /* Note we must filter TARGET_WAITKIND_SIGNALLED as well, otherwise
> if a non-leader thread exits with a signal, we'd report it to the
> core which would interpret it as the whole-process exiting.
> @@ -3012,7 +3037,20 @@ linux_process_target::wait_1 (ptid_t ptid, target_waitstatus *ourstatus,
> {
> if (WIFEXITED (w))
> {
> - ourstatus->set_exited (WEXITSTATUS (w));
> + /* If we already have the exit recorded in waitstatus, use
> + it. This will happen when we detect a zombie leader,
> + when we had GDB_THREAD_OPTION_EXIT enabled for it. We
> + want to report its exit as TARGET_WAITKIND_THREAD_EXITED,
> + as the whole process hasn't exited yet. */
> + const target_waitstatus &ws = event_child->waitstatus;
> + if (ws.kind () != TARGET_WAITKIND_IGNORE)
> + {
> + gdb_assert (ws.kind () == TARGET_WAITKIND_EXITED
> + || ws.kind () == TARGET_WAITKIND_THREAD_EXITED);
> + *ourstatus = ws;
> + }
> + else
> + ourstatus->set_exited (WEXITSTATUS (w));
>
> threads_debug_printf
> ("ret = %s, exited with retcode %d",
> @@ -3720,8 +3758,15 @@ suspend_and_send_sigstop (thread_info *thread, lwp_info *except)
> send_sigstop (thread, except);
> }
>
> +/* Mark LWP dead, with WSTAT as exit status pending to report later.
> + If THREAD_EVENT is true, interpret WSTAT as a thread exit event
> + instead of a process exit event. This is meaningful for the leader
> + thread, as we normally report a process-wide exit event when we see
> + the leader exit, and a thread exit event when we see any other
> + thread exit. */
> +
> static void
> -mark_lwp_dead (struct lwp_info *lwp, int wstat)
> +mark_lwp_dead (struct lwp_info *lwp, int wstat, bool thread_event)
> {
> /* Store the exit status for later. */
> lwp->status_pending_p = 1;
> @@ -3730,9 +3775,19 @@ mark_lwp_dead (struct lwp_info *lwp, int wstat)
> /* Store in waitstatus as well, as there's nothing else to process
> for this event. */
> if (WIFEXITED (wstat))
> - lwp->waitstatus.set_exited (WEXITSTATUS (wstat));
> + {
> + if (thread_event)
> + lwp->waitstatus.set_thread_exited (WEXITSTATUS (wstat));
> + else
> + lwp->waitstatus.set_exited (WEXITSTATUS (wstat));
> + }
> else if (WIFSIGNALED (wstat))
> - lwp->waitstatus.set_signalled (gdb_signal_from_host (WTERMSIG (wstat)));
> + {
> + gdb_assert (!thread_event);
> + lwp->waitstatus.set_signalled (gdb_signal_from_host (WTERMSIG (wstat)));
> + }
> + else
> + gdb_assert_not_reached ("unknown status kind");
>
> /* Prevent trying to stop it. */
> lwp->stopped = 1;
> diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
> index 33a14e15173..ffbc3c6f095 100644
> --- a/gdbserver/linux-low.h
> +++ b/gdbserver/linux-low.h
> @@ -574,8 +574,9 @@ class linux_process_target : public process_stratum_target
>
> /* Detect zombie thread group leaders, and "exit" them. We can't
> reap their exits until all other threads in the group have
> - exited. */
> - void check_zombie_leaders ();
> + exited. Returns true if we left any new event pending, false
> + otherwise. */
> + bool check_zombie_leaders ();
>
> /* Convenience function that is called when we're about to return an
> event to the core. If the event is an exit or signalled event,
> --
> 2.36.0