[PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.

[PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.

[Takashi Yano]

- Cygwin console with xterm compatible mode causes problem reported
  in https://www.cygwin.com/ml/cygwin-patches/2020-q1/msg00212.html
  if background/foreground colors are set to gray/black respectively
  in Win10 1903/1909. This is caused by "CSI Ps L" (IL), "CSI Ps M"
  (DL) and "ESC M" (RI) control sequences which are broken. This
  patch adds a workaround for the issue.
---
 winsup/cygwin/fhandler_console.cc | 156 +++++++++++++++++++++++++++++-
 winsup/cygwin/wincap.cc           |  10 ++
 winsup/cygwin/wincap.h            |   2 +
 3 files changed, 166 insertions(+), 2 deletions(-)

diff --git a/winsup/cygwin/fhandler_console.cc b/winsup/cygwin/fhandler_console.cc
index 328424a7d..c2198ea1e 100644
--- a/winsup/cygwin/fhandler_console.cc
+++ b/winsup/cygwin/fhandler_console.cc
@@ -57,6 +57,16 @@ bool NO_COPY fhandler_console::invisible_console;
    Only one console can exist in a process, therefore, static is suitable. */
 static struct fhandler_base::rabuf_t con_ra;
 
+/* Write pending buffer for ESC sequence handling
+   in xterm compatible mode */
+#define WPBUF_LEN 256
+static unsigned char wpbuf[WPBUF_LEN];
+static int wpixput;
+#define wpbuf_put(x) \
+  wpbuf[wpixput++] = x; \
+  if (wpixput > WPBUF_LEN) \
+    wpixput--;
+
 static void
 beep ()
 {
@@ -2014,6 +2024,82 @@ fhandler_console::char_command (char c)
   char buf[40];
   int r, g, b;
 
+  if (wincap.has_con_24bit_colors () && !con_is_legacy)
+    {
+      /* For xterm compatible mode */
+      DWORD wn;
+      switch (c)
+	{
+	case 'r': /* DECSTBM */
+	  con.scroll_region.Top = con.args[0] ? con.args[0] - 1 : 0;
+	  con.scroll_region.Bottom = con.args[1] ? con.args[1] - 1 : -1;
+	  wpbuf_put (c);
+	  /* Just send the sequence */
+	  WriteConsoleA (get_output_handle (), wpbuf, wpixput, &wn, 0);
+	  break;
+	case 'L': /* IL */
+	  if (wincap.has_con_broken_il_dl ())
+	    {
+	      /* Use "CSI Ps T" instead */
+	      cursor_get (&x, &y);
+	      __small_sprintf (buf, "\033[%d;%dr",
+			       y + 1 - con.b.srWindow.Top,
+			       srBottom + 1 - con.b.srWindow.Top);
+	      WriteConsoleA (get_output_handle (), buf, strlen (buf), &wn, 0);
+	      wpbuf_put ('T');
+	      WriteConsoleA (get_output_handle (), wpbuf, wpixput, &wn, 0);
+	      __small_sprintf (buf, "\033[%d;%dr",
+			       srTop + 1 - con.b.srWindow.Top,
+			       srBottom + 1 - con.b.srWindow.Top);
+	      WriteConsoleA (get_output_handle (), buf, strlen (buf), &wn, 0);
+	      __small_sprintf (buf, "\033[%d;%dH",
+			       y + 1 - con.b.srWindow.Top, x + 1);
+	      WriteConsoleA (get_output_handle (), buf, strlen (buf), &wn, 0);
+	    }
+	  else
+	    {
+	      wpbuf_put (c);
+	      /* Just send the sequence */
+	      WriteConsoleA (get_output_handle (), wpbuf, wpixput, &wn, 0);
+	    }
+	  break;
+	case 'M': /* DL */
+	  if (wincap.has_con_broken_il_dl ())
+	    {
+	      /* Use "CSI Ps S" instead */
+	      cursor_get (&x, &y);
+	      __small_sprintf (buf, "\033[%d;%dr",
+			       y + 1 - con.b.srWindow.Top,
+			       srBottom + 1 - con.b.srWindow.Top);
+	      WriteConsoleA (get_output_handle (), buf, strlen (buf), &wn, 0);
+	      wpbuf_put ('S');
+	      WriteConsoleA (get_output_handle (), wpbuf, wpixput, &wn, 0);
+	      __small_sprintf (buf, "\033[%d;%dr",
+			       srTop + 1 - con.b.srWindow.Top,
+			       srBottom + 1 - con.b.srWindow.Top);
+	      WriteConsoleA (get_output_handle (), buf, strlen (buf), &wn, 0);
+	      __small_sprintf (buf, "\033[%d;%dH",
+			       y + 1 - con.b.srWindow.Top, x + 1);
+	      WriteConsoleA (get_output_handle (), buf, strlen (buf), &wn, 0);
+	    }
+	  else
+	    {
+	      wpbuf_put (c);
+	      /* Just send the sequence */
+	      WriteConsoleA (get_output_handle (), wpbuf, wpixput, &wn, 0);
+	    }
+	  break;
+	default:
+	  /* Other escape sequences */
+	  wpbuf_put (c);
+	  /* Just send the sequence */
+	  WriteConsoleA (get_output_handle (), wpbuf, wpixput, &wn, 0);
+	  break;
+	}
+      return;
+    }
+
+  /* For legacy cygwin treminal */
   switch (c)
     {
     case 'm':   /* Set Graphics Rendition */
@@ -2641,6 +2727,7 @@ fhandler_console::write_normal (const unsigned char *src,
   while (found < end
 	 && found - src < CONVERT_LIMIT
 	 && base_chars[*found] != IGN
+	 && base_chars[*found] != ESC
 	 && ((wincap.has_con_24bit_colors () && !con_is_legacy)
 	     || base_chars[*found] == NOR))
     {
@@ -2712,6 +2799,7 @@ do_print:
 	  break;
 	case ESC:
 	  con.state = gotesc;
+	  wpbuf_put (*found);
 	  break;
 	case DWN:
 	  cursor_get (&x, &y);
@@ -2826,6 +2914,7 @@ fhandler_console::write (const void *vsrc, size_t len)
 	case gotesc:
 	  if (*src == '[')		/* CSI Control Sequence Introducer */
 	    {
+	      wpbuf_put (*src);
 	      con.state = gotsquare;
 	      memset (con.args, 0, sizeof con.args);
 	      con.nargs = 0;
@@ -2833,18 +2922,55 @@ fhandler_console::write (const void *vsrc, size_t len)
 	      con.saw_greater_than_sign = false;
 	      con.saw_space = false;
 	    }
+	  else if (wincap.has_con_24bit_colors () && !con_is_legacy
+		   && wincap.has_con_broken_il_dl () && *src == 'M')
+	    { /* Reverse Index (scroll down) */
+	      int x, y;
+	      DWORD n;
+	      cursor_get (&x, &y);
+	      if (y == srTop)
+		{
+		  /* Erase scroll down area */
+		  char buf[] = "\033[32768;1H\033[J\033[32768;32768";
+		  __small_sprintf (buf, "\033[%d;1H\033[J\033[%d;%dH",
+			     srBottom - con.b.srWindow.Top + 1,
+			     y + 1 - con.b.srWindow.Top, x + 1);
+		  WriteConsoleA (get_output_handle (),
+				 buf, strlen (buf), &n, 0);
+		  /* Substitute "CSI Ps T" */
+		  wpbuf_put ('[');
+		  wpbuf_put ('T');
+		}
+	      else
+		wpbuf_put (*src);
+	      WriteConsoleA (get_output_handle (), wpbuf, wpixput, &n, 0);
+	      con.state = normal;
+	      wpixput = 0;
+	    }
+	  else if (wincap.has_con_24bit_colors () && !con_is_legacy)
+	    { /* Only CSI is handled in xterm compatible mode. */
+	      wpbuf_put (*src);
+	      /* Just send the sequence */
+	      DWORD n;
+	      WriteConsoleA (get_output_handle (), wpbuf, wpixput, &n, 0);
+	      con.state = normal;
+	      wpixput = 0;
+	    }
 	  else if (*src == ']')		/* OSC Operating System Command */
 	    {
+	      wpbuf_put (*src);
 	      con.rarg = 0;
 	      con.my_title_buf[0] = '\0';
 	      con.state = gotrsquare;
 	    }
 	  else if (*src == '(')		/* Designate G0 character set */
 	    {
+	      wpbuf_put (*src);
 	      con.state = gotparen;
 	    }
 	  else if (*src == ')')		/* Designate G1 character set */
 	    {
+	      wpbuf_put (*src);
 	      con.state = gotrparen;
 	    }
 	  else if (*src == 'M')		/* Reverse Index (scroll down) */
@@ -2852,6 +2978,7 @@ fhandler_console::write (const void *vsrc, size_t len)
 	      con.fillin (get_output_handle ());
 	      scroll_buffer_screen (0, 0, -1, -1, 0, 1);
 	      con.state = normal;
+	      wpixput = 0;
 	    }
 	  else if (*src == 'c')		/* RIS Full Reset */
 	    {
@@ -2862,22 +2989,29 @@ fhandler_console::write (const void *vsrc, size_t len)
 	      cursor_set (false, 0, 0);
 	      clear_screen (cl_buf_beg, cl_buf_beg, cl_buf_end, cl_buf_end);
 	      con.state = normal;
+	      wpixput = 0;
 	    }
 	  else if (*src == '8')		/* DECRC Restore cursor position */
 	    {
 	      cursor_set (false, con.savex, con.savey);
 	      con.state = normal;
+	      wpixput = 0;
 	    }
 	  else if (*src == '7')		/* DECSC Save cursor position */
 	    {
 	      cursor_get (&con.savex, &con.savey);
 	      con.state = normal;
+	      wpixput = 0;
 	    }
 	  else if (*src == 'R')		/* ? */
+	    {
 	      con.state = normal;
+	      wpixput = 0;
+	    }
 	  else
 	    {
 	      con.state = normal;
+	      wpixput = 0;
 	    }
 	  src++;
 	  break;
@@ -2885,10 +3019,12 @@ fhandler_console::write (const void *vsrc, size_t len)
 	  if (isdigit (*src))
 	    {
 	      con.args[con.nargs] = con.args[con.nargs] * 10 + *src - '0';
+	      wpbuf_put (*src);
 	      src++;
 	    }
 	  else if (*src == ';')
 	    {
+	      wpbuf_put (*src);
 	      src++;
 	      con.nargs++;
 	      if (con.nargs > MAXARGS)
@@ -2896,6 +3032,7 @@ fhandler_console::write (const void *vsrc, size_t len)
 	    }
 	  else if (*src == ' ')
 	    {
+	      wpbuf_put (*src);
 	      src++;
 	      con.saw_space = true;
 	      con.state = gotcommand;
@@ -2909,6 +3046,7 @@ fhandler_console::write (const void *vsrc, size_t len)
 	    con.nargs--;
 	  char_command (*src++);
 	  con.state = normal;
+	  wpixput = 0;
 	  break;
 	case gotrsquare:
 	  if (isdigit (*src))
@@ -2919,6 +3057,7 @@ fhandler_console::write (const void *vsrc, size_t len)
 	    con.state = eatpalette;
 	  else
 	    con.state = eattitle;
+	  wpbuf_put (*src);
 	  src++;
 	  break;
 	case eattitle:
@@ -2930,20 +3069,28 @@ fhandler_console::write (const void *vsrc, size_t len)
 		if (*src == '\007' && con.state == gettitle)
 		  set_console_title (con.my_title_buf);
 		con.state = normal;
+		wpixput = 0;
 	      }
 	    else if (n < TITLESIZE)
 	      {
 		con.my_title_buf[n++] = *src;
 		con.my_title_buf[n] = '\0';
+		wpbuf_put (*src);
 	      }
 	    src++;
 	    break;
 	  }
 	case eatpalette:
 	  if (*src == '\033')
-	    con.state = endpalette;
+	    {
+	      wpbuf_put (*src);
+	      con.state = endpalette;
+	    }
 	  else if (*src == '\a')
-	    con.state = normal;
+	    {
+	      con.state = normal;
+	      wpixput = 0;
+	    }
 	  src++;
 	  break;
 	case endpalette:
@@ -2952,12 +3099,14 @@ fhandler_console::write (const void *vsrc, size_t len)
 	  else
 	    /* Sequence error (abort) */
 	    con.state = normal;
+	  wpixput = 0;
 	  src++;
 	  break;
 	case gotsquare:
 	  if (*src == ';')
 	    {
 	      con.state = gotarg1;
+	      wpbuf_put (*src);
 	      con.nargs++;
 	      if (con.nargs > MAXARGS)
 		con.nargs--;
@@ -2971,6 +3120,7 @@ fhandler_console::write (const void *vsrc, size_t len)
 		con.saw_question_mark = true;
 	      else if (*src == '>')
 		con.saw_greater_than_sign = true;
+	      wpbuf_put (*src);
 	      /* ignore any extra chars between [ and first arg or command */
 	      src++;
 	    }
@@ -2983,6 +3133,7 @@ fhandler_console::write (const void *vsrc, size_t len)
 	  else
 	    con.vt100_graphics_mode_G0 = false;
 	  con.state = normal;
+	  wpixput = 0;
 	  src++;
 	  break;
 	case gotrparen:	/* Designate G1 Character Set (ISO 2022) */
@@ -2991,6 +3142,7 @@ fhandler_console::write (const void *vsrc, size_t len)
 	  else
 	    con.vt100_graphics_mode_G1 = false;
 	  con.state = normal;
+	  wpixput = 0;
 	  src++;
 	  break;
 	}
diff --git a/winsup/cygwin/wincap.cc b/winsup/cygwin/wincap.cc
index a52262b89..714a6d49f 100644
--- a/winsup/cygwin/wincap.cc
+++ b/winsup/cygwin/wincap.cc
@@ -43,6 +43,7 @@ wincaps wincap_vista __attribute__((section (".cygwin_dll_common"), shared)) = {
     no_msv1_0_s4u_logon_in_wow64:true,
     has_con_24bit_colors:false,
     has_con_broken_csi3j:false,
+    has_con_broken_il_dl:false,
   },
 };
 
@@ -71,6 +72,7 @@ wincaps wincap_7 __attribute__((section (".cygwin_dll_common"), shared)) = {
     no_msv1_0_s4u_logon_in_wow64:true,
     has_con_24bit_colors:false,
     has_con_broken_csi3j:false,
+    has_con_broken_il_dl:false,
   },
 };
 
@@ -99,6 +101,7 @@ wincaps wincap_8 __attribute__((section (".cygwin_dll_common"), shared)) = {
     no_msv1_0_s4u_logon_in_wow64:false,
     has_con_24bit_colors:false,
     has_con_broken_csi3j:false,
+    has_con_broken_il_dl:false,
   },
 };
 
@@ -127,6 +130,7 @@ wincaps wincap_8_1 __attribute__((section (".cygwin_dll_common"), shared)) = {
     no_msv1_0_s4u_logon_in_wow64:false,
     has_con_24bit_colors:false,
     has_con_broken_csi3j:false,
+    has_con_broken_il_dl:false,
   },
 };
 
@@ -155,6 +159,7 @@ wincaps  wincap_10_1507 __attribute__((section (".cygwin_dll_common"), shared))
     no_msv1_0_s4u_logon_in_wow64:false,
     has_con_24bit_colors:false,
     has_con_broken_csi3j:false,
+    has_con_broken_il_dl:false,
   },
 };
 
@@ -183,6 +188,7 @@ wincaps wincap_10_1703 __attribute__((section (".cygwin_dll_common"), shared)) =
     no_msv1_0_s4u_logon_in_wow64:false,
     has_con_24bit_colors:true,
     has_con_broken_csi3j:false,
+    has_con_broken_il_dl:false,
   },
 };
 
@@ -211,6 +217,7 @@ wincaps wincap_10_1709 __attribute__((section (".cygwin_dll_common"), shared)) =
     no_msv1_0_s4u_logon_in_wow64:false,
     has_con_24bit_colors:true,
     has_con_broken_csi3j:false,
+    has_con_broken_il_dl:false,
   },
 };
 
@@ -239,6 +246,7 @@ wincaps wincap_10_1803 __attribute__((section (".cygwin_dll_common"), shared)) =
     no_msv1_0_s4u_logon_in_wow64:false,
     has_con_24bit_colors:true,
     has_con_broken_csi3j:false,
+    has_con_broken_il_dl:false,
   },
 };
 
@@ -267,6 +275,7 @@ wincaps wincap_10_1809 __attribute__((section (".cygwin_dll_common"), shared)) =
     no_msv1_0_s4u_logon_in_wow64:false,
     has_con_24bit_colors:true,
     has_con_broken_csi3j:true,
+    has_con_broken_il_dl:false,
   },
 };
 
@@ -295,6 +304,7 @@ wincaps wincap_10_1903 __attribute__((section (".cygwin_dll_common"), shared)) =
     no_msv1_0_s4u_logon_in_wow64:false,
     has_con_24bit_colors:true,
     has_con_broken_csi3j:false,
+    has_con_broken_il_dl:true,
   },
 };
 
diff --git a/winsup/cygwin/wincap.h b/winsup/cygwin/wincap.h
index 11902d976..f85a88877 100644
--- a/winsup/cygwin/wincap.h
+++ b/winsup/cygwin/wincap.h
@@ -37,6 +37,7 @@ struct wincaps
     unsigned no_msv1_0_s4u_logon_in_wow64	: 1;
     unsigned has_con_24bit_colors		: 1;
     unsigned has_con_broken_csi3j		: 1;
+    unsigned has_con_broken_il_dl		: 1;
   };
 };
 
@@ -97,6 +98,7 @@ public:
   bool	IMPLEMENT (no_msv1_0_s4u_logon_in_wow64)
   bool	IMPLEMENT (has_con_24bit_colors)
   bool	IMPLEMENT (has_con_broken_csi3j)
+  bool	IMPLEMENT (has_con_broken_il_dl)
 
   void disable_case_sensitive_dirs ()
   {
-- 
2.21.0

[PATCH v2 0/4] Modify handling of several ESC sequences in xterm mode.

[Takashi Yano]

Takashi Yano (4):
  Cygwin: console: Add workaround for broken IL/DL in xterm mode.
  Cygwin: console: Unify workaround code for CSI3J and CSI?1049h/l.
  Cygwin: console: Add support for REP escape sequence to xterm mode.
  Cygwin: console: Add emulation of CSI3J on Win10 1809.

 winsup/cygwin/fhandler_console.cc | 247 +++++++++++++++++++++++++++---
 winsup/cygwin/wincap.cc           |  20 +++
 winsup/cygwin/wincap.h            |   4 +
 3 files changed, 248 insertions(+), 23 deletions(-)

-- 
2.21.0

[PATCH v2 2/4] Cygwin: console: Unify workaround code for CSI3J and CSI?1049h/l.

[Takashi Yano]

- This patch unifies workaround code for CSI3J and CSI?1049h/l into
  the code handling other escape sequences in xterm mode.
---
 winsup/cygwin/fhandler_console.cc | 43 ++++++++++++++++---------------
 1 file changed, 22 insertions(+), 21 deletions(-)

diff --git a/winsup/cygwin/fhandler_console.cc b/winsup/cygwin/fhandler_console.cc
index c2198ea1e..3bd1d27d1 100644
--- a/winsup/cygwin/fhandler_console.cc
+++ b/winsup/cygwin/fhandler_console.cc
@@ -1786,24 +1786,6 @@ static const wchar_t __vt100_conv[31] = {
 inline bool
 fhandler_console::write_console (PWCHAR buf, DWORD len, DWORD& done)
 {
-  bool need_fix_tab_position = false;
-  /* Check if screen will be alternated. */
-  if (wincap.has_con_24bit_colors () && !con_is_legacy
-      && memmem (buf, len*sizeof (WCHAR), L"\033[?1049", 7*sizeof (WCHAR)))
-    need_fix_tab_position = true;
-  /* Workaround for broken CSI3J (ESC[3J) support in xterm compatible mode. */
-  if (wincap.has_con_24bit_colors () && !con_is_legacy &&
-      wincap.has_con_broken_csi3j ())
-    {
-      WCHAR *p = buf;
-      while ((p = (WCHAR *) memmem (p, (len - (p - buf))*sizeof (WCHAR),
-				    L"\033[3J", 4*sizeof (WCHAR))))
-	{
-	  memmove (p, p+4, (len - (p+4 - buf))*sizeof (WCHAR));
-	  len -= 4;
-	}
-    }
-
   if (con.iso_2022_G1
 	? con.vt100_graphics_mode_G1
 	: con.vt100_graphics_mode_G0)
@@ -1822,9 +1804,6 @@ fhandler_console::write_console (PWCHAR buf, DWORD len, DWORD& done)
       len -= done;
       buf += done;
     }
-  /* Call fix_tab_position() if screen has been alternated. */
-  if (need_fix_tab_position)
-    fix_tab_position ();
   return true;
 }
 
@@ -2089,6 +2068,28 @@ fhandler_console::char_command (char c)
 	      WriteConsoleA (get_output_handle (), wpbuf, wpixput, &wn, 0);
 	    }
 	  break;
+	case 'J': /* ED */
+	  wpbuf_put (c);
+	  /* Ignore CSI3J in Win10 1809 because it is broken. */
+	  if (con.args[0] != 3 || !wincap.has_con_broken_csi3j ())
+	    WriteConsoleA (get_output_handle (), wpbuf, wpixput, &wn, 0);
+	  break;
+	case 'h': /* DECSET */
+	case 'l': /* DECRST */
+	  wpbuf_put (c);
+	  /* Just send the sequence */
+	  WriteConsoleA (get_output_handle (), wpbuf, wpixput, &wn, 0);
+	  if (con.saw_question_mark)
+	    {
+	      bool need_fix_tab_position = false;
+	      for (int i = 0; i < con.nargs; i++)
+		if (con.args[i] == 1049)
+		  need_fix_tab_position = true;
+	      /* Call fix_tab_position() if screen has been alternated. */
+	      if (need_fix_tab_position)
+		fix_tab_position ();
+	    }
+	  break;
 	default:
 	  /* Other escape sequences */
 	  wpbuf_put (c);
-- 
2.21.0

[PATCH v2 4/4] Cygwin: console: Add emulation of CSI3J on Win10 1809.

[Takashi Yano]

- This patch add emulation of CSI3J, which is broken in Win10 1809,
  rather than ignoring it as before.
---
 winsup/cygwin/fhandler_console.cc | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/winsup/cygwin/fhandler_console.cc b/winsup/cygwin/fhandler_console.cc
index b0951497a..7f2e8af5c 100644
--- a/winsup/cygwin/fhandler_console.cc
+++ b/winsup/cygwin/fhandler_console.cc
@@ -2102,8 +2102,23 @@ fhandler_console::char_command (char c)
 	  break;
 	case 'J': /* ED */
 	  wpbuf_put (c);
-	  /* Ignore CSI3J in Win10 1809 because it is broken. */
-	  if (con.args[0] != 3 || !wincap.has_con_broken_csi3j ())
+	  if (con.args[0] == 3 && wincap.has_con_broken_csi3j ())
+	    { /* Workaround for broken CSI3J in Win10 1809 */
+	      CONSOLE_SCREEN_BUFFER_INFO sbi;
+	      GetConsoleScreenBufferInfo (get_output_handle (), &sbi);
+	      SMALL_RECT r = {0, sbi.srWindow.Top,
+		(SHORT) (sbi.dwSize.X - 1), (SHORT) (sbi.dwSize.Y - 1)};
+	      CHAR_INFO f = {' ', sbi.wAttributes};
+	      COORD d = {0, 0};
+	      ScrollConsoleScreenBufferA (get_output_handle (),
+					  &r, NULL, d, &f);
+	      SetConsoleCursorPosition (get_output_handle (), d);
+	      d = sbi.dwCursorPosition;
+	      d.Y -= sbi.srWindow.Top;
+	      SetConsoleCursorPosition (get_output_handle (), d);
+	    }
+	  else
+	    /* Just send the sequence */
 	    WriteConsoleA (get_output_handle (), wpbuf, wpixput, &wn, 0);
 	  break;
 	case 'h': /* DECSET */
-- 
2.21.0

[PATCH v2 3/4] Cygwin: console: Add support for REP escape sequence to xterm mode.

[Takashi Yano]

- In Win10 upto 1809, xterm compatible mode does not have REP
  escape sequence which terminfo declares. This patch adds support
  for "CSI Ps b" (REP). With this patch, bvi (binary editor) works
  normally in Win10 1809. Also, xterm compatible mode does not have
  "CSI Pm `" (HPA), "CSI Pm a" (HPR) and "CSI Ps e" (VPR). However,
  they do not appear to be declared by terminfo. Therefore, these
  have been pending.
---
 winsup/cygwin/fhandler_console.cc | 33 +++++++++++++++++++++++++++++++
 winsup/cygwin/wincap.cc           | 10 ++++++++++
 winsup/cygwin/wincap.h            |  2 ++
 3 files changed, 45 insertions(+)

diff --git a/winsup/cygwin/fhandler_console.cc b/winsup/cygwin/fhandler_console.cc
index 3bd1d27d1..b0951497a 100644
--- a/winsup/cygwin/fhandler_console.cc
+++ b/winsup/cygwin/fhandler_console.cc
@@ -62,6 +62,7 @@ static struct fhandler_base::rabuf_t con_ra;
 #define WPBUF_LEN 256
 static unsigned char wpbuf[WPBUF_LEN];
 static int wpixput;
+static unsigned char last_char;
 #define wpbuf_put(x) \
   wpbuf[wpixput++] = x; \
   if (wpixput > WPBUF_LEN) \
@@ -2009,6 +2010,37 @@ fhandler_console::char_command (char c)
       DWORD wn;
       switch (c)
 	{
+#if 0 /* These sequences, which are supported by real xterm, are
+	 not supported by xterm compatible mode. Therefore they
+	 were implemented once. However, these are not declared
+	 in terminfo of xterm-256color, therefore, do not appear
+	 to be necessary. */
+	case '`': /* HPA */
+	  if (con.args[0] == 0)
+	    con.args[0] = 1;
+	  cursor_get (&x, &y);
+	  cursor_set (false, con.args[0]-1, y);
+	  break;
+	case 'a': /* HPR */
+	  if (con.args[0] == 0)
+	    con.args[0] = 1;
+	  cursor_rel (con.args[0], 0);
+	  break;
+	case 'e': /* VPR */
+	  if (con.args[0] == 0)
+	    con.args[0] = 1;
+	  cursor_rel (0, con.args[0]);
+	  break;
+#endif
+	case 'b': /* REP */
+	  wpbuf_put (c);
+	  if (wincap.has_con_esc_rep ())
+	    /* Just send the sequence */
+	    WriteConsoleA (get_output_handle (), wpbuf, wpixput, &wn, 0);
+	  else if (last_char && last_char != '\n')
+	    for (int i = 0; i < con.args[0]; i++)
+	      WriteConsoleA (get_output_handle (), &last_char, 1, &wn, 0);
+	  break;
 	case 'r': /* DECSTBM */
 	  con.scroll_region.Top = con.args[0] ? con.args[0] - 1 : 0;
 	  con.scroll_region.Bottom = con.args[1] ? con.args[1] - 1 : -1;
@@ -2746,6 +2778,7 @@ fhandler_console::write_normal (const unsigned char *src,
 	  break;
 	default:
 	  found += ret;
+	  last_char = *(found - 1);
 	  break;
 	}
     }
diff --git a/winsup/cygwin/wincap.cc b/winsup/cygwin/wincap.cc
index 714a6d49f..922705e65 100644
--- a/winsup/cygwin/wincap.cc
+++ b/winsup/cygwin/wincap.cc
@@ -44,6 +44,7 @@ wincaps wincap_vista __attribute__((section (".cygwin_dll_common"), shared)) = {
     has_con_24bit_colors:false,
     has_con_broken_csi3j:false,
     has_con_broken_il_dl:false,
+    has_con_esc_rep:false,
   },
 };
 
@@ -73,6 +74,7 @@ wincaps wincap_7 __attribute__((section (".cygwin_dll_common"), shared)) = {
     has_con_24bit_colors:false,
     has_con_broken_csi3j:false,
     has_con_broken_il_dl:false,
+    has_con_esc_rep:false,
   },
 };
 
@@ -102,6 +104,7 @@ wincaps wincap_8 __attribute__((section (".cygwin_dll_common"), shared)) = {
     has_con_24bit_colors:false,
     has_con_broken_csi3j:false,
     has_con_broken_il_dl:false,
+    has_con_esc_rep:false,
   },
 };
 
@@ -131,6 +134,7 @@ wincaps wincap_8_1 __attribute__((section (".cygwin_dll_common"), shared)) = {
     has_con_24bit_colors:false,
     has_con_broken_csi3j:false,
     has_con_broken_il_dl:false,
+    has_con_esc_rep:false,
   },
 };
 
@@ -160,6 +164,7 @@ wincaps  wincap_10_1507 __attribute__((section (".cygwin_dll_common"), shared))
     has_con_24bit_colors:false,
     has_con_broken_csi3j:false,
     has_con_broken_il_dl:false,
+    has_con_esc_rep:false,
   },
 };
 
@@ -189,6 +194,7 @@ wincaps wincap_10_1703 __attribute__((section (".cygwin_dll_common"), shared)) =
     has_con_24bit_colors:true,
     has_con_broken_csi3j:false,
     has_con_broken_il_dl:false,
+    has_con_esc_rep:false,
   },
 };
 
@@ -218,6 +224,7 @@ wincaps wincap_10_1709 __attribute__((section (".cygwin_dll_common"), shared)) =
     has_con_24bit_colors:true,
     has_con_broken_csi3j:false,
     has_con_broken_il_dl:false,
+    has_con_esc_rep:false,
   },
 };
 
@@ -247,6 +254,7 @@ wincaps wincap_10_1803 __attribute__((section (".cygwin_dll_common"), shared)) =
     has_con_24bit_colors:true,
     has_con_broken_csi3j:false,
     has_con_broken_il_dl:false,
+    has_con_esc_rep:false,
   },
 };
 
@@ -276,6 +284,7 @@ wincaps wincap_10_1809 __attribute__((section (".cygwin_dll_common"), shared)) =
     has_con_24bit_colors:true,
     has_con_broken_csi3j:true,
     has_con_broken_il_dl:false,
+    has_con_esc_rep:false,
   },
 };
 
@@ -305,6 +314,7 @@ wincaps wincap_10_1903 __attribute__((section (".cygwin_dll_common"), shared)) =
     has_con_24bit_colors:true,
     has_con_broken_csi3j:false,
     has_con_broken_il_dl:true,
+    has_con_esc_rep:true,
   },
 };
 
diff --git a/winsup/cygwin/wincap.h b/winsup/cygwin/wincap.h
index f85a88877..6d7a1eae6 100644
--- a/winsup/cygwin/wincap.h
+++ b/winsup/cygwin/wincap.h
@@ -38,6 +38,7 @@ struct wincaps
     unsigned has_con_24bit_colors		: 1;
     unsigned has_con_broken_csi3j		: 1;
     unsigned has_con_broken_il_dl		: 1;
+    unsigned has_con_esc_rep			: 1;
   };
 };
 
@@ -99,6 +100,7 @@ public:
   bool	IMPLEMENT (has_con_24bit_colors)
   bool	IMPLEMENT (has_con_broken_csi3j)
   bool	IMPLEMENT (has_con_broken_il_dl)
+  bool	IMPLEMENT (has_con_esc_rep)
 
   void disable_case_sensitive_dirs ()
   {
-- 
2.21.0

Re: [PATCH v2 0/4] Modify handling of several ESC sequences in xterm mode.

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 678 bytes --]

On Feb 27 00:32, Takashi Yano wrote:
> Takashi Yano (4):
>   Cygwin: console: Add workaround for broken IL/DL in xterm mode.
>   Cygwin: console: Unify workaround code for CSI3J and CSI?1049h/l.
>   Cygwin: console: Add support for REP escape sequence to xterm mode.
>   Cygwin: console: Add emulation of CSI3J on Win10 1809.
> 
>  winsup/cygwin/fhandler_console.cc | 247 +++++++++++++++++++++++++++---
>  winsup/cygwin/wincap.cc           |  20 +++
>  winsup/cygwin/wincap.h            |   4 +
>  3 files changed, 248 insertions(+), 23 deletions(-)
> 
> -- 
> 2.21.0

Looks good to me.  Pushed.


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.

[Jon Turney]

On 26/02/2020 15:32, Takashi Yano wrote:
> - Cygwin console with xterm compatible mode causes problem reported
>    in https://www.cygwin.com/ml/cygwin-patches/2020-q1/msg00212.html
>    if background/foreground colors are set to gray/black respectively
>    in Win10 1903/1909. This is caused by "CSI Ps L" (IL), "CSI Ps M"
>    (DL) and "ESC M" (RI) control sequences which are broken. This
>    patch adds a workaround for the issue.
> ---
>   winsup/cygwin/fhandler_console.cc | 156 +++++++++++++++++++++++++++++-
>   winsup/cygwin/wincap.cc           |  10 ++
>   winsup/cygwin/wincap.h            |   2 +
>   3 files changed, 166 insertions(+), 2 deletions(-)
> 
> diff --git a/winsup/cygwin/fhandler_console.cc b/winsup/cygwin/fhandler_console.cc
> index 328424a7d..c2198ea1e 100644
> --- a/winsup/cygwin/fhandler_console.cc
> +++ b/winsup/cygwin/fhandler_console.cc
> @@ -57,6 +57,16 @@ bool NO_COPY fhandler_console::invisible_console;
>      Only one console can exist in a process, therefore, static is suitable. */
>   static struct fhandler_base::rabuf_t con_ra;
>   
> +/* Write pending buffer for ESC sequence handling
> +   in xterm compatible mode */
> +#define WPBUF_LEN 256
> +static unsigned char wpbuf[WPBUF_LEN];
> +static int wpixput;
> +#define wpbuf_put(x) \
> +  wpbuf[wpixput++] = x; \
> +  if (wpixput > WPBUF_LEN) \
> +    wpixput--;
> +
>   static void
>   beep ()
[...]
> +		}
> +	      else
> +		wpbuf_put (*src);
> +	      WriteConsoleA (get_output_handle (), wpbuf, wpixput, &n, 0);
> +	      con.state = normal;
> +	      wpixput = 0;
> +	    }

This generates a (useful!) warning with gcc 9.2.0:

> ../../../../winsup/cygwin/fhandler_console.cc: In member function 'virtual ssize_t fhandler_console::write(const void*, size_t)':
> ../../../../winsup/cygwin/fhandler_console.cc:67:3: error: macro expands to multiple statements [-Werror=multistatement-macros]
>    67 |   wpbuf[wpixput++] = x; \
>       |   ^~~~~
> ../../../../winsup/cygwin/fhandler_console.cc:67:3: note: in definition of macro 'wpbuf_put'
>    67 |   wpbuf[wpixput++] = x; \
>       |   ^~~~~
> ../../../../winsup/cygwin/fhandler_console.cc:2993:8: note: some parts of macro expansion are not guarded by this 'else' clause
>  2993 |        else
>       |        ^~~~

So I think either the macro need it contents contained by a 'do { ... } 
while(0)',  or that instance of it needs to be surrounded by braces, to 
do what you intend.

Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.

[Takashi Yano]

On Thu, 27 Feb 2020 18:03:47 +0000
Jon Turney wrote:
> > +#define wpbuf_put(x) \
> > +  wpbuf[wpixput++] = x; \
> > +  if (wpixput > WPBUF_LEN) \
> > +    wpixput--;
> > +
> 
> So I think either the macro need it contents contained by a 'do { ... } 
> while(0)',  or that instance of it needs to be surrounded by braces, to 
> do what you intend.

Thanks for the advice. Fortunately, "if" statement does not
cause a problem even if it is accidentally executed outside
"else" block in this case.

Hans,
as for making a patch for this issue, may I leave it to you
because you are already working on it? 

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 808 bytes --]

[CC Hans]

On Feb 28 11:14, Takashi Yano wrote:
> On Thu, 27 Feb 2020 18:03:47 +0000
> Jon Turney wrote:
> > > +#define wpbuf_put(x) \
> > > +  wpbuf[wpixput++] = x; \
> > > +  if (wpixput > WPBUF_LEN) \
> > > +    wpixput--;
> > > +
> > 
> > So I think either the macro need it contents contained by a 'do { ... } 
> > while(0)',  or that instance of it needs to be surrounded by braces, to 
> > do what you intend.
> 
> Thanks for the advice. Fortunately, "if" statement does not
> cause a problem even if it is accidentally executed outside
> "else" block in this case.
> 
> Hans,
> as for making a patch for this issue, may I leave it to you
> because you are already working on it? 
> 
> -- 
> Takashi Yano <takashi.yano@nifty.ne.jp>

-- 
Corinna Vinschen
Cygwin Maintainer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1572 bytes --]

On Feb 28 14:31, Corinna Vinschen wrote:
> [CC Hans]
> 
> On Feb 28 11:14, Takashi Yano wrote:
> > On Thu, 27 Feb 2020 18:03:47 +0000
> > Jon Turney wrote:
> > > > +#define wpbuf_put(x) \
> > > > +  wpbuf[wpixput++] = x; \
> > > > +  if (wpixput > WPBUF_LEN) \
> > > > +    wpixput--;
> > > > +
> > > 
> > > So I think either the macro need it contents contained by a 'do { ... } 
> > > while(0)',  or that instance of it needs to be surrounded by braces, to 
> > > do what you intend.
> > 
> > Thanks for the advice. Fortunately, "if" statement does not
> > cause a problem even if it is accidentally executed outside
> > "else" block in this case.
> > 
> > Hans,
> > as for making a patch for this issue, may I leave it to you
> > because you are already working on it? 
> > 
> > -- 
> > Takashi Yano <takashi.yano@nifty.ne.jp>

What about an inline function instead?

diff --git a/winsup/cygwin/fhandler_console.cc b/winsup/cygwin/fhandler_console.cc
index 64e12b8320a1..6c3e33818aca 100644
--- a/winsup/cygwin/fhandler_console.cc
+++ b/winsup/cygwin/fhandler_console.cc
@@ -63,10 +63,14 @@ static struct fhandler_base::rabuf_t con_ra;
 static unsigned char wpbuf[WPBUF_LEN];
 static int wpixput;
 static unsigned char last_char;
-#define wpbuf_put(x) \
-  wpbuf[wpixput++] = x; \
-  if (wpixput > WPBUF_LEN) \
+
+static inline void
+wpbuf_put (unsigned char x)
+{
+  wpbuf[wpixput++] = x;
+  if (wpixput > WPBUF_LEN)
     wpixput--;
+}
 
 static void
 beep ()


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1813 bytes --]

On Feb 28 15:44, Corinna Vinschen wrote:
> On Feb 28 14:31, Corinna Vinschen wrote:
> > [CC Hans]
> > 
> > On Feb 28 11:14, Takashi Yano wrote:
> > > On Thu, 27 Feb 2020 18:03:47 +0000
> > > Jon Turney wrote:
> > > > > +#define wpbuf_put(x) \
> > > > > +  wpbuf[wpixput++] = x; \
> > > > > +  if (wpixput > WPBUF_LEN) \
> > > > > +    wpixput--;
> > > > > +
> > > > 
> > > > So I think either the macro need it contents contained by a 'do { ... } 
> > > > while(0)',  or that instance of it needs to be surrounded by braces, to 
> > > > do what you intend.
> > > 
> > > Thanks for the advice. Fortunately, "if" statement does not
> > > cause a problem even if it is accidentally executed outside
> > > "else" block in this case.
> > > 
> > > Hans,
> > > as for making a patch for this issue, may I leave it to you
> > > because you are already working on it? 
> > > 
> > > -- 
> > > Takashi Yano <takashi.yano@nifty.ne.jp>
> 
> What about an inline function instead?
> 
> diff --git a/winsup/cygwin/fhandler_console.cc b/winsup/cygwin/fhandler_console.cc
> index 64e12b8320a1..6c3e33818aca 100644
> --- a/winsup/cygwin/fhandler_console.cc
> +++ b/winsup/cygwin/fhandler_console.cc
> @@ -63,10 +63,14 @@ static struct fhandler_base::rabuf_t con_ra;
>  static unsigned char wpbuf[WPBUF_LEN];
>  static int wpixput;
>  static unsigned char last_char;
> -#define wpbuf_put(x) \
> -  wpbuf[wpixput++] = x; \
> -  if (wpixput > WPBUF_LEN) \
> +
> +static inline void
> +wpbuf_put (unsigned char x)
> +{
> +  wpbuf[wpixput++] = x;
> +  if (wpixput > WPBUF_LEN)
>      wpixput--;
> +}

Also, on second thought, given wpbuf is global inside this file, doesn't
this require guarding against multi-threaded access?


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.

[Takashi Yano]

On Fri, 28 Feb 2020 15:49:05 +0100
Corinna Vinschen wrote:
> Also, on second thought, given wpbuf is global inside this file, doesn't
> this require guarding against multi-threaded access?

wpbuf_put() is used in write(), and almost whole of write()
code is guarded by output_mutex. So, I think it is already
thread-safe.

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 463 bytes --]

On Feb 29 02:06, Takashi Yano wrote:
> On Fri, 28 Feb 2020 15:49:05 +0100
> Corinna Vinschen wrote:
> > Also, on second thought, given wpbuf is global inside this file, doesn't
> > this require guarding against multi-threaded access?
> 
> wpbuf_put() is used in write(), and almost whole of write()
> code is guarded by output_mutex. So, I think it is already
> thread-safe.

Ah, right, thanks.


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.

[Hans-Bernhard Bröker]

Am 28.02.2020 um 14:31 schrieb Corinna Vinschen:
> [CC Hans]

Thanks.  I wasn't subscribed to -patches so far.  Will change that.

>> Hans,
>> as for making a patch for this issue, may I leave it to you
>> because you are already working on it?

My patch was meant only as a minimally-invasive stop-gap fix, because 
the new GCC refused to compile the code as-is (it triggers a 
-Werror...).  As such, sorry for hurting Brian's eyes. ;-)

I agree that this really should be an inline function.  I barely speak 
C++, but even I have glimpsed that multi-statement macros are rightfully 
frowned upon in C++ circles.

I believe a more C++-like implementation would have to encapsulate wpbuf 
and wpixput into a helper class.  This is what my _very_ rusty C++ 
allowed me to come up with:

// simple helper class to accumulate output in a buffer
// and send that to the console on request:
static class
{
private:
   unsigned char buf[WPBUF_LEN];
   int ixput;

public:
   inline void put(unsigned char x)
   {
     if (ixput < WPBUF_LEN)
       {
         buf[ixput++] = x;
       }
   };
   inline void empty() { ixput = 0; };
   inline void sendOut(HANDLE &handle, DWORD *wn) {
     WriteConsoleA (handle, buf, ixput, wn, 0);
   };
} wpbuf;

Using it works like this:

s/wpbuf_put/wpbuf.put/
s/wpixput = 0/wpbuf.empty()/
s/WriteConsoleA ( get_output_handle (), wpbuf, wpixput, \(&w*n\), 0 
/wpbuf.sendOut( get_output_handle (), \1/g

Yes, sendOut() is ugly --- I didn't manage to find out how this class 
could access get_output_handle() itself, so I had to let its callers 
deal with that.

Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.

[Hans-Bernhard Bröker]

One more important note: the current implementation has a potential 
buffer overrun issue, because it writes first, and only then checks 
whether that may have overrun the buffer.  And the check itself is off 
by one, too:

>    wpbuf[wpixput++] = x; \
>    if (wpixput > WPBUF_LEN) \
>     wpixput--; \

That's why my latest code snippet does it differently:

 >      if (ixput < WPBUF_LEN)
 >        {
 >          buf[ixput++] = x;
 >        }

Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.

[Takashi Yano]

[-- Attachment #1: Type: text/plain, Size: 1052 bytes --]

On Fri, 28 Feb 2020 22:00:40 +0100
Hans-Bernhard Bröker wrote:
> // simple helper class to accumulate output in a buffer
> // and send that to the console on request:
> static class
> {
> private:
>    unsigned char buf[WPBUF_LEN];
>    int ixput;
> 
> public:
>    inline void put(unsigned char x)
>    {
>      if (ixput < WPBUF_LEN)
>        {
>          buf[ixput++] = x;
>        }
>    };
>    inline void empty() { ixput = 0; };
>    inline void sendOut(HANDLE &handle, DWORD *wn) {
>      WriteConsoleA (handle, buf, ixput, wn, 0);
>    };
> } wpbuf;

I agree your solution is more C++-like and smart.
However, from the view point of performance, just inline
static function is better. Attached code measures the
performance of access speed for wpbuf.
I compiled it by g++ 7.4.0 with -O2 option.

The result is as follows.

Total1: 2.315627 second
Total2: 1.588511 second
Total3: 1.571572 second

Class implementation is slow 40% than inline or macro.
So, IMHO, inline static function is the best.

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

[-- Attachment #2: wpbuf-bench.cc --]
[-- Type: text/x-c++src, Size: 1474 bytes --]

#include <time.h>
#include <stdio.h>

#define WPBUF_LEN 256

class {
private:
	unsigned char buf[WPBUF_LEN];
	int ixput;

public:
	inline void put(unsigned char x)
	{
		if (ixput < WPBUF_LEN) buf[ixput++] = x;
	}
	inline void empty() { ixput = 0; };
} wpbuf;

unsigned char wpbuf2[WPBUF_LEN];
int ixput;
inline void wpbuf2_put(unsigned char x)
{
	if (ixput < WPBUF_LEN) wpbuf2[ixput++] = x;
}

#define wpbuf3_put(x) \
{ \
	if (ixput < WPBUF_LEN) wpbuf2[ixput++] = x; \
}

void bench1()
{
	for (int i=0; i<10000000; i++) {
		wpbuf.empty();
		for (int j=0; j<WPBUF_LEN; j++) {
			wpbuf.put('A');
		}
	}
}

void bench2()
{
	for (int i=0; i<10000000; i++) {
		ixput = 0;
		for (int j=0; j<WPBUF_LEN; j++) {
			wpbuf2_put('A');
		}
	}
}

void bench3()
{
	for (int i=0; i<10000000; i++) {
		ixput = 0;
		for (int j=0; j<WPBUF_LEN; j++) {
			wpbuf3_put('A');
		}
	}
}

int main()
{
	struct timespec tv0, tv1;
	
	clock_gettime(CLOCK_MONOTONIC, &tv0);
	bench1();
	clock_gettime(CLOCK_MONOTONIC, &tv1);
	printf("Total1: %f second\n",
		(tv1.tv_sec - tv0.tv_sec) + (tv1.tv_nsec - tv0.tv_nsec)*1e-9);

	clock_gettime(CLOCK_MONOTONIC, &tv0);
	bench2();
	clock_gettime(CLOCK_MONOTONIC, &tv1);
	printf("Total2: %f second\n",
		(tv1.tv_sec - tv0.tv_sec) + (tv1.tv_nsec - tv0.tv_nsec)*1e-9);

	clock_gettime(CLOCK_MONOTONIC, &tv0);
	bench3();
	clock_gettime(CLOCK_MONOTONIC, &tv1);
	printf("Total3: %f second\n",
		(tv1.tv_sec - tv0.tv_sec) + (tv1.tv_nsec - tv0.tv_nsec)*1e-9);
	return 0;
}

Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.

[Takashi Yano]

Hi Hans,

On Sat, 29 Feb 2020 19:10:02 +0100
Hans-Bernhard Bröker wrote:
> One more important note: the current implementation has a potential 
> buffer overrun issue, because it writes first, and only then checks 
> whether that may have overrun the buffer.  And the check itself is off 
> by one, too:
> 
> >    wpbuf[wpixput++] = x; \
> >    if (wpixput > WPBUF_LEN) \
> >     wpixput--; \
> 
> That's why my latest code snippet does it differently:
> 
>  >      if (ixput < WPBUF_LEN)
>  >        {
>  >          buf[ixput++] = x;
>  >        }

Indeed. You are right. Thanks for pointing out that.
Another similar problem exists in console code of escape
sequence handling, so I will submit a patch for that.

As for wpbuf, please continue to fix.

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.

[Hans-Bernhard Bröker]

Am 01.03.2020 um 07:33 schrieb Takashi Yano:

> However, from the view point of performance, just inline
> static function is better. 

I don't see how that could be the case.  Inline methods of a static C++ 
object should not suffer any perfomance penalty compared to inline 
functions operating on static variables.

> Attached code measures the
> performance of access speed for wpbuf.
> I compiled it by g++ 7.4.0 with -O2 option.
> 
> The result is as follows.
> 
> Total1: 2.315627 second
> Total2: 1.588511 second
> Total3: 1.571572 second

Strange.  The result here (with GCC 9.2) is rather different:

$ g++ -O2 -o tt wpbuf-bench.cc && ./tt
Total1: 0.753815 second
Total2: 0.757444 second
Total3: 1.217352 second

And on inspection, all three bench*() functions do appear to have 
exactly the same machine code, too.  They may be inlined and mixed into 
main() somewhat differently, though.  That might explain the difference 
more readily than any actual difference in speed between the three 
implementations.

Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.

[Takashi Yano]

On Sun, 1 Mar 2020 14:56:31 +0100
Hans-Bernhard Bröker wrote:
> Am 01.03.2020 um 07:33 schrieb Takashi Yano:
> 
> > However, from the view point of performance, just inline
> > static function is better. 
> 
> I don't see how that could be the case.  Inline methods of a static C++ 
> object should not suffer any perfomance penalty compared to inline 
> functions operating on static variables.
> 
> > Attached code measures the
> > performance of access speed for wpbuf.
> > I compiled it by g++ 7.4.0 with -O2 option.
> > 
> > The result is as follows.
> > 
> > Total1: 2.315627 second
> > Total2: 1.588511 second
> > Total3: 1.571572 second
> 
> Strange.  The result here (with GCC 9.2) is rather different:
> 
> $ g++ -O2 -o tt wpbuf-bench.cc && ./tt
> Total1: 0.753815 second
> Total2: 0.757444 second
> Total3: 1.217352 second
> 
> And on inspection, all three bench*() functions do appear to have 
> exactly the same machine code, too.  They may be inlined and mixed into 
> main() somewhat differently, though.  That might explain the difference 
> more readily than any actual difference in speed between the three 
> implementations.

I looked into the code generated by g++ 7.4.0 with -O2. The codes
generated are different.

With 32bit compiler,

bench1():
L3:
    cmpl    $255, %edx
    jg  L2
    movb    $65, _wpbuf(%edx)
    movl    $1, %ecx
    addl    $1, %edx
L2:
    subl    $1, %eax
    [...]

bench2(), bench3():
L22:
    cmpl    $255, %edx
    jg  L21
    movb    $65, _wpbuf2(%edx)
    addl    $1, %edx
L21:
    subl    $1, %eax
    [...]

With 64bit compiler,

bench1():
.L3:
    cmpl    $255, %edx
    jg  .L2
    movslq  %edx, %rcx
    addl    $1, %edx
    movb    $65, (%r8,%rcx)
    movl    $1, %ecx
.L2:
    subl    $1, %eax
    [...]

bench2(), bench3():
.L15:
    cmpl    $255, %edx
    jg  .L14
    movslq  %edx, %rcx
    addl    $1, %edx
    movb    $65, (%r8,%rcx)
.L14:
    subl    $1, %eax
    [...]

Obviously, code for bench2() and bench3() is shorter than
bench1().

However, with g++ 9.2.0 with -O2,

bench1(), bench2(), bench3():
L3:
    cmpl    $255, %edx
    jg  L2
    movb    $65, _wpbuf(%edx)
    addl    $1, %edx
L2:
    subl    $1, %eax
    [...]

all the codes are exactly the same, as you mentioned.

So, if we assume g++ 9.2.0, please forget the previous remarks
about speed.

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1138 bytes --]

On Mar  1 15:38, Takashi Yano wrote:
> Hi Hans,
> 
> On Sat, 29 Feb 2020 19:10:02 +0100
> Hans-Bernhard Bröker wrote:
> > One more important note: the current implementation has a potential 
> > buffer overrun issue, because it writes first, and only then checks 
> > whether that may have overrun the buffer.  And the check itself is off 
> > by one, too:
> > 
> > >    wpbuf[wpixput++] = x; \
> > >    if (wpixput > WPBUF_LEN) \
> > >     wpixput--; \
> > 
> > That's why my latest code snippet does it differently:
> > 
> >  >      if (ixput < WPBUF_LEN)
> >  >        {
> >  >          buf[ixput++] = x;
> >  >        }
> 
> Indeed. You are right. Thanks for pointing out that.
> Another similar problem exists in console code of escape
> sequence handling, so I will submit a patch for that.
> 
> As for wpbuf, please continue to fix.

Yeah, a patch in `git format-patch' format would be most welcome.

For a first-time contribution (and then never again) we also need
a 2-clause BSD license waiver per https://cygwin.com/contrib.html


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1337 bytes --]

On Mar  2 18:03, Corinna Vinschen wrote:
> On Mar  1 15:38, Takashi Yano wrote:
> > Hi Hans,
> > 
> > On Sat, 29 Feb 2020 19:10:02 +0100
> > Hans-Bernhard Bröker wrote:
> > > One more important note: the current implementation has a potential 
> > > buffer overrun issue, because it writes first, and only then checks 
> > > whether that may have overrun the buffer.  And the check itself is off 
> > > by one, too:
> > > 
> > > >    wpbuf[wpixput++] = x; \
> > > >    if (wpixput > WPBUF_LEN) \
> > > >     wpixput--; \
> > > 
> > > That's why my latest code snippet does it differently:
> > > 
> > >  >      if (ixput < WPBUF_LEN)
> > >  >        {
> > >  >          buf[ixput++] = x;
> > >  >        }
> > 
> > Indeed. You are right. Thanks for pointing out that.
> > Another similar problem exists in console code of escape
> > sequence handling, so I will submit a patch for that.
> > 
> > As for wpbuf, please continue to fix.
> 
> Yeah, a patch in `git format-patch' format would be most welcome.
> 
> For a first-time contribution (and then never again) we also need
> a 2-clause BSD license waiver per https://cygwin.com/contrib.html

I pushed wpbuf_put as a simple inline function as a stop-gap
measure so Cygwin builds on gcc 9.2.0.


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.

[Jon Turney]

On 02/03/2020 19:54, Corinna Vinschen wrote:
> On Mar  2 18:03, Corinna Vinschen wrote:
>> On Mar  1 15:38, Takashi Yano wrote:
>>> Hi Hans,
[...]
> 
> I pushed wpbuf_put as a simple inline function as a stop-gap
> measure so Cygwin builds on gcc 9.2.0.

\o/

https://ci.appveyor.com/project/cygwin/cygwin/builds/31190427