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

["Thomas Weißschuh" <thomas.weissschuh@linutronix.de>]

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