diff mbox

remove nested functions from elf/dl-load.c

Message ID CAGQ9bdxsbz1zjaNtOdEXdMd6kwJ_zqh1riNLK46nk84Qw+jXZw@mail.gmail.com
State New
Headers show

Commit Message

Konstantin Serebryany Oct. 1, 2014, 11:08 p.m. UTC 
Hi,

Please review the patch that removes nested functions from elf/dl-load.c
I had to pack 4 parameters into a struct, just like with hack_digit in
stdio-common/printf_fp.c recently. With this patch the generated code
for _dl_rtld_di_serinfo is ~8 instructions
longer than in trunk (larger function prologue/epilogue) and add_path
is 2 instructions longer.

No regressions in 'make check' on x86_64-linux-gnu (Ubuntu 14.04)

2014-10-01  Kostya Serebryany  <konstantin.s.serebryany@gmail.com>

        * elf/dl-load.c
        (add_path): New function broken out of _dl_rtld_di_serinfo.
        (_dl_rtld_di_serinfo): Remove a nested function. Update call sites.

Comments

Roland McGrath Oct. 1, 2014, 11:16 p.m. UTC | #1 
>         * elf/dl-load.c
>         (add_path): New function broken out of _dl_rtld_di_serinfo.
>         (_dl_rtld_di_serinfo): Remove a nested function. Update call sites.

Two spaces between sentences.  Say "remove that" instead of "remove a".

> +struct add_path_args

This is not the arguments to the function so much as it's the state
relevant to the function.  So add_path_state seems like a better name.

>  {
> +  struct add_path_args p;
>    if (counting)
>      {
>        si->dls_cnt = 0;
>        si->dls_size = 0;
>      }
> +  p.counting = counting;
> +  p.idx = 0;
> +  p.allocptr = (char *) &si->dls_serpath[si->dls_cnt];
> +  p.si = si;

Use an initializing definition, and put that exactly where the previous
definitions of the shared locals were.  That is, after the if block and a
blank line.

The change is OK with those fixes.


Thanks,
Roland
diff mbox

Patch

diff --git a/elf/dl-load.c b/elf/dl-load.c
index 016a99c..4e51567 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -2201,49 +2201,62 @@  _dl_map_object (struct link_map *loader, const char *name,
 				 &stack_end, nsid);
 }
 
+struct add_path_args
+{
+  bool counting;
+  unsigned int idx;
+  Dl_serinfo *si;
+  char *allocptr;
+};
+
+static void
+add_path (struct add_path_args *p, const struct r_search_path_struct *sps,
+	  unsigned int flags)
+{
+  if (sps->dirs != (void *) -1)
+    {
+      struct r_search_path_elem **dirs = sps->dirs;
+      do
+	{
+	  const struct r_search_path_elem *const r = *dirs++;
+	  if (p->counting)
+	    {
+	      p->si->dls_cnt++;
+	      p->si->dls_size += MAX (2, r->dirnamelen);
+	    }
+	  else
+	    {
+	      Dl_serpath *const sp = &p->si->dls_serpath[p->idx++];
+	      sp->dls_name = p->allocptr;
+	      if (r->dirnamelen < 2)
+		*p->allocptr++ = r->dirnamelen ? '/' : '.';
+	      else
+		p->allocptr = __mempcpy (p->allocptr,
+					  r->dirname, r->dirnamelen - 1);
+	      *p->allocptr++ = '\0';
+	      sp->dls_flags = flags;
+	    }
+	}
+      while (*dirs != NULL);
+    }
+}
 
 void
 internal_function
 _dl_rtld_di_serinfo (struct link_map *loader, Dl_serinfo *si, bool counting)
 {
+  struct add_path_args p;
   if (counting)
     {
       si->dls_cnt = 0;
       si->dls_size = 0;
     }
+  p.counting = counting;
+  p.idx = 0;
+  p.allocptr = (char *) &si->dls_serpath[si->dls_cnt];
+  p.si = si;
 
-  unsigned int idx = 0;
-  char *allocptr = (char *) &si->dls_serpath[si->dls_cnt];
-  void add_path (const struct r_search_path_struct *sps, unsigned int flags)
-# define add_path(sps, flags) add_path(sps, 0) /* XXX */
-    {
-      if (sps->dirs != (void *) -1)
-	{
-	  struct r_search_path_elem **dirs = sps->dirs;
-	  do
-	    {
-	      const struct r_search_path_elem *const r = *dirs++;
-	      if (counting)
-		{
-		  si->dls_cnt++;
-		  si->dls_size += MAX (2, r->dirnamelen);
-		}
-	      else
-		{
-		  Dl_serpath *const sp = &si->dls_serpath[idx++];
-		  sp->dls_name = allocptr;
-		  if (r->dirnamelen < 2)
-		    *allocptr++ = r->dirnamelen ? '/' : '.';
-		  else
-		    allocptr = __mempcpy (allocptr,
-					  r->dirname, r->dirnamelen - 1);
-		  *allocptr++ = '\0';
-		  sp->dls_flags = flags;
-		}
-	    }
-	  while (*dirs != NULL);
-	}
-    }
+# define add_path(p, sps, flags) add_path(p, sps, 0) /* XXX */
 
   /* When the object has the RUNPATH information we don't use any RPATHs.  */
   if (loader->l_info[DT_RUNPATH] == NULL)
@@ -2255,7 +2268,7 @@  _dl_rtld_di_serinfo (struct link_map *loader, Dl_serinfo *si, bool counting)
       do
 	{
 	  if (cache_rpath (l, &l->l_rpath_dirs, DT_RPATH, "RPATH"))
-	    add_path (&l->l_rpath_dirs, XXX_RPATH);
+	    add_path (&p, &l->l_rpath_dirs, XXX_RPATH);
 	  l = l->l_loader;
 	}
       while (l != NULL);
@@ -2266,16 +2279,16 @@  _dl_rtld_di_serinfo (struct link_map *loader, Dl_serinfo *si, bool counting)
 	  l = GL(dl_ns)[LM_ID_BASE]._ns_loaded;
 	  if (l != NULL && l->l_type != lt_loaded && l != loader)
 	    if (cache_rpath (l, &l->l_rpath_dirs, DT_RPATH, "RPATH"))
-	      add_path (&l->l_rpath_dirs, XXX_RPATH);
+	      add_path (&p, &l->l_rpath_dirs, XXX_RPATH);
 	}
     }
 
   /* Try the LD_LIBRARY_PATH environment variable.  */
-  add_path (&env_path_list, XXX_ENV);
+  add_path (&p, &env_path_list, XXX_ENV);
 
   /* Look at the RUNPATH information for this binary.  */
   if (cache_rpath (loader, &loader->l_runpath_dirs, DT_RUNPATH, "RUNPATH"))
-    add_path (&loader->l_runpath_dirs, XXX_RUNPATH);
+    add_path (&p, &loader->l_runpath_dirs, XXX_RUNPATH);
 
   /* XXX
      Here is where ld.so.cache gets checked, but we don't have
@@ -2283,7 +2296,7 @@  _dl_rtld_di_serinfo (struct link_map *loader, Dl_serinfo *si, bool counting)
 
   /* Finally, try the default path.  */
   if (!(loader->l_flags_1 & DF_1_NODEFLIB))
-    add_path (&rtld_search_dirs, XXX_default);
+    add_path (&p, &rtld_search_dirs, XXX_default);
 
   if (counting)
     /* Count the struct size before the string area, which we didn't