Re: [Patch, libstdc++] Fix data races in basic_string implementation

[Jonathan Wakely <jwakely@redhat.com>]

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