Re: [PATCH v10 3/6] nss: Add tests for the nss_hash in nss_hash.h

[Siddhesh Poyarekar <siddhesh@gotplt.org>]

On 19/05/2022 20:39, Siddhesh Poyarekar wrote:
> On 18/05/2022 22:56, Noah Goldstein via Libc-alpha wrote:
>> If we want to further optimize the function tests are needed.
>> ---
>>   nss/Makefile          |  1 +
>>   nss/nss_hash.c        | 16 +++++++++
>>   nss/simple-nss-hash.h | 42 +++++++++++++++++++++++
>>   nss/tst-nss-hash.c    | 80 +++++++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 139 insertions(+)
>>   create mode 100644 nss/simple-nss-hash.h
>>   create mode 100644 nss/tst-nss-hash.c
> 
> LGTM.
> 
> Reviewed-by: Siddhesh Poyarekar
> 
>>
>> diff --git a/nss/Makefile b/nss/Makefile
>> index d8b06b44fb..a978e3927a 100644
>> --- a/nss/Makefile
>> +++ b/nss/Makefile
>> @@ -62,6 +62,7 @@ tests := \
>>     test-digits-dots \
>>     test-netdb \
>>     tst-nss-getpwent \
>> +  tst-nss-hash \
>>     tst-nss-test1 \
>>     tst-nss-test2 \
>>     tst-nss-test4 \
>> diff --git a/nss/nss_hash.c b/nss/nss_hash.c
>> index 27a348ea9b..f9e17d068a 100644
>> --- a/nss/nss_hash.c
>> +++ b/nss/nss_hash.c
>> @@ -75,4 +75,20 @@ __nss_hash (const void *keyarg, size_t len)
>>     return h;
>>   }
>> +/* For testing/benchmarking purposes. */
>> +static uint32_t
>> +__simple_nss_hash (const void *keyarg, size_t len)
>> +{
>> +  const unsigned char *key;
>> +  size_t i;
>> +  uint32_t h = 0;
>> +  key = keyarg;
>> +
>> +  for (i = 0; i < len; ++i)
>> +    h = *key++ + 65599 * h;
>> +
>> +  return h;
>> +}
>> +
>> +

It just struck me (while reviewing 5/6) that this is duplicated in 
simple-nss-hash.h below.  Shouldn't it be one or the other?  I know it's 
"fixed" in 5/6 but it would be nice to restructure things so that the 
tree builds at this point of the patchset too.

>>   libc_hidden_def (__nss_hash)
>> diff --git a/nss/simple-nss-hash.h b/nss/simple-nss-hash.h
>> new file mode 100644
>> index 0000000000..47708972e7
>> --- /dev/null
>> +++ b/nss/simple-nss-hash.h
>> @@ -0,0 +1,42 @@
>> +/* __simple_nss_hash for testing nss_hash function
>> +   Copyright (C) 2022 Free Software Foundation, Inc.
>> +   This file is part of the GNU C Library.
>> +
>> +   The GNU C Library is free software; you can redistribute it and/or
>> +   modify it under the terms of the GNU Lesser General Public
>> +   License as published by the Free Software Foundation; either
>> +   version 2.1 of the License, or (at your option) any later version.
>> +
>> +   The GNU C Library is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> +   Lesser General Public License for more details.
>> +
>> +   You should have received a copy of the GNU Lesser General Public
>> +   License along with the GNU C Library; if not, see
>> +   <https://www.gnu.org/licenses/>.  */
>> +
>> +#ifndef _SIMPLE_NSS_HASH_H
>> +#define _SIMPLE_NSS_HASH_H 1
>> +
>> +#include <stdint.h>
>> +
>> +/* For testing/benchmarking purposes.  Real implementation in
>> +   nss/nss_hash.c.  */
>> +static uint32_t
>> +__attribute__ ((unused))
>> +__simple_nss_hash (const void *keyarg, size_t len)
>> +{
>> +  const unsigned char *key;
>> +  size_t i;
>> +  uint32_t h = 0;
>> +  key = keyarg;
>> +
>> +  for (i = 0; i < len; ++i)
>> +    h = *key++ + 65599 * h;
>> +
>> +  return h;
>> +}
>> +
>> +
>> +#endif /* simple-nss-hash.h */
>> diff --git a/nss/tst-nss-hash.c b/nss/tst-nss-hash.c
>> new file mode 100644
>> index 0000000000..5ec1f9b0c5
>> --- /dev/null
>> +++ b/nss/tst-nss-hash.c
>> @@ -0,0 +1,80 @@
>> +/* Test __nss_hash
>> +   Copyright (C) 2022 Free Software Foundation, Inc.
>> +   This file is part of the GNU C Library.
>> +
>> +   The GNU C Library is free software; you can redistribute it and/or
>> +   modify it under the terms of the GNU Lesser General Public
>> +   License as published by the Free Software Foundation; either
>> +   version 2.1 of the License, or (at your option) any later version.
>> +
>> +   The GNU C Library is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> +   Lesser General Public License for more details.
>> +
>> +   You should have received a copy of the GNU Lesser General Public
>> +   License along with the GNU C Library; if not, see
>> +   <https://www.gnu.org/licenses/>.  */
>> +
>> +#include <support/support.h>
>> +#include <support/check.h>
>> +#include <stdio.h>
>> +#include <string.h>
>> +#include <stdlib.h>
>> +#include <nss.h>
>> +#include <simple-nss-hash.h>
>> +
>> +uint32_t __nss_hash (const void *__key, size_t __length);
>> +
>> +static int
>> +do_fill_tests (size_t len, int fill)
>> +{
>> +  uint32_t expec, res;
>> +  char buf[len];
>> +  memset (buf, fill, len);
>> +
>> +  expec = __simple_nss_hash (buf, len);
>> +  res = __nss_hash (buf, len);
>> +  if (expec != res)
>> +    FAIL_EXIT1 ("FAIL: fill(%d) (%zu), %x != %x\n", fill, len, expec, 
>> res);
>> +
>> +  return 0;
>> +}
>> +
>> +static int
>> +do_rand_tests (size_t len)
>> +{
>> +  uint32_t expec, res;
>> +  size_t i;
>> +  char buf[len];
>> +  for (i = 0; i < len; ++i)
>> +    buf[i] = random ();
>> +
>> +  expec = __simple_nss_hash (buf, len);
>> +  res = __nss_hash (buf, len);
>> +  if (expec != res)
>> +    FAIL_EXIT1 ("FAIL: random (%zu), %x != %x\n", len, expec, res);
>> +
>> +  return 0;
>> +}
>> +
>> +static int
>> +do_test (void)
>> +{
>> +  size_t i, j;
>> +  for (i = 0; i < 100; ++i)
>> +    {
>> +      for (j = 0; j < 8192; ++j)
>> +    {
>> +      if (do_rand_tests (i))
>> +        return 1;
>> +
>> +      if (do_fill_tests (i, -1) || do_fill_tests (i, 1)
>> +          || do_fill_tests (i, 0x80) || do_fill_tests (i, 0x88))
>> +        return 1;
>> +    }
>> +    }
>> +  return 0;
>> +}
>> +
>> +#include <support/test-driver.c>
>