Implement proper atomic code for trace macros.

This is not meant to prevent races on what is pointed to by the uint8*. Instead, it is meant to prevent the compiler from assuming that consecutive loads of the uint8 will be the same value. This prevents the scenario described in Clarification 4: http://code.google.com/p/data-race-test/wiki/AboutRaces#Objection_4

R=dvyukov
BUG=98417


Review URL: http://codereview.chromium.org/9416044

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@122552 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 22 insertions, 15 deletions

diff --git a/base/debug/trace_event.h b/base/debug/trace_event.h
index 2298b03..5860f8d 100644
--- a/base/debug/trace_event.h
+++ b/base/debug/trace_event.h
@@ -145,9 +145,6 @@
 // Without the use of these static category pointers and enabled flags all
 // trace points would carry a significant performance cost of aquiring a lock
 // and resolving the category.
-//
-// ANNOTATE_BENIGN_RACE is used to suppress the warning on the static category
-// pointers.
 
 
 #ifndef BASE_DEBUG_TRACE_EVENT_H_
@@ -156,8 +153,9 @@
 
 #include <string>
 
-#include "build/build_config.h"
+#include "base/atomicops.h"
 #include "base/debug/trace_event_impl.h"
+#include "build/build_config.h"
 
 // By default, const char* argument values are assumed to have long-lived scope
 // and will not be copied. Use this macro to force a const char* to be copied.
@@ -485,14 +483,21 @@
     INTERNAL_TRACE_EVENT_UID2(name_prefix, __LINE__)
 
 // Implementation detail: internal macro to create static category.
-// - ANNOTATE_BENIGN_RACE, see Thread Safety above.
+// No barriers are needed, because this code is designed to operate safely
+// even when the unsigned char* points to garbage data (which may be the case
+// on processors without cache coherency).
 #define INTERNAL_TRACE_EVENT_GET_CATEGORY_INFO(category) \
-    static const unsigned char* INTERNAL_TRACE_EVENT_UID(catstatic) = NULL; \
-    ANNOTATE_BENIGN_RACE(&INTERNAL_TRACE_EVENT_UID(catstatic), \
-                         "trace_event category"); \
-    if (!INTERNAL_TRACE_EVENT_UID(catstatic)) \
+    static base::subtle::AtomicWord INTERNAL_TRACE_EVENT_UID(atomic) = 0; \
+    const uint8* INTERNAL_TRACE_EVENT_UID(catstatic) = \
+        reinterpret_cast<const uint8*>( \
+            base::subtle::NoBarrier_Load(&INTERNAL_TRACE_EVENT_UID(atomic))); \
+    if (!INTERNAL_TRACE_EVENT_UID(catstatic)) { \
       INTERNAL_TRACE_EVENT_UID(catstatic) = \
-          TRACE_EVENT_API_GET_CATEGORY_ENABLED(category);
+          TRACE_EVENT_API_GET_CATEGORY_ENABLED(category); \
+      base::subtle::NoBarrier_Store(&INTERNAL_TRACE_EVENT_UID(atomic), \
+          reinterpret_cast<base::subtle::AtomicWord>( \
+              INTERNAL_TRACE_EVENT_UID(catstatic))); \
+    }
 
 // Implementation detail: internal macro to create static category and add
 // event if the category is enabled.
diff --git a/base/debug/trace_event_impl.cc b/base/debug/trace_event_impl.cc
index 107f87f..39542f7 100644
--- a/base/debug/trace_event_impl.cc
+++ b/base/debug/trace_event_impl.cc
@@ -20,6 +20,7 @@
 #include "base/utf_string_conversions.h"
 #include "base/stl_util.h"
 #include "base/sys_info.h"
+#include "base/third_party/dynamic_annotations/dynamic_annotations.h"
 #include "base/time.h"
 
 #if defined(OS_WIN)
@@ -323,6 +324,12 @@ TraceLog* TraceLog::GetInstance() {
 TraceLog::TraceLog()
     : enabled_(false) {
   SetProcessID(static_cast<int>(base::GetCurrentProcId()));
+  // Trace is enabled or disabled on one thread while other threads are
+  // accessing the enabled flag. We don't care whether edge-case events are
+  // traced or not, so we allow races on the enabled flag to keep the trace
+  // macros fast.
+  ANNOTATE_BENIGN_RACE_SIZED(g_category_enabled, sizeof(g_category_enabled),
+                             "trace_event category enabled");
 }
 
 TraceLog::~TraceLog() {
@@ -361,8 +368,6 @@ static void EnableMatchingCategory(int category_index,
     if (is_match)
       break;
   }
-  ANNOTATE_BENIGN_RACE(&g_category_enabled[category_index],
-                       "trace_event category enabled");
   g_category_enabled[category_index] = is_match ?
       is_included : (is_included ^ 1);
 }
@@ -401,8 +406,6 @@ const unsigned char* TraceLog::GetCategoryEnabledInternal(const char* name) {
       else
         EnableMatchingCategory(new_index, excluded_categories_, 0);
     } else {
-      ANNOTATE_BENIGN_RACE(&g_category_enabled[new_index],
-                           "trace_event category enabled");
       g_category_enabled[new_index] = 0;
     }
     return &g_category_enabled[new_index];
diff --git a/base/debug/trace_event_impl.h b/base/debug/trace_event_impl.h
index dc7b59f..7f7a0cb 100644
--- a/base/debug/trace_event_impl.h
+++ b/base/debug/trace_event_impl.h
@@ -17,7 +17,6 @@
 #include "base/memory/ref_counted_memory.h"
 #include "base/string_util.h"
 #include "base/synchronization/lock.h"
-#include "base/third_party/dynamic_annotations/dynamic_annotations.h"
 #include "base/timer.h"
 
 // Older style trace macros with explicit id and extra data