[PATCH 0/3] Fix Windows C-c handling

[PATCH 0/3] Fix Windows C-c handling

[Tom Tromey]

This series attempts to fix C-c handling on Windows, which was broken
by target async.

I ran this series through the AdaCore test suite (I've never managed
to run dejagnu on Windows), but of course that doesn't really test
this area much, or else we would have seen a regression from target
async.

I also tested this by starting an RDP session (avoiding whatever
cygwin/mingw issue affects C-c handling for me normally) and trying
"run", "attach", and "run"-with-new-console; and exercising both C-c
and C-break.

Let me know what you think.

Tom

[PATCH 1/3] 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 2/3] 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.

One thing I'm not sure of here is how GetConsoleProcessList will work
in all situations.  MSDN has a warning about it:

    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, I don't precisely know what this means.
---
 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 5477624bcd5..17b1e8ce282 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 3/3] 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  | 37 +++++++++++++++++++++
 gdb/posix-hdep.c  | 24 ++++++++++++++
 gdb/windows-nat.c | 83 +++++++++++------------------------------------
 7 files changed, 98 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 17b1e8ce282..344a4eaf509 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
@@ -480,7 +480,7 @@ child_terminal_ours_1 (target_terminal_state desired_state)
 
       if (!job_control && desired_state == target_terminal_state::is_ours)
 	{
-	  signal (SIGINT, sigint_ours);
+	  install_sigint_handler (sigint_ours);
 #ifdef SIGQUIT
 	  signal (SIGQUIT, sigquit_ours);
 #endif
@@ -865,7 +865,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
@@ -877,7 +877,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..5a69282b99b 100644
--- a/gdb/mingw-hdep.c
+++ b/gdb/mingw-hdep.c
@@ -400,3 +400,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

Re: [PATCH 0/3] Fix Windows C-c handling

[Tom Tromey]

Tom> This series attempts to fix C-c handling on Windows, which was broken
Tom> by target async.

Jon, would you mind testing this on Cygwin?  I can't really do that, but
I'd rather not break the build again.

The branch is in my github as 'submit/windows-c-c', which may be more
convenient than applying via email.

thanks,
Tom

Re: [PATCH 2/3] Refactor code to check for terminal sharing

[Eli Zaretskii]

> Cc: Tom Tromey <tromey@adacore.com>
> Date: Mon,  5 Dec 2022 11:56:50 -0700
> From: Tom Tromey via Gdb-patches <gdb-patches@sourceware.org>
> 
> One thing I'm not sure of here is how GetConsoleProcessList will work
> in all situations.  MSDN has a warning about it:
> 
>     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.

AFAIU the warning, we have no reasons to be afraid of using this API.

Thanks.

Re: [PATCH 3/3] Fix control-c handling on Windows

[Hannes Domani]

 Am Montag, 5. Dezember 2022, 19:57:36 MEZ hat Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> Folgendes geschrieben:

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

I've now tested this a bit, it's a big improvement.

Now it even works to interrupt a GUI program that was started with
'new-console off', that didn't work before.

But I did notice a few problems:

1)

When I first started a program with 'new-console on', then the
sigint_ours variable would not be initialized, and I would later get
a crash on C-c.
I've fixed it like this:

diff --git a/gdb/inflow.c b/gdb/inflow.c
index f9926122099..50c93b6e15a 100644
--- a/gdb/inflow.c
+++ b/gdb/inflow.c
@@ -165,7 +165,7 @@ static struct terminal_info *get_inflow_inferior_data (struct inferior *);
    save our handlers in these two variables and set SIGINT and SIGQUIT
    to SIG_IGN.  */
 
-static sighandler_t sigint_ours;
+static sighandler_t sigint_ours = SIG_IGN;
 #ifdef SIGQUIT
 static sighandler_t sigquit_ours;
 #endif
@@ -805,7 +805,8 @@ child_terminal_ours_1 (target_terminal_state desired_state)
         }
     }
 
-      if (!job_control && desired_state == target_terminal_state::is_ours)
+      if (!job_control && desired_state == target_terminal_state::is_ours
+      && sigint_ours != SIG_IGN)
     {
       install_sigint_handler (sigint_ours);
 #ifdef SIGQUIT

Not sure if there is a better solution.


2)

And for some reason one of my builds (x86_64+TUI+python) needed the
#include <signal.h> in mingw-hdep.c, but my other (i686 basic) didn't.


3)

But my basic i686 build had another problem when starting a program with
'new-console on', because at program start it called install_sigint_handler(),
but rl_set_signals() would later override the SIGINT handler again, so C-c
didn't work work in this situation.
With 'new-console off' this didn't happen, since install_sigint_handler() was
again called later since it shared the console.


That's all I've seen so far, thanks for this.


Hannes

Re: [PATCH 3/3] Fix control-c handling on Windows

[Tom Tromey]

>>>>> "Hannes" == Hannes Domani <ssbssa@yahoo.de> writes:

Hannes> I've now tested this a bit, it's a big improvement.

Thanks for trying it.  I appreciate that.

Hannes> When I first started a program with 'new-console on', then the
Hannes> sigint_ours variable would not be initialized, and I would later get
Hannes> a crash on C-c.

I looked at this and my feeling is that this is a latent bug.

I think what's going on is that sigint_ours is set under different
conditions than it is used.

That is, it is set like:

  if (gdb_has_a_terminal ()
      && tinfo->ttystate != NULL
      && sharing_input_terminal (inf))
[...]
      if (!job_control)
	{
	  sigint_ours = install_sigint_handler (SIG_IGN);
[...]
      gdb_tty_state = target_terminal_state::is_inferior;


However later it is used:

  if (gdb_tty_state != desired_state)
[...]
      if (!job_control && desired_state == target_terminal_state::is_ours)
	{
	  install_sigint_handler (sigint_ours);

It's maybe hard to reason about but it seems to me that there's some
possibility for the value to be used even though it hasn't been set, and
I suspect that is what you are seeing.

It might be useful if you could confirm this.  Just some simple logging
at these two points would be sufficient.

If that's the issue then I can write a patch to change sigint_ours to be
a gdb::optional and check it that way.

Hannes> And for some reason one of my builds (x86_64+TUI+python) needed the
Hannes> #include <signal.h> in mingw-hdep.c, but my other (i686 basic) didn't.

I didn't see this but I went ahead and added the include to my patch,
since it seems harmless.

Hannes> But my basic i686 build had another problem when starting a program with
Hannes> 'new-console on', because at program start it called install_sigint_handler(),
Hannes> but rl_set_signals() would later override the SIGINT handler again, so C-c
Hannes> didn't work work in this situation.
Hannes> With 'new-console off' this didn't happen, since install_sigint_handler() was
Hannes> again called later since it shared the console.

I really don't understand the interaction between signal and
SetConsoleCtrlHandler.  I tried searching for some docs on this but
didn't find anything that was really enlightening.

Also it's somewhat surprising that the x86 and x86-64 builds would be
different in this regard.

Anyway ... I'm not sure what to do here yet.  The interactions with
readline are pretty hard to understand.  I guess the question is where
should install_sigint_handler be called where it is not called -- that
is, to reset the signals from readline.  Alternatively, where is it
called on x86-64 but not on x86?

I did try 'new-console on' with my (x86-64) build, and that worked fine.
I can try an x86 build and see if that works any better.

Tom

Re: [PATCH 3/3] Fix control-c handling on Windows

[Hannes Domani]

 Am Freitag, 9. Dezember 2022, 16:58:12 MEZ hat Tom Tromey <tromey@adacore.com> Folgendes geschrieben:

> >>>>> "Hannes" == Hannes Domani <ssbssa@yahoo.de> writes:
>
> Hannes> I've now tested this a bit, it's a big improvement.
>
> Thanks for trying it.  I appreciate that.
>
> Hannes> When I first started a program with 'new-console on', then the
> Hannes> sigint_ours variable would not be initialized, and I would later get
> Hannes> a crash on C-c.
>
> I looked at this and my feeling is that this is a latent bug.
>
> I think what's going on is that sigint_ours is set under different
> conditions than it is used.
>
> That is, it is set like:
>
>   if (gdb_has_a_terminal ()
>       && tinfo->ttystate != NULL
>       && sharing_input_terminal (inf))
> [...]
>       if (!job_control)
>     {
>       sigint_ours = install_sigint_handler (SIG_IGN);
> [...]
>       gdb_tty_state = target_terminal_state::is_inferior;
>
>
> However later it is used:
>
>   if (gdb_tty_state != desired_state)
> [...]
>       if (!job_control && desired_state == target_terminal_state::is_ours)
>     {
>       install_sigint_handler (sigint_ours);
>
> It's maybe hard to reason about but it seems to me that there's some
> possibility for the value to be used even though it hasn't been set, and
> I suspect that is what you are seeing.
>
> It might be useful if you could confirm this.  Just some simple logging
> at these two points would be sufficient.
>
> If that's the issue then I can write a patch to change sigint_ours to be
> a gdb::optional and check it that way.

I should have been more clear about this.
I started with 'new-console on', so sharing_input_terminal() returned false,
and that's why sigint_ours was not set.

So yes, gdb::optional would probably fix this.
I just wonder if this is never an issue on Linux, e.g. if you attach,
of does signal() maybe ignore NULL-pointer functions?


> Hannes> And for some reason one of my builds (x86_64+TUI+python) needed the
> Hannes> #include <signal.h> in mingw-hdep.c, but my other (i686 basic) didn't.
>
> I didn't see this but I went ahead and added the include to my patch,
> since it seems harmless.

Great, thanks.


> Hannes> But my basic i686 build had another problem when starting a program with
> Hannes> 'new-console on', because at program start it called install_sigint_handler(),
> Hannes> but rl_set_signals() would later override the SIGINT handler again, so C-c
> Hannes> didn't work work in this situation.
> Hannes> With 'new-console off' this didn't happen, since install_sigint_handler() was
> Hannes> again called later since it shared the console.
>
> I really don't understand the interaction between signal and
> SetConsoleCtrlHandler.  I tried searching for some docs on this but
> didn't find anything that was really enlightening.

As far as I could tell, signal() calls SetConsoleCtrlHandler(), probably
similar to how you handled this.


> Also it's somewhat surprising that the x86 and x86-64 builds would be
> different in this regard.
>
> Anyway ... I'm not sure what to do here yet.  The interactions with
> readline are pretty hard to understand.  I guess the question is where
> should install_sigint_handler be called where it is not called -- that
> is, to reset the signals from readline.  Alternatively, where is it
> called on x86-64 but not on x86?
>
> I did try 'new-console on' with my (x86-64) build, and that worked fine.
> I can try an x86 build and see if that works any better.

Again, I wasn't clear enough here.
The difference is not because of i686 and x86_64, but that the x86_64 build
has TUI+python enabled, but my i686 build has not.
Some TUI startup code would later call install_sigint_handler() again,
overriding the SIGINT handler again, and everything is fine.


Sorry that I wasn't clear enough before.


Hannes

Re: [PATCH 3/3] Fix control-c handling on Windows

[Tom Tromey]

>> If that's the issue then I can write a patch to change sigint_ours to be
>> a gdb::optional and check it that way.

Hannes> I should have been more clear about this.
Hannes> I started with 'new-console on', so sharing_input_terminal() returned false,
Hannes> and that's why sigint_ours was not set.

Hannes> So yes, gdb::optional would probably fix this.
Hannes> I just wonder if this is never an issue on Linux, e.g. if you attach,
Hannes> of does signal() maybe ignore NULL-pointer functions?

Normally SIG_DFL is NULL, but also on Linux I couldn't get this to
trigger inappropriately.  Maybe my theory about what's going on here is
incorrect.

Hannes> As far as I could tell, signal() calls SetConsoleCtrlHandler(), probably
Hannes> similar to how you handled this.

Yeah, that was my guess as well, but really we'd want more details.
Like, does calling signal reinstall the SetConsoleCtrlHandler?  If so
then why didn't that work for gdb?  But if not then why did we need to
call SetConsoleCtrlHandler again to tweak the ordering of callbacks?

Maybe I should go back and try to figure out what else is calling signal
and/or SetConsoleCtrlHandler.  I somewhat suspect Python but I don't
really know.

>> I did try 'new-console on' with my (x86-64) build, and that worked fine.
>> I can try an x86 build and see if that works any better.

Hannes> Again, I wasn't clear enough here.
Hannes> The difference is not because of i686 and x86_64, but that the x86_64 build
Hannes> has TUI+python enabled, but my i686 build has not.

Aha, I see, thanks.

Hannes> Some TUI startup code would later call install_sigint_handler() again,
Hannes> overriding the SIGINT handler again, and everything is fine.

Do you know where this happens?

Anyway I am wondering if we can have gdb_rl_deprep_term_function call
rl_clear_signals and then reinstall the gdb signal handlers.  This idea
makes me wonder if we even need SetConsoleCtrlHandler at all -- maybe gdb
could just use signal after all.

Tom

Re: [PATCH 3/3] Fix control-c handling on Windows

[Hannes Domani]

 Am Freitag, 9. Dezember 2022, 18:21:27 MEZ hat Tom Tromey <tromey@adacore.com> Folgendes geschrieben:

> >> If that's the issue then I can write a patch to change sigint_ours to be
> >> a gdb::optional and check it that way.
>
> Hannes> I should have been more clear about this.
> Hannes> I started with 'new-console on', so sharing_input_terminal() returned false,
> Hannes> and that's why sigint_ours was not set.
>
> Hannes> So yes, gdb::optional would probably fix this.
> Hannes> I just wonder if this is never an issue on Linux, e.g. if you attach,
> Hannes> of does signal() maybe ignore NULL-pointer functions?
>
> Normally SIG_DFL is NULL, but also on Linux I couldn't get this to
> trigger inappropriately.  Maybe my theory about what's going on here is
> incorrect.
>
> Hannes> As far as I could tell, signal() calls SetConsoleCtrlHandler(), probably
> Hannes> similar to how you handled this.
>
> Yeah, that was my guess as well, but really we'd want more details.
> Like, does calling signal reinstall the SetConsoleCtrlHandler?  If so
> then why didn't that work for gdb?  But if not then why did we need to
> call SetConsoleCtrlHandler again to tweak the ordering of callbacks?
>
> Maybe I should go back and try to figure out what else is calling signal
> and/or SetConsoleCtrlHandler.  I somewhat suspect Python but I don't
> really know.

I tried just now to debug signal a bit more, but now I can't reproduce
it calling SetConsoleCtrlHandler any more.

The more I look, the more confused I get again.


> >> I did try 'new-console on' with my (x86-64) build, and that worked fine.
> >> I can try an x86 build and see if that works any better.
>
> Hannes> Again, I wasn't clear enough here.
> Hannes> The difference is not because of i686 and x86_64, but that the x86_64 build
> Hannes> has TUI+python enabled, but my i686 build has not.
>
> Aha, I see, thanks.
>
> Hannes> Some TUI startup code would later call install_sigint_handler() again,
> Hannes> overriding the SIGINT handler again, and everything is fine.
>
> Do you know where this happens?

Yes, it's because of my extra python TUI windows, which call
gdbpy_register_tui_window:

#0  install_sigint_handler (fn=fn@entry=0x13fde3a50 <handle_sigint(int)>) at C:/src/repos/binutils-gdb.git/gdb/mingw-hdep.c:426
#1  0x000000013fde9cdf in install_gdb_sigint_handler (previous=0x27b88a8) at C:/src/repos/binutils-gdb.git/gdb/extension.c:679
#2  set_active_ext_lang (now_active=now_active@entry=0x1407aee00 <extension_language_python>) at C:/src/repos/binutils-gdb.git/gdb/extension.c:736
#3  0x000000013ff4503e in gdbpy_enter::gdbpy_enter (this=this@entry=0x24c6c0, gdbarch=gdbarch@entry=0x0, language=language@entry=0x0) at C:/src/repos/binutils-gdb.git/gdb/python/python.c:212
#4  0x000000013ff35ac0 in gdbpy_tui_window_maker::gdbpy_tui_window_maker (other=..., this=0x24c6b8) at C:/src/repos/binutils-gdb.git/gdb/python/py-tui.c:301
#5  gdbpy_register_tui_window (self=<optimized out>, args=<optimized out>, kw=<optimized out>) at C:/src/repos/binutils-gdb.git/gdb/python/py-tui.c:380
...


And also there is this call later on:

#0  install_sigint_handler (fn=fn@entry=0x13fde3a50 <handle_sigint(int)>) at C:/src/repos/binutils-gdb.git/gdb/mingw-hdep.c:426
#1  0x000000013fde9cdf in install_gdb_sigint_handler (previous=0x28ade48) at C:/src/repos/binutils-gdb.git/gdb/extension.c:679
#2  set_active_ext_lang (now_active=now_active@entry=0x1407aee00 <extension_language_python>) at C:/src/repos/binutils-gdb.git/gdb/extension.c:736
#3  0x000000013ff4503e in gdbpy_enter::gdbpy_enter (this=0x24f7b0, gdbarch=0x0, language=0x0) at C:/src/repos/binutils-gdb.git/gdb/python/python.c:212
#4  0x000000013ff4522a in gdbpy_before_prompt_hook (extlang=<optimized out>, current_gdb_prompt=0x140be96b0 <top_prompt+16> "(gdb) ") at C:/src/repos/binutils-gdb.git/gdb/python/python.c:1114
#5  0x000000013fde8fc3 in ext_lang_before_prompt (current_gdb_prompt=0x140be96b0 <top_prompt+16> "(gdb) ") at C:/src/repos/binutils-gdb.git/gdb/extension.c:963
#6  0x000000013fde4431 in std::function<void (char const*)>::operator()(char const*) const (__args#0=0x140be96b0 <top_prompt+16> "(gdb) ", this=0x4dee28) at c:/msys64/mingw64/x86_64-w64-mingw32/include/c++/11.2.0/bits/std_function.h:560
#7  gdb::observers::observable<char const*>::notify (args#0=0x140be96b0 <top_prompt+16> "(gdb) ", this=<optimized out>) at c:/src/repos/binutils-gdb.git/gdbsupport/observable.h:166
#8  top_level_prompt () at C:/src/repos/binutils-gdb.git/gdb/event-top.c:461
#9  display_gdb_prompt (new_prompt=<optimized out>) at C:/src/repos/binutils-gdb.git/gdb/event-top.c:428
#10 0x000000013fea4115 in captured_command_loop () at C:/src/repos/binutils-gdb.git/gdb/main.c:472
#11 0x000000013fea5de5 in captured_main (data=0x24fa70) at C:/src/repos/binutils-gdb.git/gdb/main.c:1341
#12 gdb_main (args=args@entry=0x24fad0) at C:/src/repos/binutils-gdb.git/gdb/main.c:1356
#13 0x0000000140692e27 in main (argc=2, argv=0x5c4cc0) at C:/src/repos/binutils-gdb.git/gdb/gdb.c:32


> Anyway I am wondering if we can have gdb_rl_deprep_term_function call
> rl_clear_signals and then reinstall the gdb signal handlers.  This idea
> makes me wonder if we even need SetConsoleCtrlHandler at all -- maybe gdb
> could just use signal after all.

Good question, maybe it doesn't handle C-break as well?


Hannes

Re: [PATCH 0/3] Fix Windows C-c handling

[Jon Turney]

On 05/12/2022 18:59, Tom Tromey wrote:
> Tom> This series attempts to fix C-c handling on Windows, which was broken
> Tom> by target async.
> 
> Jon, would you mind testing this on Cygwin?  I can't really do that, but
> I'd rather not break the build again.
> 
> The branch is in my github as 'submit/windows-c-c', which may be more
> convenient than applying via email.

Thanks.

This compiles ok, and seems to work.

I'm sure how, since my theory was that Cygwin should continue to use 
GenerateConsoleCtrlEvent() on the inferior, which would then experience 
a SIGINT in the Cygwin runtime, and gdb gets told about that emulated 
signal and stops the inferior.

(In practice, I don't think that reliably worked before, and I have 
precisely zero time to look into this currently, so go ahead...)

Re: [PATCH 3/3] Fix control-c handling on Windows

[Tom Tromey]

Hannes> Again, I wasn't clear enough here.
Hannes> The difference is not because of i686 and x86_64, but that the x86_64 build
Hannes> has TUI+python enabled, but my i686 build has not.

>> Aha, I see, thanks.

Today I built an x86-64 Windows gdb, but I disabled Python.  I already
had the TUI disabled for Windows.

I used RDP and started powershell, then ran gdb there.  I did 6 tests: a
C-c and a C-break test of "run", "run" with "set new-console 1", and
"attach" -- and in all these cases, it worked.

So now I'm wondering again what the difference could be between our
situations.

>> Anyway I am wondering if we can have gdb_rl_deprep_term_function call
>> rl_clear_signals and then reinstall the gdb signal handlers.  This idea
>> makes me wonder if we even need SetConsoleCtrlHandler at all -- maybe gdb
>> could just use signal after all.

Hannes> Good question, maybe it doesn't handle C-break as well?

C-break can work with signal by installing a handler for SIGBREAK.  This
worked fine when I tried it, but the issue was still that the SIGINT
handler somehow stopped working.

Tom

Re: [PATCH 3/3] Fix control-c handling on Windows

[Tom Tromey]

Tom> I used RDP and started powershell, then ran gdb there.  I did 6 tests: a
Tom> C-c and a C-break test of "run", "run" with "set new-console 1", and
Tom> "attach" -- and in all these cases, it worked.

I found out how to reproduce the crash you saw.  In my tests, I was
doing everything in a single gdb instance.  However, if I start a new
gdb, then "set new-console 1", and then "run", I can make it crash.

Changing sigint_ours to a gdb::optional fixes this problem.

I couldn't reproduce your problem #3 ... my python-less build seems to
work fine in other situations.

I'll send v2 of this series shortly.

Tom

Re: [PATCH 2/3] Refactor code to check for terminal sharing

[Tom Tromey]

>> Cc: Tom Tromey <tromey@adacore.com>
>> Date: Mon,  5 Dec 2022 11:56:50 -0700
>> From: Tom Tromey via Gdb-patches <gdb-patches@sourceware.org>
>> 
>> One thing I'm not sure of here is how GetConsoleProcessList will work
>> in all situations.  MSDN has a warning about it:
>> 
>> 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.

Eli> AFAIU the warning, we have no reasons to be afraid of using this API.

Thanks.  I've updated the commit message to reflect this.

Tom

Re: [PATCH 0/3] Fix Windows C-c handling

[Tom Tromey]

>>>>> "Jon" == Jon Turney <jon.turney@dronecode.org.uk> writes:

Jon> On 05/12/2022 18:59, Tom Tromey wrote:
Tom> This series attempts to fix C-c handling on Windows, which was broken
Tom> by target async.
>> Jon, would you mind testing this on Cygwin?  I can't really do that,
>> but
>> I'd rather not break the build again.
>> The branch is in my github as 'submit/windows-c-c', which may be
>> more
>> convenient than applying via email.

Jon> Thanks.

Jon> This compiles ok, and seems to work.

Thanks for trying it.

Jon> I'm sure how, since my theory was that Cygwin should continue to use
Jon> GenerateConsoleCtrlEvent() on the inferior, which would then
Jon> experience a SIGINT in the Cygwin runtime, and gdb gets told about
Jon> that emulated signal and stops the inferior.

Jon> (In practice, I don't think that reliably worked before, and I have
Jon> precisely zero time to look into this currently, so go ahead...)

I don't know either :)  If it works for you then that seems pretty good;
and we can always change the Cygwin code paths later if need be.

Tom

Re: [PATCH 3/3] Fix control-c handling on Windows

[Hannes Domani]

 Am Montag, 12. Dezember 2022 um 18:06:23 MEZ hat Tom Tromey <tromey@adacore.com> Folgendes geschrieben:

> Tom> I used RDP and started powershell, then ran gdb there.  I did 6 tests: a
> Tom> C-c and a C-break test of "run", "run" with "set new-console 1", and
> Tom> "attach" -- and in all these cases, it worked.
>
> I found out how to reproduce the crash you saw.  In my tests, I was
> doing everything in a single gdb instance.  However, if I start a new
> gdb, then "set new-console 1", and then "run", I can make it crash.
>
> Changing sigint_ours to a gdb::optional fixes this problem.
>
> I couldn't reproduce your problem #3 ... my python-less build seems to
> work fine in other situations.
>
> I'll send v2 of this series shortly.

I also can't reproduce problem #3 any longer with the v2 series.
So I think that this was actually the same problem that you fixed with
gdb::optional, and it was just that the different signal() calls confused me.

So, I also have no problems any more with v2 applied.


Thanks, Hannes

Re: [PATCH 3/3] Fix control-c handling on Windows

[Tom Tromey]

Hannes> I also can't reproduce problem #3 any longer with the v2 series.
Hannes> So I think that this was actually the same problem that you fixed with
Hannes> gdb::optional, and it was just that the different signal() calls confused me.

Hannes> So, I also have no problems any more with v2 applied.

Thanks.  I'm going to check it in now.

Tom