elf: Rewrite long RESOLVE_MAP macro to a debug friendly function
Checks
Context |
Check |
Description |
dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
dj/TryBot-32bit |
success
|
Build for i686
|
Commit Message
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(-)
Comments
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.
@@ -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);
+ 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"