From patchwork Mon Oct 6 18:17:00 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kostya Serebryany X-Patchwork-Id: 3114 Received: (qmail 14040 invoked by alias); 6 Oct 2014 18:17:26 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 14027 invoked by uid 89); 6 Oct 2014 18:17:24 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS, URIBL_RHS_DOB autolearn=ham version=3.3.2 X-HELO: mail-vc0-f179.google.com X-Received: by 10.52.144.195 with SMTP id so3mr7966994vdb.14.1412619441055; Mon, 06 Oct 2014 11:17:21 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20141001231600.572652C3AAA@topped-with-meat.com> References: <20141001231600.572652C3AAA@topped-with-meat.com> From: Konstantin Serebryany Date: Mon, 6 Oct 2014 11:17:00 -0700 Message-ID: Subject: Re: [PATCH] remove nested functions from elf/dl-load.c To: Roland McGrath Cc: GNU C Library Fixed all, please take another look. 2014-10-06 Kostya Serebryany * 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 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 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