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

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(+)

On Wed, Apr 13, 2022 at 9:12 PM Noah Goldstein <goldstein.w.n@gmail.com> 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)

I think this should be put in a new header file, dl-new-hash.h, and rename
the function to _dl_new_hash.

> +{
> +  uint32_t h = 5381;
> +  for (unsigned char c = *s; c != '\0'; c = *++s)
> +    h = h * 33 + c;
> +  return h;
> +}
> +
>
>  #endif /* dl-hash.h */
> 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/sysdeps/i386/i686/dl-hash.h
> @@ -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 */
> --
> 2.25.1
>

On Wed, Apr 13, 2022 at 11:32 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Wed, Apr 13, 2022 at 9:12 PM Noah Goldstein <goldstein.w.n@gmail.com> 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)
>
> I think this should be put in a new header file, dl-new-hash.h, and rename
> the function to _dl_new_hash.

Fixed in v2.

>
> > +{
> > +  uint32_t h = 5381;
> > +  for (unsigned char c = *s; c != '\0'; c = *++s)
> > +    h = h * 33 + c;
> > +  return h;
> > +}
> > +
> >
> >  #endif /* dl-hash.h */
> > 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/sysdeps/i386/i686/dl-hash.h
> > @@ -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 */
> > --
> > 2.25.1
> >
>
>
> --
> H.J.

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.

> 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.

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.