[master,+,11] gdb: disable commit-resumed on -exec-interrupt --thread-group

Message ID 20210712023302.407846-1-simon.marchi@polymtl.ca
State New
Headers show
Series
  • [master,+,11] gdb: disable commit-resumed on -exec-interrupt --thread-group
Related show

Commit Message

Simon Marchi via Gdb-patches July 12, 2021, 2:33 a.m.
As reported in PR gdb/28077, we hit an internal error when using
-exec-interrupt with --thread-group:

    info threads
    &"info threads\n"
    ~"  Id   Target Id             Frame \n"
    ~"* 1    process 403312 \"loop\" (running)\n"
    ^done
    (gdb)
    -exec-interrupt --thread-group i1
    ~"/home/simark/src/binutils-gdb/gdb/target.c:3768: internal-error: void target_stop(ptid_t): Assertion `!proc_target->commit_resumed_state' failed.\nA problem internal to GDB has been detected,\nfurther debugging may prove unreliable.\nQuit this debugging session? (y or n) "

This is because this code path never disables commit-resumed (a
requirement for calling target_stop, as documented in
process_stratum_target::┬╗commit_resumed_state) before calling
target_stop.

The other 3 code paths in mi_cmd_exec_interrupt use interrupt_target_1,
which does it.  But the --thread-group code path uses its own thing
which doesn't do it.  Fix this by adding a scoped_disable_commit_resumed
in this code path.

Calling -exec-interrupt with --thread-group is apparently not tested at
the moment (which is why this bug could creep in).  Add a new test for
that.  The test runs two inferiors and tries to interrupt them with
"-exec-interrupt --thread-group X".

This will need to be merged in the gdb-11-branch, so here are ChangeLog
entries:

gdb/ChangeLog:

	* mi/mi-main.c (mi_cmd_exec_interrupt): Use
	scoped_disable_commit_resumed in the --thread-group case.

gdb/testsuite/ChangeLog:

	* interrupt-thread-group.c: New.
	* interrupt-thread-group.exp: New.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28077
Change-Id: I615efefcbcaf2c15d47caf5e4b9d82854b2a2fcb
---
 gdb/mi/mi-main.c                              |   3 +
 gdb/testsuite/gdb.mi/interrupt-thread-group.c |  65 +++++++++
 .../gdb.mi/interrupt-thread-group.exp         | 135 ++++++++++++++++++
 3 files changed, 203 insertions(+)
 create mode 100644 gdb/testsuite/gdb.mi/interrupt-thread-group.c
 create mode 100644 gdb/testsuite/gdb.mi/interrupt-thread-group.exp

-- 
2.32.0

Comments

Pedro Alves July 12, 2021, 8:52 a.m. | #1
On 2021-07-12 3:33 a.m., Simon Marchi via Gdb-patches wrote:

> gdb/testsuite/ChangeLog:

> 

> 	* interrupt-thread-group.c: New.

> 	* interrupt-thread-group.exp: New.


Missing gdb.mi/ :

 	* gdb.mi/interrupt-thread-group.c: New.
 	* gdb.mi/interrupt-thread-group.exp: New.

> +


> +# This test uses non-stop, which requires displaced stepping.

> +if { ![support_displaced_stepping] } {

> +    unsupported "displaced stepping"

> +    return -1

> +}


This is not true nowadays -- displaced stepping isn't a requirement for non-stop, it's
"merely" an optimization.  infrun will stop all threads to step over breakpoints inline
if displaced stepping is not supported.  Are you copying this from elsewhere?  Guess a bit
of spring cleaning is in order if so.

> +

> +mi_expect_stop "breakpoint-hit" "all_threads_started" ".*" ".*" ".*" {"" "disp=\"keep\""} \

> +    "inferior i1 stops at all_threads_started"

> +

> +mi_send_resuming_command "exec-continue --thread-group i1" \

> +    "continue inferior 1"

> +

> +# We can't run a second inferior on stub targets.  We can still test with one

> +# inferior and ensure that the command has the desired effect.

> +set use_second_inferior [expr {![use_gdb_stub]}]

> +

> +if { $use_second_inferior } {

> +    # The inferior created by the -add-inferior MI command does not inherit the

> +    # target connection of the first inferior.  If debugging through an

> +    # extended-remote connection, that means we can't run that second inferior

> +    # on the remote connection.  Use the add-inferior CLI command as a stop-gap.


Yeah, we should fix that at some point...

> +    if { [mi_is_target_remote] } {

> +	mi_gdb_test "add-inferior" \

> +	    "\\^done" \

> +	    "add inferior 2"

> +    } else {

> +	mi_gdb_test "-add-inferior" \

> +	    "\\^done,inferior=\"i2\"" \

> +	    "add inferior 2"

> +    }

> +    mi_gdb_test "-file-exec-and-symbols --thread-group i2 $::binfile" \

> +	"\\^done" \

> +	"set executable of inferior 2"

> +    # Run second inferior to all_threads_started (to ensure all threads are

> +    # started) and resume it.

> +    mi_gdb_test "-exec-run \

> +	--thread-group i2" \

> +	"\\^running.*" "run inferior 2"


Note this will fail on gdb builds that do not include a native target, which is
most "--target foo" builds.

Otherwise LGTM.
Simon Marchi via Gdb-patches July 12, 2021, 8:28 p.m. | #2
On 2021-07-12 4:52 a.m., Pedro Alves wrote:
> On 2021-07-12 3:33 a.m., Simon Marchi via Gdb-patches wrote:

> 

>> gdb/testsuite/ChangeLog:

>>

>> 	* interrupt-thread-group.c: New.

>> 	* interrupt-thread-group.exp: New.

> 

> Missing gdb.mi/ :

> 

>  	* gdb.mi/interrupt-thread-group.c: New.

>  	* gdb.mi/interrupt-thread-group.exp: New.


Fixed.

>> +# This test uses non-stop, which requires displaced stepping.

>> +if { ![support_displaced_stepping] } {

>> +    unsupported "displaced stepping"

>> +    return -1

>> +}

> 

> This is not true nowadays -- displaced stepping isn't a requirement for non-stop, it's

> "merely" an optimization.  infrun will stop all threads to step over breakpoints inline

> if displaced stepping is not supported.  Are you copying this from elsewhere?  Guess a bit

> of spring cleaning is in order if so.


Ok.  So we just put nothing, and if the target doesn't support non-stop,
GDB outputs "The target does not support running in non-stop mode." and
mi_run_cmd returns -1?

>> +    mi_gdb_test "-file-exec-and-symbols --thread-group i2 $::binfile" \

>> +	"\\^done" \

>> +	"set executable of inferior 2"

>> +    # Run second inferior to all_threads_started (to ensure all threads are

>> +    # started) and resume it.

>> +    mi_gdb_test "-exec-run \

>> +	--thread-group i2" \

>> +	"\\^running.*" "run inferior 2"

> 

> Note this will fail on gdb builds that do not include a native target, which is

> most "--target foo" builds.


I don't think so, because inferior 2 is using the remote target, which
can run.  Note that the formatting above was erroneous, the line is
broken at the wrong place, I've fixed it locally.

Simon
Pedro Alves July 13, 2021, 12:24 p.m. | #3
On 2021-07-12 9:28 p.m., Simon Marchi wrote:
> On 2021-07-12 4:52 a.m., Pedro Alves wrote:


>>> +# This test uses non-stop, which requires displaced stepping.

>>> +if { ![support_displaced_stepping] } {

>>> +    unsupported "displaced stepping"

>>> +    return -1

>>> +}

>>

>> This is not true nowadays -- displaced stepping isn't a requirement for non-stop, it's

>> "merely" an optimization.  infrun will stop all threads to step over breakpoints inline

>> if displaced stepping is not supported.  Are you copying this from elsewhere?  Guess a bit

>> of spring cleaning is in order if so.

> 

> Ok.  So we just put nothing, and if the target doesn't support non-stop,

> GDB outputs "The target does not support running in non-stop mode." and

> mi_run_cmd returns -1?


That's how the CLI tests work, at least.

> 

>>> +    mi_gdb_test "-file-exec-and-symbols --thread-group i2 $::binfile" \

>>> +	"\\^done" \

>>> +	"set executable of inferior 2"

>>> +    # Run second inferior to all_threads_started (to ensure all threads are

>>> +    # started) and resume it.

>>> +    mi_gdb_test "-exec-run \

>>> +	--thread-group i2" \

>>> +	"\\^running.*" "run inferior 2"

>>

>> Note this will fail on gdb builds that do not include a native target, which is

>> most "--target foo" builds.

> 

> I don't think so, because inferior 2 is using the remote target, which

> can run.  Note that the formatting above was erroneous, the line is

> broken at the wrong place, I've fixed it locally.


OK.
Simon Marchi via Gdb-patches July 13, 2021, 1:27 p.m. | #4
On 2021-07-13 8:24 a.m., Pedro Alves wrote:
> On 2021-07-12 9:28 p.m., Simon Marchi wrote:

>> On 2021-07-12 4:52 a.m., Pedro Alves wrote:

> 

>>>> +# This test uses non-stop, which requires displaced stepping.

>>>> +if { ![support_displaced_stepping] } {

>>>> +    unsupported "displaced stepping"

>>>> +    return -1

>>>> +}

>>>

>>> This is not true nowadays -- displaced stepping isn't a requirement for non-stop, it's

>>> "merely" an optimization.  infrun will stop all threads to step over breakpoints inline

>>> if displaced stepping is not supported.  Are you copying this from elsewhere?  Guess a bit

>>> of spring cleaning is in order if so.

>>

>> Ok.  So we just put nothing, and if the target doesn't support non-stop,

>> GDB outputs "The target does not support running in non-stop mode." and

>> mi_run_cmd returns -1?

> 

> That's how the CLI tests work, at least.

> 

>>

>>>> +    mi_gdb_test "-file-exec-and-symbols --thread-group i2 $::binfile" \

>>>> +	"\\^done" \

>>>> +	"set executable of inferior 2"

>>>> +    # Run second inferior to all_threads_started (to ensure all threads are

>>>> +    # started) and resume it.

>>>> +    mi_gdb_test "-exec-run \

>>>> +	--thread-group i2" \

>>>> +	"\\^running.*" "run inferior 2"

>>>

>>> Note this will fail on gdb builds that do not include a native target, which is

>>> most "--target foo" builds.

>>

>> I don't think so, because inferior 2 is using the remote target, which

>> can run.  Note that the formatting above was erroneous, the line is

>> broken at the wrong place, I've fixed it locally.

> 

> OK.

> 


Thanks, pushed to both branches.

Simon

Patch

diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index e293dddc08d8..44008d1c0a86 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -383,6 +383,9 @@  mi_cmd_exec_interrupt (const char *command, char **argv, int argc)
     {
       struct inferior *inf = find_inferior_id (current_context->thread_group);
 
+      scoped_disable_commit_resumed disable_commit_resumed
+	("interrupting all threads of thread group");
+
       iterate_over_threads (interrupt_thread_callback, &inf->pid);
     }
   else
diff --git a/gdb/testsuite/gdb.mi/interrupt-thread-group.c b/gdb/testsuite/gdb.mi/interrupt-thread-group.c
new file mode 100644
index 000000000000..738c6fc3e3ee
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/interrupt-thread-group.c
@@ -0,0 +1,65 @@ 
+/* 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>
+#include <assert.h>
+
+#define NUM_THREADS 4
+
+static pthread_barrier_t barrier;
+
+static void *
+thread_function (void *arg)
+{
+  pthread_barrier_wait (&barrier);
+
+  for (int i = 0; i < 30; i++)
+    sleep (1);
+
+  return NULL;
+}
+
+static void
+all_threads_started (void)
+{}
+
+int
+main (void)
+{
+  pthread_t threads[NUM_THREADS];
+
+  pthread_barrier_init (&barrier, NULL, NUM_THREADS + 1);
+
+  for (int i = 0; i < NUM_THREADS; i++)
+    {
+      int res = pthread_create (&threads[i], NULL, thread_function, NULL);
+      assert (res == 0);
+    }
+
+  pthread_barrier_wait (&barrier);
+  all_threads_started ();
+
+  for (int i = 0; i < NUM_THREADS; i++)
+    {
+      int res = pthread_join (threads[i], NULL);
+      assert (res == 0);
+    }
+
+  return 0;
+}
+
diff --git a/gdb/testsuite/gdb.mi/interrupt-thread-group.exp b/gdb/testsuite/gdb.mi/interrupt-thread-group.exp
new file mode 100644
index 000000000000..b32fd1afc724
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/interrupt-thread-group.exp
@@ -0,0 +1,135 @@ 
+# 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/>.
+
+# Test "--exec-interrupt --thread-group".
+#
+# Run two inferiors, try interrupting them both with the command above.
+
+# This test uses non-stop, which requires displaced stepping.
+if { ![support_displaced_stepping] } {
+    unsupported "displaced stepping"
+    return -1
+}
+
+load_lib mi-support.exp
+set MIFLAGS "-i=mi"
+
+standard_testfile .c
+
+if {[gdb_compile_pthreads "$srcdir/$subdir/$srcfile" $binfile \
+	executable debug] != "" } {
+    return -1
+}
+
+save_vars { GDBFLAGS } {
+    append GDBFLAGS " -ex \"set non-stop on\" -ex \"set mi-async\""
+    mi_clean_restart $binfile
+}
+
+mi_detect_async
+
+# Create breakpoint by hand instead of using mi_runto, since we'll need it for
+# both inferiors.
+mi_create_breakpoint "all_threads_started" \
+    "set breakpoint on all_threads_started"
+
+# Run first inferior to all_threads_started (to ensure all threads are started)
+# and resume it.
+if { [mi_run_cmd] < 0 } {
+    return
+}
+
+mi_expect_stop "breakpoint-hit" "all_threads_started" ".*" ".*" ".*" {"" "disp=\"keep\""} \
+    "inferior i1 stops at all_threads_started"
+
+mi_send_resuming_command "exec-continue --thread-group i1" \
+    "continue inferior 1"
+
+# We can't run a second inferior on stub targets.  We can still test with one
+# inferior and ensure that the command has the desired effect.
+set use_second_inferior [expr {![use_gdb_stub]}]
+
+if { $use_second_inferior } {
+    # The inferior created by the -add-inferior MI command does not inherit the
+    # target connection of the first inferior.  If debugging through an
+    # extended-remote connection, that means we can't run that second inferior
+    # on the remote connection.  Use the add-inferior CLI command as a stop-gap.
+    if { [mi_is_target_remote] } {
+	mi_gdb_test "add-inferior" \
+	    "\\^done" \
+	    "add inferior 2"
+    } else {
+	mi_gdb_test "-add-inferior" \
+	    "\\^done,inferior=\"i2\"" \
+	    "add inferior 2"
+    }
+    mi_gdb_test "-file-exec-and-symbols --thread-group i2 $::binfile" \
+	"\\^done" \
+	"set executable of inferior 2"
+    # Run second inferior to all_threads_started (to ensure all threads are
+    # started) and resume it.
+    mi_gdb_test "-exec-run \
+	--thread-group i2" \
+	"\\^running.*" "run inferior 2"
+
+    mi_expect_stop "breakpoint-hit" "all_threads_started" ".*" ".*" ".*" {"" "disp=\"keep\""} \
+	"inferior i2 stops at all_threads_started"
+
+    mi_send_resuming_command "exec-continue --thread-group i2" \
+	"continue inferior 2"
+
+    mi_check_thread_states {
+	"running" "running" "running" "running" "running"
+	"running" "running" "running" "running" "running"
+    } "before interrupting"
+} else {
+    mi_check_thread_states {
+	"running" "running" "running" "running" "running"
+    } "before interrupting"
+}
+
+# Interrupt inferior 1, wait for events.
+mi_gdb_test "-exec-interrupt --thread-group i1" \
+    "\\^done" \
+    "interrupt inferior 1"
+
+for {set i 0} {$i < 5} {incr i} {
+    mi_expect_interrupt "inferior 1, interrupt $i"
+}
+
+if { $use_second_inferior } {
+    mi_check_thread_states {
+	"stopped" "stopped" "stopped" "stopped" "stopped"
+	"running" "running" "running" "running" "running"
+    } "after interrupting inferior 1"
+
+    # Interrupt inferior 2, wait for events.
+    mi_gdb_test "-exec-interrupt --thread-group i2" \
+	"\\^done" \
+	"interrupt inferior 2"
+
+    for {set i 0} {$i < 5} {incr i} {
+	mi_expect_interrupt "inferior 2, interrupt $i"
+    }
+
+    mi_check_thread_states {
+	"stopped" "stopped" "stopped" "stopped" "stopped"
+	"stopped" "stopped" "stopped" "stopped" "stopped"
+    } "after interrupting inferior 2"
+} else {
+    mi_check_thread_states {
+	"stopped" "stopped" "stopped" "stopped" "stopped"
+    } "after interrupting inferior 1"
+}