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

classic Classic list List threaded Threaded
22 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[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

Reply | Threaded
Open this post in threaded view
|

[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

Reply | Threaded
Open this post in threaded view
|

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

Takashi Yano
In reply to this post by 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

Reply | Threaded
Open this post in threaded view
|

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

Takashi Yano
In reply to this post by 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

Reply | Threaded
Open this post in threaded view
|

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

Takashi Yano
In reply to this post by 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

Reply | Threaded
Open this post in threaded view
|

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

Corinna Vinschen-2
In reply to this post by Takashi Yano
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Jon TURNEY
In reply to this post by Takashi Yano
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.

Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]>
Reply | Threaded
Open this post in threaded view
|

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

Corinna Vinschen-2
[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 <[hidden email]>
--
Corinna Vinschen
Cygwin Maintainer

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Corinna Vinschen-2
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 <[hidden email]>
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Corinna Vinschen-2
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 <[hidden email]>
>
> 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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]>
Reply | Threaded
Open this post in threaded view
|

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

Corinna Vinschen-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Hans-Bernhard Bröker
In reply to this post by Corinna Vinschen-2
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.
Reply | Threaded
Open this post in threaded view
|

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;
 >        }
Reply | Threaded
Open this post in threaded view
|

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

Takashi Yano
In reply to this post by Hans-Bernhard Bröker
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 <[hidden email]>

wpbuf-bench.cc (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Takashi Yano
In reply to this post by Hans-Bernhard Bröker
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 <[hidden email]>
Reply | Threaded
Open this post in threaded view
|

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

Hans-Bernhard Bröker
In reply to this post by Takashi Yano
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.
Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]>
Reply | Threaded
Open this post in threaded view
|

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

Corinna Vinschen-2
In reply to this post by Takashi Yano
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

signature.asc (849 bytes) Download Attachment
12