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
# 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
# ----------------------------------------------------------------------------
#
# 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
comp=clang
CXX=clang++
CXXFLAGS += -pedantic -Wextra -Wshadow
+ifneq ($(KERNEL),Darwin)
+ LDFLAGS += -latomic
+endif
ifeq ($(ARCH),armv7)
ifeq ($(OS),Android)
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
@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"
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;
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;
Square castlingRookSquare[CASTLING_RIGHT_NB];
Bitboard castlingPath[CASTLING_RIGHT_NB];
uint64_t nodes;
+ uint64_t tbHits;
int gamePly;
Color sideToMove;
Thread* thisThread;
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
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))
{
{
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
{
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))
if (err != TB::ProbeState::FAIL)
{
- thisThread->tbHits++;
+ pos.increment_tbHits();
int drawScore = TB::UseRule50 ? 1 : 0;
void check_time() {
- static TimePoint lastInfoTime = now();
+ static std::atomic<TimePoint> lastInfoTime = { now() };
int elapsed = Time.elapsed();
TimePoint tick = Limits.startTime + elapsed;
uint64_t nodes = 0;
for (Thread* th : *this)
- nodes += th->rootPos.nodes_searched();
+ nodes += th->nodes;
return nodes;
}
{
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);
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;
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
"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"
send "uci\n"
expect "uciok"
+ send "setoption name Threads value $threads\n"
+
send "ucinewgame\n"
send "position startpos\n"
send "go nodes 1000\n"
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"
projects / stockfish / commitdiff