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=AWL,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 B599B1F55B for ; Tue, 26 May 2020 12:47:49 +0000 (UTC) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id D3FB7386F460; Tue, 26 May 2020 12:47:48 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org D3FB7386F460 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1590497268; bh=h5SiB8UnpRn37E3C0yrXWo3wSYfevpWE9vfuI3Ya9L0=; h=To:Subject:References:Date:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=Ii7KhS5spG3Bx4lwmaUJCLnMwgxE3KFoPdByGng6OL9/QSHKVlCJorlWx+GqnRvfm OhfgN2Oi5MTJsphpcUPjEQ1elvjIwupzsIeXVfRuzmZoMSANy4KQipwEDVViEz8ZPL TL7WNH3e4q0AFxMbsH6HL5Z/9rFDqKaktOQ8T6B4= Received: from us-smtp-delivery-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.61]) by sourceware.org (Postfix) with ESMTP id 84EB13858D34 for ; Tue, 26 May 2020 12:47:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 84EB13858D34 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-390-M0wlSLkXMealLGFH2whmkw-1; Tue, 26 May 2020 08:47:44 -0400 X-MC-Unique: M0wlSLkXMealLGFH2whmkw-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 60653872FE1; Tue, 26 May 2020 12:47:43 +0000 (UTC) Received: from oldenburg2.str.redhat.com (ovpn-112-180.ams2.redhat.com [10.36.112.180]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 7EE0986C7D; Tue, 26 May 2020 12:47:42 +0000 (UTC) To: Mathieu Desnoyers Subject: Re: [PATCH glibc 3/3] rseq registration tests (v10) 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> Date: Tue, 26 May 2020 14:47:41 +0200 In-Reply-To: <1384708804.33510.1590426460701.JavaMail.zimbra@efficios.com> (Mathieu Desnoyers's message of "Mon, 25 May 2020 13:07:40 -0400 (EDT)") Message-ID: <87h7w2rhg2.fsf@oldenburg2.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain 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: Florian Weimer via Libc-alpha Reply-To: Florian Weimer Cc: libc-alpha , Joseph Myers Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" * 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. >>> +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? >>> + 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. >>> +/* 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. >> 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. Thanks, Florian