* [PATCH] remove nested functions from elf/dl-load.c
@ 2014-10-01 23:08 Konstantin Serebryany
2014-10-01 23:16 ` Roland McGrath
0 siblings, 1 reply; 6+ messages in thread
From: Konstantin Serebryany @ 2014-10-01 23:08 UTC (permalink / raw)
To: Roland McGrath, GNU C Library
[-- Attachment #1: Type: text/plain, Size: 662 bytes --]
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.
[-- Attachment #2: unnest-dl-load.patch --]
[-- Type: text/x-patch, Size: 4046 bytes --]
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] remove nested functions from elf/dl-load.c
2014-10-01 23:08 [PATCH] remove nested functions from elf/dl-load.c Konstantin Serebryany
@ 2014-10-01 23:16 ` Roland McGrath
2014-10-06 18:17 ` Konstantin Serebryany
0 siblings, 1 reply; 6+ messages in thread
From: Roland McGrath @ 2014-10-01 23:16 UTC (permalink / raw)
To: Konstantin Serebryany; +Cc: GNU C Library
> * 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] remove nested functions from elf/dl-load.c
2014-10-01 23:16 ` Roland McGrath
@ 2014-10-06 18:17 ` Konstantin Serebryany
2014-10-08 22:20 ` Roland McGrath
0 siblings, 1 reply; 6+ messages in thread
From: Konstantin Serebryany @ 2014-10-06 18:17 UTC (permalink / raw)
To: Roland McGrath; +Cc: GNU C Library
[-- Attachment #1: Type: text/plain, Size: 1268 bytes --]
Fixed all, please take another look.
2014-10-06 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 that nested function. Update call sites.
On Wed, Oct 1, 2014 at 4:16 PM, Roland McGrath <roland@hack.frob.com> wrote:
>> * 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
[-- Attachment #2: unnest-dl-load-1.patch --]
[-- Type: text/x-patch, Size: 3977 bytes --]
diff --git a/elf/dl-load.c b/elf/dl-load.c
index 016a99c..943e8f2 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -2201,6 +2201,45 @@ _dl_map_object (struct link_map *loader, const char *name,
&stack_end, nsid);
}
+struct add_path_state
+{
+ bool counting;
+ unsigned int idx;
+ Dl_serinfo *si;
+ char *allocptr;
+};
+
+static void
+add_path (struct add_path_state *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
@@ -2212,38 +2251,10 @@ _dl_rtld_di_serinfo (struct link_map *loader, Dl_serinfo *si, bool counting)
si->dls_size = 0;
}
- 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);
- }
- }
+ struct add_path_state p = {counting, 0, si,
+ (char *) &si->dls_serpath[si->dls_cnt]};
+
+# 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 +2266,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 +2277,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 +2294,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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] remove nested functions from elf/dl-load.c
2014-10-06 18:17 ` Konstantin Serebryany
@ 2014-10-08 22:20 ` Roland McGrath
2014-10-08 22:50 ` Konstantin Serebryany
0 siblings, 1 reply; 6+ messages in thread
From: Roland McGrath @ 2014-10-08 22:20 UTC (permalink / raw)
To: Konstantin Serebryany; +Cc: GNU C Library
> + struct add_path_state p = {counting, 0, si,
> + (char *) &si->dls_serpath[si->dls_cnt]};
Indent the initializer properly. It's easier to read if you use tagged
member initializers too.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] remove nested functions from elf/dl-load.c
2014-10-08 22:20 ` Roland McGrath
@ 2014-10-08 22:50 ` Konstantin Serebryany
2014-10-09 17:10 ` Roland McGrath
0 siblings, 1 reply; 6+ messages in thread
From: Konstantin Serebryany @ 2014-10-08 22:50 UTC (permalink / raw)
To: Roland McGrath; +Cc: GNU C Library
[-- Attachment #1: Type: text/plain, Size: 547 bytes --]
Please take another look.
2014-10-08 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 that nested function. Update call sites.
On Wed, Oct 8, 2014 at 3:20 PM, Roland McGrath <roland@hack.frob.com> wrote:
>> + struct add_path_state p = {counting, 0, si,
>> + (char *) &si->dls_serpath[si->dls_cnt]};
>
> Indent the initializer properly. It's easier to read if you use tagged
> member initializers too.
[-- Attachment #2: unnest-dl-load-2.patch --]
[-- Type: text/x-patch, Size: 4042 bytes --]
diff --git a/elf/dl-load.c b/elf/dl-load.c
index 016a99c..fde7137 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -2201,6 +2201,45 @@ _dl_map_object (struct link_map *loader, const char *name,
&stack_end, nsid);
}
+struct add_path_state
+{
+ bool counting;
+ unsigned int idx;
+ Dl_serinfo *si;
+ char *allocptr;
+};
+
+static void
+add_path (struct add_path_state *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
@@ -2212,38 +2251,15 @@ _dl_rtld_di_serinfo (struct link_map *loader, Dl_serinfo *si, bool counting)
si->dls_size = 0;
}
- 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 */
+ struct add_path_state p =
{
- 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);
- }
- }
+ .counting = counting,
+ .idx = 0,
+ .si = si,
+ .allocptr = (char *) &si->dls_serpath[si->dls_cnt]
+ };
+
+# 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 +2271,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 +2282,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 +2299,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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] remove nested functions from elf/dl-load.c
2014-10-08 22:50 ` Konstantin Serebryany
@ 2014-10-09 17:10 ` Roland McGrath
0 siblings, 0 replies; 6+ messages in thread
From: Roland McGrath @ 2014-10-09 17:10 UTC (permalink / raw)
To: Konstantin Serebryany; +Cc: GNU C Library
That's fine.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-10-09 17:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-10-01 23:08 [PATCH] remove nested functions from elf/dl-load.c Konstantin Serebryany
2014-10-01 23:16 ` Roland McGrath
2014-10-06 18:17 ` Konstantin Serebryany
2014-10-08 22:20 ` Roland McGrath
2014-10-08 22:50 ` Konstantin Serebryany
2014-10-09 17:10 ` Roland McGrath
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).