From patchwork Wed Sep 2 14:01:45 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dmitry Vyukov X-Patchwork-Id: 513523 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 9C5E51401C7 for ; Thu, 3 Sep 2015 00:02:19 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=shRwfsjI; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:from:date:message-id :subject:to:cc:content-type; q=dns; s=default; b=CLCBZ5IHoiSgyx/ SPNJVKr+q4EqPwqFanIOabHq+1Jc+K13jRKvLnYaXMcOe1x3zAfaAohnxV7bISC0 S2rmP0kzyG+llEveLC0RARP3mEwCJ9njskF49443pfxsSUeQoGCLRr4+uSF3wbr0 JEWiI59uax095+LGegZFsfoQfSP4= 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 :mime-version:in-reply-to:references:from:date:message-id :subject:to:cc:content-type; s=default; bh=FWOVJ9Y/mXnRzo27fYa0N YXABA8=; b=shRwfsjIi3JVF+PvJjQFHI6wEzlqTEuIT5V9lmWEdOBgf5UBT58/r r7r0A+dh4TIZwlC+7o6EF3zNrGpggS2bXRw+bTjnMdBfWgCHFSHTQnFDkZhjoxdW pBQbt++6gzP96tyFBMi0djOFRKUCFVtCGrl0+Pm/Um5J8LNqTrGnEg= Received: (qmail 19985 invoked by alias); 2 Sep 2015 14:02:12 -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 19971 invoked by uid 89); 2 Sep 2015 14:02:11 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL, BAYES_00, KAM_ASCII_DIVIDERS, RCVD_IN_DNSWL_LOW, SPF_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mail-wi0-f174.google.com Received: from mail-wi0-f174.google.com (HELO mail-wi0-f174.google.com) (209.85.212.174) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Wed, 02 Sep 2015 14:02:10 +0000 Received: by wibz8 with SMTP id z8so67178853wib.1 for ; Wed, 02 Sep 2015 07:02:07 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-type; bh=akGkCE4T0ofPzFkxk9TX08d/lTeCuTxiy87NAtv86to=; b=CU1E/GzJXkreDRDt6Z0zaBTdgmaCdhCJ44fzjMqI7TR55dSeemhwTECDSaZViRdLAs Wqqeunjrgd0GvaDFDhQUpK0TeyDw+GHWrjy2GBM2BRP1V1yRSDp5WolE/6/R8d8u2Yx9 yqjX+Hf7QSSXpuCK+vDeWyCYFSVzzUWBLG6PffWO6rQCsAzOrQ4S1KmUa4i42ok3pjzQ AndbEqgiX7Mbw2GwJo4W0uaZpCGYeBM7UUn+ZJbI4Qf7Yf40LB/TmyRHWM9p0i74Rmq8 Ntjp8Lwt8gMrdxGHhnrXJ9Smp0Rfqs4JN1cTcq3P8kRueXdnkGGSBlVfr1LzKQq+GYuh T3fg== X-Gm-Message-State: ALoCoQmnLWvBCQuJp5Kb4l9UUaGdFT5rmbgPZq8dbdx3un48knymX2qIK6thEtMMTTHgWC1E0IsS X-Received: by 10.180.206.176 with SMTP id lp16mr4147858wic.85.1441202525676; Wed, 02 Sep 2015 07:02:05 -0700 (PDT) MIME-Version: 1.0 Received: by 10.28.64.193 with HTTP; Wed, 2 Sep 2015 07:01:45 -0700 (PDT) In-Reply-To: <20150902131752.GJ2631@redhat.com> References: <20150901142713.GG2631@redhat.com> <20150901150847.GH2631@redhat.com> <20150902131752.GJ2631@redhat.com> From: Dmitry Vyukov Date: Wed, 2 Sep 2015 16:01:45 +0200 Message-ID: Subject: Re: [Patch, libstdc++] Fix data races in basic_string implementation To: Jonathan Wakely Cc: GCC Patches , libstdc++@gcc.gnu.org, Alexander Potapenko , Kostya Serebryany , Torvald Riegel X-IsSubscribed: yes Added comment to _M_dispose and restored ChangeLog entry. Please take another look. On Wed, Sep 2, 2015 at 3:17 PM, Jonathan Wakely wrote: > On 01/09/15 17:42 +0200, Dmitry Vyukov wrote: >> >> On Tue, Sep 1, 2015 at 5:08 PM, Jonathan Wakely >> wrote: >>> >>> On 01/09/15 16:56 +0200, Dmitry Vyukov wrote: >>>> >>>> >>>> I don't understand how a new gcc may not support __atomic builtins on >>>> ints. How it is even possible? That's a portable API provided by >>>> recent gcc's... >>> >>> >>> >>> The built-in function is always defined, but it might expand to a call >>> to an external function in libatomic, and it would be a regression for >>> code using std::string to start requiring libatomic (although maybe it >>> would be necessary if it's the only way to make the code correct). >>> >>> I don't know if there are any targets that define __GTHREADS and also >>> don't support __atomic_load(int*, ...) without libatomic. If such >>> targets exist then adding a new configure check that only depends on >>> __atomic_load(int*, ...) would mean we keep supporting those targets. >>> >>> Another option would be to simply do: >>> >>> bool >>> _M_is_shared() const _GLIBCXX_NOEXCEPT >>> #if defined(__GTHREADS) >>> + { return __atomic_load(&this->_M_refcount, __ATOMIC_ACQUIRE) > >>> 0; } >>> +#else >>> { return this->_M_refcount > 0; } >>> +#endif >>> >>> and see if anyone complains! >> >> >> I like this option! >> If a platform uses multithreading and has non-inlined atomic loads, >> then the way to fix this is to provide inlined atomic loads rather >> than to fix all call sites. >> >> Attaching new patch. Please take another look. > > > This looks good. Torvald suggested that it would be useful to add a > similar comment to the release operation in _M_dispose, so that both > sides of the release-acquire are similarly documented. Could you add > that and provide a suitable ChangeLog entry? > > Thanks! > > >> Index: include/bits/basic_string.h >> =================================================================== >> --- include/bits/basic_string.h (revision 227363) >> +++ include/bits/basic_string.h (working copy) >> @@ -2601,11 +2601,32 @@ >> >> bool >> _M_is_leaked() const _GLIBCXX_NOEXCEPT >> - { return this->_M_refcount < 0; } >> + { >> +#if defined(__GTHREADS) >> + // _M_refcount is mutated concurrently by >> _M_refcopy/_M_dispose, >> + // so we need to use an atomic load. However, _M_is_leaked >> + // predicate does not change concurrently (i.e. the string is >> either >> + // leaked or not), so a relaxed load is enough. >> + return __atomic_load_n(&this->_M_refcount, __ATOMIC_RELAXED) < >> 0; >> +#else >> + return this->_M_refcount < 0; >> +#endif >> + } >> >> bool >> _M_is_shared() const _GLIBCXX_NOEXCEPT >> - { return this->_M_refcount > 0; } >> + { >> +#if defined(__GTHREADS) >> + // _M_refcount is mutated concurrently by >> _M_refcopy/_M_dispose, >> + // so we need to use an atomic load. Another thread can drop >> last >> + // but one reference concurrently with this check, so we need >> this >> + // load to be acquire to synchronize with release fetch_and_add >> in >> + // _M_dispose. >> + return __atomic_load_n(&this->_M_refcount, __ATOMIC_ACQUIRE) > >> 0; >> +#else >> + return this->_M_refcount > 0; >> +#endif >> + } >> >> void >> _M_set_leaked() _GLIBCXX_NOEXCEPT > > Index: ChangeLog =================================================================== --- ChangeLog (revision 227400) +++ ChangeLog (working copy) @@ -1,3 +1,7 @@ +2015-09-02 Dmitry Vyukov + + * include/bits/basic_string.h: Fix data races on _M_refcount. + 2015-09-02 Sebastian Huber PR libstdc++/67408 Index: include/bits/basic_string.h =================================================================== --- include/bits/basic_string.h (revision 227400) +++ include/bits/basic_string.h (working copy) @@ -2601,11 +2601,32 @@ bool _M_is_leaked() const _GLIBCXX_NOEXCEPT - { return this->_M_refcount < 0; } + { +#if defined(__GTHREADS) + // _M_refcount is mutated concurrently by _M_refcopy/_M_dispose, + // so we need to use an atomic load. However, _M_is_leaked + // predicate does not change concurrently (i.e. the string is either + // leaked or not), so a relaxed load is enough. + return __atomic_load_n(&this->_M_refcount, __ATOMIC_RELAXED) < 0; +#else + return this->_M_refcount < 0; +#endif + } bool _M_is_shared() const _GLIBCXX_NOEXCEPT - { return this->_M_refcount > 0; } + { +#if defined(__GTHREADS) + // _M_refcount is mutated concurrently by _M_refcopy/_M_dispose, + // so we need to use an atomic load. Another thread can drop last + // but one reference concurrently with this check, so we need this + // load to be acquire to synchronize with release fetch_and_add in + // _M_dispose. + return __atomic_load_n(&this->_M_refcount, __ATOMIC_ACQUIRE) > 0; +#else + return this->_M_refcount > 0; +#endif + } void _M_set_leaked() _GLIBCXX_NOEXCEPT @@ -2654,6 +2675,14 @@ { // Be race-detector-friendly. For more info see bits/c++config. _GLIBCXX_SYNCHRONIZATION_HAPPENS_BEFORE(&this->_M_refcount); + // Decrement of _M_refcount is acq_rel, because: + // - all but last decrements need to release to synchronize with + // the last decrement that will delete the object. + // - the last decrement needs to acquire to synchronize with + // all the previous decrements. + // - last but one decrement needs to release to synchronize with + // the acquire load in _M_is_shared that will conclude that + // the object is not shared anymore. if (__gnu_cxx::__exchange_and_add_dispatch(&this->_M_refcount, -1) <= 0) {