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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
