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
