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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D158967#4620931 <https://reviews.llvm.org/D158967#4620931>, @zyounan wrote:

> We had already placed the initialization in ASTFrontendAction::ExecuteAction <https://github.com/llvm/llvm-project/blob/985e399647d591d6130ba6fe08c5b5f6cb87d9f6/clang/lib/Frontend/CompilerInstance.cpp#L1013-L1016>, but we don't have such if we prefer invoking the action outside the libclang. (Just as what we're doing now at the clangd site.)

That's `CompilerInstance::ExecuteAction`, Clangd invokes `ASTFrontendAction::ExecuteAction` and had the `noteBottomOfStack()` been placed there, no change would be needed on the Clangd side.

> I'm not 100% sure if `clang::ParseAST` is appropriate since this might cause a side effect to the global state. Would this break the encapsulation hierarchy?

Could you clarify what kind of encapsulation would it break?
Setting the global variable is suspicious, but the same argument applies for `CompilerInstance::ExecuteAction`.

Pragmatically, it's hard to ensure that `noteBottomOfStack` is called for each Clang tool (I'm sure there is a long tail of tools that don't do that).
Currently the code in `CompilerInstance::ExecuteAction` seems to acknowledge that there should be a fallback. I'm suggesting to move this fallback down to a function that actually runs parsing.
This would ensure the same workaround applies to more tools (including Clangd, IIUC) that don't call `noteBottomOfStack`.
Tools that do call `noteBottomOfStack` will not be affected as the function only remembers the first value for each thread.

Another alternative to enforce the right contract would be to assert that bottom of stack is set when parser and other recursive computations are called.
It's easy enough to achieve, but would be breaking many clients and would be a bigger cleanup.

That being said, it's perfectly reasonable to make sure Clangd calls `noteBottomOfStack` at the right places.
According to documentation of `noteBottomOfStack`, those should be tied to thread creation, i.e. `main` and `AsyncTaskRunner` are two most common entry points for threads that do parsing.
Clangd also creates other threads, but IIUC they are not running any recursive computations, so `noteBottomOfStack` is not strictly necessary there (but shouldn't hurt either).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158967