I think I know what's wrong. I've already seen those failures. std::mutex gets destroyed before threads waiting on it are joined. Will submit a fix shortly.
On Wed, Sep 20, 2017 at 8:22 PM, Alex L <arpha...@gmail.com> wrote: > 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[Fil >> eStr]; >> + // 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 >> > > -- Regards, Ilya Biryukov
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
