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

[Jonathan Wakely <jwakely at redhat dot 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