This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
[patch] Fix CTRL-C for remote.c (PR remote/15297)
- From: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- To: gdb-patches at sourceware dot org
- Cc: Sergio Durigan Junior <sergiodj at redhat dot com>
- Date: Mon, 17 Jun 2013 08:26:52 +0200
- Subject: [patch] Fix CTRL-C for remote.c (PR remote/15297)
Hi,
http://sourceware.org/bugzilla/show_bug.cgi?id=15297
With "set debug remote 1" hitting CTRL-C usually gets ignored, occasionally it
even locks-up GDB by calling unsafe functions from a signal handler.
This patch is in fact two unrelated patches together - one for async mode
(non-stop) and one for sync mode (all-stop).
There is a general question whether GDB should just call set_stop_requested
and stop ASAP on CTRL-C or whether GDB should just send CTRL-C to gdbserver
and wait until it gets received as inferior SIGINT notification. linux-nat
currently does the latter although remote.c was (unsuccessfully) trying to do
the former. This patch does the latter even for remote.c.
sync mode (all-stop) is not absolutely clean. The problem is we cannot call
anything from signal handler but mark_async_signal_handler also has no effect
as GDB waits/processes packets without returning to the main loop. Using
set_quit_flag is a bit invasive but that has always been the case if CTRL-C
gets caught by existing non-remote.c handle_sigint.
No regressions on {x86_64,x86_64-m32,i686}-fedora19pre-linux-gnu in gdbserver
mode.
This patch requires first to revert patch:
3b0f7442800817f8a19b8eebd3b897a75328af14
PR cli/7221
There is now a pending fix for that regression
[PATCH] Fix PR cli/15603
http://sourceware.org/ml/gdb-patches/2013-06/msg00336.html
but that fix seems to be incomplete, it does not work with the testcase below.
Thanks,
Jan
gdb/
2013-06-17 Jan Kratochvil <jan.kratochvil@redhat.com>
* remote.c (handle_remote_sigint): Use set_quit_flag for sync.
(async_remote_interrupt): Call gdb_assert for async. Replace
target_stop by send_interrupt_sequence.
(remote_interrupt, remote_interrupt_twice): Replace immediate
gdb_call_async_signal_handler by mark_async_signal_handler, call
set_quit_flag in sync mode.
(remote_wait_as): Remove redundant clear_quit_flag. Replace
remote_interrupt by send_interrupt_sequence. Re-check
check_quit_flag after waiting for a packet.
(getpkt_or_notif_sane_1): Check check_quit_flag after waiting for
a packet.
(remote_get_trace_status): New variable retry, gracefully handle
invalid input.
* ser-base.c (ser_base_wait_for): For EINTR return SERIAL_TIMEOUT.
(do_ser_base_readchar): For check_quit_flag return SERIAL_TIMEOUT.
gdb/gdbserver/
2013-06-17 Jan Kratochvil <jan.kratochvil@redhat.com>
* linux-low.c (linux_request_interrupt): New function comment.
* remote-utils.c (putpkt_binary_1, input_interrupt): Provide
REMOTE_DEBUG output for CTRL-C.
(input_interrupt):
(getpkt): New parameter may_return_interrupt, document it,
implement it.
(look_up_one_symbol, relocate_instruction): Update the callers.
* server.c (process_serial_event): Check getpkt CTRL-C event.
* server.h (getpkt): New parameter may_return_interrupt.
gdb/testsuite/
2013-06-17 Jan Kratochvil <jan.kratochvil@redhat.com>
* gdb.server/server-interrupt.c: New file.
* gdb.server/server-interrupt.exp: New file.
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index bb7298a..2fc3ccb 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -4838,6 +4838,19 @@ linux_look_up_symbols (void)
#endif
}
+/* Interrupt inferior upon receiving CTRL-C by the remote protocol.
+
+ Remote protocol specification says: If the target supports debugging
+ of multiple threads and/or processes, it should attempt to interrupt
+ all currently-executing threads and processes.
+ But it is not followed here.
+
+ Remote protocol specification says: Interrupts received while the
+ program is stopped are discarded.
+ But it is not followed here, LWP->STOPPED is not being checked.
+ GDB even sends an inferior resume request after a CTRL-C request and
+ expects to get notification inferior got SIGINT. */
+
static void
linux_request_interrupt (void)
{
diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c
index 3f055cf..c04b471 100644
--- a/gdb/gdbserver/remote-utils.c
+++ b/gdb/gdbserver/remote-utils.c
@@ -874,8 +874,16 @@ putpkt_binary_1 (char *buf, int cnt, int is_notif)
}
/* Check for an input interrupt while we're here. */
- if (cc == '\003' && current_inferior != NULL)
- (*the_target->request_interrupt) ();
+ if (cc == '\003')
+ {
+ if (remote_debug)
+ {
+ fprintf (stderr, "[putpkt: received interrupt request]\n");
+ fflush (stderr);
+ }
+ if (current_inferior != NULL)
+ (*the_target->request_interrupt) ();
+ }
}
while (cc != '+');
@@ -936,7 +944,13 @@ input_interrupt (int unused)
return;
}
- (*the_target->request_interrupt) ();
+ if (remote_debug)
+ {
+ fprintf (stderr, "[input_interrupt: received interrupt request]\n");
+ fflush (stderr);
+ }
+ if (current_inferior != NULL)
+ (*the_target->request_interrupt) ();
}
}
@@ -1118,10 +1132,13 @@ reschedule (void)
}
/* Read a packet from the remote machine, with error checking,
- and store it in BUF. Returns length of packet, or negative if error. */
+ and store it in BUF. Returns length of packet, or negative if error.
+ If MAY_RETURN_INTERRUPT is non-zero function will return -2 if
+ interrupt request (CTRL-C) has been received; inferior has been
+ already sent the interruption independently on MAY_RETURN_INTERRUPT. */
int
-getpkt (char *buf)
+getpkt (char *buf, int may_return_interrupt)
{
char *bp;
unsigned char csum, c1, c2;
@@ -1136,6 +1153,22 @@ getpkt (char *buf)
c = readchar ();
if (c == '$')
break;
+
+ /* Check for an input interrupt while we're here. */
+ if (c == '\003')
+ {
+ if (remote_debug)
+ {
+ fprintf (stderr, "[getpkt: received interrupt request]\n");
+ fflush (stderr);
+ }
+ if (current_inferior != NULL)
+ (*the_target->request_interrupt) ();
+ if (may_return_interrupt)
+ return -2;
+ continue;
+ }
+
if (remote_debug)
{
fprintf (stderr, "[getpkt: discarding char '%c']\n", c);
@@ -1614,7 +1647,7 @@ look_up_one_symbol (const char *name, CORE_ADDR *addrp, int may_ask_gdb)
return -1;
/* FIXME: Eventually add buffer overflow checking (to getpkt?) */
- len = getpkt (own_buf);
+ len = getpkt (own_buf, 0);
if (len < 0)
return -1;
@@ -1638,7 +1671,7 @@ look_up_one_symbol (const char *name, CORE_ADDR *addrp, int may_ask_gdb)
free (mem_buf);
if (putpkt (own_buf) < 0)
return -1;
- len = getpkt (own_buf);
+ len = getpkt (own_buf, 0);
if (len < 0)
return -1;
}
@@ -1697,7 +1730,7 @@ relocate_instruction (CORE_ADDR *to, CORE_ADDR oldloc)
return -1;
/* FIXME: Eventually add buffer overflow checking (to getpkt?) */
- len = getpkt (own_buf);
+ len = getpkt (own_buf, 0);
if (len < 0)
return -1;
@@ -1740,7 +1773,7 @@ relocate_instruction (CORE_ADDR *to, CORE_ADDR oldloc)
free (mem_buf);
if (putpkt (own_buf) < 0)
return -1;
- len = getpkt (own_buf);
+ len = getpkt (own_buf, 0);
if (len < 0)
return -1;
}
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 4a1d1dc..d59edc3 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -3105,7 +3105,13 @@ process_serial_event (void)
disable_async_io ();
response_needed = 0;
- packet_len = getpkt (own_buf);
+ packet_len = getpkt (own_buf, 1);
+ if (packet_len == -2)
+ {
+ /* CTRL-C has been received, return to the main loop as we may get SIGINT
+ notification from the inferior now. */
+ return 0;
+ }
if (packet_len <= 0)
{
remote_close ();
diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
index 18d060c..bed14e7 100644
--- a/gdb/gdbserver/server.h
+++ b/gdb/gdbserver/server.h
@@ -291,7 +291,7 @@ char *write_ptid (char *buf, ptid_t ptid);
int putpkt (char *buf);
int putpkt_binary (char *buf, int len);
int putpkt_notif (char *buf);
-int getpkt (char *buf);
+int getpkt (char *buf, int may_return_interrupt);
void remote_prepare (char *name);
void remote_open (char *name);
void remote_close (void);
diff --git a/gdb/remote.c b/gdb/remote.c
index 080d048..990c9bb 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -4993,7 +4993,11 @@ static void
handle_remote_sigint (int sig)
{
signal (sig, handle_remote_sigint_twice);
- mark_async_signal_handler (sigint_remote_token);
+
+ if (target_can_async_p ())
+ mark_async_signal_handler (sigint_remote_token);
+ else
+ set_quit_flag ();
}
/* Signal handler for SIGINT, installed after SIGINT has already been
@@ -5014,7 +5018,8 @@ async_remote_interrupt (gdb_client_data arg)
if (remote_debug)
fprintf_unfiltered (gdb_stdlog, "async_remote_interrupt called\n");
- target_stop (inferior_ptid);
+ gdb_assert (target_can_async_p ());
+ send_interrupt_sequence ();
}
/* Perform interrupt, if the first attempt did not succeed. Just give
@@ -5051,7 +5056,13 @@ remote_interrupt (int signo)
/* If this doesn't work, try more severe steps. */
signal (signo, remote_interrupt_twice);
- gdb_call_async_signal_handler (sigint_remote_token, 1);
+ if (target_can_async_p ())
+ mark_async_signal_handler (sigint_remote_token);
+ else
+ {
+ /* Behave like handle_sigint. */
+ set_quit_flag ();
+ }
}
/* The user typed ^C twice. */
@@ -5060,7 +5071,13 @@ static void
remote_interrupt_twice (int signo)
{
signal (signo, ofunc);
- gdb_call_async_signal_handler (sigint_remote_twice_token, 1);
+ if (target_can_async_p ())
+ mark_async_signal_handler (sigint_remote_twice_token);
+ else
+ {
+ /* Behave like handle_sigint. */
+ set_quit_flag ();
+ }
signal (signo, remote_interrupt);
}
@@ -5941,8 +5958,8 @@ remote_wait_as (ptid_t ptid, struct target_waitstatus *status, int options)
pretend that it was hit right here. */
if (check_quit_flag ())
{
- clear_quit_flag ();
- remote_interrupt (SIGINT);
+ /* check_quit_flag has already cleared itself. */
+ send_interrupt_sequence ();
}
}
@@ -5954,7 +5971,15 @@ remote_wait_as (ptid_t ptid, struct target_waitstatus *status, int options)
wait_forever_enabled_p, &is_notif);
if (!target_is_async_p ())
- signal (SIGINT, ofunc);
+ {
+ signal (SIGINT, ofunc);
+ if (ret == -1 && check_quit_flag ())
+ {
+ /* check_quit_flag has already cleared itself. */
+ send_interrupt_sequence ();
+ goto again;
+ }
+ }
/* GDB gets a notification. Return to core as this event is
not interesting. */
@@ -7676,6 +7701,12 @@ getpkt_or_notif_sane_1 (char **buf, long *sizeof_buf, int forever,
return -1; /* Don't complain, it's normal to not get
anything in this case. */
+ if (check_quit_flag ())
+ {
+ set_quit_flag ();
+ return -1;
+ }
+
if (forever) /* Watchdog went off? Kill the target. */
{
QUIT;
@@ -10837,6 +10868,7 @@ remote_get_trace_status (struct trace_status *ts)
extern int trace_regblock_size;
volatile struct gdb_exception ex;
enum packet_result result;
+ int retry;
if (remote_protocol_packets[PACKET_qTStatus].support == PACKET_DISABLE)
return -1;
@@ -10845,31 +10877,38 @@ remote_get_trace_status (struct trace_status *ts)
putpkt ("qTStatus");
- TRY_CATCH (ex, RETURN_MASK_ERROR)
- {
- p = remote_get_noisy_reply (&target_buf, &target_buf_size);
- }
- if (ex.reason < 0)
+ for (retry = 0; retry <= 1; retry++)
{
- if (ex.error != TARGET_CLOSE_ERROR)
+ TRY_CATCH (ex, RETURN_MASK_ERROR)
{
- exception_fprintf (gdb_stderr, ex, "qTStatus: ");
- return -1;
+ p = remote_get_noisy_reply (&target_buf, &target_buf_size);
+ }
+ if (ex.reason < 0)
+ {
+ if (ex.error != TARGET_CLOSE_ERROR)
+ {
+ exception_fprintf (gdb_stderr, ex, "qTStatus: ");
+ return -1;
+ }
+ throw_exception (ex);
}
- throw_exception (ex);
- }
- result = packet_ok (p, &remote_protocol_packets[PACKET_qTStatus]);
+ result = packet_ok (p, &remote_protocol_packets[PACKET_qTStatus]);
- /* If the remote target doesn't do tracing, flag it. */
- if (result == PACKET_UNKNOWN)
- return -1;
+ /* If the remote target doesn't do tracing, flag it. */
+ if (result == PACKET_UNKNOWN)
+ return -1;
- /* We're working with a live target. */
- ts->filename = NULL;
+ /* We're working with a live target. */
+ ts->filename = NULL;
- if (*p++ != 'T')
- error (_("Bogus trace status reply from target: %s"), target_buf);
+ if (*p++ == 'T')
+ break;
+ if (retry == 0)
+ warning (_("Bogus trace status reply from target: %s"), target_buf);
+ else
+ error (_("Bogus trace status reply from target: %s"), target_buf);
+ }
/* Function 'parse_trace_status' sets default value of each field of
'ts' at first, so we don't have to do it here. */
diff --git a/gdb/ser-base.c b/gdb/ser-base.c
index 52c5726..9a2e188 100644
--- a/gdb/ser-base.c
+++ b/gdb/ser-base.c
@@ -229,10 +229,8 @@ ser_base_wait_for (struct serial *scb, int timeout)
if (numfds <= 0)
{
- if (numfds == 0)
+ if (numfds == 0 || errno == EINTR)
return SERIAL_TIMEOUT;
- else if (errno == EINTR)
- continue;
else
return SERIAL_ERROR; /* Got an error from select or
poll. */
@@ -350,6 +348,12 @@ do_ser_base_readchar (struct serial *scb, int timeout)
status = SERIAL_TIMEOUT;
break;
}
+ else if (check_quit_flag ())
+ {
+ set_quit_flag ();
+ status = SERIAL_TIMEOUT;
+ break;
+ }
/* We also need to check and consume the stderr because it could
come before the stdout for some stubs. If we just sit and wait
diff --git a/gdb/testsuite/gdb.server/server-interrupt.c b/gdb/testsuite/gdb.server/server-interrupt.c
new file mode 100644
index 0000000..ffa09e4
--- /dev/null
+++ b/gdb/testsuite/gdb.server/server-interrupt.c
@@ -0,0 +1,26 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2013 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>
+
+int
+main (void)
+{
+ alarm (60);
+
+ for (;;); /* loop-line */
+}
diff --git a/gdb/testsuite/gdb.server/server-interrupt.exp b/gdb/testsuite/gdb.server/server-interrupt.exp
new file mode 100644
index 0000000..412b4ca
--- /dev/null
+++ b/gdb/testsuite/gdb.server/server-interrupt.exp
@@ -0,0 +1,142 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2013 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/>.
+
+load_lib gdbserver-support.exp
+
+standard_testfile
+
+if {[skip_gdbserver_tests]} {
+ return 0
+}
+
+if { [build_executable ${testfile}.exp ${testfile}] == -1 } {
+ return -1
+}
+
+proc do_test { nonstop } {
+ global testfile gdb_prompt binfile
+
+ # Make sure we're disconnected, in case we're testing with an
+ # extended-remote board, therefore already connected.
+ gdb_test "disconnect" ".*"
+
+ # 'gdbserver_run ""' does not handle async mode properly.
+ set res [gdbserver_spawn ${binfile}]
+ set gdbserver_protocol [lindex $res 0]
+ set gdbserver_gdbport [lindex $res 1]
+
+ set test "target $gdbserver_protocol"
+ gdb_test_multiple "target $gdbserver_protocol $gdbserver_gdbport" $test {
+ -re "Remote debugging using .*\r\n$gdb_prompt " {
+ # Do not anchor end, there may be more output in non-stop mode.
+ pass $test
+ }
+ }
+
+ if $nonstop {
+ set test "non-stop interior stop"
+ gdb_test_multiple "" $test {
+ -re " #1 stopped\\.\r\n" {
+ pass $test
+ }
+ }
+ }
+
+ gdb_breakpoint "${testfile}.c:[gdb_get_line_number "loop-line" ${testfile}.c]" \
+ temporary
+
+ gdb_continue_to_breakpoint "loop-line" ".* loop-line .*"
+
+ gdb_test_no_output "set range-stepping off"
+ gdb_test_no_output "set debug remote 1"
+ gdb_test_no_output "set debug infrun 1"
+
+ for {set pass 0} {$pass < 100} {incr pass} {
+
+ set test "run a bit #$pass"
+ set abort 1
+ gdb_test_multiple "step" $test {
+ -re "infrun: stepping inside range" {
+ # Suppress pass $test
+ verbose -log "$test"
+ set abort 0
+ }
+ }
+ if $abort {
+ return
+ }
+
+ send_gdb "\003"
+
+ set test "expect stop #$pass"
+ set abort 1
+ set stepping 0
+ gdb_test_multiple "" $test {
+ -re "loop-line .*\r\n$gdb_prompt " {
+ # Do not anchor the end - there may be more output in async mode.
+ # Suppress pass $test
+ verbose -log "$test"
+ set abort 0
+ }
+ -re "infrun: stepping inside range" {
+ incr stepping
+ if { $stepping > 20 } {
+ fail "$test (stepping inside range $stepping times)"
+ } else {
+ exp_continue
+ }
+ }
+ }
+ if $abort {
+ return
+ }
+ }
+ pass "$pass ctrl-c passes"
+
+ set test "continue"
+ gdb_test_multiple $test $test {
+ -re "\r\nContinuing\\.\r\n.*infrun: resume " {
+ pass $test
+ }
+ }
+ send_gdb "\003"
+ set test "expect stop from continue"
+ gdb_test_multiple "" $test {
+ -re "warning: Invalid remote reply:" {
+ # $gdb_prompt is not caught here but GDB gets terminated anyway.
+ fail $test
+ }
+ -re "loop-line .*\r\n$gdb_prompt " {
+ # Do not anchor the end - there may be more output in async mode.
+ pass $test
+ }
+ }
+}
+
+clean_restart ${testfile}
+gdb_test_no_output "set target-async on"
+gdb_test_no_output "set non-stop on"
+with_test_prefix "async" {
+ do_test 1
+}
+
+clean_restart ${testfile}
+gdb_test_no_output "set target-async off"
+gdb_test_no_output "set non-stop off"
+with_test_prefix "sync" {
+ do_test 0
+}