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

Wed Dec 21 11:09:13 PST 2022

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 wrote:
> bnbarham wrote:
> > sammccall wrote:
> > > bnbarham wrote:
> > > > @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)?
> > > Thanks for flagging this!
> > > 
> > > We *almost* have the sequencing we need in ~ClangdServer:
> > >  - when we fall off the end of ~ClangdServer it destroys all its members
> > >  - `ClangdServer::IndexTasks` is declared after `FIndex`, so is destroyed first
> > >  - ~AsyncTaskRunner calls `wait()`
> > > 
> > > But the task we schedule on `IndexTasks` captures a ref to `UpdateIndexCallbacks`, which is owned by the `TUScheduler`, which we explicitly destroy at the beginning of `~ClangdServer`.
> > > 
> > > However I think your patch is *also* not quite correct: we can wait for the tasks to be empty, but then the TUScheduler could fill it up again before we destroy TUScheduler.
> > > 
> > > Options include adding an explicit stop() to TUScheduler, changing TUScheduler to not (exclusively) own UpdateIndexCallbacks, or have the task not capture the callbacks by reference.
> > > I'll try the latter first, which seems least invasive.
> > > 
> > > ---
> > > 
> > > > 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)?
> > > 
> > > Clangd won't provide any top-level/namespace-level completions at all without dynamic index (index of preambles), and various other features won't work (docs on hover, include-fixer, type/call-hierarchy). We dropped support for disabling this at some point, as it didn't really seem usable and made features more complex if we tried to accommodate it. At a technical level it would be possible to disable I think, but I'd be really surprised if completion worked well, or if a language server without completion was useful.
> > > 
> > > > `StandardLibrary` indexing through the config file (but not from clangd args)
> > > 
> > > We've tried to move away from flags for options that are interesting to users, as config files are more flexible, more forgiving on errors, and allow different settings per-project in a consistent way. (We don't own the editors, so cross-editor consistency is important to being able to support users at all...)
> > > 
> > > I can see how requiring config to be materialized on disk could be inconvenient for IDEs though. I think we could add a general-purpose `--config-inline=<YAML/JSON goes here>` flag, and/or the ability to set config over LSP (this can be dynamic, accordingly bigger design space that might be hard to get right).
> > Ah, I didn't actually check `AsyncTaskRunner`. Makes sense it would wait though :). Thanks for looking into this in detail!
> > 
> > > or have the task not capture the callbacks by reference. I'll try the latter first, which seems least invasive.
> > 
> > This + moving `FIndex` after `IndexTasks` seems reasonable to me.
> > 
> > > Clangd won't provide any top-level/namespace-level completions at all without dynamic index (index of preambles), and various other features won't work (docs on hover, include-fixer, type/call-hierarchy).
> > 
> > That's good to know - I assume this extends to indexing the stdlib as well, ie. the stdlib would be missing from top/namespace level completion if not indexed? Does the dynamic index grow with every opened file, or is it just the currently opened file? If disabling breaks everything it's not something we'd want to do, we just don't need it for find refs/etc.
> > This + moving FIndex after IndexTasks seems reasonable to me.
> 
> I sent D140486. FIndex should be **before** IndexTasks in order to outlive it, unless I'm missing something.
> 
> > I assume this extends to indexing the stdlib as well, ie. the stdlib would be missing from top/namespace level completion if not indexed?
> 
> Any parts of the standard library that you haven't (transitively) included from an open file would be missing. In particular, if you start up clangd, open a blank file, and type `std::` you'll get nothing.
> 
> > Does the dynamic index grow with every opened file, or is it just the currently opened file? 
> 
> It contains symbols for all the transitively included headers of every file you've had open. So it does grow for each opened file, but in practice usually only by a little bit: if you've opened three files usually >90% of the headers visible from the fourth file are already in the index.
> I sent D140486. FIndex should be before IndexTasks in order to outlive it, unless I'm missing something.

Thanks! No, that's right. And it is already so 👍.

And thanks for the extra information too.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115232