Fix a race on Limits::ponder

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:

  Previous write of size 4 at 0x0000005c2260 by main thread:

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

The reason of teh race is that ponder is not just set in UCI go()
assignment but also is signaled by an async ponderhit in uci.cpp:

      else if (token == "ponderhit")
          Search::Limits.ponder = 0; // Switch to normal search

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.

diff --git a/src/search.cpp b/src/search.cpp
index 11dcdb34f13840a1b8ff43a7b08df4273fe972f9..3c638ca057f5e1c3c515498e005cecf88a111c8e 100644 (file)
--- a/src/search.cpp
+++ b/src/search.cpp
@@ -277,7 +277,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).
   // 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);
   {
       Threads.stopOnPonderhit = true;
       wait(Threads.stop);


@@ -499,7 +499,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 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;
                       Threads.stopOnPonderhit = true;
                   else
                       Threads.stop = true;


@@ -1489,7 +1489,7 @@ moves_loop: // When in check search starts from here
     }
 
     // An engine may not stop pondering until told so by the GUI
     }
 
     // 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)
         return;
 
     if (   (Limits.use_time_management() && elapsed > Time.maximum() - 10)

diff --git a/src/search.h b/src/search.h
index 477570e3919e3d40c7d0092e44a61630a4357e61..bbdbdfd950e9409483ccdbf0056162f741d7c54c 100644 (file)
--- a/src/search.h
+++ b/src/search.h
@@ -72,14 +72,13 @@ typedef std::vector<RootMove> RootMoves;
 
 
 /// LimitsType struct stores information sent by GUI about available time to
 
 
 /// LimitsType struct stores information sent by GUI about available time to

-/// search the current move, maximum depth/time, if we are in analysis mode or
-/// if we have to ponder while it's our opponent's turn to move.
+/// search the current move, maximum depth/time, or if we are in analysis mode.

 
 struct LimitsType {
 
   LimitsType() { // Init explicitly due to broken value-initialization of non POD in MSVC
     nodes = time[WHITE] = time[BLACK] = inc[WHITE] = inc[BLACK] =
 
 struct LimitsType {
 
   LimitsType() { // Init explicitly due to broken value-initialization of non POD in MSVC
     nodes = time[WHITE] = time[BLACK] = inc[WHITE] = inc[BLACK] =

-    npmsec = movestogo = depth = movetime = mate = infinite = ponder = 0;
+    npmsec = movestogo = depth = movetime = mate = infinite = 0;

   }
 
   bool use_time_management() const {
   }
 
   bool use_time_management() const {


@@ -87,7 +86,7 @@ struct LimitsType {
   }
 
   std::vector<Move> searchmoves;
   }
 
   std::vector<Move> searchmoves;

-  int time[COLOR_NB], inc[COLOR_NB], npmsec, movestogo, depth, movetime, mate, infinite, ponder;
+  int time[COLOR_NB], inc[COLOR_NB], npmsec, movestogo, depth, movetime, mate, infinite;

   int64_t nodes;
   TimePoint startTime;
 };
   int64_t nodes;
   TimePoint startTime;
 };

diff --git a/src/thread.cpp b/src/thread.cpp
index 86fce6aa0c20020b6c54666c874a5231793d6c9e..0b851d6a6e0b5f0f48d5535aea3a06cd5c4d909d 100644 (file)
--- a/src/thread.cpp
+++ b/src/thread.cpp
@@ -187,7 +187,7 @@ void ThreadPool::start_thinking(Position& pos, StateListPtr& states,
 
   main()->wait_for_search_finished();
 
 
   main()->wait_for_search_finished();
 

-  stopOnPonderhit = stop = false;
+  ponder = stopOnPonderhit = stop = false;

   Search::Limits = limits;
   Search::RootMoves rootMoves;
 
   Search::Limits = limits;
   Search::RootMoves rootMoves;
 

diff --git a/src/thread.h b/src/thread.h
index c1b635b7ea4db61dfca33ed0ee3423e3eeefb862..3512605c67aaa8120a0cfbea30012ca969894a41 100644 (file)
--- a/src/thread.h
+++ b/src/thread.h
@@ -101,7 +101,7 @@ struct ThreadPool : public std::vector<Thread*> {
   uint64_t nodes_searched() const;
   uint64_t tb_hits() const;
 
   uint64_t nodes_searched() const;
   uint64_t tb_hits() const;
 

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

 
 private:
   StateListPtr setupStates;
 
 private:
   StateListPtr setupStates;

diff --git a/src/uci.cpp b/src/uci.cpp
index 8f6416bdcb7b866b984e53b5ac6be64c4a7e47b8..758a648cc0eb6f25127cee983a5c65468fe5e882 100644 (file)
--- a/src/uci.cpp
+++ b/src/uci.cpp
@@ -133,7 +133,7 @@ namespace {
         else if (token == "movetime")  is >> limits.movetime;
         else if (token == "mate")      is >> limits.mate;
         else if (token == "infinite")  limits.infinite = 1;
         else if (token == "movetime")  is >> limits.movetime;
         else if (token == "mate")      is >> limits.mate;
         else if (token == "infinite")  limits.infinite = 1;

-        else if (token == "ponder")    limits.ponder = 1;
+        else if (token == "ponder")    Threads.ponder = true;

 
     Threads.start_thinking(pos, States, limits);
   }
 
     Threads.start_thinking(pos, States, limits);
   }


@@ -180,7 +180,7 @@ void UCI::loop(int argc, char* argv[]) {
           Threads.main()->start_searching(true); // Could be sleeping
       }
       else if (token == "ponderhit")
           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)
 
       else if (token == "uci")
           sync_cout << "id name " << engine_info(true)