From: khizmax Date: Wed, 22 Apr 2015 20:12:06 +0000 (+0300) Subject: Fixed data races found by tsan X-Git-Tag: v2.1.0~245^2~17 X-Git-Url: http://demsky.eecs.uci.edu/git/?a=commitdiff_plain;h=82d022ac4979b71202ba70f5d0339f34ceb023c8;p=libcds.git Fixed data races found by tsan --- diff --git a/cds/compiler/clang/defs.h b/cds/compiler/clang/defs.h index 182a5b83..ff6fb794 100644 --- a/cds/compiler/clang/defs.h +++ b/cds/compiler/clang/defs.h @@ -53,6 +53,13 @@ // Inheriting constructors #define CDS_CXX11_INHERITING_CTOR + +// ************************************************* +// Features +#if defined(__has_feature) && __has_feature(thread_sanitizer) +# define CDS_THREAD_SANITIZER_ENABLED +#endif + // ************************************************* // Alignment macro diff --git a/cds/compiler/defs.h b/cds/compiler/defs.h index 1b4739b4..a87195bd 100644 --- a/cds/compiler/defs.h +++ b/cds/compiler/defs.h @@ -1,7 +1,7 @@ //$$CDS-header$$ -#ifndef CDSLIB_ARH_COMPILER_DEFS_H -#define CDSLIB_ARH_COMPILER_DEFS_H +#ifndef CDSLIB_COMPILER_DEFS_H +#define CDSLIB_COMPILER_DEFS_H /* Required C++11 features: @@ -37,4 +37,7 @@ # define CDS_EXPORT_API #endif -#endif // #ifndef CDSLIB_ARH_COMPILER_DEFS_H +// Features +#include + +#endif // #ifndef CDSLIB_COMPILER_DEFS_H diff --git a/cds/compiler/feature_tsan.h b/cds/compiler/feature_tsan.h new file mode 100644 index 00000000..926876ad --- /dev/null +++ b/cds/compiler/feature_tsan.h @@ -0,0 +1,46 @@ +//$$CDS-header$$ + +#ifndef CDSLIB_COMPILER_FEATURE_TSAN_H +#define CDSLIB_COMPILER_FEATURE_TSAN_H + +// Thread Sanitizer annotations. +// From https://groups.google.com/d/msg/thread-sanitizer/SsrHB7FTnTk/mNTGNLQj-9cJ + +#ifdef CDS_THREAD_SANITIZER_ENABLED +# define CDS_TSAN_ANNOTATE_HAPPENS_BEFORE(addr) AnnotateHappensBefore(__FILE__, __LINE__, (void*)(addr)) +# define CDS_TSAN_ANNOTATE_HAPPENS_AFTER(addr) AnnotateHappensAfter(__FILE__, __LINE__, (void*)(addr)) + +# define CDS_TSAN_ANNOTATE_IGNORE_READS_BEGIN AnnotateIgnoreReadsBegin(__FILE__, __LINE__) +# define CDS_TSAN_ANNOTATE_IGNORE_READS_END AnnotateIgnoreReadsEnd(__FILE__, __LINE__) +# define CDS_TSAN_ANNOTATE_IGNORE_WRITES_BEGIN AnnotateIgnoreWritesBegin(__FILE__, __LINE__) +# define CDS_TSAN_ANNOTATE_IGNORE_WRITES_END AnnotateIgnoreWritesEnd(__FILE__, __LINE__) +# define CDS_TSAN_ANNOTATE_IGNORE_RW_BEGIN \ + CDS_TSAN_ANNOTATE_IGNORE_READS_BEGIN; \ + CDS_TSAN_ANNOTATE_IGNORE_WRITES_BEGIN +# define CDS_TSAN_ANNOTATE_IGNORE_RW_END \ + CDS_TSAN_ANNOTATE_IGNORE_WRITES_END;\ + CDS_TSAN_ANNOTATE_IGNORE_READS_END + + // provided by TSan + extern "C" { + void AnnotateHappensBefore(const char *f, int l, void *addr); + void AnnotateHappensAfter(const char *f, int l, void *addr); + + void AnnotateIgnoreReadsBegin(const char *f, int l); + void AnnotateIgnoreReadsEnd(const char *f, int l); + void AnnotateIgnoreWritesBegin(const char *f, int l); + void AnnotateIgnoreWritesEnd(const char *f, int l); + } +#else +# define CDS_TSAN_ANNOTATE_HAPPENS_BEFORE(addr) +# define CDS_TSAN_ANNOTATE_HAPPENS_AFTER(addr) + +# define CDS_TSAN_ANNOTATE_IGNORE_READS_BEGIN +# define CDS_TSAN_ANNOTATE_IGNORE_READS_END +# define CDS_TSAN_ANNOTATE_IGNORE_WRITES_BEGIN +# define CDS_TSAN_ANNOTATE_IGNORE_WRITES_END +# define CDS_TSAN_ANNOTATE_IGNORE_RW_BEGIN +# define CDS_TSAN_ANNOTATE_IGNORE_RW_END +#endif + +#endif // #ifndef CDSLIB_COMPILER_FEATURE_TSAN_H diff --git a/cds/compiler/gcc/defs.h b/cds/compiler/gcc/defs.h index 2dadc6e1..386c6237 100644 --- a/cds/compiler/gcc/defs.h +++ b/cds/compiler/gcc/defs.h @@ -44,6 +44,11 @@ // Inheriting constructors #define CDS_CXX11_INHERITING_CTOR +// ************************************************* +// Features +// If you run under Thread Sanitizer, pass -DCDS_THREAD_SANITIZER_ENABLED in compiler command line +//#define CDS_THREAD_SANITIZER_ENABLED + // ************************************************* // Alignment macro diff --git a/cds/container/lazy_kvlist_rcu.h b/cds/container/lazy_kvlist_rcu.h index de24d131..1ead8ccc 100644 --- a/cds/container/lazy_kvlist_rcu.h +++ b/cds/container/lazy_kvlist_rcu.h @@ -130,19 +130,28 @@ namespace cds { namespace container { template static node_type * alloc_node(const K& key) { - return cxx_allocator().New( key ); + CDS_TSAN_ANNOTATE_IGNORE_WRITES_BEGIN; + node_type * p = cxx_allocator().New( key ); + CDS_TSAN_ANNOTATE_IGNORE_WRITES_END; + return p; } template static node_type * alloc_node( const K& key, const V& val ) { - return cxx_allocator().New( key, val ); + CDS_TSAN_ANNOTATE_IGNORE_WRITES_BEGIN; + node_type * p = cxx_allocator().New( key, val ); + CDS_TSAN_ANNOTATE_IGNORE_WRITES_END; + return p; } template static node_type * alloc_node( Args&&... args ) { - return cxx_allocator().MoveNew( std::forward(args)... ); + CDS_TSAN_ANNOTATE_IGNORE_WRITES_BEGIN; + node_type * p = cxx_allocator().MoveNew( std::forward(args)... ); + CDS_TSAN_ANNOTATE_IGNORE_WRITES_END; + return p; } static void free_node( node_type * pNode ) diff --git a/cds/intrusive/impl/michael_list.h b/cds/intrusive/impl/michael_list.h index e4dc7901..bbe9b134 100644 --- a/cds/intrusive/impl/michael_list.h +++ b/cds/intrusive/impl/michael_list.h @@ -243,7 +243,7 @@ namespace cds { namespace intrusive { link_checker::is_empty( pNode ); marked_node_ptr cur(pos.pCur); - pNode->m_pNext.store( cur, memory_model::memory_order_relaxed ); + pNode->m_pNext.store( cur, memory_model::memory_order_release ); return pos.pPrev->compare_exchange_strong( cur, marked_node_ptr(pNode), memory_model::memory_order_release, atomics::memory_order_relaxed ); } @@ -258,7 +258,7 @@ namespace cds { namespace intrusive { // physical deletion may be performed by search function if it detects that a node is logically deleted (marked) // CAS may be successful here or in other thread that searching something marked_node_ptr cur(pos.pCur); - if ( pos.pPrev->compare_exchange_strong( cur, marked_node_ptr( pos.pNext ), memory_model::memory_order_release, atomics::memory_order_relaxed )) + if ( pos.pPrev->compare_exchange_strong( cur, marked_node_ptr( pos.pNext ), memory_model::memory_order_acquire, atomics::memory_order_relaxed )) retire_node( pos.pCur ); return true; } @@ -1082,9 +1082,9 @@ try_again: return false; } - pNext = pCur->m_pNext.load(memory_model::memory_order_relaxed); + pNext = pCur->m_pNext.load(memory_model::memory_order_acquire); pos.guards.assign( position::guard_next_item, node_traits::to_value_ptr( pNext.ptr() )); - if ( pCur->m_pNext.load(memory_model::memory_order_acquire).all() != pNext.all() ) { + if ( pCur->m_pNext.load(memory_model::memory_order_relaxed).all() != pNext.all() ) { bkoff(); goto try_again; } @@ -1098,7 +1098,7 @@ try_again: if ( pNext.bits() == 1 ) { // pCur marked i.e. logically deleted. Help the erase/unlink function to unlink pCur node marked_node_ptr cur( pCur.ptr()); - if ( pPrev->compare_exchange_strong( cur, marked_node_ptr( pNext.ptr() ), memory_model::memory_order_release, atomics::memory_order_relaxed )) { + if ( pPrev->compare_exchange_strong( cur, marked_node_ptr( pNext.ptr() ), memory_model::memory_order_acquire, atomics::memory_order_relaxed )) { retire_node( pCur.ptr() ); } else { diff --git a/cds/intrusive/lazy_list_rcu.h b/cds/intrusive/lazy_list_rcu.h index ceabfa00..a62cf157 100644 --- a/cds/intrusive/lazy_list_rcu.h +++ b/cds/intrusive/lazy_list_rcu.h @@ -1142,7 +1142,7 @@ namespace cds { namespace intrusive { while ( pCur.ptr() != pTail && ( pCur.ptr() == pHead || cmp( *node_traits::to_value_ptr( *pCur.ptr() ), key ) < 0 )) { pPrev = pCur; - pCur = pCur->m_pNext.load(memory_model::memory_order_relaxed); + pCur = pCur->m_pNext.load(memory_model::memory_order_acquire); } pos.pCur = pCur.ptr(); diff --git a/cds/intrusive/michael_list_rcu.h b/cds/intrusive/michael_list_rcu.h index 2114e83a..25f9ce58 100644 --- a/cds/intrusive/michael_list_rcu.h +++ b/cds/intrusive/michael_list_rcu.h @@ -138,7 +138,7 @@ namespace cds { namespace intrusive { link_checker::is_empty( pNode ); marked_node_ptr p( pos.pCur ); - pNode->m_pNext.store( p, memory_model::memory_order_relaxed ); + pNode->m_pNext.store( p, memory_model::memory_order_release ); return pos.pPrev->compare_exchange_strong( p, marked_node_ptr(pNode), memory_model::memory_order_release, atomics::memory_order_relaxed ); } @@ -146,12 +146,12 @@ namespace cds { namespace intrusive { { // Mark the node (logical deleting) marked_node_ptr next(pos.pNext, 0); - if ( pos.pCur->m_pNext.compare_exchange_strong( next, marked_node_ptr(pos.pNext, 1), memory_model::memory_order_acquire, atomics::memory_order_relaxed )) { + if ( pos.pCur->m_pNext.compare_exchange_strong( next, marked_node_ptr(pos.pNext, 1), memory_model::memory_order_release, atomics::memory_order_relaxed )) { marked_node_ptr cur(pos.pCur); - if ( pos.pPrev->compare_exchange_strong( cur, marked_node_ptr( pos.pNext ), memory_model::memory_order_release, atomics::memory_order_relaxed )) + if ( pos.pPrev->compare_exchange_strong( cur, marked_node_ptr( pos.pNext ), memory_model::memory_order_acquire, atomics::memory_order_relaxed )) return true; next |= 1; - CDS_VERIFY( pos.pCur->m_pNext.compare_exchange_strong( next, next ^ 1, memory_model::memory_order_release, atomics::memory_order_relaxed )); + CDS_VERIFY( pos.pCur->m_pNext.compare_exchange_strong( next, next ^ 1, memory_model::memory_order_relaxed, atomics::memory_order_relaxed )); } return false; } @@ -980,15 +980,15 @@ namespace cds { namespace intrusive { while ( true ) { if ( !pCur.ptr() ) { pos.pPrev = pPrev; - pos.pCur = pCur.ptr(); - pos.pNext = pNext.ptr(); + pos.pCur = nullptr; + pos.pNext = nullptr; return false; } - pNext = pCur->m_pNext.load(memory_model::memory_order_relaxed); + pNext = pCur->m_pNext.load(memory_model::memory_order_acquire); - if ( pPrev->load(memory_model::memory_order_acquire) != pCur - || pNext != pCur->m_pNext.load(memory_model::memory_order_acquire) + if ( pPrev->load(memory_model::memory_order_relaxed) != pCur + || pNext != pCur->m_pNext.load(memory_model::memory_order_relaxed) || pNext.bits() != 0 ) // pNext contains deletion mark for pCur { // if pCur is marked as deleted (pNext.bits() != 0) diff --git a/cds/intrusive/split_list.h b/cds/intrusive/split_list.h index 0288e703..e28cdf93 100644 --- a/cds/intrusive/split_list.h +++ b/cds/intrusive/split_list.h @@ -478,9 +478,8 @@ namespace cds { namespace intrusive { const size_t nNewMaxCount = (nBucketCount < m_Buckets.capacity()) ? max_item_count( nBucketCount << 1, nLoadFactor ) : std::numeric_limits::max(); - m_nMaxItemCount.compare_exchange_strong( nMaxCount, nNewMaxCount, memory_model::memory_order_relaxed, - memory_model::memory_order_relaxed ); - m_nBucketCountLog2.compare_exchange_strong( sz, sz + 1, memory_model::memory_order_relaxed, memory_model::memory_order_relaxed ); + m_nMaxItemCount.compare_exchange_strong( nMaxCount, nNewMaxCount, memory_model::memory_order_relaxed, atomics::memory_order_relaxed ); + m_nBucketCountLog2.compare_exchange_strong( sz, sz + 1, memory_model::memory_order_relaxed, atomics::memory_order_relaxed ); } template diff --git a/cds/urcu/details/gpb.h b/cds/urcu/details/gpb.h index f79b6e22..023c2638 100644 --- a/cds/urcu/details/gpb.h +++ b/cds/urcu/details/gpb.h @@ -107,8 +107,11 @@ namespace cds { namespace urcu { { epoch_retired_ptr p; while ( m_Buffer.pop( p )) { - if ( p.m_nEpoch <= nEpoch ) + if ( p.m_nEpoch <= nEpoch ) { + CDS_TSAN_ANNOTATE_IGNORE_RW_BEGIN; p.free(); + CDS_TSAN_ANNOTATE_IGNORE_RW_END; + } else { push_buffer( p ); break; @@ -122,8 +125,11 @@ namespace cds { namespace urcu { bool bPushed = m_Buffer.push( ep ); if ( !bPushed || m_Buffer.size() >= capacity() ) { synchronize(); - if ( !bPushed ) + if ( !bPushed ) { + CDS_TSAN_ANNOTATE_IGNORE_RW_BEGIN; ep.free(); + CDS_TSAN_ANNOTATE_IGNORE_RW_END; + } return true; } return false; diff --git a/cds/urcu/details/gpi.h b/cds/urcu/details/gpi.h index b72ffec6..783a3064 100644 --- a/cds/urcu/details/gpi.h +++ b/cds/urcu/details/gpi.h @@ -111,8 +111,11 @@ namespace cds { namespace urcu { virtual void retire_ptr( retired_ptr& p ) { synchronize(); - if ( p.m_p ) + if ( p.m_p ) { + CDS_TSAN_ANNOTATE_IGNORE_RW_BEGIN; p.free(); + CDS_TSAN_ANNOTATE_IGNORE_RW_END; + } } /// Retires the pointer chain [\p itFirst, \p itLast) diff --git a/cds/urcu/details/gpt.h b/cds/urcu/details/gpt.h index 9bc0d876..84d2c1a8 100644 --- a/cds/urcu/details/gpt.h +++ b/cds/urcu/details/gpt.h @@ -113,8 +113,11 @@ namespace cds { namespace urcu { bool bPushed = m_Buffer.push( p ); if ( !bPushed || m_Buffer.size() >= capacity() ) { synchronize(); - if ( !bPushed ) + if ( !bPushed ) { + CDS_TSAN_ANNOTATE_IGNORE_RW_BEGIN; p.free(); + CDS_TSAN_ANNOTATE_IGNORE_RW_END; + } return true; } return false; diff --git a/cds/urcu/details/sig_buffered.h b/cds/urcu/details/sig_buffered.h index db4d88cf..bb0e0c5d 100644 --- a/cds/urcu/details/sig_buffered.h +++ b/cds/urcu/details/sig_buffered.h @@ -68,10 +68,10 @@ namespace cds { namespace urcu { protected: //@cond - buffer_type m_Buffer; - atomics::atomic m_nCurEpoch; - lock_type m_Lock; - size_t const m_nCapacity; + buffer_type m_Buffer; + atomics::atomic m_nCurEpoch; + lock_type m_Lock; + size_t const m_nCapacity; //@endcond public: @@ -104,8 +104,11 @@ namespace cds { namespace urcu { { epoch_retired_ptr p; while ( m_Buffer.pop( p )) { - if ( p.m_nEpoch <= nEpoch ) + if ( p.m_nEpoch <= nEpoch ) { + CDS_TSAN_ANNOTATE_IGNORE_RW_BEGIN; p.free(); + CDS_TSAN_ANNOTATE_IGNORE_RW_END; + } else { push_buffer( p ); break; @@ -118,8 +121,11 @@ namespace cds { namespace urcu { bool bPushed = m_Buffer.push( ep ); if ( !bPushed || m_Buffer.size() >= capacity() ) { synchronize(); - if ( !bPushed ) + if ( !bPushed ) { + CDS_TSAN_ANNOTATE_IGNORE_RW_BEGIN; ep.free(); + CDS_TSAN_ANNOTATE_IGNORE_RW_END; + } return true; } return false; diff --git a/cds/urcu/details/sig_threaded.h b/cds/urcu/details/sig_threaded.h index cf99c543..17d01525 100644 --- a/cds/urcu/details/sig_threaded.h +++ b/cds/urcu/details/sig_threaded.h @@ -110,8 +110,11 @@ namespace cds { namespace urcu { bool bPushed = m_Buffer.push( p ); if ( !bPushed || m_Buffer.size() >= capacity() ) { synchronize(); - if ( !bPushed ) + if ( !bPushed ) { + CDS_TSAN_ANNOTATE_IGNORE_RW_BEGIN; p.free(); + CDS_TSAN_ANNOTATE_IGNORE_RW_END; + } return true; } return false; diff --git a/cds/urcu/dispose_thread.h b/cds/urcu/dispose_thread.h index ca956035..46baa8fa 100644 --- a/cds/urcu/dispose_thread.h +++ b/cds/urcu/dispose_thread.h @@ -102,8 +102,11 @@ namespace cds { namespace urcu { { epoch_retired_ptr p; while ( pBuf->pop( p ) ) { - if ( p.m_nEpoch <= nCurEpoch ) + if ( p.m_nEpoch <= nCurEpoch ) { + CDS_TSAN_ANNOTATE_IGNORE_RW_BEGIN; p.free(); + CDS_TSAN_ANNOTATE_IGNORE_RW_END; + } else { pBuf->push( p ); break; diff --git a/projects/Win/vc12/cds.vcxproj b/projects/Win/vc12/cds.vcxproj index 09504492..6b981b47 100644 --- a/projects/Win/vc12/cds.vcxproj +++ b/projects/Win/vc12/cds.vcxproj @@ -642,6 +642,7 @@ + diff --git a/projects/Win/vc12/cds.vcxproj.filters b/projects/Win/vc12/cds.vcxproj.filters index 54bc40c4..e2042f83 100644 --- a/projects/Win/vc12/cds.vcxproj.filters +++ b/projects/Win/vc12/cds.vcxproj.filters @@ -1178,5 +1178,8 @@ Header Files\cds\sync + + Header Files\cds\compiler + \ No newline at end of file diff --git a/tests/cppunit/thread.cpp b/tests/cppunit/thread.cpp index a50aa370..bac98015 100644 --- a/tests/cppunit/thread.cpp +++ b/tests/cppunit/thread.cpp @@ -1,7 +1,8 @@ //$$CDS-header$$ +#include +#include // TSan annotations #include "cppunit/thread.h" -#include namespace CppUnitMini { @@ -42,12 +43,16 @@ namespace CppUnitMini { ThreadPool::~ThreadPool() { + CDS_TSAN_ANNOTATE_IGNORE_RW_BEGIN; + delete m_pBarrierStart; delete m_pBarrierDone; for ( size_t i = 0; i < m_arrThreads.size(); ++i ) delete m_arrThreads[i]; m_arrThreads.resize( 0 ); + + CDS_TSAN_ANNOTATE_IGNORE_RW_END; } void ThreadPool::add( TestThread * pThread, size_t nCount ) @@ -75,7 +80,7 @@ namespace CppUnitMini { // Wait while all threads is done m_pBarrierDone->wait(); - boost::this_thread::sleep(boost::posix_time::milliseconds(500)); + std::this_thread::sleep_for(std::chrono::milliseconds(500)); } void ThreadPool::run( unsigned int nDuration ) @@ -87,17 +92,17 @@ namespace CppUnitMini { for ( size_t i = 0; i < nThreadCount; ++i ) m_arrThreads[i]->create(); - boost::system_time stEnd( boost::get_system_time() + boost::posix_time::seconds( nDuration ) ); + auto stEnd(std::chrono::steady_clock::now() + std::chrono::seconds( nDuration )); do { - boost::this_thread::sleep( stEnd ); - } while ( boost::get_system_time() < stEnd ); + std::this_thread::sleep_until( stEnd ); + } while ( std::chrono::steady_clock::now() < stEnd ); for ( size_t i = 0; i < nThreadCount; ++i ) m_arrThreads[i]->stop(); // Wait while all threads is done m_pBarrierDone->wait(); - boost::this_thread::sleep(boost::posix_time::milliseconds(500)); + std::this_thread::sleep_for(std::chrono::milliseconds(500)); } void ThreadPool::onThreadInitDone( TestThread * pThread )