Message ID | 20211101125003.500945-4-adhemerval.zanella@linaro.org |
---|---|
State | New |
Headers | show |
Series | Fix lld build for armhf | expand |
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 >
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.
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