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
>
>