Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.
On Sun, 30 Aug 2020 06:13:17 +0900 Takashi Yano via Cygwin-patches wrote: > On Sun, 30 Aug 2020 05:25:06 +0900 > Takashi Yano via Cygwin-patches wrote: > > On Sat, 29 Aug 2020 22:14:20 +0900 > > Takashi Yano via Cygwin-patches wrote: > > > On Sat, 29 Aug 2020 20:12:28 +0900 > > > Takashi Yano via Cygwin-patches wrote: > > > > Hi Corinna, > > > > > > > > On Sat, 29 Aug 2020 04:25:54 +0900 > > > > Takashi Yano via Cygwin-patches wrote: > > > > > Hi Corinna, > > > > > > > > > > On Fri, 28 Aug 2020 15:45:03 +0200 > > > > > Corinna Vinschen wrote: > > > > > > Hi Takashi, > > > > > > > > > > > > On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote: > > > > > > > Pseudo console generates escape sequences on execution of > > > > > > > non-cygwin > > > > > > > apps. If the terminal does not support escape sequence, output > > > > > > > will > > > > > > > be garbled. This patch prevents garbled output in dumb terminal by > > > > > > > disabling pseudo console. > > > > [...] > > > > > > > > > > > > Would you mind to encapsulate the TERM checks into a > > > > > > fhandler_pty_slave > > > > > > method so the TERM specific stuff is done in the fhandler code, not > > > > > > in spawn.cc? > > > > > > > > > > Thansk for the suggestion. I will submit v2 patch. > > > > > > > > What do you think of v3 patch attached? With this patch, > > > > terminal capability is checked by looking into terminfo > > > > database rather than just checking terminal name. This > > > > solution is more essential for the issue to be solved, > > > > I think. > > > > > > > > One downside of this solution, I noticed, is that tmux > > > > sets TERM to "screen", which does not have CSI6n, by > > > > default. As a result, pseudo console is disbled in tmux > > > > by default. Setting TERM, such as screen.xterm-256color, > > > > will solve the issue. > > > > > > Attached is the v4 patch. Small bug was fixed. > > > > Bug fixed again. v5 patch attached. > > v6: Refactor the code a little. v7: Fix another bug again. -- Takashi Yano v7-0001-Cygwin-pty-Disable-pseudo-console-if-TERM-does-no.patch Description: Binary data
Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.
On Sun, 30 Aug 2020 16:42:17 +0900 Takashi Yano via Cygwin-patches wrote: > On Sun, 30 Aug 2020 06:13:17 +0900 > Takashi Yano via Cygwin-patches wrote: > > On Sun, 30 Aug 2020 05:25:06 +0900 > > Takashi Yano via Cygwin-patches wrote: > > > On Sat, 29 Aug 2020 22:14:20 +0900 > > > Takashi Yano via Cygwin-patches wrote: > > > > On Sat, 29 Aug 2020 20:12:28 +0900 > > > > Takashi Yano via Cygwin-patches wrote: > > > > > Hi Corinna, > > > > > > > > > > On Sat, 29 Aug 2020 04:25:54 +0900 > > > > > Takashi Yano via Cygwin-patches wrote: > > > > > > Hi Corinna, > > > > > > > > > > > > On Fri, 28 Aug 2020 15:45:03 +0200 > > > > > > Corinna Vinschen wrote: > > > > > > > Hi Takashi, > > > > > > > > > > > > > > On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote: > > > > > > > > Pseudo console generates escape sequences on execution of > > > > > > > > non-cygwin > > > > > > > > apps. If the terminal does not support escape sequence, output > > > > > > > > will > > > > > > > > be garbled. This patch prevents garbled output in dumb terminal > > > > > > > > by > > > > > > > > disabling pseudo console. > > > > > [...] > > > > > > > > > > > > > > Would you mind to encapsulate the TERM checks into a > > > > > > > fhandler_pty_slave > > > > > > > method so the TERM specific stuff is done in the fhandler code, > > > > > > > not > > > > > > > in spawn.cc? > > > > > > > > > > > > Thansk for the suggestion. I will submit v2 patch. > > > > > > > > > > What do you think of v3 patch attached? With this patch, > > > > > terminal capability is checked by looking into terminfo > > > > > database rather than just checking terminal name. This > > > > > solution is more essential for the issue to be solved, > > > > > I think. > > > > > > > > > > One downside of this solution, I noticed, is that tmux > > > > > sets TERM to "screen", which does not have CSI6n, by > > > > > default. As a result, pseudo console is disbled in tmux > > > > > by default. Setting TERM, such as screen.xterm-256color, > > > > > will solve the issue. > > > > > > > > Attached is the v4 patch. Small bug was fixed. > > > > > > Bug fixed again. v5 patch attached. > > > > v6: Refactor the code a little. > > v7: Fix another bug again. v8: Yet another bug fix. -- Takashi Yano v8-0001-Cygwin-pty-Disable-pseudo-console-if-TERM-does-no.patch Description: Binary data
Re: [PATCH] Cygwin: sigproc.cc: add comment
On Aug 29 06:01, Ken Brown via Cygwin-patches wrote:
> ---
> winsup/cygwin/sigproc.cc | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/winsup/cygwin/sigproc.cc b/winsup/cygwin/sigproc.cc
> index 2a9734f00..47352c213 100644
> --- a/winsup/cygwin/sigproc.cc
> +++ b/winsup/cygwin/sigproc.cc
> @@ -44,7 +44,11 @@ char NO_COPY myself_nowait_dummy[1] = {'0'};
>
> #define Static static NO_COPY
>
> -/* All my children info. Avoid expensive constructor ops at DLL startup */
> +/* All my children info. Avoid expensive constructor ops at DLL
> + startup.
> +
> + This class can allocate memory. But there's no need to free it
> + because only one instance of the class is created per process. */
> class child_procs {
> #ifdef __i386__
> static const int _NPROCS = 256;
> --
> 2.28.0
Sure, why not.
Thanks,
Corinna
Re: [PATCH] Cygwin: Remove waitloop argument from try_to_debug()
On Aug 29 15:43, Jon Turney wrote: > Currently, when using CYGWIN='error_start=dumper', the core dump written > in response to an exception is non-deterministic, as the faulting > process isn't stopped while the dumper is started (it even seems > possible in theory that the faulting process could have exited before > the dumper process attaches). > > Remove the waitloop argument, only used in this case, so the faulting > process busy-waits until the dump starts. > > Code archeology to determine why the code is this way didn't really turn > up any answers, but this seems a low-risk change, as this only changes > the behaviour when: > > - a debugger isn't already attached > - an error_start is specified in CYGWIN env var > - an exception has occured which will be translated to a signal > > Future work: This probably can be further simplified to make it > completely synchronous by waiting for the dumper process to exit. This > would avoid the race condition of the dumper attaching and detaching > before we get around to checking for that (which we try to work around > by juggling thread priorities), and the failure state where the dumper > doesn't attach and we spin indefinitely. > --- > winsup/cygwin/exceptions.cc | 8 ++-- > winsup/cygwin/winsup.h | 2 +- > 2 files changed, 3 insertions(+), 7 deletions(-) I'm a bit fuzzy on the implications but it doesn't look like it hurts a lot(*). Let's get it in. Thanks, Corinna (*) Famous last words alarm...
Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.
Hi Takashi, On Aug 29 20:12, Takashi Yano via Cygwin-patches wrote: > Hi Corinna, > > On Sat, 29 Aug 2020 04:25:54 +0900 > Takashi Yano via Cygwin-patches wrote: > > Hi Corinna, > > > > On Fri, 28 Aug 2020 15:45:03 +0200 > > Corinna Vinschen wrote: > > > Hi Takashi, > > > > > > On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote: > > > > Pseudo console generates escape sequences on execution of non-cygwin > > > > apps. If the terminal does not support escape sequence, output will > > > > be garbled. This patch prevents garbled output in dumb terminal by > > > > disabling pseudo console. > [...] > > > > > > Would you mind to encapsulate the TERM checks into a fhandler_pty_slave > > > method so the TERM specific stuff is done in the fhandler code, not > > > in spawn.cc? > > > > Thansk for the suggestion. I will submit v2 patch. > > What do you think of v3 patch attached? With this patch, > terminal capability is checked by looking into terminfo > database rather than just checking terminal name. This > solution is more essential for the issue to be solved, > I think. > > One downside of this solution, I noticed, is that tmux > sets TERM to "screen", which does not have CSI6n, by > default. As a result, pseudo console is disbled in tmux > by default. Setting TERM, such as screen.xterm-256color, > will solve the issue. I like the idea in general, but isn't there a noticable perfomance hit? Corinna
Re: [PATCH] Cygwin: Remove waitloop argument from try_to_debug()
On 30/08/2020 13:47, Corinna Vinschen wrote: On Aug 29 15:43, Jon Turney wrote: Currently, when using CYGWIN='error_start=dumper', the core dump written in response to an exception is non-deterministic, as the faulting process isn't stopped while the dumper is started (it even seems possible in theory that the faulting process could have exited before the dumper process attaches). Remove the waitloop argument, only used in this case, so the faulting process busy-waits until the dump starts. Code archaeology to determine why the code is this way didn't really turn up any answers, but this seems a low-risk change, as this only changes the behaviour when: - a debugger isn't already attached - an error_start is specified in CYGWIN env var - an exception has occurred which will be translated to a signal Future work: This probably can be further simplified to make it completely synchronous by waiting for the dumper process to exit. This would avoid the race condition of the dumper attaching and detaching before we get around to checking for that (which we try to work around by juggling thread priorities), and the failure state where the dumper doesn't attach and we spin indefinitely. So, on reflection, this idea is wrong, and it currently is the way it has to be. If we use CYGWIN='error_start=gdb', we should be able to continue the thread which encountered an exception, which we can't do if it's blocked waiting for the error_start process to exit. So I'll tweak the patch commentary before pushing.
Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.
Hi Corinna, On Sun, 30 Aug 2020 14:49:25 +0200 Corinna Vinschen wrote: > Hi Takashi, > > On Aug 29 20:12, Takashi Yano via Cygwin-patches wrote: > > Hi Corinna, > > > > On Sat, 29 Aug 2020 04:25:54 +0900 > > Takashi Yano via Cygwin-patches wrote: > > > Hi Corinna, > > > > > > On Fri, 28 Aug 2020 15:45:03 +0200 > > > Corinna Vinschen wrote: > > > > Hi Takashi, > > > > > > > > On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote: > > > > > Pseudo console generates escape sequences on execution of non-cygwin > > > > > apps. If the terminal does not support escape sequence, output will > > > > > be garbled. This patch prevents garbled output in dumb terminal by > > > > > disabling pseudo console. > > [...] > > > > > > > > Would you mind to encapsulate the TERM checks into a fhandler_pty_slave > > > > method so the TERM specific stuff is done in the fhandler code, not > > > > in spawn.cc? > > > > > > Thansk for the suggestion. I will submit v2 patch. > > > > What do you think of v3 patch attached? With this patch, > > terminal capability is checked by looking into terminfo > > database rather than just checking terminal name. This > > solution is more essential for the issue to be solved, > > I think. > > > > One downside of this solution, I noticed, is that tmux > > sets TERM to "screen", which does not have CSI6n, by > > default. As a result, pseudo console is disbled in tmux > > by default. Setting TERM, such as screen.xterm-256color, > > will solve the issue. > > I like the idea in general, but isn't there a noticable perfomance hit? I have measured the startup time of non-cygwin process with v2 and v8 patch. mintty with v2: 92.7ms emacs-dumb with v2: 22.8ms mintty with v8: 94.6ms emacs-dumb with v8: 22.7ms There is not noticeable difference more than measurement error. By the way, I have implemented timeout strategy for CSI6n, you mentioned in the previous comment, for a test. This check is done only on the first execution of non-cygwin apps in the pty. With this patch, first checks if the terminfo has cursor_home (ESC [H). If terminfo has cursor_home, ANSI escape sequence is supposed to be supported. In this case, I expect not to display garbage "^[[6n" even if CSI6n is sent because the parser ignores unsupported CSI sequences. With this implementation, pseudo console works in tmux as well even if TERM=screen. Please have a look v9 patch attached. The performance of v9 is also checked. mintty with v9: 93.9ms emacs-dumb with v9: 22.8ms ANSI without CSI6n: 22.8ms [the first time in v9] mintty: 94.8ms emacs-dumb: 22.5ms ANSI without CSI6n: 63.5ms Most of the results are the same as v2 and v8 except for the first execution of non-cygwin apps in ansi terminal without CSI6n. It takes about 40ms (timeout) longer than dumb terminal in ANSI terminal without CSI6n support. However, this causes only on the first execution of non-cygwin apps in pty. I think this is the most reasonable one I have ever proposed. -- Takashi Yano v9-0001-Cygwin-pty-Disable-pseudo-console-if-TERM-does-no.patch Description: Binary data
