[clang-tools-extra] r313754 - [clangd] Serialize onDiagnosticsReady callbacks for the same file.
Alex L via cfe-commits
cfe-commits at lists.llvm.org
Wed Sep 20 11:22:16 PDT 2017
This commit causes the formatting.test to fail on macOS with an uncaught
exception:
libc++abi.dylib: terminating with uncaught exception of type
std::__1::system_error: mutex lock failed: Invalid argument
http://lab.llvm.org:8080/green/job/clang-stage1-configure-RA_check/35642/
I'm still looking into it. It doesn't look like the sanitizers are
reporting anything suspicious. Do you by any chance know what went wrong?
If it will turn out to be a macOS only thing it might make sense to XFAIL
formatting.test until the issue is resolved.
Thanks,
Alex
On 20 September 2017 at 13:58, Ilya Biryukov via cfe-commits <
cfe-commits at lists.llvm.org> wrote:
> Author: ibiryukov
> Date: Wed Sep 20 05:58:55 2017
> New Revision: 313754
>
> URL: http://llvm.org/viewvc/llvm-project?rev=313754&view=rev
> Log:
> [clangd] Serialize onDiagnosticsReady callbacks for the same file.
>
> Summary:
> Calls to onDiagnosticsReady were done concurrently before. This sometimes
> led to older versions of diagnostics being reported to the user after
> the newer versions.
>
> Reviewers: klimek, bkramer, krasimir
>
> Reviewed By: klimek
>
> Subscribers: cfe-commits
>
> Differential Revision: https://reviews.llvm.org/D38032
>
> Modified:
> clang-tools-extra/trunk/clangd/ClangdServer.cpp
> clang-tools-extra/trunk/clangd/ClangdServer.h
> clang-tools-extra/trunk/clangd/DraftStore.h
> clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
>
> Modified: clang-tools-extra/trunk/clangd/ClangdServer.cpp
> URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/
> trunk/clangd/ClangdServer.cpp?rev=313754&r1=313753&r2=313754&view=diff
> ============================================================
> ==================
> --- clang-tools-extra/trunk/clangd/ClangdServer.cpp (original)
> +++ clang-tools-extra/trunk/clangd/ClangdServer.cpp Wed Sep 20 05:58:55
> 2017
> @@ -315,6 +315,19 @@ std::future<void> ClangdServer::schedule
> 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));
> };
>
> Modified: clang-tools-extra/trunk/clangd/ClangdServer.h
> URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/
> trunk/clangd/ClangdServer.h?rev=313754&r1=313753&r2=313754&view=diff
> ============================================================
> ==================
> --- clang-tools-extra/trunk/clangd/ClangdServer.h (original)
> +++ clang-tools-extra/trunk/clangd/ClangdServer.h Wed Sep 20 05:58:55 2017
> @@ -284,6 +284,13 @@ private:
> // 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
>
> Modified: clang-tools-extra/trunk/clangd/DraftStore.h
> URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/
> trunk/clangd/DraftStore.h?rev=313754&r1=313753&r2=313754&view=diff
> ============================================================
> ==================
> --- clang-tools-extra/trunk/clangd/DraftStore.h (original)
> +++ clang-tools-extra/trunk/clangd/DraftStore.h Wed Sep 20 05:58:55 2017
> @@ -13,6 +13,7 @@
> #include "Path.h"
> #include "clang/Basic/LLVM.h"
> #include "llvm/ADT/StringMap.h"
> +#include <cstdint>
> #include <mutex>
> #include <string>
> #include <vector>
> @@ -20,8 +21,8 @@
> 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 {
>
> Modified: clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
> URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/
> trunk/unittests/clangd/ClangdTests.cpp?rev=313754&r1=
> 313753&r2=313754&view=diff
> ============================================================
> ==================
> --- clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp (original)
> +++ clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp Wed Sep 20
> 05:58:55 2017
> @@ -22,6 +22,7 @@
> #include <iostream>
> #include <random>
> #include <string>
> +#include <thread>
> #include <vector>
>
> namespace clang {
> @@ -898,5 +899,67 @@ int d;
> }
> }
>
> +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
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170920/b014e3cc/attachment-0001.html>
More information about the cfe-commits
mailing list