Re: [RFC 1/2] Check input interrupt first when reading packet

[Yao Qi <qiyaoltc at gmail dot com>]

Pedro Alves <palves@redhat.com> writes:

Hi Pedro,
sorry for the delayed reply.  Takes much time reading these patches and
discussions in archives.

> The trouble is that the manual says:
>
>   "Interrupts received while the program is stopped are discarded."
>
> However, I can't see how that can work in general.  This can also happen:
>
>   #1    -> vCont;s
>   #2       -- process stops
> + #2.1  -> \003 (user presses ctrl-c)
>   #3       -- kernel sends SIGCHLD to gdbserver
>   #4    <- T05 (gdbserver processes SIGCHLD, sees SIGTRAP stop)
>   #5       -- gdb processes the T05
>
> And in that case, if the SIGTRAP happens to cause a user-visible
> stop (e.g., a breakpoint hit), then the target ends up with
> a SIGINT queued, that is only seen on next resume (e.g., after
> the next "continue").  This is similar to the reason we print
>
>  "Quit (expect signal SIGINT when the program is resumed)"
>
> on some ports (utils.c:quit).

This line of doc was added due to the request from Daniel's comments
https://www.sourceware.org/ml/gdb/2005-11/msg00349.html however, I am
not sure what is the conversation Daniel referred to.
>
>
> I once wrote a patch that would fix this while preserving that
> "while the program is stopped are discarded" invariant:
>
>  https://sourceware.org/ml/gdb-patches/2013-05/msg00933.html
>  https://sourceware.org/ml/gdb-patches/2013-05/msg00933/step_over.patch
>
> See long rationale at:
>
>  https://sourceware.org/ml/gdb-patches/2013-05/txtHFb6rkZ8Dz.txt
>
> ... by making both gdb and gdbserver remember that the user pressed ctrl-c.
>
> But that was not complete -- a fuller fix would fix the user-interface
> issue of sometimes getting "Quit" instead of a SIGINT.
>
> The simpler approach of "not ignoring ctrl-c when stopped" is quite tempting.
> Given that the issue of a ctrl-c happening while the process had just stopped
> ending up producing an unexpected SIGINT when the program is next resumed can
> also happen with native debugging and "attach", maybe the simpler approach
> of always queueing is the right approach.  We'll need to tweak the
> documentation though.

I am inclined to tweak the doc as well, because looks people at that
moment believed that ^C is meaningless when the target is stopped.
See https://www.sourceware.org/ml/gdb-patches/2005-11/msg00307.html

>> diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c
>> index 05e3d63..8bb5b13 100644
>> --- a/gdb/gdbserver/remote-utils.c
>> +++ b/gdb/gdbserver/remote-utils.c
>> @@ -959,6 +959,15 @@ getpkt (char *buf)
>>        while (1)
>>  	{
>>  	  c = readchar ();
>> +
>> +	  /* The '\003' may appear before or after each packet, so
>> +	     check for an input interrupt.  */
>> +	  if (c == '\003')
>> +	    {
>> +	      (*the_target->request_interrupt) ();
>> +	      c = readchar ();
>
> I'd write "continue;" instead of another readchar,
>
> (Pedantically, you could have another '\003' in the buffer.)

Done.

-- 
Yao (éå)
From 266bf16a08d3ae1be6157cf360486d277e2820d4 Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Fri, 18 Dec 2015 12:29:29 +0000
Subject: [PATCH] Check input interrupt first when reading packet

Hi,
I see timeout in one of several runs of random-signal.exp like this,

 $ (set -e; while true; do make check RUNTESTFLAGS="--target_board=native-gdbserver random-signal.exp"; done)

In about every five runs, we can see a fail,

PASS: gdb.base/random-signal.exp: continue
^CFAIL: gdb.base/random-signal.exp: stop with control-c (timeout)

after some investigation, I find '\003' may be discarded by GDBserver when
it is expecting '$'.  In GDB side, both normal packets and '\003' are sent
via function send, but GDBserver may receive them at any time, that is to
say, in the receive buffer in GDBserver, '\003' may appear before or after
normal packet.  However, current GDBserver doesn't handle this case.

With this patch applied, I don't see this fail in multiple runs.
Although there is still timeout fail, that is a different problem, the
next patch will fix it.

gdb/gdbserver:

2016-01-05  Yao Qi  <yao.qi@linaro.org>

	* remote-utils.c (getpkt): If c is '\003', call target hook
	request_interrupt.

diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c
index 5f43820..c5f4647 100644
--- a/gdb/gdbserver/remote-utils.c
+++ b/gdb/gdbserver/remote-utils.c
@@ -959,6 +959,15 @@ getpkt (char *buf)
       while (1)
 	{
 	  c = readchar ();
+
+	  /* The '\003' may appear before or after each packet, so
+	     check for an input interrupt.  */
+	  if (c == '\003')
+	    {
+	      (*the_target->request_interrupt) ();
+	      continue;
+	    }
+
 	  if (c == '$')
 	    break;
 	  if (remote_debug)