shared_ptr : atomic ref-counts (TSan)

[os12]

Summary

This is to do with TSan instrumentation, see http://llvm.org/bugs/show_bug.cgi?id=22836

[EricWF]

This patch breaks the ABI.

[os12]

OK, two questions:

1. Are there means to evolve the ABI?
2. There are always people who build their world from scratch. Is there a way to enable such a change conditionally?

[majnemer]

__sync_add_and_fetch is lowered to atomicrmw add with seq_cst ordering, just like std::atomic<>::operator++. Same with __sync_add_and_fetch and std::atomic<>::operator-- and __sync_bool_compare_and_swap and std::atomic<>:: compare_exchange_strong

What is the functional difference here?

[os12]

Every load of the counter is now atomic and that is both correct and pleasing to TSan.

Eg, as per the aforementioned bug report:

99	__shared_weak_count*
100	__shared_weak_count::lock() _NOEXCEPT
101	{
102	    long object_owners = __shared_owners_;
103	    while (object_owners != -1)
104	    {
105	        if (__sync_bool_compare_and_swap(&__shared_owners_,
106	                                         object_owners,
107	                                         object_owners+1))
108	            return this;
109	        object_owners = __shared_owners_;
110	    }
111	    return 0;
112	}

[majnemer]

In D8147#136216, @os12 wrote:

Every load of the counter is now atomic and that is both correct and pleasing to TSan.

Eg, as per the aforementioned bug report:

99	__shared_weak_count*
100	__shared_weak_count::lock() _NOEXCEPT
101	{
102	    long object_owners = __shared_owners_;
103	    while (object_owners != -1)
104	    {
105	        if (__sync_bool_compare_and_swap(&__shared_owners_,
106	                                         object_owners,
107	                                         object_owners+1))
108	            return this;
109	        object_owners = __shared_owners_;
110	    }
111	    return 0;
112	}

Ah, so it is our crummy attempt at a relaxed load. Would the implementation be correct if we kept everything the same but used __atomic_load to do the loads of _shared_owners_?

[os12]

Unfortunately I don't know whether that is going to be picked up by TSan. Could you ask in the bug please? (so that we avoid playing the broken telephone game).

P.S. I have anecdotal evidence that TSan is pacified with the full std::atomic annotations, but that is quite a different thing.

[majnemer]

Adding cfe-commits to the review.

[mclow.lists]

1. Are there means to evolve the ABI?

I started a discussion about this a month or so ago, but it petered out w/o a conclusion.
There are users of libc++ for whom *any* ABI change is unacceptable.
There are also users of libc++ who (as you say), "build the world from scratch" regularly, for whom ABI change is a non-issue.

We need a scheme that satisfies the first group w/o limiting the second.

As for this change, would "fixing" the load on line #102 fix the observed problem?

[EricWF]

I was recently corrected. The change for long to std::atomic<long> doesn't break the ABI because they are layout compatible and there is no constructor code.

However

1. <atomic> only compiles with clang in c++11 mode and with some GCC versions. This restriction is somewhat artificial.
2. This is a pessimization when the program is single threaded. Not much we can do about that at this point though.

Neither of these things should block the patch but we should be aware.

@mclow.lists This looks like a good reason to lift the restrictions on atomic.

[EricWF]

I think a much safer change would be to use the __atomic_foo builtins. It means we wouldn't have to change the type stored in __shared_count and __shared_weak_count and we wouldn't depend on the <atomic> header.

Is there a reason this approach was not taken?

[mclow.lists]

Is this patch superseded by D10406 ? http://reviews.llvm.org/D10406

[EricWF]

Yep, This patch was superseded by D10406 which has been committed. I'm going to go ahead and resign from this revision but it should be closed.

include/memory

     __shared_count& operator=(const __shared_count&);
 
 protected:
-    long __shared_owners_;
+    std::atomic<long> __shared_owners_;
     virtual ~__shared_count();
 private:
     virtual void __on_zero_shared() _NOEXCEPT = 0;

 class _LIBCPP_TYPE_VIS __shared_weak_count
     : private __shared_count
 {
-    long __shared_weak_owners_;
+    std::atomic<long> __shared_weak_owners_;
 
 public:
     _LIBCPP_INLINE_VISIBILITY

src/memory.cpp

 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
-namespace
-{
-
-template <class T>
-inline T
-increment(T& t) _NOEXCEPT
-{
-    return __sync_add_and_fetch(&t, 1);
-}
-
-template <class T>
-inline T
-decrement(T& t) _NOEXCEPT
-{
-    return __sync_add_and_fetch(&t, -1);
-}
-
-}  // namespace
-
 const allocator_arg_t allocator_arg = allocator_arg_t();
 
 bad_weak_ptr::~bad_weak_ptr() _NOEXCEPT {}

 void
 __shared_count::__add_shared() _NOEXCEPT
 {
-    increment(__shared_owners_);
+    ++__shared_owners_;
 }
 
 bool
 __shared_count::__release_shared() _NOEXCEPT
 {
-    if (decrement(__shared_owners_) == -1)
+    if (__shared_owners_.fetch_add(-1) == 0)
     {
         __on_zero_shared();
         return true;

 void
 __shared_weak_count::__add_weak() _NOEXCEPT
 {
-    increment(__shared_weak_owners_);
+    ++__shared_weak_owners_;
 }
 
 void

 void
 __shared_weak_count::__release_weak() _NOEXCEPT
 {
-    if (decrement(__shared_weak_owners_) == -1)
+    if (__shared_weak_owners_.fetch_add(-1) == 0)
         __on_zero_shared_weak();
 }
 

     long object_owners = __shared_owners_;
     while (object_owners != -1)
     {
-        if (__sync_bool_compare_and_swap(&__shared_owners_,
-                                         object_owners,
-                                         object_owners+1))
+        if (__shared_owners_.compare_exchange_strong(object_owners,
+                                                     object_owners + 1))
             return this;
         object_owners = __shared_owners_;
     }