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

[Dmitry Vyukov <dvyukov@google.com>]

[-- Attachment #1: Type: text/plain, Size: 1405 bytes --]

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.

[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 1449 bytes --]

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