gdb: centralize "[Thread ...exited]" notifications
authorPedro Alves <pedro@palves.net>
Mon, 12 Dec 2022 20:31:00 +0000 (20:31 +0000)
committerAndrew Burgess <aburgess@redhat.com>
Wed, 23 Aug 2023 08:57:38 +0000 (09:57 +0100)
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 <exit_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 <aburgess@redhat.com>
18 files changed:
gdb/annotate.c
gdb/breakpoint.c
gdb/fbsd-nat.c
gdb/gdbthread.h
gdb/inferior.c
gdb/interps.c
gdb/interps.h
gdb/linux-nat.c
gdb/mi/mi-interp.c
gdb/mi/mi-interp.h
gdb/netbsd-nat.c
gdb/observable.h
gdb/procfs.c
gdb/python/py-inferior.c
gdb/testsuite/gdb.mi/mi-thread-bp-deleted.exp
gdb/testsuite/gdb.threads/thread-bp-deleted.exp
gdb/thread.c
gdb/windows-nat.c

index d403a47ba2f23efad0d629a15fa6f6965afde404..8385429042d3e12db00cad1904ba52aa94785092 100644 (file)
@@ -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<ULONGEST> exit_code,
+                       bool /* silent */)
 {
   if (annotation_level > 1)
     {
index f0f5328fb5ea2aceec71c2054849a65713f04ef0..c429af455fffe5232ffb454207ae5ae6bd8a97d4 100644 (file)
@@ -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<ULONGEST> /* exit_code */,
+                            int /* silent */)
 {
   for (breakpoint &b : all_breakpoints_safe ())
     {
index 41ca1f61561e0ecbf120f17424a8aff2fd574454..0ee3bccb5ef81bff560e3e36fabbaaf57bc85151 100644 (file)
@@ -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--;
index baff68aee925af8c81edcdadcf96d3a60b5358c7..48f32bb3a0bc72290c85e406aa7b1b672bacb822 100644 (file)
@@ -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<ULONGEST> exit_code = {},
+                              bool silent = false);
 
 /* Delete a step_resume_breakpoint from the thread database.  */
 extern void delete_step_resume_breakpoint (struct thread_info *);
index cf4caa923cfe81d7375be6e1f6003ec627e68eec..ce4960a508aa2ce79afa85d3bbc15768717f6d75 100644 (file)
@@ -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;
     });
index f665e86b65c751c4cd6b003677c37a5760cd464c..7baa8491eb198ac19be25c2ea6efa991dcbaffb8 100644 (file)
@@ -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<ULONGEST> 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.  */
index 3a0e8372e10e4a68bf59bf2213047702605ca976..c041d0d95b610aa69b099dbbdaf016dee5ce0450 100644 (file)
@@ -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<ULONGEST> 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<ULONGEST> exit_code,
+                                         int silent);
 
 /* Notify all interpreters that inferior INF was added.  */
 extern void interps_notify_inferior_added (inferior *inf);
index 250a8f43282b6f83502405489e1f4ddba9e985cd..e58d67183b5237d61f301c636cd3f903f1d01ffa 100644 (file)
@@ -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);
 }
index a7fcf17e51c2fb44e10f0d570c893d084f1a5634..1eff4c286ea3af486b210f87157ec3014f334ebb 100644 (file)
@@ -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<ULONGEST> /* exit_code */,
+                            int /* silent */)
 {
   target_terminal::scoped_restore_terminal_state term_state;
   target_terminal::ours_for_output ();
index 6326ec7d524f9c222754283a9ced0946f31b335e..bc975d75ad81cf6a1691a1f4e2a376db2af2f487 100644 (file)
@@ -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<ULONGEST> 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;
index 86df0c8fe6a011ff18696e0e923cfd07326adc96..d1614ec07839b0d9e7c6e6f36f585522f0c92561 100644 (file)
@@ -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);
        }
 
index f6047a239cd63760f6f4cb9b53e5c65d644afb38..c0bafc51f143c2198ac201a871dee00ba618da92 100644 (file)
@@ -109,10 +109,13 @@ extern observable<struct objfile */* objfile */> free_objfile;
 /* The thread specified by T has been created.  */
 extern observable<struct thread_info */* t */> 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<struct thread_info */* t */, int /* silent */> 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<thread_info */* t */,
+                 gdb::optional<ULONGEST> /* 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
index f0c2c2cf56c3b0454a80310e012c786535ec48e7..d55a16a9fb53cafdafc2ef7a244772c0c47048c1 100644 (file)
@@ -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;
index 792f05b118e46c011179aef8d0f510a48e92659d..906d402827ce4b45d38f72cca75a0bdb5ea7fec9 100644 (file)
@@ -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<ULONGEST> /* exit_code */,
+                     bool /* silent */)
 {
   if (!gdb_python_initialized)
     return;
index 0ebca9248015be19ee42ea0cd9a7da5ba27e5641..bffbb2900719cdc1d0b824344932f99dfda0d99d 100644 (file)
@@ -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
        }
     }
 
index 019bdddee818b1562ca82d46ff2b1469b56cb4eb..92178ff838b188783789f9701b6322ce212a927d 100644 (file)
@@ -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 {
index 2cb9e5e36d0eaec0fffc34ba6d8db41b15325724..c8145da59bcda99174389ae8c235cceec95d80b8 100644 (file)
@@ -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<ULONGEST> 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<ULONGEST> 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<ULONGEST> 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)
+                        : "<none>"),
+                       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 *
index c09e459a80197a3a35c5607c723912434f826cfa..d8b33452f467e9b3d686e0719ac5f193f017b1d5 100644 (file)
@@ -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 (),