[PATCH] D50455: Continue emitting diagnostics after a fatal error

[Dmitry via Phabricator via cfe-commits]

Dmitry.Kozhevnikov created this revision.
Dmitry.Kozhevnikov added reviewers: ilya-biryukov, sammccall.
Herald added subscribers: cfe-commits, arphaman, jkorous.

By default, diagnostics are suppressed after a fatal error. Some fatal errors 
(notably, "file not found" for include directive) are common for incomplete 
code and we probably want to have further diagnostics anyway.

Currently, this flag is optionally set by libclang (see 
CXTranslationUnit_KeepGoing option).

There are also a bunch of related problems when AST is not fully built in 
presence of fatal errors (templates are not instantiated and include directives 
are not processed), I'll address these in separate patches.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50455

Files:
  clangd/ClangdUnit.cpp
  clangd/Compiler.cpp
  test/clangd/missing-includes.test


Index: test/clangd/missing-includes.test
===
--- test/clangd/missing-includes.test
+++ test/clangd/missing-includes.test
@@ -0,0 +1,13 @@
+# RUN: clangd -lit-test < %s | FileCheck %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","version":1,"text":"#include
 \n#include \nint x;\n#include \n#include \n"}}}
+# CHECK:  "method": "textDocument/publishDiagnostics",
+# CHECK:  "message": "'a' file not found",
+# CHECK:  "message": "'b' file not found",
+# CHECK:  "message": "'c' file not found",
+# CHECK:  "message": "'d' file not found",
+---
+{"jsonrpc":"2.0","id":5,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/Compiler.cpp
===
--- clangd/Compiler.cpp
+++ clangd/Compiler.cpp
@@ -63,6 +63,7 @@
   auto Clang = llvm::make_unique(PCHs);
   Clang->setInvocation(std::move(CI));
   Clang->createDiagnostics(&DiagsClient, false);
+  Clang->getDiagnostics().setSuppressAfterFatalError(false);
 
   if (auto VFSWithRemapping = createVFSFromCompilerInvocation(
   Clang->getInvocation(), Clang->getDiagnostics(), VFS))
Index: clangd/ClangdUnit.cpp
===
--- clangd/ClangdUnit.cpp
+++ clangd/ClangdUnit.cpp
@@ -328,6 +328,8 @@
   // to read back. We rely on dynamic index for the comments instead.
   CI.getPreprocessorOpts().WriteCommentListToPCH = false;
 
+  PreambleDiagsEngine->setSuppressAfterFatalError(false);
+
   CppFilePreambleCallbacks SerializedDeclsCollector(FileName, 
PreambleCallback);
   if (Inputs.FS->setCurrentWorkingDirectory(Inputs.CompileCommand.Directory)) {
 log("Couldn't set working directory when building the preamble.");


Index: test/clangd/missing-includes.test
===
--- test/clangd/missing-includes.test
+++ test/clangd/missing-includes.test
@@ -0,0 +1,13 @@
+# RUN: clangd -lit-test < %s | FileCheck %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","version":1,"text":"#include \n#include \nint x;\n#include \n#include \n"}}}
+# CHECK:  "method": "textDocument/publishDiagnostics",
+# CHECK:  "message": "'a' file not found",
+# CHECK:  "message": "'b' file not found",
+# CHECK:  "message": "'c' file not found",
+# CHECK:  "message": "'d' file not found",
+---
+{"jsonrpc":"2.0","id":5,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/Compiler.cpp
===
--- clangd/Compiler.cpp
+++ clangd/Compiler.cpp
@@ -63,6 +63,7 @@
   auto Clang = llvm::make_unique(PCHs);
   Clang->setInvocation(std::move(CI));
   Clang->createDiagnostics(&DiagsClient, false);
+  Clang->getDiagnostics().setSuppressAfterFatalError(false);
 
   if (auto VFSWithRemapping = createVFSFromCompilerInvocation(
   Clang->getInvocation(), Clang->getDiagnostics(), VFS))
Index: clangd/ClangdUnit.cpp
===
--- clangd/ClangdUnit.cpp
+++ clangd/ClangdUnit.cpp
@@ -328,6 +328,8 @@
   // to read back. We rely on dynamic index for the comments instead.
   CI.getPreprocessorOpts().WriteCommentListToPCH = false;
 
+  PreambleDiagsEngine->setSuppressAfterFatalError(false);
+
   CppFilePreambleCallbacks SerializedDeclsCollector(FileName, PreambleCallback);
   if (Inputs.FS->setCurrentWorkingDirectory(Inputs.CompileCommand.Directory)) {
 log("Couldn't set working directory when building the preamble.");
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50462: Try building complete AST after a fatal error was emitted if further diagnostics are expected

[Dmitry via Phabricator via cfe-commits]

Dmitry.Kozhevnikov created this revision.
Dmitry.Kozhevnikov added reviewers: ilya-biryukov, sammccall, milianw.
Herald added subscribers: cfe-commits, ioeric.

Related to https://reviews.llvm.org/D50455

There is some code here and there that assume that no sane output is required 
if a fatal error has occurred. When using clangd/libclang, it's no longer true: 
diagnostics might still be requested, and AST might still be required for other 
IDE/tooling features, so it has to be as complete as possible.

Here I try to separate the following use cases:

1. Some clients check `hasFatalErrorOccurred()` because they are known to work 
unstable in presence of compile errors and want to mitigate it - they'll work 
as before
2. However, we don't want to take shortcuts in PP and Sema and still want to 
process include directives and instantiate templates

Note: I've found out that initially the flag in `DiagnosticsEngine` (which is 
now called `SuppressAfterFatalError`) had different meaning and was just 
demoting all fatal errors to non-fatal (https://reviews.llvm.org/rL262318). 
This would also fix this issue, however, it was partly reverted in 
https://reviews.llvm.org/rL301992 to the current state. Maybe we should go with 
the old approach instead (I assume the issue was that this flag was not 
serialized/restored, but probably should?)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50462

Files:
  include/clang/Basic/Diagnostic.h
  lib/Lex/PPDirectives.cpp
  lib/Sema/SemaTemplateInstantiate.cpp
  unittests/Frontend/PCHPreambleTest.cpp

Index: unittests/Frontend/PCHPreambleTest.cpp
===
--- unittests/Frontend/PCHPreambleTest.cpp
+++ unittests/Frontend/PCHPreambleTest.cpp
@@ -77,7 +77,8 @@
 RemappedFiles[Filename] = Contents;
   }
 
-  std::unique_ptr ParseAST(const std::string &EntryFile) {
+  std::unique_ptr ParseAST(const std::string &EntryFile,
+bool SuppressAfterFatalError = true) {
 PCHContainerOpts = std::make_shared();
 std::shared_ptr CI(new CompilerInvocation);
 CI->getFrontendOpts().Inputs.push_back(
@@ -88,11 +89,15 @@
 
 CI->getPreprocessorOpts().RemappedFileBuffers = GetRemappedFiles();
 
+CI->getLangOpts()->CPlusPlus = true;
+CI->getLangOpts()->CPlusPlus11 = true;
+
 PreprocessorOptions &PPOpts = CI->getPreprocessorOpts();
 PPOpts.RemappedFilesKeepOriginalName = true;
 
 IntrusiveRefCntPtr
   Diags(CompilerInstance::createDiagnostics(new DiagnosticOptions, new DiagnosticConsumer));
+Diags->setSuppressAfterFatalError(SuppressAfterFatalError);
 
 FileManager *FileMgr = new FileManager(FSOpts, VFS);
 
@@ -197,4 +202,33 @@
   ASSERT_LE(HeaderReadCount, GetFileReadCount(Header));
 }
 
+TEST_F(PCHPreambleTest, MissingHeader) {
+  std::string Header1 = "//./header1.h";
+  AddFile(Header1,
+"template  class C;\n"
+"template  class C{};\n");
+
+  std::string Header2 = "//./header2.h";
+  AddFile(Header2, "using Alias = C;\n");
+
+  std::string Main = "//./main.cpp";
+  AddFile(Main,
+"#include \"nonexistent1\"\n"
+"#include \"//./header1.h\"\n"
+"#include \"nonexistent2\"\n"
+"#include \"//./header2.h\"\n"
+"Alias Var;");
+
+  std::unique_ptr AST(ParseAST(Main, /*SuppressAfterFatalError=*/false));
+  ASSERT_TRUE(AST.get());
+
+  // only "file not found" errors should be emitted,
+  // "Alias" should be visible for lookup.
+  auto ExpectedErrorsCount = 2u;
+
+  ASSERT_EQ(AST->getDiagnostics().getClient()->getNumErrors(), ExpectedErrorsCount);
+  ASSERT_TRUE(ReparseAST(AST));
+  ASSERT_EQ(AST->getDiagnostics().getClient()->getNumErrors(), ExpectedErrorsCount);
+}
+
 } // anonymous namespace
Index: lib/Sema/SemaTemplateInstantiate.cpp
===
--- lib/Sema/SemaTemplateInstantiate.cpp
+++ lib/Sema/SemaTemplateInstantiate.cpp
@@ -218,7 +218,7 @@
   // Don't allow further instantiation if a fatal error and an uncompilable
   // error have occurred. Any diagnostics we might have raised will not be
   // visible, and we do not need to construct a correct AST.
-  if (SemaRef.Diags.hasFatalErrorOccurred() &&
+  if (SemaRef.Diags.areDiagnosticsSuppressedAfterFatalError() &&
   SemaRef.Diags.hasUncompilableErrorOccurred()) {
 Invalid = true;
 return;
Index: lib/Lex/PPDirectives.cpp
===
--- lib/Lex/PPDirectives.cpp
+++ lib/Lex/PPDirectives.cpp
@@ -1899,7 +1899,7 @@
   // Any diagnostics after the fatal error will not be visible. As the
   // compilation failed already and errors in subsequently included files won't
   // be visible, avoid preprocessing those files.
-  if (ShouldEnter && Diags->hasFatalErrorOccurred())
+  if (ShouldEnter && Diags->areDiagnosticsSuppressedAfterFatalError())
 ShouldEnter = false;
 
   // Determine whether we should try to import the module for this #include

[PATCH] D50455: Continue emitting diagnostics after a fatal error

[Dmitry via Phabricator via cfe-commits]

Dmitry.Kozhevnikov added a comment.

See also https://reviews.llvm.org/D50462


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50455: Continue emitting diagnostics after a fatal error

[Dmitry via Phabricator via cfe-commits]

Dmitry.Kozhevnikov updated this revision to Diff 160837.
Dmitry.Kozhevnikov added a comment.

Add a unit test which explicitly builds preamble/AST


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50455

Files:
  clangd/ClangdUnit.cpp
  clangd/Compiler.cpp
  test/clangd/missing-includes.test
  unittests/clangd/ClangdTests.cpp

Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -15,6 +15,8 @@
 #include "TestFS.h"
 #include "URI.h"
 #include "clang/Config/config.h"
+#include "clang/Frontend/CompilerInvocation.h"
+#include "clang/Frontend/PCHContainerOperations.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/Errc.h"
@@ -37,6 +39,7 @@
 using ::testing::Eq;
 using ::testing::Field;
 using ::testing::Gt;
+using ::testing::HasSubstr;
 using ::testing::IsEmpty;
 using ::testing::Pair;
 using ::testing::UnorderedElementsAre;
@@ -963,6 +966,53 @@
Field(&CodeCompletion::Name, "baz")));
 }
 
+TEST(DiagnosticsTest, RecoverAfterFatalError) {
+  auto FooCpp = testPath("foo.cpp");
+
+  ParseInputs PI;
+  PI.CompileCommand.Directory = testRoot();
+  PI.CompileCommand.Filename = FooCpp;
+  PI.CompileCommand.CommandLine = {"clang", "-xc++", FooCpp};
+
+  llvm::StringMap Files;
+  Files[FooCpp] = "";
+  PI.FS = buildTestFS(Files);
+
+  PI.Contents = R"cpp(
+#include "preamble1"
+#include "preamble2"
+// end of the preamble
+int x;
+#include "main1"
+#include "main2"
+  )cpp";
+
+  auto PreambleCI = buildCompilerInvocation(PI);
+
+  auto Preamble = buildPreamble(
+  FooCpp, *PreambleCI, /*OldPreamble=*/nullptr, tooling::CompileCommand(),
+  PI, std::make_shared(), /*StoreInMemory=*/true,
+  /*PreambleCallback=*/nullptr);
+  ASSERT_TRUE(Preamble) << "Failed to build preamble";
+
+  ASSERT_EQ(Preamble->Diags.size(), 2u);
+  EXPECT_THAT(Preamble->Diags[0].Message, HasSubstr("preamble1"));
+  EXPECT_THAT(Preamble->Diags[1].Message, HasSubstr("preamble2"));
+
+  auto MainFileCI = buildCompilerInvocation(PI);
+  auto AST =
+  ParsedAST::build(std::move(MainFileCI), Preamble,
+   llvm::MemoryBuffer::getMemBufferCopy(PI.Contents),
+   std::make_shared(), PI.FS);
+  ASSERT_TRUE(AST.hasValue()) << "Failed to build AST";
+
+  ASSERT_EQ(AST->getDiagnostics().size(), 4u);
+  EXPECT_THAT(AST->getDiagnostics()[0].Message, HasSubstr("preamble1"));
+  EXPECT_THAT(AST->getDiagnostics()[1].Message, HasSubstr("preamble2"));
+  EXPECT_THAT(AST->getDiagnostics()[2].Message, HasSubstr("main1"));
+  EXPECT_THAT(AST->getDiagnostics()[3].Message, HasSubstr("main2"));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: test/clangd/missing-includes.test
===
--- /dev/null
+++ test/clangd/missing-includes.test
@@ -0,0 +1,13 @@
+# RUN: clangd -lit-test < %s | FileCheck %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","version":1,"text":"#include \n#include \nint x;\n#include \n#include \n"}}}
+# CHECK:  "method": "textDocument/publishDiagnostics",
+# CHECK:  "message": "'a' file not found",
+# CHECK:  "message": "'b' file not found",
+# CHECK:  "message": "'c' file not found",
+# CHECK:  "message": "'d' file not found",
+---
+{"jsonrpc":"2.0","id":5,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/Compiler.cpp
===
--- clangd/Compiler.cpp
+++ clangd/Compiler.cpp
@@ -63,6 +63,7 @@
   auto Clang = llvm::make_unique(PCHs);
   Clang->setInvocation(std::move(CI));
   Clang->createDiagnostics(&DiagsClient, false);
+  Clang->getDiagnostics().setSuppressAfterFatalError(false);
 
   if (auto VFSWithRemapping = createVFSFromCompilerInvocation(
   Clang->getInvocation(), Clang->getDiagnostics(), VFS))
Index: clangd/ClangdUnit.cpp
===
--- clangd/ClangdUnit.cpp
+++ clangd/ClangdUnit.cpp
@@ -328,6 +328,8 @@
   // to read back. We rely on dynamic index for the comments instead.
   CI.getPreprocessorOpts().WriteCommentListToPCH = false;
 
+  PreambleDiagsEngine->setSuppressAfterFatalError(false);
+
   CppFilePreambleCallbacks SerializedDeclsCollector(FileName, PreambleCallback);
   if (Inputs.FS->setCurrentWorkingDirectory(Inputs.CompileCommand.Directory)) {
 log("Couldn't set working directory when building the preamble.");
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50455: Continue emitting diagnostics after a fatal error

[Dmitry via Phabricator via cfe-commits]

Dmitry.Kozhevnikov added a comment.

In https://reviews.llvm.org/D50455#1193468, @ilya-biryukov wrote:

> I'm a bit worried that clang may emit too many irrelevant errors, because it 
> may not cope nicely with those fatal errors, i.e. wasn't tested that 
> thoroughly in that mode.


It sort of happens already: even if you have a fatal error in a preamble, 
diagnostics for the main file are still produced. This just expands it for the 
case the fatal error and the rest of the code happen to exist both in the 
preamble/main file.

> Nevertheless, I think this is moving clangd in the right direction and if we 
> see too many irrelevant errors, we should look into improving clang to show 
> less of those.

We've enabled it in CLion by default, hopefully, we'll get some feedback.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50462: Try building complete AST after a fatal error was emitted if further diagnostics are expected

[Dmitry via Phabricator via cfe-commits]

Dmitry.Kozhevnikov updated this revision to Diff 160848.
Dmitry.Kozhevnikov changed the repository for this revision from rCTE Clang 
Tools Extra to rC Clang.
Dmitry.Kozhevnikov added a comment.

Addressed the review comment about separating "suppressing diagnostics" from 
"providing more complete AST"


Repository:
  rC Clang

https://reviews.llvm.org/D50462

Files:
  include/clang/Basic/Diagnostic.h
  lib/Lex/PPDirectives.cpp
  lib/Sema/SemaTemplateInstantiate.cpp
  unittests/Frontend/PCHPreambleTest.cpp

Index: unittests/Frontend/PCHPreambleTest.cpp
===
--- unittests/Frontend/PCHPreambleTest.cpp
+++ unittests/Frontend/PCHPreambleTest.cpp
@@ -77,7 +77,8 @@
 RemappedFiles[Filename] = Contents;
   }
 
-  std::unique_ptr ParseAST(const std::string &EntryFile) {
+  std::unique_ptr ParseAST(const std::string &EntryFile,
+bool SuppressAfterFatalError = true) {
 PCHContainerOpts = std::make_shared();
 std::shared_ptr CI(new CompilerInvocation);
 CI->getFrontendOpts().Inputs.push_back(
@@ -88,11 +89,15 @@
 
 CI->getPreprocessorOpts().RemappedFileBuffers = GetRemappedFiles();
 
+CI->getLangOpts()->CPlusPlus = true;
+CI->getLangOpts()->CPlusPlus11 = true;
+
 PreprocessorOptions &PPOpts = CI->getPreprocessorOpts();
 PPOpts.RemappedFilesKeepOriginalName = true;
 
 IntrusiveRefCntPtr
   Diags(CompilerInstance::createDiagnostics(new DiagnosticOptions, new DiagnosticConsumer));
+Diags->setSuppressAfterFatalError(SuppressAfterFatalError);
 
 FileManager *FileMgr = new FileManager(FSOpts, VFS);
 
@@ -197,4 +202,34 @@
   ASSERT_LE(HeaderReadCount, GetFileReadCount(Header));
 }
 
+TEST_F(PCHPreambleTest, MissingHeader) {
+  std::string Header1 = "//./header1.h";
+  AddFile(Header1, "template  class C;\n"
+   "template  class C{};\n");
+
+  std::string Header2 = "//./header2.h";
+  AddFile(Header2, "using Alias = C;\n");
+
+  std::string Main = "//./main.cpp";
+  AddFile(Main, "#include \"nonexistent1\"\n"
+"#include \"//./header1.h\"\n"
+"#include \"nonexistent2\"\n"
+"#include \"//./header2.h\"\n"
+"Alias Var;");
+
+  std::unique_ptr AST(
+  ParseAST(Main, /*SuppressAfterFatalError=*/false));
+  ASSERT_TRUE(AST.get());
+
+  // only "file not found" errors should be emitted,
+  // "Alias" should be visible for lookup.
+  auto ExpectedErrorsCount = 2u;
+
+  ASSERT_EQ(AST->getDiagnostics().getClient()->getNumErrors(),
+ExpectedErrorsCount);
+  ASSERT_TRUE(ReparseAST(AST));
+  ASSERT_EQ(AST->getDiagnostics().getClient()->getNumErrors(),
+ExpectedErrorsCount);
+}
+
 } // anonymous namespace
Index: lib/Sema/SemaTemplateInstantiate.cpp
===
--- lib/Sema/SemaTemplateInstantiate.cpp
+++ lib/Sema/SemaTemplateInstantiate.cpp
@@ -219,6 +219,7 @@
   // error have occurred. Any diagnostics we might have raised will not be
   // visible, and we do not need to construct a correct AST.
   if (SemaRef.Diags.hasFatalErrorOccurred() &&
+  !SemaRef.Diags.shouldRecoverAfterFatalErrors() &&
   SemaRef.Diags.hasUncompilableErrorOccurred()) {
 Invalid = true;
 return;
Index: lib/Lex/PPDirectives.cpp
===
--- lib/Lex/PPDirectives.cpp
+++ lib/Lex/PPDirectives.cpp
@@ -1899,7 +1899,8 @@
   // Any diagnostics after the fatal error will not be visible. As the
   // compilation failed already and errors in subsequently included files won't
   // be visible, avoid preprocessing those files.
-  if (ShouldEnter && Diags->hasFatalErrorOccurred())
+  if (ShouldEnter && Diags->hasFatalErrorOccurred() &&
+  !Diags->shouldRecoverAfterFatalErrors())
 ShouldEnter = false;
 
   // Determine whether we should try to import the module for this #include, if
Index: include/clang/Basic/Diagnostic.h
===
--- include/clang/Basic/Diagnostic.h
+++ include/clang/Basic/Diagnostic.h
@@ -752,6 +752,15 @@
   }
   bool hasFatalErrorOccurred() const { return FatalErrorOccurred; }
 
+  /// Determine whether the client should because a fatal error was issued, so
+  /// clients can cut off extra processing that might be wasteful in this case.
+  bool shouldRecoverAfterFatalErrors() const {
+// todo: a separate flag might be required for a client that doesn't want to
+// show any errors after the fatal, but still wants to collect as much
+// information from the source code as possible.
+return SuppressAfterFatalError;
+  }
+
   /// Determine whether any kind of unrecoverable error has occurred.
   bool hasUnrecoverableErrorOccurred() const {
 return FatalErrorOccurred || UnrecoverableErrorOccurred;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
ht

[PATCH] D50462: Try building complete AST after a fatal error was emitted if further diagnostics are expected

[Dmitry via Phabricator via cfe-commits]

Dmitry.Kozhevnikov added a comment.

In https://reviews.llvm.org/D50462#1193180, @arphaman wrote:

> On a second look I think that there is value separating the concepts of 
> 'producing diagnostics' after hitting the fatal error (which is 
> SuppressAfterFatalError currently does), and trying to build a more complete 
> AST.


Sorry for the delay.

I've introduced an accordingly-named method. I haven't created another flag 
yet, though, because it feels a bit weird currently since there are no clients 
that require it, added a todo note instead. What do you think?




Comment at: unittests/Frontend/PCHPreambleTest.cpp:220
+"#include \"//./header2.h\"\n"
+"Alias Var;");
+

The condition in `SemaTemplateInstantiate.cpp` was causing the lookup of 
`Alias` to fail: it was marked as invalid and wasn't stored in the preamble. 
I'll be happy to make a more straightforward test, but I've not yet managed to: 
when placing everything in a single file, the lookup succeeds.


Repository:
  rC Clang

https://reviews.llvm.org/D50462



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50462: Try building complete AST after a fatal error was emitted if further diagnostics are expected

[Dmitry via Phabricator via cfe-commits]

Dmitry.Kozhevnikov added a comment.

In https://reviews.llvm.org/D50462#1203038, @vsapsai wrote:

> Have you checked that produced AST is of sufficient quality to be used? 
> Because, for example, for invalid code error recovery tries to keep going but 
> the end result isn't always good.
>  In the test you verify that you can ignore bogus includes. Is this the 
> real-world use case you want to address? Or something like "stddef.h not 
> found"?


I'm assuming it should help when the recovery is good enough (e.g. when the 
symbols from the missing header are used rarely, or only used inside function 
bodies). For the case like "`stddef.h` not found" there is a chance that 
resulting AST wouldn't be too useful, however, for IDE-like tasks, where we 
usually want to get as much info from the incomplete code as possible, it's not 
too bad IMO (comparing to just disabling all IDE features altogether after a 
missing include).

The only required property is that the resulting issues shouldn't be 
catastrophic (further processing shouldn't crash or hang), however, it would be 
a separate issue - after all, you always can remove the include directive in 
question.

The main use case I have is that it's inconvenient to browse or edit code in 
clang-based IDEs when either:

- there is a typo in an include directive somewhere (e.g. because I'm still 
modifying the code, moving files around, etc)
- there is a missing/misconfigured 3rd-party dependency. Obviously, the code is 
not supposed to compile, however, I'd expect e.g. "Goto Declaration" still work 
for unrelated code, assuming it recovered properly, which, in my anecdotical 
experience, it usually does
- (a very specific use case I'm often hitting) you often have missing includes 
when you browse llvm/clang code base and haven't built some part of it, so you 
don't have generated headers. The parser usually recovers very well in this 
case, if not for these two cut-offs I'm changing in this patch


Repository:
  rC Clang

https://reviews.llvm.org/D50462



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

[Dmitry via Phabricator via cfe-commits]

Dmitry.Kozhevnikov created this revision.
Dmitry.Kozhevnikov added reviewers: ilya-biryukov, sammccall.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, 
ioeric.

By default it's 512K, which is way to small for clang parser to run on. There 
is no way to do it via platform-independent API, so it's implemented via 
pthreads directly in clangd/Threading.cpp.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50993

Files:
  clangd/Threading.cpp
  unittests/clangd/ClangdTests.cpp

Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -963,6 +963,27 @@
Field(&CodeCompletion::Name, "baz")));
 }
 
+TEST_F(ClangdVFSTest, TestStackOverflow) {
+  MockFSProvider FS;
+  ErrorCheckingDiagConsumer DiagConsumer;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  const auto SourceContents = R"cpp(
+constexpr int foo() { return foo(); }
+static_assert(foo());
+  )cpp";
+
+  auto FooCpp = testPath("foo.cpp");
+  FS.Files[FooCpp] = SourceContents;
+
+  Server.addDocument(FooCpp, SourceContents);
+  ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for diagnostics";
+  // check that we got a constexpr depth error, and not crashed by stack
+  // overflow
+  EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags());
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/Threading.cpp
===
--- clangd/Threading.cpp
+++ clangd/Threading.cpp
@@ -42,6 +42,56 @@
   SlotsChanged.notify_one();
 }
 
+namespace {
+template 
+using UserFnPtr = void (*)(Data);
+
+template 
+struct ThreadInfo {
+  UserFnPtr UserFn;
+  Data UserData;
+};
+} // namespace
+
+template 
+static void executeOnThreadAndDetach(UserFnPtr UserFn, Data UserData) {
+#if !defined(__APPLE__)
+  std::thread([UserFn](Data UserData) { UserFn(std::move(UserData)); },
+  std::move(UserData))
+  .detach();
+#else
+  // On macOS, there is no way to change the default 512K stack size for threads
+  // other than main using the std::thread API. 512K, however, is way to small
+  // for the parser to run. Here we're using pthreads API to set the 8 Mb
+  // stack size, as it's the default for the main thread in modern Linux/macOS
+  // distributions, so it's probably the most well-tested value.
+
+  pthread_attr_t Attr;
+  if (::pthread_attr_init(&Attr) != 0)
+return;
+
+  auto AttrGuard =
+  llvm::make_scope_exit([&] { ::pthread_attr_destroy(&Attr); });
+
+  if (::pthread_attr_setstacksize(&Attr, 8 * 1024 * 1024) != 0)
+return;
+
+  std::unique_ptr> Info(
+  new ThreadInfo{UserFn, std::move(UserData)});
+  pthread_t Thread;
+  auto ThreadFun = [](void *Arg) -> void * {
+std::unique_ptr> TI(static_cast *>(Arg));
+TI->UserFn(std::move(TI->UserData));
+return nullptr;
+  };
+  if (::pthread_create(&Thread, &Attr, ThreadFun, Info.get()) != 0)
+return;
+
+  // now it would be destroyed in the new thread
+  Info.release();
+#endif
+}
+
 AsyncTaskRunner::~AsyncTaskRunner() { wait(); }
 
 bool AsyncTaskRunner::wait(Deadline D) const {
@@ -67,16 +117,22 @@
 }
   });
 
-  std::thread(
-  [](std::string Name, decltype(Action) Action, decltype(CleanupTask)) {
-llvm::set_thread_name(Name);
-Action();
-// Make sure function stored by Action is destroyed before CleanupTask
-// is run.
-Action = nullptr;
-  },
-  Name.str(), std::move(Action), std::move(CleanupTask))
-  .detach();
+  struct Data {
+std::string Name;
+decltype(Action) Action;
+decltype(CleanupTask) CleanupTask;
+  };
+  Data UserData{Name.str(), std::move(Action), std::move(CleanupTask)};
+
+  auto Func = [](Data UserData) {
+llvm::set_thread_name(UserData.Name);
+UserData.Action();
+// Make sure function stored by Action is destroyed before CleanupTask
+// is run.
+UserData.Action = nullptr;
+  };
+
+  executeOnThreadAndDetach(Func, std::move(UserData));
 }
 
 Deadline timeoutSeconds(llvm::Optional Seconds) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

[Dmitry via Phabricator via cfe-commits]

Dmitry.Kozhevnikov added a comment.

In https://reviews.llvm.org/D50993#1207357, @ilya-biryukov wrote:

> 1. Can we put the helper that executes the tasks asynchronously with a stack 
> size into llvm's threading library (i.e. somewhere close to 
> `llvm::execute_on_thread`?) All platform-specific code is already there, we 
> can reuse most of the implementation too.


Do you think it would be OK to have pthreads-only version there, with 
`std::thread` fallback on Windows? I'd like to avoid writing a Windows-only 
implementation myself, as it's a bit alien for me.

> 2. I wonder if we should expose the stack size as an option to clangd?

Do you expect it would be ever used?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50993



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

[Dmitry via Phabricator via cfe-commits]

Dmitry.Kozhevnikov added a comment.

In https://reviews.llvm.org/D50993#1207757, @jfb wrote:

> Isn't this duplicating code in lib/Support/Unix/Threading.inc with a 
> different implementation?


It's definitely duplicating how the thread is created, however, here it's a bit 
more complicated as some dance with the passed function lifetime is required, 
while `llvm_execute_on_thread` just uses a local variable since it blocks till 
the thread is terminated.

Still, I see that it's desirable to have the new code in Support/Threading, 
I'll move it there.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50993



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

[Dmitry via Phabricator via cfe-commits]

Dmitry.Kozhevnikov updated this revision to Diff 161954.
Dmitry.Kozhevnikov added a comment.

I've moved the platform-specific implementation to LLVM/Support/Threading and 
created https://reviews.llvm.org/D51103.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50993

Files:
  clangd/Threading.cpp
  unittests/clangd/ClangdTests.cpp


Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -963,6 +963,27 @@
Field(&CodeCompletion::Name, "baz")));
 }
 
+TEST_F(ClangdVFSTest, TestStackOverflow) {
+  MockFSProvider FS;
+  ErrorCheckingDiagConsumer DiagConsumer;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  const auto SourceContents = R"cpp(
+constexpr int foo() { return foo(); }
+static_assert(foo());
+  )cpp";
+
+  auto FooCpp = testPath("foo.cpp");
+  FS.Files[FooCpp] = SourceContents;
+
+  Server.addDocument(FooCpp, SourceContents);
+  ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for diagnostics";
+  // check that we got a constexpr depth error, and not crashed by stack
+  // overflow
+  EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags());
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/Threading.cpp
===
--- clangd/Threading.cpp
+++ clangd/Threading.cpp
@@ -1,5 +1,6 @@
 #include "Threading.h"
 #include "Trace.h"
+#include "clang/Basic/Stack.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Threading.h"
@@ -67,16 +68,25 @@
 }
   });
 
-  std::thread(
-  [](std::string Name, decltype(Action) Action, decltype(CleanupTask)) {
-llvm::set_thread_name(Name);
-Action();
-// Make sure function stored by Action is destroyed before CleanupTask
-// is run.
-Action = nullptr;
-  },
-  Name.str(), std::move(Action), std::move(CleanupTask))
-  .detach();
+  // Manually capture Action and CleanupTask for the lack of C++14 generalized
+  // lambda captures
+  struct Callable {
+std::string ThreadName;
+decltype(Action) ThreadFunc;
+decltype(CleanupTask) Cleanup;
+
+void operator()() {
+  llvm::set_thread_name(ThreadName);
+  ThreadFunc();
+  // Make sure function stored by ThreadFunc is destroyed before Cleanup is
+  // run.
+  ThreadFunc = nullptr;
+}
+  };
+
+  llvm::llvm_execute_on_thread_async(
+  Callable{Name.str(), std::move(Action), std::move(CleanupTask)},
+  clang::DesiredStackSize);
 }
 
 Deadline timeoutSeconds(llvm::Optional Seconds) {


Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -963,6 +963,27 @@
Field(&CodeCompletion::Name, "baz")));
 }
 
+TEST_F(ClangdVFSTest, TestStackOverflow) {
+  MockFSProvider FS;
+  ErrorCheckingDiagConsumer DiagConsumer;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  const auto SourceContents = R"cpp(
+constexpr int foo() { return foo(); }
+static_assert(foo());
+  )cpp";
+
+  auto FooCpp = testPath("foo.cpp");
+  FS.Files[FooCpp] = SourceContents;
+
+  Server.addDocument(FooCpp, SourceContents);
+  ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for diagnostics";
+  // check that we got a constexpr depth error, and not crashed by stack
+  // overflow
+  EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags());
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/Threading.cpp
===
--- clangd/Threading.cpp
+++ clangd/Threading.cpp
@@ -1,5 +1,6 @@
 #include "Threading.h"
 #include "Trace.h"
+#include "clang/Basic/Stack.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Threading.h"
@@ -67,16 +68,25 @@
 }
   });
 
-  std::thread(
-  [](std::string Name, decltype(Action) Action, decltype(CleanupTask)) {
-llvm::set_thread_name(Name);
-Action();
-// Make sure function stored by Action is destroyed before CleanupTask
-// is run.
-Action = nullptr;
-  },
-  Name.str(), std::move(Action), std::move(CleanupTask))
-  .detach();
+  // Manually capture Action and CleanupTask for the lack of C++14 generalized
+  // lambda captures
+  struct Callable {
+std::string ThreadName;
+decltype(Action) ThreadFunc;
+decltype(CleanupTask) Cleanup;
+
+void operator()() {
+  llvm::set_thread_name(ThreadName);
+  ThreadFunc();
+  // Make sure function stored by ThreadFunc is destroyed before Cleanup is
+  // run.
+

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

[Dmitry via Phabricator via cfe-commits]

Dmitry.Kozhevnikov added a comment.

In https://reviews.llvm.org/D50993#1212130, @ilya-biryukov wrote:

> @Dmitry.Kozhevnikov, do you need us to land the patch?
>  Any last changes you'd like to make?


It’s blocked by the llvm review, which has some valid comments I’ll address 
today. After that, when both are accepted, I’ll need someone to commit it, yes 
:)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50993



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50455: Continue emitting diagnostics after a fatal error

[Dmitry via Phabricator via cfe-commits]

Dmitry.Kozhevnikov marked an inline comment as done.
Dmitry.Kozhevnikov added a comment.

In https://reviews.llvm.org/D50455#1193468, @ilya-biryukov wrote:

> Nevertheless, I think this is moving clangd in the right direction and if we 
> see too many irrelevant errors, we should look into improving clang to show 
> less of those.


A more conservative fix (demoting "include file not found" from fatal errors) 
was proposed in https://reviews.llvm.org/D50462, I'll try exploring that 
instead.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50462: Try building complete AST after a fatal error was emitted if further diagnostics are expected

[Dmitry via Phabricator via cfe-commits]

Dmitry.Kozhevnikov added a comment.

When lookin through the list of the fatal errors, I think there are different 
categories:

1. "Scary" ones - "module was relocated", "invalid vfs overlay file", we 
probably shouldn't try to recover from it
2. "User" errors (include not found, too many errors) - we definitely shouldn't 
treat them as fatal in IDE
3. (subclass of the previous one): "something is too deep" (instantiations, 
brackets) - I'm afraid treating them as non-fatal right now would lead to a 
possibility of them happening again and again as we recover and proceed. So, in 
the future, the recovery might be more clever (i.e. going all the way up the 
instantiation/brackets stack, and then proceeding normally), however, while 
it's not done, I'm a bit uneasy demoting them from fatal.

So I'm preparing an alternative patch to demote "file not found" in include 
directive from the fatal error in .td file, and then immediately promote it 
back by default (but not in clangd).  WDYT?

In general, I find the whole concept of "Fatal error occurred/Uncompilable 
error occurred/Too many errors occurred/etc" too coarse-grained for 
tooling/IDEs. Parse/PP/Sema should probably try harder to recover, and not 
report such global state change. I'm not ready to approach it, though :)


Repository:
  rC Clang

https://reviews.llvm.org/D50462



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

[Dmitry via Phabricator via cfe-commits]

Dmitry.Kozhevnikov added a comment.

In D50993#1496250 , @ilya-biryukov 
wrote:

> We should definitely land this.
>
> @Dmitry.Kozhevnikov, you don't have commit access, right? Should we land 
> these two revisions for you?


The depending review was never done for the Windows part, also I really don’t 
like how it’ interacting with threading disabled (that’s something I’ve 
realized after submitting the review) - so I’ve abandoned that, sorry. I could 
make an another take next week - or should we wait for D61724 
?


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D50993/new/

https://reviews.llvm.org/D50993



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

[Dmitry via Phabricator via cfe-commits]

Dmitry.Kozhevnikov added a comment.

In D50993#1496946 , @ilya-biryukov 
wrote:

> I'd still put it into LLVM to avoid platform-specific code in clangd. 
>  Maybe `std::abort()` in the added `...Async` function if threads are 
> disabled? It's a bit unusual, but would allow keeping this function where it 
> belongs.


Yes, that’s probably a better way. I’ll be back at it in a few days.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D50993/new/

https://reviews.llvm.org/D50993



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

[Dmitry via Phabricator via cfe-commits]

Dmitry.Kozhevnikov updated this revision to Diff 200432.
Dmitry.Kozhevnikov changed the repository for this revision from rCTE Clang 
Tools Extra to rG LLVM Github Monorepo.
Dmitry.Kozhevnikov added a comment.

moved to the monorepo


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D50993/new/

https://reviews.llvm.org/D50993

Files:
  clang-tools-extra/clangd/Threading.cpp
  clang-tools-extra/clangd/unittests/ClangdTests.cpp


Index: clang-tools-extra/clangd/unittests/ClangdTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdTests.cpp
@@ -1154,6 +1154,27 @@
 Field(&CodeCompletion::Scope, "ns::";
 }
 
+TEST_F(ClangdVFSTest, TestStackOverflow) {
+  MockFSProvider FS;
+  ErrorCheckingDiagConsumer DiagConsumer;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  const auto SourceContents = R"cpp(
+constexpr int foo() { return foo(); }
+static_assert(foo());
+  )cpp";
+
+  auto FooCpp = testPath("foo.cpp");
+  FS.Files[FooCpp] = SourceContents;
+
+  Server.addDocument(FooCpp, SourceContents);
+  ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for diagnostics";
+  // check that we got a constexpr depth error, and not crashed by stack
+  // overflow
+  EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags());
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/Threading.cpp
===
--- clang-tools-extra/clangd/Threading.cpp
+++ clang-tools-extra/clangd/Threading.cpp
@@ -1,5 +1,6 @@
 #include "Threading.h"
 #include "Trace.h"
+#include "clang/Basic/Stack.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Threading.h"
@@ -84,16 +85,25 @@
 }
   });
 
-  std::thread(
-  [](std::string Name, decltype(Action) Action, decltype(CleanupTask)) {
-llvm::set_thread_name(Name);
-Action();
-// Make sure function stored by Action is destroyed before CleanupTask
-// is run.
-Action = nullptr;
-  },
-  Name.str(), std::move(Action), std::move(CleanupTask))
-  .detach();
+  // Manually capture Action and CleanupTask for the lack of C++14 generalized
+  // lambda captures
+  struct Callable {
+std::string ThreadName;
+decltype(Action) ThreadFunc;
+decltype(CleanupTask) Cleanup;
+
+void operator()() {
+  llvm::set_thread_name(ThreadName);
+  ThreadFunc();
+  // Make sure function stored by ThreadFunc is destroyed before Cleanup is
+  // run.
+  ThreadFunc = nullptr;
+}
+  };
+
+  llvm::llvm_execute_on_thread_async(
+  Callable{Name.str(), std::move(Action), std::move(CleanupTask)},
+  clang::DesiredStackSize);
 }
 
 Deadline timeoutSeconds(llvm::Optional Seconds) {


Index: clang-tools-extra/clangd/unittests/ClangdTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdTests.cpp
@@ -1154,6 +1154,27 @@
 Field(&CodeCompletion::Scope, "ns::";
 }
 
+TEST_F(ClangdVFSTest, TestStackOverflow) {
+  MockFSProvider FS;
+  ErrorCheckingDiagConsumer DiagConsumer;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  const auto SourceContents = R"cpp(
+constexpr int foo() { return foo(); }
+static_assert(foo());
+  )cpp";
+
+  auto FooCpp = testPath("foo.cpp");
+  FS.Files[FooCpp] = SourceContents;
+
+  Server.addDocument(FooCpp, SourceContents);
+  ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for diagnostics";
+  // check that we got a constexpr depth error, and not crashed by stack
+  // overflow
+  EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags());
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/Threading.cpp
===
--- clang-tools-extra/clangd/Threading.cpp
+++ clang-tools-extra/clangd/Threading.cpp
@@ -1,5 +1,6 @@
 #include "Threading.h"
 #include "Trace.h"
+#include "clang/Basic/Stack.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Threading.h"
@@ -84,16 +85,25 @@
 }
   });
 
-  std::thread(
-  [](std::string Name, decltype(Action) Action, decltype(CleanupTask)) {
-llvm::set_thread_name(Name);
-Action();
-// Make sure function stored by Action is destroyed before CleanupTask
-// is run.
-Action = nullptr;
-  },
-  Name.str(), std::move(Action), std::move(CleanupTask))
-  .detach();
+  // Manually capture Action and CleanupTask for the lack of C++14 generalized