arch/riscv/include/asm/vdso/getrandom.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
As recently pointed out by Thomas, if a register is forced for two
different register variables, among them one is used as "+" (both input
and output) and another is only used as input, Clang would treat the
conflicting input parameters as undefined behaviour and optimize away
the argument assignment.
Per an example in the GCC documentation, for this purpose we can use "="
(only output) for the output, and "0" for the input for that we must
reuse the same register as the output. I'm not sure if using a simple
"r" for the input is safe or not here, so just follow the documentation.
Link: https://lore.kernel.org/all/20250603-loongarch-vdso-syscall-v1-1-6d12d6dfbdd0@linutronix.de/
Link: https://gcc.gnu.org/onlinedocs/gcc-15.1.0/gcc/Local-Register-Variables.html
Cc: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
Cc: Nathan Chancellor <nathan@kernel.org>
Signed-off-by: Xi Ruoyao <xry111@xry111.site>
---
arch/riscv/include/asm/vdso/getrandom.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/riscv/include/asm/vdso/getrandom.h b/arch/riscv/include/asm/vdso/getrandom.h
index 8dc92441702a..0d84a38e79da 100644
--- a/arch/riscv/include/asm/vdso/getrandom.h
+++ b/arch/riscv/include/asm/vdso/getrandom.h
@@ -18,8 +18,8 @@ static __always_inline ssize_t getrandom_syscall(void *_buffer, size_t _len, uns
register unsigned int flags asm("a2") = _flags;
asm volatile ("ecall\n"
- : "+r" (ret)
- : "r" (nr), "r" (buffer), "r" (len), "r" (flags)
+ : "=r" (ret)
+ : "r" (nr), "0" (buffer), "r" (len), "r" (flags)
: "memory");
return ret;
base-commit: dc5240f09bca7b5fc72ad8894d6b9321bce51139
--
2.49.0Hi Xi,
On Wed, Jun 04, 2025 at 11:09:52PM +0800, Xi Ruoyao wrote:
> As recently pointed out by Thomas, if a register is forced for two
> different register variables, among them one is used as "+" (both input
> and output) and another is only used as input, Clang would treat the
> conflicting input parameters as undefined behaviour and optimize away
> the argument assignment.
Good catch, I forgot to check the getrandom() implementations.
> Per an example in the GCC documentation, for this purpose we can use "="
> (only output) for the output, and "0" for the input for that we must
> reuse the same register as the output. I'm not sure if using a simple
> "r" for the input is safe or not here, so just follow the documentation.
This looks good in general. However the usage of the "0" constraint now differs
from the "r" constraint used by all other vDSO implementations, even the RISC-V
gettimeofday() itself.
I'd like to hold off applying this commit and wait for the conclusion of the
discussion linked below and then implement the result of that everywhere.
> Link: https://lore.kernel.org/all/20250603-loongarch-vdso-syscall-v1-1-6d12d6dfbdd0@linutronix.de/
> Link: https://gcc.gnu.org/onlinedocs/gcc-15.1.0/gcc/Local-Register-Variables.html
> Cc: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Signed-off-by: Xi Ruoyao <xry111@xry111.site>
> ---
> arch/riscv/include/asm/vdso/getrandom.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/include/asm/vdso/getrandom.h b/arch/riscv/include/asm/vdso/getrandom.h
> index 8dc92441702a..0d84a38e79da 100644
> --- a/arch/riscv/include/asm/vdso/getrandom.h
> +++ b/arch/riscv/include/asm/vdso/getrandom.h
> @@ -18,8 +18,8 @@ static __always_inline ssize_t getrandom_syscall(void *_buffer, size_t _len, uns
> register unsigned int flags asm("a2") = _flags;
>
> asm volatile ("ecall\n"
> - : "+r" (ret)
> - : "r" (nr), "r" (buffer), "r" (len), "r" (flags)
> + : "=r" (ret)
> + : "r" (nr), "0" (buffer), "r" (len), "r" (flags)
> : "memory");
>
> return ret;
>
> base-commit: dc5240f09bca7b5fc72ad8894d6b9321bce51139
> --
> 2.49.0
As recently pointed out by Thomas, if a register is forced for two
different register variables, among them one is used as "+" (both input
and output) and another is only used as input, Clang would treat the
conflicting input parameters as undefined behaviour and optimize away
the argument assignment.
Per an example in the GCC documentation, for this purpose we can use "="
(only output) for the output, and "0" for the input for that we must
reuse the same register as the output. And GCC developers have
confirmed using a simple "r" (that we use for most vDSO implementations)
instead of "0" is also fine.
Link: https://lore.kernel.org/all/20250603-loongarch-vdso-syscall-v1-1-6d12d6dfbdd0@linutronix.de/
Link: https://gcc.gnu.org/onlinedocs/gcc-15.1.0/gcc/Local-Register-Variables.html
Link: https://gcc.gnu.org/pipermail/gcc-help/2025-June/144266.html
Cc: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
Cc: Nathan Chancellor <nathan@kernel.org>
Signed-off-by: Xi Ruoyao <xry111@xry111.site>
---
v1 -> v2: Keep using "r" for buffer to follow the existing convention
(that the GCC developers have confirmed fine).
arch/riscv/include/asm/vdso/getrandom.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/riscv/include/asm/vdso/getrandom.h b/arch/riscv/include/asm/vdso/getrandom.h
index 8dc92441702a..c6d66895c1f5 100644
--- a/arch/riscv/include/asm/vdso/getrandom.h
+++ b/arch/riscv/include/asm/vdso/getrandom.h
@@ -18,7 +18,7 @@ static __always_inline ssize_t getrandom_syscall(void *_buffer, size_t _len, uns
register unsigned int flags asm("a2") = _flags;
asm volatile ("ecall\n"
- : "+r" (ret)
+ : "=r" (ret)
: "r" (nr), "r" (buffer), "r" (len), "r" (flags)
: "memory");
base-commit: dc5240f09bca7b5fc72ad8894d6b9321bce51139
--
2.49.0
On 6/6/25 02:24, Xi Ruoyao wrote:
> As recently pointed out by Thomas, if a register is forced for two
> different register variables, among them one is used as "+" (both input
> and output) and another is only used as input, Clang would treat the
> conflicting input parameters as undefined behaviour and optimize away
> the argument assignment.
>
> Per an example in the GCC documentation, for this purpose we can use "="
> (only output) for the output, and "0" for the input for that we must
> reuse the same register as the output. And GCC developers have
> confirmed using a simple "r" (that we use for most vDSO implementations)
> instead of "0" is also fine.
>
> Link: https://lore.kernel.org/all/20250603-loongarch-vdso-syscall-v1-1-6d12d6dfbdd0@linutronix.de/
> Link: https://gcc.gnu.org/onlinedocs/gcc-15.1.0/gcc/Local-Register-Variables.html
> Link: https://gcc.gnu.org/pipermail/gcc-help/2025-June/144266.html
> Cc: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Signed-off-by: Xi Ruoyao <xry111@xry111.site>
> ---
>
> v1 -> v2: Keep using "r" for buffer to follow the existing convention
> (that the GCC developers have confirmed fine).
>
> arch/riscv/include/asm/vdso/getrandom.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/riscv/include/asm/vdso/getrandom.h b/arch/riscv/include/asm/vdso/getrandom.h
> index 8dc92441702a..c6d66895c1f5 100644
> --- a/arch/riscv/include/asm/vdso/getrandom.h
> +++ b/arch/riscv/include/asm/vdso/getrandom.h
> @@ -18,7 +18,7 @@ static __always_inline ssize_t getrandom_syscall(void *_buffer, size_t _len, uns
> register unsigned int flags asm("a2") = _flags;
>
> asm volatile ("ecall\n"
> - : "+r" (ret)
> + : "=r" (ret)
> : "r" (nr), "r" (buffer), "r" (len), "r" (flags)
> : "memory");
My 2 cents as I've dabbled into this for ARC glibc syscall macros [1] where r0
is both the first syscall/function arg and also the function/syscall return.
The v2 approach still keeps 2 different variables in same local reg which has
potential for any future compiler shenanigans.
Segher's example avoided specifying the same reg.
What about something like the following: seems to generate the right code (with
gcc 15)
register long ret asm("a0");
register long nr asm("a7") = __NR_getrandom;
register size_t len asm("a1") = _len;
register unsigned int flags asm("a2") = _flags;
ret = (unsigned long) _buffer;
asm volatile ("ecall\n"
: "+r" (ret) // keep "+r"
for input _buffer / output ret
: "r" (nr), "r" (len), "r" (flags)
: "memory");
return ret;
Thx,
-Vineet
[1] https://github.com/bminor/glibc/blob/master/sysdeps/unix/sysv/linux/arc/sysdep.h
On Fri, 2025-06-06 at 15:01 -0700, Vineet Gupta wrote:
> On 6/6/25 02:24, Xi Ruoyao wrote:
> > As recently pointed out by Thomas, if a register is forced for two
> > different register variables, among them one is used as "+" (both input
> > and output) and another is only used as input, Clang would treat the
> > conflicting input parameters as undefined behaviour and optimize away
> > the argument assignment.
> >
> > Per an example in the GCC documentation, for this purpose we can use "="
> > (only output) for the output, and "0" for the input for that we must
> > reuse the same register as the output. And GCC developers have
> > confirmed using a simple "r" (that we use for most vDSO implementations)
> > instead of "0" is also fine.
> >
> > Link: https://lore.kernel.org/all/20250603-loongarch-vdso-syscall-v1-1-6d12d6dfbdd0@linutronix.de/
> > Link: https://gcc.gnu.org/onlinedocs/gcc-15.1.0/gcc/Local-Register-Variables.html
> > Link: https://gcc.gnu.org/pipermail/gcc-help/2025-June/144266.html
> > Cc: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> > Cc: Nathan Chancellor <nathan@kernel.org>
> > Signed-off-by: Xi Ruoyao <xry111@xry111.site>
> > ---
> >
> > v1 -> v2: Keep using "r" for buffer to follow the existing convention
> > (that the GCC developers have confirmed fine).
> >
> > arch/riscv/include/asm/vdso/getrandom.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/riscv/include/asm/vdso/getrandom.h b/arch/riscv/include/asm/vdso/getrandom.h
> > index 8dc92441702a..c6d66895c1f5 100644
> > --- a/arch/riscv/include/asm/vdso/getrandom.h
> > +++ b/arch/riscv/include/asm/vdso/getrandom.h
> > @@ -18,7 +18,7 @@ static __always_inline ssize_t getrandom_syscall(void *_buffer, size_t _len, uns
> > register unsigned int flags asm("a2") = _flags;
> >
> > asm volatile ("ecall\n"
> > - : "+r" (ret)
> > + : "=r" (ret)
> > : "r" (nr), "r" (buffer), "r" (len), "r" (flags)
> > : "memory");
>
> My 2 cents as I've dabbled into this for ARC glibc syscall macros [1] where r0
> is both the first syscall/function arg and also the function/syscall return.
>
> The v2 approach still keeps 2 different variables in same local reg which has
> potential for any future compiler shenanigans.
> Segher's example avoided specifying the same reg.
> What about something like the following: seems to generate the right code (with
> gcc 15)
>
> register long ret asm("a0");
Then it would be better to rename this variable to just "a0". And I
guess Thomas doesn't want a new convention different from all other
syscall wrappers in vDSO...
> register long nr asm("a7") = __NR_getrandom;
> register size_t len asm("a1") = _len;
> register unsigned int flags asm("a2") = _flags;
> ret = (unsigned long) _buffer;
>
> asm volatile ("ecall\n"
> : "+r" (ret) // keep "+r"
> for input _buffer / output ret
> : "r" (nr), "r" (len), "r" (flags)
> : "memory");
>
> return ret;
>
> Thx,
> -Vineet
>
> [1] https://github.com/bminor/glibc/blob/master/sysdeps/unix/sysv/linux/arc/sysdep.h
--
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University
On Sat, Jun 07, 2025 at 10:16:34PM +0800, Xi Ruoyao wrote:
> On Fri, 2025-06-06 at 15:01 -0700, Vineet Gupta wrote:
> > On 6/6/25 02:24, Xi Ruoyao wrote:
> > > As recently pointed out by Thomas, if a register is forced for two
> > > different register variables, among them one is used as "+" (both input
> > > and output) and another is only used as input, Clang would treat the
> > > conflicting input parameters as undefined behaviour and optimize away
> > > the argument assignment.
> > >
> > > Per an example in the GCC documentation, for this purpose we can use "="
> > > (only output) for the output, and "0" for the input for that we must
> > > reuse the same register as the output. And GCC developers have
> > > confirmed using a simple "r" (that we use for most vDSO implementations)
> > > instead of "0" is also fine.
> > >
> > > Link: https://lore.kernel.org/all/20250603-loongarch-vdso-syscall-v1-1-6d12d6dfbdd0@linutronix.de/
> > > Link: https://gcc.gnu.org/onlinedocs/gcc-15.1.0/gcc/Local-Register-Variables.html
> > > Link: https://gcc.gnu.org/pipermail/gcc-help/2025-June/144266.html
> > > Cc: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> > > Cc: Nathan Chancellor <nathan@kernel.org>
> > > Signed-off-by: Xi Ruoyao <xry111@xry111.site>
> > > ---
> > >
> > > v1 -> v2: Keep using "r" for buffer to follow the existing convention
> > > (that the GCC developers have confirmed fine).
> > >
> > > arch/riscv/include/asm/vdso/getrandom.h | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/arch/riscv/include/asm/vdso/getrandom.h b/arch/riscv/include/asm/vdso/getrandom.h
> > > index 8dc92441702a..c6d66895c1f5 100644
> > > --- a/arch/riscv/include/asm/vdso/getrandom.h
> > > +++ b/arch/riscv/include/asm/vdso/getrandom.h
> > > @@ -18,7 +18,7 @@ static __always_inline ssize_t getrandom_syscall(void *_buffer, size_t _len, uns
> > > register unsigned int flags asm("a2") = _flags;
> > >
> > > asm volatile ("ecall\n"
> > > - : "+r" (ret)
> > > + : "=r" (ret)
> > > : "r" (nr), "r" (buffer), "r" (len), "r" (flags)
> > > : "memory");
> >
> > My 2 cents as I've dabbled into this for ARC glibc syscall macros [1] where r0
> > is both the first syscall/function arg and also the function/syscall return.
> >
> > The v2 approach still keeps 2 different variables in same local reg which has
> > potential for any future compiler shenanigans.
> > Segher's example avoided specifying the same reg.
> > What about something like the following: seems to generate the right code (with
> > gcc 15)
> >
> > register long ret asm("a0");
>
> Then it would be better to rename this variable to just "a0". And I
> guess Thomas doesn't want a new convention different from all other
> syscall wrappers in vDSO...
Indeed. I want to keep it consistent. Especially for a bugfix.
Speaking of which, IMO this patch should have a Fixes tag.
Then we could start a new discussion about changing it to something else everywhere.
Although I don't think that the single-variable variant is better.
> > register long nr asm("a7") = __NR_getrandom;
> > register size_t len asm("a1") = _len;
> > register unsigned int flags asm("a2") = _flags;
> > ret = (unsigned long) _buffer;
> >
> > asm volatile ("ecall\n"
> > : "+r" (ret) // keep "+r"
> > for input _buffer / output ret
> > : "r" (nr), "r" (len), "r" (flags)
> > : "memory");
> >
> > return ret;
> >
> > Thx,
> > -Vineet
> >
> > [1] https://github.com/bminor/glibc/blob/master/sysdeps/unix/sysv/linux/arc/sysdep.h
>
> --
> Xi Ruoyao <xry111@xry111.site>
> School of Aerospace Science and Technology, Xidian University
Hi,
On 6/10/25 09:11, Thomas Weißschuh wrote:
> On Sat, Jun 07, 2025 at 10:16:34PM +0800, Xi Ruoyao wrote:
>> On Fri, 2025-06-06 at 15:01 -0700, Vineet Gupta wrote:
>>> On 6/6/25 02:24, Xi Ruoyao wrote:
>>>> As recently pointed out by Thomas, if a register is forced for two
>>>> different register variables, among them one is used as "+" (both input
>>>> and output) and another is only used as input, Clang would treat the
>>>> conflicting input parameters as undefined behaviour and optimize away
>>>> the argument assignment.
>>>>
>>>> Per an example in the GCC documentation, for this purpose we can use "="
>>>> (only output) for the output, and "0" for the input for that we must
>>>> reuse the same register as the output. And GCC developers have
>>>> confirmed using a simple "r" (that we use for most vDSO implementations)
>>>> instead of "0" is also fine.
>>>>
>>>> Link: https://lore.kernel.org/all/20250603-loongarch-vdso-syscall-v1-1-6d12d6dfbdd0@linutronix.de/
>>>> Link: https://gcc.gnu.org/onlinedocs/gcc-15.1.0/gcc/Local-Register-Variables.html
>>>> Link: https://gcc.gnu.org/pipermail/gcc-help/2025-June/144266.html
>>>> Cc: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
>>>> Cc: Nathan Chancellor <nathan@kernel.org>
>>>> Signed-off-by: Xi Ruoyao <xry111@xry111.site>
>>>> ---
>>>>
>>>> v1 -> v2: Keep using "r" for buffer to follow the existing convention
>>>> (that the GCC developers have confirmed fine).
>>>>
>>>> arch/riscv/include/asm/vdso/getrandom.h | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/riscv/include/asm/vdso/getrandom.h b/arch/riscv/include/asm/vdso/getrandom.h
>>>> index 8dc92441702a..c6d66895c1f5 100644
>>>> --- a/arch/riscv/include/asm/vdso/getrandom.h
>>>> +++ b/arch/riscv/include/asm/vdso/getrandom.h
>>>> @@ -18,7 +18,7 @@ static __always_inline ssize_t getrandom_syscall(void *_buffer, size_t _len, uns
>>>> register unsigned int flags asm("a2") = _flags;
>>>>
>>>> asm volatile ("ecall\n"
>>>> - : "+r" (ret)
>>>> + : "=r" (ret)
>>>> : "r" (nr), "r" (buffer), "r" (len), "r" (flags)
>>>> : "memory");
>>> My 2 cents as I've dabbled into this for ARC glibc syscall macros [1] where r0
>>> is both the first syscall/function arg and also the function/syscall return.
>>>
>>> The v2 approach still keeps 2 different variables in same local reg which has
>>> potential for any future compiler shenanigans.
>>> Segher's example avoided specifying the same reg.
>>> What about something like the following: seems to generate the right code (with
>>> gcc 15)
>>>
>>> register long ret asm("a0");
>> Then it would be better to rename this variable to just "a0". And I
>> guess Thomas doesn't want a new convention different from all other
>> syscall wrappers in vDSO...
> Indeed. I want to keep it consistent. Especially for a bugfix.
> Speaking of which, IMO this patch should have a Fixes tag.
Yes, here it is:
Fixes: ee0d03053e70 ("RISC-V: vDSO: Wire up getrandom() vDSO
implementation")
>
> Then we could start a new discussion about changing it to something else everywhere.
> Although I don't think that the single-variable variant is better.
Vineet feel free to propose something for all architectures if you think
that's better.
For now, I'll merge this version for inclusion in -rc2,
Thanks,
Alex
>
>>> register long nr asm("a7") = __NR_getrandom;
>>> register size_t len asm("a1") = _len;
>>> register unsigned int flags asm("a2") = _flags;
>>> ret = (unsigned long) _buffer;
>>>
>>> asm volatile ("ecall\n"
>>> : "+r" (ret) // keep "+r"
>>> for input _buffer / output ret
>>> : "r" (nr), "r" (len), "r" (flags)
>>> : "memory");
>>>
>>> return ret;
>>>
>>> Thx,
>>> -Vineet
>>>
>>> [1] https://github.com/bminor/glibc/blob/master/sysdeps/unix/sysv/linux/arc/sysdep.h
>> --
>> Xi Ruoyao <xry111@xry111.site>
>> School of Aerospace Science and Technology, Xidian University
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
On Fri, Jun 06, 2025 at 05:24:44PM +0800, Xi Ruoyao wrote:
> As recently pointed out by Thomas, if a register is forced for two
> different register variables, among them one is used as "+" (both input
> and output) and another is only used as input, Clang would treat the
> conflicting input parameters as undefined behaviour and optimize away
> the argument assignment.
>
> Per an example in the GCC documentation, for this purpose we can use "="
> (only output) for the output, and "0" for the input for that we must
> reuse the same register as the output. And GCC developers have
> confirmed using a simple "r" (that we use for most vDSO implementations)
> instead of "0" is also fine.
The wording is a bit confusing. Maybe this is better:
Instead use "=r" (only output) for the output parameter and "r" (only input)
for the input parameter.
While the example from the GCC documentation uses "0" for the input parameter,
this is not necessary as confirmed by the GCC developers and "r"
matches what the other architectures' vDSO implementations are using.
> Link: https://lore.kernel.org/all/20250603-loongarch-vdso-syscall-v1-1-6d12d6dfbdd0@linutronix.de/
> Link: https://gcc.gnu.org/onlinedocs/gcc-15.1.0/gcc/Local-Register-Variables.html
> Link: https://gcc.gnu.org/pipermail/gcc-help/2025-June/144266.html
> Cc: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
In any case, thanks and
Reviewed-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Signed-off-by: Xi Ruoyao <xry111@xry111.site>
> ---
>
> v1 -> v2: Keep using "r" for buffer to follow the existing convention
> (that the GCC developers have confirmed fine).
>
> arch/riscv/include/asm/vdso/getrandom.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/riscv/include/asm/vdso/getrandom.h b/arch/riscv/include/asm/vdso/getrandom.h
> index 8dc92441702a..c6d66895c1f5 100644
> --- a/arch/riscv/include/asm/vdso/getrandom.h
> +++ b/arch/riscv/include/asm/vdso/getrandom.h
> @@ -18,7 +18,7 @@ static __always_inline ssize_t getrandom_syscall(void *_buffer, size_t _len, uns
> register unsigned int flags asm("a2") = _flags;
>
> asm volatile ("ecall\n"
> - : "+r" (ret)
> + : "=r" (ret)
> : "r" (nr), "r" (buffer), "r" (len), "r" (flags)
> : "memory");
>
>
> base-commit: dc5240f09bca7b5fc72ad8894d6b9321bce51139
> --
> 2.49.0
>
© 2016 - 2025 Red Hat, Inc.