From patchwork Fri Feb 21 00:15:13 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Richard Henderson X-Patchwork-Id: 322375 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org 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 ozlabs.org (Postfix) with ESMTPS id 0FA052C03E4 for ; Fri, 21 Feb 2014 11:15:25 +1100 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; q=dns; s=default; b=t74n+6Tj5DT4Z9dt6 YXc4cYQUIN96vWfmNFPDtaspZln2N63nSXqOpmUDaf3yeHxws3RQXcvbaHHU4dwo 9wyy45qpIv1fp+K4v2KtSMR5g64r4xmd+FbYkkHaaq9pOFlARFn1kMMARwnRd7mY Axxfdaw25p++HyUE0RSsEBtj1U= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; s=default; bh=pK8Rc9YwG9FHJg+dtV9uR2W qgMs=; b=FyTKfmfybhF90oUZBhLpcNH92qul68pUuwKylSOyE76KL8lfUhNuQmB 38m/Jq61zo63HhxNNRqscUxoee+J5Y0DiyZQAsLQGt1EtXf/yfGRj2ynf7yj9gqx WQG18nBsr3H6z7qtayne7oSKo5I4GRHnCe6fBkgaPCOTwWpP2A/s= Received: (qmail 29480 invoked by alias); 21 Feb 2014 00:15:19 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 29470 invoked by uid 89); 21 Feb 2014 00:15:19 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.3 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 21 Feb 2014 00:15:18 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s1L0FF1D006414 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 20 Feb 2014 19:15:16 -0500 Received: from pike.twiddle.home (vpn-54-114.rdu2.redhat.com [10.10.54.114]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s1L0FEXm030185; Thu, 20 Feb 2014 19:15:14 -0500 Message-ID: <53069A91.4060007@redhat.com> Date: Thu, 20 Feb 2014 18:15:13 -0600 From: Richard Henderson User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.3.0 MIME-Version: 1.0 To: Jakub Jelinek CC: GCC Patches Subject: Re: [PATCH] Fix c++/60272 References: <5306402A.40402@redhat.com> <20140220180915.GM22862@tucnak.redhat.com> <530655F9.1060104@redhat.com> In-Reply-To: <530655F9.1060104@redhat.com> X-IsSubscribed: yes On 02/20/2014 01:22 PM, Richard Henderson wrote: > On 02/20/2014 12:09 PM, Jakub Jelinek wrote: >> On Thu, Feb 20, 2014 at 11:49:30AM -0600, Richard Henderson wrote: >>> Tested on x86_64 and i686, and manually inspecting the generated code. >>> Any ideas how to regression test this? >> >> No idea about how to test this. >> >>> @@ -5330,14 +5330,23 @@ expand_builtin_atomic_compare_exchange (enum machine_mode mode, tree exp, >>> if (tree_fits_shwi_p (weak) && tree_to_shwi (weak) != 0) >>> is_weak = true; >>> >>> + if (target == const0_rtx) >>> + target = NULL; >>> oldval = expect; >>> - if (!expand_atomic_compare_and_swap ((target == const0_rtx ? NULL : &target), >>> - &oldval, mem, oldval, desired, >>> + >>> + if (!expand_atomic_compare_and_swap (&target, &oldval, mem, oldval, desired, >> >> I'm wondering if this shouldn't be instead: >> oldval = NULL; >> if (!expand_atomic_compare_and_swap (&target, &oldval, mem, expected, desired, >> is_weak, success, failure)) >> >> because otherwise expand_atomic_compare_and_swap could in theory already >> store into expect MEM, couldn't it? I mean, it does: >> /* Load expected into a register for the compare and swap. */ >> if (MEM_P (expected)) >> expected = copy_to_reg (expected); >> >> /* Make sure we always have some place to put the return oldval. >> Further, make sure that place is distinct from the input expected, >> just in case we need that path down below. */ >> if (ptarget_oval == NULL >> || (target_oval = *ptarget_oval) == NULL >> || reg_overlap_mentioned_p (expected, target_oval)) >> target_oval = gen_reg_rtx (mode); >> so with NULL *ptarget_oval it will surely allocate a pseudo, but if it is >> the expected MEM, as expected has been forced into register earlier, >> I don't think it overlaps with that REG and thus it can be already stored >> and have oldval == expect after the call. > > I don't know any target that actually accepts a MEM for oldval, and since the > current definition of __atomic_compare_and_swap_n takes an address for > expected, we'll always have a MEM. So at present we'll always allocate a new > pseudo just as if we zero out oldval. > > But, fair enough. It does seem generally safer your way. Like so. r~ diff --git a/gcc/builtins.c b/gcc/builtins.c index 09fefe50..35969ad 100644 --- a/gcc/builtins.c +++ b/gcc/builtins.c @@ -5332,9 +5332,12 @@ expand_builtin_atomic_compare_exchange (enum machine_mode mode, tree exp, if (target == const0_rtx) target = NULL; - oldval = expect; - if (!expand_atomic_compare_and_swap (&target, &oldval, mem, oldval, desired, + /* Lest the rtl backend create a race condition with an imporoper store + to memory, always create a new pseudo for OLDVAL. */ + oldval = NULL; + + if (!expand_atomic_compare_and_swap (&target, &oldval, mem, expect, desired, is_weak, success, failure)) return NULL_RTX;