This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Consolidate object search in DSO in _dl_find_dso_for_object
- From: "Carlos O'Donell" <carlos at redhat dot com>
- To: Siddhesh Poyarekar <siddhesh at redhat dot com>
- Cc: libc-alpha at sourceware dot org
- Date: Fri, 15 Feb 2013 13:33:20 -0500
- Subject: Re: [PATCH] Consolidate object search in DSO in _dl_find_dso_for_object
- References: <20130215114306.GJ27403@spoyarek.pnq.redhat.com>
On 02/15/2013 06:43 AM, Siddhesh Poyarekar wrote:
> Hi,
>
> As Carlos, suggested, here's a patch to consolidate the object search
> loop in various places in current code into a separate function.
> There are only 3 places where this consolidation is needed (4 with
> __cxa_thread_atexit_impl); the other two have a completely different
> logic.
Thanks for doing this clean up, I appreciate it.
> Built and regression tested on x86_64. OK to commit?
>
> Siddhesh
>
> * elf/Versions (ld): Add _dl_find_dso_for_object.
> * elf/dl-addr.c (_dl_addr): Use _dl_find_dso_for_object.
> * elf/dl-open.c (_dl_find_dso_for_object): New function.
> (dl_open_worker): Use _dl_find_dso_for_object.
> * elf/dl-sym.c (do_sym): Likewise.
> * sysdeps/generic/ldsodefs.h: Declare _dl_find_dso_for_object.
>
> diff --git a/elf/Versions b/elf/Versions
> index 87e27c5..97615de 100644
> --- a/elf/Versions
> +++ b/elf/Versions
> @@ -62,5 +62,6 @@ ld {
> _dl_debug_state;
> # Pointer protection.
> __pointer_chk_guard;
> + _dl_find_dso_for_object;
While Andreas says the list should be alphabetical it doesn't look like it is :-)
I guess it would be a distinct patch to sort the list first?
How deep does the rabbit hole go?
> }
> }
> diff --git a/elf/dl-addr.c b/elf/dl-addr.c
> index 91cc443..a533466 100644
> --- a/elf/dl-addr.c
> +++ b/elf/dl-addr.c
> @@ -130,18 +130,14 @@ _dl_addr (const void *address, Dl_info *info,
> /* Protect against concurrent loads and unloads. */
> __rtld_lock_lock_recursive (GL(dl_load_lock));
>
> - /* Find the highest-addressed object that ADDRESS is not below. */
> - for (Lmid_t ns = 0; ns < GL(dl_nns); ++ns)
> - for (struct link_map *l = GL(dl_ns)[ns]._ns_loaded; l; l = l->l_next)
> - if (addr >= l->l_map_start && addr < l->l_map_end
> - && (l->l_contiguous || _dl_addr_inside_object (l, addr)))
> - {
> - determine_info (addr, l, info, mapp, symbolp);
> - result = 1;
> - goto out;
> - }
> + struct link_map *l = _dl_find_dso_for_object (addr);
> +
> + if (l)
> + {
> + determine_info (addr, l, info, mapp, symbolp);
> + result = 1;
> + }
>
> - out:
> __rtld_lock_unlock_recursive (GL(dl_load_lock));
Good. And we get rid of a goto.
>
> return result;
> diff --git a/elf/dl-open.c b/elf/dl-open.c
> index 67f7e73..1c2f864 100644
> --- a/elf/dl-open.c
> +++ b/elf/dl-open.c
> @@ -165,6 +165,26 @@ add_to_global (struct link_map *new)
> return 0;
> }
>
Add a function level comment saying what the function does,
what namespace it searches, for what, and what it returns
under which conditions.
> +struct link_map *
> +internal_function
> +_dl_find_dso_for_object (const ElfW(Addr) addr)
> +{
> + struct link_map *l;
> +
> + /* Find the highest-addressed object that ADDR is not below. */
> + for (Lmid_t ns = 0; ns < GL(dl_nns); ++ns)
> + for (l = GL(dl_ns)[ns]._ns_loaded; l != NULL; l = l->l_next)
> + if (addr >= l->l_map_start && addr < l->l_map_end
> + && (l->l_contiguous
> + || _dl_addr_inside_object (l, (ElfW(Addr)) addr)))
> + {
> + assert (ns == l->l_ns);
> + return l;
> + }
> + return NULL;
> +}
> +rtld_hidden_def (_dl_find_dso_for_object);
> +
Looks good. Correct definition visibility.
> static void
> dl_open_worker (void *a)
> {
> @@ -194,20 +214,11 @@ dl_open_worker (void *a)
> call_map = GL(dl_ns)[LM_ID_BASE]._ns_loaded;
> #endif
>
> - struct link_map *l;
> - for (Lmid_t ns = 0; ns < GL(dl_nns); ++ns)
> - for (l = GL(dl_ns)[ns]._ns_loaded; l != NULL; l = l->l_next)
> - if (caller_dlopen >= (const void *) l->l_map_start
> - && caller_dlopen < (const void *) l->l_map_end
> - && (l->l_contiguous
> - || _dl_addr_inside_object (l, (ElfW(Addr)) caller_dlopen)))
> - {
> - assert (ns == l->l_ns);
> - call_map = l;
> - goto found_caller;
> - }
> + struct link_map *l = _dl_find_dso_for_object ((ElfW(Addr)) caller_dlopen);
> +
> + if (l)
> + call_map = l;
>
> - found_caller:
OK.
> if (args->nsid == __LM_ID_CALLER)
> {
> #ifndef SHARED
> diff --git a/elf/dl-sym.c b/elf/dl-sym.c
> index d2b4db7..05de6c1 100644
> --- a/elf/dl-sym.c
> +++ b/elf/dl-sym.c
> @@ -91,20 +91,10 @@ do_sym (void *handle, const char *name, void *who,
> lookup_t result;
> ElfW(Addr) caller = (ElfW(Addr)) who;
>
> + struct link_map *l = _dl_find_dso_for_object (caller);
> /* If the address is not recognized the call comes from the main
> program (we hope). */
> - struct link_map *match = GL(dl_ns)[LM_ID_BASE]._ns_loaded;
> -
> - /* Find the highest-addressed object that CALLER is not below. */
> - for (Lmid_t ns = 0; ns < GL(dl_nns); ++ns)
> - for (struct link_map *l = GL(dl_ns)[ns]._ns_loaded; l != NULL;
> - l = l->l_next)
> - if (caller >= l->l_map_start && caller < l->l_map_end
> - && (l->l_contiguous || _dl_addr_inside_object (l, caller)))
> - {
> - match = l;
> - break;
> - }
> + struct link_map *match = l ? l : GL(dl_ns)[LM_ID_BASE]._ns_loaded;
OK.
>
> if (handle == RTLD_DEFAULT)
> {
> diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
> index d6350fa..01a2712 100644
> --- a/sysdeps/generic/ldsodefs.h
> +++ b/sysdeps/generic/ldsodefs.h
> @@ -1006,6 +1006,10 @@ extern int _dl_addr_inside_object (struct link_map *l, const ElfW(Addr) addr)
> /* Show show of an object. */
> extern void _dl_show_scope (struct link_map *new, int from);
>
> +extern struct link_map *_dl_find_dso_for_object (const ElfW(Addr) addr)
> + internal_function;
> +rtld_hidden_proto (_dl_find_dso_for_object)
> +
OK.
> __END_DECLS
>
> #endif /* ldsodefs.h */
>
I'd say OK to checkin with:
* Alphabetically sorted Version entries.
* Verbose comment for utility function.
Cheers,
Carlos.