[PATCH] Cygwin: Fix putting inferior in foreground (fix input)

[PATCH] Cygwin: Fix putting inferior in foreground (fix input)

[Pedro Alves]

From: Takashi Yano <takashi.yano@nifty.ne.jp>

gdb.base/interrupt.exp reveals that inferior input is
broken on Cygwin:

  (gdb) continue
  Continuing.
  talk to me baby
  Input/output error                                   <<< BAD
  PASS: gdb.base/interrupt.exp: process is alive
  a
  [Thread 10688.0x2590 exited with code 1]
  [Thread 10688.0x248c exited with code 1]
  [Thread 10688.0x930 exited with code 1]
  [Thread 10688.0x2c98 exited with code 1]

  Program terminated with signal SIGHUP, Hangup.
  The program no longer exists.
  (gdb) FAIL: gdb.base/interrupt.exp: child process ate our char
  a
  Ambiguous command "a": actions, add-auto-load-safe-path, add-auto-load-scripts-directory, add-inferior...
  (gdb) ERROR: "" is not a unique command name.

The problem is that inflow.c:child_terminal_inferior is failing to put
the inferior in the foreground, because we're passing down the
inferior's Windows PID instead of the Cygwin PID to Cygwin tcsetpgrp.

That is fixed by this commit.  Afterwards we will get:

  (gdb) continue
  Continuing.
  talk to me baby
  PASS: gdb.base/interrupt.exp: process is alive
  a
  a                                                              <<< GOOD
  PASS: gdb.base/interrupt.exp: child process ate our char
  [New Thread 7236.0x1c58]

  Thread 6 received signal SIGINT, Interrupt.                    <<< new thread spawned for SIGINT
  [Switching to Thread 7236.0x1c58]
  0x00007ffa6643ea6b in TlsGetValue () from /cygdrive/c/Windows/System32/KERNELBASE.dll
  (gdb) FAIL: gdb.base/interrupt.exp: send_gdb control C

We still have the FAIL seen above because this change has another
consequence.  By failing to put the inferior in the foreground
correctly, Ctrl-C was always reaching GDB first.  Now that the
inferior is put in the foreground properly, Ctrl-C reaches the
inferior first, which results in a Windows Ctrl-C event, which results
in Windows injecting a new thread in the inferior to report the Ctrl-C
exception => SIGINT.  That is, running the test manually:

Before patch:

  (gdb) c
  Continuing.
  [New Thread 2352.0x1f5c]
  [New Thread 2352.0x1988]
  [New Thread 2352.0x18cc]
                                                           <<< Ctrl-C pressed.
  Thread 7 received signal SIGTRAP, Trace/breakpoint trap.
  [Switching to Thread 2352.0x18cc]
  0x00007ffa68930b11 in ntdll!DbgBreakPoint () from /cygdrive/c/Windows/SYSTEM32/ntdll.dll
  (gdb)

Above, GDB got the SIGINT, and it manually passes it down the
inferior, which reaches windows_nat_target::interrupt(), which
interrupts the inferior with DebugBreakProcess (which injects a new
thread in the inferior that executes an int3 instruction).

After this patch, we have (with "set debugexceptions on" so
DBG_CONTROL_C is visible):

  (gdb) c
  Continuing.
  [New Thread 9940.0x1168]
  [New Thread 9940.0x5f8]
  gdb: Target exception MS_VC_EXCEPTION at 0x7ffa6638cf19
  gdb: Target exception MS_VC_EXCEPTION at 0x7ffa6638cf19
  [New Thread 9940.0x3d8]
  gdb: Target exception DBG_CONTROL_C at 0x7ffa6643ea6b   <<< Ctrl-C

  Thread 7 received signal SIGINT, Interrupt.             <<< new injected thread
  [Switching to Thread 9940.0x3d8]
  0x00007ffa6643ea6b in TlsGetValue () from /cygdrive/c/Windows/System32/KERNELBASE.dll
  (gdb)

This new behavior is exactly the same as what you see with a MinGW GDB
build.  Also, SIGINT reaching inferior first is what you get on Linux
as well currently.

I wrote an initial fix for this before I discovered that Cygwin
downstream had a similar change, so I then combined the patches.  I am
adding a Co-Authored-By for that reason.

Co-Authored-By: Takashi Yano <takashi.yano@nifty.ne.jp>
Change-Id: I3a8c3355784c6a817dbd345ba9dac24be06c4b3f
---
 gdb/inflow.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/gdb/inflow.c b/gdb/inflow.c
index a413957a1dd..3dd70b97fe5 100644
--- a/gdb/inflow.c
+++ b/gdb/inflow.c
@@ -40,6 +40,10 @@
 #include <sys/ioctl.h>
 #endif
 
+#ifdef __CYGWIN__
+#include <sys/cygwin.h>
+#endif
+
 #ifndef O_NOCTTY
 #define O_NOCTTY 0
 #endif
@@ -345,11 +349,25 @@ child_terminal_inferior (struct target_ops *self)
 	     then restore whatever was the foreground pgrp the last
 	     time the inferior was running.  See also comments
 	     describing terminal_state::process_group.  */
-#ifdef HAVE_GETPGID
-	  result = tcsetpgrp (0, getpgid (inf->pid));
-#else
-	  result = tcsetpgrp (0, tinfo->process_group);
+	  pid_t pgrp = tinfo->process_group;
+#ifdef __CYGWIN__
+	  /* The Windows native target uses Win32 routines to run or
+	     attach to processes (CreateProcess / DebugActiveProcess),
+	     so a Cygwin inferior has a Windows PID, rather than a
+	     Cygwin PID.  We want to pass the Cygwin PID to Cygwin
+	     tcsetpgrp if we have a Cygwin inferior, so try to convert
+	     first.  If we have a non-Cygwin inferior, we'll end up
+	     passing down the WINPID to tcsetpgrp, stored in
+	     terminal_state::process_group.  tcsetpgrp still succeeds
+	     in that case, and it seems preferable to switch the
+	     foreground pgrp away from GDB, for consistency.  */
+	  pid_t cygpid = cygwin_internal (CW_WINPID_TO_CYGWIN_PID, inf->pid);
+	  if (cygpid <= cygwin_internal (CW_MAX_CYGWIN_PID))
+	    pgrp = getpgid (cygpid);
+#elif defined (HAVE_GETPGID)
+	  pgrp = getpgid (inf->pid);
 #endif
+	  result = tcsetpgrp (0, pgrp);
 	  if (result == -1)
 	    {
 #if 0

base-commit: 3d2d21728b6db4430ff168ee27e12fc6e2627fad
-- 
2.43.2

Re: [PATCH] Cygwin: Fix putting inferior in foreground (fix input)

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:

Pedro> The problem is that inflow.c:child_terminal_inferior is failing to put
Pedro> the inferior in the foreground, because we're passing down the
Pedro> inferior's Windows PID instead of the Cygwin PID to Cygwin tcsetpgrp.

Thanks.  My first thought was that this should really be
target-specific, but I dug into the code and I see that this is only
called from inf_child_target -- i.e., it is.  I wonder if a bunch of
that code could be moved out of inflow.c and into inf-child.c.

Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH] Cygwin: Fix putting inferior in foreground (fix input)

[Pedro Alves]

On 2024-02-27 14:13, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:
> 
> Pedro> The problem is that inflow.c:child_terminal_inferior is failing to put
> Pedro> the inferior in the foreground, because we're passing down the
> Pedro> inferior's Windows PID instead of the Cygwin PID to Cygwin tcsetpgrp.
> 
> Thanks.  My first thought was that this should really be
> target-specific, but I dug into the code and I see that this is only
> called from inf_child_target -- i.e., it is.  I wonder if a bunch of
> that code could be moved out of inflow.c and into inf-child.c.

I'm not sure.  I think I prefer having all the terminal handling code in
the same place, as the data structures, #ifdefs, etc. are all the same.
Also, there was a suggestion to use terminal shuffling when debugging
with "local gdbserver", and remote isn't an inf_child_target target.

What I think we should do (and I've avoided it thus far because I have a lot of pending
changes to this code), is to rename the file. It is severely misnamed, I think due to
how it ended up being what it is.  At the top you see:

 /* Low level interface to ptrace, for GDB when running under Unix.

That is obviously incorrect.  We have inf-ptrace.c nowadays, and inflow.c has no
ptrace code.  I think what happened is that everything other than terminal handling moved
out of the file over the years.   So I think we could rename it.  Several of the exported
functions from inflow.c are declared in terminal.h, so I guess we could just rename it to
terminal.c.

> 
> Approved-By: Tom Tromey <tom@tromey.com>
> 

Thanks, I've merged it.

Pedro Alves

Re: [PATCH] Cygwin: Fix putting inferior in foreground (fix input)

[Simon Marchi]

On 2/27/24 09:40, Pedro Alves wrote:
> On 2024-02-27 14:13, Tom Tromey wrote:
>>>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:
>>
>> Pedro> The problem is that inflow.c:child_terminal_inferior is failing to put
>> Pedro> the inferior in the foreground, because we're passing down the
>> Pedro> inferior's Windows PID instead of the Cygwin PID to Cygwin tcsetpgrp.
>>
>> Thanks.  My first thought was that this should really be
>> target-specific, but I dug into the code and I see that this is only
>> called from inf_child_target -- i.e., it is.  I wonder if a bunch of
>> that code could be moved out of inflow.c and into inf-child.c.
> 
> I'm not sure.  I think I prefer having all the terminal handling code in
> the same place, as the data structures, #ifdefs, etc. are all the same.
> Also, there was a suggestion to use terminal shuffling when debugging
> with "local gdbserver", and remote isn't an inf_child_target target.
> 
> What I think we should do (and I've avoided it thus far because I have a lot of pending
> changes to this code), is to rename the file. It is severely misnamed, I think due to
> how it ended up being what it is.  At the top you see:
> 
>  /* Low level interface to ptrace, for GDB when running under Unix.
> 
> That is obviously incorrect.  We have inf-ptrace.c nowadays, and inflow.c has no
> ptrace code.  I think what happened is that everything other than terminal handling moved
> out of the file over the years.   So I think we could rename it.  Several of the exported
> functions from inflow.c are declared in terminal.h, so I guess we could just rename it to
> terminal.c.

I think you could rename it.  Git is usually good with following renamed
when rebasing commits.

Simon