[3/3] rseq registration tests (v10)

These tests validate that rseq is registered from various execution
contexts (main thread, constructor, destructor, other threads, other
threads created from constructor and destructor, forked process
(without exec), pthread_atfork handlers, pthread setspecific
destructors, C++ thread and process destructors, signal handlers,
atexit handlers).

tst-rseq.c only links against libc.so, testing registration of rseq in
a non-multithreaded environment.

tst-rseq-nptl.c also links against libpthread.so, testing registration
of rseq in a multithreaded environment.

See the Linux kernel selftests for extensive rseq stress-tests.

CC: Carlos O'Donell <carlos@redhat.com>
CC: Florian Weimer <fweimer@redhat.com>
CC: Joseph Myers <joseph@codesourcery.com>
CC: Szabolcs Nagy <szabolcs.nagy@arm.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ben Maurer <bmaurer@fb.com>
CC: Peter Zijlstra <peterz@infradead.org>
CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
CC: Boqun Feng <boqun.feng@gmail.com>
CC: Will Deacon <will.deacon@arm.com>
CC: Paul Turner <pjt@google.com>
CC: libc-alpha@sourceware.org
---
Changes since v1:
- Rename tst-rseq.c to tst-rseq-nptl.c.
- Introduce tst-rseq.c testing rseq registration in a non-multithreaded
  environment.

Chances since v2:
- Update file headers.
- use xpthread key create/delete.
- remove set stacksize.
- Tests depend on both __NR_rseq and RSEQ_SIG being defined.

Changes since v3:
- Update ChangeLog.

Changes since v4:
- Remove volatile from sys_rseq() rseq_abi parameter.
- Use atomic_load_relaxed to load __rseq_abi.cpu_id, consequence of the
  fact that __rseq_abi is not volatile anymore.
- Include atomic.h from tst-rseq.c for use of atomic_load_relaxed.
  Move tst-rseq.c to internal tests within Makefile due to its use of
  atomic.h.
- Test __rseq_handled initialization by glibc.

Changes since v5:
- Rebase on glibc 2.30.

Changes since v6:
- Remove __rseq_handled.

Changes since v7:
- Update copyright range to include 2020.
- Use __ASSUME_RSEQ to detect rseq availability.

Changes since v8:
- Remove use of __ASSUME_RSEQ.

Changes since v9:
- Adapt to new prototype for xpthread_key_create.
- Update copyright year to 2020.
- Remove constructor test (moved to later patch due to test harness
  modification dependency).
- Change http:// for https://.
---
 sysdeps/unix/sysv/linux/Makefile        |   6 +-
 sysdeps/unix/sysv/linux/tst-rseq-nptl.c | 338 ++++++++++++++++++++++++
 sysdeps/unix/sysv/linux/tst-rseq.c      | 108 ++++++++
 3 files changed, 450 insertions(+), 2 deletions(-)
 create mode 100644 sysdeps/unix/sysv/linux/tst-rseq-nptl.c
 create mode 100644 sysdeps/unix/sysv/linux/tst-rseq.c

* Mathieu Desnoyers via Libc-alpha:

> - Include atomic.h from tst-rseq.c for use of atomic_load_relaxed.
>   Move tst-rseq.c to internal tests within Makefile due to its use of
>   atomic.h.

You could use __atomic builtins to avoid that, or <stdatomic.h>.  Both
are fine in tests.  I suggest to do that, so that the test can be built
more easily outside of glibc.

However, this needs to remain an internal test because the test needs a
defintiion of __NR_rseq from the internal system call list.  Regular
tests use the installed kernel headers, which may not define __NR_rseq.
Maybe mention this in a comment in nptl/Makefile, along with the
tests-internal change?

> diff --git a/sysdeps/unix/sysv/linux/tst-rseq-nptl.c b/sysdeps/unix/sysv/linux/tst-rseq-nptl.c
> new file mode 100644
> index 0000000000..3e3996d686
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/tst-rseq-nptl.c
> @@ -0,0 +1,338 @@
> +/* Restartable Sequences NPTL test.
> +
> +   Copyright (C) 2020 Free Software Foundation, Inc.

Maybe rmoeve this empty line.

> +#ifdef RSEQ_SIG
> +#include <pthread.h>
> +#include <syscall.h>
> +#include <stdlib.h>
> +#include <error.h>
> +#include <errno.h>
> +#include <string.h>
> +#include <stdint.h>
> +#include <sys/types.h>
> +#include <sys/wait.h>
> +#include <signal.h>
> +#include <atomic.h>

This should be:

#ifdef RSEQ_SIG
# include <pthread.h>
# include <stdlib.h>

And so on.

Nested preprocessor conditionals need to be indented per our style
rules.

Please also remove the redundant <syscall.h> inclusion.

> +static pthread_key_t rseq_test_key;
> +
> +static int
> +rseq_thread_registered (void)
> +{
> +  return (int32_t) atomic_load_relaxed (&__rseq_abi.cpu_id) >= 0;
> +}

The return type should be bool.

> +static void
> +rseq_key_destructor (void *arg)
> +{
> +  /* Cannot use deferred failure reporting after main () returns.  */

No () after function name.

> +  if (!rseq_thread_registered ())
> +    FAIL_EXIT1 ("rseq not registered in pthread key destructor");
> +}
> +
> +static void
> +atexit_handler (void)
> +{
> +  /* Cannot use deferred failure reporting after main () returns.  */
> +  if (!rseq_thread_registered ())
> +    FAIL_EXIT1 ("rseq not registered in atexit handler");
> +}
> +
> +static int
> +do_rseq_main_test (void)
> +{
> +  if (atexit (atexit_handler))
> +    FAIL_EXIT1 ("error calling atexit");
> +  rseq_test_key = xpthread_key_create (rseq_key_destructor);
> +  if (pthread_atfork (atfork_prepare, atfork_parent, atfork_child))
> +    FAIL_EXIT1 ("error calling pthread_atfork");
> +  if (raise (SIGUSR1))
> +    FAIL_EXIT1 ("error raising signal");
> +  if (pthread_setspecific (rseq_test_key, (void *) 1l))
> +    FAIL_EXIT1 ("error in pthread_setspecific");
> +  if (!rseq_thread_registered ())
> +    {
> +      FAIL_RET ("rseq not registered in main thread");
> +    }

You could use

  TEST_COMPARE (atexit (atexit_handler), 0);
  …
  TEST_VERIFY (rseq_thread_registered ());

from <support/check.h>.  The automatically generated error messages will
provide sufficient context, I think.

> +  return 0;
> +}
> +
> +static void
> +cancel_routine (void *arg)
> +{
> +  if (!rseq_thread_registered ())
> +    {
> +      printf ("rseq not registered in cancel routine\n");
> +      support_record_failure ();
> +    }
> +}

Please add "error: " to the error message, to make it clearer that it is
an error.

> +
> +static int cancel_thread_ready;
> +
> +static void
> +test_cancel_thread (void)
> +{
> +  pthread_cleanup_push (cancel_routine, NULL);
> +  atomic_store_release (&cancel_thread_ready, 1);
> +  for (;;)
> +    usleep (100);
> +  pthread_cleanup_pop (0);
> +}

This can use a real barrier (pthread_barrier_t), I think.  No need for
polling then.

> +static void *
> +thread_function (void * arg)
> +{
> +  int i = (int) (intptr_t) arg;
> +
> +  if (raise (SIGUSR1))
> +    FAIL_EXIT1 ("error raising signal");

This can use xraise.

> +  if (i == 0)
> +    test_cancel_thread ();
> +  if (pthread_setspecific (rseq_test_key, (void *) 1l))
> +    FAIL_EXIT1 ("error in pthread_setspecific");

Could use TEST_COMPARE.

> +  return rseq_thread_registered () ? NULL : (void *) 1l;
> +}
> +
> +static void
> +sighandler (int sig)
> +{
> +  if (!rseq_thread_registered ())
> +    {
> +      printf ("rseq not registered in signal handler\n");
> +      support_record_failure ();
> +    }
> +}

Please add "error: ".

> +static void
> +setup_signals (void)
> +{
> +  struct sigaction sa;
> +
> +  sigemptyset (&sa.sa_mask);
> +  sigaddset (&sa.sa_mask, SIGUSR1);
> +  sa.sa_flags = 0;
> +  sa.sa_handler = sighandler;
> +  if (sigaction (SIGUSR1, &sa, NULL) != 0)
> +    {
> +      FAIL_EXIT1 ("sigaction failure: %s", strerror (errno));
> +    }
> +}

This could use xsigaction.

> +#define N 7
> +static const int t[N] = { 1, 2, 6, 5, 4, 3, 50 };

Should these definitions be local to do_rseq_test below?  You can avoid
defining N by using array_length from <array_length.h>.

> +static int
> +do_rseq_threads_test (int nr_threads)
> +{
> +  pthread_t th[nr_threads];
> +  int i;
> +  int result = 0;
> +
> +  cancel_thread_ready = 0;
> +  for (i = 0; i < nr_threads; ++i)
> +    if (pthread_create (&th[i], NULL, thread_function,
> +                        (void *) (intptr_t) i) != 0)
> +      {
> +        FAIL_EXIT1 ("creation of thread %d failed", i);
> +      }

Could use xpthread_create.

> +  while (!atomic_load_acquire (&cancel_thread_ready))
> +    usleep (100);

See above, could use xpthread_barrier_wait.

The present code does not wait until all threads have entered their
cancellation region, so I'm not sure if the test object is actually met
here.

> +  if (pthread_cancel (th[0]))
> +    FAIL_EXIT1 ("error in pthread_cancel");

Could use xpthread_cancel.

> +
> +  for (i = 0; i < nr_threads; ++i)
> +    {
> +      void *v;
> +      if (pthread_join (th[i], &v) != 0)
> +        {
> +          printf ("join of thread %d failed\n", i);
> +          result = 1;
> +        }
> +      else if (i != 0 && v != NULL)
> +        {
> +          printf ("join %d successful, but child failed\n", i);
> +          result = 1;
> +        }
> +      else if (i == 0 && v == NULL)
> +        {
> +          printf ("join %d successful, child did not fail as expected\n", i);
> +          result = 1;
> +        }

Could use xpthread_join.

> +static int
> +sys_rseq (struct rseq *rseq_abi, uint32_t rseq_len, int flags, uint32_t sig)
> +{
> +  return syscall (__NR_rseq, rseq_abi, rseq_len, flags, sig);
> +}

(This is the only remaining place where internal headers are potentially
required, I think.)

> +static int
> +rseq_available (void)
> +{
> +  int rc;
> +
> +  rc = sys_rseq (NULL, 0, 0, 0);
> +  if (rc != -1)
> +    FAIL_EXIT1 ("Unexpected rseq return value %d", rc);
> +  switch (errno)
> +    {
> +    case ENOSYS:
> +      return 0;
> +    case EINVAL:
> +      return 1;
> +    default:
> +      FAIL_EXIT1 ("Unexpected rseq error %s", strerror (errno));
> +    }
> +}

Maybe add a comment to explain what EINVAL means in this context?

> +static int
> +do_rseq_fork_test (void)
> +{
> +  int status;
> +  pid_t pid, retpid;
> +
> +  pid = fork ();
> +  switch (pid)
> +    {
> +      case 0:
> +        exit (do_rseq_main_test ());
> +      case -1:
> +        FAIL_EXIT1 ("Unexpected fork error %s", strerror (errno));
> +    }

Could use xfork.

> +  retpid = TEMP_FAILURE_RETRY (waitpid (pid, &status, 0));
> +  if (retpid != pid)
> +    {
> +      FAIL_EXIT1 ("waitpid returned %ld, expected %ld",
> +                  (long int) retpid, (long int) pid);
> +    }

Hmm.  Is the TEMP_FAILURE_RETRY really needed?  Our xwaitpid does not
have this.

> +  if (WEXITSTATUS (status))
> +    {
> +      printf ("rseq not registered in child\n");
> +      return 1;
> +    }

This whole thing looks a lot lie support_isolate_in_subprocess from
<support/namespace.h>.

> +static int
> +do_rseq_test (void)
> +{
> +  int i, result = 0;
> +
> +  if (!rseq_available ())
> +    {
> +      FAIL_UNSUPPORTED ("kernel does not support rseq, skipping test");
> +    }

Braces are not needed here.

> +  setup_signals ();
> +  if (raise (SIGUSR1))
> +    FAIL_EXIT1 ("error raising signal");

Could use xraise.

> +  if (do_rseq_main_test ())
> +    result = 1;
> +  for (i = 0; i < N; i++)
> +    {
> +      if (do_rseq_threads_test (t[i]))
> +        result = 1;
> +    }

Braces are unnecessary.

> +/* Test C++ destructor called at thread and process exit.  */
> +void
> +__call_tls_dtors (void)
> +{
> +  /* Cannot use deferred failure reporting after main () returns.  */
> +  if (!rseq_thread_registered ())
> +    FAIL_EXIT1 ("rseq not registered in C++ thread/process exit destructor");
> +}

Uhm, what is this supposed to accomplish, under the __call_tls_dtors
name in particular?  I don't think this gets ever called.

It may make sense to have a separate, smaller C++ test to cover this
(perhaps as a separate patch).

> +#else

This needs a comment on the same line.  I think it corresponds to the
earlier “#ifdef RSEQ_SIG”.

> +static int
> +do_rseq_test (void)
> +{
> +#ifndef RSEQ_SIG
> +  FAIL_UNSUPPORTED ("glibc does not define RSEQ_SIG, skipping test");
> +#endif

And with the comment, it should be obvious that the #ifndef is not
needed here. 8-)

> +  return 0;
> +}
> +#endif

Likewise: Add comment.

> diff --git a/sysdeps/unix/sysv/linux/tst-rseq.c b/sysdeps/unix/sysv/linux/tst-rseq.c
> new file mode 100644
> index 0000000000..48a61f9998
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/tst-rseq.c
> @@ -0,0 +1,108 @@
> +/* Restartable Sequences single-threaded tests.
> +
> +   Copyright (C) 2020 Free Software Foundation, Inc.

Perhaps remove the empty line?

> +#include <sys/syscall.h>
> +#include <unistd.h>
> +#include <stdio.h>
> +#include <support/check.h>
> +#include <sys/rseq.h>
> +
> +#ifdef RSEQ_SIG
> +#include <syscall.h>

Duplicate <syscall.h>.

> +#include <stdlib.h>
> +#include <error.h>
> +#include <errno.h>
> +#include <stdint.h>
> +#include <string.h>
> +#include <atomic.h>
> +
> +static int
> +rseq_thread_registered (void)
> +{
> +  return (int32_t) atomic_load_relaxed (&__rseq_abi.cpu_id) >= 0;
> +}

(See above.)

> +static int
> +do_rseq_main_test (void)
> +{
> +  if (!rseq_thread_registered ())
> +    {
> +      FAIL_RET ("rseq not registered in main thread");
> +    }
> +  return 0;
> +}

Additional braces.

> +static int
> +sys_rseq (struct rseq *rseq_abi, uint32_t rseq_len, int flags, uint32_t sig)
> +{
> +  return syscall (__NR_rseq, rseq_abi, rseq_len, flags, sig);
> +}
> +
> +static int
> +rseq_available (void)
> +{
> +  int rc;
> +
> +  rc = sys_rseq (NULL, 0, 0, 0);
> +  if (rc != -1)
> +    FAIL_EXIT1 ("Unexpected rseq return value %d", rc);
> +  switch (errno)
> +    {
> +    case ENOSYS:
> +      return 0;
> +    case EINVAL:
> +      return 1;
> +    default:
> +      FAIL_EXIT1 ("Unexpected rseq error %s", strerror (errno));
> +    }
> +}

It looks these three functions could be moved in to a tst-rseq.h header
file (just create the file, no Makefile updates needed).

> +static int
> +do_rseq_test (void)
> +{
> +  int result = 0;
> +
> +  if (!rseq_available ())
> +    {
> +      FAIL_UNSUPPORTED ("kernel does not support rseq, skipping test");
> +    }
> +  if (do_rseq_main_test ())
> +    result = 1;
> +  return result;
> +}
> +#else
> +static int
> +do_rseq_test (void)
> +{
> +#ifndef RSEQ_SIG
> +  FAIL_UNSUPPORTED ("glibc does not define RSEQ_SIG, skipping test");
> +#endif
> +  return 0;
> +}
> +#endif

Comments on #else and #endif, see above.

C++ test could exercise the thread exit path via thread_local, without
linking against libpthread.

Thanks,
Florian

(trimming CC list)

----- On May 20, 2020, at 6:52 AM, Florian Weimer fweimer@redhat.com wrote:

> * Mathieu Desnoyers via Libc-alpha:
> 
>> - Include atomic.h from tst-rseq.c for use of atomic_load_relaxed.
>>   Move tst-rseq.c to internal tests within Makefile due to its use of
>>   atomic.h.
> 
> You could use __atomic builtins to avoid that, or <stdatomic.h>.  Both
> are fine in tests.  I suggest to do that, so that the test can be built
> more easily outside of glibc.

OK will use __atomic_load (&__rseq_abi.cpu_id, &v, __ATOMIC_RELAXED);

> 
> However, this needs to remain an internal test because the test needs a
> defintiion of __NR_rseq from the internal system call list.  Regular
> tests use the installed kernel headers, which may not define __NR_rseq.
> Maybe mention this in a comment in nptl/Makefile, along with the
> tests-internal change?

OK

> 
>> diff --git a/sysdeps/unix/sysv/linux/tst-rseq-nptl.c
>> b/sysdeps/unix/sysv/linux/tst-rseq-nptl.c
>> new file mode 100644
>> index 0000000000..3e3996d686
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/tst-rseq-nptl.c
>> @@ -0,0 +1,338 @@
>> +/* Restartable Sequences NPTL test.
>> +
>> +   Copyright (C) 2020 Free Software Foundation, Inc.
> 
> Maybe rmoeve this empty line.

OK

> 
>> +#ifdef RSEQ_SIG
>> +#include <pthread.h>
>> +#include <syscall.h>
>> +#include <stdlib.h>
>> +#include <error.h>
>> +#include <errno.h>
>> +#include <string.h>
>> +#include <stdint.h>
>> +#include <sys/types.h>
>> +#include <sys/wait.h>
>> +#include <signal.h>
>> +#include <atomic.h>
> 
> This should be:
> 
> #ifdef RSEQ_SIG
> # include <pthread.h>
> # include <stdlib.h>
> 
> And so on.
> 
> Nested preprocessor conditionals need to be indented per our style
> rules.

OK

> 
> Please also remove the redundant <syscall.h> inclusion.

OK. I will remove the first <sys/syscall.h> include and keep the <syscall.h> include.

> 
>> +static pthread_key_t rseq_test_key;
>> +
>> +static int
>> +rseq_thread_registered (void)
>> +{
>> +  return (int32_t) atomic_load_relaxed (&__rseq_abi.cpu_id) >= 0;
>> +}
> 
> The return type should be bool.

OK, and will include <stdbool.h> as well.

> 
>> +static void
>> +rseq_key_destructor (void *arg)
>> +{
>> +  /* Cannot use deferred failure reporting after main () returns.  */
> 
> No () after function name.

OK

> 
>> +  if (!rseq_thread_registered ())
>> +    FAIL_EXIT1 ("rseq not registered in pthread key destructor");
>> +}
>> +
>> +static void
>> +atexit_handler (void)
>> +{
>> +  /* Cannot use deferred failure reporting after main () returns.  */
>> +  if (!rseq_thread_registered ())
>> +    FAIL_EXIT1 ("rseq not registered in atexit handler");
>> +}
>> +
>> +static int
>> +do_rseq_main_test (void)
>> +{
>> +  if (atexit (atexit_handler))
>> +    FAIL_EXIT1 ("error calling atexit");
>> +  rseq_test_key = xpthread_key_create (rseq_key_destructor);
>> +  if (pthread_atfork (atfork_prepare, atfork_parent, atfork_child))
>> +    FAIL_EXIT1 ("error calling pthread_atfork");
>> +  if (raise (SIGUSR1))
>> +    FAIL_EXIT1 ("error raising signal");
>> +  if (pthread_setspecific (rseq_test_key, (void *) 1l))
>> +    FAIL_EXIT1 ("error in pthread_setspecific");
>> +  if (!rseq_thread_registered ())
>> +    {
>> +      FAIL_RET ("rseq not registered in main thread");
>> +    }
> 
> You could use
> 
>  TEST_COMPARE (atexit (atexit_handler), 0);

OK

>  …
>  TEST_VERIFY (rseq_thread_registered ());

That would change the behavior from "return 1 on error" to "continue
executing on error", which is not what we want here. I think we could
use TEST_VERIFY_EXIT to achieve a similar goal here. I'll change do_rseq_main_test
to return void rather than int.

> 
> from <support/check.h>.  The automatically generated error messages will
> provide sufficient context, I think.

OK

> 
>> +  return 0;
>> +}
>> +
>> +static void
>> +cancel_routine (void *arg)
>> +{
>> +  if (!rseq_thread_registered ())
>> +    {
>> +      printf ("rseq not registered in cancel routine\n");
>> +      support_record_failure ();
>> +    }
>> +}
> 
> Please add "error: " to the error message, to make it clearer that it is
> an error.

OK

> 
>> +
>> +static int cancel_thread_ready;
>> +
>> +static void
>> +test_cancel_thread (void)
>> +{
>> +  pthread_cleanup_push (cancel_routine, NULL);
>> +  atomic_store_release (&cancel_thread_ready, 1);
>> +  for (;;)
>> +    usleep (100);
>> +  pthread_cleanup_pop (0);
>> +}
> 
> This can use a real barrier (pthread_barrier_t), I think.  No need for
> polling then.

OK.

> 
>> +static void *
>> +thread_function (void * arg)
>> +{
>> +  int i = (int) (intptr_t) arg;
>> +
>> +  if (raise (SIGUSR1))
>> +    FAIL_EXIT1 ("error raising signal");
> 
> This can use xraise.

OK

> 
>> +  if (i == 0)
>> +    test_cancel_thread ();
>> +  if (pthread_setspecific (rseq_test_key, (void *) 1l))
>> +    FAIL_EXIT1 ("error in pthread_setspecific");
> 
> Could use TEST_COMPARE.

OK

> 
>> +  return rseq_thread_registered () ? NULL : (void *) 1l;
>> +}
>> +
>> +static void
>> +sighandler (int sig)
>> +{
>> +  if (!rseq_thread_registered ())
>> +    {
>> +      printf ("rseq not registered in signal handler\n");
>> +      support_record_failure ();
>> +    }
>> +}
> 
> Please add "error: ".

OK

> 
>> +static void
>> +setup_signals (void)
>> +{
>> +  struct sigaction sa;
>> +
>> +  sigemptyset (&sa.sa_mask);
>> +  sigaddset (&sa.sa_mask, SIGUSR1);
>> +  sa.sa_flags = 0;
>> +  sa.sa_handler = sighandler;
>> +  if (sigaction (SIGUSR1, &sa, NULL) != 0)
>> +    {
>> +      FAIL_EXIT1 ("sigaction failure: %s", strerror (errno));
>> +    }
>> +}
> 
> This could use xsigaction.

OK

> 
>> +#define N 7
>> +static const int t[N] = { 1, 2, 6, 5, 4, 3, 50 };
> 
> Should these definitions be local to do_rseq_test below?  You can avoid
> defining N by using array_length from <array_length.h>.

OK

> 
>> +static int
>> +do_rseq_threads_test (int nr_threads)
>> +{
>> +  pthread_t th[nr_threads];
>> +  int i;
>> +  int result = 0;
>> +
>> +  cancel_thread_ready = 0;
>> +  for (i = 0; i < nr_threads; ++i)
>> +    if (pthread_create (&th[i], NULL, thread_function,
>> +                        (void *) (intptr_t) i) != 0)
>> +      {
>> +        FAIL_EXIT1 ("creation of thread %d failed", i);
>> +      }
> 
> Could use xpthread_create.

OK

> 
>> +  while (!atomic_load_acquire (&cancel_thread_ready))
>> +    usleep (100);
> 
> See above, could use xpthread_barrier_wait.

OK

> 
> The present code does not wait until all threads have entered their
> cancellation region, so I'm not sure if the test object is actually met
> here.

We're only cancelling the first thread in the test, which is the intent.
In terms of barrier, it's a barrier involving only 2 threads.

> 
>> +  if (pthread_cancel (th[0]))
>> +    FAIL_EXIT1 ("error in pthread_cancel");
> 
> Could use xpthread_cancel.

OK

> 
>> +
>> +  for (i = 0; i < nr_threads; ++i)
>> +    {
>> +      void *v;
>> +      if (pthread_join (th[i], &v) != 0)
>> +        {
>> +          printf ("join of thread %d failed\n", i);
>> +          result = 1;
>> +        }
>> +      else if (i != 0 && v != NULL)
>> +        {
>> +          printf ("join %d successful, but child failed\n", i);
>> +          result = 1;
>> +        }
>> +      else if (i == 0 && v == NULL)
>> +        {
>> +          printf ("join %d successful, child did not fail as expected\n", i);
>> +          result = 1;
>> +        }
> 
> Could use xpthread_join.

OK

> 
>> +static int
>> +sys_rseq (struct rseq *rseq_abi, uint32_t rseq_len, int flags, uint32_t sig)
>> +{
>> +  return syscall (__NR_rseq, rseq_abi, rseq_len, flags, sig);
>> +}
> 
> (This is the only remaining place where internal headers are potentially
> required, I think.)

OK

> 
>> +static int
>> +rseq_available (void)
>> +{
>> +  int rc;
>> +
>> +  rc = sys_rseq (NULL, 0, 0, 0);
>> +  if (rc != -1)
>> +    FAIL_EXIT1 ("Unexpected rseq return value %d", rc);
>> +  switch (errno)
>> +    {
>> +    case ENOSYS:
>> +      return 0;
>> +    case EINVAL:
>> +      return 1;
>> +    default:
>> +      FAIL_EXIT1 ("Unexpected rseq error %s", strerror (errno));
>> +    }
>> +}
> 
> Maybe add a comment to explain what EINVAL means in this context?

For instance:

/* rseq is implemented, but detected an invalid parameter.  */

> 
>> +static int
>> +do_rseq_fork_test (void)
>> +{
>> +  int status;
>> +  pid_t pid, retpid;
>> +
>> +  pid = fork ();
>> +  switch (pid)
>> +    {
>> +      case 0:
>> +        exit (do_rseq_main_test ());
>> +      case -1:
>> +        FAIL_EXIT1 ("Unexpected fork error %s", strerror (errno));
>> +    }
> 
> Could use xfork.

OK

> 
>> +  retpid = TEMP_FAILURE_RETRY (waitpid (pid, &status, 0));
>> +  if (retpid != pid)
>> +    {
>> +      FAIL_EXIT1 ("waitpid returned %ld, expected %ld",
>> +                  (long int) retpid, (long int) pid);
>> +    }
> 
> Hmm.  Is the TEMP_FAILURE_RETRY really needed?  Our xwaitpid does not
> have this.

Then how does it deal with a signal interrupting the system call performing
the waitpid (EINTR) ? I do not see WNOHANG being used.

> 
>> +  if (WEXITSTATUS (status))
>> +    {
>> +      printf ("rseq not registered in child\n");
>> +      return 1;
>> +    }
> 
> This whole thing looks a lot lie support_isolate_in_subprocess from
> <support/namespace.h>.

I'll wait until we reach an agreement on TEMP_FAILURE_RETRY before chaning
the waitpid-related code. But indeed, it looks very similar.

> 
>> +static int
>> +do_rseq_test (void)
>> +{
>> +  int i, result = 0;
>> +
>> +  if (!rseq_available ())
>> +    {
>> +      FAIL_UNSUPPORTED ("kernel does not support rseq, skipping test");
>> +    }
> 
> Braces are not needed here.

OK

> 
>> +  setup_signals ();
>> +  if (raise (SIGUSR1))
>> +    FAIL_EXIT1 ("error raising signal");
> 
> Could use xraise.

OK

> 
>> +  if (do_rseq_main_test ())
>> +    result = 1;
>> +  for (i = 0; i < N; i++)
>> +    {
>> +      if (do_rseq_threads_test (t[i]))
>> +        result = 1;
>> +    }
> 
> Braces are unnecessary.

OK

> 
>> +/* Test C++ destructor called at thread and process exit.  */
>> +void
>> +__call_tls_dtors (void)
>> +{
>> +  /* Cannot use deferred failure reporting after main () returns.  */
>> +  if (!rseq_thread_registered ())
>> +    FAIL_EXIT1 ("rseq not registered in C++ thread/process exit destructor");
>> +}
> 
> Uhm, what is this supposed to accomplish, under the __call_tls_dtors
> name in particular?  I don't think this gets ever called.
> 
> It may make sense to have a separate, smaller C++ test to cover this
> (perhaps as a separate patch).

Hrm, the intent was to implement __call_tls_dtors locally so it would
be invoked by libc on thread/process exit, but looking deeper into
stdlib/cxa_thread_atexit_impl.c I suspect the hidden _call_tls_dtors
defined there will be used.

Indeed, a separate C++ test for this would be better. Could be done in a
follow up patch later perhaps ?

> 
>> +#else
> 
> This needs a comment on the same line.  I think it corresponds to the
> earlier “#ifdef RSEQ_SIG”.

OK

> 
>> +static int
>> +do_rseq_test (void)
>> +{
>> +#ifndef RSEQ_SIG
>> +  FAIL_UNSUPPORTED ("glibc does not define RSEQ_SIG, skipping test");
>> +#endif
> 
> And with the comment, it should be obvious that the #ifndef is not
> needed here. 8-)

Indeed! :)

> 
>> +  return 0;
>> +}
>> +#endif
> 
> Likewise: Add comment.

OK

> 
>> diff --git a/sysdeps/unix/sysv/linux/tst-rseq.c
>> b/sysdeps/unix/sysv/linux/tst-rseq.c
>> new file mode 100644
>> index 0000000000..48a61f9998
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/tst-rseq.c
>> @@ -0,0 +1,108 @@
>> +/* Restartable Sequences single-threaded tests.
>> +
>> +   Copyright (C) 2020 Free Software Foundation, Inc.
> 
> Perhaps remove the empty line?

OK

> 
>> +#include <sys/syscall.h>
>> +#include <unistd.h>
>> +#include <stdio.h>
>> +#include <support/check.h>
>> +#include <sys/rseq.h>
>> +
>> +#ifdef RSEQ_SIG
>> +#include <syscall.h>
> 
> Duplicate <syscall.h>.

OK, and fixing indentation.

> 
>> +#include <stdlib.h>
>> +#include <error.h>
>> +#include <errno.h>
>> +#include <stdint.h>
>> +#include <string.h>
>> +#include <atomic.h>
>> +
>> +static int
>> +rseq_thread_registered (void)
>> +{
>> +  return (int32_t) atomic_load_relaxed (&__rseq_abi.cpu_id) >= 0;
>> +}
> 
> (See above.)

OK

> 
>> +static int
>> +do_rseq_main_test (void)
>> +{
>> +  if (!rseq_thread_registered ())
>> +    {
>> +      FAIL_RET ("rseq not registered in main thread");
>> +    }
>> +  return 0;
>> +}
> 
> Additional braces.

Will use TEST_VERIFY_EXIT (rseq_thread_registered ()); instead.

> 
>> +static int
>> +sys_rseq (struct rseq *rseq_abi, uint32_t rseq_len, int flags, uint32_t sig)
>> +{
>> +  return syscall (__NR_rseq, rseq_abi, rseq_len, flags, sig);
>> +}
>> +
>> +static int
>> +rseq_available (void)
>> +{
>> +  int rc;
>> +
>> +  rc = sys_rseq (NULL, 0, 0, 0);
>> +  if (rc != -1)
>> +    FAIL_EXIT1 ("Unexpected rseq return value %d", rc);
>> +  switch (errno)
>> +    {
>> +    case ENOSYS:
>> +      return 0;
>> +    case EINVAL:
>> +      return 1;
>> +    default:
>> +      FAIL_EXIT1 ("Unexpected rseq error %s", strerror (errno));
>> +    }
>> +}
> 
> It looks these three functions could be moved in to a tst-rseq.h header
> file (just create the file, no Makefile updates needed).

OK

> 
>> +static int
>> +do_rseq_test (void)
>> +{
>> +  int result = 0;
>> +
>> +  if (!rseq_available ())
>> +    {
>> +      FAIL_UNSUPPORTED ("kernel does not support rseq, skipping test");
>> +    }
>> +  if (do_rseq_main_test ())
>> +    result = 1;
>> +  return result;
>> +}
>> +#else
>> +static int
>> +do_rseq_test (void)
>> +{
>> +#ifndef RSEQ_SIG
>> +  FAIL_UNSUPPORTED ("glibc does not define RSEQ_SIG, skipping test");
>> +#endif
>> +  return 0;
>> +}
>> +#endif
> 
> Comments on #else and #endif, see above.

OK

> 
> C++ test could exercise the thread exit path via thread_local, without
> linking against libpthread.

Should we keep this for a future patch ?

Thanks,

Mathieu

> 
> Thanks,
> Florian

* Mathieu Desnoyers:

>> The present code does not wait until all threads have entered their
>> cancellation region, so I'm not sure if the test object is actually met
>> here.
>
> We're only cancelling the first thread in the test, which is the intent.
> In terms of barrier, it's a barrier involving only 2 threads.

Huh.  I need to look at the version with the real barrier.

>>> +static int
>>> +rseq_available (void)
>>> +{
>>> +  int rc;
>>> +
>>> +  rc = sys_rseq (NULL, 0, 0, 0);
>>> +  if (rc != -1)
>>> +    FAIL_EXIT1 ("Unexpected rseq return value %d", rc);
>>> +  switch (errno)
>>> +    {
>>> +    case ENOSYS:
>>> +      return 0;
>>> +    case EINVAL:
>>> +      return 1;
>>> +    default:
>>> +      FAIL_EXIT1 ("Unexpected rseq error %s", strerror (errno));
>>> +    }
>>> +}
>> 
>> Maybe add a comment to explain what EINVAL means in this context?
>
> For instance:
>
> /* rseq is implemented, but detected an invalid parameter.  */

Ah, so 0 is an invalid operation?

>>> +  retpid = TEMP_FAILURE_RETRY (waitpid (pid, &status, 0));
>>> +  if (retpid != pid)
>>> +    {
>>> +      FAIL_EXIT1 ("waitpid returned %ld, expected %ld",
>>> +                  (long int) retpid, (long int) pid);
>>> +    }
>> 
>> Hmm.  Is the TEMP_FAILURE_RETRY really needed?  Our xwaitpid does not
>> have this.
>
> Then how does it deal with a signal interrupting the system call performing
> the waitpid (EINTR) ? I do not see WNOHANG being used.

It obscures spurious signals.  In most test cases, if an unexpected
signal is delivered, something is quite wrong indeed.  This is why we
don't generally hide EINTR errors.

>>> +/* Test C++ destructor called at thread and process exit.  */
>>> +void
>>> +__call_tls_dtors (void)
>>> +{
>>> +  /* Cannot use deferred failure reporting after main () returns.  */
>>> +  if (!rseq_thread_registered ())
>>> +    FAIL_EXIT1 ("rseq not registered in C++ thread/process exit destructor");
>>> +}
>> 
>> Uhm, what is this supposed to accomplish, under the __call_tls_dtors
>> name in particular?  I don't think this gets ever called.
>> 
>> It may make sense to have a separate, smaller C++ test to cover this
>> (perhaps as a separate patch).
>
> Hrm, the intent was to implement __call_tls_dtors locally so it would
> be invoked by libc on thread/process exit, but looking deeper into
> stdlib/cxa_thread_atexit_impl.c I suspect the hidden _call_tls_dtors
> defined there will be used.

Right, it's not an interposable symbol.

> Indeed, a separate C++ test for this would be better. Could be done in a
> follow up patch later perhaps ?

Yes, let's remove this and add a real C++ test later.

>> C++ test could exercise the thread exit path via thread_local, without
>> linking against libpthread.
>
> Should we keep this for a future patch ?

Yes, please.

Thanks,
Florian

----- On May 26, 2020, at 8:47 AM, Florian Weimer fweimer@redhat.com wrote:

> * Mathieu Desnoyers:
> 
>>> The present code does not wait until all threads have entered their
>>> cancellation region, so I'm not sure if the test object is actually met
>>> here.
>>
>> We're only cancelling the first thread in the test, which is the intent.
>> In terms of barrier, it's a barrier involving only 2 threads.
> 
> Huh.  I need to look at the version with the real barrier.

Useful bits:

static void
cancel_routine (void *arg)
{
  if (!rseq_thread_registered ())
    {
      printf ("error: rseq not registered in cancel routine\n");
      support_record_failure ();
    }
}

static pthread_barrier_t cancel_thread_barrier;
static pthread_cond_t cancel_thread_cond = PTHREAD_COND_INITIALIZER;
static pthread_mutex_t cancel_thread_mutex = PTHREAD_MUTEX_INITIALIZER;

static void
test_cancel_thread (void)
{
  pthread_cleanup_push (cancel_routine, NULL);
  (void) xpthread_barrier_wait (&cancel_thread_barrier);
  /* Wait forever until cancellation.  */
  xpthread_cond_wait (&cancel_thread_cond, &cancel_thread_mutex);
  pthread_cleanup_pop (0);
}

static void *
thread_function (void * arg)
{
  int i = (int) (intptr_t) arg;

  xraise (SIGUSR1);
  if (i == 0)
    test_cancel_thread ();
  TEST_COMPARE (pthread_setspecific (rseq_test_key, (void *) 1l), 0);
  return rseq_thread_registered () ? NULL : (void *) 1l;
}

static int
do_rseq_threads_test (int nr_threads)
{
  pthread_t th[nr_threads];
  int i;
  int result = 0;

  xpthread_barrier_init (&cancel_thread_barrier, NULL, 2);

  for (i = 0; i < nr_threads; ++i)
    th[i] = xpthread_create (NULL, thread_function,
                             (void *) (intptr_t) i);

  (void) xpthread_barrier_wait (&cancel_thread_barrier);

  xpthread_cancel (th[0]);

  for (i = 0; i < nr_threads; ++i)
    {
      void *v;

      v = xpthread_join (th[i]);
      if (i != 0 && v != NULL)
        {
          printf ("error: join %d successful, but child failed\n", i);
          result = 1;
        }
      else if (i == 0 && v == NULL)
        {
          printf ("error: join %d successful, child did not fail as expected\n", i);
          result = 1;
        }
    }

  xpthread_barrier_destroy (&cancel_thread_barrier);

  return result;
}

> 
>>>> +static int
>>>> +rseq_available (void)
>>>> +{
>>>> +  int rc;
>>>> +
>>>> +  rc = sys_rseq (NULL, 0, 0, 0);
>>>> +  if (rc != -1)
>>>> +    FAIL_EXIT1 ("Unexpected rseq return value %d", rc);
>>>> +  switch (errno)
>>>> +    {
>>>> +    case ENOSYS:
>>>> +      return 0;
>>>> +    case EINVAL:
>>>> +      return 1;
>>>> +    default:
>>>> +      FAIL_EXIT1 ("Unexpected rseq error %s", strerror (errno));
>>>> +    }
>>>> +}
>>> 
>>> Maybe add a comment to explain what EINVAL means in this context?
>>
>> For instance:
>>
>> /* rseq is implemented, but detected an invalid parameter.  */
> 
> Ah, so 0 is an invalid operation?

So the @flags parameter is 0, which is fine.

It's the @rseq_len parameter being 0 (which differs from sizeof(struct rseq))
which returns -EINVAL. I will clarify this in the comment:

/* rseq is implemented, but detected an invalid rseq_len parameter.  */

> 
>>>> +  retpid = TEMP_FAILURE_RETRY (waitpid (pid, &status, 0));
>>>> +  if (retpid != pid)
>>>> +    {
>>>> +      FAIL_EXIT1 ("waitpid returned %ld, expected %ld",
>>>> +                  (long int) retpid, (long int) pid);
>>>> +    }
>>> 
>>> Hmm.  Is the TEMP_FAILURE_RETRY really needed?  Our xwaitpid does not
>>> have this.
>>
>> Then how does it deal with a signal interrupting the system call performing
>> the waitpid (EINTR) ? I do not see WNOHANG being used.
> 
> It obscures spurious signals.  In most test cases, if an unexpected
> signal is delivered, something is quite wrong indeed.  This is why we
> don't generally hide EINTR errors.

So it means you may have trouble using tools like strace and gdb on those
tests ? AFAIU those are heavy users of SIGSTOP and SIGCONT. Similarly for
profilers, those usually rely on a timer-driven signal.

> 
>>>> +/* Test C++ destructor called at thread and process exit.  */
>>>> +void
>>>> +__call_tls_dtors (void)
>>>> +{
>>>> +  /* Cannot use deferred failure reporting after main () returns.  */
>>>> +  if (!rseq_thread_registered ())
>>>> +    FAIL_EXIT1 ("rseq not registered in C++ thread/process exit destructor");
>>>> +}
>>> 
>>> Uhm, what is this supposed to accomplish, under the __call_tls_dtors
>>> name in particular?  I don't think this gets ever called.
>>> 
>>> It may make sense to have a separate, smaller C++ test to cover this
>>> (perhaps as a separate patch).
>>
>> Hrm, the intent was to implement __call_tls_dtors locally so it would
>> be invoked by libc on thread/process exit, but looking deeper into
>> stdlib/cxa_thread_atexit_impl.c I suspect the hidden _call_tls_dtors
>> defined there will be used.
> 
> Right, it's not an interposable symbol.
> 
>> Indeed, a separate C++ test for this would be better. Could be done in a
>> follow up patch later perhaps ?
> 
> Yes, let's remove this and add a real C++ test later.

OK

> 
>>> C++ test could exercise the thread exit path via thread_local, without
>>> linking against libpthread.
>>
>> Should we keep this for a future patch ?
> 
> Yes, please.

OK, thanks!

Mathieu

> 
> Thanks,
> Florian

----- On May 26, 2020, at 10:43 AM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:

> ----- On May 26, 2020, at 8:47 AM, Florian Weimer fweimer@redhat.com wrote:
> 
>> * Mathieu Desnoyers:

[...]

The one question below is the last thing we need to complete discussing
before I can re-spin another version of the patches:

>> 
>>>>> +  retpid = TEMP_FAILURE_RETRY (waitpid (pid, &status, 0));
>>>>> +  if (retpid != pid)
>>>>> +    {
>>>>> +      FAIL_EXIT1 ("waitpid returned %ld, expected %ld",
>>>>> +                  (long int) retpid, (long int) pid);
>>>>> +    }
>>>> 
>>>> Hmm.  Is the TEMP_FAILURE_RETRY really needed?  Our xwaitpid does not
>>>> have this.
>>>
>>> Then how does it deal with a signal interrupting the system call performing
>>> the waitpid (EINTR) ? I do not see WNOHANG being used.
>> 
>> It obscures spurious signals.  In most test cases, if an unexpected
>> signal is delivered, something is quite wrong indeed.  This is why we
>> don't generally hide EINTR errors.
> 
> So it means you may have trouble using tools like strace and gdb on those
> tests ? AFAIU those are heavy users of SIGSTOP and SIGCONT. Similarly for
> profilers, those usually rely on a timer-driven signal.
> 

Should I use xwaitpid instead, and should xwaitpid be fixed to use
TEMP_FAILURE_RETRY ?

Thanks,

Mathieu

* Mathieu Desnoyers:

>>>>> +  retpid = TEMP_FAILURE_RETRY (waitpid (pid, &status, 0));
>>>>> +  if (retpid != pid)
>>>>> +    {
>>>>> +      FAIL_EXIT1 ("waitpid returned %ld, expected %ld",
>>>>> +                  (long int) retpid, (long int) pid);
>>>>> +    }
>>>> 
>>>> Hmm.  Is the TEMP_FAILURE_RETRY really needed?  Our xwaitpid does not
>>>> have this.
>>>
>>> Then how does it deal with a signal interrupting the system call performing
>>> the waitpid (EINTR) ? I do not see WNOHANG being used.
>> 
>> It obscures spurious signals.  In most test cases, if an unexpected
>> signal is delivered, something is quite wrong indeed.  This is why we
>> don't generally hide EINTR errors.
>
> So it means you may have trouble using tools like strace and gdb on those
> tests ? AFAIU those are heavy users of SIGSTOP and SIGCONT. Similarly for
> profilers, those usually rely on a timer-driven signal.

I have never seen any problems with strace due to this.  ptrace has
become a bit more transparent to the tracee since the early days, I
think.

I haven't seen problems under GDB, either, but then tests that fork can
be rather annoying to debug anyway.

Thanks,
Florian

----- On May 27, 2020, at 11:12 AM, Florian Weimer fweimer@redhat.com wrote:

> * Mathieu Desnoyers:
> 
>>>>>> +  retpid = TEMP_FAILURE_RETRY (waitpid (pid, &status, 0));
>>>>>> +  if (retpid != pid)
>>>>>> +    {
>>>>>> +      FAIL_EXIT1 ("waitpid returned %ld, expected %ld",
>>>>>> +                  (long int) retpid, (long int) pid);
>>>>>> +    }
>>>>> 
>>>>> Hmm.  Is the TEMP_FAILURE_RETRY really needed?  Our xwaitpid does not
>>>>> have this.
>>>>
>>>> Then how does it deal with a signal interrupting the system call performing
>>>> the waitpid (EINTR) ? I do not see WNOHANG being used.
>>> 
>>> It obscures spurious signals.  In most test cases, if an unexpected
>>> signal is delivered, something is quite wrong indeed.  This is why we
>>> don't generally hide EINTR errors.
>>
>> So it means you may have trouble using tools like strace and gdb on those
>> tests ? AFAIU those are heavy users of SIGSTOP and SIGCONT. Similarly for
>> profilers, those usually rely on a timer-driven signal.
> 
> I have never seen any problems with strace due to this.  ptrace has
> become a bit more transparent to the tracee since the early days, I
> think.
> 
> I haven't seen problems under GDB, either, but then tests that fork can
> be rather annoying to debug anyway.

OK so I'll use the xwaitpid wrapper and let this be someone else's problem.

Thanks,

Mathieu

* Mathieu Desnoyers:

> ----- On May 27, 2020, at 11:12 AM, Florian Weimer fweimer@redhat.com wrote:
>
>> * Mathieu Desnoyers:
>> 
>>>>>>> +  retpid = TEMP_FAILURE_RETRY (waitpid (pid, &status, 0));
>>>>>>> +  if (retpid != pid)
>>>>>>> +    {
>>>>>>> +      FAIL_EXIT1 ("waitpid returned %ld, expected %ld",
>>>>>>> +                  (long int) retpid, (long int) pid);
>>>>>>> +    }
>>>>>> 
>>>>>> Hmm.  Is the TEMP_FAILURE_RETRY really needed?  Our xwaitpid does not
>>>>>> have this.
>>>>>
>>>>> Then how does it deal with a signal interrupting the system call performing
>>>>> the waitpid (EINTR) ? I do not see WNOHANG being used.
>>>> 
>>>> It obscures spurious signals.  In most test cases, if an unexpected
>>>> signal is delivered, something is quite wrong indeed.  This is why we
>>>> don't generally hide EINTR errors.
>>>
>>> So it means you may have trouble using tools like strace and gdb on those
>>> tests ? AFAIU those are heavy users of SIGSTOP and SIGCONT. Similarly for
>>> profilers, those usually rely on a timer-driven signal.
>> 
>> I have never seen any problems with strace due to this.  ptrace has
>> become a bit more transparent to the tracee since the early days, I
>> think.
>> 
>> I haven't seen problems under GDB, either, but then tests that fork can
>> be rather annoying to debug anyway.
>
> OK so I'll use the xwaitpid wrapper and let this be someone else's problem.

Yes please.  Or support_isolate_in_subprocess.

Thanks,
Florian

----- On May 27, 2020, at 11:21 AM, Florian Weimer fweimer@redhat.com wrote:

> * Mathieu Desnoyers:
> 
>> ----- On May 27, 2020, at 11:12 AM, Florian Weimer fweimer@redhat.com wrote:
>>
>>> * Mathieu Desnoyers:
>>> 
>>>>>>>> +  retpid = TEMP_FAILURE_RETRY (waitpid (pid, &status, 0));
>>>>>>>> +  if (retpid != pid)
>>>>>>>> +    {
>>>>>>>> +      FAIL_EXIT1 ("waitpid returned %ld, expected %ld",
>>>>>>>> +                  (long int) retpid, (long int) pid);
>>>>>>>> +    }
>>>>>>> 
>>>>>>> Hmm.  Is the TEMP_FAILURE_RETRY really needed?  Our xwaitpid does not
>>>>>>> have this.
>>>>>>
>>>>>> Then how does it deal with a signal interrupting the system call performing
>>>>>> the waitpid (EINTR) ? I do not see WNOHANG being used.
>>>>> 
>>>>> It obscures spurious signals.  In most test cases, if an unexpected
>>>>> signal is delivered, something is quite wrong indeed.  This is why we
>>>>> don't generally hide EINTR errors.
>>>>
>>>> So it means you may have trouble using tools like strace and gdb on those
>>>> tests ? AFAIU those are heavy users of SIGSTOP and SIGCONT. Similarly for
>>>> profilers, those usually rely on a timer-driven signal.
>>> 
>>> I have never seen any problems with strace due to this.  ptrace has
>>> become a bit more transparent to the tracee since the early days, I
>>> think.
>>> 
>>> I haven't seen problems under GDB, either, but then tests that fork can
>>> be rather annoying to debug anyway.
>>
>> OK so I'll use the xwaitpid wrapper and let this be someone else's problem.
> 
> Yes please.  Or support_isolate_in_subprocess.

Right, even better, thanks,

Mathieu