arm64: jump_label: use constraints "Si" instead of "i"

  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(-)
  

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.

> 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
  

On Fri, 2 Feb 2024 at 23:51, Fangrui Song <maskray@google.com> wrote:
>
> 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:)

If it helps clarify things, we might do something like

".quad  (%[key]  - .) + %[bit0]"

: : [key]"Si"(key), [bit0]"i"(branch) :  : l_yes);