[PATCH] riscv/atomic: Do proper sign extension also for unsigned in arch_cmpxchg

[PATCH] riscv/atomic: Do proper sign extension also for unsigned in arch_cmpxchg

[Andreas Schwab]

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.

Fixes: 6c58f25e6938 ("riscv/atomic: Fix sign extension for RV64I")
Signed-off-by: Andreas Schwab <schwab@suse.de>
---
 arch/riscv/include/asm/cmpxchg.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h
index 4cadc56220fe..427c41dde643 100644
--- a/arch/riscv/include/asm/cmpxchg.h
+++ b/arch/riscv/include/asm/cmpxchg.h
@@ -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,		\
-- 
2.48.1


-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] riscv/atomic: Do proper sign extension also for unsigned in arch_cmpxchg

[Alexandre Ghiti]

Hi Andreas,

On 30/01/2025 10:25, Andreas Schwab wrote:
> 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.
>
> Fixes: 6c58f25e6938 ("riscv/atomic: Fix sign extension for RV64I")
> Signed-off-by: Andreas Schwab <schwab@suse.de>
> ---
>   arch/riscv/include/asm/cmpxchg.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h
> index 4cadc56220fe..427c41dde643 100644
> --- a/arch/riscv/include/asm/cmpxchg.h
> +++ b/arch/riscv/include/asm/cmpxchg.h
> @@ -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

Re: [PATCH] riscv/atomic: Do proper sign extension also for unsigned in arch_cmpxchg

[Andreas Schwab]

On Jan 30 2025, Alexandre Ghiti wrote:

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

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] riscv/atomic: Do proper sign extension also for unsigned in arch_cmpxchg

[Alexandre Ghiti]

On 30/01/2025 15:52, Andreas Schwab wrote:
> On Jan 30 2025, Alexandre Ghiti wrote:
>
>> 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.
>

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?

Re: [PATCH] riscv/atomic: Do proper sign extension also for unsigned in arch_cmpxchg

[Andreas Schwab]

On Jan 30 2025, Alexandre Ghiti wrote:

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

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] riscv/atomic: Do proper sign extension also for unsigned in arch_cmpxchg

[Jessica Clarke]

On 30 Jan 2025, at 15:53, Andreas Schwab <schwab@suse.de> wrote:
> 
> On Jan 30 2025, Alexandre Ghiti wrote:
> 
>> 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

[1] https://reviews.freebsd.org/D22084

Re: [PATCH] riscv/atomic: Do proper sign extension also for unsigned in arch_cmpxchg

[Andreas Schwab]

On Jan 30 2025, Jessica Clarke wrote:

> On 30 Jan 2025, at 15:53, Andreas Schwab <schwab@suse.de> wrote:
>> 
>> On Jan 30 2025, Alexandre Ghiti wrote:
>> 
>>> 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.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] riscv/atomic: Do proper sign extension also for unsigned in arch_cmpxchg

[Maciej W. Rozycki]

On Thu, 30 Jan 2025, Jessica Clarke wrote:

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

Re: [PATCH] riscv/atomic: Do proper sign extension also for unsigned in arch_cmpxchg

[Jessica Clarke]

On 3 Feb 2025, at 13:57, Maciej W. Rozycki <macro@orcam.me.uk> wrote:
> 
> On Thu, 30 Jan 2025, Jessica Clarke wrote:
> 
>>>> 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.

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

Re: [PATCH] riscv/atomic: Do proper sign extension also for unsigned in arch_cmpxchg

[Maciej W. Rozycki]

On Mon, 3 Feb 2025, Jessica Clarke wrote:

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

Re: [PATCH] riscv/atomic: Do proper sign extension also for unsigned in arch_cmpxchg

[Andrew Jones]

On Thu, Jan 30, 2025 at 10:25:38AM +0100, Andreas Schwab wrote:
> 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.
> 
> Fixes: 6c58f25e6938 ("riscv/atomic: Fix sign extension for RV64I")
> Signed-off-by: Andreas Schwab <schwab@suse.de>
> ---
>  arch/riscv/include/asm/cmpxchg.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h
> index 4cadc56220fe..427c41dde643 100644
> --- a/arch/riscv/include/asm/cmpxchg.h
> +++ b/arch/riscv/include/asm/cmpxchg.h
> @@ -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,		\
> -- 
> 2.48.1
> 
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Re: [PATCH] riscv/atomic: Do proper sign extension also for unsigned in arch_cmpxchg

[David Laight]

On Thu, 30 Jan 2025 10:25:38 +0100
Andreas Schwab <schwab@suse.de> wrote:

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

Doesn't that break things by discarding the high bits of a pointer value?

	David

> 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 <schwab@suse.de>
> ---
>  arch/riscv/include/asm/cmpxchg.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h
> index 4cadc56220fe..427c41dde643 100644
> --- a/arch/riscv/include/asm/cmpxchg.h
> +++ b/arch/riscv/include/asm/cmpxchg.h
> @@ -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,		\

Re: [PATCH] riscv/atomic: Do proper sign extension also for unsigned in arch_cmpxchg

[Andreas Schwab]

On Jan 30 2025, David Laight wrote:

> On Thu, 30 Jan 2025 10:25:38 +0100
> Andreas Schwab <schwab@suse.de> wrote:
>
>> 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.
>
> 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.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] riscv/atomic: Do proper sign extension also for unsigned in arch_cmpxchg

[Xi Ruoyao]

On Thu, 2025-01-30 at 10:25 +0100, Andreas Schwab wrote:
> 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.
> 
> Fixes: 6c58f25e6938 ("riscv/atomic: Fix sign extension for RV64I")
> Signed-off-by: Andreas Schwab <schwab@suse.de>

It fixes the gnulib test-stat-time failure for me.

Tested-by: Xi Ruoyao <xry111@xry111.site>

> ---
>  arch/riscv/include/asm/cmpxchg.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h
> index 4cadc56220fe..427c41dde643 100644
> --- a/arch/riscv/include/asm/cmpxchg.h
> +++ b/arch/riscv/include/asm/cmpxchg.h
> @@ -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,		\
> -- 
> 2.48.1

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH] riscv/atomic: Do proper sign extension also for unsigned in arch_cmpxchg

[Alexandre Ghiti]

Hi Andreas,

On 30/01/2025 10:25, Andreas Schwab wrote:
> 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.
>
> Fixes: 6c58f25e6938 ("riscv/atomic: Fix sign extension for RV64I")
> Signed-off-by: Andreas Schwab <schwab@suse.de>
> ---
>   arch/riscv/include/asm/cmpxchg.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h
> index 4cadc56220fe..427c41dde643 100644
> --- a/arch/riscv/include/asm/cmpxchg.h
> +++ b/arch/riscv/include/asm/cmpxchg.h
> @@ -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,		\


Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com>

Thanks,

Alex

Re: [PATCH] riscv/atomic: Do proper sign extension also for unsigned in arch_cmpxchg

[patchwork-bot+linux-riscv]

Hello:

This patch was applied to riscv/linux.git (fixes)
by Palmer Dabbelt <palmer@rivosinc.com>:

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_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.
> 
> [...]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html