[PATCH] riscv/futex: sign extend compare value in atomic cmpxchg

[PATCH] riscv/futex: sign extend compare value in atomic cmpxchg

[Andreas Schwab]

Make sure the compare value in the lr/sc loop is sign extended to match
what lr.w does.  Fortunately, due to the compiler keeping the register
contents sign extended anyway the lack of the explicit extension didn't
result in wrong code so far, but this cannot be relied upon.

Fixes: b90edb33010b ("RISC-V: Add futex support.")
Signed-off-by: Andreas Schwab <schwab@suse.de>
---
 arch/riscv/include/asm/futex.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/riscv/include/asm/futex.h b/arch/riscv/include/asm/futex.h
index fc8130f995c1..6907c456ac8c 100644
--- a/arch/riscv/include/asm/futex.h
+++ b/arch/riscv/include/asm/futex.h
@@ -93,7 +93,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
 		_ASM_EXTABLE_UACCESS_ERR(1b, 3b, %[r])	\
 		_ASM_EXTABLE_UACCESS_ERR(2b, 3b, %[r])	\
 	: [r] "+r" (ret), [v] "=&r" (val), [u] "+m" (*uaddr), [t] "=&r" (tmp)
-	: [ov] "Jr" (oldval), [nv] "Jr" (newval)
+	: [ov] "Jr" ((long)(int)oldval), [nv] "Jr" (newval)
 	: "memory");
 	__disable_user_access();
 
-- 
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/futex: sign extend compare value in atomic cmpxchg

[Björn Töpel]

Andreas Schwab <schwab@suse.de> writes:

> Make sure the compare value in the lr/sc loop is sign extended to match
> what lr.w does.  Fortunately, due to the compiler keeping the register
> contents sign extended anyway the lack of the explicit extension didn't
> result in wrong code so far, but this cannot be relied upon.
>
> Fixes: b90edb33010b ("RISC-V: Add futex support.")
> Signed-off-by: Andreas Schwab <schwab@suse.de>

Hmm, in this scenario we *can* rely on it, no (inline vs macro)?

Regardless, having an explicit cast there doesn't hurt, and make it more
obvious!

Reviewed-by: Björn Töpel <bjorn@rivosinc.com>

Let's add a link to Jessica's comment as well:
Link: https://lore.kernel.org/linux-riscv/CC2D9220-F8DE-4CC8-ACAD-7B1A21E276FE@jrtc27.com/

> ---
>  arch/riscv/include/asm/futex.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/riscv/include/asm/futex.h b/arch/riscv/include/asm/futex.h
> index fc8130f995c1..6907c456ac8c 100644
> --- a/arch/riscv/include/asm/futex.h
> +++ b/arch/riscv/include/asm/futex.h
> @@ -93,7 +93,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
>  		_ASM_EXTABLE_UACCESS_ERR(1b, 3b, %[r])	\
>  		_ASM_EXTABLE_UACCESS_ERR(2b, 3b, %[r])	\
>  	: [r] "+r" (ret), [v] "=&r" (val), [u] "+m" (*uaddr), [t] "=&r" (tmp)
> -	: [ov] "Jr" (oldval), [nv] "Jr" (newval)
> +	: [ov] "Jr" ((long)(int)oldval), [nv] "Jr" (newval)
>  	: "memory");
>  	__disable_user_access();
>  
> -- 
> 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."
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] riscv/futex: sign extend compare value in atomic cmpxchg

[Andreas Schwab]

On Feb 03 2025, Björn Töpel wrote:

> Andreas Schwab <schwab@suse.de> writes:
>
>> Make sure the compare value in the lr/sc loop is sign extended to match
>> what lr.w does.  Fortunately, due to the compiler keeping the register
>> contents sign extended anyway the lack of the explicit extension didn't
>> result in wrong code so far, but this cannot be relied upon.
>>
>> Fixes: b90edb33010b ("RISC-V: Add futex support.")
>> Signed-off-by: Andreas Schwab <schwab@suse.de>
>
> Hmm, in this scenario we *can* rely on it, no (inline vs macro)?

No, the issue is that the asm operand (oldval) is u32, but the asm is
using the 64-bit value from the register.  You cannot expect that the
compiler keeps the upper half defined in any way at this point.  That is
different for the operand that is passed in from newval, because sc.w is
only using the low 32-bits from the operand.

-- 
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/futex: sign extend compare value in atomic cmpxchg

[Jessica Clarke]

On 3 Feb 2025, at 10:06, Andreas Schwab <schwab@suse.de> wrote:
> 
> Make sure the compare value in the lr/sc loop is sign extended to match
> what lr.w does.  Fortunately, due to the compiler keeping the register
> contents sign extended anyway the lack of the explicit extension didn't
> result in wrong code so far, but this cannot be relied upon.

GCC may guarantee this today, but LLVM does not, and definitely
generates code that does not do so. Whether that happens for any of the
consumers of this API today I don’t know, but it is definitely relying
on things that aren’t true for LLVM, and are not specified in the psABI.

Jess

> Fixes: b90edb33010b ("RISC-V: Add futex support.")
> Signed-off-by: Andreas Schwab <schwab@suse.de>
> ---
> arch/riscv/include/asm/futex.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/include/asm/futex.h b/arch/riscv/include/asm/futex.h
> index fc8130f995c1..6907c456ac8c 100644
> --- a/arch/riscv/include/asm/futex.h
> +++ b/arch/riscv/include/asm/futex.h
> @@ -93,7 +93,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
> _ASM_EXTABLE_UACCESS_ERR(1b, 3b, %[r]) \
> _ASM_EXTABLE_UACCESS_ERR(2b, 3b, %[r]) \
> : [r] "+r" (ret), [v] "=&r" (val), [u] "+m" (*uaddr), [t] "=&r" (tmp)
> - : [ov] "Jr" (oldval), [nv] "Jr" (newval)
> + : [ov] "Jr" ((long)(int)oldval), [nv] "Jr" (newval)
> : "memory");
> __disable_user_access();
> 
> -- 
> 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."
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] riscv/futex: sign extend compare value in atomic cmpxchg

[Andreas Schwab]

On Feb 03 2025, Jessica Clarke wrote:

> On 3 Feb 2025, at 10:06, Andreas Schwab <schwab@suse.de> wrote:
>> 
>> Make sure the compare value in the lr/sc loop is sign extended to match
>> what lr.w does.  Fortunately, due to the compiler keeping the register
>> contents sign extended anyway the lack of the explicit extension didn't
>> result in wrong code so far, but this cannot be relied upon.
>
> GCC may guarantee this today,

It doesn't, that's the point!

-- 
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/futex: sign extend compare value in atomic cmpxchg

[Alexandre Ghiti]

Hi Andreas,

On 03/02/2025 11:06, Andreas Schwab wrote:
> Make sure the compare value in the lr/sc loop is sign extended to match
> what lr.w does.  Fortunately, due to the compiler keeping the register
> contents sign extended anyway the lack of the explicit extension didn't
> result in wrong code so far, but this cannot be relied upon.
>
> Fixes: b90edb33010b ("RISC-V: Add futex support.")
> Signed-off-by: Andreas Schwab <schwab@suse.de>
> ---
>   arch/riscv/include/asm/futex.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/riscv/include/asm/futex.h b/arch/riscv/include/asm/futex.h
> index fc8130f995c1..6907c456ac8c 100644
> --- a/arch/riscv/include/asm/futex.h
> +++ b/arch/riscv/include/asm/futex.h
> @@ -93,7 +93,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
>   		_ASM_EXTABLE_UACCESS_ERR(1b, 3b, %[r])	\
>   		_ASM_EXTABLE_UACCESS_ERR(2b, 3b, %[r])	\
>   	: [r] "+r" (ret), [v] "=&r" (val), [u] "+m" (*uaddr), [t] "=&r" (tmp)
> -	: [ov] "Jr" (oldval), [nv] "Jr" (newval)
> +	: [ov] "Jr" ((long)(int)oldval), [nv] "Jr" (newval)
>   	: "memory");
>   	__disable_user_access();
>   


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

Thanks,

Alex

Re: [PATCH] riscv/futex: sign extend compare value in atomic cmpxchg

[patchwork-bot+linux-riscv]

Hello:

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

On Mon, 03 Feb 2025 11:06:00 +0100 you wrote:
> Make sure the compare value in the lr/sc loop is sign extended to match
> what lr.w does.  Fortunately, due to the compiler keeping the register
> contents sign extended anyway the lack of the explicit extension didn't
> result in wrong code so far, but this cannot be relied upon.
> 
> Fixes: b90edb33010b ("RISC-V: Add futex support.")
> Signed-off-by: Andreas Schwab <schwab@suse.de>
> 
> [...]

Here is the summary with links:
  - riscv/futex: sign extend compare value in atomic cmpxchg
    https://git.kernel.org/riscv/c/5c238584bce5

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [External] Re: [PATCH] riscv/futex: sign extend compare value in atomic cmpxchg

[yunhui cui]

Hi,

On Fri, Feb 14, 2025 at 2:31 AM <patchwork-bot+linux-riscv@kernel.org> wrote:
>
> Hello:
>
> This patch was applied to riscv/linux.git (fixes)
> by Palmer Dabbelt <palmer@rivosinc.com>:
>
> On Mon, 03 Feb 2025 11:06:00 +0100 you wrote:
> > Make sure the compare value in the lr/sc loop is sign extended to match
> > what lr.w does.  Fortunately, due to the compiler keeping the register
> > contents sign extended anyway the lack of the explicit extension didn't
> > result in wrong code so far, but this cannot be relied upon.
> >
> > Fixes: b90edb33010b ("RISC-V: Add futex support.")
> > Signed-off-by: Andreas Schwab <schwab@suse.de>
> >
> > [...]
>
> Here is the summary with links:
>   - riscv/futex: sign extend compare value in atomic cmpxchg
>     https://git.kernel.org/riscv/c/5c238584bce5
>
> You are awesome, thank you!
> --
> Deet-doot-dot, I am a bot.
> https://korg.docs.kernel.org/patchwork/pwbot.html
>
>

I applied this patch, but it doesn't seem to take effect. There is no
sign extension for a2 in the assembly code. What did I miss?
GCC version >= 13.

ffffffff800e87e0 <futex_atomic_cmpxchg_inatomic>:
ffffffff800e87e0: 1141                addi sp,sp,-16
ffffffff800e87e2: e422                sd s0,8(sp)
ffffffff800e87e4: 2bf01793          bseti a5,zero,0x3f
ffffffff800e87e8: 0800                addi s0,sp,16
ffffffff800e87ea: 17ed                addi a5,a5,-5
ffffffff800e87ec: 00b7f663          bgeu a5,a1,ffffffff800e87f8
<futex_atomic_cmpxchg_inatomic+0x18>
ffffffff800e87f0: 6422                ld s0,8(sp)
ffffffff800e87f2: 5549                li a0,-14
ffffffff800e87f4: 0141                addi sp,sp,16
ffffffff800e87f6: 8082                ret
ffffffff800e87f8: 872a                mv a4,a0
ffffffff800e87fa: 00040837          lui a6,0x40
ffffffff800e87fe: 10082073          csrs sstatus,a6
ffffffff800e8802: 4781                li a5,0
ffffffff800e8804: 1605a8af          lr.w.aqrl a7,(a1)
ffffffff800e8808: 00c89563          bne a7,a2,ffffffff800e8812
<futex_atomic_cmpxchg_inatomic+0x32>
ffffffff800e880c: 1ed5a52f          sc.w.aqrl a0,a3,(a1)
ffffffff800e8810: f975                bnez a0,ffffffff800e8804
<futex_atomic_cmpxchg_inatomic+0x24>
ffffffff800e8812: 0007851b          sext.w a0,a5
ffffffff800e8816: 10083073          csrc sstatus,a6
ffffffff800e881a: 01172023          sw a7,0(a4)
ffffffff800e881e: 6422                ld s0,8(sp)
ffffffff800e8820: 0141                addi sp,sp,16
ffffffff800e8822: 8082                ret

Should we do it like this:
__asm__ __volatile__ (
" sext.w %[ov], %[ov] \n"
...


Thanks,
Yunhui

Re: [External] [PATCH] riscv/futex: sign extend compare value in atomic cmpxchg

[Jessica Clarke]

On 14 Feb 2025, at 04:11, yunhui cui <cuiyunhui@bytedance.com> wrote:
> 
> Hi,
> 
> On Fri, Feb 14, 2025 at 2:31 AM <patchwork-bot+linux-riscv@kernel.org> wrote:
>> 
>> Hello:
>> 
>> This patch was applied to riscv/linux.git (fixes)
>> by Palmer Dabbelt <palmer@rivosinc.com>:
>> 
>> On Mon, 03 Feb 2025 11:06:00 +0100 you wrote:
>>> Make sure the compare value in the lr/sc loop is sign extended to match
>>> what lr.w does.  Fortunately, due to the compiler keeping the register
>>> contents sign extended anyway the lack of the explicit extension didn't
>>> result in wrong code so far, but this cannot be relied upon.
>>> 
>>> Fixes: b90edb33010b ("RISC-V: Add futex support.")
>>> Signed-off-by: Andreas Schwab <schwab@suse.de>
>>> 
>>> [...]
>> 
>> Here is the summary with links:
>>  - riscv/futex: sign extend compare value in atomic cmpxchg
>>    https://git.kernel.org/riscv/c/5c238584bce5
>> 
>> You are awesome, thank you!
>> --
>> Deet-doot-dot, I am a bot.
>> https://korg.docs.kernel.org/patchwork/pwbot.html
>> 
>> 
> 
> I applied this patch, but it doesn't seem to take effect. There is no
> sign extension for a2 in the assembly code. What did I miss?
> GCC version >= 13.
> 
> ffffffff800e87e0 <futex_atomic_cmpxchg_inatomic>:
> ffffffff800e87e0: 1141                addi sp,sp,-16
> ffffffff800e87e2: e422                sd s0,8(sp)
> ffffffff800e87e4: 2bf01793          bseti a5,zero,0x3f
> ffffffff800e87e8: 0800                addi s0,sp,16
> ffffffff800e87ea: 17ed                addi a5,a5,-5
> ffffffff800e87ec: 00b7f663          bgeu a5,a1,ffffffff800e87f8
> <futex_atomic_cmpxchg_inatomic+0x18>
> ffffffff800e87f0: 6422                ld s0,8(sp)
> ffffffff800e87f2: 5549                li a0,-14
> ffffffff800e87f4: 0141                addi sp,sp,16
> ffffffff800e87f6: 8082                ret
> ffffffff800e87f8: 872a                mv a4,a0
> ffffffff800e87fa: 00040837          lui a6,0x40
> ffffffff800e87fe: 10082073          csrs sstatus,a6
> ffffffff800e8802: 4781                li a5,0
> ffffffff800e8804: 1605a8af          lr.w.aqrl a7,(a1)
> ffffffff800e8808: 00c89563          bne a7,a2,ffffffff800e8812
> <futex_atomic_cmpxchg_inatomic+0x32>
> ffffffff800e880c: 1ed5a52f          sc.w.aqrl a0,a3,(a1)
> ffffffff800e8810: f975                bnez a0,ffffffff800e8804
> <futex_atomic_cmpxchg_inatomic+0x24>
> ffffffff800e8812: 0007851b          sext.w a0,a5
> ffffffff800e8816: 10083073          csrc sstatus,a6
> ffffffff800e881a: 01172023          sw a7,0(a4)
> ffffffff800e881e: 6422                ld s0,8(sp)
> ffffffff800e8820: 0141                addi sp,sp,16
> ffffffff800e8822: 8082                ret

The calling convention means a2 will be sign-extended on entry to the
function, and since your compiler has not done anything to change the
value that will still be true. So it has (legitimately) optimised out
any sign extension as a no-op. Are you seeing any problems that you
believe to be caused by this function, or are you just inspecting the
disassembly to satisfy your own curiosity?

> Should we do it like this:
> __asm__ __volatile__ (
> " sext.w %[ov], %[ov] \n"
> ...

No, it’s unnecessary and prevents optimisation.

Jess

> Thanks,
> Yunhui
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [External] [PATCH] riscv/futex: sign extend compare value in atomic cmpxchg

[yunhui cui]

Hi Jess,

On Fri, Feb 14, 2025 at 2:05 PM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>
> On 14 Feb 2025, at 04:11, yunhui cui <cuiyunhui@bytedance.com> wrote:
> >
> > Hi,
> >
> > On Fri, Feb 14, 2025 at 2:31 AM <patchwork-bot+linux-riscv@kernel.org> wrote:
> >>
> >> Hello:
> >>
> >> This patch was applied to riscv/linux.git (fixes)
> >> by Palmer Dabbelt <palmer@rivosinc.com>:
> >>
> >> On Mon, 03 Feb 2025 11:06:00 +0100 you wrote:
> >>> Make sure the compare value in the lr/sc loop is sign extended to match
> >>> what lr.w does.  Fortunately, due to the compiler keeping the register
> >>> contents sign extended anyway the lack of the explicit extension didn't
> >>> result in wrong code so far, but this cannot be relied upon.
> >>>
> >>> Fixes: b90edb33010b ("RISC-V: Add futex support.")
> >>> Signed-off-by: Andreas Schwab <schwab@suse.de>
> >>>
> >>> [...]
> >>
> >> Here is the summary with links:
> >>  - riscv/futex: sign extend compare value in atomic cmpxchg
> >>    https://git.kernel.org/riscv/c/5c238584bce5
> >>
> >> You are awesome, thank you!
> >> --
> >> Deet-doot-dot, I am a bot.
> >> https://korg.docs.kernel.org/patchwork/pwbot.html
> >>
> >>
> >
> > I applied this patch, but it doesn't seem to take effect. There is no
> > sign extension for a2 in the assembly code. What did I miss?
> > GCC version >= 13.
> >
> > ffffffff800e87e0 <futex_atomic_cmpxchg_inatomic>:
> > ffffffff800e87e0: 1141                addi sp,sp,-16
> > ffffffff800e87e2: e422                sd s0,8(sp)
> > ffffffff800e87e4: 2bf01793          bseti a5,zero,0x3f
> > ffffffff800e87e8: 0800                addi s0,sp,16
> > ffffffff800e87ea: 17ed                addi a5,a5,-5
> > ffffffff800e87ec: 00b7f663          bgeu a5,a1,ffffffff800e87f8
> > <futex_atomic_cmpxchg_inatomic+0x18>
> > ffffffff800e87f0: 6422                ld s0,8(sp)
> > ffffffff800e87f2: 5549                li a0,-14
> > ffffffff800e87f4: 0141                addi sp,sp,16
> > ffffffff800e87f6: 8082                ret
> > ffffffff800e87f8: 872a                mv a4,a0
> > ffffffff800e87fa: 00040837          lui a6,0x40
> > ffffffff800e87fe: 10082073          csrs sstatus,a6
> > ffffffff800e8802: 4781                li a5,0
> > ffffffff800e8804: 1605a8af          lr.w.aqrl a7,(a1)
> > ffffffff800e8808: 00c89563          bne a7,a2,ffffffff800e8812
> > <futex_atomic_cmpxchg_inatomic+0x32>
> > ffffffff800e880c: 1ed5a52f          sc.w.aqrl a0,a3,(a1)
> > ffffffff800e8810: f975                bnez a0,ffffffff800e8804
> > <futex_atomic_cmpxchg_inatomic+0x24>
> > ffffffff800e8812: 0007851b          sext.w a0,a5
> > ffffffff800e8816: 10083073          csrc sstatus,a6
> > ffffffff800e881a: 01172023          sw a7,0(a4)
> > ffffffff800e881e: 6422                ld s0,8(sp)
> > ffffffff800e8820: 0141                addi sp,sp,16
> > ffffffff800e8822: 8082                ret
>
> The calling convention means a2 will be sign-extended on entry to the
> function, and since your compiler has not done anything to change the
> value that will still be true. So it has (legitimately) optimised out
> any sign extension as a no-op.

 If so, why this patch?

> Are you seeing any problems that you
> believe to be caused by this function, or are you just inspecting the
> disassembly to satisfy your own curiosity?

No actual problem traced here.


>
> > Should we do it like this:
> > __asm__ __volatile__ (
> > " sext.w %[ov], %[ov] \n"
> > ...
>
> No, it’s unnecessary and prevents optimisation.
>
> Jess
>
> > Thanks,
> > Yunhui
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
>

Thanks,
Yunhui

Re: [External] [PATCH] riscv/futex: sign extend compare value in atomic cmpxchg

[Jessica Clarke]

On 14 Feb 2025, at 06:22, yunhui cui <cuiyunhui@bytedance.com> wrote:
> 
> Hi Jess,
> 
> On Fri, Feb 14, 2025 at 2:05 PM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>> 
>> On 14 Feb 2025, at 04:11, yunhui cui <cuiyunhui@bytedance.com> wrote:
>>> 
>>> Hi,
>>> 
>>> On Fri, Feb 14, 2025 at 2:31 AM <patchwork-bot+linux-riscv@kernel.org> wrote:
>>>> 
>>>> Hello:
>>>> 
>>>> This patch was applied to riscv/linux.git (fixes)
>>>> by Palmer Dabbelt <palmer@rivosinc.com>:
>>>> 
>>>> On Mon, 03 Feb 2025 11:06:00 +0100 you wrote:
>>>>> Make sure the compare value in the lr/sc loop is sign extended to match
>>>>> what lr.w does.  Fortunately, due to the compiler keeping the register
>>>>> contents sign extended anyway the lack of the explicit extension didn't
>>>>> result in wrong code so far, but this cannot be relied upon.
>>>>> 
>>>>> Fixes: b90edb33010b ("RISC-V: Add futex support.")
>>>>> Signed-off-by: Andreas Schwab <schwab@suse.de>
>>>>> 
>>>>> [...]
>>>> 
>>>> Here is the summary with links:
>>>> - riscv/futex: sign extend compare value in atomic cmpxchg
>>>>   https://git.kernel.org/riscv/c/5c238584bce5
>>>> 
>>>> You are awesome, thank you!
>>>> --
>>>> Deet-doot-dot, I am a bot.
>>>> https://korg.docs.kernel.org/patchwork/pwbot.html
>>>> 
>>>> 
>>> 
>>> I applied this patch, but it doesn't seem to take effect. There is no
>>> sign extension for a2 in the assembly code. What did I miss?
>>> GCC version >= 13.
>>> 
>>> ffffffff800e87e0 <futex_atomic_cmpxchg_inatomic>:
>>> ffffffff800e87e0: 1141                addi sp,sp,-16
>>> ffffffff800e87e2: e422                sd s0,8(sp)
>>> ffffffff800e87e4: 2bf01793          bseti a5,zero,0x3f
>>> ffffffff800e87e8: 0800                addi s0,sp,16
>>> ffffffff800e87ea: 17ed                addi a5,a5,-5
>>> ffffffff800e87ec: 00b7f663          bgeu a5,a1,ffffffff800e87f8
>>> <futex_atomic_cmpxchg_inatomic+0x18>
>>> ffffffff800e87f0: 6422                ld s0,8(sp)
>>> ffffffff800e87f2: 5549                li a0,-14
>>> ffffffff800e87f4: 0141                addi sp,sp,16
>>> ffffffff800e87f6: 8082                ret
>>> ffffffff800e87f8: 872a                mv a4,a0
>>> ffffffff800e87fa: 00040837          lui a6,0x40
>>> ffffffff800e87fe: 10082073          csrs sstatus,a6
>>> ffffffff800e8802: 4781                li a5,0
>>> ffffffff800e8804: 1605a8af          lr.w.aqrl a7,(a1)
>>> ffffffff800e8808: 00c89563          bne a7,a2,ffffffff800e8812
>>> <futex_atomic_cmpxchg_inatomic+0x32>
>>> ffffffff800e880c: 1ed5a52f          sc.w.aqrl a0,a3,(a1)
>>> ffffffff800e8810: f975                bnez a0,ffffffff800e8804
>>> <futex_atomic_cmpxchg_inatomic+0x24>
>>> ffffffff800e8812: 0007851b          sext.w a0,a5
>>> ffffffff800e8816: 10083073          csrc sstatus,a6
>>> ffffffff800e881a: 01172023          sw a7,0(a4)
>>> ffffffff800e881e: 6422                ld s0,8(sp)
>>> ffffffff800e8820: 0141                addi sp,sp,16
>>> ffffffff800e8822: 8082                ret
>> 
>> The calling convention means a2 will be sign-extended on entry to the
>> function, and since your compiler has not done anything to change the
>> value that will still be true. So it has (legitimately) optimised out
>> any sign extension as a no-op.
> 
> If so, why this patch?

Because there are multiple ways in which compilers can emit code that
does not keep values sign-extended in registers within a function. The
calling convention only provides that guarantee on entry to a function
following the standard calling convention. In the specific instance of
compiling the version of Linux you are compiling with the kernel config
you are using and the precise compiler you are using, the patch happens
to not be needed for this snippet of the binary because the generated
code already has the sign-extended value in the register used. But just
because there are instances when the patch does nothing doesn’t mean
all instances are like that.

Jess

>> Are you seeing any problems that you
>> believe to be caused by this function, or are you just inspecting the
>> disassembly to satisfy your own curiosity?
> 
> No actual problem traced here.
> 
> 
>> 
>>> Should we do it like this:
>>> __asm__ __volatile__ (
>>> " sext.w %[ov], %[ov] \n"
>>> ...
>> 
>> No, it’s unnecessary and prevents optimisation.
>> 
>> Jess
>> 
>>> Thanks,
>>> Yunhui
>>> 
>>> _______________________________________________
>>> linux-riscv mailing list
>>> linux-riscv@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-riscv
>> 
> 
> Thanks,
> Yunhui