* [PATCH v2 3/4] Refactor code to check for terminal sharing
2022-12-12 18:11 [PATCH v2 0/4] Fix Windows C-c handling Tom Tromey
2022-12-12 18:11 ` [PATCH v2 1/4] Rename install_sigint_handler Tom Tromey
2022-12-12 18:11 ` [PATCH v2 2/4] Use gdb::optional for sigint_ours Tom Tromey
@ 2022-12-12 18:11 ` Tom Tromey
2022-12-12 18:11 ` [PATCH v2 4/4] Fix control-c handling on Windows Tom Tromey
3 siblings, 0 replies; 5+ messages in thread
From: Tom Tromey @ 2022-12-12 18:11 UTC (permalink / raw)
To: gdb-patches; +Cc: 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
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH v2 4/4] Fix control-c handling on Windows
2022-12-12 18:11 [PATCH v2 0/4] Fix Windows C-c handling Tom Tromey
` (2 preceding siblings ...)
2022-12-12 18:11 ` [PATCH v2 3/4] Refactor code to check for terminal sharing Tom Tromey
@ 2022-12-12 18:11 ` Tom Tromey
3 siblings, 0 replies; 5+ messages in thread
From: Tom Tromey @ 2022-12-12 18:11 UTC (permalink / raw)
To: gdb-patches; +Cc: 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
^ permalink raw reply [flat|nested] 5+ messages in thread