[PATCH] aarch64: Handle fewer relocations for RTLD_BOOTSTRAP

[PATCH] aarch64: Handle fewer relocations for RTLD_BOOTSTRAP

[Fangrui Song]

The RTLD_BOOTSTRAP branch is used to relocate ld.so itself.  It only
needs to handle RELATIVE, GLOB_DAT, and JUMP_SLOT.

Tested on aarch64-linux-gnu.
---
 sysdeps/aarch64/dl-machine.h | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/sysdeps/aarch64/dl-machine.h b/sysdeps/aarch64/dl-machine.h
index 292abe5152..ae8b14425a 100644
--- a/sysdeps/aarch64/dl-machine.h
+++ b/sysdeps/aarch64/dl-machine.h
@@ -178,7 +178,9 @@ elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
       return;
   else
     {
+# ifndef RTLD_BOOTSTRAP
       const ElfW(Sym) *const refsym = sym;
+# endif
       struct link_map *sym_map = RESOLVE_MAP (map, scope, &sym, version,
 					      r_type);
       ElfW(Addr) value = SYMBOL_ADDRESS (sym_map, sym, true);
@@ -191,6 +193,18 @@ elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
 
       switch (r_type)
 	{
+	case AARCH64_R(GLOB_DAT):
+	case AARCH64_R(JUMP_SLOT):
+	  *reloc_addr = value + reloc->r_addend;
+	  break;
+
+# ifndef RTLD_BOOTSTRAP
+	case AARCH64_R(ABS32):
+#  ifdef __LP64__
+	case AARCH64_R(ABS64):
+#  endif
+	  *reloc_addr = value + reloc->r_addend;
+	  break;
 	case AARCH64_R(COPY):
 	  if (sym == NULL)
 	      break;
@@ -210,30 +224,17 @@ elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
 		  ? sym->st_size : refsym->st_size);
 	  break;
 
-	case AARCH64_R(RELATIVE):
-	case AARCH64_R(GLOB_DAT):
-	case AARCH64_R(JUMP_SLOT):
-	case AARCH64_R(ABS32):
-#ifdef __LP64__
-	case AARCH64_R(ABS64):
-#endif
-	  *reloc_addr = value + reloc->r_addend;
-	  break;
-
 	case AARCH64_R(TLSDESC):
 	  {
 	    struct tlsdesc volatile *td =
 	      (struct tlsdesc volatile *)reloc_addr;
-#ifndef RTLD_BOOTSTRAP
 	    if (! sym)
 	      {
 		td->arg = (void*)reloc->r_addend;
 		td->entry = _dl_tlsdesc_undefweak;
 	      }
 	    else
-#endif
 	      {
-#ifndef RTLD_BOOTSTRAP
 # ifndef SHARED
 		CHECK_STATIC_TLS (map, sym_map);
 # else
@@ -245,7 +246,6 @@ elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
 		  }
 		else
 # endif
-#endif
 		  {
 		    td->arg = (void*)(sym->st_value + sym_map->l_tls_offset
 				      + reloc->r_addend);
@@ -256,14 +256,10 @@ elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
 	  }
 
 	case AARCH64_R(TLS_DTPMOD):
-#ifdef RTLD_BOOTSTRAP
-	  *reloc_addr = 1;
-#else
 	  if (sym_map != NULL)
 	    {
 	      *reloc_addr = sym_map->l_tls_modid;
 	    }
-#endif
 	  break;
 
 	case AARCH64_R(TLS_DTPREL):
@@ -286,6 +282,7 @@ elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
 	    value = elf_ifunc_invoke (value);
 	  *reloc_addr = value;
 	  break;
+# endif /* !RTLD_BOOTSTRAP */
 
 	default:
 	  _dl_reloc_bad_type (map, r_type, 0);
-- 
2.36.1.255.ge46751e96f-goog

Re: [PATCH] aarch64: Handle fewer relocations for RTLD_BOOTSTRAP

[Fangrui Song]

On 2022-06-05, Fangrui Song wrote:
>The RTLD_BOOTSTRAP branch is used to relocate ld.so itself.  It only
>needs to handle RELATIVE, GLOB_DAT, and JUMP_SLOT.
>
>Tested on aarch64-linux-gnu.
>---
> sysdeps/aarch64/dl-machine.h | 33 +++++++++++++++------------------
> 1 file changed, 15 insertions(+), 18 deletions(-)
>
>diff --git a/sysdeps/aarch64/dl-machine.h b/sysdeps/aarch64/dl-machine.h
>index 292abe5152..ae8b14425a 100644
>--- a/sysdeps/aarch64/dl-machine.h
>+++ b/sysdeps/aarch64/dl-machine.h
>@@ -178,7 +178,9 @@ elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
>       return;
>   else
>     {
>+# ifndef RTLD_BOOTSTRAP
>       const ElfW(Sym) *const refsym = sym;
>+# endif
>       struct link_map *sym_map = RESOLVE_MAP (map, scope, &sym, version,
> 					      r_type);
>       ElfW(Addr) value = SYMBOL_ADDRESS (sym_map, sym, true);
>@@ -191,6 +193,18 @@ elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
>
>       switch (r_type)
> 	{
>+	case AARCH64_R(GLOB_DAT):
>+	case AARCH64_R(JUMP_SLOT):
>+	  *reloc_addr = value + reloc->r_addend;
>+	  break;
>+
>+# ifndef RTLD_BOOTSTRAP
>+	case AARCH64_R(ABS32):
>+#  ifdef __LP64__
>+	case AARCH64_R(ABS64):
>+#  endif
>+	  *reloc_addr = value + reloc->r_addend;
>+	  break;
> 	case AARCH64_R(COPY):
> 	  if (sym == NULL)
> 	      break;
>@@ -210,30 +224,17 @@ elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
> 		  ? sym->st_size : refsym->st_size);
> 	  break;
>
>-	case AARCH64_R(RELATIVE):
>-	case AARCH64_R(GLOB_DAT):
>-	case AARCH64_R(JUMP_SLOT):
>-	case AARCH64_R(ABS32):
>-#ifdef __LP64__
>-	case AARCH64_R(ABS64):
>-#endif
>-	  *reloc_addr = value + reloc->r_addend;
>-	  break;
>-
> 	case AARCH64_R(TLSDESC):
> 	  {
> 	    struct tlsdesc volatile *td =
> 	      (struct tlsdesc volatile *)reloc_addr;
>-#ifndef RTLD_BOOTSTRAP
> 	    if (! sym)
> 	      {
> 		td->arg = (void*)reloc->r_addend;
> 		td->entry = _dl_tlsdesc_undefweak;
> 	      }
> 	    else
>-#endif
> 	      {
>-#ifndef RTLD_BOOTSTRAP
> # ifndef SHARED
> 		CHECK_STATIC_TLS (map, sym_map);
> # else
>@@ -245,7 +246,6 @@ elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
> 		  }
> 		else
> # endif
>-#endif
> 		  {
> 		    td->arg = (void*)(sym->st_value + sym_map->l_tls_offset
> 				      + reloc->r_addend);
>@@ -256,14 +256,10 @@ elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
> 	  }
>
> 	case AARCH64_R(TLS_DTPMOD):
>-#ifdef RTLD_BOOTSTRAP
>-	  *reloc_addr = 1;
>-#else
> 	  if (sym_map != NULL)
> 	    {
> 	      *reloc_addr = sym_map->l_tls_modid;
> 	    }
>-#endif
> 	  break;
>
> 	case AARCH64_R(TLS_DTPREL):
>@@ -286,6 +282,7 @@ elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
> 	    value = elf_ifunc_invoke (value);
> 	  *reloc_addr = value;
> 	  break;
>+# endif /* !RTLD_BOOTSTRAP */
>
> 	default:
> 	  _dl_reloc_bad_type (map, r_type, 0);
>-- 
>2.36.1.255.ge46751e96f-goog

Cc machine maintainers

Re: [PATCH] aarch64: Handle fewer relocations for RTLD_BOOTSTRAP

[Szabolcs Nagy]

The 06/10/2022 19:32, Fangrui Song wrote:
> On 2022-06-05, Fangrui Song wrote:
> > The RTLD_BOOTSTRAP branch is used to relocate ld.so itself.  It only
> > needs to handle RELATIVE, GLOB_DAT, and JUMP_SLOT.

the motivation is not clear to me.

is there an issue with other relocations or is this just a cleanup patch?

Re: [PATCH] aarch64: Handle fewer relocations for RTLD_BOOTSTRAP

[Fangrui Song]

On 2022-06-13, Szabolcs Nagy wrote:
>The 06/10/2022 19:32, Fangrui Song wrote:
>> On 2022-06-05, Fangrui Song wrote:
>> > The RTLD_BOOTSTRAP branch is used to relocate ld.so itself.  It only
>> > needs to handle RELATIVE, GLOB_DAT, and JUMP_SLOT.
>
>the motivation is not clear to me.
>
>is there an issue with other relocations or is this just a cleanup patch?

It is a cleanup: remove dead code paths.

Re: [PATCH] aarch64: Handle fewer relocations for RTLD_BOOTSTRAP

[Florian Weimer]

* Fangrui Song via Libc-alpha:

> On 2022-06-13, Szabolcs Nagy wrote:
>>The 06/10/2022 19:32, Fangrui Song wrote:
>>> On 2022-06-05, Fangrui Song wrote:
>>> > The RTLD_BOOTSTRAP branch is used to relocate ld.so itself.  It only
>>> > needs to handle RELATIVE, GLOB_DAT, and JUMP_SLOT.
>>
>>the motivation is not clear to me.
>>
>>is there an issue with other relocations or is this just a cleanup patch?
>
> It is a cleanup: remove dead code paths.

I think it could be paired (later) with a test that checks that ld.so
uses just the relocation types implemented for self-relocation.

I do not think it's possible in general to use the same code for
self-relocation and subsequent library loading, especially on
HIDDEN_VAR_NEEDS_DYNAMIC_RELOC architectures (I like the new name, by
the way).

Thanks,
Florian

Re: [PATCH] aarch64: Handle fewer relocations for RTLD_BOOTSTRAP

[Szabolcs Nagy]

The 06/14/2022 07:07, Florian Weimer wrote:
> * Fangrui Song via Libc-alpha:
> > On 2022-06-13, Szabolcs Nagy wrote:
> >>The 06/10/2022 19:32, Fangrui Song wrote:
> >>> On 2022-06-05, Fangrui Song wrote:
> >>> > The RTLD_BOOTSTRAP branch is used to relocate ld.so itself.  It only
> >>> > needs to handle RELATIVE, GLOB_DAT, and JUMP_SLOT.
> >>
> >>the motivation is not clear to me.
> >>
> >>is there an issue with other relocations or is this just a cleanup patch?
> >
> > It is a cleanup: remove dead code paths.
> 
> I think it could be paired (later) with a test that checks that ld.so
> uses just the relocation types implemented for self-relocation.
> 
> I do not think it's possible in general to use the same code for
> self-relocation and subsequent library loading, especially on
> HIDDEN_VAR_NEEDS_DYNAMIC_RELOC architectures (I like the new name, by
> the way).

i see. cleanup is fine.

note that there is static-pie too, which requires a 3rd
way to do relocations (and ideally it would be split to
separate RELATIVE and IRELATIVE reloc processing given
that ifunc requires complex setup like dynamic allocation
for tunables handling. the split is also needed if we want
to support static-pie with HIDDEN_VAR_NEEDS_DYNAMIC_RELOC).

Re: [PATCH] aarch64: Handle fewer relocations for RTLD_BOOTSTRAP

[Fangrui Song]

On 2022-06-14, Szabolcs Nagy wrote:
>The 06/14/2022 07:07, Florian Weimer wrote:
>> * Fangrui Song via Libc-alpha:
>> > On 2022-06-13, Szabolcs Nagy wrote:
>> >>The 06/10/2022 19:32, Fangrui Song wrote:
>> >>> On 2022-06-05, Fangrui Song wrote:
>> >>> > The RTLD_BOOTSTRAP branch is used to relocate ld.so itself.  It only
>> >>> > needs to handle RELATIVE, GLOB_DAT, and JUMP_SLOT.
>> >>
>> >>the motivation is not clear to me.
>> >>
>> >>is there an issue with other relocations or is this just a cleanup patch?
>> >
>> > It is a cleanup: remove dead code paths.
>>
>> I think it could be paired (later) with a test that checks that ld.so
>> uses just the relocation types implemented for self-relocation.
>>
>> I do not think it's possible in general to use the same code for
>> self-relocation and subsequent library loading, especially on
>> HIDDEN_VAR_NEEDS_DYNAMIC_RELOC architectures (I like the new name, by
>> the way).
>
>i see. cleanup is fine.
>
>note that there is static-pie too, which requires a 3rd
>way to do relocations (and ideally it would be split to
>separate RELATIVE and IRELATIVE reloc processing given
>that ifunc requires complex setup like dynamic allocation
>for tunables handling. the split is also needed if we want
>to support static-pie with HIDDEN_VAR_NEEDS_DYNAMIC_RELOC).

dl-reloc-static-pie.c uses the !RTLD_BOOTSTRAP branch, like dl-reloc.c.
I have tested that --enable-static-pie is correct.

I agree that it will be nice to have a readelf -rW test later.
For most ports, .rela?.plt and .rela?.dyn should only contain
RELATIVE|JU?MP_SLOT|GLOB_DAT.
There are a few less popular ports that I am unclear, though.