Re: [PATCH v8] string: Adds tests for test-strncasecmp and test-strncpy

[Raphael M Zinsly <rzinsly@linux.ibm.com>]

Hi Raoni,
Thanks for the review

On 31/07/2020 11:14, Raoni Fassina Firmino wrote:
> Hi Raphael,
> 
> Sorry for my late review, hope it is useful.
> 
> I Tested the patch on powerpc-linux-gnu (with kernel ppc64) on top of
> master (0ad926f349) with no problems.
> 
> 
>> diff --git a/string/test-strncasecmp.c b/string/test-strncasecmp.c
> 
> OK.
> 
> 
>> diff --git a/string/test-strncpy.c b/string/test-strncpy.c
> ...
> 
>> +  const size_t maxoffset = 64;
> 
> I am confused about the role of maxoffset == 64 and sizeof(CHAR), if the
> intended value of 64 is for the buffer size in bytes or array elements.
> But in any case I think It surely is not a problem to be larger than the
> minimum 64 bytes, I am just pointing it out because I may be missing
> something.
> 
maxoffset is based on the usual register sizes as suggested here: 
https://sourceware.org/pipermail/libc-alpha/2020-June/114978.html

> 
>> +  /* Put s1 at the maxoffset from the edge of buf1's last page.  */
>> +  s1 = (CHAR *) buf1 + BUF1PAGES * page_size / sizeof(CHAR) - maxoffset;
> 
> I don't understand how the position in the buffer is being calculated
> here, IIUIC you want the last `maxoffset * sizeof(CHAR)` bytes at the
> end of buf1, in this case `page_size / sizeof(CHAR)` doesn't make sense
> for me, but my math may be off.
> 
This was intended for test-wcsncpy, as `wchar_t` is bigger than `char` 
we need more space in the buffer to use the same test. Your suggestion 
makes sense, but the wmemset fails if s1 is placed with just `maxoffset 
* sizeof(CHAR)`, seems it is accessing the protected area.
The page test at test-strnlen also uses `page_size / sizeof(CHAR)`.

> 
>> +  /* s2 needs room to put a string with size of maxoffset + 1 at s2 +
>> +     (maxoffset - 1).  */
>> +  s2 = (CHAR *) buf2 + page_size / sizeof(CHAR) - maxoffset * 2;
> 
> Same as before, but also `maxoffset * 2` guarantees that only one of the
> tests will touch the edge of buf2 page, I am not sure if this is the
> intended case.
> 
What do you mean by one of the tests?
If you are referring to test-strncasecmp it's a different behavior, 
strncpy fills the `dest` string with NULL if the `src` is smaller than 
`N`, so it will reach the maxoffset+1 and the end of buf2.

> 
>> +	    do_one_test (impl, (s2 + off2), (s1 + off1), maxoffset - off1 - 1,
>> +			 maxoffset + 1);
> 
> Reading do_one_test, it seems like len, here as `maxoffset - off1 - 1`,
> should be `maxoffset - off1`, as it is supposed to be the size of src.
> I know that because the trailing '\0' on src, this in practice will not
> be a problem, but for a case when sizeof(src) and sizeof(dest) is the
> same, do_one_test will ended up doing the last bit for len < n.
> 
The len expected on do_one_test is the number of chars in `src`, if you 
take a look at test-stpncpy.c:
#define STRNCPY_RESULT(dst, len, n) ((dst) + ((len) > (n) ? (n) : (len)))
Counting the trailing '\0' will use it to compare with stpncpy's result 
thus getting a false fail.

> nit, and totally optional, you don't need "()" around (s2 + off2) and
> (s1 + off1), I am only mentioning because you didn't use it for the
> other parameters.
> 
> 
> 
> o/
> Raoni
> 

Best Regards,
-- 
Raphael Moreira Zinsly
IBM
Linux on Power Toolchain