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

Message ID gerrit.1580903669000.I6de26f990dc3b32ed526e60507e7bfe37fa1c6f1@gnutoolchain-gerrit.osci.io
State New, archived
Headers

Commit Message

Simon Marchi (Code Review) Feb. 5, 2020, 11:54 a.m. UTC
  Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/766
......................................................................

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

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.

Change-Id: I6de26f990dc3b32ed526e60507e7bfe37fa1c6f1
---
M gdb/infrun.c
A gdb/testsuite/gdb.multi/stop-all-on-exit.c
A gdb/testsuite/gdb.multi/stop-all-on-exit.exp
3 files changed, 106 insertions(+), 4 deletions(-)
  

Comments

Simon Marchi (Code Review) Feb. 6, 2020, 7:51 a.m. UTC | #1
Tankut Baris Aktemur has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/766
......................................................................


Patch Set 1:

I was under the impression that Gerrit was still optional for sending patches.  I'll abandon this merge request and send the patches via email instead.
  
Simon Marchi (Code Review) Feb. 6, 2020, 7:52 a.m. UTC | #2
Tankut Baris Aktemur has abandoned this change. ( https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/766 )

Change subject: gdb/infrun: stop all threads if there exists a non-stop target
......................................................................


Abandoned

Will use send-email.
  

Patch

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 3e846f8..fd72a74 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4682,7 +4682,7 @@ 
   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 @@ 
 	     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 @@ 
 					    "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 @@ 
   /* 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 0000000..bca0c98
--- /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 0000000..a0622ae
--- /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
+    }
+}