| Message ID | mvmed0k4prh.fsf@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | riscv/atomic: Do proper sign extension also for unsigned in arch_cmpxchg | expand |
| Context | Check | Description |
|---|---|---|
| bjorn/pre-ci_am | success | Success |
| bjorn/build-rv32-defconfig | success | build-rv32-defconfig |
| bjorn/build-rv64-clang-allmodconfig | success | build-rv64-clang-allmodconfig |
| bjorn/build-rv64-gcc-allmodconfig | success | build-rv64-gcc-allmodconfig |
| bjorn/build-rv64-nommu-k210-defconfig | success | build-rv64-nommu-k210-defconfig |
| bjorn/build-rv64-nommu-k210-virt | success | build-rv64-nommu-k210-virt |
| bjorn/checkpatch | success | checkpatch |
| bjorn/dtb-warn-rv64 | success | dtb-warn-rv64 |
| bjorn/header-inline | success | header-inline |
| bjorn/kdoc | success | kdoc |
| bjorn/module-param | success | module-param |
| bjorn/verify-fixes | success | verify-fixes |
| bjorn/verify-signedoff | success | verify-signedoff |
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
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>
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.
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?
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.
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
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, \
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
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.
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.
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
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
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
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
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, \
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(-)