[patchv2 2/2] Fix CTRL-C for remote.c (PR remote/15297)

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

Hi Pedro,

there was a related patch:
	[patch] stepping over an infinite source line
	http://sourceware.org/ml/gdb-patches/2013-05/msg00933.html
	Message-ID: <EB3B29AD43CA924DA27099BC85192376E06098EE@EU-MBX-03.mgc.mentorg.com>

which this patch is based on - the patch above is a pre-requisite.

Some comparison of the patches I gave in:
	Re: [patch] Fix CTRL-C for remote.c (PR remote/15297)
	http://sourceware.org/ml/gdb-patches/2013-06/msg00587.html
	Message-ID: <20130621125631.GA8612@host2.jankratochvil.net>

Primarily this patch removes some heavy functions from the signal handlers as
signal handlers can call only few signal-safe functions according to POSIX.
Currently with "set debug remote 1" CTRL-C typically locks up on malloc where
SIGINT handler interrupted also malloc.

As discussed with Sergio one cannot effectively mark_async_signal_handler in
sync mode as such handler gets called only after current command returns to
main loop.  Therefore sync interrupts call set_quit_flag instead.

Another problem is that current getpkt_or_notif_sane_1 insists on retrying the
reads even of CTRL-C was hit.  The change affecting several functions ensures
they all return with SERIAL_TIMEOUT or -1 and check_quit_flag () set.

remote_get_trace_status is a bit problematic as it is called after CTRL-C was
detected and GDB tries to stop.  In a longterm it may be useful to optimize
all the calls of remote_get_trace_status involing remote send + receive of
a packet.  But in this patch I just ensured remote_get_trace_status can cope
with pending CTRL-C and preserves pending CTRL-C.

The testcase does not work perfectly for target-async + all-stop, it is not
being tested.  I did not find it a commonly used mode and it may be fixed in
a different/additional patch.  It works in general but not in 100% cases.
But maybe that possible FAIL is even a correct behavior which should be just
added to the expected cases, I did not spend more time on it.  There should
not be a regression due to it.

No regressions on {x86_64,x86_64-m32,i686}-fedora19pre-linux-gnu and in
gdbserver mode.


Thanks,
Jan


gdb/
2013-06-30  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* remote.c (sync_remote_interrupt_twice): Remove the declaration.
	(struct remote_state): Update comment for ctrlc_pending_p.
	(async_handle_remote_sigint, async_handle_remote_sigint_twice): New
	gdb_assert.  Set CTRLC_PENDING_P.
	(async_remote_interrupt, async_remote_interrupt_twice): New gdb_assert.
	(sync_remote_interrupt): Remove reference to
	sync_remote_interrupt_twice.  New gdb_assert.  Replace
	gdb_call_async_signal_handler by setting CTRLC_PENDING_P and
	a set_quit_flag call.
	(sync_remote_interrupt_twice): Remove the function.
	(remote_wait_ns): Call set_quit_flag if CTRLC_PENDING_P.
	(remote_wait_as): Remove redundant clear_quit_flag call.  Replace
	sync_remote_interrupt call by interrupt_query or
	send_interrupt_sequence calls.  Re-run if check_quit_flag ().
	(getpkt_or_notif_sane_1): Return -1 if check_quit_flag ().
	(remote_get_trace_status): New variable quit_seen, handle transparently
	quits.
	* ser-base.c (ser_base_wait_for): Return SERIAL_TIMEOUT on EINTR.
	(do_ser_base_readchar): Return SERIAL_TIMEOUT if check_quit_flag ().

gdb/testsuite/
2013-06-30  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.server/server-interrupt.c (main):
	* gdb.server/server-interrupt.exp:

--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -135,8 +135,6 @@ static void remote_async (void (*callback) (enum inferior_event_type event_type,
 
 static void remote_detach (struct target_ops *ops, char *args, int from_tty);
 
-static void sync_remote_interrupt_twice (int signo);
-
 static void interrupt_query (void);
 
 static void set_general_thread (struct ptid ptid);
@@ -360,7 +358,7 @@ struct remote_state
      non-empty annex.  */
   int augmented_libraries_svr4_read;
 
-  /* Nonzero if the user has pressed Ctrl-C, but the target hasn't
+  /* How many times the user has pressed Ctrl-C, but the target hasn't
      responded to that.  */
   int ctrlc_pending_p;
 };
@@ -4991,6 +4989,10 @@ static void
 async_handle_remote_sigint (int sig)
 {
   signal (sig, async_handle_remote_sigint_twice);
+
+  gdb_assert (target_can_async_p ());
+
+  get_remote_state ()->ctrlc_pending_p++;
   mark_async_signal_handler (async_sigint_remote_token);
 }
 
@@ -5001,6 +5003,10 @@ static void
 async_handle_remote_sigint_twice (int sig)
 {
   signal (sig, async_handle_remote_sigint);
+
+  gdb_assert (target_can_async_p ());
+
+  get_remote_state ()->ctrlc_pending_p++;
   mark_async_signal_handler (async_sigint_remote_twice_token);
 }
 
@@ -5012,6 +5018,8 @@ async_remote_interrupt (gdb_client_data arg)
   if (remote_debug)
     fprintf_unfiltered (gdb_stdlog, "async_remote_interrupt called\n");
 
+  gdb_assert (target_can_async_p ());
+
   target_stop (inferior_ptid);
 }
 
@@ -5023,6 +5031,8 @@ async_remote_interrupt_twice (gdb_client_data arg)
   if (remote_debug)
     fprintf_unfiltered (gdb_stdlog, "async_remote_interrupt_twice called\n");
 
+  gdb_assert (target_can_async_p ());
+
   interrupt_query ();
 }
 
@@ -5047,19 +5057,12 @@ static void
 sync_remote_interrupt (int signo)
 {
   /* If this doesn't work, try more severe steps.  */
-  signal (signo, sync_remote_interrupt_twice);
-
-  gdb_call_async_signal_handler (async_sigint_remote_token, 1);
-}
+  signal (signo, sync_remote_interrupt);
 
-/* The user typed ^C twice.  */
+  gdb_assert (!target_is_async_p ());
 
-static void
-sync_remote_interrupt_twice (int signo)
-{
-  signal (signo, ofunc);
-  gdb_call_async_signal_handler (async_sigint_remote_twice_token, 1);
-  signal (signo, sync_remote_interrupt);
+  get_remote_state ()->ctrlc_pending_p++;
+  set_quit_flag ();
 }
 
 /* Non-stop version of target_stop.  Uses `vCont;t' to stop a remote
@@ -5856,6 +5859,12 @@ remote_wait_ns (ptid_t ptid, struct target_waitstatus *status, int options)
   int ret;
   int is_notif = 0;
 
+  if (rs->ctrlc_pending_p)
+    {
+      rs->ctrlc_pending_p = 0;
+      set_quit_flag ();
+    }
+
   /* If in non-stop mode, get out of getpkt even if a
      notification is received.	*/
 
@@ -5939,8 +5948,12 @@ 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 ();
-	      sync_remote_interrupt (SIGINT);
+	      if (!rs->ctrlc_pending_p)
+		rs->ctrlc_pending_p = 1;
+	      if (rs->ctrlc_pending_p > 1)
+		interrupt_query ();
+	      else
+		send_interrupt_sequence ();
 	    }
 	}
 
@@ -5954,6 +5967,12 @@ remote_wait_as (ptid_t ptid, struct target_waitstatus *status, int options)
       if (!target_is_async_p ())
 	signal (SIGINT, ofunc);
 
+      if (ret == -1 && check_quit_flag ())
+	{
+	  set_quit_flag ();
+	  goto again;
+	}
+
       /* GDB gets a notification.  Return to core as this event is
 	 not interesting.  */
       if (ret != -1 && is_notif)
@@ -7685,6 +7704,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;
@@ -10846,26 +10871,55 @@ remote_get_trace_status (struct trace_status *ts)
   extern int trace_regblock_size;
   volatile struct gdb_exception ex;
   enum packet_result result;
+  int quit_seen;
 
   if (remote_protocol_packets[PACKET_qTStatus].support == PACKET_DISABLE)
     return -1;
 
   trace_regblock_size = get_remote_arch_state ()->sizeof_g_packet;
 
-  putpkt ("qTStatus");
-
-  TRY_CATCH (ex, RETURN_MASK_ERROR)
+  if (check_quit_flag ())
     {
-      p = remote_get_noisy_reply (&target_buf, &target_buf_size);
+      /* remote_get_noisy_reply would return immediately leaving unexpected
+	 qTStatus reply in the serial input queue.  */
+      set_quit_flag ();
+      return -1;
     }
-  if (ex.reason < 0)
+
+  putpkt ("qTStatus");
+
+  /* CTRL-C may also happen inside remote_get_noisy_reply, try to read the
+     remaining packet and call quit only afterwards.  If gdbserver got stuck we
+     abort the read on second CTRL-C.  */
+  quit_seen = 0;
+  for (;;)
     {
-      if (ex.error != TARGET_CLOSE_ERROR)
+      TRY_CATCH (ex, RETURN_MASK_ALL)
 	{
-	  exception_fprintf (gdb_stderr, ex, "qTStatus: ");
-	  return -1;
+	  p = remote_get_noisy_reply (&target_buf, &target_buf_size);
 	}
-      throw_exception (ex);
+      if (ex.reason == RETURN_QUIT
+	  || (ex.reason >= 0 && strcmp (p, "timeout") == 0
+	      && check_quit_flag ()))
+	{
+	  if (quit_seen)
+	    quit ();
+
+	  quit_seen = 1;
+	  continue;
+	}
+      if (quit_seen)
+	set_quit_flag ();
+      if (ex.reason < 0)
+	{
+	  if (ex.error != TARGET_CLOSE_ERROR)
+	    {
+	      exception_fprintf (gdb_stderr, ex, "qTStatus: ");
+	      return -1;
+	    }
+	  throw_exception (ex);
+	}
+      break;
     }
 
   result = packet_ok (p, &remote_protocol_packets[PACKET_qTStatus]);
--- 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
--- /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 */
+}
--- /dev/null
+++ b/gdb/testsuite/gdb.server/server-interrupt.exp
@@ -0,0 +1,173 @@
+# 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 ctrl-c #$pass"
+	set abort 1
+	set stepping 0
+	gdb_test_multiple "" $test {
+	    -re "\\^C" {
+		verbose -log "$test"
+		set abort 0
+	    }
+	    -re "infrun: stepping inside range" {
+		incr stepping
+		if { $stepping > 200 } {
+		    fail "$test (stepping inside range $stepping times)"
+		} else {
+		    exp_continue
+		}
+	    }
+	}
+	if $abort {
+	    return
+	}
+
+	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 "\\\[Thread \[0-9\]+\\\] #\[0-9\]+ stopped\\..*\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 "\\\[Thread \[0-9\]+\\\] #\[0-9\]+ stopped\\..*\r\n$gdb_prompt " {
+	    # Do not anchor the end - there may be more output in async mode.
+	    pass $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-nonstop" {
+    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-allstop" {
+    do_test 0
+}