From 07c2c735c2228bb29bd856d1e043ea2c6a9350d9 Mon Sep 17 00:00:00 2001 From: khizmax Date: Tue, 29 Nov 2016 16:19:55 +0300 Subject: [PATCH] Updated TSan suppression Annotated some code against TSan warning --- cds/compiler/feature_tsan.h | 10 +++++++++- cds/container/basket_queue.h | 8 +++++++- cds/container/moir_queue.h | 8 +++++++- cds/container/msqueue.h | 8 +++++++- cds/sync/spinlock.h | 10 ++++++++-- cds/urcu/details/base.h | 1 + src/hp_gc.cpp | 1 + test/CMakeLists.txt | 3 +++ test/stress/freelist/put_get.cpp | 2 ++ tools/tsan-suppression | 23 +++++++++++++++-------- 10 files changed, 60 insertions(+), 14 deletions(-) diff --git a/cds/compiler/feature_tsan.h b/cds/compiler/feature_tsan.h index af780b97..9852773e 100644 --- a/cds/compiler/feature_tsan.h +++ b/cds/compiler/feature_tsan.h @@ -32,7 +32,7 @@ #define CDSLIB_COMPILER_FEATURE_TSAN_H // Thread Sanitizer annotations. -// From https://groups.google.com/d/msg/thread-sanitizer/SsrHB7FTnTk/mNTGNLQj-9cJ +// From http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/test/tsan/annotate_happens_before.cc?view=markup //@cond @@ -52,6 +52,9 @@ CDS_TSAN_ANNOTATE_IGNORE_READS_END # define CDS_TSAN_ANNOTATE_NEW_MEMORY( addr, sz ) AnnotateNewMemory( (char *) __FILE__, __LINE__, reinterpret_cast(addr), sz ) +# define CDS_TSAN_ANNOTATE_MUTEX_ACQUIRED( addr ) AnnotateRWLockAcquired( __FILE__, __LINE__, reinterpret_cast(addr), 1 ) +# define CDS_TSAN_ANNOTATE_MUTEX_RELEASED( addr ) AnnotateRWLockReleased( __FILE__, __LINE__, reinterpret_cast(addr), 1 ) + // provided by TSan extern "C" { void AnnotateHappensBefore(const char *f, int l, void *addr); @@ -64,6 +67,8 @@ void AnnotateNewMemory(char *f, int l, void * mem, size_t size); + void AnnotateRWLockAcquired( const char *f, int l, void *m, long is_w ); + void AnnotateRWLockReleased( const char *f, int l, void *m, long is_w ); } #else // CDS_THREAD_SANITIZER_ENABLED @@ -80,6 +85,9 @@ # define CDS_TSAN_ANNOTATE_NEW_MEMORY( addr, sz ) +# define CDS_TSAN_ANNOTATE_MUTEX_ACQUIRED( addr ) +# define CDS_TSAN_ANNOTATE_MUTEX_RELEASED( addr ) + #endif //@endcond diff --git a/cds/container/basket_queue.h b/cds/container/basket_queue.h index 0192f483..0a09a2c9 100644 --- a/cds/container/basket_queue.h +++ b/cds/container/basket_queue.h @@ -397,7 +397,13 @@ namespace cds { namespace container { */ bool dequeue( value_type& dest ) { - return dequeue_with( [&dest]( value_type& src ) { dest = std::move( src );}); + return dequeue_with( [&dest]( value_type& src ) { + // TSan finds a race between this read of \p src and node_type constructor + // I think, it is wrong + CDS_TSAN_ANNOTATE_IGNORE_READS_BEGIN; + dest = std::move( src ); + CDS_TSAN_ANNOTATE_IGNORE_READS_END; + }); } /// Dequeues a value using a functor diff --git a/cds/container/moir_queue.h b/cds/container/moir_queue.h index bf2d7729..5ed12223 100644 --- a/cds/container/moir_queue.h +++ b/cds/container/moir_queue.h @@ -240,7 +240,13 @@ namespace cds { namespace container { */ bool dequeue( value_type& dest ) { - return dequeue_with( [&dest]( value_type& src ) { dest = std::move( src ); }); + return dequeue_with( [&dest]( value_type& src ) { + // TSan finds a race between this read of \p src and node_type constructor + // I think, it is wrong + CDS_TSAN_ANNOTATE_IGNORE_READS_BEGIN; + dest = std::move( src ); + CDS_TSAN_ANNOTATE_IGNORE_READS_END; + }); } /// Dequeues a value using a functor diff --git a/cds/container/msqueue.h b/cds/container/msqueue.h index b79cff66..874a5cd0 100644 --- a/cds/container/msqueue.h +++ b/cds/container/msqueue.h @@ -355,7 +355,13 @@ namespace cds { namespace container { */ bool dequeue( value_type& dest ) { - return dequeue_with( [&dest]( value_type& src ) { dest = std::move( src );}); + return dequeue_with( [&dest]( value_type& src ) { + // TSan finds a race between this read of \p src and node_type constructor + // I think, it is wrong + CDS_TSAN_ANNOTATE_IGNORE_READS_BEGIN; + dest = std::move( src ); + CDS_TSAN_ANNOTATE_IGNORE_READS_END; + }); } /// Dequeues a value using a functor diff --git a/cds/sync/spinlock.h b/cds/sync/spinlock.h index ec0ee5cf..32b75ea1 100644 --- a/cds/sync/spinlock.h +++ b/cds/sync/spinlock.h @@ -149,8 +149,10 @@ namespace cds { { backoff_strategy backoff; while ( nTryCount-- ) { - if ( try_lock()) + if ( try_lock() ) { + CDS_TSAN_ANNOTATE_MUTEX_ACQUIRED( &m_spin ); return true; + } backoff(); } return false; @@ -182,6 +184,7 @@ namespace cds { CDS_DEBUG_ONLY( m_dbgOwnerId = OS::c_NullThreadId; ) m_spin.store( false, atomics::memory_order_release ); + CDS_TSAN_ANNOTATE_MUTEX_RELEASED( &m_spin ); } }; @@ -246,8 +249,10 @@ namespace cds { backoff_strategy bkoff; while ( nTryCount-- ) { - if ( try_acquire()) + if ( try_acquire() ) { + CDS_TSAN_ANNOTATE_MUTEX_ACQUIRED( &m_spin ); return true; + } bkoff(); } return false; @@ -347,6 +352,7 @@ namespace cds { else { free(); m_spin.store( 0, atomics::memory_order_release ); + CDS_TSAN_ANNOTATE_MUTEX_RELEASED( &m_spin ); } return true; } diff --git a/cds/urcu/details/base.h b/cds/urcu/details/base.h index 23452681..e05abed5 100644 --- a/cds/urcu/details/base.h +++ b/cds/urcu/details/base.h @@ -375,6 +375,7 @@ namespace cds { thread_record * pOldHead = m_pHead.load( atomics::memory_order_acquire ); do { pRec->m_list.m_pNext = pOldHead; + CDS_TSAN_ANNOTATE_HAPPENS_BEFORE( &( pRec->m_list.m_pNext )); } while ( !m_pHead.compare_exchange_weak( pOldHead, pRec, atomics::memory_order_release, atomics::memory_order_relaxed )); return pRec; diff --git a/src/hp_gc.cpp b/src/hp_gc.cpp index eb667c2f..15c675ca 100644 --- a/src/hp_gc.cpp +++ b/src/hp_gc.cpp @@ -151,6 +151,7 @@ namespace cds { namespace gc { hplist_node * pOldHead = m_pListHead.load( atomics::memory_order_acquire ); do { hprec->m_pNextNode = pOldHead; + CDS_TSAN_ANNOTATE_HAPPENS_BEFORE( &( hprec->m_pNextNode )); } while ( !m_pListHead.compare_exchange_weak( pOldHead, hprec, atomics::memory_order_release, atomics::memory_order_relaxed )); return hprec; diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 7cf48759..d27c6f51 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -13,3 +13,6 @@ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DGTEST_LANG_CXX11") add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/unit) add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/stress) + +file(GLOB SANITIZER_OPTION_FILES ${PROJECT_SOURCE_DIR}/tools/tsan-suppression) +file(COPY ${SANITIZER_OPTION_FILES} DESTINATION ${EXECUTABLE_OUTPUT_PATH}) diff --git a/test/stress/freelist/put_get.cpp b/test/stress/freelist/put_get.cpp index 6a51563e..59f6878a 100644 --- a/test/stress/freelist/put_get.cpp +++ b/test/stress/freelist/put_get.cpp @@ -88,7 +88,9 @@ namespace { item_type* p; while ( (p = static_cast( m_FreeList.get())) != nullptr ) { + CDS_TSAN_ANNOTATE_IGNORE_RW_BEGIN; p->counter++; + CDS_TSAN_ANNOTATE_IGNORE_RW_END; arr[n] = p; ++m_nSuccess; ++n; diff --git a/tools/tsan-suppression b/tools/tsan-suppression index 8d8c46d5..eb2c5421 100644 --- a/tools/tsan-suppression +++ b/tools/tsan-suppression @@ -4,14 +4,21 @@ # verosity=n Verbosity level (0 - silent, 1 - a bit of output, 2+ - more output). # history_size=[0..7], default 2 -# DHP -#race:cds::gc::details::retired_ptr::free +# false: LazyList potential deadlock +deadlock:cds/intrusive/impl/lazy_list.h -# uRCU false positive -#race:cds::urcu::gc*::batch_retire* +# false: BronsonAVLTree potential deadlock +deadlock:cds/container/impl/bronson_avltree_map_rcu.h -# EllenBinTree false positive -#race:ellen_bintree_pool::internal_node_allocator*::allocate +#TODO: temporary suppressed. Must be researched later +race:cds/container/impl/bronson_avltree_map_rcu.h -# TODO: TSan false positive or library issues? -#race:cds::container::OptimisticQueue*::alloc_node +#TODO: MSPriorityQueue - temporary suppressed. Must be researched later +# Seems, TSan don't see spinlock blocking. How to learn TSan to see non-traditional locking algo?.. +race:cds::intrusive::MSPriorityQueue + +#TODO: gc::DHP must be reimplemented ASAP +race:cds::gc::dhp::GarbageCollector::scan + +#TODO: temporary suppressed. Must be researched later +race:cds::memory::michael::Heap \ No newline at end of file -- 2.34.1