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

[Raphael M Zinsly]

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,
-- 
Raphael Moreira Zinsly
IBM
Linux on Power Toolchain