[PATCH v2] Cygwin: pty: Make Ctrl-C work for non-cygwin app in GDB

[Corinna Vinschen]

Hi Takashi,

two (minor) points:

- is_gdb_with_foreground_non_cygwin_inferior() is defined twice.
  It would probably make sense to move all these inline functions
  is_gdb_with_foreground_non_cygwin_inferior(),
  is_foreground_special_process() and
  is_non_cygwin_foreground_process() into a header.
  pinfo.h might be a good place.

- The check in is_gdb_with_foreground_non_cygwin_inferior() looks
  a bit on the fragile side.  Wouldn't it make sense to check with
  CheckRemoteDebuggerPresent(), or at least, try to?


Thanks,
Corinna

On Mar  9 16:08, Takashi Yano wrote:
> At some point in the past, GDB sets terminal pgid to inferior pid
> when the inferior is running. Moreover, the inferior is non-cygwin
> process, GDB sets the terminal pgid to windows pid of the inferior.
> Due to this behaviour, Ctrl-C does not work if the inferior is a
> non-cygwin app. This is because, the current code sends Ctrl-C to
> GDB only when GDB's pgid equeals to terminal pgid. This patch omit
> checking pgid when recognizing GDB process whose inferior is non-
> cygwin app.
> 
> In addition, to improve the readabiliby of the code, this patch
> introduces inline functions such as:
> is_foreground_special_process (),
> is_gdb_with_foreground_non_cygwin_inferior (), etc.,
> instead of complicated conditions in 'if' clauses.
> 
> Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
> Reviewed-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> ---
>  winsup/cygwin/fhandler/termios.cc | 62 +++++++++++++++++++++----------
>  winsup/cygwin/tty.cc              | 24 ++++++++++--
>  2 files changed, 63 insertions(+), 23 deletions(-)
> 
> diff --git a/winsup/cygwin/fhandler/termios.cc b/winsup/cygwin/fhandler/termios.cc
> index 694a5c20f..08cab9a01 100644
> --- a/winsup/cygwin/fhandler/termios.cc
> +++ b/winsup/cygwin/fhandler/termios.cc
> @@ -311,6 +311,41 @@ fhandler_termios::echo_erase (int force)
>      doecho ("\b \b", 3);
>  }
>  
> +/* PID_NOTCYGWIN: check this for non-cygwin process.
> +   exec_dwProcessId == dwProcessId:
> +	       check this for GDB with non-cygwin inferior in pty
> +	       without pcon enabled. In this case, the inferior is not
> +	       cygwin process list. This condition is set true as
> +	       a marker for GDB with non-cygwin inferior in pty code.
> +   !PID_CYGPARENT: check this for GDB with cygwin inferior or
> +		   cygwin apps started from non-cygwin shell. */
> +
> +/* "Special" here means a non-cygwin process or a process whose parent
> +   is not a cygwin process */
> +inline static bool
> +is_foreground_special_process (_pinfo *p, pid_t tty_pgid)
> +{
> +  if (!p)
> +    return false;
> +  if (p->pgid != tty_pgid)
> +    return false;
> +  return !((p->process_state & PID_CYGPARENT)
> +	   && !(p->process_state & PID_NOTCYGWIN));
> +}
> +
> +/* exec_dwProcessId == dwProcessId:
> +       check this for GDB with non-cygwin inferior in pty
> +       In this case, the inferior is not cygwin process list.
> +       This condition is set true as a marker for GDB with
> +       non-cygwin inferior in pty code. */
> +inline static bool
> +is_gdb_with_foreground_non_cygwin_inferior (_pinfo *p, tty *ttyp)
> +{
> +  if (p->exec_dwProcessId != p->dwProcessId)
> +    return false;
> +  return ttyp->pty_input_state_eq (tty::to_nat);
> +}
> +
>  /* The basic policy is as follows:
>     - The signal generated by key press will be sent only to cygwin process.
>     - For non-cygwin process, CTRL_C_EVENT will be sent on Ctrl-C. */
> @@ -338,19 +373,9 @@ fhandler_termios::process_sigs (char c, tty* ttyp, fhandler_termios *fh)
>    for (unsigned i = 0; i < pids.npids; i++)
>      {
>        _pinfo *p = pids[i];
> -      /* PID_NOTCYGWIN: check this for non-cygwin process.
> -	 exec_dwProcessId == dwProcessId:
> -		     check this for GDB with non-cygwin inferior in pty
> -		     without pcon enabled. In this case, the inferior is not
> -		     cygwin process list. This condition is set true as
> -		     a marker for GDB with non-cygwin inferior in pty code.
> -	 !PID_CYGPARENT: check this for GDB with cygwin inferior or
> -			 cygwin apps started from non-cygwin shell. */
> -      if (c == '\003' && p && p->ctty == ttyp->ntty && p->pgid == pgid
> -	  && ((p->process_state & PID_NOTCYGWIN)
> -	      || ((p->exec_dwProcessId == p->dwProcessId)
> -		  && ttyp->pty_input_state_eq (tty::to_nat))
> -	      || !(p->process_state & PID_CYGPARENT)))
> +      if (c == '\003' && p && p->ctty == ttyp->ntty
> +	  && (is_foreground_special_process (p, pgid)
> +	      || is_gdb_with_foreground_non_cygwin_inferior (p, ttyp)))
>  	{
>  	  /* Ctrl-C event will be sent only to the processes attaching
>  	     to the same console. Therefore, attach to the console to
> @@ -372,7 +397,7 @@ fhandler_termios::process_sigs (char c, tty* ttyp, fhandler_termios *fh)
>  	  if (p->process_state & PID_NEW_PG)
>  	    GenerateConsoleCtrlEvent (CTRL_BREAK_EVENT, p->dwProcessId);
>  	  else if ((!fh || fh->need_send_ctrl_c_event ()
> -		    || p->exec_dwProcessId == p->dwProcessId)
> +		    || is_gdb_with_foreground_non_cygwin_inferior (p, ttyp))
>  		   && !ctrl_c_event_sent)
>  	    {
>  	      GenerateConsoleCtrlEvent (CTRL_C_EVENT, 0);
> @@ -403,12 +428,11 @@ fhandler_termios::process_sigs (char c, tty* ttyp, fhandler_termios *fh)
>  	  if (!p->cygstarted && !(p->process_state & PID_NOTCYGWIN)
>  	      && (p->process_state & PID_DEBUGGED))
>  	    with_debugger = true; /* inferior is cygwin app */
> -	  if (!(p->process_state & PID_NOTCYGWIN)
> -	      && (p->exec_dwProcessId == p->dwProcessId) /* Check marker */
> -	      && ttyp->pty_input_state_eq (tty::to_nat)
> -	      && p->pid == pgid)
> -	    with_debugger_nat = true; /* inferior is non-cygwin app */
>  	}
> +      if (p &&  p->ctty == ttyp->ntty
> +	  && !(p->process_state & PID_NOTCYGWIN)
> +	  && is_gdb_with_foreground_non_cygwin_inferior (p, ttyp))
> +	with_debugger_nat = true; /* inferior is non-cygwin app */
>      }
>    if ((with_debugger || with_debugger_nat) && need_discard_input)
>      {
> diff --git a/winsup/cygwin/tty.cc b/winsup/cygwin/tty.cc
> index 0c49dc2bd..acc21c0ca 100644
> --- a/winsup/cygwin/tty.cc
> +++ b/winsup/cygwin/tty.cc
> @@ -331,6 +331,23 @@ tty::wait_fwd ()
>      }
>  }
>  
> +inline static bool
> +is_non_cygwin_foreground_process (_pinfo *p, pid_t pgid)
> +{
> +  if (p->pgid != pgid)
> +    return false;
> +  return !!(p->process_state & PID_NOTCYGWIN);
> +}
> +
> +inline static bool
> +is_gdb_with_foreground_non_cygwin_inferior (_pinfo *p, pid_t pgid)
> +{
> +  if (p->pgid == pgid) /* GDB is the foreground process */
> +    return false;
> +  /* Below is true for GDB with non-cygwin inferior */
> +  return p->exec_dwProcessId == p->dwProcessId;
> +}
> +
>  bool
>  tty::nat_fg (pid_t pgid)
>  {
> @@ -340,10 +357,9 @@ tty::nat_fg (pid_t pgid)
>    for (unsigned i = 0; i < pids.npids; i++)
>      {
>        _pinfo *p = pids[i];
> -      if (p->ctty == ntty && p->pgid == pgid
> -	  && ((p->process_state & PID_NOTCYGWIN)
> -	      /* Below is true for GDB with non-cygwin inferior */
> -	      || p->exec_dwProcessId == p->dwProcessId))
> +      if (p->ctty == ntty
> +	  && (is_non_cygwin_foreground_process (p, pgid)
> +	      || is_gdb_with_foreground_non_cygwin_inferior (p, pgid)))
>  	return true;
>      }
>    if (pgid > MAX_PID)
> -- 
> 2.51.0