Re: [PATCH v2 2/2] riscv: Resolve symbols directly for symbols with STO_RISCV_VARIANT_CC.

[Palmer Dabbelt <palmer@dabbelt.com>]

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.