RE: [PATCHv4 1/2] gdb: Restore previously selected thread when switching inferior

["Aktemur, Tankut Baris via Gdb-patches" <gdb-patches@sourceware.org>]

On Saturday, November 7, 2020 12:02 AM, Andrew Burgess wrote:
> This commit adds a new option that allows the user to control how GDB
> behaves when switching between multi-threaded inferiors.
> 
> Currently (and this remains the default after this commit) when
> switching between inferiors GDB would select the first non-exited
> thread from the inferior being switched to.
> 
> This commit adds the following new commands:
> 
>      set restore-selected-thread on|off
>      show restore-selected-thread
> 
> This option is off by default in order to retain the existing
> behaviour, but, when switched on GDB will remember which thread was
> selected in each inferior.  As the user switches between inferiors GDB
> will attempt to restore the previously selected thread.
> 
> If the previously selected thread is no longer available, for example,
> if the thread has exited, then GDB will fall back on the old
> behaviour.
> 
> I did consider, but eventually didn't implemented, adding a warning
> when switching inferiors if the previously selected thread is no
> longer available.  My reasoning here is that GDB should already have
> informed the user that the thread has exited, and there is already a
> message indicating which thread has been switched too, so adding an
> extra warning felt like unneeded clutter.
> 
> In order to store the thread within the inferior I store a pointer to
> the thread_info object of the previously selected thread.  When
> fetching the thread_info it is important that we do actually have a
> current thread otherwise this happens:
> 
>   $ gdb
>   (gdb) add-inferior
>   (gdb) inferior 2
>   ./gdb/thread.c:95: internal-error: thread_info* inferior_thread(): Assertion
> `current_thread_ != nullptr' failed.
> 
> To avoid this I added a check that inferior_ptid is not null_ptid.
> Though it is not always the case, there are plenty of places in GDB
> where a call to inferior_thread () is guarded by such a check.
> 
> There's a new test for this functionality.
> 
> gdb/ChangeLog:
> 
> 	* inferior.c (inferior_command): Store current thread_info before
> 	switching inferiors.  Reselect the previous thread_info if
> 	possible after switching to the new inferior.
> 	(initialize_inferiors): Register restore-selected-thread option.
> 	* inferior.h (class inferior) <previous_thread_info>: New member
> 	variable.
> 	* NEWS: Mention new feature.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.threads/restore-thread.c: New file.
> 	* gdb.threads/restore-thread.exp: New file.
> 
> gdb/doc/ChangeLog:
> 
> 	* gdb.texinfo (Inferiors Connections and Programs): Mention thread
> 	tracking within the inferior command.
> 	(Threads): Mention thread tracking in the general thread
> 	discussion.
> ---
>  gdb/ChangeLog                                |  10 +
>  gdb/NEWS                                     |   9 +
>  gdb/doc/ChangeLog                            |   7 +
>  gdb/doc/gdb.texinfo                          |  19 +-
>  gdb/inferior.c                               |  58 ++++-
>  gdb/inferior.h                               |  10 +
>  gdb/testsuite/ChangeLog                      |   5 +
>  gdb/testsuite/gdb.threads/restore-thread.c   | 248 +++++++++++++++++++
>  gdb/testsuite/gdb.threads/restore-thread.exp | 219 ++++++++++++++++
>  9 files changed, 583 insertions(+), 2 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.threads/restore-thread.c
>  create mode 100644 gdb/testsuite/gdb.threads/restore-thread.exp
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 3e08aee7c6f..5a900b8a678 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -27,6 +27,15 @@ set debug event-loop
>  show debug event-loop
>    Control the display of debug output about GDB's event loop.
> 
> +set restore-selected-thread on|off
> +show restore-selected-thread
> +  This new option is off by default.  When turned on GDB will record
> +  the currently selected thread in each inferior.  When switching
> +  between inferiors GDB will attempt to restore the previously
> +  selected thread in the inferior being switched too.  If the
> +  previously selected thread is no longer available then GDB falls
> +  back to selecting the first non-exited thread.
> +
>  * Changed commands
> 
>  break [PROBE_MODIFIER] [LOCATION] [thread THREADNUM]
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 52701560006..c5819bde7ae 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -3247,11 +3247,25 @@
>  To switch focus between inferiors, use the @code{inferior} command:
> 
>  @table @code
> +@anchor{inferior command}
>  @kindex inferior @var{infno}
>  @item inferior @var{infno}
>  Make inferior number @var{infno} the current inferior.  The argument
>  @var{infno} is the inferior number assigned by @value{GDBN}, as shown
>  in the first field of the @samp{info inferiors} display.
> +
> +When switching between inferiors with multiple threads
> +(@pxref{Threads}) @value{GDBN} will select the first non-exited thread
> +in the inferior being switched to and make this the current thread.
> +
> +@kindex set restore-selected-thread
> +@kindex show restore-selected-thread
> +@item set restore-selected-thread @r{[}on|off@r{]}
> +@item show restore-selected-thread
> +When this option is on @value{GDBN} will record the currently selected
> +thread in each inferior.  When switching between inferior @value{GDBN}
> +will try to restore the previously selected thread in the inferior
> +being switched to.  This option is off by default.
>  @end table
> 
>  @vindex $_inferior@r{, convenience variable}
> @@ -3633,7 +3647,10 @@
> 
>  If you're debugging multiple inferiors, @value{GDBN} displays thread
>  IDs using the qualified @var{inferior-num}.@var{thread-num} format.
> -Otherwise, only @var{thread-num} is shown.
> +Otherwise, only @var{thread-num} is shown.  When switching between
> +inferiors @value{GDBN} will select a suitable thread in the inferior
> +being switched to, see @ref{inferior command,,the @code{inferior}
> +command} for further details on how to control this behaviour.
> 
>  If you specify the @samp{-gid} option, @value{GDBN} displays a column
>  indicating each thread's global thread ID:
> diff --git a/gdb/inferior.c b/gdb/inferior.c
> index d4a783b3e6d..4961bc467fd 100644
> --- a/gdb/inferior.c
> +++ b/gdb/inferior.c
> @@ -631,6 +631,23 @@ switch_to_inferior_no_thread (inferior *inf)
>    set_current_program_space (inf->pspace);
>  }
> 
> +/* When this is true GDB restores the inferiors previously selected thread

"inferiors" -> "inferior's".
I also think that a comma is need after "true".

> +   each time the inferior is changed (where possible).  */
> +
> +static bool restore_selected_thread_per_inferior = false;
> +
> +/* Implement 'show restore-selected-thread'.  */
> +
> +static void
> +show_restore_selected_thread_per_inferior (struct ui_file *file, int from_tty,
> +					   struct cmd_list_element *c,
> +					   const char *value)
> +{
> +  fprintf_filtered (file,
> +		    _("Restoring the selected thread is currently %s.\n"),
> +		    value);
> +}
> +
>  static void
>  inferior_command (const char *args, int from_tty)
>  {
> @@ -643,11 +660,38 @@ inferior_command (const char *args, int from_tty)
>    if (inf == NULL)
>      error (_("Inferior ID %d not known."), num);
> 
> +  /* We can only call INFERIOR_THREAD if the inferior is known to have an
> +     active thread, which it wont if the inferior is currently exited.  So,
> +     first check if we currently have a thread selected.  */
> +  if (inferior_ptid != null_ptid)
> +    {
> +      /* Now take a strong reference to the current thread_info and store
> +	 it within the inferior, this prevents the thread_info from being
> +	 deleted until the inferior has released the reference.  */
> +      thread_info *tp = inferior_thread ();
> +      tp->incref ();
> +      current_inferior ()->previous_thread_info.reset (tp);
> +    }
> +
>    if (inf->pid != 0)
>      {
>        if (inf != current_inferior ())
>  	{
> -	  thread_info *tp = any_thread_of_inferior (inf);
> +	  thread_info *tp = nullptr;
> +
> +	  if (restore_selected_thread_per_inferior
> +	      && inf->previous_thread_info != nullptr)
> +	    {
> +	      /* Release the reference to the previous thread.  We don't
> +		 switch back to this thread if it is already exited
> +		 though.  */
> +	      tp = inf->previous_thread_info.release ();
> +	      tp->decref ();
> +	      if (tp->state == THREAD_EXITED)
> +		tp = nullptr;
> +	    }
> +	  if (tp == nullptr)
> +	    tp = any_thread_of_inferior (inf);
>  	  if (tp == NULL)
>  	    error (_("Inferior has no threads."));
> 
> @@ -1025,5 +1069,17 @@ Show printing of inferior events (such as inferior start and
> exit)."), NULL,
>  	 show_print_inferior_events,
>  	 &setprintlist, &showprintlist);
> 
> +  add_setshow_boolean_cmd ("restore-selected-thread",
> +			   no_class, &restore_selected_thread_per_inferior,
> +                          _("\
> +Set whether GDB restores the selected thread when switching inferiors."), _("\
> +Show whether GDB restores the selected thread when switching inferiors."), _("\
> +When this option is on GDB will record the currently selected thread for\n\

A comma after "on" could improve the sentence, IMHO.

> +each inferior, and restore the selected thread whenever GDB switches inferiors."),
> +                          nullptr,
> +                          show_restore_selected_thread_per_inferior,
> +                          &setlist,
> +                          &showlist);
> +
>    create_internalvar_type_lazy ("_inferior", &inferior_funcs, NULL);
>  }
> diff --git a/gdb/inferior.h b/gdb/inferior.h
> index d016161fb05..517180e48e2 100644
> --- a/gdb/inferior.h
> +++ b/gdb/inferior.h
> @@ -543,6 +543,16 @@ class inferior : public refcounted_object
>    /* Data related to displaced stepping.  */
>    displaced_step_inferior_state displaced_step_state;
> 
> +  /* This field is updated when GDB switches away from this inferior to
> +     some other inferior, a reference to a thread_info is stored in here,

It feels like there should be a period after "inferior", instead of a comma, to
end the sentence.

Also, the comment "a reference to a thread_info is stored in here..." is possibly
redundant, because the type "thread_info_ref" implies this.

> +     the ref count for the thread_info should be non-zero to prevent the
> +     thread_info being deleted.
> +
> +     When the user switches back to this inferior the thread_info is taken
> +     out of this reference and used to (possibly) switch back to this
> +     thread.  */
> +  thread_info_ref previous_thread_info;
> +
>    /* Per inferior data-pointers required by other GDB modules.  */
>    REGISTRY_FIELDS;
> 

Thanks.
-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928