Re: [PATCH 1/4] strncmp: Add a testcase for page boundary [BZ #25933]

["Paul A. Clarke" <pc@us.ibm.com>]

On Mon, Jun 15, 2020 at 02:34:13PM -0700, H.J. Lu via Libc-alpha wrote:
> On Mon, Jun 15, 2020 at 1:29 PM Paul A. Clarke <pc@us.ibm.com> wrote:
> >
> > On Fri, Jun 12, 2020 at 01:10:53PM -0700, H.J. Lu via Libc-alpha wrote:
> > > Add a strncmp testcase to cover cases where one of strings ends on the
> > > page boundary.
> > > ---
> > >  string/test-strncmp.c | 25 +++++++++++++++++++++++++
> > >  1 file changed, 25 insertions(+)
> > >
> > > diff --git a/string/test-strncmp.c b/string/test-strncmp.c
> > > index d961ac4493..d0928a2864 100644
> > > --- a/string/test-strncmp.c
> > > +++ b/string/test-strncmp.c
> > > @@ -403,6 +403,30 @@ check2 (void)
> > >    free (s2);
> > >  }
> > >
> > > +static void
> > > +check3 (void)
> > > +{
> > > +  size_t size = 32 * 4;
> > > +  CHAR *s1 = (CHAR *) (buf1 + (BUF1PAGES - 1) * page_size);
> > > +  CHAR *s2 = (CHAR *) (buf2 + (BUF1PAGES - 1) * page_size);
> > > +  int exp_result;
> > > +
> > > +  memset (s1, 'a', page_size);
> > > +  memset (s2, 'a', page_size);
> > > +  s1[(page_size / CHARBYTES) - 1] = (CHAR) 0;
> > > +
> > > +  for (size_t s = 99; s <= size; s++)
> > > +    for (size_t s1a = 31; s1a < 32; s1a++)
> > > +      for (size_t s2a = 30; s2a < 32; s2a++)
> > > +     {
> > > +       CHAR *s1p = s1 + (page_size / CHARBYTES - s) - s1a;
> > > +       CHAR *s2p = s2 + (page_size / CHARBYTES - s) - s2a;
> > > +       exp_result = SIMPLE_STRNCMP (s1p, s2p, s);
> > > +       FOR_EACH_IMPL (impl, 0)
> > > +         check_result (impl, s1p, s2p, s, exp_result);
> > > +     }
> > > +}
> >
> > There are lots of magic numbers here.
> >
> > Could you add some context around those number
> 
> My commit log says
> 
> ---
> Add a strncmp testcase to cover cases where one of strings ends on the
> page boundary.
> ---

Which says nothing about why you need to test over 90000 different
cases of a string ending on a page boundary, nor what any of the
magic numbers represent.

> > If they are meant to reflect a cache line length, then it's
> 
> No.  My testcase is for correctness, not for performance.

I made no reference to performance.

> > appropriate to support different cache line sizes.
> > Rafael Zinsly just did this with strncasecmp in the
> > last week or so.

PC