| Message ID | 20220606050247.3125956-1-maskray@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | riscv: Change the relocations handled for RTLD_BOOTSTRAP | expand |
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!
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);
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(-)