[clang-tools-extra] r313754 - [clangd] Serialize onDiagnosticsReady callbacks for the same file.
Alex L via cfe-commits
cfe-commits at lists.llvm.org
Wed Sep 20 13:21:15 PDT 2017
Perfect, thanks!
On 20 September 2017 at 20:33, Ilya Biryukov <ibiryukov at google.com> wrote:
> 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/cd74ecaa/attachment.html>
More information about the cfe-commits
mailing list