[2/2] gdb/infrun: stop all threads if there exists a non-stop target

Message ID 1580977799-4371-3-git-send-email-tankut.baris.aktemur@intel.com
State New
Headers show
Series
  • All threads not stopped when a process exits
Related show

Commit Message

Tankut Baris Aktemur Feb. 6, 2020, 8:29 a.m.
Stop all threads not only if the current target is non-stop, but also
if there exists a non-stop target.

The multi-target patch (5b6d1e4fa4f "Multi-target support") made the
following change to gdb/inf-child.c:

void
 inf_child_target::maybe_unpush_target ()
 {
-  if (!inf_child_explicitly_opened && !have_inferiors ())
+  if (!inf_child_explicitly_opened)
     unpush_target (this);
 }

If we are in all-stop mode with multiple inferiors, and an exit event
is received from an inferior, target_mourn_inferior() gets to this
point and without the have_inferiors() check, the target is unpushed.
This leads to having exec_ops as the top target.

Here is a test scenario.  Two executables, ./a.out returns
immediately; ./sleepy just sleeps.

  $ gdb ./sleepy
  (gdb) start
  ...
  (gdb) add-inferior -exec ./a.out
  ...
  (gdb) inferior 2
  [Switching to inferior 2..
  (gdb) start
  ...
  (gdb) set schedule-multiple on
  (gdb) set debug infrun 1
  (gdb) continue

At this point, the exit event is received from ./a.out.  Normally,
this would lead to stop_all_threads() to also stop ./sleepy, but this
doesn't happen, because target_is_non_stop_p() returns false.  And it
returns false because the top target is no longer the process target;
it is the exec_ops.

This patch modifies 'stop_waiting' to call 'stop_all_threads' if there
exists a non-stop target, not just when the current top target is
non-stop.

Tested on X86_64 Linux.

gdb/ChangeLog:
2020-02-05  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* infrun.c (stop_all_threads): Update assertion, plus when
	stopping threads, take into account that we might be trying
	to stop an all-stop target.
	(stop_waiting): Call 'stop_all_threads' if there exists a
	non-stop target.

gdb/testsuite/ChangeLog:
2020-02-05  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* gdb.multi/stop-all-on-exit.c: New test.
	* gdb.multi/stop-all-on-exit.exp: New file.
---
 gdb/infrun.c                                 | 19 ++++--
 gdb/testsuite/gdb.multi/stop-all-on-exit.c   | 27 +++++++++
 gdb/testsuite/gdb.multi/stop-all-on-exit.exp | 64 ++++++++++++++++++++
 3 files changed, 106 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/gdb.multi/stop-all-on-exit.c
 create mode 100644 gdb/testsuite/gdb.multi/stop-all-on-exit.exp

-- 
2.17.1

Comments

Tankut Baris Aktemur via Gdb-patches April 1, 2020, 6:10 p.m. | #1
Hi Tankut,

Thanks much for this, and I'm very sorry for the delay.

This looks good to me, with minor comments below.  Please push.

On 2/6/20 8:29 AM, Tankut Baris Aktemur wrote:

>  

>    if (debug_infrun)

>      fprintf_unfiltered (gdb_stdlog, "infrun: stop_all_threads\n");

> @@ -4713,6 +4713,18 @@ stop_all_threads (void)

>  	     to tell the target to stop.  */

>  	  for (thread_info *t : all_non_exited_threads ())

>  	    {

> +	      /* For a single-target setting with an all-stop target,

> +		 we would not even arrive here.  For a multi-target

> +		 setting, until GDB is able to handle a mixture of

> +		 all-stop and non-stop targets, simply skip all-stop

> +		 targets' threads.  This should be fine due to the

> +		 protection of commit

> +		 2f4fcf00399bc0ad5a4fed6b530128e8be4f40da  */


Missing period.  But also please mention here the protection of 
check_multi_target_resumption instead of the commit id, so that
people don't have to dig into the repo every time they need to
understand this.

> +

> +	      switch_to_thread_no_regs (t);

> +	      if (!target_is_non_stop_p ())

> +		continue;

> +


> +# GDB is expected to have stopped the other inferior.  Switch to the

> +# slow inferior's thread.  It should not be running.

> +gdb_test_multiple "thread 2.1" "check thread 2.1 is not running" {

> +    -re ".*\\(running\\)\[\r\n\]+$gdb_prompt" {


These leading ".*" are unnecessary -- they're implied.

> +	fail $gdb_test_name

> +    }

> +    -re ".*$gdb_prompt" {


Here too.

> +	pass $gdb_test_name

> +    }

> +}

> 

Thanks,
Pedro Alves
Tankut Baris Aktemur via Gdb-patches April 1, 2020, 7:35 p.m. | #2
On Wednesday, April 1, 2020 8:10 PM, Pedro Alves wrote:
> Hi Tankut,

> 

> Thanks much for this, and I'm very sorry for the delay.

> 

> This looks good to me, with minor comments below.  Please push.


Thank you.  Fixed and pushed as

53cccef118b gdb/infrun: stop all threads if there exists a non-stop target
a0714d305fb gdb: define convenience function 'exists_non_stop_target'


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

Patch

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 3e846f8e680..fd72a744339 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4682,7 +4682,7 @@  stop_all_threads (void)
   int pass;
   int iterations = 0;
 
-  gdb_assert (target_is_non_stop_p ());
+  gdb_assert (exists_non_stop_target ());
 
   if (debug_infrun)
     fprintf_unfiltered (gdb_stdlog, "infrun: stop_all_threads\n");
@@ -4713,6 +4713,18 @@  stop_all_threads (void)
 	     to tell the target to stop.  */
 	  for (thread_info *t : all_non_exited_threads ())
 	    {
+	      /* For a single-target setting with an all-stop target,
+		 we would not even arrive here.  For a multi-target
+		 setting, until GDB is able to handle a mixture of
+		 all-stop and non-stop targets, simply skip all-stop
+		 targets' threads.  This should be fine due to the
+		 protection of commit
+		 2f4fcf00399bc0ad5a4fed6b530128e8be4f40da  */
+
+	      switch_to_thread_no_regs (t);
+	      if (!target_is_non_stop_p ())
+		continue;
+
 	      if (t->executing)
 		{
 		  /* If already stopping, don't request a stop again.
@@ -4724,7 +4736,6 @@  stop_all_threads (void)
 					    "infrun:   %s executing, "
 					    "need stop\n",
 					    target_pid_to_str (t->ptid).c_str ());
-		      switch_to_thread_no_regs (t);
 		      target_stop (t->ptid);
 		      t->stop_requested = 1;
 		    }
@@ -7837,9 +7848,9 @@  stop_waiting (struct execution_control_state *ecs)
   /* Let callers know we don't want to wait for the inferior anymore.  */
   ecs->wait_some_more = 0;
 
-  /* If all-stop, but the target is always in non-stop mode, stop all
+  /* If all-stop, but there exists a non-stop target, stop all
      threads now that we're presenting the stop to the user.  */
-  if (!non_stop && target_is_non_stop_p ())
+  if (!non_stop && exists_non_stop_target ())
     stop_all_threads ();
 }
 
diff --git a/gdb/testsuite/gdb.multi/stop-all-on-exit.c b/gdb/testsuite/gdb.multi/stop-all-on-exit.c
new file mode 100644
index 00000000000..bca0c98e021
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/stop-all-on-exit.c
@@ -0,0 +1,27 @@ 
+/* 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 <unistd.h>
+
+static unsigned int duration = 1;
+
+int
+main ()
+{
+  sleep (duration);
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.multi/stop-all-on-exit.exp b/gdb/testsuite/gdb.multi/stop-all-on-exit.exp
new file mode 100644
index 00000000000..a0622aeab3c
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/stop-all-on-exit.exp
@@ -0,0 +1,64 @@ 
+# 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 that in all-stop mode with multiple inferiors, GDB stops all
+# threads upon receiving an exit event from one of the inferiors.
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile]} {
+    return -1
+}
+
+if {![runto_main]} {
+    fail "starting inferior 1"
+    return -1
+}
+
+# This is a test specific for a native target, where we use the
+# "-exec" argument to "add-inferior" and we explicitly don't do
+# "maint set target-non-stop on".
+if {![gdb_is_target_native]} {
+    untested "the test is aimed at a native target"
+    return 0
+}
+
+# Add a second inferior that will sleep longer.
+gdb_test "add-inferior -exec $binfile" "Added inferior 2.*" \
+    "add the second inferior"
+gdb_test "inferior 2" ".*Switching to inferior 2.*"
+if {![runto_main]} {
+    fail "starting inferior 2"
+    return -1
+}
+gdb_test "print duration=10" "= 10"
+
+# Now continue both processes.  We should get the exit event from the
+# first inferior.
+gdb_test_no_output "set schedule-multiple on"
+gdb_continue_to_end
+
+# GDB is expected to have stopped the other inferior.  Switch to the
+# slow inferior's thread.  It should not be running.
+gdb_test_multiple "thread 2.1" "check thread 2.1 is not running" {
+    -re ".*\\(running\\)\[\r\n\]+$gdb_prompt" {
+	fail $gdb_test_name
+    }
+    -re ".*$gdb_prompt" {
+	pass $gdb_test_name
+    }
+}