From: Tom Tromey Date: Mon, 5 Dec 2022 17:56:23 +0000 (-0700) Subject: Fix control-c handling on Windows X-Git-Url: https://git.libre-soc.org/?a=commitdiff_plain;h=c88afe9cf5d7fb0bd878acb930d79aafcf182505;p=binutils-gdb.git Fix control-c handling on Windows 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. --- 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 +#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 +#include /* 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 /* 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 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. */