Fixed by r313801. Sorry for all the trouble. On Wed, Sep 20, 2017 at 9:22 PM, Ilya Biryukov <ibiryu...@google.com> wrote:
> 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[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=313 >>> 754&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 > -- Regards, Ilya Biryukov
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits