[PATCH] D158967: [clang][clangd] Ensure the stack bottom before building AST

[Younan Zhang via Phabricator via cfe-commits]

zyounan added a comment.

Thanks! I hope I'm getting things right. Specifically,

1. `UpdateIndexCallbacks::indexStdlib()::Task` in `ClangdServer.cpp`
2. `PreambleThread::run()`, `ASTWorker::run()` and `TUScheduler::runWithPreamble()::Task` in `TUScheduler.cpp`
3. The lambda of `runAsync` in `BackgroundIndex::BackgroundIndex()` in `index/Background.cpp`.



================
Comment at: clang-tools-extra/clangd/support/Threading.cpp:102
     llvm::set_thread_name(Name);
+    // Mark the bottom of the stack for clang to be aware of the stack usage and
+    // prevent stack overflow.
----------------
sammccall wrote:
> ugh, I forgot: this function is part of clangBasic (which is not small!) and clangdSupport shouldn't depend on clang at all.
> 
> I'm afraid the easiest fix is to move this to the tasks in the relevant callsites:
> - indexStdlib() in ClangdServer.cpp
> - ASTWorker::run(), PreambleWorker::run(), TUScheduler::runWithPreamble() in TUScheduler.cpp
> - BackgroundIndex() in index/Background.cpp
> 
> (I guess principled thing would be to make noteBottomOfStack() part of llvm Support, but that seems complicated)
Ah, I've never noticed clangdSupport is independent of clang.

> (I guess principled thing would be to make noteBottomOfStack() part of llvm Support, but that seems complicated)

Agreed. And I think it requires an RFC before we have that migrate to llvmSupport.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158967