* [PATCH v2] Cygwin: pty: Reduce unecessary input transfer.
@ 2021-02-11 9:09 Takashi Yano
2021-02-12 9:43 ` Corinna Vinschen
2021-12-09 23:05 ` Johannes Schindelin
0 siblings, 2 replies; 9+ messages in thread
From: Takashi Yano @ 2021-02-11 9:09 UTC (permalink / raw)
To: cygwin-patches
- Currently, input transfer is performed every time one line is read(),
if the non-cygwin app is running in the background. With this patch,
transfer is triggered by setpgid() rather than read() so that the
unnecessary input transfer can be reduced much in that situation.
---
winsup/cygwin/fhandler.h | 15 +-
winsup/cygwin/fhandler_tty.cc | 377 ++++++++++++++++++++--------------
winsup/cygwin/spawn.cc | 78 ++++---
winsup/cygwin/tty.cc | 89 ++++++++
winsup/cygwin/tty.h | 16 +-
5 files changed, 376 insertions(+), 199 deletions(-)
diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 2fad7d33c..95011b6e3 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -2244,13 +2244,13 @@ class fhandler_pty_common: public fhandler_termios
public:
fhandler_pty_common ()
: fhandler_termios (),
- output_mutex (NULL), input_mutex (NULL),
+ output_mutex (NULL), input_mutex (NULL), pcon_mutex (NULL),
input_available_event (NULL)
{
pc.file_attributes (FILE_ATTRIBUTE_NORMAL);
}
static const unsigned pipesize = 128 * 1024;
- HANDLE output_mutex, input_mutex;
+ HANDLE output_mutex, input_mutex, pcon_mutex;
HANDLE input_available_event;
bool use_archetype () const {return true;}
@@ -2306,13 +2306,6 @@ class fhandler_pty_slave: public fhandler_pty_common
void fch_close_handles ();
public:
- /* Transfer direction for transfer_input() */
- enum xfer_dir
- {
- to_nat,
- to_cyg
- };
-
/* Constructor */
fhandler_pty_slave (int);
@@ -2371,8 +2364,8 @@ class fhandler_pty_slave: public fhandler_pty_common
void setup_locale (void);
tty *get_ttyp () { return (tty *) tc (); } /* Override as public */
void create_invisible_console (void);
- static void transfer_input (xfer_dir dir, HANDLE from, tty *ttyp,
- _minor_t unit, HANDLE input_available_event);
+ static void transfer_input (tty::xfer_dir dir, HANDLE from, tty *ttyp,
+ HANDLE input_available_event);
HANDLE get_input_available_event (void) { return input_available_event; }
bool pcon_activated (void) { return get_ttyp ()->pcon_activated; }
};
diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index 48b89ae77..f6eb3ae4d 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -127,7 +127,7 @@ set_switch_to_pcon (HANDLE *in, HANDLE *out, HANDLE *err, bool iscygwin)
if (*err == cfd->get_output_handle () ||
(fd == 2 && *err == GetStdHandle (STD_ERROR_HANDLE)))
replace_err = (fhandler_base *) cfd;
- if (cfd->get_major () == DEV_PTYS_MAJOR)
+ if (cfd->get_device () == (dev_t) myself->ctty)
{
fhandler_base *fh = cfd;
fhandler_pty_slave *ptys = (fhandler_pty_slave *) fh;
@@ -154,6 +154,7 @@ set_switch_to_pcon (HANDLE *in, HANDLE *out, HANDLE *err, bool iscygwin)
/* CreateProcess() is hooked for GDB etc. */
DEF_HOOK (CreateProcessA);
DEF_HOOK (CreateProcessW);
+DEF_HOOK (exit);
static BOOL WINAPI
CreateProcessA_Hooked
@@ -268,6 +269,34 @@ CreateProcessW_Hooked
return ret;
}
+void
+exit_Hooked (int e)
+{
+ if (isHybrid)
+ {
+ cygheap_fdenum cfd (false);
+ while (cfd.next () >= 0)
+ if (cfd->get_device () == (dev_t) myself->ctty)
+ {
+ fhandler_base *fh = cfd;
+ fhandler_pty_slave *ptys = (fhandler_pty_slave *) fh;
+ tty *ttyp = ptys->get_ttyp ();
+ HANDLE from = ptys->get_handle ();
+ HANDLE input_available_event = ptys->get_input_available_event ();
+ if (ttyp->getpgid () == myself->pgid
+ && ttyp->pcon_input_state_eq (tty::to_nat))
+ {
+ WaitForSingleObject (ptys->input_mutex, INFINITE);
+ fhandler_pty_slave::transfer_input (tty::to_cyg, from, ttyp,
+ input_available_event);
+ ReleaseMutex (ptys->input_mutex);
+ }
+ break;
+ }
+ }
+ exit_Orig (e);
+}
+
static void
convert_mb_str (UINT cp_to, char *ptr_to, size_t *len_to,
UINT cp_from, const char *ptr_from, size_t len_from,
@@ -438,7 +467,8 @@ fhandler_pty_master::accept_input ()
HANDLE write_to = get_output_handle ();
tmp_pathbuf tp;
- if (to_be_read_from_pcon ())
+ if (to_be_read_from_pcon ()
+ && get_ttyp ()->pcon_input_state == tty::to_nat)
{
write_to = to_slave;
@@ -487,11 +517,27 @@ fhandler_pty_master::accept_input ()
}
else
{
- DWORD rc;
+ BOOL rc = TRUE;
DWORD written = 0;
paranoid_printf ("about to write %u chars to slave", bytes_left);
- rc = WriteFile (write_to, p, bytes_left, &written, NULL);
+ /* Write line by line for transfer input. */
+ char *p0 = p;
+ char *p1 = p;
+ DWORD n;
+ while ((p1 = (char *) memchr (p0, '\n', bytes_left - (p0 - p)))
+ || (p1 = (char *) memchr (p0, '\r', bytes_left - (p0 - p))))
+ {
+ n = p1 - p0 + 1;
+ rc = WriteFile (write_to, p0, n, &n, NULL);
+ written += n;
+ p0 = p1 + 1;
+ }
+ if ((n = bytes_left - (p0 - p)))
+ {
+ rc = WriteFile (write_to, p0, n, &n, NULL);
+ written += n;
+ }
if (!rc)
{
debug_printf ("error writing to pipe %p %E", write_to);
@@ -669,7 +715,7 @@ fhandler_pty_slave::open (int flags, mode_t)
{
&from_master_local, &input_available_event, &input_mutex, &inuse,
&output_mutex, &to_master_local, &pty_owner, &to_master_cyg_local,
- &from_master_cyg_local,
+ &from_master_cyg_local, &pcon_mutex,
NULL
};
@@ -697,6 +743,11 @@ fhandler_pty_slave::open (int flags, mode_t)
errmsg = "open input mutex failed, %E";
goto err;
}
+ if (!(pcon_mutex = get_ttyp ()->open_mutex (PCON_MUTEX, MAXIMUM_ALLOWED)))
+ {
+ errmsg = "open pcon mutex failed, %E";
+ goto err;
+ }
shared_name (buf, INPUT_AVAILABLE_EVENT, get_minor ());
if (!(input_available_event = OpenEvent (MAXIMUM_ALLOWED, TRUE, buf)))
{
@@ -960,12 +1011,19 @@ fhandler_pty_slave::set_switch_to_pcon (void)
{
isHybrid = true;
setup_locale ();
+ myself->exec_dwProcessId = myself->dwProcessId;
bool nopcon = (disable_pcon || !term_has_pcon_cap (NULL));
- if (!setup_pseudoconsole (nopcon))
- fhandler_pty_slave::transfer_input (fhandler_pty_slave::to_nat,
- get_handle_cyg (),
- get_ttyp (), get_minor (),
- input_available_event);
+ WaitForSingleObject (pcon_mutex, INFINITE);
+ bool pcon_enabled = setup_pseudoconsole (nopcon);
+ ReleaseMutex (pcon_mutex);
+ if (!pcon_enabled && get_ttyp ()->getpgid () == myself->pgid
+ && get_ttyp ()->pcon_input_state_eq (tty::to_cyg))
+ {
+ WaitForSingleObject (input_mutex, INFINITE);
+ transfer_input (tty::to_nat, get_handle_cyg (), get_ttyp (),
+ input_available_event);
+ ReleaseMutex (input_mutex);
+ }
}
}
@@ -985,19 +1043,24 @@ fhandler_pty_slave::reset_switch_to_pcon (void)
h_gdb_process = NULL;
if (isHybrid)
{
- if (get_ttyp ()->switch_to_pcon_in
- && get_ttyp ()->pcon_pid == myself->pid)
- fhandler_pty_slave::transfer_input (fhandler_pty_slave::to_cyg,
- get_handle (),
- get_ttyp (), get_minor (),
- input_available_event);
- if (get_ttyp ()->master_is_running_as_service)
+ if (get_ttyp ()->getpgid () == myself->pgid
+ && get_ttyp ()->pcon_input_state_eq (tty::to_nat))
+ {
+ WaitForSingleObject (input_mutex, INFINITE);
+ transfer_input (tty::to_cyg, get_handle (), get_ttyp (),
+ input_available_event);
+ ReleaseMutex (input_mutex);
+ }
+ if (get_ttyp ()->master_is_running_as_service
+ && get_ttyp ()->pcon_activated)
/* If the master is running as service, re-attaching to
the console of the parent process will fail.
Therefore, never close pseudo console here. */
return;
bool need_restore_handles = get_ttyp ()->pcon_activated;
+ WaitForSingleObject (pcon_mutex, INFINITE);
close_pseudoconsole (get_ttyp ());
+ ReleaseMutex (pcon_mutex);
if (need_restore_handles)
{
pinfo p (get_ttyp ()->master_pid);
@@ -1047,6 +1110,7 @@ fhandler_pty_slave::reset_switch_to_pcon (void)
if (fix_err)
SetStdHandle (STD_ERROR_HANDLE, get_output_handle ());
}
+ myself->exec_dwProcessId = 0;
isHybrid = false;
}
}
@@ -1057,11 +1121,6 @@ fhandler_pty_slave::reset_switch_to_pcon (void)
return;
if (isHybrid)
return;
- if (get_ttyp ()->switch_to_pcon_in && !get_ttyp ()->pcon_activated)
- fhandler_pty_slave::transfer_input (fhandler_pty_slave::to_cyg,
- get_handle (),
- get_ttyp (), get_minor (),
- input_available_event);
get_ttyp ()->pcon_pid = 0;
get_ttyp ()->switch_to_pcon_in = false;
get_ttyp ()->pcon_activated = false;
@@ -1119,77 +1178,47 @@ fhandler_pty_slave::mask_switch_to_pcon_in (bool mask, bool xfer)
else if (InterlockedDecrement (&num_reader) == 0)
CloseHandle (slave_reading);
- if (get_ttyp ()->switch_to_pcon_in && !!masked != mask && xfer)
- { /* Transfer input */
- bool attach_restore = false;
- DWORD pcon_winpid = 0;
- if (get_ttyp ()->pcon_pid)
- {
- pinfo p (get_ttyp ()->pcon_pid);
- if (p)
- pcon_winpid = p->exec_dwProcessId ?: p->dwProcessId;
- }
- if (mask)
- {
- HANDLE from = get_handle ();
- if (get_ttyp ()->pcon_activated && pcon_winpid
- && !get_console_process_id (pcon_winpid, true))
- {
- HANDLE pcon_owner =
- OpenProcess (PROCESS_DUP_HANDLE, FALSE, pcon_winpid);
- DuplicateHandle (pcon_owner, get_ttyp ()->h_pcon_in,
- GetCurrentProcess (), &from,
- 0, TRUE, DUPLICATE_SAME_ACCESS);
- CloseHandle (pcon_owner);
- FreeConsole ();
- AttachConsole (pcon_winpid);
- attach_restore = true;
- }
- fhandler_pty_slave::transfer_input (fhandler_pty_slave::to_cyg,
- from,
- get_ttyp (), get_minor (),
- input_available_event);
- }
- else
+ /* In GDB, transfer input based on setpgid() does not work because
+ GDB may not set terminal process group properly. Therefore,
+ transfer input here if isHybrid is set. */
+ if (get_ttyp ()->switch_to_pcon_in && !!masked != mask && xfer && isHybrid)
+ {
+ if (mask && get_ttyp ()->pcon_input_state_eq (tty::to_nat))
{
- if (get_ttyp ()->pcon_activated && pcon_winpid
- && !get_console_process_id (pcon_winpid, true))
- {
- FreeConsole ();
- AttachConsole (pcon_winpid);
- attach_restore = true;
- }
- fhandler_pty_slave::transfer_input (fhandler_pty_slave::to_nat,
- get_handle_cyg (),
- get_ttyp (), get_minor (),
- input_available_event);
+ WaitForSingleObject (input_mutex, INFINITE);
+ transfer_input (tty::to_cyg, get_handle (), get_ttyp (),
+ input_available_event);
+ ReleaseMutex (input_mutex);
}
- if (attach_restore)
+ else if (!mask && get_ttyp ()->pcon_input_state_eq (tty::to_cyg))
{
- FreeConsole ();
- pinfo p (myself->ppid);
- if (p)
- {
- if (!AttachConsole (p->dwProcessId))
- AttachConsole (ATTACH_PARENT_PROCESS);
- }
- else
- AttachConsole (ATTACH_PARENT_PROCESS);
+ WaitForSingleObject (input_mutex, INFINITE);
+ transfer_input (tty::to_nat, get_handle_cyg (), get_ttyp (),
+ input_available_event);
+ ReleaseMutex (input_mutex);
}
}
- return;
}
bool
fhandler_pty_master::to_be_read_from_pcon (void)
{
+ if (!get_ttyp ()->switch_to_pcon_in)
+ return false;
+
char name[MAX_PATH];
shared_name (name, TTY_SLAVE_READING, get_minor ());
HANDLE masked = OpenEvent (READ_CONTROL, FALSE, name);
CloseHandle (masked);
- return get_ttyp ()->pcon_start
- || (get_ttyp ()->switch_to_pcon_in && !masked);
+ if (masked) /* The foreground process is cygwin process */
+ return false;
+
+ if (!pinfo (get_ttyp ()->getpgid ()))
+ /* GDB may set invalid process group for non-cygwin process. */
+ return true;
+
+ return get_ttyp ()->pcon_fg (get_ttyp ()->getpgid ());
}
void __reg3
@@ -1720,6 +1749,7 @@ fhandler_pty_slave::fch_open_handles (bool chown)
TRUE, buf);
output_mutex = get_ttyp ()->open_output_mutex (write_access);
input_mutex = get_ttyp ()->open_input_mutex (write_access);
+ pcon_mutex = get_ttyp ()->open_mutex (PCON_MUTEX, write_access);
inuse = get_ttyp ()->open_inuse (write_access);
if (!input_available_event || !output_mutex || !input_mutex || !inuse)
{
@@ -1873,6 +1903,8 @@ fhandler_pty_common::close ()
get_minor (), get_handle (), get_output_handle ());
if (!ForceCloseHandle (input_mutex))
termios_printf ("CloseHandle (input_mutex<%p>), %E", input_mutex);
+ if (!ForceCloseHandle (pcon_mutex))
+ termios_printf ("CloseHandle (pcon_mutex<%p>), %E", pcon_mutex);
if (!ForceCloseHandle1 (get_handle (), from_pty))
termios_printf ("CloseHandle (get_handle ()<%p>), %E", get_handle ());
if (!ForceCloseHandle1 (get_output_handle (), to_pty))
@@ -2011,72 +2043,103 @@ fhandler_pty_master::write (const void *ptr, size_t len)
push_process_state process_state (PID_TTYOU);
- /* Write terminal input to to_slave pipe instead of output_handle
- if current application is native console application. */
- if (to_be_read_from_pcon () && get_ttyp ()->pcon_activated)
+ if (get_ttyp ()->pcon_start)
{
- tmp_pathbuf tp;
- char *buf = (char *) ptr;
- size_t nlen = len;
- if (get_ttyp ()->term_code_page != CP_UTF8)
- {
- static mbstate_t mbp;
- buf = tp.c_get ();
- nlen = NT_MAX_PATH;
- convert_mb_str (CP_UTF8, buf, &nlen,
- get_ttyp ()->term_code_page, (const char *) ptr, len,
- &mbp);
- }
+ /* Pseudo condole support uses "CSI6n" to get cursor position.
+ If the reply for "CSI6n" is divided into multiple writes,
+ pseudo console sometimes does not recognize it. Therefore,
+ put them together into wpbuf and write all at once. */
+ static const int wpbuf_len = strlen ("\033[32768;32868R");
+ static char wpbuf[wpbuf_len];
+ static int ixput = 0;
+ static int state = 0;
+ DWORD n;
WaitForSingleObject (input_mutex, INFINITE);
-
- DWORD wLen;
-
- if (get_ttyp ()->pcon_start)
+ for (size_t i = 0; i < len; i++)
{
- /* Pseudo condole support uses "CSI6n" to get cursor position.
- If the reply for "CSI6n" is divided into multiple writes,
- pseudo console sometimes does not recognize it. Therefore,
- put them together into wpbuf and write all at once. */
- static const int wpbuf_len = 64;
- static char wpbuf[wpbuf_len];
- static int ixput = 0;
-
- if (ixput + nlen < wpbuf_len)
+ if (p[i] == '\033')
{
- memcpy (wpbuf + ixput, buf, nlen);
- ixput += nlen;
- }
- else
- {
- WriteFile (to_slave, wpbuf, ixput, &wLen, NULL);
+ if (ixput)
+ line_edit (wpbuf, ixput, ti, &ret);
ixput = 0;
- get_ttyp ()->pcon_start = false;
- WriteFile (to_slave, buf, nlen, &wLen, NULL);
+ state = 1;
}
- if (ixput && memchr (wpbuf, 'R', ixput))
+ if (state == 1)
{
- WriteFile (to_slave, wpbuf, ixput, &wLen, NULL);
- ixput = 0;
- get_ttyp ()->pcon_start = false;
+ if (ixput < wpbuf_len)
+ wpbuf[ixput++] = p[i];
+ else
+ {
+ if (!get_ttyp ()->req_xfer_input)
+ WriteFile (to_slave, wpbuf, ixput, &n, NULL);
+ ixput = 0;
+ wpbuf[ixput++] = p[i];
+ }
}
- ReleaseMutex (input_mutex);
- if (get_ttyp ()->switch_to_pcon_in)
+ else
+ line_edit (p + i, 1, ti, &ret);
+ if (state == 1 && p[i] == 'R')
+ state = 2;
+ }
+ if (state == 2)
+ {
+ if (!get_ttyp ()->req_xfer_input)
+ WriteFile (to_slave, wpbuf, ixput, &n, NULL);
+ ixput = 0;
+ state = 0;
+ get_ttyp ()->req_xfer_input = false;
+ get_ttyp ()->pcon_start = false;
+ }
+ ReleaseMutex (input_mutex);
+
+ if (!get_ttyp ()->pcon_start)
+ {
+ pinfo pp (get_ttyp ()->pcon_start_pid);
+ bool pcon_fg = (pp && get_ttyp ()->getpgid () == pp->pgid);
+ /* GDB may set WINPID rather than cygwin PID to process group
+ when the debugged process is a non-cygwin process.*/
+ pcon_fg |= !pinfo (get_ttyp ()->getpgid ());
+ if (get_ttyp ()->switch_to_pcon_in && pcon_fg
+ && get_ttyp ()->pcon_input_state_eq (tty::to_cyg))
{
- fhandler_pty_slave::transfer_input (fhandler_pty_slave::to_nat,
- from_master_cyg,
- get_ttyp (), get_minor (),
- input_available_event);
/* This accept_input() call is needed in order to transfer input
which is not accepted yet to non-cygwin pipe. */
if (get_readahead_valid ())
accept_input ();
+ WaitForSingleObject (input_mutex, INFINITE);
+ fhandler_pty_slave::transfer_input (tty::to_nat, from_master_cyg,
+ get_ttyp (),
+ input_available_event);
+ ReleaseMutex (input_mutex);
}
- return len;
+ get_ttyp ()->pcon_start_pid = 0;
}
- WriteFile (to_slave, buf, nlen, &wLen, NULL);
+ return len;
+ }
+
+ /* Write terminal input to to_slave pipe instead of output_handle
+ if current application is native console application. */
+ if (to_be_read_from_pcon () && get_ttyp ()->pcon_activated
+ && get_ttyp ()->pcon_input_state == tty::to_nat)
+ {
+ tmp_pathbuf tp;
+ char *buf = (char *) ptr;
+ size_t nlen = len;
+ if (get_ttyp ()->term_code_page != CP_UTF8)
+ {
+ static mbstate_t mbp;
+ buf = tp.c_get ();
+ nlen = NT_MAX_PATH;
+ convert_mb_str (CP_UTF8, buf, &nlen,
+ get_ttyp ()->term_code_page, (const char *) ptr, len,
+ &mbp);
+ }
+ WaitForSingleObject (input_mutex, INFINITE);
+ DWORD n;
+ WriteFile (to_slave, buf, nlen, &n, NULL);
ReleaseMutex (input_mutex);
return len;
@@ -2248,6 +2311,8 @@ fhandler_pty_slave::fixup_after_exec ()
/* CreateProcess() is hooked for GDB etc. */
DO_HOOK (NULL, CreateProcessA);
DO_HOOK (NULL, CreateProcessW);
+ if (CreateProcessA_Orig || CreateProcessW_Orig)
+ DO_HOOK (NULL, exit);
}
/* This thread function handles the master control pipe. It waits for a
@@ -2739,6 +2804,10 @@ fhandler_pty_master::setup ()
if (!(input_mutex = CreateMutex (&sa, FALSE, buf)))
goto err;
+ errstr = shared_name (buf, PCON_MUTEX, unit);
+ if (!(pcon_mutex = CreateMutex (&sa, FALSE, buf)))
+ goto err;
+
attach_mutex = CreateMutex (&sa, FALSE, NULL);
/* Create master control pipe which allows the master to duplicate
@@ -2980,10 +3049,17 @@ fhandler_pty_slave::setup_pseudoconsole (bool nopcon)
}
HANDLE hpConIn, hpConOut;
- acquire_output_mutex (INFINITE);
if (get_ttyp ()->pcon_pid && get_ttyp ()->pcon_pid != myself->pid
&& !!pinfo (get_ttyp ()->pcon_pid) && get_ttyp ()->pcon_activated)
{
+ /* Send CSI6n just for requesting transfer input. */
+ DWORD n;
+ WaitForSingleObject (input_mutex, INFINITE);
+ get_ttyp ()->req_xfer_input = true;
+ get_ttyp ()->pcon_start = true;
+ get_ttyp ()->pcon_start_pid = myself->pid;
+ WriteFile (get_output_handle_cyg (), "\033[6n", 4, &n, NULL);
+ ReleaseMutex (input_mutex);
/* Attach to the pseudo console which already exits. */
pinfo p (get_ttyp ()->pcon_pid);
HANDLE pcon_owner =
@@ -3068,8 +3144,9 @@ fhandler_pty_slave::setup_pseudoconsole (bool nopcon)
si.StartupInfo.hStdOutput = NULL;
si.StartupInfo.hStdError = NULL;
- get_ttyp ()->pcon_start = true;
get_ttyp ()->pcon_activated = true;
+ get_ttyp ()->pcon_start = true;
+ get_ttyp ()->pcon_start_pid = myself->pid;
if (!CreateProcessW (NULL, cmd, &sec_none, &sec_none,
TRUE, EXTENDED_STARTUPINFO_PRESENT,
NULL, NULL, &si.StartupInfo, &pi))
@@ -3183,7 +3260,6 @@ skip_create:
if (get_ttyp ()->previous_output_code_page)
SetConsoleOutputCP (get_ttyp ()->previous_output_code_page);
- release_output_mutex ();
return true;
cleanup_pcon_in:
@@ -3196,6 +3272,7 @@ cleanup_helper_process:
cleanup_event_and_pipes:
CloseHandle (hello);
get_ttyp ()->pcon_start = false;
+ get_ttyp ()->pcon_start_pid = 0;
get_ttyp ()->pcon_activated = false;
skip_close_hello:
CloseHandle (goodbye);
@@ -3212,7 +3289,6 @@ cleanup_pseudo_console:
CloseHandle (tmp);
}
fallback:
- release_output_mutex ();
return false;
}
@@ -3312,6 +3388,7 @@ fhandler_pty_slave::close_pseudoconsole (tty *ttyp)
ttyp->switch_to_pcon_in = false;
ttyp->pcon_pid = 0;
ttyp->pcon_start = false;
+ ttyp->pcon_start_pid = 0;
}
}
else
@@ -3429,7 +3506,6 @@ fhandler_pty_slave::term_has_pcon_cap (const WCHAR *env)
/* Set pcon_activated and pcon_start so that the response
will sent to io_handle rather than io_handle_cyg. */
get_ttyp ()->pcon_activated = true;
- get_ttyp ()->pcon_pid = myself->pid;
/* pcon_start will be cleared in master write() when CSI6n is responded. */
get_ttyp ()->pcon_start = true;
WriteFile (get_output_handle_cyg (), "\033[6n", 4, &n, NULL);
@@ -3526,11 +3602,11 @@ fhandler_pty_master::get_master_fwd_thread_param (master_fwd_thread_param_t *p)
#define ALT_PRESSED (LEFT_ALT_PRESSED | RIGHT_ALT_PRESSED)
#define CTRL_PRESSED (LEFT_CTRL_PRESSED | RIGHT_CTRL_PRESSED)
void
-fhandler_pty_slave::transfer_input (xfer_dir dir, HANDLE from, tty *ttyp,
- _minor_t unit, HANDLE input_available_event)
+fhandler_pty_slave::transfer_input (tty::xfer_dir dir, HANDLE from, tty *ttyp,
+ HANDLE input_available_event)
{
HANDLE to;
- if (dir == to_nat)
+ if (dir == tty::to_nat)
to = ttyp->to_slave ();
else
to = ttyp->to_slave_cyg ();
@@ -3548,14 +3624,14 @@ fhandler_pty_slave::transfer_input (xfer_dir dir, HANDLE from, tty *ttyp,
char pipe[MAX_PATH];
__small_sprintf (pipe,
"\\\\.\\pipe\\cygwin-%S-pty%d-master-ctl",
- &cygheap->installation_key, unit);
+ &cygheap->installation_key, ttyp->get_minor ());
pipe_request req = { GetCurrentProcessId () };
pipe_reply repl;
DWORD len;
if (!CallNamedPipe (pipe, &req, sizeof req,
&repl, sizeof repl, &len, 500))
return; /* What can we do? */
- if (dir == to_nat)
+ if (dir == tty::to_nat)
to = repl.to_slave;
else
to = repl.to_slave_cyg;
@@ -3563,7 +3639,7 @@ fhandler_pty_slave::transfer_input (xfer_dir dir, HANDLE from, tty *ttyp,
UINT cp_from = 0, cp_to = 0;
- if (dir == to_nat)
+ if (dir == tty::to_nat)
{
cp_from = ttyp->term_code_page;
if (ttyp->pcon_activated)
@@ -3582,7 +3658,7 @@ fhandler_pty_slave::transfer_input (xfer_dir dir, HANDLE from, tty *ttyp,
bool transfered = false;
- if (dir == to_cyg && ttyp->pcon_activated)
+ if (dir == tty::to_cyg && ttyp->pcon_activated)
{ /* from handle is console handle */
INPUT_RECORD r[INREC_SIZE];
DWORD n;
@@ -3607,18 +3683,6 @@ fhandler_pty_slave::transfer_input (xfer_dir dir, HANDLE from, tty *ttyp,
&& (ctrl_key_state & CTRL_PRESSED)
&& !(ctrl_key_state & ALT_PRESSED))
buf[len++] = '\0';
- else if (r[i].Event.KeyEvent.wVirtualKeyCode == VK_F3)
- {
- /* If the cursor position report for CSI6n matches
- with e.g. "ESC[1;2R", pseudo console translates
- it to Shift-F3. This is a workaround for that. */
- int ctrl = 1;
- if (ctrl_key_state & SHIFT_PRESSED) ctrl += 1;
- if (ctrl_key_state & ALT_PRESSED) ctrl += 2;
- if (ctrl_key_state & CTRL_PRESSED) ctrl += 4;
- __small_sprintf (buf + len, "\033[1;%1dR", ctrl);
- len += 6;
- }
else
{ /* arrow/function keys */
/* FIXME: The current code generates cygwin terminal
@@ -3666,11 +3730,15 @@ fhandler_pty_slave::transfer_input (xfer_dir dir, HANDLE from, tty *ttyp,
DWORD n = MIN (bytes_in_pipe, NT_MAX_PATH);
ReadFile (from, buf, n, &n, NULL);
char *ptr = buf;
- if (dir == to_nat && ttyp->pcon_activated)
+ if (dir == tty::to_nat)
{
char *p = buf;
- while ((p = (char *) memchr (p, '\n', n - (p - buf))))
- *p = '\r';
+ if (ttyp->pcon_activated)
+ while ((p = (char *) memchr (p, '\n', n - (p - buf))))
+ *p = '\r';
+ else
+ while ((p = (char *) memchr (p, '\r', n - (p - buf))))
+ *p = '\n';
}
if (cp_to != cp_from)
{
@@ -3686,8 +3754,9 @@ fhandler_pty_slave::transfer_input (xfer_dir dir, HANDLE from, tty *ttyp,
}
}
- if (dir == to_nat)
+ if (dir == tty::to_nat)
ResetEvent (input_available_event);
else if (transfered)
SetEvent (input_available_event);
+ ttyp->pcon_input_state = dir;
}
diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index c4b612815..4d4d599ca 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -656,18 +656,19 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
bool enable_pcon = false;
HANDLE ptys_from_master = NULL;
HANDLE ptys_input_available_event = NULL;
- HANDLE ptys_output_mutex = NULL;
+ HANDLE ptys_pcon_mutex = NULL;
+ HANDLE ptys_input_mutex = NULL;
tty *ptys_ttyp = NULL;
- _minor_t ptys_unit = 0;
if (!iscygwin () && ptys_primary && is_console_app (runpath))
{
bool nopcon = mode != _P_OVERLAY && mode != _P_WAIT;
if (disable_pcon || !ptys_primary->term_has_pcon_cap (envblock))
nopcon = true;
+ ptys_ttyp = ptys_primary->get_ttyp ();
+ WaitForSingleObject (ptys_primary->pcon_mutex, INFINITE);
if (ptys_primary->setup_pseudoconsole (nopcon))
enable_pcon = true;
- ptys_ttyp = ptys_primary->get_ttyp ();
- ptys_unit = ptys_primary->get_minor ();
+ ReleaseMutex (ptys_primary->pcon_mutex);
ptys_from_master = ptys_primary->get_handle ();
DuplicateHandle (GetCurrentProcess (), ptys_from_master,
GetCurrentProcess (), &ptys_from_master,
@@ -677,14 +678,21 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
DuplicateHandle (GetCurrentProcess (), ptys_input_available_event,
GetCurrentProcess (), &ptys_input_available_event,
0, 0, DUPLICATE_SAME_ACCESS);
- DuplicateHandle (GetCurrentProcess (), ptys_primary->output_mutex,
- GetCurrentProcess (), &ptys_output_mutex,
+ DuplicateHandle (GetCurrentProcess (), ptys_primary->pcon_mutex,
+ GetCurrentProcess (), &ptys_pcon_mutex,
+ 0, 0, DUPLICATE_SAME_ACCESS);
+ DuplicateHandle (GetCurrentProcess (), ptys_primary->input_mutex,
+ GetCurrentProcess (), &ptys_input_mutex,
0, 0, DUPLICATE_SAME_ACCESS);
- if (!enable_pcon)
- fhandler_pty_slave::transfer_input (fhandler_pty_slave::to_nat,
- ptys_primary->get_handle_cyg (),
- ptys_ttyp, ptys_unit,
- ptys_input_available_event);
+ if (!enable_pcon && ptys_ttyp->getpgid () == myself->pgid
+ && ptys_ttyp->pcon_input_state_eq (tty::to_cyg))
+ {
+ WaitForSingleObject (ptys_input_mutex, INFINITE);
+ fhandler_pty_slave::transfer_input (tty::to_nat,
+ ptys_primary->get_handle_cyg (),
+ ptys_ttyp, ptys_input_available_event);
+ ReleaseMutex (ptys_input_mutex);
+ }
}
/* Set up needed handles for stdio */
@@ -969,20 +977,22 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
if (ptys_ttyp)
{
ptys_ttyp->wait_pcon_fwd ();
- /* Do not transfer input if another process using pseudo
- console exists. */
- WaitForSingleObject (ptys_output_mutex, INFINITE);
- if (!fhandler_pty_common::get_console_process_id
- (myself->exec_dwProcessId, false, true, true))
- fhandler_pty_slave::transfer_input (fhandler_pty_slave::to_cyg,
- ptys_from_master,
- ptys_ttyp, ptys_unit,
- ptys_input_available_event);
+ if (ptys_ttyp->getpgid () == myself->pgid
+ && ptys_ttyp->pcon_input_state_eq (tty::to_nat))
+ {
+ WaitForSingleObject (ptys_input_mutex, INFINITE);
+ fhandler_pty_slave::transfer_input (tty::to_cyg,
+ ptys_from_master, ptys_ttyp,
+ ptys_input_available_event);
+ ReleaseMutex (ptys_input_mutex);
+ }
CloseHandle (ptys_from_master);
+ CloseHandle (ptys_input_mutex);
CloseHandle (ptys_input_available_event);
+ WaitForSingleObject (ptys_pcon_mutex, INFINITE);
fhandler_pty_slave::close_pseudoconsole (ptys_ttyp);
- ReleaseMutex (ptys_output_mutex);
- CloseHandle (ptys_output_mutex);
+ ReleaseMutex (ptys_pcon_mutex);
+ CloseHandle (ptys_pcon_mutex);
}
if (cons_native)
{
@@ -1002,20 +1012,22 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
if (ptys_ttyp)
{
ptys_ttyp->wait_pcon_fwd ();
- /* Do not transfer input if another process using pseudo
- console exists. */
- WaitForSingleObject (ptys_output_mutex, INFINITE);
- if (!fhandler_pty_common::get_console_process_id
- (myself->exec_dwProcessId, false, true, true))
- fhandler_pty_slave::transfer_input (fhandler_pty_slave::to_cyg,
- ptys_from_master,
- ptys_ttyp, ptys_unit,
- ptys_input_available_event);
+ if (ptys_ttyp->getpgid () == myself->pgid
+ && ptys_ttyp->pcon_input_state_eq (tty::to_nat))
+ {
+ WaitForSingleObject (ptys_input_mutex, INFINITE);
+ fhandler_pty_slave::transfer_input (tty::to_cyg,
+ ptys_from_master, ptys_ttyp,
+ ptys_input_available_event);
+ ReleaseMutex (ptys_input_mutex);
+ }
CloseHandle (ptys_from_master);
+ CloseHandle (ptys_input_mutex);
CloseHandle (ptys_input_available_event);
+ WaitForSingleObject (ptys_pcon_mutex, INFINITE);
fhandler_pty_slave::close_pseudoconsole (ptys_ttyp);
- ReleaseMutex (ptys_output_mutex);
- CloseHandle (ptys_output_mutex);
+ ReleaseMutex (ptys_pcon_mutex);
+ CloseHandle (ptys_pcon_mutex);
}
if (cons_native)
{
diff --git a/winsup/cygwin/tty.cc b/winsup/cygwin/tty.cc
index fb4501c1b..41f81f694 100644
--- a/winsup/cygwin/tty.cc
+++ b/winsup/cygwin/tty.cc
@@ -241,6 +241,7 @@ tty::init ()
term_code_page = 0;
pcon_last_time = 0;
pcon_start = false;
+ pcon_start_pid = 0;
pcon_cap_checked = false;
has_csi6n = false;
need_invisible_console = false;
@@ -248,6 +249,8 @@ tty::init ()
previous_code_page = 0;
previous_output_code_page = 0;
master_is_running_as_service = false;
+ req_xfer_input = false;
+ pcon_input_state = to_cyg;
}
HANDLE
@@ -293,6 +296,77 @@ tty_min::ttyname ()
return d.name ();
}
+void
+tty_min::setpgid (int pid)
+{
+ fhandler_pty_slave *ptys = NULL;
+ cygheap_fdenum cfd (false);
+ while (cfd.next () >= 0 && ptys == NULL)
+ if (cfd->get_device () == getntty ())
+ ptys = (fhandler_pty_slave *) (fhandler_base *) cfd;
+
+ if (ptys)
+ {
+ tty *ttyp = ptys->get_ttyp ();
+ WaitForSingleObject (ptys->pcon_mutex, INFINITE);
+ bool was_pcon_fg = ttyp->pcon_fg (pgid);
+ bool pcon_fg = ttyp->pcon_fg (pid);
+ if (!was_pcon_fg && pcon_fg && ttyp->switch_to_pcon_in
+ && ttyp->pcon_input_state_eq (tty::to_cyg))
+ {
+ WaitForSingleObject (ptys->input_mutex, INFINITE);
+ fhandler_pty_slave::transfer_input (tty::to_nat,
+ ptys->get_handle_cyg (), ttyp,
+ ptys->get_input_available_event ());
+ ReleaseMutex (ptys->input_mutex);
+ }
+ else if (was_pcon_fg && !pcon_fg && ttyp->switch_to_pcon_in
+ && ttyp->pcon_input_state_eq (tty::to_nat))
+ {
+ bool attach_restore = false;
+ DWORD pcon_winpid = 0;
+ if (ttyp->pcon_pid)
+ {
+ pinfo p (ttyp->pcon_pid);
+ if (p)
+ pcon_winpid = p->exec_dwProcessId ?: p->dwProcessId;
+ }
+ HANDLE from = ptys->get_handle ();
+ if (ttyp->pcon_activated && pcon_winpid
+ && !ptys->get_console_process_id (pcon_winpid, true))
+ {
+ HANDLE pcon_owner =
+ OpenProcess (PROCESS_DUP_HANDLE, FALSE, pcon_winpid);
+ DuplicateHandle (pcon_owner, ttyp->h_pcon_in,
+ GetCurrentProcess (), &from,
+ 0, TRUE, DUPLICATE_SAME_ACCESS);
+ CloseHandle (pcon_owner);
+ FreeConsole ();
+ AttachConsole (pcon_winpid);
+ attach_restore = true;
+ }
+ WaitForSingleObject (ptys->input_mutex, INFINITE);
+ fhandler_pty_slave::transfer_input (tty::to_cyg, from, ttyp,
+ ptys->get_input_available_event ());
+ ReleaseMutex (ptys->input_mutex);
+ if (attach_restore)
+ {
+ FreeConsole ();
+ pinfo p (myself->ppid);
+ if (p)
+ {
+ if (!AttachConsole (p->dwProcessId))
+ AttachConsole (ATTACH_PARENT_PROCESS);
+ }
+ else
+ AttachConsole (ATTACH_PARENT_PROCESS);
+ }
+ }
+ ReleaseMutex (ptys->pcon_mutex);
+ }
+ pgid = pid;
+}
+
void
tty::wait_pcon_fwd (bool init)
{
@@ -312,3 +386,18 @@ tty::wait_pcon_fwd (bool init)
cygwait (tw);
}
}
+
+bool
+tty::pcon_fg (pid_t pgid)
+{
+ /* Check if the terminal pgid matches with the pgid of the
+ non-cygwin process. */
+ winpids pids ((DWORD) 0);
+ for (unsigned i = 0; i < pids.npids; i++)
+ {
+ _pinfo *p = pids[i];
+ if (p->ctty == ntty && p->pgid == pgid && p->exec_dwProcessId)
+ return true;
+ }
+ return false;
+}
diff --git a/winsup/cygwin/tty.h b/winsup/cygwin/tty.h
index e2e6dfeb6..e1de7ab46 100644
--- a/winsup/cygwin/tty.h
+++ b/winsup/cygwin/tty.h
@@ -20,6 +20,7 @@ details. */
#define INPUT_AVAILABLE_EVENT "cygtty.input.avail"
#define OUTPUT_MUTEX "cygtty.output.mutex"
#define INPUT_MUTEX "cygtty.input.mutex"
+#define PCON_MUTEX "cygtty.pcon.mutex"
#define TTY_SLAVE_ALIVE "cygtty.slave_alive"
#define TTY_SLAVE_READING "cygtty.slave_reading"
@@ -72,7 +73,7 @@ public:
dev_t getntty () const {return ntty;}
_minor_t get_minor () const {return device::minor (ntty);}
pid_t getpgid () const {return pgid;}
- void setpgid (int pid) {pgid = pid;}
+ void setpgid (int pid);
int getsid () const {return sid;}
void setsid (pid_t tsid) {sid = tsid;}
void kill_pgrp (int);
@@ -89,6 +90,13 @@ class tty: public tty_min
public:
pid_t master_pid; /* PID of tty master process */
+ /* Transfer direction for fhandler_pty_slave::transfer_input() */
+ enum xfer_dir
+ {
+ to_cyg,
+ to_nat
+ };
+
private:
HANDLE _from_master;
HANDLE _from_master_cyg;
@@ -98,6 +106,7 @@ private:
HANDLE _to_slave_cyg;
bool pcon_activated;
bool pcon_start;
+ pid_t pcon_start_pid;
bool switch_to_pcon_in;
pid_t pcon_pid;
UINT term_code_page;
@@ -114,6 +123,8 @@ private:
UINT previous_code_page;
UINT previous_output_code_page;
bool master_is_running_as_service;
+ bool req_xfer_input;
+ xfer_dir pcon_input_state;
public:
HANDLE from_master () const { return _from_master; }
@@ -148,9 +159,12 @@ public:
static void __stdcall create_master (int);
static void __stdcall init_session ();
void wait_pcon_fwd (bool init = true);
+ bool pcon_input_state_eq (xfer_dir x) { return pcon_input_state == x; }
+ bool pcon_fg (pid_t pgid);
friend class fhandler_pty_common;
friend class fhandler_pty_master;
friend class fhandler_pty_slave;
+ friend class tty_min;
};
class tty_list
--
2.30.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] Cygwin: pty: Reduce unecessary input transfer.
2021-02-11 9:09 [PATCH v2] Cygwin: pty: Reduce unecessary input transfer Takashi Yano
@ 2021-02-12 9:43 ` Corinna Vinschen
2021-12-09 23:05 ` Johannes Schindelin
1 sibling, 0 replies; 9+ messages in thread
From: Corinna Vinschen @ 2021-02-12 9:43 UTC (permalink / raw)
To: cygwin-patches
On Feb 11 18:09, Takashi Yano via Cygwin-patches wrote:
> - Currently, input transfer is performed every time one line is read(),
> if the non-cygwin app is running in the background. With this patch,
> transfer is triggered by setpgid() rather than read() so that the
> unnecessary input transfer can be reduced much in that situation.
> ---
> winsup/cygwin/fhandler.h | 15 +-
> winsup/cygwin/fhandler_tty.cc | 377 ++++++++++++++++++++--------------
> winsup/cygwin/spawn.cc | 78 ++++---
> winsup/cygwin/tty.cc | 89 ++++++++
> winsup/cygwin/tty.h | 16 +-
> 5 files changed, 376 insertions(+), 199 deletions(-)
Pushed.
Thanks,
Corinna
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] Cygwin: pty: Reduce unecessary input transfer.
2021-02-11 9:09 [PATCH v2] Cygwin: pty: Reduce unecessary input transfer Takashi Yano
2021-02-12 9:43 ` Corinna Vinschen
@ 2021-12-09 23:05 ` Johannes Schindelin
2021-12-10 10:20 ` Takashi Yano
1 sibling, 1 reply; 9+ messages in thread
From: Johannes Schindelin @ 2021-12-09 23:05 UTC (permalink / raw)
To: Takashi Yano; +Cc: cygwin-patches
Hi Takashi,
sorry for responding to a patch you sent almost 10 months ago... but... I
am struggling with it.
First of all, let me describe the problem I am seeing (see also
https://github.com/git-for-windows/git/issues/3579): after upgrading the
MSYS2 runtime to v3.3.3 in Git for Windows, whenever I ask `git.exe` to
spawn `vim.exe` to edit any file, after quitting `vim` I see spurious ANSI
sequences being "ghost-typed" into the terminal (which is a MinTTY running
under `TERM=xterm`).
Apparently the ANSI sequences report the cursor position and the
foreground/background color in response to a CSI [ 6n sent from `vim`.
Clearly, those sequences should go to `vim.exe`, but they mostly don't
arrive there (but in MinTTY instead, as if I had typed them). Sometimes,
the foreground/background color seems to arrive in the `vim` process, but
the cursor position almost always does not. I suspect that it is important
that `git.exe` is a non-MSYS2 process whereas `vim.exe` is an MSYS2
process, and something inside the MSYS2 runtime is at fault.
I've bisected this incorrect behavior to the patch I am replying to.
I tried to trigger the same bug in pure Cygwin (as opposed to MSYS2),
specifically using `disable_pcon` (because MSYS2 defaults to not using the
pseudo console support because I ran into too many issues to be confident
enough in it yet), but I think that Cygwin's `vim` is too old and
therefore might not even send that CSI [ 6n (although `:h t_RV` _does_
show the expected help).
Now, the patch which I am responding to is completely obscure to me. It is
very, very unclear to me whether it really tries to only do one thing
(namely to transfer the input no longer in `read()` but in `setpgid()`),
or rather does many things at once. Even worse, I have not the faintest
clue how this patch is trying to accomplish what the commit message
describes (_because_ it does so many things at once), nor how that could
be related to the observed incorrect behavior, and as a consequence I have
no idea how I can hope to fix said observed incorrect behavior.
Could you help shed some light into the problem?
Thank you,
Johannes
On Thu, 11 Feb 2021, Takashi Yano wrote:
> - Currently, input transfer is performed every time one line is read(),
> if the non-cygwin app is running in the background. With this patch,
> transfer is triggered by setpgid() rather than read() so that the
> unnecessary input transfer can be reduced much in that situation.
> ---
> winsup/cygwin/fhandler.h | 15 +-
> winsup/cygwin/fhandler_tty.cc | 377 ++++++++++++++++++++--------------
> winsup/cygwin/spawn.cc | 78 ++++---
> winsup/cygwin/tty.cc | 89 ++++++++
> winsup/cygwin/tty.h | 16 +-
> 5 files changed, 376 insertions(+), 199 deletions(-)
>
> diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
> index 2fad7d33c..95011b6e3 100644
> --- a/winsup/cygwin/fhandler.h
> +++ b/winsup/cygwin/fhandler.h
> @@ -2244,13 +2244,13 @@ class fhandler_pty_common: public fhandler_termios
> public:
> fhandler_pty_common ()
> : fhandler_termios (),
> - output_mutex (NULL), input_mutex (NULL),
> + output_mutex (NULL), input_mutex (NULL), pcon_mutex (NULL),
> input_available_event (NULL)
> {
> pc.file_attributes (FILE_ATTRIBUTE_NORMAL);
> }
> static const unsigned pipesize = 128 * 1024;
> - HANDLE output_mutex, input_mutex;
> + HANDLE output_mutex, input_mutex, pcon_mutex;
> HANDLE input_available_event;
>
> bool use_archetype () const {return true;}
> @@ -2306,13 +2306,6 @@ class fhandler_pty_slave: public fhandler_pty_common
> void fch_close_handles ();
>
> public:
> - /* Transfer direction for transfer_input() */
> - enum xfer_dir
> - {
> - to_nat,
> - to_cyg
> - };
> -
> /* Constructor */
> fhandler_pty_slave (int);
>
> @@ -2371,8 +2364,8 @@ class fhandler_pty_slave: public fhandler_pty_common
> void setup_locale (void);
> tty *get_ttyp () { return (tty *) tc (); } /* Override as public */
> void create_invisible_console (void);
> - static void transfer_input (xfer_dir dir, HANDLE from, tty *ttyp,
> - _minor_t unit, HANDLE input_available_event);
> + static void transfer_input (tty::xfer_dir dir, HANDLE from, tty *ttyp,
> + HANDLE input_available_event);
> HANDLE get_input_available_event (void) { return input_available_event; }
> bool pcon_activated (void) { return get_ttyp ()->pcon_activated; }
> };
> diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
> index 48b89ae77..f6eb3ae4d 100644
> --- a/winsup/cygwin/fhandler_tty.cc
> +++ b/winsup/cygwin/fhandler_tty.cc
> @@ -127,7 +127,7 @@ set_switch_to_pcon (HANDLE *in, HANDLE *out, HANDLE *err, bool iscygwin)
> if (*err == cfd->get_output_handle () ||
> (fd == 2 && *err == GetStdHandle (STD_ERROR_HANDLE)))
> replace_err = (fhandler_base *) cfd;
> - if (cfd->get_major () == DEV_PTYS_MAJOR)
> + if (cfd->get_device () == (dev_t) myself->ctty)
> {
> fhandler_base *fh = cfd;
> fhandler_pty_slave *ptys = (fhandler_pty_slave *) fh;
> @@ -154,6 +154,7 @@ set_switch_to_pcon (HANDLE *in, HANDLE *out, HANDLE *err, bool iscygwin)
> /* CreateProcess() is hooked for GDB etc. */
> DEF_HOOK (CreateProcessA);
> DEF_HOOK (CreateProcessW);
> +DEF_HOOK (exit);
>
> static BOOL WINAPI
> CreateProcessA_Hooked
> @@ -268,6 +269,34 @@ CreateProcessW_Hooked
> return ret;
> }
>
> +void
> +exit_Hooked (int e)
> +{
> + if (isHybrid)
> + {
> + cygheap_fdenum cfd (false);
> + while (cfd.next () >= 0)
> + if (cfd->get_device () == (dev_t) myself->ctty)
> + {
> + fhandler_base *fh = cfd;
> + fhandler_pty_slave *ptys = (fhandler_pty_slave *) fh;
> + tty *ttyp = ptys->get_ttyp ();
> + HANDLE from = ptys->get_handle ();
> + HANDLE input_available_event = ptys->get_input_available_event ();
> + if (ttyp->getpgid () == myself->pgid
> + && ttyp->pcon_input_state_eq (tty::to_nat))
> + {
> + WaitForSingleObject (ptys->input_mutex, INFINITE);
> + fhandler_pty_slave::transfer_input (tty::to_cyg, from, ttyp,
> + input_available_event);
> + ReleaseMutex (ptys->input_mutex);
> + }
> + break;
> + }
> + }
> + exit_Orig (e);
> +}
> +
> static void
> convert_mb_str (UINT cp_to, char *ptr_to, size_t *len_to,
> UINT cp_from, const char *ptr_from, size_t len_from,
> @@ -438,7 +467,8 @@ fhandler_pty_master::accept_input ()
>
> HANDLE write_to = get_output_handle ();
> tmp_pathbuf tp;
> - if (to_be_read_from_pcon ())
> + if (to_be_read_from_pcon ()
> + && get_ttyp ()->pcon_input_state == tty::to_nat)
> {
> write_to = to_slave;
>
> @@ -487,11 +517,27 @@ fhandler_pty_master::accept_input ()
> }
> else
> {
> - DWORD rc;
> + BOOL rc = TRUE;
> DWORD written = 0;
>
> paranoid_printf ("about to write %u chars to slave", bytes_left);
> - rc = WriteFile (write_to, p, bytes_left, &written, NULL);
> + /* Write line by line for transfer input. */
> + char *p0 = p;
> + char *p1 = p;
> + DWORD n;
> + while ((p1 = (char *) memchr (p0, '\n', bytes_left - (p0 - p)))
> + || (p1 = (char *) memchr (p0, '\r', bytes_left - (p0 - p))))
> + {
> + n = p1 - p0 + 1;
> + rc = WriteFile (write_to, p0, n, &n, NULL);
> + written += n;
> + p0 = p1 + 1;
> + }
> + if ((n = bytes_left - (p0 - p)))
> + {
> + rc = WriteFile (write_to, p0, n, &n, NULL);
> + written += n;
> + }
> if (!rc)
> {
> debug_printf ("error writing to pipe %p %E", write_to);
> @@ -669,7 +715,7 @@ fhandler_pty_slave::open (int flags, mode_t)
> {
> &from_master_local, &input_available_event, &input_mutex, &inuse,
> &output_mutex, &to_master_local, &pty_owner, &to_master_cyg_local,
> - &from_master_cyg_local,
> + &from_master_cyg_local, &pcon_mutex,
> NULL
> };
>
> @@ -697,6 +743,11 @@ fhandler_pty_slave::open (int flags, mode_t)
> errmsg = "open input mutex failed, %E";
> goto err;
> }
> + if (!(pcon_mutex = get_ttyp ()->open_mutex (PCON_MUTEX, MAXIMUM_ALLOWED)))
> + {
> + errmsg = "open pcon mutex failed, %E";
> + goto err;
> + }
> shared_name (buf, INPUT_AVAILABLE_EVENT, get_minor ());
> if (!(input_available_event = OpenEvent (MAXIMUM_ALLOWED, TRUE, buf)))
> {
> @@ -960,12 +1011,19 @@ fhandler_pty_slave::set_switch_to_pcon (void)
> {
> isHybrid = true;
> setup_locale ();
> + myself->exec_dwProcessId = myself->dwProcessId;
> bool nopcon = (disable_pcon || !term_has_pcon_cap (NULL));
> - if (!setup_pseudoconsole (nopcon))
> - fhandler_pty_slave::transfer_input (fhandler_pty_slave::to_nat,
> - get_handle_cyg (),
> - get_ttyp (), get_minor (),
> - input_available_event);
> + WaitForSingleObject (pcon_mutex, INFINITE);
> + bool pcon_enabled = setup_pseudoconsole (nopcon);
> + ReleaseMutex (pcon_mutex);
> + if (!pcon_enabled && get_ttyp ()->getpgid () == myself->pgid
> + && get_ttyp ()->pcon_input_state_eq (tty::to_cyg))
> + {
> + WaitForSingleObject (input_mutex, INFINITE);
> + transfer_input (tty::to_nat, get_handle_cyg (), get_ttyp (),
> + input_available_event);
> + ReleaseMutex (input_mutex);
> + }
> }
> }
>
> @@ -985,19 +1043,24 @@ fhandler_pty_slave::reset_switch_to_pcon (void)
> h_gdb_process = NULL;
> if (isHybrid)
> {
> - if (get_ttyp ()->switch_to_pcon_in
> - && get_ttyp ()->pcon_pid == myself->pid)
> - fhandler_pty_slave::transfer_input (fhandler_pty_slave::to_cyg,
> - get_handle (),
> - get_ttyp (), get_minor (),
> - input_available_event);
> - if (get_ttyp ()->master_is_running_as_service)
> + if (get_ttyp ()->getpgid () == myself->pgid
> + && get_ttyp ()->pcon_input_state_eq (tty::to_nat))
> + {
> + WaitForSingleObject (input_mutex, INFINITE);
> + transfer_input (tty::to_cyg, get_handle (), get_ttyp (),
> + input_available_event);
> + ReleaseMutex (input_mutex);
> + }
> + if (get_ttyp ()->master_is_running_as_service
> + && get_ttyp ()->pcon_activated)
> /* If the master is running as service, re-attaching to
> the console of the parent process will fail.
> Therefore, never close pseudo console here. */
> return;
> bool need_restore_handles = get_ttyp ()->pcon_activated;
> + WaitForSingleObject (pcon_mutex, INFINITE);
> close_pseudoconsole (get_ttyp ());
> + ReleaseMutex (pcon_mutex);
> if (need_restore_handles)
> {
> pinfo p (get_ttyp ()->master_pid);
> @@ -1047,6 +1110,7 @@ fhandler_pty_slave::reset_switch_to_pcon (void)
> if (fix_err)
> SetStdHandle (STD_ERROR_HANDLE, get_output_handle ());
> }
> + myself->exec_dwProcessId = 0;
> isHybrid = false;
> }
> }
> @@ -1057,11 +1121,6 @@ fhandler_pty_slave::reset_switch_to_pcon (void)
> return;
> if (isHybrid)
> return;
> - if (get_ttyp ()->switch_to_pcon_in && !get_ttyp ()->pcon_activated)
> - fhandler_pty_slave::transfer_input (fhandler_pty_slave::to_cyg,
> - get_handle (),
> - get_ttyp (), get_minor (),
> - input_available_event);
> get_ttyp ()->pcon_pid = 0;
> get_ttyp ()->switch_to_pcon_in = false;
> get_ttyp ()->pcon_activated = false;
> @@ -1119,77 +1178,47 @@ fhandler_pty_slave::mask_switch_to_pcon_in (bool mask, bool xfer)
> else if (InterlockedDecrement (&num_reader) == 0)
> CloseHandle (slave_reading);
>
> - if (get_ttyp ()->switch_to_pcon_in && !!masked != mask && xfer)
> - { /* Transfer input */
> - bool attach_restore = false;
> - DWORD pcon_winpid = 0;
> - if (get_ttyp ()->pcon_pid)
> - {
> - pinfo p (get_ttyp ()->pcon_pid);
> - if (p)
> - pcon_winpid = p->exec_dwProcessId ?: p->dwProcessId;
> - }
> - if (mask)
> - {
> - HANDLE from = get_handle ();
> - if (get_ttyp ()->pcon_activated && pcon_winpid
> - && !get_console_process_id (pcon_winpid, true))
> - {
> - HANDLE pcon_owner =
> - OpenProcess (PROCESS_DUP_HANDLE, FALSE, pcon_winpid);
> - DuplicateHandle (pcon_owner, get_ttyp ()->h_pcon_in,
> - GetCurrentProcess (), &from,
> - 0, TRUE, DUPLICATE_SAME_ACCESS);
> - CloseHandle (pcon_owner);
> - FreeConsole ();
> - AttachConsole (pcon_winpid);
> - attach_restore = true;
> - }
> - fhandler_pty_slave::transfer_input (fhandler_pty_slave::to_cyg,
> - from,
> - get_ttyp (), get_minor (),
> - input_available_event);
> - }
> - else
> + /* In GDB, transfer input based on setpgid() does not work because
> + GDB may not set terminal process group properly. Therefore,
> + transfer input here if isHybrid is set. */
> + if (get_ttyp ()->switch_to_pcon_in && !!masked != mask && xfer && isHybrid)
> + {
> + if (mask && get_ttyp ()->pcon_input_state_eq (tty::to_nat))
> {
> - if (get_ttyp ()->pcon_activated && pcon_winpid
> - && !get_console_process_id (pcon_winpid, true))
> - {
> - FreeConsole ();
> - AttachConsole (pcon_winpid);
> - attach_restore = true;
> - }
> - fhandler_pty_slave::transfer_input (fhandler_pty_slave::to_nat,
> - get_handle_cyg (),
> - get_ttyp (), get_minor (),
> - input_available_event);
> + WaitForSingleObject (input_mutex, INFINITE);
> + transfer_input (tty::to_cyg, get_handle (), get_ttyp (),
> + input_available_event);
> + ReleaseMutex (input_mutex);
> }
> - if (attach_restore)
> + else if (!mask && get_ttyp ()->pcon_input_state_eq (tty::to_cyg))
> {
> - FreeConsole ();
> - pinfo p (myself->ppid);
> - if (p)
> - {
> - if (!AttachConsole (p->dwProcessId))
> - AttachConsole (ATTACH_PARENT_PROCESS);
> - }
> - else
> - AttachConsole (ATTACH_PARENT_PROCESS);
> + WaitForSingleObject (input_mutex, INFINITE);
> + transfer_input (tty::to_nat, get_handle_cyg (), get_ttyp (),
> + input_available_event);
> + ReleaseMutex (input_mutex);
> }
> }
> - return;
> }
>
> bool
> fhandler_pty_master::to_be_read_from_pcon (void)
> {
> + if (!get_ttyp ()->switch_to_pcon_in)
> + return false;
> +
> char name[MAX_PATH];
> shared_name (name, TTY_SLAVE_READING, get_minor ());
> HANDLE masked = OpenEvent (READ_CONTROL, FALSE, name);
> CloseHandle (masked);
>
> - return get_ttyp ()->pcon_start
> - || (get_ttyp ()->switch_to_pcon_in && !masked);
> + if (masked) /* The foreground process is cygwin process */
> + return false;
> +
> + if (!pinfo (get_ttyp ()->getpgid ()))
> + /* GDB may set invalid process group for non-cygwin process. */
> + return true;
> +
> + return get_ttyp ()->pcon_fg (get_ttyp ()->getpgid ());
> }
>
> void __reg3
> @@ -1720,6 +1749,7 @@ fhandler_pty_slave::fch_open_handles (bool chown)
> TRUE, buf);
> output_mutex = get_ttyp ()->open_output_mutex (write_access);
> input_mutex = get_ttyp ()->open_input_mutex (write_access);
> + pcon_mutex = get_ttyp ()->open_mutex (PCON_MUTEX, write_access);
> inuse = get_ttyp ()->open_inuse (write_access);
> if (!input_available_event || !output_mutex || !input_mutex || !inuse)
> {
> @@ -1873,6 +1903,8 @@ fhandler_pty_common::close ()
> get_minor (), get_handle (), get_output_handle ());
> if (!ForceCloseHandle (input_mutex))
> termios_printf ("CloseHandle (input_mutex<%p>), %E", input_mutex);
> + if (!ForceCloseHandle (pcon_mutex))
> + termios_printf ("CloseHandle (pcon_mutex<%p>), %E", pcon_mutex);
> if (!ForceCloseHandle1 (get_handle (), from_pty))
> termios_printf ("CloseHandle (get_handle ()<%p>), %E", get_handle ());
> if (!ForceCloseHandle1 (get_output_handle (), to_pty))
> @@ -2011,72 +2043,103 @@ fhandler_pty_master::write (const void *ptr, size_t len)
>
> push_process_state process_state (PID_TTYOU);
>
> - /* Write terminal input to to_slave pipe instead of output_handle
> - if current application is native console application. */
> - if (to_be_read_from_pcon () && get_ttyp ()->pcon_activated)
> + if (get_ttyp ()->pcon_start)
> {
> - tmp_pathbuf tp;
> - char *buf = (char *) ptr;
> - size_t nlen = len;
> - if (get_ttyp ()->term_code_page != CP_UTF8)
> - {
> - static mbstate_t mbp;
> - buf = tp.c_get ();
> - nlen = NT_MAX_PATH;
> - convert_mb_str (CP_UTF8, buf, &nlen,
> - get_ttyp ()->term_code_page, (const char *) ptr, len,
> - &mbp);
> - }
> + /* Pseudo condole support uses "CSI6n" to get cursor position.
> + If the reply for "CSI6n" is divided into multiple writes,
> + pseudo console sometimes does not recognize it. Therefore,
> + put them together into wpbuf and write all at once. */
> + static const int wpbuf_len = strlen ("\033[32768;32868R");
> + static char wpbuf[wpbuf_len];
> + static int ixput = 0;
> + static int state = 0;
>
> + DWORD n;
> WaitForSingleObject (input_mutex, INFINITE);
> -
> - DWORD wLen;
> -
> - if (get_ttyp ()->pcon_start)
> + for (size_t i = 0; i < len; i++)
> {
> - /* Pseudo condole support uses "CSI6n" to get cursor position.
> - If the reply for "CSI6n" is divided into multiple writes,
> - pseudo console sometimes does not recognize it. Therefore,
> - put them together into wpbuf and write all at once. */
> - static const int wpbuf_len = 64;
> - static char wpbuf[wpbuf_len];
> - static int ixput = 0;
> -
> - if (ixput + nlen < wpbuf_len)
> + if (p[i] == '\033')
> {
> - memcpy (wpbuf + ixput, buf, nlen);
> - ixput += nlen;
> - }
> - else
> - {
> - WriteFile (to_slave, wpbuf, ixput, &wLen, NULL);
> + if (ixput)
> + line_edit (wpbuf, ixput, ti, &ret);
> ixput = 0;
> - get_ttyp ()->pcon_start = false;
> - WriteFile (to_slave, buf, nlen, &wLen, NULL);
> + state = 1;
> }
> - if (ixput && memchr (wpbuf, 'R', ixput))
> + if (state == 1)
> {
> - WriteFile (to_slave, wpbuf, ixput, &wLen, NULL);
> - ixput = 0;
> - get_ttyp ()->pcon_start = false;
> + if (ixput < wpbuf_len)
> + wpbuf[ixput++] = p[i];
> + else
> + {
> + if (!get_ttyp ()->req_xfer_input)
> + WriteFile (to_slave, wpbuf, ixput, &n, NULL);
> + ixput = 0;
> + wpbuf[ixput++] = p[i];
> + }
> }
> - ReleaseMutex (input_mutex);
> - if (get_ttyp ()->switch_to_pcon_in)
> + else
> + line_edit (p + i, 1, ti, &ret);
> + if (state == 1 && p[i] == 'R')
> + state = 2;
> + }
> + if (state == 2)
> + {
> + if (!get_ttyp ()->req_xfer_input)
> + WriteFile (to_slave, wpbuf, ixput, &n, NULL);
> + ixput = 0;
> + state = 0;
> + get_ttyp ()->req_xfer_input = false;
> + get_ttyp ()->pcon_start = false;
> + }
> + ReleaseMutex (input_mutex);
> +
> + if (!get_ttyp ()->pcon_start)
> + {
> + pinfo pp (get_ttyp ()->pcon_start_pid);
> + bool pcon_fg = (pp && get_ttyp ()->getpgid () == pp->pgid);
> + /* GDB may set WINPID rather than cygwin PID to process group
> + when the debugged process is a non-cygwin process.*/
> + pcon_fg |= !pinfo (get_ttyp ()->getpgid ());
> + if (get_ttyp ()->switch_to_pcon_in && pcon_fg
> + && get_ttyp ()->pcon_input_state_eq (tty::to_cyg))
> {
> - fhandler_pty_slave::transfer_input (fhandler_pty_slave::to_nat,
> - from_master_cyg,
> - get_ttyp (), get_minor (),
> - input_available_event);
> /* This accept_input() call is needed in order to transfer input
> which is not accepted yet to non-cygwin pipe. */
> if (get_readahead_valid ())
> accept_input ();
> + WaitForSingleObject (input_mutex, INFINITE);
> + fhandler_pty_slave::transfer_input (tty::to_nat, from_master_cyg,
> + get_ttyp (),
> + input_available_event);
> + ReleaseMutex (input_mutex);
> }
> - return len;
> + get_ttyp ()->pcon_start_pid = 0;
> }
>
> - WriteFile (to_slave, buf, nlen, &wLen, NULL);
> + return len;
> + }
> +
> + /* Write terminal input to to_slave pipe instead of output_handle
> + if current application is native console application. */
> + if (to_be_read_from_pcon () && get_ttyp ()->pcon_activated
> + && get_ttyp ()->pcon_input_state == tty::to_nat)
> + {
> + tmp_pathbuf tp;
> + char *buf = (char *) ptr;
> + size_t nlen = len;
> + if (get_ttyp ()->term_code_page != CP_UTF8)
> + {
> + static mbstate_t mbp;
> + buf = tp.c_get ();
> + nlen = NT_MAX_PATH;
> + convert_mb_str (CP_UTF8, buf, &nlen,
> + get_ttyp ()->term_code_page, (const char *) ptr, len,
> + &mbp);
> + }
>
> + WaitForSingleObject (input_mutex, INFINITE);
> + DWORD n;
> + WriteFile (to_slave, buf, nlen, &n, NULL);
> ReleaseMutex (input_mutex);
>
> return len;
> @@ -2248,6 +2311,8 @@ fhandler_pty_slave::fixup_after_exec ()
> /* CreateProcess() is hooked for GDB etc. */
> DO_HOOK (NULL, CreateProcessA);
> DO_HOOK (NULL, CreateProcessW);
> + if (CreateProcessA_Orig || CreateProcessW_Orig)
> + DO_HOOK (NULL, exit);
> }
>
> /* This thread function handles the master control pipe. It waits for a
> @@ -2739,6 +2804,10 @@ fhandler_pty_master::setup ()
> if (!(input_mutex = CreateMutex (&sa, FALSE, buf)))
> goto err;
>
> + errstr = shared_name (buf, PCON_MUTEX, unit);
> + if (!(pcon_mutex = CreateMutex (&sa, FALSE, buf)))
> + goto err;
> +
> attach_mutex = CreateMutex (&sa, FALSE, NULL);
>
> /* Create master control pipe which allows the master to duplicate
> @@ -2980,10 +3049,17 @@ fhandler_pty_slave::setup_pseudoconsole (bool nopcon)
> }
>
> HANDLE hpConIn, hpConOut;
> - acquire_output_mutex (INFINITE);
> if (get_ttyp ()->pcon_pid && get_ttyp ()->pcon_pid != myself->pid
> && !!pinfo (get_ttyp ()->pcon_pid) && get_ttyp ()->pcon_activated)
> {
> + /* Send CSI6n just for requesting transfer input. */
> + DWORD n;
> + WaitForSingleObject (input_mutex, INFINITE);
> + get_ttyp ()->req_xfer_input = true;
> + get_ttyp ()->pcon_start = true;
> + get_ttyp ()->pcon_start_pid = myself->pid;
> + WriteFile (get_output_handle_cyg (), "\033[6n", 4, &n, NULL);
> + ReleaseMutex (input_mutex);
> /* Attach to the pseudo console which already exits. */
> pinfo p (get_ttyp ()->pcon_pid);
> HANDLE pcon_owner =
> @@ -3068,8 +3144,9 @@ fhandler_pty_slave::setup_pseudoconsole (bool nopcon)
> si.StartupInfo.hStdOutput = NULL;
> si.StartupInfo.hStdError = NULL;
>
> - get_ttyp ()->pcon_start = true;
> get_ttyp ()->pcon_activated = true;
> + get_ttyp ()->pcon_start = true;
> + get_ttyp ()->pcon_start_pid = myself->pid;
> if (!CreateProcessW (NULL, cmd, &sec_none, &sec_none,
> TRUE, EXTENDED_STARTUPINFO_PRESENT,
> NULL, NULL, &si.StartupInfo, &pi))
> @@ -3183,7 +3260,6 @@ skip_create:
> if (get_ttyp ()->previous_output_code_page)
> SetConsoleOutputCP (get_ttyp ()->previous_output_code_page);
>
> - release_output_mutex ();
> return true;
>
> cleanup_pcon_in:
> @@ -3196,6 +3272,7 @@ cleanup_helper_process:
> cleanup_event_and_pipes:
> CloseHandle (hello);
> get_ttyp ()->pcon_start = false;
> + get_ttyp ()->pcon_start_pid = 0;
> get_ttyp ()->pcon_activated = false;
> skip_close_hello:
> CloseHandle (goodbye);
> @@ -3212,7 +3289,6 @@ cleanup_pseudo_console:
> CloseHandle (tmp);
> }
> fallback:
> - release_output_mutex ();
> return false;
> }
>
> @@ -3312,6 +3388,7 @@ fhandler_pty_slave::close_pseudoconsole (tty *ttyp)
> ttyp->switch_to_pcon_in = false;
> ttyp->pcon_pid = 0;
> ttyp->pcon_start = false;
> + ttyp->pcon_start_pid = 0;
> }
> }
> else
> @@ -3429,7 +3506,6 @@ fhandler_pty_slave::term_has_pcon_cap (const WCHAR *env)
> /* Set pcon_activated and pcon_start so that the response
> will sent to io_handle rather than io_handle_cyg. */
> get_ttyp ()->pcon_activated = true;
> - get_ttyp ()->pcon_pid = myself->pid;
> /* pcon_start will be cleared in master write() when CSI6n is responded. */
> get_ttyp ()->pcon_start = true;
> WriteFile (get_output_handle_cyg (), "\033[6n", 4, &n, NULL);
> @@ -3526,11 +3602,11 @@ fhandler_pty_master::get_master_fwd_thread_param (master_fwd_thread_param_t *p)
> #define ALT_PRESSED (LEFT_ALT_PRESSED | RIGHT_ALT_PRESSED)
> #define CTRL_PRESSED (LEFT_CTRL_PRESSED | RIGHT_CTRL_PRESSED)
> void
> -fhandler_pty_slave::transfer_input (xfer_dir dir, HANDLE from, tty *ttyp,
> - _minor_t unit, HANDLE input_available_event)
> +fhandler_pty_slave::transfer_input (tty::xfer_dir dir, HANDLE from, tty *ttyp,
> + HANDLE input_available_event)
> {
> HANDLE to;
> - if (dir == to_nat)
> + if (dir == tty::to_nat)
> to = ttyp->to_slave ();
> else
> to = ttyp->to_slave_cyg ();
> @@ -3548,14 +3624,14 @@ fhandler_pty_slave::transfer_input (xfer_dir dir, HANDLE from, tty *ttyp,
> char pipe[MAX_PATH];
> __small_sprintf (pipe,
> "\\\\.\\pipe\\cygwin-%S-pty%d-master-ctl",
> - &cygheap->installation_key, unit);
> + &cygheap->installation_key, ttyp->get_minor ());
> pipe_request req = { GetCurrentProcessId () };
> pipe_reply repl;
> DWORD len;
> if (!CallNamedPipe (pipe, &req, sizeof req,
> &repl, sizeof repl, &len, 500))
> return; /* What can we do? */
> - if (dir == to_nat)
> + if (dir == tty::to_nat)
> to = repl.to_slave;
> else
> to = repl.to_slave_cyg;
> @@ -3563,7 +3639,7 @@ fhandler_pty_slave::transfer_input (xfer_dir dir, HANDLE from, tty *ttyp,
>
> UINT cp_from = 0, cp_to = 0;
>
> - if (dir == to_nat)
> + if (dir == tty::to_nat)
> {
> cp_from = ttyp->term_code_page;
> if (ttyp->pcon_activated)
> @@ -3582,7 +3658,7 @@ fhandler_pty_slave::transfer_input (xfer_dir dir, HANDLE from, tty *ttyp,
>
> bool transfered = false;
>
> - if (dir == to_cyg && ttyp->pcon_activated)
> + if (dir == tty::to_cyg && ttyp->pcon_activated)
> { /* from handle is console handle */
> INPUT_RECORD r[INREC_SIZE];
> DWORD n;
> @@ -3607,18 +3683,6 @@ fhandler_pty_slave::transfer_input (xfer_dir dir, HANDLE from, tty *ttyp,
> && (ctrl_key_state & CTRL_PRESSED)
> && !(ctrl_key_state & ALT_PRESSED))
> buf[len++] = '\0';
> - else if (r[i].Event.KeyEvent.wVirtualKeyCode == VK_F3)
> - {
> - /* If the cursor position report for CSI6n matches
> - with e.g. "ESC[1;2R", pseudo console translates
> - it to Shift-F3. This is a workaround for that. */
> - int ctrl = 1;
> - if (ctrl_key_state & SHIFT_PRESSED) ctrl += 1;
> - if (ctrl_key_state & ALT_PRESSED) ctrl += 2;
> - if (ctrl_key_state & CTRL_PRESSED) ctrl += 4;
> - __small_sprintf (buf + len, "\033[1;%1dR", ctrl);
> - len += 6;
> - }
> else
> { /* arrow/function keys */
> /* FIXME: The current code generates cygwin terminal
> @@ -3666,11 +3730,15 @@ fhandler_pty_slave::transfer_input (xfer_dir dir, HANDLE from, tty *ttyp,
> DWORD n = MIN (bytes_in_pipe, NT_MAX_PATH);
> ReadFile (from, buf, n, &n, NULL);
> char *ptr = buf;
> - if (dir == to_nat && ttyp->pcon_activated)
> + if (dir == tty::to_nat)
> {
> char *p = buf;
> - while ((p = (char *) memchr (p, '\n', n - (p - buf))))
> - *p = '\r';
> + if (ttyp->pcon_activated)
> + while ((p = (char *) memchr (p, '\n', n - (p - buf))))
> + *p = '\r';
> + else
> + while ((p = (char *) memchr (p, '\r', n - (p - buf))))
> + *p = '\n';
> }
> if (cp_to != cp_from)
> {
> @@ -3686,8 +3754,9 @@ fhandler_pty_slave::transfer_input (xfer_dir dir, HANDLE from, tty *ttyp,
> }
> }
>
> - if (dir == to_nat)
> + if (dir == tty::to_nat)
> ResetEvent (input_available_event);
> else if (transfered)
> SetEvent (input_available_event);
> + ttyp->pcon_input_state = dir;
> }
> diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
> index c4b612815..4d4d599ca 100644
> --- a/winsup/cygwin/spawn.cc
> +++ b/winsup/cygwin/spawn.cc
> @@ -656,18 +656,19 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
> bool enable_pcon = false;
> HANDLE ptys_from_master = NULL;
> HANDLE ptys_input_available_event = NULL;
> - HANDLE ptys_output_mutex = NULL;
> + HANDLE ptys_pcon_mutex = NULL;
> + HANDLE ptys_input_mutex = NULL;
> tty *ptys_ttyp = NULL;
> - _minor_t ptys_unit = 0;
> if (!iscygwin () && ptys_primary && is_console_app (runpath))
> {
> bool nopcon = mode != _P_OVERLAY && mode != _P_WAIT;
> if (disable_pcon || !ptys_primary->term_has_pcon_cap (envblock))
> nopcon = true;
> + ptys_ttyp = ptys_primary->get_ttyp ();
> + WaitForSingleObject (ptys_primary->pcon_mutex, INFINITE);
> if (ptys_primary->setup_pseudoconsole (nopcon))
> enable_pcon = true;
> - ptys_ttyp = ptys_primary->get_ttyp ();
> - ptys_unit = ptys_primary->get_minor ();
> + ReleaseMutex (ptys_primary->pcon_mutex);
> ptys_from_master = ptys_primary->get_handle ();
> DuplicateHandle (GetCurrentProcess (), ptys_from_master,
> GetCurrentProcess (), &ptys_from_master,
> @@ -677,14 +678,21 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
> DuplicateHandle (GetCurrentProcess (), ptys_input_available_event,
> GetCurrentProcess (), &ptys_input_available_event,
> 0, 0, DUPLICATE_SAME_ACCESS);
> - DuplicateHandle (GetCurrentProcess (), ptys_primary->output_mutex,
> - GetCurrentProcess (), &ptys_output_mutex,
> + DuplicateHandle (GetCurrentProcess (), ptys_primary->pcon_mutex,
> + GetCurrentProcess (), &ptys_pcon_mutex,
> + 0, 0, DUPLICATE_SAME_ACCESS);
> + DuplicateHandle (GetCurrentProcess (), ptys_primary->input_mutex,
> + GetCurrentProcess (), &ptys_input_mutex,
> 0, 0, DUPLICATE_SAME_ACCESS);
> - if (!enable_pcon)
> - fhandler_pty_slave::transfer_input (fhandler_pty_slave::to_nat,
> - ptys_primary->get_handle_cyg (),
> - ptys_ttyp, ptys_unit,
> - ptys_input_available_event);
> + if (!enable_pcon && ptys_ttyp->getpgid () == myself->pgid
> + && ptys_ttyp->pcon_input_state_eq (tty::to_cyg))
> + {
> + WaitForSingleObject (ptys_input_mutex, INFINITE);
> + fhandler_pty_slave::transfer_input (tty::to_nat,
> + ptys_primary->get_handle_cyg (),
> + ptys_ttyp, ptys_input_available_event);
> + ReleaseMutex (ptys_input_mutex);
> + }
> }
>
> /* Set up needed handles for stdio */
> @@ -969,20 +977,22 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
> if (ptys_ttyp)
> {
> ptys_ttyp->wait_pcon_fwd ();
> - /* Do not transfer input if another process using pseudo
> - console exists. */
> - WaitForSingleObject (ptys_output_mutex, INFINITE);
> - if (!fhandler_pty_common::get_console_process_id
> - (myself->exec_dwProcessId, false, true, true))
> - fhandler_pty_slave::transfer_input (fhandler_pty_slave::to_cyg,
> - ptys_from_master,
> - ptys_ttyp, ptys_unit,
> - ptys_input_available_event);
> + if (ptys_ttyp->getpgid () == myself->pgid
> + && ptys_ttyp->pcon_input_state_eq (tty::to_nat))
> + {
> + WaitForSingleObject (ptys_input_mutex, INFINITE);
> + fhandler_pty_slave::transfer_input (tty::to_cyg,
> + ptys_from_master, ptys_ttyp,
> + ptys_input_available_event);
> + ReleaseMutex (ptys_input_mutex);
> + }
> CloseHandle (ptys_from_master);
> + CloseHandle (ptys_input_mutex);
> CloseHandle (ptys_input_available_event);
> + WaitForSingleObject (ptys_pcon_mutex, INFINITE);
> fhandler_pty_slave::close_pseudoconsole (ptys_ttyp);
> - ReleaseMutex (ptys_output_mutex);
> - CloseHandle (ptys_output_mutex);
> + ReleaseMutex (ptys_pcon_mutex);
> + CloseHandle (ptys_pcon_mutex);
> }
> if (cons_native)
> {
> @@ -1002,20 +1012,22 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
> if (ptys_ttyp)
> {
> ptys_ttyp->wait_pcon_fwd ();
> - /* Do not transfer input if another process using pseudo
> - console exists. */
> - WaitForSingleObject (ptys_output_mutex, INFINITE);
> - if (!fhandler_pty_common::get_console_process_id
> - (myself->exec_dwProcessId, false, true, true))
> - fhandler_pty_slave::transfer_input (fhandler_pty_slave::to_cyg,
> - ptys_from_master,
> - ptys_ttyp, ptys_unit,
> - ptys_input_available_event);
> + if (ptys_ttyp->getpgid () == myself->pgid
> + && ptys_ttyp->pcon_input_state_eq (tty::to_nat))
> + {
> + WaitForSingleObject (ptys_input_mutex, INFINITE);
> + fhandler_pty_slave::transfer_input (tty::to_cyg,
> + ptys_from_master, ptys_ttyp,
> + ptys_input_available_event);
> + ReleaseMutex (ptys_input_mutex);
> + }
> CloseHandle (ptys_from_master);
> + CloseHandle (ptys_input_mutex);
> CloseHandle (ptys_input_available_event);
> + WaitForSingleObject (ptys_pcon_mutex, INFINITE);
> fhandler_pty_slave::close_pseudoconsole (ptys_ttyp);
> - ReleaseMutex (ptys_output_mutex);
> - CloseHandle (ptys_output_mutex);
> + ReleaseMutex (ptys_pcon_mutex);
> + CloseHandle (ptys_pcon_mutex);
> }
> if (cons_native)
> {
> diff --git a/winsup/cygwin/tty.cc b/winsup/cygwin/tty.cc
> index fb4501c1b..41f81f694 100644
> --- a/winsup/cygwin/tty.cc
> +++ b/winsup/cygwin/tty.cc
> @@ -241,6 +241,7 @@ tty::init ()
> term_code_page = 0;
> pcon_last_time = 0;
> pcon_start = false;
> + pcon_start_pid = 0;
> pcon_cap_checked = false;
> has_csi6n = false;
> need_invisible_console = false;
> @@ -248,6 +249,8 @@ tty::init ()
> previous_code_page = 0;
> previous_output_code_page = 0;
> master_is_running_as_service = false;
> + req_xfer_input = false;
> + pcon_input_state = to_cyg;
> }
>
> HANDLE
> @@ -293,6 +296,77 @@ tty_min::ttyname ()
> return d.name ();
> }
>
> +void
> +tty_min::setpgid (int pid)
> +{
> + fhandler_pty_slave *ptys = NULL;
> + cygheap_fdenum cfd (false);
> + while (cfd.next () >= 0 && ptys == NULL)
> + if (cfd->get_device () == getntty ())
> + ptys = (fhandler_pty_slave *) (fhandler_base *) cfd;
> +
> + if (ptys)
> + {
> + tty *ttyp = ptys->get_ttyp ();
> + WaitForSingleObject (ptys->pcon_mutex, INFINITE);
> + bool was_pcon_fg = ttyp->pcon_fg (pgid);
> + bool pcon_fg = ttyp->pcon_fg (pid);
> + if (!was_pcon_fg && pcon_fg && ttyp->switch_to_pcon_in
> + && ttyp->pcon_input_state_eq (tty::to_cyg))
> + {
> + WaitForSingleObject (ptys->input_mutex, INFINITE);
> + fhandler_pty_slave::transfer_input (tty::to_nat,
> + ptys->get_handle_cyg (), ttyp,
> + ptys->get_input_available_event ());
> + ReleaseMutex (ptys->input_mutex);
> + }
> + else if (was_pcon_fg && !pcon_fg && ttyp->switch_to_pcon_in
> + && ttyp->pcon_input_state_eq (tty::to_nat))
> + {
> + bool attach_restore = false;
> + DWORD pcon_winpid = 0;
> + if (ttyp->pcon_pid)
> + {
> + pinfo p (ttyp->pcon_pid);
> + if (p)
> + pcon_winpid = p->exec_dwProcessId ?: p->dwProcessId;
> + }
> + HANDLE from = ptys->get_handle ();
> + if (ttyp->pcon_activated && pcon_winpid
> + && !ptys->get_console_process_id (pcon_winpid, true))
> + {
> + HANDLE pcon_owner =
> + OpenProcess (PROCESS_DUP_HANDLE, FALSE, pcon_winpid);
> + DuplicateHandle (pcon_owner, ttyp->h_pcon_in,
> + GetCurrentProcess (), &from,
> + 0, TRUE, DUPLICATE_SAME_ACCESS);
> + CloseHandle (pcon_owner);
> + FreeConsole ();
> + AttachConsole (pcon_winpid);
> + attach_restore = true;
> + }
> + WaitForSingleObject (ptys->input_mutex, INFINITE);
> + fhandler_pty_slave::transfer_input (tty::to_cyg, from, ttyp,
> + ptys->get_input_available_event ());
> + ReleaseMutex (ptys->input_mutex);
> + if (attach_restore)
> + {
> + FreeConsole ();
> + pinfo p (myself->ppid);
> + if (p)
> + {
> + if (!AttachConsole (p->dwProcessId))
> + AttachConsole (ATTACH_PARENT_PROCESS);
> + }
> + else
> + AttachConsole (ATTACH_PARENT_PROCESS);
> + }
> + }
> + ReleaseMutex (ptys->pcon_mutex);
> + }
> + pgid = pid;
> +}
> +
> void
> tty::wait_pcon_fwd (bool init)
> {
> @@ -312,3 +386,18 @@ tty::wait_pcon_fwd (bool init)
> cygwait (tw);
> }
> }
> +
> +bool
> +tty::pcon_fg (pid_t pgid)
> +{
> + /* Check if the terminal pgid matches with the pgid of the
> + non-cygwin process. */
> + winpids pids ((DWORD) 0);
> + for (unsigned i = 0; i < pids.npids; i++)
> + {
> + _pinfo *p = pids[i];
> + if (p->ctty == ntty && p->pgid == pgid && p->exec_dwProcessId)
> + return true;
> + }
> + return false;
> +}
> diff --git a/winsup/cygwin/tty.h b/winsup/cygwin/tty.h
> index e2e6dfeb6..e1de7ab46 100644
> --- a/winsup/cygwin/tty.h
> +++ b/winsup/cygwin/tty.h
> @@ -20,6 +20,7 @@ details. */
> #define INPUT_AVAILABLE_EVENT "cygtty.input.avail"
> #define OUTPUT_MUTEX "cygtty.output.mutex"
> #define INPUT_MUTEX "cygtty.input.mutex"
> +#define PCON_MUTEX "cygtty.pcon.mutex"
> #define TTY_SLAVE_ALIVE "cygtty.slave_alive"
> #define TTY_SLAVE_READING "cygtty.slave_reading"
>
> @@ -72,7 +73,7 @@ public:
> dev_t getntty () const {return ntty;}
> _minor_t get_minor () const {return device::minor (ntty);}
> pid_t getpgid () const {return pgid;}
> - void setpgid (int pid) {pgid = pid;}
> + void setpgid (int pid);
> int getsid () const {return sid;}
> void setsid (pid_t tsid) {sid = tsid;}
> void kill_pgrp (int);
> @@ -89,6 +90,13 @@ class tty: public tty_min
> public:
> pid_t master_pid; /* PID of tty master process */
>
> + /* Transfer direction for fhandler_pty_slave::transfer_input() */
> + enum xfer_dir
> + {
> + to_cyg,
> + to_nat
> + };
> +
> private:
> HANDLE _from_master;
> HANDLE _from_master_cyg;
> @@ -98,6 +106,7 @@ private:
> HANDLE _to_slave_cyg;
> bool pcon_activated;
> bool pcon_start;
> + pid_t pcon_start_pid;
> bool switch_to_pcon_in;
> pid_t pcon_pid;
> UINT term_code_page;
> @@ -114,6 +123,8 @@ private:
> UINT previous_code_page;
> UINT previous_output_code_page;
> bool master_is_running_as_service;
> + bool req_xfer_input;
> + xfer_dir pcon_input_state;
>
> public:
> HANDLE from_master () const { return _from_master; }
> @@ -148,9 +159,12 @@ public:
> static void __stdcall create_master (int);
> static void __stdcall init_session ();
> void wait_pcon_fwd (bool init = true);
> + bool pcon_input_state_eq (xfer_dir x) { return pcon_input_state == x; }
> + bool pcon_fg (pid_t pgid);
> friend class fhandler_pty_common;
> friend class fhandler_pty_master;
> friend class fhandler_pty_slave;
> + friend class tty_min;
> };
>
> class tty_list
> --
> 2.30.0
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] Cygwin: pty: Reduce unecessary input transfer.
2021-12-09 23:05 ` Johannes Schindelin
@ 2021-12-10 10:20 ` Takashi Yano
2021-12-10 11:12 ` Johannes Schindelin
0 siblings, 1 reply; 9+ messages in thread
From: Takashi Yano @ 2021-12-10 10:20 UTC (permalink / raw)
To: cygwin-patches
On Fri, 10 Dec 2021 00:05:27 +0100 (CET)
Johannes Schindelin wrote:
> sorry for responding to a patch you sent almost 10 months ago... but... I
> am struggling with it.
>
> First of all, let me describe the problem I am seeing (see also
> https://github.com/git-for-windows/git/issues/3579): after upgrading the
> MSYS2 runtime to v3.3.3 in Git for Windows, whenever I ask `git.exe` to
> spawn `vim.exe` to edit any file, after quitting `vim` I see spurious ANSI
> sequences being "ghost-typed" into the terminal (which is a MinTTY running
> under `TERM=xterm`).
>
> Apparently the ANSI sequences report the cursor position and the
> foreground/background color in response to a CSI [ 6n sent from `vim`.
>
> Clearly, those sequences should go to `vim.exe`, but they mostly don't
> arrive there (but in MinTTY instead, as if I had typed them). Sometimes,
> the foreground/background color seems to arrive in the `vim` process, but
> the cursor position almost always does not. I suspect that it is important
> that `git.exe` is a non-MSYS2 process whereas `vim.exe` is an MSYS2
> process, and something inside the MSYS2 runtime is at fault.
>
> I've bisected this incorrect behavior to the patch I am replying to.
>
> I tried to trigger the same bug in pure Cygwin (as opposed to MSYS2),
> specifically using `disable_pcon` (because MSYS2 defaults to not using the
> pseudo console support because I ran into too many issues to be confident
> enough in it yet), but I think that Cygwin's `vim` is too old and
> therefore might not even send that CSI [ 6n (although `:h t_RV` _does_
> show the expected help).
>
> Now, the patch which I am responding to is completely obscure to me. It is
> very, very unclear to me whether it really tries to only do one thing
> (namely to transfer the input no longer in `read()` but in `setpgid()`),
> or rather does many things at once. Even worse, I have not the faintest
> clue how this patch is trying to accomplish what the commit message
> describes (_because_ it does so many things at once), nor how that could
> be related to the observed incorrect behavior, and as a consequence I have
> no idea how I can hope to fix said observed incorrect behavior.
>
> Could you help shed some light into the problem?
Thanks for the report.
Could you please test if the following patch solves the issue?
diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index f523dafed..ba282b897 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -1239,10 +1239,13 @@ fhandler_pty_slave::mask_switch_to_pcon_in (bool mask, bool xfer)
else if (InterlockedDecrement (&num_reader) == 0)
CloseHandle (slave_reading);
+ bool need_xfer =
+ get_ttyp ()->switch_to_pcon_in && !get_ttyp ()->pcon_activated;
+
/* In GDB, transfer input based on setpgid() does not work because
GDB may not set terminal process group properly. Therefore,
transfer input here if isHybrid is set. */
- if (isHybrid && !!masked != mask && xfer
+ if ((isHybrid || need_xfer) && !!masked != mask && xfer
&& GetStdHandle (STD_INPUT_HANDLE) == get_handle ())
{
if (mask && get_ttyp ()->pcon_input_state_eq (tty::to_nat))
@@ -1536,7 +1539,7 @@ out:
if (ptr0)
{ /* Not tcflush() */
bool saw_eol = totalread > 0 && strchr ("\r\n", ptr0[totalread -1]);
- mask_switch_to_pcon_in (false, saw_eol);
+ mask_switch_to_pcon_in (false, saw_eol || len == 0);
}
}
@@ -2214,6 +2217,15 @@ fhandler_pty_master::write (const void *ptr, size_t len)
return len;
}
+ if (to_be_read_from_pcon () && !get_ttyp ()->pcon_activated
+ && get_ttyp ()->pcon_input_state == tty::to_cyg)
+ {
+ WaitForSingleObject (input_mutex, INFINITE);
+ fhandler_pty_slave::transfer_input (tty::to_nat, from_master,
+ get_ttyp (), input_available_event);
+ ReleaseMutex (input_mutex);
+ }
+
line_edit_status status = line_edit (p, len, ti, &ret);
if (status > line_edit_signalled && status != line_edit_pipe_full)
ret = -1;
--
Takashi Yano <takashi.yano@nifty.ne.jp>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] Cygwin: pty: Reduce unecessary input transfer.
2021-12-10 10:20 ` Takashi Yano
@ 2021-12-10 11:12 ` Johannes Schindelin
2021-12-11 13:40 ` Takashi Yano
0 siblings, 1 reply; 9+ messages in thread
From: Johannes Schindelin @ 2021-12-10 11:12 UTC (permalink / raw)
To: Takashi Yano; +Cc: cygwin-patches
Hi Takashi,
On Fri, 10 Dec 2021, Takashi Yano wrote:
> On Fri, 10 Dec 2021 00:05:27 +0100 (CET)
> Johannes Schindelin wrote:
> > sorry for responding to a patch you sent almost 10 months ago... but... I
> > am struggling with it.
> >
> > First of all, let me describe the problem I am seeing (see also
> > https://github.com/git-for-windows/git/issues/3579): after upgrading the
> > MSYS2 runtime to v3.3.3 in Git for Windows, whenever I ask `git.exe` to
> > spawn `vim.exe` to edit any file, after quitting `vim` I see spurious ANSI
> > sequences being "ghost-typed" into the terminal (which is a MinTTY running
> > under `TERM=xterm`).
> >
> > Apparently the ANSI sequences report the cursor position and the
> > foreground/background color in response to a CSI [ 6n sent from `vim`.
> >
> > Clearly, those sequences should go to `vim.exe`, but they mostly don't
> > arrive there (but in MinTTY instead, as if I had typed them). Sometimes,
> > the foreground/background color seems to arrive in the `vim` process, but
> > the cursor position almost always does not. I suspect that it is important
> > that `git.exe` is a non-MSYS2 process whereas `vim.exe` is an MSYS2
> > process, and something inside the MSYS2 runtime is at fault.
> >
> > I've bisected this incorrect behavior to the patch I am replying to.
> >
> > I tried to trigger the same bug in pure Cygwin (as opposed to MSYS2),
> > specifically using `disable_pcon` (because MSYS2 defaults to not using the
> > pseudo console support because I ran into too many issues to be confident
> > enough in it yet), but I think that Cygwin's `vim` is too old and
> > therefore might not even send that CSI [ 6n (although `:h t_RV` _does_
> > show the expected help).
> >
> > Now, the patch which I am responding to is completely obscure to me. It is
> > very, very unclear to me whether it really tries to only do one thing
> > (namely to transfer the input no longer in `read()` but in `setpgid()`),
> > or rather does many things at once. Even worse, I have not the faintest
> > clue how this patch is trying to accomplish what the commit message
> > describes (_because_ it does so many things at once), nor how that could
> > be related to the observed incorrect behavior, and as a consequence I have
> > no idea how I can hope to fix said observed incorrect behavior.
> >
> > Could you help shed some light into the problem?
>
> Thanks for the report.
> Could you please test if the following patch solves the issue?
It does!
However, I am a bit frustrated because there is still a lot light-shedding
to be done. In the current shape of the code, I do not even understand
what it does, let alone why it works around the problem.
For example, why is there such a long `pcon` stuff going on? I am in the
_disabled_ pseudo console mode, for starters. Like, why is there a
`pcon_input_state`? And why has the `disable_pcon` code path changed at
all (there was no need to touch it, was there)?
Also, `needs_xfer` clearly means `needs transfer`. What transfer? What's
`masked`? And how does it differ from `mask`?
I fear that the pseudo console/non-pseudo console code currently has a
lottery factor of 1. I spent a good part of three entire working days
pouring over it, and I still do not understand it. Usually, a combination
of reading the commit messages, reading the code, parsing
function/variable names with a sprinkling of intuition gets me very far in
understanding any kind of legacy code, but not here. And I do _a lot_ of
legacy code hacking, as part of maintaining Git for Windows. The pseudo
console code in Cygwin really is a class of its own in this regard.
And I have the very strong sense that it does not have to be that way.
I would really like it if the code in `fhandler_*` could see some tender,
loving care, bringing clarity about, for example clearly distinguishing
between the code paths that use pseudo console support vs not, and code
paths regarding Cygwin processes vs not.
I mean, even if your diff below is short, I cannot review it. Not the
context, not my study of three days of the surrounding code and the commit
messages, none of that equips me with enough knowledge to even spot an
obvious bug, because such a bug would still not be obvious to me.
I really hope that this can be fixed. Please let me know if there is
anything I can do to help bring this about.
Thank you,
Johannes
>
>
> diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
> index f523dafed..ba282b897 100644
> --- a/winsup/cygwin/fhandler_tty.cc
> +++ b/winsup/cygwin/fhandler_tty.cc
> @@ -1239,10 +1239,13 @@ fhandler_pty_slave::mask_switch_to_pcon_in (bool mask, bool xfer)
> else if (InterlockedDecrement (&num_reader) == 0)
> CloseHandle (slave_reading);
>
> + bool need_xfer =
> + get_ttyp ()->switch_to_pcon_in && !get_ttyp ()->pcon_activated;
> +
> /* In GDB, transfer input based on setpgid() does not work because
> GDB may not set terminal process group properly. Therefore,
> transfer input here if isHybrid is set. */
> - if (isHybrid && !!masked != mask && xfer
> + if ((isHybrid || need_xfer) && !!masked != mask && xfer
> && GetStdHandle (STD_INPUT_HANDLE) == get_handle ())
> {
> if (mask && get_ttyp ()->pcon_input_state_eq (tty::to_nat))
> @@ -1536,7 +1539,7 @@ out:
> if (ptr0)
> { /* Not tcflush() */
> bool saw_eol = totalread > 0 && strchr ("\r\n", ptr0[totalread -1]);
> - mask_switch_to_pcon_in (false, saw_eol);
> + mask_switch_to_pcon_in (false, saw_eol || len == 0);
> }
> }
>
> @@ -2214,6 +2217,15 @@ fhandler_pty_master::write (const void *ptr, size_t len)
> return len;
> }
>
> + if (to_be_read_from_pcon () && !get_ttyp ()->pcon_activated
> + && get_ttyp ()->pcon_input_state == tty::to_cyg)
> + {
> + WaitForSingleObject (input_mutex, INFINITE);
> + fhandler_pty_slave::transfer_input (tty::to_nat, from_master,
> + get_ttyp (), input_available_event);
> + ReleaseMutex (input_mutex);
> + }
> +
> line_edit_status status = line_edit (p, len, ti, &ret);
> if (status > line_edit_signalled && status != line_edit_pipe_full)
> ret = -1;
>
> --
> Takashi Yano <takashi.yano@nifty.ne.jp>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] Cygwin: pty: Reduce unecessary input transfer.
2021-12-10 11:12 ` Johannes Schindelin
@ 2021-12-11 13:40 ` Takashi Yano
2021-12-13 9:09 ` Takashi Yano
0 siblings, 1 reply; 9+ messages in thread
From: Takashi Yano @ 2021-12-11 13:40 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: cygwin-patches
On Fri, 10 Dec 2021 12:12:44 +0100 (CET)
Johannes Schindelin wrote:
> On Fri, 10 Dec 2021, Takashi Yano wrote:
> > Could you please test if the following patch solves the issue?
>
> It does!
It seems that you already apply this patch to msys2, however,
this is just an experimental patch to identify the cause of
the problem.
Please wait a while for actual patch.
> However, I am a bit frustrated because there is still a lot light-shedding
> to be done. In the current shape of the code, I do not even understand
> what it does, let alone why it works around the problem.
>
> For example, why is there such a long `pcon` stuff going on? I am in the
> _disabled_ pseudo console mode, for starters. Like, why is there a
> `pcon_input_state`? And why has the `disable_pcon` code path changed at
> all (there was no need to touch it, was there)?
>
> Also, `needs_xfer` clearly means `needs transfer`. What transfer? What's
> `masked`? And how does it differ from `mask`?
>
> I fear that the pseudo console/non-pseudo console code currently has a
> lottery factor of 1. I spent a good part of three entire working days
> pouring over it, and I still do not understand it. Usually, a combination
> of reading the commit messages, reading the code, parsing
> function/variable names with a sprinkling of intuition gets me very far in
> understanding any kind of legacy code, but not here. And I do _a lot_ of
> legacy code hacking, as part of maintaining Git for Windows. The pseudo
> console code in Cygwin really is a class of its own in this regard.
>
> And I have the very strong sense that it does not have to be that way.
>
> I would really like it if the code in `fhandler_*` could see some tender,
> loving care, bringing clarity about, for example clearly distinguishing
> between the code paths that use pseudo console support vs not, and code
> paths regarding Cygwin processes vs not.
>
> I mean, even if your diff below is short, I cannot review it. Not the
> context, not my study of three days of the surrounding code and the commit
> messages, none of that equips me with enough knowledge to even spot an
> obvious bug, because such a bug would still not be obvious to me.
>
> I really hope that this can be fixed. Please let me know if there is
> anything I can do to help bring this about.
The current pty code is too complicated indeed. :(
It is very difficult to explain how it works.
Basically, pty has two pairs of pipes, one is for cygwin apps
(io_handle/output_handle), the other is for non-cygwin app (
io_handle_nat/output_handle_nat). This is because these pipe-
pairs are processed differently even without ConPTY.
Outputs to output_handle_nat is forwarded to output_handle by
pty_master_fwd_thread after appropriate processing.
Input from keyboard is switched between two input pipes, i.e.
io_handle and io_handle_nat. When the cygwin-app is activated,
input from keyboard is sent to io_handle, and when the non-
cygwin app is activated, input from keyboard is sent to
io_handle_nat. This switching is done based on switch_to_pcon_in
and pcon_input_state. The name of this variable and related
function is *pcon*, however, these are also used for non-cygwin
apps even without ConPTY by historical reason.
While non-cygwin app is activated, switch_to_pcon_in is true
and pcon_input_state == to_nat. However, if the cygwin-app is
started from non-cygwin app, input from keyboard should be sent
to io_handle. This is done using mask_switch_to_pcon_in(). By
this function, input is temporary sent to io_handle even if
switch_to_pcon_in is true.
The function transfer_input() is used to transfer type ahead
inupt between two input pipes, i.e. io_handle and io_handle_nat.
Masking switch_to_pcon_in state by mask_switch_to_pcon_in() is
done in read() and select(), so input while read() or select()
is NOT called is sent to io_handle_nat rather than io_handle
if switch_to_pcon_in is true. The bug to be fixed now is just
the case.
So, transferring input from io_handle_nat to io_handle solves
the issue in this case. The patch I have sent yesterday is to
add this missing action.
In addition, this problem does not occur if ConPTY is enabled.
--
Takashi Yano <takashi.yano@nifty.ne.jp>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] Cygwin: pty: Reduce unecessary input transfer.
2021-12-11 13:40 ` Takashi Yano
@ 2021-12-13 9:09 ` Takashi Yano
2021-12-13 12:33 ` Takashi Yano
0 siblings, 1 reply; 9+ messages in thread
From: Takashi Yano @ 2021-12-13 9:09 UTC (permalink / raw)
To: cygwin-patches
On Sat, 11 Dec 2021 22:40:30 +0900
Takashi Yano wrote:
> On Fri, 10 Dec 2021 12:12:44 +0100 (CET)
> Johannes Schindelin wrote:
> > On Fri, 10 Dec 2021, Takashi Yano wrote:
> > > Could you please test if the following patch solves the issue?
> >
> > It does!
>
> It seems that you already apply this patch to msys2, however,
> this is just an experimental patch to identify the cause of
> the problem.
>
> Please wait a while for actual patch.
I submitted the patch to cygwin-patches@cygwin.com yesterday.
[PATCH] Cygwin: pty: Add missing input transfer when switch_to_pcon_in state.
--
Takashi Yano <takashi.yano@nifty.ne.jp>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] Cygwin: pty: Reduce unecessary input transfer.
2021-12-13 9:09 ` Takashi Yano
@ 2021-12-13 12:33 ` Takashi Yano
2021-12-14 11:02 ` Takashi Yano
0 siblings, 1 reply; 9+ messages in thread
From: Takashi Yano @ 2021-12-13 12:33 UTC (permalink / raw)
To: cygwin-patches
On Mon, 13 Dec 2021 18:09:35 +0900
Takashi Yano wrote:
> On Sat, 11 Dec 2021 22:40:30 +0900
> Takashi Yano wrote:
> > On Fri, 10 Dec 2021 12:12:44 +0100 (CET)
> > Johannes Schindelin wrote:
> > > On Fri, 10 Dec 2021, Takashi Yano wrote:
> > > > Could you please test if the following patch solves the issue?
> > >
> > > It does!
> >
> > It seems that you already apply this patch to msys2, however,
> > this is just an experimental patch to identify the cause of
> > the problem.
> >
> > Please wait a while for actual patch.
>
> I submitted the patch to cygwin-patches@cygwin.com yesterday.
>
> [PATCH] Cygwin: pty: Add missing input transfer when switch_to_pcon_in state.
Sorry, this has a problem with pseudo console enabled.
I will submit additional patch for that shortly.
--
Takashi Yano <takashi.yano@nifty.ne.jp>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] Cygwin: pty: Reduce unecessary input transfer.
2021-12-13 12:33 ` Takashi Yano
@ 2021-12-14 11:02 ` Takashi Yano
0 siblings, 0 replies; 9+ messages in thread
From: Takashi Yano @ 2021-12-14 11:02 UTC (permalink / raw)
To: cygwin-patches
On Mon, 13 Dec 2021 21:33:31 +0900
Takashi Yano wrote:
> On Mon, 13 Dec 2021 18:09:35 +0900
> Takashi Yano wrote:
> > On Sat, 11 Dec 2021 22:40:30 +0900
> > Takashi Yano wrote:
> > > On Fri, 10 Dec 2021 12:12:44 +0100 (CET)
> > > Johannes Schindelin wrote:
> > > > On Fri, 10 Dec 2021, Takashi Yano wrote:
> > > > > Could you please test if the following patch solves the issue?
> > > >
> > > > It does!
> > >
> > > It seems that you already apply this patch to msys2, however,
> > > this is just an experimental patch to identify the cause of
> > > the problem.
> > >
> > > Please wait a while for actual patch.
> >
> > I submitted the patch to cygwin-patches@cygwin.com yesterday.
> >
> > [PATCH] Cygwin: pty: Add missing input transfer when switch_to_pcon_in state.
>
> Sorry, this has a problem with pseudo console enabled.
> I will submit additional patch for that shortly.
I recommend applying the following three patches to fix the issue.
https://cygwin.com/git/?p=newlib-cygwin.git;a=commitdiff;h=acc44e09d1d06ae8fdf96ea8d7272de10dd6007b
https://cygwin.com/git/?p=newlib-cygwin.git;a=commitdiff;h=720234b78afb4a972c3cea4ae22d181ea83568cc
https://cygwin.com/git/?p=newlib-cygwin.git;a=commitdiff;h=0795f704f750e4b441aa256b1b849e4ebe3af684
--
Takashi Yano <takashi.yano@nifty.ne.jp>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-12-14 11:02 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-11 9:09 [PATCH v2] Cygwin: pty: Reduce unecessary input transfer Takashi Yano
2021-02-12 9:43 ` Corinna Vinschen
2021-12-09 23:05 ` Johannes Schindelin
2021-12-10 10:20 ` Takashi Yano
2021-12-10 11:12 ` Johannes Schindelin
2021-12-11 13:40 ` Takashi Yano
2021-12-13 9:09 ` Takashi Yano
2021-12-13 12:33 ` Takashi Yano
2021-12-14 11:02 ` Takashi Yano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).