riscv/atomic: Do proper sign extension also for unsigned in arch_cmpxchg
PATCH 1/1 riscv/atomic: Do proper sign extension also for unsigned in arch_cmpxchg
Sign extend also an unsigned compare value to match what lr.w is doing. Otherwise try_cmpxchg may spuriously return true when used on a u32 value that has the sign bit set, as it happens often in inode_set_ctime_current.
Do this in three conversion steps. The first conversion to long is needed to avoid a -Wpointer-to-int-cast warning when arch_cmpxchg is used with a pointer type. Then convert to int and back to long to always sign extend the 32-bit value to 64-bit.
--- arch/riscv/include/asm/cmpxchg.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/riscv/include/asm/cmpxchg.h +1 -1
@@ -231,7 +231,7 @@__arch_cmpxchg (".w" ,".w" sc_sfx,".w" cas_sfx, \ sc_prepend, sc_append, \ cas_prepend, cas_append, \ - __ret, __ptr, (long), __old, __new); \ + __ret, __ptr, (long)(int)(long), __old, __new); \break ; \case 8 : \ __arch_cmpxchg(".d", ".d" sc_sfx, ".d" cas_sfx, \
Signature · 7 lines
2.48.1 -- Andreas Schwab, SUSE Labs, [email protected] GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different."
Alexandre Ghiti
reviewerHi Andreas,
Quoted 20 lines · Andreas Schwab
Sign extend also an unsigned compare value to match what lr.w is doing. Otherwise
try_cmpxchgmay spuriously return true when used on a u32 value that has the sign bit set, as it happens often ininode_set_ctime_current.Do this in three conversion steps. The first conversion to long is needed to avoid a -Wpointer-to-int-cast warning when
arch_cmpxchgis used with a pointer type. Then convert to int and back to long to always sign extend the 32-bit value to 64-bit.Fixes 6c58f25e6938 ("riscv/atomic: Fix sign extension for RV64I")Signed-off-by Andreas Schwab <[email protected]>--- arch/riscv/include/asm/cmpxchg.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/riscv/include/asm/cmpxchg.h +0
-0
arch/riscv/include/asm/cmpxchg.h line 237
Quoted 8 lines
arch/riscv/include/asm/cmpxchg.h +1
-1@@ -231,7 +231,7 @@__arch_cmpxchg (".w" ,".w" sc_sfx,".w" cas_sfx, \ sc_prepend, sc_append, \ cas_prepend, cas_append, \ - __ret, __ptr, (long), __old, __new); \ + __ret, __ptr, (long)(int)(long), __old, __new); \break ; \case 8 : \ __arch_cmpxchg(".d", ".d" sc_sfx, ".d" cas_sfx, \
That's a nice catch indeed. IIUC, we have the same issue here https://elixir.bootlin.com/linux/v6.13/source/arch/riscv/include/asm/futex.h#L89 right?
hanks,
Alex
Quoted 3 lines · Alexandre Ghiti
That's a nice catch indeed. IIUC, we have the same issue here
https://elixir.bootlin.com/linux/v6.13/source/arch/riscv/include/asm/futex.h#L89
right?
Indeed, though it doesn't result in wrong code currently. This is because the compare value is passed unmodified as u32 to the asm and the compiler keeps the value sign extended in registers. That would break if you would add a cast to long like in commit 6c58f25e6938 as that would erroneously zero extend it.
Signature · 3 lines
Andreas Schwab, SUSE Labs, [email protected] GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different."
Alexandre Ghiti
reviewerQuoted 11 lines · Andreas Schwab
Quoted 3 lines · Alexandre Ghiti
That's a nice catch indeed. IIUC, we have the same issue here
https://elixir.bootlin.com/linux/v6.13/source/arch/riscv/include/asm/futex.h#L89
right?Indeed, though it doesn't result in wrong code currently. This is because the compare value is passed unmodified as u32 to the asm and the compiler keeps the value sign extended in registers. That would break if you would add a cast to long like in commit
6c58f25e6938as that would erroneously zero extend it.
This is the disassembly I get:
ffffffff800fc540 <futex_atomic_cmpxchg_inatomic>:
...
ffffffff800fc566: 1605a8af lr.w.aqrl a7,(a1)
ffffffff800fc56a: 00c89563 bne a7,a2,ffffffff800fc574 <futex_atomic_cmpxchg_inatomic+0x3 4>
ffffffff800fc56e: 1ed5a52f sc.w.aqrl a0,a3,(a1)
a2 is used as it is passed by the calling function, so we can't be sure a2 is sign extended to me, what am I missing?
Quoted 2 lines · Alexandre Ghiti
a2 is used as it is passed by the calling function, so we can't be sure a2 is sign extended to me, what am I missing?
32-bit scalar arguments are guaranteed to be sign extended on entry.
Signature · 3 lines
Andreas Schwab, SUSE Labs, [email protected] GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different."
Quoted 7 lines · Andreas Schwab
Quoted 2 lines · Alexandre Ghiti
a2 is used as it is passed by the calling function, so we can't be sure a2 is sign extended to me, what am I missing?
32-bit scalar arguments are guaranteed to be sign extended on entry.
Firstly, the calling convention is irrelevant if the function is inlined, which this almost always will be.
Secondly, whilst GCC (still?) maintains the invariant that 32-bit registers are kept sign-extended in registers on RISC-V, and there’s a non-normative note in the unprivileged spec about this, LLVM does not, any extra bits are wholly unspecified, so you must not write inline assembly that depends on them being well-define, either by ignoring the bits or by converting your arguments to be whole-register types. If your compiler has already extended the value in the manner you care about then that’ll be a no-op, and if it hasn’t then it’ll do what you need.
We had an issue like this years ago in FreeBSD (also for a uint32_t CAS) and this was the conclusion I reached [1].
Jess
Quoted 11 lines · Jessica Clarke
Quoted 7 lines · Andreas Schwab
Quoted 2 lines · Alexandre Ghiti
a2 is used as it is passed by the calling function, so we can't be sure a2 is sign extended to me, what am I missing?
32-bit scalar arguments are guaranteed to be sign extended on entry.
Firstly, the calling convention is irrelevant if the function is inlined, which this almost always will be.
This is only about the non-inlined variant.
Signature · 3 lines
Andreas Schwab, SUSE Labs, [email protected] GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different."
Quoted 7 lines · Jessica Clarke
Quoted 4 lines
Quoted 2 lines
a2 is used as it is passed by the calling function, so we can't be sure a2 is sign extended to me, what am I missing?
32-bit scalar arguments are guaranteed to be sign extended on entry.
Firstly, the calling convention is irrelevant if the function is inlined, which this almost always will be.
Umm, that would be a compiler bug then, as inlining is supposed not to
change language semantics. IOW the compiler is expected to explicitly sign-extend the arguments of an inlined function at their evaluation point just as it would at an actual function call unless the compiler is able to prove they have come out sign-extended already from previous operations.
Maciej
Quoted 8 lines · Maciej W. Rozycki
Quoted 5 lines · Jessica Clarke
Quoted 4 lines
Quoted 2 lines
a2 is used as it is passed by the calling function, so we can't be sure a2 is sign extended to me, what am I missing?
32-bit scalar arguments are guaranteed to be sign extended on entry.
Quoted 8 lines
Quoted 2 lines
Firstly, the calling convention is irrelevant if the function is inlined, which this almost always will be.
Umm, that would be a compiler bug then, as inlining is supposed not to change language semantics. IOW the compiler is expected to explicitly sign-extend the arguments of an inlined function at their evaluation point just as it would at an actual function call unless the compiler is able to prove they have come out sign-extended already from previous operations.
No it’s not. The calling convention is there so that each side of the call know how the data is being passed between them. When inlining occurs there is no such call and compilers can do whatever they like. Calling conventions say absolutely nothing about what the representation of a value is whilst inside a function, only at boundaries. The ABI can also tell you what the observable representation of values within a function should be (e.g. that int is a 32-bit two’s complement value), but just like other platforms there is nothing in the RISC-V ABI specifications to say that 32-bit integers are kept sign-extended in registers, and that’s not going to be added because LLVM has never implemented that for RISC-V so it would be an ABI break for all LLVM-compiled RV64 code in existence.
Jess
Quoted 9 lines · Jessica Clarke
Quoted 5 lines
Umm, that would be a compiler bug then, as inlining is supposed not to change language semantics. IOW the compiler is expected to explicitly sign-extend the arguments of an inlined function at their evaluation point just as it would at an actual function call unless the compiler is able to prove they have come out sign-extended already from previous operations.
No it’s not. The calling convention is there so that each side of the call know how the data is being passed between them. When inlining occurs there is no such call and compilers can do whatever they like.
Right, I guess I got influenced too much by my MIPS experience here.
The contract here is to pass a 32-bit integer as an argument through a
function call boundary and with the RISC-V ISA saying the upper 32-bits of an input hardware register are don't-cares for 32-bit integer operations this contract is still met if inlining discards the sign-extension madated by the psABI. And consequently a piece of inline assembly cannot expect an incoming 32-bit integer in a register to be correctly interpreted as a 64-bit integer of equal value instead.
Although I find it odd in these circumstances that the psABI mandates
sign-extension here. And honestly I think an explicit cast ought to be added to the relevant assembly input operands (or a temporary variable of the appropriate type added and used to convert between the incoming value and the input operands); it will be a no-op for an optimising compiler when it knows the value has been sign-extended already and will carry the semantics information for a human code reader even if sign-extension has been implicitly ensured by the psABI despite the data type itself not making it evident.
Thanks for correcting me overall.
Maciej
Quoted 32 lines · Andreas Schwab
Sign extend also an unsigned compare value to match what lr.w is doing. Otherwise
try_cmpxchgmay spuriously return true when used on a u32 value that has the sign bit set, as it happens often ininode_set_ctime_current.Do this in three conversion steps. The first conversion to long is needed to avoid a -Wpointer-to-int-cast warning when
arch_cmpxchgis used with a pointer type. Then convert to int and back to long to always sign extend the 32-bit value to 64-bit.Fixes 6c58f25e6938 ("riscv/atomic: Fix sign extension for RV64I")Signed-off-by Andreas Schwab <[email protected]>--- arch/riscv/include/asm/cmpxchg.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/riscv/include/asm/cmpxchg.h +1
-1@@ -231,7 +231,7 @@__arch_cmpxchg (".w" ,".w" sc_sfx,".w" cas_sfx, \ sc_prepend, sc_append, \ cas_prepend, cas_append, \ - __ret, __ptr, (long), __old, __new); \ + __ret, __ptr, (long)(int)(long), __old, __new); \break ; \case 8 : \ __arch_cmpxchg(".d", ".d" sc_sfx, ".d" cas_sfx, \Signature · 1 line
2.48.1
Quoted 7 lines · On Thu, 30 Jan 2025 10:25:38 +0100 Andreas Schwab <[email protected]> wrote:
Sign extend also an unsigned compare value to match what lr.w is doing. Otherwise
try_cmpxchgmay spuriously return true when used on a u32 value that has the sign bit set, as it happens often ininode_set_ctime_current.Do this in three conversion steps. The first conversion to long is needed to avoid a -Wpointer-to-int-cast warning when
arch_cmpxchgis used with a pointer type.
Doesn't that break things by discarding the high bits of a pointer value?
David
Quoted 22 lines
Then convert to int and back to long to always sign extend the 32-bit value to 64-bit.
Fixes 6c58f25e6938 ("riscv/atomic: Fix sign extension for RV64I")Signed-off-by Andreas Schwab <[email protected]>--- arch/riscv/include/asm/cmpxchg.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/riscv/include/asm/cmpxchg.h +1
-1@@ -231,7 +231,7 @@__arch_cmpxchg (".w" ,".w" sc_sfx,".w" cas_sfx, \ sc_prepend, sc_append, \ cas_prepend, cas_append, \ - __ret, __ptr, (long), __old, __new); \ + __ret, __ptr, (long)(int)(long), __old, __new); \break ; \case 8 : \ __arch_cmpxchg(".d", ".d" sc_sfx, ".d" cas_sfx, \
Quoted 7 lines · David Laight
Quoted 4 lines · On Thu, 30 Jan 2025 10:25:38 +0100 Andreas Schwab <[email protected]> wrote:
Sign extend also an unsigned compare value to match what lr.w is doing. Otherwise
try_cmpxchgmay spuriously return true when used on a u32 value that has the sign bit set, as it happens often ininode_set_ctime_current.
Quoted 5 lines
Quoted 3 lines
Do this in three conversion steps. The first conversion to long is needed to avoid a -Wpointer-to-int-cast warning when
arch_cmpxchgis used with a pointer type.Doesn't that break things by discarding the high bits of a pointer value?
If you have 32-bit pointers then the conversions are no-ops.
Signature · 3 lines
Andreas Schwab, SUSE Labs, [email protected] GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different."
Quoted 11 lines · Andreas Schwab
Sign extend also an unsigned compare value to match what lr.w is doing. Otherwise
try_cmpxchgmay spuriously return true when used on a u32 value that has the sign bit set, as it happens often ininode_set_ctime_current.Do this in three conversion steps. The first conversion to long is needed to avoid a -Wpointer-to-int-cast warning when
arch_cmpxchgis used with a pointer type. Then convert to int and back to long to always sign extend the 32-bit value to 64-bit.Fixes 6c58f25e6938 ("riscv/atomic: Fix sign extension for RV64I")Signed-off-by Andreas Schwab <[email protected]>
It fixes the gnulib test-stat-time failure for me.
Quoted 19 lines
---
arch/riscv/include/asm/cmpxchg.h | 2 +-1 file changed, 1 insertion(+), 1 deletion(-)arch/riscv/include/asm/cmpxchg.h +1
-1@@ -231,7 +231,7 @@ __arch_cmpxchg(".w", ".w" sc_sfx, ".w" cas_sfx, \ sc_prepend, sc_append, \ cas_prepend, cas_append, \ - __ret, __ptr, (long), __old, __new); \ + __ret, __ptr, (long)(int)(long), __old, __new); \ break; \ case 8: \ __arch_cmpxchg(".d", ".d" sc_sfx, ".d" cas_sfx, \Signature · 1 line
2.48.1
Signature · 2 lines
Xi Ruoyao <[email protected]> School of Aerospace Science and Technology, Xidian University
Hi Andreas,
Quoted 20 lines · Andreas Schwab
Sign extend also an unsigned compare value to match what lr.w is doing. Otherwise
try_cmpxchgmay spuriously return true when used on a u32 value that has the sign bit set, as it happens often ininode_set_ctime_current.Do this in three conversion steps. The first conversion to long is needed to avoid a -Wpointer-to-int-cast warning when
arch_cmpxchgis used with a pointer type. Then convert to int and back to long to always sign extend the 32-bit value to 64-bit.Fixes 6c58f25e6938 ("riscv/atomic: Fix sign extension for RV64I")Signed-off-by Andreas Schwab <[email protected]>--- arch/riscv/include/asm/cmpxchg.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/riscv/include/asm/cmpxchg.h +0
-0
arch/riscv/include/asm/cmpxchg.h line 237
Quoted 8 lines
arch/riscv/include/asm/cmpxchg.h +1
-1@@ -231,7 +231,7 @@__arch_cmpxchg (".w" ,".w" sc_sfx,".w" cas_sfx, \ sc_prepend, sc_append, \ cas_prepend, cas_append, \ - __ret, __ptr, (long), __old, __new); \ + __ret, __ptr, (long)(int)(long), __old, __new); \break ; \case 8 : \ __arch_cmpxchg(".d", ".d" sc_sfx, ".d" cas_sfx, \
Thanks,
Alex
This patch was applied to riscv/linux.git (fixes) by Palmer Dabbelt <[email protected]>:
Hello:
This patch was applied to riscv/linux.git (fixes)
by Palmer Dabbelt <[email protected]>:
Quoted 10 lines · On Thu, 30 Jan 2025 10:25:38 +0100 you wrote:
Sign extend also an unsigned compare value to match what lr.w is doing. Otherwise
try_cmpxchgmay spuriously return true when used on a u32 value that has the sign bit set, as it happens often ininode_set_ctime_current.Do this in three conversion steps. The first conversion to long is needed to avoid a -Wpointer-to-int-cast warning when
arch_cmpxchgis used with a pointer type. Then convert to int and back to long to always sign extend the 32-bit value to 64-bit.[...]
Here is the summary with links:
- riscv/atomic: Do proper sign extension also for unsigned in arch_cmpxchg
https://git.kernel.org/riscv/c/431a3bbd3249
You are awesome, thank you!
Signature · 2 lines
Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html