[RFA/win32] Improve C-c handling when process in different console

[RFA/win32] Improve C-c handling when process in different console

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 2528 bytes --]

Hello,

Nicolas Roche asked me to submit one of the changes he made for us
to improve the situation when the user hits Ctrl-c while the inferior
runs in a different console.  Right now, interrupting only works when
when debugger and inferior run in the same console, or when the user
hits Ctrl-c in the console where the program runs.

The meat of his patch is to use the DebugBreakProcess routine, which
stops a given process.  One of the issues we had was that this routine
is only available starting with Windows XP, meaning that it doesn't
work for Windows 2000. What we elected to do was to get a handle
on the kernel32.dll, and from there create a pointer to this function
if it exists.  That way, if the function exists, then we interrupt,
otherwise we do nothing, which is was we do now.

I should also mention that this patch is not perfect, as the
DebugBreakProcess routine causes a SIGTRAP event to be triggered,
whereas we would have of course prefered a SIGINT event. Not perfect,
but still better than before.  Translating the SIGTRAP into a SIGINT
is not necessarily trivial, because of possible races between the
moment GDB receives the SIGINT and the moment the DebugBreakProcess
stops the inferior.

I should also mention that Nicolas first came up with a solution
that doesn't rely on DebugBreakProcess and works on all versions
of Windows that I know. I can't remember all the details, but it
is based on creating a thread in the inferior and have that thread
send the SIGINT. A consequence of the approach is that we also get
the proper signal kind.  But it uses a bit of assembly code, which
of course probably doesn't work on x86_64, etc, so we both felt that
using DebugBreakProcess, despite the drawback mentioned above, is
a better approach. We should be able to dig out the code, if anyone
is interested.

2008-10-02  Nicolas Roche  <roche@adacore.com>

        * win32-nat.c (check_for_DebugBreakProcess): New function.
        (ctrl_c_handler): New function.
        (win32_wait): Register ctrl_c_handler as Ctrl-C handler if the inferior
        is run in a separate console.
        (_initialize_win32_nat): Check for DebugBreakProcess in kernel32.dll.


This was tested pretty extensively through local usage. Nico tested
both cygwin and mingw32.  The one thing I would appreciate help on is
testing it against the testsuite. We don't anticipate any regression,
and our own testsuite (based on a different, simpler technology)
showed no regression.

Would that be OK to commit?

Thanks,
-- 
Joel

[-- Attachment #2: win32-nat.c.diff --]
[-- Type: text/plain, Size: 6510 bytes --]

Index: win32-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/win32-nat.c,v
retrieving revision 1.165
diff -u -p -r1.165 win32-nat.c
--- win32-nat.c	2 Oct 2008 14:20:07 -0000	1.165
+++ win32-nat.c	2 Oct 2008 22:32:20 -0000
@@ -155,6 +155,14 @@ static int debug_memory = 0;		/* show ta
 static int debug_exceptions = 0;	/* show target exceptions */
 static int useshell = 0;		/* use shell for subprocesses */
 
+/* Set to non-zero if kernel32.dll provides the DebugBreakProcess
+   function (see the check_for_DebugBreakProcess function).  */
+static int has_DebugBreakProcess = 0;
+
+/* If has_DebugBreakProcess is non-zero then it contains the address of the
+   kernel32.dll DebugBreakProcess function.  */
+static WINBASEAPI BOOL WINAPI (*kernel32_DebugBreakProcess) (HANDLE);
+
 /* This vector maps GDB's idea of a register's number into an address
    in the win32 exception context vector.
 
@@ -238,6 +246,29 @@ static const struct xlate_exception
   {STATUS_FLOAT_DIVIDE_BY_ZERO, TARGET_SIGNAL_FPE},
   {-1, -1}};
 
+/* Check if system DLL kernel32.dll contains the DebugBreakProcess function.
+   If this is the case then set has_DebugBreakProcess to 1 and store the 
+   address of the function in kernel32_DebugBreakProcess.  Note that 
+   DebugBreakProcess should be available starting with Windows XP.  It gives
+   a convenient way to propagate a Ctrl-C event from GDB to the inferior when
+   they are running in separate consoles (see win32_wait).  This is the case 
+   when we attach to a process or new-console is set to 1.  */
+
+static void 
+check_for_DebugBreakProcess ()
+{
+  HMODULE kernel32_module_handle;
+
+  kernel32_module_handle = LoadLibrary ("kernel32.dll");
+  if (kernel32_module_handle) 
+    {
+      kernel32_DebugBreakProcess = 
+        GetProcAddress (kernel32_module_handle, "DebugBreakProcess");
+      if (kernel32_DebugBreakProcess != NULL) 
+        has_DebugBreakProcess = 1;
+    }
+}
+
 static void
 check (BOOL ok, const char *file, int line)
 {
@@ -1282,6 +1313,34 @@ win32_resume (ptid_t ptid, int step, enu
     win32_continue (continue_status, ptid_get_tid (ptid));
 }
 
+/* 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.  */
+
+BOOL WINAPI
+ctrl_c_handler (DWORD event_type) 
+{
+  /* Only handle Ctrl-C event.  Ignore others.  */
+  if (event_type != CTRL_C_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 && !current_inferior ()->attach_flag)
+    return TRUE;
+
+  if (has_DebugBreakProcess)
+    kernel32_DebugBreakProcess (current_process_handle);
+  else
+    warning (_("\
+Cannot interrupt program on this version of Windows.\n\
+Press Ctrl-c in the program console."));
+
+  /* Return true to tell that Ctrl-C has been handled.  */
+  return TRUE;
+}
+
 /* Get the next event from the child.  Return 1 if the event requires
    handling by WFI (or whatever).
  */
@@ -1488,22 +1547,35 @@ win32_wait (ptid_t ptid, struct target_w
     {
       int retval;
       
-      /* Ignore CTRL+C signals while waiting for a debug event.
-         FIXME: brobecker/2008-05-20: When the user presses CTRL+C while
-         the inferior is running, both the inferior and GDB receive the
-         associated signal.  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 to stop
-         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, and it would be nice
-         to find a better solution to that problem.  But in the meantime,
-         the current approach already greatly mitigate this issue.  */
-      SetConsoleCtrlHandler (NULL, TRUE);
+      /* 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); 
       retval = get_win32_debug_event (pid, ourstatus);
-      SetConsoleCtrlHandler (NULL, FALSE);
+      SetConsoleCtrlHandler (&ctrl_c_handler, FALSE);
 
       if (retval)
 	return ptid_build (current_event.dwProcessId, 0, retval);
@@ -2181,6 +2253,8 @@ _initialize_win32_nat (void)
 
   add_com_alias ("sharedlibrary", "dll-symbols", class_alias, 1);
 
+  check_for_DebugBreakProcess ();
+
 #ifdef __CYGWIN__
   add_setshow_boolean_cmd ("shell", class_support, &useshell, _("\
 Set use of shell to start subprocess."), _("\

Re: [RFA/win32] Improve C-c handling when process in different console

[Joel Brobecker]

> 2008-10-02  Nicolas Roche  <roche@adacore.com>
> 
>         * win32-nat.c (check_for_DebugBreakProcess): New function.
>         (ctrl_c_handler): New function.
>         (win32_wait): Register ctrl_c_handler as Ctrl-C handler if the inferior
>         is run in a separate console.
>         (_initialize_win32_nat): Check for DebugBreakProcess in kernel32.dll.

Ping?

http://www.sourceware.org/ml/gdb-patches/2008-10/msg00070.html

Thank you,
-- 
Joel

Re: [RFA/win32] Improve C-c handling when process in different console

[Christopher Faylor]

On Wed, Oct 22, 2008 at 09:44:47AM -0700, Joel Brobecker wrote:
>> 2008-10-02  Nicolas Roche  <roche@adacore.com>
>> 
>>         * win32-nat.c (check_for_DebugBreakProcess): New function.
>>         (ctrl_c_handler): New function.
>>         (win32_wait): Register ctrl_c_handler as Ctrl-C handler if the inferior
>>         is run in a separate console.
>>         (_initialize_win32_nat): Check for DebugBreakProcess in kernel32.dll.
>
>Ping?
>
>http://www.sourceware.org/ml/gdb-patches/2008-10/msg00070.html

I'm very sorry that it has taken me so long to respond to this.  Vacation and work
have intervened.

I have an even older patch sitting in my inbox from Pierre Muller which, I think,
attempts to address the same problem but seems to involve more code.

One of the reasons that I have not responded sooner is that I have a patch in
my sandbox which would surely conflict with this since it changes the mechanism
used to determine the existence of newer functions like DebugBreak.

So, anyway, I'll finish up my patch and adopt this change to it this weekend,
checking in both.

Sorry for the delay.  Please ping me again if you don't see this going in in
the next week or so.

cgf

Re: [RFA/win32] Improve C-c handling when process in different console

[Nicolas Roche]

On Wed, 12 Nov 2008 11:38:50 -0500
Christopher Faylor <cgf-use-the-mailinglist-please@sourceware.org>
wrote:

> On Wed, Oct 22, 2008 at 09:44:47AM -0700, Joel Brobecker wrote:
> >> 2008-10-02  Nicolas Roche  <roche@adacore.com>
> >> 
> >>         * win32-nat.c (check_for_DebugBreakProcess): New function.
> >>         (ctrl_c_handler): New function.
> >>         (win32_wait): Register ctrl_c_handler as Ctrl-C handler if the inferior
> >>         is run in a separate console.
> >>         (_initialize_win32_nat): Check for DebugBreakProcess in kernel32.dll.
> >
> >Ping?
> >
> >http://www.sourceware.org/ml/gdb-patches/2008-10/msg00070.html
> 
> I'm very sorry that it has taken me so long to respond to this.  Vacation and work
> have intervened.

No worries :-)

> I have an even older patch sitting in my inbox from Pierre Muller which, I think,
> attempts to address the same problem but seems to involve more code.
> 
> One of the reasons that I have not responded sooner is that I have a patch in
> my sandbox which would surely conflict with this since it changes the mechanism
> used to determine the existence of newer functions like DebugBreak.
> 
> So, anyway, I'll finish up my patch and adopt this change to it this weekend,
> checking in both.

Thanks a lot. It means implicitly that the patch is ok for you right ?
 
> Sorry for the delay.  Please ping me again if you don't see this going in in
> the next week or so.

Won't hesitate

Nicolas

RE: [RFA/win32] Improve C-c handling when process in different console

[Pierre Muller]

> > I have an even older patch sitting in my inbox from Pierre Muller
> which, I think,
> > attempts to address the same problem but seems to involve more code.

  The two patches look quite similar indeed, 
the only thing that is different is that I also added some code
in order to avoid gdb from believing that he has to 
handle changes of the terminal settings if 
set new-console is on.
  In that case, the debuggee is started on separate command prompt
window and will only modify that new window settings, not the one gdb
is in.

Pierre Muller
Pascal language support maintainer for GDB

Re: [RFA/win32] Improve C-c handling when process in different console

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 967 bytes --]

Hi Chris,

> 2008-10-02  Nicolas Roche  <roche@adacore.com>
> 
>         * win32-nat.c (check_for_DebugBreakProcess): New function.
>         (ctrl_c_handler): New function.
>         (win32_wait): Register ctrl_c_handler as Ctrl-C handler if the inferior
>         is run in a separate console.
>         (_initialize_win32_nat): Check for DebugBreakProcess in kernel32.dll.

Here is a new version of the patch. I had to change a couple of things:

  1. Remove the WINBASEAPI when declaring kernel32_DebugBreakProcess;
     My understanding is that this attribute doesn't make sense at all
     for a static variable.

  2. Cast the result of GetProcAddress to "void *" in order to avoid
     a compiler warning. This is consistent with what we do in the other
     GetProcAddress calls.

This patch has been working very well for us over the past year.
Do you think you could take a look to see if we can have something in
before we roll 7.0 out?

Thanks,
-- 
Joel

[-- Attachment #2: windows-nat.diff --]
[-- Type: text/x-diff, Size: 6633 bytes --]

Index: windows-nat.c
===================================================================
--- windows-nat.c	(.../branches/gdb/FSF/current/gdb/windows-nat.c)	(revision 146347)
+++ windows-nat.c	(.../trunk/gdb/gdb-head/gdb/windows-nat.c)	(revision 146347)
@@ -162,6 +162,14 @@ static int debug_memory = 0;		/* show ta
 static int debug_exceptions = 0;	/* show target exceptions */
 static int useshell = 0;		/* use shell for subprocesses */
 
+/* Set to non-zero if kernel32.dll provides the DebugBreakProcess
+   function (see the check_for_DebugBreakProcess function).  */
+static int has_DebugBreakProcess = 0;
+
+/* If has_DebugBreakProcess is non-zero then it contains the address of the
+   kernel32.dll DebugBreakProcess function.  */
+static BOOL WINAPI (*kernel32_DebugBreakProcess) (HANDLE);
+
 /* This vector maps GDB's idea of a register's number into an offset
    in the windows exception context vector.
 
@@ -211,6 +219,28 @@ windows_set_context_register_offsets (co
   mappings = offsets;
 }
 
+/* Check if system DLL kernel32.dll contains the DebugBreakProcess function.
+   If this is the case then set has_DebugBreakProcess to 1 and store the 
+   address of the function in kernel32_DebugBreakProcess.  Note that 
+   DebugBreakProcess should be available starting with Windows XP.  It gives
+   a convenient way to propagate a Ctrl-C event from GDB to the inferior when
+   they are running in separate consoles (see windows_wait).  This is the case 
+   when we attach to a process or new-console is set to 1.  */
+static void 
+check_for_DebugBreakProcess ()
+{
+  HMODULE kernel32_module_handle;
+
+  kernel32_module_handle = LoadLibrary ("kernel32.dll");
+  if (kernel32_module_handle) 
+    {
+      kernel32_DebugBreakProcess = 
+        (void *) GetProcAddress (kernel32_module_handle, "DebugBreakProcess");
+      if (kernel32_DebugBreakProcess != NULL) 
+        has_DebugBreakProcess = 1;
+    }
+}
+
 static void
 check (BOOL ok, const char *file, int line)
 {
@@ -1258,9 +1295,39 @@ windows_resume (struct target_ops *ops,
     windows_continue (continue_status, ptid_get_tid (ptid));
 }
 
+/* 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.  */
+BOOL WINAPI
+ctrl_c_handler (DWORD event_type) 
+{
+  const int attach_flag = current_inferior ()->attach_flag;
+
+  /* Only handle Ctrl-C event.  Ignore others.  */
+  if (event_type != CTRL_C_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;
+
+  if (has_DebugBreakProcess)
+    kernel32_DebugBreakProcess (current_process_handle);
+  else
+    warning (_("\
+Cannot interrupt program on this version of Windows.\n\
+Press Ctrl-c in the program console."));
+
+  /* Return true to tell that Ctrl-C has been handled.  */
+  return TRUE;
+}
+
 /* Get the next event from the child.  Return 1 if the event requires
    handling by WFI (or whatever).
  */
+
 static int
 get_windows_debug_event (struct target_ops *ops,
 			 int pid, struct target_waitstatus *ourstatus)
@@ -1465,23 +1532,36 @@ windows_wait (struct target_ops *ops,
   while (1)
     {
       int retval;
-      
-      /* Ignore CTRL+C signals while waiting for a debug event.
-         FIXME: brobecker/2008-05-20: When the user presses CTRL+C while
-         the inferior is running, both the inferior and GDB receive the
-         associated signal.  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 to stop
-         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, and it would be nice
-         to find a better solution to that problem.  But in the meantime,
-         the current approach already greatly mitigate this issue.  */
-      SetConsoleCtrlHandler (NULL, TRUE);
+     
+      /* 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); 
       retval = get_windows_debug_event (ops, pid, ourstatus);
-      SetConsoleCtrlHandler (NULL, FALSE);
+      SetConsoleCtrlHandler (&ctrl_c_handler, FALSE);
 
       if (retval)
 	return ptid_build (current_event.dwProcessId, 0, retval);
@@ -2164,6 +2251,8 @@ _initialize_windows_nat (void)
 
   add_com_alias ("sharedlibrary", "dll-symbols", class_alias, 1);
 
+  check_for_DebugBreakProcess ();
+
 #ifdef __CYGWIN__
   add_setshow_boolean_cmd ("shell", class_support, &useshell, _("\
 Set use of shell to start subprocess."), _("\

Re: [RFA/win32] Improve C-c handling when process in different console

[Christopher Faylor]

On Mon, Mar 16, 2009 at 05:43:56PM -0400, Joel Brobecker wrote:
>> 2008-10-02  Nicolas Roche  <roche@adacore.com>
>> 
>>         * win32-nat.c (check_for_DebugBreakProcess): New function.
>>         (ctrl_c_handler): New function.
>>         (win32_wait): Register ctrl_c_handler as Ctrl-C handler if the inferior
>>         is run in a separate console.
>>         (_initialize_win32_nat): Check for DebugBreakProcess in kernel32.dll.
>
>Here is a new version of the patch. I had to change a couple of things:
>
>  1. Remove the WINBASEAPI when declaring kernel32_DebugBreakProcess;
>     My understanding is that this attribute doesn't make sense at all
>     for a static variable.
>
>  2. Cast the result of GetProcAddress to "void *" in order to avoid
>     a compiler warning. This is consistent with what we do in the other
>     GetProcAddress calls.
>
>This patch has been working very well for us over the past year.
>Do you think you could take a look to see if we can have something in
>before we roll 7.0 out?

I've finally checked in some of (but not all of) the changes that I
wanted to make for dynamically loading functions in windows-nat.c.  That
reduces the size of this patch, which I've now checked in.

cgf

Re: [RFA/win32] Improve C-c handling when process in different console

[Joel Brobecker]

> I've finally checked in some of (but not all of) the changes that I
> wanted to make for dynamically loading functions in windows-nat.c.  That
> reduces the size of this patch, which I've now checked in.

Thanks, Chris.  This is great!

-- 
Joel