On Tue, Sep 1, 2015 at 5:08 PM, Jonathan Wakely <jwak...@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.
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
