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

[Palmer Dabbelt <palmer@dabbelt.com>]

[Changing to Jim's new address]

On Mon, 25 Oct 2021 14:08:26 PDT (-0700), jimw@sifive.com wrote:
> On Wed, Aug 11, 2021 at 4:52 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
>> IIUC the ABI patch still isn't merged, which is what makes it actually
>> official.  I'll take this when it lands.
>>
>
> Just following up, the psABI patch was merged in
>     https://github.com/riscv/riscv-elf-psabi-doc/pull/190
> and the psABI has gone into public review for the first official release.
> So we would like this patch from Kai merged in.
>     https://sourceware.org/pipermail/libc-alpha/2021-August/129931.html
> in case you lost track of the original message on this thread.

Sorry this took a while, there's a lot of context here and I figured 
it'd be good to run through that as the issue in play has a pretty 
roundabout effect on this patch.  This is a little long, but I promise 
it gets there at the end:

We had a long-term outstanding issue on the psABI spec
<https://github.com/riscv-non-isa/riscv-elf-psabi-doc/issues/66>, which
essentially boils down to glibc relying on lazily bound functions to follow the
standard calling convention.  The actual issue isn't specific to vector
registers, we're relying on this for X and F registers too, but AFAIK there's
no users of non-standard X/F register calling conventions for lazily bound
functions so we haven't had any user-visible bugs yet.  This all becomes a lot
more important with the recent addition of V registers, which are both more
expensive to save and all clobbered by the standard ABI.

In order to address the desire for non-standard calling conventions 
driven by the standard V register map, the following text was added to 
the psABI spec:

    Run-time linkers that use lazy binding must preserve all argument 
    registers used in the standard calling convention for the ABI in 
    use. Any functions that use additional argument registers must be 
    annotated with STO_RISCV_VARIANT_CC, as defined in Section 8.4.

As a result of adding that language the bug in question about the lazily 
bound functions was resolved.  We're relying on lazily bound functions 
following a lot more of the standard calling convention than that in 
glibc (sp, and ra for example; along with all the temporaries).  The 
text in question doesn't directly address any of those assumptions, and 
given that there's historically been user-visible breakages around these 
ambiguities I think it's prudent to be more explicit about what the 
glibc ABI is here.

As far as I can see, there are really three ways to go about this:

* We can just define the defacto divergence that glibc has always had 
  from the psABI in this area.  That's the smallest change, as IMO 
  that's as simple as

    diff --git a/manual/platform.texi b/manual/platform.texi
    index d5fdc5bd05..9153408966 100644
    --- a/manual/platform.texi
    +++ b/manual/platform.texi
    @@ -121,6 +121,9 @@ 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.
    +
     Cache management facilities specific to RISC-V systems that implement the Linux
     ABI are declared in @file{sys/cachectl.h}.

  to start, adding something like "unless those functions are 
  annotated with STO_RISCV_VARIANT_CC" after merging something very 
  similar to the patch in question (I haven't looked closely at the 
  code, but it seems pretty-straight-forward).
* We can do our best to allow for the additional functions this new text 
  implicitly allows.  I don't see anything we can do about RA and SP, 
  but we could save the registers we clobber in _dl_runtime_resolve 
  (including the F registers on non-F libraries running on F systems, 
  which will be a headache).  We'd still need some additional 
  glibc-specific ABI for the bits of the standard calling convention we 
  can't otherwise handle, but IMO those are pedantic enough that we 
  could just disallow them entirely.
* We can just stop doing any sort of lazy binding.  That would allow us to
  any relying on any unspecified behavior.

The first option is preferable from my end, as the second option would 
require a lot of complexity (both specification and implementation) as 
well as come with a likely performance hit.