[v3,3/3] elf: avoid stack allocation in dl_open_worker
diff mbox series

Message ID 1575394197-18006-4-git-send-email-david.kilroy@arm.com
State New
Headers show
Series
  • elf: Allow dlopen of filter object to work [BZ #16272]
Related show

Commit Message

David Kilroy Dec. 3, 2019, 5:30 p.m. UTC
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

Adhemerval Zanella Jan. 14, 2020, 8:04 p.m. UTC | #1
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.

Patch
diff mbox series

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.  */
-  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)
 	{