[PATCH 1/4] strncmp: Add a testcase for page boundary [BZ #25933]

[PATCH 1/4] strncmp: Add a testcase for page boundary [BZ #25933]

[H.J. Lu]

Add a strncmp testcase to cover cases where one of strings ends on the
page boundary.
---
 string/test-strncmp.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/string/test-strncmp.c b/string/test-strncmp.c
index d961ac4493..d0928a2864 100644
--- a/string/test-strncmp.c
+++ b/string/test-strncmp.c
@@ -403,6 +403,30 @@ check2 (void)
   free (s2);
 }
 
+static void
+check3 (void)
+{
+  size_t size = 32 * 4;
+  CHAR *s1 = (CHAR *) (buf1 + (BUF1PAGES - 1) * page_size);
+  CHAR *s2 = (CHAR *) (buf2 + (BUF1PAGES - 1) * page_size);
+  int exp_result;
+
+  memset (s1, 'a', page_size);
+  memset (s2, 'a', page_size);
+  s1[(page_size / CHARBYTES) - 1] = (CHAR) 0;
+
+  for (size_t s = 99; s <= size; s++)
+    for (size_t s1a = 31; s1a < 32; s1a++)
+      for (size_t s2a = 30; s2a < 32; s2a++)
+	{
+	  CHAR *s1p = s1 + (page_size / CHARBYTES - s) - s1a;
+	  CHAR *s2p = s2 + (page_size / CHARBYTES - s) - s2a;
+	  exp_result = SIMPLE_STRNCMP (s1p, s2p, s);
+	  FOR_EACH_IMPL (impl, 0)
+	    check_result (impl, s1p, s2p, s, exp_result);
+	}
+}
+
 int
 test_main (void)
 {
@@ -412,6 +436,7 @@ test_main (void)
 
   check1 ();
   check2 ();
+  check3 ();
 
   printf ("%23s", "");
   FOR_EACH_IMPL (impl, 0)
-- 
2.26.2

[PATCH 2/4] strcmp: Add a testcase for page boundary

[H.J. Lu]

Add a strncmp testcase to cover cases where both strings end on the
page boundary.
---
 string/test-strcmp.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/string/test-strcmp.c b/string/test-strcmp.c
index 8d4784de80..41d95567c7 100644
--- a/string/test-strcmp.c
+++ b/string/test-strcmp.c
@@ -359,6 +359,30 @@ check (void)
     }
 }
 
+static void
+check2 (void)
+{
+  size_t size = 32 * 4;
+  CHAR *s1 = (CHAR *) (buf1 + (BUF1PAGES - 1) * page_size);
+  CHAR *s2 = (CHAR *) (buf2 + (BUF1PAGES - 1) * page_size);
+  int exp_result;
+
+  memset (s1, 'a', page_size);
+  memset (s2, 'a', page_size);
+  s1[(page_size / CHARBYTES) - 1] = (CHAR) 0;
+  s2[(page_size / CHARBYTES) - 1] = (CHAR) 0;
+
+  for (size_t s = 99; s <= size; s++)
+    for (size_t s1a = 31; s1a < 32; s1a++)
+      for (size_t s2a = 30; s2a < 32; s2a++)
+	{
+	  CHAR *s1p = s1 + (page_size / CHARBYTES - s) - s1a;
+	  CHAR *s2p = s2 + (page_size / CHARBYTES - s) - s2a;
+	  exp_result = SIMPLE_STRCMP (s1p, s2p);
+	  FOR_EACH_IMPL (impl, 0)
+	    check_result (impl, s1p, s2p, exp_result);
+	}
+}
 
 int
 test_main (void)
@@ -367,6 +391,7 @@ test_main (void)
 
   test_init ();
   check();
+  check2 ();
 
   printf ("%23s", "");
   FOR_EACH_IMPL (impl, 0)
-- 
2.26.2

[PATCH 3/4] bench-strncmp.c: Add workloads on page boundary

[H.J. Lu]

Add strncmp workloads on page boundary.
---
 benchtests/bench-strncmp.c | 117 +++++++++++++++++++++++++++++++++++++
 1 file changed, 117 insertions(+)

diff --git a/benchtests/bench-strncmp.c b/benchtests/bench-strncmp.c
index 95a59c9465..065c7e7789 100644
--- a/benchtests/bench-strncmp.c
+++ b/benchtests/bench-strncmp.c
@@ -27,6 +27,7 @@
 
 #ifdef WIDE
 # define L(str) L##str
+# define STRDUP wcsdup
 # define SIMPLE_STRNCMP simple_wcsncmp
 
 /* Wcsncmp uses signed semantics for comparison, not unsigned.
@@ -48,6 +49,7 @@ simple_wcsncmp (const CHAR *s1, const CHAR *s2, size_t n)
 
 #else
 # define L(str) str
+# define STRDUP strdup
 # define SIMPLE_STRNCMP simple_strncmp
 
 /* Strncmp uses unsigned semantics for comparison.  */
@@ -190,6 +192,118 @@ do_test (json_ctx_t *json_ctx, size_t align1, size_t align2, size_t len, size_t
   json_element_object_end (json_ctx);
 }
 
+static void
+do_test_page_boundary_1 (json_ctx_t *json_ctx, CHAR *s1, CHAR *s2,
+			 size_t align1, size_t align2, size_t len,
+			 size_t n, int exp_result)
+{
+  json_element_object_begin (json_ctx);
+  json_attr_uint (json_ctx, "strlen", (double) len);
+  json_attr_uint (json_ctx, "len", (double) n);
+  json_attr_uint (json_ctx, "align1", (double) align1);
+  json_attr_uint (json_ctx, "align2", (double) align2);
+  json_array_begin (json_ctx, "timings");
+  FOR_EACH_IMPL (impl, 0)
+    do_one_test (json_ctx, impl, s1, s2, n, exp_result);
+  json_array_end (json_ctx);
+  json_element_object_end (json_ctx);
+}
+
+static void
+do_test_page_boundary (json_ctx_t *json_ctx)
+{
+  size_t size = 32 * 4;
+  size_t len;
+  CHAR *s1 = (CHAR *) (buf1 + (BUF1PAGES - 1) * page_size);
+  CHAR *s2 = (CHAR *) (buf2 + (BUF1PAGES - 1) * page_size);
+  int exp_result;
+
+  memset (s1, 'a', page_size);
+  memset (s2, 'a', page_size);
+
+  s1[(page_size / CHARBYTES) - 1] = (CHAR) 0;
+
+  for (size_t s = 99; s <= size; s++)
+    for (size_t s1a = 31; s1a < 32; s1a++)
+      for (size_t s2a = 30; s2a < 32; s2a++)
+	{
+	  size_t align1 = (page_size / CHARBYTES - s) - s1a;
+	  size_t align2 = (page_size / CHARBYTES - s) - s2a;
+	  CHAR *s1p = s1 + align1;
+	  CHAR *s2p = s2 + align2;
+	  len = (page_size / CHARBYTES) - 1 - align1;
+	  exp_result = SIMPLE_STRNCMP (s1p, s2p, s);
+	  do_test_page_boundary_1 (json_ctx, s1p, s2p, align1, align2,
+				   len, s, exp_result);
+	}
+}
+
+static void
+do_page_test (json_ctx_t *json_ctx, size_t offset1, size_t offset2,
+	      CHAR *s2)
+{
+  CHAR *s1;
+  int exp_result;
+
+  if (offset1 * CHARBYTES  >= page_size
+      || offset2 * CHARBYTES >= page_size)
+    return;
+
+  s1 = (CHAR *) buf1;
+  s1 += offset1;
+  s2 += offset2;
+
+  size_t len = (page_size / CHARBYTES) - offset1;
+
+  exp_result= *s1;
+
+  json_element_object_begin (json_ctx);
+  json_attr_uint (json_ctx, "strlen", (double) len);
+  json_attr_uint (json_ctx, "len", (double) page_size);
+  json_attr_uint (json_ctx, "align1", (double) offset1);
+  json_attr_uint (json_ctx, "align2", (double) offset2);
+  json_array_begin (json_ctx, "timings");
+  {
+    FOR_EACH_IMPL (impl, 0)
+      do_one_test (json_ctx, impl, s1, s2, page_size, -exp_result);
+  }
+  json_array_end (json_ctx);
+  json_element_object_end (json_ctx);
+
+  json_element_object_begin (json_ctx);
+  json_attr_uint (json_ctx, "strlen", (double) len);
+  json_attr_uint (json_ctx, "len", (double) page_size);
+  json_attr_uint (json_ctx, "align1", (double) offset1);
+  json_attr_uint (json_ctx, "align2", (double) offset2);
+  json_array_begin (json_ctx, "timings");
+  {
+    FOR_EACH_IMPL (impl, 0)
+      do_one_test (json_ctx, impl, s1, s2, page_size, exp_result);
+  }
+  json_array_end (json_ctx);
+  json_element_object_end (json_ctx);
+}
+
+static void
+do_test_page (json_ctx_t *json_ctx)
+{
+  size_t i;
+  CHAR *s1, *s2;
+
+  s1 = (CHAR *) buf1;
+  for (i = 0; i < (page_size / CHARBYTES) - 1; i++)
+    s1[i] = 23;
+  s1[i] = 0;
+
+  s2 = STRDUP (s1);
+
+  for (i = 0; i < 64; ++i)
+    do_page_test (json_ctx, (3988 / CHARBYTES) + i,
+		  (2636 / CHARBYTES), s2);
+
+  free (s2);
+}
+
 int
 test_main (void)
 {
@@ -267,6 +381,9 @@ test_main (void)
       do_test_limit (&json_ctx, 0, 0, 15 - i, 16 - i, 255, -1);
     }
 
+  do_test_page_boundary (&json_ctx);
+  do_test_page (&json_ctx);
+
   json_array_end (&json_ctx);
   json_attr_object_end (&json_ctx);
   json_attr_object_end (&json_ctx);
-- 
2.26.2

[PATCH 4/4] bench-strcmp.c: Add workloads on page boundary

[H.J. Lu]

Add strcmp workloads on page boundary.
---
 benchtests/bench-strcmp.c | 48 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/benchtests/bench-strcmp.c b/benchtests/bench-strcmp.c
index 47d0a35299..3ba1399a4d 100644
--- a/benchtests/bench-strcmp.c
+++ b/benchtests/bench-strcmp.c
@@ -144,6 +144,52 @@ do_test (json_ctx_t *json_ctx, size_t align1, size_t align2, size_t len, int
   json_element_object_end (json_ctx);
 }
 
+static void
+do_test_page_boundary_1 (json_ctx_t *json_ctx, CHAR *s1, CHAR *s2,
+			 size_t align1, size_t align2, size_t len,
+			 int exp_result)
+{
+  json_element_object_begin (json_ctx);
+  json_attr_uint (json_ctx, "length", (double) len);
+  json_attr_uint (json_ctx, "align1", (double) align1);
+  json_attr_uint (json_ctx, "align2", (double) align2);
+  json_array_begin (json_ctx, "timings");
+  FOR_EACH_IMPL (impl, 0)
+    do_one_test (json_ctx, impl, s1, s2, exp_result);
+  json_array_end (json_ctx);
+  json_element_object_end (json_ctx);
+}
+
+static void
+do_test_page_boundary (json_ctx_t *json_ctx)
+{
+  size_t size = 32 * 4;
+  size_t len;
+  CHAR *s1 = (CHAR *) (buf1 + (BUF1PAGES - 1) * page_size);
+  CHAR *s2 = (CHAR *) (buf2 + (BUF1PAGES - 1) * page_size);
+  int exp_result;
+
+  memset (s1, 'a', page_size);
+  memset (s2, 'a', page_size);
+
+  s1[(page_size / CHARBYTES) - 1] = (CHAR) 0;
+  s2[(page_size / CHARBYTES) - 1] = (CHAR) 0;
+
+  for (size_t s = 99; s <= size; s++)
+    for (size_t s1a = 31; s1a < 32; s1a++)
+      for (size_t s2a = 30; s2a < 32; s2a++)
+	{
+	  size_t align1 = (page_size / CHARBYTES - s) - s1a;
+	  size_t align2 = (page_size / CHARBYTES - s) - s2a;
+	  CHAR *s1p = s1 + align1;
+	  CHAR *s2p = s2 + align2;
+	  len = (page_size / CHARBYTES) - 1 - align1;
+	  exp_result = SIMPLE_STRCMP (s1p, s2p);
+	  do_test_page_boundary_1 (json_ctx, s1p, s2p, align1, align2,
+				   len, exp_result);
+	}
+}
+
 int
 test_main (void)
 {
@@ -197,6 +243,8 @@ test_main (void)
       do_test (&json_ctx, 2 * CHARBYTES * i, CHARBYTES * i, 8 << i, LARGECHAR, -1);
     }
 
+  do_test_page_boundary (&json_ctx);
+
   json_array_end (&json_ctx);
   json_attr_object_end (&json_ctx);
   json_attr_object_end (&json_ctx);
-- 
2.26.2

Re: [PATCH 1/4] strncmp: Add a testcase for page boundary [BZ #25933]

[Paul A. Clarke]

On Fri, Jun 12, 2020 at 01:10:53PM -0700, H.J. Lu via Libc-alpha wrote:
> Add a strncmp testcase to cover cases where one of strings ends on the
> page boundary.
> ---
>  string/test-strncmp.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/string/test-strncmp.c b/string/test-strncmp.c
> index d961ac4493..d0928a2864 100644
> --- a/string/test-strncmp.c
> +++ b/string/test-strncmp.c
> @@ -403,6 +403,30 @@ check2 (void)
>    free (s2);
>  }
> 
> +static void
> +check3 (void)
> +{
> +  size_t size = 32 * 4;
> +  CHAR *s1 = (CHAR *) (buf1 + (BUF1PAGES - 1) * page_size);
> +  CHAR *s2 = (CHAR *) (buf2 + (BUF1PAGES - 1) * page_size);
> +  int exp_result;
> +
> +  memset (s1, 'a', page_size);
> +  memset (s2, 'a', page_size);
> +  s1[(page_size / CHARBYTES) - 1] = (CHAR) 0;
> +
> +  for (size_t s = 99; s <= size; s++)
> +    for (size_t s1a = 31; s1a < 32; s1a++)
> +      for (size_t s2a = 30; s2a < 32; s2a++)
> +	{
> +	  CHAR *s1p = s1 + (page_size / CHARBYTES - s) - s1a;
> +	  CHAR *s2p = s2 + (page_size / CHARBYTES - s) - s2a;
> +	  exp_result = SIMPLE_STRNCMP (s1p, s2p, s);
> +	  FOR_EACH_IMPL (impl, 0)
> +	    check_result (impl, s1p, s2p, s, exp_result);
> +	}
> +}

There are lots of magic numbers here.

Could you add some context around those numbers?

If they are meant to reflect a cache line length, then it's
appropriate to support different cache line sizes.
Rafael Zinsly just did this with strncasecmp in the
last week or so.

> +
>  int
>  test_main (void)
>  {
> @@ -412,6 +436,7 @@ test_main (void)
> 
>    check1 ();
>    check2 ();
> +  check3 ();
> 
>    printf ("%23s", "");
>    FOR_EACH_IMPL (impl, 0)
> -- 
> 2.26.2
> 

Re: [PATCH 1/4] strncmp: Add a testcase for page boundary [BZ #25933]

[H.J. Lu]

On Mon, Jun 15, 2020 at 1:29 PM Paul A. Clarke <pc@us.ibm.com> wrote:
>
> On Fri, Jun 12, 2020 at 01:10:53PM -0700, H.J. Lu via Libc-alpha wrote:
> > Add a strncmp testcase to cover cases where one of strings ends on the
> > page boundary.
> > ---
> >  string/test-strncmp.c | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> >
> > diff --git a/string/test-strncmp.c b/string/test-strncmp.c
> > index d961ac4493..d0928a2864 100644
> > --- a/string/test-strncmp.c
> > +++ b/string/test-strncmp.c
> > @@ -403,6 +403,30 @@ check2 (void)
> >    free (s2);
> >  }
> >
> > +static void
> > +check3 (void)
> > +{
> > +  size_t size = 32 * 4;
> > +  CHAR *s1 = (CHAR *) (buf1 + (BUF1PAGES - 1) * page_size);
> > +  CHAR *s2 = (CHAR *) (buf2 + (BUF1PAGES - 1) * page_size);
> > +  int exp_result;
> > +
> > +  memset (s1, 'a', page_size);
> > +  memset (s2, 'a', page_size);
> > +  s1[(page_size / CHARBYTES) - 1] = (CHAR) 0;
> > +
> > +  for (size_t s = 99; s <= size; s++)
> > +    for (size_t s1a = 31; s1a < 32; s1a++)
> > +      for (size_t s2a = 30; s2a < 32; s2a++)
> > +     {
> > +       CHAR *s1p = s1 + (page_size / CHARBYTES - s) - s1a;
> > +       CHAR *s2p = s2 + (page_size / CHARBYTES - s) - s2a;
> > +       exp_result = SIMPLE_STRNCMP (s1p, s2p, s);
> > +       FOR_EACH_IMPL (impl, 0)
> > +         check_result (impl, s1p, s2p, s, exp_result);
> > +     }
> > +}
>
> There are lots of magic numbers here.
>
> Could you add some context around those number

My commit log says

---
Add a strncmp testcase to cover cases where one of strings ends on the
page boundary.
---

> If they are meant to reflect a cache line length, then it's

No.  My testcase is for correctness, not for performance.

> appropriate to support different cache line sizes.
> Rafael Zinsly just did this with strncasecmp in the
> last week or so.
>
> > +
> >  int
> >  test_main (void)
> >  {
> > @@ -412,6 +436,7 @@ test_main (void)
> >
> >    check1 ();
> >    check2 ();
> > +  check3 ();
> >
> >    printf ("%23s", "");
> >    FOR_EACH_IMPL (impl, 0)
> > --
> > 2.26.2
> >



-- 
H.J.

Re: [PATCH 1/4] strncmp: Add a testcase for page boundary [BZ #25933]

[Paul A. Clarke]

On Mon, Jun 15, 2020 at 02:34:13PM -0700, H.J. Lu via Libc-alpha wrote:
> On Mon, Jun 15, 2020 at 1:29 PM Paul A. Clarke <pc@us.ibm.com> wrote:
> >
> > On Fri, Jun 12, 2020 at 01:10:53PM -0700, H.J. Lu via Libc-alpha wrote:
> > > Add a strncmp testcase to cover cases where one of strings ends on the
> > > page boundary.
> > > ---
> > >  string/test-strncmp.c | 25 +++++++++++++++++++++++++
> > >  1 file changed, 25 insertions(+)
> > >
> > > diff --git a/string/test-strncmp.c b/string/test-strncmp.c
> > > index d961ac4493..d0928a2864 100644
> > > --- a/string/test-strncmp.c
> > > +++ b/string/test-strncmp.c
> > > @@ -403,6 +403,30 @@ check2 (void)
> > >    free (s2);
> > >  }
> > >
> > > +static void
> > > +check3 (void)
> > > +{
> > > +  size_t size = 32 * 4;
> > > +  CHAR *s1 = (CHAR *) (buf1 + (BUF1PAGES - 1) * page_size);
> > > +  CHAR *s2 = (CHAR *) (buf2 + (BUF1PAGES - 1) * page_size);
> > > +  int exp_result;
> > > +
> > > +  memset (s1, 'a', page_size);
> > > +  memset (s2, 'a', page_size);
> > > +  s1[(page_size / CHARBYTES) - 1] = (CHAR) 0;
> > > +
> > > +  for (size_t s = 99; s <= size; s++)
> > > +    for (size_t s1a = 31; s1a < 32; s1a++)
> > > +      for (size_t s2a = 30; s2a < 32; s2a++)
> > > +     {
> > > +       CHAR *s1p = s1 + (page_size / CHARBYTES - s) - s1a;
> > > +       CHAR *s2p = s2 + (page_size / CHARBYTES - s) - s2a;
> > > +       exp_result = SIMPLE_STRNCMP (s1p, s2p, s);
> > > +       FOR_EACH_IMPL (impl, 0)
> > > +         check_result (impl, s1p, s2p, s, exp_result);
> > > +     }
> > > +}
> >
> > There are lots of magic numbers here.
> >
> > Could you add some context around those number
> 
> My commit log says
> 
> ---
> Add a strncmp testcase to cover cases where one of strings ends on the
> page boundary.
> ---

Which says nothing about why you need to test over 90000 different
cases of a string ending on a page boundary, nor what any of the
magic numbers represent.

> > If they are meant to reflect a cache line length, then it's
> 
> No.  My testcase is for correctness, not for performance.

I made no reference to performance.

> > appropriate to support different cache line sizes.
> > Rafael Zinsly just did this with strncasecmp in the
> > last week or so.

PC

Re: [PATCH 1/4] strncmp: Add a testcase for page boundary [BZ #25933]

[Carlos O'Donell]

On 6/12/20 4:10 PM, H.J. Lu via Libc-alpha wrote:
> Add a strncmp testcase to cover cases where one of strings ends on the
> page boundary.

I would like to see this sequence of 4 patches committed because they *do*
correctly regression test swbz#25933, and I'd like to see this not regress
again.

However, I share Paul's concerns over the magic numbers, so I have made
some concrete suggestions for comments.

OK with the following changes:
- Add all suggested comments.
- s1a iterates over [30,32).

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  string/test-strncmp.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/string/test-strncmp.c b/string/test-strncmp.c
> index d961ac4493..d0928a2864 100644
> --- a/string/test-strncmp.c
> +++ b/string/test-strncmp.c
> @@ -403,6 +403,30 @@ check2 (void)
>    free (s2);
>  }
>  
> +static void
> +check3 (void)
> +{
> +  size_t size = 32 * 4;

Add a comment:

/* To trigger bug 25933 we need a size that is equal to the
   vector length times 4. In the case of AVX2 for Intel we
   need 32 * 4. We make this test generic and run it for all
   architectures as additional boundary testing for such
   related algorithms.  */

This is my understanding, that we need 32*4 to trigger the bug.

> +  CHAR *s1 = (CHAR *) (buf1 + (BUF1PAGES - 1) * page_size);
> +  CHAR *s2 = (CHAR *) (buf2 + (BUF1PAGES - 1) * page_size);

We set s1 and s2 to point into the buffers at a point 1 page
before the end. We expect the test to add 1 page PROT_NONE at
the end. Thus s1 and s2 by default point to two pages, the
first page PROT_READ|PROT_WRITE and the second page PROT_NONE.
So far so good. No comment required. You have to understand
how these test cases work to read this.

> +  int exp_result;
> +
> +  memset (s1, 'a', page_size);
> +  memset (s2, 'a', page_size);

OK. Fill both full of 'a'.

> +  s1[(page_size / CHARBYTES) - 1] = (CHAR) 0;

OK. Null terminate s1.

Note that s2 is not null terminated.

> +

Add comment:

/* Iterate over a size that is just below where we expect
   the bug to trigger up to the size we expect will trigger
   the bug e.g. [99-128].  Likewise iterate the start of
   two strings between 30 and 31 bytes away from the
   boundary to simulate alignment changes.  */

> +  for (size_t s = 99; s <= size; s++)

OK. A bit of belt-and-suspenders here we make s range
from 99-128, so we iterate just before the size we care
about up to the size we expect to trigger the bug.

> +    for (size_t s1a = 31; s1a < 32; s1a++)

s1a iterates over [31,32) e.g. 31

> +      for (size_t s2a = 30; s2a < 32; s2a++)

s2a iterates over [30,32) e.g. 30, 31

> +	{
> +	  CHAR *s1p = s1 + (page_size / CHARBYTES - s) - s1a;

Set the pointer back from the PROT_NONE page by "s+s1a" bytes.

> +	  CHAR *s2p = s2 + (page_size / CHARBYTES - s) - s2a;

Set the pointer back from the PROT_NONE page by "s+s2a" bytes.

> +	  exp_result = SIMPLE_STRNCMP (s1p, s2p, s);

Then compare.

This code comes from Adhemerval's testing in comment #2,
where the test catches a second loop that has similar problems.

At most we test [30 sizes]x[1 offset for s1a]x[2 offets for s2a] = 60 tests.

Suggest:

s1a iterate over [30,32) like s2a for the sake of simplicity.

> +	  FOR_EACH_IMPL (impl, 0)
> +	    check_result (impl, s1p, s2p, s, exp_result);
> +	}
> +}
> +
>  int
>  test_main (void)
>  {
> @@ -412,6 +436,7 @@ test_main (void)
>  
>    check1 ();
>    check2 ();
> +  check3 ();

OK.

>  
>    printf ("%23s", "");
>    FOR_EACH_IMPL (impl, 0)
> 


-- 
Cheers,
Carlos.

Re: [PATCH 2/4] strcmp: Add a testcase for page boundary

[Carlos O'Donell]

On 6/12/20 4:10 PM, H.J. Lu via Libc-alpha wrote:
> Add a strncmp testcase to cover cases where both strings end on the
> page boundary.
> ---
>  string/test-strcmp.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 

OK to commit if you:
- Add all the comments.
- s1a iterates over [30,32).

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> diff --git a/string/test-strcmp.c b/string/test-strcmp.c
> index 8d4784de80..41d95567c7 100644
> --- a/string/test-strcmp.c
> +++ b/string/test-strcmp.c
> @@ -359,6 +359,30 @@ check (void)
>      }
>  }
>  

Add a comment:

/* To trigger bug 25933 we need a size that is equal to the
   vector length times 4. In the case of AVX2 for Intel we
   need 32 * 4. We make this test generic and run it for all
   architectures as additional boundary testing for such
   related algorithms.  */

> +static void
> +check2 (void)
> +{
> +  size_t size = 32 * 4;
> +  CHAR *s1 = (CHAR *) (buf1 + (BUF1PAGES - 1) * page_size);
> +  CHAR *s2 = (CHAR *) (buf2 + (BUF1PAGES - 1) * page_size);
> +  int exp_result;
> +
> +  memset (s1, 'a', page_size);
> +  memset (s2, 'a', page_size);
> +  s1[(page_size / CHARBYTES) - 1] = (CHAR) 0;
> +  s2[(page_size / CHARBYTES) - 1] = (CHAR) 0;
> +

Add comment:

/* Iterate over a size that is just below where we expect
   the bug to trigger up to the size we expect will trigger
   the bug e.g. [99-128].  Likewise iterate the start of
   two strings between 30 and 31 bytes away from the
   boundary to simulate alignment changes.  */

> +  for (size_t s = 99; s <= size; s++)
> +    for (size_t s1a = 31; s1a < 32; s1a++)

Please make s1a iterate over [30,32) like s2a.

> +      for (size_t s2a = 30; s2a < 32; s2a++)
> +	{
> +	  CHAR *s1p = s1 + (page_size / CHARBYTES - s) - s1a;
> +	  CHAR *s2p = s2 + (page_size / CHARBYTES - s) - s2a;
> +	  exp_result = SIMPLE_STRCMP (s1p, s2p);
> +	  FOR_EACH_IMPL (impl, 0)
> +	    check_result (impl, s1p, s2p, exp_result);
> +	}
> +}
>  
>  int
>  test_main (void)
> @@ -367,6 +391,7 @@ test_main (void)
>  
>    test_init ();
>    check();
> +  check2 ();
>  
>    printf ("%23s", "");
>    FOR_EACH_IMPL (impl, 0)
> 


-- 
Cheers,
Carlos.

Re: [PATCH 3/4] bench-strncmp.c: Add workloads on page boundary

[Carlos O'Donell]

On 6/12/20 4:10 PM, H.J. Lu via Libc-alpha wrote:
> Add strncmp workloads on page boundary.

This benchmark need some work.

Please post v2.

> ---
>  benchtests/bench-strncmp.c | 117 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 117 insertions(+)
> 
> diff --git a/benchtests/bench-strncmp.c b/benchtests/bench-strncmp.c
> index 95a59c9465..065c7e7789 100644
> --- a/benchtests/bench-strncmp.c
> +++ b/benchtests/bench-strncmp.c
> @@ -27,6 +27,7 @@
>  
>  #ifdef WIDE
>  # define L(str) L##str
> +# define STRDUP wcsdup

OK.

>  # define SIMPLE_STRNCMP simple_wcsncmp
>  
>  /* Wcsncmp uses signed semantics for comparison, not unsigned.
> @@ -48,6 +49,7 @@ simple_wcsncmp (const CHAR *s1, const CHAR *s2, size_t n)
>  
>  #else
>  # define L(str) str
> +# define STRDUP strdup

OK.

>  # define SIMPLE_STRNCMP simple_strncmp
>  
>  /* Strncmp uses unsigned semantics for comparison.  */
> @@ -190,6 +192,118 @@ do_test (json_ctx_t *json_ctx, size_t align1, size_t align2, size_t len, size_t
>    json_element_object_end (json_ctx);
>  }
>  
> +static void
> +do_test_page_boundary_1 (json_ctx_t *json_ctx, CHAR *s1, CHAR *s2,

Rename to "do_one_test_page_boundary"

> +			 size_t align1, size_t align2, size_t len,
> +			 size_t n, int exp_result)
> +{
> +  json_element_object_begin (json_ctx);
> +  json_attr_uint (json_ctx, "strlen", (double) len);
> +  json_attr_uint (json_ctx, "len", (double) n);
> +  json_attr_uint (json_ctx, "align1", (double) align1);
> +  json_attr_uint (json_ctx, "align2", (double) align2);
> +  json_array_begin (json_ctx, "timings");
> +  FOR_EACH_IMPL (impl, 0)
> +    do_one_test (json_ctx, impl, s1, s2, n, exp_result);
> +  json_array_end (json_ctx);
> +  json_element_object_end (json_ctx);

OK.

> +}
> +


Add a comment:

/* To trigger bug 25933 we need a size that is equal to the
   vector length times 4. In the case of AVX2 for Intel we
   need 32 * 4. We make this test generic and run it for all
   architectures as additional boundary testing for such
   related algorithms.  */

> +static void
> +do_test_page_boundary (json_ctx_t *json_ctx)

OK.

> +{
> +  size_t size = 32 * 4;
> +  size_t len;
> +  CHAR *s1 = (CHAR *) (buf1 + (BUF1PAGES - 1) * page_size);
> +  CHAR *s2 = (CHAR *) (buf2 + (BUF1PAGES - 1) * page_size);
> +  int exp_result;
> +
> +  memset (s1, 'a', page_size);
> +  memset (s2, 'a', page_size);
> +
> +  s1[(page_size / CHARBYTES) - 1] = (CHAR) 0;
> +

Add comment:

/* Iterate over a size that is just below where we expect
   the bug to trigger up to the size we expect will trigger
   the bug e.g. [99-128].  Likewise iterate the start of
   two strings between 30 and 31 bytes away from the
   boundary to simulate alignment changes.  */

> +  for (size_t s = 99; s <= size; s++)
> +    for (size_t s1a = 31; s1a < 32; s1a++)

Please make s1a iterate over [30,32) like s2a.

> +      for (size_t s2a = 30; s2a < 32; s2a++)
> +	{
> +	  size_t align1 = (page_size / CHARBYTES - s) - s1a;
> +	  size_t align2 = (page_size / CHARBYTES - s) - s2a;
> +	  CHAR *s1p = s1 + align1;
> +	  CHAR *s2p = s2 + align2;
> +	  len = (page_size / CHARBYTES) - 1 - align1;
> +	  exp_result = SIMPLE_STRNCMP (s1p, s2p, s);
> +	  do_test_page_boundary_1 (json_ctx, s1p, s2p, align1, align2,
> +				   len, s, exp_result);
> +	}
> +}
> +
> +static void
> +do_page_test (json_ctx_t *json_ctx, size_t offset1, size_t offset2,

Rename to do_one_test_page()

> +	      CHAR *s2)
> +{
> +  CHAR *s1;
> +  int exp_result;
> +
> +  if (offset1 * CHARBYTES  >= page_size
> +      || offset2 * CHARBYTES >= page_size)
> +    return;

OK. Nothing bigger than a page size because we only have one page to work within.

> +
> +  s1 = (CHAR *) buf1;
> +  s1 += offset1;
> +  s2 += offset2;
> +
> +  size_t len = (page_size / CHARBYTES) - offset1;
> +
> +  exp_result= *s1;
> +
> +  json_element_object_begin (json_ctx);
> +  json_attr_uint (json_ctx, "strlen", (double) len);
> +  json_attr_uint (json_ctx, "len", (double) page_size);
> +  json_attr_uint (json_ctx, "align1", (double) offset1);
> +  json_attr_uint (json_ctx, "align2", (double) offset2);
> +  json_array_begin (json_ctx, "timings");
> +  {
> +    FOR_EACH_IMPL (impl, 0)
> +      do_one_test (json_ctx, impl, s1, s2, page_size, -exp_result);
> +  }
> +  json_array_end (json_ctx);
> +  json_element_object_end (json_ctx);
> +
> +  json_element_object_begin (json_ctx);
> +  json_attr_uint (json_ctx, "strlen", (double) len);
> +  json_attr_uint (json_ctx, "len", (double) page_size);
> +  json_attr_uint (json_ctx, "align1", (double) offset1);
> +  json_attr_uint (json_ctx, "align2", (double) offset2);
> +  json_array_begin (json_ctx, "timings");
> +  {
> +    FOR_EACH_IMPL (impl, 0)
> +      do_one_test (json_ctx, impl, s1, s2, page_size, exp_result);
> +  }
> +  json_array_end (json_ctx);
> +  json_element_object_end (json_ctx);
> +}

OK.

> +
> +static void
> +do_test_page (json_ctx_t *json_ctx)
> +{
> +  size_t i;
> +  CHAR *s1, *s2;
> +
> +  s1 = (CHAR *) buf1;
> +  for (i = 0; i < (page_size / CHARBYTES) - 1; i++)
> +    s1[i] = 23;

Add a comment for this magic fill number.

> +  s1[i] = 0;
> +
> +  s2 = STRDUP (s1);
> +
> +  for (i = 0; i < 64; ++i)

Add a comment for these magic numbers.

> +    do_page_test (json_ctx, (3988 / CHARBYTES) + i,
> +		  (2636 / CHARBYTES), s2);

Call do_one_test_page()

> +
> +  free (s2);
> +}
> +
>  int
>  test_main (void)
>  {
> @@ -267,6 +381,9 @@ test_main (void)
>        do_test_limit (&json_ctx, 0, 0, 15 - i, 16 - i, 255, -1);
>      }
>  
> +  do_test_page_boundary (&json_ctx);
> +  do_test_page (&json_ctx);

OK.

> +
>    json_array_end (&json_ctx);
>    json_attr_object_end (&json_ctx);
>    json_attr_object_end (&json_ctx);
> 


-- 
Cheers,
Carlos.

Re: [PATCH 4/4] bench-strcmp.c: Add workloads on page boundary

[Carlos O'Donell]

On 6/12/20 4:10 PM, H.J. Lu via Libc-alpha wrote:
> Add strcmp workloads on page boundary.
> ---
>  benchtests/bench-strcmp.c | 48 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 

These additions to the benchmark exercise the page boundary conditions
in the algorithms implemented by all the architectures. The test is
designed from the experience with bug 25933 in mind so we should mention
that in a comment. It could be made a bit more generic by parametrizing
vector sizes and number of vector registers that might be grouped e.g.
32 * 4, but it's fine as it is for now to test the page boundary.

OK with:
- Function rename
- Added comments.
- s1a iterate over [30,21)

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> diff --git a/benchtests/bench-strcmp.c b/benchtests/bench-strcmp.c
> index 47d0a35299..3ba1399a4d 100644
> --- a/benchtests/bench-strcmp.c
> +++ b/benchtests/bench-strcmp.c
> @@ -144,6 +144,52 @@ do_test (json_ctx_t *json_ctx, size_t align1, size_t align2, size_t len, int
>    json_element_object_end (json_ctx);
>  }
>  
> +static void
> +do_test_page_boundary_1 (json_ctx_t *json_ctx, CHAR *s1, CHAR *s2,

Rename to do_one_test_page_boundary ()

> +			 size_t align1, size_t align2, size_t len,
> +			 int exp_result)
> +{
> +  json_element_object_begin (json_ctx);
> +  json_attr_uint (json_ctx, "length", (double) len);
> +  json_attr_uint (json_ctx, "align1", (double) align1);
> +  json_attr_uint (json_ctx, "align2", (double) align2);
> +  json_array_begin (json_ctx, "timings");
> +  FOR_EACH_IMPL (impl, 0)
> +    do_one_test (json_ctx, impl, s1, s2, exp_result);

OK.

> +  json_array_end (json_ctx);
> +  json_element_object_end (json_ctx);
> +}
> +

Add a comment:

/* To trigger bug 25933 we need a size that is equal to the
   vector length times 4. In the case of AVX2 for Intel we
   need 32 * 4. We make this test generic and run it for all
   architectures as additional boundary testing for such
   related algorithms.  */

> +static void
> +do_test_page_boundary (json_ctx_t *json_ctx)
> +{
> +  size_t size = 32 * 4;
> +  size_t len;
> +  CHAR *s1 = (CHAR *) (buf1 + (BUF1PAGES - 1) * page_size);
> +  CHAR *s2 = (CHAR *) (buf2 + (BUF1PAGES - 1) * page_size);
> +  int exp_result;
> +
> +  memset (s1, 'a', page_size);
> +  memset (s2, 'a', page_size);
> +
> +  s1[(page_size / CHARBYTES) - 1] = (CHAR) 0;
> +  s2[(page_size / CHARBYTES) - 1] = (CHAR) 0;
> +


Add comment:

/* Iterate over a size that is just below where we expect
   the bug to trigger up to the size we expect will trigger
   the bug e.g. [99-128].  Likewise iterate the start of
   two strings between 30 and 31 bytes away from the
   boundary to simulate alignment changes.  */

> +  for (size_t s = 99; s <= size; s++)
> +    for (size_t s1a = 31; s1a < 32; s1a++)

Make s1a iterate over [30,32) like s2a.

> +      for (size_t s2a = 30; s2a < 32; s2a++)
> +	{
> +	  size_t align1 = (page_size / CHARBYTES - s) - s1a;
> +	  size_t align2 = (page_size / CHARBYTES - s) - s2a;
> +	  CHAR *s1p = s1 + align1;
> +	  CHAR *s2p = s2 + align2;
> +	  len = (page_size / CHARBYTES) - 1 - align1;
> +	  exp_result = SIMPLE_STRCMP (s1p, s2p);
> +	  do_test_page_boundary_1 (json_ctx, s1p, s2p, align1, align2,

Call do_one_test_page_boundary ()

> +				   len, exp_result);
> +	}
> +}
> +
>  int
>  test_main (void)
>  {
> @@ -197,6 +243,8 @@ test_main (void)
>        do_test (&json_ctx, 2 * CHARBYTES * i, CHARBYTES * i, 8 << i, LARGECHAR, -1);
>      }
>  
> +  do_test_page_boundary (&json_ctx);

OK.

> +
>    json_array_end (&json_ctx);
>    json_attr_object_end (&json_ctx);
>    json_attr_object_end (&json_ctx);
> 


-- 
Cheers,
Carlos.

Re: [PATCH 1/4] strncmp: Add a testcase for page boundary [BZ #25933]

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 3935 bytes --]

On Wed, Sep 23, 2020 at 12:01 PM Carlos O'Donell <carlos@redhat.com> wrote:
>
> On 6/12/20 4:10 PM, H.J. Lu via Libc-alpha wrote:
> > Add a strncmp testcase to cover cases where one of strings ends on the
> > page boundary.
>
> I would like to see this sequence of 4 patches committed because they *do*
> correctly regression test swbz#25933, and I'd like to see this not regress
> again.
>
> However, I share Paul's concerns over the magic numbers, so I have made
> some concrete suggestions for comments.
>
> OK with the following changes:
> - Add all suggested comments.
> - s1a iterates over [30,32).
>
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
>
> > ---
> >  string/test-strncmp.c | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> >
> > diff --git a/string/test-strncmp.c b/string/test-strncmp.c
> > index d961ac4493..d0928a2864 100644
> > --- a/string/test-strncmp.c
> > +++ b/string/test-strncmp.c
> > @@ -403,6 +403,30 @@ check2 (void)
> >    free (s2);
> >  }
> >
> > +static void
> > +check3 (void)
> > +{
> > +  size_t size = 32 * 4;
>
> Add a comment:
>
> /* To trigger bug 25933 we need a size that is equal to the
>    vector length times 4. In the case of AVX2 for Intel we
>    need 32 * 4. We make this test generic and run it for all
>    architectures as additional boundary testing for such
>    related algorithms.  */
>
> This is my understanding, that we need 32*4 to trigger the bug.
>
> > +  CHAR *s1 = (CHAR *) (buf1 + (BUF1PAGES - 1) * page_size);
> > +  CHAR *s2 = (CHAR *) (buf2 + (BUF1PAGES - 1) * page_size);
>
> We set s1 and s2 to point into the buffers at a point 1 page
> before the end. We expect the test to add 1 page PROT_NONE at
> the end. Thus s1 and s2 by default point to two pages, the
> first page PROT_READ|PROT_WRITE and the second page PROT_NONE.
> So far so good. No comment required. You have to understand
> how these test cases work to read this.
>
> > +  int exp_result;
> > +
> > +  memset (s1, 'a', page_size);
> > +  memset (s2, 'a', page_size);
>
> OK. Fill both full of 'a'.
>
> > +  s1[(page_size / CHARBYTES) - 1] = (CHAR) 0;
>
> OK. Null terminate s1.
>
> Note that s2 is not null terminated.
>
> > +
>
> Add comment:
>
> /* Iterate over a size that is just below where we expect
>    the bug to trigger up to the size we expect will trigger
>    the bug e.g. [99-128].  Likewise iterate the start of
>    two strings between 30 and 31 bytes away from the
>    boundary to simulate alignment changes.  */
>
> > +  for (size_t s = 99; s <= size; s++)
>
> OK. A bit of belt-and-suspenders here we make s range
> from 99-128, so we iterate just before the size we care
> about up to the size we expect to trigger the bug.
>
> > +    for (size_t s1a = 31; s1a < 32; s1a++)
>
> s1a iterates over [31,32) e.g. 31
>
> > +      for (size_t s2a = 30; s2a < 32; s2a++)
>
> s2a iterates over [30,32) e.g. 30, 31
>
> > +     {
> > +       CHAR *s1p = s1 + (page_size / CHARBYTES - s) - s1a;
>
> Set the pointer back from the PROT_NONE page by "s+s1a" bytes.
>
> > +       CHAR *s2p = s2 + (page_size / CHARBYTES - s) - s2a;
>
> Set the pointer back from the PROT_NONE page by "s+s2a" bytes.
>
> > +       exp_result = SIMPLE_STRNCMP (s1p, s2p, s);
>
> Then compare.
>
> This code comes from Adhemerval's testing in comment #2,
> where the test catches a second loop that has similar problems.
>
> At most we test [30 sizes]x[1 offset for s1a]x[2 offets for s2a] = 60 tests.
>
> Suggest:
>
> s1a iterate over [30,32) like s2a for the sake of simplicity.
>
> > +       FOR_EACH_IMPL (impl, 0)
> > +         check_result (impl, s1p, s2p, s, exp_result);
> > +     }
> > +}
> > +
> >  int
> >  test_main (void)
> >  {
> > @@ -412,6 +436,7 @@ test_main (void)
> >
> >    check1 ();
> >    check2 ();
> > +  check3 ();
>
> OK.
>
> >
> >    printf ("%23s", "");
> >    FOR_EACH_IMPL (impl, 0)
> >
>

Here is the updated patch I am checking in.

Thanks.

-- 
H.J.

[-- Attachment #2: 0001-strncmp-Add-a-testcase-for-page-boundary-BZ-25933.patch --]
[-- Type: text/x-patch, Size: 2426 bytes --]

From 48d19840f8642298823026bafc4c89244fc26889 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Thu, 7 May 2020 07:29:46 -0700
Subject: [PATCH] strncmp: Add a testcase for page boundary [BZ #25933]

Add a strncmp testcase to cover cases where one of strings ends on the
page boundary with the maximum string length less than the number bytes
of each AVX2 loop iteration and different offsets from page boundary.

The updated string/test-strncmp fails on Intel Core i7-8559U without

ommit 1c6432316bc434a72108d7b0c7cfbfdde64c3124
Author: Sunil K Pandey <skpgkp1@gmail.com>
Date:   Fri Jun 12 08:57:16 2020 -0700

    Fix avx2 strncmp offset compare condition check [BZ #25933]
---
 string/test-strncmp.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/string/test-strncmp.c b/string/test-strncmp.c
index d961ac4493..962679b384 100644
--- a/string/test-strncmp.c
+++ b/string/test-strncmp.c
@@ -403,6 +403,38 @@ check2 (void)
   free (s2);
 }
 
+static void
+check3 (void)
+{
+  /* To trigger bug 25933, we need a size that is equal to the vector
+     length times 4. In the case of AVX2 for Intel, we need 32 * 4.  We
+     make this test generic and run it for all architectures as additional
+     boundary testing for such related algorithms.  */
+  size_t size = 32 * 4;
+  CHAR *s1 = (CHAR *) (buf1 + (BUF1PAGES - 1) * page_size);
+  CHAR *s2 = (CHAR *) (buf2 + (BUF1PAGES - 1) * page_size);
+  int exp_result;
+
+  memset (s1, 'a', page_size);
+  memset (s2, 'a', page_size);
+  s1[(page_size / CHARBYTES) - 1] = (CHAR) 0;
+
+  /* Iterate over a size that is just below where we expect the bug to
+     trigger up to the size we expect will trigger the bug e.g. [99-128].
+     Likewise iterate the start of two strings between 30 and 31 bytes
+     away from the boundary to simulate alignment changes.  */
+  for (size_t s = 99; s <= size; s++)
+    for (size_t s1a = 30; s1a < 32; s1a++)
+      for (size_t s2a = 30; s2a < 32; s2a++)
+	{
+	  CHAR *s1p = s1 + (page_size / CHARBYTES - s) - s1a;
+	  CHAR *s2p = s2 + (page_size / CHARBYTES - s) - s2a;
+	  exp_result = SIMPLE_STRNCMP (s1p, s2p, s);
+	  FOR_EACH_IMPL (impl, 0)
+	    check_result (impl, s1p, s2p, s, exp_result);
+	}
+}
+
 int
 test_main (void)
 {
@@ -412,6 +444,7 @@ test_main (void)
 
   check1 ();
   check2 ();
+  check3 ();
 
   printf ("%23s", "");
   FOR_EACH_IMPL (impl, 0)
-- 
2.26.2

Re: [PATCH 2/4] strcmp: Add a testcase for page boundary

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 2466 bytes --]

On Wed, Sep 23, 2020 at 5:46 PM Carlos O'Donell <carlos@redhat.com> wrote:
>
> On 6/12/20 4:10 PM, H.J. Lu via Libc-alpha wrote:
> > Add a strncmp testcase to cover cases where both strings end on the
> > page boundary.
> > ---
> >  string/test-strcmp.c | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> >
>
> OK to commit if you:
> - Add all the comments.
> - s1a iterates over [30,32).
>
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
>
> > diff --git a/string/test-strcmp.c b/string/test-strcmp.c
> > index 8d4784de80..41d95567c7 100644
> > --- a/string/test-strcmp.c
> > +++ b/string/test-strcmp.c
> > @@ -359,6 +359,30 @@ check (void)
> >      }
> >  }
> >
>
> Add a comment:
>
> /* To trigger bug 25933 we need a size that is equal to the
>    vector length times 4. In the case of AVX2 for Intel we
>    need 32 * 4. We make this test generic and run it for all
>    architectures as additional boundary testing for such
>    related algorithms.  */
>
> > +static void
> > +check2 (void)
> > +{
> > +  size_t size = 32 * 4;
> > +  CHAR *s1 = (CHAR *) (buf1 + (BUF1PAGES - 1) * page_size);
> > +  CHAR *s2 = (CHAR *) (buf2 + (BUF1PAGES - 1) * page_size);
> > +  int exp_result;
> > +
> > +  memset (s1, 'a', page_size);
> > +  memset (s2, 'a', page_size);
> > +  s1[(page_size / CHARBYTES) - 1] = (CHAR) 0;
> > +  s2[(page_size / CHARBYTES) - 1] = (CHAR) 0;
> > +
>
> Add comment:
>
> /* Iterate over a size that is just below where we expect
>    the bug to trigger up to the size we expect will trigger
>    the bug e.g. [99-128].  Likewise iterate the start of
>    two strings between 30 and 31 bytes away from the
>    boundary to simulate alignment changes.  */
>
> > +  for (size_t s = 99; s <= size; s++)
> > +    for (size_t s1a = 31; s1a < 32; s1a++)
>
> Please make s1a iterate over [30,32) like s2a.
>
> > +      for (size_t s2a = 30; s2a < 32; s2a++)
> > +     {
> > +       CHAR *s1p = s1 + (page_size / CHARBYTES - s) - s1a;
> > +       CHAR *s2p = s2 + (page_size / CHARBYTES - s) - s2a;
> > +       exp_result = SIMPLE_STRCMP (s1p, s2p);
> > +       FOR_EACH_IMPL (impl, 0)
> > +         check_result (impl, s1p, s2p, exp_result);
> > +     }
> > +}
> >
> >  int
> >  test_main (void)
> > @@ -367,6 +391,7 @@ test_main (void)
> >
> >    test_init ();
> >    check();
> > +  check2 ();
> >
> >    printf ("%23s", "");
> >    FOR_EACH_IMPL (impl, 0)
> >
>

This is the updated patch I am checking in.

Thanks.

-- 
H.J.

[-- Attachment #2: 0001-strcmp-Add-a-testcase-for-page-boundary.patch --]
[-- Type: text/x-patch, Size: 2043 bytes --]

From 9a76969cb4182c49a8bedd597f3436793baa4448 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Thu, 11 Jun 2020 09:03:56 -0700
Subject: [PATCH] strcmp: Add a testcase for page boundary

Add a strcmp testcase to cover cases where both strings end on the page
boundary.
---
 string/test-strcmp.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/string/test-strcmp.c b/string/test-strcmp.c
index 8d4784de80..6a840fc04b 100644
--- a/string/test-strcmp.c
+++ b/string/test-strcmp.c
@@ -359,6 +359,38 @@ check (void)
     }
 }
 
+static void
+check2 (void)
+{
+  /* To trigger bug 25933, we need a size that is equal to the vector
+     length times 4. In the case of AVX2 for Intel, we need 32 * 4.  We
+     make this test generic and run it for all architectures as additional
+     boundary testing for such related algorithms.  */
+  size_t size = 32 * 4;
+  CHAR *s1 = (CHAR *) (buf1 + (BUF1PAGES - 1) * page_size);
+  CHAR *s2 = (CHAR *) (buf2 + (BUF1PAGES - 1) * page_size);
+  int exp_result;
+
+  memset (s1, 'a', page_size);
+  memset (s2, 'a', page_size);
+  s1[(page_size / CHARBYTES) - 1] = (CHAR) 0;
+  s2[(page_size / CHARBYTES) - 1] = (CHAR) 0;
+
+  /* Iterate over a size that is just below where we expect the bug to
+     trigger up to the size we expect will trigger the bug e.g. [99-128].
+     Likewise iterate the start of two strings between 30 and 31 bytes
+     away from the boundary to simulate alignment changes.  */
+  for (size_t s = 99; s <= size; s++)
+    for (size_t s1a = 30; s1a < 32; s1a++)
+      for (size_t s2a = 30; s2a < 32; s2a++)
+	{
+	  CHAR *s1p = s1 + (page_size / CHARBYTES - s) - s1a;
+	  CHAR *s2p = s2 + (page_size / CHARBYTES - s) - s2a;
+	  exp_result = SIMPLE_STRCMP (s1p, s2p);
+	  FOR_EACH_IMPL (impl, 0)
+	    check_result (impl, s1p, s2p, exp_result);
+	}
+}
 
 int
 test_main (void)
@@ -367,6 +399,7 @@ test_main (void)
 
   test_init ();
   check();
+  check2 ();
 
   printf ("%23s", "");
   FOR_EACH_IMPL (impl, 0)
-- 
2.26.2

V2 [PATCH] bench-strncmp.c: Add workloads on page boundary

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 6407 bytes --]

On Wed, Sep 23, 2020 at 5:46 PM Carlos O'Donell <carlos@redhat.com> wrote:
>
> On 6/12/20 4:10 PM, H.J. Lu via Libc-alpha wrote:
> > Add strncmp workloads on page boundary.
>
> This benchmark need some work.
>
> Please post v2.
>
> > ---
> >  benchtests/bench-strncmp.c | 117 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 117 insertions(+)
> >
> > diff --git a/benchtests/bench-strncmp.c b/benchtests/bench-strncmp.c
> > index 95a59c9465..065c7e7789 100644
> > --- a/benchtests/bench-strncmp.c
> > +++ b/benchtests/bench-strncmp.c
> > @@ -27,6 +27,7 @@
> >
> >  #ifdef WIDE
> >  # define L(str) L##str
> > +# define STRDUP wcsdup
>
> OK.
>
> >  # define SIMPLE_STRNCMP simple_wcsncmp
> >
> >  /* Wcsncmp uses signed semantics for comparison, not unsigned.
> > @@ -48,6 +49,7 @@ simple_wcsncmp (const CHAR *s1, const CHAR *s2, size_t n)
> >
> >  #else
> >  # define L(str) str
> > +# define STRDUP strdup
>
> OK.
>
> >  # define SIMPLE_STRNCMP simple_strncmp
> >
> >  /* Strncmp uses unsigned semantics for comparison.  */
> > @@ -190,6 +192,118 @@ do_test (json_ctx_t *json_ctx, size_t align1, size_t align2, size_t len, size_t
> >    json_element_object_end (json_ctx);
> >  }
> >
> > +static void
> > +do_test_page_boundary_1 (json_ctx_t *json_ctx, CHAR *s1, CHAR *s2,
>
> Rename to "do_one_test_page_boundary"

Done.

> > +                      size_t align1, size_t align2, size_t len,
> > +                      size_t n, int exp_result)
> > +{
> > +  json_element_object_begin (json_ctx);
> > +  json_attr_uint (json_ctx, "strlen", (double) len);
> > +  json_attr_uint (json_ctx, "len", (double) n);
> > +  json_attr_uint (json_ctx, "align1", (double) align1);
> > +  json_attr_uint (json_ctx, "align2", (double) align2);
> > +  json_array_begin (json_ctx, "timings");
> > +  FOR_EACH_IMPL (impl, 0)
> > +    do_one_test (json_ctx, impl, s1, s2, n, exp_result);
> > +  json_array_end (json_ctx);
> > +  json_element_object_end (json_ctx);
>
> OK.
>
> > +}
> > +
>
>
> Add a comment:
>
> /* To trigger bug 25933 we need a size that is equal to the
>    vector length times 4. In the case of AVX2 for Intel we
>    need 32 * 4. We make this test generic and run it for all
>    architectures as additional boundary testing for such
>    related algorithms.  */

Done.

> > +static void
> > +do_test_page_boundary (json_ctx_t *json_ctx)
>
> OK.
>
> > +{
> > +  size_t size = 32 * 4;
> > +  size_t len;
> > +  CHAR *s1 = (CHAR *) (buf1 + (BUF1PAGES - 1) * page_size);
> > +  CHAR *s2 = (CHAR *) (buf2 + (BUF1PAGES - 1) * page_size);
> > +  int exp_result;
> > +
> > +  memset (s1, 'a', page_size);
> > +  memset (s2, 'a', page_size);
> > +
> > +  s1[(page_size / CHARBYTES) - 1] = (CHAR) 0;
> > +
>
> Add comment:
>
> /* Iterate over a size that is just below where we expect
>    the bug to trigger up to the size we expect will trigger
>    the bug e.g. [99-128].  Likewise iterate the start of
>    two strings between 30 and 31 bytes away from the
>    boundary to simulate alignment changes.  */

Done.

> > +  for (size_t s = 99; s <= size; s++)
> > +    for (size_t s1a = 31; s1a < 32; s1a++)
>
> Please make s1a iterate over [30,32) like s2a.

Done.

> > +      for (size_t s2a = 30; s2a < 32; s2a++)
> > +     {
> > +       size_t align1 = (page_size / CHARBYTES - s) - s1a;
> > +       size_t align2 = (page_size / CHARBYTES - s) - s2a;
> > +       CHAR *s1p = s1 + align1;
> > +       CHAR *s2p = s2 + align2;
> > +       len = (page_size / CHARBYTES) - 1 - align1;
> > +       exp_result = SIMPLE_STRNCMP (s1p, s2p, s);
> > +       do_test_page_boundary_1 (json_ctx, s1p, s2p, align1, align2,
> > +                                len, s, exp_result);
> > +     }
> > +}
> > +
> > +static void
> > +do_page_test (json_ctx_t *json_ctx, size_t offset1, size_t offset2,
>
> Rename to do_one_test_page()

Done.

> > +           CHAR *s2)
> > +{
> > +  CHAR *s1;
> > +  int exp_result;
> > +
> > +  if (offset1 * CHARBYTES  >= page_size
> > +      || offset2 * CHARBYTES >= page_size)
> > +    return;
>
> OK. Nothing bigger than a page size because we only have one page to work within.
>
> > +
> > +  s1 = (CHAR *) buf1;
> > +  s1 += offset1;
> > +  s2 += offset2;
> > +
> > +  size_t len = (page_size / CHARBYTES) - offset1;
> > +
> > +  exp_result= *s1;
> > +
> > +  json_element_object_begin (json_ctx);
> > +  json_attr_uint (json_ctx, "strlen", (double) len);
> > +  json_attr_uint (json_ctx, "len", (double) page_size);
> > +  json_attr_uint (json_ctx, "align1", (double) offset1);
> > +  json_attr_uint (json_ctx, "align2", (double) offset2);
> > +  json_array_begin (json_ctx, "timings");
> > +  {
> > +    FOR_EACH_IMPL (impl, 0)
> > +      do_one_test (json_ctx, impl, s1, s2, page_size, -exp_result);
> > +  }
> > +  json_array_end (json_ctx);
> > +  json_element_object_end (json_ctx);
> > +
> > +  json_element_object_begin (json_ctx);
> > +  json_attr_uint (json_ctx, "strlen", (double) len);
> > +  json_attr_uint (json_ctx, "len", (double) page_size);
> > +  json_attr_uint (json_ctx, "align1", (double) offset1);
> > +  json_attr_uint (json_ctx, "align2", (double) offset2);
> > +  json_array_begin (json_ctx, "timings");
> > +  {
> > +    FOR_EACH_IMPL (impl, 0)
> > +      do_one_test (json_ctx, impl, s1, s2, page_size, exp_result);
> > +  }
> > +  json_array_end (json_ctx);
> > +  json_element_object_end (json_ctx);
> > +}
>
> OK.
>
> > +
> > +static void
> > +do_test_page (json_ctx_t *json_ctx)
> > +{
> > +  size_t i;
> > +  CHAR *s1, *s2;
> > +
> > +  s1 = (CHAR *) buf1;
> > +  for (i = 0; i < (page_size / CHARBYTES) - 1; i++)
> > +    s1[i] = 23;
>
> Add a comment for this magic fill number.

Done.

> > +  s1[i] = 0;
> > +
> > +  s2 = STRDUP (s1);
> > +
> > +  for (i = 0; i < 64; ++i)
>
> Add a comment for these magic numbers.

Done.

> > +    do_page_test (json_ctx, (3988 / CHARBYTES) + i,
> > +               (2636 / CHARBYTES), s2);
>
> Call do_one_test_page()

Done.

> > +
> > +  free (s2);
> > +}
> > +
> >  int
> >  test_main (void)
> >  {
> > @@ -267,6 +381,9 @@ test_main (void)
> >        do_test_limit (&json_ctx, 0, 0, 15 - i, 16 - i, 255, -1);
> >      }
> >
> > +  do_test_page_boundary (&json_ctx);
> > +  do_test_page (&json_ctx);
>
> OK.
>
> > +
> >    json_array_end (&json_ctx);
> >    json_attr_object_end (&json_ctx);
> >    json_attr_object_end (&json_ctx);
> >
>

Here is the updated patch.  OK for master?

Thanks.

-- 
H.J.

[-- Attachment #2: 0001-bench-strncmp.c-Add-workloads-on-page-boundary.patch --]
[-- Type: text/x-patch, Size: 5343 bytes --]

From 70919738604840c387e342aaea4e6b13f8a16043 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Thu, 11 Jun 2020 08:52:42 -0700
Subject: [PATCH] bench-strncmp.c: Add workloads on page boundary

Add strncmp workloads on page boundary.
---
 benchtests/bench-strncmp.c | 128 +++++++++++++++++++++++++++++++++++++
 1 file changed, 128 insertions(+)

diff --git a/benchtests/bench-strncmp.c b/benchtests/bench-strncmp.c
index 95a59c9465..b307f0c555 100644
--- a/benchtests/bench-strncmp.c
+++ b/benchtests/bench-strncmp.c
@@ -27,6 +27,7 @@
 
 #ifdef WIDE
 # define L(str) L##str
+# define STRDUP wcsdup
 # define SIMPLE_STRNCMP simple_wcsncmp
 
 /* Wcsncmp uses signed semantics for comparison, not unsigned.
@@ -48,6 +49,7 @@ simple_wcsncmp (const CHAR *s1, const CHAR *s2, size_t n)
 
 #else
 # define L(str) str
+# define STRDUP strdup
 # define SIMPLE_STRNCMP simple_strncmp
 
 /* Strncmp uses unsigned semantics for comparison.  */
@@ -190,6 +192,129 @@ do_test (json_ctx_t *json_ctx, size_t align1, size_t align2, size_t len, size_t
   json_element_object_end (json_ctx);
 }
 
+static void
+do_one_test_page_boundary (json_ctx_t *json_ctx, CHAR *s1, CHAR *s2,
+			   size_t align1, size_t align2, size_t len,
+			   size_t n, int exp_result)
+{
+  json_element_object_begin (json_ctx);
+  json_attr_uint (json_ctx, "strlen", (double) len);
+  json_attr_uint (json_ctx, "len", (double) n);
+  json_attr_uint (json_ctx, "align1", (double) align1);
+  json_attr_uint (json_ctx, "align2", (double) align2);
+  json_array_begin (json_ctx, "timings");
+  FOR_EACH_IMPL (impl, 0)
+    do_one_test (json_ctx, impl, s1, s2, n, exp_result);
+  json_array_end (json_ctx);
+  json_element_object_end (json_ctx);
+}
+
+static void
+do_test_page_boundary (json_ctx_t *json_ctx)
+{
+  /* To trigger bug 25933, we need a size that is equal to the vector
+     length times 4. In the case of AVX2 for Intel, we need 32 * 4.  We
+     make this test generic and run it for all architectures as additional
+     boundary testing for such related algorithms.  */
+  size_t size = 32 * 4;
+  size_t len;
+  CHAR *s1 = (CHAR *) (buf1 + (BUF1PAGES - 1) * page_size);
+  CHAR *s2 = (CHAR *) (buf2 + (BUF1PAGES - 1) * page_size);
+  int exp_result;
+
+  memset (s1, 'a', page_size);
+  memset (s2, 'a', page_size);
+
+  s1[(page_size / CHARBYTES) - 1] = (CHAR) 0;
+
+  /* Iterate over a size that is just below where we expect the bug to
+     trigger up to the size we expect will trigger the bug e.g. [99-128].
+     Likewise iterate the start of two strings between 30 and 31 bytes
+     away from the boundary to simulate alignment changes.  */
+  for (size_t s = 99; s <= size; s++)
+    for (size_t s1a = 30; s1a < 32; s1a++)
+      for (size_t s2a = 30; s2a < 32; s2a++)
+	{
+	  size_t align1 = (page_size / CHARBYTES - s) - s1a;
+	  size_t align2 = (page_size / CHARBYTES - s) - s2a;
+	  CHAR *s1p = s1 + align1;
+	  CHAR *s2p = s2 + align2;
+	  len = (page_size / CHARBYTES) - 1 - align1;
+	  exp_result = SIMPLE_STRNCMP (s1p, s2p, s);
+	  do_one_test_page_boundary (json_ctx, s1p, s2p, align1, align2,
+				     len, s, exp_result);
+	}
+}
+
+static void
+do_one_test_page (json_ctx_t *json_ctx, size_t offset1, size_t offset2,
+		  CHAR *s2)
+{
+  CHAR *s1;
+  int exp_result;
+
+  if (offset1 * CHARBYTES  >= page_size
+      || offset2 * CHARBYTES >= page_size)
+    return;
+
+  s1 = (CHAR *) buf1;
+  s1 += offset1;
+  s2 += offset2;
+
+  size_t len = (page_size / CHARBYTES) - offset1;
+
+  exp_result= *s1;
+
+  json_element_object_begin (json_ctx);
+  json_attr_uint (json_ctx, "strlen", (double) len);
+  json_attr_uint (json_ctx, "len", (double) page_size);
+  json_attr_uint (json_ctx, "align1", (double) offset1);
+  json_attr_uint (json_ctx, "align2", (double) offset2);
+  json_array_begin (json_ctx, "timings");
+  {
+    FOR_EACH_IMPL (impl, 0)
+      do_one_test (json_ctx, impl, s1, s2, page_size, -exp_result);
+  }
+  json_array_end (json_ctx);
+  json_element_object_end (json_ctx);
+
+  json_element_object_begin (json_ctx);
+  json_attr_uint (json_ctx, "strlen", (double) len);
+  json_attr_uint (json_ctx, "len", (double) page_size);
+  json_attr_uint (json_ctx, "align1", (double) offset1);
+  json_attr_uint (json_ctx, "align2", (double) offset2);
+  json_array_begin (json_ctx, "timings");
+  {
+    FOR_EACH_IMPL (impl, 0)
+      do_one_test (json_ctx, impl, s1, s2, page_size, exp_result);
+  }
+  json_array_end (json_ctx);
+  json_element_object_end (json_ctx);
+}
+
+static void
+do_test_page (json_ctx_t *json_ctx)
+{
+  size_t i;
+  CHAR *s1, *s2;
+
+  s1 = (CHAR *) buf1;
+  /* Fill buf1 with 23. */
+  for (i = 0; i < (page_size / CHARBYTES) - 1; i++)
+    s1[i] = 23;
+  s1[i] = 0;
+
+  /* Make a copy of buf1.  */
+  s2 = STRDUP (s1);
+
+  /* Test should terminate within the page boundary.  */
+  for (i = 0; i < (108 / CHARBYTES); ++i)
+    do_one_test_page (json_ctx, ((page_size - 108) / CHARBYTES) + i,
+		      ((page_size - 1460) / CHARBYTES), s2);
+
+  free (s2);
+}
+
 int
 test_main (void)
 {
@@ -267,6 +392,9 @@ test_main (void)
       do_test_limit (&json_ctx, 0, 0, 15 - i, 16 - i, 255, -1);
     }
 
+  do_test_page_boundary (&json_ctx);
+  do_test_page (&json_ctx);
+
   json_array_end (&json_ctx);
   json_attr_object_end (&json_ctx);
   json_attr_object_end (&json_ctx);
-- 
2.26.2

V2 [PATCH] bench-strcmp.c: Add workloads on page boundary

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 4154 bytes --]

On Wed, Sep 23, 2020 at 5:52 PM Carlos O'Donell <carlos@redhat.com> wrote:
>
> On 6/12/20 4:10 PM, H.J. Lu via Libc-alpha wrote:
> > Add strcmp workloads on page boundary.
> > ---
> >  benchtests/bench-strcmp.c | 48 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 48 insertions(+)
> >
>
> These additions to the benchmark exercise the page boundary conditions
> in the algorithms implemented by all the architectures. The test is
> designed from the experience with bug 25933 in mind so we should mention
> that in a comment. It could be made a bit more generic by parametrizing
> vector sizes and number of vector registers that might be grouped e.g.
> 32 * 4, but it's fine as it is for now to test the page boundary.
>
> OK with:
> - Function rename
> - Added comments.
> - s1a iterate over [30,21)
>
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
>
> > diff --git a/benchtests/bench-strcmp.c b/benchtests/bench-strcmp.c
> > index 47d0a35299..3ba1399a4d 100644
> > --- a/benchtests/bench-strcmp.c
> > +++ b/benchtests/bench-strcmp.c
> > @@ -144,6 +144,52 @@ do_test (json_ctx_t *json_ctx, size_t align1, size_t align2, size_t len, int
> >    json_element_object_end (json_ctx);
> >  }
> >
> > +static void
> > +do_test_page_boundary_1 (json_ctx_t *json_ctx, CHAR *s1, CHAR *s2,
>
> Rename to do_one_test_page_boundary ()

Done.

> > +                      size_t align1, size_t align2, size_t len,
> > +                      int exp_result)
> > +{
> > +  json_element_object_begin (json_ctx);
> > +  json_attr_uint (json_ctx, "length", (double) len);
> > +  json_attr_uint (json_ctx, "align1", (double) align1);
> > +  json_attr_uint (json_ctx, "align2", (double) align2);
> > +  json_array_begin (json_ctx, "timings");
> > +  FOR_EACH_IMPL (impl, 0)
> > +    do_one_test (json_ctx, impl, s1, s2, exp_result);
>
> OK.
>
> > +  json_array_end (json_ctx);
> > +  json_element_object_end (json_ctx);
> > +}
> > +
>
> Add a comment:
>
> /* To trigger bug 25933 we need a size that is equal to the
>    vector length times 4. In the case of AVX2 for Intel we
>    need 32 * 4. We make this test generic and run it for all
>    architectures as additional boundary testing for such
>    related algorithms.  */

Done.

> > +static void
> > +do_test_page_boundary (json_ctx_t *json_ctx)
> > +{
> > +  size_t size = 32 * 4;
> > +  size_t len;
> > +  CHAR *s1 = (CHAR *) (buf1 + (BUF1PAGES - 1) * page_size);
> > +  CHAR *s2 = (CHAR *) (buf2 + (BUF1PAGES - 1) * page_size);
> > +  int exp_result;
> > +
> > +  memset (s1, 'a', page_size);
> > +  memset (s2, 'a', page_size);
> > +
> > +  s1[(page_size / CHARBYTES) - 1] = (CHAR) 0;
> > +  s2[(page_size / CHARBYTES) - 1] = (CHAR) 0;
> > +
>
>
> Add comment:
>
> /* Iterate over a size that is just below where we expect
>    the bug to trigger up to the size we expect will trigger
>    the bug e.g. [99-128].  Likewise iterate the start of
>    two strings between 30 and 31 bytes away from the
>    boundary to simulate alignment changes.  */

Done.

> > +  for (size_t s = 99; s <= size; s++)
> > +    for (size_t s1a = 31; s1a < 32; s1a++)
>
> Make s1a iterate over [30,32) like s2a.

Done.

> > +      for (size_t s2a = 30; s2a < 32; s2a++)
> > +     {
> > +       size_t align1 = (page_size / CHARBYTES - s) - s1a;
> > +       size_t align2 = (page_size / CHARBYTES - s) - s2a;
> > +       CHAR *s1p = s1 + align1;
> > +       CHAR *s2p = s2 + align2;
> > +       len = (page_size / CHARBYTES) - 1 - align1;
> > +       exp_result = SIMPLE_STRCMP (s1p, s2p);
> > +       do_test_page_boundary_1 (json_ctx, s1p, s2p, align1, align2,
>
> Call do_one_test_page_boundary ()

Done.

> > +                                len, exp_result);
> > +     }
> > +}
> > +
> >  int
> >  test_main (void)
> >  {
> > @@ -197,6 +243,8 @@ test_main (void)
> >        do_test (&json_ctx, 2 * CHARBYTES * i, CHARBYTES * i, 8 << i, LARGECHAR, -1);
> >      }
> >
> > +  do_test_page_boundary (&json_ctx);
>
> OK.
>
> > +
> >    json_array_end (&json_ctx);
> >    json_attr_object_end (&json_ctx);
> >    json_attr_object_end (&json_ctx);
> >
>
>

Here is the updated patch.  OK for master?

Thanks.

-- 
H.J.

[-- Attachment #2: 0001-bench-strcmp.c-Add-workloads-on-page-boundary.patch --]
[-- Type: text/x-patch, Size: 3013 bytes --]

From eacae3fa86051ae3433dabf1323e809450ce3cc8 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Thu, 11 Jun 2020 09:00:12 -0700
Subject: [PATCH] bench-strcmp.c: Add workloads on page boundary

Add strcmp workloads on page boundary.
---
 benchtests/bench-strcmp.c | 56 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/benchtests/bench-strcmp.c b/benchtests/bench-strcmp.c
index 47d0a35299..c6f8446978 100644
--- a/benchtests/bench-strcmp.c
+++ b/benchtests/bench-strcmp.c
@@ -144,6 +144,60 @@ do_test (json_ctx_t *json_ctx, size_t align1, size_t align2, size_t len, int
   json_element_object_end (json_ctx);
 }
 
+static void
+do_one_test_page_boundary (json_ctx_t *json_ctx, CHAR *s1, CHAR *s2,
+			   size_t align1, size_t align2, size_t len,
+			   int exp_result)
+{
+  json_element_object_begin (json_ctx);
+  json_attr_uint (json_ctx, "length", (double) len);
+  json_attr_uint (json_ctx, "align1", (double) align1);
+  json_attr_uint (json_ctx, "align2", (double) align2);
+  json_array_begin (json_ctx, "timings");
+  FOR_EACH_IMPL (impl, 0)
+    do_one_test (json_ctx, impl, s1, s2, exp_result);
+  json_array_end (json_ctx);
+  json_element_object_end (json_ctx);
+}
+
+static void
+do_test_page_boundary (json_ctx_t *json_ctx)
+{
+  /* To trigger bug 25933, we need a size that is equal to the vector
+     length times 4. In the case of AVX2 for Intel, we need 32 * 4.  We
+     make this test generic and run it for all architectures as additional
+     boundary testing for such related algorithms.  */
+  size_t size = 32 * 4;
+  size_t len;
+  CHAR *s1 = (CHAR *) (buf1 + (BUF1PAGES - 1) * page_size);
+  CHAR *s2 = (CHAR *) (buf2 + (BUF1PAGES - 1) * page_size);
+  int exp_result;
+
+  memset (s1, 'a', page_size);
+  memset (s2, 'a', page_size);
+
+  s1[(page_size / CHARBYTES) - 1] = (CHAR) 0;
+  s2[(page_size / CHARBYTES) - 1] = (CHAR) 0;
+
+  /* Iterate over a size that is just below where we expect the bug to
+     trigger up to the size we expect will trigger the bug e.g. [99-128].
+     Likewise iterate the start of two strings between 30 and 31 bytes
+     away from the boundary to simulate alignment changes.  */
+  for (size_t s = 99; s <= size; s++)
+    for (size_t s1a = 30; s1a < 32; s1a++)
+      for (size_t s2a = 30; s2a < 32; s2a++)
+	{
+	  size_t align1 = (page_size / CHARBYTES - s) - s1a;
+	  size_t align2 = (page_size / CHARBYTES - s) - s2a;
+	  CHAR *s1p = s1 + align1;
+	  CHAR *s2p = s2 + align2;
+	  len = (page_size / CHARBYTES) - 1 - align1;
+	  exp_result = SIMPLE_STRCMP (s1p, s2p);
+	  do_one_test_page_boundary (json_ctx, s1p, s2p, align1, align2,
+				     len, exp_result);
+	}
+}
+
 int
 test_main (void)
 {
@@ -197,6 +251,8 @@ test_main (void)
       do_test (&json_ctx, 2 * CHARBYTES * i, CHARBYTES * i, 8 << i, LARGECHAR, -1);
     }
 
+  do_test_page_boundary (&json_ctx);
+
   json_array_end (&json_ctx);
   json_attr_object_end (&json_ctx);
   json_attr_object_end (&json_ctx);
-- 
2.26.2

Re: V2 [PATCH] bench-strncmp.c: Add workloads on page boundary

[Carlos O'Donell]

On 9/24/20 11:13 AM, H.J. Lu wrote:
> Here is the updated patch.  OK for master?

Is OK for master. Though my preference is for more verbosity on the
exactly numbers used in do_test_page() e.g. why 108 or 1460.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

-- 
Cheers,
Carlos.

Re: V2 [PATCH] bench-strcmp.c: Add workloads on page boundary

[Carlos O'Donell]

On 9/24/20 11:22 AM, H.J. Lu wrote:
> Here is the updated patch.  OK for master?

OK for master.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

-- 
Cheers,
Carlos.