From c8dc6fcb4cd29e5fb6c509149871b869b951295d Mon Sep 17 00:00:00 2001 From: Brian Anderson Date: Fri, 30 Mar 2012 18:37:30 -0700 Subject: [PATCH] Revert "rt: Remove lock_held_by_current_thread" Adds back the ability to make assertions about locks, but only under the --enable-debug configuration This reverts commit b247de64583e2ab527088813ba9192824554e801. Conflicts: src/rt/rust_sched_loop.cpp --- src/etc/x86.supp | 28 ++++++++++++++++ src/rt/rust_port_selector.cpp | 2 ++ src/rt/rust_sched_loop.cpp | 2 ++ src/rt/rust_task.cpp | 1 + src/rt/sync/lock_and_signal.cpp | 59 +++++++++++++++++++++++++++++++++ src/rt/sync/lock_and_signal.h | 19 +++++++++++ 6 files changed, 111 insertions(+) diff --git a/src/etc/x86.supp b/src/etc/x86.supp index e9c1c6c9d3b..e2d6cec94dd 100644 --- a/src/etc/x86.supp +++ b/src/etc/x86.supp @@ -389,6 +389,34 @@ fun:uv_loop_delete } +{ + lock_and_signal-probably-threadsafe-access-outside-of-lock + Helgrind:Race + fun:_ZN15lock_and_signal27lock_held_by_current_threadEv + ... +} + +{ + lock_and_signal-probably-threadsafe-access-outside-of-lock2 + Helgrind:Race + fun:_ZN15lock_and_signal6unlockEv + ... +} + +{ + lock_and_signal-probably-threadsafe-access-outside-of-lock3 + Helgrind:Race + fun:_ZN15lock_and_signal4lockEv + ... +} + +{ + lock_and_signal-probably-threadsafe-access-outside-of-lock4 + Helgrind:Race + fun:_ZN15lock_and_signal4waitEv + ... +} + { uv-async-send-does-racy-things Helgrind:Race diff --git a/src/rt/rust_port_selector.cpp b/src/rt/rust_port_selector.cpp index 042201cace5..2dcc1fa0f06 100644 --- a/src/rt/rust_port_selector.cpp +++ b/src/rt/rust_port_selector.cpp @@ -69,6 +69,8 @@ void rust_port_selector::msg_sent_on(rust_port *port) { rust_task *task = port->task; + port->lock.must_not_have_lock(); + // Prevent two ports from trying to wake up the task // simultaneously scoped_lock with(rendezvous_lock); diff --git a/src/rt/rust_sched_loop.cpp b/src/rt/rust_sched_loop.cpp index ce549db5236..d494fa487cf 100644 --- a/src/rt/rust_sched_loop.cpp +++ b/src/rt/rust_sched_loop.cpp @@ -108,6 +108,8 @@ rust_sched_loop::number_of_live_tasks() { */ void rust_sched_loop::reap_dead_tasks() { + lock.must_have_lock(); + if (dead_task == NULL) { return; } diff --git a/src/rt/rust_task.cpp b/src/rt/rust_task.cpp index d276b5c904b..4ab1f4680eb 100644 --- a/src/rt/rust_task.cpp +++ b/src/rt/rust_task.cpp @@ -212,6 +212,7 @@ rust_task::must_fail_from_being_killed() { bool rust_task::must_fail_from_being_killed_unlocked() { + kill_lock.must_have_lock(); return killed && !reentered_rust_stack; } diff --git a/src/rt/sync/lock_and_signal.cpp b/src/rt/sync/lock_and_signal.cpp index 35576f7fd97..1d0842e6c60 100644 --- a/src/rt/sync/lock_and_signal.cpp +++ b/src/rt/sync/lock_and_signal.cpp @@ -10,8 +10,15 @@ #include "lock_and_signal.h" +// FIXME: This is not a portable way of specifying an invalid pthread_t +#define INVALID_THREAD 0 + + #if defined(__WIN32__) lock_and_signal::lock_and_signal() +#if defined(DEBUG_LOCKS) + : _holding_thread(INVALID_THREAD) +#endif { _event = CreateEvent(NULL, FALSE, FALSE, NULL); @@ -30,6 +37,9 @@ lock_and_signal::lock_and_signal() #else lock_and_signal::lock_and_signal() +#if defined(DEBUG_LOCKS) + : _holding_thread(INVALID_THREAD) +#endif { CHECKED(pthread_cond_init(&_cond, NULL)); CHECKED(pthread_mutex_init(&_mutex, NULL)); @@ -47,14 +57,25 @@ lock_and_signal::~lock_and_signal() { } void lock_and_signal::lock() { + must_not_have_lock(); #if defined(__WIN32__) EnterCriticalSection(&_cs); +#if defined(DEBUG_LOCKS) + _holding_thread = GetCurrentThreadId(); +#endif #else CHECKED(pthread_mutex_lock(&_mutex)); +#if defined(DEBUG_LOCKS) + _holding_thread = pthread_self(); +#endif #endif } void lock_and_signal::unlock() { + must_have_lock(); +#if defined(DEBUG_LOCKS) + _holding_thread = INVALID_THREAD; +#endif #if defined(__WIN32__) LeaveCriticalSection(&_cs); #else @@ -66,12 +87,24 @@ void lock_and_signal::unlock() { * Wait indefinitely until condition is signaled. */ void lock_and_signal::wait() { + must_have_lock(); +#if defined(DEBUG_LOCKS) + _holding_thread = INVALID_THREAD; +#endif #if defined(__WIN32__) LeaveCriticalSection(&_cs); WaitForSingleObject(_event, INFINITE); EnterCriticalSection(&_cs); + must_not_be_locked(); +#if defined(DEBUG_LOCKS) + _holding_thread = GetCurrentThreadId(); +#endif #else CHECKED(pthread_cond_wait(&_cond, &_mutex)); + must_not_be_locked(); +#if defined(DEBUG_LOCKS) + _holding_thread = pthread_self(); +#endif #endif } @@ -86,6 +119,32 @@ void lock_and_signal::signal() { #endif } +#if defined(DEBUG_LOCKS) +bool lock_and_signal::lock_held_by_current_thread() +{ +#if defined(__WIN32__) + return _holding_thread == GetCurrentThreadId(); +#else + return pthread_equal(_holding_thread, pthread_self()); +#endif +} +#endif + +#if defined(DEBUG_LOCKS) +void lock_and_signal::must_have_lock() { + assert(lock_held_by_current_thread() && "must have lock"); +} +void lock_and_signal::must_not_have_lock() { + assert(!lock_held_by_current_thread() && "must not have lock"); +} +void lock_and_signal::must_not_be_locked() { +} +#else +void lock_and_signal::must_have_lock() { } +void lock_and_signal::must_not_have_lock() { } +void lock_and_signal::must_not_be_locked() { } +#endif + scoped_lock::scoped_lock(lock_and_signal &lock) : lock(lock) { diff --git a/src/rt/sync/lock_and_signal.h b/src/rt/sync/lock_and_signal.h index f4ffcc30a68..fae9b1c24ea 100644 --- a/src/rt/sync/lock_and_signal.h +++ b/src/rt/sync/lock_and_signal.h @@ -2,14 +2,30 @@ #ifndef LOCK_AND_SIGNAL_H #define LOCK_AND_SIGNAL_H +#ifndef RUST_NDEBUG +#define DEBUG_LOCKS +#endif + class lock_and_signal { #if defined(__WIN32__) HANDLE _event; CRITICAL_SECTION _cs; +#if defined(DEBUG_LOCKS) + DWORD _holding_thread; +#endif #else pthread_cond_t _cond; pthread_mutex_t _mutex; +#if defined(DEBUG_LOCKS) + pthread_t _holding_thread; #endif +#endif + +#if defined(DEBUG_LOCKS) + bool lock_held_by_current_thread(); +#endif + + void must_not_be_locked(); public: lock_and_signal(); @@ -19,6 +35,9 @@ public: void unlock(); void wait(); void signal(); + + void must_have_lock(); + void must_not_have_lock(); }; class scoped_lock {