RE: [PATCHv2 11/13] gdb/remote: avoid SIGINT after calling remote_target::stop

["Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com>]

On Wednesday, January 18, 2023 5:18 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 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 a mechanism where we can track that a stop
> has been requested for a particular thread through
> remote_target::stop, then, when the stop arrives, we can convert the
> SIGINT to a GDB_SIGNAL_0.  With this done GDB will now display the
> "stopped" based messages rather than the "received SIGINT" messages.
> 
> 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.
> ---
>  gdb/remote.c                                    | 17 +++++++++++++++++
>  gdb/testsuite/gdb.base/infcall-timeout.exp      |  9 +--------
>  .../infcall-from-bp-cond-timeout.exp            |  9 +--------
>  3 files changed, 19 insertions(+), 16 deletions(-)
> 
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 218bca30d04..61781a24820 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -1139,6 +1139,10 @@ struct remote_thread_info : public private_thread_info
>    std::string name;
>    int core = -1;
> 
> +  /* Only used when not in non-stop mode.  Set to true when a stop is
> +     requested for the thread.  */
> +  bool stop_requested = false;

The thread_info struct already has a `stop_requested` field.  Why can't we use it?

>    /* Thread handle, perhaps a pthread_t or thread_t value, stored as a
>       sequence of bytes.  */
>    gdb::byte_vector thread_handle;
> @@ -7114,6 +7118,12 @@ remote_target::stop (ptid_t ptid)
>        /* We don't currently have a way to transparently pause the
>  	 remote target in all-stop mode.  Interrupt it instead.  */
>        remote_interrupt_as ();
> +
> +      /* Record that this thread's stop is a result of GDB asking for the
> +	 stop, rather than the user asking for an interrupt.  We can use
> +	 this information to adjust the waitstatus when it arrives.  */
> +      remote_thread_info *remote_thr = get_remote_thread_info (this, ptid);
> +      remote_thr->stop_requested = true;
>      }
>  }
> 
> @@ -8097,9 +8107,16 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply,
>  	  /* If the target works in non-stop mode, a stop-reply indicates that
>  	     only this thread stopped.  */
>  	  remote_thr->set_not_resumed ();
> +	  gdb_assert (!remote_thr->stop_requested);
>  	}
>        else
>  	{
> +	  if (status->kind () == TARGET_WAITKIND_STOPPED
> +	      && status->sig () == GDB_SIGNAL_INT
> +	      && remote_thr->stop_requested)
> +	    status->set_stopped (GDB_SIGNAL_0);
> +	  remote_thr->stop_requested = false;
> +
>  	  /* If the target works in all-stop mode, a stop-reply indicates that
>  	     all the target's threads stopped.  */
>  	  for (thread_info *tp : all_non_exited_threads (this))
> diff --git a/gdb/testsuite/gdb.base/infcall-timeout.exp b/gdb/testsuite/gdb.base/infcall-
> timeout.exp
> index a5b0111ed04..bd6b2bfac3e 100644
> --- a/gdb/testsuite/gdb.base/infcall-timeout.exp
> +++ b/gdb/testsuite/gdb.base/infcall-timeout.exp
> @@ -45,16 +45,9 @@ proc_with_prefix run_test { target_async target_non_stop } {
> 
>      gdb_test_no_output "set direct-call-timeout 5"
> 
> -    # When non-stop mode is off we get slightly different output from GDB.
> -    if { [gdb_is_remote_or_extended_remote_target] && $target_non_stop == "off" } {
> -	set stopped_line_pattern "Program received signal SIGINT, Interrupt\\."
> -    } else {
> -	set stopped_line_pattern "Program stopped\\."
> -    }
> -
>      gdb_test "print function_that_never_returns ()" \
>  	[multi_line \
> -	     $stopped_line_pattern \
> +	     "Program stopped\\." \
>  	     ".*" \
>  	     "The program being debugged timed out while in a function called from GDB\\." \
>  	     "GDB remains in the frame where the timeout occurred\\." \
> diff --git a/gdb/testsuite/gdb.threads/infcall-from-bp-cond-timeout.exp
> b/gdb/testsuite/gdb.threads/infcall-from-bp-cond-timeout.exp
> index 3341ff33f19..9ba38e6896a 100644
> --- a/gdb/testsuite/gdb.threads/infcall-from-bp-cond-timeout.exp
> +++ b/gdb/testsuite/gdb.threads/infcall-from-bp-cond-timeout.exp
> @@ -92,16 +92,9 @@ proc run_test { target_async target_non_stop other_thread_bp } {
>  				 "get number for segfault breakpoint"]
>      }
> 
> -    # When non-stop mode is off we get slightly different output from GDB.
> -    if { [gdb_is_remote_or_extended_remote_target] && $target_non_stop == "off" } {
> -	set stopped_line_pattern "Thread ${::decimal} \"\[^\r\n\"\]+\" received signal
> SIGINT, Interrupt\\."
> -    } else {
> -	set stopped_line_pattern "Thread ${::decimal} \"\[^\r\n\"\]+\" stopped\\."
> -    }
> -
>      gdb_test "continue" \
>  	[multi_line \
> -	     $stopped_line_pattern \
> +	     "Thread ${::decimal} \"\[^\r\n\"\]+\" stopped\\." \
>  	     ".*" \
>  	     "Error in testing condition for breakpoint ${bp_num}:" \
>  	     "The program being debugged timed out while in a function called from GDB\\." \
> --
> 2.25.4

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