[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