Re: [patch] Make std::tr1::shared_ptr thread-safe.

["Peter Dimov" <pdimov at mmltd dot net>]

Jonathan Wakely wrote:
> These patches make std::tr1::shared_ptr thread-safe and change some
> tests to check equality, not inequality, since we know what value we
> expect and it's better to be precise about what we're checking.

It is, unfortunately, still not thread safe. There are two memory visibility 
issues with the code.

 void
 release() // nothrow
 {
   if (__gnu_cxx::__exchange_and_add(&_M_use_count, -1) == 1)
   {
     dispose();
     weak_release();
   }
 }

 void
 weak_release() // nothrow
 {
   if (__gnu_cxx::__exchange_and_add(&_M_weak_count, -1) == 1)
   {
     destroy();
   }
 }

The first problem is with mutable synchronized pointees:

// thread A

p1->f(); // modifies *p1
p1.reset();

// thread B

p2.reset(); // calls destructor

Assume that p1 and p2, both of type shared_ptr<X>, share ownership and are 
the only remaining references to *p1. f() is synchronized, but ~X may not 
be. In order to fix this, release() needs to be changed as follows:

 void
 release() // nothrow
 {
   if (__gnu_cxx::__exchange_and_add(&_M_use_count, -1) == 1)
   {
     __acquire_memory_barrier();
     dispose();
     weak_release();
   }
   else
   {
     __release_memory_barrier();
   }
 }

(here I am assuming that __exchange_and_add has unspecified memory 
synchronization properties.)

I'm inclined to "ignore" this problem from shared_ptr's side and rely on 
documentation or custom deleters instead to make X responsible for its 
synchronization, because the barriers on every release() may introduce a 
significant performance hit, at least in theory. (An empty lock/unlock at 
the beginning of ~X would be enough.)

The second problem is that there must exist a release/acquire pair between 
the dispose() and destroy() calls, otherwise ~_Deleter will not be able to 
observe the effects of _M_del( _M_ptr ).

I think that it is unreasonable to push that responsibility towards users, 
because here the object (the deleter) does not need synchronization for its 
normal operation, as in the other example.

This problem can be fixed as follows:

 void
 release() // nothrow
 {
   if (__gnu_cxx::__exchange_and_add(&_M_use_count, -1) == 1)
   {
     dispose();
     __release_memory_barrier();
     weak_release();
   }
 }

 void
 weak_release() // nothrow
 {
   if (__gnu_cxx::__exchange_and_add(&_M_weak_count, -1) == 1)
   {
     __acquire_memory_barrier();
     destroy();
   }
 }

Unfortunately, while libstdc++ has _GLIBCXX_READ_MEM_BARRIER and 
_GLIBCXX_WRITE_MEM_BARRIER (which purpose I don't understand that well), 
these cannot be used as an acquire/release pair, IIUC. (But I barely 
understand memory visibility.)

The only reliable implementation of these barriers that I see is an empty 
pthread_mutex_lock/pthread_mutex_unlock pair. This is likely to be a bit 
more expensive than necessary.