ilya-biryukov updated this revision to Diff 115985. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment.
- Added a comment to version map. - Fixed compilation after rebase. https://reviews.llvm.org/D38032 Files: clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/DraftStore.h unittests/clangd/ClangdTests.cpp
Index: unittests/clangd/ClangdTests.cpp =================================================================== --- unittests/clangd/ClangdTests.cpp +++ unittests/clangd/ClangdTests.cpp @@ -22,6 +22,7 @@ #include <iostream> #include <random> #include <string> +#include <thread> #include <vector> namespace clang { @@ -898,5 +899,67 @@ } } +TEST_F(ClangdThreadingTest, NoConcurrentDiagnostics) { + class NoConcurrentAccessDiagConsumer : public DiagnosticsConsumer { + public: + NoConcurrentAccessDiagConsumer(std::promise<void> StartSecondReparse) + : StartSecondReparse(std::move(StartSecondReparse)) {} + + void onDiagnosticsReady( + PathRef File, + Tagged<std::vector<DiagWithFixIts>> Diagnostics) override { + + std::unique_lock<std::mutex> Lock(Mutex, std::try_to_lock_t()); + ASSERT_TRUE(Lock.owns_lock()) + << "Detected concurrent onDiagnosticsReady calls for the same file."; + if (FirstRequest) { + FirstRequest = false; + StartSecondReparse.set_value(); + // Sleep long enough for the second request to be processed. + std::this_thread::sleep_for(std::chrono::milliseconds(50)); + } + } + + private: + std::mutex Mutex; + bool FirstRequest = true; + std::promise<void> StartSecondReparse; + }; + + const auto SourceContentsWithoutErrors = R"cpp( +int a; +int b; +int c; +int d; +)cpp"; + + const auto SourceContentsWithErrors = R"cpp( +int a = x; +int b; +int c; +int d; +)cpp"; + + auto FooCpp = getVirtualTestFilePath("foo.cpp"); + llvm::StringMap<std::string> FileContents; + FileContents[FooCpp] = ""; + ConstantFSProvider FS(buildTestFS(FileContents)); + + std::promise<void> StartSecondReparsePromise; + std::future<void> StartSecondReparse = StartSecondReparsePromise.get_future(); + + NoConcurrentAccessDiagConsumer DiagConsumer( + std::move(StartSecondReparsePromise)); + + MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true); + ClangdServer Server(CDB, DiagConsumer, FS, 4, /*SnippetCompletions=*/false, + EmptyLogger::getInstance()); + Server.addDocument(FooCpp, SourceContentsWithErrors); + StartSecondReparse.wait(); + + auto Future = Server.addDocument(FooCpp, SourceContentsWithoutErrors); + Future.wait(); +} + } // namespace clangd } // namespace clang Index: clangd/DraftStore.h =================================================================== --- clangd/DraftStore.h +++ clangd/DraftStore.h @@ -13,15 +13,16 @@ #include "Path.h" #include "clang/Basic/LLVM.h" #include "llvm/ADT/StringMap.h" +#include <cstdint> #include <mutex> #include <string> #include <vector> namespace clang { namespace clangd { -/// Using 'unsigned' here to avoid undefined behaviour on overflow. -typedef unsigned DocVersion; +/// Using unsigned int type here to avoid undefined behaviour on overflow. +typedef uint64_t DocVersion; /// Document draft with a version of this draft. struct VersionedDraft { Index: clangd/ClangdServer.h =================================================================== --- clangd/ClangdServer.h +++ clangd/ClangdServer.h @@ -284,6 +284,13 @@ // ClangdServer ClangdScheduler WorkScheduler; bool SnippetCompletions; + + /// Used to serialize diagnostic callbacks. + /// FIXME(ibiryukov): get rid of an extra map and put all version counters + /// into CppFile. + std::mutex DiagnosticsMutex; + /// Maps from a filename to the latest version of reported diagnostics. + llvm::StringMap<DocVersion> ReportedDiagnosticVersions; }; } // namespace clangd Index: clangd/ClangdServer.cpp =================================================================== --- clangd/ClangdServer.cpp +++ clangd/ClangdServer.cpp @@ -315,6 +315,19 @@ auto Diags = DeferredRebuild.get(); if (!Diags) return; // A new reparse was requested before this one completed. + + // We need to serialize access to resulting diagnostics to avoid calling + // `onDiagnosticsReady` in the wrong order. + std::lock_guard<std::mutex> DiagsLock(DiagnosticsMutex); + DocVersion &LastReportedDiagsVersion = ReportedDiagnosticVersions[FileStr]; + // FIXME(ibiryukov): get rid of '<' comparison here. In the current + // implementation diagnostics will not be reported after version counters' + // overflow. This should not happen in practice, since DocVersion is a + // 64-bit unsigned integer. + if (Version < LastReportedDiagsVersion) + return; + LastReportedDiagsVersion = Version; + DiagConsumer.onDiagnosticsReady(FileStr, make_tagged(std::move(*Diags), Tag)); };
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits