[PATCH v2 0/3] Improve wcsstr

[PATCH v2 0/3] Improve wcsstr

[Adhemerval Zanella]

Different than strstr, wcsstr still uses an O(m*n) algorithm that might
be considered a security issue (although BZ 23865 was marked security-
since there is no actual application impact). 

The gnulib recently added a wrapper to fix it [1] and it is used as the
base de str-two-way.h implementation. This patch adds a similar
implementation, and different than strstr, neither the "shift table"
optimization nor the self-adapting filtering check is used because it
would result in a too-large shift table (and it also simplifies the
implementation bit).

The patchset also added a proper tests for wcsstr, based on strstr one.

[1] https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=commit;h=9411c5e467cf60f6295b9fed306029f341a0f24f

Changes from v1:
* Add more tests from gnulib.
* Removed unused macros from wcsstr.

Adhemerval Zanella (3):
  string: Remove c_strstr from test-strstr
  wcsmbs: Add test-wcsstr
  wcsmbs: Ensure wcstr worst-case linear execution time (BZ 23865)

 string/test-strstr.c | 305 ++++++++++++++++++++++++++++++++++--------
 wcsmbs/Makefile      |   1 +
 wcsmbs/test-wcsstr.c |  20 +++
 wcsmbs/wcs-two-way.h | 312 +++++++++++++++++++++++++++++++++++++++++++
 wcsmbs/wcsstr.c      |  95 ++++---------
 5 files changed, 609 insertions(+), 124 deletions(-)
 create mode 100644 wcsmbs/test-wcsstr.c
 create mode 100644 wcsmbs/wcs-two-way.h

-- 
2.34.1

[PATCH v2 1/3] string: Remove c_strstr from test-strstr

[Adhemerval Zanella]

There is no much point is checking the generic code if this is not
really used by libc.
---
 string/test-strstr.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/string/test-strstr.c b/string/test-strstr.c
index 4115f7d2fd..05d0b7c98c 100644
--- a/string/test-strstr.c
+++ b/string/test-strstr.c
@@ -21,11 +21,6 @@
 #include "test-string.h"
 
 
-#define STRSTR c_strstr
-#define libc_hidden_builtin_def(arg) /* nothing */
-#define __strnlen strnlen
-#include "strstr.c"
-
 /* Naive implementation to verify results.  */
 static char *
 simple_strstr (const char *s1, const char *s2)
@@ -52,7 +47,6 @@ simple_strstr (const char *s1, const char *s2)
 
 typedef char *(*proto_t) (const char *, const char *);
 
-IMPL (c_strstr, 0)
 IMPL (strstr, 1)
 
 
-- 
2.34.1

[PATCH v2 2/3] wcsmbs: Add test-wcsstr

[Adhemerval Zanella]

Parametrize test-strstr.c so it can be used to check wcsstr.

Checked on x86_64-linux-gnu and aarch64-linux-gnu.
---
 string/test-strstr.c | 122 +++++++++++++++++++++++++++----------------
 wcsmbs/Makefile      |   1 +
 wcsmbs/test-wcsstr.c |  20 +++++++
 3 files changed, 97 insertions(+), 46 deletions(-)
 create mode 100644 wcsmbs/test-wcsstr.c

diff --git a/string/test-strstr.c b/string/test-strstr.c
index 05d0b7c98c..f82aeb2cfa 100644
--- a/string/test-strstr.c
+++ b/string/test-strstr.c
@@ -17,16 +17,44 @@
    <https://www.gnu.org/licenses/>.  */
 
 #define TEST_MAIN
-#define TEST_NAME "strstr"
-#include "test-string.h"
+#ifndef WIDE
+# define TEST_NAME "strstr"
+#else
+# define TEST_NAME "wcsstr"
+#endif
+
+#ifndef WIDE
+# define CHAR char
+# define STRLEN strlen
+# define STRSTR strstr
+# define STRCPY strcpy
+# define MEMCPY memcpy
+# define MEMSET memset
+# define MEMPCPY mempcpy
+# define L(s) s
+#else
+# include <wchar.h>
+# define CHAR wchar_t
+# define STRLEN wcslen
+# define STRCPY wcscpy
+# define STRSTR wcsstr
+# define MEMCPY wmemcpy
+# define MEMSET wmemset
+# define MEMPCPY wmempcpy
+# define L(s) L ## s
+/* The test requires up to 8191 charateres, so allocate at least 32Kb
+   (considering 4kb page size).  */
+# define BUF1PAGES 4
+#endif
 
+#include "test-string.h"
 
 /* Naive implementation to verify results.  */
-static char *
-simple_strstr (const char *s1, const char *s2)
+static CHAR *
+simple_strstr (const CHAR *s1, const CHAR *s2)
 {
-  ssize_t s1len = strlen (s1);
-  ssize_t s2len = strlen (s2);
+  ssize_t s1len = STRLEN (s1);
+  ssize_t s2len = STRLEN (s2);
 
   if (s2len > s1len)
     return NULL;
@@ -38,28 +66,27 @@ simple_strstr (const char *s1, const char *s2)
 	if (s1[i + j] != s2[j])
 	  break;
       if (j == s2len)
-	return (char *) s1 + i;
+	return (CHAR *) s1 + i;
     }
 
   return NULL;
 }
 
 
-typedef char *(*proto_t) (const char *, const char *);
+typedef CHAR *(*proto_t) (const CHAR *, const CHAR *);
 
-IMPL (strstr, 1)
+IMPL (STRSTR, 1)
 
 
 static int
-check_result (impl_t *impl, const char *s1, const char *s2,
-	      char *exp_result)
+check_result (impl_t *impl, const CHAR *s1, const CHAR *s2,
+	      CHAR *exp_result)
 {
-  char *result = CALL (impl, s1, s2);
+  CHAR *result = CALL (impl, s1, s2);
   if (result != exp_result)
     {
-      error (0, 0, "Wrong result in function %s %s %s", impl->name,
-	     (result == NULL) ? "(null)" : result,
-	     (exp_result == NULL) ? "(null)" : exp_result);
+      error (0, 0, "Wrong result in function %p %p %p", impl->name,
+	     result, exp_result);
       ret = 1;
       return -1;
     }
@@ -68,7 +95,7 @@ check_result (impl_t *impl, const char *s1, const char *s2,
 }
 
 static void
-do_one_test (impl_t *impl, const char *s1, const char *s2, char *exp_result)
+do_one_test (impl_t *impl, const CHAR *s1, const CHAR *s2, CHAR *exp_result)
 {
   if (check_result (impl, s1, s2, exp_result) < 0)
     return;
@@ -79,49 +106,51 @@ static void
 do_test (size_t align1, size_t align2, size_t len1, size_t len2,
 	 int fail)
 {
-  char *s1 = (char *) (buf1 + align1);
-  char *s2 = (char *) (buf2 + align2);
+  align1 = align1 * sizeof (CHAR);
+  align2 = align2 * sizeof (CHAR);
 
-  static const char d[] = "1234567890abcdef";
-#define dl (sizeof (d) - 1)
-  char *ss2 = s2;
+  CHAR *s1 = (CHAR *) (buf1 + align1);
+  CHAR *s2 = (CHAR *) (buf2 + align2);
+
+  static const CHAR d[] = L("1234567890abcdef");
+  const size_t dl = STRLEN (d);
+  CHAR *ss2 = s2;
   for (size_t l = len2; l > 0; l = l > dl ? l - dl : 0)
     {
       size_t t = l > dl ? dl : l;
-      ss2 = mempcpy (ss2, d, t);
+      ss2 = MEMPCPY (ss2, d, t);
     }
   s2[len2] = '\0';
 
   if (fail)
     {
-      char *ss1 = s1;
+      CHAR *ss1 = s1;
       for (size_t l = len1; l > 0; l = l > dl ? l - dl : 0)
 	{
 	  size_t t = l > dl ? dl : l;
-	  memcpy (ss1, d, t);
+	  MEMCPY (ss1, d, t);
 	  ++ss1[len2 > 7 ? 7 : len2 - 1];
 	  ss1 += t;
 	}
     }
   else
     {
-      memset (s1, '0', len1);
-      memcpy (s1 + len1 - len2, s2, len2);
+      MEMSET (s1, '0', len1);
+      MEMCPY (s1 + len1 - len2, s2, len2);
     }
   s1[len1] = '\0';
 
   FOR_EACH_IMPL (impl, 0)
     do_one_test (impl, s1, s2, fail ? NULL : s1 + len1 - len2);
-
 }
 
 static void
 check1 (void)
 {
-  const char s1[] =
-    "F_BD_CE_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD_C3_88_20_EF_BF_BD_EF_BF_BD_EF_BF_BD_C3_A7_20_EF_BF_BD";
-  const char s2[] = "_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD";
-  char *exp_result;
+  const CHAR s1[] =
+    L("F_BD_CE_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD_C3_88_20_EF_BF_BD_EF_BF_BD_EF_BF_BD_C3_A7_20_EF_BF_BD");
+  const CHAR s2[] = L("_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD");
+  CHAR *exp_result;
 
   exp_result = simple_strstr (s1, s2);
   FOR_EACH_IMPL (impl, 0)
@@ -131,30 +160,31 @@ check1 (void)
 static void
 check2 (void)
 {
-  const char s1_stack[] = ", enable_static, \0, enable_shared, ";
+  const CHAR s1_stack[] = L(", enable_static, \0, enable_shared, ");
   const size_t s1_byte_count = 18;
-  const char *s2_stack = &(s1_stack[s1_byte_count]);
-  const size_t s2_byte_count = 18;
-  char *exp_result;
+  const size_t s1_byte_len = 18 * sizeof (CHAR);
+  const CHAR *s2_stack = &(s1_stack[s1_byte_count]);
+  const size_t s2_byte_len = 18 * sizeof (CHAR);;
+  CHAR *exp_result;
   const size_t page_size_real = getpagesize ();
 
   /* Haystack at end of page.  The following page is protected.  */
-  char *s1_page_end = (void *) buf1 + page_size - s1_byte_count;
-  strcpy (s1_page_end, s1_stack);
+  CHAR *s1_page_end = (void *) buf1 + page_size - s1_byte_len;
+  STRCPY (s1_page_end, s1_stack);
 
   /* Haystack which crosses a page boundary.
      Note: page_size is at least 2 * getpagesize.  See test_init.  */
-  char *s1_page_cross = (void *) buf1 + page_size_real - 8;
-  strcpy (s1_page_cross, s1_stack);
+  CHAR *s1_page_cross = (void *) buf1 + page_size_real - 8;
+  STRCPY (s1_page_cross, s1_stack);
 
   /* Needle at end of page.  The following page is protected.  */
-  char *s2_page_end = (void *) buf2 + page_size - s2_byte_count;
-  strcpy (s2_page_end, s2_stack);
+  CHAR *s2_page_end = (void *) buf2 + page_size - s2_byte_len;
+  STRCPY (s2_page_end, s2_stack);
 
   /* Needle which crosses a page boundary.
      Note: page_size is at least 2 * getpagesize.  See test_init.  */
-  char *s2_page_cross = (void *) buf2 + page_size_real - 8;
-  strcpy (s2_page_cross, s2_stack);
+  CHAR *s2_page_cross = (void *) buf2 + page_size_real - 8;
+  STRCPY (s2_page_cross, s2_stack);
 
   exp_result = simple_strstr (s1_stack, s2_stack);
   FOR_EACH_IMPL (impl, 0)
@@ -178,8 +208,8 @@ check2 (void)
 static void
 pr23637 (void)
 {
-  char *h = (char*) buf1;
-  char *n = (char*) buf2;
+  CHAR *h = (CHAR*) buf1;
+  CHAR *n = (CHAR*) buf2;
 
   for (int i = 0; i < N; i++)
     {
@@ -194,7 +224,7 @@ pr23637 (void)
   /* Ensure we don't match at the first 'x'.  */
   h[0] = 'x';
 
-  char *exp_result = simple_strstr (h, n);
+  CHAR *exp_result = simple_strstr (h, n);
   FOR_EACH_IMPL (impl, 0)
     check_result (impl, h, n, exp_result);
 }
diff --git a/wcsmbs/Makefile b/wcsmbs/Makefile
index f3333c6a4b..1cddd8cc6d 100644
--- a/wcsmbs/Makefile
+++ b/wcsmbs/Makefile
@@ -163,6 +163,7 @@ tests := \
   test-wcspbrk \
   test-wcsrchr \
   test-wcsspn \
+  test-wcsstr \
   test-wmemchr \
   test-wmemcmp \
   test-wmemset \
diff --git a/wcsmbs/test-wcsstr.c b/wcsmbs/test-wcsstr.c
new file mode 100644
index 0000000000..23d6517255
--- /dev/null
+++ b/wcsmbs/test-wcsstr.c
@@ -0,0 +1,20 @@
+/* Test wcsstr function.
+   Copyright (C) 2024 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/>.  */
+
+#define WIDE 1
+#include <string/test-strstr.c>
-- 
2.34.1

[PATCH v2 3/3] wcsmbs: Ensure wcstr worst-case linear execution time (BZ 23865)

[Adhemerval Zanella]

It uses the same two-way algorithm used on strstr, strcasestr, and
memmem.  Different than strstr, neither the "shift table" optimization
nor the self-adapting filtering check is used because it would result in
a too-large shift table (and it also simplifies the implementation bit).

Checked on x86_64-linux-gnu and aarch64-linux-gnu.
---
 string/test-strstr.c | 177 ++++++++++++++++++++++++
 wcsmbs/wcs-two-way.h | 312 +++++++++++++++++++++++++++++++++++++++++++
 wcsmbs/wcsstr.c      |  95 ++++---------
 3 files changed, 512 insertions(+), 72 deletions(-)
 create mode 100644 wcsmbs/wcs-two-way.h

diff --git a/string/test-strstr.c b/string/test-strstr.c
index f82aeb2cfa..7506f0db90 100644
--- a/string/test-strstr.c
+++ b/string/test-strstr.c
@@ -203,6 +203,31 @@ check2 (void)
     }
 }
 
+static void
+check3 (void)
+{
+  /* Check that a long periodic needle does not cause false positives.  */
+  {
+    const CHAR input[] = L("F_BD_CE_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD"
+                            "_C3_88_20_EF_BF_BD_EF_BF_BD_EF_BF_BD"
+                            "_C3_A7_20_EF_BF_BD");
+    const CHAR need[] = L("_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD");
+    FOR_EACH_IMPL (impl, 0)
+      check_result (impl, input, need, NULL);
+  }
+
+  {
+    const CHAR input[] = L("F_BD_CE_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD"
+			   "_C3_88_20_EF_BF_BD_EF_BF_BD_EF_BF_BD"
+			   "_C3_A7_20_EF_BF_BD_DA_B5_C2_A6_20"
+			   "_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD");
+    const CHAR need[] = L("_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD");
+    FOR_EACH_IMPL (impl, 0)
+      check_result (impl, input, need, (CHAR *) input + 115);
+  }
+}
+
+
 #define N 1024
 
 static void
@@ -229,6 +254,156 @@ pr23637 (void)
     check_result (impl, h, n, exp_result);
 }
 
+static void
+pr23865 (void)
+{
+  /* Check that a very long haystack is handled quickly if the needle is
+     short and occurs near the beginning.  */
+  {
+    size_t repeat = 10000;
+    size_t m = 1000000;
+    const CHAR *needle =
+      L("AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+        "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA");
+    CHAR *haystack = xmalloc ((m + 1) * sizeof (CHAR));
+    MEMSET (haystack, L('A'), m);
+    haystack[0] = L('B');
+    haystack[m] = L('\0');
+
+    for (; repeat > 0; repeat--)
+      {
+	FOR_EACH_IMPL (impl, 0)
+	  check_result (impl, haystack, needle, haystack + 1);
+      }
+
+    free (haystack);
+  }
+
+  /* Check that a very long needle is discarded quickly if the haystack is
+     short.  */
+  {
+    size_t repeat = 10000;
+    size_t m = 1000000;
+    const CHAR *haystack =
+      L("AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+	"ABABABABABABABABABABABABABABABABABABABABABABABABABABABABABABABABABAB");
+    CHAR *needle = xmalloc ((m + 1) * sizeof (CHAR));
+    MEMSET (needle, L'A', m);
+    needle[m] = L('\0');
+
+    for (; repeat > 0; repeat--)
+      {
+	FOR_EACH_IMPL (impl, 0)
+	  check_result (impl, haystack, needle, NULL);
+      }
+
+    free (needle);
+  }
+
+  /* Check that the asymptotic worst-case complexity is not quadratic.  */
+  {
+    size_t m = 1000000;
+    CHAR *haystack = xmalloc ((2 * m + 2) * sizeof (CHAR));
+    CHAR *needle = xmalloc ((m + 2) * sizeof (CHAR));
+
+    MEMSET (haystack, L('A'), 2 * m);
+    haystack[2 * m] = L('B');
+    haystack[2 * m + 1] = L('\0');
+
+    MEMSET (needle, L('A'), m);
+    needle[m] = L('B');
+    needle[m + 1] = L('\0');
+
+    FOR_EACH_IMPL (impl, 0)
+      check_result (impl, haystack, needle, haystack + m);
+
+    free (needle);
+    free (haystack);
+  }
+
+  {
+    /* Ensure that with a barely periodic "short" needle, STRSTR's
+       search does not mistakenly skip just past the match point.  */
+    const CHAR *haystack =
+      L("\n"
+       "with_build_libsubdir\n"
+       "with_local_prefix\n"
+       "with_gxx_include_dir\n"
+       "with_cpp_install_dir\n"
+       "enable_generated_files_in_srcdir\n"
+       "with_gnu_ld\n"
+       "with_ld\n"
+       "with_demangler_in_ld\n"
+       "with_gnu_as\n"
+       "with_as\n"
+       "enable_largefile\n"
+       "enable_werror_always\n"
+       "enable_checking\n"
+       "enable_coverage\n"
+       "enable_gather_detailed_mem_stats\n"
+       "enable_build_with_cxx\n"
+       "with_stabs\n"
+       "enable_multilib\n"
+       "enable___cxa_atexit\n"
+       "enable_decimal_float\n"
+       "enable_fixed_point\n"
+       "enable_threads\n"
+       "enable_tls\n"
+       "enable_objc_gc\n"
+       "with_dwarf2\n"
+       "enable_shared\n"
+       "with_build_sysroot\n"
+       "with_sysroot\n"
+       "with_specs\n"
+       "with_pkgversion\n"
+       "with_bugurl\n"
+       "enable_languages\n"
+       "with_multilib_list\n");
+    const CHAR *needle =
+      L("\n"
+       "with_gnu_ld\n");
+
+    FOR_EACH_IMPL (impl, 0)
+      check_result (impl, haystack, needle, (CHAR*) haystack + 114);
+  }
+
+  {
+    /* Same bug, shorter trigger.  */
+    const CHAR *haystack = L("..wi.d.");
+    const CHAR *needle = L(".d.");
+    FOR_EACH_IMPL (impl, 0)
+      check_result (impl, haystack, needle, (CHAR*) haystack + 4);
+  }
+
+  /* Test case from Yves Bastide.
+     <https://www.openwall.com/lists/musl/2014/04/18/2>  */
+  {
+    const CHAR *input = L("playing play play play always");
+    const CHAR *needle = L("play play play");
+    FOR_EACH_IMPL (impl, 0)
+      check_result (impl, input, needle, (CHAR*) input + 8);
+  }
+
+  /* Test long needles.  */
+  {
+    size_t m = 1024;
+    CHAR *haystack = xmalloc ((2 * m + 1) * sizeof (CHAR));
+    CHAR *needle = xmalloc ((m + 1) * sizeof (CHAR));
+    haystack[0] = L('x');
+    MEMSET (haystack + 1, L(' '), m - 1);
+    MEMSET (haystack + m, L('x'), m);
+    haystack[2 * m] = L('\0');
+    MEMSET (needle, L('x'), m);
+    needle[m] = L('\0');
+
+    FOR_EACH_IMPL (impl, 0)
+      check_result (impl, haystack, needle, haystack + m);
+
+    free (needle);
+    free (haystack);
+  }
+}
+
 static int
 test_main (void)
 {
@@ -236,7 +411,9 @@ test_main (void)
 
   check1 ();
   check2 ();
+  check3 ();
   pr23637 ();
+  pr23865 ();
 
   printf ("%23s", "");
   FOR_EACH_IMPL (impl, 0)
diff --git a/wcsmbs/wcs-two-way.h b/wcsmbs/wcs-two-way.h
new file mode 100644
index 0000000000..2dcee7fc1a
--- /dev/null
+++ b/wcsmbs/wcs-two-way.h
@@ -0,0 +1,312 @@
+/* Byte-wise substring search, using the Two-Way algorithm.
+   Copyright (C) 2024 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/>.  */
+
+/* Before including this file, you need to include <string.h> (and
+   <config.h> before that, if not part of libc), and define:
+     AVAILABLE(h, h_l, j, n_l)
+			     A macro that returns nonzero if there are
+			     at least N_L characters left starting at H[J].
+			     H is 'wchar_t *', H_L, J, and N_L are 'size_t';
+			     H_L is an lvalue.  For NUL-terminated searches,
+			     H_L can be modified each iteration to avoid
+			     having to compute the end of H up front.
+
+  For case-insensitivity, you may optionally define:
+     CMP_FUNC(p1, p2, l)     A macro that returns 0 iff the first L
+			     characters of P1 and P2 are equal.
+     CANON_ELEMENT(c)        A macro that canonicalizes an element right after
+			     it has been fetched from one of the two strings.
+			     The argument is an 'wchar_t'; the result must
+			     be an 'wchar_t' as well.
+*/
+
+#include <limits.h>
+#include <stdint.h>
+#include <sys/param.h>                  /* Defines MAX.  */
+
+/* We use the Two-Way string matching algorithm, which guarantees
+   linear complexity with constant space.
+
+   See http://www-igm.univ-mlv.fr/~lecroq/string/node26.html#SECTION00260
+   and http://en.wikipedia.org/wiki/Boyer-Moore_string_search_algorithm
+*/
+
+#ifndef CANON_ELEMENT
+# define CANON_ELEMENT(c) c
+#endif
+#ifndef CMP_FUNC
+# define CMP_FUNC __wmemcmp
+#endif
+
+/* Perform a critical factorization of NEEDLE, of length NEEDLE_LEN.
+   Return the index of the first character in the right half, and set
+   *PERIOD to the global period of the right half.
+
+   The global period of a string is the smallest index (possibly its
+   length) at which all remaining bytes in the string are repetitions
+   of the prefix (the last repetition may be a subset of the prefix).
+
+   When NEEDLE is factored into two halves, a local period is the
+   length of the smallest word that shares a suffix with the left half
+   and shares a prefix with the right half.  All factorizations of a
+   non-empty NEEDLE have a local period of at least 1 and no greater
+   than NEEDLE_LEN.
+
+   A critical factorization has the property that the local period
+   equals the global period.  All strings have at least one critical
+   factorization with the left half smaller than the global period.
+
+   Given an ordered alphabet, a critical factorization can be computed
+   in linear time, with 2 * NEEDLE_LEN comparisons, by computing the
+   larger of two ordered maximal suffixes.  The ordered maximal
+   suffixes are determined by lexicographic comparison of
+   periodicity.  */
+static size_t
+critical_factorization (const wchar_t *needle, size_t needle_len,
+			size_t *period)
+{
+  /* Index of last character of left half, or SIZE_MAX.  */
+  size_t max_suffix, max_suffix_rev;
+  size_t j; /* Index into NEEDLE for current candidate suffix.  */
+  size_t k; /* Offset into current period.  */
+  size_t p; /* Intermediate period.  */
+  wchar_t a, b; /* Current comparison bytes.  */
+
+  /* Special case NEEDLE_LEN of 1 or 2 (all callers already filtered
+     out 0-length needles.  */
+  if (needle_len < 3)
+    {
+      *period = 1;
+      return needle_len - 1;
+    }
+
+  /* Invariants:
+     0 <= j < NEEDLE_LEN - 1
+     -1 <= max_suffix{,_rev} < j (treating SIZE_MAX as if it were signed)
+     min(max_suffix, max_suffix_rev) < global period of NEEDLE
+     1 <= p <= global period of NEEDLE
+     p == global period of the substring NEEDLE[max_suffix{,_rev}+1...j]
+     1 <= k <= p
+  */
+
+  /* Perform lexicographic search.  */
+  max_suffix = SIZE_MAX;
+  j = 0;
+  k = p = 1;
+  while (j + k < needle_len)
+    {
+      a = CANON_ELEMENT (needle[j + k]);
+      b = CANON_ELEMENT (needle[max_suffix + k]);
+      if (a < b)
+	{
+	  /* Suffix is smaller, period is entire prefix so far.  */
+	  j += k;
+	  k = 1;
+	  p = j - max_suffix;
+	}
+      else if (a == b)
+	{
+	  /* Advance through repetition of the current period.  */
+	  if (k != p)
+	    ++k;
+	  else
+	    {
+	      j += p;
+	      k = 1;
+	    }
+	}
+      else /* b < a */
+	{
+	  /* Suffix is larger, start over from current location.  */
+	  max_suffix = j++;
+	  k = p = 1;
+	}
+    }
+  *period = p;
+
+  /* Perform reverse lexicographic search.  */
+  max_suffix_rev = SIZE_MAX;
+  j = 0;
+  k = p = 1;
+  while (j + k < needle_len)
+    {
+      a = CANON_ELEMENT (needle[j + k]);
+      b = CANON_ELEMENT (needle[max_suffix_rev + k]);
+      if (b < a)
+	{
+	  /* Suffix is smaller, period is entire prefix so far.  */
+	  j += k;
+	  k = 1;
+	  p = j - max_suffix_rev;
+	}
+      else if (a == b)
+	{
+	  /* Advance through repetition of the current period.  */
+	  if (k != p)
+	    ++k;
+	  else
+	    {
+	      j += p;
+	      k = 1;
+	    }
+	}
+      else /* a < b */
+	{
+	  /* Suffix is larger, start over from current location.  */
+	  max_suffix_rev = j++;
+	  k = p = 1;
+	}
+    }
+
+  /* Choose the shorted suffix.  Return the first character of the right
+     half, rather than the last character of the left half.  */
+  if (max_suffix_rev + 1 < max_suffix + 1)
+    return max_suffix + 1;
+  *period = p;
+  return max_suffix_rev + 1;
+}
+
+/* Return the first location of non-empty NEEDLE within HAYSTACK, or
+   NULL.  HAYSTACK_LEN is the minimum known length of HAYSTACK.
+
+   If AVAILABLE does not modify HAYSTACK_LEN (as in memmem), then at
+   most 2 * HAYSTACK_LEN - NEEDLE_LEN comparisons occur in searching.
+   If AVAILABLE modifies HAYSTACK_LEN (as in strstr), then at most 3 *
+   HAYSTACK_LEN - NEEDLE_LEN comparisons occur in searching.  */
+static inline wchar_t *
+two_way_short_needle (const wchar_t *haystack, size_t haystack_len,
+		      const wchar_t *needle, size_t needle_len)
+{
+  size_t i; /* Index into current character of NEEDLE.  */
+  size_t j; /* Index into current window of HAYSTACK.  */
+  size_t period; /* The period of the right half of needle.  */
+  size_t suffix; /* The index of the right half of needle.  */
+
+  /* Factor the needle into two halves, such that the left half is
+     smaller than the global period, and the right half is
+     periodic (with a period as large as NEEDLE_LEN - suffix).  */
+  suffix = critical_factorization (needle, needle_len, &period);
+
+  /* Perform the search.  Each iteration compares the right half
+     first.  */
+  if (CMP_FUNC (needle, needle + period, suffix) == 0)
+    {
+      /* Entire needle is periodic; a mismatch can only advance by the
+	 period, so use memory to avoid rescanning known occurrences
+	 of the period.  */
+      size_t memory = 0;
+      j = 0;
+      while (AVAILABLE (haystack, haystack_len, j, needle_len))
+	{
+	  const wchar_t *pneedle;
+	  const wchar_t *phaystack;
+
+	  /* Scan for matches in right half.  */
+	  i = MAX (suffix, memory);
+	  pneedle = &needle[i];
+	  phaystack = &haystack[i + j];
+	  while (i < needle_len && (CANON_ELEMENT (*pneedle++)
+				    == CANON_ELEMENT (*phaystack++)))
+	    ++i;
+	  if (needle_len <= i)
+	    {
+	      /* Scan for matches in left half.  */
+	      i = suffix - 1;
+	      pneedle = &needle[i];
+	      phaystack = &haystack[i + j];
+	      while (memory < i + 1 && (CANON_ELEMENT (*pneedle--)
+					== CANON_ELEMENT (*phaystack--)))
+		--i;
+	      if (i + 1 < memory + 1)
+		return (wchar_t *) (haystack + j);
+	      /* No match, so remember how many repetitions of period
+		 on the right half were scanned.  */
+	      j += period;
+	      memory = needle_len - period;
+	    }
+	  else
+	    {
+	      j += i - suffix + 1;
+	      memory = 0;
+	    }
+	}
+    }
+  else
+    {
+      const wchar_t *phaystack;
+      /* The comparison always starts from needle[suffix], so cache it
+	 and use an optimized first-character loop.  */
+      wchar_t needle_suffix = CANON_ELEMENT (needle[suffix]);
+
+      /* The two halves of needle are distinct; no extra memory is
+	 required, and any mismatch results in a maximal shift.  */
+      period = MAX (suffix, needle_len - suffix) + 1;
+      j = 0;
+      while (AVAILABLE (haystack, haystack_len, j, needle_len))
+	{
+	  wchar_t haystack_char;
+	  const wchar_t *pneedle;
+
+	  phaystack = &haystack[suffix + j];
+
+	  while (needle_suffix
+	      != (haystack_char = CANON_ELEMENT (*phaystack++)))
+	    {
+	      ++j;
+	      if (!AVAILABLE (haystack, haystack_len, j, needle_len))
+		goto ret0;
+	    }
+
+	  /* Scan for matches in right half.  */
+	  i = suffix + 1;
+	  pneedle = &needle[i];
+	  while (i < needle_len)
+	    {
+	      if (CANON_ELEMENT (*pneedle++)
+		  != (haystack_char = CANON_ELEMENT (*phaystack++)))
+		break;
+	      ++i;
+	    }
+	  if (needle_len <= i)
+	    {
+	      /* Scan for matches in left half.  */
+	      i = suffix - 1;
+	      pneedle = &needle[i];
+	      phaystack = &haystack[i + j];
+	      while (i != SIZE_MAX)
+		{
+		  if (CANON_ELEMENT (*pneedle--)
+		      != (haystack_char = CANON_ELEMENT (*phaystack--)))
+		    break;
+		  --i;
+		}
+	      if (i == SIZE_MAX)
+		return (wchar_t *) (haystack + j);
+	      j += period;
+	    }
+	  else
+	    j += i - suffix + 1;
+	}
+    }
+ret0: __attribute__ ((unused))
+  return NULL;
+}
+
+#undef AVAILABLE
+#undef CANON_ELEMENT
+#undef CMP_FUNC
diff --git a/wcsmbs/wcsstr.c b/wcsmbs/wcsstr.c
index 78f1cc9ce0..f97ea70010 100644
--- a/wcsmbs/wcsstr.c
+++ b/wcsmbs/wcsstr.c
@@ -1,4 +1,5 @@
-/* Copyright (C) 1995-2024 Free Software Foundation, Inc.
+/* Locate a substring in a wide-character string.
+   Copyright (C) 1995-2024 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
@@ -15,82 +16,32 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
-/*
- * The original strstr() file contains the following comment:
- *
- * My personal strstr() implementation that beats most other algorithms.
- * Until someone tells me otherwise, I assume that this is the
- * fastest implementation of strstr() in C.
- * I deliberately chose not to comment it.  You should have at least
- * as much fun trying to understand it, as I had to write it :-).
- *
- * Stephen R. van den Berg, berg@pool.informatik.rwth-aachen.de */
-
 #include <wchar.h>
+#include <string.h>
+
+#define AVAILABLE(h, h_l, j, n_l)					\
+  (((j) + (n_l) <= (h_l))						\
+   || ((h_l) += __wcsnlen ((void*)((h) + (h_l)), (n_l) + 128),		\
+       (j) + (n_l) <= (h_l)))
+#include "wcs-two-way.h"
 
 wchar_t *
 wcsstr (const wchar_t *haystack, const wchar_t *needle)
 {
-  wchar_t b, c;
-
-  if ((b = *needle) != L'\0')
-    {
-      haystack--;				/* possible ANSI violation */
-      do
-	if ((c = *++haystack) == L'\0')
-	  goto ret0;
-      while (c != b);
-
-      if (!(c = *++needle))
-	goto foundneedle;
-      ++needle;
-      goto jin;
-
-      for (;;)
-	{
-	  wchar_t a;
-	  const wchar_t *rhaystack, *rneedle;
-
-	  do
-	    {
-	      if (!(a = *++haystack))
-		goto ret0;
-	      if (a == b)
-		break;
-	      if ((a = *++haystack) == L'\0')
-		goto ret0;
-shloop:	      ;
-	    }
-	  while (a != b);
-
-jin:	  if (!(a = *++haystack))
-	    goto ret0;
-
-	  if (a != c)
-	    goto shloop;
-
-	  if (*(rhaystack = haystack-- + 1) == (a = *(rneedle = needle)))
-	    do
-	      {
-		if (a == L'\0')
-		  goto foundneedle;
-		if (*++rhaystack != (a = *++needle))
-		  break;
-		if (a == L'\0')
-		  goto foundneedle;
-	      }
-	    while (*++rhaystack == (a = *++needle));
-
-	  needle = rneedle;		  /* took the register-poor approach */
-
-	  if (a == L'\0')
-	    break;
-	}
-    }
-foundneedle:
-  return (wchar_t*) haystack;
-ret0:
-  return NULL;
+  /* Ensure haystack length is at least as long as needle length.
+     Since a match may occur early on in a huge haystack, use strnlen
+     and read ahead a few cachelines for improved performance.  */
+  size_t ne_len = __wcslen (needle);
+  size_t hs_len = __wcsnlen (haystack, ne_len | 128);
+  if (hs_len < ne_len)
+    return NULL;
+
+  /* Check whether we have a match.  This improves performance since we
+     avoid initialization overheads.  */
+  if (__wmemcmp (haystack, needle, ne_len) == 0)
+    return (wchar_t *) haystack;
+
+  return two_way_short_needle (haystack, hs_len, needle, ne_len);
 }
 /* This alias is for backward compatibility with drafts of the ISO C
    standard.  Unfortunately the Unix(TM) standard requires this name.  */
-- 
2.34.1

Re: [PATCH v2 1/3] string: Remove c_strstr from test-strstr

[DJ Delorie]

Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
> There is no much point is checking the generic code if this is not
> really used by libc.

The point isn't to test the generic code, it's to validate the test.
Without such, you get a "pass" as long as the test and the code are both
broken in compatible ways.

Every test should have this kind of "baseline" check to make sure the
test is operating corectly, and so that any failures can be shown to be
failures in implementation, and not failures in the test itself.

Re: [PATCH v2 1/3] string: Remove c_strstr from test-strstr

[Adhemerval Zanella Netto]

On 27/02/24 01:42, DJ Delorie wrote:
> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
>> There is no much point is checking the generic code if this is not
>> really used by libc.
> 
> The point isn't to test the generic code, it's to validate the test.
> Without such, you get a "pass" as long as the test and the code are both
> broken in compatible ways.
> 
> Every test should have this kind of "baseline" check to make sure the
> test is operating corectly, and so that any failures can be shown to be
> failures in implementation, and not failures in the test itself.
> 

Fair enough, it would waste some time if the system uses the generic
implementation (since it testing twice) but I guess it make sense for
architecture that implement it with a different/optimized version.

Re: [PATCH v2 2/3] wcsmbs: Add test-wcsstr

[DJ Delorie]

Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
> Parametrize test-strstr.c so it can be used to check wcsstr.

LGTM although I suggest renaming one of the variables to more accurately
reflect its new role.  Also a question about error() formatting, but you
could commit this as-is if you want.

Reviewed-by: DJ Delorie <dj@redhat.com>

> diff --git a/string/test-strstr.c b/string/test-strstr.c
> index 05d0b7c98c..f82aeb2cfa 100644
> --- a/string/test-strstr.c
> +++ b/string/test-strstr.c
> @@ -17,16 +17,44 @@
>     <https://www.gnu.org/licenses/>.  */
>  
>  #define TEST_MAIN
> -#define TEST_NAME "strstr"
> -#include "test-string.h"
> +#ifndef WIDE
> +# define TEST_NAME "strstr"
> +#else
> +# define TEST_NAME "wcsstr"
> +#endif

Ok.

> +#ifndef WIDE
> +# define CHAR char
> +# define STRLEN strlen
> +# define STRSTR strstr
> +# define STRCPY strcpy
> +# define MEMCPY memcpy
> +# define MEMSET memset
> +# define MEMPCPY mempcpy
> +# define L(s) s
> +#else
> +# include <wchar.h>
> +# define CHAR wchar_t
> +# define STRLEN wcslen
> +# define STRCPY wcscpy
> +# define STRSTR wcsstr
> +# define MEMCPY wmemcpy
> +# define MEMSET wmemset
> +# define MEMPCPY wmempcpy
> +# define L(s) L ## s
> +/* The test requires up to 8191 charateres, so allocate at least 32Kb
> +   (considering 4kb page size).  */
> +# define BUF1PAGES 4
> +#endif
>  
> +#include "test-string.h"

Ok.

>  /* Naive implementation to verify results.  */
> -static char *
> -simple_strstr (const char *s1, const char *s2)
> +static CHAR *
> +simple_strstr (const CHAR *s1, const CHAR *s2)
>  {
> -  ssize_t s1len = strlen (s1);
> -  ssize_t s2len = strlen (s2);
> +  ssize_t s1len = STRLEN (s1);
> +  ssize_t s2len = STRLEN (s2);
>  
>    if (s2len > s1len)
>      return NULL;
> @@ -38,28 +66,27 @@ simple_strstr (const char *s1, const char *s2)
>  	if (s1[i + j] != s2[j])
>  	  break;
>        if (j == s2len)
> -	return (char *) s1 + i;
> +	return (CHAR *) s1 + i;
>      }
>  
>    return NULL;
>  }

Ok.


> -typedef char *(*proto_t) (const char *, const char *);
> +typedef CHAR *(*proto_t) (const CHAR *, const CHAR *);
>  
> -IMPL (strstr, 1)
> +IMPL (STRSTR, 1)

Ok.

>  static int
> -check_result (impl_t *impl, const char *s1, const char *s2,
> -	      char *exp_result)
> +check_result (impl_t *impl, const CHAR *s1, const CHAR *s2,
> +	      CHAR *exp_result)

Ok.

>  {
> -  char *result = CALL (impl, s1, s2);
> +  CHAR *result = CALL (impl, s1, s2);

Ok.

>    if (result != exp_result)
>      {
> -      error (0, 0, "Wrong result in function %s %s %s", impl->name,
> -	     (result == NULL) ? "(null)" : result,
> -	     (exp_result == NULL) ? "(null)" : exp_result);
> +      error (0, 0, "Wrong result in function %p %p %p", impl->name,
> +	     result, exp_result);

Ok... but do we not have a way to printf() wide character strings?

> @@ -68,7 +95,7 @@ check_result (impl_t *impl, const char *s1, const char *s2,
>  }
>  
>  static void
> -do_one_test (impl_t *impl, const char *s1, const char *s2, char *exp_result)
> +do_one_test (impl_t *impl, const CHAR *s1, const CHAR *s2, CHAR *exp_result)

Ok.

> @@ -79,49 +106,51 @@ static void
>  do_test (size_t align1, size_t align2, size_t len1, size_t len2,
>  	 int fail)
>  {
> -  char *s1 = (char *) (buf1 + align1);
> -  char *s2 = (char *) (buf2 + align2);
> +  align1 = align1 * sizeof (CHAR);
> +  align2 = align2 * sizeof (CHAR);
> -  char *ss2 = s2;
> +  CHAR *s1 = (CHAR *) (buf1 + align1);
> +  CHAR *s2 = (CHAR *) (buf2 + align2);

Ok.

> -  static const char d[] = "1234567890abcdef";
> -#define dl (sizeof (d) - 1)
> +  static const CHAR d[] = L("1234567890abcdef");
> +  const size_t dl = STRLEN (d);

Ok.  The choice of L() macro confused me at first (I read L"" instead of
L("")) but it works.

> +  CHAR *ss2 = s2;

Ok.

>    for (size_t l = len2; l > 0; l = l > dl ? l - dl : 0)
>      {
>        size_t t = l > dl ? dl : l;
> -      ss2 = mempcpy (ss2, d, t);
> +      ss2 = MEMPCPY (ss2, d, t);

Ok.

>    s2[len2] = '\0';
>  
>    if (fail)
>      {
> -      char *ss1 = s1;
> +      CHAR *ss1 = s1;

Ok.

>        for (size_t l = len1; l > 0; l = l > dl ? l - dl : 0)
>  	{
>  	  size_t t = l > dl ? dl : l;
> -	  memcpy (ss1, d, t);
> +	  MEMCPY (ss1, d, t);

Ok.

> -      memset (s1, '0', len1);
> -      memcpy (s1 + len1 - len2, s2, len2);
> +      MEMSET (s1, '0', len1);
> +      MEMCPY (s1 + len1 - len2, s2, len2);

Ok.

>  static void
>  check1 (void)
>  {
> -  const char s1[] =
> -    "F_BD_CE_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD_C3_88_20_EF_BF_BD_EF_BF_BD_EF_BF_BD_C3_A7_20_EF_BF_BD";
> -  const char s2[] = "_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD";
> -  char *exp_result;
> +  const CHAR s1[] =
> +    L("F_BD_CE_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD_C3_88_20_EF_BF_BD_EF_BF_BD_EF_BF_BD_C3_A7_20_EF_BF_BD");
> +  const CHAR s2[] = L("_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD");
> +  CHAR *exp_result;

Ok.

>  static void
>  check2 (void)
>  {
> -  const char s1_stack[] = ", enable_static, \0, enable_shared, ";
> +  const CHAR s1_stack[] = L(", enable_static, \0, enable_shared, ");

Ok.

>    const size_t s1_byte_count = 18;

I think this needs to be renamed to s1_char_count or something, since
with wide characters, S1 is no longer 18 bytes long.

> -  const char *s2_stack = &(s1_stack[s1_byte_count]);
> -  const size_t s2_byte_count = 18;
> -  char *exp_result;
> +  const size_t s1_byte_len = 18 * sizeof (CHAR);
> +  const CHAR *s2_stack = &(s1_stack[s1_byte_count]);
> +  const size_t s2_byte_len = 18 * sizeof (CHAR);;
> +  CHAR *exp_result;

Ok.

>    /* Haystack at end of page.  The following page is protected.  */
> -  char *s1_page_end = (void *) buf1 + page_size - s1_byte_count;
> -  strcpy (s1_page_end, s1_stack);
> +  CHAR *s1_page_end = (void *) buf1 + page_size - s1_byte_len;
> +  STRCPY (s1_page_end, s1_stack);

Ok.

>    /* Haystack which crosses a page boundary.
>       Note: page_size is at least 2 * getpagesize.  See test_init.  */
> -  char *s1_page_cross = (void *) buf1 + page_size_real - 8;
> -  strcpy (s1_page_cross, s1_stack);
> +  CHAR *s1_page_cross = (void *) buf1 + page_size_real - 8;
> +  STRCPY (s1_page_cross, s1_stack);

Ok.

>    /* Needle at end of page.  The following page is protected.  */
> -  char *s2_page_end = (void *) buf2 + page_size - s2_byte_count;
> -  strcpy (s2_page_end, s2_stack);
> +  CHAR *s2_page_end = (void *) buf2 + page_size - s2_byte_len;
> +  STRCPY (s2_page_end, s2_stack);

Ok.

>    /* Needle which crosses a page boundary.
>       Note: page_size is at least 2 * getpagesize.  See test_init.  */
> -  char *s2_page_cross = (void *) buf2 + page_size_real - 8;
> -  strcpy (s2_page_cross, s2_stack);
> +  CHAR *s2_page_cross = (void *) buf2 + page_size_real - 8;
> +  STRCPY (s2_page_cross, s2_stack);

Ok.

>  static void
>  pr23637 (void)
>  {
> -  char *h = (char*) buf1;
> -  char *n = (char*) buf2;
> +  CHAR *h = (CHAR*) buf1;
> +  CHAR *n = (CHAR*) buf2;

Ok.

> -  char *exp_result = simple_strstr (h, n);
> +  CHAR *exp_result = simple_strstr (h, n);

Ok.

> diff --git a/wcsmbs/Makefile b/wcsmbs/Makefile
> +  test-wcsstr \

Ok.

> diff --git a/wcsmbs/test-wcsstr.c b/wcsmbs/test-wcsstr.c
> +/* Test wcsstr function.
> +   Copyright (C) 2024 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/>.  */
> +
> +#define WIDE 1
> +#include <string/test-strstr.c>

Ok.

Re: [PATCH v2 3/3] wcsmbs: Ensure wcstr worst-case linear execution time (BZ 23865)

[DJ Delorie]

Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
> It uses the same two-way algorithm used on strstr, strcasestr, and
> memmem.  Different than strstr, neither the "shift table" optimization
> nor the self-adapting filtering check is used because it would result in
> a too-large shift table (and it also simplifies the implementation bit).

Question about the repeat loops and untimed tests.
Question about copyright years.
One typo.

> +static void
> +check3 (void)
> +{
> +  /* Check that a long periodic needle does not cause false positives.  */
> +  {
> +    const CHAR input[] = L("F_BD_CE_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD"
> +                            "_C3_88_20_EF_BF_BD_EF_BF_BD_EF_BF_BD"
> +                            "_C3_A7_20_EF_BF_BD");
> +    const CHAR need[] = L("_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD");
> +    FOR_EACH_IMPL (impl, 0)
> +      check_result (impl, input, need, NULL);
> +  }

Ok.

> +  {
> +    const CHAR input[] = L("F_BD_CE_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD"
> +			   "_C3_88_20_EF_BF_BD_EF_BF_BD_EF_BF_BD"
> +			   "_C3_A7_20_EF_BF_BD_DA_B5_C2_A6_20"
> +			   "_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD");
> +    const CHAR need[] = L("_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD");
> +    FOR_EACH_IMPL (impl, 0)
> +      check_result (impl, input, need, (CHAR *) input + 115);
> +  }
> +}

Ok.

> +static void
> +pr23865 (void)
> +{
> +  /* Check that a very long haystack is handled quickly if the needle is
> +     short and occurs near the beginning.  */
> +  {
> +    size_t repeat = 10000;
> +    size_t m = 1000000;
> +    const CHAR *needle =
> +      L("AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
> +        "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA");
> +    CHAR *haystack = xmalloc ((m + 1) * sizeof (CHAR));
> +    MEMSET (haystack, L('A'), m);
> +    haystack[0] = L('B');
> +    haystack[m] = L('\0');
> +
> +    for (; repeat > 0; repeat--)
> +      {
> +	FOR_EACH_IMPL (impl, 0)
> +	  check_result (impl, haystack, needle, haystack + 1);
> +      }

What is the purpose of the repeat loop?  This function is only called
once, and not timed...

The test itself is OK.

> +    free (haystack);
> +  }
> +
> +  /* Check that a very long needle is discarded quickly if the haystack is
> +     short.  */
> +  {
> +    size_t repeat = 10000;
> +    size_t m = 1000000;
> +    const CHAR *haystack =
> +      L("AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
> +	"ABABABABABABABABABABABABABABABABABABABABABABABABABABABABABABABABABAB");
> +    CHAR *needle = xmalloc ((m + 1) * sizeof (CHAR));
> +    MEMSET (needle, L'A', m);
> +    needle[m] = L('\0');
> +
> +    for (; repeat > 0; repeat--)
> +      {
> +	FOR_EACH_IMPL (impl, 0)
> +	  check_result (impl, haystack, needle, NULL);
> +      }
> +
> +    free (needle);
> +  }

Test OK but question the repeat look.

> +  /* Check that the asymptotic worst-case complexity is not quadratic.  */
> +  {
> +    size_t m = 1000000;
> +    CHAR *haystack = xmalloc ((2 * m + 2) * sizeof (CHAR));
> +    CHAR *needle = xmalloc ((m + 2) * sizeof (CHAR));
> +
> +    MEMSET (haystack, L('A'), 2 * m);
> +    haystack[2 * m] = L('B');
> +    haystack[2 * m + 1] = L('\0');
> +
> +    MEMSET (needle, L('A'), m);
> +    needle[m] = L('B');
> +    needle[m + 1] = L('\0');
> +
> +    FOR_EACH_IMPL (impl, 0)
> +      check_result (impl, haystack, needle, haystack + m);
> +
> +    free (needle);
> +    free (haystack);
> +  }

Without timing it, how do we test that it is not quadratic?

> +  {
> +    /* Ensure that with a barely periodic "short" needle, STRSTR's
> +       search does not mistakenly skip just past the match point.  */
> +    const CHAR *haystack =
> +      L("\n"
> +       "with_build_libsubdir\n"
> +       "with_local_prefix\n"
> +       "with_gxx_include_dir\n"
> +       "with_cpp_install_dir\n"
> +       "enable_generated_files_in_srcdir\n"
> +       "with_gnu_ld\n"
> +       "with_ld\n"
> +       "with_demangler_in_ld\n"
> +       "with_gnu_as\n"
> +       "with_as\n"
> +       "enable_largefile\n"
> +       "enable_werror_always\n"
> +       "enable_checking\n"
> +       "enable_coverage\n"
> +       "enable_gather_detailed_mem_stats\n"
> +       "enable_build_with_cxx\n"
> +       "with_stabs\n"
> +       "enable_multilib\n"
> +       "enable___cxa_atexit\n"
> +       "enable_decimal_float\n"
> +       "enable_fixed_point\n"
> +       "enable_threads\n"
> +       "enable_tls\n"
> +       "enable_objc_gc\n"
> +       "with_dwarf2\n"
> +       "enable_shared\n"
> +       "with_build_sysroot\n"
> +       "with_sysroot\n"
> +       "with_specs\n"
> +       "with_pkgversion\n"
> +       "with_bugurl\n"
> +       "enable_languages\n"
> +       "with_multilib_list\n");
> +    const CHAR *needle =
> +      L("\n"
> +       "with_gnu_ld\n");
> +
> +    FOR_EACH_IMPL (impl, 0)
> +      check_result (impl, haystack, needle, (CHAR*) haystack + 114);
> +  }

Ok.

> +  {
> +    /* Same bug, shorter trigger.  */
> +    const CHAR *haystack = L("..wi.d.");
> +    const CHAR *needle = L(".d.");
> +    FOR_EACH_IMPL (impl, 0)
> +      check_result (impl, haystack, needle, (CHAR*) haystack + 4);
> +  }

Ok.

> +  /* Test case from Yves Bastide.
> +     <https://www.openwall.com/lists/musl/2014/04/18/2>  */
> +  {
> +    const CHAR *input = L("playing play play play always");
> +    const CHAR *needle = L("play play play");
> +    FOR_EACH_IMPL (impl, 0)
> +      check_result (impl, input, needle, (CHAR*) input + 8);
> +  }

Ok.

> +  /* Test long needles.  */
> +  {
> +    size_t m = 1024;
> +    CHAR *haystack = xmalloc ((2 * m + 1) * sizeof (CHAR));
> +    CHAR *needle = xmalloc ((m + 1) * sizeof (CHAR));
> +    haystack[0] = L('x');
> +    MEMSET (haystack + 1, L(' '), m - 1);
> +    MEMSET (haystack + m, L('x'), m);
> +    haystack[2 * m] = L('\0');
> +    MEMSET (needle, L('x'), m);
> +    needle[m] = L('\0');
> +
> +    FOR_EACH_IMPL (impl, 0)
> +      check_result (impl, haystack, needle, haystack + m);
> +
> +    free (needle);
> +    free (haystack);
> +  }
> +}

Ok.

> @@ -236,7 +411,9 @@ test_main (void)
>  
>    check1 ();
>    check2 ();
> +  check3 ();
>    pr23637 ();
> +  pr23865 ();
>  
>    printf ("%23s", "");
>    FOR_EACH_IMPL (impl, 0)

Ok.

> diff --git a/wcsmbs/wcs-two-way.h b/wcsmbs/wcs-two-way.h
> new file mode 100644
> index 0000000000..2dcee7fc1a
> --- /dev/null
> +++ b/wcsmbs/wcs-two-way.h
> @@ -0,0 +1,312 @@
> +/* Byte-wise substring search, using the Two-Way algorithm.
> +   Copyright (C) 2024 Free Software Foundation, Inc.

This appears to be an edited copy of string/str-two-way.h

If so, shouldn't the copyright years include the span from
string/str-two-way.h?  Also I noted that the author attribution has been
removed, but I assume that's glibc policy.

> +  /* Choose the shorted suffix.  Return the first character of the right

s/shorted/shorter/

> diff --git a/wcsmbs/wcsstr.c b/wcsmbs/wcsstr.c
> index 78f1cc9ce0..f97ea70010 100644
> --- a/wcsmbs/wcsstr.c
> +++ b/wcsmbs/wcsstr.c
> @@ -1,4 +1,5 @@
> -/* Copyright (C) 1995-2024 Free Software Foundation, Inc.
> +/* Locate a substring in a wide-character string.
> +   Copyright (C) 1995-2024 Free Software Foundation, Inc.

Ok.

>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> -/*
> - * The original strstr() file contains the following comment:
> - *
> - * My personal strstr() implementation that beats most other algorithms.
> - * Until someone tells me otherwise, I assume that this is the
> - * fastest implementation of strstr() in C.
> - * I deliberately chose not to comment it.  You should have at least
> - * as much fun trying to understand it, as I had to write it :-).
> - *
> - * Stephen R. van den Berg, berg@pool.informatik.rwth-aachen.de */
> -

Heh.  Ok.

>  #include <wchar.h>
> +#include <string.h>
> +
> +#define AVAILABLE(h, h_l, j, n_l)					\
> +  (((j) + (n_l) <= (h_l))						\
> +   || ((h_l) += __wcsnlen ((void*)((h) + (h_l)), (n_l) + 128),		\
> +       (j) + (n_l) <= (h_l)))
> +#include "wcs-two-way.h"

Ok.

>  wchar_t *
>  wcsstr (const wchar_t *haystack, const wchar_t *needle)
>  {
> -  wchar_t b, c;
> -
> . . .
> -  return (wchar_t*) haystack;
> -ret0:
> -  return NULL;
> +  /* Ensure haystack length is at least as long as needle length.
> +     Since a match may occur early on in a huge haystack, use strnlen
> +     and read ahead a few cachelines for improved performance.  */
> +  size_t ne_len = __wcslen (needle);
> +  size_t hs_len = __wcsnlen (haystack, ne_len | 128);
> +  if (hs_len < ne_len)
> +    return NULL;
> +
> +  /* Check whether we have a match.  This improves performance since we
> +     avoid initialization overheads.  */
> +  if (__wmemcmp (haystack, needle, ne_len) == 0)
> +    return (wchar_t *) haystack;
> +
> +  return two_way_short_needle (haystack, hs_len, needle, ne_len);
>  }

Ok.

Re: [PATCH v2 2/3] wcsmbs: Add test-wcsstr

[Adhemerval Zanella Netto]

On 28/02/24 21:45, DJ Delorie wrote:
> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
>> Parametrize test-strstr.c so it can be used to check wcsstr.
> 
> LGTM although I suggest renaming one of the variables to more accurately
> reflect its new role.  Also a question about error() formatting, but you
> could commit this as-is if you want.
> 
> Reviewed-by: DJ Delorie <dj@redhat.com>
> 
>> diff --git a/string/test-strstr.c b/string/test-strstr.c
>> index 05d0b7c98c..f82aeb2cfa 100644
>> --- a/string/test-strstr.c
>> +++ b/string/test-strstr.c
>> @@ -17,16 +17,44 @@
>>     <https://www.gnu.org/licenses/>.  */
>>  
>>  #define TEST_MAIN
>> -#define TEST_NAME "strstr"
>> -#include "test-string.h"
>> +#ifndef WIDE
>> +# define TEST_NAME "strstr"
>> +#else
>> +# define TEST_NAME "wcsstr"
>> +#endif
> 
> Ok.
> 
>> +#ifndef WIDE
>> +# define CHAR char
>> +# define STRLEN strlen
>> +# define STRSTR strstr
>> +# define STRCPY strcpy
>> +# define MEMCPY memcpy
>> +# define MEMSET memset
>> +# define MEMPCPY mempcpy
>> +# define L(s) s
>> +#else
>> +# include <wchar.h>
>> +# define CHAR wchar_t
>> +# define STRLEN wcslen
>> +# define STRCPY wcscpy
>> +# define STRSTR wcsstr
>> +# define MEMCPY wmemcpy
>> +# define MEMSET wmemset
>> +# define MEMPCPY wmempcpy
>> +# define L(s) L ## s
>> +/* The test requires up to 8191 charateres, so allocate at least 32Kb
>> +   (considering 4kb page size).  */
>> +# define BUF1PAGES 4
>> +#endif
>>  
>> +#include "test-string.h"
> 
> Ok.
> 
>>  /* Naive implementation to verify results.  */
>> -static char *
>> -simple_strstr (const char *s1, const char *s2)
>> +static CHAR *
>> +simple_strstr (const CHAR *s1, const CHAR *s2)
>>  {
>> -  ssize_t s1len = strlen (s1);
>> -  ssize_t s2len = strlen (s2);
>> +  ssize_t s1len = STRLEN (s1);
>> +  ssize_t s2len = STRLEN (s2);
>>  
>>    if (s2len > s1len)
>>      return NULL;
>> @@ -38,28 +66,27 @@ simple_strstr (const char *s1, const char *s2)
>>  	if (s1[i + j] != s2[j])
>>  	  break;
>>        if (j == s2len)
>> -	return (char *) s1 + i;
>> +	return (CHAR *) s1 + i;
>>      }
>>  
>>    return NULL;
>>  }
> 
> Ok.
> 
> 
>> -typedef char *(*proto_t) (const char *, const char *);
>> +typedef CHAR *(*proto_t) (const CHAR *, const CHAR *);
>>  
>> -IMPL (strstr, 1)
>> +IMPL (STRSTR, 1)
> 
> Ok.
> 
>>  static int
>> -check_result (impl_t *impl, const char *s1, const char *s2,
>> -	      char *exp_result)
>> +check_result (impl_t *impl, const CHAR *s1, const CHAR *s2,
>> +	      CHAR *exp_result)
> 
> Ok.
> 
>>  {
>> -  char *result = CALL (impl, s1, s2);
>> +  CHAR *result = CALL (impl, s1, s2);
> 
> Ok.
> 
>>    if (result != exp_result)
>>      {
>> -      error (0, 0, "Wrong result in function %s %s %s", impl->name,
>> -	     (result == NULL) ? "(null)" : result,
>> -	     (exp_result == NULL) ? "(null)" : exp_result);
>> +      error (0, 0, "Wrong result in function %p %p %p", impl->name,
>> +	     result, exp_result);
> 
> Ok... but do we not have a way to printf() wide character strings?

Indeed, I think we can use "%Ls" here. I think I have used %p just to avoid
adding another parametrized macro.  I will fix it.

> 
>> @@ -68,7 +95,7 @@ check_result (impl_t *impl, const char *s1, const char *s2,
>>  }
>>  
>>  static void
>> -do_one_test (impl_t *impl, const char *s1, const char *s2, char *exp_result)
>> +do_one_test (impl_t *impl, const CHAR *s1, const CHAR *s2, CHAR *exp_result)
> 
> Ok.
> 
>> @@ -79,49 +106,51 @@ static void
>>  do_test (size_t align1, size_t align2, size_t len1, size_t len2,
>>  	 int fail)
>>  {
>> -  char *s1 = (char *) (buf1 + align1);
>> -  char *s2 = (char *) (buf2 + align2);
>> +  align1 = align1 * sizeof (CHAR);
>> +  align2 = align2 * sizeof (CHAR);
>> -  char *ss2 = s2;
>> +  CHAR *s1 = (CHAR *) (buf1 + align1);
>> +  CHAR *s2 = (CHAR *) (buf2 + align2);
> 
> Ok.
> 
>> -  static const char d[] = "1234567890abcdef";
>> -#define dl (sizeof (d) - 1)
>> +  static const CHAR d[] = L("1234567890abcdef");
>> +  const size_t dl = STRLEN (d);
> 
> Ok.  The choice of L() macro confused me at first (I read L"" instead of
> L("")) but it works.
> 
>> +  CHAR *ss2 = s2;
> 
> Ok.
> 
>>    for (size_t l = len2; l > 0; l = l > dl ? l - dl : 0)
>>      {
>>        size_t t = l > dl ? dl : l;
>> -      ss2 = mempcpy (ss2, d, t);
>> +      ss2 = MEMPCPY (ss2, d, t);
> 
> Ok.
> 
>>    s2[len2] = '\0';
>>  
>>    if (fail)
>>      {
>> -      char *ss1 = s1;
>> +      CHAR *ss1 = s1;
> 
> Ok.
> 
>>        for (size_t l = len1; l > 0; l = l > dl ? l - dl : 0)
>>  	{
>>  	  size_t t = l > dl ? dl : l;
>> -	  memcpy (ss1, d, t);
>> +	  MEMCPY (ss1, d, t);
> 
> Ok.
> 
>> -      memset (s1, '0', len1);
>> -      memcpy (s1 + len1 - len2, s2, len2);
>> +      MEMSET (s1, '0', len1);
>> +      MEMCPY (s1 + len1 - len2, s2, len2);
> 
> Ok.
> 
>>  static void
>>  check1 (void)
>>  {
>> -  const char s1[] =
>> -    "F_BD_CE_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD_C3_88_20_EF_BF_BD_EF_BF_BD_EF_BF_BD_C3_A7_20_EF_BF_BD";
>> -  const char s2[] = "_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD";
>> -  char *exp_result;
>> +  const CHAR s1[] =
>> +    L("F_BD_CE_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD_C3_88_20_EF_BF_BD_EF_BF_BD_EF_BF_BD_C3_A7_20_EF_BF_BD");
>> +  const CHAR s2[] = L("_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD");
>> +  CHAR *exp_result;
> 
> Ok.
> 
>>  static void
>>  check2 (void)
>>  {
>> -  const char s1_stack[] = ", enable_static, \0, enable_shared, ";
>> +  const CHAR s1_stack[] = L(", enable_static, \0, enable_shared, ");
> 
> Ok.
> 
>>    const size_t s1_byte_count = 18;
> 
> I think this needs to be renamed to s1_char_count or something, since
> with wide characters, S1 is no longer 18 bytes long.

Ack.

> 
>> -  const char *s2_stack = &(s1_stack[s1_byte_count]);
>> -  const size_t s2_byte_count = 18;
>> -  char *exp_result;
>> +  const size_t s1_byte_len = 18 * sizeof (CHAR);
>> +  const CHAR *s2_stack = &(s1_stack[s1_byte_count]);
>> +  const size_t s2_byte_len = 18 * sizeof (CHAR);;
>> +  CHAR *exp_result;
> 
> Ok.
> 
>>    /* Haystack at end of page.  The following page is protected.  */
>> -  char *s1_page_end = (void *) buf1 + page_size - s1_byte_count;
>> -  strcpy (s1_page_end, s1_stack);
>> +  CHAR *s1_page_end = (void *) buf1 + page_size - s1_byte_len;
>> +  STRCPY (s1_page_end, s1_stack);
> 
> Ok.
> 
>>    /* Haystack which crosses a page boundary.
>>       Note: page_size is at least 2 * getpagesize.  See test_init.  */
>> -  char *s1_page_cross = (void *) buf1 + page_size_real - 8;
>> -  strcpy (s1_page_cross, s1_stack);
>> +  CHAR *s1_page_cross = (void *) buf1 + page_size_real - 8;
>> +  STRCPY (s1_page_cross, s1_stack);
> 
> Ok.
> 
>>    /* Needle at end of page.  The following page is protected.  */
>> -  char *s2_page_end = (void *) buf2 + page_size - s2_byte_count;
>> -  strcpy (s2_page_end, s2_stack);
>> +  CHAR *s2_page_end = (void *) buf2 + page_size - s2_byte_len;
>> +  STRCPY (s2_page_end, s2_stack);
> 
> Ok.
> 
>>    /* Needle which crosses a page boundary.
>>       Note: page_size is at least 2 * getpagesize.  See test_init.  */
>> -  char *s2_page_cross = (void *) buf2 + page_size_real - 8;
>> -  strcpy (s2_page_cross, s2_stack);
>> +  CHAR *s2_page_cross = (void *) buf2 + page_size_real - 8;
>> +  STRCPY (s2_page_cross, s2_stack);
> 
> Ok.
> 
>>  static void
>>  pr23637 (void)
>>  {
>> -  char *h = (char*) buf1;
>> -  char *n = (char*) buf2;
>> +  CHAR *h = (CHAR*) buf1;
>> +  CHAR *n = (CHAR*) buf2;
> 
> Ok.
> 
>> -  char *exp_result = simple_strstr (h, n);
>> +  CHAR *exp_result = simple_strstr (h, n);
> 
> Ok.
> 
>> diff --git a/wcsmbs/Makefile b/wcsmbs/Makefile
>> +  test-wcsstr \
> 
> Ok.
> 
>> diff --git a/wcsmbs/test-wcsstr.c b/wcsmbs/test-wcsstr.c
>> +/* Test wcsstr function.
>> +   Copyright (C) 2024 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/>.  */
>> +
>> +#define WIDE 1
>> +#include <string/test-strstr.c>
> 
> Ok.
> 

Re: [PATCH v2 3/3] wcsmbs: Ensure wcstr worst-case linear execution time (BZ 23865)

[Adhemerval Zanella Netto]

On 29/02/24 02:19, DJ Delorie wrote:
> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
>> It uses the same two-way algorithm used on strstr, strcasestr, and
>> memmem.  Different than strstr, neither the "shift table" optimization
>> nor the self-adapting filtering check is used because it would result in
>> a too-large shift table (and it also simplifies the implementation bit).
> 
> Question about the repeat loops and untimed tests.
> Question about copyright years.
> One typo.
> 
>> +static void
>> +check3 (void)
>> +{
>> +  /* Check that a long periodic needle does not cause false positives.  */
>> +  {
>> +    const CHAR input[] = L("F_BD_CE_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD"
>> +                            "_C3_88_20_EF_BF_BD_EF_BF_BD_EF_BF_BD"
>> +                            "_C3_A7_20_EF_BF_BD");
>> +    const CHAR need[] = L("_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD");
>> +    FOR_EACH_IMPL (impl, 0)
>> +      check_result (impl, input, need, NULL);
>> +  }
> 
> Ok.
> 
>> +  {
>> +    const CHAR input[] = L("F_BD_CE_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD"
>> +			   "_C3_88_20_EF_BF_BD_EF_BF_BD_EF_BF_BD"
>> +			   "_C3_A7_20_EF_BF_BD_DA_B5_C2_A6_20"
>> +			   "_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD");
>> +    const CHAR need[] = L("_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD_EF_BF_BD");
>> +    FOR_EACH_IMPL (impl, 0)
>> +      check_result (impl, input, need, (CHAR *) input + 115);
>> +  }
>> +}
> 
> Ok.
> 
>> +static void
>> +pr23865 (void)
>> +{
>> +  /* Check that a very long haystack is handled quickly if the needle is
>> +     short and occurs near the beginning.  */
>> +  {
>> +    size_t repeat = 10000;
>> +    size_t m = 1000000;
>> +    const CHAR *needle =
>> +      L("AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
>> +        "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA");
>> +    CHAR *haystack = xmalloc ((m + 1) * sizeof (CHAR));
>> +    MEMSET (haystack, L('A'), m);
>> +    haystack[0] = L('B');
>> +    haystack[m] = L('\0');
>> +
>> +    for (; repeat > 0; repeat--)
>> +      {
>> +	FOR_EACH_IMPL (impl, 0)
>> +	  check_result (impl, haystack, needle, haystack + 1);
>> +      }
> 
> What is the purpose of the repeat loop?  This function is only called
> once, and not timed...
> 
> The test itself is OK.

I copied this tests from gnulib (9411c5e467cf60f6295b9fed306029f341a0f24f)
and my understanding is to enforce that wcsstr won't take too much time
by triggering a timeout.  These kind of pattern are slower for newer 
implementation compared to the current one.

The repeat number is indeed arbitrary and depending of the hardware should 
not make any different (on my box I only see some different with way larger
inputs).

> 
>> +    free (haystack);
>> +  }
>> +
>> +  /* Check that a very long needle is discarded quickly if the haystack is
>> +     short.  */
>> +  {
>> +    size_t repeat = 10000;
>> +    size_t m = 1000000;
>> +    const CHAR *haystack =
>> +      L("AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
>> +	"ABABABABABABABABABABABABABABABABABABABABABABABABABABABABABABABABABAB");
>> +    CHAR *needle = xmalloc ((m + 1) * sizeof (CHAR));
>> +    MEMSET (needle, L'A', m);
>> +    needle[m] = L('\0');
>> +
>> +    for (; repeat > 0; repeat--)
>> +      {
>> +	FOR_EACH_IMPL (impl, 0)
>> +	  check_result (impl, haystack, needle, NULL);
>> +      }
>> +
>> +    free (needle);
>> +  }
> 
> Test OK but question the repeat look.
> 
>> +  /* Check that the asymptotic worst-case complexity is not quadratic.  */
>> +  {
>> +    size_t m = 1000000;
>> +    CHAR *haystack = xmalloc ((2 * m + 2) * sizeof (CHAR));
>> +    CHAR *needle = xmalloc ((m + 2) * sizeof (CHAR));
>> +
>> +    MEMSET (haystack, L('A'), 2 * m);
>> +    haystack[2 * m] = L('B');
>> +    haystack[2 * m + 1] = L('\0');
>> +
>> +    MEMSET (needle, L('A'), m);
>> +    needle[m] = L('B');
>> +    needle[m + 1] = L('\0');
>> +
>> +    FOR_EACH_IMPL (impl, 0)
>> +      check_result (impl, haystack, needle, haystack + m);
>> +
>> +    free (needle);
>> +    free (haystack);
>> +  }
> 
> Without timing it, how do we test that it is not quadratic?

Because on current implementation it takes forever (about 4 minutes
on my box) due the input sizes.

> 
>> +  {
>> +    /* Ensure that with a barely periodic "short" needle, STRSTR's
>> +       search does not mistakenly skip just past the match point.  */
>> +    const CHAR *haystack =
>> +      L("\n"
>> +       "with_build_libsubdir\n"
>> +       "with_local_prefix\n"
>> +       "with_gxx_include_dir\n"
>> +       "with_cpp_install_dir\n"
>> +       "enable_generated_files_in_srcdir\n"
>> +       "with_gnu_ld\n"
>> +       "with_ld\n"
>> +       "with_demangler_in_ld\n"
>> +       "with_gnu_as\n"
>> +       "with_as\n"
>> +       "enable_largefile\n"
>> +       "enable_werror_always\n"
>> +       "enable_checking\n"
>> +       "enable_coverage\n"
>> +       "enable_gather_detailed_mem_stats\n"
>> +       "enable_build_with_cxx\n"
>> +       "with_stabs\n"
>> +       "enable_multilib\n"
>> +       "enable___cxa_atexit\n"
>> +       "enable_decimal_float\n"
>> +       "enable_fixed_point\n"
>> +       "enable_threads\n"
>> +       "enable_tls\n"
>> +       "enable_objc_gc\n"
>> +       "with_dwarf2\n"
>> +       "enable_shared\n"
>> +       "with_build_sysroot\n"
>> +       "with_sysroot\n"
>> +       "with_specs\n"
>> +       "with_pkgversion\n"
>> +       "with_bugurl\n"
>> +       "enable_languages\n"
>> +       "with_multilib_list\n");
>> +    const CHAR *needle =
>> +      L("\n"
>> +       "with_gnu_ld\n");
>> +
>> +    FOR_EACH_IMPL (impl, 0)
>> +      check_result (impl, haystack, needle, (CHAR*) haystack + 114);
>> +  }
> 
> Ok.
> 
>> +  {
>> +    /* Same bug, shorter trigger.  */
>> +    const CHAR *haystack = L("..wi.d.");
>> +    const CHAR *needle = L(".d.");
>> +    FOR_EACH_IMPL (impl, 0)
>> +      check_result (impl, haystack, needle, (CHAR*) haystack + 4);
>> +  }
> 
> Ok.
> 
>> +  /* Test case from Yves Bastide.
>> +     <https://www.openwall.com/lists/musl/2014/04/18/2>  */
>> +  {
>> +    const CHAR *input = L("playing play play play always");
>> +    const CHAR *needle = L("play play play");
>> +    FOR_EACH_IMPL (impl, 0)
>> +      check_result (impl, input, needle, (CHAR*) input + 8);
>> +  }
> 
> Ok.
> 
>> +  /* Test long needles.  */
>> +  {
>> +    size_t m = 1024;
>> +    CHAR *haystack = xmalloc ((2 * m + 1) * sizeof (CHAR));
>> +    CHAR *needle = xmalloc ((m + 1) * sizeof (CHAR));
>> +    haystack[0] = L('x');
>> +    MEMSET (haystack + 1, L(' '), m - 1);
>> +    MEMSET (haystack + m, L('x'), m);
>> +    haystack[2 * m] = L('\0');
>> +    MEMSET (needle, L('x'), m);
>> +    needle[m] = L('\0');
>> +
>> +    FOR_EACH_IMPL (impl, 0)
>> +      check_result (impl, haystack, needle, haystack + m);
>> +
>> +    free (needle);
>> +    free (haystack);
>> +  }
>> +}
> 
> Ok.
> 
>> @@ -236,7 +411,9 @@ test_main (void)
>>  
>>    check1 ();
>>    check2 ();
>> +  check3 ();
>>    pr23637 ();
>> +  pr23865 ();
>>  
>>    printf ("%23s", "");
>>    FOR_EACH_IMPL (impl, 0)
> 
> Ok.
> 
>> diff --git a/wcsmbs/wcs-two-way.h b/wcsmbs/wcs-two-way.h
>> new file mode 100644
>> index 0000000000..2dcee7fc1a
>> --- /dev/null
>> +++ b/wcsmbs/wcs-two-way.h
>> @@ -0,0 +1,312 @@
>> +/* Byte-wise substring search, using the Two-Way algorithm.
>> +   Copyright (C) 2024 Free Software Foundation, Inc.
> 
> This appears to be an edited copy of string/str-two-way.h
> 
> If so, shouldn't the copyright years include the span from
> string/str-two-way.h?  Also I noted that the author attribution has been
> removed, but I assume that's glibc policy.

Yes, I decided to added a new file instead of parametrize str-two-way.h
because wcsstr only uses two_way_short_needle and to make
two_way_long_needle work correctly it would need a quite large shift_table. 
And I remove the author as current glibc policy indeed.

I will update the Copyright years.

> 
>> +  /* Choose the shorted suffix.  Return the first character of the right
> 
> s/shorted/shorter/
> 

Ack.

>> diff --git a/wcsmbs/wcsstr.c b/wcsmbs/wcsstr.c
>> index 78f1cc9ce0..f97ea70010 100644
>> --- a/wcsmbs/wcsstr.c
>> +++ b/wcsmbs/wcsstr.c
>> @@ -1,4 +1,5 @@
>> -/* Copyright (C) 1995-2024 Free Software Foundation, Inc.
>> +/* Locate a substring in a wide-character string.
>> +   Copyright (C) 1995-2024 Free Software Foundation, Inc.
> 
> Ok.
> 
>>     License along with the GNU C Library; if not, see
>>     <https://www.gnu.org/licenses/>.  */
>>  
>> -/*
>> - * The original strstr() file contains the following comment:
>> - *
>> - * My personal strstr() implementation that beats most other algorithms.
>> - * Until someone tells me otherwise, I assume that this is the
>> - * fastest implementation of strstr() in C.
>> - * I deliberately chose not to comment it.  You should have at least
>> - * as much fun trying to understand it, as I had to write it :-).
>> - *
>> - * Stephen R. van den Berg, berg@pool.informatik.rwth-aachen.de */
>> -
> 
> Heh.  Ok.
> 
>>  #include <wchar.h>
>> +#include <string.h>
>> +
>> +#define AVAILABLE(h, h_l, j, n_l)					\
>> +  (((j) + (n_l) <= (h_l))						\
>> +   || ((h_l) += __wcsnlen ((void*)((h) + (h_l)), (n_l) + 128),		\
>> +       (j) + (n_l) <= (h_l)))
>> +#include "wcs-two-way.h"
> 
> Ok.
> 
>>  wchar_t *
>>  wcsstr (const wchar_t *haystack, const wchar_t *needle)
>>  {
>> -  wchar_t b, c;
>> -
>> . . .
>> -  return (wchar_t*) haystack;
>> -ret0:
>> -  return NULL;
>> +  /* Ensure haystack length is at least as long as needle length.
>> +     Since a match may occur early on in a huge haystack, use strnlen
>> +     and read ahead a few cachelines for improved performance.  */
>> +  size_t ne_len = __wcslen (needle);
>> +  size_t hs_len = __wcsnlen (haystack, ne_len | 128);
>> +  if (hs_len < ne_len)
>> +    return NULL;
>> +
>> +  /* Check whether we have a match.  This improves performance since we
>> +     avoid initialization overheads.  */
>> +  if (__wmemcmp (haystack, needle, ne_len) == 0)
>> +    return (wchar_t *) haystack;
>> +
>> +  return two_way_short_needle (haystack, hs_len, needle, ne_len);
>>  }
> 
> Ok.
> 

Re: [PATCH v2 2/3] wcsmbs: Add test-wcsstr

[Adhemerval Zanella Netto]

On 01/03/24 12:52, Adhemerval Zanella Netto wrote:
> 
> 
> On 28/02/24 21:45, DJ Delorie wrote:
>> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
>>> Parametrize test-strstr.c so it can be used to check wcsstr.
>>
>> LGTM although I suggest renaming one of the variables to more accurately
>> reflect its new role.  Also a question about error() formatting, but you
>> could commit this as-is if you want.
>>
>> Reviewed-by: DJ Delorie <dj@redhat.com>
>>
>>> diff --git a/string/test-strstr.c b/string/test-strstr.c
>>> index 05d0b7c98c..f82aeb2cfa 100644
>>> --- a/string/test-strstr.c
>>> +++ b/string/test-strstr.c
>>> @@ -17,16 +17,44 @@
>>>     <https://www.gnu.org/licenses/>.  */
>>>  
>>>  #define TEST_MAIN
>>> -#define TEST_NAME "strstr"
>>> -#include "test-string.h"
>>> +#ifndef WIDE
>>> +# define TEST_NAME "strstr"
>>> +#else
>>> +# define TEST_NAME "wcsstr"
>>> +#endif
>>
>> Ok.
>>
>>> +#ifndef WIDE
>>> +# define CHAR char
>>> +# define STRLEN strlen
>>> +# define STRSTR strstr
>>> +# define STRCPY strcpy
>>> +# define MEMCPY memcpy
>>> +# define MEMSET memset
>>> +# define MEMPCPY mempcpy
>>> +# define L(s) s
>>> +#else
>>> +# include <wchar.h>
>>> +# define CHAR wchar_t
>>> +# define STRLEN wcslen
>>> +# define STRCPY wcscpy
>>> +# define STRSTR wcsstr
>>> +# define MEMCPY wmemcpy
>>> +# define MEMSET wmemset
>>> +# define MEMPCPY wmempcpy
>>> +# define L(s) L ## s
>>> +/* The test requires up to 8191 charateres, so allocate at least 32Kb
>>> +   (considering 4kb page size).  */
>>> +# define BUF1PAGES 4
>>> +#endif
>>>  
>>> +#include "test-string.h"
>>
>> Ok.
>>
>>>  /* Naive implementation to verify results.  */
>>> -static char *
>>> -simple_strstr (const char *s1, const char *s2)
>>> +static CHAR *
>>> +simple_strstr (const CHAR *s1, const CHAR *s2)
>>>  {
>>> -  ssize_t s1len = strlen (s1);
>>> -  ssize_t s2len = strlen (s2);
>>> +  ssize_t s1len = STRLEN (s1);
>>> +  ssize_t s2len = STRLEN (s2);
>>>  
>>>    if (s2len > s1len)
>>>      return NULL;
>>> @@ -38,28 +66,27 @@ simple_strstr (const char *s1, const char *s2)
>>>  	if (s1[i + j] != s2[j])
>>>  	  break;
>>>        if (j == s2len)
>>> -	return (char *) s1 + i;
>>> +	return (CHAR *) s1 + i;
>>>      }
>>>  
>>>    return NULL;
>>>  }
>>
>> Ok.
>>
>>
>>> -typedef char *(*proto_t) (const char *, const char *);
>>> +typedef CHAR *(*proto_t) (const CHAR *, const CHAR *);
>>>  
>>> -IMPL (strstr, 1)
>>> +IMPL (STRSTR, 1)
>>
>> Ok.
>>
>>>  static int
>>> -check_result (impl_t *impl, const char *s1, const char *s2,
>>> -	      char *exp_result)
>>> +check_result (impl_t *impl, const CHAR *s1, const CHAR *s2,
>>> +	      CHAR *exp_result)
>>
>> Ok.
>>
>>>  {
>>> -  char *result = CALL (impl, s1, s2);
>>> +  CHAR *result = CALL (impl, s1, s2);
>>
>> Ok.
>>
>>>    if (result != exp_result)
>>>      {
>>> -      error (0, 0, "Wrong result in function %s %s %s", impl->name,
>>> -	     (result == NULL) ? "(null)" : result,
>>> -	     (exp_result == NULL) ? "(null)" : exp_result);
>>> +      error (0, 0, "Wrong result in function %p %p %p", impl->name,
>>> +	     result, exp_result);
>>
>> Ok... but do we not have a way to printf() wide character strings?
> 
> Indeed, I think we can use "%Ls" here. I think I have used %p just to avoid
> adding another parametrized macro.  I will fix it.

In fact I think I will keep '%p', the others tests functions also only print
the address (I think to avoid polluting the console/log file with gibberish).

Re: [PATCH v2 3/3] wcsmbs: Ensure wcstr worst-case linear execution time (BZ 23865)

[DJ Delorie]

Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> writes:
>>> +    for (; repeat > 0; repeat--)
>>> +      {
>>> +	FOR_EACH_IMPL (impl, 0)
>>> +	  check_result (impl, haystack, needle, haystack + 1);
>>> +      }
>> 
>> What is the purpose of the repeat loop?  This function is only called
>> once, and not timed...
>> 
>> The test itself is OK.
>
> I copied this tests from gnulib (9411c5e467cf60f6295b9fed306029f341a0f24f)
> and my understanding is to enforce that wcsstr won't take too much time
> by triggering a timeout.  These kind of pattern are slower for newer 
> implementation compared to the current one.
>
> The repeat number is indeed arbitrary and depending of the hardware should 
> not make any different (on my box I only see some different with way larger
> inputs).

My point is, there isn't anything in the test that either (1) computes
how long the test takes, or (2) has a way to FAIL the test if the
timings are off.  I.e. you are not testing for the bug you're fixing.

Given how common it is to set TIMEOUTFACTOR to some large value when
running tests, I don't think relying on a timeout - for anything other
than "infinite time" - is reasonable.

>>> +	FOR_EACH_IMPL (impl, 0)
>>> +	  check_result (impl, haystack, needle, NULL);
>>> +      }
>>> +
>>> +    free (needle);
>>> +  }
>> 
>> Test OK but question the repeat look.

Same here.

>>> +    FOR_EACH_IMPL (impl, 0)
>>> +      check_result (impl, haystack, needle, haystack + m);
>> 
>> Without timing it, how do we test that it is not quadratic?
>
> Because on current implementation it takes forever (about 4 minutes
> on my box) due the input sizes.

Same here.  Nobody will notice the 4 minute test time in a scripted CI
environment unless the test returns FAIL.

Re: [PATCH v2 3/3] wcsmbs: Ensure wcstr worst-case linear execution time (BZ 23865)

[Adhemerval Zanella Netto]

On 01/03/24 16:23, DJ Delorie wrote:
> Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> writes:
>>>> +    for (; repeat > 0; repeat--)
>>>> +      {
>>>> +	FOR_EACH_IMPL (impl, 0)
>>>> +	  check_result (impl, haystack, needle, haystack + 1);
>>>> +      }
>>>
>>> What is the purpose of the repeat loop?  This function is only called
>>> once, and not timed...
>>>
>>> The test itself is OK.
>>
>> I copied this tests from gnulib (9411c5e467cf60f6295b9fed306029f341a0f24f)
>> and my understanding is to enforce that wcsstr won't take too much time
>> by triggering a timeout.  These kind of pattern are slower for newer 
>> implementation compared to the current one.
>>
>> The repeat number is indeed arbitrary and depending of the hardware should 
>> not make any different (on my box I only see some different with way larger
>> inputs).
> 
> My point is, there isn't anything in the test that either (1) computes
> how long the test takes, or (2) has a way to FAIL the test if the
> timings are off.  I.e. you are not testing for the bug you're fixing.
> 
> Given how common it is to set TIMEOUTFACTOR to some large value when
> running tests, I don't think relying on a timeout - for anything other
> than "infinite time" - is reasonable.

Fair enough, although I still some value in adding tests that might trigger
quadratic behavior on possible quadradic implementation.  I agree that the
loop adds little gain and I think it would be better to just remove it.

> 
>>>> +	FOR_EACH_IMPL (impl, 0)
>>>> +	  check_result (impl, haystack, needle, NULL);
>>>> +      }
>>>> +
>>>> +    free (needle);
>>>> +  }
>>>
>>> Test OK but question the repeat look.
> 
> Same here.
> 
>>>> +    FOR_EACH_IMPL (impl, 0)
>>>> +      check_result (impl, haystack, needle, haystack + m);
>>>
>>> Without timing it, how do we test that it is not quadratic?
>>
>> Because on current implementation it takes forever (about 4 minutes
>> on my box) due the input sizes.
> 
> Same here.  Nobody will notice the 4 minute test time in a scripted CI
> environment unless the test returns FAIL.
> 

I think we can still enforce a timer without being subject to TIMEOUTFACTOR
by using support_create_timer.