Fix four data races.

the nodes, tbHits, rootDepth and lastInfoTime variables are read by multiple threads, but not declared atomic, leading to data races as found by -fsanitize=thread. This patch fixes this issue. It is based on top of the CI-threading branch (PR #1129), and should fix the corresponding CI error messages.

The patch passed an STC check for no regression:

http://tests.stockfishchess.org/tests/view/5925d5590ebc59035df34b9f
LLR: 2.96 (-2.94,2.94) [-3.00,1.00]
Total: 169597 W: 29938 L: 30066 D: 109593

Whereas rootDepth and lastInfoTime are not performance critical, nodes and tbHits are. Indeed, an earlier version using relaxed atomic updates on the latter two variables failed STC testing (http://tests.stockfishchess.org/tests/view/592001700ebc59035df34924), which can be shown to be due to x86-32 (http://tests.stockfishchess.org/tests/view/592330ac0ebc59035df34a89). Indeed, the latter have no instruction to atomically update a 64bit variable. The proposed solution thus uses a variable in Position that is accessed only by one thread, which is copied every few thousand nodes to the shared variable in Thread.

No functional change.

Closes #1130
Closes #1129

.travis.yml

@@ -72,4 +72,5 @@ script:
   # sanitizer
   #
   # use g++-6 as a proxy for having sanitizers, might need revision as they become available for more recent versions of clang/gcc
-  - if [[ "$COMPILER" == "g++-6" ]]; then make clean && make ARCH=x86-64 sanitize=yes build > /dev/null && ../tests/instrumented.sh --sanitizer; fi
+  - if [[ "$COMPILER" == "g++-6" ]]; then make clean && make ARCH=x86-64 sanitize=undefined optimize=no debug=yes build > /dev/null && ../tests/instrumented.sh --sanitizer-undefined; fi
+  - if [[ "$COMPILER" == "g++-6" ]]; then make clean && make ARCH=x86-64 sanitize=thread optimize=no debug=yes build > /dev/null && ../tests/instrumented.sh --sanitizer-thread; fi

src/Makefile

@@ -50,7 +50,9 @@ OBJS = benchmark.o bitbase.o bitboard.o endgame.o evaluate.o main.o \
 # ----------------------------------------------------------------------------
 #
 # debug = yes/no      --- -DNDEBUG         --- Enable/Disable debug mode
-# sanitize = yes/no   --- (-fsanitize )    --- enable undefined behavior checks
+# sanitize = undefined/thread/no (-fsanitize )
+#                     --- ( undefined )    --- enable undefined behavior checks
+#                     --- ( thread    )    --- enable threading error  checks
 # optimize = yes/no   --- (-O3/-fast etc.) --- Enable/Disable optimizations
 # arch = (name)       --- (-arch)          --- Target architecture
 # bits = 64/32        --- -DIS_64BIT       --- 64-/32-bit operating system
@@ -202,6 +204,9 @@ ifeq ($(COMP),clang)
 	comp=clang
 	CXX=clang++
 	CXXFLAGS += -pedantic -Wextra -Wshadow
+ifneq ($(KERNEL),Darwin)
+	LDFLAGS += -latomic
+endif
 
 	ifeq ($(ARCH),armv7)
 		ifeq ($(OS),Android)
@@ -261,9 +266,9 @@ else
 endif
 
 ### 3.2.2 Debugging with undefined behavior sanitizers
-ifeq ($(sanitize),yes)
-        CXXFLAGS += -g3 -fsanitize=undefined
-        LDFLAGS += -fsanitize=undefined
+ifneq ($(sanitize),no)
+        CXXFLAGS += -g3 -fsanitize=$(sanitize) -fuse-ld=gold
+        LDFLAGS += -fsanitize=$(sanitize) -fuse-ld=gold
 endif
 
 ### 3.3 Optimization
@@ -496,7 +501,7 @@ config-sanity:
 	@echo "Testing config sanity. If this fails, try 'make help' ..."
 	@echo ""
 	@test "$(debug)" = "yes" || test "$(debug)" = "no"
-	@test "$(sanitize)" = "yes" || test "$(sanitize)" = "no"
+	@test "$(sanitize)" = "undefined" || test "$(sanitize)" = "thread" || test "$(sanitize)" = "no"
 	@test "$(optimize)" = "yes" || test "$(optimize)" = "no"
 	@test "$(arch)" = "any" || test "$(arch)" = "x86_64" || test "$(arch)" = "i386" || \
 	 test "$(arch)" = "ppc64" || test "$(arch)" = "ppc" || test "$(arch)" = "armv7"

src/position.h

@@ -134,6 +134,8 @@ public:
   void undo_move(Move m);
   void do_null_move(StateInfo& newSt);
   void undo_null_move();
+  void increment_nodes();
+  void increment_tbHits();
 
   // Static Exchange Evaluation
   bool see_ge(Move m, Value value = VALUE_ZERO) const;
@@ -151,6 +153,7 @@ public:
   bool is_chess960() const;
   Thread* this_thread() const;
   uint64_t nodes_searched() const;
+  uint64_t tb_hits() const;
   bool is_draw(int ply) const;
   int rule50_count() const;
   Score psq_score() const;
@@ -185,6 +188,7 @@ private:
   Square castlingRookSquare[CASTLING_RIGHT_NB];
   Bitboard castlingPath[CASTLING_RIGHT_NB];
   uint64_t nodes;
+  uint64_t tbHits;
   int gamePly;
   Color sideToMove;
   Thread* thisThread;
@@ -353,6 +357,18 @@ inline uint64_t Position::nodes_searched() const {
   return nodes;
 }
 
+inline void Position::increment_nodes() {
+  nodes++;
+}
+
+inline uint64_t Position::tb_hits() const {
+  return tbHits;
+}
+
+inline void Position::increment_tbHits() {
+  tbHits++;
+}
+
 inline bool Position::opposite_bishops() const {
   return   pieceCount[W_BISHOP] == 1
         && pieceCount[B_BISHOP] == 1

src/search.cpp

@@ -361,7 +361,7 @@ void Thread::search() {
   multiPV = std::min(multiPV, rootMoves.size());
 
   // Iterative deepening loop until requested to stop or the target depth is reached
-  while (   (rootDepth += ONE_PLY) < DEPTH_MAX
+  while (   (rootDepth = rootDepth + ONE_PLY) < DEPTH_MAX
          && !Signals.stop
          && (!Limits.depth || Threads.main()->rootDepth / ONE_PLY <= Limits.depth))
   {
@@ -400,6 +400,9 @@ void Thread::search() {
           {
               bestValue = ::search<PV>(rootPos, ss, alpha, beta, rootDepth, false, false);
 
+              this->tbHits = rootPos.tb_hits();
+              this->nodes = rootPos.nodes_searched();
+
               // Bring the best move to the front. It is critical that sorting
               // is done with a stable algorithm because all the values but the
               // first and eventually the new best one are set to -VALUE_INFINITE
@@ -568,6 +571,9 @@ namespace {
     {
         thisThread->resetCalls = false;
 
+        thisThread->tbHits = pos.tb_hits();
+        thisThread->nodes = pos.nodes_searched();
+
         // At low node count increase the checking rate to about 0.1% of nodes
         // otherwise use a default value.
         thisThread->callsCnt = Limits.nodes ? std::min(4096, int(Limits.nodes / 1024))
@@ -668,7 +674,7 @@ namespace {
 
             if (err != TB::ProbeState::FAIL)
             {
-                thisThread->tbHits++;
+                pos.increment_tbHits();
 
                 int drawScore = TB::UseRule50 ? 1 : 0;
 
@@ -1473,7 +1479,7 @@ moves_loop: // When in check search starts from here
 
   void check_time() {
 
-    static TimePoint lastInfoTime = now();
+    static std::atomic<TimePoint> lastInfoTime = { now() };
 
     int elapsed = Time.elapsed();
     TimePoint tick = Limits.startTime + elapsed;

src/thread.cpp

@@ -163,7 +163,7 @@ uint64_t ThreadPool::nodes_searched() const {
 
   uint64_t nodes = 0;
   for (Thread* th : *this)
-      nodes += th->rootPos.nodes_searched();
+      nodes += th->nodes;
   return nodes;
 }
 
@@ -212,6 +212,7 @@ void ThreadPool::start_thinking(Position& pos, StateListPtr& states,
   {
       th->maxPly = 0;
       th->tbHits = 0;
+      th->nodes = 0;
       th->rootDepth = DEPTH_ZERO;
       th->rootMoves = rootMoves;
       th->rootPos.set(pos.fen(), pos.is_chess960(), &setupStates->back(), th);

src/thread.h

@@ -61,11 +61,12 @@ public:
   Endgames endgames;
   size_t idx, PVIdx;
   int maxPly, callsCnt;
-  uint64_t tbHits;
+  std::atomic<uint64_t> tbHits;
+  std::atomic<uint64_t> nodes;
 
   Position rootPos;
   Search::RootMoves rootMoves;
-  Depth rootDepth;
+  std::atomic<Depth> rootDepth;
   Depth completedDepth;
   std::atomic_bool resetCalls;
   CounterMoveStat counterMoves;

tests/instrumented.sh

@@ -15,18 +15,45 @@ case $1 in
     prefix=''
     exeprefix='valgrind --error-exitcode=42'
     postfix='1>/dev/null'
+    threads="1"
   ;;
-  --sanitizer)
+  --sanitizer-undefined)
     echo "sanitizer testing started"
     prefix='!'
     exeprefix=''
     postfix='2>&1 | grep "runtime error:"'
+    threads="1"
+  ;;
+  --sanitizer-thread)
+    echo "sanitizer testing started"
+    prefix='!'
+    exeprefix=''
+    postfix='2>&1 | grep "WARNING: ThreadSanitizer:"'
+    threads="2"
+
+cat << EOF > tsan.supp
+race:TTEntry::move
+race:TTEntry::depth
+race:TTEntry::bound
+race:TTEntry::save
+race:TTEntry::value
+race:TTEntry::eval
+
+race:TranspositionTable::probe
+race:TranspositionTable::generation
+race:TranspositionTable::hashfull
+
+EOF
+
+    export TSAN_OPTIONS="suppressions=./tsan.supp"
+
   ;;
   *)
     echo "unknown testing started"
     prefix=''
     exeprefix=''
     postfix=''
+    threads="1"
   ;;
 esac
 
@@ -36,7 +63,7 @@ for args in "eval" \
             "go depth 10" \
             "go movetime 1000" \
             "go wtime 8000 btime 8000 winc 500 binc 500" \
-            "bench 128 1 10 default depth"
+            "bench 128 $threads 10 default depth"
 do
 
    echo "$prefix $exeprefix ./stockfish $args $postfix"
@@ -52,6 +79,8 @@ cat << EOF > game.exp
  send "uci\n"
  expect "uciok"
 
+ send "setoption name Threads value $threads\n"
+
  send "ucinewgame\n"
  send "position startpos\n"
  send "go nodes 1000\n"
@@ -76,11 +105,13 @@ EOF
 for exps in game.exp
 do
 
-  echo "$prefix expect game.exp $postfix"
-  eval "$prefix expect game.exp $postfix"
+  echo "$prefix expect $exps $postfix"
+  eval "$prefix expect $exps $postfix"
+
+  rm $exps
 
 done
 
-rm game.exp
+rm -f tsan.supp
 
 echo "instrumented testing OK"