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

[Andrew Waterman <andrew@sifive.com>]

On Thu, Dec 2, 2021 at 1:06 PM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>
> 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?

I think so.

>
> Jess
>