From e8a625d126e4be34d23b5df535bed134b2bb3156 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Wed, 15 Jun 2022 13:19:47 +0100 Subject: [PATCH] Report thread exit event for leader if reporting thread exit events 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\n\n\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. Reviewed-By: Andrew Burgess 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 44d0fe38030..f9001e2fa17 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); @@ -1803,10 +1804,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)); @@ -1875,9 +1878,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 @@ -2336,7 +2349,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 @@ -2655,7 +2668,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) { @@ -2902,6 +2916,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. @@ -3021,7 +3046,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", @@ -3727,8 +3765,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; @@ -3737,9 +3782,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 d46ea5aa3ec..51d1899893a 100644 --- a/gdbserver/linux-low.h +++ b/gdbserver/linux-low.h @@ -574,8 +574,9 @@ private: /* Back to private. */ /* 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.30.2