Improve test coverage of strlen function
Checks
Commit Message
This patch covers following test condition.
- String starts with different alignment and ends at the page boundary
with less than 32 byte length.
- String starts with different alignment and cross page boundary with
31 byte fixed length.
---
string/test-strlen.c | 47 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 47 insertions(+)
Comments
On Thu, May 27, 2021 at 4:34 PM Sunil K Pandey <skpgkp2@gmail.com> wrote:
>
> This patch covers following test condition.
This patch covers following conditions:
>
> - String starts with different alignment and ends at the page boundary
alignments
> with less than 32 byte length.
> - String starts with different alignment and cross page boundary with
alignments
> 31 byte fixed length.
> ---
> string/test-strlen.c | 47 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 47 insertions(+)
>
> diff --git a/string/test-strlen.c b/string/test-strlen.c
> index 6e67d1f1f1..5f4ce9eafd 100644
> --- a/string/test-strlen.c
> +++ b/string/test-strlen.c
> @@ -132,6 +132,51 @@ do_random_tests (void)
> }
> }
>
> +/* Test page cross boundary logic but string stay on same page. String
says on the same page.
> + start position and length both change. */
> +
> +static void
> +do_test2 (void)
> +{
> + size_t i;
> +
> + CHAR *buf = (CHAR *) (buf1 + page_size - 32);
> +
> + size_t maxlength = 32 / sizeof (CHAR) - 1;
> + buf[maxlength] = 0;
> +
> + for (size_t pos = 0; pos < maxlength - 1; pos++)
> + {
> + for (i = pos; i < maxlength; i++)
> + buf[i] = 1 + 11111 * i % MAX_CHAR;
> +
> + FOR_EACH_IMPL (impl, 0)
> + do_one_test (impl, (CHAR *) (buf + pos), maxlength - pos);
> + }
> +}
> +
> +/* Test page cross boundary logic and string do cross the page. String
> + start position get adjusted but length remains constant. */
gets
> +
> +static void
> +do_test3 (void)
> +{
> + size_t i;
> +
> + CHAR *buf = (CHAR *) (buf1 + getpagesize() - 32);
Add a space before ().
> +
> + size_t maxlength = 32 / sizeof (CHAR) - 1;
> +
> + for (size_t pos = 0; pos < maxlength; pos++)
> + {
> + for (i = pos; i < pos + maxlength; i++)
> + buf[i] = 1 + 11111 * i % MAX_CHAR;
> + buf[i] = 0;
> + FOR_EACH_IMPL (impl, 0)
> + do_one_test (impl, (CHAR *) (buf + pos), maxlength);
> + }
> +}
> +
> int
> test_main (void)
> {
> @@ -161,6 +206,8 @@ test_main (void)
> }
>
> do_random_tests ();
> + do_test2 ();
> + do_test3 ();
> return ret;
> }
>
> --
> 2.31.1
>
On Thu, May 27, 2021 at 8:24 PM H.J. Lu via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> On Thu, May 27, 2021 at 4:34 PM Sunil K Pandey <skpgkp2@gmail.com> wrote:
> >
> > This patch covers following test condition.
>
> This patch covers following conditions:
>
> >
> > - String starts with different alignment and ends at the page boundary
> alignments
> > with less than 32 byte length.
> > - String starts with different alignment and cross page boundary with
> alignments
> > 31 byte fixed length.
> > ---
> > string/test-strlen.c | 47 ++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 47 insertions(+)
> >
> > diff --git a/string/test-strlen.c b/string/test-strlen.c
> > index 6e67d1f1f1..5f4ce9eafd 100644
> > --- a/string/test-strlen.c
> > +++ b/string/test-strlen.c
> > @@ -132,6 +132,51 @@ do_random_tests (void)
> > }
> > }
> >
> > +/* Test page cross boundary logic but string stay on same page. String
>
> says on the same page.
> > + start position and length both change. */
> > +
> > +static void
> > +do_test2 (void)
> > +{
> > + size_t i;
> > +
> > + CHAR *buf = (CHAR *) (buf1 + page_size - 32);
> > +
> > + size_t maxlength = 32 / sizeof (CHAR) - 1;
> > + buf[maxlength] = 0;
> > +
> > + for (size_t pos = 0; pos < maxlength - 1; pos++)
> > + {
> > + for (i = pos; i < maxlength; i++)
> > + buf[i] = 1 + 11111 * i % MAX_CHAR;
> > +
> > + FOR_EACH_IMPL (impl, 0)
> > + do_one_test (impl, (CHAR *) (buf + pos), maxlength - pos);
> > + }
> > +}
> > +
> > +/* Test page cross boundary logic and string do cross the page. String
> > + start position get adjusted but length remains constant. */
> gets
> > +
> > +static void
> > +do_test3 (void)
> > +{
> > + size_t i;
> > +
> > + CHAR *buf = (CHAR *) (buf1 + getpagesize() - 32);
>
> Add a space before ().
> > +
> > + size_t maxlength = 32 / sizeof (CHAR) - 1;
> > +
> > + for (size_t pos = 0; pos < maxlength; pos++)
> > + {
> > + for (i = pos; i < pos + maxlength; i++)
> > + buf[i] = 1 + 11111 * i % MAX_CHAR;
> > + buf[i] = 0;
> > + FOR_EACH_IMPL (impl, 0)
> > + do_one_test (impl, (CHAR *) (buf + pos), maxlength);
> > + }
> > +}
> > +
> > int
> > test_main (void)
> > {
> > @@ -161,6 +206,8 @@ test_main (void)
> > }
> >
> > do_random_tests ();
> > + do_test2 ();
> > + do_test3 ();
> > return ret;
> > }
> >
> > --
> > 2.31.1
> >
>
>
> --
> H.J.
I think the functionality of this patch could be added more easily
by just adjusting do_test so that:
`align &= 63;` -> `align %= getpagesize()`;
then adding these cases for do_test in main.
On Thu, May 27, 2021 at 6:20 PM Noah Goldstein <goldstein.w.n@gmail.com>
wrote:
> On Thu, May 27, 2021 at 8:24 PM H.J. Lu via Libc-alpha
> <libc-alpha@sourceware.org> wrote:
> >
> > On Thu, May 27, 2021 at 4:34 PM Sunil K Pandey <skpgkp2@gmail.com>
> wrote:
> > >
> > > This patch covers following test condition.
> >
> > This patch covers following conditions:
> >
> > >
> > > - String starts with different alignment and ends at the page boundary
> > alignments
> > > with less than 32 byte length.
> > > - String starts with different alignment and cross page boundary with
> > alignments
> > > 31 byte fixed length.
> > > ---
> > > string/test-strlen.c | 47 ++++++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 47 insertions(+)
> > >
> > > diff --git a/string/test-strlen.c b/string/test-strlen.c
> > > index 6e67d1f1f1..5f4ce9eafd 100644
> > > --- a/string/test-strlen.c
> > > +++ b/string/test-strlen.c
> > > @@ -132,6 +132,51 @@ do_random_tests (void)
> > > }
> > > }
> > >
> > > +/* Test page cross boundary logic but string stay on same page.
> String
> >
> > says on the same page.
> > > + start position and length both change. */
> > > +
> > > +static void
> > > +do_test2 (void)
> > > +{
> > > + size_t i;
> > > +
> > > + CHAR *buf = (CHAR *) (buf1 + page_size - 32);
> > > +
> > > + size_t maxlength = 32 / sizeof (CHAR) - 1;
> > > + buf[maxlength] = 0;
> > > +
> > > + for (size_t pos = 0; pos < maxlength - 1; pos++)
> > > + {
> > > + for (i = pos; i < maxlength; i++)
> > > + buf[i] = 1 + 11111 * i % MAX_CHAR;
> > > +
> > > + FOR_EACH_IMPL (impl, 0)
> > > + do_one_test (impl, (CHAR *) (buf + pos), maxlength - pos);
> > > + }
> > > +}
> > > +
> > > +/* Test page cross boundary logic and string do cross the page.
> String
> > > + start position get adjusted but length remains constant. */
> > gets
> > > +
> > > +static void
> > > +do_test3 (void)
> > > +{
> > > + size_t i;
> > > +
> > > + CHAR *buf = (CHAR *) (buf1 + getpagesize() - 32);
> >
> > Add a space before ().
> > > +
> > > + size_t maxlength = 32 / sizeof (CHAR) - 1;
> > > +
> > > + for (size_t pos = 0; pos < maxlength; pos++)
> > > + {
> > > + for (i = pos; i < pos + maxlength; i++)
> > > + buf[i] = 1 + 11111 * i % MAX_CHAR;
> > > + buf[i] = 0;
> > > + FOR_EACH_IMPL (impl, 0)
> > > + do_one_test (impl, (CHAR *) (buf + pos), maxlength);
> > > + }
> > > +}
> > > +
> > > int
> > > test_main (void)
> > > {
> > > @@ -161,6 +206,8 @@ test_main (void)
> > > }
> > >
> > > do_random_tests ();
> > > + do_test2 ();
> > > + do_test3 ();
> > > return ret;
> > > }
> > >
> > > --
> > > 2.31.1
> > >
> >
> >
> > --
> > H.J.
>
> I think the functionality of this patch could be added more easily
> by just adjusting do_test so that:
> `align &= 63;` -> `align %= getpagesize()`;
>
I believe you mean `align &= 63;` -> `align %= getpagesize() - 1 `;
In this case the value of align will be very high near pagesize.
>
> then adding these cases for do_test in main.
>
While it will be much simpler and work fine for strlen but not for
wcslen(same code used).
For example:
If the value of align is 4096-32=4064, in the current do_test
implementation, the first wide char it will write at
buf[4064 *4=16256], which is outside the buf allocated region, unless we
increase allocated space for buf.
On Sat, May 29, 2021 at 1:12 AM Sunil Pandey <skpgkp2@gmail.com> wrote:
>
>
>
> On Thu, May 27, 2021 at 6:20 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
>>
>> On Thu, May 27, 2021 at 8:24 PM H.J. Lu via Libc-alpha
>> <libc-alpha@sourceware.org> wrote:
>> >
>> > On Thu, May 27, 2021 at 4:34 PM Sunil K Pandey <skpgkp2@gmail.com> wrote:
>> > >
>> > > This patch covers following test condition.
>> >
>> > This patch covers following conditions:
>> >
>> > >
>> > > - String starts with different alignment and ends at the page boundary
>> > alignments
>> > > with less than 32 byte length.
>> > > - String starts with different alignment and cross page boundary with
>> > alignments
>> > > 31 byte fixed length.
>> > > ---
>> > > string/test-strlen.c | 47 ++++++++++++++++++++++++++++++++++++++++++++
>> > > 1 file changed, 47 insertions(+)
>> > >
>> > > diff --git a/string/test-strlen.c b/string/test-strlen.c
>> > > index 6e67d1f1f1..5f4ce9eafd 100644
>> > > --- a/string/test-strlen.c
>> > > +++ b/string/test-strlen.c
>> > > @@ -132,6 +132,51 @@ do_random_tests (void)
>> > > }
>> > > }
>> > >
>> > > +/* Test page cross boundary logic but string stay on same page. String
>> >
>> > says on the same page.
>> > > + start position and length both change. */
>> > > +
>> > > +static void
>> > > +do_test2 (void)
>> > > +{
>> > > + size_t i;
>> > > +
>> > > + CHAR *buf = (CHAR *) (buf1 + page_size - 32);
>> > > +
>> > > + size_t maxlength = 32 / sizeof (CHAR) - 1;
>> > > + buf[maxlength] = 0;
>> > > +
>> > > + for (size_t pos = 0; pos < maxlength - 1; pos++)
>> > > + {
>> > > + for (i = pos; i < maxlength; i++)
>> > > + buf[i] = 1 + 11111 * i % MAX_CHAR;
>> > > +
>> > > + FOR_EACH_IMPL (impl, 0)
>> > > + do_one_test (impl, (CHAR *) (buf + pos), maxlength - pos);
>> > > + }
>> > > +}
>> > > +
>> > > +/* Test page cross boundary logic and string do cross the page. String
>> > > + start position get adjusted but length remains constant. */
>> > gets
>> > > +
>> > > +static void
>> > > +do_test3 (void)
>> > > +{
>> > > + size_t i;
>> > > +
>> > > + CHAR *buf = (CHAR *) (buf1 + getpagesize() - 32);
>> >
>> > Add a space before ().
>> > > +
>> > > + size_t maxlength = 32 / sizeof (CHAR) - 1;
>> > > +
>> > > + for (size_t pos = 0; pos < maxlength; pos++)
>> > > + {
>> > > + for (i = pos; i < pos + maxlength; i++)
>> > > + buf[i] = 1 + 11111 * i % MAX_CHAR;
>> > > + buf[i] = 0;
>> > > + FOR_EACH_IMPL (impl, 0)
>> > > + do_one_test (impl, (CHAR *) (buf + pos), maxlength);
>> > > + }
>> > > +}
>> > > +
>> > > int
>> > > test_main (void)
>> > > {
>> > > @@ -161,6 +206,8 @@ test_main (void)
>> > > }
>> > >
>> > > do_random_tests ();
>> > > + do_test2 ();
>> > > + do_test3 ();
>> > > return ret;
>> > > }
>> > >
>> > > --
>> > > 2.31.1
>> > >
>> >
>> >
>> > --
>> > H.J.
>>
>> I think the functionality of this patch could be added more easily
>> by just adjusting do_test so that:
>> `align &= 63;` -> `align %= getpagesize()`;
>
> I believe you mean `align &= 63;` -> `align %= getpagesize() - 1 `;
> In this case the value of align will be very high near pagesize.
>>
>>
>> then adding these cases for do_test in main.
>
>
> While it will be much simpler and work fine for strlen but not for wcslen(same code used).
>
> For example:
> If the value of align is 4096-32=4064, in the current do_test implementation, the first wide char it will write at
> buf[4064 *4=16256], which is outside the buf allocated region, unless we increase allocated space for buf.
>
You can adjust align based on sizeof CHAR.
On Sat, May 29, 2021 at 4:12 AM Sunil Pandey <skpgkp2@gmail.com> wrote:
>
>
>
> On Thu, May 27, 2021 at 6:20 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
>>
>> On Thu, May 27, 2021 at 8:24 PM H.J. Lu via Libc-alpha
>> <libc-alpha@sourceware.org> wrote:
>> >
>> > On Thu, May 27, 2021 at 4:34 PM Sunil K Pandey <skpgkp2@gmail.com> wrote:
>> > >
>> > > This patch covers following test condition.
>> >
>> > This patch covers following conditions:
>> >
>> > >
>> > > - String starts with different alignment and ends at the page boundary
>> > alignments
>> > > with less than 32 byte length.
>> > > - String starts with different alignment and cross page boundary with
>> > alignments
>> > > 31 byte fixed length.
>> > > ---
>> > > string/test-strlen.c | 47 ++++++++++++++++++++++++++++++++++++++++++++
>> > > 1 file changed, 47 insertions(+)
>> > >
>> > > diff --git a/string/test-strlen.c b/string/test-strlen.c
>> > > index 6e67d1f1f1..5f4ce9eafd 100644
>> > > --- a/string/test-strlen.c
>> > > +++ b/string/test-strlen.c
>> > > @@ -132,6 +132,51 @@ do_random_tests (void)
>> > > }
>> > > }
>> > >
>> > > +/* Test page cross boundary logic but string stay on same page. String
>> >
>> > says on the same page.
>> > > + start position and length both change. */
>> > > +
>> > > +static void
>> > > +do_test2 (void)
>> > > +{
>> > > + size_t i;
>> > > +
>> > > + CHAR *buf = (CHAR *) (buf1 + page_size - 32);
>> > > +
>> > > + size_t maxlength = 32 / sizeof (CHAR) - 1;
>> > > + buf[maxlength] = 0;
>> > > +
>> > > + for (size_t pos = 0; pos < maxlength - 1; pos++)
>> > > + {
>> > > + for (i = pos; i < maxlength; i++)
>> > > + buf[i] = 1 + 11111 * i % MAX_CHAR;
>> > > +
>> > > + FOR_EACH_IMPL (impl, 0)
>> > > + do_one_test (impl, (CHAR *) (buf + pos), maxlength - pos);
>> > > + }
>> > > +}
>> > > +
>> > > +/* Test page cross boundary logic and string do cross the page. String
>> > > + start position get adjusted but length remains constant. */
>> > gets
>> > > +
>> > > +static void
>> > > +do_test3 (void)
>> > > +{
>> > > + size_t i;
>> > > +
>> > > + CHAR *buf = (CHAR *) (buf1 + getpagesize() - 32);
>> >
>> > Add a space before ().
>> > > +
>> > > + size_t maxlength = 32 / sizeof (CHAR) - 1;
>> > > +
>> > > + for (size_t pos = 0; pos < maxlength; pos++)
>> > > + {
>> > > + for (i = pos; i < pos + maxlength; i++)
>> > > + buf[i] = 1 + 11111 * i % MAX_CHAR;
>> > > + buf[i] = 0;
>> > > + FOR_EACH_IMPL (impl, 0)
>> > > + do_one_test (impl, (CHAR *) (buf + pos), maxlength);
>> > > + }
>> > > +}
>> > > +
>> > > int
>> > > test_main (void)
>> > > {
>> > > @@ -161,6 +206,8 @@ test_main (void)
>> > > }
>> > >
>> > > do_random_tests ();
>> > > + do_test2 ();
>> > > + do_test3 ();
>> > > return ret;
>> > > }
>> > >
>> > > --
>> > > 2.31.1
>> > >
>> >
>> >
>> > --
>> > H.J.
>>
>> I think the functionality of this patch could be added more easily
>> by just adjusting do_test so that:
>> `align &= 63;` -> `align %= getpagesize()`;
>
> I believe you mean `align &= 63;` -> `align %= getpagesize() - 1 `;
> In this case the value of align will be very high near pagesize.
>>
>>
>> then adding these cases for do_test in main.
>
>
> While it will be much simpler and work fine for strlen but not for wcslen(same code used).
How about `align %= (getpagesize() / sizeof(CHAR))`?
But either way support the patch. More tests are always good!
>
> For example:
> If the value of align is 4096-32=4064, in the current do_test implementation, the first wide char it will write at
> buf[4064 *4=16256], which is outside the buf allocated region, unless we increase allocated space for buf.
>
On 5/27/21 7:34 PM, Sunil K Pandey via Libc-alpha wrote:
> This patch covers following test condition.
>
> - String starts with different alignment and ends at the page boundary
> with less than 32 byte length.
> - String starts with different alignment and cross page boundary with
> 31 byte fixed length.
Is this work covered by the Intel's copyright assignment with the FSF?
On Mon, May 31, 2021 at 7:42 AM Carlos O'Donell <carlos@redhat.com> wrote:
>
> On 5/27/21 7:34 PM, Sunil K Pandey via Libc-alpha wrote:
> > This patch covers following test condition.
> >
> > - String starts with different alignment and ends at the page boundary
> > with less than 32 byte length.
> > - String starts with different alignment and cross page boundary with
> > 31 byte fixed length.
>
> Is this work covered by the Intel's copyright assignment with the FSF?
>
Yes. Sunil will submit a v2 patch.
On Mon, May 31, 2021 at 11:21 AM H.J. Lu via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> On Mon, May 31, 2021 at 7:42 AM Carlos O'Donell <carlos@redhat.com> wrote:
> >
> > On 5/27/21 7:34 PM, Sunil K Pandey via Libc-alpha wrote:
> > > This patch covers following test condition.
> > >
> > > - String starts with different alignment and ends at the page boundary
> > > with less than 32 byte length.
> > > - String starts with different alignment and cross page boundary with
> > > 31 byte fixed length.
> >
> > Is this work covered by the Intel's copyright assignment with the FSF?
> >
>
> Yes. Sunil will submit a v2 patch.
Are you and Sunil working on a project together to improve testing
of string/memory functions?
>
> --
> H.J.
On Wed, Jun 2, 2021 at 10:18 AM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
>
> On Mon, May 31, 2021 at 11:21 AM H.J. Lu via Libc-alpha
> <libc-alpha@sourceware.org> wrote:
> >
> > On Mon, May 31, 2021 at 7:42 AM Carlos O'Donell <carlos@redhat.com> wrote:
> > >
> > > On 5/27/21 7:34 PM, Sunil K Pandey via Libc-alpha wrote:
> > > > This patch covers following test condition.
> > > >
> > > > - String starts with different alignment and ends at the page boundary
> > > > with less than 32 byte length.
> > > > - String starts with different alignment and cross page boundary with
> > > > 31 byte fixed length.
> > >
> > > Is this work covered by the Intel's copyright assignment with the FSF?
> > >
> >
> > Yes. Sunil will submit a v2 patch.
>
> Are you and Sunil working on a project together to improve testing
> of string/memory functions?
>
We are working on Intel LAM in glibc:
https://gitlab.com/x86-glibc/glibc/-/tree/users/intel/lam/master
@@ -132,6 +132,51 @@ do_random_tests (void)
}
}
+/* Test page cross boundary logic but string stay on same page. String
+ start position and length both change. */
+
+static void
+do_test2 (void)
+{
+ size_t i;
+
+ CHAR *buf = (CHAR *) (buf1 + page_size - 32);
+
+ size_t maxlength = 32 / sizeof (CHAR) - 1;
+ buf[maxlength] = 0;
+
+ for (size_t pos = 0; pos < maxlength - 1; pos++)
+ {
+ for (i = pos; i < maxlength; i++)
+ buf[i] = 1 + 11111 * i % MAX_CHAR;
+
+ FOR_EACH_IMPL (impl, 0)
+ do_one_test (impl, (CHAR *) (buf + pos), maxlength - pos);
+ }
+}
+
+/* Test page cross boundary logic and string do cross the page. String
+ start position get adjusted but length remains constant. */
+
+static void
+do_test3 (void)
+{
+ size_t i;
+
+ CHAR *buf = (CHAR *) (buf1 + getpagesize() - 32);
+
+ size_t maxlength = 32 / sizeof (CHAR) - 1;
+
+ for (size_t pos = 0; pos < maxlength; pos++)
+ {
+ for (i = pos; i < pos + maxlength; i++)
+ buf[i] = 1 + 11111 * i % MAX_CHAR;
+ buf[i] = 0;
+ FOR_EACH_IMPL (impl, 0)
+ do_one_test (impl, (CHAR *) (buf + pos), maxlength);
+ }
+}
+
int
test_main (void)
{
@@ -161,6 +206,8 @@ test_main (void)
}
do_random_tests ();
+ do_test2 ();
+ do_test3 ();
return ret;
}