This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Consolidate object search in DSO in _dl_find_dso_for_object


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]