| Message ID | 20240220200445.4000158-4-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Improve wcsstr | expand |
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.
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. >
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.
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.
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. */