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

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG69a39dc1f0d0: [clangd] Increase stack size of the new 
threads on macOS (authored by sammccall).

Changed prior to commit:
  https://reviews.llvm.org/D50993?vs=200432=242319#toc

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
@@ -1065,6 +1065,27 @@
 Field(::Scope, "ns::";
 }
 
+TEST_F(ClangdVFSTest, TestStackOverflow) {
+  MockFSProvider FS;
+  ErrorCheckingCallbacks DiagConsumer;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, ClangdServer::optsForTest(), );
+
+  const char *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"
@@ -86,16 +87,16 @@
 }
   });
 
-  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();
+  auto Task = [Name = Name.str(), Action = std::move(Action),
+   Cleanup = std::move(CleanupTask)]() mutable {
+llvm::set_thread_name(Name);
+Action();
+// Make sure function stored by ThreadFunc is destroyed before Cleanup 
runs.
+Action = nullptr;
+  };
+
+  // Ensure our worker threads have big enough stacks to run clang.
+  llvm::llvm_execute_on_thread_async(std::move(Task), 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
@@ -1065,6 +1065,27 @@
 Field(::Scope, "ns::";
 }
 
+TEST_F(ClangdVFSTest, TestStackOverflow) {
+  MockFSProvider FS;
+  ErrorCheckingCallbacks DiagConsumer;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, ClangdServer::optsForTest(), );
+
+  const char *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"
@@ -86,16 +87,16 @@
 }
   });
 
-  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();
+  auto Task = [Name = Name.str(), Action = std::move(Action),
+   Cleanup = std::move(CleanupTask)]() mutable {
+llvm::set_thread_name(Name);
+Action();
+// Make sure function stored by ThreadFunc is destroyed before Cleanup runs.
+ 

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.
Herald added a subscriber: usaxena95.

Sorry we let this last piece get stuck in limbo, of course I was reminded of it 
when someone else hit this problem.

Thanks for working on this @Dmitry.Kozhevnikov, I'll update for c++14 and 
(finally) land it now.


Repository:
  rG LLVM Github Monorepo

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(::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(::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
+  

[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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

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.


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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D50993#1496638 , 
@Dmitry.Kozhevnikov wrote:

> 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 
> ?


D61724  has landed.

`LLVM_ENABLE_THREADS=Off` is now "officially" unsupported in clangd (rL360115 
), so if it's not sensible to have this in 
llvm/Support due to LLVM_ENABLE_THREADS then we can put it in clangd instead.


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#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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

D61724  refactors `BackgroundIndexer` to use 
`AsyncTaskRunner`. 
So by the time this lands, it should cover all places where threads are created 
in clangd.


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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

We should definitely land this.

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


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

[Brennan Vincent via Phabricator via cfe-commits]

umanwizard added a comment.

By the way, index/Background.{cpp,h} also need to be changed to use the new 
API. I made that change locally and clangd with full project indexing is now 
working for me on macOS.


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

[Brennan Vincent via Phabricator via cfe-commits]

umanwizard added a comment.
Herald added a subscriber: dexonsmith.
Herald added a project: clang.

Is there any good reason not to land this? Clangd is crashing for me on macOS 
with stack overflow in the worker threads.


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 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] D50993: [clangd] Increase stack size of the new threads on macOS

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

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


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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.

LGTM. Many thanks!
See the NIT about avoiding the helper class too.




Comment at: clangd/Threading.cpp:88
+  llvm::llvm_execute_on_thread_async(
+  Callable{Name.str(), std::move(Action), std::move(CleanupTask)},
+  clang::DesiredStackSize);

NIT: we usually do the following to avoid writing those classes (not great, but 
arguably a bit less boilerplate):
```

// 'Bind' is in 'Function.h'
Bind([](decltype(ThreadFunc) ThreadFunc, decltype(CleanupTask), std::string 
ThreadName) {
// ... code
},
std::move(Action), std::move(CelanupTask), std::move(ThreadName));
```


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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

This part looks good (I think everything controversial got moved into the other 
patch). Thanks!




Comment at: clangd/Threading.cpp:75
+std::string ThreadName;
+decltype(Action) ThreadFunc;
+decltype(CleanupTask) Cleanup;

nit: maybe call these CallAction and CallCleanupTask or something to make the 
parallels completely explicit. Up to you


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(::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(::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;
+

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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

WRT to the configuration argument, using stack size suggested by @arphaman 
seems like a better option. So let's not add any extra options to clangd.




Comment at: clangd/Threading.cpp:76
+
+  if (::pthread_attr_setstacksize(, 8 * 1024 * 1024) != 0)
+return;

arphaman wrote:
> please use clang::DesiredStackSize instead.
Oh, cool! TIL we have this. Thanks for mentioning it.


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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.



Comment at: clangd/Threading.cpp:76
+
+  if (::pthread_attr_setstacksize(, 8 * 1024 * 1024) != 0)
+return;

please use clang::DesiredStackSize instead.


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

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

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


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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D50993#1207464, @Dmitry.Kozhevnikov wrote:

> 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.


Yeah, we should probably do both Unix and Windows there. My guess is that the 
Windows implementation shouldn't be too hard, since most of the code is already 
there.
In any way, it's fine to start with a Unix-only version and a stub in Windows 
and figuring out the implementation during the review. Happy to look into 
Windows myself, it shouldn't be too hard :-)

>> 2. I wonder if we should expose the stack size as an option to clangd?
> 
> Do you expect it would be ever used?

Not frequently, but I don't see how we can figure out good defaults there. It's 
one of those things where the guideline is "as small as possible, as long as it 
does not crash clangd".
An option could provide a simple workaround for cases when clangd is actually 
crashing.


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#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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

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.
2. I wonder if we should expose the stack size as an option to clangd?




Comment at: clangd/Threading.cpp:57
+template 
+static void executeOnThreadAndDetach(UserFnPtr UserFn, Data UserData) {
+#if !defined(__APPLE__)

Maybe add a parameter for the stack size to this function? (And document it's 
only taken into account when pthread-based implementation is used)



Comment at: clangd/Threading.cpp:57
+template 
+static void executeOnThreadAndDetach(UserFnPtr UserFn, Data UserData) {
+#if !defined(__APPLE__)

ilya-biryukov wrote:
> Maybe add a parameter for the stack size to this function? (And document it's 
> only taken into account when pthread-based implementation is used)
Maybe accept a `llvm::unique_function` instead of `UserFn` and `Data`? 
This split is clearly implementation detail of pthreads-based solution. 
`std::thread` does not need it.


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 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(::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() != 0)
+return;
+
+  auto AttrGuard =
+  llvm::make_scope_exit([&] { ::pthread_attr_destroy(); });
+
+  if (::pthread_attr_setstacksize(, 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(, , 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