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 <[email protected]>

Signed-off-by Fangrui Song <[email protected]>

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/[email protected]/)

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

arch/arm64/include/asm/jump_label.h +4 -0

index 48ddc0f45d22..1f7d529be608 100644
@@ -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)
 {

Quoted 41 lines · Fangrui Song

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 <[email protected]>

Signed-off-by Fangrui Song <[email protected]>

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/[email protected]/)

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

arch/arm64/include/asm/jump_label.h +4 -0

index 48ddc0f45d22..1f7d529be608 100644
@@ -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)
 {

arch/arm64/include/asm/jump_label.h static __always_inline bool arch_static_branch(struct static_key * const key, · line 32

Quoted 8 lines

arch/arm64/include/asm/jump_label.h +2 -2

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

(quoted diff) +1 -1

-                :  :  "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...)

arch/arm64/include/asm/jump_label.h static __always_inline bool arch_static_branch_jump(struct static_key * const ke · line 49

Quoted 12 lines

arch/arm64/include/asm/jump_label.h +2 -2

      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

Quoted 62 lines · Dave Martin

Quoted 41 lines · Fangrui Song

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 <[email protected]>

Signed-off-by Fangrui Song <[email protected]>

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/[email protected]/)

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

arch/arm64/include/asm/jump_label.h +4 -0

index 48ddc0f45d22..1f7d529be608 100644
@@ -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)
 {

arch/arm64/include/asm/jump_label.h static __always_inline bool arch_static_branch(struct static_key * const key, · line 32

Quoted 8 lines

arch/arm64/include/asm/jump_label.h +2 -2

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

Quoted 2 lines

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

Quoted 3 lines

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.

Quoted 63 lines · Ard Biesheuvel

Quoted 62 lines · Dave Martin

Quoted 41 lines · Fangrui Song

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 <[email protected]>

Signed-off-by Fangrui Song <[email protected]>

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/[email protected]/)

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

arch/arm64/include/asm/jump_label.h +4 -0

index 48ddc0f45d22..1f7d529be608 100644
@@ -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)
 {

arch/arm64/include/asm/jump_label.h static __always_inline bool arch_static_branch(struct static_key * const key, · line 32

Quoted 8 lines

arch/arm64/include/asm/jump_label.h +2 -2

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

Quoted 6 lines

Quoted 2 lines

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

Quoted 4 lines

Quoted 3 lines

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.

Quoted 8 lines

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

Quoted 71 lines · Dave Martin

Quoted 63 lines · Ard Biesheuvel

Quoted 62 lines · Dave Martin

Quoted 41 lines · Fangrui Song

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 <[email protected]>

Signed-off-by Fangrui Song <[email protected]>

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/[email protected]/)

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

arch/arm64/include/asm/jump_label.h +4 -0

index 48ddc0f45d22..1f7d529be608 100644
@@ -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)
 {

arch/arm64/include/asm/jump_label.h static __always_inline bool arch_static_branch(struct static_key * const key, · line 32

Quoted 8 lines

arch/arm64/include/asm/jump_label.h +2 -2

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

Quoted 6 lines

Quoted 2 lines

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

Quoted 6 lines

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

Quoted 21 lines

Quoted 4 lines

Quoted 3 lines

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.

Quoted 8 lines

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

Quoted 4 lines

[...]

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

On Fri, 2 Feb 2024 at 23:51, Fangrui Song <[email protected]> wrote:

Quoted 5 lines

"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

Quoted 8 lines

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

Quoted 3 lines

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