You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by al...@apache.org on 2018/02/21 02:32:29 UTC
[3/4] kudu git commit: gutil: properly hook up
ANNOTATE_HAPPENS_BEFORE/AFTER
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 <mp...@apache.org>
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 <to...@apache.org>
Authored: Wed Feb 14 09:28:03 2018 -0800
Committer: Todd Lipcon <to...@apache.org>
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_;