Re: libstdc++ and race detectors

[Kostya Serebryany <kcc at google dot com>]

On Wed, Aug 11, 2010 at 2:36 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> On 08/11/2010 12:23 PM, Kostya Serebryany wrote:
>> On Wed, Aug 11, 2010 at 2:21 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
>>
>>> ... by the way, why are you touching only those two headers? Didn't we
>>> agree to change consistently all the occurrences of
>>>
>> We did agree on that, I just wanted to have the first patch to be
>> simpler and cover only a few files.
>> Shall I add everything into one patch?
>>
> Oh yes, the changes are mechanical, isn't that we have yet to decide
> whether we want to do this kind of change or not, basing on testing or
> something similar.

I changed the patch:
  - removed the docs section (but leaved small docs section in c++config)
  - annotated all other cases of refcount decrement
  - tried hard to follow the coding style.
http://codereview.appspot.com/download/issue1962044_1.diff (inlined below)

> Well, it's true that without a Copyright assignment
> (right? Or you are covered by Google?) we are on risky grounds, but if
> you can keep the patch otherwise as simple as possible, we can still
> make it this time.

I was assured that I am 'covered by Google'

> And yes, maybe better taking out from the comment the
> documentation bits and opening a PR as a reminder to ourselves.

How do I do it?


--kcc

Index: src/ios_init.cc
===================================================================
--- src/ios_init.cc	(revision 163093)
+++ src/ios_init.cc	(working copy)
@@ -122,8 +122,11 @@

   ios_base::Init::~Init()
   {
+    // Be race-detector-friendly. For more info see bits/c++config.
+    _GLIBCXX_SYNCHRONIZATION_HAPPENS_BEFORE(&_S_refcount);
     if (__gnu_cxx::__exchange_and_add_dispatch(&_S_refcount, -1) == 2)
       {
+        _GLIBCXX_SYNCHRONIZATION_HAPPENS_AFTER(&_S_refcount);
 	// Catch any exceptions thrown by basic_ostream::flush()
 	__try
 	  {
Index: include/tr1_impl/boost_sp_counted_base.h
===================================================================
--- include/tr1_impl/boost_sp_counted_base.h	(revision 163093)
+++ include/tr1_impl/boost_sp_counted_base.h	(working copy)
@@ -139,8 +139,11 @@
       void
       _M_release() // nothrow
       {
+        // Be race-detector-friendly. For more info see bits/c++config.
+        _GLIBCXX_SYNCHRONIZATION_HAPPENS_BEFORE(&_M_use_count);
 	if (__gnu_cxx::__exchange_and_add_dispatch(&_M_use_count, -1) == 1)
 	  {
+            _GLIBCXX_SYNCHRONIZATION_HAPPENS_AFTER(&_M_use_count);
 	    _M_dispose();
 	    // There must be a memory barrier between dispose() and destroy()
 	    // to ensure that the effects of dispose() are observed in the
@@ -152,9 +155,14 @@
 	        _GLIBCXX_WRITE_MEM_BARRIER;
 	      }

+            // Be race-detector-friendly. For more info see bits/c++config.
+            _GLIBCXX_SYNCHRONIZATION_HAPPENS_BEFORE(&_M_weak_count);
 	    if (__gnu_cxx::__exchange_and_add_dispatch(&_M_weak_count,
 						       -1) == 1)
-	      _M_destroy();
+              {
+                _GLIBCXX_SYNCHRONIZATION_HAPPENS_AFTER(&_M_weak_count);
+	        _M_destroy();
+              }
 	  }
       }

@@ -165,8 +173,11 @@
       void
       _M_weak_release() // nothrow
       {
+        // Be race-detector-friendly. For more info see bits/c++config.
+        _GLIBCXX_SYNCHRONIZATION_HAPPENS_BEFORE(&_M_weak_count);
 	if (__gnu_cxx::__exchange_and_add_dispatch(&_M_weak_count, -1) == 1)
 	  {
+            _GLIBCXX_SYNCHRONIZATION_HAPPENS_AFTER(&_M_weak_count);
 	    if (_Mutex_base<_Lp>::_S_need_barriers)
 	      {
 	        // See _M_release(),
Index: include/bits/locale_classes.h
===================================================================
--- include/bits/locale_classes.h	(revision 163093)
+++ include/bits/locale_classes.h	(working copy)
@@ -402,8 +402,11 @@
     void
     _M_remove_reference() const throw()
     {
+      // Be race-detector-friendly. For more info see bits/c++config.
+      _GLIBCXX_SYNCHRONIZATION_HAPPENS_BEFORE(&_M_refcount);
       if (__gnu_cxx::__exchange_and_add_dispatch(&_M_refcount, -1) == 1)
 	{
+          _GLIBCXX_SYNCHRONIZATION_HAPPENS_AFTER(&_M_refcount);
 	  __try
 	    { delete this; }
 	  __catch(...)
@@ -508,8 +511,11 @@
     void
     _M_remove_reference() throw()
     {
+      // Be race-detector-friendly. For more info see bits/c++config.
+      _GLIBCXX_SYNCHRONIZATION_HAPPENS_BEFORE(&_M_refcount);
       if (__gnu_cxx::__exchange_and_add_dispatch(&_M_refcount, -1) == 1)
 	{
+          _GLIBCXX_SYNCHRONIZATION_HAPPENS_AFTER(&_M_refcount);
 	  __try
 	    { delete this; }
 	  __catch(...)
Index: include/bits/basic_string.h
===================================================================
--- include/bits/basic_string.h	(revision 163093)
+++ include/bits/basic_string.h	(working copy)
@@ -232,9 +232,14 @@
 #ifndef _GLIBCXX_FULLY_DYNAMIC_STRING
 	  if (__builtin_expect(this != &_S_empty_rep(), false))
 #endif
+            // Be race-detector-friendly. For more info see bits/c++config.
+            _GLIBCXX_SYNCHRONIZATION_HAPPENS_BEFORE(&this->_M_refcount);
 	    if (__gnu_cxx::__exchange_and_add_dispatch(&this->_M_refcount,
 						       -1) <= 0)
-	      _M_destroy(__a);
+              {
+                _GLIBCXX_SYNCHRONIZATION_HAPPENS_AFTER(&this->_M_refcount);
+	        _M_destroy(__a);
+              }
 	}  // XXX MT

 	void
Index: include/bits/ios_base.h
===================================================================
--- include/bits/ios_base.h	(revision 163093)
+++ include/bits/ios_base.h	(working copy)
@@ -474,7 +474,16 @@
       // 0 => OK to delete.
       int
       _M_remove_reference()
-      { return __gnu_cxx::__exchange_and_add_dispatch(&_M_refcount, -1); }
+      {
+        // Be race-detector-friendly. For more info see bits/c++config.
+        _GLIBCXX_SYNCHRONIZATION_HAPPENS_BEFORE(&_M_refcount);
+        int __res = _gnu_cxx::__exchange_and_add_dispatch(&_M_refcount, -1);
+        if (__res == 0)
+          {
+            _GLIBCXX_SYNCHRONIZATION_HAPPENS_AFTER(&_M_refcount);
+          }
+        return __res;
+      }
     };

      _Callback_list*	_M_callbacks;
@@ -962,4 +971,3 @@
 _GLIBCXX_END_NAMESPACE

 #endif /* _IOS_BASE_H */
-
Index: include/bits/c++config
===================================================================
--- include/bits/c++config	(revision 163093)
+++ include/bits/c++config	(working copy)
@@ -60,6 +60,40 @@
 # define _GLIBCXX_DEPRECATED_ATTR
 #endif

+// Macros for race detectors.
+// _GLIBCXX_SYNCHRONIZATION_HAPPENS_BEFORE(x) and
+// _GLIBCXX_SYNCHRONIZATION_HAPPENS_AFTER(x) should be used to explain
+// atomic (lock-free) synchronization to race detectors:
+// the race detector will infer a happens-before arc from the former to the
+// latter when they share the same argument pointer.
+//
+// The most frequent use case for these macros (and the only case in the
+// current implementation of the library) is atomic reference counting:
+//   void _M_remove_reference()
+//   {
+//     _GLIBCXX_SYNCHRONIZATION_HAPPENS_BEFORE(&this->_M_refcount);
+//     if (__gnu_cxx::__exchange_and_add_dispatch(&this->_M_refcount, -1) <= 0)
+//       {
+//         _GLIBCXX_SYNCHRONIZATION_HAPPENS_AFTER(&this->_M_refcount);
+//         _M_destroy(__a);
+//       }
+//   }
+// The annotations in this example tell the race detector that all memory
+// accesses occurred when the refcount was positive do not race with
+// memory accesses which occurred after the refcount became zero.
+
+// -------------- end docs ----------
+
+
+#ifndef _GLIBCXX_SYNCHRONIZATION_HAPPENS_BEFORE
+# define  _GLIBCXX_SYNCHRONIZATION_HAPPENS_BEFORE(a)
+#endif
+#ifndef _GLIBCXX_SYNCHRONIZATION_HAPPENS_AFTER
+# define  _GLIBCXX_SYNCHRONIZATION_HAPPENS_AFTER(a)
+#endif
+
+
+
 // Macros for activating various namespace association modes.
 // _GLIBCXX_NAMESPACE_ASSOCIATION_DEBUG
 // _GLIBCXX_NAMESPACE_ASSOCIATION_PARALLEL