Message ID | 20220118043159.27521-3-vincent.chen@sifive.com |
---|---|
State | New |
Headers | show |
Series | RISC-V: Add vector ISA support | expand |
Context | Check | Description |
---|---|---|
dj/TryBot-apply_patch | success | Patch applied to master at the time it was sent |
dj/TryBot-32bit | success | Build for i686 |
Sorry, I missed the fixed-up patch set (which is why I just sent out a similar bit of documentation). On Mon, 17 Jan 2022 20:31:59 PST (-0800), vincent.chen@sifive.com wrote: > From: Hsiangkai Wang <kai.wang@sifive.com> > > In some cases, we do not want to go through the resolver for function > calls. For example, functions with vector arguments will use vector > registers to pass arguments. In the resolver, we do not save/restore the > vector argument registers for lazy binding efficiency. To avoid ruining > the vector arguments, functions with vector arguments will not go > through the resolver. > > To achieve the goal, we will annotate the function symbols with > STO_RISCV_VARIANT_CC flag and add DT_RISCV_VARIANT_CC tag in the dynamic > section. In the first pass on PLT relocations, we do not set up to call > _dl_runtime_resolve. Instead, we resolve the functions directly. > > Signed-off-by: Hsiangkai Wang <kai.wang@sifive.com> > Signed-off-by: Vincent Chen <vincent.chen@sifive.com> > --- > elf/elf.h | 7 +++++++ > manual/platform.texi | 6 ++++++ > sysdeps/riscv/dl-dtprocnum.h | 21 +++++++++++++++++++++ > sysdeps/riscv/dl-machine.h | 26 ++++++++++++++++++++++++++ > 4 files changed, 60 insertions(+) > create mode 100644 sysdeps/riscv/dl-dtprocnum.h > > diff --git a/elf/elf.h b/elf/elf.h > index 0735f6b579..9c95544050 100644 > --- a/elf/elf.h > +++ b/elf/elf.h > @@ -3911,6 +3911,13 @@ enum > > #define R_TILEGX_NUM 130 > > +/* RISC-V specific values for the Dyn d_tag field. */ > +#define DT_RISCV_VARIANT_CC (DT_LOPROC + 1) > +#define DT_RISCV_NUM 2 > + > +/* RISC-V specific values for the st_other field. */ > +#define STO_RISCV_VARIANT_CC 0x80 > + > /* RISC-V ELF Flags */ > #define EF_RISCV_RVC 0x0001 > #define EF_RISCV_FLOAT_ABI 0x0006 > diff --git a/manual/platform.texi b/manual/platform.texi > index d5fdc5bd05..a1a740f381 100644 > --- a/manual/platform.texi > +++ b/manual/platform.texi > @@ -121,6 +121,12 @@ when it is not allowed, the priority is set to medium. > @node RISC-V > @appendixsec RISC-V-specific Facilities > > +Functions that are lazily bound must be compatible with the standard calling > +convention. When a function is annotated with STO_RISCV_VARIANT_CC, it means > +this function is not compatible with the standard calling convention. The > +dynamic linker will directly resolve it instead of using the lazy binding > +mechanism. IMO this is the wrong way to go: we're essentially re-defining a bit used be the standard ABI to mean something else. I guess we've already defacto forked from the psABI with that "standard calling convention" language, but IMO it'd be prudent to use a different bit to represent this new behavior. In the long term one could imagine trying to get back in line with the psABI, but if we're repurposing two bit patterns it'll be a bit harder than if we're just repurposing one. > + > Cache management facilities specific to RISC-V systems that implement the Linux > ABI are declared in @file{sys/cachectl.h}. > > diff --git a/sysdeps/riscv/dl-dtprocnum.h b/sysdeps/riscv/dl-dtprocnum.h > new file mode 100644 > index 0000000000..f189fd700a > --- /dev/null > +++ b/sysdeps/riscv/dl-dtprocnum.h > @@ -0,0 +1,21 @@ > +/* Configuration of lookup functions. RISC-V version. > + Copyright (C) 2019-2021 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library. If not, see > + <https://www.gnu.org/licenses/>. */ > + > +/* Number of extra dynamic section entries for this architecture. By > + default there are none. */ > +#define DT_THISPROCNUM DT_RISCV_NUM > diff --git a/sysdeps/riscv/dl-machine.h b/sysdeps/riscv/dl-machine.h > index 1d3e2e588c..cdbaca6533 100644 > --- a/sysdeps/riscv/dl-machine.h > +++ b/sysdeps/riscv/dl-machine.h > @@ -53,6 +53,9 @@ > || (__WORDSIZE == 64 && (type) == R_RISCV_TLS_TPREL64))) \ > | (ELF_RTYPE_CLASS_COPY * ((type) == R_RISCV_COPY))) > > +//* Translate a processor specific dynamic tag to the index in l_info array. */ > +#define DT_RISCV(x) (DT_RISCV_##x - DT_LOPROC + DT_NUM) > + > /* Return nonzero iff ELF header is compatible with the running host. */ > static inline int __attribute_used__ > elf_machine_matches_host (const ElfW(Ehdr) *ehdr) > @@ -305,6 +308,29 @@ elf_machine_lazy_rel (struct link_map *map, struct r_scope_elem *scope[], > /* Check for unexpected PLT reloc type. */ > if (__glibc_likely (r_type == R_RISCV_JUMP_SLOT)) > { > + if (__glibc_unlikely (map->l_info[DT_RISCV (VARIANT_CC)] != NULL)) > + { > + /* Check the symbol table for variant CC symbols. */ > + const Elf_Symndx symndx = ELFW(R_SYM) (reloc->r_info); > + const ElfW(Sym) *symtab = > + (const void *)D_PTR (map, l_info[DT_SYMTAB]); > + const ElfW(Sym) *sym = &symtab[symndx]; > + if (__glibc_unlikely (sym->st_other & STO_RISCV_VARIANT_CC)) > + { > + /* Avoid lazy resolution of variant CC symbols. */ > + const struct r_found_version *version = NULL; > + if (map->l_info[VERSYMIDX (DT_VERSYM)] != NULL) > + { > + const ElfW(Half) *vernum = > + (const void *)D_PTR (map, l_info[VERSYMIDX (DT_VERSYM)]); > + version = &map->l_versions[vernum[symndx] & 0x7fff]; > + } > + elf_machine_rela (map, scope, reloc, sym, version, reloc_addr, > + skip_ifunc); > + return; > + } > + } > + > if (__glibc_unlikely (map->l_mach.plt == 0)) > { > if (l_addr) Aside from that this one looks fine to me. Given the complexity around this psABI spec deviation and how close we are to release I'd prefer to wait and see if we can come up with a better solution, though -- for example, I'd been kicking around some ideas related to ELF object attributes saying "this follows the psABI-1.0" vs "this follows the legacy GNU psABI extensions". That way we could at least tag binaries that explicitly rely on this new behavior as such, which would give us a shot at eventually getting rid of them.
On Wed, Jan 19, 2022 at 6:22 PM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > Sorry, I missed the fixed-up patch set (which is why I just sent out a > similar bit of documentation). > > On Mon, 17 Jan 2022 20:31:59 PST (-0800), vincent.chen@sifive.com wrote: > > From: Hsiangkai Wang <kai.wang@sifive.com> > > > > In some cases, we do not want to go through the resolver for function > > calls. For example, functions with vector arguments will use vector > > registers to pass arguments. In the resolver, we do not save/restore the > > vector argument registers for lazy binding efficiency. To avoid ruining > > the vector arguments, functions with vector arguments will not go > > through the resolver. > > > > To achieve the goal, we will annotate the function symbols with > > STO_RISCV_VARIANT_CC flag and add DT_RISCV_VARIANT_CC tag in the dynamic > > section. In the first pass on PLT relocations, we do not set up to call > > _dl_runtime_resolve. Instead, we resolve the functions directly. > > > > Signed-off-by: Hsiangkai Wang <kai.wang@sifive.com> > > Signed-off-by: Vincent Chen <vincent.chen@sifive.com> > > --- > > elf/elf.h | 7 +++++++ > > manual/platform.texi | 6 ++++++ > > sysdeps/riscv/dl-dtprocnum.h | 21 +++++++++++++++++++++ > > sysdeps/riscv/dl-machine.h | 26 ++++++++++++++++++++++++++ > > 4 files changed, 60 insertions(+) > > create mode 100644 sysdeps/riscv/dl-dtprocnum.h > > > > diff --git a/elf/elf.h b/elf/elf.h > > index 0735f6b579..9c95544050 100644 > > --- a/elf/elf.h > > +++ b/elf/elf.h > > @@ -3911,6 +3911,13 @@ enum > > > > #define R_TILEGX_NUM 130 > > > > +/* RISC-V specific values for the Dyn d_tag field. */ > > +#define DT_RISCV_VARIANT_CC (DT_LOPROC + 1) > > +#define DT_RISCV_NUM 2 > > + > > +/* RISC-V specific values for the st_other field. */ > > +#define STO_RISCV_VARIANT_CC 0x80 > > + > > /* RISC-V ELF Flags */ > > #define EF_RISCV_RVC 0x0001 > > #define EF_RISCV_FLOAT_ABI 0x0006 > > diff --git a/manual/platform.texi b/manual/platform.texi > > index d5fdc5bd05..a1a740f381 100644 > > --- a/manual/platform.texi > > +++ b/manual/platform.texi > > @@ -121,6 +121,12 @@ when it is not allowed, the priority is set to medium. > > @node RISC-V > > @appendixsec RISC-V-specific Facilities > > > > +Functions that are lazily bound must be compatible with the standard calling > > +convention. When a function is annotated with STO_RISCV_VARIANT_CC, it means > > +this function is not compatible with the standard calling convention. The > > +dynamic linker will directly resolve it instead of using the lazy binding > > +mechanism. > > IMO this is the wrong way to go: we're essentially re-defining a bit > used be the standard ABI to mean something else. I guess we've already > defacto forked from the psABI with that "standard calling convention" > language, but IMO it'd be prudent to use a different bit to represent > this new behavior. In the long term one could imagine trying to get > back in line with the psABI, but if we're repurposing two bit patterns > it'll be a bit harder than if we're just repurposing one. > > > + > > Cache management facilities specific to RISC-V systems that implement the Linux > > ABI are declared in @file{sys/cachectl.h}. > > > > diff --git a/sysdeps/riscv/dl-dtprocnum.h b/sysdeps/riscv/dl-dtprocnum.h > > new file mode 100644 > > index 0000000000..f189fd700a > > --- /dev/null > > +++ b/sysdeps/riscv/dl-dtprocnum.h > > @@ -0,0 +1,21 @@ > > +/* Configuration of lookup functions. RISC-V version. > > + Copyright (C) 2019-2021 Free Software Foundation, Inc. > > + This file is part of the GNU C Library. > > + > > + The GNU C Library is free software; you can redistribute it and/or > > + modify it under the terms of the GNU Lesser General Public > > + License as published by the Free Software Foundation; either > > + version 2.1 of the License, or (at your option) any later version. > > + > > + The GNU C Library is distributed in the hope that it will be useful, > > + but WITHOUT ANY WARRANTY; without even the implied warranty of > > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > + Lesser General Public License for more details. > > + > > + You should have received a copy of the GNU Lesser General Public > > + License along with the GNU C Library. If not, see > > + <https://www.gnu.org/licenses/>. */ > > + > > +/* Number of extra dynamic section entries for this architecture. By > > + default there are none. */ > > +#define DT_THISPROCNUM DT_RISCV_NUM > > diff --git a/sysdeps/riscv/dl-machine.h b/sysdeps/riscv/dl-machine.h > > index 1d3e2e588c..cdbaca6533 100644 > > --- a/sysdeps/riscv/dl-machine.h > > +++ b/sysdeps/riscv/dl-machine.h > > @@ -53,6 +53,9 @@ > > || (__WORDSIZE == 64 && (type) == R_RISCV_TLS_TPREL64))) \ > > | (ELF_RTYPE_CLASS_COPY * ((type) == R_RISCV_COPY))) > > > > +//* Translate a processor specific dynamic tag to the index in l_info array. */ > > +#define DT_RISCV(x) (DT_RISCV_##x - DT_LOPROC + DT_NUM) > > + > > /* Return nonzero iff ELF header is compatible with the running host. */ > > static inline int __attribute_used__ > > elf_machine_matches_host (const ElfW(Ehdr) *ehdr) > > @@ -305,6 +308,29 @@ elf_machine_lazy_rel (struct link_map *map, struct r_scope_elem *scope[], > > /* Check for unexpected PLT reloc type. */ > > if (__glibc_likely (r_type == R_RISCV_JUMP_SLOT)) > > { > > + if (__glibc_unlikely (map->l_info[DT_RISCV (VARIANT_CC)] != NULL)) > > + { > > + /* Check the symbol table for variant CC symbols. */ > > + const Elf_Symndx symndx = ELFW(R_SYM) (reloc->r_info); > > + const ElfW(Sym) *symtab = > > + (const void *)D_PTR (map, l_info[DT_SYMTAB]); > > + const ElfW(Sym) *sym = &symtab[symndx]; > > + if (__glibc_unlikely (sym->st_other & STO_RISCV_VARIANT_CC)) > > + { > > + /* Avoid lazy resolution of variant CC symbols. */ > > + const struct r_found_version *version = NULL; > > + if (map->l_info[VERSYMIDX (DT_VERSYM)] != NULL) > > + { > > + const ElfW(Half) *vernum = > > + (const void *)D_PTR (map, l_info[VERSYMIDX (DT_VERSYM)]); > > + version = &map->l_versions[vernum[symndx] & 0x7fff]; > > + } > > + elf_machine_rela (map, scope, reloc, sym, version, reloc_addr, > > + skip_ifunc); > > + return; > > + } > > + } > > + > > if (__glibc_unlikely (map->l_mach.plt == 0)) > > { > > if (l_addr) > > Aside from that this one looks fine to me. > > Given the complexity around this psABI spec deviation and how close we > are to release I'd prefer to wait and see if we can come up with a > better solution, though -- for example, I'd been kicking around some > ideas related to ELF object attributes saying "this follows the > psABI-1.0" vs "this follows the legacy GNU psABI extensions". That way > we could at least tag binaries that explicitly rely on this new behavior > as such, which would give us a shot at eventually getting rid of them. If you want to go this route, I suggest you use GNU property for this. ld.so supports GNU property.
On Wed, 19 Jan 2022 18:38:35 PST (-0800), H.J. Lu wrote: > On Wed, Jan 19, 2022 at 6:22 PM Palmer Dabbelt <palmer@dabbelt.com> wrote: [snip] >> Given the complexity around this psABI spec deviation and how close we >> are to release I'd prefer to wait and see if we can come up with a >> better solution, though -- for example, I'd been kicking around some >> ideas related to ELF object attributes saying "this follows the >> psABI-1.0" vs "this follows the legacy GNU psABI extensions". That way >> we could at least tag binaries that explicitly rely on this new behavior >> as such, which would give us a shot at eventually getting rid of them. > > If you want to go this route, I suggest you use GNU property for this. > ld.so supports GNU property. Makes sense. I'd been thinking of essentially just defining one bit for each of these incompatibilites, with an absence of them meaning the legacy behavior and then a true/false meaning that users have explicitly opted into to the spec'd or legacy behavior. I hadn't gotten as far as actually figuring out where to put those bits -- I got hung up on the pc-relative vs position-independent one, and that's clearly for after this round of releases so I kind of just put it on the backburner. Given that pretty much all of these are going to need to drive runtime behavior, though, it sounds like a GNU property is a reasonable way to go. There's going to be a lot of edge cases here, though, so happy to hear if anyone has ideas ;) Thanks!
On Thu, Jan 20, 2022 at 10:21 AM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > Sorry, I missed the fixed-up patch set (which is why I just sent out a > similar bit of documentation). > > On Mon, 17 Jan 2022 20:31:59 PST (-0800), vincent.chen@sifive.com wrote: > > From: Hsiangkai Wang <kai.wang@sifive.com> > > > > In some cases, we do not want to go through the resolver for function > > calls. For example, functions with vector arguments will use vector > > registers to pass arguments. In the resolver, we do not save/restore the > > vector argument registers for lazy binding efficiency. To avoid ruining > > the vector arguments, functions with vector arguments will not go > > through the resolver. > > > > To achieve the goal, we will annotate the function symbols with > > STO_RISCV_VARIANT_CC flag and add DT_RISCV_VARIANT_CC tag in the dynamic > > section. In the first pass on PLT relocations, we do not set up to call > > _dl_runtime_resolve. Instead, we resolve the functions directly. > > > > Signed-off-by: Hsiangkai Wang <kai.wang@sifive.com> > > Signed-off-by: Vincent Chen <vincent.chen@sifive.com> > > --- > > elf/elf.h | 7 +++++++ > > manual/platform.texi | 6 ++++++ > > sysdeps/riscv/dl-dtprocnum.h | 21 +++++++++++++++++++++ > > sysdeps/riscv/dl-machine.h | 26 ++++++++++++++++++++++++++ > > 4 files changed, 60 insertions(+) > > create mode 100644 sysdeps/riscv/dl-dtprocnum.h > > > > diff --git a/elf/elf.h b/elf/elf.h > > index 0735f6b579..9c95544050 100644 > > --- a/elf/elf.h > > +++ b/elf/elf.h > > @@ -3911,6 +3911,13 @@ enum > > > > #define R_TILEGX_NUM 130 > > > > +/* RISC-V specific values for the Dyn d_tag field. */ > > +#define DT_RISCV_VARIANT_CC (DT_LOPROC + 1) > > +#define DT_RISCV_NUM 2 > > + > > +/* RISC-V specific values for the st_other field. */ > > +#define STO_RISCV_VARIANT_CC 0x80 > > + > > /* RISC-V ELF Flags */ > > #define EF_RISCV_RVC 0x0001 > > #define EF_RISCV_FLOAT_ABI 0x0006 > > diff --git a/manual/platform.texi b/manual/platform.texi > > index d5fdc5bd05..a1a740f381 100644 > > --- a/manual/platform.texi > > +++ b/manual/platform.texi > > @@ -121,6 +121,12 @@ when it is not allowed, the priority is set to medium. > > @node RISC-V > > @appendixsec RISC-V-specific Facilities > > > > +Functions that are lazily bound must be compatible with the standard calling > > +convention. When a function is annotated with STO_RISCV_VARIANT_CC, it means > > +this function is not compatible with the standard calling convention. The > > +dynamic linker will directly resolve it instead of using the lazy binding > > +mechanism. > > IMO this is the wrong way to go: we're essentially re-defining a bit > used be the standard ABI to mean something else. I guess we've already > defacto forked from the psABI with that "standard calling convention" > language, but IMO it'd be prudent to use a different bit to represent > this new behavior. In the long term one could imagine trying to get > back in line with the psABI, but if we're repurposing two bit patterns > it'll be a bit harder than if we're just repurposing one. > OK, I understand. I reviewed the psABI spec again and did some modifications. Did you think is it better? Functions that are lazily bound must be compatible with the standard calling convention. Any functions that use additional argument registers must be annotated with STO_RISCV_VARIANT_CC. To prevent these additional argument registers from being corrupted during the lazy binding process, this patch makes such functions be always resolved at load time, not lazily. > > + > > Cache management facilities specific to RISC-V systems that implement the Linux > > ABI are declared in @file{sys/cachectl.h}. > > > > diff --git a/sysdeps/riscv/dl-dtprocnum.h b/sysdeps/riscv/dl-dtprocnum.h > > new file mode 100644 > > index 0000000000..f189fd700a > > --- /dev/null > > +++ b/sysdeps/riscv/dl-dtprocnum.h > > @@ -0,0 +1,21 @@ > > +/* Configuration of lookup functions. RISC-V version. > > + Copyright (C) 2019-2021 Free Software Foundation, Inc. > > + This file is part of the GNU C Library. > > + > > + The GNU C Library is free software; you can redistribute it and/or > > + modify it under the terms of the GNU Lesser General Public > > + License as published by the Free Software Foundation; either > > + version 2.1 of the License, or (at your option) any later version. > > + > > + The GNU C Library is distributed in the hope that it will be useful, > > + but WITHOUT ANY WARRANTY; without even the implied warranty of > > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > + Lesser General Public License for more details. > > + > > + You should have received a copy of the GNU Lesser General Public > > + License along with the GNU C Library. If not, see > > + <https://www.gnu.org/licenses/>. */ > > + > > +/* Number of extra dynamic section entries for this architecture. By > > + default there are none. */ > > +#define DT_THISPROCNUM DT_RISCV_NUM > > diff --git a/sysdeps/riscv/dl-machine.h b/sysdeps/riscv/dl-machine.h > > index 1d3e2e588c..cdbaca6533 100644 > > --- a/sysdeps/riscv/dl-machine.h > > +++ b/sysdeps/riscv/dl-machine.h > > @@ -53,6 +53,9 @@ > > || (__WORDSIZE == 64 && (type) == R_RISCV_TLS_TPREL64))) \ > > | (ELF_RTYPE_CLASS_COPY * ((type) == R_RISCV_COPY))) > > > > +//* Translate a processor specific dynamic tag to the index in l_info array. */ > > +#define DT_RISCV(x) (DT_RISCV_##x - DT_LOPROC + DT_NUM) > > + > > /* Return nonzero iff ELF header is compatible with the running host. */ > > static inline int __attribute_used__ > > elf_machine_matches_host (const ElfW(Ehdr) *ehdr) > > @@ -305,6 +308,29 @@ elf_machine_lazy_rel (struct link_map *map, struct r_scope_elem *scope[], > > /* Check for unexpected PLT reloc type. */ > > if (__glibc_likely (r_type == R_RISCV_JUMP_SLOT)) > > { > > + if (__glibc_unlikely (map->l_info[DT_RISCV (VARIANT_CC)] != NULL)) > > + { > > + /* Check the symbol table for variant CC symbols. */ > > + const Elf_Symndx symndx = ELFW(R_SYM) (reloc->r_info); > > + const ElfW(Sym) *symtab = > > + (const void *)D_PTR (map, l_info[DT_SYMTAB]); > > + const ElfW(Sym) *sym = &symtab[symndx]; > > + if (__glibc_unlikely (sym->st_other & STO_RISCV_VARIANT_CC)) > > + { > > + /* Avoid lazy resolution of variant CC symbols. */ > > + const struct r_found_version *version = NULL; > > + if (map->l_info[VERSYMIDX (DT_VERSYM)] != NULL) > > + { > > + const ElfW(Half) *vernum = > > + (const void *)D_PTR (map, l_info[VERSYMIDX (DT_VERSYM)]); > > + version = &map->l_versions[vernum[symndx] & 0x7fff]; > > + } > > + elf_machine_rela (map, scope, reloc, sym, version, reloc_addr, > > + skip_ifunc); > > + return; > > + } > > + } > > + > > if (__glibc_unlikely (map->l_mach.plt == 0)) > > { > > if (l_addr) > > Aside from that this one looks fine to me. > > Given the complexity around this psABI spec deviation and how close we > are to release I'd prefer to wait and see if we can come up with a > better solution, though -- for example, I'd been kicking around some > ideas related to ELF object attributes saying "this follows the > psABI-1.0" vs "this follows the legacy GNU psABI extensions". That way > we could at least tag binaries that explicitly rely on this new behavior > as such, which would give us a shot at eventually getting rid of them. I agree that we don't need to rush to come up with a solution in this release. But, I have a little confused. Even if the ELF object attribute is able to say "this follows the psABI-1.0" vs "this follows the legacy GNU psABI extensions", we still need to use STO_RISCV_VARIANT_CC to tell ld.so whether needs to directly resolve this symbol. Is it correct?
On Thu, 20 Jan 2022 17:43:14 PST (-0800), vincent.chen@sifive.com wrote: > On Thu, Jan 20, 2022 at 10:21 AM Palmer Dabbelt <palmer@dabbelt.com> wrote: >> >> Sorry, I missed the fixed-up patch set (which is why I just sent out a >> similar bit of documentation). >> >> On Mon, 17 Jan 2022 20:31:59 PST (-0800), vincent.chen@sifive.com wrote: >> > From: Hsiangkai Wang <kai.wang@sifive.com> >> > >> > In some cases, we do not want to go through the resolver for function >> > calls. For example, functions with vector arguments will use vector >> > registers to pass arguments. In the resolver, we do not save/restore the >> > vector argument registers for lazy binding efficiency. To avoid ruining >> > the vector arguments, functions with vector arguments will not go >> > through the resolver. >> > >> > To achieve the goal, we will annotate the function symbols with >> > STO_RISCV_VARIANT_CC flag and add DT_RISCV_VARIANT_CC tag in the dynamic >> > section. In the first pass on PLT relocations, we do not set up to call >> > _dl_runtime_resolve. Instead, we resolve the functions directly. >> > >> > Signed-off-by: Hsiangkai Wang <kai.wang@sifive.com> >> > Signed-off-by: Vincent Chen <vincent.chen@sifive.com> >> > --- >> > elf/elf.h | 7 +++++++ >> > manual/platform.texi | 6 ++++++ >> > sysdeps/riscv/dl-dtprocnum.h | 21 +++++++++++++++++++++ >> > sysdeps/riscv/dl-machine.h | 26 ++++++++++++++++++++++++++ >> > 4 files changed, 60 insertions(+) >> > create mode 100644 sysdeps/riscv/dl-dtprocnum.h >> > >> > diff --git a/elf/elf.h b/elf/elf.h >> > index 0735f6b579..9c95544050 100644 >> > --- a/elf/elf.h >> > +++ b/elf/elf.h >> > @@ -3911,6 +3911,13 @@ enum >> > >> > #define R_TILEGX_NUM 130 >> > >> > +/* RISC-V specific values for the Dyn d_tag field. */ >> > +#define DT_RISCV_VARIANT_CC (DT_LOPROC + 1) >> > +#define DT_RISCV_NUM 2 >> > + >> > +/* RISC-V specific values for the st_other field. */ >> > +#define STO_RISCV_VARIANT_CC 0x80 >> > + >> > /* RISC-V ELF Flags */ >> > #define EF_RISCV_RVC 0x0001 >> > #define EF_RISCV_FLOAT_ABI 0x0006 >> > diff --git a/manual/platform.texi b/manual/platform.texi >> > index d5fdc5bd05..a1a740f381 100644 >> > --- a/manual/platform.texi >> > +++ b/manual/platform.texi >> > @@ -121,6 +121,12 @@ when it is not allowed, the priority is set to medium. >> > @node RISC-V >> > @appendixsec RISC-V-specific Facilities >> > >> > +Functions that are lazily bound must be compatible with the standard calling >> > +convention. When a function is annotated with STO_RISCV_VARIANT_CC, it means >> > +this function is not compatible with the standard calling convention. The >> > +dynamic linker will directly resolve it instead of using the lazy binding >> > +mechanism. >> >> IMO this is the wrong way to go: we're essentially re-defining a bit >> used be the standard ABI to mean something else. I guess we've already >> defacto forked from the psABI with that "standard calling convention" >> language, but IMO it'd be prudent to use a different bit to represent >> this new behavior. In the long term one could imagine trying to get >> back in line with the psABI, but if we're repurposing two bit patterns >> it'll be a bit harder than if we're just repurposing one. >> > OK, I understand. I reviewed the psABI spec again and did some > modifications. Did you think is it better? > > Functions that are lazily bound must be compatible with the standard > calling convention. Any functions that use additional argument > registers must be annotated with STO_RISCV_VARIANT_CC. To prevent > these additional argument registers from being corrupted during the > lazy binding process, this patch makes such functions be always > resolved at load time, not lazily. I was trying to suggest using a different bit (with a different name) for the "does not follow the standard calling convention" behavior, rather than re-defining the bit allocated for STO_RISCV_VARIANT_CC in the psABI for that behavior. Maybe it just doesn't matter, given that we're forking, but re-using the same bit will just make things more confusing for everyone in the future. Aside from that the original text looked OK. >> > + >> > Cache management facilities specific to RISC-V systems that implement the Linux >> > ABI are declared in @file{sys/cachectl.h}. >> > >> > diff --git a/sysdeps/riscv/dl-dtprocnum.h b/sysdeps/riscv/dl-dtprocnum.h >> > new file mode 100644 >> > index 0000000000..f189fd700a >> > --- /dev/null >> > +++ b/sysdeps/riscv/dl-dtprocnum.h >> > @@ -0,0 +1,21 @@ >> > +/* Configuration of lookup functions. RISC-V version. >> > + Copyright (C) 2019-2021 Free Software Foundation, Inc. >> > + This file is part of the GNU C Library. >> > + >> > + The GNU C Library is free software; you can redistribute it and/or >> > + modify it under the terms of the GNU Lesser General Public >> > + License as published by the Free Software Foundation; either >> > + version 2.1 of the License, or (at your option) any later version. >> > + >> > + The GNU C Library is distributed in the hope that it will be useful, >> > + but WITHOUT ANY WARRANTY; without even the implied warranty of >> > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> > + Lesser General Public License for more details. >> > + >> > + You should have received a copy of the GNU Lesser General Public >> > + License along with the GNU C Library. If not, see >> > + <https://www.gnu.org/licenses/>. */ >> > + >> > +/* Number of extra dynamic section entries for this architecture. By >> > + default there are none. */ >> > +#define DT_THISPROCNUM DT_RISCV_NUM >> > diff --git a/sysdeps/riscv/dl-machine.h b/sysdeps/riscv/dl-machine.h >> > index 1d3e2e588c..cdbaca6533 100644 >> > --- a/sysdeps/riscv/dl-machine.h >> > +++ b/sysdeps/riscv/dl-machine.h >> > @@ -53,6 +53,9 @@ >> > || (__WORDSIZE == 64 && (type) == R_RISCV_TLS_TPREL64))) \ >> > | (ELF_RTYPE_CLASS_COPY * ((type) == R_RISCV_COPY))) >> > >> > +//* Translate a processor specific dynamic tag to the index in l_info array. */ >> > +#define DT_RISCV(x) (DT_RISCV_##x - DT_LOPROC + DT_NUM) >> > + >> > /* Return nonzero iff ELF header is compatible with the running host. */ >> > static inline int __attribute_used__ >> > elf_machine_matches_host (const ElfW(Ehdr) *ehdr) >> > @@ -305,6 +308,29 @@ elf_machine_lazy_rel (struct link_map *map, struct r_scope_elem *scope[], >> > /* Check for unexpected PLT reloc type. */ >> > if (__glibc_likely (r_type == R_RISCV_JUMP_SLOT)) >> > { >> > + if (__glibc_unlikely (map->l_info[DT_RISCV (VARIANT_CC)] != NULL)) >> > + { >> > + /* Check the symbol table for variant CC symbols. */ >> > + const Elf_Symndx symndx = ELFW(R_SYM) (reloc->r_info); >> > + const ElfW(Sym) *symtab = >> > + (const void *)D_PTR (map, l_info[DT_SYMTAB]); >> > + const ElfW(Sym) *sym = &symtab[symndx]; >> > + if (__glibc_unlikely (sym->st_other & STO_RISCV_VARIANT_CC)) >> > + { >> > + /* Avoid lazy resolution of variant CC symbols. */ >> > + const struct r_found_version *version = NULL; >> > + if (map->l_info[VERSYMIDX (DT_VERSYM)] != NULL) >> > + { >> > + const ElfW(Half) *vernum = >> > + (const void *)D_PTR (map, l_info[VERSYMIDX (DT_VERSYM)]); >> > + version = &map->l_versions[vernum[symndx] & 0x7fff]; >> > + } >> > + elf_machine_rela (map, scope, reloc, sym, version, reloc_addr, >> > + skip_ifunc); >> > + return; >> > + } >> > + } >> > + >> > if (__glibc_unlikely (map->l_mach.plt == 0)) >> > { >> > if (l_addr) >> >> Aside from that this one looks fine to me. >> >> Given the complexity around this psABI spec deviation and how close we >> are to release I'd prefer to wait and see if we can come up with a >> better solution, though -- for example, I'd been kicking around some >> ideas related to ELF object attributes saying "this follows the >> psABI-1.0" vs "this follows the legacy GNU psABI extensions". That way >> we could at least tag binaries that explicitly rely on this new behavior >> as such, which would give us a shot at eventually getting rid of them. > > I agree that we don't need to rush to come up with a solution in this > release. But, I have a little confused. Even if the ELF object > attribute is able to say "this follows the psABI-1.0" vs "this follows > the legacy GNU psABI extensions", we still need to use > STO_RISCV_VARIANT_CC to tell ld.so whether needs to directly resolve > this symbol. Is it correct? We need to directly resolve all symbols compatible with psABI-1.0, lazy binding will only be legal for symbols that follow the legacy GNU extensions.
diff --git a/elf/elf.h b/elf/elf.h index 0735f6b579..9c95544050 100644 --- a/elf/elf.h +++ b/elf/elf.h @@ -3911,6 +3911,13 @@ enum #define R_TILEGX_NUM 130 +/* RISC-V specific values for the Dyn d_tag field. */ +#define DT_RISCV_VARIANT_CC (DT_LOPROC + 1) +#define DT_RISCV_NUM 2 + +/* RISC-V specific values for the st_other field. */ +#define STO_RISCV_VARIANT_CC 0x80 + /* RISC-V ELF Flags */ #define EF_RISCV_RVC 0x0001 #define EF_RISCV_FLOAT_ABI 0x0006 diff --git a/manual/platform.texi b/manual/platform.texi index d5fdc5bd05..a1a740f381 100644 --- a/manual/platform.texi +++ b/manual/platform.texi @@ -121,6 +121,12 @@ when it is not allowed, the priority is set to medium. @node RISC-V @appendixsec RISC-V-specific Facilities +Functions that are lazily bound must be compatible with the standard calling +convention. When a function is annotated with STO_RISCV_VARIANT_CC, it means +this function is not compatible with the standard calling convention. The +dynamic linker will directly resolve it instead of using the lazy binding +mechanism. + Cache management facilities specific to RISC-V systems that implement the Linux ABI are declared in @file{sys/cachectl.h}. diff --git a/sysdeps/riscv/dl-dtprocnum.h b/sysdeps/riscv/dl-dtprocnum.h new file mode 100644 index 0000000000..f189fd700a --- /dev/null +++ b/sysdeps/riscv/dl-dtprocnum.h @@ -0,0 +1,21 @@ +/* Configuration of lookup functions. RISC-V version. + Copyright (C) 2019-2021 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library. If not, see + <https://www.gnu.org/licenses/>. */ + +/* Number of extra dynamic section entries for this architecture. By + default there are none. */ +#define DT_THISPROCNUM DT_RISCV_NUM diff --git a/sysdeps/riscv/dl-machine.h b/sysdeps/riscv/dl-machine.h index 1d3e2e588c..cdbaca6533 100644 --- a/sysdeps/riscv/dl-machine.h +++ b/sysdeps/riscv/dl-machine.h @@ -53,6 +53,9 @@ || (__WORDSIZE == 64 && (type) == R_RISCV_TLS_TPREL64))) \ | (ELF_RTYPE_CLASS_COPY * ((type) == R_RISCV_COPY))) +//* Translate a processor specific dynamic tag to the index in l_info array. */ +#define DT_RISCV(x) (DT_RISCV_##x - DT_LOPROC + DT_NUM) + /* Return nonzero iff ELF header is compatible with the running host. */ static inline int __attribute_used__ elf_machine_matches_host (const ElfW(Ehdr) *ehdr) @@ -305,6 +308,29 @@ elf_machine_lazy_rel (struct link_map *map, struct r_scope_elem *scope[], /* Check for unexpected PLT reloc type. */ if (__glibc_likely (r_type == R_RISCV_JUMP_SLOT)) { + if (__glibc_unlikely (map->l_info[DT_RISCV (VARIANT_CC)] != NULL)) + { + /* Check the symbol table for variant CC symbols. */ + const Elf_Symndx symndx = ELFW(R_SYM) (reloc->r_info); + const ElfW(Sym) *symtab = + (const void *)D_PTR (map, l_info[DT_SYMTAB]); + const ElfW(Sym) *sym = &symtab[symndx]; + if (__glibc_unlikely (sym->st_other & STO_RISCV_VARIANT_CC)) + { + /* Avoid lazy resolution of variant CC symbols. */ + const struct r_found_version *version = NULL; + if (map->l_info[VERSYMIDX (DT_VERSYM)] != NULL) + { + const ElfW(Half) *vernum = + (const void *)D_PTR (map, l_info[VERSYMIDX (DT_VERSYM)]); + version = &map->l_versions[vernum[symndx] & 0x7fff]; + } + elf_machine_rela (map, scope, reloc, sym, version, reloc_addr, + skip_ifunc); + return; + } + } + if (__glibc_unlikely (map->l_mach.plt == 0)) { if (l_addr)