[PATCH v2 0/4] Fix Windows C-c handling

[PATCH v2 0/4] Fix Windows C-c handling

[Tom Tromey]

Here's v2 of the series to fix Windows C-c handling.

This version fixes another bug that Hannes found, where sigint_ours
could be used without first having been set.  This is the new patch #2.

Because the new patch changes some generic code, I regression tested
this on x86-64 Fedora 34 as well.

I tried various C-c/C-break scenarios on Windows, using powershell
over RDP, and these worked fine for me.

Tom

[PATCH v2 1/4] Rename install_sigint_handler

[Tom Tromey]

A subsequent patch will introduce a global 'install_sigint_handler'
function, so first rename the static one in extension.c.
---
 gdb/extension.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/extension.c b/gdb/extension.c
index 8cbd80f45d5..1d951d60041 100644
--- a/gdb/extension.c
+++ b/gdb/extension.c
@@ -657,7 +657,7 @@ get_active_ext_lang (void)
 /* Install a SIGINT handler.  */
 
 static void
-install_sigint_handler (const struct signal_handler *handler_state)
+install_ext_sigint_handler (const struct signal_handler *handler_state)
 {
   gdb_assert (handler_state->handler_saved);
 
@@ -756,7 +756,7 @@ restore_active_ext_lang (struct active_ext_lang_state *previous)
     {
       /* Restore the previous SIGINT handler if one was saved.  */
       if (previous->sigint_handler.handler_saved)
-	install_sigint_handler (&previous->sigint_handler);
+	install_ext_sigint_handler (&previous->sigint_handler);
 
       /* If there's a SIGINT recorded in the cooperative extension languages,
 	 move it to the new language, or save it in GDB's global flag if the
-- 
2.34.3

[PATCH v2 2/4] Use gdb::optional for sigint_ours

[Tom Tromey]

sigint_ours (and sigquit_ours) can be used without being set.  Avoid
this problem by changing them to gdb::optional and checking that they
are in fact set before using the value.
---
 gdb/inflow.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/gdb/inflow.c b/gdb/inflow.c
index 5477624bcd5..da2005a9318 100644
--- a/gdb/inflow.c
+++ b/gdb/inflow.c
@@ -113,9 +113,9 @@ static struct terminal_info *get_inflow_inferior_data (struct inferior *);
    we save our handlers in these two variables and set SIGINT and SIGQUIT
    to SIG_IGN.  */
 
-static sighandler_t sigint_ours;
+static gdb::optional<sighandler_t> sigint_ours;
 #ifdef SIGQUIT
-static sighandler_t sigquit_ours;
+static gdb::optional<sighandler_t> sigquit_ours;
 #endif
 
 /* The name of the tty (from the `tty' command) that we're giving to
@@ -501,9 +501,13 @@ child_terminal_ours_1 (target_terminal_state desired_state)
 
       if (!job_control && desired_state == target_terminal_state::is_ours)
 	{
-	  signal (SIGINT, sigint_ours);
+	  if (sigint_ours.has_value ())
+	    signal (SIGINT, *sigint_ours);
+	  sigint_ours.reset ();
 #ifdef SIGQUIT
-	  signal (SIGQUIT, sigquit_ours);
+	  if (sigquit_ours.has_value ())
+	    signal (SIGQUIT, *sigquit_ours);
+	  sigquit_ours.reset ();
 #endif
 	}
 
-- 
2.34.3

[PATCH v2 3/4] Refactor code to check for terminal sharing

[Tom Tromey]

This refactors the code to check for terminal sharing.
is_gdb_terminal is exported, and sharing_input_terminal_1 is renamed,
slightly refactored, and moved to posix-hdep.c.  A new
Windows-specific implementation of this function is added to
mingw-hdep.c.

MSDN has a warning about GetConsoleProcessList

    This API is not recommended and does not have a virtual terminal
    equivalent. [...] Applications remoting via cross-platform
    utilities and transports like SSH may not work as expected if
    using this API.

However, we believe this isn't likely to be an issue for gdb.
---
 gdb/inferior.h   | 11 +++++++++++
 gdb/inflow.c     | 27 +++------------------------
 gdb/mingw-hdep.c | 31 ++++++++++++++++++++++++++++++-
 gdb/posix-hdep.c | 20 +++++++++++++++++++-
 4 files changed, 63 insertions(+), 26 deletions(-)

diff --git a/gdb/inferior.h b/gdb/inferior.h
index 69525a2e053..547e8751d08 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -167,6 +167,17 @@ extern void default_print_float_info (struct gdbarch *gdbarch,
 				      frame_info_ptr frame,
 				      const char *args);
 
+/* Try to determine whether TTY is GDB's input terminal.  Returns
+   TRIBOOL_UNKNOWN if we can't tell.  */
+
+extern tribool is_gdb_terminal (const char *tty);
+
+/* Helper for sharing_input_terminal.  Try to determine whether pid
+   PID is using the same TTY for input as GDB is.  Returns
+   TRIBOOL_UNKNOWN if we can't tell.  */
+
+extern tribool sharing_input_terminal (int pid);
+
 extern void child_terminal_info (struct target_ops *self, const char *, int);
 
 extern void child_terminal_ours (struct target_ops *self);
diff --git a/gdb/inflow.c b/gdb/inflow.c
index da2005a9318..cd4de98d3ed 100644
--- a/gdb/inflow.c
+++ b/gdb/inflow.c
@@ -212,10 +212,9 @@ gdb_save_tty_state (void)
     }
 }
 
-/* Try to determine whether TTY is GDB's input terminal.  Returns
-   TRIBOOL_UNKNOWN if we can't tell.  */
+/* See inferior.h.  */
 
-static tribool
+tribool
 is_gdb_terminal (const char *tty)
 {
   struct stat gdb_tty;
@@ -236,26 +235,6 @@ is_gdb_terminal (const char *tty)
 	  : TRIBOOL_FALSE);
 }
 
-/* Helper for sharing_input_terminal.  Try to determine whether
-   inferior INF is using the same TTY for input as GDB is.  Returns
-   TRIBOOL_UNKNOWN if we can't tell.  */
-
-static tribool
-sharing_input_terminal_1 (inferior *inf)
-{
-  /* Using host-dependent code here is fine, because the
-     child_terminal_foo functions are meant to be used by child/native
-     targets.  */
-#if defined (__linux__) || defined (__sun__)
-  char buf[100];
-
-  xsnprintf (buf, sizeof (buf), "/proc/%d/fd/0", inf->pid);
-  return is_gdb_terminal (buf);
-#else
-  return TRIBOOL_UNKNOWN;
-#endif
-}
-
 /* Return true if the inferior is using the same TTY for input as GDB
    is.  If this is true, then we save/restore terminal flags/state.
 
@@ -287,7 +266,7 @@ sharing_input_terminal (inferior *inf)
 {
   terminal_info *tinfo = get_inflow_inferior_data (inf);
 
-  tribool res = sharing_input_terminal_1 (inf);
+  tribool res = sharing_input_terminal (inf->pid);
 
   if (res == TRIBOOL_UNKNOWN)
     {
diff --git a/gdb/mingw-hdep.c b/gdb/mingw-hdep.c
index 651e795216d..9d0337cf4ef 100644
--- a/gdb/mingw-hdep.c
+++ b/gdb/mingw-hdep.c
@@ -21,8 +21,8 @@
 #include "main.h"
 #include "serial.h"
 #include "gdbsupport/event-loop.h"
-
 #include "gdbsupport/gdb_select.h"
+#include "inferior.h"
 
 #include <windows.h>
 
@@ -371,3 +371,32 @@ gdb_console_fputs (const char *linebuf, FILE *fstream)
   last_style = style;
   return 1;
 }
+
+/* See inferior.h.  */
+
+tribool
+sharing_input_terminal (int pid)
+{
+  std::vector<DWORD> results (10);
+  DWORD len = 0;
+  while (true)
+    {
+      len = GetConsoleProcessList (results.data (), results.size ());
+      /* Note that LEN == 0 is a failure, but we can treat it the same
+	 as a "no".  */
+      if (len < results.size ())
+	break;
+
+      results.resize (len);
+    }
+  /* In case the vector was too big.  */
+  results.resize (len);
+  if (std::find (results.begin (), results.end (), pid) != results.end ())
+    {
+      /* The pid is in the list sharing the console, so don't
+	 interrupt the inferior -- it will get the signal itself.  */
+      return TRIBOOL_TRUE;
+    }
+
+  return TRIBOOL_FALSE;
+}
diff --git a/gdb/posix-hdep.c b/gdb/posix-hdep.c
index 3d44338d2ae..26211978d06 100644
--- a/gdb/posix-hdep.c
+++ b/gdb/posix-hdep.c
@@ -19,8 +19,8 @@
 
 #include "defs.h"
 #include "gdbsupport/event-loop.h"
-
 #include "gdbsupport/gdb_select.h"
+#include "inferior.h"
 
 /* Wrapper for select.  Nothing special needed on POSIX platforms.  */
 
@@ -38,3 +38,21 @@ gdb_console_fputs (const char *buf, FILE *f)
 {
   return 0;
 }
+
+/* See inferior.h.  */
+
+tribool
+sharing_input_terminal (int pid)
+{
+  /* Using host-dependent code here is fine, because the
+     child_terminal_foo functions are meant to be used by child/native
+     targets.  */
+#if defined (__linux__) || defined (__sun__)
+  char buf[100];
+
+  xsnprintf (buf, sizeof (buf), "/proc/%d/fd/0", pid);
+  return is_gdb_terminal (buf);
+#else
+  return TRIBOOL_UNKNOWN;
+#endif
+}
-- 
2.34.3

[PATCH v2 4/4] Fix control-c handling on Windows

[Tom Tromey]

As Hannes pointed out, the Windows target-async patches broke C-c
handling there.  Looking into this, I found a few oddities, fixed
here.

First, windows_nat_target::interrupt calls GenerateConsoleCtrlEvent.
I think this event can be ignored by the inferior, so it's not a great
way to interrupt.  Instead, using DebugBreakProcess (or a more
complicated thing for Wow64) seems better.

Second, windows_nat_target did not implement the pass_ctrlc method.
Implementing this lets us remove the special code to call
SetConsoleCtrlHandler and instead integrate into gdb's approach to C-c
handling.  I believe that this should also fix the race that's
described in the comment that's being removed.

Initially, I thought a simpler version of this patch would work.
However, I think what happens is that some other library (I'm not sure
what) calls SetConsoleCtrlHandler while gdb is running, and this
intercepts and handles C-c -- so that the gdb SIGINT handler is not
called.  C-break continues to work, presumably because whatever
handler is installed ignores it.

This patch works around this issue by ensuring that the gdb handler
always comes first.
---
 gdb/event-top.c   |  2 +-
 gdb/extension.c   |  5 +--
 gdb/inferior.h    | 10 ++++++
 gdb/inflow.c      |  8 ++---
 gdb/mingw-hdep.c  | 38 ++++++++++++++++++++++
 gdb/posix-hdep.c  | 24 ++++++++++++++
 gdb/windows-nat.c | 83 +++++++++++------------------------------------
 7 files changed, 99 insertions(+), 71 deletions(-)

diff --git a/gdb/event-top.c b/gdb/event-top.c
index 0e371194ee3..29dd151f0b5 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -1071,7 +1071,7 @@ gdb_init_signals (void)
 
   sigint_token =
     create_async_signal_handler (async_request_quit, NULL, "sigint");
-  signal (SIGINT, handle_sigint);
+  install_sigint_handler (handle_sigint);
 
   async_sigterm_token
     = create_async_signal_handler (async_sigterm_handler, NULL, "sigterm");
diff --git a/gdb/extension.c b/gdb/extension.c
index 1d951d60041..ae8ef0d6e31 100644
--- a/gdb/extension.c
+++ b/gdb/extension.c
@@ -33,6 +33,7 @@
 #include "python/python.h"
 #include "guile/guile.h"
 #include <array>
+#include "inferior.h"
 
 static script_sourcer_func source_gdb_script;
 static objfile_script_sourcer_func source_gdb_objfile_script;
@@ -661,7 +662,7 @@ install_ext_sigint_handler (const struct signal_handler *handler_state)
 {
   gdb_assert (handler_state->handler_saved);
 
-  signal (SIGINT, handler_state->handler);
+  install_sigint_handler (handler_state->handler);
 }
 
 /* Install GDB's SIGINT handler, storing the previous version in *PREVIOUS.
@@ -675,7 +676,7 @@ install_gdb_sigint_handler (struct signal_handler *previous)
   /* Save here to simplify comparison.  */
   sighandler_t handle_sigint_for_compare = handle_sigint;
 
-  previous->handler = signal (SIGINT, handle_sigint);
+  previous->handler = install_sigint_handler (handle_sigint);
   if (previous->handler != handle_sigint_for_compare)
     previous->handler_saved = 1;
   else
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 547e8751d08..6fc0a30b12c 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -178,6 +178,16 @@ extern tribool is_gdb_terminal (const char *tty);
 
 extern tribool sharing_input_terminal (int pid);
 
+/* The type of the function that is called when SIGINT is handled.  */
+
+typedef void c_c_handler_ftype (int);
+
+/* Install a new SIGINT handler in a host-dependent way.  The previous
+   handler is returned.  It is fine to pass SIG_IGN for FN, but not
+   SIG_DFL.  */
+
+extern c_c_handler_ftype *install_sigint_handler (c_c_handler_ftype *fn);
+
 extern void child_terminal_info (struct target_ops *self, const char *, int);
 
 extern void child_terminal_ours (struct target_ops *self);
diff --git a/gdb/inflow.c b/gdb/inflow.c
index cd4de98d3ed..7913cb55403 100644
--- a/gdb/inflow.c
+++ b/gdb/inflow.c
@@ -332,7 +332,7 @@ child_terminal_inferior (struct target_ops *self)
 
       if (!job_control)
 	{
-	  sigint_ours = signal (SIGINT, SIG_IGN);
+	  sigint_ours = install_sigint_handler (SIG_IGN);
 #ifdef SIGQUIT
 	  sigquit_ours = signal (SIGQUIT, SIG_IGN);
 #endif
@@ -481,7 +481,7 @@ child_terminal_ours_1 (target_terminal_state desired_state)
       if (!job_control && desired_state == target_terminal_state::is_ours)
 	{
 	  if (sigint_ours.has_value ())
-	    signal (SIGINT, *sigint_ours);
+	    install_sigint_handler (*sigint_ours);
 	  sigint_ours.reset ();
 #ifdef SIGQUIT
 	  if (sigquit_ours.has_value ())
@@ -869,7 +869,7 @@ set_sigint_trap (void)
 
   if (inf->attach_flag || !tinfo->run_terminal.empty ())
     {
-      osig = signal (SIGINT, pass_signal);
+      osig = install_sigint_handler (pass_signal);
       osig_set = 1;
     }
   else
@@ -881,7 +881,7 @@ clear_sigint_trap (void)
 {
   if (osig_set)
     {
-      signal (SIGINT, osig);
+      install_sigint_handler (osig);
       osig_set = 0;
     }
 }
diff --git a/gdb/mingw-hdep.c b/gdb/mingw-hdep.c
index 9d0337cf4ef..a4cd8befdf5 100644
--- a/gdb/mingw-hdep.c
+++ b/gdb/mingw-hdep.c
@@ -25,6 +25,7 @@
 #include "inferior.h"
 
 #include <windows.h>
+#include <signal.h>
 
 /* Return an absolute file name of the running GDB, if possible, or
    ARGV0 if not.  The return value is in malloc'ed storage.  */
@@ -400,3 +401,40 @@ sharing_input_terminal (int pid)
 
   return TRIBOOL_FALSE;
 }
+
+/* Current C-c handler.  */
+static c_c_handler_ftype *current_handler;
+
+/* The Windows callback that forwards requests to the C-c handler.  */
+static BOOL WINAPI
+ctrl_c_handler (DWORD event_type)
+{
+  if (event_type == CTRL_BREAK_EVENT || event_type == CTRL_C_EVENT)
+    {
+      if (current_handler != SIG_IGN)
+	current_handler (SIGINT);
+    }
+  else
+    return FALSE;
+  return TRUE;
+}
+
+/* See inferior.h.  */
+
+c_c_handler_ftype *
+install_sigint_handler (c_c_handler_ftype *fn)
+{
+  /* We want to make sure the gdb handler always comes first, so that
+     gdb gets to handle the C-c.  This is why the handler is always
+     removed and reinstalled here.  Note that trying to remove the
+     function without installing it first will cause a crash.  */
+  static bool installed = false;
+  if (installed)
+    SetConsoleCtrlHandler (ctrl_c_handler, FALSE);
+  SetConsoleCtrlHandler (ctrl_c_handler, TRUE);
+  installed = true;
+
+  c_c_handler_ftype *result = current_handler;
+  current_handler = fn;
+  return result;
+}
diff --git a/gdb/posix-hdep.c b/gdb/posix-hdep.c
index 26211978d06..eddf73f38c9 100644
--- a/gdb/posix-hdep.c
+++ b/gdb/posix-hdep.c
@@ -21,6 +21,7 @@
 #include "gdbsupport/event-loop.h"
 #include "gdbsupport/gdb_select.h"
 #include "inferior.h"
+#include <signal.h>
 
 /* Wrapper for select.  Nothing special needed on POSIX platforms.  */
 
@@ -56,3 +57,26 @@ sharing_input_terminal (int pid)
   return TRIBOOL_UNKNOWN;
 #endif
 }
+
+/* Current C-c handler.  */
+static c_c_handler_ftype *current_handler;
+
+/* A wrapper that reinstalls the current signal handler.  */
+static void
+handler_wrapper (int num)
+{
+  signal (num, handler_wrapper);
+  if (current_handler != SIG_IGN)
+    current_handler (num);
+}
+
+/* See inferior.h.  */
+
+c_c_handler_ftype *
+install_sigint_handler (c_c_handler_ftype *fn)
+{
+  signal (SIGINT, handler_wrapper);
+  c_c_handler_ftype *result = current_handler;
+  current_handler = fn;
+  return result;
+}
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index b3329cd1a0d..dafda4781b9 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -298,6 +298,7 @@ struct windows_nat_target final : public x86_nat_target<inf_child_target>
   std::string pid_to_str (ptid_t) override;
 
   void interrupt () override;
+  void pass_ctrlc () override;
 
   const char *pid_to_exec_file (int pid) override;
 
@@ -1509,24 +1510,12 @@ windows_nat_target::resume (ptid_t ptid, int step, enum gdb_signal sig)
     windows_continue (continue_status, ptid.lwp (), 0);
 }
 
-/* Ctrl-C handler used when the inferior is not run in the same console.  The
-   handler is in charge of interrupting the inferior using DebugBreakProcess.
-   Note that this function is not available prior to Windows XP.  In this case
-   we emit a warning.  */
-static BOOL WINAPI
-ctrl_c_handler (DWORD event_type)
-{
-  const int attach_flag = current_inferior ()->attach_flag;
-
-  /* Only handle Ctrl-C and Ctrl-Break events.  Ignore others.  */
-  if (event_type != CTRL_C_EVENT && event_type != CTRL_BREAK_EVENT)
-    return FALSE;
-
-  /* If the inferior and the debugger share the same console, do nothing as
-     the inferior has also received the Ctrl-C event.  */
-  if (!new_console && !attach_flag)
-    return TRUE;
+/* Interrupt the inferior.  */
 
+void
+windows_nat_target::interrupt ()
+{
+  DEBUG_EVENTS ("interrupt");
 #ifdef __x86_64__
   if (windows_process.wow64_process)
     {
@@ -1548,19 +1537,24 @@ ctrl_c_handler (DWORD event_type)
 					      windows_process.wow64_dbgbreak,
 					      NULL, 0, NULL);
 	  if (thread)
-	    CloseHandle (thread);
+	    {
+	      CloseHandle (thread);
+	      return;
+	    }
 	}
     }
   else
 #endif
-    {
-      if (!DebugBreakProcess (windows_process.handle))
-	warning (_("Could not interrupt program.  "
-		   "Press Ctrl-c in the program console."));
-    }
+    if (DebugBreakProcess (windows_process.handle))
+      return;
+  warning (_("Could not interrupt program.  "
+	     "Press Ctrl-c in the program console."));
+}
 
-  /* Return true to tell that Ctrl-C has been handled.  */
-  return TRUE;
+void
+windows_nat_target::pass_ctrlc ()
+{
+  interrupt ();
 }
 
 /* Get the next event from the child.  Returns the thread ptid.  */
@@ -1840,35 +1834,7 @@ windows_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
 
   while (1)
     {
-      /* If the user presses Ctrl-c while the debugger is waiting
-	 for an event, he expects the debugger to interrupt his program
-	 and to get the prompt back.  There are two possible situations:
-
-	   - The debugger and the program do not share the console, in
-	     which case the Ctrl-c event only reached the debugger.
-	     In that case, the ctrl_c handler will take care of interrupting
-	     the inferior.  Note that this case is working starting with
-	     Windows XP.  For Windows 2000, Ctrl-C should be pressed in the
-	     inferior console.
-
-	   - The debugger and the program share the same console, in which
-	     case both debugger and inferior will receive the Ctrl-c event.
-	     In that case the ctrl_c handler will ignore the event, as the
-	     Ctrl-c event generated inside the inferior will trigger the
-	     expected debug event.
-
-	     FIXME: brobecker/2008-05-20: If the inferior receives the
-	     signal first and the delay until GDB receives that signal
-	     is sufficiently long, GDB can sometimes receive the SIGINT
-	     after we have unblocked the CTRL+C handler.  This would
-	     lead to the debugger stopping prematurely while handling
-	     the new-thread event that comes with the handling of the SIGINT
-	     inside the inferior, and then stop again immediately when
-	     the user tries to resume the execution in the inferior.
-	     This is a classic race that we should try to fix one day.  */
-      SetConsoleCtrlHandler (&ctrl_c_handler, TRUE);
       ptid_t result = get_windows_debug_event (pid, ourstatus, options);
-      SetConsoleCtrlHandler (&ctrl_c_handler, FALSE);
 
       if (result != null_ptid)
 	{
@@ -2868,17 +2834,6 @@ windows_nat_target::mourn_inferior ()
   inf_child_target::mourn_inferior ();
 }
 
-/* Send a SIGINT to the process group.  This acts just like the user typed a
-   ^C on the controlling terminal.  */
-
-void
-windows_nat_target::interrupt ()
-{
-  DEBUG_EVENTS ("GenerateConsoleCtrlEvent (CTRLC_EVENT, 0)");
-  CHECK (GenerateConsoleCtrlEvent (CTRL_C_EVENT,
-				   windows_process.current_event.dwProcessId));
-}
-
 /* Helper for windows_xfer_partial that handles memory transfers.
    Arguments are like target_xfer_partial.  */
 
-- 
2.34.3