Re: [PATCH v4 1/1] Cygwin: pty: Fix state management for pseudo console support.
Hi Corinna, I have posted several patches for PTY with pseudo console support. Please apply them in the following order. [PATCH 1/4] Cygwin: pty: Code cleanup - Cleanup the code which is commented out by #if 0 regarding pseudo console. - Remove #if 1 for experimental code which seems to be stable. [PATCH 2/4] Cygwin: pty: Speed up a little hooked Win32 API for pseudo console. - Some Win32 APIs are hooked in pty code for pseudo console support. This causes slow down. This patch improves speed a little. [PATCH 3/4] Cygwin: pty: Move function hook_api() into hookapi.cc. - PTY uses Win32 API hook for pseudo console suppot. The function hook_api() is used for this purpose and defined in fhandler_tty.cc previously. This patch moves it into hookapi.cc. [PATCH 4/4] Cygwin: pty: Limit API hook to the program linked with the APIs. - API hook used for pseudo console support causes slow down. This patch limits API hook to only program which is linked with the corresponding APIs. Normal cygwin program is not linked with such APIs (such as WriteFile, etc...) directly, therefore, no slow down occurs. However, console access by cygwin.dll itself cannot switch the r/w pipe to pseudo console side. Therefore, the code to switch it forcely to pseudo console side is added to smallprint.cc and strace.cc. [PATCH v5 1/1] Cygwin: pty: Fix state management for pseudo console support. - Pseudo console support introduced by commit 169d65a5774acc76ce3f3feeedcbae7405aa9b57 has some bugs which cause mismatch between state variables and real pseudo console state regarding console attaching and r/w pipe switching. This patch fixes this issue by redesigning the state management. This hopefully fixes the problem 3 in https://cygwin.com/ml/cygwin/2019-08/msg00401.html This also fixes the first problem regarding "Bad file descriptor" error reported in https://cygwin.com/ml/cygwin-patches/2019-q3/msg00104.html [PATCH 1/2] Cygwin: pty: Add a workaround for ^C handling. - Pseudo console support introduced by commit 169d65a5774acc76ce3f3feeedcbae7405aa9b57 sometimes cause random crash or freeze by pressing ^C while cygwin and non-cygwin processes are executed simultaneously in the same pty. This patch is a workaround for this issue. [PATCH 2/2] Cygwin: pty: Disable clear screen on new pty if TERM=dumb or emacs*. - Pseudo console support introduced by commit 169d65a5774acc76ce3f3feeedcbae7405aa9b57 shows garbage ^[[H^[[J in some of emacs screens. These screens do not handle ANSI escape sequences. Therefore, clear screen is disabled on these screens. This fixes the second problem on emacs reported in https://cygwin.com/ml/cygwin-patches/2019-q3/msg00104.html On Tue, 3 Sep 2019 11:16:38 +0200 Corinna Vinschen wrote: > This is a slowdown of about 15%. That's quite a lot. Can't you just > check the incoming handle against the interesting handles somehow? > If there's no other way around it, we should at least make sure (in a > separate patch) that Cygwin calls NtReadFile/NtWriteFile throughout, > except in tty and console code. I came up with an idea, and implemented it. As described obove, Win32 APIs are not hooked any more in normal cygwin process. Hook is done only if the program is directly linked with corresponding APIs. However, this strategy does not have the effect for console access by cygwin1.dll itself. So, to switch r/w pipe to pseudo console side, I added the code in strace.cc and smallprint.cc. Could you please have a look? -- Takashi Yano
Re: [PATCH v4 1/1] Cygwin: pty: Fix state management for pseudo console support.
Hi Takashi,
On Sep 3 12:05, Takashi Yano wrote:
> > > - Sleep (60); /* Wait for pty_master_fwd_thread() */
> > > + Sleep (20); /* Wait for pty_master_fwd_thread() */
> >
> > Isn't that a separate issue as well? A separate patch may be in order
> > here, kind of like "Cygwin: pseudo console: reduce time sleeping ..."
> > with a short description why that makes sense?
>
> Actually it is not. The wait time became able to be reduced by
> redesigning switching of r/w pipes which managed via variable
> switch_to_pcon. So I think this should be included in this patch.
Ok.
> > However, I have a few questions in terms of the code in general, namely
> > in terms of
> >
> > ALWAYS_USE_PCON
> > USE_API_HOOK
> > USE_OWN_NLS_FUNC
> >
> > Can you describe again why you introduced these macros?
>
> These are defined for debugging purpose.
>
> If ALWAYS_USE_PCON is defined to true, pseudo console pipe is used for
> all process including pure cygwin process. Usually, this should be false
> so that the cygwin process use named pipe as previous.
>
> USE_API_HOOK is for enabling/disabling the API hook to detect direct
> console access in cygwin process. This should be true so that the
> r/w pipe switching is set to pseudo console side for the cygwin
> process which directly access console.
>
> As for USE_OWN_NLS_FUNC, I have not decided yet which codes should be
> used. If USE_OWN_NLS_FUNC is false, setlocale (LC_CTYPE, "") is
> called therefore it may affect to some programs wihch do not call
> setlocale().
>
> > In terms of USE_API_HOOK:
> >
> > - Shouldn't the hook_api function be moved to hookapi.cc?
>
> I will move it into hookapi.cc, and post it as a separate patch.
>
> > - Do we really want to hook every invocation of WriteFile/ReadFile?
> > Doesn't that potentially slow down an exec'ed process a lot?
> > We're still not using the NT functions throughout outside of the
> > console/tty code.
>
> I measured the time for calling WriteFile() 100 times writing
> 1 byte to a disk file for each call.
Files are not affected in Cygwin, fortunately. They use
NtReadFile/NtWriteFile, not ReadFile/WriteFile. However, other
stuff is still affected...
> Not hooked:
> Total: 4.558321 seconsd
>
> Hooked:
> Total: 6.522692 seconsd
>
> Hooking causes slow down indeed. It seems that GetConsoleMode()
> is slow. So I have added the check for GetFileType() before
> GetConsoleMode() and made check in two stages.
>
> Hooked (new):
> Total: 5.217996 seconsd
>
> This results in speed up a little. I will post another patch for this.
This is a slowdown of about 15%. That's quite a lot. Can't you just
check the incoming handle against the interesting handles somehow?
If there's no other way around it, we should at least make sure (in a
separate patch) that Cygwin calls NtReadFile/NtWriteFile throughout,
except in tty and console code.
> > In terms of USE_OWN_NLS_FUNC:
> >
> > - Why do we need this function at all? Can't this be handled by
> > __loadlocale instead? If not, what is __loadlocale missing to make
> > this work without duplicating the function?
>
> Calling __loadlocale() here causes execution error.
>
> mintty:
> 0 [main] tcsh 1901 sig_send: error sending signal 6, pid 1901, pipe
> handle 0x0, nb 0, packsize 164, Win32 error 6
>
> script:
> Script started, file is typescript
> script: failed to execute /bin/tcsh: Bad address
> Script done, file is typescript
>
> I could not find out the reason. Some kind of initialization which
> is needed by __loadlocale() may not be done yet. So I copied
> only necessary part from __loadlocale() and nl_langinfo().
>
> Simply,
> path_conv a ("/usr/share/locale/locale.alias");
> also causes errors on starting mintty.
>
> Ideally, the cause of the error should be found out, I suppose.
Indeed. We can keep the code in for now, but the end result should
call a tweaked version of __loadlocale instead. As long as the
tweak only requires a single extra argument, or if the category or
new_locale argument can be used as indicator to trigger the required
special behavour, we should have no problem to get that into newlib.
Thanks,
Corinna
--
Corinna Vinschen
Cygwin Maintainer
signature.asc
Description: PGP signature
Re: [PATCH v4 1/1] Cygwin: pty: Fix state management for pseudo console support.
Hi Corinna,
On Mon, 2 Sep 2019 16:37:16 +0200
Corinna Vinschen wrote:
> > class fhandler_pty_slave: public fhandler_pty_common
> > {
> >HANDLE inuse;// used to indicate that a tty is in use
> >HANDLE output_handle_cyg, io_handle_cyg;
> > + DWORD pidRestore;
>
> Please don't use camelback. s/pidRestore/pid_restore/g
I will fix it in v5 patch.
> > - HeapAlloc (GetProcessHeap (), 0, num * sizeof (DWORD));
> > - num = GetConsoleProcessList (list, num);
> > + HeapAlloc (GetProcessHeap (), 0, (num + 16) * sizeof (DWORD));
> > + num = GetConsoleProcessList (list, num + 16);
>
> You're still going to change that, right?
This is temporary code while debugging. Sorry. I will fix it in v5.
> > @@ -855,26 +868,6 @@ fhandler_pty_slave::cleanup ()
> > int
> > fhandler_pty_slave::close ()
> > {
> > -#if 0
> > - if (getPseudoConsole ())
> > -{
> > - INPUT_RECORD inp[128];
> > - DWORD n;
> > - PeekFunc =
> > - PeekConsoleInputA_Orig ? PeekConsoleInputA_Orig : PeekConsoleInput;
> > - PeekFunc (get_handle (), inp, 128, );
> > - bool pipe_empty = true;
> > - while (n-- > 0)
> > - if (inp[n].EventType == KEY_EVENT && inp[n].Event.KeyEvent.bKeyDown)
> > - pipe_empty = false;
> > - if (pipe_empty)
> > - {
> > - /* Flush input buffer */
> > - size_t len = UINT_MAX;
> > - read (NULL, len);
> > - }
> > -}
> > -#endif
>
> Ideally stuff like that should be in a separate code cleanup patch.
I see. I will post it as a separate patch.
> > - Sleep (60); /* Wait for pty_master_fwd_thread() */
> > + Sleep (20); /* Wait for pty_master_fwd_thread() */
>
> Isn't that a separate issue as well? A separate patch may be in order
> here, kind of like "Cygwin: pseudo console: reduce time sleeping ..."
> with a short description why that makes sense?
Actually it is not. The wait time became able to be reduced by
redesigning switching of r/w pipes which managed via variable
switch_to_pcon. So I think this should be included in this patch.
> > + /* If not attached this pseudo console, try to attach temporarily. */
>
> to
Thanks.
> > - get_ttyp ()->hPseudoConsole = NULL;
> > + //get_ttyp ()->hPseudoConsole = NULL; // Do not clear for safty.
>
> Why don't you just drop the line?
OK. Just drop it.
> Other than that, the patch looks good.
>
> However, I have a few questions in terms of the code in general, namely
> in terms of
>
> ALWAYS_USE_PCON
> USE_API_HOOK
> USE_OWN_NLS_FUNC
>
> Can you describe again why you introduced these macros?
These are defined for debugging purpose.
If ALWAYS_USE_PCON is defined to true, pseudo console pipe is used for
all process including pure cygwin process. Usually, this should be false
so that the cygwin process use named pipe as previous.
USE_API_HOOK is for enabling/disabling the API hook to detect direct
console access in cygwin process. This should be true so that the
r/w pipe switching is set to pseudo console side for the cygwin
process which directly access console.
As for USE_OWN_NLS_FUNC, I have not decided yet which codes should be
used. If USE_OWN_NLS_FUNC is false, setlocale (LC_CTYPE, "") is
called therefore it may affect to some programs wihch do not call
setlocale().
> In terms of USE_API_HOOK:
>
> - Shouldn't the hook_api function be moved to hookapi.cc?
I will move it into hookapi.cc, and post it as a separate patch.
> - Do we really want to hook every invocation of WriteFile/ReadFile?
> Doesn't that potentially slow down an exec'ed process a lot?
> We're still not using the NT functions throughout outside of the
> console/tty code.
I measured the time for calling WriteFile() 100 times writing
1 byte to a disk file for each call.
Not hooked:
Total: 4.558321 seconsd
Hooked:
Total: 6.522692 seconsd
Hooking causes slow down indeed. It seems that GetConsoleMode()
is slow. So I have added the check for GetFileType() before
GetConsoleMode() and made check in two stages.
Hooked (new):
Total: 5.217996 seconsd
This results in speed up a little. I will post another patch for this.
> In terms of USE_OWN_NLS_FUNC:
>
> - Why do we need this function at all? Can't this be handled by
> __loadlocale instead? If not, what is __loadlocale missing to make
> this work without duplicating the function?
Calling __loadlocale() here causes execution error.
mintty:
0 [main] tcsh 1901 sig_send: error sending signal 6, pid 1901, pipe
handle 0x0, nb 0, packsize 164, Win32 error 6
script:
Script started, file is typescript
script: failed to execute /bin/tcsh: Bad address
Script done, file is typescript
I could not find out the reason. Some kind of initialization which
is needed by __loadlocale() may not be done yet. So I copied
only necessary part from __loadlocale() and nl_langinfo().
Simply,
path_conv a ("/usr/share/locale/locale.alias");
also causes errors
Re: [PATCH v4 1/1] Cygwin: pty: Fix state management for pseudo console support.
On Sep 2 07:11, Takashi Yano wrote:
> - Pseudo console support introduced by commit
> 169d65a5774acc76ce3f3feeedcbae7405aa9b57 has some bugs which
> cause mismatch between state variables and real pseudo console
> state regarding console attaching and r/w pipe switching. This
> patch fixes this issue by redesigning the state management.
> ---
> winsup/cygwin/dtable.cc | 15 +-
> winsup/cygwin/fhandler.h | 6 +-
> winsup/cygwin/fhandler_console.cc | 6 +-
> winsup/cygwin/fhandler_tty.cc | 415 --
> winsup/cygwin/fork.cc | 24 +-
> winsup/cygwin/spawn.cc| 65 +++--
> 6 files changed, 289 insertions(+), 242 deletions(-)
> [...]
> class fhandler_pty_slave: public fhandler_pty_common
> {
>HANDLE inuse; // used to indicate that a tty is in use
>HANDLE output_handle_cyg, io_handle_cyg;
> + DWORD pidRestore;
Please don't use camelback. s/pidRestore/pid_restore/g
> - HeapAlloc (GetProcessHeap (), 0, num * sizeof (DWORD));
> - num = GetConsoleProcessList (list, num);
> + HeapAlloc (GetProcessHeap (), 0, (num + 16) * sizeof (DWORD));
> + num = GetConsoleProcessList (list, num + 16);
You're still going to change that, right?
> @@ -855,26 +868,6 @@ fhandler_pty_slave::cleanup ()
> int
> fhandler_pty_slave::close ()
> {
> -#if 0
> - if (getPseudoConsole ())
> -{
> - INPUT_RECORD inp[128];
> - DWORD n;
> - PeekFunc =
> - PeekConsoleInputA_Orig ? PeekConsoleInputA_Orig : PeekConsoleInput;
> - PeekFunc (get_handle (), inp, 128, );
> - bool pipe_empty = true;
> - while (n-- > 0)
> - if (inp[n].EventType == KEY_EVENT && inp[n].Event.KeyEvent.bKeyDown)
> - pipe_empty = false;
> - if (pipe_empty)
> - {
> - /* Flush input buffer */
> - size_t len = UINT_MAX;
> - read (NULL, len);
> - }
> -}
> -#endif
Ideally stuff like that should be in a separate code cleanup patch.
> - Sleep (60); /* Wait for pty_master_fwd_thread() */
> + Sleep (20); /* Wait for pty_master_fwd_thread() */
Isn't that a separate issue as well? A separate patch may be in order
here, kind of like "Cygwin: pseudo console: reduce time sleeping ..."
with a short description why that makes sense?
> + /* If not attached this pseudo console, try to attach temporarily. */
to
> - get_ttyp ()->hPseudoConsole = NULL;
> + //get_ttyp ()->hPseudoConsole = NULL; // Do not clear for safty.
Why don't you just drop the line?
Other than that, the patch looks good.
However, I have a few questions in terms of the code in general, namely
in terms of
ALWAYS_USE_PCON
USE_API_HOOK
USE_OWN_NLS_FUNC
Can you describe again why you introduced these macros?
In terms of USE_API_HOOK:
- Shouldn't the hook_api function be moved to hookapi.cc?
- Do we really want to hook every invocation of WriteFile/ReadFile?
Doesn't that potentially slow down an exec'ed process a lot?
We're still not using the NT functions throughout outside of the
console/tty code.
In terms of USE_OWN_NLS_FUNC:
- Why do we need this function at all? Can't this be handled by
__loadlocale instead? If not, what is __loadlocale missing to make
this work without duplicating the function?
Thanks,
Corinna
--
Corinna Vinschen
Cygwin Maintainer
signature.asc
Description: PGP signature
