From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-Status: No, score=-4.2 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,MAILING_LIST_MULTI, SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by dcvr.yhbt.net (Postfix) with ESMTPS id 720941F55B for ; Mon, 25 May 2020 17:07:54 +0000 (UTC) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 51F1C3892017; Mon, 25 May 2020 17:07:53 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 51F1C3892017 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1590426473; bh=+RovjWeHKbViWgqgCvFxyLTRgKN+nSrFBev0bm0L1po=; h=Date:To:In-Reply-To:References:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=DUiEWksbBp3HVA3cp8EIDiWz87W6xrgiVe9nD/wJTlr+3BtUasV+z4mkiSygQld08 3mG0X7hpwgECJkRzd4VWCCMb5V6OYAaveVOTCS6WNXV9kzbl0mpRK9arUbAKD3NnVV RUAimJbkOGfAtmYS7btWX9ZdCiGtgm3h4BgqAwAA= Received: from mail.efficios.com (mail.efficios.com [167.114.26.124]) by sourceware.org (Postfix) with ESMTPS id D0CE5389200D for ; Mon, 25 May 2020 17:07:41 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org D0CE5389200D Received: from localhost (localhost [127.0.0.1]) by mail.efficios.com (Postfix) with ESMTP id 9585D2C408E; Mon, 25 May 2020 13:07:41 -0400 (EDT) Received: from mail.efficios.com ([127.0.0.1]) by localhost (mail03.efficios.com [127.0.0.1]) (amavisd-new, port 10032) with ESMTP id BP_erZIs7CaF; Mon, 25 May 2020 13:07:40 -0400 (EDT) Received: from localhost (localhost [127.0.0.1]) by mail.efficios.com (Postfix) with ESMTP id CC3D92C3BFC; Mon, 25 May 2020 13:07:40 -0400 (EDT) DKIM-Filter: OpenDKIM Filter v2.10.3 mail.efficios.com CC3D92C3BFC X-Virus-Scanned: amavisd-new at efficios.com Received: from mail.efficios.com ([127.0.0.1]) by localhost (mail03.efficios.com [127.0.0.1]) (amavisd-new, port 10026) with ESMTP id V4sisSguCGTJ; Mon, 25 May 2020 13:07:40 -0400 (EDT) Received: from mail03.efficios.com (mail03.efficios.com [167.114.26.124]) by mail.efficios.com (Postfix) with ESMTP id C10982C3E54; Mon, 25 May 2020 13:07:40 -0400 (EDT) Date: Mon, 25 May 2020 13:07:40 -0400 (EDT) To: Florian Weimer Message-ID: <1384708804.33510.1590426460701.JavaMail.zimbra@efficios.com> In-Reply-To: <871rnedgjg.fsf@oldenburg2.str.redhat.com> References: <20200501021439.2456-1-mathieu.desnoyers@efficios.com> <20200501021439.2456-4-mathieu.desnoyers@efficios.com> <871rnedgjg.fsf@oldenburg2.str.redhat.com> Subject: Re: [PATCH glibc 3/3] rseq registration tests (v10) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Originating-IP: [167.114.26.124] X-Mailer: Zimbra 8.8.15_GA_3928 (ZimbraWebClient - FF76 (Linux)/8.8.15_GA_3928) Thread-Topic: rseq registration tests (v10) Thread-Index: dlsvlGGbXCCDBOEjOjF7i/CEdjsyVg== X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Mathieu Desnoyers via Libc-alpha Reply-To: Mathieu Desnoyers Cc: libc-alpha , Joseph Myers Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" (trimming CC list) ----- On May 20, 2020, at 6:52 AM, Florian Weimer fweimer@redhat.com wrote: > * Mathieu Desnoyers via Libc-alpha: >=20 >> - 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. >=20 > You could use __atomic builtins to avoid that, or . 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); >=20 > 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 >=20 >> 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. >=20 > Maybe rmoeve this empty line. OK >=20 >> +#ifdef RSEQ_SIG >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >=20 > This should be: >=20 > #ifdef RSEQ_SIG > # include > # include >=20 > And so on. >=20 > Nested preprocessor conditionals need to be indented per our style > rules. OK >=20 > Please also remove the redundant inclusion. OK. I will remove the first include and keep the include. >=20 >> +static pthread_key_t rseq_test_key; >> + >> +static int >> +rseq_thread_registered (void) >> +{ >> + return (int32_t) atomic_load_relaxed (&__rseq_abi.cpu_id) >=3D 0; >> +} >=20 > The return type should be bool. OK, and will include as well. >=20 >> +static void >> +rseq_key_destructor (void *arg) >> +{ >> + /* Cannot use deferred failure reporting after main () returns. */ >=20 > No () after function name. OK >=20 >> + 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 =3D 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"); >> + } >=20 > You could use >=20 > TEST_COMPARE (atexit (atexit_handler), 0); OK > =E2=80=A6 > 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_ma= in_test to return void rather than int. >=20 > from . The automatically generated error messages will > provide sufficient context, I think. OK >=20 >> + return 0; >> +} >> + >> +static void >> +cancel_routine (void *arg) >> +{ >> + if (!rseq_thread_registered ()) >> + { >> + printf ("rseq not registered in cancel routine\n"); >> + support_record_failure (); >> + } >> +} >=20 > Please add "error: " to the error message, to make it clearer that it is > an error. OK >=20 >> + >> +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); >> +} >=20 > This can use a real barrier (pthread_barrier_t), I think. No need for > polling then. OK. >=20 >> +static void * >> +thread_function (void * arg) >> +{ >> + int i =3D (int) (intptr_t) arg; >> + >> + if (raise (SIGUSR1)) >> + FAIL_EXIT1 ("error raising signal"); >=20 > This can use xraise. OK >=20 >> + if (i =3D=3D 0) >> + test_cancel_thread (); >> + if (pthread_setspecific (rseq_test_key, (void *) 1l)) >> + FAIL_EXIT1 ("error in pthread_setspecific"); >=20 > Could use TEST_COMPARE. OK >=20 >> + 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 (); >> + } >> +} >=20 > Please add "error: ". OK >=20 >> +static void >> +setup_signals (void) >> +{ >> + struct sigaction sa; >> + >> + sigemptyset (&sa.sa_mask); >> + sigaddset (&sa.sa_mask, SIGUSR1); >> + sa.sa_flags =3D 0; >> + sa.sa_handler =3D sighandler; >> + if (sigaction (SIGUSR1, &sa, NULL) !=3D 0) >> + { >> + FAIL_EXIT1 ("sigaction failure: %s", strerror (errno)); >> + } >> +} >=20 > This could use xsigaction. OK >=20 >> +#define N 7 >> +static const int t[N] =3D { 1, 2, 6, 5, 4, 3, 50 }; >=20 > Should these definitions be local to do_rseq_test below? You can avoid > defining N by using array_length from . OK >=20 >> +static int >> +do_rseq_threads_test (int nr_threads) >> +{ >> + pthread_t th[nr_threads]; >> + int i; >> + int result =3D 0; >> + >> + cancel_thread_ready =3D 0; >> + for (i =3D 0; i < nr_threads; ++i) >> + if (pthread_create (&th[i], NULL, thread_function, >> + (void *) (intptr_t) i) !=3D 0) >> + { >> + FAIL_EXIT1 ("creation of thread %d failed", i); >> + } >=20 > Could use xpthread_create. OK >=20 >> + while (!atomic_load_acquire (&cancel_thread_ready)) >> + usleep (100); >=20 > See above, could use xpthread_barrier_wait. OK >=20 > 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. >=20 >> + if (pthread_cancel (th[0])) >> + FAIL_EXIT1 ("error in pthread_cancel"); >=20 > Could use xpthread_cancel. OK >=20 >> + >> + for (i =3D 0; i < nr_threads; ++i) >> + { >> + void *v; >> + if (pthread_join (th[i], &v) !=3D 0) >> + { >> + printf ("join of thread %d failed\n", i); >> + result =3D 1; >> + } >> + else if (i !=3D 0 && v !=3D NULL) >> + { >> + printf ("join %d successful, but child failed\n", i); >> + result =3D 1; >> + } >> + else if (i =3D=3D 0 && v =3D=3D NULL) >> + { >> + printf ("join %d successful, child did not fail as expected\n= ", i); >> + result =3D 1; >> + } >=20 > Could use xpthread_join. OK >=20 >> +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); >> +} >=20 > (This is the only remaining place where internal headers are potentially > required, I think.) OK >=20 >> +static int >> +rseq_available (void) >> +{ >> + int rc; >> + >> + rc =3D sys_rseq (NULL, 0, 0, 0); >> + if (rc !=3D -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)); >> + } >> +} >=20 > Maybe add a comment to explain what EINVAL means in this context? For instance: /* rseq is implemented, but detected an invalid parameter. */ >=20 >> +static int >> +do_rseq_fork_test (void) >> +{ >> + int status; >> + pid_t pid, retpid; >> + >> + pid =3D fork (); >> + switch (pid) >> + { >> + case 0: >> + exit (do_rseq_main_test ()); >> + case -1: >> + FAIL_EXIT1 ("Unexpected fork error %s", strerror (errno)); >> + } >=20 > Could use xfork. OK >=20 >> + retpid =3D TEMP_FAILURE_RETRY (waitpid (pid, &status, 0)); >> + if (retpid !=3D pid) >> + { >> + FAIL_EXIT1 ("waitpid returned %ld, expected %ld", >> + (long int) retpid, (long int) pid); >> + } >=20 > 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. >=20 >> + if (WEXITSTATUS (status)) >> + { >> + printf ("rseq not registered in child\n"); >> + return 1; >> + } >=20 > This whole thing looks a lot lie support_isolate_in_subprocess from > . I'll wait until we reach an agreement on TEMP_FAILURE_RETRY before chaning the waitpid-related code. But indeed, it looks very similar. >=20 >> +static int >> +do_rseq_test (void) >> +{ >> + int i, result =3D 0; >> + >> + if (!rseq_available ()) >> + { >> + FAIL_UNSUPPORTED ("kernel does not support rseq, skipping test"); >> + } >=20 > Braces are not needed here. OK >=20 >> + setup_signals (); >> + if (raise (SIGUSR1)) >> + FAIL_EXIT1 ("error raising signal"); >=20 > Could use xraise. OK >=20 >> + if (do_rseq_main_test ()) >> + result =3D 1; >> + for (i =3D 0; i < N; i++) >> + { >> + if (do_rseq_threads_test (t[i])) >> + result =3D 1; >> + } >=20 > Braces are unnecessary. OK >=20 >> +/* 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 destruc= tor"); >> +} >=20 > Uhm, what is this supposed to accomplish, under the __call_tls_dtors > name in particular? I don't think this gets ever called. >=20 > 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 ? >=20 >> +#else >=20 > This needs a comment on the same line. I think it corresponds to the > earlier =E2=80=9C#ifdef RSEQ_SIG=E2=80=9D. OK >=20 >> +static int >> +do_rseq_test (void) >> +{ >> +#ifndef RSEQ_SIG >> + FAIL_UNSUPPORTED ("glibc does not define RSEQ_SIG, skipping test"); >> +#endif >=20 > And with the comment, it should be obvious that the #ifndef is not > needed here. 8-) Indeed! :) >=20 >> + return 0; >> +} >> +#endif >=20 > Likewise: Add comment. OK >=20 >> 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. >=20 > Perhaps remove the empty line? OK >=20 >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#ifdef RSEQ_SIG >> +#include >=20 > Duplicate . OK, and fixing indentation. >=20 >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +static int >> +rseq_thread_registered (void) >> +{ >> + return (int32_t) atomic_load_relaxed (&__rseq_abi.cpu_id) >=3D 0; >> +} >=20 > (See above.) OK >=20 >> +static int >> +do_rseq_main_test (void) >> +{ >> + if (!rseq_thread_registered ()) >> + { >> + FAIL_RET ("rseq not registered in main thread"); >> + } >> + return 0; >> +} >=20 > Additional braces. Will use TEST_VERIFY_EXIT (rseq_thread_registered ()); instead. >=20 >> +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 =3D sys_rseq (NULL, 0, 0, 0); >> + if (rc !=3D -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)); >> + } >> +} >=20 > 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 >=20 >> +static int >> +do_rseq_test (void) >> +{ >> + int result =3D 0; >> + >> + if (!rseq_available ()) >> + { >> + FAIL_UNSUPPORTED ("kernel does not support rseq, skipping test"); >> + } >> + if (do_rseq_main_test ()) >> + result =3D 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 >=20 > Comments on #else and #endif, see above. OK >=20 > 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 >=20 > Thanks, > Florian --=20 Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com