[v1,1/6] elf: Refactor dl_new_hash so it can be tested / benchmarked
Checks
Context |
Check |
Description |
dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
Commit Message
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(+)
Comments
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.
@@ -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 */
@@ -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 */