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
2026-02-04 02:18:37 +00:00 · 2009-06-03 08:11:02 +00:00 · 2009-06-03 08:11:02 +00:00 · 85a0aea8c7
commit 85a0aea8c7
parent 5f8f34f8c0
6 changed files with 178 additions and 69 deletions
--- 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/
--- 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)
--- 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]);
--- 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
--- 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 <assert.h>
@ -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_));
--- 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 <vector>
 #include <string>
 #include <map>
+#include <queue>
 #include <algorithm>
 #include <cstring>      // strlen(), index(), rindex()
 #include <ctime>
@ -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<void*>(BOUNDARY+1));
+    ANNOTATE_HAPPENS_BEFORE(reinterpret_cast<void*>(BOUNDARY+1));
    BOUNDARY++;
    usleep(1000);
  }
@ -1526,7 +1523,7 @@ void Reader() {
  do {
    n = BOUNDARY;
    if (n == 0) continue; 
-    ANNOTATE_CONDVAR_WAIT(reinterpret_cast<void*>(n));
+    ANNOTATE_HAPPENS_AFTER(reinterpret_cast<void*>(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<void*>(BOUNDARY+1));
+    ANNOTATE_HAPPENS_BEFORE(reinterpret_cast<void*>(BOUNDARY+1));
    BOUNDARY++;
    usleep(1000);
  }
@ -1579,7 +1576,7 @@ void Writer2() {
  do {
    n = BOUNDARY;
    if (n == 0) continue; 
-    ANNOTATE_CONDVAR_WAIT(reinterpret_cast<void*>(n));
+    ANNOTATE_HAPPENS_AFTER(reinterpret_cast<void*>(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<int> 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;