[v3,3/3] elf: avoid stack allocation in dl_open_worker
Commit Message
As the sort was removed, there's no need to keep a separate map of
links. Instead, when relocating objects iterate over l_initfini
directly.
This allows us to remove the loop copying l_initfini elements into
map. We still need a loop to identify the first and last elements that
need relocation.
Tested by running the testsuite on x86_64.
---
elf/dl-open.c | 28 ++++++++++++----------------
1 file changed, 12 insertions(+), 16 deletions(-)
Comments
On 03/12/2019 14:30, David Kilroy wrote:
> As the sort was removed, there's no need to keep a separate map of
> links. Instead, when relocating objects iterate over l_initfini
> directly.
>
> This allows us to remove the loop copying l_initfini elements into
> map. We still need a loop to identify the first and last elements that
> need relocation.
>
> Tested by running the testsuite on x86_64.
LGTM, thanks.
Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> ---
> elf/dl-open.c | 28 ++++++++++++----------------
> 1 file changed, 12 insertions(+), 16 deletions(-)
>
> diff --git a/elf/dl-open.c b/elf/dl-open.c
> index c4d09c7c..eb36a91 100644
> --- a/elf/dl-open.c
> +++ b/elf/dl-open.c
> @@ -636,25 +636,18 @@ dl_open_worker (void *a)
> /* Sort the objects by dependency for the relocation process. This
> allows IFUNC relocations to work and it also means copy
> relocation of dependencies are if necessary overwritten. */
I think we need to update the comment to state this is not sorting
anymore, but rather using the already sorted list (done by
_dl_map_object_deps).
> - unsigned int nmaps = 0;
> + unsigned int first = UINT_MAX;
> + unsigned int last = 0;
> unsigned int j = 0;
> struct link_map *l = new->l_initfini[0];
> do
> {
> if (! l->l_real->l_relocated)
> - ++nmaps;
> - l = new->l_initfini[++j];
> - }
> - while (l != NULL);
> - /* Stack allocation is limited by the number of loaded objects. */
> - struct link_map *maps[nmaps];
> - nmaps = 0;
> - j = 0;
> - l = new->l_initfini[0];
> - do
> - {
> - if (! l->l_real->l_relocated)
> - maps[nmaps++] = l;
> + {
> + if (first == UINT_MAX)
> + first = j;
> + last = j + 1;
> + }
> l = new->l_initfini[++j];
> }
> while (l != NULL);
Ok, it sets the range [first, last] as the potential maps that requires
relocation.
> @@ -669,9 +662,12 @@ dl_open_worker (void *a)
> them. However, such relocation dependencies in IFUNC resolvers
> are undefined anyway, so this is not a problem. */
>
> - for (unsigned int i = nmaps; i-- > 0; )
> + for (unsigned int i = last; i-- > first; )
> {
> - l = maps[i];
> + l = new->l_initfini[i];
> +
> + if (l->l_real->l_relocated)
> + continue;
>
> if (! relocation_in_progress)
> {
>
Ok, some objects objects might already relocated.
@@ -636,25 +636,18 @@ dl_open_worker (void *a)
/* Sort the objects by dependency for the relocation process. This
allows IFUNC relocations to work and it also means copy
relocation of dependencies are if necessary overwritten. */
- unsigned int nmaps = 0;
+ unsigned int first = UINT_MAX;
+ unsigned int last = 0;
unsigned int j = 0;
struct link_map *l = new->l_initfini[0];
do
{
if (! l->l_real->l_relocated)
- ++nmaps;
- l = new->l_initfini[++j];
- }
- while (l != NULL);
- /* Stack allocation is limited by the number of loaded objects. */
- struct link_map *maps[nmaps];
- nmaps = 0;
- j = 0;
- l = new->l_initfini[0];
- do
- {
- if (! l->l_real->l_relocated)
- maps[nmaps++] = l;
+ {
+ if (first == UINT_MAX)
+ first = j;
+ last = j + 1;
+ }
l = new->l_initfini[++j];
}
while (l != NULL);
@@ -669,9 +662,12 @@ dl_open_worker (void *a)
them. However, such relocation dependencies in IFUNC resolvers
are undefined anyway, so this is not a problem. */
- for (unsigned int i = nmaps; i-- > 0; )
+ for (unsigned int i = last; i-- > first; )
{
- l = maps[i];
+ l = new->l_initfini[i];
+
+ if (l->l_real->l_relocated)
+ continue;
if (! relocation_in_progress)
{