On 2023-07-16 01:43, Bruno Haible wrote:
Paul Eggert wrote:
However, after implementing mbszero with this data and enabling its use
in many places, I got test failures on NetBSD and Solaris.
- On NetBSD, the minimum we need to clear is 28 bytes.
- On Solaris OmniOS and OpenIndiana, the minimum we need to clear is 16
bytes.
- On proprietary Solaris, the minimum we need to clear is 20 or 28 bytes
(depending on 32-bit or 64-bit mode).
So, clearly this is fragile stuff.
Were the test failures for single calls to mbrtoc32, or were they for
something else? If the former, what went wrong with the source-code
analysis?
Assuming the problem was not with single calls to mbrtoc32, how about if
we define another function mbszerotoc32 that is the minimum number of
bytes to clear so that a single call to mbrtoc32 will work? Such a macro
would be useful for diffutils, and I expect elsewhere.
> +/* _GL_MBSTATE_INIT_SIZE describes how mbsinit() behaves: It is the
number of
+ bytes at the beginning of an mbstate_t that need to be zero, for mbsinit()
+ to return true.
This macro is not used anywhere. How about adding a comment explaining
why it's defined but not used? Or if it's not needed we can remove it.
+# elif __GLIBC__ >= 2 /* glibc */
Should be glibc 2.2 not glibc 2, if I read the history right. Also
should check that __GLIBC__ is defined, for the benefit of picky compilers.
# elif defined MUSL_LIBC /* musl libc */
/* mbstate_t is defined in <bits/alltypes.h>.
It is an opaque aligned 8-byte struct, of which at most the first
4 bytes are used.
For more details, see src/multibyte/mbrtowc.c. */
# define _GL_MBSTATE_INIT_SIZE 4
# define _GL_MBSTATE_ZERO_SIZE 4
Better to say 'sizeof (unsigned)' instead of '4', as the source code
uses 'unsigned'. This wouldn't affect the machine code on existing
platforms, would be better documentation, and would be theoretically
better on oddball future platforms. Similarly for other parts of wchar.in.h.
+# elif defined __sun /* Solaris */
+/* On Solaris, mbstate_t is defined in <wchar_impl.h>.
+ It is an opaque aligned 24-byte or 32-byte struct, of which at most the
first
+ 20 or 28 bytes are used.
+ For more details, see the *State types in
+ illumos-gate/usr/src/lib/libc/port/locale/
+ {none,euc,mskanji,big5,gb2312,gbk,gb18030,utf8}.c. */
+/* File INIT_SIZE ZERO_SIZE
+ none.c 0 0
+ euc.c 12 12
+ mskanji.c 4 4
+ big5.c 4 4
+ gb2312.c 4 6
+ gbk.c 4 4
+ gb18030.c 4 8
+ utf8.c 12 12 */
+/* But 12 is not the correct value: we get test failures
+ - in OpenIndiana and OmniOS: for values < 16,
+ - in Solaris 10 and 11: for values < 20 (in 32-bit mode)
+ or < 28 (in 64-bit mode). */
+# if defined _LP64
+# define _GL_MBSTATE_INIT_SIZE 28
+# define _GL_MBSTATE_ZERO_SIZE 28
+# else
+# define _GL_MBSTATE_INIT_SIZE 20
+# define _GL_MBSTATE_ZERO_SIZE 20
+# endif
On 64-bit Solaris 10 sparc with either GCC or Oracle's compiler, it's
the same number of insns to initialize 28 bytes vs 32 bytes. So if 28 is
needed let's drop the optimization for Solaris as not worth the
aggravation of maintaining and worrying about its brittleness. 32-bit
Solaris is obsolete and will stop working in 2038 and is not worth
worrying about even if it's one less insn, so let's drop it for that too.
+# define _GL_MBSTATE_ZERO_SIZE sizeof (mbstate_t)
Might be better to have this sort of thing at the end, as the default,
rather than sprinkle lots of copies of it elsewhere. Likewise for
defaulting _GL_MBSTATE_INIT_SIZE to _GL_MBSTATE_ZERO_SIZE, since
initializing the latter implies initializing the former.
Proposed patch attached; it implements the above suggestions except for
the comment about _GL_MBSTATE_INIT_SIZE. I haven't installed this.
From 5def9d91a35b3a472577424abc21cda37d21dd35 Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Sun, 16 Jul 2023 15:23:52 -0700
Subject: [PATCH] wchar: new function mbszerotoc32
This is like mbszero, but specialized for when mbrtoc32 is
called once and no other functions are called.
It can be used by diffutils's mbcel code.
* lib/btoc32.c (btoc32):
* lib/localeinfo.c (init_localeinfo):
Prefer mbszerotoc32 to mbszero when either will do.
* lib/localeinfo.c (mbszerotoc32) [GAWK]: New macro.
* lib/wchar.in.h (_GL_MBSTATE_MBRTOC32_SIZE): New macro.
(_GL_MBSTATE_INIT_SIZE, _GL_MBSTATE_ZERO_SIZE):
Simplify defns by relying on defaults.
Don't bother optimizing Solaris.
(mbszerotoc32): New function.
---
ChangeLog | 16 +++++++++
lib/btoc32.c | 2 +-
lib/localeinfo.c | 3 +-
lib/wchar.in.h | 93 +++++++++++++++++++++++++++---------------------
4 files changed, 72 insertions(+), 42 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 2caecc594a..5bda436f81 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,19 @@
+2023-07-16 Paul Eggert <egg...@cs.ucla.edu>
+
+ wchar: new function mbszerotoc32
+ This is like mbszero, but specialized for when mbrtoc32 is
+ called once and no other functions are called.
+ It can be used by diffutils's mbcel code.
+ * lib/btoc32.c (btoc32):
+ * lib/localeinfo.c (init_localeinfo):
+ Prefer mbszerotoc32 to mbszero when either will do.
+ * lib/localeinfo.c (mbszerotoc32) [GAWK]: New macro.
+ * lib/wchar.in.h (_GL_MBSTATE_MBRTOC32_SIZE): New macro.
+ (_GL_MBSTATE_INIT_SIZE, _GL_MBSTATE_ZERO_SIZE):
+ Simplify defns by relying on defaults.
+ Don't bother optimizing Solaris.
+ (mbszerotoc32): New function.
+
2023-07-16 Bruno Haible <br...@clisp.org>
dfa: Optimize clearing an mbstate_t.
diff --git a/lib/btoc32.c b/lib/btoc32.c
index 4d5b9067e7..af33a96b49 100644
--- a/lib/btoc32.c
+++ b/lib/btoc32.c
@@ -45,7 +45,7 @@ btoc32 (int c)
char s[1];
char32_t wc;
- mbszero (&state);
+ mbszerotoc32 (&state);
s[0] = (unsigned char) c;
if (mbrtoc32 (&wc, s, 1, &state) <= 1)
return wc;
diff --git a/lib/localeinfo.c b/lib/localeinfo.c
index 16a17e4643..bb4b2bb30e 100644
--- a/lib/localeinfo.c
+++ b/lib/localeinfo.c
@@ -34,6 +34,7 @@
# define mbrtoc32 mbrtowc
# define c32tolower towlower
# define c32toupper towupper
+# define mbszerotoc32(s) memset (s, 0, sizeof *(s))
#else
/* Use ISO C 11 + gnulib API. */
# include <uchar.h>
@@ -104,7 +105,7 @@ init_localeinfo (struct localeinfo *localeinfo)
{
char c = i;
unsigned char uc = i;
- mbstate_t s = {0};
+ mbstate_t s; mbszerotoc32 (&s);
char32_t wc;
size_t len = mbrtoc32 (&wc, &c, 1, &s);
localeinfo->sbclen[uc] = len <= 1 ? 1 : - (int) - len;
diff --git a/lib/wchar.in.h b/lib/wchar.in.h
index 7d2c5ecd12..00c08c9cfe 100644
--- a/lib/wchar.in.h
+++ b/lib/wchar.in.h
@@ -340,34 +340,39 @@ _GL_WARN_ON_USE (mbsinit, "mbsinit is unportable - "
mbstate_t state;
mbszero (&state);
+
+ If the only use of STATE is as an argument to a single call to mbrtoc32,
+ this can be even faster:
+
+ mbstate_t state;
+ mbszerotoc32 (&state);
*/
-/* _GL_MBSTATE_INIT_SIZE describes how mbsinit() behaves: It is the number of
- bytes at the beginning of an mbstate_t that need to be zero, for mbsinit()
- to return true.
- _GL_MBSTATE_ZERO_SIZE is the number of bytes at the beginning of an mbstate_t
- that need to be zero,
- - for mbsinit() to return true, and
- - for all other multibyte-aware functions to operate properly.
+/* If _GL_MBSTATE_INIT_SIZE bytes at the beginning of an mbstate_t are zero,
+ mbsinit() returns true.
+ If _GL_MBSTATE_MBRTOC32_SIZE bytes at the beginning of an mbstate_t are zero,
+ a single call to mbrtoc32() operates as if all bytes were zero.
+ If _GL_MBSTATE_ZERO_SIZE bytes at the beginning of an mbstate_t are zero,
+ all multibyte-aware functions operate as if all bytes are zero
0 < _GL_MBSTATE_INIT_SIZE <= _GL_MBSTATE_ZERO_SIZE <= sizeof (mbstate_t).
- These values are determined by source code inspection. */
+ 0 < _GL_MBSTATE_MBRTOC32_SIZE <= _GL_MBSTATE_ZERO_SIZE.
+ These values are determined by source code inspection and by testing. */
# if GNULIB_defined_mbstate_t /* AIX, IRIX */
/* mbstate_t has at least 4 bytes. They are used as coded in
gnulib/lib/mbrtowc.c. */
# define _GL_MBSTATE_INIT_SIZE 1
-/* Note that 4 is not the correct value: it causes test failures. */
-# define _GL_MBSTATE_ZERO_SIZE sizeof (mbstate_t)
-# elif __GLIBC__ >= 2 /* glibc */
+# define _GL_MBSTATE_MBRTOC32_SIZE 4
+/* 4 is not correct for _GL_MBSTATE_ZERO_SIZE: it causes test failures. */
+# elif defined __GLIBC__ && 2 < __GLIBC__ + (2 <= __GLIBC_MINOR__)
/* mbstate_t is defined in <bits/types/__mbstate_t.h>.
For more details, see glibc/iconv/skeleton.c. */
-# define _GL_MBSTATE_INIT_SIZE 4
-# define _GL_MBSTATE_ZERO_SIZE /* 8 */ sizeof (mbstate_t)
+# define _GL_MBSTATE_INIT_SIZE _GL_MBSTATE_MBRTOC32_SIZE
+# define _GL_MBSTATE_MBRTOC32_SIZE sizeof (((mbstate_t) {0}).__count)
# elif defined MUSL_LIBC /* musl libc */
/* mbstate_t is defined in <bits/alltypes.h>.
It is an opaque aligned 8-byte struct, of which at most the first
- 4 bytes are used.
+ sizeof (unsigned) bytes are used.
For more details, see src/multibyte/mbrtowc.c. */
-# define _GL_MBSTATE_INIT_SIZE 4
-# define _GL_MBSTATE_ZERO_SIZE 4
+# define _GL_MBSTATE_ZERO_SIZE sizeof (unsigned)
# elif defined __APPLE__ && defined __MACH__ /* macOS */
/* On macOS, mbstate_t is defined in <machine/_types.h>.
It is an opaque aligned 128-byte struct, of which at most the first
@@ -386,7 +391,6 @@ _GL_WARN_ON_USE (mbsinit, "mbsinit is unportable - "
gb18030.c 4 8
utf8.c 8 10
utf2.c 8 12 */
-# define _GL_MBSTATE_INIT_SIZE 12
# define _GL_MBSTATE_ZERO_SIZE 12
# elif defined __FreeBSD__ /* FreeBSD */
/* On FreeBSD, mbstate_t is defined in src/sys/sys/_types.h.
@@ -405,7 +409,6 @@ _GL_WARN_ON_USE (mbsinit, "mbsinit is unportable - "
gbk.c 4 4
gb18030.c 4 8
utf8.c 8 12 */
-# define _GL_MBSTATE_INIT_SIZE 12
# define _GL_MBSTATE_ZERO_SIZE 12
# elif defined __NetBSD__ /* NetBSD */
/* On NetBSD, mbstate_t is defined in src/sys/sys/ansi.h.
@@ -425,8 +428,9 @@ _GL_WARN_ON_USE (mbsinit, "mbsinit is unportable - "
citrus/modules/citrus_dechanyu.c 8
citrus/modules/citrus_johab.c 6
citrus/modules/citrus_utf8.c 12 */
-/* But 12 is not the correct value: we get test failures for values < 28. */
-# define _GL_MBSTATE_INIT_SIZE 28
+# define _GL_MBSTATE_MBRTOC32_SIZE 12
+/* 12 is wrong for _GL_MBSTATE_ZERO_SIZE:
+ we get test failures for values < 28. */
# define _GL_MBSTATE_ZERO_SIZE 28
# elif defined __OpenBSD__ /* OpenBSD */
/* On OpenBSD, mbstate_t is defined in src/sys/sys/_types.h.
@@ -436,7 +440,6 @@ _GL_WARN_ON_USE (mbsinit, "mbsinit is unportable - "
/* File INIT_SIZE ZERO_SIZE
citrus_none.c 0 0
citrus_utf8.c 12 12 */
-# define _GL_MBSTATE_INIT_SIZE 12
# define _GL_MBSTATE_ZERO_SIZE 12
# elif defined __minix /* Minix */
/* On Minix, mbstate_t is defined in sys/sys/ansi.h.
@@ -445,7 +448,6 @@ _GL_WARN_ON_USE (mbsinit, "mbsinit is unportable - "
lib/libc/citrus/citrus_*.c. */
/* File INIT_SIZE ZERO_SIZE
citrus_none.c 0 0 */
-# define _GL_MBSTATE_INIT_SIZE 1
# define _GL_MBSTATE_ZERO_SIZE 1
# elif defined __sun /* Solaris */
/* On Solaris, mbstate_t is defined in <wchar_impl.h>.
@@ -463,29 +465,24 @@ _GL_WARN_ON_USE (mbsinit, "mbsinit is unportable - "
gbk.c 4 4
gb18030.c 4 8
utf8.c 12 12 */
-/* But 12 is not the correct value: we get test failures
+# define _GL_MBSTATE_MBRTOC32_SIZE 12
+/* 12 is not the correct value for _GL_MBSTATE_INIT_SIZE: we get test failures
- in OpenIndiana and OmniOS: for values < 16,
- in Solaris 10 and 11: for values < 20 (in 32-bit mode)
- or < 28 (in 64-bit mode). */
-# if defined _LP64
-# define _GL_MBSTATE_INIT_SIZE 28
-# define _GL_MBSTATE_ZERO_SIZE 28
-# else
-# define _GL_MBSTATE_INIT_SIZE 20
-# define _GL_MBSTATE_ZERO_SIZE 20
-# endif
+ or < 28 (in 64-bit mode).
+ Since Solaris values are so close to sizeof (mbstate_t), and
+ Solaris source code is not public, it is not worth the trouble and
+ risk to optimize for it. */
# elif defined __CYGWIN__ /* Cygwin */
/* On Cygwin, mbstate_t is defined in <sys/_types.h>.
For more details, see newlib/libc/stdlib/mbtowc_r.c and
winsup/cygwin/strfuncs.cc. */
-# define _GL_MBSTATE_INIT_SIZE 4
-# define _GL_MBSTATE_ZERO_SIZE 8
+# define _GL_MBSTATE_INIT_SIZE sizeof (int)
+# define _GL_MBSTATE_MBRTOC32_SIZE sizeof (int)
# elif defined _WIN32 && !defined __CYGWIN__ /* Native Windows. */
/* MSVC defines 'mbstate_t' as an aligned 8-byte struct.
On mingw, 'mbstate_t' is sometimes defined as 'int', sometimes defined
as an aligned 8-byte struct, of which the first 4 bytes matter. */
-# define _GL_MBSTATE_INIT_SIZE sizeof (mbstate_t)
-# define _GL_MBSTATE_ZERO_SIZE sizeof (mbstate_t)
# elif defined __ANDROID__ /* Android */
/* Android defines 'mbstate_t' in <bits/mbstate_t.h>.
It is an opaque 4-byte or 8-byte struct.
@@ -494,14 +491,18 @@ _GL_WARN_ON_USE (mbsinit, "mbsinit is unportable - "
bionic/libc/bionic/mbrtoc32.cpp
bionic/libc/bionic/mbrtoc16.cpp
*/
-# define _GL_MBSTATE_INIT_SIZE 4
# define _GL_MBSTATE_ZERO_SIZE 4
-# else
-/* On platforms where we don't know how the multibyte functions behave, use
- these safe values. */
-# define _GL_MBSTATE_INIT_SIZE sizeof (mbstate_t)
+# endif
+/* Default to safe values. */
+# ifndef _GL_MBSTATE_ZERO_SIZE
# define _GL_MBSTATE_ZERO_SIZE sizeof (mbstate_t)
# endif
+# ifndef _GL_MBSTATE_INIT_SIZE
+# define _GL_MBSTATE_INIT_SIZE _GL_MBSTATE_ZERO_SIZE
+# endif
+# ifndef _GL_MBSTATE_MBRTOC32_SIZE
+# define _GL_MBSTATE_MBRTOC32_SIZE _GL_MBSTATE_ZERO_SIZE
+# endif
_GL_BEGIN_C_LINKAGE
# if defined IN_MBSZERO
_GL_EXTERN_INLINE
@@ -513,9 +514,21 @@ mbszero (mbstate_t *ps)
{
memset (ps, 0, _GL_MBSTATE_ZERO_SIZE);
}
+# if defined IN_MBSZERO
+_GL_EXTERN_INLINE
+# else
+_GL_INLINE
+# endif
+_GL_ARG_NONNULL ((1)) void
+mbszerotoc32 (mbstate_t *ps)
+{
+ memset (ps, 0, _GL_MBSTATE_MBRTOC32_SIZE);
+}
_GL_END_C_LINKAGE
_GL_CXXALIAS_SYS (mbszero, void, (mbstate_t *ps));
_GL_CXXALIASWARN (mbszero);
+_GL_CXXALIAS_SYS (mbszerotoc32, void, (mbstate_t *ps));
+_GL_CXXALIASWARN (mbszerotoc32);
#endif
--
2.39.2
