[PATCH] [RFC] Move SetConsoleCtrlHandler calls to async thread

[PATCH] [RFC] Move SetConsoleCtrlHandler calls to async thread

[Hannes Domani]

Ctrl-C and Ctrl-Break don't work any more since the async commit, so I've tried
to move the SetConsoleCtrlHandler calls.
Now Ctrl-C seems to work, but Ctrl-Break completely stop both gdb and the
inferior (if new-console is disabled).

I'm not sure what's the proper fix is here, please advise.
---
 gdb/windows-nat.c | 66 ++++++++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 29 deletions(-)

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 17422e15f80..e1034e48da3 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -209,6 +209,8 @@ static CORE_ADDR cygwin_get_dr (int i);
 static unsigned long cygwin_get_dr6 (void);
 static unsigned long cygwin_get_dr7 (void);
 
+static BOOL WINAPI ctrl_c_handler (DWORD event_type);
+
 /* User options.  */
 static bool new_console = false;
 #ifdef __CYGWIN__
@@ -477,7 +479,37 @@ windows_nat_target::process_thread ()
 	{
 	  if (!m_debug_event_pending)
 	    {
+	      /* 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);
 	      wait_for_debug_event (&m_last_debug_event, INFINITE);
+	      SetConsoleCtrlHandler (&ctrl_c_handler, FALSE);
 	      m_debug_event_pending = true;
 	    }
 	  serial_event_set (m_wait_event);
@@ -505,7 +537,11 @@ windows_nat_target::wait_for_debug_event_main_thread (DEBUG_EVENT *event)
 	  serial_event_clear (m_wait_event);
 	}
       else
-	wait_for_debug_event (event, INFINITE);
+	{
+	  SetConsoleCtrlHandler (&ctrl_c_handler, TRUE);
+	  wait_for_debug_event (event, INFINITE);
+	  SetConsoleCtrlHandler (&ctrl_c_handler, FALSE);
+	}
       return false;
     });
 }
@@ -1844,35 +1880,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)
 	{
-- 
2.35.1

Re: [PATCH] [RFC] Move SetConsoleCtrlHandler calls to async thread

[Tom Tromey]

>>>>> "Hannes" == Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> writes:

Hannes> Ctrl-C and Ctrl-Break don't work any more since the async commit, so I've tried
Hannes> to move the SetConsoleCtrlHandler calls.
Hannes> Now Ctrl-C seems to work, but Ctrl-Break completely stop both gdb and the
Hannes> inferior (if new-console is disabled).

Hannes> I'm not sure what's the proper fix is here, please advise.

In my normal environment (ssh to a Windows machine), none of this has
ever worked at all.  I don't know why, something to do with ssh and/or
cygwin.

Anyway, I managed to test C-c by using remmina (RDP client) and then
running gdb in the console provided there.  I used conemu which I gather
opened some kind of Cygwin shell.

According to winver this machine is "Windows Server 2016".

I tried a few versions:

1. AdaCore internal gdb from before the target async patches.  (AdaCore
   has some local changes, but nothing relevant to this problem.)

2. AdaCore gdb corresponding to git master, plus your other recent
   windows-nat patches.

3. #2 plus your C-c patch.

When I use "run" in gdb, control-c and control-break worked fine with 1
and 2.  (FWIW I used the on-screen keyboard to send control-break, I
don't know any other way to do it.)

When I use "attach", neither 1 nor 2 work at all -- but your patch does,
but only for C-c.  C-break just makes gdb exit.

Now, TBH, I find all of this surprising, because on seeing your patch, I
agree that the target async work should cause a problem here.  And, I
don't understand why your patch fixes the "attach" behavior.

With your patch installed, C-c still works, but C-break does not.  I
don't understand why this happens.  My understanding is that
SetConsoleCtrlHandler intercepts C-break as well, and the body of that
callback hasn't changed.

One other thing I don't understand is that C-c doesn't cause gdb to exit
when it is waiting at the prompt.  Instead I get this quit message:

    throw_quit ("Quit (expect signal SIGINT when the program is resumed)");

Like, if this happens, do we really need ctrl_c_handler for the C-c case
at all?  (Maybe we do need the handler because windows-nat doesn't use
set_sigint_trap.)  And should gdb default to handling ctrl-break as
well?  Like, I wonder if there's a race between something like the call
to signal in child_terminal_inferior and installing the C-c handler.

The comment in windows-nat says:

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

Maybe the 'attach' issue is a known problem?

Anyway.  I've learned that there's a lot in this area I don't
understand.  I think the first thing to do is understand why we get
different results.  I wonder what I could change here to reproduce the
problem.  Maybe I shouldn't be using conemu but instead something else?

Tom

Re: [PATCH] [RFC] Move SetConsoleCtrlHandler calls to async thread

[Hannes Domani]

 Am Donnerstag, 1. Dezember 2022, 18:30:33 MEZ hat Tom Tromey <tromey@adacore.com> Folgendes geschrieben:

> >>>>> "Hannes" == Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Hannes> Ctrl-C and Ctrl-Break don't work any more since the async commit, so I've tried
> Hannes> to move the SetConsoleCtrlHandler calls.
> Hannes> Now Ctrl-C seems to work, but Ctrl-Break completely stop both gdb and the
> Hannes> inferior (if new-console is disabled).
>
> Hannes> I'm not sure what's the proper fix is here, please advise.
>
> In my normal environment (ssh to a Windows machine), none of this has
> ever worked at all.  I don't know why, something to do with ssh and/or
> cygwin.
>
> Anyway, I managed to test C-c by using remmina (RDP client) and then
> running gdb in the console provided there.  I used conemu which I gather
> opened some kind of Cygwin shell.

I'm not sure if C-c works the same inside of Cygwin.


> According to winver this machine is "Windows Server 2016".
>
> I tried a few versions:
>
> 1. AdaCore internal gdb from before the target async patches.  (AdaCore
>   has some local changes, but nothing relevant to this problem.)
>
> 2. AdaCore gdb corresponding to git master, plus your other recent
>   windows-nat patches.
>
> 3. #2 plus your C-c patch.
>
> When I use "run" in gdb, control-c and control-break worked fine with 1
> and 2.  (FWIW I used the on-screen keyboard to send control-break, I
> don't know any other way to do it.)
>
> When I use "attach", neither 1 nor 2 work at all -- but your patch does,
> but only for C-c.  C-break just makes gdb exit.

Yes, that C-break just exits gdb is the issue I meant, that didn't happen
before.


> Now, TBH, I find all of this surprising, because on seeing your patch, I
> agree that the target async work should cause a problem here.  And, I
> don't understand why your patch fixes the "attach" behavior.
>
> With your patch installed, C-c still works, but C-break does not.  I
> don't understand why this happens.  My understanding is that
> SetConsoleCtrlHandler intercepts C-break as well, and the body of that
> callback hasn't changed.
>
> One other thing I don't understand is that C-c doesn't cause gdb to exit
> when it is waiting at the prompt.  Instead I get this quit message:
>
>     throw_quit ("Quit (expect signal SIGINT when the program is resumed)");

I don't think I've ever seen this.
When I press C-c on the gdb prompt, nothing at all happens, which is also
what I expected.


> Like, if this happens, do we really need ctrl_c_handler for the C-c case
> at all?  (Maybe we do need the handler because windows-nat doesn't use
> set_sigint_trap.)  And should gdb default to handling ctrl-break as
> well?  Like, I wonder if there's a race between something like the call
> to signal in child_terminal_inferior and installing the C-c handler.
>
> The comment in windows-nat says:
>
>         [...]  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.
>
> Maybe the 'attach' issue is a known problem?
>
> Anyway.  I've learned that there's a lot in this area I don't
> understand.  I think the first thing to do is understand why we get
> different results.  I wonder what I could change here to reproduce the
> problem.  Maybe I shouldn't be using conemu but instead something else?

Note that it also makes a difference if you started a process with
'set new-console on' enabled (which is what I always have set).


Hannes

Re: [PATCH] [RFC] Move SetConsoleCtrlHandler calls to async thread

[Tom Tromey]

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

>> Anyway, I managed to test C-c by using remmina (RDP client) and then
>> running gdb in the console provided there.  I used conemu which I gather
>> opened some kind of Cygwin shell.

> I'm not sure if C-c works the same inside of Cygwin.

Me neither.  I'll see if I can try a different console tomorrow.

> Note that it also makes a difference if you started a process with
> 'set new-console on' enabled (which is what I always have set).

Ok, I wasn't even aware of this.  And, I see there's also "new-group",
which defaults to true.

Looking into this, first, when would anyone want to disable new-group?

Second, new-console disables new-group.  From the MS docs:

    CREATE_NEW_PROCESS_GROUP
    [...]
    This flag is ignored if specified with CREATE_NEW_CONSOLE.

So this may help explain the differences.

I'll also experiment with new-console tomorrow.

Tom

Re: [PATCH] [RFC] Move SetConsoleCtrlHandler calls to async thread

[Tom Tromey]

Tom> Me neither.  I'll see if I can try a different console tomorrow.

I used powershell today.  It seems to mostly work.  I do see the
ctrl-break exit when running with 'set new-console on'... but

One weird thing is if I add logging (see appended hack), the problem
goes away.  That seems extra mysterious to me.

Another problem I thought of is what should happen with these interrupt
sequences when the user puts the inferior in the background, like with
"continue &".  Having them stop the inferior seems wrong, the user might
be trying to interrupt some gdb command instead.

I don't know how this area is supposed to work.

Tom

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 3d6a3528914..d8e627621e5 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -462,6 +462,17 @@ windows_nat_target::process_thread_starter (LPVOID self)
   return 0;
 }
 
+static BOOL
+wrap_wait_for_debug_event (DEBUG_EVENT *event, DWORD timeout)
+{
+  fprintf (stderr, "+++ installing ctrl_c_handler\n");
+  SetConsoleCtrlHandler (&ctrl_c_handler, TRUE);
+  BOOL r = wait_for_debug_event (event, INFINITE);
+  fprintf (stderr, "+++ removing ctrl_c_handler\n");
+  SetConsoleCtrlHandler (&ctrl_c_handler, FALSE);
+  return r;
+}
+
 void
 windows_nat_target::process_thread ()
 {
@@ -507,9 +518,7 @@ windows_nat_target::process_thread ()
 		     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);
-	      wait_for_debug_event (&m_last_debug_event, INFINITE);
-	      SetConsoleCtrlHandler (&ctrl_c_handler, FALSE);
+	      wrap_wait_for_debug_event (&m_last_debug_event, INFINITE);
 	      m_debug_event_pending = true;
 	    }
 	  serial_event_set (m_wait_event);
@@ -537,11 +546,7 @@ windows_nat_target::wait_for_debug_event_main_thread (DEBUG_EVENT *event)
 	  serial_event_clear (m_wait_event);
 	}
       else
-	{
-	  SetConsoleCtrlHandler (&ctrl_c_handler, TRUE);
-	  wait_for_debug_event (event, INFINITE);
-	  SetConsoleCtrlHandler (&ctrl_c_handler, FALSE);
-	}
+	wrap_wait_for_debug_event (event, INFINITE);
       return false;
     });
 }
@@ -1558,9 +1563,14 @@ ctrl_c_handler (DWORD event_type)
 {
   const int attach_flag = current_inferior ()->attach_flag;
 
+  fprintf (stderr, "+++ ctrl_c_handler, attach = %d\n", 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;
+  fprintf (stderr, "+++ ctrl_c_handler, event type ok, new_console = %d, type = %s\n",
+	   (int) new_console,
+	   event_type == CTRL_C_EVENT ? "control-c" : "BREAK");
 
   /* If the inferior and the debugger share the same console, do nothing as
      the inferior has also received the Ctrl-C event.  */

Re: [PATCH] [RFC] Move SetConsoleCtrlHandler calls to async thread

[Tom Tromey]

Tom> Another problem I thought of is what should happen with these interrupt
Tom> sequences when the user puts the inferior in the background, like with
Tom> "continue &".  Having them stop the inferior seems wrong, the user might
Tom> be trying to interrupt some gdb command instead.

Tom> I don't know how this area is supposed to work.

I looked into how Linux handles this.  Of course, it turns out to be
super complicated.  I did learn some things, though.

First, windows_nat_target doesn't override pass_ctrlc.  However, the one
it inherits calls child_pass_ctrlc, which does:

#ifndef _WIN32
	  kill (inf->pid, SIGINT);
#endif

... oops for this case.

However, this function is pretty interesting because it looks for a
foreground process to interrupt, and if it doesn't find one:

  gdb_assert_not_reached ("no inferior resumed in the fg found");

This leads me to think that if we can integrate windows_nat_target into
the pass_ctrlc codepath, things should work.

I have some patches to attempt this, but of course they don't really
work.  The main mystery right now is that in some scenarios (like the
new-console scenario), when I type C-c, default_quit_handler doesn't
seem to be called at all.

C-break does still work.  (I have this working on my branch by
installing a signal handler for SIGBREAK.)  So I thought maybe SIGINT is
being redirected somewhere, but I couldn't find that.

So once again, not sure what's going on.  Maybe closer though.

Tom

Re: [PATCH] [RFC] Move SetConsoleCtrlHandler calls to async thread

[Hannes Domani]

 Am Freitag, 2. Dezember 2022, 23:22:22 MEZ hat Tom Tromey <tromey@adacore.com> Folgendes geschrieben:

> Tom> Another problem I thought of is what should happen with these interrupt
> Tom> sequences when the user puts the inferior in the background, like with
> Tom> "continue &".  Having them stop the inferior seems wrong, the user might
> Tom> be trying to interrupt some gdb command instead.
>
> Tom> I don't know how this area is supposed to work.
>
> I looked into how Linux handles this.  Of course, it turns out to be
> super complicated.  I did learn some things, though.
>
> First, windows_nat_target doesn't override pass_ctrlc.  However, the one
> it inherits calls child_pass_ctrlc, which does:
>
> #ifndef _WIN32
>       kill (inf->pid, SIGINT);
> #endif
>
> ... oops for this case.
>
> However, this function is pretty interesting because it looks for a
> foreground process to interrupt, and if it doesn't find one:
>
>   gdb_assert_not_reached ("no inferior resumed in the fg found");
>
> This leads me to think that if we can integrate windows_nat_target into
> the pass_ctrlc codepath, things should work.
>
> I have some patches to attempt this, but of course they don't really
> work.  The main mystery right now is that in some scenarios (like the
> new-console scenario), when I type C-c, default_quit_handler doesn't
> seem to be called at all.
>
> C-break does still work.  (I have this working on my branch by
> installing a signal handler for SIGBREAK.)  So I thought maybe SIGINT is
> being redirected somewhere, but I couldn't find that.
>
> So once again, not sure what's going on.  Maybe closer though.

I wouldn't mind trying out some patches, even if they aren't fully functional yet.


Hannes

Re: [PATCH] [RFC] Move SetConsoleCtrlHandler calls to async thread

[Tom Tromey]

Hannes> I wouldn't mind trying out some patches, even if they aren't
Hannes> fully functional yet.

I'm going to send some in a minute.  They are also on the
'submit/windows-c-c' branch in my github.

Tom