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
This adds a new llvm::thread class with the same interface as std::thread
except there is an extra constructor that allows us to set the new thread's
stack size. On Darwin even the default size is boosted to 8MB to match the main
thread.
It also switches all users of the older C-style `llvm_execute_on_thread` API
family over to `llvm::thread` followed by either a `detach` or `join` call and
removes the old API.
Moved definition of DefaultStackSize into the .cpp file to hopefully
fix the build on some (GCC-6?) machines.
This adds a new llvm::thread class with the same interface as std::thread
except there is an extra constructor that allows us to set the new thread's
stack size. On Darwin even the default size is boosted to 8MB to match the main
thread.
It also switches all users of the older C-style `llvm_execute_on_thread` API
family over to `llvm::thread` followed by either a `detach` or `join` call and
removes the old API.
Compiling clangd with Clang modules and libc++ revealed that
`support/Threading.h` uses `std::atomic` but wasn't including the
correct header.
Differential Revision: https://reviews.llvm.org/D105400
For files directly under clangd/, -Iclang-tools-extra/clangd (and the
equivalent for generated files) are not required, as CMake/the compiler puts
these directories on the include path by default.
However this means each subdirectory needs to
include_directories(.. ${CMAKE_CURRENT_BINARY_DIR}/..) etc, and this
proved annoying and error-prone to maintain and debug.
Since include_directories is inherited by subdirectories, we just
configure this explicitly at the top level instead.
Dummy is a word with inappropriate associations. This patch updates the
references to it in clangd code base with more precise ones.
The only user-visible change is the default variable name used when extracting a
variable. It will be named as `placeholder` from now on.
Differential Revision: https://reviews.llvm.org/D99065
Path{Match,Exclude} and MountPoint were checking paths case-sensitively
on all platforms, as with other features, this was causing problems on
windows. Since users can have capital drive letters on config files, but
editors might lower-case them.
This patch addresses that issue by:
- Creating regexes with case-insensitive matching on those platforms.
- Introducing a new pathIsAncestor helper, which performs checks in a
case-correct manner where needed.
Differential Revision: https://reviews.llvm.org/D96690
`std::lock_guard` is an RAII class that needs a variable name whose scope determines the guard's lifetime. This particular usage lacked a variable name, meaning the guard could be destroyed before the line that it was indented to protect.
This line was identified by building clang with the latest MSVC preview release, which declares the std::lock_guard constructor to be `[[nodiscard]]` to draw attention to such issues.
Reviewed By: kadircet
Differential Revision: https://reviews.llvm.org/D95725
Instead of always locking/unlocking a contended mutex, we now do one atomic read
in the common case, and one read + one exchange if the timer has expried.
Also use this for memory profiling which has similar/compatible requirements.
Differential Revision: https://reviews.llvm.org/D93726
SmallVector<T> with default size is now the recommended version (D92522).
Reviewed By: sammccall
Differential Revision: https://reviews.llvm.org/D92788
Allow a `std::unique_ptr` to be moved into the an `IntrusiveRefCntPtr`,
and remove a couple of now-unnecessary `release()` calls.
Differential Revision: https://reviews.llvm.org/D92888
The plan is to use this to use this for .clang-format, .clang-tidy, and
compile_commands.json. (Currently the former two are reparsed every
time, and the latter is cached forever and changes are never seen).
Differential Revision: https://reviews.llvm.org/D88172
The index server has access to potentially-sensitive information, e.g. a
sequence of fuzzyFind requests reveals details about code completions in the
editor which in turn reveals details about the code being edited.
This information is necessary to provide the service, and our intention[1] is it
should never be retained beyond the scope of the request (e.g. not logged).
At the same time, some log messages should be exposed:
- server startup/index reloads etc that don't pertain to a user request
- basic request logs (method, latency, #results, error code) for monitoring
- errors while handling requests, without request-specific data
The -log=public design accommodates these by allowing three types of logs:
- those not associated with any user RPC request (via context-propagation)
- those explicitly tagged as [public] in the log line
- logging of format strings only, with no interpolated data (error level only)
[1] Specifically: Google is likely to run public instances of this server
for LLVM and potentially other projects, they will run in this configuration.
The details agreed in a Google-internal privacy review.
As clangd developers, we'd encourage others to use this configuration for public
instances too.
Differential Revision: https://reviews.llvm.org/D90526
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:
By making all overloads non-virtual and delegating to a differently-named
private method, we avoid any (harmless) name-hiding in the subclasses.
Reviewers: kadircet
Subscribers: kristof.beyls, ilya-biryukov, MaskRay, jkorous, arphaman, usaxena95, cfe-commits, Quuxplusone, dblaikie
Tags: #clang
Differential Revision: https://reviews.llvm.org/D82793
Summary:
Clangd uses FSProvider to get threadsafe views into file systems. This
patch changes naming to make that more explicit.
Depends on D81920
Reviewers: sammccall
Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D81998
Summary:
We've faced a couple of problems when the returned FS didn't have the
proper working directory. New signature makes the API safer against such
problems.
Reviewers: sammccall
Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D81920
Summary:
This fixes a reported bug: if clang and libc++ are installed under
/usr/lib/llvm-11/... but there'- a symlink /usr/bin/clang++-11, then a
compile_commands.json with "/usr/bin/clang++-11 -stdlib=libc++" would previously
look for libc++ under /usr/include instead of /usr/lib/llvm-11/include.
The PATH change makes this work if the compiler is just "clang++-11" too.
As this is now doing IO potentially on every getCompileCommand(), we cache
the results for each distinct driver.
While here:
- Added a Memoize helper for this as multithreaded caching is a bit noisy.
- Used this helper to simplify QueryDriverDatabase and reduce blocking there.
(This makes use of the fact that llvm::Regex is now threadsafe)
Reviewers: kadircet
Subscribers: jyknight, ormris, ilya-biryukov, MaskRay, jkorous, arphaman, jfb, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D75414
It was failing with:
/work/llvm.monorepo/clang-tools-extra/clangd/ClangdServer.cpp: In lambda function:
/work/llvm.monorepo/clang-tools-extra/clangd/ClangdServer.cpp:374:75:
error: could not convert ‘(const char*)""’ from ‘const char*’ to ‘llvm::StringLiteral’
trace::Metric::Distribution);
^
Shoaib Meenai