Author: sammccall Date: Tue Oct 16 02:05:13 2018 New Revision: 344595 URL: http://llvm.org/viewvc/llvm-project?rev=344595&view=rev Log: [clangd] Fix threading bugs in (not-yet-used) BackgroundIndex, re-enable test.
Summary: One relatively boring bug: forgot to notify the CV after enqueue. One much more fun bug: the thread member could access instance variables before they were initialized. Although the thread was last in the init list, QueueCV etc were listed after Thread in the class, so their default constructors raced with the thread itself. We have to get very unlucky to lose this race, I saw it 0.02% of the time. Reviewers: ioeric Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, jfb, cfe-commits Differential Revision: https://reviews.llvm.org/D53313 Modified: clang-tools-extra/trunk/clangd/index/Background.cpp clang-tools-extra/trunk/clangd/index/Background.h clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp Modified: clang-tools-extra/trunk/clangd/index/Background.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Background.cpp?rev=344595&r1=344594&r2=344595&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/index/Background.cpp (original) +++ clang-tools-extra/trunk/clangd/index/Background.cpp Tue Oct 16 02:05:13 2018 @@ -75,8 +75,11 @@ void BackgroundIndex::blockUntilIdleForT void BackgroundIndex::enqueue(StringRef Directory, tooling::CompileCommand Cmd) { - std::lock_guard<std::mutex> Lock(QueueMu); - enqueueLocked(std::move(Cmd)); + { + std::lock_guard<std::mutex> Lock(QueueMu); + enqueueLocked(std::move(Cmd)); + } + QueueCV.notify_all(); } void BackgroundIndex::enqueueAll(StringRef Directory, Modified: clang-tools-extra/trunk/clangd/index/Background.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Background.h?rev=344595&r1=344594&r2=344595&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/index/Background.h (original) +++ clang-tools-extra/trunk/clangd/index/Background.h Tue Oct 16 02:05:13 2018 @@ -65,12 +65,12 @@ private: using Task = std::function<void()>; // FIXME: use multiple worker threads. void run(); // Main loop executed by Thread. Runs tasks from Queue. void enqueueLocked(tooling::CompileCommand Cmd); - std::thread Thread; std::mutex QueueMu; unsigned NumActiveTasks = 0; // Only idle when queue is empty *and* no tasks. std::condition_variable QueueCV; bool ShouldStop = false; std::deque<Task> Queue; + std::thread Thread; // Must be last, spawned thread reads instance vars. }; } // namespace clangd Modified: clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp?rev=344595&r1=344594&r2=344595&view=diff ============================================================================== --- clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp (original) +++ clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp Tue Oct 16 02:05:13 2018 @@ -11,8 +11,6 @@ namespace clangd { MATCHER_P(Named, N, "") { return arg.Name == N; } -// Temporarily disabled: test timing out on buildbots. -#if 0 TEST(BackgroundIndexTest, IndexTwoFiles) { MockFSProvider FS; // a.h yields different symbols when included by A.cc vs B.cc. @@ -34,7 +32,6 @@ TEST(BackgroundIndexTest, IndexTwoFiles) EXPECT_THAT(runFuzzyFind(Idx, ""), UnorderedElementsAre(Named("a_h"), Named("foo"), Named("bar"))); } -#endif } // namespace clangd } // namespace clang _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
