[RFC] Move SetConsoleCtrlHandler calls to async thread

  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(-)
  

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

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
  

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

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

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
  

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
  

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