| Message ID | 5294628E.30507@linaro.org |
|---|---|
| State | Accepted |
| Headers | show |
On 26 November 2013 08:57, Will Newton <will.newton@linaro.org> wrote: > diff --git a/ports/sysdeps/aarch64/dl-irel.h b/ports/sysdeps/aarch64/dl-irel.h > index 1a3811e..f37ee39 100644 > --- a/ports/sysdeps/aarch64/dl-irel.h > +++ b/ports/sysdeps/aarch64/dl-irel.h > @@ -22,15 +22,31 @@ > > #include <stdio.h> > #include <unistd.h> > +#include <ldsodefs.h> Will, is this include needed? /Marcus
On 26 November 2013 14:51, Marcus Shawcroft <marcus.shawcroft@gmail.com> wrote: > On 26 November 2013 08:57, Will Newton <will.newton@linaro.org> wrote: > >> diff --git a/ports/sysdeps/aarch64/dl-irel.h b/ports/sysdeps/aarch64/dl-irel.h >> index 1a3811e..f37ee39 100644 >> --- a/ports/sysdeps/aarch64/dl-irel.h >> +++ b/ports/sysdeps/aarch64/dl-irel.h >> @@ -22,15 +22,31 @@ >> >> #include <stdio.h> >> #include <unistd.h> >> +#include <ldsodefs.h> > > Will, is this include needed? Yes, I believe it is needed for GLRO and ELFW, although everything builds without it as ldsodefs.h is included all over the place. stdio.h is required for __libc_fatal, not sure about unistd.h.
On 26 November 2013 08:57, Will Newton <will.newton@linaro.org> wrote: > > Add support for handling the R_AARCH64_IRELATIVE relocation and > STT_GNU_IFUNC symbols to the aarch64 port. > > ports/ChangeLog.aarch64: > > 2013-11-25 Will Newton <will.newton@linaro.org> > > * sysdeps/aarch64/dl-irel.h: Include ldsodefs.h. > (ELF_MACHINE_IRELA): Define. (elf_ifunc_invoke): Pass > hwcap to ifunc resolver function. (elf_irela): New function. > * sysdeps/aarch64/dl-machine.h: Include dl-irel.h. > (elf_machine_rela) Handle STT_GNU_IFUNC symbols and > R_AARCH64_IRELATIVE relocations. (elf_machine_lazy_rel): > Handle R_AARCH64_IRELATIVE relocations. OK /Marcus
On 11/26/2013 03:57 AM, Will Newton wrote: > > Add support for handling the R_AARCH64_IRELATIVE relocation and > STT_GNU_IFUNC symbols to the aarch64 port. > > ports/ChangeLog.aarch64: > > 2013-11-25 Will Newton <will.newton@linaro.org> > > * sysdeps/aarch64/dl-irel.h: Include ldsodefs.h. > (ELF_MACHINE_IRELA): Define. (elf_ifunc_invoke): Pass > hwcap to ifunc resolver function. (elf_irela): New function. > * sysdeps/aarch64/dl-machine.h: Include dl-irel.h. > (elf_machine_rela) Handle STT_GNU_IFUNC symbols and > R_AARCH64_IRELATIVE relocations. (elf_machine_lazy_rel): > Handle R_AARCH64_IRELATIVE relocations. You don't appear to fix elf_ifunc_invoke to pass dl_hwcap to the resolver which will need this to detect the different kinds of hardware? Please see the 32-bit ARM port. Cheers, Carlos.
On 11/26/2013 10:16 AM, Carlos O'Donell wrote: > On 11/26/2013 03:57 AM, Will Newton wrote: >> >> Add support for handling the R_AARCH64_IRELATIVE relocation and >> STT_GNU_IFUNC symbols to the aarch64 port. >> >> ports/ChangeLog.aarch64: >> >> 2013-11-25 Will Newton <will.newton@linaro.org> >> >> * sysdeps/aarch64/dl-irel.h: Include ldsodefs.h. >> (ELF_MACHINE_IRELA): Define. (elf_ifunc_invoke): Pass >> hwcap to ifunc resolver function. (elf_irela): New function. >> * sysdeps/aarch64/dl-machine.h: Include dl-irel.h. >> (elf_machine_rela) Handle STT_GNU_IFUNC symbols and >> R_AARCH64_IRELATIVE relocations. (elf_machine_lazy_rel): >> Handle R_AARCH64_IRELATIVE relocations. > > You don't appear to fix elf_ifunc_invoke to pass dl_hwcap to the > resolver which will need this to detect the different kinds of > hardware? Sorry, I'm wrong, I missed that line in your patch. You do pass dl_hwcap. c.
On 11/26/2013 03:57 AM, Will Newton wrote: > > Add support for handling the R_AARCH64_IRELATIVE relocation and > STT_GNU_IFUNC symbols to the aarch64 port. How did you test this? > ports/ChangeLog.aarch64: > > 2013-11-25 Will Newton <will.newton@linaro.org> > > * sysdeps/aarch64/dl-irel.h: Include ldsodefs.h. > (ELF_MACHINE_IRELA): Define. (elf_ifunc_invoke): Pass > hwcap to ifunc resolver function. (elf_irela): New function. > * sysdeps/aarch64/dl-machine.h: Include dl-irel.h. > (elf_machine_rela) Handle STT_GNU_IFUNC symbols and > R_AARCH64_IRELATIVE relocations. (elf_machine_lazy_rel): > Handle R_AARCH64_IRELATIVE relocations. > --- > ports/sysdeps/aarch64/dl-irel.h | 22 +++++++++++++++++++--- > ports/sysdeps/aarch64/dl-machine.h | 20 ++++++++++++++++++++ > 2 files changed, 39 insertions(+), 3 deletions(-) > > diff --git a/ports/sysdeps/aarch64/dl-irel.h b/ports/sysdeps/aarch64/dl-irel.h > index 1a3811e..f37ee39 100644 > --- a/ports/sysdeps/aarch64/dl-irel.h > +++ b/ports/sysdeps/aarch64/dl-irel.h > @@ -22,15 +22,31 @@ > > #include <stdio.h> > #include <unistd.h> > +#include <ldsodefs.h> > > -/* AArch64 does not yet implement IFUNC support. However since > - 2011-06-20 provision of a elf_ifunc_invoke has been mandatory. */ > +#define ELF_MACHINE_IRELA 1 OK. > > static inline ElfW(Addr) > __attribute ((always_inline)) > elf_ifunc_invoke (ElfW(Addr) addr) > { > - return ((ElfW(Addr) (*) (void)) (addr)) (); > + return ((ElfW(Addr) (*) (unsigned long int)) (addr)) (GLRO(dl_hwcap)); OK. > +} > + > +static inline void > +__attribute ((always_inline)) > +elf_irela (const ElfW(Rela) *reloc) > +{ > + ElfW(Addr) *const reloc_addr = (void *) reloc->r_offset; > + const unsigned long int r_type = ELFW(R_TYPE) (reloc->r_info); > + > + if (__glibc_likely (r_type == R_AARCH64_IRELATIVE)) > + { > + ElfW(Addr) value = elf_ifunc_invoke (reloc->r_addend); > + *reloc_addr = value; > + } > + else > + __libc_fatal ("unexpected reloc type in static binary"); > } OK. > > #endif > diff --git a/ports/sysdeps/aarch64/dl-machine.h b/ports/sysdeps/aarch64/dl-machine.h > index 71dd6b3..01a214f 100644 > --- a/ports/sysdeps/aarch64/dl-machine.h > +++ b/ports/sysdeps/aarch64/dl-machine.h > @@ -23,6 +23,7 @@ > > #include <tls.h> > #include <dl-tlsdesc.h> > +#include <dl-irel.h> > > /* Return nonzero iff ELF header is compatible with the running host. */ > static inline int __attribute__ ((unused)) > @@ -243,6 +244,12 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc, > struct link_map *sym_map = RESOLVE_MAP (&sym, version, r_type); > ElfW(Addr) value = sym_map == NULL ? 0 : sym_map->l_addr + sym->st_value; > > + if (sym != NULL > + && __glibc_unlikely (ELFW(ST_TYPE) (sym->st_info) == STT_GNU_IFUNC) > + && __glibc_likely (sym->st_shndx != SHN_UNDEF) > + && __glibc_likely (!skip_ifunc)) > + value = elf_ifunc_invoke (value); OK. > + > switch (r_type) > { > case R_AARCH64_COPY: > @@ -331,6 +338,12 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc, > } > break; > > + case R_AARCH64_IRELATIVE: > + value = map->l_addr + reloc->r_addend; > + value = elf_ifunc_invoke (value); > + *reloc_addr = value; > + break; OK. > + > default: > _dl_reloc_bad_type (map, r_type, 0); > break; > @@ -374,6 +387,13 @@ elf_machine_lazy_rel (struct link_map *map, > td->entry = (void*)(D_PTR (map, l_info[ADDRIDX (DT_TLSDESC_PLT)]) > + map->l_addr); > } > + else if (__glibc_unlikely (r_type == R_AARCH64_IRELATIVE)) > + { > + ElfW(Addr) value = map->l_addr + reloc->r_addend; > + if (__glibc_likely (!skip_ifunc)) > + value = elf_ifunc_invoke (value); > + *reloc_addr = value; OK. > + } > else > _dl_reloc_bad_type (map, r_type, 1); > } > Looks good to me as long as your testing makes sense. Cheers, Carlos.
On 26 November 2013 15:24, Carlos O'Donell <carlos@redhat.com> wrote: > On 11/26/2013 03:57 AM, Will Newton wrote: >> >> Add support for handling the R_AARCH64_IRELATIVE relocation and >> STT_GNU_IFUNC symbols to the aarch64 port. > > How did you test this? Running the glibc testsuite - which is actually a pretty good test for the ifunc code. There's no functionality in the aarch64 port yet that actually uses this yet so I couldn't test that. The only screw up that the testsuite does not find is failing to pass hwcap correctly to the resover, but I pretty intensively stared at that code and seen as everything goes through elf_ifunc_invoke I am happy that it should work in practice.
On 11/26/2013 10:39 AM, Will Newton wrote: > On 26 November 2013 15:24, Carlos O'Donell <carlos@redhat.com> wrote: >> On 11/26/2013 03:57 AM, Will Newton wrote: >>> >>> Add support for handling the R_AARCH64_IRELATIVE relocation and >>> STT_GNU_IFUNC symbols to the aarch64 port. >> >> How did you test this? > > Running the glibc testsuite - which is actually a pretty good test for > the ifunc code. There's no functionality in the aarch64 port yet that > actually uses this yet so I couldn't test that. The only screw up that > the testsuite does not find is failing to pass hwcap correctly to the > resover, but I pretty intensively stared at that code and seen as > everything goes through elf_ifunc_invoke I am happy that it should > work in practice. Perfect, then I'm happy with the patch. If Marcus is happy then I think you get to check this in. Cheers, Carlos.
diff --git a/ports/sysdeps/aarch64/dl-irel.h b/ports/sysdeps/aarch64/dl-irel.h index 1a3811e..f37ee39 100644 --- a/ports/sysdeps/aarch64/dl-irel.h +++ b/ports/sysdeps/aarch64/dl-irel.h @@ -22,15 +22,31 @@ #include <stdio.h> #include <unistd.h> +#include <ldsodefs.h> -/* AArch64 does not yet implement IFUNC support. However since - 2011-06-20 provision of a elf_ifunc_invoke has been mandatory. */ +#define ELF_MACHINE_IRELA 1 static inline ElfW(Addr) __attribute ((always_inline)) elf_ifunc_invoke (ElfW(Addr) addr) { - return ((ElfW(Addr) (*) (void)) (addr)) (); + return ((ElfW(Addr) (*) (unsigned long int)) (addr)) (GLRO(dl_hwcap)); +} + +static inline void +__attribute ((always_inline)) +elf_irela (const ElfW(Rela) *reloc) +{ + ElfW(Addr) *const reloc_addr = (void *) reloc->r_offset; + const unsigned long int r_type = ELFW(R_TYPE) (reloc->r_info); + + if (__glibc_likely (r_type == R_AARCH64_IRELATIVE)) + { + ElfW(Addr) value = elf_ifunc_invoke (reloc->r_addend); + *reloc_addr = value; + } + else + __libc_fatal ("unexpected reloc type in static binary"); } #endif diff --git a/ports/sysdeps/aarch64/dl-machine.h b/ports/sysdeps/aarch64/dl-machine.h index 71dd6b3..01a214f 100644 --- a/ports/sysdeps/aarch64/dl-machine.h +++ b/ports/sysdeps/aarch64/dl-machine.h @@ -23,6 +23,7 @@ #include <tls.h> #include <dl-tlsdesc.h> +#include <dl-irel.h> /* Return nonzero iff ELF header is compatible with the running host. */ static inline int __attribute__ ((unused)) @@ -243,6 +244,12 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc, struct link_map *sym_map = RESOLVE_MAP (&sym, version, r_type); ElfW(Addr) value = sym_map == NULL ? 0 : sym_map->l_addr + sym->st_value; + if (sym != NULL + && __glibc_unlikely (ELFW(ST_TYPE) (sym->st_info) == STT_GNU_IFUNC) + && __glibc_likely (sym->st_shndx != SHN_UNDEF) + && __glibc_likely (!skip_ifunc)) + value = elf_ifunc_invoke (value); + switch (r_type) { case R_AARCH64_COPY: @@ -331,6 +338,12 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc, } break; + case R_AARCH64_IRELATIVE: + value = map->l_addr + reloc->r_addend; + value = elf_ifunc_invoke (value); + *reloc_addr = value; + break; + default: _dl_reloc_bad_type (map, r_type, 0); break; @@ -374,6 +387,13 @@ elf_machine_lazy_rel (struct link_map *map, td->entry = (void*)(D_PTR (map, l_info[ADDRIDX (DT_TLSDESC_PLT)]) + map->l_addr); } + else if (__glibc_unlikely (r_type == R_AARCH64_IRELATIVE)) + { + ElfW(Addr) value = map->l_addr + reloc->r_addend; + if (__glibc_likely (!skip_ifunc)) + value = elf_ifunc_invoke (value); + *reloc_addr = value; + } else _dl_reloc_bad_type (map, r_type, 1); }