[RFC,1/2] 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 in one time, that is to
say, in the GDBserver's receive buffer, '\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 another timeout fail, that is a different problem,
the next patch will fix it.

gdb/gdbserver:

2015-12-21  Yao Qi  <yao.qi@linaro.org>

	* remote-utils.c (getpkt): If c is '\003', call target hook
	request_interrupt.
---
 gdb/gdbserver/remote-utils.c | 9 +++++++++
 1 file changed, 9 insertions(+)
  

On 12/21/2015 11:30 AM, Yao Qi wrote:
> 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 in one time, that is to

By "in one time", ITYM, "at any time".

Yeah, the target may have already stopped just while the user presses
ctrl-c, and gdb sends '\003' over.  E.g., the test is doing this in
a loop:

  #1    -> vCont;s
  #2       -- process stops
  #3       -- kernel sends SIGCHLD to gdbserver
  #4    <- T05 (gdbserver processes SIGCHLD, sees SIGTRAP stop)
  #5       -- gdb processes the T05
  #6       -- watchpoint didn't trigger, re-resume.

The \003 can arrive at any time above.  For example:

  #1    -> vCont;s
  #2       -- process stops
  #3       -- kernel sends SIGCHLD to gdbserver
  #4    <- T05 (gdbserver processes SIGCHLD, sees SIGTRAP stop)
+ #4.1  -> \003 (user presses ctrl-c)
  #5       -- gdb processes the T05
  #6       -- watchpoint didn't trigger, re-resume.

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).


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.

> say, in the GDBserver's receive buffer, '\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 another timeout fail, that is a different problem,
> the next patch will fix it.
> 
> gdb/gdbserver:
> 
> 2015-12-21  Yao Qi  <yao.qi@linaro.org>
> 
> 	* remote-utils.c (getpkt): If c is '\003', call target hook
> 	request_interrupt.
> ---
>  gdb/gdbserver/remote-utils.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> 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.)

> +	    }
> +
>  	  if (c == '$')
>  	    break;
>  	  if (remote_debug)
> 

Thanks,
Pedro Alves