[PATCH glibc 3/3] rseq registration tests (v10)
Mathieu Desnoyers
mathieu.desnoyers@efficios.com
Mon May 25 17:07:40 GMT 2020
(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
More information about the Libc-alpha
mailing list