[PATCH] console UTF-8 fixes

[PATCH] console UTF-8 fixes

[Egmont Koblinger]

Hi folks,

I send a patch to the UTF-8 part of the vt driver. I know that this code has
recently been updated to be better than it had been previously, but it still
suffers from plenty of bugs. My patch addresses all the issues known by me.
The difference in the behavior can easily be seen by trying either Markus
Kuhn's UTF-8 stress-test, or editing a multi-lingual file (such as
applications' .desktop files), I believe that my re-write does not only fix
several bugs but also yields better usability of the console as well as
cleaner source code.

Here is the description of the bugs or misfeatures that I fix:

- If a certain (otherwise valid UTF-8) character is not found in the glyph
  table, the current code does one of these two (depending on other
  circumstances):

  - Either it displays the replacement character U+FFFD, falling back to a
    simple question mark. Note that the Unicode replacement character U+FFFD
    is to be used for invalid sequences. However, it shouldn't necessarily
    be used when replacing a valid but undisplayable character. Think of
    Pango for example that renders these as four hex digits inside a square.
    To be able to visually distinguish between illegal sequences and legal
    but undisplayable characters, I think U+FFFD or the question mark are
    bad choices. In fact, any symbol that may normally occur in the text is
    a bad choice if is displayed simply. Hence I chose to display an
    inverted dot.

  - Another possible thing the current code may do (for latin1-compatible
    characters) is to simply display the glyph loaded in that position.
    Suppose I have loaded a latin2 font. In latin2, 0xFB is an "u with
    double accent". An applications prints U+00FB, which is an "u with
    circumflex". Since this glyph is not present in latin2, it cannot be
    printed with the current font. Still, the current code falls back to
    printing the glyph from the 0xFB position of the glyph table. Hence my
    app asked to print "u with circumflex" but an "u with double accent"
    appears on the screen. This is totally contrary to the goals of Unicode
    and shouldn't ever happen.

- The replacement character for invalid UTF-8 sequences is U+FFFD, falling
  back to a question mark. I've changed the fallback version to an inverted
  question mark. This way it's more similar to the common glyph of U+FFFD,
  and it's more trivial to the user that it's not a literal question mark
  but rather some erroneous situation.

- Overlong sequences are not caught currently, they're displayed as if these
  were valid representations. This may even have security impacts.

- Lone continuation bytes (section 3.1 of the UTF-8 stress test) are
  currently displayed as some "random" glyphs rather than the replacement
  character.

- Incomplete sequences (sections 3.2 and 3.3) emit no replacement character,
  but rather cause the subsequent valid character to be displayed more
  times(!).

- There's no concept of double-width characters. It's way beyond the scope
  of my patch to try to display them, but at least I think it's important
  for the cursor to jump two positions when printing such characters, since
  this is what applications (such as text editors) expect. If the cursor
  didn't jump two positions, applications would suffer from displaying and
  refreshing problems, and editing some English letters that are preceded by
  some CJK characters in the same line became a nightmare. With my patch an
  inverted dot followed by an inverted space is displayed for double-width
  characters so it's quite easy to see that they are tied together.

- There's no concept of zero-width characters (such as combining accents)
  either. Yet again it's beyond the scope of my patch to properly handle
  them. Instead of the current behavior (write a replacement character) I
  just ignore them so that full-screen applications can keep track of the
  cursor position correctly.

- I believe (at least I do hope) that my code is cleaner, more
  straightforward, easier to understand, and is slightly better documented
  than the current version. The current code doesn't separate UTF-8 decoding
  and glyph displaying parts. I clearly separated them. First I perform
  UTF-8 decoding (this emits U+FFFD for invalid sequences), then check for
  the width of the resulting character, change it to U+FFFD if it's
  unprintable (e.g. an UTF-16 surrogate), and finally comes the part that
  does its best in displaying the character on the screen.

I hope you like it. :)


cheers,

Egmont





diff -Naur linux-2.6.20.orig/drivers/char/consolemap.c linux-2.6.20/drivers/char/consolemap.c
--- linux-2.6.20.orig/drivers/char/consolemap.c	2007-02-04 19:44:54.000000000 +0100
+++ linux-2.6.20/drivers/char/consolemap.c	2007-04-06 17:28:05.000000000 +0200
@@ -627,10 +627,8 @@
 	/* Only 16-bit codes supported at this time */
 	if (ucs > 0xffff)
 		ucs = 0xfffd;		/* U+FFFD: REPLACEMENT CHARACTER */
-	else if (ucs < 0x20 || ucs >= 0xfffe)
+	else if (ucs < 0x20)
 		return -1;		/* Not a printable character */
-	else if (ucs == 0xfeff || (ucs >= 0x200a && ucs <= 0x200f))
-		return -2;			/* Zero-width space */
 	/*
 	 * UNI_DIRECT_BASE indicates the start of the region in the User Zone
 	 * which always has a 1:1 mapping to the currently loaded font.  The
diff -Naur linux-2.6.20.orig/drivers/char/vt.c linux-2.6.20/drivers/char/vt.c
--- linux-2.6.20.orig/drivers/char/vt.c	2007-02-04 19:44:54.000000000 +0100
+++ linux-2.6.20/drivers/char/vt.c	2007-04-06 20:13:09.000000000 +0200
@@ -70,6 +70,16 @@
  * malformed UTF sequences represented as sequences of replacement glyphs,
  * original codes or '?' as a last resort if replacement glyph is undefined
  * by Adam Tla/lka <atlka@pg.gda.pl>, Aug 2006
+ *
+ * More robust UTF-8 decoder. Make it work on malformed sequences as Markus Kuhn's
+ * UTF-8 decoder stress test suggests. Emit a U+FFFD on illegal sequences as well
+ * as for invalid Unicode code points.
+ * If U+FFFD is not available in the font, print an inverse question mark instead.
+ * Display an inverted dot for valid characters that are not available in the font.
+ * Do not print zero-width characters, pad double-width characters with an extra
+ * space so that the cursor moves by zero/two positions in these cases.
+ * 6 April 2007, Egmont Koblinger <egmont@uhulinux.hu>,
+ * using Markus Kuhn's wcwidth() implementation.
  */
 
 #include <linux/module.h>
@@ -1934,6 +1944,109 @@
 char con_buf[CON_BUF_SIZE];
 DECLARE_MUTEX(con_buf_sem);
 
+/* wcwidth() based on the implementation by
+ * Markus Kuhn -- 2003-05-20 (Unicode 4.0)
+ * Latest version: http://www.cl.cam.ac.uk/~mgk25/ucs/wcwidth.c
+ */
+struct interval {
+  int first;
+  int last;
+};
+
+static int bisearch(long ucs, const struct interval *table, int max) {
+  int min = 0;
+  int mid;
+
+  if (ucs < table[0].first || ucs > table[max].last)
+    return 0;
+  while (max >= min) {
+    mid = (min + max) / 2;
+    if (ucs > table[mid].last)
+      min = mid + 1;
+    else if (ucs < table[mid].first)
+      max = mid - 1;
+    else
+      return 1;
+  }
+
+  return 0;
+}
+
+static int wcwidth(long ucs)
+{
+  static const struct interval unprintable[] = {
+    { 0xD800, 0xDFFF }, { 0xFFFE, 0xFFFF }
+  };
+  static const struct interval zero_width[] = {
+    { 0x0300, 0x0357 }, { 0x035D, 0x036F }, { 0x0483, 0x0486 },
+    { 0x0488, 0x0489 }, { 0x0591, 0x05A1 }, { 0x05A3, 0x05B9 },
+    { 0x05BB, 0x05BD }, { 0x05BF, 0x05BF }, { 0x05C1, 0x05C2 },
+    { 0x05C4, 0x05C4 }, { 0x0600, 0x0603 }, { 0x0610, 0x0615 },
+    { 0x064B, 0x0658 }, { 0x0670, 0x0670 }, { 0x06D6, 0x06E4 },
+    { 0x06E7, 0x06E8 }, { 0x06EA, 0x06ED }, { 0x070F, 0x070F },
+    { 0x0711, 0x0711 }, { 0x0730, 0x074A }, { 0x07A6, 0x07B0 },
+    { 0x0901, 0x0902 }, { 0x093C, 0x093C }, { 0x0941, 0x0948 },
+    { 0x094D, 0x094D }, { 0x0951, 0x0954 }, { 0x0962, 0x0963 },
+    { 0x0981, 0x0981 }, { 0x09BC, 0x09BC }, { 0x09C1, 0x09C4 },
+    { 0x09CD, 0x09CD }, { 0x09E2, 0x09E3 }, { 0x0A01, 0x0A02 },
+    { 0x0A3C, 0x0A3C }, { 0x0A41, 0x0A42 }, { 0x0A47, 0x0A48 },
+    { 0x0A4B, 0x0A4D }, { 0x0A70, 0x0A71 }, { 0x0A81, 0x0A82 },
+    { 0x0ABC, 0x0ABC }, { 0x0AC1, 0x0AC5 }, { 0x0AC7, 0x0AC8 },
+    { 0x0ACD, 0x0ACD }, { 0x0AE2, 0x0AE3 }, { 0x0B01, 0x0B01 },
+    { 0x0B3C, 0x0B3C }, { 0x0B3F, 0x0B3F }, { 0x0B41, 0x0B43 },
+    { 0x0B4D, 0x0B4D }, { 0x0B56, 0x0B56 }, { 0x0B82, 0x0B82 },
+    { 0x0BC0, 0x0BC0 }, { 0x0BCD, 0x0BCD }, { 0x0C3E, 0x0C40 },
+    { 0x0C46, 0x0C48 }, { 0x0C4A, 0x0C4D }, { 0x0C55, 0x0C56 },
+    { 0x0CBC, 0x0CBC }, { 0x0CBF, 0x0CBF }, { 0x0CC6, 0x0CC6 },
+    { 0x0CCC, 0x0CCD }, { 0x0D41, 0x0D43 }, { 0x0D4D, 0x0D4D },
+    { 0x0DCA, 0x0DCA }, { 0x0DD2, 0x0DD4 }, { 0x0DD6, 0x0DD6 },
+    { 0x0E31, 0x0E31 }, { 0x0E34, 0x0E3A }, { 0x0E47, 0x0E4E },
+    { 0x0EB1, 0x0EB1 }, { 0x0EB4, 0x0EB9 }, { 0x0EBB, 0x0EBC },
+    { 0x0EC8, 0x0ECD }, { 0x0F18, 0x0F19 }, { 0x0F35, 0x0F35 },
+    { 0x0F37, 0x0F37 }, { 0x0F39, 0x0F39 }, { 0x0F71, 0x0F7E },
+    { 0x0F80, 0x0F84 }, { 0x0F86, 0x0F87 }, { 0x0F90, 0x0F97 },
+    { 0x0F99, 0x0FBC }, { 0x0FC6, 0x0FC6 }, { 0x102D, 0x1030 },
+    { 0x1032, 0x1032 }, { 0x1036, 0x1037 }, { 0x1039, 0x1039 },
+    { 0x1058, 0x1059 }, { 0x1160, 0x11FF }, { 0x1712, 0x1714 },
+    { 0x1732, 0x1734 }, { 0x1752, 0x1753 }, { 0x1772, 0x1773 },
+    { 0x17B4, 0x17B5 }, { 0x17B7, 0x17BD }, { 0x17C6, 0x17C6 },
+    { 0x17C9, 0x17D3 }, { 0x17DD, 0x17DD }, { 0x180B, 0x180D },
+    { 0x18A9, 0x18A9 }, { 0x1920, 0x1922 }, { 0x1927, 0x1928 },
+    { 0x1932, 0x1932 }, { 0x1939, 0x193B }, { 0x200B, 0x200F },
+    { 0x202A, 0x202E }, { 0x2060, 0x2063 }, { 0x206A, 0x206F },
+    { 0x20D0, 0x20EA }, { 0x302A, 0x302F }, { 0x3099, 0x309A },
+    { 0xFB1E, 0xFB1E }, { 0xFE00, 0xFE0F }, { 0xFE20, 0xFE23 },
+    { 0xFEFF, 0xFEFF }, { 0xFFF9, 0xFFFB }, { 0x1D167, 0x1D169 },
+    { 0x1D173, 0x1D182 }, { 0x1D185, 0x1D18B }, { 0x1D1AA, 0x1D1AD },
+    { 0xE0001, 0xE0001 }, { 0xE0020, 0xE007F }, { 0xE0100, 0xE01EF }
+  };
+  static const struct interval double_width[] = {
+    { 0x1100, 0x115F }, { 0x2329, 0x232A }, { 0x2E80, 0x303E },
+    { 0x3040, 0xA4CF }, { 0xAC00, 0xD7A3 }, { 0xF900, 0xFAFF },
+    { 0xFE30, 0xFE6F }, { 0xFF00, 0xFF60 }, { 0xFFE0, 0xFFE6 },
+    { 0x20000, 0x2FFFD }, { 0x30000, 0x3FFFD }
+  };
+
+  if (ucs == 0)
+    return 0;
+  if (ucs < 0x20 || (ucs >= 0x7F && ucs < 0xA0))
+    return -1;
+  if (ucs < 0x0300)
+    return 1;
+
+  if (bisearch(ucs, unprintable,
+	       sizeof(unprintable) / sizeof(struct interval) - 1))
+    return -1;
+  if (bisearch(ucs, zero_width,
+	       sizeof(zero_width) / sizeof(struct interval) - 1))
+    return 0;
+  if (bisearch(ucs, double_width,
+	       sizeof(double_width) / sizeof(struct interval) - 1))
+    return 2;
+
+  return 1;
+}
+
 /* acquires console_sem */
 static int do_con_write(struct tty_struct *tty, const unsigned char *buf, int count)
 {
@@ -1950,6 +2063,10 @@
 	unsigned int currcons;
 	unsigned long draw_from = 0, draw_to = 0;
 	struct vc_data *vc;
+	unsigned char vc_attr;
+	int inverse;
+	int width;
+	int rescan;
 	u16 himask, charmask;
 	const unsigned char *orig_buf = NULL;
 	int orig_count;
@@ -2012,51 +2129,80 @@
 		buf++;
 		n++;
 		count--;
+		inverse = 0;
+		width = 1;
+		rescan = 0;
 
 		/* Do no translation at all in control states */
 		if (vc->vc_state != ESnormal) {
 			tc = c;
 		} else if (vc->vc_utf && !vc->vc_disp_ctrl) {
-		    /* Combine UTF-8 into Unicode */
-		    /* Malformed sequences as sequences of replacement glyphs */
+		    /* Combine UTF-8 into Unicode in vc_utf_char */
+		    /* vc_utf_count is the number of continuation bytes still expected to arrive */
+		    /* vc_npar is the number of continuation bytes arrived so far */
 rescan_last_byte:
-		    if(c > 0x7f) {
+		    inverse = 0;
+		    rescan = 0;
+		    if ((c & 0xc0) == 0x80) {
+			/* Continuation byte */
+			static const int utf8_length_changes[] = { 0x0000007f, 0x000007ff, 0x0000ffff, 0x001fffff, 0x03ffffff, 0x7fffffff };
 			if (vc->vc_utf_count) {
-			       if ((c & 0xc0) == 0x80) {
-				       vc->vc_utf_char = (vc->vc_utf_char << 6) | (c & 0x3f);
-       				       if (--vc->vc_utf_count) {
-					       vc->vc_npar++;
-				   	       continue;
-       				       }
-				       tc = c = vc->vc_utf_char;
-			       } else
-				       goto replacement_glyph;
-			} else {
-				vc->vc_npar = 0;
-				if ((c & 0xe0) == 0xc0) {
-				    vc->vc_utf_count = 1;
-				    vc->vc_utf_char = (c & 0x1f);
-				} else if ((c & 0xf0) == 0xe0) {
-				    vc->vc_utf_count = 2;
-				    vc->vc_utf_char = (c & 0x0f);
-				} else if ((c & 0xf8) == 0xf0) {
-				    vc->vc_utf_count = 3;
-				    vc->vc_utf_char = (c & 0x07);
-				} else if ((c & 0xfc) == 0xf8) {
-				    vc->vc_utf_count = 4;
-				    vc->vc_utf_char = (c & 0x03);
-				} else if ((c & 0xfe) == 0xfc) {
-				    vc->vc_utf_count = 5;
-				    vc->vc_utf_char = (c & 0x01);
-				} else
-	    			    goto replacement_glyph;
+			    vc->vc_utf_char = (vc->vc_utf_char << 6) | (c & 0x3f);
+			    vc->vc_npar++;
+			    if (--vc->vc_utf_count) {
+				/* Still need some bytes */
 				continue;
-			      }
+			    }
+			    /* Got a whole character */
+			    c = vc->vc_utf_char;
+			    /* Reject overlong sequences */
+			    if (c <= utf8_length_changes[vc->vc_npar - 1] || c > utf8_length_changes[vc->vc_npar]) {
+				vc->vc_utf_count = 0;
+				c = 0xfffd;
+			    }
+			} else {
+			    /* Unexpected continuation byte */
+			    vc->vc_utf_count = 0;
+			    c = 0xfffd;
+			}
 		    } else {
-		      if (vc->vc_utf_count)
-	  		      goto replacement_glyph;
-		      tc = c;
+			/* Single ASCII byte or first byte of a sequence */
+			if (vc->vc_utf_count) {
+			    /* Continuation byte expected */
+			    rescan = 1;
+			    vc->vc_utf_count = 0;
+			    c = 0xfffd;
+			} else if (c > 0x7f) {
+			    /* First byte of a sequence */
+			    vc->vc_npar = 0;
+			    if ((c & 0xe0) == 0xc0) {
+				vc->vc_utf_count = 1;
+				vc->vc_utf_char = (c & 0x1f);
+			    } else if ((c & 0xf0) == 0xe0) {
+				vc->vc_utf_count = 2;
+				vc->vc_utf_char = (c & 0x0f);
+			    } else if ((c & 0xf8) == 0xf0) {
+				vc->vc_utf_count = 3;
+				vc->vc_utf_char = (c & 0x07);
+			    } else if ((c & 0xfc) == 0xf8) {
+				vc->vc_utf_count = 4;
+				vc->vc_utf_char = (c & 0x03);
+			    } else if ((c & 0xfe) == 0xfc) {
+				vc->vc_utf_count = 5;
+				vc->vc_utf_char = (c & 0x01);
+			    } else {
+				/* 254 and 255 are invalid */
+				c = 0xfffd;
+			    }
+			    if (vc->vc_utf_count) {
+				/* Still need some bytes */
+				continue;
+			    }
+			}
+			/* Nothing to do if an ASCII byte was received */
 		    }
+		    /* End of UTF-8 decoding */
+		    tc = c;
 		} else {	/* no utf or alternate charset mode */
 		  tc = vc->vc_translate[vc->vc_toggle_meta ? (c | 0x80) : c];
 		}
@@ -2078,56 +2224,83 @@
 			&& (c != 128+27);
 
 		if (vc->vc_state == ESnormal && ok) {
+			if (vc->vc_utf && !vc->vc_disp_ctrl) {
+				width = wcwidth(c);
+				if (width == 0) {
+					/* Nothing to display */
+					continue;
+				} else if (width < 0) {
+					/* Replace unprintable or invalid Unicode characters */
+					tc = c = 0xfffd;
+					width = 1;
+				}
+			}
 			/* Now try to find out how to display it */
 			tc = conv_uni_to_pc(vc, tc);
 			if (tc & ~charmask) {
-				if ( tc == -4 ) {
-                                /* If we got -4 (not found) then see if we have
-                                   defined a replacement character (U+FFFD) */
-replacement_glyph:
-                                	tc = conv_uni_to_pc(vc, 0xfffd);
-					if (!(tc & ~charmask))
-						goto display_glyph;
-                        	} else if ( tc != -3 )
-                                	continue; /* nothing to display */
-                                /* no hash table or no replacement --
-				 * hope for the best */
-				if ( c & ~charmask )
-					tc = '?';
-				else
-					tc = c;
+				if ( tc == -3 ) {
+				    continue; /* nothing to display */
+				}
+				/* Glyph not found */
+				if (!(vc->vc_utf && !vc->vc_disp_ctrl) && !(c & ~charmask)) {
+				    /* In legacy mode use the glyph we get by a 1:1 mapping.
+				       This would make absolutely no sense with Unicode in mind. */
+				    tc = c;
+				} else {
+				    /* Display an inverse dot,
+				       except for 0xfffd which is displayed as
+				       an inverse question mark. */
+				    inverse = 1;
+				    tc = conv_uni_to_pc(vc, c == 0xfffd ? '?' : '.');
+				    if (tc < 0) tc = (c == 0xfffd ? '?' : '.');
+				}
 			}
 
-display_glyph:
-			if (vc->vc_need_wrap || vc->vc_decim)
-				FLUSH
-			if (vc->vc_need_wrap) {
-				cr(vc);
-				lf(vc);
-			}
-			if (vc->vc_decim)
-				insert_char(vc, 1);
-			scr_writew(himask ?
-				     ((vc->vc_attr << 8) & ~himask) + ((tc & 0x100) ? himask : 0) + (tc & 0xff) :
-				     (vc->vc_attr << 8) + tc,
-				   (u16 *) vc->vc_pos);
-			if (DO_UPDATE(vc) && draw_x < 0) {
-				draw_x = vc->vc_x;
-				draw_from = vc->vc_pos;
-			}
-			if (vc->vc_x == vc->vc_cols - 1) {
-				vc->vc_need_wrap = vc->vc_decawm;
-				draw_to = vc->vc_pos + 2;
+			if (!inverse) {
+				vc_attr = vc->vc_attr;
 			} else {
-				vc->vc_x++;
-				draw_to = (vc->vc_pos += 2);
+				/* invert vc_attr */
+				if (!vc->vc_can_do_color) {
+					vc_attr = (vc->vc_attr) ^ 0x08;
+				} else if (vc->vc_hi_font_mask == 0x100) {
+					vc_attr = ((vc->vc_attr) & 0x11) | (((vc->vc_attr) & 0xe0) >> 4) | (((vc->vc_attr) & 0x0e) << 4);
+				} else {
+					vc_attr = ((vc->vc_attr) & 0x88) | (((vc->vc_attr) & 0x70) >> 4) | (((vc->vc_attr) & 0x07) << 4);
+				}
 			}
-			if (vc->vc_utf_count) {
-				if (vc->vc_npar) {
-					vc->vc_npar--;
-					goto display_glyph;
+
+			while (1) {
+				if (vc->vc_need_wrap || vc->vc_decim)
+					FLUSH
+				if (vc->vc_need_wrap) {
+					cr(vc);
+					lf(vc);
 				}
-				vc->vc_utf_count = 0;
+				if (vc->vc_decim)
+					insert_char(vc, 1);
+				scr_writew(himask ?
+					     ((vc_attr << 8) & ~himask) + ((tc & 0x100) ? himask : 0) + (tc & 0xff) :
+					     (vc_attr << 8) + tc,
+					   (u16 *) vc->vc_pos);
+				if (DO_UPDATE(vc) && draw_x < 0) {
+					draw_x = vc->vc_x;
+					draw_from = vc->vc_pos;
+				}
+				if (vc->vc_x == vc->vc_cols - 1) {
+					vc->vc_need_wrap = vc->vc_decawm;
+					draw_to = vc->vc_pos + 2;
+				} else {
+					vc->vc_x++;
+					draw_to = (vc->vc_pos += 2);
+				}
+
+				if (!--width) break;
+
+				tc = conv_uni_to_pc(vc, ' '); /* A space is printed in the second column */
+				if (tc < 0) tc = ' ';
+			}
+
+			if (rescan) {
 				c = orig;
 				goto rescan_last_byte;
 			}

Re: [PATCH] console UTF-8 fixes

[H. Peter Anvin]

Egmont Koblinger wrote:
> 
> - If a certain (otherwise valid UTF-8) character is not found in the glyph
>   table, the current code does one of these two (depending on other
>   circumstances):
> 
>   - Either it displays the replacement character U+FFFD, falling back to a
>     simple question mark. Note that the Unicode replacement character U+FFFD
>     is to be used for invalid sequences. However, it shouldn't necessarily
>     be used when replacing a valid but undisplayable character. Think of
>     Pango for example that renders these as four hex digits inside a square.
>     To be able to visually distinguish between illegal sequences and legal
>     but undisplayable characters, I think U+FFFD or the question mark are
>     bad choices. In fact, any symbol that may normally occur in the text is
>     a bad choice if is displayed simply. Hence I chose to display an
>     inverted dot.
> 

I strongly disagree.  First of all, you're changing the semantics of a 
13-year-old API.  The semantics of the Linux console is that by 
specifying U+FFFD SUBSTITUTION GLYPH in your unicode table, you have 
specified the fallback glyph.

What's worse, you've hard-coded the uses of specific visual 
representations.  That is completely unacceptable.

>   - Another possible thing the current code may do (for latin1-compatible
>     characters) is to simply display the glyph loaded in that position.
>     Suppose I have loaded a latin2 font. In latin2, 0xFB is an "u with
>     double accent". An applications prints U+00FB, which is an "u with
>     circumflex". Since this glyph is not present in latin2, it cannot be
>     printed with the current font. Still, the current code falls back to
>     printing the glyph from the 0xFB position of the glyph table. Hence my
>     app asked to print "u with circumflex" but an "u with double accent"
>     appears on the screen. This is totally contrary to the goals of Unicode
>     and shouldn't ever happen.

When does that happen?  That is clearly a bug.

> - The replacement character for invalid UTF-8 sequences is U+FFFD, falling
>   back to a question mark. I've changed the fallback version to an inverted
>   question mark. This way it's more similar to the common glyph of U+FFFD,
>   and it's more trivial to the user that it's not a literal question mark
>   but rather some erroneous situation.

Brilliant.  You've picked a fallback glyph which is unlikely to exist in 
all fonts.  The whole point of falling back to ? is that it's an ASCII 
character, which means that if the font designer failed to designate a 
fallback glyph -- which is an error!!! -- there is at least some hope of 
conveying the error back to the user.

> - Overlong sequences are not caught currently, they're displayed as if these
>   were valid representations. This may even have security impacts.
> 
> - Lone continuation bytes (section 3.1 of the UTF-8 stress test) are
>   currently displayed as some "random" glyphs rather than the replacement
>   character.
> 
> - Incomplete sequences (sections 3.2 and 3.3) emit no replacement character,
>   but rather cause the subsequent valid character to be displayed more
>   times(!).

These are valid issues.

> - There's no concept of double-width characters. It's way beyond the scope
>   of my patch to try to display them, but at least I think it's important
>   for the cursor to jump two positions when printing such characters, since
>   this is what applications (such as text editors) expect. If the cursor
>   didn't jump two positions, applications would suffer from displaying and
>   refreshing problems, and editing some English letters that are preceded by
>   some CJK characters in the same line became a nightmare. With my patch an
>   inverted dot followed by an inverted space is displayed for double-width
>   characters so it's quite easy to see that they are tied together.

To be able to do CJK you need something like Kon anyway.  This feels 
like bloat.

> - There's no concept of zero-width characters (such as combining accents)
>   either. Yet again it's beyond the scope of my patch to properly handle
>   them. Instead of the current behavior (write a replacement character) I
>   just ignore them so that full-screen applications can keep track of the
>   cursor position correctly.

There is a concept of combining sequences.  Anything else, I suspect 
it's better to let the user know that something bad is happening.

> - I believe (at least I do hope) that my code is cleaner, more
>   straightforward, easier to understand, and is slightly better documented
>   than the current version. The current code doesn't separate UTF-8 decoding
>   and glyph displaying parts. I clearly separated them. First I perform
>   UTF-8 decoding (this emits U+FFFD for invalid sequences), then check for
>   the width of the resulting character, change it to U+FFFD if it's
>   unprintable (e.g. an UTF-16 surrogate), and finally comes the part that
>   does its best in displaying the character on the screen.
> 
> I hope you like it. :)

Please see above comments.

	-hpa

Re: [PATCH] console UTF-8 fixes

[Egmont Koblinger]

On Fri, Apr 06, 2007 at 12:43:03PM -0700, H. Peter Anvin wrote:

Hi,

> I strongly disagree.  First of all, you're changing the semantics of a 
> 13-year-old API.  The semantics of the Linux console is that by 
> specifying U+FFFD SUBSTITUTION GLYPH in your unicode table, you have 
> specified the fallback glyph.

OK, I'm not against using U+FFFD for missing glyphs. In the mean time I
think it's still a good idea to clearly separate the two cases in the code
(that is, the case of invalid sequence from the case of missing glyph), but
we can still use the same replacement character in these two cases. I'll
send an updated patch after Easter if it sounds good for you.


> What's worse, you've hard-coded the uses of specific visual 
> representations.  That is completely unacceptable.

Now that we've dropped the idea of "dot" for missing glyphs, the other thing
that remains is the hard-coded '?' if and only if U+FFFD is not present in
the font. It is even hardcoded in the current code and I have no better
idea, there must be a last-resort hardcoded fallback. The only thing I
changed is that I inverted the color attributes for this question mark. Do
you think that the old behavior, a normal question mark would be better? No
problem, I'll adjust the code in this case. Just please tell me what the
expected behavior is, I'm not sure I clearly understand your thoughts.


> >  - Another possible thing the current code may do (for latin1-compatible
> >    characters) is to simply display the glyph loaded in that position.
> >    Suppose I have loaded a latin2 font. In latin2, 0xFB is an "u with
> >    double accent". An applications prints U+00FB, which is an "u with
> >    circumflex". Since this glyph is not present in latin2, it cannot be
> >    printed with the current font. Still, the current code falls back to
> >    printing the glyph from the 0xFB position of the glyph table. Hence my
> >    app asked to print "u with circumflex" but an "u with double accent"
> >    appears on the screen. This is totally contrary to the goals of Unicode
> >    and shouldn't ever happen.
> 
> When does that happen?  That is clearly a bug.

I think I've (mostly) described it above. Set everything to UTF-8, load a
latin2 font (containing 256 glyphs, e.g. "setfont lat2-16"), make an
application print U+00FB (alt + numpad 251 is one trivial way), you'll see
an "u with double accent", though the symbol to be displayed is "u with
circumflex". This isn't present in the current font, so the replacement
character should appear, not a different letter.


> >- The replacement character for invalid UTF-8 sequences is U+FFFD, falling
> >  back to a question mark. I've changed the fallback version to an inverted
> >  question mark. This way it's more similar to the common glyph of U+FFFD,
> >  and it's more trivial to the user that it's not a literal question mark
> >  but rather some erroneous situation.
> 
> Brilliant.  You've picked a fallback glyph which is unlikely to exist in 
> all fonts.  The whole point of falling back to ? is that it's an ASCII 
> character, which means that if the font designer failed to designate a 
> fallback glyph -- which is an error!!! -- there is at least some hope of 
> conveying the error back to the user.

Sorry, I wasn't clear enough and I think you misunderstood me. The symbol I
choose for fallback is still '?' (the ASCII question mark), I just invert
the color attributes of the cell where this is printed. This way it becomes
visually distinguisable from the literal question mark. Using the current
kernel you just cannot know whether the character printed is a real question
mark, or a replacement glyph. Still, should you stongly disagree with this
decision, the color inverting part can easily be removed.

> >- There's no concept of double-width characters. It's way beyond the scope
> >  of my patch to try to display them, but at least I think it's important
> >  for the cursor to jump two positions when printing such characters, since
> >  this is what applications (such as text editors) expect. If the cursor
> >  didn't jump two positions, applications would suffer from displaying and
> >  refreshing problems, and editing some English letters that are preceded 
> >  by
> >  some CJK characters in the same line became a nightmare. With my patch an
> >  inverted dot followed by an inverted space is displayed for double-width
> >  characters so it's quite easy to see that they are tied together.
> 
> To be able to do CJK you need something like Kon anyway.  This feels 
> like bloat.

I don't want CJK support. All that I want is to be able to edit English
words within a file that contains mixture of English and CJK, with a text
editor like vim or joe. Just try it with the current kernel, and with my
patch. Suppose that within a line some CJK text is followed by an English
word, and you want to edit the latter one. It's going to be a huge headache
with the current kernel. Where you see the cursor is not where the text
editor thinks it is. You try to delete a letter and actually another letter
gets deleted, for example.

The only thing my patch does here is that it inserts an extra space to
guarantee that the cursor goes to the correct position, where applications
expect it to be. I can't see any reason why this behavior could be worse
than the current one. Is there any?

> >- There's no concept of zero-width characters (such as combining accents)
> >  either. Yet again it's beyond the scope of my patch to properly handle
> >  them. Instead of the current behavior (write a replacement character) I
> >  just ignore them so that full-screen applications can keep track of the
> >  cursor position correctly.
> 
> There is a concept of combining sequences.  Anything else, I suspect 
> it's better to let the user know that something bad is happening.

There are two choices. Either let the user know that something bad is
happening, and then he'll face the same cursor-mispositioning issues in his
editor that we've just discussed with CJK, or we silently drop them and text
editing will work correctly. It's a harder decision than CJK, I can see pros
and cons for both approaches. I accept if you say the former one is the
desirable way; but still I'd prefer the source to be structured in a way
that it's easy to change (doesn't need complete rewrite, only a two-line
patch) if someone wants the other behaviour. Actually all you have to do is
that if wcwidth() returns 0 then pretend it was 1, instead of skipping the
whole glyph displaying part. Please confirm that the desirable way is to
print the glyph (or replacement) instead of ignoring these characters, I'll
adjust the patch.


> >I hope you like it. :)
> 
> Please see above comments.

Rewording: I hope that after some discussion we'll come to a patch that you
like :-)))))


-- 
Egmont

Re: [PATCH] console UTF-8 fixes

[Jan Engelhardt]

Hi,


I just wanted to give my opinion on things...

(and enable utf8 to read this properly)

On Apr 7 2007 11:24, Egmont Koblinger wrote:
>
>> I strongly disagree.  First of all, you're changing the semantics of a 
>> 13-year-old API.  The semantics of the Linux console is that by 
>> specifying U+FFFD SUBSTITUTION GLYPH in your unicode table, you have 
>> specified the fallback glyph.
>
>OK, I'm not against using U+FFFD for missing glyphs. In the mean time I
>think it's still a good idea to clearly separate the two cases in the code
>(that is, the case of invalid sequence from the case of missing glyph), but
>we can still use the same replacement character in these two cases. I'll
>send an updated patch after Easter if it sounds good for you.

I am quite ok with the way things are right now.

 - vc displays <?> for illegal sequences

 - vc displays e.g. "U" (latin capital U) in place when Û (latin capital
   U with accent circumflex) is not available in this font 
   (determined by the unicodemap) (I do use an unicode map, because I
   use a 4096-byte cp437 "DOS" font which requires one)

 - vc displays <?> for sequences it does not know how to print

 - xterm displays <?> for illegal sequences

 - xterm seems to display <?> on undefined glyphs (U+DFFF for ex.,
   using the "Unicode Best" font from the xterm menu)

 - xterm seems to display nothing on undefined glyphs (U+E000 for ex.,
   "Unicode Best" again)

>> What's worse, you've hard-coded the uses of specific visual 
>> representations.  That is completely unacceptable.
>
>Now that we've dropped the idea of "dot" for missing glyphs, the other thing
>
>[...]
>
>Sorry, I wasn't clear enough and I think you misunderstood me. The symbol I
>choose for fallback is still '?' (the ASCII question mark), I just invert
>the color attributes of the cell where this is printed. This way it becomes
>visually distinguisable from the literal question mark. Using the current
>kernel you just cannot know whether the character printed is a real question
>mark, or a replacement glyph. Still, should you stongly disagree with this
>decision, the color inverting part can easily be removed.

Please, no dot, and no inverse color.
Imagine someone had the following bitmap for <unknown glyph/illegal sequence>:

################
################
################
####........####
##....####....##
##....####....##
########....####
######....######
######....######
################
######....######
######....######
################
################
################
################

Then inverting that again would be susceptible to confusion with
the regular '?' at 0x3F. 

(cp437 for example maps unknown/illegal to 0xFD which happens to be the
block graphic '■', but YMMV depending on font.)

>I think I've (mostly) described it above. Set everything to UTF-8, load a
>latin2 font (containing 256 glyphs, e.g. "setfont lat2-16"), make an
>application print U+00FB (alt + numpad 251 is one trivial way), you'll see
>an "u with double accent", though the symbol to be displayed is "u with
>circumflex". This isn't present in the current font, so the replacement
>character should appear, not a different letter.

I blame your latin2 unicode map. (See above about 'Û'.)
It should perhaps display a regular 'u' if it cannot display 'û',
but definitely not 'ü' (which is not called a double accent, btw).

>> To be able to do CJK you need something like Kon anyway.  This feels 
>> like bloat.
>
>I don't want CJK support. All that I want is to be able to edit English
>words within a file that contains mixture of English and CJK, with a text
>editor like vim or joe.

+1 for this one :)

xterm## echo "韓国と日本にようこそ!" >/tmp/foobar.txt
vc## cat foobar.txt

currently gets things not so right, because multibyte characters are not
displayed with as many <?> as they are wide.


Jan
-- 

Re: [PATCH] console UTF-8 fixes

[Egmont Koblinger]

On Sat, Apr 07, 2007 at 01:00:48PM +0200, Jan Engelhardt wrote:

Hi,

> Please, no dot, and no inverse color.
> Imagine someone had the following bitmap for <unknown glyph/illegal sequence>:

No dot, I'm already convinced. To clarify the inverse thingy:

This is what the current kernel does:
  1) tries to display the desired symbol
  2) if it fails, tries to display U+FFFD (which usually looks similar to an
     inverted question mark)
  3) if this fails again then displays a normal '?'
     (or a different symbol due to a bug discussed below)

Here's my proposal. This only alters the 3rd step, not the first two:
  1) tries to display the desired symbol
  2) if it fails, tries to display U+FFFD, still with _normal_ attributes
  3) if this fails then display an ascii '?' with inverted attributes

So you won't get "double" inversion. If you do have U+FFFD in your font then
this will introduce no chance. If you don't have U+FFFD, you'll see inverse
question marks instead of normal ones.


> I blame your latin2 unicode map. (See above about 'Û'.)

There's nothing wrong with my latin2 unicode map, and I've located and
changed the part _in the kernel_ that displays a false glyph using the
algorithm I've outlined. It just uses "the glyph at that code position
within the glyph table" as a fallback, which might be okay in 8-bit mode
(and I haven't modified the behavior in that case), but I got rid of this
behavior in UTF-8 mode since it's definitely a fault in the world of
Unicode.

> It should perhaps display a regular 'u' if it cannot display 'û',

I rather think it should display U+FFFD but YMMV.

> but definitely not 'ü' (which is not called a double accent, btw).

This is not the character I've been talking about, I actually _did_ talk
about u with double acute accent (ű - you might not have seen this character
so far, AFAIK it's only used in Hungarian, no other languages). But we agree
that the kernel definitely shouldn't display a character with a different
accent on it. This is one of the bugs my patch addresses.


bye,

Egmont

Re: [PATCH] console UTF-8 fixes

[H. Peter Anvin]

Egmont Koblinger wrote:
> On Sat, Apr 07, 2007 at 01:00:48PM +0200, Jan Engelhardt wrote:
> 
> Hi,
> 
>> Please, no dot, and no inverse color.
>> Imagine someone had the following bitmap for <unknown glyph/illegal sequence>:
> 
> No dot, I'm already convinced. To clarify the inverse thingy:
> 
> This is what the current kernel does:
>   1) tries to display the desired symbol
>   2) if it fails, tries to display U+FFFD (which usually looks similar to an
>      inverted question mark)
>   3) if this fails again then displays a normal '?'
>      (or a different symbol due to a bug discussed below)
> 
> Here's my proposal. This only alters the 3rd step, not the first two:
>   1) tries to display the desired symbol
>   2) if it fails, tries to display U+FFFD, still with _normal_ attributes
>   3) if this fails then display an ascii '?' with inverted attributes
> 
> So you won't get "double" inversion. If you do have U+FFFD in your font then
> this will introduce no chance. If you don't have U+FFFD, you'll see inverse
> question marks instead of normal ones.
> 

This seems fine.

> 
>> I blame your latin2 unicode map. (See above about 'Û'.)
> 
> There's nothing wrong with my latin2 unicode map, and I've located and
> changed the part _in the kernel_ that displays a false glyph using the
> algorithm I've outlined. It just uses "the glyph at that code position
> within the glyph table" as a fallback, which might be okay in 8-bit mode
> (and I haven't modified the behavior in that case), but I got rid of this
> behavior in UTF-8 mode since it's definitely a fault in the world of
> Unicode.
> 
>> It should perhaps display a regular 'u' if it cannot display 'û',
> 
> I rather think it should display U+FFFD but YMMV.

That's a policy decision for the maker of the Unicode map.  The kernel 
cannot by default know that a pre-composed ű is a modified u; obviously, 
if the ű is send in decomposed form the kernel probably will display it 
as u? or some such.

>> but definitely not 'ü' (which is not called a double accent, btw).
> 
> This is not the character I've been talking about, I actually _did_ talk
> about u with double acute accent (ű - you might not have seen this character
> so far, AFAIK it's only used in Hungarian, no other languages). But we agree
> that the kernel definitely shouldn't display a character with a different
> accent on it. This is one of the bugs my patch addresses.

As far as width handling -- in order to make all the text line up under 
all circumstances you need more than width handling.  The wcwidth() 
stuff is specific to CJK -- a character set which is totally implausible 
to display on the builtin console.  You also need bidir support (in case 
you encounter Hebrew or Arabic), you need Indic shape handling (Indic 
langauges have some *very* odd composing rules), etc, and this is just 
to know how much space to take up on the screen.

is is ridiculous.  It's much better to draw a line in the sand and say 
that this is beyond the scope of the in-kernel Linux console.

	-hpa

Re: [PATCH] console UTF-8 fixes

[Egmont Koblinger]

On Sat, Apr 07, 2007 at 10:59:19AM -0700, H. Peter Anvin wrote:

> As far as width handling -- in order to make all the text line up under 
> all circumstances you need more than width handling.  [...]
> 
> is is ridiculous.  It's much better to draw a line in the sand and say 
> that this is beyond the scope of the in-kernel Linux console.

Hi,

It seems this is the only detail we don't yet agree on and I'd like to come
to an agreement before I update the patch.

I know that correctly handling all Unicode scripts, including CJK, Hebrew,
Arabic, Indic are a much more complicated story and it's way beyond the
scope of kernel. I don't even know whether there's any graphical user-space
application handling all these issues perfectly. So I really don't want to
address them.

I'd like only one small modification: the cursor to jump two columns for CJK
characters instead of just one. (Either two FFFD's, or rather an FFFD
followed by a space printed.) This would allow you to edit English words
within a mixed CJK-English text file. As my experiences show, such a minor
change in the terminal driver would solve cursor aligning issues in *many*
cases. With this change the console would still be very-very far from being
perfect, it just would be simply better in practice.

(Also note that handling two-column characters is probably amongst the first
things a good text editor implements, while handling RTL and other stuff are
trickier for applications too. I guess there are a plenty of text editors
that handle CJK correctly (provided that the underlying terminal emulator
does so too) but have troubles with combining accents, bidi and other more
complicated stuff.)

Are you still definitely against this change? I see no drawbacks this could
cause, while it would make the console better in some circumstances. I think
this is just a small step towards a better console driver.



-- 
Egmont

Re: [PATCH] console UTF-8 fixes

[H. Peter Anvin]

Egmont Koblinger wrote:
> 
> I know that correctly handling all Unicode scripts, including CJK, Hebrew,
> Arabic, Indic are a much more complicated story and it's way beyond the
> scope of kernel. I don't even know whether there's any graphical user-space
> application handling all these issues perfectly. So I really don't want to
> address them.
> 
> I'd like only one small modification: the cursor to jump two columns for CJK
> characters instead of just one. (Either two FFFD's, or rather an FFFD
> followed by a space printed.) This would allow you to edit English words
> within a mixed CJK-English text file. As my experiences show, such a minor
> change in the terminal driver would solve cursor aligning issues in *many*
> cases. With this change the console would still be very-very far from being
> perfect, it just would be simply better in practice.
> 

I don't see the point in dealing with one particular corner case, 
especially a corner case for which font support is inherently impossible.

> Are you still definitely against this change? I see no drawbacks this could
> cause, while it would make the console better in some circumstances. I think
> this is just a small step towards a better console driver.

It's bloat.

	-hpa

Re: [PATCH] console UTF-8 fixes

[Egmont Koblinger]

On Tue, Apr 10, 2007 at 08:43:14AM -0700, H. Peter Anvin wrote:

> I don't see the point in dealing with one particular corner case,

I wouldn't really call CJK a *corner* case, just think of how many people
use these writing systems.

Theoretically it's just one particular case, I agree. In practice, however,
this case occurs relatively often compared to all the nontrivial cases a
full-featured terminal should cope with.

> especially a corner case for which font support is inherently impossible.

I'd not make this chance to properly display CJK. I'd make this change to
properly display and to be able to edit English letters that happen to
follow some CJK within the same row.

> It's bloat.

What do you exactly mean by this? Doing a binary search in a table of 11
intervals to find out whether a character is double-wide? Adding
approximately 30 lines of code (including the table and the binary search
routine) to the kernel to handle this case? I don't think it's bloat. It's a
small, simple, easy to understand piece of code that would make the kernel
slightly better, and in some cases would make the users' life easier by
letting some text editors work correctly in one not-so-special case. Why not
then?

If there's no chance to make something perfect then isn't it worth it to
improve that? I don't think so.

I still haven't heard any arguments from you that the resulting *behavior*
would be worse in any respect. I have arguments that it'd be better. It's
only 30 lines of code... And even if it's a bloat, it wouldn't be the first
one in the kernel, would it? :-) A "bloat" that makes it better...


Opinions from anyone else, maybe?


I already have fixes to the UTF-8 decoder, but I still have to adjust that
patch according to what we've already discussed. I really want this CJK
issue to get accepted, unless I'm convinced that it has drawbacks. I am
happy to discuss and argue, but I'm not going to fight (I hope the
difference is clear). So please don't be on my way unless you do have a good
reason. If possible, I don't want to create two incremental patches either
(first for the decoder, second for the double-width) because it does take
some valuable time for me to stress-test each and every patch before I send
it. On the other hand, I don't know what's really needed for a patch to be
accepted. How much your word counts, for example. (I guess it counts much,
being almost the only person with worthful comments on the topic.) How much
do my chances change if I include the CJK part? I don't know...

I'd be very glad if -- based on my arguments -- you changed your mind and
wouldn't be _against_ this feature. A "don't care" state would be perfect
for me :-)



-- 
Egmont

Re: [PATCH] console UTF-8 fixes

[H. Peter Anvin]

Egmont Koblinger wrote:
> On Tue, Apr 10, 2007 at 08:43:14AM -0700, H. Peter Anvin wrote:
> 
>> I don't see the point in dealing with one particular corner case,
> 
> I wouldn't really call CJK a *corner* case, just think of how many people
> use these writing systems.
> 
> Theoretically it's just one particular case, I agree. In practice, however,
> this case occurs relatively often compared to all the nontrivial cases a
> full-featured terminal should cope with.
> 

Really?  Why is CJK so much more fundamental than, say, Arabic?

	-hpa

Re: [PATCH] console UTF-8 fixes

[Egmont Koblinger]

On Tue, Apr 10, 2007 at 10:30:07AM -0700, H. Peter Anvin wrote:

> Really?  Why is CJK so much more fundamental than, say, Arabic?

Not more fundamental at all. It's just perhaps easier to "support" (I mean
keep track of the cursor, not to really support them of course).

I can't see any reason why these two scripts should be handled identically:
either support both or none. If it's technically easier to support one and
harder to support the other, why not implement the first now? Maybe someone
will implement the other one later.

Oh! Wait a moment! I haven't yet looked at bidi in Unicode, but taking the
first glimpse it seems to me that U+200E and U+200F control the writing
direction. Currently the kernel already skips 200E and 200F, doesn't print
anything, not even a replacement character (see char/consolemap.c). This
means that RTL is already "supported" at this level: eventually (when RTL
mode is turned off) the cursor stands where it is expected to stand. In
between, you either see the right number of replacement symbols, or (if your
font supports Arabic) you may see the symbols in reverse order. So, after
all, it's not worse at all than what I want to reach with CJK.



-- 
Egmont

Re: [PATCH] console UTF-8 fixes

[Jan Engelhardt]

On Apr 10 2007 20:51, Egmont Koblinger wrote:
>On Tue, Apr 10, 2007 at 10:30:07AM -0700, H. Peter Anvin wrote:
>
>> Really?  Why is CJK so much more fundamental than, say, Arabic?
>
>Not more fundamental at all. It's just perhaps easier to "support" (I mean
>keep track of the cursor, not to really support them of course).
>
>I can't see any reason why these two scripts should be handled identically:
>either support both or none. If it's technically easier to support one and
>harder to support the other, why not implement the first now? Maybe someone
>will implement the other one later.
>
>Oh! Wait a moment! I haven't yet looked at bidi in Unicode, but taking the
>first glimpse it seems to me that U+200E and U+200F control the writing
>direction. Currently the kernel already skips 200E and 200F, doesn't print
>anything, not even a replacement character (see char/consolemap.c). This
>means that RTL is already "supported" at this level: eventually (when RTL
>mode is turned off) the cursor stands where it is expected to stand. In
>between, you either see the right number of replacement symbols, or (if your
>font supports Arabic) you may see the symbols in reverse order. So, after
>all, it's not worse at all than what I want to reach with CJK.

RTL is not supported, and perhaps we should not try. xterm does not do it
either AFAICT.

http://ttyrpld.svn.sourceforge.net/viewvc/ttyrpld/trunk/locale/fa_IR/LC_MESSAGES/ttyrpld.po?revision=15&view=markup
for simple sample data.


Jan
-- 

Re: [PATCH] console UTF-8 fixes

[Alan Cox]

> What do you exactly mean by this? Doing a binary search in a table of 11
> intervals to find out whether a character is double-wide? Adding
> approximately 30 lines of code (including the table and the binary search
> routine) to the kernel to handle this case? I don't think it's bloat. It's a

I don't have a problem with it. It is a situation people find themselves
in and framebuffer consoles can handle CJK although PC text mode ones
can't do it well. It all comes down to a clean and small implementation.

Alan

Re: [PATCH] console UTF-8 fixes

[H. Peter Anvin]

Alan Cox wrote:
>> What do you exactly mean by this? Doing a binary search in a table of 11
>> intervals to find out whether a character is double-wide? Adding
>> approximately 30 lines of code (including the table and the binary search
>> routine) to the kernel to handle this case? I don't think it's bloat. It's a
> 
> I don't have a problem with it. It is a situation people find themselves
> in and framebuffer consoles can handle CJK although PC text mode ones
> can't do it well. It all comes down to a clean and small implementation.

All the CJK framebuffer consoles that handle CJK I've seen run in 
userspace on top of the kernel framebuffer.  Keeping a CJK font in the 
kernel seems prohibitive regardless of the hardware.

Have you seen anything different?

	-hpa

Re: [PATCH] console UTF-8 fixes

[Egmont Koblinger]

Hi,

I send a reworked version of the patch.

Removed from the first version:
  - any sign of '.' as substitute glyph
  - don't ignore zero-width characters (except for a few zero-width spaces
    that are ignored in the current kernel too). However, I kept the code
    organized and commented so that someone can have the other behavior very
    easily (by removing a pair of comment signs).

Kept features, fixes:
  - lots of UTF-8 decoder fixes. Emit one U+FFFD for every standalone
    continuation byte and for every incomplete sequence, as Markus Kuhn
    recommends. Reject overlong sequences too.
  - D800..DFFF and FFFE..FFFF are substituted by FFFD too, since these are
    not valid Unicode code points.
  - no "random" replacement glyph (e.g. u with double acute instead of
    u with circumflex) in UTF-8 mode
  - if U+FFFD is not found in the font, the fallback replacement '?' (ascii
    question mark) is printed with inverse color attributes
  - U+200A was ignored so far as a zero-width space character. I think it
    was a mistake, it's not zero-width.
  - print an extra space for double-wide characters for the cursor to stand
    at the right place. Yet again the code is organized so that it is very
    easy to change to jump only one character cell, should someone prefer
    that behavior (which I still see no good reason to).

Signed-off-by: Egmont Koblinger <egmont@uhulinux.hu>

diff -Naur linux-2.6.20.orig/drivers/char/consolemap.c linux-2.6.20/drivers/char/consolemap.c
--- linux-2.6.20.orig/drivers/char/consolemap.c	2007-02-04 19:44:54.000000000 +0100
+++ linux-2.6.20/drivers/char/consolemap.c	2007-04-11 18:45:45.000000000 +0200
@@ -626,10 +626,10 @@
   
 	/* Only 16-bit codes supported at this time */
 	if (ucs > 0xffff)
-		ucs = 0xfffd;		/* U+FFFD: REPLACEMENT CHARACTER */
-	else if (ucs < 0x20 || ucs >= 0xfffe)
+		return -4;		/* Not found */
+	else if (ucs < 0x20)
 		return -1;		/* Not a printable character */
-	else if (ucs == 0xfeff || (ucs >= 0x200a && ucs <= 0x200f))
+	else if (ucs == 0xfeff || (ucs >= 0x200b && ucs <= 0x200f))
 		return -2;			/* Zero-width space */
 	/*
 	 * UNI_DIRECT_BASE indicates the start of the region in the User Zone
diff -Naur linux-2.6.20.orig/drivers/char/vt.c linux-2.6.20/drivers/char/vt.c
--- linux-2.6.20.orig/drivers/char/vt.c	2007-02-04 19:44:54.000000000 +0100
+++ linux-2.6.20/drivers/char/vt.c	2007-04-11 19:29:19.000000000 +0200
@@ -70,6 +70,15 @@
  * malformed UTF sequences represented as sequences of replacement glyphs,
  * original codes or '?' as a last resort if replacement glyph is undefined
  * by Adam Tla/lka <atlka@pg.gda.pl>, Aug 2006
+ *
+ * More robust UTF-8 decoder. Make it work on malformed sequences as Markus Kuhn's
+ * UTF-8 decoder stress test suggests. Emit a U+FFFD on illegal sequences (including
+ * overlong representations) as well as for invalid Unicode code points.
+ * If U+FFFD is not available in the font, print an inverse question mark instead.
+ * Pad double-width characters with an extra space so that the cursor moves by two
+ * positions in these cases.
+ * 11 April 2007, Egmont Koblinger <egmont@uhulinux.hu>,
+ * using code from Markus Kuhn's wcwidth() implementation.
  */
 
 #include <linux/module.h>
@@ -1934,6 +1943,99 @@
 char con_buf[CON_BUF_SIZE];
 DECLARE_MUTEX(con_buf_sem);
 
+/* is_{zero,double}_width() are based on the wcwidth() implementation by
+ * Markus Kuhn -- 2003-05-20 (Unicode 4.0)
+ * Latest version: http://www.cl.cam.ac.uk/~mgk25/ucs/wcwidth.c
+ */
+struct interval {
+  int first;
+  int last;
+};
+
+static int bisearch(long ucs, const struct interval *table, int max) {
+  int min = 0;
+  int mid;
+
+  if (ucs < table[0].first || ucs > table[max].last)
+    return 0;
+  while (max >= min) {
+    mid = (min + max) / 2;
+    if (ucs > table[mid].last)
+      min = mid + 1;
+    else if (ucs < table[mid].first)
+      max = mid - 1;
+    else
+      return 1;
+  }
+
+  return 0;
+}
+
+static int is_zero_width(long ucs)
+{
+  static const struct interval zero_width[] = {
+    { 0x0300, 0x0357 }, { 0x035D, 0x036F }, { 0x0483, 0x0486 },
+    { 0x0488, 0x0489 }, { 0x0591, 0x05A1 }, { 0x05A3, 0x05B9 },
+    { 0x05BB, 0x05BD }, { 0x05BF, 0x05BF }, { 0x05C1, 0x05C2 },
+    { 0x05C4, 0x05C4 }, { 0x0600, 0x0603 }, { 0x0610, 0x0615 },
+    { 0x064B, 0x0658 }, { 0x0670, 0x0670 }, { 0x06D6, 0x06E4 },
+    { 0x06E7, 0x06E8 }, { 0x06EA, 0x06ED }, { 0x070F, 0x070F },
+    { 0x0711, 0x0711 }, { 0x0730, 0x074A }, { 0x07A6, 0x07B0 },
+    { 0x0901, 0x0902 }, { 0x093C, 0x093C }, { 0x0941, 0x0948 },
+    { 0x094D, 0x094D }, { 0x0951, 0x0954 }, { 0x0962, 0x0963 },
+    { 0x0981, 0x0981 }, { 0x09BC, 0x09BC }, { 0x09C1, 0x09C4 },
+    { 0x09CD, 0x09CD }, { 0x09E2, 0x09E3 }, { 0x0A01, 0x0A02 },
+    { 0x0A3C, 0x0A3C }, { 0x0A41, 0x0A42 }, { 0x0A47, 0x0A48 },
+    { 0x0A4B, 0x0A4D }, { 0x0A70, 0x0A71 }, { 0x0A81, 0x0A82 },
+    { 0x0ABC, 0x0ABC }, { 0x0AC1, 0x0AC5 }, { 0x0AC7, 0x0AC8 },
+    { 0x0ACD, 0x0ACD }, { 0x0AE2, 0x0AE3 }, { 0x0B01, 0x0B01 },
+    { 0x0B3C, 0x0B3C }, { 0x0B3F, 0x0B3F }, { 0x0B41, 0x0B43 },
+    { 0x0B4D, 0x0B4D }, { 0x0B56, 0x0B56 }, { 0x0B82, 0x0B82 },
+    { 0x0BC0, 0x0BC0 }, { 0x0BCD, 0x0BCD }, { 0x0C3E, 0x0C40 },
+    { 0x0C46, 0x0C48 }, { 0x0C4A, 0x0C4D }, { 0x0C55, 0x0C56 },
+    { 0x0CBC, 0x0CBC }, { 0x0CBF, 0x0CBF }, { 0x0CC6, 0x0CC6 },
+    { 0x0CCC, 0x0CCD }, { 0x0D41, 0x0D43 }, { 0x0D4D, 0x0D4D },
+    { 0x0DCA, 0x0DCA }, { 0x0DD2, 0x0DD4 }, { 0x0DD6, 0x0DD6 },
+    { 0x0E31, 0x0E31 }, { 0x0E34, 0x0E3A }, { 0x0E47, 0x0E4E },
+    { 0x0EB1, 0x0EB1 }, { 0x0EB4, 0x0EB9 }, { 0x0EBB, 0x0EBC },
+    { 0x0EC8, 0x0ECD }, { 0x0F18, 0x0F19 }, { 0x0F35, 0x0F35 },
+    { 0x0F37, 0x0F37 }, { 0x0F39, 0x0F39 }, { 0x0F71, 0x0F7E },
+    { 0x0F80, 0x0F84 }, { 0x0F86, 0x0F87 }, { 0x0F90, 0x0F97 },
+    { 0x0F99, 0x0FBC }, { 0x0FC6, 0x0FC6 }, { 0x102D, 0x1030 },
+    { 0x1032, 0x1032 }, { 0x1036, 0x1037 }, { 0x1039, 0x1039 },
+    { 0x1058, 0x1059 }, { 0x1160, 0x11FF }, { 0x1712, 0x1714 },
+    { 0x1732, 0x1734 }, { 0x1752, 0x1753 }, { 0x1772, 0x1773 },
+    { 0x17B4, 0x17B5 }, { 0x17B7, 0x17BD }, { 0x17C6, 0x17C6 },
+    { 0x17C9, 0x17D3 }, { 0x17DD, 0x17DD }, { 0x180B, 0x180D },
+    { 0x18A9, 0x18A9 }, { 0x1920, 0x1922 }, { 0x1927, 0x1928 },
+    { 0x1932, 0x1932 }, { 0x1939, 0x193B }, { 0x200B, 0x200F },
+    { 0x202A, 0x202E }, { 0x2060, 0x2063 }, { 0x206A, 0x206F },
+    { 0x20D0, 0x20EA }, { 0x302A, 0x302F }, { 0x3099, 0x309A },
+    { 0xFB1E, 0xFB1E }, { 0xFE00, 0xFE0F }, { 0xFE20, 0xFE23 },
+    { 0xFEFF, 0xFEFF }, { 0xFFF9, 0xFFFB }, { 0x1D167, 0x1D169 },
+    { 0x1D173, 0x1D182 }, { 0x1D185, 0x1D18B }, { 0x1D1AA, 0x1D1AD },
+    { 0xE0001, 0xE0001 }, { 0xE0020, 0xE007F }, { 0xE0100, 0xE01EF }
+  };
+  if (ucs < zero_width[0].first)
+    return 0;
+  return bisearch(ucs, zero_width,
+                  sizeof(zero_width) / sizeof(struct interval) - 1);
+}
+
+static int is_double_width(long ucs)
+{
+  static const struct interval double_width[] = {
+    { 0x1100, 0x115F }, { 0x2329, 0x232A }, { 0x2E80, 0x303E },
+    { 0x3040, 0xA4CF }, { 0xAC00, 0xD7A3 }, { 0xF900, 0xFAFF },
+    { 0xFE30, 0xFE6F }, { 0xFF00, 0xFF60 }, { 0xFFE0, 0xFFE6 },
+    { 0x20000, 0x2FFFD }, { 0x30000, 0x3FFFD }
+  };
+  if (ucs < double_width[0].first)
+    return 0;
+  return bisearch(ucs, double_width,
+                  sizeof(double_width) / sizeof(struct interval) - 1);
+}
+
 /* acquires console_sem */
 static int do_con_write(struct tty_struct *tty, const unsigned char *buf, int count)
 {
@@ -1950,6 +2052,10 @@
 	unsigned int currcons;
 	unsigned long draw_from = 0, draw_to = 0;
 	struct vc_data *vc;
+	unsigned char vc_attr;
+	int rescan;
+	int inverse;
+	int width;
 	u16 himask, charmask;
 	const unsigned char *orig_buf = NULL;
 	int orig_count;
@@ -2012,51 +2118,81 @@
 		buf++;
 		n++;
 		count--;
+		rescan = 0;
+		inverse = 0;
+		width = 1;
 
 		/* Do no translation at all in control states */
 		if (vc->vc_state != ESnormal) {
 			tc = c;
 		} else if (vc->vc_utf && !vc->vc_disp_ctrl) {
-		    /* Combine UTF-8 into Unicode */
-		    /* Malformed sequences as sequences of replacement glyphs */
+		    /* Combine UTF-8 into Unicode in vc_utf_char */
+		    /* vc_utf_count is the number of continuation bytes still expected to arrive */
+		    /* vc_npar is the number of continuation bytes arrived so far */
 rescan_last_byte:
-		    if(c > 0x7f) {
+		    if ((c & 0xc0) == 0x80) {
+			/* Continuation byte received */
+			static const int utf8_length_changes[] = { 0x0000007f, 0x000007ff, 0x0000ffff, 0x001fffff, 0x03ffffff, 0x7fffffff };
 			if (vc->vc_utf_count) {
-			       if ((c & 0xc0) == 0x80) {
-				       vc->vc_utf_char = (vc->vc_utf_char << 6) | (c & 0x3f);
-       				       if (--vc->vc_utf_count) {
-					       vc->vc_npar++;
-				   	       continue;
-       				       }
-				       tc = c = vc->vc_utf_char;
-			       } else
-				       goto replacement_glyph;
-			} else {
-				vc->vc_npar = 0;
-				if ((c & 0xe0) == 0xc0) {
-				    vc->vc_utf_count = 1;
-				    vc->vc_utf_char = (c & 0x1f);
-				} else if ((c & 0xf0) == 0xe0) {
-				    vc->vc_utf_count = 2;
-				    vc->vc_utf_char = (c & 0x0f);
-				} else if ((c & 0xf8) == 0xf0) {
-				    vc->vc_utf_count = 3;
-				    vc->vc_utf_char = (c & 0x07);
-				} else if ((c & 0xfc) == 0xf8) {
-				    vc->vc_utf_count = 4;
-				    vc->vc_utf_char = (c & 0x03);
-				} else if ((c & 0xfe) == 0xfc) {
-				    vc->vc_utf_count = 5;
-				    vc->vc_utf_char = (c & 0x01);
-				} else
-	    			    goto replacement_glyph;
+			    vc->vc_utf_char = (vc->vc_utf_char << 6) | (c & 0x3f);
+			    vc->vc_npar++;
+			    if (--vc->vc_utf_count) {
+				/* Still need some bytes */
 				continue;
-			      }
+			    }
+			    /* Got a whole character */
+			    c = vc->vc_utf_char;
+			    /* Reject overlong sequences */
+			    if (c <= utf8_length_changes[vc->vc_npar - 1] || c > utf8_length_changes[vc->vc_npar]) {
+				c = 0xfffd;
+			    }
+			} else {
+			    /* Unexpected continuation byte */
+			    vc->vc_utf_count = 0;
+			    c = 0xfffd;
+			}
 		    } else {
-		      if (vc->vc_utf_count)
-	  		      goto replacement_glyph;
-		      tc = c;
+			/* Single ASCII byte or first byte of a sequence received */
+			if (vc->vc_utf_count) {
+			    /* Continuation byte expected */
+			    rescan = 1;
+			    vc->vc_utf_count = 0;
+			    c = 0xfffd;
+			} else if (c > 0x7f) {
+			    /* First byte of a multibyte sequence received */
+			    vc->vc_npar = 0;
+			    if ((c & 0xe0) == 0xc0) {
+				vc->vc_utf_count = 1;
+				vc->vc_utf_char = (c & 0x1f);
+			    } else if ((c & 0xf0) == 0xe0) {
+				vc->vc_utf_count = 2;
+				vc->vc_utf_char = (c & 0x0f);
+			    } else if ((c & 0xf8) == 0xf0) {
+				vc->vc_utf_count = 3;
+				vc->vc_utf_char = (c & 0x07);
+			    } else if ((c & 0xfc) == 0xf8) {
+				vc->vc_utf_count = 4;
+				vc->vc_utf_char = (c & 0x03);
+			    } else if ((c & 0xfe) == 0xfc) {
+				vc->vc_utf_count = 5;
+				vc->vc_utf_char = (c & 0x01);
+			    } else {
+				/* 254 and 255 are invalid */
+				c = 0xfffd;
+			    }
+			    if (vc->vc_utf_count) {
+				/* Still need some bytes */
+				continue;
+			    }
+			}
+			/* Nothing to do if an ASCII byte was received */
 		    }
+		    /* End of UTF-8 decoding. */
+		    /* c is the received character, or U+FFFD for invalid sequences. */
+		    /* Replace invalid Unicode code points with U+FFFD too */
+		    if ((c >= 0xd800 && c <= 0xdfff) || c == 0xfffe || c == 0xffff)
+			c = 0xfffd;
+		    tc = c;
 		} else {	/* no utf or alternate charset mode */
 		  tc = vc->vc_translate[vc->vc_toggle_meta ? (c | 0x80) : c];
 		}
@@ -2078,56 +2214,88 @@
 			&& (c != 128+27);
 
 		if (vc->vc_state == ESnormal && ok) {
+			if (vc->vc_utf && !vc->vc_disp_ctrl) {
+				if (is_double_width(c)) {
+					width = 2;
+				}
+				/* Uncomment this if you'd prefer zero-width characters to be silently dropped
+				   instead of printing the replacement glyph */
+				/*
+				else if (is_zero_width(c)) {
+					continue;
+				}
+				*/
+			}
 			/* Now try to find out how to display it */
 			tc = conv_uni_to_pc(vc, tc);
 			if (tc & ~charmask) {
-				if ( tc == -4 ) {
-                                /* If we got -4 (not found) then see if we have
-                                   defined a replacement character (U+FFFD) */
-replacement_glyph:
-                                	tc = conv_uni_to_pc(vc, 0xfffd);
-					if (!(tc & ~charmask))
-						goto display_glyph;
-                        	} else if ( tc != -3 )
-                                	continue; /* nothing to display */
-                                /* no hash table or no replacement --
-				 * hope for the best */
-				if ( c & ~charmask )
-					tc = '?';
-				else
-					tc = c;
+				if (tc == -1 || tc == -2) {
+				    continue; /* nothing to display */
+				}
+				/* Glyph not found */
+				if (!(vc->vc_utf && !vc->vc_disp_ctrl) && !(c & ~charmask)) {
+				    /* In legacy mode use the glyph we get by a 1:1 mapping.
+				       This would make absolutely no sense with Unicode in mind. */
+				    tc = c;
+				} else {
+				    /* Display U+FFFD. If it's not found, display an inverse question mark. */
+				    tc = conv_uni_to_pc(vc, 0xfffd);
+				    if (tc < 0) {
+					inverse = 1;
+					tc = conv_uni_to_pc(vc, '?');
+					if (tc < 0) tc = '?';
+				    }
+				}
 			}
 
-display_glyph:
-			if (vc->vc_need_wrap || vc->vc_decim)
-				FLUSH
-			if (vc->vc_need_wrap) {
-				cr(vc);
-				lf(vc);
-			}
-			if (vc->vc_decim)
-				insert_char(vc, 1);
-			scr_writew(himask ?
-				     ((vc->vc_attr << 8) & ~himask) + ((tc & 0x100) ? himask : 0) + (tc & 0xff) :
-				     (vc->vc_attr << 8) + tc,
-				   (u16 *) vc->vc_pos);
-			if (DO_UPDATE(vc) && draw_x < 0) {
-				draw_x = vc->vc_x;
-				draw_from = vc->vc_pos;
-			}
-			if (vc->vc_x == vc->vc_cols - 1) {
-				vc->vc_need_wrap = vc->vc_decawm;
-				draw_to = vc->vc_pos + 2;
+			if (!inverse) {
+				vc_attr = vc->vc_attr;
 			} else {
-				vc->vc_x++;
-				draw_to = (vc->vc_pos += 2);
+				/* invert vc_attr */
+				if (!vc->vc_can_do_color) {
+					vc_attr = (vc->vc_attr) ^ 0x08;
+				} else if (vc->vc_hi_font_mask == 0x100) {
+					vc_attr = ((vc->vc_attr) & 0x11) | (((vc->vc_attr) & 0xe0) >> 4) | (((vc->vc_attr) & 0x0e) << 4);
+				} else {
+					vc_attr = ((vc->vc_attr) & 0x88) | (((vc->vc_attr) & 0x70) >> 4) | (((vc->vc_attr) & 0x07) << 4);
+				}
 			}
-			if (vc->vc_utf_count) {
-				if (vc->vc_npar) {
-					vc->vc_npar--;
-					goto display_glyph;
+
+			while (1) {
+				if (vc->vc_need_wrap || vc->vc_decim)
+					FLUSH
+				if (vc->vc_need_wrap) {
+					cr(vc);
+					lf(vc);
+				}
+				if (vc->vc_decim)
+					insert_char(vc, 1);
+				scr_writew(himask ?
+					     ((vc_attr << 8) & ~himask) + ((tc & 0x100) ? himask : 0) + (tc & 0xff) :
+					     (vc_attr << 8) + tc,
+					   (u16 *) vc->vc_pos);
+				if (DO_UPDATE(vc) && draw_x < 0) {
+					draw_x = vc->vc_x;
+					draw_from = vc->vc_pos;
+				}
+				if (vc->vc_x == vc->vc_cols - 1) {
+					vc->vc_need_wrap = vc->vc_decawm;
+					draw_to = vc->vc_pos + 2;
+				} else {
+					vc->vc_x++;
+					draw_to = (vc->vc_pos += 2);
 				}
-				vc->vc_utf_count = 0;
+
+				if (!--width) break;
+
+				tc = conv_uni_to_pc(vc, ' '); /* A space is printed in the second column */
+				if (tc < 0) tc = ' ';
+			}
+
+			if (rescan) {
+				rescan = 0;
+				inverse = 0;
+				width = 1;
 				c = orig;
 				goto rescan_last_byte;
 			}

Re: [PATCH] console UTF-8 fixes

[H. Peter Anvin]

Egmont Koblinger wrote:
> +static int is_zero_width(long ucs)
> +{
> +  static const struct interval zero_width[] = {
/* lots */
> +  };

I'm still unhappy about these large search tables in the kernel, not 
because they take a huge amount of space (it's not that much), but 
because they're invariably going to be stale, as they're Unicode-version 
dependent.

At least please put them in a separate .c file and include a script to 
generate them clean from UnicodeData.txt.

Besides, would it not make more sense to have a single table with the 
width information, if you insist on having one, instead of multiple ones?

	-hpa

Re: [PATCH] console UTF-8 fixes

[Egmont Koblinger]

On Wed, Apr 11, 2007 at 11:36:40AM -0700, H. Peter Anvin wrote:
> Egmont Koblinger wrote:
> >+static int is_zero_width(long ucs)
> >+{
> >+  static const struct interval zero_width[] = {
> /* lots */
> >+  };
> 
> I'm still unhappy about these large search tables in the kernel, not 
> because they take a huge amount of space (it's not that much), but 
> because they're invariably going to be stale, as they're Unicode-version 
> dependent.

I don't think width information for characters in BMP is going to change
that often.

By the way, a note about the size: the larger one of the two tables is
unused and hence optimised away by the compiler. I just left in the source
so that it only takes a minor modification for people go get a different
sane behavior (ie. ignore combining chars). So only the small table, with 11
pairs of longs (88 bytes) are compiled to the kernel.

> At least please put them in a separate .c file and include a script to 
> generate them clean from UnicodeData.txt.

I'll look at it, but I didn't want to alter the building procedure, modify
Makefiles... Or do you mean I should only ship the generated .c file plus
the script, instead of the (1MB) UnicodeData.txt and generating it compile
time? Sounds reasonable...

> Besides, would it not make more sense to have a single table with the 
> width information, if you insist on having one, instead of multiple ones?

I've been thinking on it and I'm not sure which one the right way is. The
reason for choosing this was probably that this way information that is not
used by the code can be omitted by the compiler.



-- 
Egmont

Re: [PATCH] console UTF-8 fixes

[H. Peter Anvin]

Egmont Koblinger wrote:
> 
> I don't think width information for characters in BMP is going to change
> that often.
> 
> By the way, a note about the size: the larger one of the two tables is
> unused and hence optimised away by the compiler. I just left in the source
> so that it only takes a minor modification for people go get a different
> sane behavior (ie. ignore combining chars). So only the small table, with 11
> pairs of longs (88 bytes) are compiled to the kernel.
> 

Every version has added combining chars.  But anyway, please don't leave 
unused code in the kernel.  I agree doublewidth characters are largely 
range-based and thus not all that likely to change.

>> At least please put them in a separate .c file and include a script to 
>> generate them clean from UnicodeData.txt.
> 
> I'll look at it, but I didn't want to alter the building procedure, modify
> Makefiles... Or do you mean I should only ship the generated .c file plus
> the script, instead of the (1MB) UnicodeData.txt and generating it compile
> time? Sounds reasonable...

Right.  However, see above.

>> Besides, would it not make more sense to have a single table with the 
>> width information, if you insist on having one, instead of multiple ones?
> 
> I've been thinking on it and I'm not sure which one the right way is. The
> reason for choosing this was probably that this way information that is not
> used by the code can be omitted by the compiler.

Then let's leave it out of the source.

Re: [PATCH] console UTF-8 fixes

[Jan Engelhardt]

On Apr 12 2007 08:36, H. Peter Anvin wrote:
> Egmont Koblinger wrote:
>
>> > Besides, would it not make more sense to have a single table with the
>> > width information, if you insist on having one, instead of multiple
>> > ones?
>> 
>> I've been thinking on it and I'm not sure which one the right way is. The
>> reason for choosing this was probably that this way information that is
>> not
>> used by the code can be omitted by the compiler.
>
> Then let's leave it out of the source.

I however will put this into my rpm if it works/isstable.


Jan
-- 

Re: [PATCH] console UTF-8 fixes

[Egmont Koblinger]

On Thu, Apr 12, 2007 at 06:41:22PM +0200, Jan Engelhardt wrote:

> >> I've been thinking on it and I'm not sure which one the right way is. The
> >> reason for choosing this was probably that this way information that is
> >> not
> >> used by the code can be omitted by the compiler.
> >
> > Then let's leave it out of the source.
> 
> I however will put this into my rpm if it works/isstable.

We've arrived at another coding policy :)

There are two possible behaviors, each have pros and cons. HPA prefers one,
while Jan and me would prefer the other. The difference is one function that
contains a large table and an invocation of that function in a small if
branch.

In my latest version of the patch I've put the function itself inside
comments too. So it's not the real compiler, it's the preprocessor that
omits this code for the default behavior. HPA, I hope you don't mind if the
other reasonable behavior is there in the source, within comments. I think
let's make the job easier for those who have a different opinion. Or is it a
completely stupid idea?


-- 
Egmont

Re: [PATCH] console UTF-8 fixes

[H. Peter Anvin]

Egmont Koblinger wrote:
> 
> We've arrived at another coding policy :)
> 
> There are two possible behaviors, each have pros and cons. HPA prefers one,
> while Jan and me would prefer the other. The difference is one function that
> contains a large table and an invocation of that function in a small if
> branch.
> 
> In my latest version of the patch I've put the function itself inside
> comments too. So it's not the real compiler, it's the preprocessor that
> omits this code for the default behavior. HPA, I hope you don't mind if the
> other reasonable behavior is there in the source, within comments. I think
> let's make the job easier for those who have a different opinion. Or is it a
> completely stupid idea?
> 

Not leaving dead code in the kernel is long-standing policy; it's 
nothing new.  We constantly remove #if 0'd code that the authors have 
left in.

	-hpa

Re: [PATCH] console UTF-8 fixes

[Egmont Koblinger]

On Thu, Apr 12, 2007 at 09:58:38AM -0700, H. Peter Anvin wrote:

> Not leaving dead code in the kernel is long-standing policy; it's 
> nothing new.  We constantly remove #if 0'd code that the authors have 
> left in.

I see. However, you wrote it recently:

> Besides, would it not make more sense to have a single table with the
> width information, if you insist on having one, instead of multiple ones?

If I rework the code according to this then the zero-width information will
no longer be dead code, right? The unchanged kernel would be slightly larger
and would run slightly slower, but it would be much easier to patch it to
reach the other possible behavior regarding combining characters.



-- 
Egmont

Re: [PATCH] console UTF-8 fixes

[H. Peter Anvin]

Egmont Koblinger wrote:
> On Thu, Apr 12, 2007 at 09:58:38AM -0700, H. Peter Anvin wrote:
> 
>> Not leaving dead code in the kernel is long-standing policy; it's 
>> nothing new.  We constantly remove #if 0'd code that the authors have 
>> left in.
> 
> I see. However, you wrote it recently:
> 
>> Besides, would it not make more sense to have a single table with the
>> width information, if you insist on having one, instead of multiple ones?
> 
> If I rework the code according to this then the zero-width information will
> no longer be dead code, right? The unchanged kernel would be slightly larger
> and would run slightly slower, but it would be much easier to patch it to
> reach the other possible behavior regarding combining characters.
> 

Yes, I didn't realize at the time that that was dead code.

	-hpa

Re: [PATCH] console UTF-8 fixes

[Egmont Koblinger]

On Thu, Apr 12, 2007 at 10:35:24AM -0700, H. Peter Anvin wrote:

> Yes, I didn't realize at the time that that was dead code.

:-))))

Version 4 of the patch follows. Dead code omitted from version 3.


Signed-off-by: Egmont Koblinger <egmont@uhulinux.hu>

diff -Naur linux-2.6.20.orig/drivers/char/consolemap.c linux-2.6.20/drivers/char/consolemap.c
--- linux-2.6.20.orig/drivers/char/consolemap.c	2007-02-04 19:44:54.000000000 +0100
+++ linux-2.6.20/drivers/char/consolemap.c	2007-04-11 18:45:45.000000000 +0200
@@ -626,10 +626,10 @@
   
 	/* Only 16-bit codes supported at this time */
 	if (ucs > 0xffff)
-		ucs = 0xfffd;		/* U+FFFD: REPLACEMENT CHARACTER */
-	else if (ucs < 0x20 || ucs >= 0xfffe)
+		return -4;		/* Not found */
+	else if (ucs < 0x20)
 		return -1;		/* Not a printable character */
-	else if (ucs == 0xfeff || (ucs >= 0x200a && ucs <= 0x200f))
+	else if (ucs == 0xfeff || (ucs >= 0x200b && ucs <= 0x200f))
 		return -2;			/* Zero-width space */
 	/*
 	 * UNI_DIRECT_BASE indicates the start of the region in the User Zone
diff -Naur linux-2.6.20.orig/drivers/char/vt.c linux-2.6.20/drivers/char/vt.c
--- linux-2.6.20.orig/drivers/char/vt.c	2007-02-04 19:44:54.000000000 +0100
+++ linux-2.6.20/drivers/char/vt.c	2007-04-12 19:38:51.000000000 +0200
@@ -1934,6 +1934,46 @@
 char con_buf[CON_BUF_SIZE];
 DECLARE_MUTEX(con_buf_sem);
 
+/* is_double_width() is based on the wcwidth() implementation by
+ * Markus Kuhn -- 2003-05-20 (Unicode 4.0)
+ * Latest version: http://www.cl.cam.ac.uk/~mgk25/ucs/wcwidth.c
+ */
+struct interval {
+	uint32_t first;
+	uint32_t last;
+};
+
+static int bisearch(uint32_t ucs, const struct interval *table, int max)
+{
+	int min = 0;
+	int mid;
+
+	if (ucs < table[0].first || ucs > table[max].last)
+		return 0;
+	while (max >= min) {
+		mid = (min + max) / 2;
+		if (ucs > table[mid].last)
+			min = mid + 1;
+		else if (ucs < table[mid].first)
+			max = mid - 1;
+		else
+			return 1;
+	}
+	return 0;
+}
+
+static int is_double_width(uint32_t ucs)
+{
+	static const struct interval double_width[] = {
+		{ 0x1100, 0x115F }, { 0x2329, 0x232A }, { 0x2E80, 0x303E },
+		{ 0x3040, 0xA4CF }, { 0xAC00, 0xD7A3 }, { 0xF900, 0xFAFF },
+		{ 0xFE30, 0xFE6F }, { 0xFF00, 0xFF60 }, { 0xFFE0, 0xFFE6 },
+		{ 0x20000, 0x2FFFD }, { 0x30000, 0x3FFFD }
+	};
+	return bisearch(ucs, double_width,
+		sizeof(double_width) / sizeof(*double_width) - 1);
+}
+
 /* acquires console_sem */
 static int do_con_write(struct tty_struct *tty, const unsigned char *buf, int count)
 {
@@ -1950,6 +1990,10 @@
 	unsigned int currcons;
 	unsigned long draw_from = 0, draw_to = 0;
 	struct vc_data *vc;
+	unsigned char vc_attr;
+	uint8_t rescan;
+	uint8_t inverse;
+	uint8_t width;
 	u16 himask, charmask;
 	const unsigned char *orig_buf = NULL;
 	int orig_count;
@@ -2012,51 +2056,81 @@
 		buf++;
 		n++;
 		count--;
+		rescan = 0;
+		inverse = 0;
+		width = 1;
 
 		/* Do no translation at all in control states */
 		if (vc->vc_state != ESnormal) {
 			tc = c;
 		} else if (vc->vc_utf && !vc->vc_disp_ctrl) {
-		    /* Combine UTF-8 into Unicode */
-		    /* Malformed sequences as sequences of replacement glyphs */
+		    /* Combine UTF-8 into Unicode in vc_utf_char */
+		    /* vc_utf_count is the number of continuation bytes still expected to arrive */
+		    /* vc_npar is the number of continuation bytes arrived so far */
 rescan_last_byte:
-		    if(c > 0x7f) {
+		    if ((c & 0xc0) == 0x80) {
+			/* Continuation byte received */
+			static const uint32_t utf8_length_changes[] = { 0x0000007f, 0x000007ff, 0x0000ffff, 0x001fffff, 0x03ffffff, 0x7fffffff };
 			if (vc->vc_utf_count) {
-			       if ((c & 0xc0) == 0x80) {
-				       vc->vc_utf_char = (vc->vc_utf_char << 6) | (c & 0x3f);
-       				       if (--vc->vc_utf_count) {
-					       vc->vc_npar++;
-				   	       continue;
-       				       }
-				       tc = c = vc->vc_utf_char;
-			       } else
-				       goto replacement_glyph;
-			} else {
-				vc->vc_npar = 0;
-				if ((c & 0xe0) == 0xc0) {
-				    vc->vc_utf_count = 1;
-				    vc->vc_utf_char = (c & 0x1f);
-				} else if ((c & 0xf0) == 0xe0) {
-				    vc->vc_utf_count = 2;
-				    vc->vc_utf_char = (c & 0x0f);
-				} else if ((c & 0xf8) == 0xf0) {
-				    vc->vc_utf_count = 3;
-				    vc->vc_utf_char = (c & 0x07);
-				} else if ((c & 0xfc) == 0xf8) {
-				    vc->vc_utf_count = 4;
-				    vc->vc_utf_char = (c & 0x03);
-				} else if ((c & 0xfe) == 0xfc) {
-				    vc->vc_utf_count = 5;
-				    vc->vc_utf_char = (c & 0x01);
-				} else
-	    			    goto replacement_glyph;
+			    vc->vc_utf_char = (vc->vc_utf_char << 6) | (c & 0x3f);
+			    vc->vc_npar++;
+			    if (--vc->vc_utf_count) {
+				/* Still need some bytes */
 				continue;
-			      }
+			    }
+			    /* Got a whole character */
+			    c = vc->vc_utf_char;
+			    /* Reject overlong sequences */
+			    if (c <= utf8_length_changes[vc->vc_npar - 1] || c > utf8_length_changes[vc->vc_npar]) {
+				c = 0xfffd;
+			    }
+			} else {
+			    /* Unexpected continuation byte */
+			    vc->vc_utf_count = 0;
+			    c = 0xfffd;
+			}
 		    } else {
-		      if (vc->vc_utf_count)
-	  		      goto replacement_glyph;
-		      tc = c;
+			/* Single ASCII byte or first byte of a sequence received */
+			if (vc->vc_utf_count) {
+			    /* Continuation byte expected */
+			    rescan = 1;
+			    vc->vc_utf_count = 0;
+			    c = 0xfffd;
+			} else if (c > 0x7f) {
+			    /* First byte of a multibyte sequence received */
+			    vc->vc_npar = 0;
+			    if ((c & 0xe0) == 0xc0) {
+				vc->vc_utf_count = 1;
+				vc->vc_utf_char = (c & 0x1f);
+			    } else if ((c & 0xf0) == 0xe0) {
+				vc->vc_utf_count = 2;
+				vc->vc_utf_char = (c & 0x0f);
+			    } else if ((c & 0xf8) == 0xf0) {
+				vc->vc_utf_count = 3;
+				vc->vc_utf_char = (c & 0x07);
+			    } else if ((c & 0xfc) == 0xf8) {
+				vc->vc_utf_count = 4;
+				vc->vc_utf_char = (c & 0x03);
+			    } else if ((c & 0xfe) == 0xfc) {
+				vc->vc_utf_count = 5;
+				vc->vc_utf_char = (c & 0x01);
+			    } else {
+				/* 254 and 255 are invalid */
+				c = 0xfffd;
+			    }
+			    if (vc->vc_utf_count) {
+				/* Still need some bytes */
+				continue;
+			    }
+			}
+			/* Nothing to do if an ASCII byte was received */
 		    }
+		    /* End of UTF-8 decoding. */
+		    /* c is the received character, or U+FFFD for invalid sequences. */
+		    /* Replace invalid Unicode code points with U+FFFD too */
+		    if ((c >= 0xd800 && c <= 0xdfff) || c == 0xfffe || c == 0xffff)
+			c = 0xfffd;
+		    tc = c;
 		} else {	/* no utf or alternate charset mode */
 		  tc = vc->vc_translate[vc->vc_toggle_meta ? (c | 0x80) : c];
 		}
@@ -2078,56 +2152,81 @@
 			&& (c != 128+27);
 
 		if (vc->vc_state == ESnormal && ok) {
+			if (vc->vc_utf && !vc->vc_disp_ctrl) {
+				if (is_double_width(c)) {
+					width = 2;
+				}
+			}
 			/* Now try to find out how to display it */
 			tc = conv_uni_to_pc(vc, tc);
 			if (tc & ~charmask) {
-				if ( tc == -4 ) {
-                                /* If we got -4 (not found) then see if we have
-                                   defined a replacement character (U+FFFD) */
-replacement_glyph:
-                                	tc = conv_uni_to_pc(vc, 0xfffd);
-					if (!(tc & ~charmask))
-						goto display_glyph;
-                        	} else if ( tc != -3 )
-                                	continue; /* nothing to display */
-                                /* no hash table or no replacement --
-				 * hope for the best */
-				if ( c & ~charmask )
-					tc = '?';
-				else
-					tc = c;
+				if (tc == -1 || tc == -2) {
+				    continue; /* nothing to display */
+				}
+				/* Glyph not found */
+				if (!(vc->vc_utf && !vc->vc_disp_ctrl) && !(c & ~charmask)) {
+				    /* In legacy mode use the glyph we get by a 1:1 mapping.
+				       This would make absolutely no sense with Unicode in mind. */
+				    tc = c;
+				} else {
+				    /* Display U+FFFD. If it's not found, display an inverse question mark. */
+				    tc = conv_uni_to_pc(vc, 0xfffd);
+				    if (tc < 0) {
+					inverse = 1;
+					tc = conv_uni_to_pc(vc, '?');
+					if (tc < 0) tc = '?';
+				    }
+				}
 			}
 
-display_glyph:
-			if (vc->vc_need_wrap || vc->vc_decim)
-				FLUSH
-			if (vc->vc_need_wrap) {
-				cr(vc);
-				lf(vc);
-			}
-			if (vc->vc_decim)
-				insert_char(vc, 1);
-			scr_writew(himask ?
-				     ((vc->vc_attr << 8) & ~himask) + ((tc & 0x100) ? himask : 0) + (tc & 0xff) :
-				     (vc->vc_attr << 8) + tc,
-				   (u16 *) vc->vc_pos);
-			if (DO_UPDATE(vc) && draw_x < 0) {
-				draw_x = vc->vc_x;
-				draw_from = vc->vc_pos;
-			}
-			if (vc->vc_x == vc->vc_cols - 1) {
-				vc->vc_need_wrap = vc->vc_decawm;
-				draw_to = vc->vc_pos + 2;
+			if (!inverse) {
+				vc_attr = vc->vc_attr;
 			} else {
-				vc->vc_x++;
-				draw_to = (vc->vc_pos += 2);
+				/* invert vc_attr */
+				if (!vc->vc_can_do_color) {
+					vc_attr = (vc->vc_attr) ^ 0x08;
+				} else if (vc->vc_hi_font_mask == 0x100) {
+					vc_attr = ((vc->vc_attr) & 0x11) | (((vc->vc_attr) & 0xe0) >> 4) | (((vc->vc_attr) & 0x0e) << 4);
+				} else {
+					vc_attr = ((vc->vc_attr) & 0x88) | (((vc->vc_attr) & 0x70) >> 4) | (((vc->vc_attr) & 0x07) << 4);
+				}
 			}
-			if (vc->vc_utf_count) {
-				if (vc->vc_npar) {
-					vc->vc_npar--;
-					goto display_glyph;
+
+			while (1) {
+				if (vc->vc_need_wrap || vc->vc_decim)
+					FLUSH
+				if (vc->vc_need_wrap) {
+					cr(vc);
+					lf(vc);
+				}
+				if (vc->vc_decim)
+					insert_char(vc, 1);
+				scr_writew(himask ?
+					     ((vc_attr << 8) & ~himask) + ((tc & 0x100) ? himask : 0) + (tc & 0xff) :
+					     (vc_attr << 8) + tc,
+					   (u16 *) vc->vc_pos);
+				if (DO_UPDATE(vc) && draw_x < 0) {
+					draw_x = vc->vc_x;
+					draw_from = vc->vc_pos;
+				}
+				if (vc->vc_x == vc->vc_cols - 1) {
+					vc->vc_need_wrap = vc->vc_decawm;
+					draw_to = vc->vc_pos + 2;
+				} else {
+					vc->vc_x++;
+					draw_to = (vc->vc_pos += 2);
 				}
-				vc->vc_utf_count = 0;
+
+				if (!--width) break;
+
+				tc = conv_uni_to_pc(vc, ' '); /* A space is printed in the second column */
+				if (tc < 0) tc = ' ';
+			}
+
+			if (rescan) {
+				rescan = 0;
+				inverse = 0;
+				width = 1;
 				c = orig;
 				goto rescan_last_byte;
 			}

Re: [PATCH] console UTF-8 fixes

[H. Peter Anvin]

Egmont Koblinger wrote:
> On Thu, Apr 12, 2007 at 10:35:24AM -0700, H. Peter Anvin wrote:
> 
>> Yes, I didn't realize at the time that that was dead code.
> 
> :-))))
> 
> Version 4 of the patch follows. Dead code omitted from version 3.

Looks good to me.

	-hpa

Re: [PATCH] console UTF-8 fixes

[Jan Engelhardt]

On Apr 12 2007 18:55, Egmont Koblinger wrote:
>
>> >> I've been thinking on it and I'm not sure which one the right
>> >> way is. The reason for choosing this was probably that this way
>> >> information that is not used by the code can be omitted by the
>> >> compiler.
>> >
>> > Then let's leave it out of the source.
>> 
>> I however will put this into my rpm if it works/isstable.
>
>We've arrived at another coding policy :)
>
>There are two possible behaviors, each have pros and cons. HPA
>prefers one, while Jan and me would prefer the other.

I don't really care how it's done - however it should at least not
hurt in the eyes (<->CodingStyle). I just welcome this
functionality/bugfix, because I find myself editing text with
undisplayable characters more often than not. Should be ok now.


Jan
-- 

Re: [PATCH] console UTF-8 fixes

[Egmont Koblinger]

Hi,

I send the third version. No major modifications from the second version,
only small cleanups, coding style...

H. Peter Anvin wrote:

> I'm still unhappy about these large search tables in the kernel, not 
> because they take a huge amount of space (it's not that much), but 
> because they're invariably going to be stale, as they're Unicode-version 
> dependent.
> 
> At least please put them in a separate .c file and include a script to 
> generate them clean from UnicodeData.txt.

I tried to create such a script using ideas for regexps from glibc's
charmaps/UTF-8, but it seemed to be quite hopeless to create a small table.
It seems that Markus probably performed some reasonal manual optimisations
that cannot really be scripted. For example, within a huge CJK area there
are plenty of codepoints that are not (yet?) assigned. However, if they'll
be assigned in the future, most likely they'll be double-wide characters
too. A simple script, on the other hand, would skip them and create more
smaller intervals of CJK characters. However, automatically assuming
double-width for consecutive sequence of unassigned codepoints between two
double-wide character may not be a good idea either.

For zero-width the situation is the same. I couldn't find which piece of
information within UnicodeData.txt I could 100% rely on to create a simple
table.

So please accept these hard-coded tables in the first round. Maybe one day
somebody will come up with a better solution. This one should be okay until
then. (I can also send the script I've written so he can improve it.)


Jan: "unsigned int rescan:1" only seems to work within structure
definitions, but you recommended it for variables inside a function. So now
I use uint8_t.

Pavel and Jan: rbtree is an excellent data structure if you have to modify
the contents run-time. Here we have a constant data structure, and static
initialization of an rbtree (with lots of pointers and other internal data)
seems to be (quite) impossible for me.


Signed-off-by: Egmont Koblinger <egmont@uhulinux.hu>

diff -Naur linux-2.6.20.orig/drivers/char/consolemap.c linux-2.6.20/drivers/char/consolemap.c
--- linux-2.6.20.orig/drivers/char/consolemap.c	2007-02-04 19:44:54.000000000 +0100
+++ linux-2.6.20/drivers/char/consolemap.c	2007-04-11 18:45:45.000000000 +0200
@@ -626,10 +626,10 @@
   
 	/* Only 16-bit codes supported at this time */
 	if (ucs > 0xffff)
-		ucs = 0xfffd;		/* U+FFFD: REPLACEMENT CHARACTER */
-	else if (ucs < 0x20 || ucs >= 0xfffe)
+		return -4;		/* Not found */
+	else if (ucs < 0x20)
 		return -1;		/* Not a printable character */
-	else if (ucs == 0xfeff || (ucs >= 0x200a && ucs <= 0x200f))
+	else if (ucs == 0xfeff || (ucs >= 0x200b && ucs <= 0x200f))
 		return -2;			/* Zero-width space */
 	/*
 	 * UNI_DIRECT_BASE indicates the start of the region in the User Zone
diff -Naur linux-2.6.20.orig/drivers/char/vt.c linux-2.6.20/drivers/char/vt.c
--- linux-2.6.20.orig/drivers/char/vt.c	2007-02-04 19:44:54.000000000 +0100
+++ linux-2.6.20/drivers/char/vt.c	2007-04-12 14:18:05.000000000 +0200
@@ -1934,6 +1934,97 @@
 char con_buf[CON_BUF_SIZE];
 DECLARE_MUTEX(con_buf_sem);
 
+/* is_{zero,double}_width() are based on the wcwidth() implementation by
+ * Markus Kuhn -- 2003-05-20 (Unicode 4.0)
+ * Latest version: http://www.cl.cam.ac.uk/~mgk25/ucs/wcwidth.c
+ */
+struct interval {
+	uint32_t first;
+	uint32_t last;
+};
+
+static int bisearch(uint32_t ucs, const struct interval *table, int max)
+{
+	int min = 0;
+	int mid;
+
+	if (ucs < table[0].first || ucs > table[max].last)
+		return 0;
+	while (max >= min) {
+		mid = (min + max) / 2;
+		if (ucs > table[mid].last)
+			min = mid + 1;
+		else if (ucs < table[mid].first)
+			max = mid - 1;
+		else
+			return 1;
+	}
+	return 0;
+}
+
+/*
+static int is_zero_width(uint32_t ucs)
+{
+	static const struct interval zero_width[] = {
+		{ 0x0300, 0x0357 }, { 0x035D, 0x036F }, { 0x0483, 0x0486 },
+		{ 0x0488, 0x0489 }, { 0x0591, 0x05A1 }, { 0x05A3, 0x05B9 },
+		{ 0x05BB, 0x05BD }, { 0x05BF, 0x05BF }, { 0x05C1, 0x05C2 },
+		{ 0x05C4, 0x05C4 }, { 0x0600, 0x0603 }, { 0x0610, 0x0615 },
+		{ 0x064B, 0x0658 }, { 0x0670, 0x0670 }, { 0x06D6, 0x06E4 },
+		{ 0x06E7, 0x06E8 }, { 0x06EA, 0x06ED }, { 0x070F, 0x070F },
+		{ 0x0711, 0x0711 }, { 0x0730, 0x074A }, { 0x07A6, 0x07B0 },
+		{ 0x0901, 0x0902 }, { 0x093C, 0x093C }, { 0x0941, 0x0948 },
+		{ 0x094D, 0x094D }, { 0x0951, 0x0954 }, { 0x0962, 0x0963 },
+		{ 0x0981, 0x0981 }, { 0x09BC, 0x09BC }, { 0x09C1, 0x09C4 },
+		{ 0x09CD, 0x09CD }, { 0x09E2, 0x09E3 }, { 0x0A01, 0x0A02 },
+		{ 0x0A3C, 0x0A3C }, { 0x0A41, 0x0A42 }, { 0x0A47, 0x0A48 },
+		{ 0x0A4B, 0x0A4D }, { 0x0A70, 0x0A71 }, { 0x0A81, 0x0A82 },
+		{ 0x0ABC, 0x0ABC }, { 0x0AC1, 0x0AC5 }, { 0x0AC7, 0x0AC8 },
+		{ 0x0ACD, 0x0ACD }, { 0x0AE2, 0x0AE3 }, { 0x0B01, 0x0B01 },
+		{ 0x0B3C, 0x0B3C }, { 0x0B3F, 0x0B3F }, { 0x0B41, 0x0B43 },
+		{ 0x0B4D, 0x0B4D }, { 0x0B56, 0x0B56 }, { 0x0B82, 0x0B82 },
+		{ 0x0BC0, 0x0BC0 }, { 0x0BCD, 0x0BCD }, { 0x0C3E, 0x0C40 },
+		{ 0x0C46, 0x0C48 }, { 0x0C4A, 0x0C4D }, { 0x0C55, 0x0C56 },
+		{ 0x0CBC, 0x0CBC }, { 0x0CBF, 0x0CBF }, { 0x0CC6, 0x0CC6 },
+		{ 0x0CCC, 0x0CCD }, { 0x0D41, 0x0D43 }, { 0x0D4D, 0x0D4D },
+		{ 0x0DCA, 0x0DCA }, { 0x0DD2, 0x0DD4 }, { 0x0DD6, 0x0DD6 },
+		{ 0x0E31, 0x0E31 }, { 0x0E34, 0x0E3A }, { 0x0E47, 0x0E4E },
+		{ 0x0EB1, 0x0EB1 }, { 0x0EB4, 0x0EB9 }, { 0x0EBB, 0x0EBC },
+		{ 0x0EC8, 0x0ECD }, { 0x0F18, 0x0F19 }, { 0x0F35, 0x0F35 },
+		{ 0x0F37, 0x0F37 }, { 0x0F39, 0x0F39 }, { 0x0F71, 0x0F7E },
+		{ 0x0F80, 0x0F84 }, { 0x0F86, 0x0F87 }, { 0x0F90, 0x0F97 },
+		{ 0x0F99, 0x0FBC }, { 0x0FC6, 0x0FC6 }, { 0x102D, 0x1030 },
+		{ 0x1032, 0x1032 }, { 0x1036, 0x1037 }, { 0x1039, 0x1039 },
+		{ 0x1058, 0x1059 }, { 0x1160, 0x11FF }, { 0x1712, 0x1714 },
+		{ 0x1732, 0x1734 }, { 0x1752, 0x1753 }, { 0x1772, 0x1773 },
+		{ 0x17B4, 0x17B5 }, { 0x17B7, 0x17BD }, { 0x17C6, 0x17C6 },
+		{ 0x17C9, 0x17D3 }, { 0x17DD, 0x17DD }, { 0x180B, 0x180D },
+		{ 0x18A9, 0x18A9 }, { 0x1920, 0x1922 }, { 0x1927, 0x1928 },
+		{ 0x1932, 0x1932 }, { 0x1939, 0x193B }, { 0x200B, 0x200F },
+		{ 0x202A, 0x202E }, { 0x2060, 0x2063 }, { 0x206A, 0x206F },
+		{ 0x20D0, 0x20EA }, { 0x302A, 0x302F }, { 0x3099, 0x309A },
+		{ 0xFB1E, 0xFB1E }, { 0xFE00, 0xFE0F }, { 0xFE20, 0xFE23 },
+		{ 0xFEFF, 0xFEFF }, { 0xFFF9, 0xFFFB }, { 0x1D167, 0x1D169 },
+		{ 0x1D173, 0x1D182 }, { 0x1D185, 0x1D18B }, { 0x1D1AA, 0x1D1AD },
+		{ 0xE0001, 0xE0001 }, { 0xE0020, 0xE007F }, { 0xE0100, 0xE01EF }
+	};
+	return bisearch(ucs, zero_width,
+		sizeof(zero_width) / sizeof(*zero_width) - 1);
+}
+*/
+
+static int is_double_width(uint32_t ucs)
+{
+	static const struct interval double_width[] = {
+		{ 0x1100, 0x115F }, { 0x2329, 0x232A }, { 0x2E80, 0x303E },
+		{ 0x3040, 0xA4CF }, { 0xAC00, 0xD7A3 }, { 0xF900, 0xFAFF },
+		{ 0xFE30, 0xFE6F }, { 0xFF00, 0xFF60 }, { 0xFFE0, 0xFFE6 },
+		{ 0x20000, 0x2FFFD }, { 0x30000, 0x3FFFD }
+	};
+	return bisearch(ucs, double_width,
+		sizeof(double_width) / sizeof(*double_width) - 1);
+}
+
 /* acquires console_sem */
 static int do_con_write(struct tty_struct *tty, const unsigned char *buf, int count)
 {
@@ -1950,6 +2041,10 @@
 	unsigned int currcons;
 	unsigned long draw_from = 0, draw_to = 0;
 	struct vc_data *vc;
+	unsigned char vc_attr;
+	uint8_t rescan;
+	uint8_t inverse;
+	uint8_t width;
 	u16 himask, charmask;
 	const unsigned char *orig_buf = NULL;
 	int orig_count;
@@ -2012,51 +2107,81 @@
 		buf++;
 		n++;
 		count--;
+		rescan = 0;
+		inverse = 0;
+		width = 1;
 
 		/* Do no translation at all in control states */
 		if (vc->vc_state != ESnormal) {
 			tc = c;
 		} else if (vc->vc_utf && !vc->vc_disp_ctrl) {
-		    /* Combine UTF-8 into Unicode */
-		    /* Malformed sequences as sequences of replacement glyphs */
+		    /* Combine UTF-8 into Unicode in vc_utf_char */
+		    /* vc_utf_count is the number of continuation bytes still expected to arrive */
+		    /* vc_npar is the number of continuation bytes arrived so far */
 rescan_last_byte:
-		    if(c > 0x7f) {
+		    if ((c & 0xc0) == 0x80) {
+			/* Continuation byte received */
+			static const uint32_t utf8_length_changes[] = { 0x0000007f, 0x000007ff, 0x0000ffff, 0x001fffff, 0x03ffffff, 0x7fffffff };
 			if (vc->vc_utf_count) {
-			       if ((c & 0xc0) == 0x80) {
-				       vc->vc_utf_char = (vc->vc_utf_char << 6) | (c & 0x3f);
-       				       if (--vc->vc_utf_count) {
-					       vc->vc_npar++;
-				   	       continue;
-       				       }
-				       tc = c = vc->vc_utf_char;
-			       } else
-				       goto replacement_glyph;
-			} else {
-				vc->vc_npar = 0;
-				if ((c & 0xe0) == 0xc0) {
-				    vc->vc_utf_count = 1;
-				    vc->vc_utf_char = (c & 0x1f);
-				} else if ((c & 0xf0) == 0xe0) {
-				    vc->vc_utf_count = 2;
-				    vc->vc_utf_char = (c & 0x0f);
-				} else if ((c & 0xf8) == 0xf0) {
-				    vc->vc_utf_count = 3;
-				    vc->vc_utf_char = (c & 0x07);
-				} else if ((c & 0xfc) == 0xf8) {
-				    vc->vc_utf_count = 4;
-				    vc->vc_utf_char = (c & 0x03);
-				} else if ((c & 0xfe) == 0xfc) {
-				    vc->vc_utf_count = 5;
-				    vc->vc_utf_char = (c & 0x01);
-				} else
-	    			    goto replacement_glyph;
+			    vc->vc_utf_char = (vc->vc_utf_char << 6) | (c & 0x3f);
+			    vc->vc_npar++;
+			    if (--vc->vc_utf_count) {
+				/* Still need some bytes */
 				continue;
-			      }
+			    }
+			    /* Got a whole character */
+			    c = vc->vc_utf_char;
+			    /* Reject overlong sequences */
+			    if (c <= utf8_length_changes[vc->vc_npar - 1] || c > utf8_length_changes[vc->vc_npar]) {
+				c = 0xfffd;
+			    }
+			} else {
+			    /* Unexpected continuation byte */
+			    vc->vc_utf_count = 0;
+			    c = 0xfffd;
+			}
 		    } else {
-		      if (vc->vc_utf_count)
-	  		      goto replacement_glyph;
-		      tc = c;
+			/* Single ASCII byte or first byte of a sequence received */
+			if (vc->vc_utf_count) {
+			    /* Continuation byte expected */
+			    rescan = 1;
+			    vc->vc_utf_count = 0;
+			    c = 0xfffd;
+			} else if (c > 0x7f) {
+			    /* First byte of a multibyte sequence received */
+			    vc->vc_npar = 0;
+			    if ((c & 0xe0) == 0xc0) {
+				vc->vc_utf_count = 1;
+				vc->vc_utf_char = (c & 0x1f);
+			    } else if ((c & 0xf0) == 0xe0) {
+				vc->vc_utf_count = 2;
+				vc->vc_utf_char = (c & 0x0f);
+			    } else if ((c & 0xf8) == 0xf0) {
+				vc->vc_utf_count = 3;
+				vc->vc_utf_char = (c & 0x07);
+			    } else if ((c & 0xfc) == 0xf8) {
+				vc->vc_utf_count = 4;
+				vc->vc_utf_char = (c & 0x03);
+			    } else if ((c & 0xfe) == 0xfc) {
+				vc->vc_utf_count = 5;
+				vc->vc_utf_char = (c & 0x01);
+			    } else {
+				/* 254 and 255 are invalid */
+				c = 0xfffd;
+			    }
+			    if (vc->vc_utf_count) {
+				/* Still need some bytes */
+				continue;
+			    }
+			}
+			/* Nothing to do if an ASCII byte was received */
 		    }
+		    /* End of UTF-8 decoding. */
+		    /* c is the received character, or U+FFFD for invalid sequences. */
+		    /* Replace invalid Unicode code points with U+FFFD too */
+		    if ((c >= 0xd800 && c <= 0xdfff) || c == 0xfffe || c == 0xffff)
+			c = 0xfffd;
+		    tc = c;
 		} else {	/* no utf or alternate charset mode */
 		  tc = vc->vc_translate[vc->vc_toggle_meta ? (c | 0x80) : c];
 		}
@@ -2078,56 +2203,89 @@
 			&& (c != 128+27);
 
 		if (vc->vc_state == ESnormal && ok) {
+			if (vc->vc_utf && !vc->vc_disp_ctrl) {
+				if (is_double_width(c)) {
+					width = 2;
+				}
+				/* Uncomment this and the definition of is_zero_width()
+				   if you'd prefer zero-width characters to be silently
+				   dropped instead of printing the replacement glyph */
+				/*
+				else if (is_zero_width(c)) {
+					continue;
+				}
+				*/
+			}
 			/* Now try to find out how to display it */
 			tc = conv_uni_to_pc(vc, tc);
 			if (tc & ~charmask) {
-				if ( tc == -4 ) {
-                                /* If we got -4 (not found) then see if we have
-                                   defined a replacement character (U+FFFD) */
-replacement_glyph:
-                                	tc = conv_uni_to_pc(vc, 0xfffd);
-					if (!(tc & ~charmask))
-						goto display_glyph;
-                        	} else if ( tc != -3 )
-                                	continue; /* nothing to display */
-                                /* no hash table or no replacement --
-				 * hope for the best */
-				if ( c & ~charmask )
-					tc = '?';
-				else
-					tc = c;
+				if (tc == -1 || tc == -2) {
+				    continue; /* nothing to display */
+				}
+				/* Glyph not found */
+				if (!(vc->vc_utf && !vc->vc_disp_ctrl) && !(c & ~charmask)) {
+				    /* In legacy mode use the glyph we get by a 1:1 mapping.
+				       This would make absolutely no sense with Unicode in mind. */
+				    tc = c;
+				} else {
+				    /* Display U+FFFD. If it's not found, display an inverse question mark. */
+				    tc = conv_uni_to_pc(vc, 0xfffd);
+				    if (tc < 0) {
+					inverse = 1;
+					tc = conv_uni_to_pc(vc, '?');
+					if (tc < 0) tc = '?';
+				    }
+				}
 			}
 
-display_glyph:
-			if (vc->vc_need_wrap || vc->vc_decim)
-				FLUSH
-			if (vc->vc_need_wrap) {
-				cr(vc);
-				lf(vc);
-			}
-			if (vc->vc_decim)
-				insert_char(vc, 1);
-			scr_writew(himask ?
-				     ((vc->vc_attr << 8) & ~himask) + ((tc & 0x100) ? himask : 0) + (tc & 0xff) :
-				     (vc->vc_attr << 8) + tc,
-				   (u16 *) vc->vc_pos);
-			if (DO_UPDATE(vc) && draw_x < 0) {
-				draw_x = vc->vc_x;
-				draw_from = vc->vc_pos;
-			}
-			if (vc->vc_x == vc->vc_cols - 1) {
-				vc->vc_need_wrap = vc->vc_decawm;
-				draw_to = vc->vc_pos + 2;
+			if (!inverse) {
+				vc_attr = vc->vc_attr;
 			} else {
-				vc->vc_x++;
-				draw_to = (vc->vc_pos += 2);
+				/* invert vc_attr */
+				if (!vc->vc_can_do_color) {
+					vc_attr = (vc->vc_attr) ^ 0x08;
+				} else if (vc->vc_hi_font_mask == 0x100) {
+					vc_attr = ((vc->vc_attr) & 0x11) | (((vc->vc_attr) & 0xe0) >> 4) | (((vc->vc_attr) & 0x0e) << 4);
+				} else {
+					vc_attr = ((vc->vc_attr) & 0x88) | (((vc->vc_attr) & 0x70) >> 4) | (((vc->vc_attr) & 0x07) << 4);
+				}
 			}
-			if (vc->vc_utf_count) {
-				if (vc->vc_npar) {
-					vc->vc_npar--;
-					goto display_glyph;
+
+			while (1) {
+				if (vc->vc_need_wrap || vc->vc_decim)
+					FLUSH
+				if (vc->vc_need_wrap) {
+					cr(vc);
+					lf(vc);
+				}
+				if (vc->vc_decim)
+					insert_char(vc, 1);
+				scr_writew(himask ?
+					     ((vc_attr << 8) & ~himask) + ((tc & 0x100) ? himask : 0) + (tc & 0xff) :
+					     (vc_attr << 8) + tc,
+					   (u16 *) vc->vc_pos);
+				if (DO_UPDATE(vc) && draw_x < 0) {
+					draw_x = vc->vc_x;
+					draw_from = vc->vc_pos;
+				}
+				if (vc->vc_x == vc->vc_cols - 1) {
+					vc->vc_need_wrap = vc->vc_decawm;
+					draw_to = vc->vc_pos + 2;
+				} else {
+					vc->vc_x++;
+					draw_to = (vc->vc_pos += 2);
 				}
-				vc->vc_utf_count = 0;
+
+				if (!--width) break;
+
+				tc = conv_uni_to_pc(vc, ' '); /* A space is printed in the second column */
+				if (tc < 0) tc = ' ';
+			}
+
+			if (rescan) {
+				rescan = 0;
+				inverse = 0;
+				width = 1;
 				c = orig;
 				goto rescan_last_byte;
 			}

Re: [PATCH] console UTF-8 fixes

[Alan Cox]

> So please accept these hard-coded tables in the first round. Maybe one day
> somebody will come up with a better solution. This one should be okay until
> then. (I can also send the script I've written so he can improve it.)

You can pack them a little differently and they'll shrink a lot.

Firstly store
		start, size

Secondly pack the offset as 16 bit with the top 4 bits

		0-3 value
		4 - F
		5 - 1D
		6 - E0

And the size as an 8 bit range. You need to open code the rule for 
20000/30000 but that is tiny anyway.

Unpack is trivial

	start = (toptab[*entry>>4]|(*entry&0x0F)) << 8 | entry[1];
	end = start + entry[2];

Re: [PATCH] console UTF-8 fixes

[Egmont Koblinger]

On Thu, Apr 12, 2007 at 02:13:06PM +0100, Alan Cox wrote:

> You can pack them a little differently and they'll shrink a lot.

The smaller table would actually slightly grow instead of shrinking. In my
patch there are 11 intervals, each consume 2*4 bytes, that's 88 bytes. Your
variant would store each interval in 3 bytes, that's 33 bytes so far, plus
you need 4 byte values in the toptab array, that's 64 more bytes.

The larger table is 984 byte large now (123 intervals). You cannot compress
each interval to 3 bytes (at least not the way you described) since the
index for toptab needs more than 16 possible values. But it's easily
possible to compresss each interval in 4 bytes. That's 492 bytes for the
intervals themselves, plus at least 64 bytes for toptab (oh, probably it's
possible to store 3 byte values in toptab, too).

You might gain approx. 400 bytes in a table that's actually commented out in
the current patch since HPA doesn't like the behavior where it would be
used. On the other hand, you get a database that is harder to understand,
maintain, verify, you get code that runs a little bit slower. We're from
different worlds. You're a great kernel hacker and bit-magician. I'm rather
developing applications, so for me having a less straightforward code to
save 400 bytes is simply not worth it.

Which version would fit in the spirit of the kernel better? I don't know,
I'd let you decide it :)


bye,
Egmont

Re: [PATCH] console UTF-8 fixes

[Roman Zippel]

Hi,

On Thu, 12 Apr 2007, Egmont Koblinger wrote:

> I tried to create such a script using ideas for regexps from glibc's
> charmaps/UTF-8, but it seemed to be quite hopeless to create a small table.
> It seems that Markus probably performed some reasonal manual optimisations
> that cannot really be scripted. For example, within a huge CJK area there
> are plenty of codepoints that are not (yet?) assigned. However, if they'll
> be assigned in the future, most likely they'll be double-wide characters
> too.

Considering this possible volatility I'm not certain we really need this 
in the kernel.
The other point is that I have problems imagining, that this should be 
enough to edit random text files with a random editor without problems. 
Especially cursor positioning becomes a whole lot more complex and the 
editor has to do exactly the same parsing as the kernel, i.e. changes to 
this table are not easily possible anymore or later you may need to add 
the possibility to read the table.
OTOH if the editor has to all this parsing anyway, the whole thing could 
be pushed to userspace and the Linux terminal could be marked as handling 
all characters equally (a good hint would be if the terminal doesn't even 
support wide characters). The terminfo database exists for a good reason 
and a good editor has to support a wide range of terminals anyway, so I 
don't really see the problem as to mark variable width characters as not 
supported and keep the kernel implementation sane and simple.

bye, Roman

Re: [PATCH] console UTF-8 fixes

[Egmont Koblinger]

On Thu, Apr 12, 2007 at 04:38:54PM +0200, Roman Zippel wrote:

> Considering this possible volatility I'm not certain we really need this 
> in the kernel.
> The other point is that I have problems imagining, that this should be 
> enough to edit random text files with a random editor without problems. 

No, this would not be enough for all special corner cases. It would just be
better than currently it is. That's my goal.

> OTOH if the editor has to all this parsing anyway, the whole thing could 
> be pushed to userspace and the Linux terminal could be marked as handling 
> all characters equally (a good hint would be if the terminal doesn't even 
> support wide characters). The terminfo database exists for a good reason 

Currently not even applications and terminal emulators agree on the width. I
don't think terminfo has anything to do with it. Width information should
come from glibc, but unfortunately its database is quite old, so
applications tend to implement their own version. (Example: according to
glibc 2.5, U+0221 is not printable. Still it's present in many fonts, and at
least vim and joe display them.)

So, there are several (maybe a few hundred or few thousands) characters that
are handled differently by different applications/libraries. On the other
hand, there are approximately 42.000 characters in BMP that are double-width
according to every width specification or implementation I've seen so far.
That's roughly the 2/3 of all the code points in BMP. Why is a problem if
the kernel knows to jump 2 character cells on them?

I'm not seeking for a perfect solution. (Taking a look at the current state
of specs/libs/apps perfect solution doesn't even exist.) I'm seeking for
something that is just way better than the current situation.


-- 
Egmont

Re: [PATCH] console UTF-8 fixes

[Roman Zippel]

Hi,

On Thu, 12 Apr 2007, Egmont Koblinger wrote:

> > Considering this possible volatility I'm not certain we really need this 
> > in the kernel.
> > The other point is that I have problems imagining, that this should be 
> > enough to edit random text files with a random editor without problems. 
> 
> No, this would not be enough for all special corner cases. It would just be
> better than currently it is. That's my goal.

Well, it often doesn't end there, other users may report these as bugs and 
want to get them fixed, so we have to look ahead a little for possible 
problems.

> > OTOH if the editor has to all this parsing anyway, the whole thing could 
> > be pushed to userspace and the Linux terminal could be marked as handling 
> > all characters equally (a good hint would be if the terminal doesn't even 
> > support wide characters). The terminfo database exists for a good reason 
> 
> Currently not even applications and terminal emulators agree on the width. I
> don't think terminfo has anything to do with it.

It has everything to do with this. It describes the terminal capabilities, 
so why should variable width support not one part of it?

> So, there are several (maybe a few hundred or few thousands) characters that
> are handled differently by different applications/libraries. On the other
> hand, there are approximately 42.000 characters in BMP that are double-width
> according to every width specification or implementation I've seen so far.
> That's roughly the 2/3 of all the code points in BMP. Why is a problem if
> the kernel knows to jump 2 character cells on them?

Wrong question: why is it a problem the kernel doesn't know this?
It's usually easy to "fix" this in the kernel, because it's last in line, 
but we have to look at the whole picture and the kernel isn't solely 
responsible for unicode handling, so why can't this be fixed at 
application level?

bye, Roman

Re: [PATCH] console UTF-8 fixes

[Egmont Koblinger]

On Thu, Apr 12, 2007 at 05:52:49PM +0200, Roman Zippel wrote:

> Well, it often doesn't end there, other users may report these as bugs and 
> want to get them fixed, so we have to look ahead a little for possible 
> problems.

They may even report that the current behavior of not knowing anything about
double-width characters is wrong. If Unicode is updated and someone reports
the kernel code is outdated, it's going to be fairly easy to update that one
as well.

I still can't see why my proposed version is by any means worse than the
current one. (I can see however that my version is still not perfect.)

> It has everything to do with this. It describes the terminal capabilities, 
> so why should variable width support not one part of it?

Nice idea. Terminfo database should tell whether a particular terminal
supports double-width characters. Applications should expect the cursor to
move one or two cells depending on this. So both terminfo databases as well
as plenty of libraries and applications would need update. I'll ask Thomas
E. Dickey for his opinion.


-- 
Egmont

Re: [PATCH] console UTF-8 fixes

[Roman Zippel]

Hi,

On Thu, 12 Apr 2007, Egmont Koblinger wrote:

> On Thu, Apr 12, 2007 at 05:52:49PM +0200, Roman Zippel wrote:
> 
> > Well, it often doesn't end there, other users may report these as bugs and 
> > want to get them fixed, so we have to look ahead a little for possible 
> > problems.
> 
> They may even report that the current behavior of not knowing anything about
> double-width characters is wrong. If Unicode is updated and someone reports
> the kernel code is outdated, it's going to be fairly easy to update that one
> as well.

As far as Unicode is concerned we could go really wild here, e.g. how do 
we handle decomposed characters, there are also halfwidth characters...
So the primary question is still: is this a kernel problem? The kernel 
only implements a simple fixed char width output device. Does it really 
make sense to pretend being able to handle characters, we can't display 
in first place?

> > It has everything to do with this. It describes the terminal capabilities, 
> > so why should variable width support not one part of it?
> 
> Nice idea. Terminfo database should tell whether a particular terminal
> supports double-width characters. Applications should expect the cursor to
> move one or two cells depending on this. So both terminfo databases as well
> as plenty of libraries and applications would need update.

If everyone reimplements this kind of stuff, something is seriously 
wrong...

bye, Roman

Re: [PATCH] console UTF-8 fixes

[Jan Engelhardt]

On Apr 11 2007 20:28, Egmont Koblinger wrote:

>I send a reworked version of the patch.
>
>Removed from the first version:
>  - any sign of '.' as substitute glyph
>  - don't ignore zero-width characters (except for a few zero-width spaces
>    that are ignored in the current kernel too). However, I kept the code
>    organized and commented so that someone can have the other behavior very
>    easily (by removing a pair of comment signs).
>
>Kept features, fixes:
>  - lots of UTF-8 decoder fixes. Emit one U+FFFD for every standalone
>    continuation byte and for every incomplete sequence, as Markus Kuhn
>    recommends. Reject overlong sequences too.
>  - D800..DFFF and FFFE..FFFF are substituted by FFFD too, since these are
>    not valid Unicode code points.
>  - no "random" replacement glyph (e.g. u with double acute instead of
>    u with circumflex) in UTF-8 mode
>  - if U+FFFD is not found in the font, the fallback replacement '?' (ascii
>    question mark) is printed with inverse color attributes
>  - U+200A was ignored so far as a zero-width space character. I think it
>    was a mistake, it's not zero-width.
>  - print an extra space for double-wide characters for the cursor to stand
>    at the right place. Yet again the code is organized so that it is very
>    easy to change to jump only one character cell, should someone prefer
>    that behavior (which I still see no good reason to).
>
>Signed-off-by: Egmont Koblinger <egmont@uhulinux.hu>
>
>@@ -1934,6 +1943,99 @@
> char con_buf[CON_BUF_SIZE];
> DECLARE_MUTEX(con_buf_sem);
> 
>+/* is_{zero,double}_width() are based on the wcwidth() implementation by
>+ * Markus Kuhn -- 2003-05-20 (Unicode 4.0)
>+ * Latest version: http://www.cl.cam.ac.uk/~mgk25/ucs/wcwidth.c
>+ */
>+struct interval {
>+  int first;
>+  int last;
>+};

CodingStyle? uint16_t instead of int?

>+static int is_zero_width(long ucs)
>+{
>+  static const struct interval zero_width[] = {
>+    { 0x0300, 0x0357 }, { 0x035D, 0x036F }, { 0x0483, 0x0486 },
[...]
>+    { 0xFB1E, 0xFB1E }, { 0xFE00, 0xFE0F }, { 0xFE20, 0xFE23 },
>+    { 0xFEFF, 0xFEFF }, { 0xFFF9, 0xFFFB }, { 0x1D167, 0x1D169 },
>+    { 0x1D173, 0x1D182 }, { 0x1D185, 0x1D18B }, { 0x1D1AA, 0x1D1AD },
>+    { 0xE0001, 0xE0001 }, { 0xE0020, 0xE007F }, { 0xE0100, 0xE01EF }
>+  };

Since Unicode above 0xFFFF is unsupported, could not these entries be killed?

>+static int is_double_width(long ucs)
>+{
>+  static const struct interval double_width[] = {
>+    { 0x1100, 0x115F }, { 0x2329, 0x232A }, { 0x2E80, 0x303E },
>+    { 0x3040, 0xA4CF }, { 0xAC00, 0xD7A3 }, { 0xF900, 0xFAFF },
>+    { 0xFE30, 0xFE6F }, { 0xFF00, 0xFF60 }, { 0xFFE0, 0xFFE6 },
>+    { 0x20000, 0x2FFFD }, { 0x30000, 0x3FFFD }
>+  };

Similarly.

>@@ -1950,6 +2052,10 @@
> 	unsigned int currcons;
> 	unsigned long draw_from = 0, draw_to = 0;
> 	struct vc_data *vc;
>+	unsigned char vc_attr;
>+	int rescan;
unsigned int rescan:1;
>+	int inverse;
unsigned int inverse:1;
>+	int width;
unsigned int width; or even uint8_t.

> 	u16 himask, charmask;
> 	const unsigned char *orig_buf = NULL;
> 	int orig_count;

>@@ -2012,51 +2118,81 @@
> 		buf++;
> 		n++;
> 		count--;
>+		rescan = 0;
>+		inverse = 0;
>+		width = 1;
> 
> 		/* Do no translation at all in control states */
> 		if (vc->vc_state != ESnormal) {
> 			tc = c;
> 		} else if (vc->vc_utf && !vc->vc_disp_ctrl) {
>-		    /* Combine UTF-8 into Unicode */
>-		    /* Malformed sequences as sequences of replacement glyphs */
>+		    /* Combine UTF-8 into Unicode in vc_utf_char */
>+		    /* vc_utf_count is the number of continuation bytes still expected to arrive */
>+		    /* vc_npar is the number of continuation bytes arrived so far */
> rescan_last_byte:
>-		    if(c > 0x7f) {
>+		    if ((c & 0xc0) == 0x80) {
>+			/* Continuation byte received */
>+			static const int utf8_length_changes[] = { 0x0000007f, 0x000007ff, 0x0000ffff, 0x001fffff, 0x03ffffff, 0x7fffffff };

I would not mind unsigned.


Jan
-- 

Re: [PATCH] console UTF-8 fixes

[Egmont Koblinger]

On Wed, Apr 11, 2007 at 09:00:49PM +0200, Jan Engelhardt wrote:

> >+struct interval {
> >+  int first;
> >+  int last;
> >+};
> 
> CodingStyle? uint16_t instead of int?

> >+    { 0x1D173, 0x1D182 }, { 0x1D185, 0x1D18B }, { 0x1D1AA, 0x1D1AD },
> >+    { 0xE0001, 0xE0001 }, { 0xE0020, 0xE007F }, { 0xE0100, 0xE01EF }
> >+  };
> 
> Since Unicode above 0xFFFF is unsupported, could not these entries be killed?

The UTF-8 decoder part already supports full 31-bit Unicode (including 5 and
6 byte long UTF-8 sequences). It's only the font handling part that doesn't
support Unicode beyond BMP. If an application prints a non-BMP character
that is double-wide, or is a zero-width space, the expected behavior is to
move the cursor by two or zero positions. In order to do this, width
information is needed even beyond BMP.  It's a completely different story
that there would be no real glyph displayed, just e.g. a replacement symbol
followed by a space to pretend a real double-width character was printed.


> unsigned int rescan:1;
> unsigned int inverse:1;
> unsigned int width; or even uint8_t.
> I would not mind unsigned.

Okay.



-- 
Egmont

Re: [PATCH] console UTF-8 fixes

[Pavel Machek]

Hi!

> I hope you like it. :)

Well, more or less... but you need signed-off-by line, and

> @@ -70,6 +70,16 @@
>   * malformed UTF sequences represented as sequences of replacement glyphs,
>   * original codes or '?' as a last resort if replacement glyph is undefined
>   * by Adam Tla/lka <atlka@pg.gda.pl>, Aug 2006
> + *
> + * More robust UTF-8 decoder. Make it work on malformed sequences as Markus Kuhn's
> + * UTF-8 decoder stress test suggests. Emit a U+FFFD on illegal sequences as well
> + * as for invalid Unicode code points.
> + * If U+FFFD is not available in the font, print an inverse question mark instead.
> + * Display an inverted dot for valid characters that are not available in the font.
> + * Do not print zero-width characters, pad double-width characters with an extra
> + * space so that the cursor moves by zero/two positions in these cases.
> + * 6 April 2007, Egmont Koblinger <egmont@uhulinux.hu>,
> + * using Markus Kuhn's wcwidth() implementation.
>   */

We no longer put changelogs in code.

> +/* wcwidth() based on the implementation by
> + * Markus Kuhn -- 2003-05-20 (Unicode 4.0)
> + * Latest version: http://www.cl.cam.ac.uk/~mgk25/ucs/wcwidth.c
> + */
> +struct interval {
> +  int first;
> +  int last;
> +};
> +
> +static int bisearch(long ucs, const struct interval *table, int max) {
> +  int min = 0;
> +  int mid;
> +
> +  if (ucs < table[0].first || ucs > table[max].last)
> +    return 0;

...and you really need to read coding style.

> +  while (max >= min) {
> +    mid = (min + max) / 2;
> +    if (ucs > table[mid].last)
> +      min = mid + 1;
> +    else if (ucs < table[mid].first)
> +      max = mid - 1;
> +    else
> +      return 1;
> +  }
> +
> +  return 0;
> +}

(Don't we already have rbtrees handling this just fine?)

							Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] console UTF-8 fixes

[Jan Engelhardt]

On Apr 11 2007 19:36, Pavel Machek wrote:
>
>> +  while (max >= min) {
>> +    mid = (min + max) / 2;
>> +    if (ucs > table[mid].last)
>> +      min = mid + 1;
>> +    else if (ucs < table[mid].first)
>> +      max = mid - 1;
>> +    else
>> +      return 1;
>> +  }
>> +
>> +  return 0;
>> +}
>
>(Don't we already have rbtrees handling this just fine?)

Static initialization of an rbtre would become a beauty.


Jan
-- 

[PATCH] console UTF-8 fixes

[Egmont Koblinger]

Hi Andrew,

I've been told to send this patch to you for inclusion in your patchset, and
hopefully sooner or later in the mainline kernel too. It has been discussed
on lkml and finally found to be OK by HPA and Jan.

The UTF-8 part of the vt driver suffers from the following issues which are
addressed in my patch:

1) If there's no glyph found for a particular valid UTF-8 character, we try
   to display U+FFFD. However if this one is not found either, here's what
   the current kernel does:

   - First, if the Unicode value is less than the number of glyphs, use the
     glyph directly from that position of the glyph table. While it may be a
     good idea in the 8-bit world, it has absolutely no sense with Unicode
     in mind. For example, if a Latin-2 font is loaded and an application
     prints U+00FB ("u with circumflex", not present in Latin-2) then as a
     fallback solution the glyph from the 0xFB position of the Latin-2
     fontset (which is an "u with double accent" - a different character) is
     displayed.

   - Second, if this fallback fails too, a simple ASCII question mark is
     printed, which is visually undistinguishable from a real question mark.

   I changed the code to skip the first step (except if in non-UTF-8 mode),
   and changed the second step to print the question mark with inverse color
   attributes, so it is visually clear that it's not a real question mark,
   and resembles more to the common glyph of U+FFFD.

2) The UTF-8 decoder is buggy in many ways:

   - Lone continuation bytes (section 3.1 of Markus Kuhn's UTF-8 stress
     test) are not caught, they are displayed as some "random" (taken
     directly form the font table, see above) glyphs instead the replacement
     character.

   - Incomplete sequences (sections 3.2 and 3.3 of the stress test) emit no
     replacement character, but rather cause the subsequent valid character
     to be displayed more times(!).

   - The decoder is not safe: overlong sequences are not caught currently,
     they are displayed as if these were valid representations. This may
     even have security impacts.

   - The decoder does not handle D800..DFFF and FFFE..FFFF specially, it
     just emits these code points and lets it be looked up in the glyph
     table. Since these are invalid code points, I replace them by U+FFFD
     and hence give no chance for them to be looked up in the glyph table. 
     (Assuming no font ships glyphs for these code points, this change is
     not visible to the users since the glyph shown will be the same.)

   With my fixes to the decoder it now behaves exactly as Markus Kuhn's
   stress test recommends.

3) It has no concept of double-width (CJK) characters. It's way beyond the
   scope of my patch to try to display them, but at least I think it's
   important for the cursor to jump two positions when printing such
   characters, since this is what applications (such as text editors)
   expect. Currently the cursor only jumps one position, and hence
   applications suffer from displaying and refreshing problems, and editing
   some English letters that are preceded by some CJK characters in the same
   line is a nightmare. With my patch an additional space is inserted after
   the CJK character has been printed (which usually means a replacement
   symbol of course). (If U+FFFD isn't availble and hence an inverse
   question mark is displayed in the first cell, I keep the inverted state
   for the space in the 2nd column so it's quite easy to see that they are
   tied together.)

4) There is a small built-in table of zero-width spaces that are not to be
   printed but silently skipped. U+200A is included there, but it's not a
   zero-width character, so I remove it from there.


The patch is exactly the same as I've sent to the list the last time
(labelled as "version 4").


Signed-off-by: Egmont Koblinger <egmont@uhulinux.hu>

diff -Naur linux-2.6.20.orig/drivers/char/consolemap.c linux-2.6.20/drivers/char/consolemap.c
--- linux-2.6.20.orig/drivers/char/consolemap.c	2007-02-04 19:44:54.000000000 +0100
+++ linux-2.6.20/drivers/char/consolemap.c	2007-04-11 18:45:45.000000000 +0200
@@ -626,10 +626,10 @@
   
 	/* Only 16-bit codes supported at this time */
 	if (ucs > 0xffff)
-		ucs = 0xfffd;		/* U+FFFD: REPLACEMENT CHARACTER */
-	else if (ucs < 0x20 || ucs >= 0xfffe)
+		return -4;		/* Not found */
+	else if (ucs < 0x20)
 		return -1;		/* Not a printable character */
-	else if (ucs == 0xfeff || (ucs >= 0x200a && ucs <= 0x200f))
+	else if (ucs == 0xfeff || (ucs >= 0x200b && ucs <= 0x200f))
 		return -2;			/* Zero-width space */
 	/*
 	 * UNI_DIRECT_BASE indicates the start of the region in the User Zone
diff -Naur linux-2.6.20.orig/drivers/char/vt.c linux-2.6.20/drivers/char/vt.c
--- linux-2.6.20.orig/drivers/char/vt.c	2007-02-04 19:44:54.000000000 +0100
+++ linux-2.6.20/drivers/char/vt.c	2007-04-12 19:38:51.000000000 +0200
@@ -1934,6 +1934,46 @@
 char con_buf[CON_BUF_SIZE];
 DECLARE_MUTEX(con_buf_sem);
 
+/* is_double_width() is based on the wcwidth() implementation by
+ * Markus Kuhn -- 2003-05-20 (Unicode 4.0)
+ * Latest version: http://www.cl.cam.ac.uk/~mgk25/ucs/wcwidth.c
+ */
+struct interval {
+	uint32_t first;
+	uint32_t last;
+};
+
+static int bisearch(uint32_t ucs, const struct interval *table, int max)
+{
+	int min = 0;
+	int mid;
+
+	if (ucs < table[0].first || ucs > table[max].last)
+		return 0;
+	while (max >= min) {
+		mid = (min + max) / 2;
+		if (ucs > table[mid].last)
+			min = mid + 1;
+		else if (ucs < table[mid].first)
+			max = mid - 1;
+		else
+			return 1;
+	}
+	return 0;
+}
+
+static int is_double_width(uint32_t ucs)
+{
+	static const struct interval double_width[] = {
+		{ 0x1100, 0x115F }, { 0x2329, 0x232A }, { 0x2E80, 0x303E },
+		{ 0x3040, 0xA4CF }, { 0xAC00, 0xD7A3 }, { 0xF900, 0xFAFF },
+		{ 0xFE30, 0xFE6F }, { 0xFF00, 0xFF60 }, { 0xFFE0, 0xFFE6 },
+		{ 0x20000, 0x2FFFD }, { 0x30000, 0x3FFFD }
+	};
+	return bisearch(ucs, double_width,
+		sizeof(double_width) / sizeof(*double_width) - 1);
+}
+
 /* acquires console_sem */
 static int do_con_write(struct tty_struct *tty, const unsigned char *buf, int count)
 {
@@ -1950,6 +1990,10 @@
 	unsigned int currcons;
 	unsigned long draw_from = 0, draw_to = 0;
 	struct vc_data *vc;
+	unsigned char vc_attr;
+	uint8_t rescan;
+	uint8_t inverse;
+	uint8_t width;
 	u16 himask, charmask;
 	const unsigned char *orig_buf = NULL;
 	int orig_count;
@@ -2012,51 +2056,81 @@
 		buf++;
 		n++;
 		count--;
+		rescan = 0;
+		inverse = 0;
+		width = 1;
 
 		/* Do no translation at all in control states */
 		if (vc->vc_state != ESnormal) {
 			tc = c;
 		} else if (vc->vc_utf && !vc->vc_disp_ctrl) {
-		    /* Combine UTF-8 into Unicode */
-		    /* Malformed sequences as sequences of replacement glyphs */
+		    /* Combine UTF-8 into Unicode in vc_utf_char */
+		    /* vc_utf_count is the number of continuation bytes still expected to arrive */
+		    /* vc_npar is the number of continuation bytes arrived so far */
 rescan_last_byte:
-		    if(c > 0x7f) {
+		    if ((c & 0xc0) == 0x80) {
+			/* Continuation byte received */
+			static const uint32_t utf8_length_changes[] = { 0x0000007f, 0x000007ff, 0x0000ffff, 0x001fffff, 0x03ffffff, 0x7fffffff };
 			if (vc->vc_utf_count) {
-			       if ((c & 0xc0) == 0x80) {
-				       vc->vc_utf_char = (vc->vc_utf_char << 6) | (c & 0x3f);
-       				       if (--vc->vc_utf_count) {
-					       vc->vc_npar++;
-				   	       continue;
-       				       }
-				       tc = c = vc->vc_utf_char;
-			       } else
-				       goto replacement_glyph;
-			} else {
-				vc->vc_npar = 0;
-				if ((c & 0xe0) == 0xc0) {
-				    vc->vc_utf_count = 1;
-				    vc->vc_utf_char = (c & 0x1f);
-				} else if ((c & 0xf0) == 0xe0) {
-				    vc->vc_utf_count = 2;
-				    vc->vc_utf_char = (c & 0x0f);
-				} else if ((c & 0xf8) == 0xf0) {
-				    vc->vc_utf_count = 3;
-				    vc->vc_utf_char = (c & 0x07);
-				} else if ((c & 0xfc) == 0xf8) {
-				    vc->vc_utf_count = 4;
-				    vc->vc_utf_char = (c & 0x03);
-				} else if ((c & 0xfe) == 0xfc) {
-				    vc->vc_utf_count = 5;
-				    vc->vc_utf_char = (c & 0x01);
-				} else
-	    			    goto replacement_glyph;
+			    vc->vc_utf_char = (vc->vc_utf_char << 6) | (c & 0x3f);
+			    vc->vc_npar++;
+			    if (--vc->vc_utf_count) {
+				/* Still need some bytes */
 				continue;
-			      }
+			    }
+			    /* Got a whole character */
+			    c = vc->vc_utf_char;
+			    /* Reject overlong sequences */
+			    if (c <= utf8_length_changes[vc->vc_npar - 1] || c > utf8_length_changes[vc->vc_npar]) {
+				c = 0xfffd;
+			    }
+			} else {
+			    /* Unexpected continuation byte */
+			    vc->vc_utf_count = 0;
+			    c = 0xfffd;
+			}
 		    } else {
-		      if (vc->vc_utf_count)
-	  		      goto replacement_glyph;
-		      tc = c;
+			/* Single ASCII byte or first byte of a sequence received */
+			if (vc->vc_utf_count) {
+			    /* Continuation byte expected */
+			    rescan = 1;
+			    vc->vc_utf_count = 0;
+			    c = 0xfffd;
+			} else if (c > 0x7f) {
+			    /* First byte of a multibyte sequence received */
+			    vc->vc_npar = 0;
+			    if ((c & 0xe0) == 0xc0) {
+				vc->vc_utf_count = 1;
+				vc->vc_utf_char = (c & 0x1f);
+			    } else if ((c & 0xf0) == 0xe0) {
+				vc->vc_utf_count = 2;
+				vc->vc_utf_char = (c & 0x0f);
+			    } else if ((c & 0xf8) == 0xf0) {
+				vc->vc_utf_count = 3;
+				vc->vc_utf_char = (c & 0x07);
+			    } else if ((c & 0xfc) == 0xf8) {
+				vc->vc_utf_count = 4;
+				vc->vc_utf_char = (c & 0x03);
+			    } else if ((c & 0xfe) == 0xfc) {
+				vc->vc_utf_count = 5;
+				vc->vc_utf_char = (c & 0x01);
+			    } else {
+				/* 254 and 255 are invalid */
+				c = 0xfffd;
+			    }
+			    if (vc->vc_utf_count) {
+				/* Still need some bytes */
+				continue;
+			    }
+			}
+			/* Nothing to do if an ASCII byte was received */
 		    }
+		    /* End of UTF-8 decoding. */
+		    /* c is the received character, or U+FFFD for invalid sequences. */
+		    /* Replace invalid Unicode code points with U+FFFD too */
+		    if ((c >= 0xd800 && c <= 0xdfff) || c == 0xfffe || c == 0xffff)
+			c = 0xfffd;
+		    tc = c;
 		} else {	/* no utf or alternate charset mode */
 		  tc = vc->vc_translate[vc->vc_toggle_meta ? (c | 0x80) : c];
 		}
@@ -2078,56 +2152,81 @@
 			&& (c != 128+27);
 
 		if (vc->vc_state == ESnormal && ok) {
+			if (vc->vc_utf && !vc->vc_disp_ctrl) {
+				if (is_double_width(c)) {
+					width = 2;
+				}
+			}
 			/* Now try to find out how to display it */
 			tc = conv_uni_to_pc(vc, tc);
 			if (tc & ~charmask) {
-				if ( tc == -4 ) {
-                                /* If we got -4 (not found) then see if we have
-                                   defined a replacement character (U+FFFD) */
-replacement_glyph:
-                                	tc = conv_uni_to_pc(vc, 0xfffd);
-					if (!(tc & ~charmask))
-						goto display_glyph;
-                        	} else if ( tc != -3 )
-                                	continue; /* nothing to display */
-                                /* no hash table or no replacement --
-				 * hope for the best */
-				if ( c & ~charmask )
-					tc = '?';
-				else
-					tc = c;
+				if (tc == -1 || tc == -2) {
+				    continue; /* nothing to display */
+				}
+				/* Glyph not found */
+				if (!(vc->vc_utf && !vc->vc_disp_ctrl) && !(c & ~charmask)) {
+				    /* In legacy mode use the glyph we get by a 1:1 mapping.
+				       This would make absolutely no sense with Unicode in mind. */
+				    tc = c;
+				} else {
+				    /* Display U+FFFD. If it's not found, display an inverse question mark. */
+				    tc = conv_uni_to_pc(vc, 0xfffd);
+				    if (tc < 0) {
+					inverse = 1;
+					tc = conv_uni_to_pc(vc, '?');
+					if (tc < 0) tc = '?';
+				    }
+				}
 			}
 
-display_glyph:
-			if (vc->vc_need_wrap || vc->vc_decim)
-				FLUSH
-			if (vc->vc_need_wrap) {
-				cr(vc);
-				lf(vc);
-			}
-			if (vc->vc_decim)
-				insert_char(vc, 1);
-			scr_writew(himask ?
-				     ((vc->vc_attr << 8) & ~himask) + ((tc & 0x100) ? himask : 0) + (tc & 0xff) :
-				     (vc->vc_attr << 8) + tc,
-				   (u16 *) vc->vc_pos);
-			if (DO_UPDATE(vc) && draw_x < 0) {
-				draw_x = vc->vc_x;
-				draw_from = vc->vc_pos;
-			}
-			if (vc->vc_x == vc->vc_cols - 1) {
-				vc->vc_need_wrap = vc->vc_decawm;
-				draw_to = vc->vc_pos + 2;
+			if (!inverse) {
+				vc_attr = vc->vc_attr;
 			} else {
-				vc->vc_x++;
-				draw_to = (vc->vc_pos += 2);
+				/* invert vc_attr */
+				if (!vc->vc_can_do_color) {
+					vc_attr = (vc->vc_attr) ^ 0x08;
+				} else if (vc->vc_hi_font_mask == 0x100) {
+					vc_attr = ((vc->vc_attr) & 0x11) | (((vc->vc_attr) & 0xe0) >> 4) | (((vc->vc_attr) & 0x0e) << 4);
+				} else {
+					vc_attr = ((vc->vc_attr) & 0x88) | (((vc->vc_attr) & 0x70) >> 4) | (((vc->vc_attr) & 0x07) << 4);
+				}
 			}
-			if (vc->vc_utf_count) {
-				if (vc->vc_npar) {
-					vc->vc_npar--;
-					goto display_glyph;
+
+			while (1) {
+				if (vc->vc_need_wrap || vc->vc_decim)
+					FLUSH
+				if (vc->vc_need_wrap) {
+					cr(vc);
+					lf(vc);
+				}
+				if (vc->vc_decim)
+					insert_char(vc, 1);
+				scr_writew(himask ?
+					     ((vc_attr << 8) & ~himask) + ((tc & 0x100) ? himask : 0) + (tc & 0xff) :
+					     (vc_attr << 8) + tc,
+					   (u16 *) vc->vc_pos);
+				if (DO_UPDATE(vc) && draw_x < 0) {
+					draw_x = vc->vc_x;
+					draw_from = vc->vc_pos;
+				}
+				if (vc->vc_x == vc->vc_cols - 1) {
+					vc->vc_need_wrap = vc->vc_decawm;
+					draw_to = vc->vc_pos + 2;
+				} else {
+					vc->vc_x++;
+					draw_to = (vc->vc_pos += 2);
 				}
-				vc->vc_utf_count = 0;
+
+				if (!--width) break;
+
+				tc = conv_uni_to_pc(vc, ' '); /* A space is printed in the second column */
+				if (tc < 0) tc = ' ';
+			}
+
+			if (rescan) {
+				rescan = 0;
+				inverse = 0;
+				width = 1;
 				c = orig;
 				goto rescan_last_byte;
 			}

Re: [PATCH] console UTF-8 fixes

[Egmont Koblinger]

Hi folks,

Recently my console UTF-8 patch went mainline. Here I send a very small
additinal patch that fixes two nasty issues and improves a third one,
namely:

1. My patch changed the behavior if a glyph is not found in the Unicode
   mapping table. Previously for Unicode values less than 256 or 512 the
   kernel tried to display the glyph from that position of the glyph table,
   which could lead to a different accented letter being displayed. I
   removed this fallback possibility and changed it to display the
   replacement symbol.

   As Behdad pointed out, some fonts (e.g. sun12x22 from the kbd package)
   lack Unicode mapping information, hence all you get is lots of question
   marks. Though theoretically it's actually a user-space bug (the font
   should be fixed), Behdad and I both believe that it'd be good to work
   around in the kernel by re-introducing the fallback solution for ASCII
   characters only. This sounds a quite reasonable decision, since all fonts
   ship the ASCII characters in the first 128 positions. This way users
   won't be surprised by lots of question marks just because s/he issued a
   not-so-perfectly parameterized setfont command. As this fallback is only
   re-introduced for code points below 128, you still won't see an accented
   letter replaced by another, but at least you'll always get the English
   letters right.

2. My patch introduced "question mark with inverted color attributes" as a
   last resort fallback glyph. Though it perfectly works on VGA console, on
   framebuffer you may end up with question marks that are highlighed but
   shouldn't be, and normal characters that are accidentally highlighed.
   This is caused by missing FLUSHes when changing the color attribute.

3. I've updated the table of double-width character based on Markus's
   updated version. Only ten new code poings (one interval) is added.


Please review and commit this patch... I'm afraid it might be too late, but
it would be nice to see it in 2.6.22 -- as this will be the first stable
release that introduces my work, and the first two issues addressed now are
regressions since 2.6.21.

Thanks,

Signed-off-by: Egmont Koblinger <egmont@uhulinux.hu>

diff -Naur linux-2.6.22-rc5.orig/drivers/char/vt.c linux-2.6.22-rc5/drivers/char/vt.c
--- linux-2.6.22-rc5.orig/drivers/char/vt.c	2007-06-17 04:09:12.000000000 +0200
+++ linux-2.6.22-rc5/drivers/char/vt.c	2007-06-19 13:49:55.000000000 +0200
@@ -1956,7 +1956,7 @@
 DEFINE_MUTEX(con_buf_mtx);
 
 /* is_double_width() is based on the wcwidth() implementation by
- * Markus Kuhn -- 2003-05-20 (Unicode 4.0)
+ * Markus Kuhn -- 2007-05-26 (Unicode 5.0)
  * Latest version: http://www.cl.cam.ac.uk/~mgk25/ucs/wcwidth.c
  */
 struct interval {
@@ -1988,8 +1988,8 @@
 	static const struct interval double_width[] = {
 		{ 0x1100, 0x115F }, { 0x2329, 0x232A }, { 0x2E80, 0x303E },
 		{ 0x3040, 0xA4CF }, { 0xAC00, 0xD7A3 }, { 0xF900, 0xFAFF },
-		{ 0xFE30, 0xFE6F }, { 0xFF00, 0xFF60 }, { 0xFFE0, 0xFFE6 },
-		{ 0x20000, 0x2FFFD }, { 0x30000, 0x3FFFD }
+		{ 0xFE10, 0xFE19 }, { 0xFE30, 0xFE6F }, { 0xFF00, 0xFF60 },
+		{ 0xFFE0, 0xFFE6 }, { 0x20000, 0x2FFFD }, { 0x30000, 0x3FFFD }
 	};
 	return bisearch(ucs, double_width,
 		sizeof(double_width) / sizeof(*double_width) - 1);
@@ -2187,9 +2187,12 @@
 				    continue; /* nothing to display */
 				}
 				/* Glyph not found */
-				if (!(vc->vc_utf && !vc->vc_disp_ctrl) && !(c & ~charmask)) {
+				if ((!(vc->vc_utf && !vc->vc_disp_ctrl) || c < 128) && !(c & ~charmask)) {
 				    /* In legacy mode use the glyph we get by a 1:1 mapping.
-				       This would make absolutely no sense with Unicode in mind. */
+				       This would make absolutely no sense with Unicode in mind,
+				       but do this for ASCII characters since a font may lack
+				       Unicode mapping info and we don't want to end up with
+				       having question marks only. */
 				    tc = c;
 				} else {
 				    /* Display U+FFFD. If it's not found, display an inverse question mark. */
@@ -2213,6 +2216,7 @@
 				} else {
 					vc_attr = ((vc->vc_attr) & 0x88) | (((vc->vc_attr) & 0x70) >> 4) | (((vc->vc_attr) & 0x07) << 4);
 				}
+				FLUSH
 			}
 
 			while (1) {
@@ -2246,6 +2250,10 @@
 				if (tc < 0) tc = ' ';
 			}
 
+			if (inverse) {
+				FLUSH
+			}
+
 			if (rescan) {
 				rescan = 0;
 				inverse = 0;

Re: [PATCH] console UTF-8 fixes

[Bodo Eggert]

Egmont Koblinger <egmont@uhulinux.hu> wrote:

> 2. My patch introduced "question mark with inverted color attributes" as a
>    last resort fallback glyph. Though it perfectly works on VGA console, on
>    framebuffer you may end up with question marks that are highlighed but
>    shouldn't be, and normal characters that are accidentally highlighed.
>    This is caused by missing FLUSHes when changing the color attribute.

Does the FLUSH DTRT by design, or does it just shrink and hide the original
race? And why wasn't VGA affected, too?
-- 
Backups? We doan NEED no steenking baX%^~,VbKx NO CARRIER 

Friß, Spammer: fc6TtrpBa@6e.7eggert.dyndns.org
 hKipagwey2@mhl7jy.7eggert.dyndns.org B@d.7eggert.dyndns.org

Re: [PATCH] console UTF-8 fixes

[Egmont Koblinger]

On Tue, Jun 19, 2007 at 03:54:52PM +0200, Bodo Eggert wrote:

> Does the FLUSH DTRT by design, or does it just shrink and hide the original
> race?

I haven't deeply studied this aspect of the source, don't know what
_exactly_ this FLUSH does, but of course I have an inner feeling about this.
Kind of this macro does the _real_ print on the screen. It actually calls
vc->vc_sw->con_putcs, whereas I guess vc_sw refers to the particular
implementation (vga, fb) and con_putcs might be doing the real job.

Without these two new FLUSHes the behavior was very funny: applications
(e.g. text editors) highlighed incorrect cells, but either switching between
VTs, or gpming over these cells corrected the display. So the logical
content behind the scenes (reflected by /dev/vcsa*) was maintained
correctly, the bug is somewhere when the content is first displayed.

I saw that this FLUSH was already used 3 times inside do_con_write(), I
placed it here 2 more times (when changing color attr to inverse and when
reverting to normal). I think it cannot hurt, and apparently it fixes the
problem.

But you may be right: yes, it might be a bug (or misfeature) in the FB code,
too. Could you please look after it? I don't think I'd be able to do it and
have time for this.

> And why wasn't VGA affected, too?

I guess tt must be some difference between the actual VGA and FB code that
is called from here. I don't know whether it's a bug in FB or just some
difference.

Perhaps it's that the FB driver's con_putcs() call uses vc->vc_attr (the
current attr for the console) for each char cell to be printed, instead of
the own attr values of the cells themselves. Just an idea...



-- 
Egmont

Re: [PATCH] console UTF-8 fixes

[Bodo Eggert]

On Tue, 19 Jun 2007, Egmont Koblinger wrote:
> On Tue, Jun 19, 2007 at 03:54:52PM +0200, Bodo Eggert wrote:
> 
> > Does the FLUSH DTRT by design, or does it just shrink and hide the original
> > race?

<long snip>

> But you may be right: yes, it might be a bug (or misfeature) in the FB code,
> too. Could you please look after it? I don't think I'd be able to do it and
> have time for this.

I have neither the insight nor the time to do that. I just read the bug 
description and drew my conclusions.
-- 
Never stand when you can sit, never sit when you can lie down, never stay
awake when you can sleep.