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

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

On Thu, May 19, 2022 at 9:47 AM Siddhesh Poyarekar <siddhesh@gotplt.org> wrote:
>
> On 18/05/2022 22:56, Noah Goldstein via Libc-alpha wrote:
> > No change to the code other than moving the function to
> > dl-new-hash.h. Changed name so its now in the reserved namespace.
> > ---
> >   elf/dl-lookup.c   | 13 ++-----------
> >   elf/dl-new-hash.h | 40 ++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 42 insertions(+), 11 deletions(-)
> >   create mode 100644 elf/dl-new-hash.h
> >
> > diff --git a/elf/dl-lookup.c b/elf/dl-lookup.c
> > index 989b073e4f..a42f6d5390 100644
> > --- a/elf/dl-lookup.c
> > +++ b/elf/dl-lookup.c
> > @@ -24,6 +24,7 @@
> >   #include <ldsodefs.h>
> >   #include <dl-hash.h>
> >   #include <dl-machine.h>
> > +#include <dl-new-hash.h>
> >   #include <dl-protected.h>
> >   #include <sysdep-cancel.h>
> >   #include <libc-lock.h>
> > @@ -558,16 +559,6 @@ skip:
> >   }
> >
> >
> > -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;
> > -}
> > -
> > -
> >   /* Add extra dependency on MAP to UNDEF_MAP.  */
> >   static int
> >   add_dependency (struct link_map *undef_map, struct link_map *map, int flags)
> > @@ -816,7 +807,7 @@ _dl_lookup_symbol_x (const char *undef_name, struct link_map *undef_map,
> >                    const struct r_found_version *version,
> >                    int type_class, int flags, struct link_map *skip_map)
> >   {
> > -  const unsigned int new_hash = dl_new_hash (undef_name);
> > +  const unsigned int new_hash = _dl_new_hash (undef_name);
> >     unsigned long int old_hash = 0xffffffff;
> >     struct sym_val current_value = { NULL, NULL };
> >     struct r_scope_elem **scope = symbol_scope;
> > diff --git a/elf/dl-new-hash.h b/elf/dl-new-hash.h
> > new file mode 100644
> > index 0000000000..8641bb4196
> > --- /dev/null
> > +++ b/elf/dl-new-hash.h
> > @@ -0,0 +1,40 @@
> > +/* _dl_new_hash for elf symbol lookup
> > +   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 _DL_NEW_HASH_H
> > +#define _DL_NEW_HASH_H 1
> > +
> > +#include <stdint.h>
> > +/* For __always_inline.  */
> > +#include <sys/cdefs.h>
> > +
> > +static __always_inline uint32_t
> > +__attribute__ ((unused))
> > +_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;
> > +}
> > +
> > +/* For testing/benchmarking purposes.  */
> > +#define __simple_dl_new_hash _dl_new_hash
> > +
> > +
> > +#endif /* dl-new-hash.h */
>
> Uhmm, you're going to call it __simple_dl_new_hash.  A bit roundabout,
> but OK.

It's in a header. Doesn't it need to be in reserved namespace?
>
> Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>