Re: [PATCH v1 1/6] elf: Refactor dl_new_hash so it can be tested / benchmarked

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

On Mon, Apr 25, 2022 at 10:59 AM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 14/04/2022 01:12, Noah Goldstein via Libc-alpha wrote:
> > No change to the code other than moving it to
> > sysdeps/generic/dl-hash.h. Changed name so its now in the
> > reserved namespace.
> > ---
> >  sysdeps/generic/dl-hash.h   | 13 +++++++++++++
> >  sysdeps/i386/i686/dl-hash.h |  3 +++
> >  2 files changed, 16 insertions(+)
> >
> > diff --git a/sysdeps/generic/dl-hash.h b/sysdeps/generic/dl-hash.h
> > index 9bc7e3bd67..c041074352 100644
> > --- a/sysdeps/generic/dl-hash.h
> > +++ b/sysdeps/generic/dl-hash.h
> > @@ -19,7 +19,9 @@
> >  #ifndef _DL_HASH_H
> >  #define _DL_HASH_H   1
> >
> > +#include <stdint.h>
> >
> > +#ifndef __HAS_DL_ELF_HASH
> >  /* This is the hashing function specified by the ELF ABI.  In the
> >     first five operations no overflow is possible so we optimized it a
> >     bit.  */
> > @@ -71,5 +73,16 @@ _dl_elf_hash (const char *name_arg)
> >      }
> >    return hash;
> >  }
> > +#endif /* !__HAS_DL_ELF_HASH */
> > +
> > +static uint32_t
> > +__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;
> > +}
> > +
> >
> >  #endif /* dl-hash.h */
>
> Since you refactoring it, it would be better to remove the elf/dl-lookup.c
> version.

V2 removes the impl in elf/dl-lookup.c
>
> > diff --git a/sysdeps/i386/i686/dl-hash.h b/sysdeps/i386/i686/dl-hash.h
> > index c124480e77..d18370350d 100644
> > --- a/sysdeps/i386/i686/dl-hash.h
> > +++ b/
> > @@ -75,4 +75,7 @@ _dl_elf_hash (const char *name)
> >    return result;
> >  }
> >
> > +#define __HAS_DL_ELF_HASH    1
> > +#include <sysdeps/generic/dl-hash.h>
> > +
> >  #endif /* dl-hash.h */
>
> Do we still need this file? The comments seems quite outdated and I think
> it would be better to just remove it

Not really sure. The commit no longer touches it as of V2.

The patchset ultimately doesn't update `_dl_elf_hash` (there appears to
be room for improvement; I just couldn't get a version that had no regression
for any size) so would prefer to leave it as a separate issue.