Add TSAN annotations for TypedArray accesses

TSAN finds data races in generated JavaScript code that use
access the SharedArrayBuffer backing store racily. These are races, but
they are OK in the sense that the JavaScript memory model allows for the
potential bad behavior they could introduce (e.g. potentially tearing
reads). Relaxed atomics could be used here instead, but that could
introduce performance regressions.

This change adds TSAN annotations to the TypedArray reads/writes to
prevent TSAN from warning about them.

Bug: chromium:722871
Change-Id: I0776475f02a352b678ade7d32ed6bd4a6be98c36
Reviewed-on: https://chromium-review.googlesource.com/656509
Commit-Queue: Ben Smith <binji@chromium.org>
Reviewed-by: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#47929}

BUILD.gn

@@ -2460,6 +2460,7 @@ v8_component("v8_libbase") {
     "src/base/sys-info.h",
     "src/base/template-utils.h",
     "src/base/timezone-cache.h",
+    "src/base/tsan.h",
     "src/base/utils/random-number-generator.cc",
     "src/base/utils/random-number-generator.h",
   ]

src/base/tsan.h

+// Copyright 2017 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef V8_BASE_TSAN_H_
+#define V8_BASE_TSAN_H_
+
+namespace v8 {
+namespace base {
+
+// This file contains annotations for ThreadSanitizer (TSan), a race detector.
+// See
+// https://llvm.org/svn/llvm-project/compiler-rt/trunk/lib/tsan/rtl/tsan_interface_ann.cc
+
+#if THREAD_SANITIZER
+
+#define TSAN_ANNOTATE_IGNORE_READS_BEGIN \
+  v8::base::AnnotateIgnoreReadsBegin(__FILE__, __LINE__)
+#define TSAN_ANNOTATE_IGNORE_READS_END \
+  v8::base::AnnotateIgnoreReadsEnd(__FILE__, __LINE__)
+#define TSAN_ANNOTATE_IGNORE_WRITES_BEGIN \
+  v8::base::AnnotateIgnoreWritesBegin(__FILE__, __LINE__)
+#define TSAN_ANNOTATE_IGNORE_WRITES_END \
+  v8::base::AnnotateIgnoreWritesEnd(__FILE__, __LINE__)
+
+extern "C" {
+
+void AnnotateIgnoreReadsBegin(const char* file, int line);
+void AnnotateIgnoreReadsEnd(const char* file, int line);
+void AnnotateIgnoreWritesBegin(const char* file, int line);
+void AnnotateIgnoreWritesEnd(const char* file, int line);
+
+}  // extern "C"
+
+#else
+
+#define TSAN_ANNOTATE_IGNORE_READS_BEGIN ((void)0)
+#define TSAN_ANNOTATE_IGNORE_READS_END ((void)0)
+#define TSAN_ANNOTATE_IGNORE_WRITES_BEGIN ((void)0)
+#define TSAN_ANNOTATE_IGNORE_WRITES_END ((void)0)
+
+#endif
+
+}  // namespace base
+}  // namespace v8
+
+#endif  // V8_BASE_TSAN_H_

src/objects-inl.h

@@ -14,6 +14,7 @@
 
 #include "src/base/atomicops.h"
 #include "src/base/bits.h"
+#include "src/base/tsan.h"
 #include "src/builtins/builtins.h"
 #include "src/contexts-inl.h"
 #include "src/conversions-inl.h"
@@ -2994,20 +2995,34 @@ double Float64ArrayTraits::defaultValue() {
   return std::numeric_limits<double>::quiet_NaN();
 }
 
-
 template <class Traits>
 typename Traits::ElementType FixedTypedArray<Traits>::get_scalar(int index) {
   DCHECK((index >= 0) && (index < this->length()));
-  ElementType* ptr = reinterpret_cast<ElementType*>(DataPtr());
-  return ptr[index];
+  // The JavaScript memory model allows for racy reads and writes to a
+  // SharedArrayBuffer's backing store, which will always be a FixedTypedArray.
+  // ThreadSanitizer will catch these racy accesses and warn about them, so we
+  // disable TSAN for these reads and writes using annotations.
+  //
+  // The access is marked as volatile so the reads/writes will not be elided or
+  // duplicated. We don't use relaxed atomics here, as it is not a requirement
+  // of the JavaScript memory model to have tear-free reads of overlapping
+  // accesses, and using relaxed atomics may introduce overhead.
+  auto* ptr = reinterpret_cast<volatile ElementType*>(DataPtr());
+  TSAN_ANNOTATE_IGNORE_READS_BEGIN;
+  auto result = ptr[index];
+  TSAN_ANNOTATE_IGNORE_READS_END;
+  return result;
 }
 
 
 template <class Traits>
 void FixedTypedArray<Traits>::set(int index, ElementType value) {
   CHECK((index >= 0) && (index < this->length()));
-  ElementType* ptr = reinterpret_cast<ElementType*>(DataPtr());
+  // See the comment in FixedTypedArray<Traits>::get_scalar.
+  auto* ptr = reinterpret_cast<volatile ElementType*>(DataPtr());
+  TSAN_ANNOTATE_IGNORE_WRITES_BEGIN;
   ptr[index] = value;
+  TSAN_ANNOTATE_IGNORE_WRITES_END;
 }
 
 template <class Traits>

src/v8.gyp

@@ -1889,6 +1889,7 @@
         'base/sys-info.h',
         'base/template-utils.h',
         'base/timezone-cache.h',
+        'base/tsan.h',
         'base/utils/random-number-generator.cc',
         'base/utils/random-number-generator.h',
       ],

test/mjsunit/regress/regress-crbug-722871.js

+// Copyright 2017 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+let sab = new SharedArrayBuffer(10 * 4);
+let memory = new Int32Array(sab);
+let workers = [];
+let runningWorkers = 0;
+
+function startWorker(script) {
+  let worker = new Worker(script);
+  worker.done = false;
+  worker.idx = workers.length;
+  workers.push(worker);
+  worker.postMessage(memory);
+  ++runningWorkers;
+};
+
+let shared = `
+  function wait(memory, index, waitCondition, wakeCondition) {
+    while (memory[index] == waitCondition) {
+      var result = Atomics.wait(memory, index, waitCondition);
+      switch (result) {
+        case 'not-equal':
+        case 'ok':
+          break;
+        default:
+          postMessage('Error: bad result from wait: ' + result);
+          break;
+      }
+      var value = memory[index];
+      if (value != wakeCondition) {
+        postMessage(
+            'Error: wait returned not-equal but the memory has a bad value: ' +
+            value);
+      }
+    }
+    var value = memory[index];
+    if (value != wakeCondition) {
+      postMessage(
+          'Error: done waiting but the memory has a bad value: ' + value);
+    }
+  }
+
+  function wake(memory, index) {
+    var result = Atomics.wake(memory, index, 1);
+    if (result != 0 && result != 1) {
+      postMessage('Error: bad result from wake: ' + result);
+    }
+  }
+`;
+
+let worker1 = startWorker(shared + `
+  onmessage = function(msg) {
+    let memory = msg;
+    const didStartIdx = 0;
+    const shouldGoIdx = 1;
+    const didEndIdx = 2;
+
+    postMessage("started");
+    postMessage("memory: " + memory);
+    wait(memory, didStartIdx, 0, 1);
+    memory[shouldGoIdx] = 1;
+    wake(memory, shouldGoIdx);
+    wait(memory, didEndIdx, 0, 1);
+    postMessage("memory: " + memory);
+    postMessage("done");
+  };
+`);
+
+let worker2 = startWorker(shared + `
+  onmessage = function(msg) {
+    let memory = msg;
+    const didStartIdx = 0;
+    const shouldGoIdx = 1;
+    const didEndIdx = 2;
+
+    postMessage("started");
+    postMessage("memory: " + memory);
+    Atomics.store(memory, didStartIdx, 1);
+    wake(memory, didStartIdx);
+    wait(memory, shouldGoIdx, 0, 1);
+    Atomics.store(memory, didEndIdx, 1);
+    wake(memory, didEndIdx, 1);
+    postMessage("memory: " + memory);
+    postMessage("done");
+  };
+`);
+
+let running = true;
+while (running) {
+  for (let worker of workers) {
+    if (worker.done) continue;
+
+    let msg = worker.getMessage();
+    if (msg) {
+      switch (msg) {
+        case "done":
+          if (worker.done === false) {
+            print("worker #" + worker.idx + " done.");
+            worker.done = true;
+            if (--runningWorkers === 0) {
+              running = false;
+            }
+          }
+          break;
+
+        default:
+          print("msg from worker #" + worker.idx + ": " + msg);
+          break;
+      }
+    }
+  }
+}