[PATCH v4] gdb: Fix deleted thread when issuing next command

[PATCH v4] gdb: Fix deleted thread when issuing next command

[Aleksandar Paunovic]

V3 can be found here https://sourceware.org/pipermail/gdb-patches/2021-June/180310.html
V4 addresses two comments from Simon.
Changes compared to V3:
* Removed "WARNING: Timed out waiting for EOF" by doing gdbserver_exit at the end of the test
* standard_testfile break-while-other-inf-steps-1.c is shortened to standard_testfile -1.c

Best,
Aleksandar

When issuing "next" command the thread got deleted even though it was still
alive and running.  This happened because the thread was examined under a
wrong inferior and therefor target_thread_alive was called on the wrong
target stack.

The fixed scenario:
~~~
$ gdb -q break-while-other-inf-steps-1-exe
(gdb) set schedule-multiple off
(gdb) break break-while-other-inf-steps-1.c:26
(gdb) run
(gdb) add-inferior -no-connection
(gdb) inferior 2
(gdb) spawn gdbserver :2346 break-while-other-inf-steps-2-exe
(gdb) target remote :2346
(gdb) break break-while-other-inf-steps-2.c:26
(gdb) continue
(gdb) thread 1.1
(gdb) clear break-while-other-inf-steps-2.c:26
(gdb) set schedule-multiple on
(gdb) next
(gdb) thread 1.1
~~~

Before:
~~~
Thread ID 1.1 has terminated.
~~~

Now:
~~~
Switching to thread 1.1
~~~

gdb/ChangeLog:
2021-04-30  Aleksandar Paunovic  <aleksandar.paunovic@intel.com>

	* infrun.c (keep_going_stepped_thread): Switch to correct
	inferior and check if thread is executing.

gdb/testsuite/ChangeLog:
2021-04-30  Aleksandar Paunovic  <aleksandar.paunovic@intel.com>

	* gdb.multi/break-while-other-inf-steps-1.c: New file.
	* gdb.multi/break-while-other-inf-steps-2.c: New file.
	* gdb.multi/break-while-other-inf-steps.exp: New file.

2021-06-24 Aleksandar Paunovic <aleksandar.paunovic@intel.com>
---
 gdb/infrun.c                                  |   2 +
 .../gdb.multi/break-while-other-inf-steps-1.c |  39 +++++++
 .../gdb.multi/break-while-other-inf-steps-2.c |  39 +++++++
 .../gdb.multi/break-while-other-inf-steps.exp | 102 ++++++++++++++++++
 4 files changed, 182 insertions(+)
 create mode 100644 gdb/testsuite/gdb.multi/break-while-other-inf-steps-1.c
 create mode 100644 gdb/testsuite/gdb.multi/break-while-other-inf-steps-2.c
 create mode 100644 gdb/testsuite/gdb.multi/break-while-other-inf-steps.exp

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 9469b74af3..1f50771e75 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -7559,6 +7559,8 @@ keep_going_stepped_thread (struct thread_info *tp)
        stepping thread is still alive.  For that reason, we need to
        synchronously query the target now.  */
 
+  /* Make sure that we are within a correct inferior.  */
+  switch_to_inferior_no_thread (tp->inf);
   if (tp->state == THREAD_EXITED || !target_thread_alive (tp->ptid))
     {
       infrun_debug_printf ("not resuming previously stepped thread, it has "
diff --git a/gdb/testsuite/gdb.multi/break-while-other-inf-steps-1.c b/gdb/testsuite/gdb.multi/break-while-other-inf-steps-1.c
new file mode 100644
index 0000000000..475b2c4126
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/break-while-other-inf-steps-1.c
@@ -0,0 +1,39 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2021 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 <unistd.h>
+#include <pthread.h>
+
+void *
+forever ()
+{
+  /* Wait for alarm.  */
+  while (1)
+    sleep (1); /* break here  */
+}
+
+int
+main ()
+{
+  alarm (30);
+
+  pthread_t forever_thread;
+  pthread_create (&forever_thread, NULL, *forever, NULL);
+  pthread_join (forever_thread, NULL);
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.multi/break-while-other-inf-steps-2.c b/gdb/testsuite/gdb.multi/break-while-other-inf-steps-2.c
new file mode 100644
index 0000000000..475b2c4126
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/break-while-other-inf-steps-2.c
@@ -0,0 +1,39 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2021 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 <unistd.h>
+#include <pthread.h>
+
+void *
+forever ()
+{
+  /* Wait for alarm.  */
+  while (1)
+    sleep (1); /* break here  */
+}
+
+int
+main ()
+{
+  alarm (30);
+
+  pthread_t forever_thread;
+  pthread_create (&forever_thread, NULL, *forever, NULL);
+  pthread_join (forever_thread, NULL);
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.multi/break-while-other-inf-steps.exp b/gdb/testsuite/gdb.multi/break-while-other-inf-steps.exp
new file mode 100644
index 0000000000..b628d06815
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/break-while-other-inf-steps.exp
@@ -0,0 +1,102 @@
+# Copyright 2021 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/>.
+
+# Create two inferiors with different targets.  The first runs on top of
+# a native target while the second on a remote target.  Both inferiors
+# use the copy of the same source code. The copy is done in order to make
+# sure that a breakpoint is only in inferior 2.  While in inferior 1, do
+# a "next" which should break in a thread in inferior 2.
+# Both executables will run total of 4 threads (2 per executable) and
+# we will put a breakpoint only in the second executable to achieve this.
+
+load_lib gdbserver-support.exp
+
+# This test explicitly sets up native and remote target.  Do not run
+# it with a board file which defaults to a remote target.
+if {[target_info gdb_protocol] != ""} {
+	return 0
+}
+
+standard_testfile -1.c -2.c
+set binfile2 ${binfile}2
+
+with_test_prefix "testfile 1" {
+    if  { [gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+        untested "failed to compile first testcase"
+        return -1
+    }
+}
+
+with_test_prefix "testfile 2" {
+    if  { [gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile2}" "${binfile2}" executable {debug}] != "" } {
+        untested "failed to compile second testcase"
+        return -1
+    }
+}
+
+# We need ability to stop all threads.
+# Hence, we go with the all-stop-on-top-of-non-stop mode.
+save_vars { GDBFLAGS } {
+    append GDBFLAGS " -ex \"maint set target-non-stop on\""
+    clean_restart ${binfile}
+}
+
+gdb_test_no_output "set schedule-multiple off"
+
+if {![runto_main]} {
+    untested "failed to run to main"
+    return -1
+}
+set break_pos [gdb_get_line_number "break here" ${srcfile}]
+gdb_breakpoint "$srcfile:$break_pos"
+gdb_continue_to_breakpoint "continue to breakpoint in inf 1"
+
+gdb_test "add-inferior -no-connection" "Added inferior 2.*" "add empty inferior 2"
+gdb_test "inferior 2" "Switching to inferior 2.*" "switch to inferior 2"
+
+# Start gdbserver and connect.
+set res [gdbserver_start "" $binfile2]
+set gdbserver_protocol [lindex $res 0]
+set gdbserver_gdbport [lindex $res 1]
+set res [gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport]
+if ![gdb_assert {$res == 0} "connect"] {
+    return -1
+}
+
+set break_pos [gdb_get_line_number "break here" ${srcfile2}]
+gdb_breakpoint "$srcfile2:$break_pos"
+
+gdb_continue_to_breakpoint "continue to breakpoint in inf 2"
+
+gdb_test "thread 1.1" "Switching to thread 1.1.*" "first switch to thread 1.1"
+
+# Remove the breakpoint from inf 1.  Now only a breakpoint in inf 2 remains.
+gdb_test "clear $srcfile:$break_pos" "Deleted breakpoint 2 " "remove inf 1 breakpoint"
+gdb_test_no_output "set schedule-multiple on"
+gdb_test "next" ".*Thread 2.*hit Breakpoint.*" "next while in inferior 1"
+
+# We should be able to normally switch to thread 1.1.
+# In case of a bad GDB flow the GDB was losing the thread.
+gdb_test_multiple "thread 1.1" "Switching to thread 1.1" {
+    -re "\\\[Switching to thread 1.1 \\(Thread .*\\)\\\].*$gdb_prompt $" {
+        pass $gdb_test_name
+    }
+    -re ".*Thread ID 1.1 has terminated.*$gdb_prompt $" {
+        fail $gdb_test_name
+    }
+}
+
+gdb_test "inferior 2" "Switching to inferior 2.*" "switch back to inferior 2"
+gdbserver_exit 0
-- 
2.17.1

Re: [PATCH v4] gdb: Fix deleted thread when issuing next command

[Simon Marchi]

On 2021-06-24 11:41 a.m., Aleksandar Paunovic wrote:
> V3 can be found here https://sourceware.org/pipermail/gdb-patches/2021-June/180310.html
> V4 addresses two comments from Simon.
> Changes compared to V3:
> * Removed "WARNING: Timed out waiting for EOF" by doing gdbserver_exit at the end of the test
> * standard_testfile break-while-other-inf-steps-1.c is shortened to standard_testfile -1.c
> 
> Best,
> Aleksandar
> 
> When issuing "next" command the thread got deleted even though it was still
> alive and running.  This happened because the thread was examined under a
> wrong inferior and therefor target_thread_alive was called on the wrong
> target stack.
> 
> The fixed scenario:
> ~~~
> $ gdb -q break-while-other-inf-steps-1-exe
> (gdb) set schedule-multiple off
> (gdb) break break-while-other-inf-steps-1.c:26
> (gdb) run
> (gdb) add-inferior -no-connection
> (gdb) inferior 2
> (gdb) spawn gdbserver :2346 break-while-other-inf-steps-2-exe
> (gdb) target remote :2346
> (gdb) break break-while-other-inf-steps-2.c:26
> (gdb) continue
> (gdb) thread 1.1
> (gdb) clear break-while-other-inf-steps-2.c:26
> (gdb) set schedule-multiple on
> (gdb) next
> (gdb) thread 1.1
> ~~~
> 
> Before:
> ~~~
> Thread ID 1.1 has terminated.
> ~~~
> 
> Now:
> ~~~
> Switching to thread 1.1
> ~~~
> 
> gdb/ChangeLog:
> 2021-04-30  Aleksandar Paunovic  <aleksandar.paunovic@intel.com>
> 
> 	* infrun.c (keep_going_stepped_thread): Switch to correct
> 	inferior and check if thread is executing.
> 
> gdb/testsuite/ChangeLog:
> 2021-04-30  Aleksandar Paunovic  <aleksandar.paunovic@intel.com>
> 
> 	* gdb.multi/break-while-other-inf-steps-1.c: New file.
> 	* gdb.multi/break-while-other-inf-steps-2.c: New file.
> 	* gdb.multi/break-while-other-inf-steps.exp: New file.

Thanks for taking care of the timeout.

Now, I just tried your test without the fix applied, and it still
passes.  So it doesn't seem to catch the problem you are fixing.  If you
do the same, do you see it failing?

Simon

[PATCH v4] gdb: Fix deleted thread when issuing next command

[Paunovic, Aleksandar]

>On 2021-06-24 11:41 a.m., Aleksandar Paunovic wrote:
>> V3 can be found here 
>> https://sourceware.org/pipermail/gdb-patches/2021-June/180310.html
>> V4 addresses two comments from Simon.
>> Changes compared to V3:
>> * Removed "WARNING: Timed out waiting for EOF" by doing gdbserver_exit 
>> at the end of the test
>> * standard_testfile break-while-other-inf-steps-1.c is shortened to 
>> standard_testfile -1.c
>> 
>> Best,
>> Aleksandar
>> 
>> When issuing "next" command the thread got deleted even though it was 
>> still alive and running.  This happened because the thread was 
>> examined under a wrong inferior and therefor target_thread_alive was 
>> called on the wrong target stack.
>> 
>> The fixed scenario:
>> ~~~
>> $ gdb -q break-while-other-inf-steps-1-exe
>> (gdb) set schedule-multiple off
>> (gdb) break break-while-other-inf-steps-1.c:26
>> (gdb) run
>> (gdb) add-inferior -no-connection
>> (gdb) inferior 2
>> (gdb) spawn gdbserver :2346 break-while-other-inf-steps-2-exe
>> (gdb) target remote :2346
>> (gdb) break break-while-other-inf-steps-2.c:26
>> (gdb) continue
>> (gdb) thread 1.1
>> (gdb) clear break-while-other-inf-steps-2.c:26
>> (gdb) set schedule-multiple on
>> (gdb) next
>> (gdb) thread 1.1
>> ~~~
>> 
>> Before:
>> ~~~
>> Thread ID 1.1 has terminated.
>> ~~~
>> 
>> Now:
>> ~~~
>> Switching to thread 1.1
>> ~~~
>> 
>> gdb/ChangeLog:
>> 2021-04-30  Aleksandar Paunovic  <aleksandar.paunovic@intel.com>
>> 
>> 	* infrun.c (keep_going_stepped_thread): Switch to correct
>> 	inferior and check if thread is executing.
>> 
>> gdb/testsuite/ChangeLog:
>> 2021-04-30  Aleksandar Paunovic  <aleksandar.paunovic@intel.com>
>> 
>> 	* gdb.multi/break-while-other-inf-steps-1.c: New file.
>> 	* gdb.multi/break-while-other-inf-steps-2.c: New file.
>> 	* gdb.multi/break-while-other-inf-steps.exp: New file.
>
>Thanks for taking care of the timeout.
>
>Now, I just tried your test without the fix applied, and it still passes.  So it doesn't seem to catch the problem you are fixing.  If you do the same, do you see it failing?
>
>Simon

Hmm, yes, I see the same. Something is changed in the meantime and function keep_going_stepped_thread is not called anymore within the "next" call. I will try to refactor the test to implicitly use restart_threads or restart_stepped_thread functions from infrun.c. Please let me know if there is a more obvious way to reproduce this.

Best,
Aleksandar
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928