Re: [PATCH v2] RISC-V: vDSO: Correct inline assembly constraints in the getrandom syscall wrapper

[Xi Ruoyao]

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@xxxxxxxxxxxxx/
> > 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@xxxxxxxxxxxxx>
> > Cc: Nathan Chancellor <nathan@xxxxxxxxxx>
> > Signed-off-by: Xi Ruoyao <xry111@xxxxxxxxxxx>
> > ---
> > 
> > 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@xxxxxxxxxxx>
School of Aerospace Science and Technology, Xidian University