Added comment to _M_dispose and restored ChangeLog entry. Please take another look.
On Wed, Sep 2, 2015 at 3:17 PM, Jonathan Wakely <jwak...@redhat.com> wrote: > On 01/09/15 17:42 +0200, Dmitry Vyukov wrote: >> >> On Tue, Sep 1, 2015 at 5:08 PM, Jonathan Wakely <jwak...@redhat.com> >> 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 <dvyu...@google.com> + + * include/bits/basic_string.h: Fix data races on _M_refcount. + 2015-09-02 Sebastian Huber <sebastian.hu...@embedded-brains.de> 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) {