[PATCHv7,4/6] gdb/remote: avoid SIGINT after calling remote_target::stop

  Currently, if the remote target is not running in non-stop mode, then,
when GDB calls remote_target::stop, we end up sending an interrupt
packet \x03 to the remote target.

If the user interrupts the inferior from the GDB prompt (e.g. by
typing Ctrl-c), then GDB calls remote_target::interrupt, which also
ends up sending the interrupt packet.

The problem here is that both of these mechanisms end up sending the
interrupt packet, which means, when the target stops with a SIGINT,
and this is reported back to GDB, we have no choice but to report this
to the user as a SIGINT stop event.

Now maybe this is the correct thing to do, after all the target has
been stopped with SIGINT.  However, this leads to an unfortunate
change in behaviour when comparing non-stop vs all-stop mode.

When running in non-stop mode, and remote_target::stop is called, the
target will be stopped with a vCont packet, and this stop is then
reported back to GDB as GDB_SIGNAL_0, this will cause GDB to print a
message like:

  Program stopped.

Or:

  Thread NN "binary name" stopped.

In contrast, when non-stop mode is off, we get messages like:

  Program received SIGINT, Segmentation fault.

Or:

  Thread NN "binary name" received SIGINT, Segmentation fault.

In this commit I propose making use of thread_info::stop_requested
within remote.c to know if the stop was triggered by GDB (and the
SIGINT should be hidden) or if the stop was a user interrupt, and the
SIGINT should be printed.

In remote_target::process_stop_reply if the inferior stopped with
SIGINT and the thread_info::stop_requested flag is set, then we change
the stop signal to GDB_SIGNAL_0.

Two of the tests added in the previous commit exposed this issue.  In
the previous commit the tests looked for either of the above
patterns.  In this commit I've updated these tests to only look for
the "stopped" based messages.

This commit is the reason why the previous commit took care to set the
thread_info::stop_requested flag in infcall.c.
---
 gdb/remote.c                                           | 10 ++++++++++
 gdb/testsuite/gdb.base/infcall-timeout.exp             |  9 +--------
 .../gdb.threads/infcall-from-bp-cond-timeout.exp       |  9 +--------
 3 files changed, 12 insertions(+), 16 deletions(-)
  

On Monday, May 15, 2023 9:22 PM, Andrew Burgess wrote:
> Currently, if the remote target is not running in non-stop mode, then,
> when GDB calls remote_target::stop, we end up sending an interrupt
> packet \x03 to the remote target.
> 
> If the user interrupts the inferior from the GDB prompt (e.g. by
> typing Ctrl-c), then GDB calls remote_target::interrupt, which also
> ends up sending the interrupt packet.
> 
> The problem here is that both of these mechanisms end up sending the
> interrupt packet, which means, when the target stops with a SIGINT,
> and this is reported back to GDB, we have no choice but to report this
> to the user as a SIGINT stop event.
> 
> Now maybe this is the correct thing to do, after all the target has
> been stopped with SIGINT.  However, this leads to an unfortunate
> change in behaviour when comparing non-stop vs all-stop mode.
> 
> When running in non-stop mode, and remote_target::stop is called, the
> target will be stopped with a vCont packet, and this stop is then
> reported back to GDB as GDB_SIGNAL_0, this will cause GDB to print a
> message like:
> 
>   Program stopped.
> 
> Or:
> 
>   Thread NN "binary name" stopped.
> 
> In contrast, when non-stop mode is off, we get messages like:
> 
>   Program received SIGINT, Segmentation fault.
> 
> Or:
> 
>   Thread NN "binary name" received SIGINT, Segmentation fault.
> 
> In this commit I propose making use of thread_info::stop_requested
> within remote.c to know if the stop was triggered by GDB (and the
> SIGINT should be hidden) or if the stop was a user interrupt, and the
> SIGINT should be printed.
> 
> In remote_target::process_stop_reply if the inferior stopped with
> SIGINT and the thread_info::stop_requested flag is set, then we change
> the stop signal to GDB_SIGNAL_0.
> 
> Two of the tests added in the previous commit exposed this issue.  In
> the previous commit the tests looked for either of the above
> patterns.  In this commit I've updated these tests to only look for
> the "stopped" based messages.
> 
> This commit is the reason why the previous commit took care to set the
> thread_info::stop_requested flag in infcall.c.
> ---
>  gdb/remote.c                                           | 10 ++++++++++
>  gdb/testsuite/gdb.base/infcall-timeout.exp             |  9 +--------
>  .../gdb.threads/infcall-from-bp-cond-timeout.exp       |  9 +--------
>  3 files changed, 12 insertions(+), 16 deletions(-)
> 
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 8eaa1b2c4d1..c61eeeadc52 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -8275,6 +8275,16 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply,
>  	}
>        else
>  	{
> +	  /* If this stop was actually requested by GDB then we can hide
> +	     the SIGINT from the user.  */
> +	  if (status->kind () == TARGET_WAITKIND_STOPPED
> +	      && status->sig () == GDB_SIGNAL_INT)
> +	    {
> +	      thread_info *thr = this->find_thread (ptid);

A few lines above, there is

      remote_notice_new_inferior (ptid, false);
      remote_thread_info *remote_thr = get_remote_thread_info (this, ptid);

which first does 'target->find_thread (ptid)' to find the thread_info*.
It is unfortunate that we cannot access the thread_info* directly from
remote_thread_info*.  It's not in this patch's scope to add such a backlink.
How about changing 

      remote_thread_info *remote_thr = get_remote_thread_info (this, ptid);

to

      thread_info *thr = this->find_thread (ptid);
      remote_thread_info *remote_thr = get_remote_thread_info (thr);

so that we can reuse 'thr' without having to call 'find_thread'
a second time?

Besides this suggestion, the patch looks fine to me.  FWIW
Reviewed-By: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>

Thanks
-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
  

"Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com> writes:

> On Monday, May 15, 2023 9:22 PM, Andrew Burgess wrote:
>> Currently, if the remote target is not running in non-stop mode, then,
>> when GDB calls remote_target::stop, we end up sending an interrupt
>> packet \x03 to the remote target.
>> 
>> If the user interrupts the inferior from the GDB prompt (e.g. by
>> typing Ctrl-c), then GDB calls remote_target::interrupt, which also
>> ends up sending the interrupt packet.
>> 
>> The problem here is that both of these mechanisms end up sending the
>> interrupt packet, which means, when the target stops with a SIGINT,
>> and this is reported back to GDB, we have no choice but to report this
>> to the user as a SIGINT stop event.
>> 
>> Now maybe this is the correct thing to do, after all the target has
>> been stopped with SIGINT.  However, this leads to an unfortunate
>> change in behaviour when comparing non-stop vs all-stop mode.
>> 
>> When running in non-stop mode, and remote_target::stop is called, the
>> target will be stopped with a vCont packet, and this stop is then
>> reported back to GDB as GDB_SIGNAL_0, this will cause GDB to print a
>> message like:
>> 
>>   Program stopped.
>> 
>> Or:
>> 
>>   Thread NN "binary name" stopped.
>> 
>> In contrast, when non-stop mode is off, we get messages like:
>> 
>>   Program received SIGINT, Segmentation fault.
>> 
>> Or:
>> 
>>   Thread NN "binary name" received SIGINT, Segmentation fault.
>> 
>> In this commit I propose making use of thread_info::stop_requested
>> within remote.c to know if the stop was triggered by GDB (and the
>> SIGINT should be hidden) or if the stop was a user interrupt, and the
>> SIGINT should be printed.
>> 
>> In remote_target::process_stop_reply if the inferior stopped with
>> SIGINT and the thread_info::stop_requested flag is set, then we change
>> the stop signal to GDB_SIGNAL_0.
>> 
>> Two of the tests added in the previous commit exposed this issue.  In
>> the previous commit the tests looked for either of the above
>> patterns.  In this commit I've updated these tests to only look for
>> the "stopped" based messages.
>> 
>> This commit is the reason why the previous commit took care to set the
>> thread_info::stop_requested flag in infcall.c.
>> ---
>>  gdb/remote.c                                           | 10 ++++++++++
>>  gdb/testsuite/gdb.base/infcall-timeout.exp             |  9 +--------
>>  .../gdb.threads/infcall-from-bp-cond-timeout.exp       |  9 +--------
>>  3 files changed, 12 insertions(+), 16 deletions(-)
>> 
>> diff --git a/gdb/remote.c b/gdb/remote.c
>> index 8eaa1b2c4d1..c61eeeadc52 100644
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -8275,6 +8275,16 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply,
>>  	}
>>        else
>>  	{
>> +	  /* If this stop was actually requested by GDB then we can hide
>> +	     the SIGINT from the user.  */
>> +	  if (status->kind () == TARGET_WAITKIND_STOPPED
>> +	      && status->sig () == GDB_SIGNAL_INT)
>> +	    {
>> +	      thread_info *thr = this->find_thread (ptid);
>
> A few lines above, there is
>
>       remote_notice_new_inferior (ptid, false);
>       remote_thread_info *remote_thr = get_remote_thread_info (this, ptid);
>
> which first does 'target->find_thread (ptid)' to find the thread_info*.
> It is unfortunate that we cannot access the thread_info* directly from
> remote_thread_info*.  It's not in this patch's scope to add such a backlink.
> How about changing 
>
>       remote_thread_info *remote_thr = get_remote_thread_info (this, ptid);
>
> to
>
>       thread_info *thr = this->find_thread (ptid);
>       remote_thread_info *remote_thr = get_remote_thread_info (thr);
>
> so that we can reuse 'thr' without having to call 'find_thread'
> a second time?

Great idea.  I've reworked things as you suggest.  This now means one of
the get_remote_thread_info overloads can be deleted too, which I've done.

>
> Besides this suggestion, the patch looks fine to me.  FWIW
> Reviewed-By: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>

I added your reviewed-by tag.

Thanks,
Andrew

>
> Thanks
> -Baris
>
>
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
> Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928