Re: libstdc++ and race detectors

[Kostya Serebryany <kcc at google dot com>]

Ping?
How do we proceed with the patch
http://codereview.appspot.com/download/issue1800042_16001.diff??
Thanks,
--kcc


Index: tr1_impl/boost_sp_counted_base.h
===================================================================
--- tr1_impl/boost_sp_counted_base.h	(revision 162071)
+++ 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)
+            {
+              _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: bits/basic_string.h
===================================================================
--- bits/basic_string.h	(revision 162071)
+++ 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)
+            {
+              _GLIBCXX_SYNCHRONIZATION_HAPPENS_AFTER(&this->_M_refcount);
 	      _M_destroy(__a);
+            }
 	}  // XXX MT

 	void
Index: bits/c++config
===================================================================
--- bits/c++config	(revision 162071)
+++ bits/c++config	(working copy)
@@ -60,6 +60,93 @@
 # 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.
+//
+// For more details refer to <TODO: link to DOCS (below)>
+
+/* move to DOCS:
+Support for race detectors.
+
+All synchronization primitives used in the library internals should be
+understood by race detectors so that the race detectors do not produce
+false reports.
+
+We use two annotations (macros) to explain low-level
+synchronization to race detectors:
+ - _GLIBCXX_SYNCHRONIZATION_HAPPENS_BEFORE() and
+ - _GLIBCXX_SYNCHRONIZATION_HAPPENS_AFTER().
+
+By default, these two macros are defined empty -- anyone who wants
+to use a race detector will need to redefine these macros to call
+appropriate race detector API.
+
+Since these macros are empty by default, redefining them in the user code
+will affect only the inline template code, i.e.
+ - shared_ptr
+ - foo
+ - bar
+
+In order to redefine the macros in basic_string one will need to disable
+extern templates (by defining _GLIBCXX_EXTERN_TEMPLATE=-1) or rebuild the .so.
+
+The rest of the cases will require to rebuild the .so:
+  - ios_base::Init::~Init
+  - bazz
+  - zoo
+
+The approach described above works at least with the following
+race detection tools:
+ - DRD, http://valgrind.org/docs/manual/drd-manual.html
+ - Helgrind, http://valgrind.org/docs/manual/hg-manual.html
+ - Intel Parallel Inspector,
http://software.intel.com/en-us/intel-parallel-inspector
+ - ThreadSanitizer, http://code.google.com/p/data-race-test
+
+With DRD, Helgrind and ThreadSanitizer you will need to define
+the macros like this:
+#define _GLIBCXX_SYNCHRONIZATION_HAPPENS_BEFORE(a) ANNOTATE_HAPPENS_BEFORE(a)
+#define _GLIBCXX_SYNCHRONIZATION_HAPPENS_AFTER(a) ANNOTATE_HAPPENS_AFTER(a)
+
+For Intel Parallel Inspector:
+#define _GLIBCXX_SYNCHRONIZATION_HAPPENS_BEFORE(a) itt_notify_sync_releasing(a)
+#define _GLIBCXX_SYNCHRONIZATION_HAPPENS_AFTER(a) itt_notify_sync_acquired(a)
+
+(refer to documentation of a particular tool for the details).
+
+*/
+
+
+
+// -------------- 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



On Mon, Jul 19, 2010 at 1:02 PM, Kostya Serebryany <kcc@google.com> wrote:
>
>
> On Wed, Jul 14, 2010 at 6:00 PM, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
>>
>> On 14 July 2010 13:54, Kostya Serebryany wrote:
>> >> You know, my idea would be, adding the macros for consistency
>> >> *everywhere* an atomic counter is decreased (I gather this is the
>> >> technical core of the problem). And then, in the docs, point out which
>> >> are effective (that is, able to actually suppress the warnings) with a
>> >> normally built library, which require the extern template trick on the
>> >> command line (I think only basic_string), which require a library
>> >> rebuild. I'm not saying you have to provide the whole package at once,
>> >> for instance you can split out the documentation work, but please commit
>> >> to something consistent for 4.6.x,
>> > Which is when?
>> > I do commit if there is enough time.
>>
>> Stage 1 started in April, so I would be surprised if 4.6.0 is released
>> before next April,
>
> Plenty of time. I commit, it's in my best interest.
> If libstdc++ will cause numerous false race reports in basic_string and (worse!) shared_ptr, the race detection tools will become much less usable.
> Any comment on the patch?
> http://codereview.appspot.com/download/issue1800042_16001.diff
> (I'd prefer to annotate other parts as a separate checkin. )
>
>>
>> based on the timelines of recent releases:
>> http://gcc.gnu.org/develop.html#timeline
>>
>> > A related question: does libstdc++ have other kinds of atomic
>> > synchronization (e.g. lock free queues, locks free hash maps, etc)?
>>
>> There are no lock free containers in libstdc++. ?We use atomics for
>> reference-counting in a few places that you already know (strings,
>> shared_ptr, locales, ios::Init) and also for exception handling in the
>> runtime, see libsupc++/eh_throw.cc and libsupc++/eh_ptr.cc (both of
>> these are exported in a .so and would need recompilation for
>> annotations to help.)
>>
>> The C++0x thread features (<thread>, <mutex>, (condition_variable> and
>> <future>) are built from pthreads (or similar) primitives, so are not
>> a problem for race detectors. ?The C++0x <atomic> header is only an
>> experimental implementation and is not used by the library. Programs
>> which make use of those features would need to add their own
>> annotations as required.
>
> Yes, right.
>
> --kcc
>>
>> The profile mode and parallel mode use some atomic operations in
>> places, but I'm not sure exactly how they're used or what for. Not
>> reference counting AFAIK.
>