Merge patch from JeremyF:

33-pre_mutex_lock

HELGRIND: two updates: add a pre_mutex_lock tracking function, so the
skin can do something before the thread blocks. This allows us to do
lock ordering tests before the thread blocks in the deadlock we'd like
to report...

git-svn-id: svn://svn.valgrind.org/valgrind/trunk@1306

diff --git a/coregrind/vg_scheduler.c b/coregrind/vg_scheduler.c
index f8765e5163c0a8541bf23e7ab281935a430c2c27..72f643364861216cf5333049d5255b38c1058a09 100644 (file)
--- a/coregrind/vg_scheduler.c
+++ b/coregrind/vg_scheduler.c
@@ -2435,6 +2435,8 @@ void do_pthread_mutex_lock( ThreadId tid,
             /* caller is polling; so return immediately. */
             SET_EDX(tid, EBUSY);
          } else {
+           VG_TRACK ( pre_mutex_lock, tid, mutex );
+
             VG_(threads)[tid].status        = VgTs_WaitMX;
             VG_(threads)[tid].associated_mx = mutex;
             SET_EDX(tid, 0); /* pth_mx_lock success value */
@@ -2450,6 +2452,9 @@ void do_pthread_mutex_lock( ThreadId tid,
    } else {
       /* Nobody owns it.  Sanity check ... */
       vg_assert(mutex->__m_owner == VG_INVALID_THREADID);
+
+      VG_TRACK ( pre_mutex_lock, tid, mutex );
+
       /* We get it! [for the first time]. */
       mutex->__m_count = 1;
       mutex->__m_owner = (_pthread_descr)tid;
@@ -2489,6 +2494,7 @@ void do_pthread_mutex_unlock ( ThreadId tid,
       locked now so that it balances with unlocks */
    if (mutex->__m_kind & VG_PTHREAD_PREHISTORY) {
       mutex->__m_kind &= ~VG_PTHREAD_PREHISTORY;
+      VG_TRACK( pre_mutex_lock, (ThreadId)mutex->__m_owner, mutex );
       VG_TRACK( post_mutex_lock, (ThreadId)mutex->__m_owner, mutex );
    }
 
@@ -2616,6 +2622,8 @@ void do_pthread_cond_timedwait_TIMEOUT ( ThreadId tid )
    } else {
       /* Currently held.  Make thread tid be blocked on it. */
       vg_assert(mx->__m_count > 0);
+      VG_TRACK( pre_mutex_lock, tid, mx );
+
       VG_(threads)[tid].status        = VgTs_WaitMX;
       SET_EDX(tid, ETIMEDOUT);      /* pthread_cond_wait return value */
       VG_(threads)[tid].associated_cv = NULL;
@@ -2662,6 +2670,8 @@ void release_N_threads_waiting_on_cond ( pthread_cond_t* cond,
       mx = VG_(threads)[i].associated_mx;
       vg_assert(mx != NULL);
 
+      VG_TRACK( pre_mutex_lock, i, mx );
+
       if (mx->__m_owner == VG_INVALID_THREADID) {
          /* Currently unheld; hand it out to thread i. */
          vg_assert(mx->__m_count == 0);

diff --git a/helgrind/hg_main.c b/helgrind/hg_main.c
index e547a15842ef8f3e644befc08cef5738841978a4..be7051df9952f6e43bc0ab6f1ad9e80f8420337b 100644 (file)
--- a/helgrind/hg_main.c
+++ b/helgrind/hg_main.c
@@ -1453,18 +1453,14 @@ static Bool check_cycle(const Mutex *start, const LockSet* lockset)
    return check_cycle_inner(start, lockset);
 }
 
-/* catch bad mutex state changes (though the common ones are handled
-   by core) */
-static void set_mutex_state(Mutex *mutex, MutexState state,
-                           ThreadId tid, ThreadState *tst)
+/* test to see if a mutex state change would be problematic; this
+   makes no changes to the mutex state.  This should be called before
+   the locking thread has actually blocked. */
+static void test_mutex_state(Mutex *mutex, MutexState state,
+                            ThreadId tid, ThreadState *tst)
 {
    static const Bool debug = False;
 
-   if (debug)
-      VG_(printf)("\ntid %d changing mutex (%p)->%p%(y state %s -> %s\n",
-                 tid, mutex, mutex->mutexp, mutex->mutexp,
-                 pp_MutexState(mutex->state), pp_MutexState(state));
-
    if (mutex->state == MxDead) {
       Char *str;
 
@@ -1481,18 +1477,6 @@ static void set_mutex_state(Mutex *mutex, MutexState state,
 
    switch(state) {
    case MxLocked:
-      if (mutex->state == MxLocked) {
-        if (mutex->tid != tid)
-           record_mutex_error(tid, mutex, "take lock held by someone else", 
-                              mutex->location);
-        else
-           record_mutex_error(tid, mutex, "take lock we already hold", 
-                              mutex->location);
-
-        VG_(skin_panic)("core should have checked this\n");
-        break;
-      }
-
       sk_assert(!check_cycle(mutex, mutex->lockdep));
 
       if (debug)
@@ -1509,7 +1493,6 @@ static void set_mutex_state(Mutex *mutex, MutexState state,
            print_LockSet("lockdep", mutex->lockdep);
         }
       }
-      mutex->tid = tid;
       break;
 
    case MxUnlocked:
@@ -1524,6 +1507,67 @@ static void set_mutex_state(Mutex *mutex, MutexState state,
         record_mutex_error(tid, mutex, 
                            "unlock someone else's mutex", mutex->location);
       }
+      break;
+
+   case MxDead:
+      break;
+
+   default:
+      break;
+   }
+}
+
+/* Update a mutex state.  Expects most error testing and reporting to
+   have happened in test_mutex_state().  The assumption is that no
+   client code is run by thread tid between test and set, either
+   because it is blocked or test and set are called together
+   atomically.  
+
+   Setting state to MxDead is the exception, since that can happen as
+   a result of any thread freeing memory; in this case set_mutex_state
+   does all the error reporting as well.
+*/
+static void set_mutex_state(Mutex *mutex, MutexState state,
+                           ThreadId tid, ThreadState *tst)
+{
+   static const Bool debug = False;
+
+   if (debug)
+      VG_(printf)("\ntid %d changing mutex (%p)->%p%(y state %s -> %s\n",
+                 tid, mutex, mutex->mutexp, mutex->mutexp,
+                 pp_MutexState(mutex->state), pp_MutexState(state));
+
+   if (mutex->state == MxDead) {
+      /* can't do anything legal to a destroyed mutex */
+      return;
+   }
+
+   switch(state) {
+   case MxLocked:
+      if (mutex->state == MxLocked) {
+        if (mutex->tid != tid)
+           record_mutex_error(tid, mutex, "take lock held by someone else", 
+                              mutex->location);
+        else
+           record_mutex_error(tid, mutex, "take lock we already hold", 
+                              mutex->location);
+
+        VG_(skin_panic)("core should have checked this\n");
+        break;
+      }
+
+      sk_assert(!check_cycle(mutex, mutex->lockdep));
+
+      mutex->tid = tid;
+      break;
+
+   case MxUnlocked:
+      if (debug)
+        print_LockSet("thread holding", thread_locks[tid]);
+
+      if (mutex->state != MxLocked || mutex->tid != tid)
+        break;
+
       mutex->tid = VG_INVALID_THREADID;
       break;
 
@@ -2406,6 +2450,13 @@ Bool SK_(error_matches_suppression)(SkinError* err, SkinSupp* su)
 }
 
 
+static void eraser_pre_mutex_lock(ThreadId tid, void* void_mutex)
+{
+   Mutex *mutex = get_mutex((Addr)void_mutex);
+
+   test_mutex_state(mutex, MxLocked, tid, VG_(get_ThreadState)(tid));
+}
+
 static void eraser_post_mutex_lock(ThreadId tid, void* void_mutex)
 {
    static const Bool debug = False;
@@ -2447,6 +2498,7 @@ static void eraser_post_mutex_unlock(ThreadId tid, void* void_mutex)
    Mutex *mutex = get_mutex((Addr)void_mutex);
    const LockSet *ls;
 
+   test_mutex_state(mutex, MxUnlocked, tid, VG_(get_ThreadState)(tid));
    set_mutex_state(mutex, MxUnlocked, tid, VG_(get_ThreadState)(tid));
 
    if (!ismember(thread_locks[tid], mutex))
@@ -2870,6 +2922,7 @@ void SK_(pre_clo_init)(VgDetails* details, VgNeeds* needs, VgTrackEvents* track)
    track->pre_mem_write         = & eraser_pre_mem_write;
    track->post_mem_write        = NULL;
 
+   track->pre_mutex_lock        = & eraser_pre_mutex_lock;
    track->post_mutex_lock       = & eraser_post_mutex_lock;
    track->post_mutex_unlock     = & eraser_post_mutex_unlock;
 

diff --git a/include/vg_skin.h b/include/vg_skin.h
index 79d8471abc6756e03bf13c24995425b761377813..540fe5f90abfed598aea735a692d64a4e0837832 100644 (file)
--- a/include/vg_skin.h
+++ b/include/vg_skin.h
@@ -1345,12 +1345,27 @@ typedef
 
 
       /* Mutex events (not exhaustive) */
+
+      /* Called before a thread can block while waiting for a mutex
+        (called regardless of whether the thread will block or
+        not) */
+      void (*pre_mutex_lock)    ( ThreadId tid, 
+                                  void* /*pthread_mutex_t* */ mutex );
+      /* Called once the thread actually holds the mutex (always
+        paired with pre_mutex_lock) */
       void (*post_mutex_lock)   ( ThreadId tid, 
                                   void* /*pthread_mutex_t* */ mutex );
+      /* Called after a thread has released a mutex (no need for a
+        corresponding pre_mutex_unlock, because unlocking can't
+        block) */
       void (*post_mutex_unlock) ( ThreadId tid, 
                                   void* /*pthread_mutex_t* */ mutex );
 
+      /* Called during thread create, before the new thread has run
+        any instructions (or touched any memory). */
       void (*post_thread_create)( ThreadId tid, ThreadId child );
+      /* Called once the joinee thread is terminated and the joining
+        thread is about to resume. */
       void (*post_thread_join)  ( ThreadId joiner, ThreadId joinee );
       
       /* Others... thread, condition variable, signal events... */