From 85a0aea8c79fce2103fcc66356fda687620f05eb Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Wed, 3 Jun 2009 08:11:02 +0000 Subject: [PATCH] Updated ANNOTATE_*() macro's as discussed on the valgrind-developers mailing list. Merged drt/unittest r1007:1014. Updated to do list. git-svn-id: svn://svn.valgrind.org/valgrind/trunk@10220 --- drd/TODO.txt | 3 + drd/drd.h | 84 +++++++------- drd/drd_clientreq.c | 8 -- drd/tests/Makefile.am | 5 +- drd/tests/tsan_thread_wrappers_pthread.h | 11 +- drd/tests/tsan_unittest.cpp | 136 +++++++++++++++++++++-- 6 files changed, 178 insertions(+), 69 deletions(-) diff --git a/drd/TODO.txt b/drd/TODO.txt index b84acbd6f..4c19450c8 100644 --- a/drd/TODO.txt +++ b/drd/TODO.txt @@ -7,6 +7,9 @@ The DRD tool - Eliminate Valgrind's thread ID's from DRD's output, and only keep the DrdThreadId. - Add support for Ist_CAS once the DCAS branch has been merged to the trunk. +- Update the description of DRD in docs/xml/manual-intro.xml before 3.4.2 or + 3.5.0 is released, whichever is released first. +- Update the DRD manual before 3.5.0 is released. - Add locking order checking. Start from the following information: * http://sourceforge.net/mailarchive/message.php?msg_id=alpine.LNX.1.10.0803270822080.17890%40mudge.stoecker.eu * http://lwn.net/Articles/185605/ diff --git a/drd/drd.h b/drd/drd.h index 19c4a82b6..8e1e858f0 100644 --- a/drd/drd.h +++ b/drd/drd.h @@ -90,62 +90,65 @@ */ /** - * Tell DRD to insert a mark. addr is either the address of a pthread condition - * variable or the address of an object that is not a pthread synchronization - * object. Inserting two 'happens before' annotations while - * no thread has passed by a 'happens after' annotation is an error. + * Tell DRD to insert a mark. addr is the address of an object that is not a + * pthread synchronization object. Inserting two 'happens before' annotations + * while no thread has passed by a 'happens after' annotation is an error. */ #define ANNOTATE_HAPPENS_BEFORE(addr) DRDCL_(annotate_happens_before)(addr) /** * Tell DRD that the memory accesses executed after this annotation will happen * after the memory accesses performed before the most recent - * ANNOTATE_HAPPENS_BEFORE(addr). addr is either the address of a pthread - * condition variable or the address of an object that is not a pthread - * synchronization object. Inserting a 'happens after' annotation before any - * other thread has passed by a 'happens before' annotation for the same - * address or inserting two 'happens after' annotations while no thread has - * passed by a 'happens before' annotation is an error. + * ANNOTATE_HAPPENS_BEFORE(addr). addr is the address of an object that is not + * a pthread synchronization object. Inserting a 'happens after' annotation + * before any other thread has passed by a 'happens before' annotation for the + * same address is an error. */ #define ANNOTATE_HAPPENS_AFTER(addr) DRDCL_(annotate_happens_after)(addr) -/** - * Tell DRD that no more ANNOTATE_HAPPENS_AFTER(addr) annotations - * will be inserted before the next ANNOTATE_HAPPENS_BEFORE(addr). - */ -#define ANNOTATE_HAPPENS_AFTER_DONE(addr) \ - DRDCL_(annotate_happens_after_done)(addr) - /** * Tell DRD that waiting on the condition variable at address cv has succeeded * and a lock on the mutex at address mtx is now held. Since DRD always inserts * a happens before relation between the pthread_cond_signal() or * pthread_cond_broadcast() call that wakes up a pthread_cond_wait() or * pthread_cond_timedwait() call and the woken up thread, this macro has been - * left empty. + * defined such that it has no effect. */ -#define ANNOTATE_CONDVAR_LOCK_WAIT(cv, mtx) +#define ANNOTATE_CONDVAR_LOCK_WAIT(cv, mtx) do { } while(0) /** * Tell DRD that the condition variable at address cv is about to be signaled. - * cv is either the address of a condition variable or the address of an object - * that is not a POSIX synchronization object. */ -#define ANNOTATE_CONDVAR_SIGNAL(cv) ANNOTATE_HAPPENS_BEFORE(cv) +#define ANNOTATE_CONDVAR_SIGNAL(cv) do { } while(0) /** - * Tell DRD that waiting on condition variable at address cv succeeded. - * cv is either the address of a condition variable or the address of an object - * that is not a POSIX synchronization object. + * Tell DRD that waiting on condition variable at address cv succeeded and that + * the memory operations performed after this annotation should be considered + * to happen after the matching ANNOTATE_CONDVAR_SIGNAL(cv). Since this is the + * default behavior of DRD, this macro and the macro above have been defined + * such that they have no effect. */ -#define ANNOTATE_CONDVAR_WAIT(cv) ANNOTATE_HAPPENS_AFTER(cv) +#define ANNOTATE_CONDVAR_WAIT(cv) do { } while(0) /** * Tell DRD to consider the memory operations that happened before a mutex * unlock event and after the subsequent mutex lock event on the same mutex as - * ordered. This is how DRD always behaves, so this macro has been left empty. + * ordered. This is how DRD always behaves, so this macro has been defined + * such that it has no effect. */ -#define ANNOTATE_MUTEX_IS_USED_AS_CONDVAR(mtx) +#define ANNOTATE_MUTEX_IS_USED_AS_CONDVAR(mtx) do { } while(0) + +/** + * Tell DRD to handle the specified memory range like a pure happens-before + * detector would do. Since this is how DRD always behaves, this annotation + * has been defined such that it has no effect. + */ +#define ANNOTATE_PUBLISH_MEMORY_RANGE(addr, size) do { } while(0) + +/** + * Tell DRD to undo the effect of ANNOTATE_PUBLISH_MEMORY_RANGE(). + */ +#define ANNOTATE_UNPUBLISH_MEMORY_RANGE(addr, size) do { } while(0) /** Tell DRD that a reader-writer lock object has been initialized. */ #define ANNOTATE_RWLOCK_CREATE(rwlock) \ @@ -171,6 +174,18 @@ #define ANNOTATE_RWLOCK_RELEASED(rwlock, is_w) \ DRDCL_(annotate_rwlock)(rwlock, 3, is_w) +/** @todo Implement this annotation. */ +#define ANNOTATE_PCQ_CREATE(pcq) do { } while(0) + +/** @todo Implement this annotation. */ +#define ANNOTATE_PCQ_DESTROY(pcq) do { } while(0) + +/** @todo Implement this annotation. */ +#define ANNOTATE_PCQ_PUT(pcq) do { } while(0) + +/** @todo Implement this annotation. */ +#define ANNOTATE_PCQ_GET(pcq) do { } while(0) + /** * Tell DRD that data races in the specified address range are expected and * must not be reported. @@ -273,10 +288,6 @@ enum { /* Tell DRD to insert a happens after annotation. */ VG_USERREQ__DRD_ANNOTATE_HAPPENS_AFTER, /* args: Addr. */ - /* Tell DRD that no more happens after annotations will follow until the - * next happens before annotation. */ - VG_USERREQ__DRD_ANNOTATE_HAPPENS_AFTER_DONE, - /* args: Addr. */ /* Tell DRD about an operation performed on a user-defined reader-writer * synchronization object. */ @@ -373,15 +384,6 @@ void DRDCL_(annotate_happens_after)(const void* const addr) addr, 0, 0, 0, 0); } -static __inline__ -void DRDCL_(annotate_happens_after_done)(const void* const addr) -{ - int res; - VALGRIND_DO_CLIENT_REQUEST(res, 0, - VG_USERREQ__DRD_ANNOTATE_HAPPENS_AFTER_DONE, - addr, 0, 0, 0, 0); -} - static __inline__ void DRDCL_(annotate_rwlock)(const void* const rwlock, const int op, const int is_w) diff --git a/drd/drd_clientreq.c b/drd/drd_clientreq.c index 5d677b2fd..3bc622e6b 100644 --- a/drd/drd_clientreq.c +++ b/drd/drd_clientreq.c @@ -137,14 +137,6 @@ static Bool handle_client_request(ThreadId vg_tid, UWord* arg, UWord* ret) } break; - case VG_USERREQ__DRD_ANNOTATE_HAPPENS_AFTER_DONE: - { - struct cond_info* const cond_p = DRD_(cond_get)(arg[1]); - if (! cond_p) - DRD_(mutex_post_destroy)(arg[1]); - } - break; - case VG_USERREQ__DRD_ANNOTATE_RWLOCK: { struct mutex_info* const mutex_p = DRD_(mutex_get)(arg[1]); diff --git a/drd/tests/Makefile.am b/drd/tests/Makefile.am index b34fdb583..d6168b019 100644 --- a/drd/tests/Makefile.am +++ b/drd/tests/Makefile.am @@ -290,8 +290,9 @@ monitor_example_SOURCES = monitor_example.cpp new_delete_SOURCES = new_delete.cpp tsan_unittest_SOURCES = tsan_unittest.cpp -tsan_unittest_CXXFLAGS = $(AM_CXXFLAGS) -Wno-sign-compare\ - -Wno-shadow @FLAG_W_NO_EMPTY_BODY@ +tsan_unittest_CXXFLAGS = $(AM_CXXFLAGS) \ + -DTHREAD_WRAPPERS='"tsan_thread_wrappers_pthread.h"' \ + -Wno-sign-compare -Wno-shadow @FLAG_W_NO_EMPTY_BODY@ if HAVE_BOOST_1_35 boost_thread_SOURCES = boost_thread.cpp diff --git a/drd/tests/tsan_thread_wrappers_pthread.h b/drd/tests/tsan_thread_wrappers_pthread.h index 84ae17ae5..58b00ccc5 100644 --- a/drd/tests/tsan_thread_wrappers_pthread.h +++ b/drd/tests/tsan_thread_wrappers_pthread.h @@ -66,8 +66,6 @@ using namespace std; #include "../../drd/drd.h" #define ANNOTATE_NO_OP(arg) do { } while(0) #define ANNOTATE_EXPECT_RACE(addr, descr) DRDCL_(ignore_range)(addr, 4) -#define ANNOTATE_PUBLISH_MEMORY_RANGE(addr, size) do { } while(0) -#define ANNOTATE_UNPUBLISH_MEMORY_RANGE(addr, size) do { } while(0) static inline bool RunningOnValgrind() { return RUNNING_ON_VALGRIND; } #include @@ -512,7 +510,7 @@ class ThreadPool { //! Start all threads. void StartWorkers() { - for (int i = 0; i < workers_.size(); i++) { + for (size_t i = 0; i < workers_.size(); i++) { workers_[i]->Start(); } } @@ -526,10 +524,10 @@ class ThreadPool { //! Wait workers to finish, then join all threads. ~ThreadPool() { - for (int i = 0; i < workers_.size(); i++) { + for (size_t i = 0; i < workers_.size(); i++) { Add(NULL); } - for (int i = 0; i < workers_.size(); i++) { + for (size_t i = 0; i < workers_.size(); i++) { workers_[i]->Join(); delete workers_[i]; } @@ -573,9 +571,10 @@ class BlockingCounter { public: explicit BlockingCounter(int initial_count) : count_(initial_count) {} - void DecrementCount() { + bool DecrementCount() { MutexLock lock(&mu_); count_--; + return count_ == 0; } void Wait() { mu_.LockWhen(Condition(&IsZero, &count_)); diff --git a/drd/tests/tsan_unittest.cpp b/drd/tests/tsan_unittest.cpp index dc6f6a759..e4c0236b7 100644 --- a/drd/tests/tsan_unittest.cpp +++ b/drd/tests/tsan_unittest.cpp @@ -46,7 +46,7 @@ // This test must not include any other file specific to threading library, // everything should be inside THREAD_WRAPPERS. #ifndef THREAD_WRAPPERS -# define THREAD_WRAPPERS "tsan_thread_wrappers_pthread.h" +# define THREAD_WRAPPERS "thread_wrappers_pthread.h" #endif #include THREAD_WRAPPERS @@ -73,6 +73,7 @@ #include #include #include +#include #include #include // strlen(), index(), rindex() #include @@ -1486,7 +1487,7 @@ namespace test30 { // // Writer: Reader1, Reader2, ..., ReaderN: // 1. write(GLOB[i]: i >= BOUNDARY) a. n = BOUNDARY -// 2. ANNOTATE_SIGNAL(BOUNDARY+1) -------> b. ANNOTATE_WAIT(n) +// 2. HAPPENS_BEFORE(BOUNDARY+1) -------> b. HAPPENS_AFTER(n) // 3. BOUNDARY++; c. read(GLOB[i]: i < n) // // Here we have a 'safe' race on accesses to BOUNDARY and @@ -1499,10 +1500,6 @@ namespace test30 { // are free to rearrange memory operations. // I am actually sure that this scheme is wrong unless we use // some smart memory fencing... -// -// For this unit test we use ANNOTATE_CONDVAR_WAIT/ANNOTATE_CONDVAR_SIGNAL -// but for real life we will need separate annotations -// (if we ever want to annotate this synchronization scheme at all). const int N = 48; @@ -1515,7 +1512,7 @@ void Writer() { for (int j = i; j < N; j++) { GLOB[j] = j; } - ANNOTATE_CONDVAR_SIGNAL(reinterpret_cast(BOUNDARY+1)); + ANNOTATE_HAPPENS_BEFORE(reinterpret_cast(BOUNDARY+1)); BOUNDARY++; usleep(1000); } @@ -1526,7 +1523,7 @@ void Reader() { do { n = BOUNDARY; if (n == 0) continue; - ANNOTATE_CONDVAR_WAIT(reinterpret_cast(n)); + ANNOTATE_HAPPENS_AFTER(reinterpret_cast(n)); for (int i = 0; i < n; i++) { CHECK(GLOB[i] == i); } @@ -1554,7 +1551,7 @@ namespace test31 { // // Writer1: Writer2 // 1. write(GLOB[i]: i >= BOUNDARY) a. n = BOUNDARY -// 2. ANNOTATE_SIGNAL(BOUNDARY+1) -------> b. ANNOTATE_WAIT(n) +// 2. HAPPENS_BEFORE(BOUNDARY+1) -------> b. HAPPENS_AFTER(n) // 3. BOUNDARY++; c. write(GLOB[i]: i < n) // @@ -1568,7 +1565,7 @@ void Writer1() { for (int j = i; j < N; j++) { GLOB[j] = j; } - ANNOTATE_CONDVAR_SIGNAL(reinterpret_cast(BOUNDARY+1)); + ANNOTATE_HAPPENS_BEFORE(reinterpret_cast(BOUNDARY+1)); BOUNDARY++; usleep(1000); } @@ -1579,7 +1576,7 @@ void Writer2() { do { n = BOUNDARY; if (n == 0) continue; - ANNOTATE_CONDVAR_WAIT(reinterpret_cast(n)); + ANNOTATE_HAPPENS_AFTER(reinterpret_cast(n)); for (int i = 0; i < n; i++) { if(GLOB[i] == i) { GLOB[i]++; @@ -1796,7 +1793,7 @@ void Run() { for (int i = 0; i < N_free; i++) delete ARR[i]; delete [] ARR; - for (int i = 0; i < mus.size(); i++) { + for (size_t i = 0; i < mus.size(); i++) { delete mus[i]; } } @@ -6546,10 +6543,125 @@ void Run() { MyThreadArray mta2(Waker2, Waiter2); mta2.Start(); mta2.Join(); + free(filename); + filename = 0; + free(dir_name); + dir_name = 0; } REGISTER_TEST(Run, 141) } // namespace test141 + +// Simple FIFO queue annotated with PCQ annotations. {{{1 +class FifoMessageQueue { + public: + FifoMessageQueue() { ANNOTATE_PCQ_CREATE(this); } + ~FifoMessageQueue() { ANNOTATE_PCQ_DESTROY(this); } + // Send a message. 'message' should be positive. + void Put(int message) { + CHECK(message); + MutexLock lock(&mu_); + ANNOTATE_PCQ_PUT(this); + q_.push(message); + } + // Return the message from the queue and pop it + // or return 0 if there are no messages. + int Get() { + MutexLock lock(&mu_); + if (q_.empty()) return 0; + int res = q_.front(); + q_.pop(); + ANNOTATE_PCQ_GET(this); + return res; + } + private: + Mutex mu_; + queue q_; +}; + + +// test142: TN. Check PCQ_* annotations. {{{1 +namespace test142 { +// Putter writes to array[i] and sends a message 'i'. +// Getters receive messages and read array[message]. +// PCQ_* annotations calm down the hybrid detectors. + +const int N = 1000; +int array[N+1]; + +FifoMessageQueue q; + +void Putter() { + for (int i = 1; i <= N; i++) { + array[i] = i*i; + q.Put(i); + usleep(1000); + } +} + +void Getter() { + int non_zero_received = 0; + for (int i = 1; i <= N; i++) { + int res = q.Get(); + if (res > 0) { + CHECK(array[res] = res * res); + non_zero_received++; + } + usleep(1000); + } + printf("T=%d: non_zero_received=%d\n", + (int)pthread_self(), non_zero_received); +} + +void Run() { + printf("test142: tests PCQ annotations\n"); + MyThreadArray t(Putter, Getter, Getter); + t.Start(); + t.Join(); +} +REGISTER_TEST(Run, 142) +} // namespace test142 + + +// test143: TP. Check PCQ_* annotations. {{{1 +namespace test143 { +// True positive. +// We have a race on GLOB between Putter and one of the Getters. +// Pure h-b will not see it. +// If FifoMessageQueue was annotated using HAPPENS_BEFORE/AFTER, the race would +// be missed too. +// PCQ_* annotations do not hide this race. +int GLOB = 0; + +FifoMessageQueue q; + +void Putter() { + GLOB = 1; + q.Put(1); +} + +void Getter() { + usleep(10000); + q.Get(); + CHECK(GLOB == 1); // Race here +} + +void Run() { + q.Put(1); + if (!Tsan_PureHappensBefore()) { + ANNOTATE_EXPECT_RACE_FOR_TSAN(&GLOB, "true races"); + } + printf("test143: tests PCQ annotations (true positive)\n"); + MyThreadArray t(Putter, Getter, Getter); + t.Start(); + t.Join(); +} +REGISTER_TEST(Run, 143); +} // namespace test143 + + + + // test300: {{{1 namespace test300 { int GLOB = 0;