From 8eae6a95fb3b446e2782be12aff519164e521e4d Mon Sep 17 00:00:00 2001 From: Marco Costalba Date: Sun, 14 Feb 2010 13:31:05 +0100 Subject: [PATCH] Fix 'stop' flag changed out of lock protection This is the first nice effect of previous patch ! Because thread_should_stop() should be declared 'const' we need to remove the setting of 'stop' flag to true that turns out to be a bug because thread_should_stop() is called outside from lock protection while 'stop' flag is a volatile shared variable so cannot be changed when not in lock. Note that this bugs fires ONLY when we use more then 2 threads, so commonly only in a QUAD or OCTAL machine. Signed-off-by: Marco Costalba --- src/search.cpp | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/search.cpp b/src/search.cpp index 857aaff8..8d0f758d 100644 --- a/src/search.cpp +++ b/src/search.cpp @@ -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--; @@ -2764,7 +2768,7 @@ 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); @@ -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; } -- 2.39.2