elf: Rewrite long RESOLVE_MAP macro to a debug friendly function

  A static function that may be inlined is way better to find where exactly a
crash happens. So one can step into the function with GDB. The macro still
remains for compatibility with other code.

Signed-off-by: Nicholas Guriev <nicholas@guriev.su>
---

I was playing with the DT_AUXILIARY flag and run into a crash in the
RESOLVE_MAP macro. Alas, GDB is unable to see what is going on in a
multi-line macro, so I turned it to a static function. Hope this is useful.

 elf/dl-reloc.c | 55 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 22 deletions(-)
  

On 2022-05-02, Nicholas Guriev wrote:
>A static function that may be inlined is way better to find where exactly a
>crash happens. So one can step into the function with GDB. The macro still
>remains for compatibility with other code.

I think this is useful. Does the size of dl-reloc.os code increase or
decrease with the change?

>Signed-off-by: Nicholas Guriev <nicholas@guriev.su>
>---
>
>I was playing with the DT_AUXILIARY flag and run into a crash in the
>RESOLVE_MAP macro. Alas, GDB is unable to see what is going on in a
>multi-line macro, so I turned it to a static function. Hope this is useful.
>
> elf/dl-reloc.c | 55 ++++++++++++++++++++++++++++++--------------------
> 1 file changed, 33 insertions(+), 22 deletions(-)
>
>diff --git a/elf/dl-reloc.c b/elf/dl-reloc.c
>index 771a34bd14..88d7fee041 100644
>--- a/elf/dl-reloc.c
>+++ b/elf/dl-reloc.c
>@@ -162,29 +162,40 @@ _dl_nothread_init_static_tls (struct link_map *map)
> }
> #endif /* !PTHREAD_IN_LIBC */
>
>+static lookup_t
>+_dl_resolve_map (lookup_t l, struct r_scope_elem *scope[], const ElfW(Sym) **ref,
>+		 const struct r_found_version *version, unsigned long 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
>+    {
>+      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);

Use `lookup_t _lr = ` ?

>+      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 _dl_resolve_map
>
> #include "dynamic-link.h"
>
>-- 
>2.32.0
>
  

On Пн, 2022-05-02 at 14:20 -0700, Fangrui Song wrote:
> I think this is useful. Does the size of dl-reloc.os code increase or
> decrease with the change?
> 

It's an interesting thing but the file has decreased by 7472 bytes with
-Og and by 6896 bytes with -O3 optimization level (debug info enabled).
Despite that readelf(1) sees a new local symbol, _dl_resolve_map.

When debug info is disabled with the -g0 flag, size of the file is also
reduced by 3296 and 1440 bytes respectively.
  

On 2022-05-03, Nicholas Guriev wrote:
>On Пн, 2022-05-02 at 14:20 -0700, Fangrui Song wrote:
>> I think this is useful. Does the size of dl-reloc.os code increase or
>> decrease with the change?
>>
>
>It's an interesting thing but the file has decreased by 7472 bytes with
>-Og and by 6896 bytes with -O3 optimization level (debug info enabled).
>Despite that readelf(1) sees a new local symbol, _dl_resolve_map.
>
>When debug info is disabled with the -g0 flag, size of the file is also
>reduced by 3296 and 1440 bytes respectively.

This is because in the default build mode (gcc -O2 -fPIC), gcc decides
to not inline _dl_resolve_map. That said, I run LD_DEBUG=statistics on
a large PIE (500+MiB) and do not see meaningful cycle difference.

The function has internal linkage (static) and it seems that the
prevailing style doesn't use `_dl_` prefix for such functions.

Otherwise the patch looks good to me.