From 9d7d58e7262eff313be6a1e66b8b026e3e7fed0d Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Mon, 12 Dec 2022 20:31:00 +0000 Subject: [PATCH] gdb: centralize "[Thread ...exited]" notifications Currently, each target backend is responsible for printing "[Thread ...exited]" before deleting a thread. This leads to unnecessary differences between targets, like e.g. with the remote target, we never print such messages, even though we do print "[New Thread ...]". E.g., debugging the gdb.threads/attach-many-short-lived-threads.exp with gdbserver, letting it run for a bit, and then pressing Ctrl-C, we currently see: (gdb) c Continuing. ^C[New Thread 3850398.3887449] [New Thread 3850398.3887500] [New Thread 3850398.3887551] [New Thread 3850398.3887602] [New Thread 3850398.3887653] ... Thread 1 "attach-many-sho" received signal SIGINT, Interrupt. 0x00007ffff7e6a23f in __GI___clock_nanosleep (clock_id=clock_id@entry=0, flags=flags@entry=0, req=req@entry=0x7fffffffda80, rem=rem@entry=0x7fffffffda80) at ../sysdeps/unix/sysv/linux/clock_nanosleep.c:78 78 in ../sysdeps/unix/sysv/linux/clock_nanosleep.c (gdb) Above, we only see "New Thread" notifications, even though threads were deleted. After this patch, we'll see: (gdb) c Continuing. ^C[Thread 3558643.3577053 exited] [Thread 3558643.3577104 exited] [Thread 3558643.3577155 exited] [Thread 3558643.3579603 exited] ... [New Thread 3558643.3597415] [New Thread 3558643.3600015] [New Thread 3558643.3599965] ... Thread 1 "attach-many-sho" received signal SIGINT, Interrupt. 0x00007ffff7e6a23f in __GI___clock_nanosleep (clock_id=clock_id@entry=0, flags=flags@entry=0, req=req@entry=0x7fffffffda80, rem=rem@entry=0x7fffffffda80) at ../sysdeps/unix/sysv/linux/clock_nanosleep.c:78 78 in ../sysdeps/unix/sysv/linux/clock_nanosleep.c (gdb) q This commit fixes this by moving the thread exit printing to common code instead, triggered from within delete_thread (or rather, set_thread_exited). There's one wrinkle, though. While most targest want to print: [Thread ... exited] the Windows target wants to print: [Thread ... exited with code ] ... and sometimes wants to suppress the notification for the main thread. To address that, this commits adds a delete_thread_with_code function, only used by that target (so far). This fix was originally posted as part of a larger series: https://inbox.sourceware.org/gdb-patches/20221212203101.1034916-1-pedro@palves.net/ But didn't really need to be part of that series. In order to get this fix merged sooner, I (Andrew Burgess) have rebased this commit outside of the original series. Any bugs introduced while splitting this patch out and rebasing, are entirely my own. Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30129 Co-Authored-By: Andrew Burgess --- gdb/annotate.c | 4 +- gdb/breakpoint.c | 4 +- gdb/fbsd-nat.c | 3 -- gdb/gdbthread.h | 22 ++++++-- gdb/inferior.c | 2 +- gdb/interps.c | 6 ++- gdb/interps.h | 8 ++- gdb/linux-nat.c | 8 +-- gdb/mi/mi-interp.c | 4 +- gdb/mi/mi-interp.h | 3 +- gdb/netbsd-nat.c | 3 -- gdb/observable.h | 11 ++-- gdb/procfs.c | 6 --- gdb/python/py-inferior.c | 4 +- gdb/testsuite/gdb.mi/mi-thread-bp-deleted.exp | 14 ++--- .../gdb.threads/thread-bp-deleted.exp | 12 +---- gdb/thread.c | 54 +++++++++++++------ gdb/windows-nat.c | 20 +++---- 18 files changed, 100 insertions(+), 88 deletions(-) diff --git a/gdb/annotate.c b/gdb/annotate.c index d403a47ba2f..8385429042d 100644 --- a/gdb/annotate.c +++ b/gdb/annotate.c @@ -232,7 +232,9 @@ annotate_thread_changed (void) /* Emit notification on thread exit. */ static void -annotate_thread_exited (struct thread_info *t, int silent) +annotate_thread_exited (thread_info *t, + gdb::optional exit_code, + bool /* silent */) { if (annotation_level > 1) { diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index f0f5328fb5e..c429af455ff 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -3362,7 +3362,9 @@ remove_breakpoints (void) that thread. */ static void -remove_threaded_breakpoints (struct thread_info *tp, int silent) +remove_threaded_breakpoints (thread_info *tp, + gdb::optional /* exit_code */, + int /* silent */) { for (breakpoint &b : all_breakpoints_safe ()) { diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c index 41ca1f61561..0ee3bccb5ef 100644 --- a/gdb/fbsd-nat.c +++ b/gdb/fbsd-nat.c @@ -1394,9 +1394,6 @@ fbsd_nat_target::wait_1 (ptid_t ptid, struct target_waitstatus *ourstatus, { fbsd_lwp_debug_printf ("deleting thread for LWP %u", pl.pl_lwpid); - if (print_thread_events) - gdb_printf (_("[%s exited]\n"), - target_pid_to_str (wptid).c_str ()); low_delete_thread (thr); delete_thread (thr); fbsd_inf->num_lwps--; diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h index baff68aee92..48f32bb3a0b 100644 --- a/gdb/gdbthread.h +++ b/gdb/gdbthread.h @@ -623,16 +623,30 @@ extern struct thread_info *add_thread_with_info (process_stratum_target *targ, /* Delete thread THREAD and notify of thread exit. If the thread is currently not deletable, don't actually delete it but still tag it - as exited and do the notification. */ -extern void delete_thread (struct thread_info *thread); + as exited and do the notification. EXIT_CODE is the thread's exit + code. If SILENT, don't actually notify the CLI. THREAD must not + be NULL or an assertion will fail. */ +extern void delete_thread_with_exit_code (thread_info *thread, + ULONGEST exit_code, + bool silent = false); + +/* Delete thread THREAD and notify of thread exit. If the thread is + currently not deletable, don't actually delete it but still tag it + as exited and do the notification. THREAD must not be NULL or an + assertion will fail. */ +extern void delete_thread (thread_info *thread); /* Like delete_thread, but be quiet about it. Used when the process this thread belonged to has already exited, for example. */ extern void delete_thread_silent (struct thread_info *thread); /* Mark the thread exited, but don't delete it or remove it from the - inferior thread list. */ -extern void set_thread_exited (thread_info *tp, bool silent); + inferior thread list. EXIT_CODE is the thread's exit code, if + available. If SILENT, then don't inform the CLI about the + exit. */ +extern void set_thread_exited (thread_info *tp, + gdb::optional exit_code = {}, + bool silent = false); /* Delete a step_resume_breakpoint from the thread database. */ extern void delete_step_resume_breakpoint (struct thread_info *); diff --git a/gdb/inferior.c b/gdb/inferior.c index cf4caa923cf..ce4960a508a 100644 --- a/gdb/inferior.c +++ b/gdb/inferior.c @@ -253,7 +253,7 @@ inferior::clear_thread_list () { threads_debug_printf ("deleting thread %s", thr->ptid.to_string ().c_str ()); - set_thread_exited (thr, true /* silent */); + set_thread_exited (thr, {}, true /* silent */); if (thr->deletable ()) delete thr; }); diff --git a/gdb/interps.c b/gdb/interps.c index f665e86b65c..7baa8491eb1 100644 --- a/gdb/interps.c +++ b/gdb/interps.c @@ -457,9 +457,11 @@ interps_notify_new_thread (thread_info *t) /* See interps.h. */ void -interps_notify_thread_exited (thread_info *t, int silent) +interps_notify_thread_exited (thread_info *t, + gdb::optional exit_code, + int silent) { - interps_notify (&interp::on_thread_exited, t, silent); + interps_notify (&interp::on_thread_exited, t, exit_code, silent); } /* See interps.h. */ diff --git a/gdb/interps.h b/gdb/interps.h index 3a0e8372e10..c041d0d95b6 100644 --- a/gdb/interps.h +++ b/gdb/interps.h @@ -122,7 +122,9 @@ public: virtual void on_new_thread (thread_info *t) {} /* Notify the interpreter that thread T has exited. */ - virtual void on_thread_exited (thread_info *, int silent) {} + virtual void on_thread_exited (thread_info *, + gdb::optional exit_code, + int silent) {} /* Notify the interpreter that inferior INF was added. */ virtual void on_inferior_added (inferior *inf) {} @@ -297,7 +299,9 @@ extern void interps_notify_user_selected_context_changed extern void interps_notify_new_thread (thread_info *t); /* Notify all interpreters that thread T has exited. */ -extern void interps_notify_thread_exited (thread_info *t, int silent); +extern void interps_notify_thread_exited (thread_info *t, + gdb::optional exit_code, + int silent); /* Notify all interpreters that inferior INF was added. */ extern void interps_notify_inferior_added (inferior *inf); diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c index 250a8f43282..e58d67183b5 100644 --- a/gdb/linux-nat.c +++ b/gdb/linux-nat.c @@ -906,13 +906,7 @@ exit_lwp (struct lwp_info *lp) struct thread_info *th = linux_target->find_thread (lp->ptid); if (th) - { - if (print_thread_events) - gdb_printf (_("[%s exited]\n"), - target_pid_to_str (lp->ptid).c_str ()); - - delete_thread (th); - } + delete_thread (th); delete_lwp (lp->ptid); } diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c index a7fcf17e51c..1eff4c286ea 100644 --- a/gdb/mi/mi-interp.c +++ b/gdb/mi/mi-interp.c @@ -277,7 +277,9 @@ mi_interp::on_new_thread (thread_info *t) } void -mi_interp::on_thread_exited (thread_info *t, int silent) +mi_interp::on_thread_exited (thread_info *t, + gdb::optional /* exit_code */, + int /* silent */) { target_terminal::scoped_restore_terminal_state term_state; target_terminal::ours_for_output (); diff --git a/gdb/mi/mi-interp.h b/gdb/mi/mi-interp.h index 6326ec7d524..bc975d75ad8 100644 --- a/gdb/mi/mi-interp.h +++ b/gdb/mi/mi-interp.h @@ -51,7 +51,8 @@ public: void on_command_error () override; void on_user_selected_context_changed (user_selected_what selection) override; void on_new_thread (thread_info *t) override; - void on_thread_exited (thread_info *t, int silent) override; + void on_thread_exited (thread_info *t, gdb::optional exit_code, + int silent) override; void on_inferior_added (inferior *inf) override; void on_inferior_appeared (inferior *inf) override; void on_inferior_disappeared (inferior *inf) override; diff --git a/gdb/netbsd-nat.c b/gdb/netbsd-nat.c index 86df0c8fe6a..d1614ec0783 100644 --- a/gdb/netbsd-nat.c +++ b/gdb/netbsd-nat.c @@ -626,9 +626,6 @@ nbsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus, /* NetBSD does not store an LWP exit status. */ ourstatus->set_thread_exited (0); - if (print_thread_events) - gdb_printf (_("[%s exited]\n"), - target_pid_to_str (wptid).c_str ()); delete_thread (thr); } diff --git a/gdb/observable.h b/gdb/observable.h index f6047a239cd..c0bafc51f14 100644 --- a/gdb/observable.h +++ b/gdb/observable.h @@ -109,10 +109,13 @@ extern observable free_objfile; /* The thread specified by T has been created. */ extern observable new_thread; -/* The thread specified by T has exited. The SILENT argument - indicates that gdb is removing the thread from its tables without - wanting to notify the user about it. */ -extern observable thread_exit; +/* The thread specified by T has exited. EXIT_CODE is the thread's + exit code, if available. The SILENT argument indicates that GDB is + removing the thread from its tables without wanting to notify the + CLI about it. */ +extern observable /* exit_code */, + bool /* silent */> thread_exit; /* An explicit stop request was issued to PTID. If PTID equals minus_one_ptid, the request applied to all threads. If diff --git a/gdb/procfs.c b/gdb/procfs.c index f0c2c2cf56c..d55a16a9fb5 100644 --- a/gdb/procfs.c +++ b/gdb/procfs.c @@ -2117,9 +2117,6 @@ wait_again: case PR_SYSENTRY: if (what == SYS_lwp_exit) { - if (print_thread_events) - gdb_printf (_("[%s exited]\n"), - target_pid_to_str (retval).c_str ()); delete_thread (this->find_thread (retval)); proc_resume (pi, ptid, 0, GDB_SIGNAL_0); goto wait_again; @@ -2223,9 +2220,6 @@ wait_again: } else if (what == SYS_lwp_exit) { - if (print_thread_events) - gdb_printf (_("[%s exited]\n"), - target_pid_to_str (retval).c_str ()); delete_thread (this->find_thread (retval)); status->set_spurious (); return retval; diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c index 792f05b118e..906d402827c 100644 --- a/gdb/python/py-inferior.c +++ b/gdb/python/py-inferior.c @@ -361,7 +361,9 @@ add_thread_object (struct thread_info *tp) } static void -delete_thread_object (struct thread_info *tp, int ignore) +delete_thread_object (thread_info *tp, + gdb::optional /* exit_code */, + bool /* silent */) { if (!gdb_python_initialized) return; diff --git a/gdb/testsuite/gdb.mi/mi-thread-bp-deleted.exp b/gdb/testsuite/gdb.mi/mi-thread-bp-deleted.exp index 0ebca924801..bffbb290071 100644 --- a/gdb/testsuite/gdb.mi/mi-thread-bp-deleted.exp +++ b/gdb/testsuite/gdb.mi/mi-thread-bp-deleted.exp @@ -235,17 +235,9 @@ foreach_mi_ui_mode mode { # The output has arrived! Check how we did. There are other bugs # that come into play here which change what output we'll see. - if { $saw_mi_thread_exited && $saw_mi_bp_deleted \ - && $saw_cli_thread_exited \ - && $saw_cli_bp_deleted } { - kpass "gdb/30129" $gdb_test_name - } elseif { $saw_mi_thread_exited && $saw_mi_bp_deleted \ - && !$saw_cli_thread_exited \ - && $saw_cli_bp_deleted } { - kfail "gdb/30129" $gdb_test_name - } else { - fail "$gdb_test_name" - } + gdb_assert { $saw_mi_thread_exited && $saw_mi_bp_deleted \ + && $saw_cli_thread_exited \ + && $saw_cli_bp_deleted } $gdb_test_name } } diff --git a/gdb/testsuite/gdb.threads/thread-bp-deleted.exp b/gdb/testsuite/gdb.threads/thread-bp-deleted.exp index 019bdddee81..92178ff838b 100644 --- a/gdb/testsuite/gdb.threads/thread-bp-deleted.exp +++ b/gdb/testsuite/gdb.threads/thread-bp-deleted.exp @@ -155,17 +155,7 @@ if {$is_remote} { exp_continue } - # When PR gdb/30129 is fixed then this can all be collapsed down - # into a single gdb_assert call. This is split out like this - # because the SAW_BP_DELETED part is working, and we want to - # spot if that stops working. - if { $saw_thread_exited && $saw_bp_deleted } { - kpass "gdb/30129" $gdb_test_name - } elseif {!$saw_thread_exited && $saw_bp_deleted} { - kfail "gdb/30129" $gdb_test_name - } else { - fail $gdb_test_name - } + gdb_assert { $saw_thread_exited && $saw_bp_deleted } $gdb_test_name } } } else { diff --git a/gdb/thread.c b/gdb/thread.c index 2cb9e5e36d0..c8145da59bc 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -194,16 +194,29 @@ clear_thread_inferior_resources (struct thread_info *tp) /* Notify interpreters and observers that thread T has exited. */ static void -notify_thread_exited (thread_info *t, int silent) +notify_thread_exited (thread_info *t, gdb::optional exit_code, + int silent) { - interps_notify_thread_exited (t, silent); - gdb::observers::thread_exit.notify (t, silent); + if (!silent && print_thread_events) + { + if (exit_code.has_value ()) + gdb_printf (_("[%s exited with code %s]\n"), + target_pid_to_str (t->ptid).c_str (), + pulongest (*exit_code)); + else + gdb_printf (_("[%s exited]\n"), + target_pid_to_str (t->ptid).c_str ()); + } + + interps_notify_thread_exited (t, exit_code, silent); + gdb::observers::thread_exit.notify (t, exit_code, silent); } /* See gdbthread.h. */ void -set_thread_exited (thread_info *tp, bool silent) +set_thread_exited (thread_info *tp, gdb::optional exit_code, + bool silent) { /* Dead threads don't need to step-over. Remove from chain. */ if (thread_is_in_step_over_chain (tp)) @@ -222,7 +235,7 @@ set_thread_exited (thread_info *tp, bool silent) if (proc_target != nullptr) proc_target->maybe_remove_resumed_with_pending_wait_status (tp); - notify_thread_exited (tp, silent); + notify_thread_exited (tp, exit_code, silent); /* Tag it as exited. */ tp->state = THREAD_EXITED; @@ -470,20 +483,22 @@ global_thread_step_over_chain_remove (struct thread_info *tp) global_thread_step_over_list.erase (it); } -/* Delete the thread referenced by THR. If SILENT, don't notify - the observer of this exit. - - THR must not be NULL or a failed assertion will be raised. */ +/* Helper for the different delete_thread variants. */ static void -delete_thread_1 (thread_info *thr, bool silent) +delete_thread_1 (thread_info *thr, gdb::optional exit_code, + bool silent) { gdb_assert (thr != nullptr); - threads_debug_printf ("deleting thread %s, silent = %d", - thr->ptid.to_string ().c_str (), silent); + threads_debug_printf ("deleting thread %s, exit_code = %s, silent = %d", + thr->ptid.to_string ().c_str (), + (exit_code.has_value () + ? pulongest (*exit_code) + : ""), + silent); - set_thread_exited (thr, silent); + set_thread_exited (thr, exit_code, silent); if (!thr->deletable ()) { @@ -499,16 +514,25 @@ delete_thread_1 (thread_info *thr, bool silent) /* See gdbthread.h. */ +void +delete_thread_with_exit_code (thread_info *thread, ULONGEST exit_code, + bool silent) +{ + delete_thread_1 (thread, exit_code, silent); +} + +/* See gdbthread.h. */ + void delete_thread (thread_info *thread) { - delete_thread_1 (thread, false /* not silent */); + delete_thread_1 (thread, {}, false /* not silent */); } void delete_thread_silent (thread_info *thread) { - delete_thread_1 (thread, true /* silent */); + delete_thread_1 (thread, {}, true /* not silent */); } struct thread_info * diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c index c09e459a801..d8b33452f46 100644 --- a/gdb/windows-nat.c +++ b/gdb/windows-nat.c @@ -612,21 +612,13 @@ windows_nat_target::delete_thread (ptid_t ptid, DWORD exit_code, id = ptid.lwp (); - /* Emit a notification about the thread being deleted. + /* Note that no notification was printed when the main thread was + created, and thus, unless in verbose mode, we should be symmetrical, + and avoid an exit notification for the main thread here as well. */ - Note that no notification was printed when the main thread - was created, and thus, unless in verbose mode, we should be - symmetrical, and avoid that notification for the main thread - here as well. */ - - if (info_verbose) - gdb_printf ("[Deleting %s]\n", target_pid_to_str (ptid).c_str ()); - else if (print_thread_events && !main_thread_p) - gdb_printf (_("[%s exited with code %u]\n"), - target_pid_to_str (ptid).c_str (), - (unsigned) exit_code); - - ::delete_thread (this->find_thread (ptid)); + bool silent = (main_thread_p && !info_verbose); + thread_info *to_del = this->find_thread (ptid); + delete_thread_with_exit_code (to_del, exit_code, silent); auto iter = std::find_if (windows_process.thread_list.begin (), windows_process.thread_list.end (), -- 2.30.2