string: Add page tests for test-strncasecmp and test-strncpy

  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 | 39 +++++++++++++++++++++++++++++++++++++++
 string/test-strncpy.c     | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+)
  

On Fri, May 29, 2020 at 10:53:05AM -0300, Raphael Moreira Zinsly via Libc-alpha wrote:
> 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.
> ---
[snip]
> diff --git a/string/test-strncasecmp.c b/string/test-strncasecmp.c
> index 6a9c27beae..0399f37117 100644
> --- a/string/test-strncasecmp.c
> +++ b/string/test-strncasecmp.c
> @@ -137,6 +137,44 @@ do_test (size_t align1, size_t align2, size_t n, size_t len, int max_char,

These tests could use some comments, since there seems to be some
arbitrary choices, making it difficult to determine what is being
tested...

> +static void
> +do_page_tests (void)
> +{
> +  size_t i, offset1, offset2;
> +  char *s1, *s2, *s3;
> +  int exp_result;
> +
> +  s1 = (char *) buf1;
> +  for (i = 0; i < page_size - 1; i++)
> +    s1[i] = 23;

Magic number. Does this 23 have any significance?

> +  s1[i] = 0;
> +
> +  s3 = strdup (s1);
> +  offset2 = 2636;

Magic number.  How was it chosen and why?

> +
> +  for (i = 0; i < 64; ++i)

Does 64 have some signficance?
It might be a cache line length, in that you are trying to cross
the page boundary at every offset in a cache line?  And if that's
the case, maybe 128 would be better, since it covers more
architectures?  (Or, maybe even query the cache line size?)

> +    {
> +      offset1 = 3988 + i;

Another magic number.

> +
> +      if (offset1 >= page_size
> +	  || offset2 >= page_size)
> +	return;

Maybe a comment before this stating something like
"make sure we don't run past the end of the buffer"?

Also, offset2 is fixed, so that part of this test need
not be in the loop.  And, if you pick a good offset2,
as I suggest below, there is no need to test it at all.
If the constant to which offset2 is set just happens to
be greater than page_size, then this entire test does
nothing.

> +
> +      s1 = (char *) buf1;
> +      s2 = s3;

Maybe a comment "reset pointers to base addresses" or the like?

> +      s1 += offset1;
> +      s2 += offset2;

So, offset1 ranges from [3988..3988+63] ? offset2 is fixed at 2636.
Since offset2 is fixed, why not just set "s3 = s2 + offset2"
before the loop and then this increment is unnecessary.

Maybe we need to note somewhere that "page_size" is actually
*2* pages and that doing a strncasecmp from an offset which is
less than a full page to page_size is guaranteed to cross a page
boundary.
It might be a good idea to write code that ensures that the
offsets are both indeed less than a single page size.
Something like "offset2 = page_size / 4" and change the
3988 to something relative to page_size ("3 * page_size / 4"?).

> +
> +      exp_result= *s1;

strncasecmp man page says it returns "an integer less than, equal to,
or greater than zero".  It doesn't seem to be appropriate to look for
a specific value.

> +
> +      FOR_EACH_IMPL (impl, 0)
> +	{
> +	  check_result (impl, s1, s2, page_size, -exp_result);
> +	  check_result (impl, s2, s1, page_size, exp_result);
> +	}
> +    }
> +}
[snip]
> diff --git a/string/test-strncpy.c b/string/test-strncpy.c
> index c978753ad8..30c69dd34b 100644
> --- a/string/test-strncpy.c
> +++ b/string/test-strncpy.c
> @@ -155,6 +155,42 @@ do_test (size_t align1, size_t align2, size_t len, size_t n, int max_char)
[snip]
> +static void
> +do_page_tests (void)
> +{
> +  size_t i, len, start_offset, 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';
> +
> +  /* Place short strings ending at page boundary.  */
> +  offset = last_offset;
> +  for (i = 0; i < 128; i++)

Is 128 enough? Why was this chosen?

> +    {
> +      offset--;
> +      len = last_offset - offset;
> +      FOR_EACH_IMPL (impl, 0)
> +	do_one_test (impl, s2, (CHAR *) (s1 + offset), len, len);
> +    }
> +
> +  /* Place long strings ending at page boundary.  */
> +  start_offset = (last_offset + 1) / 2;
> +  for (i = 0; i < 64; ++i)

Is 64 enough? Why was this chosen?

> +    {
> +      offset = start_offset + i;
> +      if (offset >= (last_offset + 1))
> +	break;
> +      len = last_offset - offset;
> +      FOR_EACH_IMPL (impl, 0)
> +	do_one_test (impl, s2, (CHAR *) (s1 + offset), page_size, len);
> +    }

Would it be bad to collapse these 2 loops into one that tests every
length from 1..page_size?

> +}
[snip]

PC
  

Thanks for the review, it helped me find some bugs, I'll fix it and add 
more comments to the code:

On 29/05/2020 18:13, Paul A. Clarke wrote:
> On Fri, May 29, 2020 at 10:53:05AM -0300, Raphael Moreira Zinsly via Libc-alpha wrote:
>> 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.
>> ---
> [snip]
>> diff --git a/string/test-strncasecmp.c b/string/test-strncasecmp.c
>> index 6a9c27beae..0399f37117 100644
>> --- a/string/test-strncasecmp.c
>> +++ b/string/test-strncasecmp.c
>> @@ -137,6 +137,44 @@ do_test (size_t align1, size_t align2, size_t n, size_t len, int max_char,
> 
> These tests could use some comments, since there seems to be some
> arbitrary choices, making it difficult to determine what is being
> tested...
> 
>> +static void
>> +do_page_tests (void)
>> +{
>> +  size_t i, offset1, offset2;
>> +  char *s1, *s2, *s3;
>> +  int exp_result;
>> +
>> +  s1 = (char *) buf1;
>> +  for (i = 0; i < page_size - 1; i++)
>> +    s1[i] = 23;
> 
> Magic number. Does this 23 have any significance?

No, but it actually would be better to use memset and '\1' as I did for 
the strncpy test.

> 
>> +  s1[i] = 0;
>> +
>> +  s3 = strdup (s1);
>> +  offset2 = 2636;
> 
> Magic number.  How was it chosen and why?

These offsets were picked from the strncmp test, that came originally 
from https://sourceware.org/bugzilla/show_bug.cgi?id=12597

> 
>> +
>> +  for (i = 0; i < 64; ++i)
> 
> Does 64 have some signficance?
> It might be a cache line length, in that you are trying to cross
> the page boundary at every offset in a cache line?  And if that's
> the case, maybe 128 would be better, since it covers more
> architectures?  (Or, maybe even query the cache line size?)

Yes, I liked the suggestion of using 128.

> 
>> +    {
>> +      offset1 = 3988 + i;
> 
> Another magic number >
>> +
>> +      if (offset1 >= page_size
>> +	  || offset2 >= page_size)
>> +	return;
> 
> Maybe a comment before this stating something like
> "make sure we don't run past the end of the buffer"?
> 
> Also, offset2 is fixed, so that part of this test need
> not be in the loop.  And, if you pick a good offset2,
> as I suggest below, there is no need to test it at all.
> If the constant to which offset2 is set just happens to
> be greater than page_size, then this entire test does
> nothing.

There is a bug here, it should be "continue" instead of return.

> 
>> +
>> +      s1 = (char *) buf1;
>> +      s2 = s3;
> 
> Maybe a comment "reset pointers to base addresses" or the like?
> 
>> +      s1 += offset1;
>> +      s2 += offset2;
> 
> So, offset1 ranges from [3988..3988+63] ? offset2 is fixed at 2636.
> Since offset2 is fixed, why not just set "s3 = s2 + offset2"
> before the loop and then this increment is unnecessary.

Agreed, thanks for the suggestion.

> 
> Maybe we need to note somewhere that "page_size" is actually
> *2* pages and that doing a strncasecmp from an offset which is
> less than a full page to page_size is guaranteed to cross a page
> boundary.
> It might be a good idea to write code that ensures that the
> offsets are both indeed less than a single page size.
> Something like "offset2 = page_size / 4" and change the
> 3988 to something relative to page_size ("3 * page_size / 4"?).
> 

Agreed.

>> +
>> +      exp_result= *s1;
> 
> strncasecmp man page says it returns "an integer less than, equal to,
> or greater than zero".  It doesn't seem to be appropriate to look for
> a specific value.

This is a valid point, but all other functions besides do_random_tests 
look for a specific value, I'm just keeping the behavior.

> 
>> +
>> +      FOR_EACH_IMPL (impl, 0)
>> +	{
>> +	  check_result (impl, s1, s2, page_size, -exp_result);
>> +	  check_result (impl, s2, s1, page_size, exp_result);
>> +	}
>> +    }
>> +}
> [snip]
>> diff --git a/string/test-strncpy.c b/string/test-strncpy.c
>> index c978753ad8..30c69dd34b 100644
>> --- a/string/test-strncpy.c
>> +++ b/string/test-strncpy.c
>> @@ -155,6 +155,42 @@ do_test (size_t align1, size_t align2, size_t len, size_t n, int max_char)
> [snip]
>> +static void
>> +do_page_tests (void)
>> +{
>> +  size_t i, len, start_offset, 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';
>> +
>> +  /* Place short strings ending at page boundary.  */
>> +  offset = last_offset;
>> +  for (i = 0; i < 128; i++)
> 
> Is 128 enough? Why was this chosen?

It should be SMALL_CHAR.

> 
>> +    {
>> +      offset--;
>> +      len = last_offset - offset;
>> +      FOR_EACH_IMPL (impl, 0)
>> +	do_one_test (impl, s2, (CHAR *) (s1 + offset), len, len);
>> +    }
>> +
>> +  /* Place long strings ending at page boundary.  */
>> +  start_offset = (last_offset + 1) / 2;
>> +  for (i = 0; i < 64; ++i)
> 
> Is 64 enough? Why was this chosen?

This was intended to test each alignment in a cache line, it could use 
128 to cover more architectures as suggested for test-strncacecmp.

> 
>> +    {
>> +      offset = start_offset + i;
>> +      if (offset >= (last_offset + 1))
>> +	break;
>> +      len = last_offset - offset;
>> +      FOR_EACH_IMPL (impl, 0)
>> +	do_one_test (impl, s2, (CHAR *) (s1 + offset), page_size, len);
>> +    }
> 
> Would it be bad to collapse these 2 loops into one that tests every
> length from 1..page_size?

We could collapse both into one loop with 128 iterations, I don't think 
using page_size would be bad but we would cover many alignments that are 
already covered by other tests.

> 
>> +}
> [snip]
> 
> PC
> 

Best regards,
  

Hi Raphael, Thanks for the patch :)

Overall it will be best if there was some comments for the magic numbers and
test functions. I think Paul point that. 

Quoting Raphael Moreira Zinsly via Libc-alpha (2020-05-29 10:53:05)
> 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 | 39 +++++++++++++++++++++++++++++++++++++++
>  string/test-strncpy.c     | 37 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 76 insertions(+)
> 
> diff --git a/string/test-strncasecmp.c b/string/test-strncasecmp.c
> index 6a9c27beae..0399f37117 100644
> --- a/string/test-strncasecmp.c
> +++ b/string/test-strncasecmp.c
> @@ -137,6 +137,44 @@ 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, offset1, offset2;
> +  char *s1, *s2, *s3;
> +  int exp_result;
> +
> +  s1 = (char *) buf1;

What is the size of buf1 1 page_size?

> +  for (i = 0; i < page_size - 1; i++)
> +    s1[i] = 23;
> +  s1[i] = 0;
> +
> +  s3 = strdup (s1);
> +  offset2 = 2636;
> +
> +  for (i = 0; i < 64; ++i)
> +    {
> +      offset1 = 3988 + i;
> +
The max size of offset1 is 4052.  AFAIK this will never cross a page. So
the check bellow is not necessarie.
> +      if (offset1 >= page_size
> +         || offset2 >= page_size)
> +       return;

Why do you check offset2 here?  As it never increments you could check it
outside of the loop. But AFAIK 2636 will never cross a page.
> +
> +      s1 = (char *) buf1;
I notice that o the other function you use (CHAR *) instead of (char *)
> +      s2 = s3;
> +      s1 += offset1;

Well, a sum is a sum. But Why don't use a simpler for like:

s1 = magic_number1 + buf1
s2 = magic_number2 + s3

for (i = 0; i < 64; ++i){
	s1++;
	s2++;
	do test
{

> +      s2 += offset2;
> +
> +      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);
> +       }
> +    }
> +}
> +
>  static void
>  do_random_tests (void)
>  {
> @@ -334,6 +372,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..30c69dd34b 100644
> --- a/string/test-strncpy.c
> +++ b/string/test-strncpy.c
> @@ -155,6 +155,42 @@ 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, len, start_offset, 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';
Ok.

> +
> +  /* Place short strings ending at page boundary.  */
> +  offset = last_offset;
> +  for (i = 0; i < 128; i++)
> +    {
> +      offset--;
> +      len = last_offset - offset;
> +      FOR_EACH_IMPL (impl, 0)
> +       do_one_test (impl, s2, (CHAR *) (s1 + offset), len, len);
> +    }
Ok.
> +
> +  /* Place long strings ending at page boundary.  */
> +  start_offset = (last_offset + 1) / 2;
> +  for (i = 0; i < 64; ++i)
> +    {
> +      offset = start_offset + i;
> +      if (offset >= (last_offset + 1))
> +       break;

Is this possible?

> +      len = last_offset - offset;
> +      FOR_EACH_IMPL (impl, 0)
> +       do_one_test (impl, s2, (CHAR *) (s1 + offset), page_size, len);
> +    }
> +}
> +
>  static void
>  do_random_tests (void)
>  {
> @@ -317,6 +353,7 @@ test_main (void)
>      }
>  
>    do_random_tests ();
> +  do_page_tests ();
>    return ret;
>  }
>  
> -- 
> 2.26.2
> 
Ok.

---
Lucas A. M. Magalhães
  

Hi Lucas,

On 02/06/2020 09:58, Lucas A. M. Magalhaes wrote:
> Hi Raphael, Thanks for the patch :)
> 
> Overall it will be best if there was some comments for the magic numbers and
> test functions. I think Paul point that.
> 
> Quoting Raphael Moreira Zinsly via Libc-alpha (2020-05-29 10:53:05)
>> 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 | 39 +++++++++++++++++++++++++++++++++++++++
>>   string/test-strncpy.c     | 37 +++++++++++++++++++++++++++++++++++++
>>   2 files changed, 76 insertions(+)
>>
>> diff --git a/string/test-strncasecmp.c b/string/test-strncasecmp.c
>> index 6a9c27beae..0399f37117 100644
>> --- a/string/test-strncasecmp.c
>> +++ b/string/test-strncasecmp.c
>> @@ -137,6 +137,44 @@ 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, offset1, offset2;
>> +  char *s1, *s2, *s3;
>> +  int exp_result;
>> +
>> +  s1 = (char *) buf1;
> 
> What is the size of buf1 1 page_size?
> 

It's BUF1PAGES*page_size, it could be 1 or 20 pages long.

>> +  for (i = 0; i < page_size - 1; i++)
>> +    s1[i] = 23;
>> +  s1[i] = 0;
>> +
>> +  s3 = strdup (s1);
>> +  offset2 = 2636;
>> +
>> +  for (i = 0; i < 64; ++i)
>> +    {
>> +      offset1 = 3988 + i;
>> +
> The max size of offset1 is 4052.  AFAIK this will never cross a page. So
> the check bellow is not necessarie. >> +      if (offset1 >= page_size
>> +         || offset2 >= page_size)
>> +       return;
> 
> Why do you check offset2 here?  As it never increments you could check it
> outside of the loop. But AFAIK 2636 will never cross a page.

Yes, I'll change this.

>> +
>> +      s1 = (char *) buf1;
> I notice that o the other function you use (CHAR *) instead of (char *)

The other function also tests wide chars, CHAR can be char or wchar_t.

>> +      s2 = s3;
>> +      s1 += offset1;
> 
> Well, a sum is a sum. But Why don't use a simpler for like:
> 
> s1 = magic_number1 + buf1
> s2 = magic_number2 + s3
> 
> for (i = 0; i < 64; ++i){
> 	s1++;
> 	s2++;
> 	do test
> {
> 
>> +      s2 += offset2;
>> +
>> +      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);
>> +       }
>> +    }
>> +}
>> +
>>   static void
>>   do_random_tests (void)
>>   {
>> @@ -334,6 +372,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..30c69dd34b 100644
>> --- a/string/test-strncpy.c
>> +++ b/string/test-strncpy.c
>> @@ -155,6 +155,42 @@ 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, len, start_offset, 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';
> Ok.
> 
>> +
>> +  /* Place short strings ending at page boundary.  */
>> +  offset = last_offset;
>> +  for (i = 0; i < 128; i++)
>> +    {
>> +      offset--;
>> +      len = last_offset - offset;
>> +      FOR_EACH_IMPL (impl, 0)
>> +       do_one_test (impl, s2, (CHAR *) (s1 + offset), len, len);
>> +    }
> Ok.
>> +
>> +  /* Place long strings ending at page boundary.  */
>> +  start_offset = (last_offset + 1) / 2;
>> +  for (i = 0; i < 64; ++i)
>> +    {
>> +      offset = start_offset + i;
>> +      if (offset >= (last_offset + 1))
>> +       break;
> 
> Is this possible?
> 

Yes, on aarch64 the MIN_PAGE_SIZE can be 16 if compiled with 
-DTEST_PAGE_CROSS.

>> +      len = last_offset - offset;
>> +      FOR_EACH_IMPL (impl, 0)
>> +       do_one_test (impl, s2, (CHAR *) (s1 + offset), page_size, len);
>> +    }
>> +}
>> +
>>   static void
>>   do_random_tests (void)
>>   {
>> @@ -317,6 +353,7 @@ test_main (void)
>>       }
>>   
>>     do_random_tests ();
>> +  do_page_tests ();
>>     return ret;
>>   }
>>   
>> -- 
>> 2.26.2
>>
> Ok.
> 
> ---
> Lucas A. M. Magalhães
> 

Thanks,