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

[Dave Martin <Dave.Martin@arm.com>]

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-

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.

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