* [PATCH] Reset converter state after second wchar_t output (Bug 25734) @ 2020-03-27 21:18 Carlos O'Donell 2020-03-28 20:37 ` Carlos O'Donell 2020-03-30 12:11 ` [PATCH] " Andreas Schwab 0 siblings, 2 replies; 22+ messages in thread From: Carlos O'Donell @ 2020-03-27 21:18 UTC (permalink / raw) To: Andreas Schwab, libc-alpha, Rafal Luzynski Andreas, Does this look good to you? I admit it is odd that the converter returns 0-bytes consumed on the second conversion, it could be enhanced to correctly show that we're really consume 1-byte at a time, but that's another bug. 8< --- 8< --- 8< --- From ea8a94fafeebc87ccd030c825484328a6da43f47 Mon Sep 17 00:00:00 2001 From: Carlos O'Donell <carlos@redhat.com> Date: Fri, 27 Mar 2020 17:03:36 -0400 Subject: [PATCH] Reset converter state after second wchar_t output (Bug 25734) An input BIG5-HKSCS character may be converted into at most 2 whcar_t characters. After outputting the second whcar_t character (which was saved in the converter state) we must reset the state. If we fail to reset the state we will be stuck continually copying that character to the output even if we have further input to consider. We add a new test case that covers the 4 BIG5-HKSCS characters that may become 2 wchar_t characters. --- iconvdata/Makefile | 6 +- iconvdata/big5hkscs.c | 3 + iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c | 146 ++++++++++++++++++++++ 3 files changed, 154 insertions(+), 1 deletion(-) create mode 100644 iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c diff --git a/iconvdata/Makefile b/iconvdata/Makefile index c83962f351..71ddef09db 100644 --- a/iconvdata/Makefile +++ b/iconvdata/Makefile @@ -73,7 +73,7 @@ modules.so := $(addsuffix .so, $(modules)) ifeq (yes,$(build-shared)) tests = bug-iconv1 bug-iconv2 tst-loading tst-e2big tst-iconv4 bug-iconv4 \ tst-iconv6 bug-iconv5 bug-iconv6 tst-iconv7 bug-iconv8 bug-iconv9 \ - bug-iconv10 bug-iconv11 bug-iconv12 + bug-iconv10 bug-iconv11 bug-iconv12 tst-iconv-big5-hkscs-to-2ucs4 ifeq ($(have-thread-library),yes) tests += bug-iconv3 endif @@ -340,3 +340,7 @@ tst-tables-clean: $(objpfx)gconv-modules: gconv-modules cat $(sysdeps-gconv-modules) $^ > $@ + +# Test requires BIG5HKSCS. +$(objpfx)tst-iconv-big5-hkscs-to-2ucs4.out: $(objpfx)gconv-modules \ + $(addprefix $(objpfx),BIG5HKSCS.so) diff --git a/iconvdata/big5hkscs.c b/iconvdata/big5hkscs.c index 01fcfeba76..ef325119b1 100644 --- a/iconvdata/big5hkscs.c +++ b/iconvdata/big5hkscs.c @@ -17895,6 +17895,9 @@ static struct else \ ++inptr; \ } \ + else \ + /* Clear the queue and proceed to output the saved character. */ \ + *statep = 0; \ \ put32 (outptr, ch); \ outptr += 4; \ diff --git a/iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c b/iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c new file mode 100644 index 0000000000..a10b8fefff --- /dev/null +++ b/iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c @@ -0,0 +1,146 @@ +/* Verify the BIG5HKSCS outputs that generate 2 wchar_t's (Bug 25734). + Copyright (C) 2020 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <stdio.h> +#include <string.h> +#include <locale.h> +#include <wchar.h> +#include <support/check.h> +#include <support/support.h> + +/* A few BIG5-HKSCS characters map in two unicode code points. + They are: + /x88/x62 => <U00CA><U0304> + /x88/x64 => <U00CA><U030C> + /x88/xa3 => <U00EA><U0304> + /x88/xa5 => <U00EA><U030C> + Each of these is special cased in iconvdata/big5hkscs.c. + This test ensures that we correctly reset the shift state after + outputting any of these characters. We do this by converting + each them followed by converting an ASCII character. If we fail + to reset the shift state (bug 25734) then we'll see the last + character in the queue output again. */ + +/* Each test has name, input bytes, and expected wide character + output. */ +struct testdata { + const char *name; + const char input[3]; + wchar_t expected[3]; +}; + +/* In BIG5-HKSCS (2008) there are 4 characters that generate multiple + wide characters. */ +struct testdata tests[4] = { + /* <H-8862>X => <U+00CA><U+0304>X */ + { "<H-8862>", "\x88\x62\x58", { 0x00CA, 0x0304, 0x0058 } }, + /* <H-8864>X => <U+00CA><U+030C>X */ + { "<H-8864>", "\x88\x64\x58", { 0x00CA, 0x030C, 0x0058 } }, + /* <H-88A3>X => <U+00EA><U+0304>X */ + { "<H-88A3>", "\x88\xa3\x58", { 0x00EA, 0x0304, 0x0058 } }, + /* <H-88A5>X => <U+00EA><U+030C>X */ + { "<H-88A5>", "\x88\xa5\x58", { 0x00EA, 0x030C, 0x0058 } } +}; + +/* Each test is of the form: + - Translate first code sequence (two bytes) + - Translate second (zero bytes) + - Translate the third (one byte). */ +static int +check_conversion (struct testdata test) +{ + wchar_t wc; + mbstate_t st; + size_t ret; + const char *mbs = test.input; + int consumed = 0; + /* Input is always 3 bytes long. */ + int inlen = 3; + + memset (&st, 0, sizeof (st)); + /* First conversion: Consumes first 2 bytes. */ + ret = mbrtowc (&wc, mbs, inlen - consumed, &st); + if (ret != 2) + { + printf ("error: First conversion consumed only %zd bytes.\n", ret); + return 1; + } + /* Advance the two consumed bytes. */ + mbs += ret; + consumed += ret; + if (wc != test.expected[0]) + { + printf ("error: Result of first conversion was wrong.\n"); + return 1; + } + /* Second conversion: Consumes 0 bytes. */ + ret = mbrtowc (&wc, mbs, inlen - consumed, &st); + if (ret != 0) + { + printf ("error: Second conversion consumed only %zd bytes.\n", ret); + return 1; + } + /* Advance the zero consumed bytes. */ + mbs += ret; + consumed += ret; + if (wc != test.expected[1]) + { + printf ("error: Result of second conversion was wrong.\n"); + return 1; + } + /* Third conversion: Consumes 1 byte (it's an ASCII character). */ + ret = mbrtowc (&wc, mbs, inlen - consumed, &st); + if (ret != 1) + { + printf ("error: Third conversion consumed only %zd bytes.\n", ret); + return 1; + } + /* Don't advance any bytes becuase none were consumed. */ + if (wc != test.expected[2]) + { + printf ("error: Result of first conversion was wrong.\n"); + return 1; + } + /* Success. */ + return 0; +} + +static int +do_test (void) +{ + int err = 0; + /* Testing BIG5-HKSCS. */ + xsetlocale (LC_ALL, "zh_HK.BIG5-HKSCS"); + + /* Run all the special conversions. */ + for (int i = 0; i < (sizeof (tests) / sizeof (struct testdata)); i++) + { + printf ("Running test for %s\n", tests[i].name); + err += check_conversion (tests[i]); + if (err > 0) + printf ("Test %s failed.\n", tests[i].name); + } + + /* Fail if any conversion had an error. */ + if (err > 0) + FAIL_EXIT1 ("One of the conversions failed."); + + return 0; +} + +#include <support/test-driver.c> -- 2.21.1 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] Reset converter state after second wchar_t output (Bug 25734) 2020-03-27 21:18 [PATCH] Reset converter state after second wchar_t output (Bug 25734) Carlos O'Donell @ 2020-03-28 20:37 ` Carlos O'Donell 2020-03-28 20:51 ` Florian Weimer 2020-03-30 17:40 ` Tom Honermann 2020-03-30 12:11 ` [PATCH] " Andreas Schwab 1 sibling, 2 replies; 22+ messages in thread From: Carlos O'Donell @ 2020-03-28 20:37 UTC (permalink / raw) To: Andreas Schwab, libc-alpha, Rafal Luzynski, Tom Honermann On 3/27/20 5:18 PM, Carlos O'Donell wrote: > Andreas, > > Does this look good to you? > > I admit it is odd that the converter returns 0-bytes consumed on the > second conversion, it could be enhanced to correctly show that we're > really consume 1-byte at a time, but that's another bug. > Tom, Thanks for your review! If you want you can give a "Reviewed-by:" Andreas, I'd like to see a review from you given your experience with BIG5. v2 - Fixed typo. - Added mbsinit test (thanks Tom). - Fixed logical flaw in test loop in do_test. > 8< --- 8< --- 8< --- From 40353453e97c321b20e683939c919e6d997fffcb Mon Sep 17 00:00:00 2001 From: Carlos O'Donell <carlos@redhat.com> Date: Fri, 27 Mar 2020 17:03:36 -0400 Subject: [PATCH] Reset converter state after second wchar_t output (Bug 25734) An input BIG5-HKSCS character may be converted into at most 2 whcar_t characters. After outputting the second whcar_t character (which was saved in the converter state) we must reset the state. If we fail to reset the state we will be stuck continually copying that character to the output even if we have further input to consider. We add a new test case that covers the 4 BIG5-HKSCS characters that may become 2 wchar_t characters. --- iconvdata/Makefile | 6 +- iconvdata/big5hkscs.c | 3 + iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c | 160 ++++++++++++++++++++++ 3 files changed, 168 insertions(+), 1 deletion(-) create mode 100644 iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c diff --git a/iconvdata/Makefile b/iconvdata/Makefile index c83962f351..71ddef09db 100644 --- a/iconvdata/Makefile +++ b/iconvdata/Makefile @@ -73,7 +73,7 @@ modules.so := $(addsuffix .so, $(modules)) ifeq (yes,$(build-shared)) tests = bug-iconv1 bug-iconv2 tst-loading tst-e2big tst-iconv4 bug-iconv4 \ tst-iconv6 bug-iconv5 bug-iconv6 tst-iconv7 bug-iconv8 bug-iconv9 \ - bug-iconv10 bug-iconv11 bug-iconv12 + bug-iconv10 bug-iconv11 bug-iconv12 tst-iconv-big5-hkscs-to-2ucs4 ifeq ($(have-thread-library),yes) tests += bug-iconv3 endif @@ -340,3 +340,7 @@ tst-tables-clean: $(objpfx)gconv-modules: gconv-modules cat $(sysdeps-gconv-modules) $^ > $@ + +# Test requires BIG5HKSCS. +$(objpfx)tst-iconv-big5-hkscs-to-2ucs4.out: $(objpfx)gconv-modules \ + $(addprefix $(objpfx),BIG5HKSCS.so) diff --git a/iconvdata/big5hkscs.c b/iconvdata/big5hkscs.c index 01fcfeba76..ef325119b1 100644 --- a/iconvdata/big5hkscs.c +++ b/iconvdata/big5hkscs.c @@ -17895,6 +17895,9 @@ static struct else \ ++inptr; \ } \ + else \ + /* Clear the queue and proceed to output the saved character. */ \ + *statep = 0; \ \ put32 (outptr, ch); \ outptr += 4; \ diff --git a/iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c b/iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c new file mode 100644 index 0000000000..8389adebf2 --- /dev/null +++ b/iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c @@ -0,0 +1,160 @@ +/* Verify the BIG5HKSCS outputs that generate 2 wchar_t's (Bug 25734). + Copyright (C) 2020 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <stdio.h> +#include <string.h> +#include <locale.h> +#include <wchar.h> +#include <support/check.h> +#include <support/support.h> + +/* A few BIG5-HKSCS characters map in two unicode code points. + They are: + /x88/x62 => <U00CA><U0304> + /x88/x64 => <U00CA><U030C> + /x88/xa3 => <U00EA><U0304> + /x88/xa5 => <U00EA><U030C> + Each of these is special cased in iconvdata/big5hkscs.c. + This test ensures that we correctly reset the shift state after + outputting any of these characters. We do this by converting + each them followed by converting an ASCII character. If we fail + to reset the shift state (bug 25734) then we'll see the last + character in the queue output again. */ + +/* Each test has name, input bytes, and expected wide character + output. */ +struct testdata { + const char *name; + const char input[3]; + wchar_t expected[3]; +}; + +/* In BIG5-HKSCS (2008) there are 4 characters that generate multiple + wide characters. */ +struct testdata tests[4] = { + /* <H-8862>X => <U+00CA><U+0304>X */ + { "<H-8862>", "\x88\x62\x58", { 0x00CA, 0x0304, 0x0058 } }, + /* <H-8864>X => <U+00CA><U+030C>X */ + { "<H-8864>", "\x88\x64\x58", { 0x00CA, 0x030C, 0x0058 } }, + /* <H-88A3>X => <U+00EA><U+0304>X */ + { "<H-88A3>", "\x88\xa3\x58", { 0x00EA, 0x0304, 0x0058 } }, + /* <H-88A5>X => <U+00EA><U+030C>X */ + { "<H-88A5>", "\x88\xa5\x58", { 0x00EA, 0x030C, 0x0058 } } +}; + +/* Each test is of the form: + - Translate first code sequence (two bytes) + - Translate second (zero bytes) + - Translate the third (one byte). */ +static int +check_conversion (struct testdata test) +{ + int err = 0; + wchar_t wc; + mbstate_t st; + size_t ret; + const char *mbs = test.input; + int consumed = 0; + /* Input is always 3 bytes long. */ + int inlen = 3; + + memset (&st, 0, sizeof (st)); + /* First conversion: Consumes first 2 bytes. */ + ret = mbrtowc (&wc, mbs, inlen - consumed, &st); + if (ret != 2) + { + printf ("error: First conversion consumed only %zd bytes.\n", ret); + err++; + } + /* Advance the two consumed bytes. */ + mbs += ret; + consumed += ret; + if (wc != test.expected[0]) + { + printf ("error: Result of first conversion was wrong.\n"); + err++; + } + /* Second conversion: Consumes 0 bytes. */ + ret = mbrtowc (&wc, mbs, inlen - consumed, &st); + if (ret != 0) + { + printf ("error: Second conversion consumed only %zd bytes.\n", ret); + err++; + } + /* Advance the zero consumed bytes. */ + mbs += ret; + consumed += ret; + if (wc != test.expected[1]) + { + printf ("error: Result of second conversion was wrong.\n"); + err++; + } + /* After the second conversion the state of the converter should be + in the initial state. It is in the initial state because the two + input BIG5-HKSCS bytes have been consumed and the 2 wchar_t's have + been output. */ + if (mbsinit (&st) == 0) + { + printf ("error: Converter not in initial state.\n"); + err++; + } + /* Third conversion: Consumes 1 byte (it's an ASCII character). */ + ret = mbrtowc (&wc, mbs, inlen - consumed, &st); + if (ret != 1) + { + printf ("error: Third conversion consumed only %zd bytes.\n", ret); + err++; + } + /* Advance the one byte. */ + mbs += ret; + consumed += ret; + if (wc != test.expected[2]) + { + printf ("error: Result of third conversion was wrong.\n"); + err++; + } + /* Return 0 if we saw no errors. */ + return err; +} + +static int +do_test (void) +{ + int err = 0; + int ret; + /* Testing BIG5-HKSCS. */ + xsetlocale (LC_ALL, "zh_HK.BIG5-HKSCS"); + + /* Run all the special conversions. */ + for (int i = 0; i < (sizeof (tests) / sizeof (struct testdata)); i++) + { + printf ("Running test for %s\n", tests[i].name); + ret = check_conversion (tests[i]); + if (ret > 0) + printf ("Test %s failed.\n", tests[i].name); + err += ret; + } + + /* Fail if any conversion had an error. */ + if (err > 0) + FAIL_EXIT1 ("One or more conversions failed."); + + return 0; +} + +#include <support/test-driver.c> -- 2.21.1 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] Reset converter state after second wchar_t output (Bug 25734) 2020-03-28 20:37 ` Carlos O'Donell @ 2020-03-28 20:51 ` Florian Weimer 2020-03-28 23:42 ` Carlos O'Donell 2020-03-30 17:40 ` Tom Honermann 1 sibling, 1 reply; 22+ messages in thread From: Florian Weimer @ 2020-03-28 20:51 UTC (permalink / raw) To: Carlos O'Donell via Libc-alpha Cc: Andreas Schwab, Rafal Luzynski, Tom Honermann, Carlos O'Donell * Carlos O'Donell via Libc-alpha: > An input BIG5-HKSCS character may be converted into at most 2 whcar_t Typo: whcar_t ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] Reset converter state after second wchar_t output (Bug 25734) 2020-03-28 20:51 ` Florian Weimer @ 2020-03-28 23:42 ` Carlos O'Donell 0 siblings, 0 replies; 22+ messages in thread From: Carlos O'Donell @ 2020-03-28 23:42 UTC (permalink / raw) To: Florian Weimer, Carlos O'Donell via Libc-alpha Cc: Andreas Schwab, Rafal Luzynski, Tom Honermann On 3/28/20 4:51 PM, Florian Weimer wrote: > * Carlos O'Donell via Libc-alpha: > >> An input BIG5-HKSCS character may be converted into at most 2 whcar_t > > Typo: whcar_t > Fixed. Thanks :-) -- Cheers, Carlos. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] Reset converter state after second wchar_t output (Bug 25734) 2020-03-28 20:37 ` Carlos O'Donell 2020-03-28 20:51 ` Florian Weimer @ 2020-03-30 17:40 ` Tom Honermann 2020-04-01 17:34 ` Tom Honermann 1 sibling, 1 reply; 22+ messages in thread From: Tom Honermann @ 2020-03-30 17:40 UTC (permalink / raw) To: Carlos O'Donell, Andreas Schwab, libc-alpha, Rafal Luzynski On 3/28/20 4:37 PM, Carlos O'Donell wrote: > On 3/27/20 5:18 PM, Carlos O'Donell wrote: >> Andreas, >> >> Does this look good to you? >> >> I admit it is odd that the converter returns 0-bytes consumed on the >> second conversion, it could be enhanced to correctly show that we're >> really consume 1-byte at a time, but that's another bug. >> > Tom, > > Thanks for your review! > > If you want you can give a "Reviewed-by:" Thanks, Carlos! Reviewed-by: Tom Honermann<tom@honermann.net> Tom. > > Andreas, > > I'd like to see a review from you given your experience with BIG5. > > v2 > - Fixed typo. > - Added mbsinit test (thanks Tom). > - Fixed logical flaw in test loop in do_test. > >> 8< --- 8< --- 8< --- > From 40353453e97c321b20e683939c919e6d997fffcb Mon Sep 17 00:00:00 2001 > From: Carlos O'Donell <carlos@redhat.com> > Date: Fri, 27 Mar 2020 17:03:36 -0400 > Subject: [PATCH] Reset converter state after second wchar_t output (Bug 25734) > > An input BIG5-HKSCS character may be converted into at most 2 whcar_t > characters. After outputting the second whcar_t character (which was > saved in the converter state) we must reset the state. If we fail > to reset the state we will be stuck continually copying that > character to the output even if we have further input to consider. > > We add a new test case that covers the 4 BIG5-HKSCS characters > that may become 2 wchar_t characters. > --- > iconvdata/Makefile | 6 +- > iconvdata/big5hkscs.c | 3 + > iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c | 160 ++++++++++++++++++++++ > 3 files changed, 168 insertions(+), 1 deletion(-) > create mode 100644 iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c > > diff --git a/iconvdata/Makefile b/iconvdata/Makefile > index c83962f351..71ddef09db 100644 > --- a/iconvdata/Makefile > +++ b/iconvdata/Makefile > @@ -73,7 +73,7 @@ modules.so := $(addsuffix .so, $(modules)) > ifeq (yes,$(build-shared)) > tests = bug-iconv1 bug-iconv2 tst-loading tst-e2big tst-iconv4 bug-iconv4 \ > tst-iconv6 bug-iconv5 bug-iconv6 tst-iconv7 bug-iconv8 bug-iconv9 \ > - bug-iconv10 bug-iconv11 bug-iconv12 > + bug-iconv10 bug-iconv11 bug-iconv12 tst-iconv-big5-hkscs-to-2ucs4 > ifeq ($(have-thread-library),yes) > tests += bug-iconv3 > endif > @@ -340,3 +340,7 @@ tst-tables-clean: > > $(objpfx)gconv-modules: gconv-modules > cat $(sysdeps-gconv-modules) $^ > $@ > + > +# Test requires BIG5HKSCS. > +$(objpfx)tst-iconv-big5-hkscs-to-2ucs4.out: $(objpfx)gconv-modules \ > + $(addprefix $(objpfx),BIG5HKSCS.so) > diff --git a/iconvdata/big5hkscs.c b/iconvdata/big5hkscs.c > index 01fcfeba76..ef325119b1 100644 > --- a/iconvdata/big5hkscs.c > +++ b/iconvdata/big5hkscs.c > @@ -17895,6 +17895,9 @@ static struct > else \ > ++inptr; \ > } \ > + else \ > + /* Clear the queue and proceed to output the saved character. */ \ > + *statep = 0; \ > \ > put32 (outptr, ch); \ > outptr += 4; \ > diff --git a/iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c b/iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c > new file mode 100644 > index 0000000000..8389adebf2 > --- /dev/null > +++ b/iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c > @@ -0,0 +1,160 @@ > +/* Verify the BIG5HKSCS outputs that generate 2 wchar_t's (Bug 25734). > + Copyright (C) 2020 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <https://www.gnu.org/licenses/>. */ > + > +#include <stdio.h> > +#include <string.h> > +#include <locale.h> > +#include <wchar.h> > +#include <support/check.h> > +#include <support/support.h> > + > +/* A few BIG5-HKSCS characters map in two unicode code points. > + They are: > + /x88/x62 => <U00CA><U0304> > + /x88/x64 => <U00CA><U030C> > + /x88/xa3 => <U00EA><U0304> > + /x88/xa5 => <U00EA><U030C> > + Each of these is special cased in iconvdata/big5hkscs.c. > + This test ensures that we correctly reset the shift state after > + outputting any of these characters. We do this by converting > + each them followed by converting an ASCII character. If we fail > + to reset the shift state (bug 25734) then we'll see the last > + character in the queue output again. */ > + > +/* Each test has name, input bytes, and expected wide character > + output. */ > +struct testdata { > + const char *name; > + const char input[3]; > + wchar_t expected[3]; > +}; > + > +/* In BIG5-HKSCS (2008) there are 4 characters that generate multiple > + wide characters. */ > +struct testdata tests[4] = { > + /* <H-8862>X => <U+00CA><U+0304>X */ > + { "<H-8862>", "\x88\x62\x58", { 0x00CA, 0x0304, 0x0058 } }, > + /* <H-8864>X => <U+00CA><U+030C>X */ > + { "<H-8864>", "\x88\x64\x58", { 0x00CA, 0x030C, 0x0058 } }, > + /* <H-88A3>X => <U+00EA><U+0304>X */ > + { "<H-88A3>", "\x88\xa3\x58", { 0x00EA, 0x0304, 0x0058 } }, > + /* <H-88A5>X => <U+00EA><U+030C>X */ > + { "<H-88A5>", "\x88\xa5\x58", { 0x00EA, 0x030C, 0x0058 } } > +}; > + > +/* Each test is of the form: > + - Translate first code sequence (two bytes) > + - Translate second (zero bytes) > + - Translate the third (one byte). */ > +static int > +check_conversion (struct testdata test) > +{ > + int err = 0; > + wchar_t wc; > + mbstate_t st; > + size_t ret; > + const char *mbs = test.input; > + int consumed = 0; > + /* Input is always 3 bytes long. */ > + int inlen = 3; > + > + memset (&st, 0, sizeof (st)); > + /* First conversion: Consumes first 2 bytes. */ > + ret = mbrtowc (&wc, mbs, inlen - consumed, &st); > + if (ret != 2) > + { > + printf ("error: First conversion consumed only %zd bytes.\n", ret); > + err++; > + } > + /* Advance the two consumed bytes. */ > + mbs += ret; > + consumed += ret; > + if (wc != test.expected[0]) > + { > + printf ("error: Result of first conversion was wrong.\n"); > + err++; > + } > + /* Second conversion: Consumes 0 bytes. */ > + ret = mbrtowc (&wc, mbs, inlen - consumed, &st); > + if (ret != 0) > + { > + printf ("error: Second conversion consumed only %zd bytes.\n", ret); > + err++; > + } > + /* Advance the zero consumed bytes. */ > + mbs += ret; > + consumed += ret; > + if (wc != test.expected[1]) > + { > + printf ("error: Result of second conversion was wrong.\n"); > + err++; > + } > + /* After the second conversion the state of the converter should be > + in the initial state. It is in the initial state because the two > + input BIG5-HKSCS bytes have been consumed and the 2 wchar_t's have > + been output. */ > + if (mbsinit (&st) == 0) > + { > + printf ("error: Converter not in initial state.\n"); > + err++; > + } > + /* Third conversion: Consumes 1 byte (it's an ASCII character). */ > + ret = mbrtowc (&wc, mbs, inlen - consumed, &st); > + if (ret != 1) > + { > + printf ("error: Third conversion consumed only %zd bytes.\n", ret); > + err++; > + } > + /* Advance the one byte. */ > + mbs += ret; > + consumed += ret; > + if (wc != test.expected[2]) > + { > + printf ("error: Result of third conversion was wrong.\n"); > + err++; > + } > + /* Return 0 if we saw no errors. */ > + return err; > +} > + > +static int > +do_test (void) > +{ > + int err = 0; > + int ret; > + /* Testing BIG5-HKSCS. */ > + xsetlocale (LC_ALL, "zh_HK.BIG5-HKSCS"); > + > + /* Run all the special conversions. */ > + for (int i = 0; i < (sizeof (tests) / sizeof (struct testdata)); i++) > + { > + printf ("Running test for %s\n", tests[i].name); > + ret = check_conversion (tests[i]); > + if (ret > 0) > + printf ("Test %s failed.\n", tests[i].name); > + err += ret; > + } > + > + /* Fail if any conversion had an error. */ > + if (err > 0) > + FAIL_EXIT1 ("One or more conversions failed."); > + > + return 0; > +} > + > +#include <support/test-driver.c> ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] Reset converter state after second wchar_t output (Bug 25734) 2020-03-30 17:40 ` Tom Honermann @ 2020-04-01 17:34 ` Tom Honermann 2020-04-04 18:41 ` [PATCH v2] " Carlos O'Donell 0 siblings, 1 reply; 22+ messages in thread From: Tom Honermann @ 2020-04-01 17:34 UTC (permalink / raw) To: Carlos O'Donell, Andreas Schwab, libc-alpha, Rafal Luzynski Carlos, I think this patch needs an additional update. Comment inlined below. On 3/30/20 1:40 PM, Tom Honermann wrote: > On 3/28/20 4:37 PM, Carlos O'Donell wrote: >> On 3/27/20 5:18 PM, Carlos O'Donell wrote: >>> Andreas, >>> >>> Does this look good to you? >>> >>> I admit it is odd that the converter returns 0-bytes consumed on the >>> second conversion, it could be enhanced to correctly show that we're >>> really consume 1-byte at a time, but that's another bug. >>> >> Tom, >> >> Thanks for your review! >> >> If you want you can give a "Reviewed-by:" > > Thanks, Carlos! > > Reviewed-by: Tom Honermann<tom@honermann.net> > > Tom. > >> >> Andreas, >> >> I'd like to see a review from you given your experience with BIG5. >> >> v2 >> - Fixed typo. >> - Added mbsinit test (thanks Tom). >> - Fixed logical flaw in test loop in do_test. >> >>> 8< --- 8< --- 8< --- >> From 40353453e97c321b20e683939c919e6d997fffcb Mon Sep 17 00:00:00 2001 >> From: Carlos O'Donell <carlos@redhat.com> >> Date: Fri, 27 Mar 2020 17:03:36 -0400 >> Subject: [PATCH] Reset converter state after second wchar_t output >> (Bug 25734) >> >> An input BIG5-HKSCS character may be converted into at most 2 whcar_t >> characters. After outputting the second whcar_t character (which was >> saved in the converter state) we must reset the state. If we fail >> to reset the state we will be stuck continually copying that >> character to the output even if we have further input to consider. >> >> We add a new test case that covers the 4 BIG5-HKSCS characters >> that may become 2 wchar_t characters. >> --- >> iconvdata/Makefile | 6 +- >> iconvdata/big5hkscs.c | 3 + >> iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c | 160 ++++++++++++++++++++++ >> 3 files changed, 168 insertions(+), 1 deletion(-) >> create mode 100644 iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c >> >> diff --git a/iconvdata/Makefile b/iconvdata/Makefile >> index c83962f351..71ddef09db 100644 >> --- a/iconvdata/Makefile >> +++ b/iconvdata/Makefile >> @@ -73,7 +73,7 @@ modules.so := $(addsuffix .so, $(modules)) >> ifeq (yes,$(build-shared)) >> tests = bug-iconv1 bug-iconv2 tst-loading tst-e2big tst-iconv4 >> bug-iconv4 \ >> tst-iconv6 bug-iconv5 bug-iconv6 tst-iconv7 bug-iconv8 >> bug-iconv9 \ >> - bug-iconv10 bug-iconv11 bug-iconv12 >> + bug-iconv10 bug-iconv11 bug-iconv12 tst-iconv-big5-hkscs-to-2ucs4 >> ifeq ($(have-thread-library),yes) >> tests += bug-iconv3 >> endif >> @@ -340,3 +340,7 @@ tst-tables-clean: >> $(objpfx)gconv-modules: gconv-modules >> cat $(sysdeps-gconv-modules) $^ > $@ >> + >> +# Test requires BIG5HKSCS. >> +$(objpfx)tst-iconv-big5-hkscs-to-2ucs4.out: $(objpfx)gconv-modules \ >> + $(addprefix $(objpfx),BIG5HKSCS.so) >> diff --git a/iconvdata/big5hkscs.c b/iconvdata/big5hkscs.c >> index 01fcfeba76..ef325119b1 100644 >> --- a/iconvdata/big5hkscs.c >> +++ b/iconvdata/big5hkscs.c >> @@ -17895,6 +17895,9 @@ static struct >> else \ >> ++inptr; \ >> } \ >> + else \ >> + /* Clear the queue and proceed to output the saved character. >> */ \ >> + *statep = 0; \ >> \ >> put32 (outptr, ch); \ >> outptr += 4; \ >> diff --git a/iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c >> b/iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c >> new file mode 100644 >> index 0000000000..8389adebf2 >> --- /dev/null >> +++ b/iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c >> @@ -0,0 +1,160 @@ >> +/* Verify the BIG5HKSCS outputs that generate 2 wchar_t's (Bug 25734). >> + Copyright (C) 2020 Free Software Foundation, Inc. >> + This file is part of the GNU C Library. >> + >> + The GNU C Library is free software; you can redistribute it and/or >> + modify it under the terms of the GNU Lesser General Public >> + License as published by the Free Software Foundation; either >> + version 2.1 of the License, or (at your option) any later version. >> + >> + The GNU C Library is distributed in the hope that it will be useful, >> + but WITHOUT ANY WARRANTY; without even the implied warranty of >> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + Lesser General Public License for more details. >> + >> + You should have received a copy of the GNU Lesser General Public >> + License along with the GNU C Library; if not, see >> + <https://www.gnu.org/licenses/>. */ >> + >> +#include <stdio.h> >> +#include <string.h> >> +#include <locale.h> >> +#include <wchar.h> >> +#include <support/check.h> >> +#include <support/support.h> >> + >> +/* A few BIG5-HKSCS characters map in two unicode code points. >> + They are: >> + /x88/x62 => <U00CA><U0304> >> + /x88/x64 => <U00CA><U030C> >> + /x88/xa3 => <U00EA><U0304> >> + /x88/xa5 => <U00EA><U030C> >> + Each of these is special cased in iconvdata/big5hkscs.c. >> + This test ensures that we correctly reset the shift state after >> + outputting any of these characters. We do this by converting >> + each them followed by converting an ASCII character. If we fail >> + to reset the shift state (bug 25734) then we'll see the last >> + character in the queue output again. */ >> + >> +/* Each test has name, input bytes, and expected wide character >> + output. */ >> +struct testdata { >> + const char *name; >> + const char input[3]; >> + wchar_t expected[3]; >> +}; >> + >> +/* In BIG5-HKSCS (2008) there are 4 characters that generate multiple >> + wide characters. */ >> +struct testdata tests[4] = { >> + /* <H-8862>X => <U+00CA><U+0304>X */ >> + { "<H-8862>", "\x88\x62\x58", { 0x00CA, 0x0304, 0x0058 } }, >> + /* <H-8864>X => <U+00CA><U+030C>X */ >> + { "<H-8864>", "\x88\x64\x58", { 0x00CA, 0x030C, 0x0058 } }, >> + /* <H-88A3>X => <U+00EA><U+0304>X */ >> + { "<H-88A3>", "\x88\xa3\x58", { 0x00EA, 0x0304, 0x0058 } }, >> + /* <H-88A5>X => <U+00EA><U+030C>X */ >> + { "<H-88A5>", "\x88\xa5\x58", { 0x00EA, 0x030C, 0x0058 } } >> +}; >> + >> +/* Each test is of the form: >> + - Translate first code sequence (two bytes) >> + - Translate second (zero bytes) >> + - Translate the third (one byte). */ >> +static int >> +check_conversion (struct testdata test) >> +{ >> + int err = 0; >> + wchar_t wc; >> + mbstate_t st; >> + size_t ret; >> + const char *mbs = test.input; >> + int consumed = 0; >> + /* Input is always 3 bytes long. */ >> + int inlen = 3; >> + >> + memset (&st, 0, sizeof (st)); >> + /* First conversion: Consumes first 2 bytes. */ >> + ret = mbrtowc (&wc, mbs, inlen - consumed, &st); >> + if (ret != 2) >> + { >> + printf ("error: First conversion consumed only %zd bytes.\n", >> ret); >> + err++; >> + } >> + /* Advance the two consumed bytes. */ >> + mbs += ret; >> + consumed += ret; >> + if (wc != test.expected[0]) >> + { >> + printf ("error: Result of first conversion was wrong.\n"); >> + err++; >> + } >> + /* Second conversion: Consumes 0 bytes. */ >> + ret = mbrtowc (&wc, mbs, inlen - consumed, &st); >> + if (ret != 0) >> + { >> + printf ("error: Second conversion consumed only %zd bytes.\n", >> ret); >> + err++; >> + } >> + /* Advance the zero consumed bytes. */ >> + mbs += ret; >> + consumed += ret; >> + if (wc != test.expected[1]) >> + { >> + printf ("error: Result of second conversion was wrong.\n"); >> + err++; >> + } >> + /* After the second conversion the state of the converter should be >> + in the initial state. It is in the initial state because the two >> + input BIG5-HKSCS bytes have been consumed and the 2 wchar_t's have >> + been output. */ >> + if (mbsinit (&st) == 0) >> + { >> + printf ("error: Converter not in initial state.\n"); >> + err++; >> + } >> + /* Third conversion: Consumes 1 byte (it's an ASCII character). */ >> + ret = mbrtowc (&wc, mbs, inlen - consumed, &st); >> + if (ret != 1) >> + { >> + printf ("error: Third conversion consumed only %zd bytes.\n", >> ret); >> + err++; >> + } >> + /* Advance the one byte. */ >> + mbs += ret; >> + consumed += ret; >> + if (wc != test.expected[2]) >> + { >> + printf ("error: Result of third conversion was wrong.\n"); >> + err++; >> + } >> + /* Return 0 if we saw no errors. */ >> + return err; >> +} >> + >> +static int >> +do_test (void) >> +{ >> + int err = 0; >> + int ret; >> + /* Testing BIG5-HKSCS. */ >> + xsetlocale (LC_ALL, "zh_HK.BIG5-HKSCS"); Though my initial tests didn't reveal a problem, this call to xsetlocale() is now failing for me in a fresh environment. The reason is that a locale for "zh_HK.BIG5-HKSCS" is not generated. For other work I've been doing, I had generated that locale, but the glibc testsuite doesn't automatically generate it today. The following patch addresses this. Note that the prior dependence on BIG5HKSCS.so was not sufficient (and is no longer needed) since this test really depends on the locale (not just the converter) being available. The requirement for the locale may be sufficient reason to move this test under wcsmbs. The iconvdata Makefile currently only requires generation of the de_DE.UTF-8 locale while the wcsmbs Makefile already requires many locales. And since this test exercises mbrtowc specifically, that might be a more appropriate place for it anyway. diff --git a/iconvdata/Makefile b/iconvdata/Makefile index 71ddef09db..3c0c5807b7 100644 --- a/iconvdata/Makefile +++ b/iconvdata/Makefile @@ -278,11 +278,12 @@ endif include ../Rules ifeq ($(run-built-tests),yes) -LOCALES := de_DE.UTF-8 +LOCALES := de_DE.UTF-8 zh_HK.BIG5-HKSCS include ../gen-locales.mk $(objpfx)bug-iconv6.out: $(gen-locales) $(objpfx)tst-iconv7.out: $(gen-locales) +$(objpfx)tst-iconv-big5-hkscs-to-2ucs4.out: $(gen-locales) endif # Set libof-* for each routine. @@ -340,7 +341,3 @@ tst-tables-clean: $(objpfx)gconv-modules: gconv-modules cat $(sysdeps-gconv-modules) $^ > $@ - -# Test requires BIG5HKSCS. -$(objpfx)tst-iconv-big5-hkscs-to-2ucs4.out: $(objpfx)gconv-modules \ - $(addprefix $(objpfx),BIG5HKSCS.so) Tom. >> + >> + /* Run all the special conversions. */ >> + for (int i = 0; i < (sizeof (tests) / sizeof (struct testdata)); i++) >> + { >> + printf ("Running test for %s\n", tests[i].name); >> + ret = check_conversion (tests[i]); >> + if (ret > 0) >> + printf ("Test %s failed.\n", tests[i].name); >> + err += ret; >> + } >> + >> + /* Fail if any conversion had an error. */ >> + if (err > 0) >> + FAIL_EXIT1 ("One or more conversions failed."); >> + >> + return 0; >> +} >> + >> +#include <support/test-driver.c> > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2] Reset converter state after second wchar_t output (Bug 25734) 2020-04-01 17:34 ` Tom Honermann @ 2020-04-04 18:41 ` Carlos O'Donell 2020-04-07 4:20 ` Tom Honermann 2020-04-14 19:03 ` Tom Honermann 0 siblings, 2 replies; 22+ messages in thread From: Carlos O'Donell @ 2020-04-04 18:41 UTC (permalink / raw) To: Tom Honermann, Andreas Schwab, libc-alpha, Rafal Luzynski On 4/1/20 1:34 PM, Tom Honermann wrote: > Though my initial tests didn't reveal a problem, this call to > xsetlocale() is now failing for me in a fresh environment. The > reason is that a locale for "zh_HK.BIG5-HKSCS" is not generated. For > other work I've been doing, I had generated that locale, but the > glibc testsuite doesn't automatically generate it today. Correct, I've added that dep in now. > The following patch addresses this. Note that the prior dependence > on BIG5HKSCS.so was not sufficient (and is no longer needed) since > this test really depends on the locale (not just the converter) being > available. The requirement for the locale may be sufficient reason > to move this test under wcsmbs. The iconvdata Makefile currently > only requires generation of the de_DE.UTF-8 locale while the wcsmbs > Makefile already requires many locales. And since this test exercises > mbrtowc specifically, that might be a more appropriate place for it > anyway. The test depends on and needs both. I think this test should stay in iconvdata/ because we're really exercising the converter. > diff --git a/iconvdata/Makefile b/iconvdata/Makefile > index 71ddef09db..3c0c5807b7 100644 > --- a/iconvdata/Makefile > +++ b/iconvdata/Makefile > @@ -278,11 +278,12 @@ endif > include ../Rules > > ifeq ($(run-built-tests),yes) > -LOCALES := de_DE.UTF-8 > +LOCALES := de_DE.UTF-8 zh_HK.BIG5-HKSCS Yup. > include ../gen-locales.mk > > $(objpfx)bug-iconv6.out: $(gen-locales) > $(objpfx)tst-iconv7.out: $(gen-locales) > +$(objpfx)tst-iconv-big5-hkscs-to-2ucs4.out: $(gen-locales) > endif > > # Set libof-* for each routine. > @@ -340,7 +341,3 @@ tst-tables-clean: > > $(objpfx)gconv-modules: gconv-modules > cat $(sysdeps-gconv-modules) $^ > $@ > - > -# Test requires BIG5HKSCS. > -$(objpfx)tst-iconv-big5-hkscs-to-2ucs4.out: $(objpfx)gconv-modules \ > - $(addprefix $(objpfx),BIG5HKSCS.so) v2 - Add test dependency on locale. - Make test use generic converter dependency (builds all converters) - Move Rules inclusion to after locale generation to match localedata/ order. No regressions on x86_64 and i686. I'll push this next week unless someone objects. There is still the issue of *what* to return from the converter when processing the queue byte, but that will take longer to reach consensus than just fixing the current processing hang. 8< --- 8< --- 8< From fccb8e5dfd80933c881a983353998e79cff9559c Mon Sep 17 00:00:00 2001 From: Carlos O'Donell <carlos@redhat.com> Date: Fri, 27 Mar 2020 17:03:36 -0400 Subject: [PATCH] Reset converter state after second wchar_t output (Bug 25734) An input BIG5-HKSCS character may be converted into at most 2 wchar_t characters. After outputting the second whcar_t character (which was saved in the converter state) we must reset the state. If we fail to reset the state we will be stuck continually copying that character to the output even if we have further input to consider. We add a new test case that covers the 4 BIG5-HKSCS characters that may become 2 wchar_t characters. Reviewed-by: Tom Honermann <tom@honermann.net> --- iconvdata/Makefile | 17 ++- iconvdata/big5hkscs.c | 3 + iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c | 160 ++++++++++++++++++++++ 3 files changed, 176 insertions(+), 4 deletions(-) create mode 100644 iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c diff --git a/iconvdata/Makefile b/iconvdata/Makefile index c83962f351..4ec2741cdc 100644 --- a/iconvdata/Makefile +++ b/iconvdata/Makefile @@ -73,7 +73,7 @@ modules.so := $(addsuffix .so, $(modules)) ifeq (yes,$(build-shared)) tests = bug-iconv1 bug-iconv2 tst-loading tst-e2big tst-iconv4 bug-iconv4 \ tst-iconv6 bug-iconv5 bug-iconv6 tst-iconv7 bug-iconv8 bug-iconv9 \ - bug-iconv10 bug-iconv11 bug-iconv12 + bug-iconv10 bug-iconv11 bug-iconv12 tst-iconv-big5-hkscs-to-2ucs4 ifeq ($(have-thread-library),yes) tests += bug-iconv3 endif @@ -275,16 +275,21 @@ endif endif endif -include ../Rules - ifeq ($(run-built-tests),yes) -LOCALES := de_DE.UTF-8 +LOCALES := \ + de_DE.UTF-8 \ + zh_HK.BIG5-HKSCS \ + $(NULL) + include ../gen-locales.mk $(objpfx)bug-iconv6.out: $(gen-locales) $(objpfx)tst-iconv7.out: $(gen-locales) +$(objpfx)tst-iconv-big5-hkscs-to-2ucs4.out: $(gen-locales) endif +include ../Rules + # Set libof-* for each routine. cpp-srcs-left := $(modules) $(generated-modules) $(libJIS-routines) \ $(libKSC-routines) $(libGB-routines) $(libCNS-routines) \ @@ -340,3 +345,7 @@ tst-tables-clean: $(objpfx)gconv-modules: gconv-modules cat $(sysdeps-gconv-modules) $^ > $@ + +# Test requires BIG5HKSCS. +$(objpfx)tst-iconv-big5-hkscs-to-2ucs4.out: $(objpfx)gconv-modules \ + $(addprefix $(objpfx),$(modules.so)) diff --git a/iconvdata/big5hkscs.c b/iconvdata/big5hkscs.c index 01fcfeba76..ef325119b1 100644 --- a/iconvdata/big5hkscs.c +++ b/iconvdata/big5hkscs.c @@ -17895,6 +17895,9 @@ static struct else \ ++inptr; \ } \ + else \ + /* Clear the queue and proceed to output the saved character. */ \ + *statep = 0; \ \ put32 (outptr, ch); \ outptr += 4; \ diff --git a/iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c b/iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c new file mode 100644 index 0000000000..8389adebf2 --- /dev/null +++ b/iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c @@ -0,0 +1,160 @@ +/* Verify the BIG5HKSCS outputs that generate 2 wchar_t's (Bug 25734). + Copyright (C) 2020 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <stdio.h> +#include <string.h> +#include <locale.h> +#include <wchar.h> +#include <support/check.h> +#include <support/support.h> + +/* A few BIG5-HKSCS characters map in two unicode code points. + They are: + /x88/x62 => <U00CA><U0304> + /x88/x64 => <U00CA><U030C> + /x88/xa3 => <U00EA><U0304> + /x88/xa5 => <U00EA><U030C> + Each of these is special cased in iconvdata/big5hkscs.c. + This test ensures that we correctly reset the shift state after + outputting any of these characters. We do this by converting + each them followed by converting an ASCII character. If we fail + to reset the shift state (bug 25734) then we'll see the last + character in the queue output again. */ + +/* Each test has name, input bytes, and expected wide character + output. */ +struct testdata { + const char *name; + const char input[3]; + wchar_t expected[3]; +}; + +/* In BIG5-HKSCS (2008) there are 4 characters that generate multiple + wide characters. */ +struct testdata tests[4] = { + /* <H-8862>X => <U+00CA><U+0304>X */ + { "<H-8862>", "\x88\x62\x58", { 0x00CA, 0x0304, 0x0058 } }, + /* <H-8864>X => <U+00CA><U+030C>X */ + { "<H-8864>", "\x88\x64\x58", { 0x00CA, 0x030C, 0x0058 } }, + /* <H-88A3>X => <U+00EA><U+0304>X */ + { "<H-88A3>", "\x88\xa3\x58", { 0x00EA, 0x0304, 0x0058 } }, + /* <H-88A5>X => <U+00EA><U+030C>X */ + { "<H-88A5>", "\x88\xa5\x58", { 0x00EA, 0x030C, 0x0058 } } +}; + +/* Each test is of the form: + - Translate first code sequence (two bytes) + - Translate second (zero bytes) + - Translate the third (one byte). */ +static int +check_conversion (struct testdata test) +{ + int err = 0; + wchar_t wc; + mbstate_t st; + size_t ret; + const char *mbs = test.input; + int consumed = 0; + /* Input is always 3 bytes long. */ + int inlen = 3; + + memset (&st, 0, sizeof (st)); + /* First conversion: Consumes first 2 bytes. */ + ret = mbrtowc (&wc, mbs, inlen - consumed, &st); + if (ret != 2) + { + printf ("error: First conversion consumed only %zd bytes.\n", ret); + err++; + } + /* Advance the two consumed bytes. */ + mbs += ret; + consumed += ret; + if (wc != test.expected[0]) + { + printf ("error: Result of first conversion was wrong.\n"); + err++; + } + /* Second conversion: Consumes 0 bytes. */ + ret = mbrtowc (&wc, mbs, inlen - consumed, &st); + if (ret != 0) + { + printf ("error: Second conversion consumed only %zd bytes.\n", ret); + err++; + } + /* Advance the zero consumed bytes. */ + mbs += ret; + consumed += ret; + if (wc != test.expected[1]) + { + printf ("error: Result of second conversion was wrong.\n"); + err++; + } + /* After the second conversion the state of the converter should be + in the initial state. It is in the initial state because the two + input BIG5-HKSCS bytes have been consumed and the 2 wchar_t's have + been output. */ + if (mbsinit (&st) == 0) + { + printf ("error: Converter not in initial state.\n"); + err++; + } + /* Third conversion: Consumes 1 byte (it's an ASCII character). */ + ret = mbrtowc (&wc, mbs, inlen - consumed, &st); + if (ret != 1) + { + printf ("error: Third conversion consumed only %zd bytes.\n", ret); + err++; + } + /* Advance the one byte. */ + mbs += ret; + consumed += ret; + if (wc != test.expected[2]) + { + printf ("error: Result of third conversion was wrong.\n"); + err++; + } + /* Return 0 if we saw no errors. */ + return err; +} + +static int +do_test (void) +{ + int err = 0; + int ret; + /* Testing BIG5-HKSCS. */ + xsetlocale (LC_ALL, "zh_HK.BIG5-HKSCS"); + + /* Run all the special conversions. */ + for (int i = 0; i < (sizeof (tests) / sizeof (struct testdata)); i++) + { + printf ("Running test for %s\n", tests[i].name); + ret = check_conversion (tests[i]); + if (ret > 0) + printf ("Test %s failed.\n", tests[i].name); + err += ret; + } + + /* Fail if any conversion had an error. */ + if (err > 0) + FAIL_EXIT1 ("One or more conversions failed."); + + return 0; +} + +#include <support/test-driver.c> -- 2.21.1 -- Cheers, Carlos. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] Reset converter state after second wchar_t output (Bug 25734) 2020-04-04 18:41 ` [PATCH v2] " Carlos O'Donell @ 2020-04-07 4:20 ` Tom Honermann 2020-04-14 19:03 ` Tom Honermann 1 sibling, 0 replies; 22+ messages in thread From: Tom Honermann @ 2020-04-07 4:20 UTC (permalink / raw) To: Carlos O'Donell, Andreas Schwab, libc-alpha, Rafal Luzynski On 4/4/20 2:41 PM, Carlos O'Donell wrote: > On 4/1/20 1:34 PM, Tom Honermann wrote: >> Though my initial tests didn't reveal a problem, this call to >> xsetlocale() is now failing for me in a fresh environment. The >> reason is that a locale for "zh_HK.BIG5-HKSCS" is not generated. For >> other work I've been doing, I had generated that locale, but the >> glibc testsuite doesn't automatically generate it today. > Correct, I've added that dep in now. > >> The following patch addresses this. Note that the prior dependence >> on BIG5HKSCS.so was not sufficient (and is no longer needed) since >> this test really depends on the locale (not just the converter) being >> available. The requirement for the locale may be sufficient reason >> to move this test under wcsmbs. The iconvdata Makefile currently >> only requires generation of the de_DE.UTF-8 locale while the wcsmbs >> Makefile already requires many locales. And since this test exercises >> mbrtowc specifically, that might be a more appropriate place for it >> anyway. > The test depends on and needs both. > > I think this test should stay in iconvdata/ because we're really exercising > the converter. > >> diff --git a/iconvdata/Makefile b/iconvdata/Makefile >> index 71ddef09db..3c0c5807b7 100644 >> --- a/iconvdata/Makefile >> +++ b/iconvdata/Makefile >> @@ -278,11 +278,12 @@ endif >> include ../Rules >> >> ifeq ($(run-built-tests),yes) >> -LOCALES := de_DE.UTF-8 >> +LOCALES := de_DE.UTF-8 zh_HK.BIG5-HKSCS > Yup. > >> include ../gen-locales.mk >> >> $(objpfx)bug-iconv6.out: $(gen-locales) >> $(objpfx)tst-iconv7.out: $(gen-locales) >> +$(objpfx)tst-iconv-big5-hkscs-to-2ucs4.out: $(gen-locales) >> endif >> >> # Set libof-* for each routine. >> @@ -340,7 +341,3 @@ tst-tables-clean: >> >> $(objpfx)gconv-modules: gconv-modules >> cat $(sysdeps-gconv-modules) $^ > $@ >> - >> -# Test requires BIG5HKSCS. >> -$(objpfx)tst-iconv-big5-hkscs-to-2ucs4.out: $(objpfx)gconv-modules \ >> - $(addprefix $(objpfx),BIG5HKSCS.so) > v2 > - Add test dependency on locale. > - Make test use generic converter dependency (builds all converters) > - Move Rules inclusion to after locale generation to match localedata/ order. > > No regressions on x86_64 and i686. > > I'll push this next week unless someone objects. I confirmed that the test now passes for me in a clean environment with this patch. Looks good to go from my perspective! Tom. > > There is still the issue of *what* to return from the converter when processing > the queue byte, but that will take longer to reach consensus than just fixing > the current processing hang. > > 8< --- 8< --- 8< > From fccb8e5dfd80933c881a983353998e79cff9559c Mon Sep 17 00:00:00 2001 > From: Carlos O'Donell <carlos@redhat.com> > Date: Fri, 27 Mar 2020 17:03:36 -0400 > Subject: [PATCH] Reset converter state after second wchar_t output (Bug 25734) > > An input BIG5-HKSCS character may be converted into at most 2 wchar_t > characters. After outputting the second whcar_t character (which was > saved in the converter state) we must reset the state. If we fail > to reset the state we will be stuck continually copying that > character to the output even if we have further input to consider. > > We add a new test case that covers the 4 BIG5-HKSCS characters > that may become 2 wchar_t characters. > > Reviewed-by: Tom Honermann <tom@honermann.net> > --- > iconvdata/Makefile | 17 ++- > iconvdata/big5hkscs.c | 3 + > iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c | 160 ++++++++++++++++++++++ > 3 files changed, 176 insertions(+), 4 deletions(-) > create mode 100644 iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c > > diff --git a/iconvdata/Makefile b/iconvdata/Makefile > index c83962f351..4ec2741cdc 100644 > --- a/iconvdata/Makefile > +++ b/iconvdata/Makefile > @@ -73,7 +73,7 @@ modules.so := $(addsuffix .so, $(modules)) > ifeq (yes,$(build-shared)) > tests = bug-iconv1 bug-iconv2 tst-loading tst-e2big tst-iconv4 bug-iconv4 \ > tst-iconv6 bug-iconv5 bug-iconv6 tst-iconv7 bug-iconv8 bug-iconv9 \ > - bug-iconv10 bug-iconv11 bug-iconv12 > + bug-iconv10 bug-iconv11 bug-iconv12 tst-iconv-big5-hkscs-to-2ucs4 > ifeq ($(have-thread-library),yes) > tests += bug-iconv3 > endif > @@ -275,16 +275,21 @@ endif > endif > endif > > -include ../Rules > - > ifeq ($(run-built-tests),yes) > -LOCALES := de_DE.UTF-8 > +LOCALES := \ > + de_DE.UTF-8 \ > + zh_HK.BIG5-HKSCS \ > + $(NULL) > + > include ../gen-locales.mk > > $(objpfx)bug-iconv6.out: $(gen-locales) > $(objpfx)tst-iconv7.out: $(gen-locales) > +$(objpfx)tst-iconv-big5-hkscs-to-2ucs4.out: $(gen-locales) > endif > > +include ../Rules > + > # Set libof-* for each routine. > cpp-srcs-left := $(modules) $(generated-modules) $(libJIS-routines) \ > $(libKSC-routines) $(libGB-routines) $(libCNS-routines) \ > @@ -340,3 +345,7 @@ tst-tables-clean: > > $(objpfx)gconv-modules: gconv-modules > cat $(sysdeps-gconv-modules) $^ > $@ > + > +# Test requires BIG5HKSCS. > +$(objpfx)tst-iconv-big5-hkscs-to-2ucs4.out: $(objpfx)gconv-modules \ > + $(addprefix $(objpfx),$(modules.so)) > diff --git a/iconvdata/big5hkscs.c b/iconvdata/big5hkscs.c > index 01fcfeba76..ef325119b1 100644 > --- a/iconvdata/big5hkscs.c > +++ b/iconvdata/big5hkscs.c > @@ -17895,6 +17895,9 @@ static struct > else \ > ++inptr; \ > } \ > + else \ > + /* Clear the queue and proceed to output the saved character. */ \ > + *statep = 0; \ > \ > put32 (outptr, ch); \ > outptr += 4; \ > diff --git a/iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c b/iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c > new file mode 100644 > index 0000000000..8389adebf2 > --- /dev/null > +++ b/iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c > @@ -0,0 +1,160 @@ > +/* Verify the BIG5HKSCS outputs that generate 2 wchar_t's (Bug 25734). > + Copyright (C) 2020 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <https://www.gnu.org/licenses/>. */ > + > +#include <stdio.h> > +#include <string.h> > +#include <locale.h> > +#include <wchar.h> > +#include <support/check.h> > +#include <support/support.h> > + > +/* A few BIG5-HKSCS characters map in two unicode code points. > + They are: > + /x88/x62 => <U00CA><U0304> > + /x88/x64 => <U00CA><U030C> > + /x88/xa3 => <U00EA><U0304> > + /x88/xa5 => <U00EA><U030C> > + Each of these is special cased in iconvdata/big5hkscs.c. > + This test ensures that we correctly reset the shift state after > + outputting any of these characters. We do this by converting > + each them followed by converting an ASCII character. If we fail > + to reset the shift state (bug 25734) then we'll see the last > + character in the queue output again. */ > + > +/* Each test has name, input bytes, and expected wide character > + output. */ > +struct testdata { > + const char *name; > + const char input[3]; > + wchar_t expected[3]; > +}; > + > +/* In BIG5-HKSCS (2008) there are 4 characters that generate multiple > + wide characters. */ > +struct testdata tests[4] = { > + /* <H-8862>X => <U+00CA><U+0304>X */ > + { "<H-8862>", "\x88\x62\x58", { 0x00CA, 0x0304, 0x0058 } }, > + /* <H-8864>X => <U+00CA><U+030C>X */ > + { "<H-8864>", "\x88\x64\x58", { 0x00CA, 0x030C, 0x0058 } }, > + /* <H-88A3>X => <U+00EA><U+0304>X */ > + { "<H-88A3>", "\x88\xa3\x58", { 0x00EA, 0x0304, 0x0058 } }, > + /* <H-88A5>X => <U+00EA><U+030C>X */ > + { "<H-88A5>", "\x88\xa5\x58", { 0x00EA, 0x030C, 0x0058 } } > +}; > + > +/* Each test is of the form: > + - Translate first code sequence (two bytes) > + - Translate second (zero bytes) > + - Translate the third (one byte). */ > +static int > +check_conversion (struct testdata test) > +{ > + int err = 0; > + wchar_t wc; > + mbstate_t st; > + size_t ret; > + const char *mbs = test.input; > + int consumed = 0; > + /* Input is always 3 bytes long. */ > + int inlen = 3; > + > + memset (&st, 0, sizeof (st)); > + /* First conversion: Consumes first 2 bytes. */ > + ret = mbrtowc (&wc, mbs, inlen - consumed, &st); > + if (ret != 2) > + { > + printf ("error: First conversion consumed only %zd bytes.\n", ret); > + err++; > + } > + /* Advance the two consumed bytes. */ > + mbs += ret; > + consumed += ret; > + if (wc != test.expected[0]) > + { > + printf ("error: Result of first conversion was wrong.\n"); > + err++; > + } > + /* Second conversion: Consumes 0 bytes. */ > + ret = mbrtowc (&wc, mbs, inlen - consumed, &st); > + if (ret != 0) > + { > + printf ("error: Second conversion consumed only %zd bytes.\n", ret); > + err++; > + } > + /* Advance the zero consumed bytes. */ > + mbs += ret; > + consumed += ret; > + if (wc != test.expected[1]) > + { > + printf ("error: Result of second conversion was wrong.\n"); > + err++; > + } > + /* After the second conversion the state of the converter should be > + in the initial state. It is in the initial state because the two > + input BIG5-HKSCS bytes have been consumed and the 2 wchar_t's have > + been output. */ > + if (mbsinit (&st) == 0) > + { > + printf ("error: Converter not in initial state.\n"); > + err++; > + } > + /* Third conversion: Consumes 1 byte (it's an ASCII character). */ > + ret = mbrtowc (&wc, mbs, inlen - consumed, &st); > + if (ret != 1) > + { > + printf ("error: Third conversion consumed only %zd bytes.\n", ret); > + err++; > + } > + /* Advance the one byte. */ > + mbs += ret; > + consumed += ret; > + if (wc != test.expected[2]) > + { > + printf ("error: Result of third conversion was wrong.\n"); > + err++; > + } > + /* Return 0 if we saw no errors. */ > + return err; > +} > + > +static int > +do_test (void) > +{ > + int err = 0; > + int ret; > + /* Testing BIG5-HKSCS. */ > + xsetlocale (LC_ALL, "zh_HK.BIG5-HKSCS"); > + > + /* Run all the special conversions. */ > + for (int i = 0; i < (sizeof (tests) / sizeof (struct testdata)); i++) > + { > + printf ("Running test for %s\n", tests[i].name); > + ret = check_conversion (tests[i]); > + if (ret > 0) > + printf ("Test %s failed.\n", tests[i].name); > + err += ret; > + } > + > + /* Fail if any conversion had an error. */ > + if (err > 0) > + FAIL_EXIT1 ("One or more conversions failed."); > + > + return 0; > +} > + > +#include <support/test-driver.c> ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] Reset converter state after second wchar_t output (Bug 25734) 2020-04-04 18:41 ` [PATCH v2] " Carlos O'Donell 2020-04-07 4:20 ` Tom Honermann @ 2020-04-14 19:03 ` Tom Honermann 2020-04-14 19:56 ` Carlos O'Donell 1 sibling, 1 reply; 22+ messages in thread From: Tom Honermann @ 2020-04-14 19:03 UTC (permalink / raw) To: Carlos O'Donell, Andreas Schwab, libc-alpha, Rafal Luzynski On 4/4/20 2:41 PM, Carlos O'Donell via Libc-alpha wrote: > v2 > - Add test dependency on locale. > - Make test use generic converter dependency (builds all converters) > - Move Rules inclusion to after locale generation to match localedata/ order. > > No regressions on x86_64 and i686. > > I'll push this next week unless someone objects. Hi, Carlos. Do you have an update on pushing this? I sent a patch [1] for bug 25744 that depends on your changes. Do you have any concerns to share with that patch? If not, could you please commit it along with your changes for bug 25734? Tom. [1]: https://sourceware.org/pipermail/libc-alpha/2020-April/112595.html ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] Reset converter state after second wchar_t output (Bug 25734) 2020-04-14 19:03 ` Tom Honermann @ 2020-04-14 19:56 ` Carlos O'Donell 2020-04-16 2:41 ` Carlos O'Donell 0 siblings, 1 reply; 22+ messages in thread From: Carlos O'Donell @ 2020-04-14 19:56 UTC (permalink / raw) To: Tom Honermann, Andreas Schwab, libc-alpha, Rafal Luzynski On 4/14/20 3:03 PM, Tom Honermann wrote: > On 4/4/20 2:41 PM, Carlos O'Donell via Libc-alpha wrote: >> v2 >> - Add test dependency on locale. >> - Make test use generic converter dependency (builds all converters) >> - Move Rules inclusion to after locale generation to match localedata/ order. >> >> No regressions on x86_64 and i686. >> >> I'll push this next week unless someone objects. > > Hi, Carlos. Do you have an update on pushing this? > > I sent a patch [1] for bug 25744 that depends on your changes. Do you have any concerns to share with that patch? If not, could you please commit it along with your changes for bug 25734? > > Tom. > > [1]: https://sourceware.org/pipermail/libc-alpha/2020-April/112595.html I'm retesting this again today on 64-bit and 32-bit and will push shortly. Thanks for the ping. Sorry for the delay. -- Cheers, Carlos. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] Reset converter state after second wchar_t output (Bug 25734) 2020-04-14 19:56 ` Carlos O'Donell @ 2020-04-16 2:41 ` Carlos O'Donell 2020-04-16 3:08 ` Tom Honermann 0 siblings, 1 reply; 22+ messages in thread From: Carlos O'Donell @ 2020-04-16 2:41 UTC (permalink / raw) To: Tom Honermann, Andreas Schwab, libc-alpha, Rafal Luzynski On 4/14/20 3:56 PM, Carlos O'Donell wrote: > On 4/14/20 3:03 PM, Tom Honermann wrote: >> On 4/4/20 2:41 PM, Carlos O'Donell via Libc-alpha wrote: >>> v2 >>> - Add test dependency on locale. >>> - Make test use generic converter dependency (builds all converters) >>> - Move Rules inclusion to after locale generation to match localedata/ order. >>> >>> No regressions on x86_64 and i686. >>> >>> I'll push this next week unless someone objects. >> >> Hi, Carlos. Do you have an update on pushing this? >> >> I sent a patch [1] for bug 25744 that depends on your changes. Do you have any concerns to share with that patch? If not, could you please commit it along with your changes for bug 25734? >> >> Tom. >> >> [1]: https://sourceware.org/pipermail/libc-alpha/2020-April/112595.html > > I'm retesting this again today on 64-bit and 32-bit and will push shortly. > > Thanks for the ping. Sorry for the delay. All clean. Pushed v2. -- Cheers, Carlos. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] Reset converter state after second wchar_t output (Bug 25734) 2020-04-16 2:41 ` Carlos O'Donell @ 2020-04-16 3:08 ` Tom Honermann 2020-04-16 3:10 ` Carlos O'Donell 0 siblings, 1 reply; 22+ messages in thread From: Tom Honermann @ 2020-04-16 3:08 UTC (permalink / raw) To: Carlos O'Donell, Andreas Schwab, libc-alpha, Rafal Luzynski On 4/15/20 10:41 PM, Carlos O'Donell wrote: > On 4/14/20 3:56 PM, Carlos O'Donell wrote: >> On 4/14/20 3:03 PM, Tom Honermann wrote: >>> On 4/4/20 2:41 PM, Carlos O'Donell via Libc-alpha wrote: >>>> v2 >>>> - Add test dependency on locale. >>>> - Make test use generic converter dependency (builds all converters) >>>> - Move Rules inclusion to after locale generation to match localedata/ order. >>>> >>>> No regressions on x86_64 and i686. >>>> >>>> I'll push this next week unless someone objects. >>> Hi, Carlos. Do you have an update on pushing this? >>> >>> I sent a patch [1] for bug 25744 that depends on your changes. Do you have any concerns to share with that patch? If not, could you please commit it along with your changes for bug 25734? >>> >>> Tom. >>> >>> [1]: https://sourceware.org/pipermail/libc-alpha/2020-April/112595.html >> I'm retesting this again today on 64-bit and 32-bit and will push shortly. >> >> Thanks for the ping. Sorry for the delay. > All clean. Pushed v2. > Thanks, Carlos. I see the patch <https://sourceware.org/pipermail/libc-alpha/2020-April/112595.html> [1] I submitted for bug 25744 wasn't included. Do you have some concerns with that patch? If not, could you please commit it as well? Tom. [1]: https://sourceware.org/pipermail/libc-alpha/2020-April/112595.html ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] Reset converter state after second wchar_t output (Bug 25734) 2020-04-16 3:08 ` Tom Honermann @ 2020-04-16 3:10 ` Carlos O'Donell 0 siblings, 0 replies; 22+ messages in thread From: Carlos O'Donell @ 2020-04-16 3:10 UTC (permalink / raw) To: Tom Honermann, Andreas Schwab, libc-alpha, Rafal Luzynski On 4/15/20 11:08 PM, Tom Honermann wrote: > On 4/15/20 10:41 PM, Carlos O'Donell wrote: >> On 4/14/20 3:56 PM, Carlos O'Donell wrote: >>> On 4/14/20 3:03 PM, Tom Honermann wrote: >>>> On 4/4/20 2:41 PM, Carlos O'Donell via Libc-alpha wrote: >>>>> v2 - Add test dependency on locale. - Make test use generic >>>>> converter dependency (builds all converters) - Move Rules >>>>> inclusion to after locale generation to match localedata/ >>>>> order. >>>>> >>>>> No regressions on x86_64 and i686. >>>>> >>>>> I'll push this next week unless someone objects. >>>> Hi, Carlos. Do you have an update on pushing this? >>>> >>>> I sent a patch [1] for bug 25744 that depends on your changes. >>>> Do you have any concerns to share with that patch? If not, >>>> could you please commit it along with your changes for bug >>>> 25734? >>>> >>>> Tom. >>>> >>>> [1]: >>>> https://sourceware.org/pipermail/libc-alpha/2020-April/112595.html >>> >>>> I'm retesting this again today on 64-bit and 32-bit and will push shortly. >>> >>> Thanks for the ping. Sorry for the delay. >> All clean. Pushed v2. >> > Thanks, Carlos. I see the patch > <https://sourceware.org/pipermail/libc-alpha/2020-April/112595.html> > [1] I submitted for bug 25744 wasn't included. Do you have some > concerns with that patch? If not, could you please commit it as > well? I have to review your patch, and get consensus on that. The converter hang is an obvious fix (bug 25734), but I haven't had a chance to fully review your change. Next is to review your change and see if we can finally smooth out the bugs in the converter for these cases. Thank you for your patience! -- Cheers, Carlos. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] Reset converter state after second wchar_t output (Bug 25734) 2020-03-27 21:18 [PATCH] Reset converter state after second wchar_t output (Bug 25734) Carlos O'Donell 2020-03-28 20:37 ` Carlos O'Donell @ 2020-03-30 12:11 ` Andreas Schwab 2020-03-30 14:19 ` Florian Weimer 2020-03-30 14:34 ` Carlos O'Donell 1 sibling, 2 replies; 22+ messages in thread From: Andreas Schwab @ 2020-03-30 12:11 UTC (permalink / raw) To: Carlos O'Donell; +Cc: libc-alpha, Rafal Luzynski On Mär 27 2020, Carlos O'Donell wrote: > An input BIG5-HKSCS character may be converted into at most 2 whcar_t > characters. Could someone please file an interpretation request for POSIX, what should happen in that two wchar_t case? I think a case could be made that mbrtowc should return -1/EILSEQ, but that also has implications for mbsrtowcs, since that is defined in terms of repeated application of mbrtowc. Andreas. -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different." ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] Reset converter state after second wchar_t output (Bug 25734) 2020-03-30 12:11 ` [PATCH] " Andreas Schwab @ 2020-03-30 14:19 ` Florian Weimer 2020-03-30 14:28 ` Andreas Schwab 2020-03-30 14:34 ` Carlos O'Donell 1 sibling, 1 reply; 22+ messages in thread From: Florian Weimer @ 2020-03-30 14:19 UTC (permalink / raw) To: Andreas Schwab; +Cc: Carlos O'Donell, libc-alpha * Andreas Schwab: > On Mär 27 2020, Carlos O'Donell wrote: > >> An input BIG5-HKSCS character may be converted into at most 2 whcar_t >> characters. > > Could someone please file an interpretation request for POSIX, what > should happen in that two wchar_t case? Why POSIX and not ISO C? I'm not sure if the C committee wants implementations to be able to support Big5 (without Unicode changes first, to add characters which avoid the two-codepoint special cases). ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] Reset converter state after second wchar_t output (Bug 25734) 2020-03-30 14:19 ` Florian Weimer @ 2020-03-30 14:28 ` Andreas Schwab 2020-03-30 14:32 ` Florian Weimer 2020-03-30 14:36 ` Carlos O'Donell 0 siblings, 2 replies; 22+ messages in thread From: Andreas Schwab @ 2020-03-30 14:28 UTC (permalink / raw) To: Florian Weimer; +Cc: libc-alpha On Mär 30 2020, Florian Weimer wrote: > I'm not sure if the C committee wants implementations to be able to > support Big5 (without Unicode changes first, to add characters which > avoid the two-codepoint special cases). Are you saying mbrtowc should return -1 here? Andreas. -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different." ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] Reset converter state after second wchar_t output (Bug 25734) 2020-03-30 14:28 ` Andreas Schwab @ 2020-03-30 14:32 ` Florian Weimer 2020-03-30 14:36 ` Carlos O'Donell 1 sibling, 0 replies; 22+ messages in thread From: Florian Weimer @ 2020-03-30 14:32 UTC (permalink / raw) To: Andreas Schwab; +Cc: libc-alpha * Andreas Schwab: > On Mär 30 2020, Florian Weimer wrote: > >> I'm not sure if the C committee wants implementations to be able to >> support Big5 (without Unicode changes first, to add characters which >> avoid the two-codepoint special cases). > > Are you saying mbrtowc should return -1 here? Sorry, I haven't looked at the situation in detail, to determine what the best possible choice in the context of our own implementation. I merely don't think we can rely on C or POSIX to provide guidance how to support locales with these fake shift states. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] Reset converter state after second wchar_t output (Bug 25734) 2020-03-30 14:28 ` Andreas Schwab 2020-03-30 14:32 ` Florian Weimer @ 2020-03-30 14:36 ` Carlos O'Donell 2020-03-30 15:28 ` Andreas Schwab 1 sibling, 1 reply; 22+ messages in thread From: Carlos O'Donell @ 2020-03-30 14:36 UTC (permalink / raw) To: Andreas Schwab, Florian Weimer; +Cc: libc-alpha On 3/30/20 10:28 AM, Andreas Schwab wrote: > On Mär 30 2020, Florian Weimer wrote: > >> I'm not sure if the C committee wants implementations to be able to >> support Big5 (without Unicode changes first, to add characters which >> avoid the two-codepoint special cases). > > Are you saying mbrtowc should return -1 here? No. That indicates an invalid multibyte sequence was found. The sequence is not invalid. There are in fact no semantics that apply given the current API design. -- Cheers, Carlos. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] Reset converter state after second wchar_t output (Bug 25734) 2020-03-30 14:36 ` Carlos O'Donell @ 2020-03-30 15:28 ` Andreas Schwab 2020-03-30 17:52 ` Carlos O'Donell 0 siblings, 1 reply; 22+ messages in thread From: Andreas Schwab @ 2020-03-30 15:28 UTC (permalink / raw) To: Carlos O'Donell via Libc-alpha; +Cc: Florian Weimer, Carlos O'Donell On Mär 30 2020, Carlos O'Donell via Libc-alpha wrote: > On 3/30/20 10:28 AM, Andreas Schwab wrote: >> On Mär 30 2020, Florian Weimer wrote: >> >>> I'm not sure if the C committee wants implementations to be able to >>> support Big5 (without Unicode changes first, to add characters which >>> avoid the two-codepoint special cases). >> >> Are you saying mbrtowc should return -1 here? > > No. That indicates an invalid multibyte sequence was found. It is not representable, thus invalid. Andreas. -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different." ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] Reset converter state after second wchar_t output (Bug 25734) 2020-03-30 15:28 ` Andreas Schwab @ 2020-03-30 17:52 ` Carlos O'Donell 0 siblings, 0 replies; 22+ messages in thread From: Carlos O'Donell @ 2020-03-30 17:52 UTC (permalink / raw) To: Andreas Schwab, Carlos O'Donell via Libc-alpha; +Cc: Florian Weimer On 3/30/20 11:28 AM, Andreas Schwab wrote: > On Mär 30 2020, Carlos O'Donell via Libc-alpha wrote: > >> On 3/30/20 10:28 AM, Andreas Schwab wrote: >>> On Mär 30 2020, Florian Weimer wrote: >>> >>>> I'm not sure if the C committee wants implementations to be able to >>>> support Big5 (without Unicode changes first, to add characters which >>>> avoid the two-codepoint special cases). >>> >>> Are you saying mbrtowc should return -1 here? >> >> No. That indicates an invalid multibyte sequence was found. > > It is not representable, thus invalid. Sorry, I think I misunderstood your question. I think you are actually asking what should a hypothetically correct implementation do in this case? If that is your question, then I agree, it should return -1 when it finds any input that violates the C requirements. I would *not* change glibc to do this though since BIG5-HKSCS is supported and in use in glibc. A simple converter can be written that goes through all input bytes bytes until the input is at the end or errors out (rather than stopping at the observed L'\0'), but it requires you know the length of the input. I have seen many examples looking for result > 0 though, so I expect such codes would immediately stop when encountering such BIG5-HKSCS input that generates a 0 return. The Microsoft docs have a similar example stopping the conversion when 0 is returned, but using -2 to continue stepping through the input, advancing by one byte to attempt to put together the incomplete sequence (expecting the state to accrue). The Microsoft docs are here: https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/mbrtowc?view=vs-2019 Example here: https://rextester.com/UYPGU65292 Windows: 1st mbrtowc call: 0xF325 result: 2 2nd mbrtowc call: 0x0062 result: 1 3rd mbrtowc call: 0x0058 result: 1 Linux: 1st mbrtowc call: 0x00CA result: 2 2nd mbrtowc call: 0x0304 result: 0 3rd mbrtowc call: 0x0058 result: 1 Note that in the Microsoft implementation you *can't* use the value of the return from mbrtowc to walk the input forward, and that seems like a mistake to me, at least 0 is an honest (if wrong) answer. -- Cheers, Carlos. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] Reset converter state after second wchar_t output (Bug 25734) 2020-03-30 12:11 ` [PATCH] " Andreas Schwab 2020-03-30 14:19 ` Florian Weimer @ 2020-03-30 14:34 ` Carlos O'Donell 2020-03-30 17:36 ` Tom Honermann 1 sibling, 1 reply; 22+ messages in thread From: Carlos O'Donell @ 2020-03-30 14:34 UTC (permalink / raw) To: Andreas Schwab; +Cc: libc-alpha, Rafal Luzynski, Tom Honermann On 3/30/20 8:11 AM, Andreas Schwab wrote: > On Mär 27 2020, Carlos O'Donell wrote: > >> An input BIG5-HKSCS character may be converted into at most 2 whcar_t >> characters. > > Could someone please file an interpretation request for POSIX, what > should happen in that two wchar_t case? I think a case could be made > that mbrtowc should return -1/EILSEQ, but that also has implications for > mbsrtowcs, since that is defined in terms of repeated application of > mbrtowc. Tom had already reached out to ISO C WG14 to discuss this since there is harmonization there between C and POSIX. The current thinking in WG14 is that BIG5-HKSCS violates the definition of "wide character" because the wchar_t value cannot represent the original character in the locale and so the semantics underlying BIG5-HKSCS will not map to the current API designs. Florian raised a similar issue in May of 2019 and the general feedback at that time was that BIG5-HKSCS is simply not supported by ISO C. I expect the same answer from POSIX which is harmonized with ISO C in this case. If BIG5-HKSCS is not supported, then the standard will have nothing to say about which values can be returned after the first or second input bytes are read. Options: (a) Do not change the current converter. We return 2 consumed bytes in the first conversion, and 0 on the second. (b) Look ahead and split the conversion. We return 1 consumed in the first conversion, and 1 on the second. This prevents us from returning 0 which may be interpreted as a L'\0'. This leads to false assumption that the user could stop the conversion at this point and modify the input. (c) Design something new. Return (size_t) -3 indicating a One-to-Many conversion is happening and that there is more output to be generated. Do you still want me to file an interpretation request with POSIX? -- Cheers, Carlos. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] Reset converter state after second wchar_t output (Bug 25734) 2020-03-30 14:34 ` Carlos O'Donell @ 2020-03-30 17:36 ` Tom Honermann 0 siblings, 0 replies; 22+ messages in thread From: Tom Honermann @ 2020-03-30 17:36 UTC (permalink / raw) To: Carlos O'Donell, Andreas Schwab; +Cc: libc-alpha, Rafal Luzynski On 3/30/20 10:34 AM, Carlos O'Donell wrote: > On 3/30/20 8:11 AM, Andreas Schwab wrote: >> On Mär 27 2020, Carlos O'Donell wrote: >> >>> An input BIG5-HKSCS character may be converted into at most 2 whcar_t >>> characters. >> Could someone please file an interpretation request for POSIX, what >> should happen in that two wchar_t case? I think a case could be made >> that mbrtowc should return -1/EILSEQ, but that also has implications for >> mbsrtowcs, since that is defined in terms of repeated application of >> mbrtowc. > Tom had already reached out to ISO C WG14 to discuss this since there is > harmonization there between C and POSIX. > > The current thinking in WG14 is that BIG5-HKSCS violates the definition > of "wide character" because the wchar_t value cannot represent the > original character in the locale and so the semantics underlying > BIG5-HKSCS will not map to the current API designs. > > Florian raised a similar issue in May of 2019 and the general feedback > at that time was that BIG5-HKSCS is simply not supported by ISO C. > I expect the same answer from POSIX which is harmonized with ISO C in > this case. > > If BIG5-HKSCS is not supported, then the standard will have nothing to > say about which values can be returned after the first or second input > bytes are read. If this is determined to be the case, then my preference is for option (c) below as this has the advantage of adopting behavior that is already standard for mbrtoc16. Tom. > > Options: > > (a) Do not change the current converter. We return 2 consumed bytes in > the first conversion, and 0 on the second. > > (b) Look ahead and split the conversion. We return 1 consumed in the > first conversion, and 1 on the second. This prevents us from returning > 0 which may be interpreted as a L'\0'. This leads to false assumption > that the user could stop the conversion at this point and modify the > input. > > (c) Design something new. Return (size_t) -3 indicating a One-to-Many > conversion is happening and that there is more output to be generated. > > Do you still want me to file an interpretation request with POSIX? > ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2020-04-16 3:10 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-27 21:18 [PATCH] Reset converter state after second wchar_t output (Bug 25734) Carlos O'Donell 2020-03-28 20:37 ` Carlos O'Donell 2020-03-28 20:51 ` Florian Weimer 2020-03-28 23:42 ` Carlos O'Donell 2020-03-30 17:40 ` Tom Honermann 2020-04-01 17:34 ` Tom Honermann 2020-04-04 18:41 ` [PATCH v2] " Carlos O'Donell 2020-04-07 4:20 ` Tom Honermann 2020-04-14 19:03 ` Tom Honermann 2020-04-14 19:56 ` Carlos O'Donell 2020-04-16 2:41 ` Carlos O'Donell 2020-04-16 3:08 ` Tom Honermann 2020-04-16 3:10 ` Carlos O'Donell 2020-03-30 12:11 ` [PATCH] " Andreas Schwab 2020-03-30 14:19 ` Florian Weimer 2020-03-30 14:28 ` Andreas Schwab 2020-03-30 14:32 ` Florian Weimer 2020-03-30 14:36 ` Carlos O'Donell 2020-03-30 15:28 ` Andreas Schwab 2020-03-30 17:52 ` Carlos O'Donell 2020-03-30 14:34 ` Carlos O'Donell 2020-03-30 17:36 ` Tom Honermann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).