[clang-tools-extra] r313754 - [clangd] Serialize onDiagnosticsReady callbacks for the same file.
Ilya Biryukov via cfe-commits
cfe-commits at lists.llvm.org
Wed Sep 20 12:22:13 PDT 2017
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 <arphaman at 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 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[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 at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
>
--
Regards,
Ilya Biryukov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170920/099d3e58/attachment-0001.html>
More information about the cfe-commits
mailing list