This increases cardinality of span latency metrics. Currently this was
being shown to the user via file status updates as `Running Update (x)` after
this change we'll only display `Running Update`. This also affects logs in case
of a crash, but contents and version number for inputs are printed separately in
that case already.
Differential Revision: https://reviews.llvm.org/D124013
In some deployments, for example when running on FUSE or using some
network-based VFS implementation, the filesystem operations might add up
to a significant fraction of preamble build time. This change allows us
to track time spent in FS operations to better understand the problem.
Differential Revision: https://reviews.llvm.org/D121712
Auto-generated patch based on clang-tidy readability-identifier-naming.
Only some manual cleanup for `extern "C"` declarations and a GTest change was required.
I'm not sure if this cleanup is actually very useful. It cleans up clang-tidy findings to the number of warnings from clang-tidy should be lower. Since it was easy to do and required only little cleanup I thought I'd upload it for discussion.
One pattern that keeps recurring: Test **matchers** are also supposed to start with a lowercase letter as per LLVM convention. However GTest naming convention for matchers start with upper case. I would propose to keep stay consistent with the GTest convention there. However that would imply a lot of `//NOLINT` throughout these files.
To re-product this patch run:
```
run-clang-tidy -checks="-*,readability-identifier-naming" -fix -format ./clang-tools-extra/clangd
```
To convert the macro names, I was using this script with some manual cleanup afterwards:
https://gist.github.com/ChristianKuehnel/a01cc4362b07c58281554ab46235a077
Differential Revision: https://reviews.llvm.org/D115634
This is a cleanup of all llvm-qualified-auto findings.
This patch was created by automatically applying the fixes from
clang-tidy.
Differential Revision: https://reviews.llvm.org/D113898
This will drop file version information from span names, reducing
overall cardinality and also effect logging when skipping actions in scheduler.
Differential Revision: https://reviews.llvm.org/D113390
Cleanup of clang-tidy findings: removing "else" after a return statement
to improve readability of the code.
This patch was created by applying the clang-tidy fixes automatically.
Differential Revision: https://reviews.llvm.org/D113892
This replaces the test removed in 51be7061d0
It is more principled and tests more critical cases: a crash while parsing.
We need two pieces of plumbing:
- a way to re-enable the crashing #pragmas via a flag, to test parse crashes
- a bit of reshuffling around ASTWorker execution so that we set up the
crash handler in both sync/async modes.
Sync mode is useful for debugging, so I tested both.
Differential Revision: https://reviews.llvm.org/D112565
Motivation:
At the moment it is hard to attribute a clangd crash to a specific request out of all in-flight requests that might be processed concurrently. So before we can act on production clangd crashes, we have to do quite some digging through the log tables populated by our in-house VSCode extension or sometimes even directly reach out to the affected developer. Having all the details needed to reproduce a crash printed alongside its stack trace has a potential to save us quite some time, that could better be spent on fixing the actual problems.
Implementation approach:
* introduce `ThreadCrashReporter` class that allows to set a temporary signal handler for the current thread
* follow RAII pattern to simplify printing context for crashes occurring within a particular scope
* hold `std::function` as a handler to allow capturing context to print
* set local `ThreadCrashReporter` within `JSONTransport::loop()` to print request JSON for main thread crashes, and in `ASTWorker::run()` to print the file paths, arguments and contents for worker thread crashes
`ThreadCrashReporter` currently allows only one active handler per thread, but the approach can be extended to support stacked handlers printing context incrementally.
Example output for main thread crashes:
```
...
#15 0x00007f7ddc819493 __libc_start_main (/lib64/libc.so.6+0x23493)
#16 0x000000000249775e _start (/home/emmablink/local/llvm-project/build/bin/clangd+0x249775e)
Signalled while processing message:
{"jsonrpc": "2.0", "method": "textDocument/didOpen", "params": {"textDocument": {"uri": "file:///home/emmablink/test.cpp", "languageId": "cpp", "version": 1, "text": "template <typename>\nclass Bar {\n Bar<int> *variables_to_modify;\n foo() {\n for (auto *c : *variables_to_modify)\n delete c;\n }\n};\n"}}}
```
Example output for AST worker crashes:
```
...
#41 0x00007fb18304c14a start_thread pthread_create.c:0:0
#42 0x00007fb181bfcdc3 clone (/lib64/libc.so.6+0xfcdc3)
Signalled during AST action:
Filename: test.cpp
Directory: /home/emmablink
Command Line: /usr/bin/clang -resource-dir=/data/users/emmablink/llvm-project/build/lib/clang/14.0.0 -- /home/emmablink/test.cpp
Version: 1
Contents:
template <typename>
class Bar {
Bar<int> *variables_to_modify;
foo() {
for (auto *c : *variables_to_modify)
delete c;
}
};
```
Testing:
The unit test covers the thread-localitity and nesting aspects of `ThreadCrashReporter`. There might be way to set up a lit-based integration test that would spawn clangd, send a message to it, signal it immediately and check the standard output, but this might be prone to raceconditions.
Reviewed By: sammccall
Differential Revision: https://reviews.llvm.org/D109506
Includer cache could get into a bad state when a main file went bad and
added back afterwards. This patch adds a check to invalidate to prevent
that.
Differential Revision: https://reviews.llvm.org/D112130
BlockUntilIdle is supposed to return false if it fails.
If an intermediate step fails to clear the queue, we shouldn't
charge ahead and assert on the state of the queue.
This enables requests like workspaceSymbols to be dispatched using the
file user was most recently operating on. A replacement for D103179.
Differential Revision: https://reviews.llvm.org/D103476
Previously notification of the Server about semantic happened strictly
before notification of the AST thread.
Hence a racy Server could make a request (like semantic tokens) after
the notification, with the assumption that it'll be served fresh
content. But it wasn't true if AST thread wasn't notified about the
change yet.
This change reverses the order of those notifications to prevent racy
interactions.
Differential Revision: https://reviews.llvm.org/D102761
Refactor cross file rename to use a Filesystem instead of a function for getting buffer contents of open files.
Depends on D94554
Reviewed By: sammccall
Differential Revision: https://reviews.llvm.org/D95043
Currently our strategy for getting header compile flags is something like:
A) look for flags for the header in compile_commands.json
This basically never works, build systems don't generate this info.
B) try to match to an impl file in compile_commands.json and use its flags
This only (mostly) works if the headers are in the same project.
C) give up and use fallback flags
This kind of works for stdlib in the default configuration, and
otherwise doesn't.
Obviously there are big gaps here.
This patch inserts a new attempt between A and B: if the header is
transitively included by any open file (whether same project or not),
then we use its compile command.
This doesn't make any attempt to solve some related problems:
- parsing non-self-contained header files in context (importing PP state)
- using the compile flags of non-opened candidate files found in the index
Fixes https://github.com/clangd/clangd/issues/123
Fixes https://github.com/clangd/clangd/issues/695
See https://github.com/clangd/clangd/issues/519
Differential Revision: https://reviews.llvm.org/D97351
This is obsoleted by the standard semanticTokens request family.
As well as the protocol details, this allows us to remove a bunch of plumbing
around pushing highlights to clients.
This should not land until the new protocol has feature parity, see D77702.
Differential Revision: https://reviews.llvm.org/D95576
The new trace event includes what's already in the queue when adding.
For tracers that follow contexts, the trace event will span the time that the action
spends in the queue.
For tracers that follow threads, the trace will be a tiny span on the enqueuing thread.
Differential Revision: https://reviews.llvm.org/D96027
This allows quick tasks without dependencies that
need to run fast to run ASAP. This is mostly useful
for code formatting.
----------------------------
This fixes something that's been annoying me:
- Open your IDE workspace and its 20 open files
- Clangd spends 5 minutes parsing it all
- In the meantime you start to work
- Save a file, trigger format-on-save, which hangs because clangd is busy
- You're stuck waiting until clangd is done parsing your files before the formatting and save takes place
Reviewed By: sammccall
Differential Revision: https://reviews.llvm.org/D94875
Many useful signals can be derived from a valid AST which is regularly updated by
the ASTWorker. `runWithPreamble` does not have access to the ParsedAST
but it can be provided access to some signals derived from a (possibly
stale) AST.
Differential Revision: https://reviews.llvm.org/D94424
clangd actions have various naming schemes, the most
common being PascalCase. This commit applies PascalCase
to all clangd actions, and fix the status rendering
in `renderTUAction` to look more consistent.
Reviewed By: sammccall
Differential Revision: https://reviews.llvm.org/D93546
There's an unfortunate collision between two features:
- we implicitly cancel certain requests when the file changes, to avoid
the queue getting clogged building old revisions to service stale requests
- we "reparse-if-needed" by synthesizing a file change, e.g. on didSave
We could explicitly mark these synthetic requests to avoid this, but
looking for changes in file content clutters our APIs less and is
arguably the correct thing to do in any case.
Fixes https://github.com/clangd/clangd/issues/620
SmallVector<T> with default size is now the recommended version (D92522).
Reviewed By: sammccall
Differential Revision: https://reviews.llvm.org/D92788
Summary:
This is considerably terser than the makeStringError and friends, and
avoids verbosity cliffs that discourage adding log information.
It follows the syntax used in log/elog/vlog/dlog that have been successful.
The main caveats are:
- it's strictly out-of-place in logger.h, though kind of fits thematically and
in implementation
- it claims the "error" identifier, which seems a bit too opinionated
to put higher up in llvm
I've updated some users of StringError mostly at random - there are lots
more mechanical changes but I'd like to get this reviewed before making
them all.
Reviewers: kbobyrev, hokein
Subscribers: mgorny, ilya-biryukov, javed.absar, MaskRay, jkorous, arphaman, kadircet, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D83419
Summary:
I hit this while trying to add a config-over-LSP lit test, which I think
is an appropriate way to test this feature.
That needs a few more changes though...
Reviewers: kadircet
Subscribers: ilya-biryukov, javed.absar, MaskRay, jkorous, arphaman, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D83802
Summary:
ClangdServer owns the question of exactly which config to create, but
TUScheduler/BackgroundIndex control threads and so decide at which point
to inject it.
Reviewers: kadircet
Subscribers: ilya-biryukov, javed.absar, MaskRay, jkorous, arphaman, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D83095
Summary:
Instead of a notification, we make use of a CV and store the boolean on
LatestPreamble by converting it into an optional.
Depends on D80293.
Reviewers: sammccall
Subscribers: ilya-biryukov, javed.absar, MaskRay, jkorous, arphaman, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D80784
Summary: Depends on D80198.
This patch implies ASTs might be built with stale preambles without
blocking for a fresh one. It also drops any guarantees on every preamble
version being built. In case of multiple preamble build requests, in
addition to being debounced.
Any preamble requested with a WantDiags::Yes will always be built, this
is ensured by blocking enqueueing of any subsequent reqest.
AST worker will still block for initial preamble to reduce duplicate
work.
Subscribers: ilya-biryukov, javed.absar, MaskRay, jkorous, arphaman, jfb, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D80293
Summary:
This is achieved by calculating newly added includes and implicitly
parsing them as if they were part of the main file.
This also gets rid of the need for consistent preamble reads.
Reviewers: sammccall
Subscribers: ilya-biryukov, javed.absar, MaskRay, jkorous, mgrang, arphaman, jfb, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D77392
Summary:
We want to be sure they don't cause AST rebuilds or evict items from the cache.
D77847 is going to start sending spurious no-op changes (in case the preamble
was invalidated), this is cheap enough but we shouldn't regress that in future.
Reviewers: kadircet
Subscribers: ilya-biryukov, javed.absar, MaskRay, jkorous, arphaman, jfb, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D78048
Summary:
This assertion was bad. It will show up once we start running preamble
thread async. Think about the following case:
- Update 1
builds a preamble, and an AST. Caches the AST.
- Update 2
Invalidates the cache, preamble hasn't changed.
- Update 3
Invalidates the cache, preamble hasn't changed
- Read
builds AST using preamble v1, and caches it.
preamble for v2 gets build, cache isn't invalidated since preamble is same.
generateDiags tries to reuse cached AST but latest version is 3 not 2, so
assertion fails.
Reviewers: sammccall
Subscribers: ilya-biryukov, javed.absar, MaskRay, jkorous, arphaman, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D77664
Summary:
FileInputs are only written by ASTWorker thread, therefore it is safe
to read them without the lock inside that thread. It can still be read by other
threads through ASTWorker::getCurrentCompileCommand though.
This patch also gets rid of the smart pointer wrapping FileInputs as there is
never mutliple owners.
Reviewers: sammccall
Subscribers: ilya-biryukov, javed.absar, MaskRay, jkorous, arphaman, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D77309
Summary:
This is another step for out-of-order preamble builds. To keep the
diagnostic behavior same, we only build ASTs either with "usable" preambles,
the ones that are fully applicable to a given ParseInput, or after building a
new preamble. Which is the same behaviour as what we do today. ASTs
built in the latter is called golden ASTs.
Reviewers: sammccall
Subscribers: ilya-biryukov, javed.absar, MaskRay, jkorous, arphaman, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D76725
Kadir Cetinkaya