gutil: properly hook up ANNOTATE_HAPPENS_BEFORE/AFTER This updates dynamic_annotations to hook up ANNOTATE_HAPPENS_BEFORE and ANNOTATE_HAPPENS_AFTER to the appropriate annotations in the TSAN runtime library. According to [1] (committed in 2011) the functions have been implemented in TSAN for quite some time, and it was an error that we weren't using them.
The previous implementation of the function called some condition-variable annotations in the TSAN runtime library instead. Those functions actually turn out to be implemented as no-ops. So, I looked for existing call sites to ANNOTATE_HAPPENS_BEFORE and AFTER and removed them. This cleaned up atomic_refcount pretty substantially. Again I found a matching change[2] in Chromium, from which this code is derived. [1] https://codereview.chromium.org/6982022 [2] https://codereview.chromium.org/580813002 Change-Id: Ida27aff6b9771c0009fba5e31ec7a0c7c53caa59 Reviewed-on: http://gerrit.cloudera.org:8080/9325 Tested-by: Kudu Jenkins Reviewed-by: Mike Percy <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/a964b0e3 Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/a964b0e3 Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/a964b0e3 Branch: refs/heads/master Commit: a964b0e36c7bb16bdbbd7f8aa98c67e1da5a99eb Parents: 2165ce5 Author: Todd Lipcon <[email protected]> Authored: Wed Feb 14 09:28:03 2018 -0800 Committer: Todd Lipcon <[email protected]> Committed: Wed Feb 21 01:48:58 2018 +0000 ---------------------------------------------------------------------- src/kudu/gutil/atomic_refcount.h | 58 +++++-------------------------- src/kudu/gutil/dynamic_annotations.c | 4 +++ src/kudu/gutil/dynamic_annotations.h | 12 +++++-- src/kudu/gutil/once.cc | 3 +- src/kudu/gutil/once.h | 4 --- 5 files changed, 22 insertions(+), 59 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/a964b0e3/src/kudu/gutil/atomic_refcount.h ---------------------------------------------------------------------- diff --git a/src/kudu/gutil/atomic_refcount.h b/src/kudu/gutil/atomic_refcount.h index 9c80921..6500d2d 100644 --- a/src/kudu/gutil/atomic_refcount.h +++ b/src/kudu/gutil/atomic_refcount.h @@ -32,9 +32,6 @@ // initialization you should use the word only via the routines below; the // "volatile" in the signatures below is for backwards compatibility. // -// The implementation includes annotations to avoid some false alarms -// when using Helgrind (the data race detector). -// // If you need to do something very different from this, use a Mutex. #include <glog/logging.h> @@ -42,7 +39,6 @@ #include "kudu/gutil/atomicops.h" #include "kudu/gutil/integral_types.h" #include "kudu/gutil/logging-inl.h" -#include "kudu/gutil/dynamic_annotations.h" namespace base { @@ -65,11 +61,7 @@ inline void RefCountIncN(volatile Atomic32 *ptr, Atomic32 increment) { // became zero will be visible to a thread that has just made the count zero. inline bool RefCountDecN(volatile Atomic32 *ptr, Atomic32 decrement) { DCHECK_GT(decrement, 0); - ANNOTATE_HAPPENS_BEFORE(ptr); bool res = base::subtle::Barrier_AtomicIncrement(ptr, -decrement) != 0; - if (!res) { - ANNOTATE_HAPPENS_AFTER(ptr); - } return res; } @@ -94,22 +86,14 @@ inline bool RefCountDec(volatile Atomic32 *ptr) { // to act on the object, knowing that it has exclusive access to the // object. inline bool RefCountIsOne(const volatile Atomic32 *ptr) { - bool res = base::subtle::Acquire_Load(ptr) == 1; - if (res) { - ANNOTATE_HAPPENS_AFTER(ptr); - } - return res; + return base::subtle::Acquire_Load(ptr) == 1; } // Return whether the reference count is zero. With conventional object // referencing counting, the object will be destroyed, so the reference count // should never be zero. Hence this is generally used for a debug check. inline bool RefCountIsZero(const volatile Atomic32 *ptr) { - bool res = (subtle::Acquire_Load(ptr) == 0); - if (res) { - ANNOTATE_HAPPENS_AFTER(ptr); - } - return res; + return subtle::Acquire_Load(ptr) == 0; } #if BASE_HAS_ATOMIC64 @@ -122,12 +106,7 @@ inline void RefCountIncN(volatile base::subtle::Atomic64 *ptr, inline bool RefCountDecN(volatile base::subtle::Atomic64 *ptr, base::subtle::Atomic64 decrement) { DCHECK_GT(decrement, 0); - ANNOTATE_HAPPENS_BEFORE(ptr); - bool res = base::subtle::Barrier_AtomicIncrement(ptr, -decrement) != 0; - if (!res) { - ANNOTATE_HAPPENS_AFTER(ptr); - } - return res; + return base::subtle::Barrier_AtomicIncrement(ptr, -decrement) != 0; } inline void RefCountInc(volatile base::subtle::Atomic64 *ptr) { base::RefCountIncN(ptr, 1); @@ -136,18 +115,10 @@ inline bool RefCountDec(volatile base::subtle::Atomic64 *ptr) { return base::RefCountDecN(ptr, 1); } inline bool RefCountIsOne(const volatile base::subtle::Atomic64 *ptr) { - bool res = base::subtle::Acquire_Load(ptr) == 1; - if (res) { - ANNOTATE_HAPPENS_AFTER(ptr); - } - return res; + return base::subtle::Acquire_Load(ptr) == 1; } inline bool RefCountIsZero(const volatile base::subtle::Atomic64 *ptr) { - bool res = (base::subtle::Acquire_Load(ptr) == 0); - if (res) { - ANNOTATE_HAPPENS_AFTER(ptr); - } - return res; + return base::subtle::Acquire_Load(ptr) == 0; } #endif @@ -158,13 +129,8 @@ inline void RefCountIncN(volatile AtomicWord *ptr, AtomicWord increment) { reinterpret_cast<volatile AtomicWordCastType *>(ptr), increment); } inline bool RefCountDecN(volatile AtomicWord *ptr, AtomicWord decrement) { - ANNOTATE_HAPPENS_BEFORE(ptr); - bool res = base::RefCountDecN( + return base::RefCountDecN( reinterpret_cast<volatile AtomicWordCastType *>(ptr), decrement); - if (!res) { - ANNOTATE_HAPPENS_AFTER(ptr); - } - return res; } inline void RefCountInc(volatile AtomicWord *ptr) { base::RefCountIncN(ptr, 1); @@ -173,20 +139,12 @@ inline bool RefCountDec(volatile AtomicWord *ptr) { return base::RefCountDecN(ptr, 1); } inline bool RefCountIsOne(const volatile AtomicWord *ptr) { - bool res = base::subtle::Acquire_Load( + return base::subtle::Acquire_Load( reinterpret_cast<const volatile AtomicWordCastType *>(ptr)) == 1; - if (res) { - ANNOTATE_HAPPENS_AFTER(ptr); - } - return res; } inline bool RefCountIsZero(const volatile AtomicWord *ptr) { - bool res = base::subtle::Acquire_Load( + return base::subtle::Acquire_Load( reinterpret_cast<const volatile AtomicWordCastType *>(ptr)) == 0; - if (res) { - ANNOTATE_HAPPENS_AFTER(ptr); - } - return res; } #endif http://git-wip-us.apache.org/repos/asf/kudu/blob/a964b0e3/src/kudu/gutil/dynamic_annotations.c ---------------------------------------------------------------------- diff --git a/src/kudu/gutil/dynamic_annotations.c b/src/kudu/gutil/dynamic_annotations.c index 17a94da..f93b634 100644 --- a/src/kudu/gutil/dynamic_annotations.c +++ b/src/kudu/gutil/dynamic_annotations.c @@ -84,6 +84,10 @@ void AnnotateCondVarSignal(const char *file, int line, const volatile void *cv){} void AnnotateCondVarSignalAll(const char *file, int line, const volatile void *cv){} +void AnnotateHappensBefore(const char *file, int line, + const volatile void *obj); +void AnnotateHappensAfter(const char *file, int line, + const volatile void *obj); void AnnotatePublishMemoryRange(const char *file, int line, const volatile void *address, long size){} http://git-wip-us.apache.org/repos/asf/kudu/blob/a964b0e3/src/kudu/gutil/dynamic_annotations.h ---------------------------------------------------------------------- diff --git a/src/kudu/gutil/dynamic_annotations.h b/src/kudu/gutil/dynamic_annotations.h index 9f838f2..7e03d45 100644 --- a/src/kudu/gutil/dynamic_annotations.h +++ b/src/kudu/gutil/dynamic_annotations.h @@ -121,8 +121,10 @@ AnnotateCondVarSignalAll(__FILE__, __LINE__, cv) /* Annotations for user-defined synchronization mechanisms. */ - #define ANNOTATE_HAPPENS_BEFORE(obj) ANNOTATE_CONDVAR_SIGNAL(obj) - #define ANNOTATE_HAPPENS_AFTER(obj) ANNOTATE_CONDVAR_WAIT(obj) + #define ANNOTATE_HAPPENS_BEFORE(obj) \ + AnnotateHappensBefore(__FILE__, __LINE__, obj) + #define ANNOTATE_HAPPENS_AFTER(obj) \ + AnnotateHappensAfter(__FILE__, __LINE__, obj) /* Report that the bytes in the range [pointer, pointer+size) are about to be published safely. The race checker will create a happens-before @@ -490,9 +492,13 @@ void AnnotateCondVarSignal(const char *file, int line, const volatile void *cv); void AnnotateCondVarSignalAll(const char *file, int line, const volatile void *cv); +void AnnotateHappensBefore(const char *file, int line, + const volatile void *obj); +void AnnotateHappensAfter(const char *file, int line, + const volatile void *obj); void AnnotatePublishMemoryRange(const char *file, int line, const volatile void *address, - long size); + long size); // NOLINT void AnnotateUnpublishMemoryRange(const char *file, int line, const volatile void *address, long size); http://git-wip-us.apache.org/repos/asf/kudu/blob/a964b0e3/src/kudu/gutil/once.cc ---------------------------------------------------------------------- diff --git a/src/kudu/gutil/once.cc b/src/kudu/gutil/once.cc index 34787c6..4171d0a 100644 --- a/src/kudu/gutil/once.cc +++ b/src/kudu/gutil/once.cc @@ -5,7 +5,7 @@ #include <glog/logging.h> #include "kudu/gutil/atomicops.h" -#include "kudu/gutil/dynamic_annotations.h" +#include "kudu/gutil/macros.h" #include "kudu/gutil/integral_types.h" #include "kudu/gutil/logging-inl.h" #include "kudu/gutil/once.h" @@ -44,7 +44,6 @@ void GoogleOnceInternalInit(Atomic32 *control, void (*func)(), } else { (*func_with_arg)(arg); } - ANNOTATE_HAPPENS_BEFORE(control); int32 old_control = base::subtle::NoBarrier_Load(control); base::subtle::Release_Store(control, GOOGLE_ONCE_INTERNAL_DONE); if (old_control == GOOGLE_ONCE_INTERNAL_WAITER) { http://git-wip-us.apache.org/repos/asf/kudu/blob/a964b0e3/src/kudu/gutil/once.h ---------------------------------------------------------------------- diff --git a/src/kudu/gutil/once.h b/src/kudu/gutil/once.h index 460a2ec..640fe02 100644 --- a/src/kudu/gutil/once.h +++ b/src/kudu/gutil/once.h @@ -25,7 +25,6 @@ #define BASE_ONCE_H_ #include "kudu/gutil/atomicops.h" -#include "kudu/gutil/dynamic_annotations.h" #include "kudu/gutil/macros.h" #include "kudu/gutil/port.h" #include "kudu/gutil/type_traits.h" @@ -53,7 +52,6 @@ inline void GoogleOnceInit(GoogleOnceType* state, void (*func)()) { if (PREDICT_FALSE(s != GOOGLE_ONCE_INTERNAL_DONE)) { GoogleOnceInternalInit(&state->state, func, 0, 0); } - ANNOTATE_HAPPENS_AFTER(&state->state); } // A version of GoogleOnceInit where the function argument takes a pointer @@ -69,7 +67,6 @@ inline void GoogleOnceInitArg(GoogleOnceType* state, reinterpret_cast<void(*)(void*)>(func_with_arg), const_cast<mutable_T*>(arg)); } - ANNOTATE_HAPPENS_AFTER(&state->state); } // GoogleOnceDynamic is like GoogleOnceType, but is dynamically @@ -108,7 +105,6 @@ class GoogleOnceDynamic { reinterpret_cast<void (*)(void*)>(func_with_arg), const_cast<mutable_T*>(arg)); } - ANNOTATE_HAPPENS_AFTER(&this->state_); } private: Atomic32 state_;
