[PATCH 0/3] Fix lld build for armhf

[PATCH 0/3] Fix lld build for armhf

[Adhemerval Zanella]

With this patchset I can build and run make check on arm-linux-gnueabihf
with lld 13.0.  I see 12 regressions, however I think it something
related to lld instead of a glibc issue:

  FAIL: elf/ifuncmain1pic
  FAIL: elf/ifuncmain1picstatic
  FAIL: elf/ifuncmain1pie
  FAIL: elf/ifuncmain1staticpic
  FAIL: elf/ifuncmain1staticpie
  FAIL: elf/ifuncmain1vis
  FAIL: elf/ifuncmain1vispic
  FAIL: elf/ifuncmain1vispie
  FAIL: elf/ifuncmain3
  FAIL: elf/ifuncmain4picstatic
  FAIL: elf/ifuncmain5picstatic
  FAIL: elf/ifuncmain5staticpic

Adhemerval Zanella (3):
  arm: Use internal symbol for _dl_argv on _dl_start_user
  arm: Use have-mtls-dialect-gnu2 to check for ARM TLS descriptors
    support
  elf: Assume disjointed .rela.dyn and .rela.plt for loader

 elf/dynamic-link.h       | 32 +++++++++-----------------------
 sysdeps/arm/Makefile     |  7 +------
 sysdeps/arm/dl-machine.h |  2 +-
 3 files changed, 11 insertions(+), 30 deletions(-)

-- 
2.32.0

[PATCH 1/3] arm: Use internal symbol for _dl_argv on _dl_start_user

[Adhemerval Zanella]

The lld does not support R_ARM_GOTOFF32 to preemptible symbol (_dl_argv
has global visibility).  Use the internal alias instead (one option
would to use HIDDEN_JUMPTARGET, bu the macro is not defined for
!__ASSEMBLER__ and I made this patch arm-specific to avoid require to
check extensivelly on other architecture it this might break something).

Checked on arm-linux-gnueabihf.
---
 sysdeps/arm/dl-machine.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sysdeps/arm/dl-machine.h b/sysdeps/arm/dl-machine.h
index c758bba169..4ecdde4355 100644
--- a/sysdeps/arm/dl-machine.h
+++ b/sysdeps/arm/dl-machine.h
@@ -202,7 +202,7 @@ _dl_start_user:\n\
 .L_FINI_PROC:\n\
 	.word	_dl_fini(GOTOFF)\n\
 .L_ARGV:\n\
-	.word	_dl_argv(GOTOFF)\n\
+	.word	__GI__dl_argv(GOTOFF)\n\
 .L_LOADED:\n\
 	.word	_rtld_local(GOTOFF)\n\
 .previous\n\
-- 
2.32.0

[PATCH 2/3] arm: Use have-mtls-dialect-gnu2 to check for ARM TLS descriptors support

[Adhemerval Zanella]

The lld linker does not support TLSDESC for arm.  The have-arm-tls-desc
is a leftover of 56583289b1 to support NaCL.

Checked on arm-linux-gnueabihf.
---
 sysdeps/arm/Makefile | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/sysdeps/arm/Makefile b/sysdeps/arm/Makefile
index fb4164f0d9..da4226c8c8 100644
--- a/sysdeps/arm/Makefile
+++ b/sysdeps/arm/Makefile
@@ -1,11 +1,6 @@
 gnulib-arch = $(elf-objpfx)libgcc-stubs.a
 static-gnulib-arch = $(elf-objpfx)libgcc-stubs.a
 
-# All supported build tools support TLS descriptors, but the OS may not.
-ifndef have-arm-tls-desc
-have-arm-tls-desc = yes
-endif
-
 ifeq ($(subdir),elf)
 sysdep-dl-routines += tlsdesc dl-tlsdesc
 sysdep_routines += aeabi_unwind_cpp_pr1 find_exidx
@@ -18,7 +13,7 @@ $(objpfx)libgcc-stubs.a: $(objpfx)aeabi_unwind_cpp_pr1.os
 lib-noranlib: $(objpfx)libgcc-stubs.a
 
 ifeq ($(build-shared),yes)
-ifeq ($(have-arm-tls-desc),yes)
+ifeq (yes,$(have-mtls-dialect-gnu2))
 tests += tst-armtlsdescloc tst-armtlsdescextnow tst-armtlsdescextlazy
 modules-names += tst-armtlsdesclocmod
 modules-names += tst-armtlsdescextlazymod tst-armtlsdescextnowmod
-- 
2.32.0

[PATCH 3/3] elf: Assume disjointed .rela.dyn and .rela.plt for loader

[Adhemerval Zanella]

The linker might add another section between the two relocation section
for the loader as well.  For instance on arm, lld does:

  [ 7] .rel.dyn          REL             000007f0 0007f0 000088 08   A	1   0  4
  [ 8] .ARM.exidx        ARM_EXIDX       00000878 000878 0000b0 00  AL1	2   0  4
  [ 9] .rel.plt          REL             00000928 000928 000028 08  AI	1  17  4

This patch removes the ELF_DURING_STARTUP optimization and assume
both section might no be linear.

Checked on x86_64, i686, aarch64, armhf, powerpc64le, powerpc64,
and powerpc (to check if this breaks on different architectures).
---
 elf/dynamic-link.h | 32 +++++++++-----------------------
 1 file changed, 9 insertions(+), 23 deletions(-)

diff --git a/elf/dynamic-link.h b/elf/dynamic-link.h
index ac4cc70dea..f619615e5c 100644
--- a/elf/dynamic-link.h
+++ b/elf/dynamic-link.h
@@ -65,12 +65,6 @@ elf_machine_lazy_rel (struct link_map *map, struct r_scope_elem *scope[],
 
 #ifdef RESOLVE_MAP
 
-# if defined RTLD_BOOTSTRAP || defined STATIC_PIE_BOOTSTRAP
-#  define ELF_DURING_STARTUP (1)
-# else
-#  define ELF_DURING_STARTUP (0)
-# endif
-
 /* Get the definitions of `elf_dynamic_do_rel' and `elf_dynamic_do_rela'.
    These functions are almost identical, so we use cpp magic to avoid
    duplicating their code.  It cannot be done in a more general function
@@ -106,9 +100,8 @@ elf_machine_lazy_rel (struct link_map *map, struct r_scope_elem *scope[],
 									      \
 	if (ranges[0].start + ranges[0].size == (start + size))		      \
 	  ranges[0].size -= size;					      \
-	if (ELF_DURING_STARTUP						      \
-	    || (!(do_lazy)						      \
-		&& (ranges[0].start + ranges[0].size) == start))	      \
+	if (!(do_lazy)							      \
+	    && (ranges[0].start + ranges[0].size) == start)		      \
 	  {								      \
 	    /* Combine processing the sections.  */			      \
 	    ranges[0].size += size;					      \
@@ -121,20 +114,13 @@ elf_machine_lazy_rel (struct link_map *map, struct r_scope_elem *scope[],
 	  }								      \
       }									      \
 									      \
-    if (ELF_DURING_STARTUP)						      \
-      elf_dynamic_do_##reloc ((map), scope, ranges[0].start, ranges[0].size,  \
-			      ranges[0].nrelative, 0, skip_ifunc);  \
-    else								      \
-      {									      \
-	int ranges_index;						      \
-	for (ranges_index = 0; ranges_index < 2; ++ranges_index)	      \
-	  elf_dynamic_do_##reloc ((map), scope,				      \
-				  ranges[ranges_index].start,		      \
-				  ranges[ranges_index].size,		      \
-				  ranges[ranges_index].nrelative,	      \
-				  ranges[ranges_index].lazy,		      \
-				  skip_ifunc);		      \
-      }									      \
+      for (int ranges_index = 0; ranges_index < 2; ++ranges_index)	      \
+        elf_dynamic_do_##reloc ((map), scope,				      \
+				ranges[ranges_index].start,		      \
+				ranges[ranges_index].size,		      \
+				ranges[ranges_index].nrelative,		      \
+				ranges[ranges_index].lazy,		      \
+				skip_ifunc);				      \
   } while (0)
 
 # if ELF_MACHINE_NO_REL || ELF_MACHINE_NO_RELA
-- 
2.32.0

Re: [PATCH 1/3] arm: Use internal symbol for _dl_argv on _dl_start_user

[Fāng-ruì Sòng]

On 2021-11-01, Adhemerval Zanella wrote:
>The lld does not support R_ARM_GOTOFF32 to preemptible symbol (_dl_argv
>has global visibility).  Use the internal alias instead (one option

s/global visibility/default visibility/

>would to use HIDDEN_JUMPTARGET, bu the macro is not defined for
>!__ASSEMBLER__ and I made this patch arm-specific to avoid require to
>check extensivelly on other architecture it this might break something).
>
>Checked on arm-linux-gnueabihf.
>---
> sysdeps/arm/dl-machine.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/sysdeps/arm/dl-machine.h b/sysdeps/arm/dl-machine.h
>index c758bba169..4ecdde4355 100644
>--- a/sysdeps/arm/dl-machine.h
>+++ b/sysdeps/arm/dl-machine.h
>@@ -202,7 +202,7 @@ _dl_start_user:\n\
> .L_FINI_PROC:\n\
> 	.word	_dl_fini(GOTOFF)\n\
> .L_ARGV:\n\
>-	.word	_dl_argv(GOTOFF)\n\
>+	.word	__GI__dl_argv(GOTOFF)\n\
> .L_LOADED:\n\
> 	.word	_rtld_local(GOTOFF)\n\
> .previous\n\
>-- 
>2.32.0
>

Reviewed-by: Fangrui Song <maskray@google.com>

Re: [PATCH 2/3] arm: Use have-mtls-dialect-gnu2 to check for ARM TLS descriptors support

[Fāng-ruì Sòng]

On 2021-11-01, Adhemerval Zanella wrote:
>The lld linker does not support TLSDESC for arm.  The have-arm-tls-desc
>is a leftover of 56583289b1 to support NaCL.
>
>Checked on arm-linux-gnueabihf.

Thanks!

>---
> sysdeps/arm/Makefile | 7 +------
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
>diff --git a/sysdeps/arm/Makefile b/sysdeps/arm/Makefile
>index fb4164f0d9..da4226c8c8 100644
>--- a/sysdeps/arm/Makefile
>+++ b/sysdeps/arm/Makefile
>@@ -1,11 +1,6 @@
> gnulib-arch = $(elf-objpfx)libgcc-stubs.a
> static-gnulib-arch = $(elf-objpfx)libgcc-stubs.a
>
>-# All supported build tools support TLS descriptors, but the OS may not.
>-ifndef have-arm-tls-desc
>-have-arm-tls-desc = yes
>-endif
>-
> ifeq ($(subdir),elf)
> sysdep-dl-routines += tlsdesc dl-tlsdesc
> sysdep_routines += aeabi_unwind_cpp_pr1 find_exidx
>@@ -18,7 +13,7 @@ $(objpfx)libgcc-stubs.a: $(objpfx)aeabi_unwind_cpp_pr1.os
> lib-noranlib: $(objpfx)libgcc-stubs.a
>
> ifeq ($(build-shared),yes)
>-ifeq ($(have-arm-tls-desc),yes)
>+ifeq (yes,$(have-mtls-dialect-gnu2))
> tests += tst-armtlsdescloc tst-armtlsdescextnow tst-armtlsdescextlazy
> modules-names += tst-armtlsdesclocmod
> modules-names += tst-armtlsdescextlazymod tst-armtlsdescextnowmod
>-- 
>2.32.0
>
Reviewed-by: Fangrui Song <maskray@google.com>

Re: [PATCH 3/3] elf: Assume disjointed .rela.dyn and .rela.plt for loader

[Adhemerval Zanella]

On 01/11/2021 09:50, Adhemerval Zanella wrote:
> The linker might add another section between the two relocation section
> for the loader as well.  For instance on arm, lld does:
> 
>   [ 7] .rel.dyn          REL             000007f0 0007f0 000088 08   A	1   0  4
>   [ 8] .ARM.exidx        ARM_EXIDX       00000878 000878 0000b0 00  AL1	2   0  4
>   [ 9] .rel.plt          REL             00000928 000928 000028 08  AI	1  17  4
> 
> This patch removes the ELF_DURING_STARTUP optimization and assume
> both section might no be linear.
> 
> Checked on x86_64, i686, aarch64, armhf, powerpc64le, powerpc64,
> and powerpc (to check if this breaks on different architectures).

Following up the discussion on glibc call, below it an update of the
patch description.

---

The patch removes the the ELF_DURING_STARTUP optimization and assume
both .rel.dyn and .rel.plt might no be linear.  This allows some
code simplification since relocation will be handled independently
where it done on bootstrap.

I could not see any performance implications, at least on x86_64.
Running 10000 time the command

  LD_DEBUG=statistics ./elf/ld.so ./libc.so

And filtering the "total startup time in dynamic loader" result,
the geometric mean I see are:

                  patched       master
  Ryzen 7 5900x     24140        24952
  i7-4510U          45957        45982

(the results do show some variation, I did not make any statistical
analysis).

It also allows build arm with lld, since it inserts ".ARM.exidx"
between ".rel.dyn" and ".rel.plt" for the loader.

Checked on x86_64-linux-gnu and arm-linux-gnueabihf.

> ---
>  elf/dynamic-link.h | 32 +++++++++-----------------------
>  1 file changed, 9 insertions(+), 23 deletions(-)
> 
> diff --git a/elf/dynamic-link.h b/elf/dynamic-link.h
> index ac4cc70dea..f619615e5c 100644
> --- a/elf/dynamic-link.h
> +++ b/elf/dynamic-link.h
> @@ -65,12 +65,6 @@ elf_machine_lazy_rel (struct link_map *map, struct r_scope_elem *scope[],
>  
>  #ifdef RESOLVE_MAP
>  
> -# if defined RTLD_BOOTSTRAP || defined STATIC_PIE_BOOTSTRAP
> -#  define ELF_DURING_STARTUP (1)
> -# else
> -#  define ELF_DURING_STARTUP (0)
> -# endif
> -
>  /* Get the definitions of `elf_dynamic_do_rel' and `elf_dynamic_do_rela'.
>     These functions are almost identical, so we use cpp magic to avoid
>     duplicating their code.  It cannot be done in a more general function
> @@ -106,9 +100,8 @@ elf_machine_lazy_rel (struct link_map *map, struct r_scope_elem *scope[],
>  									      \
>  	if (ranges[0].start + ranges[0].size == (start + size))		      \
>  	  ranges[0].size -= size;					      \
> -	if (ELF_DURING_STARTUP						      \
> -	    || (!(do_lazy)						      \
> -		&& (ranges[0].start + ranges[0].size) == start))	      \
> +	if (!(do_lazy)							      \
> +	    && (ranges[0].start + ranges[0].size) == start)		      \
>  	  {								      \
>  	    /* Combine processing the sections.  */			      \
>  	    ranges[0].size += size;					      \
> @@ -121,20 +114,13 @@ elf_machine_lazy_rel (struct link_map *map, struct r_scope_elem *scope[],
>  	  }								      \
>        }									      \
>  									      \
> -    if (ELF_DURING_STARTUP)						      \
> -      elf_dynamic_do_##reloc ((map), scope, ranges[0].start, ranges[0].size,  \
> -			      ranges[0].nrelative, 0, skip_ifunc);  \
> -    else								      \
> -      {									      \
> -	int ranges_index;						      \
> -	for (ranges_index = 0; ranges_index < 2; ++ranges_index)	      \
> -	  elf_dynamic_do_##reloc ((map), scope,				      \
> -				  ranges[ranges_index].start,		      \
> -				  ranges[ranges_index].size,		      \
> -				  ranges[ranges_index].nrelative,	      \
> -				  ranges[ranges_index].lazy,		      \
> -				  skip_ifunc);		      \
> -      }									      \
> +      for (int ranges_index = 0; ranges_index < 2; ++ranges_index)	      \
> +        elf_dynamic_do_##reloc ((map), scope,				      \
> +				ranges[ranges_index].start,		      \
> +				ranges[ranges_index].size,		      \
> +				ranges[ranges_index].nrelative,		      \
> +				ranges[ranges_index].lazy,		      \
> +				skip_ifunc);				      \
>    } while (0)
>  
>  # if ELF_MACHINE_NO_REL || ELF_MACHINE_NO_RELA
> 

Re: [PATCH 3/3] elf: Assume disjointed .rela.dyn and .rela.plt for loader

[H.J. Lu]

On Mon, Nov 1, 2021 at 9:44 AM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 01/11/2021 09:50, Adhemerval Zanella wrote:
> > The linker might add another section between the two relocation section
> > for the loader as well.  For instance on arm, lld does:
> >
> >   [ 7] .rel.dyn          REL             000007f0 0007f0 000088 08   A        1   0  4
> >   [ 8] .ARM.exidx        ARM_EXIDX       00000878 000878 0000b0 00  AL1       2   0  4
> >   [ 9] .rel.plt          REL             00000928 000928 000028 08  AI        1  17  4
> >
> > This patch removes the ELF_DURING_STARTUP optimization and assume
> > both section might no be linear.
> >
> > Checked on x86_64, i686, aarch64, armhf, powerpc64le, powerpc64,
> > and powerpc (to check if this breaks on different architectures).
>
> Following up the discussion on glibc call, below it an update of the
> patch description.
>
> ---
>
> The patch removes the the ELF_DURING_STARTUP optimization and assume
> both .rel.dyn and .rel.plt might no be linear.  This allows some
> code simplification since relocation will be handled independently
> where it done on bootstrap.
>
> I could not see any performance implications, at least on x86_64.
> Running 10000 time the command
>
>   LD_DEBUG=statistics ./elf/ld.so ./libc.so
>
> And filtering the "total startup time in dynamic loader" result,
> the geometric mean I see are:
>
>                   patched       master
>   Ryzen 7 5900x     24140        24952
>   i7-4510U          45957        45982
>
> (the results do show some variation, I did not make any statistical
> analysis).
>
> It also allows build arm with lld, since it inserts ".ARM.exidx"
> between ".rel.dyn" and ".rel.plt" for the loader.
>
> Checked on x86_64-linux-gnu and arm-linux-gnueabihf.
>
> > ---
> >  elf/dynamic-link.h | 32 +++++++++-----------------------
> >  1 file changed, 9 insertions(+), 23 deletions(-)
> >
> > diff --git a/elf/dynamic-link.h b/elf/dynamic-link.h
> > index ac4cc70dea..f619615e5c 100644
> > --- a/elf/dynamic-link.h
> > +++ b/elf/dynamic-link.h
> > @@ -65,12 +65,6 @@ elf_machine_lazy_rel (struct link_map *map, struct r_scope_elem *scope[],
> >
> >  #ifdef RESOLVE_MAP
> >
> > -# if defined RTLD_BOOTSTRAP || defined STATIC_PIE_BOOTSTRAP
> > -#  define ELF_DURING_STARTUP (1)
> > -# else
> > -#  define ELF_DURING_STARTUP (0)
> > -# endif
> > -
> >  /* Get the definitions of `elf_dynamic_do_rel' and `elf_dynamic_do_rela'.
> >     These functions are almost identical, so we use cpp magic to avoid
> >     duplicating their code.  It cannot be done in a more general function
> > @@ -106,9 +100,8 @@ elf_machine_lazy_rel (struct link_map *map, struct r_scope_elem *scope[],
> >                                                                             \
> >       if (ranges[0].start + ranges[0].size == (start + size))               \
> >         ranges[0].size -= size;                                             \
> > -     if (ELF_DURING_STARTUP                                                \
> > -         || (!(do_lazy)                                                    \
> > -             && (ranges[0].start + ranges[0].size) == start))              \
> > +     if (!(do_lazy)                                                        \
> > +         && (ranges[0].start + ranges[0].size) == start)                   \
> >         {                                                                   \
> >           /* Combine processing the sections.  */                           \
> >           ranges[0].size += size;                                           \
> > @@ -121,20 +114,13 @@ elf_machine_lazy_rel (struct link_map *map, struct r_scope_elem *scope[],
> >         }                                                                   \
> >        }                                                                            \
> >                                                                             \
> > -    if (ELF_DURING_STARTUP)                                                \
> > -      elf_dynamic_do_##reloc ((map), scope, ranges[0].start, ranges[0].size,  \
> > -                           ranges[0].nrelative, 0, skip_ifunc);  \
> > -    else                                                                   \
> > -      {                                                                            \
> > -     int ranges_index;                                                     \
> > -     for (ranges_index = 0; ranges_index < 2; ++ranges_index)              \
> > -       elf_dynamic_do_##reloc ((map), scope,                               \
> > -                               ranges[ranges_index].start,                 \
> > -                               ranges[ranges_index].size,                  \
> > -                               ranges[ranges_index].nrelative,             \
> > -                               ranges[ranges_index].lazy,                  \
> > -                               skip_ifunc);                \
> > -      }                                                                            \
> > +      for (int ranges_index = 0; ranges_index < 2; ++ranges_index)         \
> > +        elf_dynamic_do_##reloc ((map), scope,                                      \
> > +                             ranges[ranges_index].start,                   \
> > +                             ranges[ranges_index].size,                    \
> > +                             ranges[ranges_index].nrelative,               \
> > +                             ranges[ranges_index].lazy,                    \
> > +                             skip_ifunc);                                  \
> >    } while (0)
> >
> >  # if ELF_MACHINE_NO_REL || ELF_MACHINE_NO_RELA
> >

Please submit the v2 patch with the updated commit log.

Thanks.

-- 
H.J.