Re: [PATCH] arm64: jump_label: use constraints "Si" instead of "i"

[Fangrui Song]

On 2024-02-02, Dave Martin wrote:
> On Fri, Feb 02, 2024 at 05:32:33PM +0100, Ard Biesheuvel wrote:
> > On Fri, 2 Feb 2024 at 16:56, Dave Martin <Dave.Martin@xxxxxxx> wrote:
> > >
> > > On Thu, Feb 01, 2024 at 02:35:45PM -0800, Fangrui Song wrote:
> > > > The generic constraint "i" seems to be copied from x86 or arm (and with
> > > > a redundant generic operand modifier "c"). It works with -fno-PIE but
> > > > not with -fPIE/-fPIC in GCC's aarch64 port.
> > > >
> > > > The machine constraint "S", which denotes a symbol or label reference
> > > > with a constant offset, supports PIC and has been available in GCC since
> > > > 2012 and in Clang since 7.0. However, Clang before 19 does not support
> > > > "S" on a symbol with a constant offset [1] (e.g.
> > > > `static_key_false(&nf_hooks_needed[pf][hook])` in
> > > > include/linux/netfilter.h), so we use "i" as a fallback.
> > > >
> > > > Suggested-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> > > > Signed-off-by: Fangrui Song <maskray@xxxxxxxxxx>
> > > > Link: https://github.com/llvm/llvm-project/pull/80255 [1]
> > > >
> > > > ---
> > > > Changes from
> > > > arm64: jump_label: use constraint "S" instead of "i" (https://lore.kernel.org/all/20240131065322.1126831-1-maskray@xxxxxxxxxx/)
> > > >
> > > > * Use "Si" as Ard suggested to support Clang<19
> > > > * Make branch a separate operand
> > > > ---
> > > >  arch/arm64/include/asm/jump_label.h | 12 ++++++++----
> > > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/include/asm/jump_label.h b/arch/arm64/include/asm/jump_label.h
> > > > index 48ddc0f45d22..1f7d529be608 100644
> > > > --- a/arch/arm64/include/asm/jump_label.h
> > > > +++ b/arch/arm64/include/asm/jump_label.h
> > > > @@ -15,6 +15,10 @@
> > > >
> > > >  #define JUMP_LABEL_NOP_SIZE          AARCH64_INSN_SIZE
> > > >
> > > > +/*
> > > > + * Prefer the constraint "S" to support PIC with GCC. Clang before 19 does not
> > > > + * support "S" on a symbol with a constant offset, so we use "i" as a fallback.
> > > > + */
> > > >  static __always_inline bool arch_static_branch(struct static_key * const key,
> > > >                                              const bool branch)
> > > >  {
> > > > @@ -23,9 +27,9 @@ static __always_inline bool arch_static_branch(struct static_key * const key,
> > > >                "      .pushsection    __jump_table, \"aw\"    \n\t"
> > > >                "      .align          3                       \n\t"
> > > >                "      .long           1b - ., %l[l_yes] - .   \n\t"
> > > > -              "      .quad           %c0 - .                 \n\t"
> > > > +              "      .quad           %0 + %1 - .             \n\t"
> > > >                "      .popsection                             \n\t"
> > > > -              :  :  "i"(&((char *)key)[branch]) :  : l_yes);
> > > > +              :  :  "Si"(key), "i"(branch) :  : l_yes);
> > >
> > > Is the meaning of multi-alternatives well defined if different arguments
> > > specify different numbers of alternatives?  The GCC documentation says:
> > >
> > > https://gcc.gnu.org/onlinedocs/gcc/Multi-Alternative.html:
> > >
> > > -8<-
> > >
> > > [...] All operands for a single instruction must have the same number of
> > > alternatives.
> > >
> > > ->8-
> > >
> >
> > AIUI that does not apply here. That reasons about multiple arguments
> > having more than one constraint, where not all combinations of those
> > constraints are supported by the instruction.
>
> Ah, scratch that: I'm confusing multi-alternatives with simple
> constraints: all arguments must have the same number of comma-separated
> lists of constraint letters, but each list can contain as many or as few
> letters as are needed to match the operand.
>
> So "Si"(), "i"() is fine.

"Si" is fine for GCC and Clang.
"i" is fine for Clang but not for GCC PIC.

    https://maskray.me/blog/2024-01-30-raw-symbol-names-in-inline-assembly#aarch64

    In gcc/config/aarch64, LEGITIMATE_PIC_OPERAND_P(X) disallows any symbol
    reference, which means that "i" and "s" cannot be used for PIC. Instead,
    the constraint "S" has been supported since the initial port (2012) to
    reference a symbol or label.

I am also not familiar with
https://gcc.gnu.org/onlinedocs/gcc/Multi-Alternative.html (comma in a
constraint string). Thankfully we don't need this powerful construct:)

> > > Also, I still think it may be redundant to move the addition inside the
> > > asm, so long as Clang is happy with the symbol having an offset.
> > >
> >
> > Older Clang is not happy with the symbol having an offset.
> >
> > And given that the key pointer and the 'branch' boolean are two
> > distinct inputs to this function, I struggle to understand why you
> > feel it is better to combine them in the argument. 'branch' is used to
> > decide whether or not to set bit 0, independently of the value of key.
> > Using array indexing to combine those values together to avoid an
> > addition in the asm obfuscates the code.
>
> This was more about not making changes that don't need to be made,
> unless it clearly makes the code better.
>
> But if some verions of Clang accept "S" but choke if there's an offset,
> then moving the addition into the asm seems the way to go.
>
> (I may have misread the commit message on this point.)
>
> [...]
>
> Cheers
> ---Dave

I am convinced by Ard' argument that two inputs (key, branch) deserve
two operands.
The existing "i"(&((char *)key)[branch]) is kinda ugly and also longer:)