From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-0.9 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED, SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.0 Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dcvr.yhbt.net (Postfix) with ESMTPS id 1886B200B9 for ; Mon, 7 May 2018 02:49:56 +0000 (UTC) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:in-reply-to:references :content-transfer-encoding; q=dns; s=default; b=yXUvRUqP3b1bOoNH vGOKu3qG6J0dgi+DQUDNeFp8YSeT5aNOU9Ohn0V8J/O9PV+R5f7AAefsH61hlxP7 PlWxH95vHmPtuAck9jhFm3NashVWblWLmffLTAT9bUJAVMHYwv90EDTHRUAxV/wf CN+FG4FEBinjGHZKJBqrTU2cG9s= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:in-reply-to:references :content-transfer-encoding; s=default; bh=WDb8pXmYlo8IIy+51LFwWZ KlB90=; b=ieter9qdk/v14D197fvC/A+KAO5N97Qd/xo/LTJE1+Dw6yBhS5vfQd AH+WvrKb9ugGueRRPw8tnpZXK7m+ezxZPmy45YSN/UaE2buXW8Hr3smmeDH3kXcF vZVwmxoKVH4P5LRlbjUtnABOdANhVTZfKRQiy8jZ/RgQwAlKx5bdc= Received: (qmail 51574 invoked by alias); 7 May 2018 02:49:29 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 49798 invoked by uid 89); 7 May 2018 02:49:19 -0000 Authentication-Results: sourceware.org; auth=none X-HELO: mailbackend.panix.com From: Zack Weinberg To: libc-alpha@sourceware.org Cc: adhemerval.zanella@linaro.org Subject: Re: [PATCH v2 02/21] nptl: Fix testcases for new pthread cancellation mechanism Date: Sun, 6 May 2018 22:49:06 -0400 Message-Id: <20180507024909.5598-2-zackw@panix.com> In-Reply-To: <20180507024909.5598-1-zackw@panix.com> References: <20180507024909.5598-1-zackw@panix.com> MIME-Version: 1.0 In-Reply-To: <1519679016-12241-3-git-send-email-adhemerval.zanella@linaro.org> References: <1519679016-12241-3-git-send-email-adhemerval.zanella@linaro.org> Content-Transfer-Encoding: 8bit On 26 Feb 2018, Adhemerval Zanella wrote: > With upcoming fix for BZ#12683, pthread cancellation does not act for: > > 1. If syscall is blocked but with some side effects already having > taken place (e.g. a partial read or write). > 2. After the syscall has returned. > > The main change is due the fact programs need to act in syscalls with > side-effects (for instance, to avoid leak of allocated resources or > handle partial read/write). > > This patch changes the NPTL testcase that assumes the old behavior and > also remove the tst-cancel-wrappers.sh test (which checks for symbols > that will not exist anymore). It's OK and expected that we have to adjust test cases that were expecting the old behavior ... > For tst-cancel{2,3} case it remove the pipe close because it might > cause the write syscall to return with side effects if the close is > executed before the pthread cancel. ... however, this change appears to be wrong. If cancellation is broken, these tests will now deadlock rather than failing cleanly. If I understand correctly, the problem you're trying to avoid is that the 'tf' thread could conceivably receive the closed-pipe event before the cancellation signal, even though the 'do_test' thread triggers the cancellation signal first. I don't know of any way to fix this 100%, but I think it would be good enough to use pthread_timedjoin_np to sleep for a hundred milliseconds or so in the 'do_test' thread, and then close the pipe if the 'tf' thread is still alive. (tst-cancel4.c appears to be trying to ensure that cancellations are pending with a pthread_barrier_t, but as far as I know there's no guarantee that if a thread does pthread_cancel(th); pthread_barrier_wait(ba); where 'th' also waits on 'ba', the SIGCANCEL will actually be delivered before the barrier unblocks, either. Feh.) > It also changes how to call the read syscall on tst-backtrace{5,6} > to use syscall instead of read cancelable syscall to avoid need to > handle the cancelable bridge function calls. It requires a change > on powerpc syscall implementation to create a stackframe, since > powerpc backtrace rely on such information. It doesn't look technically difficult to me to handle an additional stack frame or two in the trace. They're always going to be there, aren't they? In the new world order, the stack trace will always be 0 handle_signal 1 2 __syscall_cancel_arch 3 __syscall_cancel 4 read 5 fn 6 fn 7 fn 8 do_test won't it? I think teaching the backtrace logic about this would be better than needing to use a raw syscall() and then mess with the PowerPC implementation of syscall(). I might feel differently about this change if __read_nocancel were a public API, but it isn't... > Checked on i686-linux-gnu, x86_64-linux-gnu, x86_64-linux-gnux32, > aarch64-linux-gnu, arm-linux-gnueabihf, powerpc64le-linux-gnu, > powerpc-linux-gnu, sparcv9-linux-gnu, and sparc64-linux-gnu. When you say checked, do you mean you actually ran the test cases, or did you just compile them (perhaps with a cross-compiler)? > * nptl/Makefile [$(run-built-tests) = yes] (tests-special): Remove > tst-cancel-wrappers.sh. > * nptl/tst-cancel-wrappers.sh: Remove file. This part is OK. > * nptl/tst-cancel4.c (tf_write): Handle cancelled syscall with > side-effects. > (tf_send): Likewise. This part is also OK. I think the test can still fail spuriously due to races, but this one can't deadlock, at least, so we can live with it. > * sysdeps/unix/sysv/linux/powerpc/syscall.S (syscall): Create stack > frame. This ChangeLog entry belongs to patch 1. zw