]> git.sesse.net Git - stockfish/blobdiff - src/search.cpp
Fix a possible out of range access in previous patch
[stockfish] / src / search.cpp
index e47657fdcca35d6392c0e5d311e7c738c3246c90..ed15902d4838c5800b53fc32a98ae0140e890dce 100644 (file)
@@ -54,6 +54,10 @@ namespace {
   /// Types
   enum NodeType { NonPV, PV };
 
+  // Set to true to force running with one thread.
+  // Used for debugging SMP code.
+  const bool FakeSplit = false;
+
   // ThreadsManager class is used to handle all the threads related stuff in search,
   // init, starting, parking and, the most important, launching a slave thread at a
   // split point are what this class does. All the access to shared thread data is
@@ -1349,6 +1353,7 @@ namespace {
                   alpha = value;
 
               update_pv(ss, ply);
+
               if (value == value_mate_in(ply + 1))
                   ss[ply].mateKiller = move;
           }
@@ -1362,21 +1367,9 @@ namespace {
           && TM.available_thread_exists(threadID)
           && !AbortSearch
           && !TM.thread_should_stop(threadID)
-          && TM.split<false>(pos, ss, ply, &alpha, beta, &bestValue,
-                      depth, mateThreat, &moveCount, &mp, threadID, PvNode))
-          break;
-
-      // Uncomment to debug sp_search() in single thread mode
-      /*
-      if (   bestValue < beta
-          && depth >= 4
-          && Iteration <= 99
-          && !AbortSearch
-          && !TM.thread_should_stop(threadID)
-          && TM.split<true>(pos, ss, ply, &alpha, beta, &bestValue,
-                      depth, mateThreat, &moveCount, &mp, threadID, PvNode))
+          && TM.split<FakeSplit>(pos, ss, ply, &alpha, beta, &bestValue, depth,
+                                 mateThreat, &moveCount, &mp, threadID, PvNode))
           break;
-      */
     }
 
     // Step 19. Check for mate and stalemate
@@ -1618,6 +1611,7 @@ namespace {
   void sp_search(SplitPoint* sp, int threadID) {
 
     assert(threadID >= 0 && threadID < TM.active_threads());
+    assert(TM.active_threads() > 1);
 
     StateInfo st;
     Move move;
@@ -1638,8 +1632,8 @@ namespace {
     lock_grab(&(sp->lock));
 
     while (    sp->bestValue < sp->beta
-           && !TM.thread_should_stop(threadID)
-           && (move = sp->mp->get_next_move()) != MOVE_NONE)
+           && (move = sp->mp->get_next_move()) != MOVE_NONE
+           && !TM.thread_should_stop(threadID))
     {
       moveCount = ++sp->moves;
       lock_release(&(sp->lock));
@@ -1704,7 +1698,7 @@ namespace {
           {
               Value localAlpha = sp->alpha;
               value = -search<NonPV>(pos, ss, -(localAlpha+1), -localAlpha, newDepth-ss[sp->ply].reduction, sp->ply+1, true, threadID);
-              doFullDepthSearch = (value > localAlpha && !TM.thread_should_stop(threadID));
+              doFullDepthSearch = (value > localAlpha);
           }
       }
 
@@ -1715,7 +1709,7 @@ namespace {
           Value localAlpha = sp->alpha;
           value = -search<NonPV>(pos, ss, -(localAlpha+1), -localAlpha, newDepth, sp->ply+1, true, threadID);
 
-          if (PvNode && value > localAlpha && value < sp->beta && !TM.thread_should_stop(threadID))
+          if (PvNode && value > localAlpha && value < sp->beta)
               value = -search<PV>(pos, ss, -sp->beta, -sp->alpha, newDepth, sp->ply+1, false, threadID);
       }
 
@@ -1740,9 +1734,6 @@ namespace {
                   sp->alpha = value;
 
               sp_update_pv(sp->parentSstack, ss, sp->ply);
-
-              if (PvNode && value == value_mate_in(sp->ply + 1))
-                  ss[sp->ply].mateKiller = move;
           }
       }
     }
@@ -1750,7 +1741,6 @@ namespace {
     /* Here we have the lock still grabbed */
 
     sp->slaves[threadID] = 0;
-    sp->cpus--;
 
     lock_release(&(sp->lock));
   }
@@ -1779,8 +1769,6 @@ namespace {
     }
     ss[ply].init(ply);
     ss[ply + 2].initKillers();
-
-
   }
 
   // update_pv() is called whenever a search returns a value > alpha.
@@ -2418,12 +2406,15 @@ namespace {
             threads[threadID].state = THREAD_AVAILABLE;
         }
 
-        // If this thread is the master of a split point and all threads have
+        // If this thread is the master of a split point and all slaves have
         // finished their work at this split point, return from the idle loop.
-        if (sp && sp->cpus == 0)
+        int i = 0;
+        for ( ; sp && i < ActiveThreads && !sp->slaves[i]; i++) {}
+
+        if (i == ActiveThreads)
         {
-            // Because sp->cpus is decremented under lock protection,
-            // be sure sp->lock has been released before to proceed.
+            // Because sp->slaves[] is reset under lock protection,
+            // be sure sp->lock has been released before to return.
             lock_grab(&(sp->lock));
             lock_release(&(sp->lock));
 
@@ -2605,15 +2596,13 @@ namespace {
   // data that must be copied to the helper threads (the current position and
   // search stack, alpha, beta, the search depth, etc.), and we tell our
   // helper threads that they have been assigned work. This will cause them
-  // to instantly leave their idle loops and call sp_search_pv(). When all
-  // threads have returned from sp_search_pv (or, equivalently, when
-  // splitPoint->cpus becomes 0), split() returns true.
+  // to instantly leave their idle loops and call sp_search(). When all
+  // threads have returned from sp_search() then split() returns true.
 
   template <bool Fake>
-  bool ThreadsManager::split(const Position& p, SearchStack* sstck, int ply,
-             Value* alpha, const Value beta, Value* bestValue,
-             Depth depth, bool mateThreat, int* moves, MovePicker* mp, int master, bool pvNode) {
-
+  bool ThreadsManager::split(const Position& p, SearchStack* sstck, int ply, Value* alpha,
+                             const Value beta, Value* bestValue, Depth depth, bool mateThreat,
+                             int* moves, MovePicker* mp, int master, bool pvNode) {
     assert(p.is_ok());
     assert(sstck != NULL);
     assert(ply >= 0 && ply < PLY_MAX);
@@ -2623,7 +2612,7 @@ namespace {
     assert(beta <= VALUE_INFINITE);
     assert(depth > Depth(0));
     assert(master >= 0 && master < ActiveThreads);
-    assert(Fake || ActiveThreads > 1);
+    assert(ActiveThreads > 1);
 
     SplitPoint* splitPoint;
 
@@ -2631,7 +2620,7 @@ namespace {
 
     // If no other thread is available to help us, or if we have too many
     // active split points, don't split.
-    if (   (!Fake && !available_thread_exists(master))
+    if (   !available_thread_exists(master)
         || threads[master].activeSplitPoints >= ACTIVE_SPLIT_POINTS_MAX)
     {
         lock_release(&MPLock);
@@ -2654,7 +2643,6 @@ namespace {
     splitPoint->master = master;
     splitPoint->mp = mp;
     splitPoint->moves = *moves;
-    splitPoint->cpus = 1;
     splitPoint->pos = &p;
     splitPoint->parentSstack = sstck;
     for (int i = 0; i < ActiveThreads; i++)
@@ -2666,17 +2654,19 @@ namespace {
     // If we are here it means we are not available
     assert(threads[master].state != THREAD_AVAILABLE);
 
+    int workersCnt = 1; // At least the master is included
+
     // Allocate available threads setting state to THREAD_BOOKED
-    for (int i = 0; i < ActiveThreads && splitPoint->cpus < MaxThreadsPerSplitPoint; i++)
-        if (!Fake && thread_is_available(i, master))
+    for (int i = 0; !Fake && i < ActiveThreads && workersCnt < MaxThreadsPerSplitPoint; i++)
+        if (thread_is_available(i, master))
         {
             threads[i].state = THREAD_BOOKED;
             threads[i].splitPoint = splitPoint;
             splitPoint->slaves[i] = 1;
-            splitPoint->cpus++;
+            workersCnt++;
         }
 
-    assert(Fake || splitPoint->cpus > 1);
+    assert(Fake || workersCnt > 1);
 
     // We can release the lock because slave threads are already booked and master is not available
     lock_release(&MPLock);
@@ -2697,8 +2687,7 @@ namespace {
     // which it will instantly launch a search, because its state is
     // THREAD_WORKISWAITING.  We send the split point as a second parameter to the
     // idle loop, which means that the main thread will return from the idle
-    // loop when all threads have finished their work at this split point
-    // (i.e. when splitPoint->cpus == 0).
+    // loop when all threads have finished their work at this split point.
     idle_loop(master, splitPoint);
 
     // We have returned from the idle loop, which means that all threads are