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

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Andrew Burgess
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.

gdb/ChangeLog:

        * inferior.c (inferior_command): Store current inferior_ptid
        before switching inferiors.  Reselect the previous inferior_ptid
        if possible after switching to the new inferior.
        * inferior.h (class inferior) <previous_inferior_ptid>: New member
        variable.
        * NEWS: Mention new feature.

gdb/testsuite/ChangeLog:

        * gdb.mi/user-selected-context-sync.exp: Update expected results.
        * 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                                      |   9 +
 gdb/NEWS                                           |   4 +
 gdb/doc/ChangeLog                                  |   7 +
 gdb/doc/gdb.texinfo                                |  15 ++
 gdb/inferior.c                                     |  11 +-
 gdb/inferior.h                                     |  10 +
 gdb/testsuite/ChangeLog                            |   6 +
 .../gdb.mi/user-selected-context-sync.exp          |   1 +
 gdb/testsuite/gdb.threads/restore-thread.c         | 246 +++++++++++++++++++++
 gdb/testsuite/gdb.threads/restore-thread.exp       | 217 ++++++++++++++++++
 10 files changed, 525 insertions(+), 1 deletion(-)
 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 6657f6fadce..63aabeed2e5 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -42,6 +42,10 @@
 * On Windows targets, it is now possible to debug 32-bit programs with a
   64-bit GDB.
 
+* GDB now keeps track of the selected thread in each inferior, and
+  will restore the selected thread when switching inferior (if
+  possible).
+
 * New commands
 
 set exec-file-mismatch -- Set exec-file-mismatch handling (ask|warn|off).
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 385c832f222..2373c515c29 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -3095,11 +3095,18 @@
 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 previously selected
+thread for that inferior.  If this is not possible, for example, if
+the thread has exited, then @value{GDBN} will select a random thread
+within the inferior.
 @end table
 
 @vindex $_inferior@r{, convenience variable}
@@ -3431,6 +3438,14 @@
 Thread 1 "main" received signal SIGINT, Interrupt.
 @end smallexample
 
+If you're debugging multiple inferiors, @value{GDBN} keeps track of
+which thread is selected within each inferior.  When you switch
+between inferiors (@pxref{inferior command,,the @code{inferior}
+command}) @value{GDBN} will restore the inferior's previously selected
+thread where possible.  If the thread can't be selected, for example,
+if the thread has exited, then @value{GDBN} will select a random
+thread from the inferior.
+
 @table @code
 @kindex info threads
 @item info threads @r{[}@var{thread-id-list}@r{]}
diff --git a/gdb/inferior.c b/gdb/inferior.c
index c2e9da3d3d5..523dccde085 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -636,11 +636,20 @@ inferior_command (const char *args, int from_tty)
   if (inf == NULL)
     error (_("Inferior ID %d not known."), num);
 
+  /* Preserve the current thread within this inferior so we can restore it
+     in the future when we switch back to this inferior.  */
+  current_inferior ()->previous_inferior_ptid = inferior_ptid;
+
   if (inf->pid != 0)
     {
       if (inf != current_inferior ())
  {
-  thread_info *tp = any_thread_of_inferior (inf);
+  thread_info *tp = nullptr;
+
+  if (inf->previous_inferior_ptid != null_ptid)
+    tp = find_thread_ptid (inf, inf->previous_inferior_ptid);
+  if (tp == nullptr)
+    tp = any_thread_of_inferior (inf);
   if (tp == NULL)
     error (_("Inferior has no threads."));
 
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;
+
   /* Per inferior data-pointers required by other GDB modules.  */
   REGISTRY_FIELDS;
 
diff --git a/gdb/testsuite/gdb.mi/user-selected-context-sync.exp b/gdb/testsuite/gdb.mi/user-selected-context-sync.exp
index 390df005542..779ed18e1ae 100644
--- a/gdb/testsuite/gdb.mi/user-selected-context-sync.exp
+++ b/gdb/testsuite/gdb.mi/user-selected-context-sync.exp
@@ -533,6 +533,7 @@ proc match_re_or_ensure_not_output { re test } {
 proc_with_prefix test_cli_inferior { mode } {
     global gdb_main_spawn_id mi_spawn_id
 
+    reset_selection "2.1"
     reset_selection "1.1"
 
     set mi_re [make_mi_re $mode 4 2 event]
diff --git a/gdb/testsuite/gdb.threads/restore-thread.c b/gdb/testsuite/gdb.threads/restore-thread.c
new file mode 100644
index 00000000000..fe2c13ac429
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/restore-thread.c
@@ -0,0 +1,246 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2020 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <pthread.h>
+#include <signal.h>
+#include <sys/types.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <pthread.h>
+#include <errno.h>
+
+/* The number of threads to create.  */
+volatile int thread_count = 3;
+
+/* Is initialised with our pid, GDB will read this.  */
+pid_t global_pid;
+
+/* Holds one end of two different pipes.  Things written to READ will not
+   appear on WRITE.  */
+struct pipe_fds
+{
+  int read;
+  int write;
+};
+
+/* Information passed into each thread.  */
+struct thread_info
+{
+  /* Just a numeric id for the thread.  */
+  int id;
+
+  /* File handles with which the worker thread can communicate with the
+     master thread.  */
+  struct pipe_fds fds;
+};
+
+/* The control information held by the master thread, one of these for each
+   worker thread.  */
+struct thread_ctrl
+{
+  /* The actual pthread handle, used to join the threads.  */
+  pthread_t thread;
+
+  /* File handles with which the master thread can communicate with the
+     worker threads.  */
+  struct pipe_fds fds;
+
+  /* The information that is passed into the worker thread.  */
+  struct thread_info info;
+};
+
+/* Wait for a single byte of the read file handle in FDS.  */
+static void
+wait_on_byte (struct pipe_fds *fds)
+{
+  ssize_t rtn;
+  char c;
+
+  while ((rtn = read (fds->read, &c, 1)) != 1)
+    {
+      if (rtn != -1 || errno != EINTR)
+ abort ();
+    }
+}
+
+/* Send a single byte to the write file handle in FDS.  */
+static void
+send_byte (struct pipe_fds *fds)
+{
+  ssize_t rtn;
+  char c = 'x';
+  while ((rtn = write (fds->write, &c, 1)) != 1)
+    {
+      if (rtn != -1 || errno != EINTR)
+ abort ();
+    }
+}
+
+/* Create a function used to mark a breakpoint location.  */
+#define BREAKPOINT_FUNC(N) \
+  static void \
+  breakpt_ ## N () \
+  { \
+    printf ("Hit breakpt_" #N "\n"); \
+  }
+
+BREAKPOINT_FUNC (0) /* breakpt_0 */
+BREAKPOINT_FUNC (1) /* breakpt_1 */
+BREAKPOINT_FUNC (2) /* breakpt_2 */
+
+/* The worker thread entry point.  */
+static void *
+thread_worker (void *arg)
+{
+  struct thread_info *info = (struct thread_info *) arg;
+  int id = info->id;
+
+  printf ("Thread %d created.\n", id);
+  breakpt_0 ();
+
+  /* Let the main thread know that this thread is now running.  */
+  send_byte (&info->fds);
+
+  /* The thread with id #2 is special, it waits here for a nudge from the
+     main thread.  */
+  if (id == 2)
+    {
+      wait_on_byte (&info->fds);
+      breakpt_2 ();
+      send_byte (&info->fds);
+    }
+
+  /* Now wait for an incoming message indicating that the thread should
+     exit.  */
+  wait_on_byte (&info->fds);
+  printf ("In thread %d, exiting...\n", id);
+  return NULL;
+}
+
+/* Initialise CTRL for thread ID, this includes setting up all of the pipe
+   file handles.  */
+static void
+thread_ctrl_init (struct thread_ctrl *ctrl, int id)
+{
+  int fds[2];
+
+  ctrl->info.id = id;
+  if (pipe (fds))
+    abort ();
+  ctrl->info.fds.read = fds[0];
+  ctrl->fds.write = fds[1];
+
+  if (pipe (fds))
+    abort ();
+  ctrl->fds.read = fds[0];
+  ctrl->info.fds.write = fds[1];
+}
+
+/* Wait for a SIGUSR1 to arrive.  Assumes that SIGUSR1 is blocked on entry
+   to this function.  */
+static void
+wait_for_sigusr1 (void)
+{
+  int signo;
+  sigset_t set;
+
+  sigemptyset (&set);
+  sigaddset (&set, SIGUSR1);
+
+  /* Wait for a SIGUSR1.  */
+  if (sigwait (&set, &signo) != 0)
+    abort ();
+  if (signo != SIGUSR1)
+    abort ();
+}
+
+/* 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.  */
+  if (max >= 2)
+    {
+      send_byte (&info[1].fds);
+      wait_on_byte (&info[1].fds);
+    }
+
+  breakpt_1 ();
+
+  /* For each thread in turn wait for a SIGUSR1 to arrive, signal the
+     thread so that it will exit (by sending it a byte down its pipe), then
+     join the newly exited thread.  */
+  for (i = 0; i < max; ++i)
+    {
+      struct thread_ctrl *thr = &info[i];
+
+      wait_for_sigusr1 ();
+
+      printf ("Telling thread %d to exit\n", thr->info.id);
+      send_byte (&thr->fds);
+
+      if (pthread_join (thr->thread, NULL) != 0)
+ abort ();
+
+      printf ("Thread %d exited\n", thr->info.id);
+    }
+
+  free (info);
+
+  /* Final wait before exiting.  */
+  wait_for_sigusr1 ();
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.threads/restore-thread.exp b/gdb/testsuite/gdb.threads/restore-thread.exp
new file mode 100644
index 00000000000..b66d412b292
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/restore-thread.exp
@@ -0,0 +1,217 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2020 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test GDB's ability to restore the selected thread when switching
+# between inferiors, and check what happens when the selected thread
+# of one inferior exits while we have a different inferior selected.
+
+standard_testfile
+
+if [prepare_for_testing "failed to prepare" $binfile $srcfile \
+ {debug pthreads}] {
+    return -1
+}
+
+# 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 ""} } {
+    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 ""} } {
+    if {${testname} == ""} {
+ set testname "switch_to_thread $inf $thr"
+    }
+
+    gdb_test "thread ${thr}" \
+ "Switching to thread ${inf}.${thr} .*" \
+ "${testname}: select thread ${thr}"
+    check_current_thread $inf $thr \
+ "${testname}: check current thread"
+}
+
+# Continue the program in the background.
+proc continue_in_bg { testname } {
+    global gdb_prompt
+
+    gdb_test_multiple "continue&" $testname {
+ -re "Continuing\\.\r\n$gdb_prompt " {
+    pass $gdb_test_name
+ }
+    }
+}
+
+# Send SIGUSR1 to PID, this will cause one of that processes threads
+# to exit (assuming the process is currently running).
+proc send_thread_exit_signal { pid } {
+    global decimal
+
+    remote_exec target "kill -USR1 ${pid}"
+    gdb_test_multiple "" "wait for thread to exit" {
+ -re "Thread $decimal exited.*exited\\\].*" {
+ }
+    }
+}
+
+# Start of test script.
+
+set pid_1 0
+set pid_2 0
+
+if ![runto_main] {
+    return -1
+}
+
+gdb_breakpoint "breakpt_0"
+gdb_breakpoint "breakpt_1"
+
+with_test_prefix "start inferior 1" {
+    gdb_continue_to_breakpoint "created thread 1.2" ".* breakpt_0 .*"
+    gdb_continue_to_breakpoint "created thread 1.3" ".* breakpt_0 .*"
+    gdb_continue_to_breakpoint "created thread 1.4" ".* breakpt_0 .*"
+    gdb_continue_to_breakpoint "all inferior 1 threads created" \
+ ".* breakpt_1 .*"
+    gdb_test "info threads" ".*"
+    set pid_1 [get_valueof "/d" "global_pid" 0]
+}
+
+# Start another inferior.
+gdb_test "add-inferior" [multi_line \
+     "\\\[New inferior 2\\\]" \
+     "Added inferior 2 .*" ] \
+    "add empty inferior 2"
+gdb_test "inferior 2" "Switching to inferior 2.*" \
+    "switch to inferior 2"
+gdb_test "file ${binfile}" ".*" "load file in inferior 2"
+
+with_test_prefix "start inferior 2" {
+    gdb_breakpoint "breakpt_2"
+    gdb_run_cmd
+    gdb_test "" "hit Breakpoint .*" \
+ "runto breakpoint in main"
+    gdb_continue_to_breakpoint "created thread 2.2" ".* breakpt_0 .*"
+    gdb_continue_to_breakpoint "created thread 2.3" ".* breakpt_0 .*"
+    gdb_continue_to_breakpoint "created thread 2.4" ".* breakpt_0 .*"
+    gdb_continue_to_breakpoint "all inferior 2 threads created" \
+ ".* breakpt_2 .*"
+    gdb_test "info threads" ".*"
+    set pid_2 [get_valueof "/d" "global_pid" 0]
+}
+
+gdb_assert {${pid_1} != 0} "read the pid for inferior 1"
+gdb_assert {${pid_2} != 0} "read the pid for inferior 2"
+
+check_current_thread 2 3 "check initial thread is 2.3"
+switch_to_inferior 1 1 "first switch to thread 1.1"
+switch_to_inferior 2 3
+switch_to_thread 2 2
+
+switch_to_inferior 1 1 "second switch to thread 1.1"
+switch_to_thread 1 3
+switch_to_inferior 2 2
+
+# Inferior 2 is special; it will have stopped at breakpt_2, in thread
+# 2.3.  To set this inferior up so that threads can exit we need to
+# continue to breakpt_1.
+gdb_continue_to_breakpoint "all inferior 2 threads created" \
+    ".* breakpt_1 .*"
+
+with_test_prefix "inferior 2 ready" {
+    check_current_thread 2 1
+
+    switch_to_inferior 1 3
+    switch_to_thread 1 2
+
+    continue_in_bg "continue inferior 1"
+    switch_to_inferior 2 1
+    switch_to_thread 2 2
+    continue_in_bg "continue inferior 2"
+}
+
+# Cause thread 1.2 to exit.
+send_thread_exit_signal ${pid_1}
+
+with_test_prefix "after 1.2 exited" {
+    # We should go back to 1.1 now as 1.2 has exited.
+    switch_to_inferior 1 1
+    switch_to_thread 1 4
+
+    # Cause thread 2.2 to exit.
+    send_thread_exit_signal ${pid_2}
+}
+
+with_test_prefix "after 2.2 exited" {
+    # We should go back to 2.1 now as 2.2 has exited.
+    switch_to_inferior 2 1
+
+    # Cause thread 1.3 to exit.
+    send_thread_exit_signal ${pid_1}
+}
+
+with_test_prefix "after 1.3 exited" {
+    # We should still switch back to 1.4 as only 1.3 exited.
+    switch_to_inferior 1 4
+
+    # Cause thread 2.3 to exit.
+    send_thread_exit_signal ${pid_2}
+}
+
+with_test_prefix "after 2.3 exited" {
+    # Switch back to 2.1, which should still be selected.
+    switch_to_inferior 2 1
+
+    # Cause thread 1.4 to exit.
+    send_thread_exit_signal ${pid_1}
+}
+
+with_test_prefix "after 1.4 exited" {
+    # We should now switch back to 1.1 as 1.4 exited, and 1.1 is the
+    # only thread left now.
+    switch_to_inferior 1 1
+
+    # Cause thread 2.4 to exit.
+    send_thread_exit_signal ${pid_2}
+}
+
+with_test_prefix "after 2.4 exited" {
+    # Switch back to 2.1, which should still be selected.
+    switch_to_inferior 2 1
+}
+
--
2.14.5

Reply | Threaded
Open this post in threaded view
|

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

Simon Marchi-4
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
Reply | Threaded
Open this post in threaded view
|

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

Tom Tromey-2
In reply to this post by Andrew Burgess
>>>>> "Andrew" == Andrew Burgess <[hidden email]> writes:

Andrew> I did consider making this behaviour switchable, but the new behaviour
Andrew> seems (to me) obviously better, so I haven't added a switch for
Andrew> it.

FWIW, I agree.

The patch looks good to me as well.

thanks,
Tom
Reply | Threaded
Open this post in threaded view
|

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

Sourceware - gdb-patches mailing list
In reply to this post by Andrew Burgess
On 3/27/20 1:30 PM, 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.

Frankly, I'm not really sure I like this.  It seems like a can of worms to
me...  It's going to cause us to have to decide whether it's a good idea to
save/restore all kinds of state in the objects hierarchy.  E.g., if this is
reasonable, then it would also be reasonable to restore the selected
Ada task.  And frames.  And maybe the selected source file and line for
list.  Once we gain support for fibers, coroutines, etc. we'll
then need to apply the same logic.  Etc.  And then maybe we'll need
some way to query the selected thread of a given inferior.  Etc.

The current rule is quite simple.  If you select a object then its container
is implicitly selected.  So if you select a thread, you implicitly select
its inferior, and implicitly select its target.  And the first/initial container
object is selected.  It seems very natural to me that "inferior 2" ends up
selecting the initial thread of inferior 2.  I.e., normally, thread 2.1.

Thanks,
Pedro Alves

Reply | Threaded
Open this post in threaded view
|

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

Andrew Burgess
* Pedro Alves <[hidden email]> [2020-04-01 18:46:21 +0100]:

> On 3/27/20 1:30 PM, 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.
>
> Frankly, I'm not really sure I like this.  It seems like a can of worms to
> me...  It's going to cause us to have to decide whether it's a good idea to
> save/restore all kinds of state in the objects hierarchy.  E.g., if this is
> reasonable, then it would also be reasonable to restore the selected
> Ada task.

Sounds like a good idea to me

>           And frames.

That sounds so good to me, I already have a patch for it:

  https://sourceware.org/pipermail/gdb-patches/2020-February/166202.html

>                        And maybe the selected source file and line for
> list.

I'm slightly less convinced by this one, I think I'd have to mock it
up and see if it felt more intuitive.

>        Once we gain support for fibers, coroutines, etc. we'll
> then need to apply the same logic.  Etc.

Maybe we could design in this functionality from the ground up instead
of having to retro-fit it?

>                                           And then maybe we'll need
> some way to query the selected thread of a given inferior.  Etc.

Maybe, but right now GDB doesn't much like referencing anything other
than the current inferior, right?  For example 'info threads', we
don't iterate over each thread and query it's state, we iterate over
each thread, make it current, and then print the current thread's
state.  So right now, I've not found a reason to query an inferiors
current thread.

>
> The current rule is quite simple.  If you select a object then its container
> is implicitly selected.  So if you select a thread, you implicitly select
> its inferior, and implicitly select its target.  And the first/initial container
> object is selected.  It seems very natural to me that "inferior 2" ends up
> selecting the initial thread of inferior 2.  I.e., normally, thread
> 2.1.

Sure, I'm not disagreeing that the existing model is simple, but my
thinking is that a user would be more interested in going back to their
previous thread, or that turning up back in the previous thread would
not surprise them.

Is it that you like the simpler model because you feel it gives a
simpler or more consistent user experience?  Or is your concern that
adding the extra complexity will make GDB internals unnecessarily
complex?  Or maybe both?

To move this forward I think I have a few questions:

 - How would you feel if this behaviour was switchable? Default on?
   Default off?

 - The current implementation is already pretty minimalist, but would
   a different approach concern you less?  I did experiment with an
   approach using observers, however, I was finding that I was getting
   too many non-user driven events, or maybe I was missing some
   events... I don't really recall now, but it had problems.  But I
   could revisit this approach if something like this would feel
   better to you.  Or feel free to suggest something different, and
   I'll give it a go.

 - A final idea would be to move this functionality out of core GDB
   entirely, and add a new Python APIs so that we could do all of this
   functionality in a Python extension.  We could then possibly even
   include the script to add this functionality as an "extra", but
   this would only be loaded if required.

Thanks,
Andrew