elf: Move elf_dynamic_do_Rel RTLD_BOOTSTRAP branches outside

Message ID 20220418073711.2471534-1-maskray@google.com
State Committed
Commit 84e02af1ebc9988126eebe60bf19226cea835623
Headers
Series elf: Move elf_dynamic_do_Rel RTLD_BOOTSTRAP branches outside |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

Fangrui Song April 18, 2022, 7:37 a.m. UTC
  elf_dynamic_do_Rel checks RTLD_BOOTSTRAP in several #ifdef branches.
Create an outside RTLD_BOOTSTRAP branch to simplify reasoning about the
function at the cost of a few duplicate lines.

Since dl_naudit is zero in RTLD_BOOTSTRAP code, the RTLD_BOOTSTRAP
branch can avoid _dl_audit_symbind calls to save code size (stripped
x86-64 ld.so is 672 bytes smaller).
---
 elf/do-rel.h | 49 ++++++++++++++++++++++++++++---------------------
 1 file changed, 28 insertions(+), 21 deletions(-)
  

Comments

Adhemerval Zanella April 20, 2022, 6:33 p.m. UTC | #1
On 18/04/2022 04:37, Fangrui Song via Libc-alpha wrote:
> elf_dynamic_do_Rel checks RTLD_BOOTSTRAP in several #ifdef branches.
> Create an outside RTLD_BOOTSTRAP branch to simplify reasoning about the
> function at the cost of a few duplicate lines.
> 
> Since dl_naudit is zero in RTLD_BOOTSTRAP code, the RTLD_BOOTSTRAP
> branch can avoid _dl_audit_symbind calls to save code size (stripped
> x86-64 ld.so is 672 bytes smaller).


Thanks, it does looks clear. I am seeing a code decrease on all archers,
although not a large as you are seeing (x86_64 went from 191222 to
191094).

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  elf/do-rel.h | 49 ++++++++++++++++++++++++++++---------------------
>  1 file changed, 28 insertions(+), 21 deletions(-)
> 
> diff --git a/elf/do-rel.h b/elf/do-rel.h
> index d3e022267b..27b9b9f427 100644
> --- a/elf/do-rel.h
> +++ b/elf/do-rel.h
> @@ -45,15 +45,35 @@ elf_dynamic_do_Rel (struct link_map *map, struct r_scope_elem *scope[],
>  		    __typeof (((ElfW(Dyn) *) 0)->d_un.d_val) nrelative,
>  		    int lazy, int skip_ifunc)
>  {
> -  const ElfW(Rel) *r = (const void *) reladdr;
> +  const ElfW(Rel) *relative = (const void *) reladdr;
> +  const ElfW(Rel) *r = relative + nrelative;
>    const ElfW(Rel) *end = (const void *) (reladdr + relsize);
>    ElfW(Addr) l_addr = map->l_addr;
> -# if defined ELF_MACHINE_IRELATIVE && !defined RTLD_BOOTSTRAP
> +  const ElfW(Sym) *const symtab
> +      = (const void *) D_PTR (map, l_info[DT_SYMTAB]);
> +
> +#ifdef RTLD_BOOTSTRAP
> +  for (; relative < r; ++relative)
> +    DO_ELF_MACHINE_REL_RELATIVE (map, l_addr, relative);
> +
> +  const ElfW (Half) *const version
> +      = (const void *) D_PTR (map, l_info[VERSYMIDX (DT_VERSYM)]);
> +  for (; r < end; ++r)
> +    {
> +      ElfW (Half) ndx = version[ELFW (R_SYM) (r->r_info)] & 0x7fff;
> +      const ElfW (Sym) *sym = &symtab[ELFW (R_SYM) (r->r_info)];
> +      void *const r_addr_arg = (void *) (l_addr + r->r_offset);
> +      const struct r_found_version *rversion = &map->l_versions[ndx];
> +
> +      elf_machine_rel (map, scope, r, sym, rversion, r_addr_arg, skip_ifunc);
> +    }
> +#else /* !RTLD_BOOTSTRAP */
> +# if defined ELF_MACHINE_IRELATIVE
>    const ElfW(Rel) *r2 = NULL;
>    const ElfW(Rel) *end2 = NULL;
>  # endif
>  
> -#if (!defined DO_RELA || !defined ELF_MACHINE_PLT_REL) && !defined RTLD_BOOTSTRAP
> +#if !defined DO_RELA || !defined ELF_MACHINE_PLT_REL
>    /* We never bind lazily during ld.so bootstrap.  Unfortunately gcc is
>       not clever enough to see through all the function calls to realize
>       that.  */
> @@ -82,12 +102,6 @@ elf_dynamic_do_Rel (struct link_map *map, struct r_scope_elem *scope[],
>    else
>  #endif
>      {
> -      const ElfW(Sym) *const symtab =
> -	(const void *) D_PTR (map, l_info[DT_SYMTAB]);
> -      const ElfW(Rel) *relative = r;
> -      r += nrelative;
> -
> -#ifndef RTLD_BOOTSTRAP
>        /* This is defined in rtld.c, but nowhere in the static libc.a; make
>  	 the reference weak so static programs can still link.  This
>  	 declaration cannot be done when compiling rtld.c (i.e. #ifdef
> @@ -106,16 +120,10 @@ elf_dynamic_do_Rel (struct link_map *map, struct r_scope_elem *scope[],
>  	   memory_loc += l_addr...  */
>  	if (l_addr != 0)
>  # endif
> -#endif
>  	  for (; relative < r; ++relative)
>  	    DO_ELF_MACHINE_REL_RELATIVE (map, l_addr, relative);
>  
> -#ifdef RTLD_BOOTSTRAP
> -      /* The dynamic linker always uses versioning.  */
> -      assert (map->l_info[VERSYMIDX (DT_VERSYM)] != NULL);
> -#else
>        if (map->l_info[VERSYMIDX (DT_VERSYM)])
> -#endif
>  	{
>  	  const ElfW(Half) *const version =
>  	    (const void *) D_PTR (map, l_info[VERSYMIDX (DT_VERSYM)]);
> @@ -126,7 +134,7 @@ elf_dynamic_do_Rel (struct link_map *map, struct r_scope_elem *scope[],
>  	      const ElfW(Sym) *sym = &symtab[ELFW(R_SYM) (r->r_info)];
>  	      void *const r_addr_arg = (void *) (l_addr + r->r_offset);
>  	      const struct r_found_version *rversion = &map->l_versions[ndx];
> -#if defined ELF_MACHINE_IRELATIVE && !defined RTLD_BOOTSTRAP
> +#if defined ELF_MACHINE_IRELATIVE
>  	      if (ELFW(R_TYPE) (r->r_info) == ELF_MACHINE_IRELATIVE)
>  		{
>  		  if (r2 == NULL)
> @@ -138,7 +146,7 @@ elf_dynamic_do_Rel (struct link_map *map, struct r_scope_elem *scope[],
>  
>  	      elf_machine_rel (map, scope, r, sym, rversion, r_addr_arg,
>  			       skip_ifunc);
> -#if defined SHARED && !defined RTLD_BOOTSTRAP
> +#if defined SHARED
>  	      if (ELFW(R_TYPE) (r->r_info) == ELF_MACHINE_JMP_SLOT
>  		  && GLRO(dl_naudit) > 0)
>  		{
> @@ -151,7 +159,7 @@ elf_dynamic_do_Rel (struct link_map *map, struct r_scope_elem *scope[],
>  #endif
>  	    }
>  
> -#if defined ELF_MACHINE_IRELATIVE && !defined RTLD_BOOTSTRAP
> +#if defined ELF_MACHINE_IRELATIVE
>  	  if (r2 != NULL)
>  	    for (; r2 <= end2; ++r2)
>  	      if (ELFW(R_TYPE) (r2->r_info) == ELF_MACHINE_IRELATIVE)
> @@ -166,7 +174,6 @@ elf_dynamic_do_Rel (struct link_map *map, struct r_scope_elem *scope[],
>  		}
>  #endif
>  	}
> -#ifndef RTLD_BOOTSTRAP
>        else
>  	{
>  	  for (; r < end; ++r)
> @@ -184,7 +191,7 @@ elf_dynamic_do_Rel (struct link_map *map, struct r_scope_elem *scope[],
>  # endif
>  	      elf_machine_rel (map, scope, r, sym, NULL, r_addr_arg,
>  			       skip_ifunc);
> -# if defined SHARED && !defined RTLD_BOOTSTRAP
> +# if defined SHARED
>  	      if (ELFW(R_TYPE) (r->r_info) == ELF_MACHINE_JMP_SLOT
>  		  && GLRO(dl_naudit) > 0)
>  		{
> @@ -207,8 +214,8 @@ elf_dynamic_do_Rel (struct link_map *map, struct r_scope_elem *scope[],
>  				 skip_ifunc);
>  # endif
>  	}
> -#endif
>      }
> +#endif /* !RTLD_BOOTSTRAP */
>  }
>  
>  #undef elf_dynamic_do_Rel
  
Andreas Schwab April 22, 2022, 10 a.m. UTC | #2
This breaks powerpc.

https://build.opensuse.org/package/live_build_log/home:Andreas_Schwab:glibc/glibc/p/ppc
  
Fangrui Song April 24, 2022, 1:49 a.m. UTC | #3
On 2022-04-22, Andreas Schwab wrote:
>This breaks powerpc.
>
>https://build.opensuse.org/package/live_build_log/home:Andreas_Schwab:glibc/glibc/p/ppc

This is similar to the m68k breakage. This time the reordering of a GOT
load before self relocations makes ld.so fail..

https://sourceware.org/pipermail/libc-alpha/2022-April/138118.html
should fix it.
  

Patch

diff --git a/elf/do-rel.h b/elf/do-rel.h
index d3e022267b..27b9b9f427 100644
--- a/elf/do-rel.h
+++ b/elf/do-rel.h
@@ -45,15 +45,35 @@  elf_dynamic_do_Rel (struct link_map *map, struct r_scope_elem *scope[],
 		    __typeof (((ElfW(Dyn) *) 0)->d_un.d_val) nrelative,
 		    int lazy, int skip_ifunc)
 {
-  const ElfW(Rel) *r = (const void *) reladdr;
+  const ElfW(Rel) *relative = (const void *) reladdr;
+  const ElfW(Rel) *r = relative + nrelative;
   const ElfW(Rel) *end = (const void *) (reladdr + relsize);
   ElfW(Addr) l_addr = map->l_addr;
-# if defined ELF_MACHINE_IRELATIVE && !defined RTLD_BOOTSTRAP
+  const ElfW(Sym) *const symtab
+      = (const void *) D_PTR (map, l_info[DT_SYMTAB]);
+
+#ifdef RTLD_BOOTSTRAP
+  for (; relative < r; ++relative)
+    DO_ELF_MACHINE_REL_RELATIVE (map, l_addr, relative);
+
+  const ElfW (Half) *const version
+      = (const void *) D_PTR (map, l_info[VERSYMIDX (DT_VERSYM)]);
+  for (; r < end; ++r)
+    {
+      ElfW (Half) ndx = version[ELFW (R_SYM) (r->r_info)] & 0x7fff;
+      const ElfW (Sym) *sym = &symtab[ELFW (R_SYM) (r->r_info)];
+      void *const r_addr_arg = (void *) (l_addr + r->r_offset);
+      const struct r_found_version *rversion = &map->l_versions[ndx];
+
+      elf_machine_rel (map, scope, r, sym, rversion, r_addr_arg, skip_ifunc);
+    }
+#else /* !RTLD_BOOTSTRAP */
+# if defined ELF_MACHINE_IRELATIVE
   const ElfW(Rel) *r2 = NULL;
   const ElfW(Rel) *end2 = NULL;
 # endif
 
-#if (!defined DO_RELA || !defined ELF_MACHINE_PLT_REL) && !defined RTLD_BOOTSTRAP
+#if !defined DO_RELA || !defined ELF_MACHINE_PLT_REL
   /* We never bind lazily during ld.so bootstrap.  Unfortunately gcc is
      not clever enough to see through all the function calls to realize
      that.  */
@@ -82,12 +102,6 @@  elf_dynamic_do_Rel (struct link_map *map, struct r_scope_elem *scope[],
   else
 #endif
     {
-      const ElfW(Sym) *const symtab =
-	(const void *) D_PTR (map, l_info[DT_SYMTAB]);
-      const ElfW(Rel) *relative = r;
-      r += nrelative;
-
-#ifndef RTLD_BOOTSTRAP
       /* This is defined in rtld.c, but nowhere in the static libc.a; make
 	 the reference weak so static programs can still link.  This
 	 declaration cannot be done when compiling rtld.c (i.e. #ifdef
@@ -106,16 +120,10 @@  elf_dynamic_do_Rel (struct link_map *map, struct r_scope_elem *scope[],
 	   memory_loc += l_addr...  */
 	if (l_addr != 0)
 # endif
-#endif
 	  for (; relative < r; ++relative)
 	    DO_ELF_MACHINE_REL_RELATIVE (map, l_addr, relative);
 
-#ifdef RTLD_BOOTSTRAP
-      /* The dynamic linker always uses versioning.  */
-      assert (map->l_info[VERSYMIDX (DT_VERSYM)] != NULL);
-#else
       if (map->l_info[VERSYMIDX (DT_VERSYM)])
-#endif
 	{
 	  const ElfW(Half) *const version =
 	    (const void *) D_PTR (map, l_info[VERSYMIDX (DT_VERSYM)]);
@@ -126,7 +134,7 @@  elf_dynamic_do_Rel (struct link_map *map, struct r_scope_elem *scope[],
 	      const ElfW(Sym) *sym = &symtab[ELFW(R_SYM) (r->r_info)];
 	      void *const r_addr_arg = (void *) (l_addr + r->r_offset);
 	      const struct r_found_version *rversion = &map->l_versions[ndx];
-#if defined ELF_MACHINE_IRELATIVE && !defined RTLD_BOOTSTRAP
+#if defined ELF_MACHINE_IRELATIVE
 	      if (ELFW(R_TYPE) (r->r_info) == ELF_MACHINE_IRELATIVE)
 		{
 		  if (r2 == NULL)
@@ -138,7 +146,7 @@  elf_dynamic_do_Rel (struct link_map *map, struct r_scope_elem *scope[],
 
 	      elf_machine_rel (map, scope, r, sym, rversion, r_addr_arg,
 			       skip_ifunc);
-#if defined SHARED && !defined RTLD_BOOTSTRAP
+#if defined SHARED
 	      if (ELFW(R_TYPE) (r->r_info) == ELF_MACHINE_JMP_SLOT
 		  && GLRO(dl_naudit) > 0)
 		{
@@ -151,7 +159,7 @@  elf_dynamic_do_Rel (struct link_map *map, struct r_scope_elem *scope[],
 #endif
 	    }
 
-#if defined ELF_MACHINE_IRELATIVE && !defined RTLD_BOOTSTRAP
+#if defined ELF_MACHINE_IRELATIVE
 	  if (r2 != NULL)
 	    for (; r2 <= end2; ++r2)
 	      if (ELFW(R_TYPE) (r2->r_info) == ELF_MACHINE_IRELATIVE)
@@ -166,7 +174,6 @@  elf_dynamic_do_Rel (struct link_map *map, struct r_scope_elem *scope[],
 		}
 #endif
 	}
-#ifndef RTLD_BOOTSTRAP
       else
 	{
 	  for (; r < end; ++r)
@@ -184,7 +191,7 @@  elf_dynamic_do_Rel (struct link_map *map, struct r_scope_elem *scope[],
 # endif
 	      elf_machine_rel (map, scope, r, sym, NULL, r_addr_arg,
 			       skip_ifunc);
-# if defined SHARED && !defined RTLD_BOOTSTRAP
+# if defined SHARED
 	      if (ELFW(R_TYPE) (r->r_info) == ELF_MACHINE_JMP_SLOT
 		  && GLRO(dl_naudit) > 0)
 		{
@@ -207,8 +214,8 @@  elf_dynamic_do_Rel (struct link_map *map, struct r_scope_elem *scope[],
 				 skip_ifunc);
 # endif
 	}
-#endif
     }
+#endif /* !RTLD_BOOTSTRAP */
 }
 
 #undef elf_dynamic_do_Rel