riscv: Change the relocations handled for RTLD_BOOTSTRAP

  The RTLD_BOOTSTRAP branch is used to relocate ld.so itself.  It only
needs to handle RELATIVE, GLOB_DAT, and the symbolic relocation type
(R_RISCV_{32,64}).  NONE and IRELATIVE can be removed.

The code relies on ld.so having DT_RELACOUNT so that the RTLD_BOOTSTRAP
branch does not need handle RELATIVE.  Drop this minor size
optimization for clarity.
---
 sysdeps/riscv/dl-machine.h | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)
  

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 the symbolic relocation type
>(R_RISCV_{32,64}).  NONE and IRELATIVE can be removed.
>
>The code relies on ld.so having DT_RELACOUNT so that the RTLD_BOOTSTRAP
>branch does not need handle RELATIVE.  Drop this minor size
>optimization for clarity.
>---
> sysdeps/riscv/dl-machine.h | 23 ++++++++++-------------
> 1 file changed, 10 insertions(+), 13 deletions(-)
>
>diff --git a/sysdeps/riscv/dl-machine.h b/sysdeps/riscv/dl-machine.h
>index 9e026ae011..a60a452952 100644
>--- a/sysdeps/riscv/dl-machine.h
>+++ b/sysdeps/riscv/dl-machine.h
>@@ -181,7 +181,15 @@ elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
>
>   switch (r_type)
>     {
>-#ifndef RTLD_BOOTSTRAP
>+    case R_RISCV_RELATIVE:
>+      *addr_field = map->l_addr + reloc->r_addend;
>+      break;
>+    case R_RISCV_JUMP_SLOT:
>+    case __WORDSIZE == 64 ? R_RISCV_64 : R_RISCV_32:
>+      *addr_field = value;
>+      break;
>+
>+# ifndef RTLD_BOOTSTRAP
>     case __WORDSIZE == 64 ? R_RISCV_TLS_DTPMOD64 : R_RISCV_TLS_DTPMOD32:
>       if (sym_map)
> 	*addr_field = sym_map->l_tls_modid;
>@@ -232,13 +240,6 @@ elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
> 	memcpy (reloc_addr, (void *)value, size);
> 	break;
>       }
>-#endif
>-
>-#if !defined RTLD_BOOTSTRAP
>-    case R_RISCV_RELATIVE:
>-      *addr_field = map->l_addr + reloc->r_addend;
>-      break;
>-#endif
>
>     case R_RISCV_IRELATIVE:
>       value = map->l_addr + reloc->r_addend;
>@@ -247,13 +248,9 @@ elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
>       *addr_field = value;
>       break;
>
>-    case R_RISCV_JUMP_SLOT:
>-    case __WORDSIZE == 64 ? R_RISCV_64 : R_RISCV_32:
>-      *addr_field = value;
>-      break;
>-
>     case R_RISCV_NONE:
>       break;
>+# endif /* !RTLD_BOOTSTRAP */
>
>     default:
>       _dl_reloc_bad_type (map, r_type, 0);
>-- 
>2.36.1.255.ge46751e96f-goog
>

Ping:)
  

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 the symbolic relocation type
>(R_RISCV_{32,64}).  NONE and IRELATIVE can be removed.
>
>The code relies on ld.so having DT_RELACOUNT so that the RTLD_BOOTSTRAP
>branch does not need handle RELATIVE.  Drop this minor size
>optimization for clarity.
>---
> sysdeps/riscv/dl-machine.h | 23 ++++++++++-------------
> 1 file changed, 10 insertions(+), 13 deletions(-)
>
>diff --git a/sysdeps/riscv/dl-machine.h b/sysdeps/riscv/dl-machine.h
>index 9e026ae011..a60a452952 100644
>--- a/sysdeps/riscv/dl-machine.h
>+++ b/sysdeps/riscv/dl-machine.h
>@@ -181,7 +181,15 @@ elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
>
>   switch (r_type)
>     {
>-#ifndef RTLD_BOOTSTRAP
>+    case R_RISCV_RELATIVE:
>+      *addr_field = map->l_addr + reloc->r_addend;
>+      break;
>+    case R_RISCV_JUMP_SLOT:
>+    case __WORDSIZE == 64 ? R_RISCV_64 : R_RISCV_32:
>+      *addr_field = value;
>+      break;
>+
>+# ifndef RTLD_BOOTSTRAP
>     case __WORDSIZE == 64 ? R_RISCV_TLS_DTPMOD64 : R_RISCV_TLS_DTPMOD32:
>       if (sym_map)
> 	*addr_field = sym_map->l_tls_modid;
>@@ -232,13 +240,6 @@ elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
> 	memcpy (reloc_addr, (void *)value, size);
> 	break;
>       }
>-#endif
>-
>-#if !defined RTLD_BOOTSTRAP
>-    case R_RISCV_RELATIVE:
>-      *addr_field = map->l_addr + reloc->r_addend;
>-      break;
>-#endif
>
>     case R_RISCV_IRELATIVE:
>       value = map->l_addr + reloc->r_addend;
>@@ -247,13 +248,9 @@ elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
>       *addr_field = value;
>       break;
>
>-    case R_RISCV_JUMP_SLOT:
>-    case __WORDSIZE == 64 ? R_RISCV_64 : R_RISCV_32:
>-      *addr_field = value;
>-      break;
>-
>     case R_RISCV_NONE:
>       break;
>+# endif /* !RTLD_BOOTSTRAP */
>
>     default:
>       _dl_reloc_bad_type (map, r_type, 0);
>-- 
>2.36.1.255.ge46751e96f-goog
  

On Wed, 15 Jun 2022 16:07:37 PDT (-0700), maskray@google.com 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 the symbolic relocation type
>>(R_RISCV_{32,64}).  NONE and IRELATIVE can be removed.
>>
>>The code relies on ld.so having DT_RELACOUNT so that the RTLD_BOOTSTRAP
>>branch does not need handle RELATIVE.  Drop this minor size
>>optimization for clarity.
>>---
>> sysdeps/riscv/dl-machine.h | 23 ++++++++++-------------
>> 1 file changed, 10 insertions(+), 13 deletions(-)
>>
>>diff --git a/sysdeps/riscv/dl-machine.h b/sysdeps/riscv/dl-machine.h
>>index 9e026ae011..a60a452952 100644
>>--- a/sysdeps/riscv/dl-machine.h
>>+++ b/sysdeps/riscv/dl-machine.h
>>@@ -181,7 +181,15 @@ elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
>>
>>   switch (r_type)
>>     {
>>-#ifndef RTLD_BOOTSTRAP
>>+    case R_RISCV_RELATIVE:
>>+      *addr_field = map->l_addr + reloc->r_addend;
>>+      break;
>>+    case R_RISCV_JUMP_SLOT:
>>+    case __WORDSIZE == 64 ? R_RISCV_64 : R_RISCV_32:
>>+      *addr_field = value;
>>+      break;
>>+
>>+# ifndef RTLD_BOOTSTRAP
>>     case __WORDSIZE == 64 ? R_RISCV_TLS_DTPMOD64 : R_RISCV_TLS_DTPMOD32:
>>       if (sym_map)
>> 	*addr_field = sym_map->l_tls_modid;
>>@@ -232,13 +240,6 @@ elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
>> 	memcpy (reloc_addr, (void *)value, size);
>> 	break;
>>       }
>>-#endif
>>-
>>-#if !defined RTLD_BOOTSTRAP
>>-    case R_RISCV_RELATIVE:
>>-      *addr_field = map->l_addr + reloc->r_addend;
>>-      break;
>>-#endif
>>
>>     case R_RISCV_IRELATIVE:
>>       value = map->l_addr + reloc->r_addend;
>>@@ -247,13 +248,9 @@ elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
>>       *addr_field = value;
>>       break;
>>
>>-    case R_RISCV_JUMP_SLOT:
>>-    case __WORDSIZE == 64 ? R_RISCV_64 : R_RISCV_32:
>>-      *addr_field = value;
>>-      break;
>>-
>>     case R_RISCV_NONE:
>>       break;
>>+# endif /* !RTLD_BOOTSTRAP */
>>
>>     default:
>>       _dl_reloc_bad_type (map, r_type, 0);
>>--
>>2.36.1.255.ge46751e96f-goog

Hey, sorry I missed the pings.  This is really doing two things 
(dropping IRELATIVE and merging the ifdef) which makes it harder to read 
the diff than it could be.

IIUC this should work right now, but it also adds a dependency on 
RTDL_BOOTSTRAP having no IRELATIVE.  That should be fine as it is, 
because we don't have any IFUNCs in ld.so.  I'm not sure if it's ever 
sane to end up with those, but there's been some discussion around how 
that all fits together in that dynamic resolver patch/psABI issue.

If IFUNCs currently work in ld.so then I think we could call this an ABI 
break, but that's kind of a stretch as we could also say no binaries 
exist that have that behavior.  If they're currently broken then this 
seems fine, but if they're not then I'd err on the side of keeping the 
IRELATIVE case under RTLD_BOOTSTRAP as we can always remove it later.

That said,

Acked-by: Palmer Dabbelt <palmer@rivosinc.com>

in case someone wants to commit it and I drop the ball again.

Thanks!
  

On 2022-06-15, Palmer Dabbelt wrote:
>On Wed, 15 Jun 2022 16:07:37 PDT (-0700), maskray@google.com 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 the symbolic relocation type
>>>(R_RISCV_{32,64}).  NONE and IRELATIVE can be removed.
>>>
>>>The code relies on ld.so having DT_RELACOUNT so that the RTLD_BOOTSTRAP
>>>branch does not need handle RELATIVE.  Drop this minor size
>>>optimization for clarity.
>>>---
>>>sysdeps/riscv/dl-machine.h | 23 ++++++++++-------------
>>>1 file changed, 10 insertions(+), 13 deletions(-)
>>>
>>>diff --git a/sysdeps/riscv/dl-machine.h b/sysdeps/riscv/dl-machine.h
>>>index 9e026ae011..a60a452952 100644
>>>--- a/sysdeps/riscv/dl-machine.h
>>>+++ b/sysdeps/riscv/dl-machine.h
>>>@@ -181,7 +181,15 @@ elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
>>>
>>>  switch (r_type)
>>>    {
>>>-#ifndef RTLD_BOOTSTRAP
>>>+    case R_RISCV_RELATIVE:
>>>+      *addr_field = map->l_addr + reloc->r_addend;
>>>+      break;
>>>+    case R_RISCV_JUMP_SLOT:
>>>+    case __WORDSIZE == 64 ? R_RISCV_64 : R_RISCV_32:
>>>+      *addr_field = value;
>>>+      break;
>>>+
>>>+# ifndef RTLD_BOOTSTRAP
>>>    case __WORDSIZE == 64 ? R_RISCV_TLS_DTPMOD64 : R_RISCV_TLS_DTPMOD32:
>>>      if (sym_map)
>>>	*addr_field = sym_map->l_tls_modid;
>>>@@ -232,13 +240,6 @@ elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
>>>	memcpy (reloc_addr, (void *)value, size);
>>>	break;
>>>      }
>>>-#endif
>>>-
>>>-#if !defined RTLD_BOOTSTRAP
>>>-    case R_RISCV_RELATIVE:
>>>-      *addr_field = map->l_addr + reloc->r_addend;
>>>-      break;
>>>-#endif
>>>
>>>    case R_RISCV_IRELATIVE:
>>>      value = map->l_addr + reloc->r_addend;
>>>@@ -247,13 +248,9 @@ elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
>>>      *addr_field = value;
>>>      break;
>>>
>>>-    case R_RISCV_JUMP_SLOT:
>>>-    case __WORDSIZE == 64 ? R_RISCV_64 : R_RISCV_32:
>>>-      *addr_field = value;
>>>-      break;
>>>-
>>>    case R_RISCV_NONE:
>>>      break;
>>>+# endif /* !RTLD_BOOTSTRAP */
>>>
>>>    default:
>>>      _dl_reloc_bad_type (map, r_type, 0);
>>>--
>>>2.36.1.255.ge46751e96f-goog
>
>Hey, sorry I missed the pings.  This is really doing two things 
>(dropping IRELATIVE and merging the ifdef) which makes it harder to 
>read the diff than it could be.

No worries:)  Just tring to figure out what email addresses you prefer
nowadays.  I assume that your adjustment of the Cc: list has answered my
question.

>IIUC this should work right now, but it also adds a dependency on 
>RTDL_BOOTSTRAP having no IRELATIVE.  That should be fine as it is, 
>because we don't have any IFUNCs in ld.so.  I'm not sure if it's ever 
>sane to end up with those, but there's been some discussion around how 
>that all fits together in that dynamic resolver patch/psABI issue.

Right.  ld.so should not have IRELATIVE.
sysdeps/x86_64/dl-machine.h's RTDL_BOOTSTRAP code does not handle IRELATIVE.
I am fixing aarch64
(https://sourceware.org/pipermail/libc-alpha/2022-June/139425.html) and
riscv so that new ports (e.g. LoongArch) will not unnecessarily copy
them due to cargo culting
(https://sourceware.org/pipermail/libc-alpha/2022-June/139689.html)

An idea is that we may need a `readelf -rW` check that there are only
expected relocations.  I may try adding such a test.

Thanks!

>If IFUNCs currently work in ld.so then I think we could call this an 
>ABI break, but that's kind of a stretch as we could also say no 
>binaries exist that have that behavior.  If they're currently broken 
>then this seems fine, but if they're not then I'd err on the side of 
>keeping the IRELATIVE case under RTLD_BOOTSTRAP as we can always 
>remove it later.
>
>That said,
>
>Acked-by: Palmer Dabbelt <palmer@rivosinc.com>
>
>in case someone wants to commit it and I drop the ball again.
>
>Thanks!