diff mbox

[libstdc++] Fix data races in basic_string implementation

Message ID CACT4Y+aDrmSvqxZ_mX6V7yF6qEhf-L8KLchL0FUd-dx4xNYnEg@mail.gmail.com
State New
Headers show

Commit Message

Dmitry Vyukov Sept. 2, 2015, 2:01 p.m. UTC 
Added comment to _M_dispose and restored ChangeLog entry.
Please take another look.


On Wed, Sep 2, 2015 at 3:17 PM, Jonathan Wakely <jwakely@redhat.com> wrote:
> 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
>
>

Comments

Jonathan Wakely Sept. 2, 2015, 2:08 p.m. UTC | #1 
On 02/09/15 16:01 +0200, Dmitry Vyukov wrote:
>Added comment to _M_dispose and restored ChangeLog entry.
>Please take another look.

Thanks, this is OK for trunk.

I assume you are covered by the Google company-wide copyright
assignment, so someone just needs to commit it, which I can do if you
like.
Dmitry Vyukov Sept. 2, 2015, 2:38 p.m. UTC | #2 
Thank you.

Yes, I am covered by the Google copyright assignment, and I have commit access.
I don't commit frequently, hope I didn't mess up things fundamentally :)
https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=227403



On Wed, Sep 2, 2015 at 4:08 PM, Jonathan Wakely <jwakely@redhat.com> wrote:
> On 02/09/15 16:01 +0200, Dmitry Vyukov wrote:
>>
>> Added comment to _M_dispose and restored ChangeLog entry.
>> Please take another look.
>
>
> Thanks, this is OK for trunk.
>
> I assume you are covered by the Google company-wide copyright
> assignment, so someone just needs to commit it, which I can do if you
> like.
>
diff mbox

Patch

Index: ChangeLog
===================================================================
--- ChangeLog	(revision 227400)
+++ ChangeLog	(working copy)
@@ -1,3 +1,7 @@ 
+2015-09-02  Dmitry Vyukov  <dvyukov@google.com>
+
+	* include/bits/basic_string.h: Fix data races on _M_refcount.
+
 2015-09-02  Sebastian Huber  <sebastian.huber@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)
 		{