[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]
Related show

Commit Message

Adhemerval Zanella via Libc-alpha June 12, 2020, 8:10 p.m.
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(+)

-- 
2.26.2

Comments

Adhemerval Zanella via Libc-alpha June 15, 2020, 8:29 p.m. | #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

>
Adhemerval Zanella via Libc-alpha June 15, 2020, 9:34 p.m. | #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

> >




-- 
H.J.
Adhemerval Zanella via Libc-alpha June 15, 2020, 10:03 p.m. | #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

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)