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

[Fangrui Song <maskray@google.com>]

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@arm.com> 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@kernel.org>
>> > > Signed-off-by: Fangrui Song <maskray@google.com>
>> > > 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@google.com/)
>> > >
>> > > * 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:)