Proposed patch to skip the last atomic decrements in _Sp_counted_base::_M_release

[Jonathan Wakely]

On 07/12/20 15:57 +0000, Jonathan Wakely wrote:
>On 07/12/20 07:30 -0500, Maged Michael via Libstdc++ wrote:
>>      void
>>      _M_release() noexcept
>>+      {
>>+        if (!__LP64__ || sizeof(_Atomic_word) != 4 || sizeof(long long) !=
>>8)
>>+          {
>>+            _M_release_orig();
>>+            return;
>>+          }
>>+        _GLIBCXX_SYNCHRONIZATION_HAPPENS_BEFORE(&_M_use_count);
>>+        _GLIBCXX_SYNCHRONIZATION_HAPPENS_BEFORE(&_M_weak_count);
>>+        if ((__atomic_load_n((long long*)(&_M_use_count), __ATOMIC_ACQUIRE)
>>+            == 0x100000001))
>>+          {
>
>I think we want a comment here explaining what it's doing:
>
>            // Both counts are 1, so there are no weak references and
>            // we are releasing the last strong reference. No other
>            // threads can observe the effects of this _M_release()
>            // call (e.g. calling use_count()) without a data race.
>
>
>>+            _GLIBCXX_SYNCHRONIZATION_HAPPENS_AFTER(&_M_use_count);
>>+            _GLIBCXX_SYNCHRONIZATION_HAPPENS_AFTER(&_M_weak_count);
>>+            _M_dispose();
>
>_M_dispose() will run the destructor for the owned object. If that
>object has a reference to the shared_ptr then it can call use_count()
>on it. That isn't a data race, because it's within the same thread.
>Previously the object's destructor would have observed that the use
>count had dropped to zero. In this new implementation, the use count
>is still 1 when the object is destroyed.
>
>Similarly, the object's destructor could create a new weak_ptr from
>*this. Previously that would see that the use count is zero, and so
>fail. With the new code it would succeed, and increase the weak count,
>and be left with a dangling pointer after the destroy here:
>
>>+            _M_destroy();
>
>I think to avoid these problems you need to set both counts to 0
>before calling _M_dispose(). Those can be simple non-atomic writes
>though, since we know that only the current thread can observe those
>values without data races.

Ah, actually maybe this isn't a problem, because of how we implement
shared_ptr::reset() and shared_ptr::operator=.

We do:

       void
       reset() noexcept
       { __shared_ptr().swap(*this); }

Which means that when the final _M_release() is called via an unnamed
temporary, which the object's destructor cannot refer to.

But we should make sure we have a test verifying that. Something like
this:

#include <memory>

struct X
{
   ~X();
};

std::shared_ptr<X> p;
std::weak_ptr<X> w(p);

X::~X()
{
   w = p;
   if (!w.expired())
     std::terminate();
}

int main()
{
   p = std::make_shared<X>();
   p.reset();
   p = w.lock();
   if (p)
     std::terminate();
}