[PATCH v4 0/6] Optimize posix_spawn signal setup with clone3

[PATCH v4 0/6] Optimize posix_spawn signal setup with clone3

[Adhemerval Zanella]

Currently posix_spawn has to issue at least NSIG sigaction syscalls
to obtain current signal disposition and more if they there are not
either SIG_IGN or SIG_DFL.  The new clone3 CLONE_CLEAR_SIGHAND flags
introduced with Linux 5.5 resets all signal handlers of the child not
set to SIG_IGN to SIG_DFL, thus allowing posix_spawn to skip this
preparation phase.

The exception is the signals defined by posix_spawnattr_setsigdefault
when POSIX_SPAWN_SETSIGDEF is set (since they can be SIG_IGN).  In
this case posix_spawn helper process needs to issue a sigction to
set the signal disposition to SIG_DFL.

The patchset also adds clone3 implementation for aarch64.  The clone3
for x86_64 is change to not align the stack, since this is not
async-signal safe (although current calls does mask/unmask signals).

* Changes from v3:
  - Changes commit message and comments wording.
  - Removed powerpc, s390x, riscv, arm, and mips clone3 optimizations.

* Changes from v2:
  - Improve clone3 internal documentation.

* Changes from v1:
  - Extend clone3 documentation.
  - Remove x86_64 stack alignment on clone3 child.
  - Fixed powerpc64 missing initial stack frame.
  - Fixed missing save/restore r7 for arm.

Adhemerval Zanella (6):
  linux: Do not reset signal handler in posix_spawn if it is already
    SIG_DFL
  linux: Extend internal clone3 documentation
  Linux: Do not align the stack for __clone3
  linux: Add clone3 CLONE_CLEAR_SIGHAND optimization to posix_spawn
  aarch64: Add the clone3 wrapper
  Linux: optimize clone3 internal usage

 include/clone_internal.h                      |  37 +++-
 posix/Makefile                                |   3 +-
 posix/tst-spawn7.c                            | 179 ++++++++++++++++++
 sysdeps/unix/sysv/linux/Makefile              |   3 +-
 sysdeps/unix/sysv/linux/aarch64/clone3.S      |  85 +++++++++
 sysdeps/unix/sysv/linux/aarch64/sysdep.h      |   2 +
 sysdeps/unix/sysv/linux/clone-internal.c      |  59 ++++--
 sysdeps/unix/sysv/linux/clone3.h              |  15 +-
 sysdeps/unix/sysv/linux/kernel-features.h     |   9 +
 sysdeps/unix/sysv/linux/spawni.c              |  33 +++-
 .../sysv/linux/tst-misalign-clone-internal.c  |  74 --------
 sysdeps/unix/sysv/linux/x86_64/clone3.S       |   3 -
 12 files changed, 384 insertions(+), 118 deletions(-)
 create mode 100644 posix/tst-spawn7.c
 create mode 100644 sysdeps/unix/sysv/linux/aarch64/clone3.S
 delete mode 100644 sysdeps/unix/sysv/linux/tst-misalign-clone-internal.c

-- 
2.34.1

[PATCH v4 1/6] linux: Do not reset signal handler in posix_spawn if it is already SIG_DFL

[Adhemerval Zanella]

There is no need to issue another sigaction if the disposition is
already SIG_DFL.

Checked on x86_64-linux-gnu.
---
 sysdeps/unix/sysv/linux/spawni.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
index b7a6fe320d..a1a14a58ae 100644
--- a/sysdeps/unix/sysv/linux/spawni.c
+++ b/sysdeps/unix/sysv/linux/spawni.c
@@ -129,7 +129,7 @@ __spawni_child (void *arguments)
 	  else
 	    {
 	      __libc_sigaction (sig, 0, &sa);
-	      if (sa.sa_handler == SIG_IGN)
+	      if (sa.sa_handler == SIG_IGN || sa.sa_handler == SIG_DFL)
 		continue;
 	      sa.sa_handler = SIG_DFL;
 	    }
-- 
2.34.1

[PATCH v4 2/6] linux: Extend internal clone3 documentation

[Adhemerval Zanella]

Different than kernel, clone3 returns EINVAL for NULL struct
clone_args or function pointer.  This is similar to clone
interface that return EINVAL for NULL function argument.

It also clean up the Linux clone3.h interface, since it not
currently exported.

Checked on x86_64-linux-gnu.
---
 include/clone_internal.h         | 24 +++++++++++++++++++-----
 sysdeps/unix/sysv/linux/clone3.h | 10 +---------
 2 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/include/clone_internal.h b/include/clone_internal.h
index 4b23ef33ce..73b8114df4 100644
--- a/include/clone_internal.h
+++ b/include/clone_internal.h
@@ -1,10 +1,24 @@
-#ifndef _CLONE3_H
-#include_next <clone3.h>
+#ifndef _CLONE_INTERNAL_H
+#define _CLONE_INTERNAL_H
 
-extern __typeof (clone3) __clone3;
+#include <clone3.h>
 
-/* The internal wrapper of clone/clone2 and clone3.  If __clone3 returns
-   -1 with ENOSYS, fall back to clone or clone2.  */
+/* The clone3 syscall provides a superset of the functionality of the clone
+   interface.  The kernel might extend __CL_ARGS struct in the future, with
+   each version with a diffent __SIZE.  If the child is created, it will
+   start __FUNC function with __ARG arguments.
+
+   Different than kernel, the implementation also returns EINVAL for an
+   invalid NULL __CL_ARGS or __FUNC (similar to __clone).
+
+   This function is only implemented if the ABI defines HAVE_CLONE3_WRAPPER.
+*/
+extern int __clone3 (struct clone_args *__cl_args, size_t __size,
+		     int (*__func) (void *__arg), void *__arg);
+
+/* The internal wrapper of clone/clone2 and clone3.  Different than __clone3,
+   it will align the stack if required.  If __clone3 returns -1 with ENOSYS,
+   fall back to clone or clone2.  */
 extern int __clone_internal (struct clone_args *__cl_args,
 			     int (*__func) (void *__arg), void *__arg);
 
diff --git a/sysdeps/unix/sysv/linux/clone3.h b/sysdeps/unix/sysv/linux/clone3.h
index 42055ea981..e4d642e521 100644
--- a/sysdeps/unix/sysv/linux/clone3.h
+++ b/sysdeps/unix/sysv/linux/clone3.h
@@ -1,4 +1,4 @@
-/* The wrapper of clone3.
+/* The clone3 kernel interface definitions.
    Copyright (C) 2021-2023 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
 
@@ -23,8 +23,6 @@
 #include <stddef.h>
 #include <bits/types.h>
 
-__BEGIN_DECLS
-
 /* The unsigned 64-bit and 8-byte aligned integer type.  */
 typedef __U64_TYPE __aligned_uint64_t __attribute__ ((__aligned__ (8)));
 
@@ -58,10 +56,4 @@ struct clone_args
   __aligned_uint64_t cgroup;
 };
 
-/* The wrapper of clone3.  */
-extern int clone3 (struct clone_args *__cl_args, size_t __size,
-		   int (*__func) (void *__arg), void *__arg);
-
-__END_DECLS
-
 #endif /* clone3.h */
-- 
2.34.1

[PATCH v4 3/6] Linux: Do not align the stack for __clone3

[Adhemerval Zanella]

All internal callers of __clone3 should provide an already aligned
stack.  Removing the stack alignment in __clone3 is a net gain: it
simplifies the internal function contract (mask/unmask signals) along
with the arch-specific code.

Checked on x86_64-linux-gnu.
---
 include/clone_internal.h                      |  3 +
 .../sysv/linux/tst-misalign-clone-internal.c  | 74 -------------------
 sysdeps/unix/sysv/linux/x86_64/clone3.S       |  3 -
 3 files changed, 3 insertions(+), 77 deletions(-)
 delete mode 100644 sysdeps/unix/sysv/linux/tst-misalign-clone-internal.c

diff --git a/include/clone_internal.h b/include/clone_internal.h
index 73b8114df4..f8198d8059 100644
--- a/include/clone_internal.h
+++ b/include/clone_internal.h
@@ -11,6 +11,9 @@
    Different than kernel, the implementation also returns EINVAL for an
    invalid NULL __CL_ARGS or __FUNC (similar to __clone).
 
+   All callers are responsible for correctly aligning the stack.  The stack is
+   not aligned prior to the syscall (this differs from the exported __clone).
+
    This function is only implemented if the ABI defines HAVE_CLONE3_WRAPPER.
 */
 extern int __clone3 (struct clone_args *__cl_args, size_t __size,
diff --git a/sysdeps/unix/sysv/linux/tst-misalign-clone-internal.c b/sysdeps/unix/sysv/linux/tst-misalign-clone-internal.c
deleted file mode 100644
index 8b94a74819..0000000000
--- a/sysdeps/unix/sysv/linux/tst-misalign-clone-internal.c
+++ /dev/null
@@ -1,74 +0,0 @@
-/* Verify that __clone_internal properly aligns the child stack.
-   Copyright (C) 2021-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 <sched.h>
-#include <stdbool.h>
-#include <stdint.h>
-#include <stdio.h>
-#include <string.h>
-#include <sys/wait.h>
-#include <unistd.h>
-#include <libc-pointer-arith.h>
-#include <tst-stack-align.h>
-#include <clone_internal.h>
-#include <support/xunistd.h>
-#include <support/check.h>
-
-static int
-check_stack_alignment (void *arg)
-{
-  puts ("in f");
-
-  return TEST_STACK_ALIGN () ? 1 : 0;
-}
-
-static int
-do_test (void)
-{
-  puts ("in do_test");
-
-  if (TEST_STACK_ALIGN ())
-    FAIL_EXIT1 ("stack isn't aligned\n");
-
-#ifdef __ia64__
-# define STACK_SIZE (256 * 1024)
-#else
-# define STACK_SIZE (128 * 1024)
-#endif
-  char st[STACK_SIZE + 1];
-  /* NB: Align child stack to 1 byte.  */
-  char *stack = PTR_ALIGN_UP (&st[0], 2) + 1;
-  struct clone_args clone_args =
-    {
-      .stack = (uintptr_t) stack,
-      .stack_size = STACK_SIZE,
-    };
-  pid_t p = __clone_internal (&clone_args, check_stack_alignment, 0);
-
-  /* Clone must not fail.  */
-  TEST_VERIFY_EXIT (p != -1);
-
-  int e;
-  xwaitpid (p, &e, __WCLONE);
-  TEST_VERIFY (WIFEXITED (e));
-  TEST_COMPARE (WEXITSTATUS (e), 0);
-
-  return 0;
-}
-
-#include <support/test-driver.c>
diff --git a/sysdeps/unix/sysv/linux/x86_64/clone3.S b/sysdeps/unix/sysv/linux/x86_64/clone3.S
index bd4a834e46..802d56cbac 100644
--- a/sysdeps/unix/sysv/linux/x86_64/clone3.S
+++ b/sysdeps/unix/sysv/linux/x86_64/clone3.S
@@ -73,9 +73,6 @@ L(thread_start):
 	   the outermost frame obviously.  */
 	xorl	%ebp, %ebp
 
-	/* Align stack to 16 bytes per the x86-64 psABI.  */
-	and	$-16, %RSP_LP
-
 	/* Set up arguments for the function call.  */
 	mov	%R8_LP, %RDI_LP	/* Argument.  */
 	call	*%rdx		/* Call function.  */
-- 
2.34.1

[PATCH v4 4/6] linux: Add clone3 CLONE_CLEAR_SIGHAND optimization to posix_spawn

[Adhemerval Zanella]

The clone3 flag resets all signal handlers of the child not set to
SIG_IGN to SIG_DFL.  It allows to skip most of the sigaction calls
to setup child signal handling, where previously a posix_spawn
had to issue 2 times NSIG sigaction calls (one to obtain the current
disposition and another to set either SIG_DFL or SIG_IGN).

With POSIX_SPAWN_SETSIGDEF the child will setup the signal for the case
where the disposition is SIG_IGN.

The code must handle the fallback where clone3 is not available. This is
done by splitting __clone_internal_fallback from __clone_internal.

Checked on x86_64-linux-gnu.
---
 include/clone_internal.h                 |   5 +
 posix/Makefile                           |   3 +-
 posix/tst-spawn7.c                       | 179 +++++++++++++++++++++++
 sysdeps/unix/sysv/linux/Makefile         |   3 +-
 sysdeps/unix/sysv/linux/clone-internal.c |  37 +++--
 sysdeps/unix/sysv/linux/clone3.h         |   7 +
 sysdeps/unix/sysv/linux/spawni.c         |  31 ++--
 7 files changed, 238 insertions(+), 27 deletions(-)
 create mode 100644 posix/tst-spawn7.c

diff --git a/include/clone_internal.h b/include/clone_internal.h
index f8198d8059..3b6cd85f02 100644
--- a/include/clone_internal.h
+++ b/include/clone_internal.h
@@ -24,6 +24,11 @@ extern int __clone3 (struct clone_args *__cl_args, size_t __size,
    fall back to clone or clone2.  */
 extern int __clone_internal (struct clone_args *__cl_args,
 			     int (*__func) (void *__arg), void *__arg);
+/* The fallback code which calls clone/clone2 based on clone3 arguments.  */
+extern int __clone_internal_fallback (struct clone_args *__cl_args,
+				      int (*__func) (void *__arg),
+				      void *__arg)
+     attribute_hidden;
 
 #ifndef _ISOMAC
 libc_hidden_proto (__clone3)
diff --git a/posix/Makefile b/posix/Makefile
index 38a4279599..7e4f252a40 100644
--- a/posix/Makefile
+++ b/posix/Makefile
@@ -109,7 +109,7 @@ tests		:= test-errno tstgetopt testfnm runtests runptests \
 		   tst-glob-tilde test-ssize-max tst-spawn4 bug-regex37 \
 		   bug-regex38 tst-regcomp-truncated tst-spawn-chdir \
 		   tst-wordexp-nocmd tst-execveat tst-spawn5 \
-		   tst-sched_getaffinity tst-spawn6
+		   tst-sched_getaffinity tst-spawn6 tst-spawn7
 
 # Test for the glob symbol version that was replaced in glibc 2.27.
 ifeq ($(have-GLIBC_2.26)$(build-shared),yesyes)
@@ -291,6 +291,7 @@ tst-spawn-ARGS = -- $(host-test-program-cmd)
 tst-spawn-static-ARGS = $(tst-spawn-ARGS)
 tst-spawn5-ARGS = -- $(host-test-program-cmd)
 tst-spawn6-ARGS = -- $(host-test-program-cmd)
+tst-spawn7-ARGS = -- $(host-test-program-cmd)
 tst-dir-ARGS = `pwd` `cd $(common-objdir)/$(subdir); pwd` `cd $(common-objdir); pwd` $(objpfx)tst-dir
 tst-chmod-ARGS = $(objdir)
 tst-vfork3-ARGS = --test-dir=$(objpfx)
diff --git a/posix/tst-spawn7.c b/posix/tst-spawn7.c
new file mode 100644
index 0000000000..a1c0d9d942
--- /dev/null
+++ b/posix/tst-spawn7.c
@@ -0,0 +1,179 @@
+/* Tests for posix_spawn signal handling.
+   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
+   <http://www.gnu.org/licenses/>.  */
+
+#include <assert.h>
+#include <getopt.h>
+#include <spawn.h>
+#include <stdlib.h>
+#include <string.h>
+#include <support/check.h>
+#include <support/xsignal.h>
+#include <support/xunistd.h>
+#include <unistd.h>
+
+/* Nonzero if the program gets called via `exec'.  */
+#define CMDLINE_OPTIONS \
+  { "restart", no_argument, &restart, 1 },
+static int restart;
+
+/* Hold the four initial argument used to respawn the process, plus the extra
+   '--direct', '--restart', the check type ('SIG_IGN' or 'SIG_DFL'), and a
+   final NULL.  */
+static char *spargs[8];
+static int check_type_argc;
+
+/* Called on process re-execution.  */
+_Noreturn static void
+handle_restart (int argc, char *argv[])
+{
+  assert (argc == 1);
+
+  if (strcmp (argv[0], "SIG_DFL") == 0)
+    {
+      for (int i = 1; i < NSIG; i++)
+	{
+	  struct sigaction sa;
+	  int r = sigaction (i, NULL, &sa);
+	  /* Skip internal signals (such as SIGCANCEL).  */
+	  if (r == -1)
+	    continue;
+	  TEST_VERIFY_EXIT (sa.sa_handler == SIG_DFL);
+	}
+      exit (EXIT_SUCCESS);
+    }
+  else if (strcmp (argv[0], "SIG_IGN") == 0)
+    {
+      for (int i = 1; i < NSIG; i++)
+	{
+	  struct sigaction sa;
+	  int r = sigaction (i, NULL, &sa);
+	  /* Skip internal signals (such as SIGCANCEL).  */
+	  if (r == -1)
+	    continue;
+	  if (i == SIGUSR1 || i == SIGUSR2)
+	    TEST_VERIFY_EXIT (sa.sa_handler == SIG_IGN);
+	  else
+	    TEST_VERIFY_EXIT (sa.sa_handler == SIG_DFL);
+	}
+      exit (EXIT_SUCCESS);
+    }
+
+  exit (EXIT_FAILURE);
+}
+
+static void
+spawn_signal_test (const char *type, const posix_spawnattr_t *attr)
+{
+  spargs[check_type_argc] = (char*) type;
+
+  pid_t pid;
+  int status;
+
+  TEST_COMPARE (posix_spawn (&pid, spargs[0], NULL, attr, spargs, environ), 0);
+  TEST_COMPARE (xwaitpid (pid, &status, 0), pid);
+  TEST_VERIFY (WIFEXITED (status));
+  TEST_VERIFY (!WIFSIGNALED (status));
+  TEST_COMPARE (WEXITSTATUS (status), 0);
+}
+
+static void
+dummy_sa_handler (int)
+{
+}
+
+static void
+do_test_signals (void)
+{
+  {
+    /* Check if all signals handler are set to SIG_DFL on spawned process.  */
+    spawn_signal_test ("SIG_DFL", NULL);
+  }
+
+  {
+    /* Same as before, but set SIGUSR1 and SIGUSR2 to a handler different than
+       SIG_IGN or SIG_DFL.  */
+    struct sigaction sa = { 0 };
+    sa.sa_handler = dummy_sa_handler;
+    xsigaction (SIGUSR1, &sa, NULL);
+    xsigaction (SIGUSR2, &sa, NULL);
+    spawn_signal_test ("SIG_DFL", NULL);
+  }
+
+  {
+    /* Check if SIG_IGN is keep as is.  */
+    struct sigaction sa = { 0 };
+    sa.sa_handler = SIG_IGN;
+    xsigaction (SIGUSR1, &sa, NULL);
+    xsigaction (SIGUSR2, &sa, NULL);
+    spawn_signal_test ("SIG_IGN", NULL);
+  }
+
+  {
+    /* Check if SIG_IGN handlers are set to SIG_DFL.  */
+    posix_spawnattr_t attr;
+    posix_spawnattr_init (&attr);
+    sigset_t mask;
+    sigemptyset (&mask);
+    sigaddset (&mask, SIGUSR1);
+    sigaddset (&mask, SIGUSR2);
+    posix_spawnattr_setsigdefault (&attr, &mask);
+    posix_spawnattr_setflags (&attr, POSIX_SPAWN_SETSIGDEF);
+    spawn_signal_test ("SIG_DFL", &attr);
+    posix_spawnattr_destroy (&attr);
+  }
+}
+
+static int
+do_test (int argc, char *argv[])
+{
+  /* We must have either:
+
+     - one or four parameters if called initially:
+       + argv[1]: path for ld.so        optional
+       + argv[2]: "--library-path"      optional
+       + argv[3]: the library path      optional
+       + argv[4]: the application name
+
+     - six parameters left if called through re-execution:
+       + argv[1]: the application name
+       + argv[2]: check SIG_IGN/SIG_DFL.
+
+     * When built with --enable-hardcoded-path-in-tests or issued without
+       using the loader directly.  */
+
+  if (restart)
+    handle_restart (argc - 1, &argv[1]);
+
+  TEST_VERIFY_EXIT (argc == 2 || argc == 5);
+
+  int i;
+  for (i = 0; i < argc - 1; i++)
+    spargs[i] = argv[i + 1];
+  spargs[i++] = (char *) "--direct";
+  spargs[i++] = (char *) "--restart";
+  check_type_argc = i++;
+  spargs[i] = NULL;
+
+
+  do_test_signals ();
+
+  return 0;
+}
+
+#define TEST_FUNCTION_ARGV do_test
+#include <support/test-driver.c>
diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
index f298878e8f..f8bd12d991 100644
--- a/sysdeps/unix/sysv/linux/Makefile
+++ b/sysdeps/unix/sysv/linux/Makefile
@@ -269,8 +269,7 @@ tests-clone-internal = \
   tst-align-clone-internal \
   tst-clone2-internal \
   tst-clone3-internal \
-  tst-getpid1-internal \
-  tst-misalign-clone-internal
+  tst-getpid1-internal
 tests-internal += $(tests-clone-internal)
 tests-static += $(tests-clone-internal)
 
diff --git a/sysdeps/unix/sysv/linux/clone-internal.c b/sysdeps/unix/sysv/linux/clone-internal.c
index a8611772a2..e125c9125e 100644
--- a/sysdeps/unix/sysv/linux/clone-internal.c
+++ b/sysdeps/unix/sysv/linux/clone-internal.c
@@ -44,27 +44,15 @@ _Static_assert (sizeof (struct clone_args) == CLONE_ARGS_SIZE_VER2,
 		"sizeof (struct clone_args) != CLONE_ARGS_SIZE_VER2");
 
 int
-__clone_internal (struct clone_args *cl_args,
-		  int (*func) (void *arg), void *arg)
+__clone_internal_fallback (struct clone_args *cl_args,
+			   int (*func) (void *arg), void *arg)
 {
-  int ret;
-#ifdef HAVE_CLONE3_WRAPPER
-  /* Try clone3 first.  */
-  int saved_errno = errno;
-  ret = __clone3 (cl_args, sizeof (*cl_args), func, arg);
-  if (ret != -1 || errno != ENOSYS)
-    return ret;
-
-  /* NB: Restore errno since errno may be checked against non-zero
-     return value.  */
-  __set_errno (saved_errno);
-#endif
-
   /* Map clone3 arguments to clone arguments.  NB: No need to check
      invalid clone3 specific bits in flags nor exit_signal since this
      is an internal function.  */
   int flags = cl_args->flags | cl_args->exit_signal;
   void *stack = cast_to_pointer (cl_args->stack);
+  int ret;
 
 #ifdef __ia64__
   ret = __clone2 (func, stack, cl_args->stack_size,
@@ -88,4 +76,23 @@ __clone_internal (struct clone_args *cl_args,
   return ret;
 }
 
+
+int
+__clone_internal (struct clone_args *cl_args,
+		  int (*func) (void *arg), void *arg)
+{
+#ifdef HAVE_CLONE3_WRAPPER
+  int saved_errno = errno;
+  int ret = __clone3 (cl_args, sizeof (*cl_args), func, arg);
+  if (ret != -1 || errno != ENOSYS)
+    return ret;
+
+  /* NB: Restore errno since errno may be checked against non-zero
+     return value.  */
+  __set_errno (saved_errno);
+#endif
+
+  return __clone_internal_fallback (cl_args, func, arg);
+}
+
 libc_hidden_def (__clone_internal)
diff --git a/sysdeps/unix/sysv/linux/clone3.h b/sysdeps/unix/sysv/linux/clone3.h
index e4d642e521..451b156405 100644
--- a/sysdeps/unix/sysv/linux/clone3.h
+++ b/sysdeps/unix/sysv/linux/clone3.h
@@ -23,6 +23,13 @@
 #include <stddef.h>
 #include <bits/types.h>
 
+
+/* Flags for the clone3 syscall.  */
+#define CLONE_CLEAR_SIGHAND 0x100000000ULL /* Clear any signal handler and
+					      reset to SIG_DFL.  */
+#define CLONE_INTO_CGROUP 0x200000000ULL /* Clone into a specific cgroup given
+					    the right permissions.  */
+
 /* The unsigned 64-bit and 8-byte aligned integer type.  */
 typedef __U64_TYPE __aligned_uint64_t __attribute__ ((__aligned__ (8)));
 
diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
index a1a14a58ae..bc321d4c58 100644
--- a/sysdeps/unix/sysv/linux/spawni.c
+++ b/sysdeps/unix/sysv/linux/spawni.c
@@ -66,6 +66,7 @@ struct posix_spawn_args
   ptrdiff_t argc;
   char *const *envp;
   int xflags;
+  bool use_clone3;
   int err;
 };
 
@@ -104,12 +105,11 @@ __spawni_child (void *arguments)
   const posix_spawnattr_t *restrict attr = args->attr;
   const posix_spawn_file_actions_t *file_actions = args->fa;
 
-  /* The child must ensure that no signal handler are enabled because it shared
-     memory with parent, so the signal disposition must be either SIG_DFL or
-     SIG_IGN.  It does by iterating over all signals and although it could
-     possibly be more optimized (by tracking which signal potentially have a
-     signal handler), it might requires system specific solutions (since the
-     sigset_t data type can be very different on different architectures).  */
+  /* The child must ensure that no signal handler is enabled because it
+     shares memory with parent, so all signal dispositions must be either
+     SIG_DFL or SIG_IGN.  If clone3/CLONE_CLEAR_SIGHAND is used, there is
+     only the need to set the defined signals POSIX_SPAWN_SETSIGDEF to
+     SIG_DFL; otherwise, the code iterates over all signals.  */
   struct sigaction sa;
   memset (&sa, '\0', sizeof (sa));
 
@@ -122,7 +122,7 @@ __spawni_child (void *arguments)
 	{
 	  sa.sa_handler = SIG_DFL;
 	}
-      else if (__sigismember (&hset, sig))
+      else if (!args->use_clone3 && __sigismember (&hset, sig))
 	{
 	  if (is_internal_signal (sig))
 	    sa.sa_handler = SIG_IGN;
@@ -382,12 +382,25 @@ __spawnix (pid_t * pid, const char *file,
      for instance).  */
   struct clone_args clone_args =
     {
-      .flags = CLONE_VM | CLONE_VFORK,
+      /* Unsupported flags like CLONE_CLEAR_SIGHAND will be cleared up by
+	 __clone_internal_fallback.  */
+      .flags = CLONE_CLEAR_SIGHAND | CLONE_VM | CLONE_VFORK,
       .exit_signal = SIGCHLD,
       .stack = (uintptr_t) stack,
       .stack_size = stack_size,
     };
-  new_pid = __clone_internal (&clone_args, __spawni_child, &args);
+#ifdef HAVE_CLONE3_WRAPPER
+  args.use_clone3 = true;
+  new_pid = __clone3 (&clone_args, sizeof (clone_args), __spawni_child,
+		      &args);
+  /* clone3 was added in 5.3 and CLONE_CLEAR_SIGHAND in 5.5.  */
+  if (new_pid == -1 && (errno == ENOSYS || errno == EINVAL))
+#endif
+    {
+      args.use_clone3 = false;
+      new_pid = __clone_internal_fallback (&clone_args, __spawni_child,
+					   &args);
+    }
 
   /* It needs to collect the case where the auxiliary process was created
      but failed to execute the file (due either any preparation step or
-- 
2.34.1

[PATCH v4 5/6] aarch64: Add the clone3 wrapper

[Adhemerval Zanella]

It follow the internal signature:

  extern int clone3 (struct clone_args *__cl_args, size_t __size,
 int (*__func) (void *__arg), void *__arg);

Checked on aarch64-linux-gnu.
---
 sysdeps/unix/sysv/linux/aarch64/clone3.S | 85 ++++++++++++++++++++++++
 sysdeps/unix/sysv/linux/aarch64/sysdep.h |  2 +
 2 files changed, 87 insertions(+)
 create mode 100644 sysdeps/unix/sysv/linux/aarch64/clone3.S

diff --git a/sysdeps/unix/sysv/linux/aarch64/clone3.S b/sysdeps/unix/sysv/linux/aarch64/clone3.S
new file mode 100644
index 0000000000..88a1fa1e89
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/aarch64/clone3.S
@@ -0,0 +1,85 @@
+/* The clone3 syscall wrapper.  Linux/aarch64 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:
+   x0: cl_args
+   x1: size
+   x2: func
+   x3: arg  */
+
+        .text
+ENTRY(__clone3)
+	PTR_ARG (0)
+	PTR_ARG (1)
+	PTR_ARG (3)
+	PTR_ARG (4)
+	/* Save args for the child.  */
+	mov	x10, x0		/* cl_args  */
+	mov	x11, x2		/* func	 */
+	mov	x12, x3		/* args  */
+
+	/* Sanity check args.  */
+	mov	x0, #-EINVAL
+	cbz	x10, .Lsyscall_error	/* No NULL cl_args pointer.  */
+	cbz	x2, .Lsyscall_error	/* No NULL function pointer.  */
+
+	/* Do the system call, the kernel expects:
+	   x8: system call number
+	   x0: cl_args
+	   x1: size  */
+	mov	x0, x10
+	mov	x8, #SYS_ify(clone3)
+	svc	0x0
+
+	cmp	x0, #0
+	beq	thread_start
+	blt	.Lsyscall_error
+	RET
+PSEUDO_END (__clone3)
+
+	.align 4
+	.type thread_start, %function
+thread_start:
+	cfi_startproc
+	cfi_undefined (x30)
+	mov	x29, 0
+
+	/* Pick the function arg and execute.  */
+	mov	x0, x12
+	blr	x11
+
+	/* We are done, pass the return value through x0.  */
+	mov	x8, #SYS_ify(exit)
+	svc	0x0
+	cfi_endproc
+	.size thread_start, .-thread_start
+
+libc_hidden_def (__clone3)
+weak_alias (__clone3, clone3)
diff --git a/sysdeps/unix/sysv/linux/aarch64/sysdep.h b/sysdeps/unix/sysv/linux/aarch64/sysdep.h
index 6520fa15da..e94d1703ad 100644
--- a/sysdeps/unix/sysv/linux/aarch64/sysdep.h
+++ b/sysdeps/unix/sysv/linux/aarch64/sysdep.h
@@ -165,6 +165,8 @@
 # define HAVE_CLOCK_GETTIME64_VSYSCALL	"__kernel_clock_gettime"
 # define HAVE_GETTIMEOFDAY_VSYSCALL	"__kernel_gettimeofday"
 
+# define HAVE_CLONE3_WRAPPER		1
+
 # undef INTERNAL_SYSCALL_RAW
 # define INTERNAL_SYSCALL_RAW(name, nr, args...)		\
   ({ long _sys_result;						\
-- 
2.34.1

[PATCH v4 6/6] Linux: optimize clone3 internal usage

[Adhemerval Zanella]

Add an optimization to avoid calling clone3 when glibc detects that
there is no kernel support.  It also adds __ASSUME_CLONE3, which allows
skipping this optimization and issuing the clone3 syscall directly.

It does not handle the the small window between 5.3 and 5.5 for
posix_spawn (CLONE_CLEAR_SIGHAND was added in 5.5).

Checked on x86_64-linux-gnu.
---
 include/clone_internal.h                  |  5 +++++
 sysdeps/unix/sysv/linux/clone-internal.c  | 24 ++++++++++++++++++++++-
 sysdeps/unix/sysv/linux/kernel-features.h |  9 +++++++++
 3 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/include/clone_internal.h b/include/clone_internal.h
index 3b6cd85f02..dd380f119e 100644
--- a/include/clone_internal.h
+++ b/include/clone_internal.h
@@ -24,6 +24,11 @@ extern int __clone3 (struct clone_args *__cl_args, size_t __size,
    fall back to clone or clone2.  */
 extern int __clone_internal (struct clone_args *__cl_args,
 			     int (*__func) (void *__arg), void *__arg);
+/* clone3 wrapper with a sticky check to avoid re-issuing the syscall if
+   it fails with ENOSYS.  */
+extern int __clone3_internal (struct clone_args *cl_args,
+			      int (*func) (void *args), void *arg)
+     attribute_hidden;
 /* The fallback code which calls clone/clone2 based on clone3 arguments.  */
 extern int __clone_internal_fallback (struct clone_args *__cl_args,
 				      int (*__func) (void *__arg),
diff --git a/sysdeps/unix/sysv/linux/clone-internal.c b/sysdeps/unix/sysv/linux/clone-internal.c
index e125c9125e..790739cfce 100644
--- a/sysdeps/unix/sysv/linux/clone-internal.c
+++ b/sysdeps/unix/sysv/linux/clone-internal.c
@@ -76,6 +76,28 @@ __clone_internal_fallback (struct clone_args *cl_args,
   return ret;
 }
 
+int
+__clone3_internal (struct clone_args *cl_args, int (*func) (void *args),
+		   void *arg)
+{
+#ifdef HAVE_CLONE3_WRAPPER
+# if __ASSUME_CLONE3
+  return __clone3 (cl_args, sizeof (*cl_args), func, arg);
+# else
+  static int clone3_supported = 1;
+  if (atomic_load_relaxed (&clone3_supported) == 1)
+    {
+      int ret = __clone3 (cl_args, sizeof (*cl_args), func, arg);
+      if (ret != -1 || errno != ENOSYS)
+	return ret;
+
+      atomic_store_relaxed (&clone3_supported, 0);
+    }
+# endif
+#endif
+  __set_errno (ENOSYS);
+  return -1;
+}
 
 int
 __clone_internal (struct clone_args *cl_args,
@@ -83,7 +105,7 @@ __clone_internal (struct clone_args *cl_args,
 {
 #ifdef HAVE_CLONE3_WRAPPER
   int saved_errno = errno;
-  int ret = __clone3 (cl_args, sizeof (*cl_args), func, arg);
+  int ret = __clone3_internal (cl_args, func, arg);
   if (ret != -1 || errno != ENOSYS)
     return ret;
 
diff --git a/sysdeps/unix/sysv/linux/kernel-features.h b/sysdeps/unix/sysv/linux/kernel-features.h
index a23d0079a1..4b5c4afbc1 100644
--- a/sysdeps/unix/sysv/linux/kernel-features.h
+++ b/sysdeps/unix/sysv/linux/kernel-features.h
@@ -241,4 +241,13 @@
 # define __ASSUME_FUTEX_LOCK_PI2 0
 #endif
 
+/* The clone3 system call was introduced across on most architectures in
+   Linux 5.3.  Not all ports implements it, so it should be used along
+   HAVE_CLONE3_WRAPPER define.  */
+#if __LINUX_KERNEL_VERSION >= 0x050300
+# define __ASSUME_CLONE3 1
+#else
+# define __ASSUME_CLONE3 0
+#endif
+
 #endif /* kernel-features.h */
-- 
2.34.1

Re: [PATCH v4 0/6] Optimize posix_spawn signal setup with clone3

[Adhemerval Zanella Netto]

Carlos, do we still want this for 2.37?

On 12/01/23 10:58, Adhemerval Zanella wrote:
> Currently posix_spawn has to issue at least NSIG sigaction syscalls
> to obtain current signal disposition and more if they there are not
> either SIG_IGN or SIG_DFL.  The new clone3 CLONE_CLEAR_SIGHAND flags
> introduced with Linux 5.5 resets all signal handlers of the child not
> set to SIG_IGN to SIG_DFL, thus allowing posix_spawn to skip this
> preparation phase.
> 
> The exception is the signals defined by posix_spawnattr_setsigdefault
> when POSIX_SPAWN_SETSIGDEF is set (since they can be SIG_IGN).  In
> this case posix_spawn helper process needs to issue a sigction to
> set the signal disposition to SIG_DFL.
> 
> The patchset also adds clone3 implementation for aarch64.  The clone3
> for x86_64 is change to not align the stack, since this is not
> async-signal safe (although current calls does mask/unmask signals).
> 
> * Changes from v3:
>   - Changes commit message and comments wording.
>   - Removed powerpc, s390x, riscv, arm, and mips clone3 optimizations.
> 
> * Changes from v2:
>   - Improve clone3 internal documentation.
> 
> * Changes from v1:
>   - Extend clone3 documentation.
>   - Remove x86_64 stack alignment on clone3 child.
>   - Fixed powerpc64 missing initial stack frame.
>   - Fixed missing save/restore r7 for arm.
> 
> Adhemerval Zanella (6):
>   linux: Do not reset signal handler in posix_spawn if it is already
>     SIG_DFL
>   linux: Extend internal clone3 documentation
>   Linux: Do not align the stack for __clone3
>   linux: Add clone3 CLONE_CLEAR_SIGHAND optimization to posix_spawn
>   aarch64: Add the clone3 wrapper
>   Linux: optimize clone3 internal usage
> 
>  include/clone_internal.h                      |  37 +++-
>  posix/Makefile                                |   3 +-
>  posix/tst-spawn7.c                            | 179 ++++++++++++++++++
>  sysdeps/unix/sysv/linux/Makefile              |   3 +-
>  sysdeps/unix/sysv/linux/aarch64/clone3.S      |  85 +++++++++
>  sysdeps/unix/sysv/linux/aarch64/sysdep.h      |   2 +
>  sysdeps/unix/sysv/linux/clone-internal.c      |  59 ++++--
>  sysdeps/unix/sysv/linux/clone3.h              |  15 +-
>  sysdeps/unix/sysv/linux/kernel-features.h     |   9 +
>  sysdeps/unix/sysv/linux/spawni.c              |  33 +++-
>  .../sysv/linux/tst-misalign-clone-internal.c  |  74 --------
>  sysdeps/unix/sysv/linux/x86_64/clone3.S       |   3 -
>  12 files changed, 384 insertions(+), 118 deletions(-)
>  create mode 100644 posix/tst-spawn7.c
>  create mode 100644 sysdeps/unix/sysv/linux/aarch64/clone3.S
>  delete mode 100644 sysdeps/unix/sysv/linux/tst-misalign-clone-internal.c
> 

Re: [PATCH v4 0/6] Optimize posix_spawn signal setup with clone3

[Carlos O'Donell]

On 1/18/23 12:53, Adhemerval Zanella Netto wrote:
> Carlos, do we still want this for 2.37?

I think we should just push this to 2.38.

Start by committing everything we have for x86_64 and aarch64.

Then repost the other arches for review.
 
> On 12/01/23 10:58, Adhemerval Zanella wrote:
>> Currently posix_spawn has to issue at least NSIG sigaction syscalls
>> to obtain current signal disposition and more if they there are not
>> either SIG_IGN or SIG_DFL.  The new clone3 CLONE_CLEAR_SIGHAND flags
>> introduced with Linux 5.5 resets all signal handlers of the child not
>> set to SIG_IGN to SIG_DFL, thus allowing posix_spawn to skip this
>> preparation phase.
>>
>> The exception is the signals defined by posix_spawnattr_setsigdefault
>> when POSIX_SPAWN_SETSIGDEF is set (since they can be SIG_IGN).  In
>> this case posix_spawn helper process needs to issue a sigction to
>> set the signal disposition to SIG_DFL.
>>
>> The patchset also adds clone3 implementation for aarch64.  The clone3
>> for x86_64 is change to not align the stack, since this is not
>> async-signal safe (although current calls does mask/unmask signals).
>>
>> * Changes from v3:
>>   - Changes commit message and comments wording.
>>   - Removed powerpc, s390x, riscv, arm, and mips clone3 optimizations.
>>
>> * Changes from v2:
>>   - Improve clone3 internal documentation.
>>
>> * Changes from v1:
>>   - Extend clone3 documentation.
>>   - Remove x86_64 stack alignment on clone3 child.
>>   - Fixed powerpc64 missing initial stack frame.
>>   - Fixed missing save/restore r7 for arm.
>>
>> Adhemerval Zanella (6):
>>   linux: Do not reset signal handler in posix_spawn if it is already
>>     SIG_DFL
>>   linux: Extend internal clone3 documentation
>>   Linux: Do not align the stack for __clone3
>>   linux: Add clone3 CLONE_CLEAR_SIGHAND optimization to posix_spawn
>>   aarch64: Add the clone3 wrapper
>>   Linux: optimize clone3 internal usage
>>
>>  include/clone_internal.h                      |  37 +++-
>>  posix/Makefile                                |   3 +-
>>  posix/tst-spawn7.c                            | 179 ++++++++++++++++++
>>  sysdeps/unix/sysv/linux/Makefile              |   3 +-
>>  sysdeps/unix/sysv/linux/aarch64/clone3.S      |  85 +++++++++
>>  sysdeps/unix/sysv/linux/aarch64/sysdep.h      |   2 +
>>  sysdeps/unix/sysv/linux/clone-internal.c      |  59 ++++--
>>  sysdeps/unix/sysv/linux/clone3.h              |  15 +-
>>  sysdeps/unix/sysv/linux/kernel-features.h     |   9 +
>>  sysdeps/unix/sysv/linux/spawni.c              |  33 +++-
>>  .../sysv/linux/tst-misalign-clone-internal.c  |  74 --------
>>  sysdeps/unix/sysv/linux/x86_64/clone3.S       |   3 -
>>  12 files changed, 384 insertions(+), 118 deletions(-)
>>  create mode 100644 posix/tst-spawn7.c
>>  create mode 100644 sysdeps/unix/sysv/linux/aarch64/clone3.S
>>  delete mode 100644 sysdeps/unix/sysv/linux/tst-misalign-clone-internal.c
>>
> 

-- 
Cheers,
Carlos.

Re: [PATCH v4 1/6] linux: Do not reset signal handler in posix_spawn if it is already SIG_DFL

[Carlos O'Donell]

On 1/12/23 08:58, Adhemerval Zanella wrote:
> There is no need to issue another sigaction if the disposition is
> already SIG_DFL.
> 
> Checked on x86_64-linux-gnu.

OK for 2.28.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  sysdeps/unix/sysv/linux/spawni.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
> index b7a6fe320d..a1a14a58ae 100644
> --- a/sysdeps/unix/sysv/linux/spawni.c
> +++ b/sysdeps/unix/sysv/linux/spawni.c
> @@ -129,7 +129,7 @@ __spawni_child (void *arguments)
>  	  else
>  	    {
>  	      __libc_sigaction (sig, 0, &sa);
> -	      if (sa.sa_handler == SIG_IGN)
> +	      if (sa.sa_handler == SIG_IGN || sa.sa_handler == SIG_DFL)
>  		continue;
>  	      sa.sa_handler = SIG_DFL;
>  	    }

-- 
Cheers,
Carlos.

Re: [PATCH v4 2/6] linux: Extend internal clone3 documentation

[Carlos O'Donell]

On 1/12/23 08:58, Adhemerval Zanella wrote:
> Different than kernel, clone3 returns EINVAL for NULL struct
> clone_args or function pointer.  This is similar to clone
> interface that return EINVAL for NULL function argument.
> 
> It also clean up the Linux clone3.h interface, since it not
> currently exported.
> 
> Checked on x86_64-linux-gnu.

OK for 2.28.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  include/clone_internal.h         | 24 +++++++++++++++++++-----
>  sysdeps/unix/sysv/linux/clone3.h | 10 +---------
>  2 files changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/include/clone_internal.h b/include/clone_internal.h
> index 4b23ef33ce..73b8114df4 100644
> --- a/include/clone_internal.h
> +++ b/include/clone_internal.h
> @@ -1,10 +1,24 @@
> -#ifndef _CLONE3_H
> -#include_next <clone3.h>
> +#ifndef _CLONE_INTERNAL_H
> +#define _CLONE_INTERNAL_H
>  
> -extern __typeof (clone3) __clone3;
> +#include <clone3.h>
>  
> -/* The internal wrapper of clone/clone2 and clone3.  If __clone3 returns
> -   -1 with ENOSYS, fall back to clone or clone2.  */
> +/* The clone3 syscall provides a superset of the functionality of the clone
> +   interface.  The kernel might extend __CL_ARGS struct in the future, with
> +   each version with a diffent __SIZE.  If the child is created, it will
> +   start __FUNC function with __ARG arguments.
> +
> +   Different than kernel, the implementation also returns EINVAL for an
> +   invalid NULL __CL_ARGS or __FUNC (similar to __clone).
> +
> +   This function is only implemented if the ABI defines HAVE_CLONE3_WRAPPER.
> +*/
> +extern int __clone3 (struct clone_args *__cl_args, size_t __size,
> +		     int (*__func) (void *__arg), void *__arg);
> +
> +/* The internal wrapper of clone/clone2 and clone3.  Different than __clone3,
> +   it will align the stack if required.  If __clone3 returns -1 with ENOSYS,
> +   fall back to clone or clone2.  */
>  extern int __clone_internal (struct clone_args *__cl_args,
>  			     int (*__func) (void *__arg), void *__arg);
>  
> diff --git a/sysdeps/unix/sysv/linux/clone3.h b/sysdeps/unix/sysv/linux/clone3.h
> index 42055ea981..e4d642e521 100644
> --- a/sysdeps/unix/sysv/linux/clone3.h
> +++ b/sysdeps/unix/sysv/linux/clone3.h
> @@ -1,4 +1,4 @@
> -/* The wrapper of clone3.
> +/* The clone3 kernel interface definitions.
>     Copyright (C) 2021-2023 Free Software Foundation, Inc.
>     This file is part of the GNU C Library.
>  
> @@ -23,8 +23,6 @@
>  #include <stddef.h>
>  #include <bits/types.h>
>  
> -__BEGIN_DECLS
> -
>  /* The unsigned 64-bit and 8-byte aligned integer type.  */
>  typedef __U64_TYPE __aligned_uint64_t __attribute__ ((__aligned__ (8)));
>  
> @@ -58,10 +56,4 @@ struct clone_args
>    __aligned_uint64_t cgroup;
>  };
>  
> -/* The wrapper of clone3.  */
> -extern int clone3 (struct clone_args *__cl_args, size_t __size,
> -		   int (*__func) (void *__arg), void *__arg);
> -
> -__END_DECLS
> -
>  #endif /* clone3.h */

-- 
Cheers,
Carlos.

Re: [PATCH v4 3/6] Linux: Do not align the stack for __clone3

[Carlos O'Donell]

On 1/12/23 08:58, Adhemerval Zanella wrote:
> All internal callers of __clone3 should provide an already aligned
> stack.  Removing the stack alignment in __clone3 is a net gain: it
> simplifies the internal function contract (mask/unmask signals) along
> with the arch-specific code.
> 
> Checked on x86_64-linux-gnu.

OK for 2.28.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  include/clone_internal.h                      |  3 +
>  .../sysv/linux/tst-misalign-clone-internal.c  | 74 -------------------
>  sysdeps/unix/sysv/linux/x86_64/clone3.S       |  3 -
>  3 files changed, 3 insertions(+), 77 deletions(-)
>  delete mode 100644 sysdeps/unix/sysv/linux/tst-misalign-clone-internal.c
> 
> diff --git a/include/clone_internal.h b/include/clone_internal.h
> index 73b8114df4..f8198d8059 100644
> --- a/include/clone_internal.h
> +++ b/include/clone_internal.h
> @@ -11,6 +11,9 @@
>     Different than kernel, the implementation also returns EINVAL for an
>     invalid NULL __CL_ARGS or __FUNC (similar to __clone).
>  
> +   All callers are responsible for correctly aligning the stack.  The stack is
> +   not aligned prior to the syscall (this differs from the exported __clone).
> +
>     This function is only implemented if the ABI defines HAVE_CLONE3_WRAPPER.
>  */
>  extern int __clone3 (struct clone_args *__cl_args, size_t __size,
> diff --git a/sysdeps/unix/sysv/linux/tst-misalign-clone-internal.c b/sysdeps/unix/sysv/linux/tst-misalign-clone-internal.c
> deleted file mode 100644
> index 8b94a74819..0000000000
> --- a/sysdeps/unix/sysv/linux/tst-misalign-clone-internal.c
> +++ /dev/null
> @@ -1,74 +0,0 @@
> -/* Verify that __clone_internal properly aligns the child stack.
> -   Copyright (C) 2021-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 <sched.h>
> -#include <stdbool.h>
> -#include <stdint.h>
> -#include <stdio.h>
> -#include <string.h>
> -#include <sys/wait.h>
> -#include <unistd.h>
> -#include <libc-pointer-arith.h>
> -#include <tst-stack-align.h>
> -#include <clone_internal.h>
> -#include <support/xunistd.h>
> -#include <support/check.h>
> -
> -static int
> -check_stack_alignment (void *arg)
> -{
> -  puts ("in f");
> -
> -  return TEST_STACK_ALIGN () ? 1 : 0;
> -}
> -
> -static int
> -do_test (void)
> -{
> -  puts ("in do_test");
> -
> -  if (TEST_STACK_ALIGN ())
> -    FAIL_EXIT1 ("stack isn't aligned\n");
> -
> -#ifdef __ia64__
> -# define STACK_SIZE (256 * 1024)
> -#else
> -# define STACK_SIZE (128 * 1024)
> -#endif
> -  char st[STACK_SIZE + 1];
> -  /* NB: Align child stack to 1 byte.  */
> -  char *stack = PTR_ALIGN_UP (&st[0], 2) + 1;
> -  struct clone_args clone_args =
> -    {
> -      .stack = (uintptr_t) stack,
> -      .stack_size = STACK_SIZE,
> -    };
> -  pid_t p = __clone_internal (&clone_args, check_stack_alignment, 0);
> -
> -  /* Clone must not fail.  */
> -  TEST_VERIFY_EXIT (p != -1);
> -
> -  int e;
> -  xwaitpid (p, &e, __WCLONE);
> -  TEST_VERIFY (WIFEXITED (e));
> -  TEST_COMPARE (WEXITSTATUS (e), 0);
> -
> -  return 0;
> -}
> -
> -#include <support/test-driver.c>
> diff --git a/sysdeps/unix/sysv/linux/x86_64/clone3.S b/sysdeps/unix/sysv/linux/x86_64/clone3.S
> index bd4a834e46..802d56cbac 100644
> --- a/sysdeps/unix/sysv/linux/x86_64/clone3.S
> +++ b/sysdeps/unix/sysv/linux/x86_64/clone3.S
> @@ -73,9 +73,6 @@ L(thread_start):
>  	   the outermost frame obviously.  */
>  	xorl	%ebp, %ebp
>  
> -	/* Align stack to 16 bytes per the x86-64 psABI.  */
> -	and	$-16, %RSP_LP
> -
>  	/* Set up arguments for the function call.  */
>  	mov	%R8_LP, %RDI_LP	/* Argument.  */
>  	call	*%rdx		/* Call function.  */

-- 
Cheers,
Carlos.

Re: [PATCH v4 4/6] linux: Add clone3 CLONE_CLEAR_SIGHAND optimization to posix_spawn

[Carlos O'Donell]

On 1/12/23 08:58, Adhemerval Zanella wrote:
> The clone3 flag resets all signal handlers of the child not set to
> SIG_IGN to SIG_DFL.  It allows to skip most of the sigaction calls
> to setup child signal handling, where previously a posix_spawn
> had to issue 2 times NSIG sigaction calls (one to obtain the current
> disposition and another to set either SIG_DFL or SIG_IGN).
> 
> With POSIX_SPAWN_SETSIGDEF the child will setup the signal for the case
> where the disposition is SIG_IGN.
> 
> The code must handle the fallback where clone3 is not available. This is
> done by splitting __clone_internal_fallback from __clone_internal.

OK for 2.28.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> 
> Checked on x86_64-linux-gnu.
> ---
>  include/clone_internal.h                 |   5 +
>  posix/Makefile                           |   3 +-
>  posix/tst-spawn7.c                       | 179 +++++++++++++++++++++++
>  sysdeps/unix/sysv/linux/Makefile         |   3 +-
>  sysdeps/unix/sysv/linux/clone-internal.c |  37 +++--
>  sysdeps/unix/sysv/linux/clone3.h         |   7 +
>  sysdeps/unix/sysv/linux/spawni.c         |  31 ++--
>  7 files changed, 238 insertions(+), 27 deletions(-)
>  create mode 100644 posix/tst-spawn7.c
> 
> diff --git a/include/clone_internal.h b/include/clone_internal.h
> index f8198d8059..3b6cd85f02 100644
> --- a/include/clone_internal.h
> +++ b/include/clone_internal.h
> @@ -24,6 +24,11 @@ extern int __clone3 (struct clone_args *__cl_args, size_t __size,
>     fall back to clone or clone2.  */
>  extern int __clone_internal (struct clone_args *__cl_args,
>  			     int (*__func) (void *__arg), void *__arg);
> +/* The fallback code which calls clone/clone2 based on clone3 arguments.  */
> +extern int __clone_internal_fallback (struct clone_args *__cl_args,
> +				      int (*__func) (void *__arg),
> +				      void *__arg)

OK.

> +     attribute_hidden;
>  
>  #ifndef _ISOMAC
>  libc_hidden_proto (__clone3)
> diff --git a/posix/Makefile b/posix/Makefile
> index 38a4279599..7e4f252a40 100644
> --- a/posix/Makefile
> +++ b/posix/Makefile
> @@ -109,7 +109,7 @@ tests		:= test-errno tstgetopt testfnm runtests runptests \
>  		   tst-glob-tilde test-ssize-max tst-spawn4 bug-regex37 \
>  		   bug-regex38 tst-regcomp-truncated tst-spawn-chdir \
>  		   tst-wordexp-nocmd tst-execveat tst-spawn5 \
> -		   tst-sched_getaffinity tst-spawn6
> +		   tst-sched_getaffinity tst-spawn6 tst-spawn7
>  
>  # Test for the glob symbol version that was replaced in glibc 2.27.
>  ifeq ($(have-GLIBC_2.26)$(build-shared),yesyes)
> @@ -291,6 +291,7 @@ tst-spawn-ARGS = -- $(host-test-program-cmd)
>  tst-spawn-static-ARGS = $(tst-spawn-ARGS)
>  tst-spawn5-ARGS = -- $(host-test-program-cmd)
>  tst-spawn6-ARGS = -- $(host-test-program-cmd)
> +tst-spawn7-ARGS = -- $(host-test-program-cmd)
>  tst-dir-ARGS = `pwd` `cd $(common-objdir)/$(subdir); pwd` `cd $(common-objdir); pwd` $(objpfx)tst-dir
>  tst-chmod-ARGS = $(objdir)
>  tst-vfork3-ARGS = --test-dir=$(objpfx)
> diff --git a/posix/tst-spawn7.c b/posix/tst-spawn7.c
> new file mode 100644
> index 0000000000..a1c0d9d942
> --- /dev/null
> +++ b/posix/tst-spawn7.c
> @@ -0,0 +1,179 @@
> +/* Tests for posix_spawn signal handling.
> +   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
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <assert.h>
> +#include <getopt.h>
> +#include <spawn.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <support/check.h>
> +#include <support/xsignal.h>
> +#include <support/xunistd.h>
> +#include <unistd.h>
> +
> +/* Nonzero if the program gets called via `exec'.  */
> +#define CMDLINE_OPTIONS \
> +  { "restart", no_argument, &restart, 1 },
> +static int restart;
> +
> +/* Hold the four initial argument used to respawn the process, plus the extra
> +   '--direct', '--restart', the check type ('SIG_IGN' or 'SIG_DFL'), and a
> +   final NULL.  */
> +static char *spargs[8];
> +static int check_type_argc;
> +
> +/* Called on process re-execution.  */
> +_Noreturn static void
> +handle_restart (int argc, char *argv[])
> +{
> +  assert (argc == 1);
> +
> +  if (strcmp (argv[0], "SIG_DFL") == 0)
> +    {
> +      for (int i = 1; i < NSIG; i++)
> +	{
> +	  struct sigaction sa;
> +	  int r = sigaction (i, NULL, &sa);
> +	  /* Skip internal signals (such as SIGCANCEL).  */
> +	  if (r == -1)
> +	    continue;
> +	  TEST_VERIFY_EXIT (sa.sa_handler == SIG_DFL);
> +	}
> +      exit (EXIT_SUCCESS);
> +    }
> +  else if (strcmp (argv[0], "SIG_IGN") == 0)
> +    {
> +      for (int i = 1; i < NSIG; i++)
> +	{
> +	  struct sigaction sa;
> +	  int r = sigaction (i, NULL, &sa);
> +	  /* Skip internal signals (such as SIGCANCEL).  */
> +	  if (r == -1)
> +	    continue;
> +	  if (i == SIGUSR1 || i == SIGUSR2)
> +	    TEST_VERIFY_EXIT (sa.sa_handler == SIG_IGN);
> +	  else
> +	    TEST_VERIFY_EXIT (sa.sa_handler == SIG_DFL);
> +	}
> +      exit (EXIT_SUCCESS);
> +    }
> +
> +  exit (EXIT_FAILURE);
> +}
> +
> +static void
> +spawn_signal_test (const char *type, const posix_spawnattr_t *attr)
> +{
> +  spargs[check_type_argc] = (char*) type;
> +
> +  pid_t pid;
> +  int status;
> +
> +  TEST_COMPARE (posix_spawn (&pid, spargs[0], NULL, attr, spargs, environ), 0);
> +  TEST_COMPARE (xwaitpid (pid, &status, 0), pid);
> +  TEST_VERIFY (WIFEXITED (status));
> +  TEST_VERIFY (!WIFSIGNALED (status));
> +  TEST_COMPARE (WEXITSTATUS (status), 0);
> +}
> +
> +static void
> +dummy_sa_handler (int)
> +{
> +}
> +
> +static void
> +do_test_signals (void)
> +{
> +  {
> +    /* Check if all signals handler are set to SIG_DFL on spawned process.  */
> +    spawn_signal_test ("SIG_DFL", NULL);
> +  }
> +
> +  {
> +    /* Same as before, but set SIGUSR1 and SIGUSR2 to a handler different than
> +       SIG_IGN or SIG_DFL.  */
> +    struct sigaction sa = { 0 };
> +    sa.sa_handler = dummy_sa_handler;
> +    xsigaction (SIGUSR1, &sa, NULL);
> +    xsigaction (SIGUSR2, &sa, NULL);
> +    spawn_signal_test ("SIG_DFL", NULL);
> +  }
> +
> +  {
> +    /* Check if SIG_IGN is keep as is.  */
> +    struct sigaction sa = { 0 };
> +    sa.sa_handler = SIG_IGN;
> +    xsigaction (SIGUSR1, &sa, NULL);
> +    xsigaction (SIGUSR2, &sa, NULL);
> +    spawn_signal_test ("SIG_IGN", NULL);
> +  }
> +
> +  {
> +    /* Check if SIG_IGN handlers are set to SIG_DFL.  */
> +    posix_spawnattr_t attr;
> +    posix_spawnattr_init (&attr);
> +    sigset_t mask;
> +    sigemptyset (&mask);
> +    sigaddset (&mask, SIGUSR1);
> +    sigaddset (&mask, SIGUSR2);
> +    posix_spawnattr_setsigdefault (&attr, &mask);
> +    posix_spawnattr_setflags (&attr, POSIX_SPAWN_SETSIGDEF);
> +    spawn_signal_test ("SIG_DFL", &attr);
> +    posix_spawnattr_destroy (&attr);
> +  }
> +}
> +
> +static int
> +do_test (int argc, char *argv[])
> +{
> +  /* We must have either:
> +
> +     - one or four parameters if called initially:
> +       + argv[1]: path for ld.so        optional
> +       + argv[2]: "--library-path"      optional
> +       + argv[3]: the library path      optional
> +       + argv[4]: the application name
> +
> +     - six parameters left if called through re-execution:
> +       + argv[1]: the application name
> +       + argv[2]: check SIG_IGN/SIG_DFL.
> +
> +     * When built with --enable-hardcoded-path-in-tests or issued without
> +       using the loader directly.  */
> +
> +  if (restart)
> +    handle_restart (argc - 1, &argv[1]);
> +
> +  TEST_VERIFY_EXIT (argc == 2 || argc == 5);
> +
> +  int i;
> +  for (i = 0; i < argc - 1; i++)
> +    spargs[i] = argv[i + 1];
> +  spargs[i++] = (char *) "--direct";
> +  spargs[i++] = (char *) "--restart";
> +  check_type_argc = i++;
> +  spargs[i] = NULL;
> +
> +
> +  do_test_signals ();
> +
> +  return 0;
> +}
> +
> +#define TEST_FUNCTION_ARGV do_test
> +#include <support/test-driver.c>
> diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
> index f298878e8f..f8bd12d991 100644
> --- a/sysdeps/unix/sysv/linux/Makefile
> +++ b/sysdeps/unix/sysv/linux/Makefile
> @@ -269,8 +269,7 @@ tests-clone-internal = \
>    tst-align-clone-internal \
>    tst-clone2-internal \
>    tst-clone3-internal \
> -  tst-getpid1-internal \
> -  tst-misalign-clone-internal
> +  tst-getpid1-internal
>  tests-internal += $(tests-clone-internal)
>  tests-static += $(tests-clone-internal)
>  
> diff --git a/sysdeps/unix/sysv/linux/clone-internal.c b/sysdeps/unix/sysv/linux/clone-internal.c
> index a8611772a2..e125c9125e 100644
> --- a/sysdeps/unix/sysv/linux/clone-internal.c
> +++ b/sysdeps/unix/sysv/linux/clone-internal.c
> @@ -44,27 +44,15 @@ _Static_assert (sizeof (struct clone_args) == CLONE_ARGS_SIZE_VER2,
>  		"sizeof (struct clone_args) != CLONE_ARGS_SIZE_VER2");
>  
>  int
> -__clone_internal (struct clone_args *cl_args,
> -		  int (*func) (void *arg), void *arg)
> +__clone_internal_fallback (struct clone_args *cl_args,
> +			   int (*func) (void *arg), void *arg)
>  {
> -  int ret;
> -#ifdef HAVE_CLONE3_WRAPPER
> -  /* Try clone3 first.  */
> -  int saved_errno = errno;
> -  ret = __clone3 (cl_args, sizeof (*cl_args), func, arg);
> -  if (ret != -1 || errno != ENOSYS)
> -    return ret;
> -
> -  /* NB: Restore errno since errno may be checked against non-zero
> -     return value.  */
> -  __set_errno (saved_errno);
> -#endif
> -
>    /* Map clone3 arguments to clone arguments.  NB: No need to check
>       invalid clone3 specific bits in flags nor exit_signal since this
>       is an internal function.  */
>    int flags = cl_args->flags | cl_args->exit_signal;
>    void *stack = cast_to_pointer (cl_args->stack);
> +  int ret;
>  
>  #ifdef __ia64__
>    ret = __clone2 (func, stack, cl_args->stack_size,
> @@ -88,4 +76,23 @@ __clone_internal (struct clone_args *cl_args,
>    return ret;
>  }
>  
> +
> +int
> +__clone_internal (struct clone_args *cl_args,
> +		  int (*func) (void *arg), void *arg)
> +{
> +#ifdef HAVE_CLONE3_WRAPPER
> +  int saved_errno = errno;
> +  int ret = __clone3 (cl_args, sizeof (*cl_args), func, arg);
> +  if (ret != -1 || errno != ENOSYS)
> +    return ret;
> +
> +  /* NB: Restore errno since errno may be checked against non-zero
> +     return value.  */
> +  __set_errno (saved_errno);
> +#endif
> +
> +  return __clone_internal_fallback (cl_args, func, arg);
> +}
> +
>  libc_hidden_def (__clone_internal)
> diff --git a/sysdeps/unix/sysv/linux/clone3.h b/sysdeps/unix/sysv/linux/clone3.h
> index e4d642e521..451b156405 100644
> --- a/sysdeps/unix/sysv/linux/clone3.h
> +++ b/sysdeps/unix/sysv/linux/clone3.h
> @@ -23,6 +23,13 @@
>  #include <stddef.h>
>  #include <bits/types.h>
>  
> +
> +/* Flags for the clone3 syscall.  */
> +#define CLONE_CLEAR_SIGHAND 0x100000000ULL /* Clear any signal handler and
> +					      reset to SIG_DFL.  */
> +#define CLONE_INTO_CGROUP 0x200000000ULL /* Clone into a specific cgroup given
> +					    the right permissions.  */

OK.

> +
>  /* The unsigned 64-bit and 8-byte aligned integer type.  */
>  typedef __U64_TYPE __aligned_uint64_t __attribute__ ((__aligned__ (8)));
>  
> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
> index a1a14a58ae..bc321d4c58 100644
> --- a/sysdeps/unix/sysv/linux/spawni.c
> +++ b/sysdeps/unix/sysv/linux/spawni.c
> @@ -66,6 +66,7 @@ struct posix_spawn_args
>    ptrdiff_t argc;
>    char *const *envp;
>    int xflags;
> +  bool use_clone3;

OK.

>    int err;
>  };
>  
> @@ -104,12 +105,11 @@ __spawni_child (void *arguments)
>    const posix_spawnattr_t *restrict attr = args->attr;
>    const posix_spawn_file_actions_t *file_actions = args->fa;
>  
> -  /* The child must ensure that no signal handler are enabled because it shared
> -     memory with parent, so the signal disposition must be either SIG_DFL or
> -     SIG_IGN.  It does by iterating over all signals and although it could
> -     possibly be more optimized (by tracking which signal potentially have a
> -     signal handler), it might requires system specific solutions (since the
> -     sigset_t data type can be very different on different architectures).  */
> +  /* The child must ensure that no signal handler is enabled because it
> +     shares memory with parent, so all signal dispositions must be either
> +     SIG_DFL or SIG_IGN.  If clone3/CLONE_CLEAR_SIGHAND is used, there is
> +     only the need to set the defined signals POSIX_SPAWN_SETSIGDEF to
> +     SIG_DFL; otherwise, the code iterates over all signals.  */

OK.

>    struct sigaction sa;
>    memset (&sa, '\0', sizeof (sa));
>  
> @@ -122,7 +122,7 @@ __spawni_child (void *arguments)
>  	{
>  	  sa.sa_handler = SIG_DFL;
>  	}
> -      else if (__sigismember (&hset, sig))
> +      else if (!args->use_clone3 && __sigismember (&hset, sig))
>  	{
>  	  if (is_internal_signal (sig))
>  	    sa.sa_handler = SIG_IGN;
> @@ -382,12 +382,25 @@ __spawnix (pid_t * pid, const char *file,
>       for instance).  */
>    struct clone_args clone_args =
>      {
> -      .flags = CLONE_VM | CLONE_VFORK,
> +      /* Unsupported flags like CLONE_CLEAR_SIGHAND will be cleared up by
> +	 __clone_internal_fallback.  */
> +      .flags = CLONE_CLEAR_SIGHAND | CLONE_VM | CLONE_VFORK,
>        .exit_signal = SIGCHLD,
>        .stack = (uintptr_t) stack,
>        .stack_size = stack_size,
>      };
> -  new_pid = __clone_internal (&clone_args, __spawni_child, &args);
> +#ifdef HAVE_CLONE3_WRAPPER
> +  args.use_clone3 = true;
> +  new_pid = __clone3 (&clone_args, sizeof (clone_args), __spawni_child,
> +		      &args);
> +  /* clone3 was added in 5.3 and CLONE_CLEAR_SIGHAND in 5.5.  */
> +  if (new_pid == -1 && (errno == ENOSYS || errno == EINVAL))
> +#endif
> +    {
> +      args.use_clone3 = false;
> +      new_pid = __clone_internal_fallback (&clone_args, __spawni_child,
> +					   &args);

OK.

> +    }
>  
>    /* It needs to collect the case where the auxiliary process was created
>       but failed to execute the file (due either any preparation step or

-- 
Cheers,
Carlos.

Re: [PATCH v4 5/6] aarch64: Add the clone3 wrapper

[Carlos O'Donell]

On 1/12/23 08:58, Adhemerval Zanella wrote:
> It follow the internal signature:
> 
>   extern int clone3 (struct clone_args *__cl_args, size_t __size,
>  int (*__func) (void *__arg), void *__arg);
> 
> Checked on aarch64-linux-gnu.

OK for 2.28.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  sysdeps/unix/sysv/linux/aarch64/clone3.S | 85 ++++++++++++++++++++++++
>  sysdeps/unix/sysv/linux/aarch64/sysdep.h |  2 +
>  2 files changed, 87 insertions(+)
>  create mode 100644 sysdeps/unix/sysv/linux/aarch64/clone3.S
> 
> diff --git a/sysdeps/unix/sysv/linux/aarch64/clone3.S b/sysdeps/unix/sysv/linux/aarch64/clone3.S
> new file mode 100644
> index 0000000000..88a1fa1e89
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/aarch64/clone3.S
> @@ -0,0 +1,85 @@
> +/* The clone3 syscall wrapper.  Linux/aarch64 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:
> +   x0: cl_args
> +   x1: size
> +   x2: func
> +   x3: arg  */
> +
> +        .text
> +ENTRY(__clone3)
> +	PTR_ARG (0)
> +	PTR_ARG (1)
> +	PTR_ARG (3)
> +	PTR_ARG (4)
> +	/* Save args for the child.  */
> +	mov	x10, x0		/* cl_args  */
> +	mov	x11, x2		/* func	 */
> +	mov	x12, x3		/* args  */
> +
> +	/* Sanity check args.  */
> +	mov	x0, #-EINVAL
> +	cbz	x10, .Lsyscall_error	/* No NULL cl_args pointer.  */
> +	cbz	x2, .Lsyscall_error	/* No NULL function pointer.  */
> +
> +	/* Do the system call, the kernel expects:
> +	   x8: system call number
> +	   x0: cl_args
> +	   x1: size  */
> +	mov	x0, x10
> +	mov	x8, #SYS_ify(clone3)
> +	svc	0x0
> +
> +	cmp	x0, #0
> +	beq	thread_start
> +	blt	.Lsyscall_error
> +	RET
> +PSEUDO_END (__clone3)
> +
> +	.align 4
> +	.type thread_start, %function
> +thread_start:
> +	cfi_startproc
> +	cfi_undefined (x30)
> +	mov	x29, 0
> +
> +	/* Pick the function arg and execute.  */
> +	mov	x0, x12
> +	blr	x11
> +
> +	/* We are done, pass the return value through x0.  */
> +	mov	x8, #SYS_ify(exit)
> +	svc	0x0
> +	cfi_endproc
> +	.size thread_start, .-thread_start

OK.

> +
> +libc_hidden_def (__clone3)
> +weak_alias (__clone3, clone3)
> diff --git a/sysdeps/unix/sysv/linux/aarch64/sysdep.h b/sysdeps/unix/sysv/linux/aarch64/sysdep.h
> index 6520fa15da..e94d1703ad 100644
> --- a/sysdeps/unix/sysv/linux/aarch64/sysdep.h
> +++ b/sysdeps/unix/sysv/linux/aarch64/sysdep.h
> @@ -165,6 +165,8 @@
>  # define HAVE_CLOCK_GETTIME64_VSYSCALL	"__kernel_clock_gettime"
>  # define HAVE_GETTIMEOFDAY_VSYSCALL	"__kernel_gettimeofday"
>  
> +# define HAVE_CLONE3_WRAPPER		1

OK. Enables the optimizations where possible.

> +
>  # undef INTERNAL_SYSCALL_RAW
>  # define INTERNAL_SYSCALL_RAW(name, nr, args...)		\
>    ({ long _sys_result;						\

-- 
Cheers,
Carlos.

Re: [PATCH v4 6/6] Linux: optimize clone3 internal usage

[Carlos O'Donell]

On 1/12/23 08:58, Adhemerval Zanella wrote:
> Add an optimization to avoid calling clone3 when glibc detects that
> there is no kernel support.  It also adds __ASSUME_CLONE3, which allows
> skipping this optimization and issuing the clone3 syscall directly.
> 
> It does not handle the the small window between 5.3 and 5.5 for
> posix_spawn (CLONE_CLEAR_SIGHAND was added in 5.5).
> 
> Checked on x86_64-linux-gnu.

OK for 2.28.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  include/clone_internal.h                  |  5 +++++
>  sysdeps/unix/sysv/linux/clone-internal.c  | 24 ++++++++++++++++++++++-
>  sysdeps/unix/sysv/linux/kernel-features.h |  9 +++++++++
>  3 files changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/include/clone_internal.h b/include/clone_internal.h
> index 3b6cd85f02..dd380f119e 100644
> --- a/include/clone_internal.h
> +++ b/include/clone_internal.h
> @@ -24,6 +24,11 @@ extern int __clone3 (struct clone_args *__cl_args, size_t __size,
>     fall back to clone or clone2.  */
>  extern int __clone_internal (struct clone_args *__cl_args,
>  			     int (*__func) (void *__arg), void *__arg);
> +/* clone3 wrapper with a sticky check to avoid re-issuing the syscall if
> +   it fails with ENOSYS.  */
> +extern int __clone3_internal (struct clone_args *cl_args,
> +			      int (*func) (void *args), void *arg)
> +     attribute_hidden;
>  /* The fallback code which calls clone/clone2 based on clone3 arguments.  */
>  extern int __clone_internal_fallback (struct clone_args *__cl_args,
>  				      int (*__func) (void *__arg),
> diff --git a/sysdeps/unix/sysv/linux/clone-internal.c b/sysdeps/unix/sysv/linux/clone-internal.c
> index e125c9125e..790739cfce 100644
> --- a/sysdeps/unix/sysv/linux/clone-internal.c
> +++ b/sysdeps/unix/sysv/linux/clone-internal.c
> @@ -76,6 +76,28 @@ __clone_internal_fallback (struct clone_args *cl_args,
>    return ret;
>  }
>  
> +int
> +__clone3_internal (struct clone_args *cl_args, int (*func) (void *args),
> +		   void *arg)
> +{
> +#ifdef HAVE_CLONE3_WRAPPER
> +# if __ASSUME_CLONE3
> +  return __clone3 (cl_args, sizeof (*cl_args), func, arg);
> +# else
> +  static int clone3_supported = 1;
> +  if (atomic_load_relaxed (&clone3_supported) == 1)
> +    {
> +      int ret = __clone3 (cl_args, sizeof (*cl_args), func, arg);
> +      if (ret != -1 || errno != ENOSYS)
> +	return ret;
> +
> +      atomic_store_relaxed (&clone3_supported, 0);
> +    }
> +# endif
> +#endif
> +  __set_errno (ENOSYS);
> +  return -1;
> +}

OK.

>  
>  int
>  __clone_internal (struct clone_args *cl_args,
> @@ -83,7 +105,7 @@ __clone_internal (struct clone_args *cl_args,
>  {
>  #ifdef HAVE_CLONE3_WRAPPER
>    int saved_errno = errno;
> -  int ret = __clone3 (cl_args, sizeof (*cl_args), func, arg);
> +  int ret = __clone3_internal (cl_args, func, arg);
>    if (ret != -1 || errno != ENOSYS)
>      return ret;
>  
> diff --git a/sysdeps/unix/sysv/linux/kernel-features.h b/sysdeps/unix/sysv/linux/kernel-features.h
> index a23d0079a1..4b5c4afbc1 100644
> --- a/sysdeps/unix/sysv/linux/kernel-features.h
> +++ b/sysdeps/unix/sysv/linux/kernel-features.h
> @@ -241,4 +241,13 @@
>  # define __ASSUME_FUTEX_LOCK_PI2 0
>  #endif
>  
> +/* The clone3 system call was introduced across on most architectures in
> +   Linux 5.3.  Not all ports implements it, so it should be used along
> +   HAVE_CLONE3_WRAPPER define.  */
> +#if __LINUX_KERNEL_VERSION >= 0x050300
> +# define __ASSUME_CLONE3 1
> +#else
> +# define __ASSUME_CLONE3 0
> +#endif
> +
>  #endif /* kernel-features.h */

-- 
Cheers,
Carlos.

Re: [PATCH v4 0/6] Optimize posix_spawn signal setup with clone3

[Adhemerval Zanella Netto]

Just to confirm that it is for 2.38, since you replied as 2.28 on the patches :}.

On 18/01/23 19:24, Carlos O'Donell wrote:
> On 1/18/23 12:53, Adhemerval Zanella Netto wrote:
>> Carlos, do we still want this for 2.37?
> 
> I think we should just push this to 2.38.
> 
> Start by committing everything we have for x86_64 and aarch64.
> 
> Then repost the other arches for review.
>  
>> On 12/01/23 10:58, Adhemerval Zanella wrote:
>>> Currently posix_spawn has to issue at least NSIG sigaction syscalls
>>> to obtain current signal disposition and more if they there are not
>>> either SIG_IGN or SIG_DFL.  The new clone3 CLONE_CLEAR_SIGHAND flags
>>> introduced with Linux 5.5 resets all signal handlers of the child not
>>> set to SIG_IGN to SIG_DFL, thus allowing posix_spawn to skip this
>>> preparation phase.
>>>
>>> The exception is the signals defined by posix_spawnattr_setsigdefault
>>> when POSIX_SPAWN_SETSIGDEF is set (since they can be SIG_IGN).  In
>>> this case posix_spawn helper process needs to issue a sigction to
>>> set the signal disposition to SIG_DFL.
>>>
>>> The patchset also adds clone3 implementation for aarch64.  The clone3
>>> for x86_64 is change to not align the stack, since this is not
>>> async-signal safe (although current calls does mask/unmask signals).
>>>
>>> * Changes from v3:
>>>   - Changes commit message and comments wording.
>>>   - Removed powerpc, s390x, riscv, arm, and mips clone3 optimizations.
>>>
>>> * Changes from v2:
>>>   - Improve clone3 internal documentation.
>>>
>>> * Changes from v1:
>>>   - Extend clone3 documentation.
>>>   - Remove x86_64 stack alignment on clone3 child.
>>>   - Fixed powerpc64 missing initial stack frame.
>>>   - Fixed missing save/restore r7 for arm.
>>>
>>> Adhemerval Zanella (6):
>>>   linux: Do not reset signal handler in posix_spawn if it is already
>>>     SIG_DFL
>>>   linux: Extend internal clone3 documentation
>>>   Linux: Do not align the stack for __clone3
>>>   linux: Add clone3 CLONE_CLEAR_SIGHAND optimization to posix_spawn
>>>   aarch64: Add the clone3 wrapper
>>>   Linux: optimize clone3 internal usage
>>>
>>>  include/clone_internal.h                      |  37 +++-
>>>  posix/Makefile                                |   3 +-
>>>  posix/tst-spawn7.c                            | 179 ++++++++++++++++++
>>>  sysdeps/unix/sysv/linux/Makefile              |   3 +-
>>>  sysdeps/unix/sysv/linux/aarch64/clone3.S      |  85 +++++++++
>>>  sysdeps/unix/sysv/linux/aarch64/sysdep.h      |   2 +
>>>  sysdeps/unix/sysv/linux/clone-internal.c      |  59 ++++--
>>>  sysdeps/unix/sysv/linux/clone3.h              |  15 +-
>>>  sysdeps/unix/sysv/linux/kernel-features.h     |   9 +
>>>  sysdeps/unix/sysv/linux/spawni.c              |  33 +++-
>>>  .../sysv/linux/tst-misalign-clone-internal.c  |  74 --------
>>>  sysdeps/unix/sysv/linux/x86_64/clone3.S       |   3 -
>>>  12 files changed, 384 insertions(+), 118 deletions(-)
>>>  create mode 100644 posix/tst-spawn7.c
>>>  create mode 100644 sysdeps/unix/sysv/linux/aarch64/clone3.S
>>>  delete mode 100644 sysdeps/unix/sysv/linux/tst-misalign-clone-internal.c
>>>
>>
> 

Re: [PATCH v4 0/6] Optimize posix_spawn signal setup with clone3

[Carlos O'Donell]

On 1/19/23 09:39, Adhemerval Zanella Netto wrote:
> Just to confirm that it is for 2.38, since you replied as 2.28 on the patches :}.

Yes, 2.38, not 2.28 :-)

Thank you, and sorry.
 
> On 18/01/23 19:24, Carlos O'Donell wrote:
>> On 1/18/23 12:53, Adhemerval Zanella Netto wrote:
>>> Carlos, do we still want this for 2.37?
>>
>> I think we should just push this to 2.38.
>>
>> Start by committing everything we have for x86_64 and aarch64.
>>
>> Then repost the other arches for review.
>>  
>>> On 12/01/23 10:58, Adhemerval Zanella wrote:
>>>> Currently posix_spawn has to issue at least NSIG sigaction syscalls
>>>> to obtain current signal disposition and more if they there are not
>>>> either SIG_IGN or SIG_DFL.  The new clone3 CLONE_CLEAR_SIGHAND flags
>>>> introduced with Linux 5.5 resets all signal handlers of the child not
>>>> set to SIG_IGN to SIG_DFL, thus allowing posix_spawn to skip this
>>>> preparation phase.
>>>>
>>>> The exception is the signals defined by posix_spawnattr_setsigdefault
>>>> when POSIX_SPAWN_SETSIGDEF is set (since they can be SIG_IGN).  In
>>>> this case posix_spawn helper process needs to issue a sigction to
>>>> set the signal disposition to SIG_DFL.
>>>>
>>>> The patchset also adds clone3 implementation for aarch64.  The clone3
>>>> for x86_64 is change to not align the stack, since this is not
>>>> async-signal safe (although current calls does mask/unmask signals).
>>>>
>>>> * Changes from v3:
>>>>   - Changes commit message and comments wording.
>>>>   - Removed powerpc, s390x, riscv, arm, and mips clone3 optimizations.
>>>>
>>>> * Changes from v2:
>>>>   - Improve clone3 internal documentation.
>>>>
>>>> * Changes from v1:
>>>>   - Extend clone3 documentation.
>>>>   - Remove x86_64 stack alignment on clone3 child.
>>>>   - Fixed powerpc64 missing initial stack frame.
>>>>   - Fixed missing save/restore r7 for arm.
>>>>
>>>> Adhemerval Zanella (6):
>>>>   linux: Do not reset signal handler in posix_spawn if it is already
>>>>     SIG_DFL
>>>>   linux: Extend internal clone3 documentation
>>>>   Linux: Do not align the stack for __clone3
>>>>   linux: Add clone3 CLONE_CLEAR_SIGHAND optimization to posix_spawn
>>>>   aarch64: Add the clone3 wrapper
>>>>   Linux: optimize clone3 internal usage
>>>>
>>>>  include/clone_internal.h                      |  37 +++-
>>>>  posix/Makefile                                |   3 +-
>>>>  posix/tst-spawn7.c                            | 179 ++++++++++++++++++
>>>>  sysdeps/unix/sysv/linux/Makefile              |   3 +-
>>>>  sysdeps/unix/sysv/linux/aarch64/clone3.S      |  85 +++++++++
>>>>  sysdeps/unix/sysv/linux/aarch64/sysdep.h      |   2 +
>>>>  sysdeps/unix/sysv/linux/clone-internal.c      |  59 ++++--
>>>>  sysdeps/unix/sysv/linux/clone3.h              |  15 +-
>>>>  sysdeps/unix/sysv/linux/kernel-features.h     |   9 +
>>>>  sysdeps/unix/sysv/linux/spawni.c              |  33 +++-
>>>>  .../sysv/linux/tst-misalign-clone-internal.c  |  74 --------
>>>>  sysdeps/unix/sysv/linux/x86_64/clone3.S       |   3 -
>>>>  12 files changed, 384 insertions(+), 118 deletions(-)
>>>>  create mode 100644 posix/tst-spawn7.c
>>>>  create mode 100644 sysdeps/unix/sysv/linux/aarch64/clone3.S
>>>>  delete mode 100644 sysdeps/unix/sysv/linux/tst-misalign-clone-internal.c
>>>>
>>>
>>
> 

-- 
Cheers,
Carlos.

Re: [PATCH v4 0/6] Optimize posix_spawn signal setup with clone3

[H.J. Lu]

On Thu, Jan 12, 2023 at 5:59 AM Adhemerval Zanella via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> Currently posix_spawn has to issue at least NSIG sigaction syscalls
> to obtain current signal disposition and more if they there are not
> either SIG_IGN or SIG_DFL.  The new clone3 CLONE_CLEAR_SIGHAND flags
> introduced with Linux 5.5 resets all signal handlers of the child not
> set to SIG_IGN to SIG_DFL, thus allowing posix_spawn to skip this
> preparation phase.
>
> The exception is the signals defined by posix_spawnattr_setsigdefault
> when POSIX_SPAWN_SETSIGDEF is set (since they can be SIG_IGN).  In
> this case posix_spawn helper process needs to issue a sigction to
> set the signal disposition to SIG_DFL.
>
> The patchset also adds clone3 implementation for aarch64.  The clone3
> for x86_64 is change to not align the stack, since this is not
> async-signal safe (although current calls does mask/unmask signals).
>
> * Changes from v3:
>   - Changes commit message and comments wording.
>   - Removed powerpc, s390x, riscv, arm, and mips clone3 optimizations.
>
> * Changes from v2:
>   - Improve clone3 internal documentation.
>
> * Changes from v1:
>   - Extend clone3 documentation.
>   - Remove x86_64 stack alignment on clone3 child.
>   - Fixed powerpc64 missing initial stack frame.
>   - Fixed missing save/restore r7 for arm.
>
> Adhemerval Zanella (6):
>   linux: Do not reset signal handler in posix_spawn if it is already
>     SIG_DFL
>   linux: Extend internal clone3 documentation
>   Linux: Do not align the stack for __clone3
>   linux: Add clone3 CLONE_CLEAR_SIGHAND optimization to posix_spawn
>   aarch64: Add the clone3 wrapper
>   Linux: optimize clone3 internal usage
>
>  include/clone_internal.h                      |  37 +++-
>  posix/Makefile                                |   3 +-
>  posix/tst-spawn7.c                            | 179 ++++++++++++++++++
>  sysdeps/unix/sysv/linux/Makefile              |   3 +-
>  sysdeps/unix/sysv/linux/aarch64/clone3.S      |  85 +++++++++
>  sysdeps/unix/sysv/linux/aarch64/sysdep.h      |   2 +
>  sysdeps/unix/sysv/linux/clone-internal.c      |  59 ++++--
>  sysdeps/unix/sysv/linux/clone3.h              |  15 +-
>  sysdeps/unix/sysv/linux/kernel-features.h     |   9 +
>  sysdeps/unix/sysv/linux/spawni.c              |  33 +++-
>  .../sysv/linux/tst-misalign-clone-internal.c  |  74 --------
>  sysdeps/unix/sysv/linux/x86_64/clone3.S       |   3 -
>  12 files changed, 384 insertions(+), 118 deletions(-)
>  create mode 100644 posix/tst-spawn7.c
>  create mode 100644 sysdeps/unix/sysv/linux/aarch64/clone3.S
>  delete mode 100644 sysdeps/unix/sysv/linux/tst-misalign-clone-internal.c
>
> --
> 2.34.1
>

On x86-64, I am getting

error: tst-spawn7.c:55: not true: sa.sa_handler == SIG_DFL
tst-spawn7.c:91: numeric comparison failure
   left: 1 (0x1); from: WEXITSTATUS (status)
  right: 0 (0x0); from: 0
error: tst-spawn7.c:55: not true: sa.sa_handler == SIG_DFL
tst-spawn7.c:91: numeric comparison failure
   left: 1 (0x1); from: WEXITSTATUS (status)
  right: 0 (0x0); from: 0
error: tst-spawn7.c:71: not true: sa.sa_handler == SIG_DFL
tst-spawn7.c:91: numeric comparison failure
   left: 1 (0x1); from: WEXITSTATUS (status)
  right: 0 (0x0); from: 0
error: tst-spawn7.c:55: not true: sa.sa_handler == SIG_DFL
tst-spawn7.c:91: numeric comparison failure
   left: 1 (0x1); from: WEXITSTATUS (status)
  right: 0 (0x0); from: 0
error: 4 test failures
FAIL: posix/tst-spawn7

with

$ make check -j12

But it passes when I run it by hand.  Is this expected?

-- 
H.J.

Re: [PATCH v4 0/6] Optimize posix_spawn signal setup with clone3

[Adhemerval Zanella Netto]

On 28/02/23 13:34, H.J. Lu wrote:
> On Thu, Jan 12, 2023 at 5:59 AM Adhemerval Zanella via Libc-alpha
> <libc-alpha@sourceware.org> wrote:
>>
>> Currently posix_spawn has to issue at least NSIG sigaction syscalls
>> to obtain current signal disposition and more if they there are not
>> either SIG_IGN or SIG_DFL.  The new clone3 CLONE_CLEAR_SIGHAND flags
>> introduced with Linux 5.5 resets all signal handlers of the child not
>> set to SIG_IGN to SIG_DFL, thus allowing posix_spawn to skip this
>> preparation phase.
>>
>> The exception is the signals defined by posix_spawnattr_setsigdefault
>> when POSIX_SPAWN_SETSIGDEF is set (since they can be SIG_IGN).  In
>> this case posix_spawn helper process needs to issue a sigction to
>> set the signal disposition to SIG_DFL.
>>
>> The patchset also adds clone3 implementation for aarch64.  The clone3
>> for x86_64 is change to not align the stack, since this is not
>> async-signal safe (although current calls does mask/unmask signals).
>>
>> * Changes from v3:
>>   - Changes commit message and comments wording.
>>   - Removed powerpc, s390x, riscv, arm, and mips clone3 optimizations.
>>
>> * Changes from v2:
>>   - Improve clone3 internal documentation.
>>
>> * Changes from v1:
>>   - Extend clone3 documentation.
>>   - Remove x86_64 stack alignment on clone3 child.
>>   - Fixed powerpc64 missing initial stack frame.
>>   - Fixed missing save/restore r7 for arm.
>>
>> Adhemerval Zanella (6):
>>   linux: Do not reset signal handler in posix_spawn if it is already
>>     SIG_DFL
>>   linux: Extend internal clone3 documentation
>>   Linux: Do not align the stack for __clone3
>>   linux: Add clone3 CLONE_CLEAR_SIGHAND optimization to posix_spawn
>>   aarch64: Add the clone3 wrapper
>>   Linux: optimize clone3 internal usage
>>
>>  include/clone_internal.h                      |  37 +++-
>>  posix/Makefile                                |   3 +-
>>  posix/tst-spawn7.c                            | 179 ++++++++++++++++++
>>  sysdeps/unix/sysv/linux/Makefile              |   3 +-
>>  sysdeps/unix/sysv/linux/aarch64/clone3.S      |  85 +++++++++
>>  sysdeps/unix/sysv/linux/aarch64/sysdep.h      |   2 +
>>  sysdeps/unix/sysv/linux/clone-internal.c      |  59 ++++--
>>  sysdeps/unix/sysv/linux/clone3.h              |  15 +-
>>  sysdeps/unix/sysv/linux/kernel-features.h     |   9 +
>>  sysdeps/unix/sysv/linux/spawni.c              |  33 +++-
>>  .../sysv/linux/tst-misalign-clone-internal.c  |  74 --------
>>  sysdeps/unix/sysv/linux/x86_64/clone3.S       |   3 -
>>  12 files changed, 384 insertions(+), 118 deletions(-)
>>  create mode 100644 posix/tst-spawn7.c
>>  create mode 100644 sysdeps/unix/sysv/linux/aarch64/clone3.S
>>  delete mode 100644 sysdeps/unix/sysv/linux/tst-misalign-clone-internal.c
>>
>> --
>> 2.34.1
>>
> 
> On x86-64, I am getting
> 
> error: tst-spawn7.c:55: not true: sa.sa_handler == SIG_DFL
> tst-spawn7.c:91: numeric comparison failure
>    left: 1 (0x1); from: WEXITSTATUS (status)
>   right: 0 (0x0); from: 0
> error: tst-spawn7.c:55: not true: sa.sa_handler == SIG_DFL
> tst-spawn7.c:91: numeric comparison failure
>    left: 1 (0x1); from: WEXITSTATUS (status)
>   right: 0 (0x0); from: 0
> error: tst-spawn7.c:71: not true: sa.sa_handler == SIG_DFL
> tst-spawn7.c:91: numeric comparison failure
>    left: 1 (0x1); from: WEXITSTATUS (status)
>   right: 0 (0x0); from: 0
> error: tst-spawn7.c:55: not true: sa.sa_handler == SIG_DFL
> tst-spawn7.c:91: numeric comparison failure
>    left: 1 (0x1); from: WEXITSTATUS (status)
>   right: 0 (0x0); from: 0
> error: 4 test failures
> FAIL: posix/tst-spawn7
> 
> with
> 
> $ make check -j12
> 
> But it passes when I run it by hand.  Is this expected?
> 

I am not sure, I constantly check with make -jN (24 usually).  Can you
instrument the test to print the signal that has failed the check?

Re: [PATCH v4 0/6] Optimize posix_spawn signal setup with clone3

[Florian Weimer]

* H. J. Lu via Libc-alpha:

> On x86-64, I am getting
>
> error: tst-spawn7.c:55: not true: sa.sa_handler == SIG_DFL
> tst-spawn7.c:91: numeric comparison failure
>    left: 1 (0x1); from: WEXITSTATUS (status)
>   right: 0 (0x0); from: 0
> error: tst-spawn7.c:55: not true: sa.sa_handler == SIG_DFL
> tst-spawn7.c:91: numeric comparison failure
>    left: 1 (0x1); from: WEXITSTATUS (status)
>   right: 0 (0x0); from: 0
> error: tst-spawn7.c:71: not true: sa.sa_handler == SIG_DFL
> tst-spawn7.c:91: numeric comparison failure
>    left: 1 (0x1); from: WEXITSTATUS (status)
>   right: 0 (0x0); from: 0
> error: tst-spawn7.c:55: not true: sa.sa_handler == SIG_DFL
> tst-spawn7.c:91: numeric comparison failure
>    left: 1 (0x1); from: WEXITSTATUS (status)
>   right: 0 (0x0); from: 0
> error: 4 test failures
> FAIL: posix/tst-spawn7
>
> with
>
> $ make check -j12
>
> But it passes when I run it by hand.  Is this expected?

I see it as well, but it's not consistent for me, either.  Could it be a
make bug, leaking unusual handler dispositions in some cases?  The test
should probably check that the disposition are as expected at the start,
and not incorrect to begin with.

Thanks,
Florian

Re: [PATCH v4 0/6] Optimize posix_spawn signal setup with clone3

[Adhemerval Zanella Netto]

On 28/02/23 15:09, Florian Weimer wrote:
> * H. J. Lu via Libc-alpha:
> 
>> On x86-64, I am getting
>>
>> error: tst-spawn7.c:55: not true: sa.sa_handler == SIG_DFL
>> tst-spawn7.c:91: numeric comparison failure
>>    left: 1 (0x1); from: WEXITSTATUS (status)
>>   right: 0 (0x0); from: 0
>> error: tst-spawn7.c:55: not true: sa.sa_handler == SIG_DFL
>> tst-spawn7.c:91: numeric comparison failure
>>    left: 1 (0x1); from: WEXITSTATUS (status)
>>   right: 0 (0x0); from: 0
>> error: tst-spawn7.c:71: not true: sa.sa_handler == SIG_DFL
>> tst-spawn7.c:91: numeric comparison failure
>>    left: 1 (0x1); from: WEXITSTATUS (status)
>>   right: 0 (0x0); from: 0
>> error: tst-spawn7.c:55: not true: sa.sa_handler == SIG_DFL
>> tst-spawn7.c:91: numeric comparison failure
>>    left: 1 (0x1); from: WEXITSTATUS (status)
>>   right: 0 (0x0); from: 0
>> error: 4 test failures
>> FAIL: posix/tst-spawn7
>>
>> with
>>
>> $ make check -j12
>>
>> But it passes when I run it by hand.  Is this expected?
> 
> I see it as well, but it's not consistent for me, either.  Could it be a
> make bug, leaking unusual handler dispositions in some cases?  The test
> should probably check that the disposition are as expected at the start,
> and not incorrect to begin with.

Right, it makes sense. I will add some improvement to have a consistent signal
handling disposition prior spawn the tests processes.

Re: [PATCH v4 0/6] Optimize posix_spawn signal setup with clone3

[H.J. Lu]

On Tue, Feb 28, 2023 at 10:16 AM Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 28/02/23 15:09, Florian Weimer wrote:
> > * H. J. Lu via Libc-alpha:
> >
> >> On x86-64, I am getting
> >>
> >> error: tst-spawn7.c:55: not true: sa.sa_handler == SIG_DFL
> >> tst-spawn7.c:91: numeric comparison failure
> >>    left: 1 (0x1); from: WEXITSTATUS (status)
> >>   right: 0 (0x0); from: 0
> >> error: tst-spawn7.c:55: not true: sa.sa_handler == SIG_DFL
> >> tst-spawn7.c:91: numeric comparison failure
> >>    left: 1 (0x1); from: WEXITSTATUS (status)
> >>   right: 0 (0x0); from: 0
> >> error: tst-spawn7.c:71: not true: sa.sa_handler == SIG_DFL
> >> tst-spawn7.c:91: numeric comparison failure
> >>    left: 1 (0x1); from: WEXITSTATUS (status)
> >>   right: 0 (0x0); from: 0
> >> error: tst-spawn7.c:55: not true: sa.sa_handler == SIG_DFL
> >> tst-spawn7.c:91: numeric comparison failure
> >>    left: 1 (0x1); from: WEXITSTATUS (status)
> >>   right: 0 (0x0); from: 0
> >> error: 4 test failures
> >> FAIL: posix/tst-spawn7
> >>
> >> with
> >>
> >> $ make check -j12
> >>
> >> But it passes when I run it by hand.  Is this expected?
> >
> > I see it as well, but it's not consistent for me, either.  Could it be a
> > make bug, leaking unusual handler dispositions in some cases?  The test
> > should probably check that the disposition are as expected at the start,
> > and not incorrect to begin with.
>
> Right, it makes sense. I will add some improvement to have a consistent signal
> handling disposition prior spawn the tests processes.

In all cases, sa.sa_handler was SIG_IGN while SIG_DFL was expected.

-- 
H.J.

Re: [PATCH v4 0/6] Optimize posix_spawn signal setup with clone3

[Andreas Schwab]

On Feb 28 2023, Florian Weimer via Libc-alpha wrote:

> Could it be a make bug, leaking unusual handler dispositions in some
> cases?

Don't use make 4.4, it's broken.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH v4 0/6] Optimize posix_spawn signal setup with clone3

[H.J. Lu]

On Tue, Feb 28, 2023 at 1:19 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
>
> On Feb 28 2023, Florian Weimer via Libc-alpha wrote:
>
> > Could it be a make bug, leaking unusual handler dispositions in some
> > cases?
>
> Don't use make 4.4, it's broken.
>

I am using make 4.3 on Fedora 37.


-- 
H.J.