From 221e1a37cd14f7c98b4ea1ab24bd721a8b144711 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Thu, 20 Aug 2015 18:27:55 +0100 Subject: [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-20 Pedro Alves * 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-20 Pedro Alves * gdb.server/connect-stopped-target.c: New file. * gdb.server/connect-stopped-target.exp: New file. --- gdb/ChangeLog | 9 ++ gdb/infrun.c | 4 +- gdb/infrun.h | 5 + gdb/remote.c | 117 ++++++++++++++---- gdb/testsuite/ChangeLog | 5 + .../gdb.server/connect-stopped-target.c | 22 ++++ .../gdb.server/connect-stopped-target.exp | 82 ++++++++++++ 7 files changed, 219 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/ChangeLog b/gdb/ChangeLog index ed93dd2e28a..44968089267 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,12 @@ +2015-08-20 Pedro Alves + + * 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. + 2015-08-20 Pedro Alves * dwarf2read.c (process_full_comp_unit): To tell whether diff --git a/gdb/infrun.c b/gdb/infrun.c index a695f2e3b5d..25036a466d1 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -3332,9 +3332,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 6108648f698..7978d2c8200 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 4e483fda7bf..89f7fafaa9d 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -226,6 +226,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; @@ -3411,21 +3413,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) @@ -3553,6 +3540,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) { @@ -3765,6 +3823,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 @@ -3786,15 +3846,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) @@ -3812,10 +3865,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 @@ -3824,6 +3878,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. */ @@ -5486,6 +5549,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/ChangeLog b/gdb/testsuite/ChangeLog index 49ea9fb2df1..d2b9c6a9fd4 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,3 +1,8 @@ +2015-08-20 Pedro Alves + + * gdb.server/connect-stopped-target.c: New file. + * gdb.server/connect-stopped-target.exp: New file. + 2015-08-20 Pedro Alves * gdb.dwarf2/comp-unit-lang.exp: New file. 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 00000000000..7e362b2940e --- /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 . */ + +int +main (void) +{ + 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 00000000000..15bf381fa01 --- /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 . */ + +# 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 + } +} -- 2.30.2