Skip to content

Commit

Permalink
avoid a race between uci and search threads (II)
Browse files Browse the repository at this point in the history
limits.ponder was used as a signal between uci and search threads, but is not an atomic variable,
leading to the following race as flagged by a sanitized binary.

Expect input:
```
 spawn  ./stockfish
 send "uci\n"
 expect "uciok"
 send "setoption name Ponder value true\n"
 send "go wtime 4000 btime 4000\n"
 expect "bestmove"
 send "position startpos e2e4 d7d5\n"
 send "go wtime 4000 btime 4000 ponder\n"
 sleep 0.01
 send "ponderhit\n"
 expect "bestmove"
 send "quit\n"
 expect eof
```

Race:
```
WARNING: ThreadSanitizer: data race (pid=7191)
  Read of size 4 at 0x0000005c2260 by thread T1:
    #0 MainThread::check_time() src/search.cpp:1488 (stockfish+0x000000454471)
    #1 search<(<unnamed>::NodeType)0u> src/search.cpp:566 (stockfish+0x0000004594e0)
    #2 search<(<unnamed>::NodeType)0u> src/search.cpp:997 (stockfish+0x00000045bfb6)
    #3 search<(<unnamed>::NodeType)0u> src/search.cpp:1006 (stockfish+0x00000045c1a3)
    #4 search<(<unnamed>::NodeType)1u> src/search.cpp:997 (stockfish+0x000000457658)
    #5 Thread::search() src/search.cpp:402 (stockfish+0x000000452e28)
    #6 MainThread::search() src/search.cpp:264 (stockfish+0x000000451c32)
    #7 Thread::idle_loop() src/thread.cpp:114 (stockfish+0x000000468c90)

  Previous write of size 4 at 0x0000005c2260 by main thread:
    #0 UCI::loop(int, char**) src/uci.cpp:193 (stockfish+0x000000473c9b)
    #1 main src/main.cpp:47 (stockfish+0x000000433322)

  Location is global 'Search::Limits' of size 88 at 0x0000005c2220 (stockfish+0x0000005c2260)
```

The fix is to add an atomic bool to the threads structure to signal the ponder status, letting Search::Limits to reflect just what was passed to 'go'.

No functional change.
  • Loading branch information
vondele committed Aug 2, 2017
1 parent 53c2d9d commit c7cd9e6
Show file tree
Hide file tree
Showing 4 changed files with 6 additions and 5 deletions.
6 changes: 3 additions & 3 deletions src/search.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ void MainThread::search() {
// the UCI protocol states that we shouldn't print the best move before the
// GUI sends a "stop" or "ponderhit" command. We therefore simply wait here
// until the GUI sends one of those commands (which also raises Threads.stop).
if (!Threads.stop && (Limits.ponder || Limits.infinite))
if (!Threads.stop && (Threads.ponder || Limits.infinite))
{
Threads.stopOnPonderhit = true;
wait(Threads.stop);
Expand Down Expand Up @@ -496,7 +496,7 @@ void Thread::search() {
{
// If we are allowed to ponder do not stop the search now but
// keep pondering until the GUI sends "ponderhit" or "stop".
if (Limits.ponder)
if (Threads.ponder)
Threads.stopOnPonderhit = true;
else
Threads.stop = true;
Expand Down Expand Up @@ -1485,7 +1485,7 @@ namespace {
}

// An engine may not stop pondering until told so by the GUI
if (Limits.ponder)
if (Threads.ponder)
return;

if ( (Limits.use_time_management() && elapsed > Time.maximum() - 10)
Expand Down
1 change: 1 addition & 0 deletions src/thread.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ void ThreadPool::start_thinking(Position& pos, StateListPtr& states,
main()->wait_for_search_finished();

stopOnPonderhit = stop = false;
ponder = limits.ponder;
Search::Limits = limits;
Search::RootMoves rootMoves;

Expand Down
2 changes: 1 addition & 1 deletion src/thread.h
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ struct ThreadPool : public std::vector<Thread*> {
uint64_t nodes_searched() const;
uint64_t tb_hits() const;

std::atomic_bool stop, stopOnPonderhit;
std::atomic_bool stop, ponder, stopOnPonderhit;

private:
StateListPtr setupStates;
Expand Down
2 changes: 1 addition & 1 deletion src/uci.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ void UCI::loop(int argc, char* argv[]) {
Threads.main()->start_searching(true); // Could be sleeping
}
else if (token == "ponderhit")
Search::Limits.ponder = 0; // Switch to normal search
Threads.ponder = false; // Switch to normal search

else if (token == "uci")
sync_cout << "id name " << engine_info(true)
Expand Down

0 comments on commit c7cd9e6

Please sign in to comment.