From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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-Archive: List-Post: List-Help: Sender: gcc-patches-owner@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, 02 Sep 2015 14:02:00 -0000 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 Content-Type: multipart/mixed; boundary=001a11c37be0980276051ec41c49 X-IsSubscribed: yes X-SW-Source: 2015-09/txt/msg00161.txt.bz2 --001a11c37be0980276051ec41c49 Content-Type: text/plain; charset=UTF-8 Content-length: 3607 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 > > --001a11c37be0980276051ec41c49 Content-Type: text/plain; charset=US-ASCII; name="patch.diff" Content-Disposition: attachment; filename="patch.diff" Content-Transfer-Encoding: base64 X-Attachment-Id: f_ie2uqe2f0 Content-length: 3571 SW5kZXg6IENoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBD aGFuZ2VMb2cJKHJldmlzaW9uIDIyNzQwMCkKKysrIENoYW5nZUxvZwkod29y a2luZyBjb3B5KQpAQCAtMSwzICsxLDcgQEAKKzIwMTUtMDktMDIgIERtaXRy eSBWeXVrb3YgIDxkdnl1a292QGdvb2dsZS5jb20+CisKKwkqIGluY2x1ZGUv Yml0cy9iYXNpY19zdHJpbmcuaDogRml4IGRhdGEgcmFjZXMgb24gX01fcmVm Y291bnQuCisKIDIwMTUtMDktMDIgIFNlYmFzdGlhbiBIdWJlciAgPHNlYmFz dGlhbi5odWJlckBlbWJlZGRlZC1icmFpbnMuZGU+CiAKIAlQUiBsaWJzdGRj KysvNjc0MDgKSW5kZXg6IGluY2x1ZGUvYml0cy9iYXNpY19zdHJpbmcuaAo9 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09Ci0tLSBpbmNsdWRlL2JpdHMvYmFzaWNf c3RyaW5nLmgJKHJldmlzaW9uIDIyNzQwMCkKKysrIGluY2x1ZGUvYml0cy9i YXNpY19zdHJpbmcuaAkod29ya2luZyBjb3B5KQpAQCAtMjYwMSwxMSArMjYw MSwzMiBAQAogCiAgICAgICAgIGJvb2wKIAlfTV9pc19sZWFrZWQoKSBjb25z dCBfR0xJQkNYWF9OT0VYQ0VQVAotICAgICAgICB7IHJldHVybiB0aGlzLT5f TV9yZWZjb3VudCA8IDA7IH0KKyAgICAgICAgeworI2lmIGRlZmluZWQoX19H VEhSRUFEUykKKyAgICAgICAgICAvLyBfTV9yZWZjb3VudCBpcyBtdXRhdGVk IGNvbmN1cnJlbnRseSBieSBfTV9yZWZjb3B5L19NX2Rpc3Bvc2UsCisgICAg ICAgICAgLy8gc28gd2UgbmVlZCB0byB1c2UgYW4gYXRvbWljIGxvYWQuIEhv d2V2ZXIsIF9NX2lzX2xlYWtlZAorICAgICAgICAgIC8vIHByZWRpY2F0ZSBk b2VzIG5vdCBjaGFuZ2UgY29uY3VycmVudGx5IChpLmUuIHRoZSBzdHJpbmcg aXMgZWl0aGVyCisgICAgICAgICAgLy8gbGVha2VkIG9yIG5vdCksIHNvIGEg cmVsYXhlZCBsb2FkIGlzIGVub3VnaC4KKyAgICAgICAgICByZXR1cm4gX19h dG9taWNfbG9hZF9uKCZ0aGlzLT5fTV9yZWZjb3VudCwgX19BVE9NSUNfUkVM QVhFRCkgPCAwOworI2Vsc2UKKyAgICAgICAgICByZXR1cm4gdGhpcy0+X01f cmVmY291bnQgPCAwOworI2VuZGlmCisgICAgICAgIH0KIAogICAgICAgICBi b29sCiAJX01faXNfc2hhcmVkKCkgY29uc3QgX0dMSUJDWFhfTk9FWENFUFQK LSAgICAgICAgeyByZXR1cm4gdGhpcy0+X01fcmVmY291bnQgPiAwOyB9CisJ eworI2lmIGRlZmluZWQoX19HVEhSRUFEUykKKyAgICAgICAgICAvLyBfTV9y ZWZjb3VudCBpcyBtdXRhdGVkIGNvbmN1cnJlbnRseSBieSBfTV9yZWZjb3B5 L19NX2Rpc3Bvc2UsCisgICAgICAgICAgLy8gc28gd2UgbmVlZCB0byB1c2Ug YW4gYXRvbWljIGxvYWQuIEFub3RoZXIgdGhyZWFkIGNhbiBkcm9wIGxhc3QK KyAgICAgICAgICAvLyBidXQgb25lIHJlZmVyZW5jZSBjb25jdXJyZW50bHkg d2l0aCB0aGlzIGNoZWNrLCBzbyB3ZSBuZWVkIHRoaXMKKyAgICAgICAgICAv LyBsb2FkIHRvIGJlIGFjcXVpcmUgdG8gc3luY2hyb25pemUgd2l0aCByZWxl YXNlIGZldGNoX2FuZF9hZGQgaW4KKyAgICAgICAgICAvLyBfTV9kaXNwb3Nl LgorICAgICAgICAgIHJldHVybiBfX2F0b21pY19sb2FkX24oJnRoaXMtPl9N X3JlZmNvdW50LCBfX0FUT01JQ19BQ1FVSVJFKSA+IDA7CisjZWxzZQorICAg ICAgICAgIHJldHVybiB0aGlzLT5fTV9yZWZjb3VudCA+IDA7CisjZW5kaWYK KyAgICAgICAgfQogCiAgICAgICAgIHZvaWQKIAlfTV9zZXRfbGVha2VkKCkg X0dMSUJDWFhfTk9FWENFUFQKQEAgLTI2NTQsNiArMjY3NSwxNCBAQAogCSAg ICB7CiAJICAgICAgLy8gQmUgcmFjZS1kZXRlY3Rvci1mcmllbmRseS4gIEZv ciBtb3JlIGluZm8gc2VlIGJpdHMvYysrY29uZmlnLgogCSAgICAgIF9HTElC Q1hYX1NZTkNIUk9OSVpBVElPTl9IQVBQRU5TX0JFRk9SRSgmdGhpcy0+X01f cmVmY291bnQpOworICAgICAgICAgICAgICAvLyBEZWNyZW1lbnQgb2YgX01f cmVmY291bnQgaXMgYWNxX3JlbCwgYmVjYXVzZToKKyAgICAgICAgICAgICAg Ly8gLSBhbGwgYnV0IGxhc3QgZGVjcmVtZW50cyBuZWVkIHRvIHJlbGVhc2Ug dG8gc3luY2hyb25pemUgd2l0aAorICAgICAgICAgICAgICAvLyAgIHRoZSBs YXN0IGRlY3JlbWVudCB0aGF0IHdpbGwgZGVsZXRlIHRoZSBvYmplY3QuCisg ICAgICAgICAgICAgIC8vIC0gdGhlIGxhc3QgZGVjcmVtZW50IG5lZWRzIHRv IGFjcXVpcmUgdG8gc3luY2hyb25pemUgd2l0aAorICAgICAgICAgICAgICAv LyAgIGFsbCB0aGUgcHJldmlvdXMgZGVjcmVtZW50cy4KKyAgICAgICAgICAg ICAgLy8gLSBsYXN0IGJ1dCBvbmUgZGVjcmVtZW50IG5lZWRzIHRvIHJlbGVh c2UgdG8gc3luY2hyb25pemUgd2l0aAorICAgICAgICAgICAgICAvLyAgIHRo ZSBhY3F1aXJlIGxvYWQgaW4gX01faXNfc2hhcmVkIHRoYXQgd2lsbCBjb25j bHVkZSB0aGF0CisgICAgICAgICAgICAgIC8vICAgdGhlIG9iamVjdCBpcyBu b3Qgc2hhcmVkIGFueW1vcmUuCiAJICAgICAgaWYgKF9fZ251X2N4eDo6X19l eGNoYW5nZV9hbmRfYWRkX2Rpc3BhdGNoKCZ0aGlzLT5fTV9yZWZjb3VudCwK IAkJCQkJCQkgLTEpIDw9IDApCiAJCXsK --001a11c37be0980276051ec41c49--