<div dir="ltr">This commit causes the formatting.test to fail on macOS with an uncaught exception:<div><br></div><div>libc++abi.dylib: terminating with uncaught exception of type std::__1::system_error: mutex lock failed: Invalid argument<br></div><div><a href="http://lab.llvm.org:8080/green/job/clang-stage1-configure-RA_check/35642/">http://lab.llvm.org:8080/green/job/clang-stage1-configure-RA_check/35642/</a><br></div><div><br></div><div>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?</div><div>If it will turn out to be a macOS only thing it might make sense to XFAIL formatting.test until the issue is resolved.</div><div><br></div><div>Thanks,</div><div>Alex</div></div><div class="gmail_extra"><br><div class="gmail_quote">On 20 September 2017 at 13:58, Ilya Biryukov via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: ibiryukov<br>
Date: Wed Sep 20 05:58:55 2017<br>
New Revision: 313754<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=313754&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=313754&view=rev</a><br>
Log:<br>
[clangd] Serialize onDiagnosticsReady callbacks for the same file.<br>
<br>
Summary:<br>
Calls to onDiagnosticsReady were done concurrently before. This sometimes<br>
led to older versions of diagnostics being reported to the user after<br>
the newer versions.<br>
<br>
Reviewers: klimek, bkramer, krasimir<br>
<br>
Reviewed By: klimek<br>
<br>
Subscribers: cfe-commits<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D38032" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D38032</a><br>
<br>
Modified:<br>
    clang-tools-extra/trunk/<wbr>clangd/ClangdServer.cpp<br>
    clang-tools-extra/trunk/<wbr>clangd/ClangdServer.h<br>
    clang-tools-extra/trunk/<wbr>clangd/DraftStore.h<br>
    clang-tools-extra/trunk/<wbr>unittests/clangd/ClangdTests.<wbr>cpp<br>
<br>
Modified: clang-tools-extra/trunk/<wbr>clangd/ClangdServer.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.cpp?rev=313754&r1=313753&r2=313754&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/clang-tools-extra/<wbr>trunk/clangd/ClangdServer.cpp?<wbr>rev=313754&r1=313753&r2=<wbr>313754&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- clang-tools-extra/trunk/<wbr>clangd/ClangdServer.cpp (original)<br>
+++ clang-tools-extra/trunk/<wbr>clangd/ClangdServer.cpp Wed Sep 20 05:58:55 2017<br>
@@ -315,6 +315,19 @@ std::future<void> ClangdServer::schedule<br>
     auto Diags = DeferredRebuild.get();<br>
     if (!Diags)<br>
       return; // A new reparse was requested before this one completed.<br>
+<br>
+    // We need to serialize access to resulting diagnostics to avoid calling<br>
+    // `onDiagnosticsReady` in the wrong order.<br>
+    std::lock_guard<std::mutex> DiagsLock(DiagnosticsMutex);<br>
+    DocVersion &LastReportedDiagsVersion = ReportedDiagnosticVersions[<wbr>FileStr];<br>
+    // FIXME(ibiryukov): get rid of '<' comparison here. In the current<br>
+    // implementation diagnostics will not be reported after version counters'<br>
+    // overflow. This should not happen in practice, since DocVersion is a<br>
+    // 64-bit unsigned integer.<br>
+    if (Version < LastReportedDiagsVersion)<br>
+      return;<br>
+    LastReportedDiagsVersion = Version;<br>
+<br>
     DiagConsumer.<wbr>onDiagnosticsReady(FileStr,<br>
                                     make_tagged(std::move(*Diags), Tag));<br>
   };<br>
<br>
Modified: clang-tools-extra/trunk/<wbr>clangd/ClangdServer.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.h?rev=313754&r1=313753&r2=313754&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/clang-tools-extra/<wbr>trunk/clangd/ClangdServer.h?<wbr>rev=313754&r1=313753&r2=<wbr>313754&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- clang-tools-extra/trunk/<wbr>clangd/ClangdServer.h (original)<br>
+++ clang-tools-extra/trunk/<wbr>clangd/ClangdServer.h Wed Sep 20 05:58:55 2017<br>
@@ -284,6 +284,13 @@ private:<br>
   // ClangdServer<br>
   ClangdScheduler WorkScheduler;<br>
   bool SnippetCompletions;<br>
+<br>
+  /// Used to serialize diagnostic callbacks.<br>
+  /// FIXME(ibiryukov): get rid of an extra map and put all version counters<br>
+  /// into CppFile.<br>
+  std::mutex DiagnosticsMutex;<br>
+  /// Maps from a filename to the latest version of reported diagnostics.<br>
+  llvm::StringMap<DocVersion> ReportedDiagnosticVersions;<br>
 };<br>
<br>
 } // namespace clangd<br>
<br>
Modified: clang-tools-extra/trunk/<wbr>clangd/DraftStore.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/DraftStore.h?rev=313754&r1=313753&r2=313754&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/clang-tools-extra/<wbr>trunk/clangd/DraftStore.h?rev=<wbr>313754&r1=313753&r2=313754&<wbr>view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- clang-tools-extra/trunk/<wbr>clangd/DraftStore.h (original)<br>
+++ clang-tools-extra/trunk/<wbr>clangd/DraftStore.h Wed Sep 20 05:58:55 2017<br>
@@ -13,6 +13,7 @@<br>
 #include "Path.h"<br>
 #include "clang/Basic/LLVM.h"<br>
 #include "llvm/ADT/StringMap.h"<br>
+#include <cstdint><br>
 #include <mutex><br>
 #include <string><br>
 #include <vector><br>
@@ -20,8 +21,8 @@<br>
 namespace clang {<br>
 namespace clangd {<br>
<br>
-/// Using 'unsigned' here to avoid undefined behaviour on overflow.<br>
-typedef unsigned DocVersion;<br>
+/// Using unsigned int type here to avoid undefined behaviour on overflow.<br>
+typedef uint64_t DocVersion;<br>
<br>
 /// Document draft with a version of this draft.<br>
 struct VersionedDraft {<br>
<br>
Modified: clang-tools-extra/trunk/<wbr>unittests/clangd/ClangdTests.<wbr>cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp?rev=313754&r1=313753&r2=313754&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/clang-tools-extra/<wbr>trunk/unittests/clangd/<wbr>ClangdTests.cpp?rev=313754&r1=<wbr>313753&r2=313754&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- clang-tools-extra/trunk/<wbr>unittests/clangd/ClangdTests.<wbr>cpp (original)<br>
+++ clang-tools-extra/trunk/<wbr>unittests/clangd/ClangdTests.<wbr>cpp Wed Sep 20 05:58:55 2017<br>
@@ -22,6 +22,7 @@<br>
 #include <iostream><br>
 #include <random><br>
 #include <string><br>
+#include <thread><br>
 #include <vector><br>
<br>
 namespace clang {<br>
@@ -898,5 +899,67 @@ int d;<br>
   }<br>
 }<br>
<br>
+TEST_F(ClangdThreadingTest, NoConcurrentDiagnostics) {<br>
+  class NoConcurrentAccessDiagConsumer : public DiagnosticsConsumer {<br>
+  public:<br>
+    NoConcurrentAccessDiagConsumer<wbr>(std::promise<void> StartSecondReparse)<br>
+        : StartSecondReparse(std::move(<wbr>StartSecondReparse)) {}<br>
+<br>
+    void onDiagnosticsReady(<br>
+        PathRef File,<br>
+        Tagged<std::vector<<wbr>DiagWithFixIts>> Diagnostics) override {<br>
+<br>
+      std::unique_lock<std::mutex> Lock(Mutex, std::try_to_lock_t());<br>
+      ASSERT_TRUE(Lock.owns_lock())<br>
+          << "Detected concurrent onDiagnosticsReady calls for the same file.";<br>
+      if (FirstRequest) {<br>
+        FirstRequest = false;<br>
+        StartSecondReparse.set_value()<wbr>;<br>
+        // Sleep long enough for the second request to be processed.<br>
+        std::this_thread::sleep_for(<wbr>std::chrono::milliseconds(50))<wbr>;<br>
+      }<br>
+    }<br>
+<br>
+  private:<br>
+    std::mutex Mutex;<br>
+    bool FirstRequest = true;<br>
+    std::promise<void> StartSecondReparse;<br>
+  };<br>
+<br>
+  const auto SourceContentsWithoutErrors = R"cpp(<br>
+int a;<br>
+int b;<br>
+int c;<br>
+int d;<br>
+)cpp";<br>
+<br>
+  const auto SourceContentsWithErrors = R"cpp(<br>
+int a = x;<br>
+int b;<br>
+int c;<br>
+int d;<br>
+)cpp";<br>
+<br>
+  auto FooCpp = getVirtualTestFilePath("foo.<wbr>cpp");<br>
+  llvm::StringMap<std::string> FileContents;<br>
+  FileContents[FooCpp] = "";<br>
+  ConstantFSProvider FS(buildTestFS(FileContents));<br>
+<br>
+  std::promise<void> StartSecondReparsePromise;<br>
+  std::future<void> StartSecondReparse = StartSecondReparsePromise.get_<wbr>future();<br>
+<br>
+  NoConcurrentAccessDiagConsumer DiagConsumer(<br>
+      std::move(<wbr>StartSecondReparsePromise));<br>
+<br>
+  MockCompilationDatabase CDB(/*AddFreestandingFlag=*/<wbr>true);<br>
+  ClangdServer Server(CDB, DiagConsumer, FS, 4, /*SnippetCompletions=*/false,<br>
+                      EmptyLogger::getInstance());<br>
+  Server.addDocument(FooCpp, SourceContentsWithErrors);<br>
+  StartSecondReparse.wait();<br>
+<br>
+  auto Future = Server.addDocument(FooCpp, SourceContentsWithoutErrors);<br>
+  Future.wait();<br>
+}<br>
+<br>
 } // namespace clangd<br>
 } // namespace clang<br>
<br>
<br>
______________________________<wbr>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div>