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

[Ard Biesheuvel]

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.

> 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.


> I.e., leave the .quad the same and revert to the one-liner
>
> -                :  :  "i"(&((char *)key)[branch]) :  : l_yes);
> +                :  :  "Si"(&((char *)key)[branch]) :  : l_yes);
>
> This remains a bit nasty, but splitting the arguments and adding them
> inside the asm is not really any cleaner.  Changing the way this works
> doesn't seem relevant to what this patch is fixing (and apologies if I
> created confusion here...)
>
> >
> >       return false;
> >  l_yes:
> > @@ -40,9 +44,9 @@ static __always_inline bool arch_static_branch_jump(struct static_key * const ke
> >                "      .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);
>
> Ditto.
>
> [...]
>
> Cheers
> ---Dave