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

[Dmitry.Kozhevnikov]

Summary

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.

[ilya-biryukov]

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?

clangd/Threading.cpp

57 ↗

(On Diff #161549)

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)

57 ↗

(On Diff #161549)

Maybe accept a llvm::unique_function<void()> instead of UserFn and Data? This split is clearly implementation detail of pthreads-based solution. std::thread does not need it.

[Dmitry.Kozhevnikov]

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

[ilya-biryukov]

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

[jfb]

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

[arphaman]

clangd/Threading.cpp

76 ↗

(On Diff #161549)

please use clang::DesiredStackSize instead.

[Dmitry.Kozhevnikov]

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

[ilya-biryukov]

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.

clangd/Threading.cpp

76 ↗

(On Diff #161549)

Oh, cool! TIL we have this. Thanks for mentioning it.

[Dmitry.Kozhevnikov]

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

[sammccall]

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

clangd/Threading.cpp

75 ↗

(On Diff #161954)

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

[ilya-biryukov]

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

clangd/Threading.cpp

88 ↗

(On Diff #161954)

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));

[ilya-biryukov]

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

[Dmitry.Kozhevnikov]

In 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 :)

[ilya-biryukov]

In D50993#1212162, @Dmitry.Kozhevnikov wrote:

In 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 :)

Ping us when the prerequisites are ready and this should be merged.
And make sure to request commit access for yourself after someone lands a few patches for you ;-)

[• umanwizard]

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

[• umanwizard]

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.

[ilya-biryukov]

We should definitely land this.

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

[ilya-biryukov]

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

[Dmitry.Kozhevnikov]

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?

[sammccall]

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.

[ilya-biryukov]

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.

[Dmitry.Kozhevnikov]

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.

[Dmitry.Kozhevnikov]

moved to the monorepo

[sammccall]

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.

clang-tools-extra/clangd/Threading.cpp

 #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"
 #include <atomic>
 #include <thread>
 #ifdef __USE_POSIX
 #include <pthread.h>
 #elif defined(__APPLE__)
auto CleanupTask = llvm::make_scope_exit([this]() {
     int NewTasksCnt = --InFlightTasks;
     if (NewTasksCnt == 0) {
       // Note: we can't unlock here because we don't want the object to be
       // destroyed before we notify.
       TasksReachedZero.notify_one();
     }
   });
 
-  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 {
+    // Make sure function stored by ThreadFunc is destroyed before Cleanup runs.
+  };
+
+  // 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<double> Seconds) {
   using namespace std::chrono;
   if (!Seconds)
     return Deadline::infinity();
   return steady_clock::now() +
          duration_cast<steady_clock::duration>(duration<double>(*Seconds));

clang-tools-extra/clangd/unittests/ClangdTests.cpp

TEST_F(ClangdVFSTest, FallbackWhenWaitingForCompileCommand) {
   ASSERT_TRUE(Server.blockUntilIdleForTest());
   EXPECT_THAT(cantFail(runCodeComplete(Server, FooCpp, Code.point(),
                                        clangd::CodeCompleteOptions()))
                   .Completions,
               ElementsAre(AllOf(Field(&CodeCompletion::Name, "xyz"),
                                 Field(&CodeCompletion::Scope, "ns::"))));
 }
 
+TEST_F(ClangdVFSTest, TestStackOverflow) {
+  MockFSProvider FS;
+  ErrorCheckingCallbacks DiagConsumer;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, ClangdServer::optsForTest(), &DiagConsumer);
+
+  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