Re: [PATCH v2 1/4] s390x: Add the clone3 wrapper

[Stefan Liebler <stli@linux.ibm.com>]

Hi Adhemerval,

I have applied your patches and tested it on s390x. I've ensured that
the wrapper is now used for pthread_create/posix_spawn and the testsuite
runs fine. Nevertheless I have some minor comments: see below.

And one more question:
Is there a reason, why the wrapper is only added to s390x (64bit), but
not for s390 (31bit)? Should there be an #undef __ASSUME_CLONE3?
According to the kernel commit it is also available for compat-mode:
"s390: wire up clone3 system call"
https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?id=5518aed82d2abd97f8d3ec91d8ba455d939e8cd1

On 10.07.23 20:40, Adhemerval Zanella via Libc-alpha wrote:
> It follows the internal signature:
> 
>   extern int clone3 (struct clone_args *__cl_args, size_t __size,
>  int (*__func) (void *__arg), void *__arg);
> 
> Checked on s390x-linux-gnu.
> ---
>  sysdeps/unix/sysv/linux/s390/s390-64/clone3.S | 81 +++++++++++++++++++
>  sysdeps/unix/sysv/linux/s390/sysdep.h         |  1 +
>  2 files changed, 82 insertions(+)
>  create mode 100644 sysdeps/unix/sysv/linux/s390/s390-64/clone3.S
> 
> diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/clone3.S b/sysdeps/unix/sysv/linux/s390/s390-64/clone3.S
> new file mode 100644
> index 0000000000..417fdcb785
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/s390/s390-64/clone3.S
> @@ -0,0 +1,81 @@
> +/* The clone3 syscall wrapper.  Linux/s390x version.
> +   Copyright (C) 2023 Free Software Foundation, Inc.
> +
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <sysdep.h>
> +#define _ERRNO_H        1
> +#include <bits/errno.h>
> +
> +/* The userland implementation is:
> +   int clone3 (struct clone_args *cl_args, size_t size,
> +               int (*func)(void *arg), void *arg);
> +
> +   the kernel entry is:
> +   int clone3 (struct clone_args *cl_args, size_t size);
> +
> +   The parameters are passed in registers from userland:
> +   r2: cl_args
> +   r3: size
> +   r4: func
> +   r5: arg  */
> +
> +        .text
> +ENTRY(__clone3)
> +	stg	%r6, 48(%r15)
Add cfi_offset (%r6,-112)


> +
> +	/* Sanity check args.  */
> +	ltgr	%r2, %r2
> +	je	error
> +	ltgr	%r6, %r4
> +	je	error
> +
> +	/* Do the system call, the kernel expects:
> +	   r1: system call number
> +	   r2: cl_args
> +	   r3: size  */
> +	lghi	%r1, SYS_ify(clone3)
> +	svc	0
> +	ltgr	%r2,%r2			/* check return code */
> +	jz	thread_start
> +	lg	%r6, 48(%r15)
> +	jgm	SYSCALL_ERROR_LABEL
> +	br	%r14
> +error:
> +	lghi	%r2,-EINVAL
> +	jg	SYSCALL_ERROR_LABEL
> +PSEUDO_END (__clone3)
> +
> +	.align 4
The ENTRY-macro from sysdep.h is using an alignment of 16 bytes. Can you
please also use it here?

> +	.type thread_start, %function
> +thread_start:
> +	cfi_startproc
> +	/* Mark r14 as undefined in order to stop unwinding here.  */
> +	cfi_undefined (r14)
> +
> +	/* func is in gpr 6, arg in gpr 5.  */
> +	lgr	%r2, %r5
> +	aghi	%r15, -160
> +	xc	0(8,%r15),0(%r15)
> +	basr	%r14, %r6
> +
> +	DO_CALL (exit, 1)
> +	cfi_endproc
> +	.size thread_start, .-thread_start
If you want, you can use ASM_SIZE_DIRECTIVE (thread_start) from sysdep.h
here, which is also used in END-macros.
> +
> +libc_hidden_def (__clone3)
> +weak_alias (__clone3, clone3)
> diff --git a/sysdeps/unix/sysv/linux/s390/sysdep.h b/sysdeps/unix/sysv/linux/s390/sysdep.h
> index 3bde8ba19c..b099fea9c6 100644
> --- a/sysdeps/unix/sysv/linux/s390/sysdep.h
> +++ b/sysdeps/unix/sysv/linux/s390/sysdep.h
> @@ -72,6 +72,7 @@
>  #ifdef __s390x__
>  #define HAVE_CLOCK_GETRES64_VSYSCALL	"__kernel_clock_getres"
>  #define HAVE_CLOCK_GETTIME64_VSYSCALL	"__kernel_clock_gettime"
> +#define HAVE_CLONE3_WRAPPER		1
>  #else
>  #define HAVE_CLOCK_GETRES_VSYSCALL	"__kernel_clock_getres"
>  #define HAVE_CLOCK_GETTIME_VSYSCALL	"__kernel_clock_gettime"

Thanks,
Stefan