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 [IPv6:2620:52:3:1:0:246e:9693:128c]) (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 2B07C1FA04 for ; Tue, 26 May 2020 14:43:19 +0000 (UTC) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 4C3113896148; Tue, 26 May 2020 14:43:18 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 4C3113896148 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1590504198; bh=/juvh6UX7cH2pblrlhfoEA6rempLnAGJOIN8KHkQxkk=; 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=hzRHSjBmRKhXWgi3WrLGjzZgS9T/+bi+NOUCXvxIdd8JSsoAWhgKecpdFS/EQWkJ2 CGI5XS468T9p1ptJFduiUGtuulNUMTj+blguFjudxEsFLKfH/tYw18b7GghBtGyrHv JY/0xj7pDTJHZ4MoHIcIDknDrPirMnWjahk42EAQ= Received: from mail.efficios.com (mail.efficios.com [167.114.26.124]) by sourceware.org (Postfix) with ESMTPS id 3ACCC3851C23 for ; Tue, 26 May 2020 14:43:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 3ACCC3851C23 Received: from localhost (localhost [127.0.0.1]) by mail.efficios.com (Postfix) with ESMTP id 05F5825295E; Tue, 26 May 2020 10:43:16 -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 28r-ZDOhEBYk; Tue, 26 May 2020 10:43:15 -0400 (EDT) Received: from localhost (localhost [127.0.0.1]) by mail.efficios.com (Postfix) with ESMTP id 9DC4625308F; Tue, 26 May 2020 10:43:15 -0400 (EDT) DKIM-Filter: OpenDKIM Filter v2.10.3 mail.efficios.com 9DC4625308F 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 aZxg-Sk5sI0b; Tue, 26 May 2020 10:43:15 -0400 (EDT) Received: from mail03.efficios.com (mail03.efficios.com [167.114.26.124]) by mail.efficios.com (Postfix) with ESMTP id 8C8FD25310D; Tue, 26 May 2020 10:43:15 -0400 (EDT) Date: Tue, 26 May 2020 10:43:15 -0400 (EDT) To: Florian Weimer Message-ID: <1647263261.34186.1590504195448.JavaMail.zimbra@efficios.com> In-Reply-To: <87h7w2rhg2.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> <1384708804.33510.1590426460701.JavaMail.zimbra@efficios.com> <87h7w2rhg2.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: 7bit 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: YPfIt1dIPVeJ6nTlxheYfou8BfxIAQ== 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" ----- 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 -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com