diff mbox series

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

Message ID 0d839e4d9288c80fb4cba0b7f02ea6fd52b20fc6.1686131880.git.aburgess@redhat.com
State New
Headers
Series Infcalls from B/P conditions in multi-threaded inferiors |

Commit Message

Andrew Burgess June 7, 2023, 10:01 a.m. UTC 
  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.

Due to the changes to how get_remote_thread_info is now called in
remote.c, one of the overloads of get_remote_thread_info is no longer
used and can be deleted.
---
 gdb/remote.c                                  | 23 +++++++++----------
 gdb/testsuite/gdb.base/infcall-timeout.exp    | 11 +--------
 .../infcall-from-bp-cond-timeout.exp          | 11 +--------
 3 files changed, 13 insertions(+), 32 deletions(-)

Comments

Pedro Alves July 7, 2023, 5:18 p.m. UTC | #1 
(I'll go back to reviewing the other patches in the series, but I wanted to
reply to this one immediately.)

I think this patch is just incorrect, design wise, and we should not merge it.

There is absolutely no connection between any selected thread in GDB to the
thread that eventually reports the SIGINT, because the \x03 packet carries no
thread context.  So GDB could ask for target_stop({pid=123, lwpid=999}), and
if we end up sending \x03, the thread that gets the SIGINT could be
say, {pid=123, lwpid=123}, or even {pid=666, lwpid=666}.

On Windows, both GenerateConsoleCtrlEvent and DebugBreakProcess (the functions
used by win32_process_target::request_interrupt()) inject a new thread
in the inferior and has _thread_ thread report the event.  So the thread that
reports the interrupt is completely unknown to GDB at the time the \x03 is sent.

Also, there's even no guarantee that you'll get a SIGINT instead of some
other signal.  On Windows, DebugBreakProcess results in SIGTRAP.

We should just not assume that target_stop with an all-stop backend is able
to stop a specific thread.

Pedro Alves
Andrew Burgess July 10, 2023, 8:04 p.m. UTC | #2 
Pedro Alves <pedro@palves.net> writes:

> (I'll go back to reviewing the other patches in the series, but I wanted to
> reply to this one immediately.)
>
> I think this patch is just incorrect, design wise, and we should not merge it.
>
> There is absolutely no connection between any selected thread in GDB to the
> thread that eventually reports the SIGINT, because the \x03 packet carries no
> thread context.  So GDB could ask for target_stop({pid=123, lwpid=999}), and
> if we end up sending \x03, the thread that gets the SIGINT could be
> say, {pid=123, lwpid=123}, or even {pid=666, lwpid=666}.
>
> On Windows, both GenerateConsoleCtrlEvent and DebugBreakProcess (the functions
> used by win32_process_target::request_interrupt()) inject a new thread
> in the inferior and has _thread_ thread report the event.  So the thread that
> reports the interrupt is completely unknown to GDB at the time the \x03 is sent.
>
> Also, there's even no guarantee that you'll get a SIGINT instead of some
> other signal.  On Windows, DebugBreakProcess results in SIGTRAP.
>
> We should just not assume that target_stop with an all-stop backend is able
> to stop a specific thread.

Thanks for the feedback.

Andrew
diff mbox series

Patch

diff --git a/gdb/remote.c b/gdb/remote.c
index 02ff3a12bdb..d50802bc4a0 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -2729,8 +2729,6 @@  remote_target::remote_add_inferior (bool fake_pid_p, int pid, int attached,
 }
 
 static remote_thread_info *get_remote_thread_info (thread_info *thread);
-static remote_thread_info *get_remote_thread_info (remote_target *target,
-						   ptid_t ptid);
 
 /* Add thread PTID to GDB's thread list.  Tag it as executing/running
    according to EXECUTING and RUNNING respectively.  If SILENT_P (or the
@@ -2870,15 +2868,6 @@  get_remote_thread_info (thread_info *thread)
   return gdb::checked_static_cast<remote_thread_info *> (thread->priv.get ());
 }
 
-/* Return PTID's private thread data, creating it if necessary.  */
-
-static remote_thread_info *
-get_remote_thread_info (remote_target *target, ptid_t ptid)
-{
-  thread_info *thr = target->find_thread (ptid);
-  return get_remote_thread_info (thr);
-}
-
 /* Call this function as a result of
    1) A halt indication (T packet) containing a thread id
    2) A direct query of currthread
@@ -8263,7 +8252,8 @@  remote_target::process_stop_reply (struct stop_reply *stop_reply,
 	}
 
       remote_notice_new_inferior (ptid, false);
-      remote_thread_info *remote_thr = get_remote_thread_info (this, ptid);
+      thread_info *tinfo = this->find_thread (ptid);
+      remote_thread_info *remote_thr = get_remote_thread_info (tinfo);
       remote_thr->core = stop_reply->core;
       remote_thr->stop_reason = stop_reply->stop_reason;
       remote_thr->watch_data_address = stop_reply->watch_data_address;
@@ -8276,6 +8266,15 @@  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)
+	    {
+	      if (tinfo->stop_requested)
+		status->set_stopped (GDB_SIGNAL_0);
+	    }
+
 	  /* 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 729d8d8734a..2af598e044d 100644
--- a/gdb/testsuite/gdb.base/infcall-timeout.exp
+++ b/gdb/testsuite/gdb.base/infcall-timeout.exp
@@ -44,18 +44,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 { ([target_info gdb_protocol] == "remote"
-	  || [target_info gdb_protocol] == "extended-remote")
-	 && !$target_non_stop } {
-	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 3e74958589c..a94c6a58192 100644
--- a/gdb/testsuite/gdb.threads/infcall-from-bp-cond-timeout.exp
+++ b/gdb/testsuite/gdb.threads/infcall-from-bp-cond-timeout.exp
@@ -91,18 +91,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 { ([target_info gdb_protocol] == "remote"
-	  || [target_info gdb_protocol] == "extended-remote")
-	 && !$target_non_stop} {
-	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\\." \