Re: [PATCH v9 2/6] elf: Add tests for the dl hash funcs (_dl_new_hash and _dl_elf_hash)

[Noah Goldstein <goldstein.w.n@gmail.com>]

On Mon, May 16, 2022 at 11:19 PM Siddhesh Poyarekar <siddhesh@gotplt.org> wrote:
>
> On 17/05/2022 02:00, Noah Goldstein via Libc-alpha wrote:
> > If we want to further optimize the functions tests are needed.
> > ---
> >   elf/Makefile      |   1 +
> >   elf/tst-dl-hash.c | 147 ++++++++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 148 insertions(+)
> >   create mode 100644 elf/tst-dl-hash.c
> >
> > diff --git a/elf/Makefile b/elf/Makefile
> > index fc9860edee..0e72f913a0 100644
> > --- a/elf/Makefile
> > +++ b/elf/Makefile
> > @@ -309,6 +309,7 @@ tests := \
> >     tst-array4 \
> >     tst-array5 \
> >     tst-auxv \
> > +  tst-dl-hash \
> >     tst-leaks1 \
> >     tst-stringtable \
> >     tst-tls9 \
> > diff --git a/elf/tst-dl-hash.c b/elf/tst-dl-hash.c
> > new file mode 100644
> > index 0000000000..e806a274ca
> > --- /dev/null
> > +++ b/elf/tst-dl-hash.c
> > @@ -0,0 +1,147 @@
> > +/* 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/>.  */
> > +/* Simple implementation of ELF ABI hash function. */
>
> The one line description is typically the first line at the top, just
> before the copyright notice.  And perhaps you want to call it "Test ELF
> ABI hash functions" or something similar :)

Fixed in V10.
>
> > +
> > +#include <dl-hash.h>
> > +#include <dl-new-hash.h>
> > +#include <support/support.h>
> > +#include <support/check.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +#include <stdlib.h>
> > +
> > +typedef unsigned int (*hash_f) (const char *);
> > +
> > +static unsigned int
> > +simple_dl_new_hash (const char *s)
> > +{
> > +  uint32_t h = 5381;
> > +  for (unsigned char c = *s; c != '\0'; c = *++s)
> > +    h = h * 33 + c;
> > +  return h;
> > +}
>
> Maybe just `#define dl_new_hash simple_dl_new_hash` and include
> elf/dl-new-hash.h here?  And then don't get rid of elf/dl-new-hash.h in
> 6/6, let it remain the reference implementation to test against.
> Perhaps also add a comment in that file stating that it is a reference
> implementation to test against and that sysdeps has the actual
> implementation that gets used, depending on the target.

Done in V10.
>
> > +
> > +static unsigned int
> > +simple_dl_elf_hash (const char *name_arg)
> > +{
> > +  unsigned long int hash = 0;
> > +  for (unsigned char c = *name_arg; c != '\0'; c = *(++name_arg))
> > +    {
> > +      unsigned long int hi;
> > +      hash = (hash << 4) + c;
> > +      hi = hash & 0xf0000000;
> > +      hash ^= hi >> 24;
> > +      hash &= 0x0fffffff;
> > +    }
> > +  return hash;
> > +}
>
> Likewise, add elf/dl-hash.h with this reference implementation.

Done in V10.
>
> > +static int
> > +do_fill_test (size_t len, int fill, const char *name, hash_f testf,
> > +           hash_f expecf)
> > +{
> > +  uint32_t expec, res;
> > +  char buf[len + 1];
> > +  memset (buf, fill, len);
> > +  buf[len] = '\0';
> > +
> > +  expec = expecf (buf);
> > +  res = testf (buf);
> > +  if (expec != res)
> > +    {
> > +      FAIL_EXIT1 ("FAIL: fill(%d) %s(%zu), %x != %x\n", fill, name, len, expec,
> > +               res);
> > +    }
> > +
> > +  return 0;
> > +}
> > +
> > +static int
> > +do_fill_tests (size_t len, int fill)
> > +{
> > +  if (do_fill_test (len, fill, "dl_new_hash", &_dl_new_hash,
> > +                 &simple_dl_new_hash))
> > +    {
>
> Redundant paranthesis.
Fixed in V10.
>
> > +      return 1;
> > +    }
> > +  return do_fill_test (len, fill, "dl_elf_hash", &_dl_elf_hash,
> > +                    &simple_dl_elf_hash);
> > +}
> > +
> > +static int
> > +do_rand_test (size_t len, const char *name, hash_f testf, hash_f expecf)
> > +{
> > +  uint32_t expec, res;
> > +  size_t i;
> > +  char buf[len + 1];
> > +  char v;
> > +  for (i = 0; i < len; ++i)
> > +    {
> > +      v = random ();
> > +      if (v == 0)
> > +     {
>
> Likewise.
Fixed in V10.
>
> > +       v = 1;
> > +     }
> > +      buf[i] = v;
> > +    }
> > +  buf[len] = '\0';
> > +
> > +  expec = expecf (buf);
> > +  res = testf (buf);
> > +  if (expec != res)
> > +    {
> > +      printf ("FAIL: random %s(%zu), %x != %x\n", name, len, expec, res);
> > +      return 1;
> > +    }
> > +
> > +  return 0;
> > +}
> > +
> > +static int
> > +do_rand_tests (size_t len)
> > +{
> > +  if (do_rand_test (len, "dl_new_hash", &_dl_new_hash, &simple_dl_new_hash))
> > +    {
>
> Likewise.
Fixed in V10.
>
> > +      return 1;
> > +    }
> > +  return do_rand_test (len, "dl_elf_hash", &_dl_elf_hash, &simple_dl_elf_hash);
> > +}
> > +
> > +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))
> > +         {
>
> Likewise.
Fixed in V10.
>
> > +           return 1;
> > +         }
> > +
> > +       if (do_fill_tests (i, -1) || do_fill_tests (i, 1)
> > +           || do_fill_tests (i, 0x80) || do_fill_tests (i, 0x88))
> > +         {
>
> Likewise.
Fixed in V10.
>
> > +           return 1;
> > +         }
> > +     }
> > +    }
> > +  return 0;
> > +}
> > +
> > +#include <support/test-driver.c>
>