diff mbox series

[v2] string: Add page tests for test-strncasecmp and test-strncpy

Message ID 20200603200645.478668-1-rzinsly@linux.ibm.com
State Superseded
Headers show
Series [v2] string: Add page tests for test-strncasecmp and test-strncpy | expand

Commit Message

Raphael M Zinsly June 3, 2020, 8:06 p.m. UTC
Adds tests to check if strings placed at page bondaries are
handled correctly by strncasecmp and strncpy similar to tests
for strncmp and strnlen.  This should catch regressions in the
optmized functions.
---
 string/test-strncasecmp.c | 37 +++++++++++++++++++++++++++++++++++++
 string/test-strncpy.c     | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+)

Comments

Paul A. Clarke June 3, 2020, 9:48 p.m. UTC | #1
On Wed, Jun 03, 2020 at 05:06:45PM -0300, Raphael Moreira Zinsly wrote:
> Adds tests to check if strings placed at page bondaries are
nit: Add tests
nit: boundaries

> handled correctly by strncasecmp and strncpy similar to tests
> for strncmp and strnlen.  This should catch regressions in the
> optmized functions.
nit: optimized
...but, you can delete this sentence.

> ---
>  string/test-strncasecmp.c | 37 +++++++++++++++++++++++++++++++++++++
>  string/test-strncpy.c     | 35 +++++++++++++++++++++++++++++++++++
>  2 files changed, 72 insertions(+)
> 
> diff --git a/string/test-strncasecmp.c b/string/test-strncasecmp.c
> index 6a9c27beae..bf9838dacb 100644
> --- a/string/test-strncasecmp.c
> +++ b/string/test-strncasecmp.c
> @@ -137,6 +137,42 @@ do_test (size_t align1, size_t align2, size_t n, size_t len, int max_char,
>      do_one_test (impl, s1, s2, n, exp_result);
>  }
> 
> +static void
> +do_page_tests (void)
> +{
> +  size_t i, offset, cacheline_size;
> +  char *s1, *s2;
> +  int exp_result;
> +
> +  offset = page_size - 1;
> +  s1 = (char *) buf1;
> +  memset (s1, '\1', offset);
> +  s1[offset] = '\0';
> +
> +  /* s2 has a fixed offset, almost one page long.
> +     page_size is actually 2 * getpagesize.  */
> +  offset = (page_size / 2) - 10;
> +  s2 = strdup (s1) + offset;

s2 is fixed at 10 bytes short of a page boundary...

> +  /* Start offset for s1.  */
> +  offset = 3 * page_size / 4;
> +  s1 += offset;

s1 starts from midway through the 2nd page...

I misspoke in my earlier review. To start midway through the first
page (of the two pages), it should be page_size / 4.
I was originally thinking 3/4 into the first actual page, which would
actually be 3 * page_size / 8.  That "page_size" is actually two pages
is quite confusing.

> +
> +  /* Try to cross the page boundary at every offset of a cache line.  */
> +  cacheline_size = sysconf (_SC_LEVEL1_DCACHE_LINESIZE);
> +  for (i = 0; i < cacheline_size; ++i)
> +    {
> +      exp_result = *s1;
> +
> +      FOR_EACH_IMPL (impl, 0)
> +	{
> +	  check_result (impl, s1, s2, page_size, -exp_result);
> +	  check_result (impl, s2, s1, page_size, exp_result);
> +	}
> +
> +      s1++;

s1 moves a byte at a time for the length of a cacheline.

It seems that s2 will always cross the boundary at the same offset (10),
and s1 will never cross a page boundary, so you should adjust s1 above
to be somewhere in the middle of the first page.

> +    }
> +}
> +
>  static void
>  do_random_tests (void)
>  {
> @@ -334,6 +370,7 @@ test_locale (const char *locale)
>      }
> 
>    do_random_tests ();
> +  do_page_tests ();
>  }
> 
>  int
> diff --git a/string/test-strncpy.c b/string/test-strncpy.c
> index c978753ad8..fb3332cbac 100644
> --- a/string/test-strncpy.c
> +++ b/string/test-strncpy.c
> @@ -155,6 +155,40 @@ do_test (size_t align1, size_t align2, size_t len, size_t n, int max_char)
>      do_one_test (impl, s2, s1, len, n);
>  }
> 
> +static void
> +do_page_tests (void)
> +{
> +  size_t i, small_len, big_len, short_offset, long_offset;
> +  CHAR *s1, *s2;
> +  /* Calculate the null character offset.  */
> +  size_t last_offset = (page_size / sizeof (CHAR)) - 1;
> +
> +  s2 = (CHAR *) buf1;
> +  s1 = (CHAR *) buf2;
> +  MEMSET (s1, '\1', last_offset);
> +  s1[last_offset] = '\0';
> +
> +  long_offset = (last_offset + 1) / 2;
> +  short_offset = last_offset;
> +  for (i = 0; i < 128; i++)

Consider a comment before this line like what you added for
test-strncasecmp, above:
   /* Try to cross the page boundary at every offset of a cache line.  */
...which then prompts the question as to whether you should
query and use the cache line size as you did there. Probably yes.

> +    {
> +      /* Place long strings ending at page boundary.  */
> +      long_offset++;
> +      big_len = last_offset - long_offset;
> +      /* Place short strings ending at page boundary.  */
> +      short_offset--;
> +      small_len = last_offset - short_offset;
> +
> +      FOR_EACH_IMPL (impl, 0)
> +        {
> +	  do_one_test (impl, s2, (CHAR *) (s1 + short_offset), small_len,
> +	               small_len);
> +	  do_one_test (impl, s2, (CHAR *) (s1 + long_offset), page_size,
> +	               big_len);
> +	}
> +    }
> +}
> +
>  static void
>  do_random_tests (void)
>  {
> @@ -317,6 +351,7 @@ test_main (void)
>      }
> 
>    do_random_tests ();
> +  do_page_tests ();
>    return ret;
>  }

PC
diff mbox series

Patch

diff --git a/string/test-strncasecmp.c b/string/test-strncasecmp.c
index 6a9c27beae..bf9838dacb 100644
--- a/string/test-strncasecmp.c
+++ b/string/test-strncasecmp.c
@@ -137,6 +137,42 @@  do_test (size_t align1, size_t align2, size_t n, size_t len, int max_char,
     do_one_test (impl, s1, s2, n, exp_result);
 }
 
+static void
+do_page_tests (void)
+{
+  size_t i, offset, cacheline_size;
+  char *s1, *s2;
+  int exp_result;
+
+  offset = page_size - 1;
+  s1 = (char *) buf1;
+  memset (s1, '\1', offset);
+  s1[offset] = '\0';
+
+  /* s2 has a fixed offset, almost one page long.
+     page_size is actually 2 * getpagesize.  */
+  offset = (page_size / 2) - 10;
+  s2 = strdup (s1) + offset;
+  /* Start offset for s1.  */
+  offset = 3 * page_size / 4;
+  s1 += offset;
+
+  /* Try to cross the page boundary at every offset of a cache line.  */
+  cacheline_size = sysconf (_SC_LEVEL1_DCACHE_LINESIZE);
+  for (i = 0; i < cacheline_size; ++i)
+    {
+      exp_result = *s1;
+
+      FOR_EACH_IMPL (impl, 0)
+	{
+	  check_result (impl, s1, s2, page_size, -exp_result);
+	  check_result (impl, s2, s1, page_size, exp_result);
+	}
+
+      s1++;
+    }
+}
+
 static void
 do_random_tests (void)
 {
@@ -334,6 +370,7 @@  test_locale (const char *locale)
     }
 
   do_random_tests ();
+  do_page_tests ();
 }
 
 int
diff --git a/string/test-strncpy.c b/string/test-strncpy.c
index c978753ad8..fb3332cbac 100644
--- a/string/test-strncpy.c
+++ b/string/test-strncpy.c
@@ -155,6 +155,40 @@  do_test (size_t align1, size_t align2, size_t len, size_t n, int max_char)
     do_one_test (impl, s2, s1, len, n);
 }
 
+static void
+do_page_tests (void)
+{
+  size_t i, small_len, big_len, short_offset, long_offset;
+  CHAR *s1, *s2;
+  /* Calculate the null character offset.  */
+  size_t last_offset = (page_size / sizeof (CHAR)) - 1;
+
+  s2 = (CHAR *) buf1;
+  s1 = (CHAR *) buf2;
+  MEMSET (s1, '\1', last_offset);
+  s1[last_offset] = '\0';
+
+  long_offset = (last_offset + 1) / 2;
+  short_offset = last_offset;
+  for (i = 0; i < 128; i++)
+    {
+      /* Place long strings ending at page boundary.  */
+      long_offset++;
+      big_len = last_offset - long_offset;
+      /* Place short strings ending at page boundary.  */
+      short_offset--;
+      small_len = last_offset - short_offset;
+
+      FOR_EACH_IMPL (impl, 0)
+        {
+	  do_one_test (impl, s2, (CHAR *) (s1 + short_offset), small_len,
+	               small_len);
+	  do_one_test (impl, s2, (CHAR *) (s1 + long_offset), page_size,
+	               big_len);
+	}
+    }
+}
+
 static void
 do_random_tests (void)
 {
@@ -317,6 +351,7 @@  test_main (void)
     }
 
   do_random_tests ();
+  do_page_tests ();
   return ret;
 }