Re: [glibc] elf: Rewrite long RESOLVE_MAP macro to an always_inline static function

Re: [glibc] elf: Rewrite long RESOLVE_MAP macro to an always_inline static function

[Carlos O'Donell]

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.

Re: [glibc] elf: Rewrite long RESOLVE_MAP macro to an always_inline static function

[Fangrui Song]

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.
>

Re: [glibc] elf: Rewrite long RESOLVE_MAP macro to an always_inline static function

[Carlos O'Donell]

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.