Message ID | 20220518172635.291119-1-goldstein.w.n@gmail.com |
---|---|
State | New |
Headers | show |
Series | [v10,1/6] elf: Refactor dl_new_hash so it can be tested / benchmarked | expand |
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. Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
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>
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
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 */