[v5] string: Adds tests for test-strncasecmp and test-strncpy

Changes since v4:
	- Changed do_page_tests() to test every string offset in both inputs
	using usual register sizes instead of relying on dcache following
	Adhemerval's suggestions.

--- >8 ---
	
Adds tests to check if strings placed at page bondaries are
handled correctly by strncasecmp and strncpy similar to tests
for strncmp and strnlen.
---
 string/test-strncasecmp.c | 44 +++++++++++++++++++++++++++++++++++++++
 string/test-strncpy.c     | 32 ++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+)

Hi Raphael,


Following is my limited review, I may be misunderstanding something
crucial about the tests, so take my review with a grain of salt.

I understand that right now the test should pass, right? Is there any
patch to strncasecmp or strncpy to make it buggy to trigger the test, so
it catch the intended failure state it is checking?

For my education, the problem with the page boundaries can happen at the
beginning of the string or only at the end?

> diff --git a/string/test-strncasecmp.c b/string/test-strncasecmp.c
> index 6a9c27beae..628135b962 100644
> --- a/string/test-strncasecmp.c
> +++ b/string/test-strncasecmp.c
> @@ -137,6 +137,49 @@ 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)
> +{
> +  char *s1, *s2;
> +  int exp_result;
> +  /* Assumes up to 512-bit wide read/stores.  */
> +  const size_t maxoffset = 64;
> +
> +  s1 = (char *) buf1 + (BUF1PAGES) * page_size - maxoffset;
> +  memset (s1, 'a', maxoffset - 1);
> +  s1[maxoffset - 1] = '\0';
> +
> +  s2 = (char *) buf2 + page_size - maxoffset;
> +  memset (s2, 'a', maxoffset - 1);
> +  s2[maxoffset - 1] = '\0';
> +

This is just a cosmetic suggestion: you can declare s* and set them like
do_random_test (But I am not sure if this is the standard or the
exception).


> diff --git a/string/test-strncpy.c b/string/test-strncpy.c
> index c978753ad8..05c56e06a5 100644
> --- a/string/test-strncpy.c
> +++ b/string/test-strncpy.c
> @@ -155,6 +155,37 @@ 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)
> +{
> +  CHAR *s1, *s2;
> +  /* Assumes up to 512-bit wide read/stores.  */
> +  const size_t last_offset = 64;
> +
> +  s2 = (CHAR *) buf2;
> +  s1 = (CHAR *) buf1;

I believe s* should be at the end of buf*, the same as in
test-strncasecmp.c?

Cosmetics suggestion: Same as in test-strncasecmp.c.


> +  MEMSET (s1, '\1', last_offset);
> +  s1[last_offset] = '\0';

Not sure if relevant, but in test-strncasecmp.c, the size is 63 + '\n',
here it is 64  + '\0'.  But My math may be off, so sorry in advance.


> +
> +  /* Both strings are bounded to a page with write/read access and the next
> +     page is protected with PROT_NONE (meaning that any access outside of the
> +     page regions will trigger an invalid memory access).
> +
> +     The loop copies the string s1 for all possible offset up to last_offset
> +     for both inputs with a size larger than the string (so memory access
> +     outside the expected memory regions might trigger invalid access).  */
> +
> +  for (size_t off1 = 0; off1 < last_offset; off1++)
> +    {
> +      for (size_t off2 = 0; off2 < last_offset; off2++)
> +	{
> +	  FOR_EACH_IMPL (impl, 0)
> +	    do_one_test (impl, (CHAR *) (s2 + off2), (CHAR *) (s1 + off1),
> +			  last_offset - off1, last_offset + 1);

Should s2 be cleanup before each test? I am thinking that the copy my
fail but do_one_test compare the result as copied because the leftover
from the previous copy.

Cosmetic suggestion: Are the castings needed here as s* seems to be the
right type already.  I didn't test it without it though, so I am
genuinely asking.


o/
Raoni Fassina Firmino

Quoting Raphael Moreira Zinsly via Libc-alpha (2020-06-25 14:33:02)
> Changes since v4:
>         - Changed do_page_tests() to test every string offset in both inputs
>         using usual register sizes instead of relying on dcache following
>         Adhemerval's suggestions.
> 
> --- >8 ---
>         
> Adds tests to check if strings placed at page bondaries are
> handled correctly by strncasecmp and strncpy similar to tests
> for strncmp and strnlen.
> ---
>  string/test-strncasecmp.c | 44 +++++++++++++++++++++++++++++++++++++++
>  string/test-strncpy.c     | 32 ++++++++++++++++++++++++++++
>  2 files changed, 76 insertions(+)
> 
> diff --git a/string/test-strncasecmp.c b/string/test-strncasecmp.c
> index 6a9c27beae..628135b962 100644
> --- a/string/test-strncasecmp.c
> +++ b/string/test-strncasecmp.c
> @@ -137,6 +137,49 @@ 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)
> +{
> +  char *s1, *s2;
> +  int exp_result;
> +  /* Assumes up to 512-bit wide read/stores.  */
> +  const size_t maxoffset = 64;
> +
> +  s1 = (char *) buf1 + (BUF1PAGES) * page_size - maxoffset;
> +  memset (s1, 'a', maxoffset - 1);
> +  s1[maxoffset - 1] = '\0';
> +
> +  s2 = (char *) buf2 + page_size - maxoffset;
> +  memset (s2, 'a', maxoffset - 1);
> +  s2[maxoffset - 1] = '\0';
> +

OK.

> +  /* At this point s1 and s2 points to distinct memory regions containing
> +     "aa..." with size of 63 plus '\0'.  Also, both strings are bounded to a
> +     page with write/read access and the next page is protected with PROT_NONE
> +     (meaning that any access outside of the page regions will trigger an
> +     invalid memory access).
> +
> +     The loop checks for all possible offset up to maxoffset for both
> +     inputs with a size larger than the string (so memory access outside
> +     the expected memory regions might trigger invalid access).  */
> +

OK.

> +  for (size_t off1 = 0; off1 < maxoffset; off1++)
> +    {
> +      for (size_t off2 = 0; off2 < maxoffset; off2++)
> +       {
> +         exp_result = off1 == off2
> +                       ? 0
> +                       : off1 < off2
> +                         ? 'a'
> +                         : -'a';
> +
> +         FOR_EACH_IMPL (impl, 0)
> +           check_result (impl, s1 + off1, s2 + off2, maxoffset + 1,
> +                         exp_result);

Should't this be maxoffset - Max(off1,off2)

> +       }
> +    }
> +}
> +
>  static void
>  do_random_tests (void)
>  {
> @@ -334,6 +377,7 @@ test_locale (const char *locale)
>      }
>  
>    do_random_tests ();
> +  do_page_tests ();
>  }
>  

OK.

>  int
> diff --git a/string/test-strncpy.c b/string/test-strncpy.c
> index c978753ad8..05c56e06a5 100644
> --- a/string/test-strncpy.c
> +++ b/string/test-strncpy.c
> @@ -155,6 +155,37 @@ 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)
> +{
> +  CHAR *s1, *s2;
> +  /* Assumes up to 512-bit wide read/stores.  */
> +  const size_t last_offset = 64;
> +

Here you had a calculation for the last offset and now it's just 64.
This is not setting s1 to be the last offset for the buf1 last page.  Is
it correct?  AFAICS it should be the same calculation from the above
test (BUF1PAGES*page_sizes - 64)

> +  s2 = (CHAR *) buf2;
> +  s1 = (CHAR *) buf1;
> +  MEMSET (s1, '\1', last_offset);
> +  s1[last_offset] = '\0';

If I'm correct above this should be:
s1 = (CHAR *)buf1 + last_offset;
MEMSET (s1, '\1', 63);
s1[64] = '\0';

> +

OK.

> +  /* Both strings are bounded to a page with write/read access and the next
> +     page is protected with PROT_NONE (meaning that any access outside of the
> +     page regions will trigger an invalid memory access).
> +
> +     The loop copies the string s1 for all possible offset up to last_offset
> +     for both inputs with a size larger than the string (so memory access
> +     outside the expected memory regions might trigger invalid access).  */
> +

OK.

> +  for (size_t off1 = 0; off1 < last_offset; off1++)
> +    {
> +      for (size_t off2 = 0; off2 < last_offset; off2++)
> +       {
> +         FOR_EACH_IMPL (impl, 0)
> +           do_one_test (impl, (CHAR *) (s2 + off2), (CHAR *) (s1 + off1),
> +                         last_offset - off1, last_offset + 1);
> +       }
> +    }
> +}
> +

If I was correct above last_offset must be changed here to 64.

>  static void
>  do_random_tests (void)
>  {
> @@ -317,6 +348,7 @@ test_main (void)
>      }
>  
>    do_random_tests ();
> +  do_page_tests ();
>    return ret;
>  }
>  

OK.

> -- 
> 2.26.2
>

On 01/07/2020 18:47, Raoni Fassina Firmino wrote:
> Hi Raphael,
> 
> 
> Following is my limited review, I may be misunderstanding something
> crucial about the tests, so take my review with a grain of salt.
> 
> I understand that right now the test should pass, right? Is there any
> patch to strncasecmp or strncpy to make it buggy to trigger the test, so
> it catch the intended failure state it is checking?
> 
It is passing, the motivation is to improve the coverage of these tests 
and avoid errors on future optimizations.

> For my education, the problem with the page boundaries can happen at the
> beginning of the string or only at the end?
> 
Only at the end. Unless a user force it I can't see how it would cross 
the page boundary at the beginning of the string, AFAICS this is not 
being handle by these functions and it shouldn't.

>> diff --git a/string/test-strncasecmp.c b/string/test-strncasecmp.c
>> index 6a9c27beae..628135b962 100644
>> --- a/string/test-strncasecmp.c
>> +++ b/string/test-strncasecmp.c
>> @@ -137,6 +137,49 @@ 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)
>> +{
>> +  char *s1, *s2;
>> +  int exp_result;
>> +  /* Assumes up to 512-bit wide read/stores.  */
>> +  const size_t maxoffset = 64;
>> +
>> +  s1 = (char *) buf1 + (BUF1PAGES) * page_size - maxoffset;
>> +  memset (s1, 'a', maxoffset - 1);
>> +  s1[maxoffset - 1] = '\0';
>> +
>> +  s2 = (char *) buf2 + page_size - maxoffset;
>> +  memset (s2, 'a', maxoffset - 1);
>> +  s2[maxoffset - 1] = '\0';
>> +
> 
> This is just a cosmetic suggestion: you can declare s* and set them like
> do_random_test (But I am not sure if this is the standard or the
> exception).
> 
It doesn't seem to have a standard between the string tests.

> 
>> diff --git a/string/test-strncpy.c b/string/test-strncpy.c
>> index c978753ad8..05c56e06a5 100644
>> --- a/string/test-strncpy.c
>> +++ b/string/test-strncpy.c
>> @@ -155,6 +155,37 @@ 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)
>> +{
>> +  CHAR *s1, *s2;
>> +  /* Assumes up to 512-bit wide read/stores.  */
>> +  const size_t last_offset = 64;
>> +
>> +  s2 = (CHAR *) buf2;
>> +  s1 = (CHAR *) buf1;
> 
> I believe s* should be at the end of buf*, the same as in
> test-strncasecmp.c?
> 
I was reusing code from v4 e left this unchanged, you are right it 
should be as in the strncasecmp test, however I got a segfault testing 
the change, I'm investigating to see if this is a bug in my test or in 
strncpy.

> Cosmetics suggestion: Same as in test-strncasecmp.c.
> 
> 
>> +  MEMSET (s1, '\1', last_offset);
>> +  s1[last_offset] = '\0';
> 
> Not sure if relevant, but in test-strncasecmp.c, the size is 63 + '\n',
> here it is 64  + '\0'.  But My math may be off, so sorry in advance.
> 
Correct, I was using last_offset 63 and decided to change it in the end 
and mistakenly left this part unchanged.

> 
>> +
>> +  /* Both strings are bounded to a page with write/read access and the next
>> +     page is protected with PROT_NONE (meaning that any access outside of the
>> +     page regions will trigger an invalid memory access).
>> +
>> +     The loop copies the string s1 for all possible offset up to last_offset
>> +     for both inputs with a size larger than the string (so memory access
>> +     outside the expected memory regions might trigger invalid access).  */
>> +
>> +  for (size_t off1 = 0; off1 < last_offset; off1++)
>> +    {
>> +      for (size_t off2 = 0; off2 < last_offset; off2++)
>> +	{
>> +	  FOR_EACH_IMPL (impl, 0)
>> +	    do_one_test (impl, (CHAR *) (s2 + off2), (CHAR *) (s1 + off1),
>> +			  last_offset - off1, last_offset + 1);
> 
> Should s2 be cleanup before each test? I am thinking that the copy my
> fail but do_one_test compare the result as copied because the leftover
> from the previous copy.
> 
If it gets an invalid memory access it will fail even with the leftover.

> Cosmetic suggestion: Are the castings needed here as s* seems to be the
> right type already.  I didn't test it without it though, so I am
> genuinely asking.
> 
I had to used it for wcsncpy at the beggining, but I tested and it works 
fine now without the cast.

> 
> o/
> Raoni Fassina Firmino
> 

Thanks,

On 02/07/2020 10:35, Lucas A. M. Magalhaes wrote:
> Quoting Raphael Moreira Zinsly via Libc-alpha (2020-06-25 14:33:02)
>> Changes since v4:
>>          - Changed do_page_tests() to test every string offset in both inputs
>>          using usual register sizes instead of relying on dcache following
>>          Adhemerval's suggestions.
>>
>> --- >8 ---
>>          
>> Adds tests to check if strings placed at page bondaries are
>> handled correctly by strncasecmp and strncpy similar to tests
>> for strncmp and strnlen.
>> ---
>>   string/test-strncasecmp.c | 44 +++++++++++++++++++++++++++++++++++++++
>>   string/test-strncpy.c     | 32 ++++++++++++++++++++++++++++
>>   2 files changed, 76 insertions(+)
>>
>> diff --git a/string/test-strncasecmp.c b/string/test-strncasecmp.c
>> index 6a9c27beae..628135b962 100644
>> --- a/string/test-strncasecmp.c
>> +++ b/string/test-strncasecmp.c
>> @@ -137,6 +137,49 @@ 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)
>> +{
>> +  char *s1, *s2;
>> +  int exp_result;
>> +  /* Assumes up to 512-bit wide read/stores.  */
>> +  const size_t maxoffset = 64;
>> +
>> +  s1 = (char *) buf1 + (BUF1PAGES) * page_size - maxoffset;
>> +  memset (s1, 'a', maxoffset - 1);
>> +  s1[maxoffset - 1] = '\0';
>> +
>> +  s2 = (char *) buf2 + page_size - maxoffset;
>> +  memset (s2, 'a', maxoffset - 1);
>> +  s2[maxoffset - 1] = '\0';
>> +
> 
> OK.
> 
>> +  /* At this point s1 and s2 points to distinct memory regions containing
>> +     "aa..." with size of 63 plus '\0'.  Also, both strings are bounded to a
>> +     page with write/read access and the next page is protected with PROT_NONE
>> +     (meaning that any access outside of the page regions will trigger an
>> +     invalid memory access).
>> +
>> +     The loop checks for all possible offset up to maxoffset for both
>> +     inputs with a size larger than the string (so memory access outside
>> +     the expected memory regions might trigger invalid access).  */
>> +
> 
> OK.
> 
>> +  for (size_t off1 = 0; off1 < maxoffset; off1++)
>> +    {
>> +      for (size_t off2 = 0; off2 < maxoffset; off2++)
>> +       {
>> +         exp_result = off1 == off2
>> +                       ? 0
>> +                       : off1 < off2
>> +                         ? 'a'
>> +                         : -'a';
>> +
>> +         FOR_EACH_IMPL (impl, 0)
>> +           check_result (impl, s1 + off1, s2 + off2, maxoffset + 1,
>> +                         exp_result);
> 
> Should't this be maxoffset - Max(off1,off2)
> 
Why? The idea is to test with a size larger than the string, this would 
be shorter.

>> +       }
>> +    }
>> +}
>> +
>>   static void
>>   do_random_tests (void)
>>   {
>> @@ -334,6 +377,7 @@ test_locale (const char *locale)
>>       }
>>   
>>     do_random_tests ();
>> +  do_page_tests ();
>>   }
>>   
> 
> OK.
> 
>>   int
>> diff --git a/string/test-strncpy.c b/string/test-strncpy.c
>> index c978753ad8..05c56e06a5 100644
>> --- a/string/test-strncpy.c
>> +++ b/string/test-strncpy.c
>> @@ -155,6 +155,37 @@ 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)
>> +{
>> +  CHAR *s1, *s2;
>> +  /* Assumes up to 512-bit wide read/stores.  */
>> +  const size_t last_offset = 64;
>> +
> 
> Here you had a calculation for the last offset and now it's just 64.
> This is not setting s1 to be the last offset for the buf1 last page.  Is
> it correct?  AFAICS it should be the same calculation from the above
> test (BUF1PAGES*page_sizes - 64)
>
Yes, as I replied to Raoni, this should be as the other test.

>> +  s2 = (CHAR *) buf2;
>> +  s1 = (CHAR *) buf1;
>> +  MEMSET (s1, '\1', last_offset);
>> +  s1[last_offset] = '\0';
> 
> If I'm correct above this should be:
> s1 = (CHAR *)buf1 + last_offset;
> MEMSET (s1, '\1', 63);
> s1[64] = '\0';
> 
>> +
> 
> OK.
> 
>> +  /* Both strings are bounded to a page with write/read access and the next
>> +     page is protected with PROT_NONE (meaning that any access outside of the
>> +     page regions will trigger an invalid memory access).
>> +
>> +     The loop copies the string s1 for all possible offset up to last_offset
>> +     for both inputs with a size larger than the string (so memory access
>> +     outside the expected memory regions might trigger invalid access).  */
>> +
> 
> OK.
> 
>> +  for (size_t off1 = 0; off1 < last_offset; off1++)
>> +    {
>> +      for (size_t off2 = 0; off2 < last_offset; off2++)
>> +       {
>> +         FOR_EACH_IMPL (impl, 0)
>> +           do_one_test (impl, (CHAR *) (s2 + off2), (CHAR *) (s1 + off1),
>> +                         last_offset - off1, last_offset + 1);
>> +       }
>> +    }
>> +}
>> +
> 
> If I was correct above last_offset must be changed here to 64.
> 
>>   static void
>>   do_random_tests (void)
>>   {
>> @@ -317,6 +348,7 @@ test_main (void)
>>       }
>>   
>>     do_random_tests ();
>> +  do_page_tests ();
>>     return ret;
>>   }
>>   
> 
> OK.
> 
>> -- 
>> 2.26.2
>>

Thank you,