This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH] remote non-stop: Process initially stopped threads before other commands


The main motivation for this is making non-stop / all-stop behave
similarly on initial connection, in order to move in the direction of
reimplementing all-stop mode with the remote target always running in
non-stop mode.

When we connect to a remote target in non-stop mode, we may find
threads either running or already stopped.  The act of connecting
itself does not force threads to stop.  To handle that, the remote
non-stop connection is currently roughly like this:

 #1 - Fetch list of remote threads (qXfer:threads:read, qfThreadInfo,
    etc).  All threads are assumed to be running until the target
    reports an asynchronous stop reply for them.

 #2 - Fetch the initial set of threads that were already stopped, with
    the '?'  packet.  (In non-stop, this is coupled with the vStopped
    mechanism to be able to retrieve the status of more than one
    thread.)

The stop replies fetched in #2 are placed in the pending stop reply
queue, and left for the regular event loop to process.  That is,
"target remote" finishes and returns _before_ those stops are
processed.

That means that it's possible to have GDB process further commands
before the initial set of stopped threads is reported to the user.

E.g., before the patch, note how the prompt is printed before the
frame:

 Remote debugging using :9999
 (gdb)
 [Thread 15296] #1 stopped.
 0x0000003615a011f0 in ?? ()

Even though thread #1 was not running, for a moment, the user can see
it as such:

 $ gdb a.out -ex "set non-stop 1" -ex "tar rem :9999"  -ex "info threads" -ex "info registers"
 Remote debugging using :9999
   Id   Target Id         Frame
 * 1    Thread 4772       (running)
 Target is executing.                 <<<<<<< info registers
 (gdb)
 [Thread 4772] #1 stopped.
 0x0000003615a011f0 in ?? ()

To fix that, this commit makes gdb process all threads found already
stopped at connection time, before giving the prompt to the user.

The fix takes a cue from fork-child.c:startup_inferior [1], and
processes the events locally in remote.c, avoiding the whole
wait_for_inferior/handle_inferior_event path.  I decided to try this
approach after noticing that:

 - several cases in handle_inferior_event miss checking stop_soon.
 - we don't want to fetch the thread list in normal_stop.

and trying to fix them was resulting in sprinkling stop_soon checks in
many places, and uglifying normal_stop even more.

While with this patch, I'm avoiding changing GDB's output other than
when the prompt is printed, I think this approach is more flexible if
we do want to change it.  And also, it's likely easier to get rid of
the MI *running event that is still sent for threads that are
initially found stopped, if we want to.

This happens to fix the testsuite too.  All non-stop tests are racy
against "target remote" / gdbserver testing currently.  That is,
sometimes the tests run, but other times they're just skipped without
any indication of PASS/FAIL.  When that happens, the logs show:

 target remote localhost:2346
 Remote debugging using localhost:2346
 (gdb)
 [Thread 25418] #1 stopped.
 0x0000003615a011f0 in ?? ()
 ^CQuit
 (gdb) Remote debugging from host 127.0.0.1
 Killing process(es): 25418
 monitor exit
 (gdb) Remote connection closed
 (gdb) testcase /home/pedro/gdb/mygit/build/../src/gdb/testsuite/gdb.threads/multi-create-ns-info-thr.exp completed in 61 seconds

The trouble here is that there's output after the prompt, and the
regex in question doesn't expect that:

   -re "Remote debugging using .*$serialport_re.*$gdb_prompt $" {
	verbose "Set target to $targetname"
	return 0
    }

[1] - before startup_inferior was added, we'd go through
wait_for_inferior/handle_inferior_event while going through the shell,
and that turned out problematic.

Tested on x86_64 Fedora 20, gdbserver.

gdb/ChangeLog:
2015-08-02  Pedro Alves  <palves@redhat.com>

	* infrun.c (print_target_wait_results): Make extern.
	* infrun.h (print_target_wait_results): Declare.
	* remote.c (set_stop_requested_callback): Delete.
	(process_initial_stop_replies): New function.
	(remote_start_remote): Use it.
	(stop_reply_queue_length): New function.

gdb/testsuite/ChangeLog:
2015-08-02  Pedro Alves  <palves@redhat.com>

	* gdb.server/connect-stopped-target.c: New file.
	* gdb.server/connect-stopped-target.exp: New file.
---
 gdb/infrun.c                                       |   4 +-
 gdb/infrun.h                                       |   5 +
 gdb/remote.c                                       | 117 +++++++++++++++++----
 gdb/testsuite/gdb.server/connect-stopped-target.c  |  22 ++++
 .../gdb.server/connect-stopped-target.exp          |  82 +++++++++++++++
 5 files changed, 205 insertions(+), 25 deletions(-)
 create mode 100644 gdb/testsuite/gdb.server/connect-stopped-target.c
 create mode 100644 gdb/testsuite/gdb.server/connect-stopped-target.exp

diff --git a/gdb/infrun.c b/gdb/infrun.c
index a2e7cfc..44135e5 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3033,9 +3033,9 @@ delete_just_stopped_threads_infrun_breakpoints_cleanup (void *arg)
   delete_just_stopped_threads_infrun_breakpoints ();
 }
 
-/* Pretty print the results of target_wait, for debugging purposes.  */
+/* See infrun.h.  */
 
-static void
+void
 print_target_wait_results (ptid_t waiton_ptid, ptid_t result_ptid,
 			   const struct target_waitstatus *ws)
 {
diff --git a/gdb/infrun.h b/gdb/infrun.h
index 75bb075..64d0fbe 100644
--- a/gdb/infrun.h
+++ b/gdb/infrun.h
@@ -150,6 +150,11 @@ extern void print_no_history_reason (struct ui_out *uiout);
 
 extern void print_stop_event (struct target_waitstatus *ws);
 
+/* Pretty print the results of target_wait, for debugging purposes.  */
+
+extern void print_target_wait_results (ptid_t waiton_ptid, ptid_t result_ptid,
+				       const struct target_waitstatus *ws);
+
 extern int signal_stop_state (int);
 
 extern int signal_print_state (int);
diff --git a/gdb/remote.c b/gdb/remote.c
index c047f35..1dbaed3 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -225,6 +225,8 @@ static int remote_can_run_breakpoint_commands (struct target_ops *self);
 
 static void remote_btrace_reset (void);
 
+static int stop_reply_queue_length (void);
+
 /* For "remote".  */
 
 static struct cmd_list_element *remote_cmdlist;
@@ -3410,21 +3412,6 @@ get_offsets (void)
   objfile_relocate (symfile_objfile, offs);
 }
 
-/* Callback for iterate_over_threads.  Set the STOP_REQUESTED flags in
-   threads we know are stopped already.  This is used during the
-   initial remote connection in non-stop mode --- threads that are
-   reported as already being stopped are left stopped.  */
-
-static int
-set_stop_requested_callback (struct thread_info *thread, void *data)
-{
-  /* If we have a stop reply for this thread, it must be stopped.  */
-  if (peek_stop_reply (thread->ptid))
-    set_stop_requested (thread->ptid, 1);
-
-  return 0;
-}
-
 /* Send interrupt_sequence to remote target.  */
 static void
 send_interrupt_sequence (void)
@@ -3552,6 +3539,77 @@ add_current_inferior_and_thread (char *wait_status)
   add_thread_silent (inferior_ptid);
 }
 
+/* Process all initial stop replies the remote side sent in response
+   to the ? packet.  These indicate threads that were already stopped
+   on initial connection.  We mark these threads as stopped and print
+   their current frame before giving the user the prompt.  */
+
+static void
+process_initial_stop_replies (void)
+{
+  int pending_stop_replies = stop_reply_queue_length ();
+
+  /* Consume the initial pending events.  */
+  while (pending_stop_replies-- > 0)
+    {
+      ptid_t waiton_ptid = minus_one_ptid;
+      ptid_t event_ptid;
+      struct target_waitstatus ws;
+      int ignore_event = 0;
+
+      memset (&ws, 0, sizeof (ws));
+      event_ptid = target_wait (waiton_ptid, &ws, TARGET_WNOHANG);
+      if (remote_debug)
+	print_target_wait_results (waiton_ptid, event_ptid, &ws);
+
+      switch (ws.kind)
+	{
+	case TARGET_WAITKIND_IGNORE:
+	case TARGET_WAITKIND_NO_RESUMED:
+	case TARGET_WAITKIND_SIGNALLED:
+	case TARGET_WAITKIND_EXITED:
+	  /* We shouldn't see these, but if we do, just ignore.  */
+	  if (remote_debug)
+	    fprintf_unfiltered (gdb_stdlog, "remote: event ignored\n");
+	  ignore_event = 1;
+	  break;
+
+	case TARGET_WAITKIND_EXECD:
+	  xfree (ws.value.execd_pathname);
+	  break;
+	default:
+	  break;
+	}
+
+      if (ignore_event)
+	continue;
+
+      switch_to_thread (event_ptid);
+      set_executing (event_ptid, 0);
+      set_running (event_ptid, 0);
+
+      stop_pc = get_frame_pc (get_current_frame ());
+      set_current_sal_from_frame (get_current_frame ());
+
+      if (ws.kind == TARGET_WAITKIND_STOPPED)
+	{
+	  enum gdb_signal sig = ws.value.sig;
+
+	  /* Stubs traditionally report SIGTRAP as initial signal,
+	     instead of signal 0.  Suppress it.  */
+	  if (sig == GDB_SIGNAL_TRAP)
+	    sig = GDB_SIGNAL_0;
+	  inferior_thread ()->suspend.stop_signal = sig;
+
+	  if (signal_print_state (sig))
+	    observer_notify_signal_received (sig);
+        }
+
+      print_stop_event (&ws);
+      observer_notify_normal_stop (NULL, 1);
+    }
+}
+
 static void
 remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
 {
@@ -3764,6 +3822,8 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
     }
   else
     {
+      ptid_t current_ptid;
+
       /* Clear WFI global state.  Do this before finding about new
 	 threads and inferiors, and setting the current inferior.
 	 Otherwise we would clear the proceed status of the current
@@ -3785,15 +3845,8 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
 	  rs->notif_state->pending_event[notif_client_stop.id]
 	    = remote_notif_parse (notif, rs->buf);
 	  remote_notif_get_pending_events (notif);
-
-	  /* Make sure that threads that were stopped remain
-	     stopped.  */
-	  iterate_over_threads (set_stop_requested_callback, NULL);
 	}
 
-      if (target_can_async_p ())
-	target_async (1);
-
       if (thread_count () == 0)
 	{
 	  if (!extended_p)
@@ -3811,10 +3864,11 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
       set_general_thread (null_ptid);
 
       /* Query it.  */
-      inferior_ptid = remote_current_thread (minus_one_ptid);
+      current_ptid = remote_current_thread (minus_one_ptid);
       if (ptid_equal (inferior_ptid, minus_one_ptid))
 	error (_("remote didn't report the current thread in non-stop mode"));
 
+      inferior_ptid = current_ptid;
       get_offsets ();		/* Get text, data & bss offsets.  */
 
       /* In non-stop mode, any cached wait status will be stored in
@@ -3823,6 +3877,15 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
 
       /* Report all signals during attach/startup.  */
       remote_pass_signals (target, 0, NULL);
+
+      /* If there are already stopped threads, mark them stopped and
+	 report their stops before giving the prompt to the user.  */
+      process_initial_stop_replies ();
+
+      switch_to_thread (current_ptid);
+
+      if (target_can_async_p ())
+	target_async (1);
     }
 
   /* If we connected to a live target, do some additional setup.  */
@@ -5464,6 +5527,14 @@ stop_reply_xfree (struct stop_reply *r)
   notif_event_xfree ((struct notif_event *) r);
 }
 
+/* Return the length of the stop reply queue.  */
+
+static int
+stop_reply_queue_length (void)
+{
+  return QUEUE_length (stop_reply_p, stop_reply_queue);
+}
+
 static void
 remote_notif_stop_parse (struct notif_client *self, char *buf,
 			 struct notif_event *event)
diff --git a/gdb/testsuite/gdb.server/connect-stopped-target.c b/gdb/testsuite/gdb.server/connect-stopped-target.c
new file mode 100644
index 0000000..d4206cb
--- /dev/null
+++ b/gdb/testsuite/gdb.server/connect-stopped-target.c
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2015 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/>.  */
+
+int
+main ()
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.server/connect-stopped-target.exp b/gdb/testsuite/gdb.server/connect-stopped-target.exp
new file mode 100644
index 0000000..15bf381
--- /dev/null
+++ b/gdb/testsuite/gdb.server/connect-stopped-target.exp
@@ -0,0 +1,82 @@
+# Copyright 2010-2015 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/>.  */
+
+# Check that when GDB connects to a stopped target in either non-stop
+# or all-stop modes, "target remote" does not return with the remote
+# thread marked running.
+
+load_lib gdbserver-support.exp
+load_lib prelink-support.exp
+
+if {[skip_gdbserver_tests]} {
+    return
+}
+
+standard_testfile
+set executable ${testfile}
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
+    return -1
+}
+
+proc do_test {nonstop} {
+    global binfile
+    global gdb_prompt
+    global hex
+
+    clean_restart $binfile
+
+    # Make sure we're disconnected, in case we're testing with an
+    # extended-remote board, therefore already connected.
+    gdb_test "disconnect" ".*"
+
+    gdb_test "set non-stop $nonstop"
+
+    set res [gdbserver_spawn ${binfile}]
+    set gdbserver_protocol [lindex $res 0]
+    set gdbserver_gdbport [lindex $res 1]
+
+    gdb_test_multiple "define connect" "define user command: connect" {
+	-re "Type commands for definition of \"connect\".\r\nEnd with a line saying just \"end\".\r\n>$" {
+	    gdb_test [join [list \
+				"target $gdbserver_protocol $gdbserver_gdbport" \
+				"info threads" \
+				"p /x \$pc" \
+				"end" \
+			       ] "\n"] \
+		"" \
+		"define user command: connect"
+	}
+    }
+
+    # In non-stop mode, "target remote" used to return before the
+    # already-stopped thread was marked stopped.  Because of that, a
+    # script (or fast user) could see the target as running right
+    # after connection:
+    #
+    # (gdb) connect
+    #   Id   Target Id         Frame
+    # * 1    Thread 31179      (running)
+    # Target is executing.
+    # (gdb)
+    gdb_test "connect" " = $hex" "connect and print pc"
+    gdb_test "print /x \$pc" " = $hex" "print pc again"
+}
+
+foreach nonstop { "off" "on" } {
+    with_test_prefix "non-stop=$nonstop" {
+	do_test $nonstop
+    }
+}
-- 
1.9.3


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]