[PATCH] gdb: Restore previously selected thread when switching inferior

[Simon Marchi]

On 2020-03-27 9:30 a.m., Andrew Burgess wrote:
> This commit changes how GDB behaves when switching between
> multi-threaded inferiors.
> 
> Previously when switching GDB would select "any" thread from the
> inferior being switched to.  In practice this usually means the thread
> with the lowest thread number.
> 
> After this commit GDB remembers which thread the user had selected
> within the inferior and tries to reselect that thread when switching
> back to an inferior.  The assumption is that the selected thread is
> the one that the user is most interested in, and so switching back to
> that makes more sense than selecting some other 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 making this behaviour switchable, but the new behaviour
> seems (to me) obviously better, so I haven't added a switch for it.
> However, if people feel the old behaviour is worth preserving then I'm
> happy to add a switch.
> 
> The other thing I considered, but didn't do is to add 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, but again, if people feel strongly
> that there should be a warning, I'm happy to add one.
> 
> One test needed to be updated after this change, and there's a new
> test that covers this behaviour in more depth.

I think it's a good idea.  As a user I wouldn't be surprised with that behavior
(though I'd be suprised that the thread is saved when switching inferiors, but
the frame is not saved when switching thread).

We might get some reports that changing this behavior breaks some scripts... that's
the only argument against I can think of.  That might be a reason to add a switch
to toggle to old behavior (I would keep the new behavior as the default, since it's
user friendly), at least it would give an escape hatch for people to make their
scripts work as before.  I don't really know if that's necessary though.

Since it requires running a process, I believe the test should be skipped if
[use_gdb_stub] returns true.

> diff --git a/gdb/inferior.h b/gdb/inferior.h
> index 4229c6017d9..2cc6c8bd09f 100644
> --- a/gdb/inferior.h
> +++ b/gdb/inferior.h
> @@ -536,6 +536,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, and records the current INFERIOR_PTID value
> +     before switching to the new inferior.
> +
> +     This value is then used when switching back to this inferior to try
> +     and restore the value of INFERIOR_PTID.  If the thread represented by
> +     INFERIOR_PTID no longer exists when switching back to this inferior
> +     then GDB will select some other thread from this inferior.  */
> +  ptid_t previous_inferior_ptid;

To be safe, I think this should be explicitly initialized to null_ptid, since
ptid_t has:

  /* Must have a trivial defaulted default constructor so that the
     type remains POD.  */
  ptid_t () noexcept = default;

> +/* Main program.  */
> +int
> +main ()
> +{
> +  sigset_t set;
> +  int i, max = thread_count;
> +
> +  /* Set an alarm in case the testsuite crashes, don't leave the test
> +     running forever.  */
> +  alarm (300);
> +
> +  struct thread_ctrl *info = malloc (sizeof (struct thread_ctrl) * max);
> +  if (info == NULL)
> +    abort ();
> +
> +  /* Put the pid somewhere easy for GDB to read, also print it.  */
> +  global_pid = getpid ();
> +  printf ("pid = %lld\n", ((long long) global_pid));
> +
> +  /* Block SIGUSR1, all threads will inherit this sigmask. */
> +  sigemptyset (&set);
> +  sigaddset (&set, SIGUSR1);
> +  if (pthread_sigmask (SIG_BLOCK, &set, NULL))
> +    abort ();
> +
> +  /* Create each thread.  */
> +  for (i = 0; i < max; ++i)
> +    {
> +      struct thread_ctrl *thr = &info[i];
> +      thread_ctrl_init (thr, i + 1);
> +
> +      if (pthread_create (&thr->thread, NULL, thread_worker, &thr->info) != 0)
> +	abort ();
> +
> +      /* Wait for an indication that the thread has started, and is ready
> +	 for action.  */
> +      wait_on_byte (&thr->fds);
> +    }
> +
> +  printf ("All threads created.\n");
> +
> +  /* Give thread thread #1 a little nudge.  */

Here you say thread #1, in `thread_worker` you say thread #2?

> +# Check that the current thread is THR in inferior INF.
> +proc check_current_thread { inf thr {testname ""} } {
> +    if {${testname} == ""} {
> +	set testname "check_current_thread ${inf} ${thr}"
> +    }
> +
> +    # As a final check, lets check the output for the 'thread'
> +    # command.
> +    gdb_test "thread" "Current thread is ${inf}.${thr} .*" \
> +	"current thread is ${inf}.${thr}: $testname"
> +}
> +
> +# Switch to inferior number INF, we expect that thread number THR
> +# within the inferior will be selected.
> +proc switch_to_inferior { inf thr {testname ""} } {

I'd name this one switch_to_inferior_and_check, otherwise at the top-level it
just sounds like you are having fun switching between inferiors and threads without
checking anything.

> +    if {${testname} == ""} {
> +	set testname "switch_to_inferior $inf $thr"
> +    }
> +
> +    gdb_test "inferior $inf" \
> +	"Switching to inferior ${inf} .*Switching to thread ${inf}.${thr} .*" \
> +	"$testname: select inferior ${inf}"
> +
> +    check_current_thread $inf $thr "$testname: check current thread"
> +}
> +
> +# Switch to thread number THR.  INF should be the number of the
> +# currently selected inferior and is used when checking the currently
> +# selected thread.
> +proc switch_to_thread { inf thr {testname ""} } {

And this one to switch_to_thread_and_check.

Simon