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

[Jessica Clarke <jrtc27@jrtc27.com>]

On 2 Dec 2021, at 20:21, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> 
> [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.

Thanks for the detailed review of this corner of the spec, it’s
especially valuable at the moment whilst we’re trying to reach a
ratified version (though all the confusing/inconsistent/ill-defined
terminology is even less appropriate here, at the end of the day the
ABI is what the toolchains and runtimes say it is so it’s really just
about getting the necessary rubber stamps to get the next label and
reviewing the document style and exact language used...).

The intent of the spec is not to make repurposing just ra/sp/gp/tp as
anything other than their ABI-defined meanings (per table 1 of riscv-cc
and, in the case of the stack pointer, the alignment required by the
integer calling convention) legal unless you use STO_RISCV_VARIANT_CC,
so the glibc requirement, which is also true of FreeBSD for exactly the
same reasons, is intended to be what is specified. Upon re-reading what
was written I can see that requirement was lost or forgotten, so I’ll
look at tightening it up, probably changing

  Any functions that use additional argument registers ...

to

  Any functions that use registers in a way that is incompatible with
  the register convention of the ABI in use ...

I also note we currently only talk about the run-time linker preserving
argument registers, and say nothing about preserved registers, nor the
return address. I’m not sure quite why we do the former (maybe I’ve
just forgotten a past conversation), I feel like that’s implied by lazy
binding being an implementation optimisation that must not break the
calling convention, but if we want to keep that language then it should
probably be changed to:

  Run-time linkers that use lazy binding must only clobber registers
  defined as temporary registers for the ABI in use.

Does that all sound sensible, and sufficient, to you?

Jess