From c043ff79e38bbc02e1eee8e9aca6e86986096276 Mon Sep 17 00:00:00 2001 From: Alexander Kjeldaas Date: Sat, 10 Nov 2012 22:21:07 -0300 Subject: [PATCH 1/3] o Added threadsafety.h - a set of macros using the -Wthread-safety feature in clang. These macros should primarily be used to document which locks protect a given piece of data. Secondary it can be used to document the set of held and excluded locks when entering a function. --- src/threadsafety.h | 53 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 src/threadsafety.h diff --git a/src/threadsafety.h b/src/threadsafety.h new file mode 100644 index 00000000..3d3d526f --- /dev/null +++ b/src/threadsafety.h @@ -0,0 +1,53 @@ +// Copyright (c) 2009-2010 Satoshi Nakamoto +// Copyright (c) 2009-2012 The Bitcoin developers +// Distributed under the MIT/X11 software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. +#ifndef BITCOIN_THREADSAFETY_H +#define BITCOIN_THREADSAFETY_H + +#ifdef __clang__ +// TL;DR Add GUARDED_BY(mutex) to member variables. The others are +// rarely necessary. Ex: int nFoo GUARDED_BY(cs_foo); +// +// See http://clang.llvm.org/docs/LanguageExtensions.html#threadsafety +// for documentation. The clang compiler can do advanced static analysis +// of locking when given the -Wthread-safety option. +#define LOCKABLE __attribute__ ((lockable)) +#define SCOPED_LOCKABLE __attribute__ ((scoped_lockable)) +#define GUARDED_BY(x) __attribute__ ((guarded_by(x))) +#define GUARDED_VAR __attribute__ ((guarded_var)) +#define PT_GUARDED_BY(x) __attribute__ ((pt_guarded_by(x))) +#define PT_GUARDED_VAR __attribute__ ((pt_guarded_var)) +#define ACQUIRED_AFTER(...) __attribute__ ((acquired_after(__VA_ARGS__))) +#define ACQUIRED_BEFORE(...) __attribute__ ((acquired_before(__VA_ARGS__))) +#define EXCLUSIVE_LOCK_FUNCTION(...) __attribute__ ((exclusive_lock_function(__VA_ARGS__))) +#define SHARED_LOCK_FUNCTION(...) __attribute__ ((shared_lock_function(__VA_ARGS__))) +#define EXCLUSIVE_TRYLOCK_FUNCTION(...) __attribute__ ((exclusive_trylock_function(__VA_ARGS__))) +#define SHARED_TRYLOCK_FUNCTION(...) __attribute__ ((shared_trylock_function(__VA_ARGS__))) +#define UNLOCK_FUNCTION(...) __attribute__ ((unlock_function(__VA_ARGS__))) +#define LOCK_RETURNED(x) __attribute__ ((lock_returned(x))) +#define LOCKS_EXCLUDED(...) __attribute__ ((locks_excluded(__VA_ARGS__))) +#define EXCLUSIVE_LOCKS_REQUIRED(...) __attribute__ ((exclusive_locks_required(__VA_ARGS__))) +#define SHARED_LOCKS_REQUIRED(...) __attribute__ ((shared_locks_required(__VA_ARGS__))) +#define NO_THREAD_SAFETY_ANALYSIS __attribute__ ((no_thread_safety_analysis)) +#else +#define LOCKABLE +#define SCOPED_LOCKABLE +#define GUARDED_BY(x) +#define GUARDED_VAR +#define PT_GUARDED_BY(x) +#define PT_GUARDED_VAR +#define ACQUIRED_AFTER(...) +#define ACQUIRED_BEFORE(...) +#define EXCLUSIVE_LOCK_FUNCTION(...) +#define SHARED_LOCK_FUNCTION(...) +#define EXCLUSIVE_TRYLOCK_FUNCTION(...) +#define SHARED_TRYLOCK_FUNCTION(...) +#define UNLOCK_FUNCTION(...) +#define LOCK_RETURNED(x) +#define LOCKS_EXCLUDED(...) +#define EXCLUSIVE_LOCKS_REQUIRED(...) +#define SHARED_LOCKS_REQUIRED(...) +#define NO_THREAD_SAFETY_ANALYSIS +#endif // __GNUC__ +#endif // BITCOIN_THREADSAFETY_H From 05f97d1263540b5f9faae971374a25ddc3f6dadb Mon Sep 17 00:00:00 2001 From: Alexander Kjeldaas Date: Sat, 10 Nov 2012 23:50:26 -0300 Subject: [PATCH 2/3] o Added AnnotatedMixin which adds locking annotations to the mutex API, compatible with clang's -Wthread-safety --- src/sync.h | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/src/sync.h b/src/sync.h index e80efbe0..b9c89027 100644 --- a/src/sync.h +++ b/src/sync.h @@ -9,15 +9,36 @@ #include #include #include +#include "threadsafety.h" +// Template mixin that adds -Wthread-safety locking annotations to a +// subset of the mutex API. +template +class LOCKABLE AnnotatedMixin : public PARENT +{ +public: + void lock() EXCLUSIVE_LOCK_FUNCTION() + { + PARENT::lock(); + } + void unlock() UNLOCK_FUNCTION() + { + PARENT::unlock(); + } + bool try_lock() EXCLUSIVE_TRYLOCK_FUNCTION(true) + { + return PARENT::try_lock(); + } +}; /** Wrapped boost mutex: supports recursive locking, but no waiting */ -typedef boost::recursive_mutex CCriticalSection; +// TODO: We should move away from using the recursive lock by default. +typedef AnnotatedMixin CCriticalSection; /** Wrapped boost mutex: supports waiting but not recursive locking */ -typedef boost::mutex CWaitableCriticalSection; +typedef AnnotatedMixin CWaitableCriticalSection; #ifdef DEBUG_LOCKORDER void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false); From 25511af4a57816c4f9bb960527f090a9719c9010 Mon Sep 17 00:00:00 2001 From: Alexander Kjeldaas Date: Sat, 10 Nov 2012 23:51:50 -0300 Subject: [PATCH 3/3] o Annotated lock-like functions in net.h. o Removed unused function EndMessageAbortIfEmpty --- src/net.h | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/src/net.h b/src/net.h index 57c53035..07da6ede 100644 --- a/src/net.h +++ b/src/net.h @@ -311,7 +311,8 @@ public: - void BeginMessage(const char* pszCommand) + // TODO: Document the postcondition of this function. Is cs_vSend locked? + void BeginMessage(const char* pszCommand) EXCLUSIVE_LOCK_FUNCTION(cs_vSend) { ENTER_CRITICAL_SECTION(cs_vSend); if (nHeaderStart != -1) @@ -323,7 +324,8 @@ public: printf("sending: %s ", pszCommand); } - void AbortMessage() + // TODO: Document the precondition of this function. Is cs_vSend locked? + void AbortMessage() UNLOCK_FUNCTION(cs_vSend) { if (nHeaderStart < 0) return; @@ -336,7 +338,8 @@ public: printf("(aborted)\n"); } - void EndMessage() + // TODO: Document the precondition of this function. Is cs_vSend locked? + void EndMessage() UNLOCK_FUNCTION(cs_vSend) { if (mapArgs.count("-dropmessagestest") && GetRand(atoi(mapArgs["-dropmessagestest"])) == 0) { @@ -368,19 +371,6 @@ public: LEAVE_CRITICAL_SECTION(cs_vSend); } - void EndMessageAbortIfEmpty() - { - if (nHeaderStart < 0) - return; - int nSize = vSend.size() - nMessageStart; - if (nSize > 0) - EndMessage(); - else - AbortMessage(); - } - - - void PushVersion();