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

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

(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
EfficiOS Inc.
http://www.efficios.com