[PATCH] Fix fail in gdb.base/interrupt-noterm.exp

[PATCH] Fix fail in gdb.base/interrupt-noterm.exp

[Yao Qi]

Hi,
In my testing, I see the following fail intermittently,

interrupt
(gdb) PASS: gdb.base/interrupt-noterm.exp: interrupt
[Inferior 1 (process 13407) exited normally]

Child exited with status 0
FAIL: gdb.base/interrupt-noterm.exp: inferior received SIGINT (timeout)

because the interrupt packet may be sent to GDBserver before the SIGIO
handler is installed.  The fix in this patch is to let GDB wait
for 500 ms between "continue &" and "interrupt" to make sure
SIGIO handler is installed already in GDBserver side.

gdb/testsuite:

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

	* gdb.base/interrupt-noterm.exp: Add "after 500".
---
 gdb/testsuite/gdb.base/interrupt-noterm.exp | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/gdb/testsuite/gdb.base/interrupt-noterm.exp b/gdb/testsuite/gdb.base/interrupt-noterm.exp
index 05f6076..9b5bb17 100644
--- a/gdb/testsuite/gdb.base/interrupt-noterm.exp
+++ b/gdb/testsuite/gdb.base/interrupt-noterm.exp
@@ -55,6 +55,9 @@ if { $async_supported < 0 } {
     return 1
 }
 
+# Wait a while so that GDBserver's SIGIO handler is in place.
+after 500
+
 # With native debugging, and no terminal (emulated by interactive-mode
 # off, above), GDB had a bug where "interrupt" would send SIGINT to
 # its own process group, instead of the inferior's.
-- 
1.9.1

Re: [PATCH] Fix fail in gdb.base/interrupt-noterm.exp

[Pedro Alves]

On 01/22/2016 04:29 PM, Yao Qi wrote:
> Hi,
> In my testing, I see the following fail intermittently,
> 
> interrupt
> (gdb) PASS: gdb.base/interrupt-noterm.exp: interrupt
> [Inferior 1 (process 13407) exited normally]
> 
> Child exited with status 0
> FAIL: gdb.base/interrupt-noterm.exp: inferior received SIGINT (timeout)
> 
> because the interrupt packet may be sent to GDBserver before the SIGIO
> handler is installed.  The fix in this patch is to let GDB wait
> for 500 ms between "continue &" and "interrupt" to make sure
> SIGIO handler is installed already in GDBserver side.

Can you expand the rationale some more?

E.g., why is this not a gdbserver bug?  Instintively I'd say it is.

Thanks,
Pedro Alves

> 
> gdb/testsuite:
> 
> 2016-01-22  Yao Qi  <yao.qi@linaro.org>
> 
> 	* gdb.base/interrupt-noterm.exp: Add "after 500".
> ---
>  gdb/testsuite/gdb.base/interrupt-noterm.exp | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/gdb/testsuite/gdb.base/interrupt-noterm.exp b/gdb/testsuite/gdb.base/interrupt-noterm.exp
> index 05f6076..9b5bb17 100644
> --- a/gdb/testsuite/gdb.base/interrupt-noterm.exp
> +++ b/gdb/testsuite/gdb.base/interrupt-noterm.exp
> @@ -55,6 +55,9 @@ if { $async_supported < 0 } {
>      return 1
>  }
>  
> +# Wait a while so that GDBserver's SIGIO handler is in place.
> +after 500
> +
>  # With native debugging, and no terminal (emulated by interactive-mode
>  # off, above), GDB had a bug where "interrupt" would send SIGINT to
>  # its own process group, instead of the inferior's.
> 

Re: [PATCH] Fix fail in gdb.base/interrupt-noterm.exp

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> Can you expand the rationale some more?
>
> E.g., why is this not a gdbserver bug?  Instintively I'd say it is.

The interaction between GDB and GDBserver is like this,

  1. GDB sends vCont;c and doesn't wait for the stop reply because
  "continue &" is background command,
  2. GDBserver receives vCont;c, enables the async i/o (by
  enable_async_io) and resumes the inferior.
  3. GDB sends interrupt packet,

#1 happens before #2 and #3, but the order of #2 and #3 is not
determined.  If #2 happens before #3, it is fine, otherwise, the
GDBserver doesn't know the interrupt from GDB.

-- 
Yao (齐尧)

Re: [PATCH] Fix fail in gdb.base/interrupt-noterm.exp

[Pedro Alves]

On 01/22/2016 05:14 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> Can you expand the rationale some more?
>>
>> E.g., why is this not a gdbserver bug?  Instintively I'd say it is.
> 
> The interaction between GDB and GDBserver is like this,
> 
>   1. GDB sends vCont;c and doesn't wait for the stop reply because
>   "continue &" is background command,
>   2. GDBserver receives vCont;c, enables the async i/o (by
>   enable_async_io) and resumes the inferior.
>   3. GDB sends interrupt packet,
> 
> #1 happens before #2 and #3, but the order of #2 and #3 is not
> determined.  If #2 happens before #3, it is fine, otherwise, the
> GDBserver doesn't know the interrupt from GDB.

If 1. is followed by 3., then the \\003 is always read by gdb
after the vCont;c.  We call enable_async_io before reaching
mywait.  Since we're in all-stop, that means we'll block
inside mywait -> waitpid, all the while \\003 is already available to
read in the socket.  Since we're blocked in waitpid, we won't see
the \\003 until after the next time the program happens to stop.

Agree?

It still seems to me like a gdbserver bug.

I think that after calling enable_async_io, we need to check whether
input is already pending from GDB, and if so, process it immediately -- we
know the only input coming from GDB at this point is a \\003.  IOW, I think
we need to call input_interrupt after calling enable_async_io.  input_interrupt
already uses select before reading, so it handles the case of there
being no input available without blocking.

However, we need to be careful, because a SIGIO can race with calling
input_interrupt from mainline code...

Thanks,
Pedro Alves

Re: [PATCH] Fix fail in gdb.base/interrupt-noterm.exp

[Pedro Alves]

On 01/22/2016 05:35 PM, Pedro Alves wrote:
> On 01/22/2016 05:14 PM, Yao Qi wrote:
>> Pedro Alves <palves@redhat.com> writes:
>>
>>> Can you expand the rationale some more?
>>>
>>> E.g., why is this not a gdbserver bug?  Instintively I'd say it is.
>>
>> The interaction between GDB and GDBserver is like this,
>>
>>   1. GDB sends vCont;c and doesn't wait for the stop reply because
>>   "continue &" is background command,
>>   2. GDBserver receives vCont;c, enables the async i/o (by
>>   enable_async_io) and resumes the inferior.
>>   3. GDB sends interrupt packet,
>>
>> #1 happens before #2 and #3, but the order of #2 and #3 is not
>> determined.  If #2 happens before #3, it is fine, otherwise, the
>> GDBserver doesn't know the interrupt from GDB.
> 
> If 1. is followed by 3., then the \\003 is always read by gdb
> after the vCont;c.  We call enable_async_io before reaching
> mywait.  Since we're in all-stop, that means we'll block
> inside mywait -> waitpid, all the while \\003 is already available to
> read in the socket.  Since we're blocked in waitpid, we won't see
> the \\003 until after the next time the program happens to stop.
> 
> Agree?
> 
> It still seems to me like a gdbserver bug.
> 
> I think that after calling enable_async_io, we need to check whether
> input is already pending from GDB, and if so, process it immediately -- we
> know the only input coming from GDB at this point is a \\003.  IOW, I think
> we need to call input_interrupt after calling enable_async_io.  input_interrupt
> already uses select before reading, so it handles the case of there
> being no input available without blocking.
> 
> However, we need to be careful, because a SIGIO can race with calling
> input_interrupt from mainline code...

Might be simpler to always have the SIGIO handler installed (install
it early), and change enable_async_io/disable_async_io to use
sigprocmask to block/unblock the signal.  That way, if input comes
before the signal is unblocked, the handler is called immediately
when enable_async_io is called.

Thanks,
Pedro Alves

Re: [PATCH] Fix fail in gdb.base/interrupt-noterm.exp

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> If 1. is followed by 3., then the \\003 is always read by gdb

s/ready by/sent by/ ?

> after the vCont;c.  We call enable_async_io before reaching
> mywait.  Since we're in all-stop, that means we'll block

Although we call enable_async_io earlier, so the window between received
vCont;c and calling enable_async_io is tiny, it is still possible that
\\003 arrives at that period.

> inside mywait -> waitpid, all the while \\003 is already available to
> read in the socket.  Since we're blocked in waitpid, we won't see
> the \\003 until after the next time the program happens to stop.
>
> Agree?

Yes, I agree.

>
> It still seems to me like a gdbserver bug.
>
> I think that after calling enable_async_io, we need to check whether
> input is already pending from GDB, and if so, process it immediately -- we
> know the only input coming from GDB at this point is a \\003.  IOW, I think
> we need to call input_interrupt after calling enable_async_io.  input_interrupt
> already uses select before reading, so it handles the case of there
> being no input available without blocking.
>
> However, we need to be careful, because a SIGIO can race with calling
> input_interrupt from mainline code...

What you mean here is that we can call input_interrupt after calling
enable_async_io, but meanwhile, \\0003 arrives, and input_interrupt is
invoked as a SIGIO handler, so there is a race.  Is it correct?

I agree your next email about the approach of block/unblock SIGIO is
better.  I'll give a fix that way.

-- 
Yao (齐尧)

Re: [PATCH] Fix fail in gdb.base/interrupt-noterm.exp

[Pedro Alves]

On 01/25/2016 09:30 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> If 1. is followed by 3., then the \\003 is always read by gdb
> 
> s/ready by/sent by/ ?

I meant, s/by gdb/by gdbserver/.

>>
>> It still seems to me like a gdbserver bug.
>>
>> I think that after calling enable_async_io, we need to check whether
>> input is already pending from GDB, and if so, process it immediately -- we
>> know the only input coming from GDB at this point is a \\003.  IOW, I think
>> we need to call input_interrupt after calling enable_async_io.  input_interrupt
>> already uses select before reading, so it handles the case of there
>> being no input available without blocking.
>>
>> However, we need to be careful, because a SIGIO can race with calling
>> input_interrupt from mainline code...
> 
> What you mean here is that we can call input_interrupt after calling
> enable_async_io, but meanwhile, \\0003 arrives, and input_interrupt is
> invoked as a SIGIO handler, so there is a race.  Is it correct?

That's correct.

> 
> I agree your next email about the approach of block/unblock SIGIO is
> better.  I'll give a fix that way.
> 

Great, thanks!

-- 
Pedro Alves

[PATCH 0/2 V2] Fix a fail in gdb.base/interrupt-noterm.exp

[Yao Qi]

Hi,
In my test, I see the following fail intermittently,

 interrupt
 (gdb) PASS: gdb.base/interrupt-noterm.exp: interrupt
 [Inferior 1 (process 13407) exited normally]

 Child exited with status 0
 FAIL: gdb.base/interrupt-noterm.exp: inferior received SIGINT (timeout)

because interrupt character isn't processed by GDBserver in time due to
two different problems:

 1. GDBserver may receive vCont;c and '\003' together, and read them
    into the buffer.  GDBserver will wait for the inferior but there
    won't be SIGIO because GDBserver has already read '\003' into the
    buffer.

 2. GDBserver may receive vCont;c only.  GDBserver will resume the
    inferior and wait for it.  If '\003' arrives at the moment between
    GDBserver received vCont;c and install SIGIO handler, GDBserver
    can't receive SIGIO.

these two problems above cause GDB wait for the inferior and leave the
interrupt there, so the test is timeout.

Two patches in the series fix these two problems separately.  Patch 1
fixes the problem 1 by checking the buffer *after* reading in each
packet.  Patch 2 fixes the problem 2 by block/unblock SIGIO rather than
install/uninstall signal handler.

These two patches are tested on {x86_64,arm,aarch64}-linux.  No
regression.

*** BLURB HERE ***

Yao Qi (2):
  [GDBserver] Check input interrupt after reading in a packet
  [GDBserver] Block and unblock SIGIO

 gdb/gdbserver/remote-utils.c | 60 ++++++++++++++++++++++++++++----------------
 1 file changed, 39 insertions(+), 21 deletions(-)

-- 
1.9.1

[PATCH 1/2] [GDBserver] Check input interrupt after reading in a packet

[Yao Qi]

GDBserver may read some packet together with '\003' in one go.  We've
already checked '\003' first when reading packet by my patch,

  Check input interrupt first when reading packet
  https://sourceware.org/ml/gdb-patches/2016-01/msg00057.html

but if we don't check '\003' *after* each packet, the interrupt will
be processed next time GDBserver reads from the buffer, so that the
interrupt isn't processed in time.  For example, GDB sends vCont;c and
interrupt (see gdb.base/interrupt-noterm.exp), we'll resume the
inferior and wait once packet vCont;c is seen.  If we don't check the
interrupt character after vCont;c packet, interrupt character will stay
in the buffer unattended until GDBserver returns from the wait, which
may take a while.  Note that since we've read '\003' from file
descriptor, SIGIO signal handler input_interrupt doesn't help either.

This issue can be exposed by hacking the end of getpkt like
@@ -1041,6 +1050,9 @@ getpkt (char *buf)
        }
     }

+  if (readchar_bufcnt > 0)
+    gdb_assert (*readchar_bufp != '\003');
+
   return bp - buf;
 }

and this can trigger internal error,
(gdb) PASS: gdb.base/interrupt-noterm.exp: interrupt
Remote connection closed^M
(gdb) FAIL: gdb.base/interrupt-noterm.exp: inferior received SIGINT
Remote debugging from host 10.2.206.40^M
/home/yao/SourceCode/gnu/gdb/git/gdb/gdbserver/remote-utils.c:1054: A problem internal to GDBserver has been detected.^M
getpkt: Assertion `*readchar_bufp != '\003'' failed.^M

This patch is to peek the buffer, if it is '\003', consume it and call
*the_target->request_interrupt.

gdb/gdbserver:

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

	* remote-utils.c (getpkt): If the buffer isn't empty, and the
	first character is '\003', call *the_target->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 292197a..000457d 100644
--- a/gdb/gdbserver/remote-utils.c
+++ b/gdb/gdbserver/remote-utils.c
@@ -1041,6 +1041,15 @@ getpkt (char *buf)
 	}
     }
 
+  /* The '\003' may appear after some packet, and check it in the buffer,
+     so that we can process the interrupt in time.  */
+  if (readchar_bufcnt > 0 && *readchar_bufp == '\003')
+    {
+      /* Consume the interrupt character in the buffer.  */
+      readchar ();
+      (*the_target->request_interrupt) ();
+    }
+
   return bp - buf;
 }
 
-- 
1.9.1

[PATCH 2/2] [GDBserver] Block and unblock SIGIO

[Yao Qi]

Nowadays, GDBserver disables async io (by ignoring SIGIO) when process
a serial event, and enables async io (by installing signal handler) when
resume the inferior and wait.  GDBserver may miss SIGIO (by interrupt)
and doesn't process SIGIO in time, which is shown by
gdb.base/interrupt-noterm.exp.  In the test, GDB sends "continue &" and
then "interrupt".  if '\003' arrives at a period between GDBserver
receives vCont;c and enables async io, SIGIO is ignored because signal
handler isn't installed.  GDBserver waits for the inferior and can not
notice '\003' until it returns from wait.

This patch changes the code to install SIGIO handler early, but block
and unblock SIGIO as needed.  In this way, we don't remove SIGIO
handler, so SIGIO can't be ignored.  However, GDBserver needs to
remove the signal handler when connection is closed.

gdb/gdbserver:

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

	* remote-utils.c (remote_close) [!USE_WIN32API]: Ignore SIGIO.
	(unblock_async_io): Remove.
	(enable_async_io): Don't install SIGIO handler.  Unblock it
	instead.
	(disable_async_io): Don't ignore SIGIO.  Block it instead.
	(initialize_async_io): Install SIGIO handler.  Don't call
	unblock_async_io.
---
 gdb/gdbserver/remote-utils.c | 51 ++++++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 21 deletions(-)

diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c
index 000457d..541675a 100644
--- a/gdb/gdbserver/remote-utils.c
+++ b/gdb/gdbserver/remote-utils.c
@@ -402,6 +402,11 @@ remote_close (void)
 {
   delete_file_handler (remote_desc);
 
+#ifndef USE_WIN32API
+  /* Remove SIGIO handler.  */
+  signal (SIGIO, SIG_IGN);
+#endif
+
 #ifdef USE_WIN32API
   closesocket (remote_desc);
 #else
@@ -775,21 +780,9 @@ check_remote_input_interrupt_request (void)
   input_interrupt (0);
 }
 
-/* Asynchronous I/O support.  SIGIO must be enabled when waiting, in order to
-   accept Control-C from the client, and must be disabled when talking to
-   the client.  */
-
-static void
-unblock_async_io (void)
-{
-#ifndef USE_WIN32API
-  sigset_t sigio_set;
-
-  sigemptyset (&sigio_set);
-  sigaddset (&sigio_set, SIGIO);
-  sigprocmask (SIG_UNBLOCK, &sigio_set, NULL);
-#endif
-}
+/* Asynchronous I/O support.  SIGIO must be unblocked when waiting,
+   in order to accept Control-C from the client, and must be blocked
+   when talking to the client.  */
 
 #ifdef __QNX__
 static void
@@ -820,12 +813,19 @@ static int async_io_enabled;
 void
 enable_async_io (void)
 {
+#ifndef USE_WIN32API
+  sigset_t sigio_set;
+#endif
+
   if (async_io_enabled)
     return;
 
 #ifndef USE_WIN32API
-  signal (SIGIO, input_interrupt);
+  sigemptyset (&sigio_set);
+  sigaddset (&sigio_set, SIGIO);
+  sigprocmask (SIG_UNBLOCK, &sigio_set, NULL);
 #endif
+
   async_io_enabled = 1;
 #ifdef __QNX__
   nto_comctrl (1);
@@ -836,12 +836,19 @@ enable_async_io (void)
 void
 disable_async_io (void)
 {
+#ifndef USE_WIN32API
+  sigset_t sigio_set;
+#endif
+
   if (!async_io_enabled)
     return;
 
 #ifndef USE_WIN32API
-  signal (SIGIO, SIG_IGN);
+  sigemptyset (&sigio_set);
+  sigaddset (&sigio_set, SIGIO);
+  sigprocmask (SIG_BLOCK, &sigio_set, NULL);
 #endif
+
   async_io_enabled = 0;
 #ifdef __QNX__
   nto_comctrl (0);
@@ -852,12 +859,14 @@ disable_async_io (void)
 void
 initialize_async_io (void)
 {
-  /* Make sure that async I/O starts disabled.  */
+  /* Install the signal handler.  */
+#ifndef USE_WIN32API
+  signal (SIGIO, input_interrupt);
+#endif
+
+  /* Make sure that async I/O starts blocked.  */
   async_io_enabled = 1;
   disable_async_io ();
-
-  /* Make sure the signal is unblocked.  */
-  unblock_async_io ();
 }
 
 /* Internal buffer used by readchar.
-- 
1.9.1

Re: [PATCH 1/2] [GDBserver] Check input interrupt after reading in a packet

[Pedro Alves]

On 01/26/2016 09:58 AM, Yao Qi wrote:

> +  /* The '\003' may appear after some packet, and check it in the buffer,
> +     so that we can process the interrupt in time.  */

I can't parse seem to parse this correctly.

I think we should expand the explanation, like:

  /* The readchar above may have already read a '\003' out of the socket and
   moved it to the local buffer.  For example, when GDB sends vCont;c immediately
   followed by interrupt (see gdb.base/interrupt-noterm.exp).  As soon as we see
   the vCont;c, we'll resume the inferior and wait.  Since we've already moved
   the '\003' to the local buffer, SIGIO won't help.  In that case, if we don't
   check for interrupt after the vCont;c packet, the interrupt character would
   stay in the buffer unattended until after the next (unrelated) stop.  */

> +  if (readchar_bufcnt > 0 && *readchar_bufp == '\003')

This should be a "while" instead of a single "if".

> +    {
> +      /* Consume the interrupt character in the buffer.  */
> +      readchar ();
> +      (*the_target->request_interrupt) ();
> +    }
> +
>    return bp - buf;
>  }

Otherwise LGTM.

Thanks,
Pedro Alves

Re: [PATCH 2/2] [GDBserver] Block and unblock SIGIO

[Pedro Alves]

On 01/26/2016 09:58 AM, Yao Qi wrote:

> -static void
> -unblock_async_io (void)
> -{
> -#ifndef USE_WIN32API
> -  sigset_t sigio_set;
> -
> -  sigemptyset (&sigio_set);
> -  sigaddset (&sigio_set, SIGIO);
> -  sigprocmask (SIG_UNBLOCK, &sigio_set, NULL);
> -#endif
> -}
> +/* Asynchronous I/O support.  SIGIO must be unblocked when waiting,
> +   in order to accept Control-C from the client, and must be blocked
> +   when talking to the client.  */
>  
>  #ifdef __QNX__
>  static void
> @@ -820,12 +813,19 @@ static int async_io_enabled;
>  void
>  enable_async_io (void)
>  {
> +#ifndef USE_WIN32API
> +  sigset_t sigio_set;
> +#endif
> +
>    if (async_io_enabled)
>      return;
>  
>  #ifndef USE_WIN32API
> -  signal (SIGIO, input_interrupt);
> +  sigemptyset (&sigio_set);
> +  sigaddset (&sigio_set, SIGIO);
> +  sigprocmask (SIG_UNBLOCK, &sigio_set, NULL);
>  #endif
> +
>    async_io_enabled = 1;
>  #ifdef __QNX__
>    nto_comctrl (1);
> @@ -836,12 +836,19 @@ enable_async_io (void)
>  void
>  disable_async_io (void)
>  {
> +#ifndef USE_WIN32API
> +  sigset_t sigio_set;
> +#endif
> +
>    if (!async_io_enabled)
>      return;
>  
>  #ifndef USE_WIN32API
> -  signal (SIGIO, SIG_IGN);
> +  sigemptyset (&sigio_set);
> +  sigaddset (&sigio_set, SIGIO);
> +  sigprocmask (SIG_BLOCK, &sigio_set, NULL);
>  #endif
> +

I'd suggest factoring out this duplicate sigprocmask handling,
like, say, rename unblock_async_io and add a parameter:

/* Block or unblock SIGIO.  */

static void
block_unblock_async_io (int block)
{
#ifndef USE_WIN32API
  sigset_t sigio_set;

  sigemptyset (&sigio_set);
  sigaddset (&sigio_set, SIGIO);
  sigprocmask (block ? SIG_BLOCK : SIG_UNBLOCK, &sigio_set, NULL);
#endif
}

>    async_io_enabled = 0;
>  #ifdef __QNX__
>    nto_comctrl (0);
> @@ -852,12 +859,14 @@ disable_async_io (void)
>  void
>  initialize_async_io (void)
>  {
> -  /* Make sure that async I/O starts disabled.  */
> +  /* Install the signal handler.  */
> +#ifndef USE_WIN32API
> +  signal (SIGIO, input_interrupt);
> +#endif
> +
> +  /* Make sure that async I/O starts blocked.  */
>    async_io_enabled = 1;
>    disable_async_io ();

I think it's safer practice to block the signal before
installing the handler.

Otherwise LGTM.

> -
> -  /* Make sure the signal is unblocked.  */
> -  unblock_async_io ();
>  }
>  
>  /* Internal buffer used by readchar.
> 

Thanks,
Pedro Alves

Re: [PATCH 1/2] [GDBserver] Check input interrupt after reading in a packet

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> I think we should expand the explanation, like:
>
>   /* The readchar above may have already read a '\003' out of the socket and
>    moved it to the local buffer.  For example, when GDB sends vCont;c immediately
>    followed by interrupt (see gdb.base/interrupt-noterm.exp).  As soon as we see
>    the vCont;c, we'll resume the inferior and wait.  Since we've already moved
>    the '\003' to the local buffer, SIGIO won't help.  In that case, if we don't
>    check for interrupt after the vCont;c packet, the interrupt character would
>    stay in the buffer unattended until after the next (unrelated) stop.  */
>

Take it into the patch.

>> +  if (readchar_bufcnt > 0 && *readchar_bufp == '\003')
>
> This should be a "while" instead of a single "if".
>

Fixed.

>> +    {
>> +      /* Consume the interrupt character in the buffer.  */
>> +      readchar ();
>> +      (*the_target->request_interrupt) ();
>> +    }
>> +
>>    return bp - buf;
>>  }
>
> Otherwise LGTM.

Patch below is pushed in.

-- 
Yao (齐尧)
From 18879fef1741464e522624bcc529048400453e0d Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Tue, 26 Jan 2016 13:50:22 +0000
Subject: [PATCH] [GDBserver] Check input interrupt after reading in a packet

GDBserver may read some packet together with '\003' in one go.  We've
already checked '\003' first when reading packet by my patch,

  Check input interrupt first when reading packet
  https://sourceware.org/ml/gdb-patches/2016-01/msg00057.html

but if we don't check '\003' *after* each packet, the interrupt will
be processed next time GDBserver reads from the buffer, so that the
interrupt isn't processed in time.  For example, GDB sends vCont;c and
interrupt (see gdb.base/interrupt-noterm.exp), we'll resume the
inferior and wait once packet vCont;c is seen.  If we don't check the
interrupt character after vCont;c packet, interrupt character will stay
in the buffer unattended until GDBserver returns from the wait, which
may take a while.  Note that since we've read '\003' from file
descriptor, SIGIO signal handler input_interrupt doesn't help either.

This issue can be exposed by hacking the end of getpkt like
@@ -1041,6 +1050,9 @@ getpkt (char *buf)
        }
     }

+  if (readchar_bufcnt > 0)
+    gdb_assert (*readchar_bufp != '\003');
+
   return bp - buf;
 }

and this can trigger internal error,
(gdb) PASS: gdb.base/interrupt-noterm.exp: interrupt
Remote connection closed^M
(gdb) FAIL: gdb.base/interrupt-noterm.exp: inferior received SIGINT
Remote debugging from host 10.2.206.40^M
/home/yao/SourceCode/gnu/gdb/git/gdb/gdbserver/remote-utils.c:1054: A problem internal to GDBserver has been detected.^M
getpkt: Assertion `*readchar_bufp != '\003'' failed.^M

This patch is to peek the buffer, if it is '\003', consume it and call
*the_target->request_interrupt.

gdb/gdbserver:

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

	* remote-utils.c (getpkt): If the buffer isn't empty, and the
	first character is '\003', call *the_target->request_interrupt.

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 2e94aa8..4aa7350 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,8 @@
+2016-01-26  Yao Qi  <yao.qi@linaro.org>
+
+	* remote-utils.c (getpkt): If the buffer isn't empty, and the
+	first character is '\003', call *the_target->request_interrupt.
+
 2016-01-25  Yao Qi  <yao.qi@linaro.org>
 
 	* remote-utils.c (new_thread_notify): Remove.
diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c
index 292197a..ccc99f1 100644
--- a/gdb/gdbserver/remote-utils.c
+++ b/gdb/gdbserver/remote-utils.c
@@ -1041,6 +1041,22 @@ getpkt (char *buf)
 	}
     }
 
+  /* The readchar above may have already read a '\003' out of the socket
+     and moved it to the local buffer.  For example, when GDB sends
+     vCont;c immediately followed by interrupt (see
+     gdb.base/interrupt-noterm.exp).  As soon as we see the vCont;c, we'll
+     resume the inferior and wait.  Since we've already moved the '\003'
+     to the local buffer, SIGIO won't help.  In that case, if we don't
+     check for interrupt after the vCont;c packet, the interrupt character
+     would stay in the buffer unattended until after the next (unrelated)
+     stop.  */
+  while (readchar_bufcnt > 0 && *readchar_bufp == '\003')
+    {
+      /* Consume the interrupt character in the buffer.  */
+      readchar ();
+      (*the_target->request_interrupt) ();
+    }
+
   return bp - buf;
 }
 

Re: [PATCH 2/2] [GDBserver] Block and unblock SIGIO

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> I'd suggest factoring out this duplicate sigprocmask handling,
> like, say, rename unblock_async_io and add a parameter:
>
> /* Block or unblock SIGIO.  */
>
> static void
> block_unblock_async_io (int block)
> {
> #ifndef USE_WIN32API
>   sigset_t sigio_set;
>
>   sigemptyset (&sigio_set);
>   sigaddset (&sigio_set, SIGIO);
>   sigprocmask (block ? SIG_BLOCK : SIG_UNBLOCK, &sigio_set, NULL);
> #endif
> }

Done.

>
>>    async_io_enabled = 0;
>>  #ifdef __QNX__
>>    nto_comctrl (0);
>> @@ -852,12 +859,14 @@ disable_async_io (void)
>>  void
>>  initialize_async_io (void)
>>  {
>> -  /* Make sure that async I/O starts disabled.  */
>> +  /* Install the signal handler.  */
>> +#ifndef USE_WIN32API
>> +  signal (SIGIO, input_interrupt);
>> +#endif
>> +
>> +  /* Make sure that async I/O starts blocked.  */
>>    async_io_enabled = 1;
>>    disable_async_io ();
>
> I think it's safer practice to block the signal before
> installing the handler.

OK, and fixed.

>
> Otherwise LGTM.

Patch below is pushed in.

-- 
Yao (齐尧)
From 8b2073398477b33d425b0570236fe4e4222fe2c4 Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Tue, 26 Jan 2016 13:50:22 +0000
Subject: [PATCH] [GDBserver] Block and unblock SIGIO

Nowadays, GDBserver disables async io (by ignoring SIGIO) when process
a serial event, and enables async io (by installing signal handler) when
resume the inferior and wait.  GDBserver may miss SIGIO (by interrupt)
and doesn't process SIGIO in time, which is shown by
gdb.base/interrupt-noterm.exp.  In the test, GDB sends "continue &" and
then "interrupt".  if '\003' arrives at a period between GDBserver
receives vCont;c and enables async io, SIGIO is ignored because signal
handler isn't installed.  GDBserver waits for the inferior and can not
notice '\003' until it returns from wait.

This patch changes the code to install SIGIO handler early, but block
and unblock SIGIO as needed.  In this way, we don't remove SIGIO
handler, so SIGIO can't be ignored.  However, GDBserver needs to
remove the signal handler when connection is closed.

gdb/gdbserver:

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

	* remote-utils.c (remote_close) [!USE_WIN32API]: Ignore SIGIO.
	(unblock_async_io): Rename to ...
	(block_unblock_async_io): ... it.  New function.
	(enable_async_io): Don't install SIGIO handler.  Unblock it
	instead.
	(disable_async_io): Don't ignore SIGIO.  Block it instead.
	(initialize_async_io): Install SIGIO handler.  Don't call
	unblock_async_io.

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 4aa7350..0ba902e 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,5 +1,16 @@
 2016-01-26  Yao Qi  <yao.qi@linaro.org>
 
+	* remote-utils.c (remote_close) [!USE_WIN32API]: Ignore SIGIO.
+	(unblock_async_io): Rename to ...
+	(block_unblock_async_io): ... it.  New function.
+	(enable_async_io): Don't install SIGIO handler.  Unblock it
+	instead.
+	(disable_async_io): Don't ignore SIGIO.  Block it instead.
+	(initialize_async_io): Install SIGIO handler.  Don't call
+	unblock_async_io.
+
+2016-01-26  Yao Qi  <yao.qi@linaro.org>
+
 	* remote-utils.c (getpkt): If the buffer isn't empty, and the
 	first character is '\003', call *the_target->request_interrupt.
 
diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c
index ccc99f1..e751473 100644
--- a/gdb/gdbserver/remote-utils.c
+++ b/gdb/gdbserver/remote-utils.c
@@ -402,6 +402,11 @@ remote_close (void)
 {
   delete_file_handler (remote_desc);
 
+#ifndef USE_WIN32API
+  /* Remove SIGIO handler.  */
+  signal (SIGIO, SIG_IGN);
+#endif
+
 #ifdef USE_WIN32API
   closesocket (remote_desc);
 #else
@@ -775,19 +780,19 @@ check_remote_input_interrupt_request (void)
   input_interrupt (0);
 }
 
-/* Asynchronous I/O support.  SIGIO must be enabled when waiting, in order to
-   accept Control-C from the client, and must be disabled when talking to
-   the client.  */
+/* Asynchronous I/O support.  SIGIO must be unblocked when waiting,
+   in order to accept Control-C from the client, and must be blocked
+   when talking to the client.  */
 
 static void
-unblock_async_io (void)
+block_unblock_async_io (int block)
 {
 #ifndef USE_WIN32API
   sigset_t sigio_set;
 
   sigemptyset (&sigio_set);
   sigaddset (&sigio_set, SIGIO);
-  sigprocmask (SIG_UNBLOCK, &sigio_set, NULL);
+  sigprocmask (block ? SIG_BLOCK : SIG_UNBLOCK, &sigio_set, NULL);
 #endif
 }
 
@@ -823,9 +828,8 @@ enable_async_io (void)
   if (async_io_enabled)
     return;
 
-#ifndef USE_WIN32API
-  signal (SIGIO, input_interrupt);
-#endif
+  block_unblock_async_io (0);
+
   async_io_enabled = 1;
 #ifdef __QNX__
   nto_comctrl (1);
@@ -839,9 +843,8 @@ disable_async_io (void)
   if (!async_io_enabled)
     return;
 
-#ifndef USE_WIN32API
-  signal (SIGIO, SIG_IGN);
-#endif
+  block_unblock_async_io (1);
+
   async_io_enabled = 0;
 #ifdef __QNX__
   nto_comctrl (0);
@@ -852,12 +855,14 @@ disable_async_io (void)
 void
 initialize_async_io (void)
 {
-  /* Make sure that async I/O starts disabled.  */
+  /* Make sure that async I/O starts blocked.  */
   async_io_enabled = 1;
   disable_async_io ();
 
-  /* Make sure the signal is unblocked.  */
-  unblock_async_io ();
+  /* Install the signal handler.  */
+#ifndef USE_WIN32API
+  signal (SIGIO, input_interrupt);
+#endif
 }
 
 /* Internal buffer used by readchar.