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@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@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
