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

[Siddhesh Poyarekar <siddhesh@gotplt.org>]

On 19/05/2022 20:20, Noah Goldstein wrote:
> 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?

Ah not that.  What I'd have done was to keep it as a real implementation 
and just have the sysdeps one override it, like some of the posix 
implementations that return ENOSYS and then implementations in sysdeps 
override them.

Then you include <elf/dl-new-hash.h> in the test case as a reference 
implementations, like the string tests do for some of the C string 
implementations.

But what you've done is also fine.

Siddhesh