diff mbox series

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

Message ID 20200612201056.228614-1-hjl.tools@gmail.com
State New
Headers show
Series [1/4] strncmp: Add a testcase for page boundary [BZ #25933] | expand

Commit Message

H.J. Lu June 12, 2020, 8:10 p.m. UTC
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(+)

Comments

Paul A. Clarke June 15, 2020, 8:29 p.m. UTC | #1
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
>
H.J. Lu June 15, 2020, 9:34 p.m. UTC | #2
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
> >
Paul A. Clarke June 15, 2020, 10:03 p.m. UTC | #3
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
diff mbox series

Patch

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)