Perfect, thanks! On 20 September 2017 at 20:33, Ilya Biryukov <ibiryu...@google.com> wrote:
> 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