Re: [PATCH 30/31] Centralize "[Thread ...exited]" notifications

[Andrew Burgess <aburgess@redhat.com>]

Pedro Alves <pedro@palves.net> writes:

> 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).

You mention gdb.threads/attach-many-short-lived-threads.exp as an
example, but I don't think that test actually fails due to this issue,
right?

It might be worth mentioning gdb.threads/thread-specific-bp.exp, that is
a test script that should actually start passing after this commit.

Thanks,
Andrew

>
> Change-Id: I06ec07b7c51527872a9713dd11cf7867b50fc5ff
> ---
>  gdb/annotate.c           |  4 +++-
>  gdb/breakpoint.c         |  4 +++-
>  gdb/fbsd-nat.c           |  3 ---
>  gdb/gdbthread.h          | 22 +++++++++++++----
>  gdb/inferior.c           |  2 +-
>  gdb/inferior.h           |  2 ++
>  gdb/linux-nat.c          | 11 +++------
>  gdb/mi/mi-interp.c       |  8 +++++--
>  gdb/netbsd-nat.c         |  4 ----
>  gdb/observable.h         | 11 +++++----
>  gdb/procfs.c             |  6 -----
>  gdb/python/py-inferior.c |  4 +++-
>  gdb/thread.c             | 51 ++++++++++++++++++++++++++++++----------
>  gdb/windows-nat.c        | 16 ++++---------
>  14 files changed, 89 insertions(+), 59 deletions(-)
>
> diff --git a/gdb/annotate.c b/gdb/annotate.c
> index 33805dcdb30..b45384ddb15 100644
> --- a/gdb/annotate.c
> +++ b/gdb/annotate.c
> @@ -233,7 +233,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)
>      {
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index f0276a963c0..0736231e470 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -3237,7 +3237,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,
> +			     bool /* silent */)
>  {
>    for (breakpoint *b : all_breakpoints_safe ())
>      {
> diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
> index 1aec75050ae..3d1e742f4e3 100644
> --- a/gdb/fbsd-nat.c
> +++ b/gdb/fbsd-nat.c
> @@ -1300,9 +1300,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);
>  		}
> diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
> index 43e9d6ea484..7ab02873f17 100644
> --- a/gdb/gdbthread.h
> +++ b/gdb/gdbthread.h
> @@ -636,16 +636,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 *);
> diff --git a/gdb/inferior.c b/gdb/inferior.c
> index eacb65ec1d7..834eabdf2ca 100644
> --- a/gdb/inferior.c
> +++ b/gdb/inferior.c
> @@ -174,7 +174,7 @@ inferior::clear_thread_list ()
>      {
>        threads_debug_printf ("deleting thread %s",
>  			    thr->ptid.to_string ().c_str ());
> -      set_thread_exited (thr, true);
> +      set_thread_exited (thr, {}, true);
>        if (thr->deletable ())
>  	delete thr;
>      });
> diff --git a/gdb/inferior.h b/gdb/inferior.h
> index 07d9527a802..cf3f1275cc1 100644
> --- a/gdb/inferior.h
> +++ b/gdb/inferior.h
> @@ -628,6 +628,8 @@ extern void detach_inferior (inferior *inf);
>  
>  extern void exit_inferior (inferior *inf);
>  
> +/* Like exit_inferior, but be quiet -- don't announce the exit of the
> +   inferior's threads to the CLI.  */
>  extern void exit_inferior_silent (inferior *inf);
>  
>  extern void exit_inferior_num_silent (int num);
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index 75f81edf20a..acf5fd3f1b1 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -916,15 +916,10 @@ linux_nat_switch_fork (ptid_t new_ptid)
>  static void
>  exit_lwp (struct lwp_info *lp, bool del_thread = true)
>  {
> -  struct thread_info *th = find_thread_ptid (linux_target, lp->ptid);
> -
> -  if (th)
> +  if (del_thread)
>      {
> -      if (print_thread_events)
> -	gdb_printf (_("[%s exited]\n"),
> -		    target_pid_to_str (lp->ptid).c_str ());
> -
> -      if (del_thread)
> +      thread_info *th = find_thread_ptid (linux_target, lp->ptid);
> +      if (th != nullptr)
>  	delete_thread (th);
>      }
>  
> diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
> index 3cc2462f672..189dd1f302f 100644
> --- a/gdb/mi/mi-interp.c
> +++ b/gdb/mi/mi-interp.c
> @@ -68,7 +68,9 @@ static void mi_on_normal_stop (struct bpstat *bs, int print_frame);
>  static void mi_on_no_history (void);
>  
>  static void mi_new_thread (struct thread_info *t);
> -static void mi_thread_exit (struct thread_info *t, int silent);
> +static void mi_thread_exit (thread_info *t,
> +			    gdb::optional<ULONGEST> exit_code,
> +			    bool silent);
>  static void mi_record_changed (struct inferior*, int, const char *,
>  			       const char *);
>  static void mi_inferior_added (struct inferior *inf);
> @@ -351,8 +353,10 @@ mi_new_thread (struct thread_info *t)
>      }
>  }
>  
> +/* Observer for the thread_exit notification.  */
> +
>  static void
> -mi_thread_exit (struct thread_info *t, int silent)
> +mi_thread_exit (thread_info *t, gdb::optional<ULONGEST> exit_code, bool silent)
>  {
>    SWITCH_THRU_ALL_UIS ()
>      {
> diff --git a/gdb/netbsd-nat.c b/gdb/netbsd-nat.c
> index aa16a6cc5bd..9674baeb846 100644
> --- a/gdb/netbsd-nat.c
> +++ b/gdb/netbsd-nat.c
> @@ -625,10 +625,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 ());
>  	}
>  
>        /* The GDB core expects that the rest of the threads are running.  */
> diff --git a/gdb/observable.h b/gdb/observable.h
> index 1103c5c98a6..a4ab4f1e38f 100644
> --- a/gdb/observable.h
> +++ b/gdb/observable.h
> @@ -126,10 +126,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
> diff --git a/gdb/procfs.c b/gdb/procfs.c
> index ffc26c8fb9e..7d0e6e9a4c9 100644
> --- a/gdb/procfs.c
> +++ b/gdb/procfs.c
> @@ -2115,9 +2115,6 @@ procfs_target::wait (ptid_t ptid, struct target_waitstatus *status,
>  	      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 (find_thread_ptid (this, retval));
>  		    target_continue_no_signal (ptid);
>  		    goto wait_again;
> @@ -2222,9 +2219,6 @@ procfs_target::wait (ptid_t ptid, struct target_waitstatus *status,
>  		  }
>  		else if (what == SYS_lwp_exit)
>  		  {
> -		    if (print_thread_events)
> -		      gdb_printf (_("[%s exited]\n"),
> -				  target_pid_to_str (retval).c_str ());
>  		    delete_thread (find_thread_ptid (this, retval));
>  		    status->set_spurious ();
>  		    return retval;
> diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
> index 4d5e09db680..be5597c4a2e 100644
> --- a/gdb/python/py-inferior.c
> +++ b/gdb/python/py-inferior.c
> @@ -360,7 +360,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;
> diff --git a/gdb/thread.c b/gdb/thread.c
> index 2ca3a867d8c..7ab30562fd3 100644
> --- a/gdb/thread.c
> +++ b/gdb/thread.c
> @@ -192,7 +192,8 @@ clear_thread_inferior_resources (struct thread_info *tp)
>  /* 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))
> @@ -211,7 +212,22 @@ set_thread_exited (thread_info *tp, bool silent)
>        if (proc_target != nullptr)
>  	proc_target->maybe_remove_resumed_with_pending_wait_status (tp);
>  
> -      gdb::observers::thread_exit.notify (tp, silent);
> +      if (!silent && print_thread_events)
> +	{
> +	  if (exit_code.has_value ())
> +	    {
> +	      gdb_printf (_("[%s exited with code %s]\n"),
> +			  target_pid_to_str (tp->ptid).c_str (),
> +			  pulongest (*exit_code));
> +	    }
> +	  else
> +	    {
> +	      gdb_printf (_("[%s exited]\n"),
> +			  target_pid_to_str (tp->ptid).c_str ());
> +	    }
> +	}
> +
> +      gdb::observers::thread_exit.notify (tp, exit_code, silent);
>  
>        /* Tag it as exited.  */
>        tp->state = THREAD_EXITED;
> @@ -468,20 +484,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 ())
>      {
> @@ -497,16 +515,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, false /* not 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 ee4e78bdabf..2764fc694b3 100644
> --- a/gdb/windows-nat.c
> +++ b/gdb/windows-nat.c
> @@ -611,21 +611,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
> +  /* 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 (find_thread_ptid (this, ptid));
> +  bool silent = (main_thread_p && !info_verbose);
> +  thread_info *todel = find_thread_ptid (this, ptid);
> +  delete_thread_with_exit_code (todel, exit_code, silent);
>  
>    auto iter = std::find_if (windows_process.thread_list.begin (),
>  			    windows_process.thread_list.end (),
> -- 
> 2.36.0