[PATCH] D115232: [clangd] Indexing of standard library

[Ben Barham via Phabricator via cfe-commits]

bnbarham added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:1008-1010
+  // TUScheduler is the only thing that starts background indexing work.
+  if (IndexTasks && !IndexTasks->wait(timeoutSeconds(TimeoutSeconds)))
+    return false;
----------------
@sammccall shouldn't we also be waiting for this to finish when `ClangdServer` is destroyed? IIUC right now the both `FileIndex` itself (stored in `ClangdServer`) and the actual `UpdateIndexCallbacks` (stored in `TUScheduler`) can be freed while `indexStdlib` is running asynchronously, resulting in a use-after-free on eg. `FIndex->updatePreamble(std::move(IF))`. I was confused as to why this wasn't happening in the tests, but these lines would explain it 😅 

Adding a `IndexTasks->wait()` to `~ClangdServer` fixes the crash I'm seeing in the sourcekit-lsp tests (see https://github.com/apple/llvm-project/pull/5837), though ideally we (sourcekit-lsp) wouldn't be running any indexing at all. As far as I can tell there's no way to turn off dynamic indexing now though, except for `StandardLibrary` indexing through the config file (but not from clangd args)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115232