[Patch, libstdc++] Fix data races in basic_string implementation
Jonathan Wakely
jwakely@redhat.com
Wed Sep 2 13:17:00 GMT 2015
On 01/09/15 17:42 +0200, Dmitry Vyukov wrote:
>On Tue, Sep 1, 2015 at 5:08 PM, Jonathan Wakely <jwakely@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
More information about the Gcc-patches
mailing list