[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:33:48 PDT 2017
Fixed by r313801.
Sorry for all the trouble.
On Wed, Sep 20, 2017 at 9:22 PM, Ilya Biryukov <ibiryukov at 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 <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[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 at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>>
>>
>
>
> --
> Regards,
> Ilya Biryukov
>
--
Regards,
Ilya Biryukov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170920/d7ce74f6/attachment-0001.html>
More information about the cfe-commits
mailing list