<div dir="ltr">I think I know what's wrong. I've already seen those failures. std::mutex gets destroyed before threads waiting on it are joined.<div>Will submit a fix shortly.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Sep 20, 2017 at 8:22 PM, Alex L <span dir="ltr"><<a href="mailto:arphaman@gmail.com" target="_blank">arphaman@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><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/" target="_blank">http://lab.llvm.org:8080/<wbr>green/job/clang-stage1-<wbr>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="HOEnZb"><div class="h5"><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-pr<wbr>oject?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/D3803<wbr>2</a><br>
<br>
Modified:<br>
clang-tools-extra/trunk/clangd<wbr>/ClangdServer.cpp<br>
clang-tools-extra/trunk/clangd<wbr>/ClangdServer.h<br>
clang-tools-extra/trunk/clangd<wbr>/DraftStore.h<br>
clang-tools-extra/trunk/unitte<wbr>sts/clangd/ClangdTests.cpp<br>
<br>
Modified: clang-tools-extra/trunk/clangd<wbr>/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-pr<wbr>oject/clang-tools-extra/trunk/<wbr>clangd/ClangdServer.cpp?rev=<wbr>313754&r1=313753&r2=313754&<wbr>view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- clang-tools-extra/trunk/clangd<wbr>/ClangdServer.cpp (original)<br>
+++ clang-tools-extra/trunk/clangd<wbr>/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[Fil<wbr>eStr];<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.onDiagnosticsRea<wbr>dy(FileStr,<br>
make_tagged(std::move(*Diags)<wbr>, Tag));<br>
};<br>
<br>
Modified: clang-tools-extra/trunk/clangd<wbr>/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-pr<wbr>oject/clang-tools-extra/trunk/<wbr>clangd/ClangdServer.h?rev=<wbr>313754&r1=313753&r2=313754&<wbr>view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- clang-tools-extra/trunk/clangd<wbr>/ClangdServer.h (original)<br>
+++ clang-tools-extra/trunk/clangd<wbr>/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/clangd<wbr>/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-pr<wbr>oject/clang-tools-extra/trunk/<wbr>clangd/DraftStore.h?rev=313754<wbr>&r1=313753&r2=313754&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- clang-tools-extra/trunk/clangd<wbr>/DraftStore.h (original)<br>
+++ clang-tools-extra/trunk/clangd<wbr>/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/unitte<wbr>sts/clangd/ClangdTests.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-pr<wbr>oject/clang-tools-extra/trunk/<wbr>unittests/clangd/ClangdTests.<wbr>cpp?rev=313754&r1=313753&r2=<wbr>313754&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- clang-tools-extra/trunk/unitte<wbr>sts/clangd/ClangdTests.cpp (original)<br>
+++ clang-tools-extra/trunk/unitte<wbr>sts/clangd/ClangdTests.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(S<wbr>tartSecondReparse)) {}<br>
+<br>
+ void onDiagnosticsReady(<br>
+ PathRef File,<br>
+ Tagged<std::vector<DiagWithFix<wbr>Its>> 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(st<wbr>d::chrono::milliseconds(50));<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.cp<wbr>p");<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(StartSecondReparsePr<wbr>omise));<br>
+<br>
+ MockCompilationDatabase CDB(/*AddFreestandingFlag=*/tr<wbr>ue);<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" target="_blank">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>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div>Regards,</div><div>Ilya Biryukov</div></div></div></div></div>
</div>