TSAN: use somewhat pithier SK_ANNOTATE_UNPROTECTED_READ.

This is a little bit better practice to be i than the existing SK_ANNOTATE_BENIGN_RACE, as UNPROTECTED_READ will only ignore reads, not writes.

Tag SkRefCnt::unique() as a safe unprotected read like SkOnce's double-checked locking.

BUG=skia:
R=reed@google.com, bungeman@google.com

Author: mtklein@google.com

Review URL: https://codereview.chromium.org/144953005

git-svn-id: http://skia.googlecode.com/svn/trunk@13309 2bbb7eff-a529-9590-31e7-b0007b416f81

diff --git a/include/core/SkDynamicAnnotations.h b/include/core/SkDynamicAnnotations.h
new file mode 100644 (file)
index 0000000..6d21cdd
--- /dev/null
+++ b/include/core/SkDynamicAnnotations.h
@@ -0,0 +1,46 @@
+/*
+ * Copyright 2014 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#ifndef SkDynamicAnnotations_DEFINED
+#define SkDynamicAnnotations_DEFINED
+
+// This file contains macros used to send out-of-band signals to dynamic instrumentation systems,
+// namely thread sanitizer.  This is a cut-down version of the full dynamic_annotations library with
+// only the features used by Skia.
+
+// We check the same define to know to enable the annotations, but prefix all our macros with SK_.
+#if DYNAMIC_ANNOTATIONS_ENABLED
+
+extern "C" {
+// TSAN provides these hooks.
+void AnnotateIgnoreReadsBegin(const char* file, int line);
+void AnnotateIgnoreReadsEnd(const char* file, int line);
+}  // extern "C"
+
+// SK_ANNOTATE_UNPROTECTED_READ can wrap any variable read to tell TSAN to ignore that it appears to
+// be a racy read.  This should be used only when we can make an external guarantee that though this
+// particular read is racy, it is being used as part of a mechanism which is thread safe.  Examples:
+//   - the first check in double-checked locking;
+//   - checking if a ref count is equal to 1.
+// Note that in both these cases, we must still add terrifyingly subtle memory barriers to provide
+// that overall thread safety guarantee.  Using this macro to shut TSAN up without providing such an
+// external guarantee is pretty much never correct.
+template <typename T>
+inline T SK_ANNOTATE_UNPROTECTED_READ(const volatile T& x) {
+    AnnotateIgnoreReadsBegin(__FILE__, __LINE__);
+    T read = x;
+    AnnotateIgnoreReadsEnd(__FILE__, __LINE__);
+    return read;
+}
+
+#else  // !DYNAMIC_ANNOTATIONS_ENABLED
+
+#define SK_ANNOTATE_UNPROTECTED_READ(x) (x)
+
+#endif
+
+#endif//SkDynamicAnnotations_DEFINED

diff --git a/include/core/SkOnce.h b/include/core/SkOnce.h
index 9663ef1..59eaf59 100644 (file)
--- a/include/core/SkOnce.h
+++ b/include/core/SkOnce.h
@@ -28,6 +28,7 @@
 //
 // You may optionally pass SkOnce a second function to be called at exit for cleanup.
 
+#include "SkDynamicAnnotations.h"
 #include "SkThread.h"
 #include "SkTypes.h"
 
@@ -115,26 +116,10 @@ static void sk_once_slow(SkOnceFlag* once, Func f, Arg arg, void (*atExit)()) {
     }
 }
 
-// We nabbed this code from the dynamic_annotations library, and in their honor
-// we check the same define.  If you find yourself wanting more than just
-// SK_ANNOTATE_BENIGN_RACE, it might make sense to pull that in as a dependency
-// rather than continue to reproduce it here.
-
-#if DYNAMIC_ANNOTATIONS_ENABLED
-// TSAN provides this hook to supress a known-safe apparent race.
-extern "C" {
-void AnnotateBenignRace(const char* file, int line, const volatile void* mem, const char* desc);
-}
-#define SK_ANNOTATE_BENIGN_RACE(mem, desc) AnnotateBenignRace(__FILE__, __LINE__, mem, desc)
-#else
-#define SK_ANNOTATE_BENIGN_RACE(mem, desc)
-#endif
-
 // This is our fast path, called all the time.  We do really want it to be inlined.
 template <typename Func, typename Arg>
 inline void SkOnce(SkOnceFlag* once, Func f, Arg arg, void(*atExit)()) {
-    SK_ANNOTATE_BENIGN_RACE(&(once->done), "Don't worry TSAN, we're sure this is safe.");
-    if (!once->done) {
+    if (!SK_ANNOTATE_UNPROTECTED_READ(once->done)) {
         sk_once_slow(once, f, arg, atExit);
     }
     // Also known as a load-load/load-store barrier, this acquire barrier makes

diff --git a/include/core/SkRefCnt.h b/include/core/SkRefCnt.h
index 2859192..e4524be 100644 (file)
--- a/include/core/SkRefCnt.h
+++ b/include/core/SkRefCnt.h
@@ -10,6 +10,7 @@
 #ifndef SkRefCnt_DEFINED
 #define SkRefCnt_DEFINED
 
+#include "SkDynamicAnnotations.h"
 #include "SkThread.h"
 #include "SkInstCnt.h"
 #include "SkTemplates.h"
@@ -44,11 +45,13 @@ public:
     /** Return the reference count. Use only for debugging. */
     int32_t getRefCnt() const { return fRefCnt; }
 
-    /** Returns true if the caller is the only owner.
+    /** May return true if the caller is the only owner.
      *  Ensures that all previous owner's actions are complete.
      */
     bool unique() const {
-        bool const unique = (1 == fRefCnt);
+        // We believe we're reading fRefCnt in a safe way here, so we stifle the TSAN warning about
+        // an unproctected read.  Generally, don't read fRefCnt, and don't stifle this warning.
+        bool const unique = (1 == SK_ANNOTATE_UNPROTECTED_READ(fRefCnt));
         if (unique) {
             // Acquire barrier (L/SL), if not provided by load of fRefCnt.
             // Prevents user's 'unique' code from happening before decrements.