[llvm] r265618 - Fix a race condition in support library ThreadPool.

[Jason Henline via llvm-commits]

This issue would have been caught by running unittests/Support/SupportTests
under tsan, so I think it would be good to add a tsan bot to check at least
that target.

On Fri, Apr 15, 2016 at 5:29 PM Kostya Serebryany via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> ouch!
> Will it make sense to add a tsan bot to the sanitizer bot where we
> currently run asan, ubsan, and msan, but not tsan?
> If yes, which tests to run? (if not all of "check-llvm")?
>
> --kcc
>
> On Wed, Apr 6, 2016 at 4:46 PM, Justin Lebar via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> Author: jlebar
>> Date: Wed Apr  6 18:46:40 2016
>> New Revision: 265618
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=265618&view=rev
>> Log:
>> Fix a race condition in support library ThreadPool.
>>
>> By running TSAN on the ThreadPool unit tests it was discovered that the
>> threads in the pool can pop tasks off the queue at the same time the
>> "wait" routine is trying to check if the task queue is empty. This patch
>> fixes this problem by checking for active threads in the waiter before
>> checking whether the queue is empty.
>>
>> Patch by Jason Henline.
>>
>> Differential Revision: http://reviews.llvm.org/D18811
>>
>> Reviewers: joker.eph, jlebar
>>
>> Modified:
>>     llvm/trunk/lib/Support/ThreadPool.cpp
>>
>> Modified: llvm/trunk/lib/Support/ThreadPool.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/ThreadPool.cpp?rev=265618&r1=265617&r2=265618&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/lib/Support/ThreadPool.cpp (original)
>> +++ llvm/trunk/lib/Support/ThreadPool.cpp Wed Apr  6 18:46:40 2016
>> @@ -75,8 +75,11 @@ ThreadPool::ThreadPool(unsigned ThreadCo
>>  void ThreadPool::wait() {
>>    // Wait for all threads to complete and the queue to be empty
>>    std::unique_lock<std::mutex> LockGuard(CompletionLock);
>> +  // The order of the checks for ActiveThreads and Tasks.empty() matters
>> because
>> +  // any active threads might be modifying the Tasks queue, and this
>> would be a
>> +  // race.
>>    CompletionCondition.wait(LockGuard,
>> -                           [&] { return Tasks.empty() && !ActiveThreads;
>> });
>> +                           [&] { return !ActiveThreads && Tasks.empty();
>> });
>>  }
>>
>>  std::shared_future<ThreadPool::VoidTy> ThreadPool::asyncImpl(TaskTy
>> Task) {
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160418/9239659e/attachment.html>