* Re: [glibc] elf: Rewrite long RESOLVE_MAP macro to an always_inline static function [not found] <20220523190646.4D9683858D32@sourceware.org> @ 2022-05-24 0:12 ` Carlos O'Donell 2022-05-24 0:21 ` Fangrui Song 0 siblings, 1 reply; 3+ messages in thread From: Carlos O'Donell @ 2022-05-24 0:12 UTC (permalink / raw) To: Fangrui Song, libc-alpha, Adhemerval Zanella, Siddhesh Poyarekar On 5/23/22 15:06, Fangrui Song via Glibc-cvs wrote: > https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=a8b11bd1f8dc68795b377138b5d94638ef75a50d > > commit a8b11bd1f8dc68795b377138b5d94638ef75a50d > Author: Nicholas Guriev <nicholas@guriev.su> > Date: Mon May 23 12:06:44 2022 -0700 > > elf: Rewrite long RESOLVE_MAP macro to an always_inline static function > > An __always_inline static function is better to find where exactly a > crash happens, so one can step into the function with GDB. > > Reviewed-by: Fangrui Song <maskray@google.com> > Fangrui, It looks like we missed the Signed-off-by from the original author? The original authors posts contained it, but this commit lacks it. Could you please commit a change to dl-reloc.c which adds the "Copyright The GNU Toolchain Authors." following how it is done for other files (after other Copyright lines e.g. malloc/malloc.c example)? It would be valuable if the commit message referenced this commit to ensure future readers can put the two together. Mentioning we missed the original Signed-off-by might be good too. Thank you. > Diff: > --- > elf/dl-reloc.c | 56 ++++++++++++++++++++++++++++++++++---------------------- > 1 file changed, 34 insertions(+), 22 deletions(-) > > diff --git a/elf/dl-reloc.c b/elf/dl-reloc.c > index 771a34bd14..4267c2efb6 100644 > --- a/elf/dl-reloc.c > +++ b/elf/dl-reloc.c > @@ -162,29 +162,41 @@ _dl_nothread_init_static_tls (struct link_map *map) > } > #endif /* !PTHREAD_IN_LIBC */ > > +static __always_inline lookup_t > +resolve_map (lookup_t l, struct r_scope_elem *scope[], const ElfW(Sym) **ref, > + const struct r_found_version *version, unsigned long int r_type) > +{ > + if (ELFW(ST_BIND) ((*ref)->st_info) == STB_LOCAL > + || __glibc_unlikely (dl_symbol_visibility_binds_local_p (*ref))) > + return l; > + > + if (__glibc_unlikely (*ref == l->l_lookup_cache.sym) > + && elf_machine_type_class (r_type) == l->l_lookup_cache.type_class) > + { > + bump_num_cache_relocations (); > + *ref = l->l_lookup_cache.ret; > + } > + else > + { > + const int tc = elf_machine_type_class (r_type); > + l->l_lookup_cache.type_class = tc; > + l->l_lookup_cache.sym = *ref; > + const char *undef_name > + = (const char *) D_PTR (l, l_info[DT_STRTAB]) + (*ref)->st_name; > + const struct r_found_version *v = NULL; > + if (version != NULL && version->hash != 0) > + v = version; > + lookup_t lr = _dl_lookup_symbol_x ( > + undef_name, l, ref, scope, v, tc, > + DL_LOOKUP_ADD_DEPENDENCY | DL_LOOKUP_FOR_RELOCATE, NULL); > + l->l_lookup_cache.ret = *ref; > + l->l_lookup_cache.value = lr; > + } > + return l->l_lookup_cache.value; > +} > + > /* This macro is used as a callback from the ELF_DYNAMIC_RELOCATE code. */ > -#define RESOLVE_MAP(l, scope, ref, version, r_type) \ > - ((ELFW(ST_BIND) ((*ref)->st_info) != STB_LOCAL \ > - && __glibc_likely (!dl_symbol_visibility_binds_local_p (*ref))) \ > - ? ((__glibc_unlikely ((*ref) == l->l_lookup_cache.sym) \ > - && elf_machine_type_class (r_type) == l->l_lookup_cache.type_class) \ > - ? (bump_num_cache_relocations (), \ > - (*ref) = l->l_lookup_cache.ret, \ > - l->l_lookup_cache.value) \ > - : ({ lookup_t _lr; \ > - int _tc = elf_machine_type_class (r_type); \ > - l->l_lookup_cache.type_class = _tc; \ > - l->l_lookup_cache.sym = (*ref); \ > - const struct r_found_version *v = NULL; \ > - if ((version) != NULL && (version)->hash != 0) \ > - v = (version); \ > - _lr = _dl_lookup_symbol_x ((const char *) D_PTR (l, l_info[DT_STRTAB]) + (*ref)->st_name, \ > - l, (ref), scope, v, _tc, \ > - DL_LOOKUP_ADD_DEPENDENCY \ > - | DL_LOOKUP_FOR_RELOCATE, NULL); \ > - l->l_lookup_cache.ret = (*ref); \ > - l->l_lookup_cache.value = _lr; })) \ > - : l) > +#define RESOLVE_MAP resolve_map > > #include "dynamic-link.h" > -- Cheers, Carlos. ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [glibc] elf: Rewrite long RESOLVE_MAP macro to an always_inline static function 2022-05-24 0:12 ` [glibc] elf: Rewrite long RESOLVE_MAP macro to an always_inline static function Carlos O'Donell @ 2022-05-24 0:21 ` Fangrui Song 2022-05-24 0:25 ` Carlos O'Donell 0 siblings, 1 reply; 3+ messages in thread From: Fangrui Song @ 2022-05-24 0:21 UTC (permalink / raw) To: Carlos O'Donell; +Cc: libc-alpha, Adhemerval Zanella, Siddhesh Poyarekar On 2022-05-23, Carlos O'Donell wrote: >On 5/23/22 15:06, Fangrui Song via Glibc-cvs wrote: >> https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=a8b11bd1f8dc68795b377138b5d94638ef75a50d >> >> commit a8b11bd1f8dc68795b377138b5d94638ef75a50d >> Author: Nicholas Guriev <nicholas@guriev.su> >> Date: Mon May 23 12:06:44 2022 -0700 >> >> elf: Rewrite long RESOLVE_MAP macro to an always_inline static function >> >> An __always_inline static function is better to find where exactly a >> crash happens, so one can step into the function with GDB. >> >> Reviewed-by: Fangrui Song <maskray@google.com> >> > >Fangrui, > >It looks like we missed the Signed-off-by from the original author? The original >authors posts contained it, but this commit lacks it. > >Could you please commit a change to dl-reloc.c which adds the "Copyright The GNU >Toolchain Authors." following how it is done for other files (after other Copyright >lines e.g. malloc/malloc.c example)? > >It would be valuable if the commit message referenced this commit to ensure future >readers can put the two together. Mentioning we missed the original Signed-off-by >might be good too. > >Thank you. Hi Carlos, appreciate you comment:) I am sorry that I was thinking "seems that most commits don't have Signed-off-by:, the tag seems redundant." With your reply, I have searched around and found this rule: https://sourceware.org/glibc/wiki/Contribution%20checklist > The glibc project does not require tags from anyone other than the > original author of the patch, but other people involved in passing along > the patch can add their own Signed-off-by if they wish. Again, sorry for my mistake... I have found `The GNU Toolchain Authors.` at 3.5. Update copyright information and will follow some existing commits to add the tag. >> Diff: >> --- >> elf/dl-reloc.c | 56 ++++++++++++++++++++++++++++++++++---------------------- >> 1 file changed, 34 insertions(+), 22 deletions(-) >> >> diff --git a/elf/dl-reloc.c b/elf/dl-reloc.c >> index 771a34bd14..4267c2efb6 100644 >> --- a/elf/dl-reloc.c >> +++ b/elf/dl-reloc.c >> @@ -162,29 +162,41 @@ _dl_nothread_init_static_tls (struct link_map *map) >> } >> #endif /* !PTHREAD_IN_LIBC */ >> >> +static __always_inline lookup_t >> +resolve_map (lookup_t l, struct r_scope_elem *scope[], const ElfW(Sym) **ref, >> + const struct r_found_version *version, unsigned long int r_type) >> +{ >> + if (ELFW(ST_BIND) ((*ref)->st_info) == STB_LOCAL >> + || __glibc_unlikely (dl_symbol_visibility_binds_local_p (*ref))) >> + return l; >> + >> + if (__glibc_unlikely (*ref == l->l_lookup_cache.sym) >> + && elf_machine_type_class (r_type) == l->l_lookup_cache.type_class) >> + { >> + bump_num_cache_relocations (); >> + *ref = l->l_lookup_cache.ret; >> + } >> + else >> + { >> + const int tc = elf_machine_type_class (r_type); >> + l->l_lookup_cache.type_class = tc; >> + l->l_lookup_cache.sym = *ref; >> + const char *undef_name >> + = (const char *) D_PTR (l, l_info[DT_STRTAB]) + (*ref)->st_name; >> + const struct r_found_version *v = NULL; >> + if (version != NULL && version->hash != 0) >> + v = version; >> + lookup_t lr = _dl_lookup_symbol_x ( >> + undef_name, l, ref, scope, v, tc, >> + DL_LOOKUP_ADD_DEPENDENCY | DL_LOOKUP_FOR_RELOCATE, NULL); >> + l->l_lookup_cache.ret = *ref; >> + l->l_lookup_cache.value = lr; >> + } >> + return l->l_lookup_cache.value; >> +} >> + >> /* This macro is used as a callback from the ELF_DYNAMIC_RELOCATE code. */ >> -#define RESOLVE_MAP(l, scope, ref, version, r_type) \ >> - ((ELFW(ST_BIND) ((*ref)->st_info) != STB_LOCAL \ >> - && __glibc_likely (!dl_symbol_visibility_binds_local_p (*ref))) \ >> - ? ((__glibc_unlikely ((*ref) == l->l_lookup_cache.sym) \ >> - && elf_machine_type_class (r_type) == l->l_lookup_cache.type_class) \ >> - ? (bump_num_cache_relocations (), \ >> - (*ref) = l->l_lookup_cache.ret, \ >> - l->l_lookup_cache.value) \ >> - : ({ lookup_t _lr; \ >> - int _tc = elf_machine_type_class (r_type); \ >> - l->l_lookup_cache.type_class = _tc; \ >> - l->l_lookup_cache.sym = (*ref); \ >> - const struct r_found_version *v = NULL; \ >> - if ((version) != NULL && (version)->hash != 0) \ >> - v = (version); \ >> - _lr = _dl_lookup_symbol_x ((const char *) D_PTR (l, l_info[DT_STRTAB]) + (*ref)->st_name, \ >> - l, (ref), scope, v, _tc, \ >> - DL_LOOKUP_ADD_DEPENDENCY \ >> - | DL_LOOKUP_FOR_RELOCATE, NULL); \ >> - l->l_lookup_cache.ret = (*ref); \ >> - l->l_lookup_cache.value = _lr; })) \ >> - : l) >> +#define RESOLVE_MAP resolve_map >> >> #include "dynamic-link.h" >> > > >-- >Cheers, >Carlos. > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [glibc] elf: Rewrite long RESOLVE_MAP macro to an always_inline static function 2022-05-24 0:21 ` Fangrui Song @ 2022-05-24 0:25 ` Carlos O'Donell 0 siblings, 0 replies; 3+ messages in thread From: Carlos O'Donell @ 2022-05-24 0:25 UTC (permalink / raw) To: Fangrui Song; +Cc: libc-alpha, Adhemerval Zanella, Siddhesh Poyarekar On 5/23/22 20:21, Fangrui Song wrote: > On 2022-05-23, Carlos O'Donell wrote: >> On 5/23/22 15:06, Fangrui Song via Glibc-cvs wrote: >>> https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=a8b11bd1f8dc68795b377138b5d94638ef75a50d >>> >>> commit a8b11bd1f8dc68795b377138b5d94638ef75a50d >>> Author: Nicholas Guriev <nicholas@guriev.su> >>> Date: Mon May 23 12:06:44 2022 -0700 >>> >>> elf: Rewrite long RESOLVE_MAP macro to an always_inline static function >>> >>> An __always_inline static function is better to find where exactly a >>> crash happens, so one can step into the function with GDB. >>> >>> Reviewed-by: Fangrui Song <maskray@google.com> >>> >> >> Fangrui, >> >> It looks like we missed the Signed-off-by from the original author? The original >> authors posts contained it, but this commit lacks it. >> >> Could you please commit a change to dl-reloc.c which adds the "Copyright The GNU >> Toolchain Authors." following how it is done for other files (after other Copyright >> lines e.g. malloc/malloc.c example)? >> >> It would be valuable if the commit message referenced this commit to ensure future >> readers can put the two together. Mentioning we missed the original Signed-off-by >> might be good too. >> >> Thank you. > > Hi Carlos, appreciate you comment:) I am sorry that I was thinking > "seems that most commits don't have Signed-off-by:, the tag seems redundant." > > With your reply, I have searched around and found this rule: > > https://sourceware.org/glibc/wiki/Contribution%20checklist > >> The glibc project does not require tags from anyone other than the >> original author of the patch, but other people involved in passing along >> the patch can add their own Signed-off-by if they wish. > > Again, sorry for my mistake... I have found `The GNU Toolchain Authors.` > at 3.5. Update copyright information and will follow some existing > commits to add the tag. Thank you very much. I apologize that we as a community do not have process which is easy to follow and self correcting (git commit hooks which DTRT). And I very much appreciate you help reviewing patches :-) -- Cheers, Carlos. ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-05-24 0:25 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20220523190646.4D9683858D32@sourceware.org>
2022-05-24 0:12 ` [glibc] elf: Rewrite long RESOLVE_MAP macro to an always_inline static function Carlos O'Donell
2022-05-24 0:21 ` Fangrui Song
2022-05-24 0:25 ` Carlos O'Donell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).