Re: [PATCH glibc 3/3] rseq registration tests (v10)

[Florian Weimer <fweimer@redhat.com>]

* 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