[review] dlsym: Do not determine caller link map if not needed

[review] dlsym: Do not determine caller link map if not needed

[Florian Weimer (Code Review)]

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/528
......................................................................

dlsym: Do not determine caller link map if not needed

Obtaining the link map is potentially very slow because it requires
iterating over all loaded objects in the current implementation.  If
the caller supplied an explicit handle (i.e., not one of the RTLD_*
constants), the dlsym implementation does not need the identity of the
caller (except in the special cause of auditing), so this change
avoids computing it in that case.

Even in the minimal case (dlsym called from a main program linked with
-dl), this shows a small speedup, perhaps around five percent.  The
performance improvement can be arbitrarily large in principle (if
_dl_find_dso_for_object has to iterate over many link maps).

Change-Id: Ide5d9e2cc7ac25a0ffae8fb4c26def0c898efa29
---
M elf/dl-sym.c
1 file changed, 21 insertions(+), 4 deletions(-)



diff --git a/elf/dl-sym.c b/elf/dl-sym.c
index 8209342..7282e09 100644
--- a/elf/dl-sym.c
+++ b/elf/dl-sym.c
@@ -80,6 +80,18 @@
 					args->flags, NULL);
 }
 
+/* Return the link map containing the caller address.  */
+static inline struct link_map *
+find_caller_link_map (ElfW(Addr) caller)
+{
+  struct link_map *l = _dl_find_dso_for_object (caller);
+  if (l != NULL)
+    return l;
+  else
+    /* If the address is not recognized the call comes from the main
+       program (we hope).  */
+    return GL(dl_ns)[LM_ID_BASE]._ns_loaded;
+}
 
 static void *
 do_sym (void *handle, const char *name, void *who,
@@ -89,13 +101,13 @@
   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 = l ? l : GL(dl_ns)[LM_ID_BASE]._ns_loaded;
+  /* Link map of the caller if needed.  */
+  struct link_map *match = NULL;
 
   if (handle == RTLD_DEFAULT)
     {
+      match = find_caller_link_map (caller);
+
       /* Search the global scope.  We have the simple case where
 	 we look up in the scope of an object which was part of
 	 the initial binary.  And then the more complex part
@@ -128,6 +140,8 @@
     }
   else if (handle == RTLD_NEXT)
     {
+      match = find_caller_link_map (caller);
+
       if (__glibc_unlikely (match == GL(dl_ns)[LM_ID_BASE]._ns_loaded))
 	{
 	  if (match == NULL
@@ -187,6 +201,9 @@
 	  unsigned int ndx = (ref - (ElfW(Sym) *) D_PTR (result,
 							 l_info[DT_SYMTAB]));
 
+	  if (match == NULL)
+	    match = find_caller_link_map (caller);
+
 	  if ((match->l_audit_any_plt | result->l_audit_any_plt) != 0)
 	    {
 	      unsigned int altvalue = 0;

-- 
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: Ide5d9e2cc7ac25a0ffae8fb4c26def0c898efa29
Gerrit-Change-Number: 528
Gerrit-PatchSet: 1
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-MessageType: newchange

[review] dlsym: Do not determine caller link map if not needed

[Szabolcs Nagy (Code Review)]

Szabolcs Nagy has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/528
......................................................................


Patch Set 1: Code-Review+1

the optimization makes sense to me, and the code looks correct.


-- 
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: Ide5d9e2cc7ac25a0ffae8fb4c26def0c898efa29
Gerrit-Change-Number: 528
Gerrit-PatchSet: 1
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Szabolcs Nagy <szabolcs.nagy@arm.com>
Gerrit-Comment-Date: Fri, 15 Nov 2019 12:14:05 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[review] dlsym: Do not determine caller link map if not needed

[Carlos O'Donell (Code Review)]

Carlos O'Donell has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/528
......................................................................


Patch Set 1: Code-Review+2

(6 comments)

Optimization looks correct and covers RTLD_GLOBAL, RTLD_NEXT and audit cases.

OK for master.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

| --- /dev/null
| +++ /COMMIT_MSG
| @@ -1,0 +4,18 @@ AuthorDate: 2019-11-08 15:48:51 +0100
| +Commit:     Florian Weimer <fweimer@redhat.com>
| +CommitDate: 2019-11-08 15:48:51 +0100
| +
| +dlsym: Do not determine caller link map if not needed
| +
| +Obtaining the link map is potentially very slow because it requires
| +iterating over all loaded objects in the current implementation.  If
| +the caller supplied an explicit handle (i.e., not one of the RTLD_*
| +constants), the dlsym implementation does not need the identity of the
| +caller (except in the special cause of auditing), so this change

PS1, Line 13:

s/cause/case/g

| +avoids computing it in that case.
| +
| +Even in the minimal case (dlsym called from a main program linked with
| +-dl), this shows a small speedup, perhaps around five percent.  The
| +performance improvement can be arbitrarily large in principle (if
| +_dl_find_dso_for_object has to iterate over many link maps).

PS1, Line 19:

OK, great reason to fix this. I agree completely.

| +
| +Change-Id: Ide5d9e2cc7ac25a0ffae8fb4c26def0c898efa29
| --- elf/dl-sym.c
| +++ elf/dl-sym.c
| @@ -74,33 +74,45 @@ };
|  static void
|  call_dl_lookup (void *ptr)
|  {
|    struct call_dl_lookup_args *args = (struct call_dl_lookup_args *) ptr;
|    args->map = GLRO(dl_lookup_symbol_x) (args->name, args->map, args->refp,
|  					args->map->l_scope, args->vers, 0,
|  					args->flags, NULL);
|  }
|  
| +/* Return the link map containing the caller address.  */
| +static inline struct link_map *
| +find_caller_link_map (ElfW(Addr) caller)
| +{
| +  struct link_map *l = _dl_find_dso_for_object (caller);
| +  if (l != NULL)
| +    return l;
| +  else
| +    /* If the address is not recognized the call comes from the main
| +       program (we hope).  */
| +    return GL(dl_ns)[LM_ID_BASE]._ns_loaded;
| +}

PS1, Line 94:

OK. Refactored.

|  
|  static void *
|  do_sym (void *handle, const char *name, void *who,
|  	struct r_found_version *vers, int flags)
|  {
|    const ElfW(Sym) *ref = NULL;
|    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 = l ? l : GL(dl_ns)[LM_ID_BASE]._ns_loaded;
| +  /* Link map of the caller if needed.  */
| +  struct link_map *match = NULL;
|  
|    if (handle == RTLD_DEFAULT)
|      {
| +      match = find_caller_link_map (caller);

PS1, Line 109:

OK. Use the new refactored function.

| +
|        /* Search the global scope.  We have the simple case where
|  	 we look up in the scope of an object which was part of
|  	 the initial binary.  And then the more complex part
|  	 where the object is dynamically loaded and the scope
|  	 array can change.  */
|        if (RTLD_SINGLE_THREAD_P)
|  	result = GLRO(dl_lookup_symbol_x) (name, match, &ref,
|  					   match->l_scope, vers, 0,

 ...

| @@ -122,17 +134,19 @@ do_sym (void *handle, const char *name, void *who,
|  	  THREAD_GSCOPE_RESET_FLAG ();
|  	  if (__glibc_unlikely (exception.errstring != NULL))
|  	    _dl_signal_exception (err, &exception, NULL);
|  
|  	  result = args.map;
|  	}
|      }
|    else if (handle == RTLD_NEXT)
|      {
| +      match = find_caller_link_map (caller);

PS1, Line 143:

OK. Likewise.

| +
|        if (__glibc_unlikely (match == GL(dl_ns)[LM_ID_BASE]._ns_loaded))
|  	{
|  	  if (match == NULL
|  	      || caller < match->l_map_start
|  	      || caller >= match->l_map_end)
|  	    _dl_signal_error (0, NULL, NULL, N_("\
|  RTLD_NEXT used in code not dynamically loaded"));
|  	}

 ...

| @@ -182,16 +196,19 @@ #ifdef SHARED
|  	{
|  	  const char *strtab = (const char *) D_PTR (result,
|  						     l_info[DT_STRTAB]);
|  	  /* Compute index of the symbol entry in the symbol table of
|  	     the DSO with the definition.  */
|  	  unsigned int ndx = (ref - (ElfW(Sym) *) D_PTR (result,
|  							 l_info[DT_SYMTAB]));
|  
| +	  if (match == NULL)
| +	    match = find_caller_link_map (caller);

PS1, Line 205:

OK, and again for auditing.

| +
|  	  if ((match->l_audit_any_plt | result->l_audit_any_plt) != 0)
|  	    {
|  	      unsigned int altvalue = 0;
|  	      struct audit_ifaces *afct = GLRO(dl_audit);
|  	      /* Synthesize a symbol record where the st_value field is
|  		 the result.  */
|  	      ElfW(Sym) sym = *ref;
|  	      sym.st_value = (ElfW(Addr)) value;

-- 
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: Ide5d9e2cc7ac25a0ffae8fb4c26def0c898efa29
Gerrit-Change-Number: 528
Gerrit-PatchSet: 1
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Carlos O'Donell <carlos@redhat.com>
Gerrit-Reviewer: Szabolcs Nagy <szabolcs.nagy@arm.com>
Gerrit-Comment-Date: Wed, 27 Nov 2019 18:47:20 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[review v2] dlsym: Do not determine caller link map if not needed

[Florian Weimer (Code Review)]

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/528
......................................................................

dlsym: Do not determine caller link map if not needed

Obtaining the link map is potentially very slow because it requires
iterating over all loaded objects in the current implementation.  If
the caller supplied an explicit handle (i.e., not one of the RTLD_*
constants), the dlsym implementation does not need the identity of the
caller (except in the special case of auditing), so this change
avoids computing it in that case.

Even in the minimal case (dlsym called from a main program linked with
-dl), this shows a small speedup, perhaps around five percent.  The
performance improvement can be arbitrarily large in principle (if
_dl_find_dso_for_object has to iterate over many link maps).

Change-Id: Ide5d9e2cc7ac25a0ffae8fb4c26def0c898efa29
---
M elf/dl-sym.c
1 file changed, 21 insertions(+), 4 deletions(-)



diff --git a/elf/dl-sym.c b/elf/dl-sym.c
index 8209342..7282e09 100644
--- a/elf/dl-sym.c
+++ b/elf/dl-sym.c
@@ -80,6 +80,18 @@
 					args->flags, NULL);
 }
 
+/* Return the link map containing the caller address.  */
+static inline struct link_map *
+find_caller_link_map (ElfW(Addr) caller)
+{
+  struct link_map *l = _dl_find_dso_for_object (caller);
+  if (l != NULL)
+    return l;
+  else
+    /* If the address is not recognized the call comes from the main
+       program (we hope).  */
+    return GL(dl_ns)[LM_ID_BASE]._ns_loaded;
+}
 
 static void *
 do_sym (void *handle, const char *name, void *who,
@@ -89,13 +101,13 @@
   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 = l ? l : GL(dl_ns)[LM_ID_BASE]._ns_loaded;
+  /* Link map of the caller if needed.  */
+  struct link_map *match = NULL;
 
   if (handle == RTLD_DEFAULT)
     {
+      match = find_caller_link_map (caller);
+
       /* Search the global scope.  We have the simple case where
 	 we look up in the scope of an object which was part of
 	 the initial binary.  And then the more complex part
@@ -128,6 +140,8 @@
     }
   else if (handle == RTLD_NEXT)
     {
+      match = find_caller_link_map (caller);
+
       if (__glibc_unlikely (match == GL(dl_ns)[LM_ID_BASE]._ns_loaded))
 	{
 	  if (match == NULL
@@ -187,6 +201,9 @@
 	  unsigned int ndx = (ref - (ElfW(Sym) *) D_PTR (result,
 							 l_info[DT_SYMTAB]));
 
+	  if (match == NULL)
+	    match = find_caller_link_map (caller);
+
 	  if ((match->l_audit_any_plt | result->l_audit_any_plt) != 0)
 	    {
 	      unsigned int altvalue = 0;

-- 
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: Ide5d9e2cc7ac25a0ffae8fb4c26def0c898efa29
Gerrit-Change-Number: 528
Gerrit-PatchSet: 2
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Carlos O'Donell <carlos@redhat.com>
Gerrit-Reviewer: Szabolcs Nagy <szabolcs.nagy@arm.com>
Gerrit-MessageType: newpatchset

[review v2] dlsym: Do not determine caller link map if not needed

[Carlos O'Donell (Code Review)]

Carlos O'Donell has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/528
......................................................................


Patch Set 2: Code-Review+2

New commit message looks good.
OK for master.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>


-- 
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: Ide5d9e2cc7ac25a0ffae8fb4c26def0c898efa29
Gerrit-Change-Number: 528
Gerrit-PatchSet: 2
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Carlos O'Donell <carlos@redhat.com>
Gerrit-Reviewer: Szabolcs Nagy <szabolcs.nagy@arm.com>
Gerrit-Comment-Date: Wed, 27 Nov 2019 19:01:31 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[pushed] dlsym: Do not determine caller link map if not needed

[Sourceware to Gerrit sync (Code Review)]

Sourceware to Gerrit sync has submitted this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/528
......................................................................

dlsym: Do not determine caller link map if not needed

Obtaining the link map is potentially very slow because it requires
iterating over all loaded objects in the current implementation.  If
the caller supplied an explicit handle (i.e., not one of the RTLD_*
constants), the dlsym implementation does not need the identity of the
caller (except in the special case of auditing), so this change
avoids computing it in that case.

Even in the minimal case (dlsym called from a main program linked with
-dl), this shows a small speedup, perhaps around five percent.  The
performance improvement can be arbitrarily large in principle (if
_dl_find_dso_for_object has to iterate over many link maps).

Change-Id: Ide5d9e2cc7ac25a0ffae8fb4c26def0c898efa29
---
M elf/dl-sym.c
1 file changed, 21 insertions(+), 4 deletions(-)

Approvals:
  Carlos O'Donell: Looks good to me, approved


diff --git a/elf/dl-sym.c b/elf/dl-sym.c
index 21e025b..3933194 100644
--- a/elf/dl-sym.c
+++ b/elf/dl-sym.c
@@ -80,6 +80,18 @@
 					args->flags, NULL);
 }
 
+/* Return the link map containing the caller address.  */
+static inline struct link_map *
+find_caller_link_map (ElfW(Addr) caller)
+{
+  struct link_map *l = _dl_find_dso_for_object (caller);
+  if (l != NULL)
+    return l;
+  else
+    /* If the address is not recognized the call comes from the main
+       program (we hope).  */
+    return GL(dl_ns)[LM_ID_BASE]._ns_loaded;
+}
 
 static void *
 do_sym (void *handle, const char *name, void *who,
@@ -89,13 +101,13 @@
   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 = l ? l : GL(dl_ns)[LM_ID_BASE]._ns_loaded;
+  /* Link map of the caller if needed.  */
+  struct link_map *match = NULL;
 
   if (handle == RTLD_DEFAULT)
     {
+      match = find_caller_link_map (caller);
+
       /* Search the global scope.  We have the simple case where
 	 we look up in the scope of an object which was part of
 	 the initial binary.  And then the more complex part
@@ -128,6 +140,8 @@
     }
   else if (handle == RTLD_NEXT)
     {
+      match = find_caller_link_map (caller);
+
       if (__glibc_unlikely (match == GL(dl_ns)[LM_ID_BASE]._ns_loaded))
 	{
 	  if (match == NULL
@@ -187,6 +201,9 @@
 	  unsigned int ndx = (ref - (ElfW(Sym) *) D_PTR (result,
 							 l_info[DT_SYMTAB]));
 
+	  if (match == NULL)
+	    match = find_caller_link_map (caller);
+
 	  if ((match->l_audit_any_plt | result->l_audit_any_plt) != 0)
 	    {
 	      unsigned int altvalue = 0;

-- 
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: Ide5d9e2cc7ac25a0ffae8fb4c26def0c898efa29
Gerrit-Change-Number: 528
Gerrit-PatchSet: 3
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Carlos O'Donell <carlos@redhat.com>
Gerrit-Reviewer: Szabolcs Nagy <szabolcs.nagy@arm.com>
Gerrit-MessageType: merged