From 3cb02004596f868ae405b09fbf3a2038a680a989 Mon Sep 17 00:00:00 2001 From: Joost VandeVondele Date: Wed, 21 Jun 2017 13:36:53 -0700 Subject: [PATCH] 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 | 3 ++- src/Makefile | 15 ++++++++++----- src/position.h | 16 ++++++++++++++++ src/search.cpp | 12 +++++++++--- src/thread.cpp | 3 ++- src/thread.h | 5 +++-- tests/instrumented.sh | 41 ++++++++++++++++++++++++++++++++++++----- 7 files changed, 78 insertions(+), 17 deletions(-) diff --git a/.travis.yml b/.travis.yml index 4310b1bd..ba20fdc8 100644 --- a/.travis.yml +++ b/.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 diff --git a/src/Makefile b/src/Makefile index fd928384..6eb96461 100644 --- a/src/Makefile +++ b/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" diff --git a/src/position.h b/src/position.h index 2b44ef32..25e57512 100644 --- a/src/position.h +++ b/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 diff --git a/src/search.cpp b/src/search.cpp index d59edbae..5447ed3c 100644 --- a/src/search.cpp +++ b/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(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 lastInfoTime = { now() }; int elapsed = Time.elapsed(); TimePoint tick = Limits.startTime + elapsed; diff --git a/src/thread.cpp b/src/thread.cpp index a5523fc1..c80b9bd1 100644 --- a/src/thread.cpp +++ b/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); diff --git a/src/thread.h b/src/thread.h index 7666a34f..0f117c2d 100644 --- a/src/thread.h +++ b/src/thread.h @@ -61,11 +61,12 @@ public: Endgames endgames; size_t idx, PVIdx; int maxPly, callsCnt; - uint64_t tbHits; + std::atomic tbHits; + std::atomic nodes; Position rootPos; Search::RootMoves rootMoves; - Depth rootDepth; + std::atomic rootDepth; Depth completedDepth; std::atomic_bool resetCalls; CounterMoveStat counterMoves; diff --git a/tests/instrumented.sh b/tests/instrumented.sh index 4c688151..838ed8f4 100755 --- a/tests/instrumented.sh +++ b/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" -- 2.39.2