helgrind: fix lock-kind mismatch assert failure

tl_assert(lk->kind == LK_rdwr) in evhH__post_thread_r_acquires_lock
may fail in cases where memory containing an rwlock or mutex is
reused for a lock of a different kind without having called
pthread_*_destroy on the first lock.

Fix this by having map_locks_lookup_or_create detect when a lock kind at
a given guest address has changed from an rwlock to mutex or mutex to
rwlock.  The old lock record is removed and a new one is added reflecting
the current lock kind.

https://bugs.kde.org/show_bug.cgi?id=513598

diff --git a/.gitignore b/.gitignore
index a185e7dab6f7d46f205b12a1b3d09f0f88c75250..5472497625605177697a0db715a22273af5e24fc 100644 (file)
--- a/.gitignore
+++ b/.gitignore
@@ -682,6 +682,7 @@
 /helgrind/tests/bug327548
 /helgrind/tests/bug392331
 /helgrind/tests/bug484480
+/helgrind/tests/bug513598
 /helgrind/tests/cond_init_destroy
 /helgrind/tests/cond_timedwait_invalid
 /helgrind/tests/cond_timedwait_test

diff --git a/NEWS b/NEWS
index ae2449c0f078852c55bc482e7a1890ab637489de..d2ff4de3d0aa80af0da2be7931b9baab2647fe75 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -97,6 +97,8 @@ are not entered into bugzilla tend to get forgotten about or ignored.
 513522  m_libcassert.c: 'ordered comparison of pointer with integer zero'
         compiler warning
 513475  Add SSE4.1 PMULLD instruction for x86 32 bit
+513598  Helgrind should detect locks without pthread_{rwlock,mutex}_destroy
+        being called - Assertion 'lk->kind == LK_rdwr' failed.
 514094  readlink("/proc/self/exe") overwrites buffer beyond its return value
 514206  Assertion '!sr_isError(sr)' failed - mmap fd points to an open
         descriptor to a PCI device

diff --git a/helgrind/hg_main.c b/helgrind/hg_main.c
index 27739c37a57edbc43df37c696cb965d53f59c81f..f282b8dd6be98fbfd2211fc280b483042ceb2179 100644 (file)
--- a/helgrind/hg_main.c
+++ b/helgrind/hg_main.c
@@ -750,6 +750,10 @@ static Int HG_(get_pthread_create_nesting_level)(ThreadId tid) {
 /*--- map_locks :: WordFM guest-Addr-of-lock Lock*             ---*/
 /*----------------------------------------------------------------*/
 
+static void map_locks_delete ( Addr ga ); /* fwds */
+__attribute__((noinline))
+static void laog__handle_one_lock_deletion ( Lock* lk ); /* fwds */
+
 /* Make sure there is a lock table entry for the given (lock) guest
    address.  If not, create one of the stated 'kind' in unheld state.
    In any case, return the address of the existing or new Lock. */
@@ -772,6 +776,33 @@ Lock* map_locks_lookup_or_create ( LockKind lkk, Addr ga, ThreadId tid )
       tl_assert(oldlock != NULL);
       tl_assert(HG_(is_sane_LockN)(oldlock));
       tl_assert(oldlock->guestaddr == ga);
+      /* Check for a mismatch between an rwlock and a mutex of either
+         kind. evh__HG_PTHREAD_MUTEX_LOCK_POST uses LK_mbRec as the
+         fallback kind, so a non-recursive mutex first encountered
+         at lock time rather than init time is expected to mismatch. */
+      Bool old_is_mutex = (oldlock->kind == LK_nonRec
+                           || oldlock->kind == LK_mbRec);
+      Bool new_is_mutex = (lkk == LK_nonRec || lkk == LK_mbRec);
+      if (old_is_mutex != new_is_mutex) {
+         /* At address GA a mutex has replaced an rwlock or an rwlock
+            has replaced a mutex.  Remove the stale oldlock record.  */
+         if (oldlock->heldBy) {
+            remove_Lock_from_locksets_of_all_owning_Threads(oldlock);
+            VG_(deleteBag)(oldlock->heldBy);
+            oldlock->heldBy = NULL;
+            oldlock->heldW = False;
+            oldlock->acquired_at = NULL;
+         }
+         if (HG_(clo_track_lockorders))
+            laog__handle_one_lock_deletion(oldlock);
+         map_locks_delete(ga);
+         del_LockN(oldlock);
+         Lock* lock = mk_LockN( lkk, ga );
+         lock->appeared_at = VG_(record_ExeContext)( tid, 0 );
+         tl_assert(HG_(is_sane_LockN)(lock));
+         VG_(addToFM)( map_locks, (UWord)ga, (UWord)lock );
+         return lock;
+      }
       return oldlock;
    }
 }
@@ -1008,8 +1039,6 @@ static void all__sanity_check ( const HChar* who ) {
 static void laog__pre_thread_acquires_lock ( Thread*, Lock* ); /* fwds */
 //static void laog__handle_lock_deletions    ( WordSetID ); /* fwds */
 static inline Thread* get_current_Thread ( void ); /* fwds */
-__attribute__((noinline))
-static void laog__handle_one_lock_deletion ( Lock* lk ); /* fwds */
 
 
 /* Block-copy states (needed for implementing realloc()). */

diff --git a/helgrind/tests/Makefile.am b/helgrind/tests/Makefile.am
index f7b2b19807383e526b30451aeee89c997b36b24b..029c543077688fee1a6979b36de95ab5adbfdd9c 100644 (file)
--- a/helgrind/tests/Makefile.am
+++ b/helgrind/tests/Makefile.am
@@ -15,6 +15,7 @@ noinst_SCRIPTS = \
        filter_stderr
 
 EXTRA_DIST = \
+       bug513598.vgtest \
        annotate_hbefore.vgtest \
        annotate_rwlock.vgtest annotate_rwlock.stderr.exp \
        annotate_smart_pointer.vgtest annotate_smart_pointer.stderr.exp \
@@ -146,6 +147,7 @@ noinst_HEADERS = safe-pthread.h safe-semaphore.h
 # should be conditionally compiled like tc20_verifywrap is.
 check_PROGRAMS = \
        annotate_hbefore \
+       bug513598 \
        bug327548 \
        bug484480 \
        cond_init_destroy \

diff --git a/helgrind/tests/bug513598.c b/helgrind/tests/bug513598.c
new file mode 100644 (file)
index 0000000..9cc66fe
--- /dev/null
+++ b/helgrind/tests/bug513598.c
@@ -0,0 +1,22 @@
+/* Verify that no assert failure occurs when memory holding a mutex
+   is reused for an rwlock without calling pthread_mutex_destroy first. */
+
+#include <pthread.h>
+
+int main(void)
+{
+   /* Force both locks to occupy the same address. */
+   union {
+      pthread_mutex_t  mutex;
+      pthread_rwlock_t rwlock;
+   } u;
+
+   pthread_mutex_init(&u.mutex, NULL);
+   /* Deliberately skip pthread_mutex_destroy, simulating memory reuse
+      with a different lock type. */
+   pthread_rwlock_init(&u.rwlock, NULL);
+   pthread_rwlock_rdlock(&u.rwlock);
+   pthread_rwlock_unlock(&u.rwlock);
+   pthread_rwlock_destroy(&u.rwlock);
+   return 0;
+}

diff --git a/helgrind/tests/bug513598.vgtest b/helgrind/tests/bug513598.vgtest
new file mode 100644 (file)
index 0000000..99a2ae9
--- /dev/null
+++ b/helgrind/tests/bug513598.vgtest
@@ -0,0 +1,3 @@
+prereq: test -e bug513598
+prog: bug513598
+vgopts: -q