Fix another setting of a flag out of lock protection

[stockfish] / src / search.cpp

diff --git a/src/search.cpp b/src/search.cpp
index 857aaff87ea9fd1d0812fcd2519f560ab1adf4fd..dece7fd4958217523da01dbdae0dc0e5196fc971 100644 (file)
--- a/src/search.cpp
+++ b/src/search.cpp
@@ -70,7 +70,7 @@ namespace {
 
     int active_threads() const { return ActiveThreads; }
     void set_active_threads(int newActiveThreads) { ActiveThreads = newActiveThreads; }
-    void set_stop_request(int threadID) { threads[threadID].stop = true; }
+    void set_stop_request(int threadID) { threads[threadID].stopRequest = true; }
     void incrementNodeCounter(int threadID) { threads[threadID].nodes++; }
     void incrementBetaCounter(Color us, Depth d, int threadID) { threads[threadID].betaCutOffs[us] += unsigned(d); }
     void print_current_line(SearchStack ss[], int ply, int threadID);
@@ -81,7 +81,7 @@ namespace {
     void get_beta_counters(Color us, int64_t& our, int64_t& their) const;
     bool idle_thread_exists(int master) const;
     bool thread_is_available(int slave, int master) const;
-    bool thread_should_stop(int threadID);
+    bool thread_should_stop(int threadID) const;
     void wake_sleeping_threads();
     void put_threads_to_sleep();
     void idle_loop(int threadID, SplitPoint* waitSp);
@@ -1881,10 +1881,12 @@ namespace {
     /* Here we have the lock still grabbed */
 
     // If this is the master thread and we have been asked to stop because of
-    // a beta cutoff higher up in the tree, stop all slave threads.
+    // a beta cutoff higher up in the tree, stop all slave threads. Note that
+    // thread_should_stop(threadID) does not imply that 'stop' flag is set, so
+    // do this explicitly now, under lock protection.
     if (sp->master == threadID && TM.thread_should_stop(threadID))
         for (int i = 0; i < TM.active_threads(); i++)
-            if (sp->slaves[i])
+            if (sp->slaves[i] || i == threadID)
                 TM.set_stop_request(i);
 
     sp->cpus--;
@@ -2016,10 +2018,12 @@ namespace {
     /* Here we have the lock still grabbed */
 
     // If this is the master thread and we have been asked to stop because of
-    // a beta cutoff higher up in the tree, stop all slave threads.
+    // a beta cutoff higher up in the tree, stop all slave threads. Note that
+    // thread_should_stop(threadID) does not imply that 'stop' flag is set, so
+    // do this explicitly now, under lock protection.
     if (sp->master == threadID && TM.thread_should_stop(threadID))
         for (int i = 0; i < TM.active_threads(); i++)
-            if (sp->slaves[i])
+            if (sp->slaves[i] || i == threadID)
                 TM.set_stop_request(i);
 
     sp->cpus--;
@@ -2748,7 +2752,7 @@ namespace {
     AllThreadsShouldExit = true;
     for (int i = 1; i < THREAD_MAX; i++)
     {
-        threads[i].stop = true;
+        threads[i].stopRequest = true;
         while (threads[i].running);
     }
 
@@ -2764,13 +2768,13 @@ namespace {
   // cutoff has occurred in the thread's currently active split point, or in
   // some ancestor of the current split point.
 
-  bool ThreadsManager::thread_should_stop(int threadID) {
+  bool ThreadsManager::thread_should_stop(int threadID) const {
 
     assert(threadID >= 0 && threadID < ActiveThreads);
 
     SplitPoint* sp;
 
-    if (threads[threadID].stop)
+    if (threads[threadID].stopRequest)
         return true;
 
     if (ActiveThreads <= 2)
@@ -2778,10 +2782,7 @@ namespace {
 
     for (sp = threads[threadID].splitPoint; sp != NULL; sp = sp->parent)
         if (sp->finished)
-        {
-            threads[threadID].stop = true;
             return true;
-        }
 
     return false;
   }
@@ -2903,16 +2904,22 @@ namespace {
     for (int i = 0; i < ActiveThreads; i++)
         splitPoint->slaves[i] = 0;
 
-    threads[master].idle = false;
-    threads[master].stop = false;
     threads[master].splitPoint = splitPoint;
 
+    // If we are here it means we are not idle
+    assert(!threads[master].idle);
+
+    // Following assert could fail because we could be slave of a master
+    // thread that has just raised a stop request. Note that stopRequest
+    // can be changed with only splitPoint::lock held, not with MPLock.
+    /* assert(!threads[master].stopRequest); */
+
     // Allocate available threads setting idle flag to false
     for (int i = 0; i < ActiveThreads && splitPoint->cpus < MaxThreadsPerSplitPoint; i++)
         if (thread_is_available(i, master))
         {
             threads[i].idle = false;
-            threads[i].stop = false;
+            threads[i].stopRequest = false;
             threads[i].splitPoint = splitPoint;
             splitPoint->slaves[i] = 1;
             splitPoint->cpus++;
@@ -2949,7 +2956,7 @@ namespace {
 
     *beta = splitPoint->beta;
     *bestValue = splitPoint->bestValue;
-    threads[master].stop = false;
+    threads[master].stopRequest = false;
     threads[master].idle = false;
     threads[master].activeSplitPoints--;
     threads[master].splitPoint = splitPoint->parent;
@@ -3005,11 +3012,20 @@ namespace {
 
     AllThreadsShouldSleep = true;
 
-    // Wait for the threads to be all sleeping
+    // Wait for the threads to be all sleeping and reset flags
+    // to a known state.
     for (int i = 1; i < ActiveThreads; i++)
+    {
         while (!threads[i].sleeping);
-  }
 
+        assert(threads[i].idle);
+        assert(threads[i].running);
+        assert(!threads[i].workIsWaiting);
+
+        // These two flags can be in a random state
+        threads[i].stopRequest = threads[i].printCurrentLineRequest = false;
+    }
+  }
 
   // print_current_line() prints _once_ the current line of search for a
   // given thread and then setup the print request for the next thread.