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

  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.
  

On 01/05/2016 04:43 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
> Hi Pedro,
> sorry for the delayed reply.  Takes much time reading these patches and
> discussions in archives.

Likewise here.

> 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

Alright, let's do this then.

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

LGTM.

Thanks,
Pedro Alves
  

Pedro Alves <palves@redhat.com> writes:

>> 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
>
> Alright, let's do this then.
>

>
> LGTM.

Patch is pushed in, and I'll work on the doc.