0x1eaf created this revision.
0x1eaf added reviewers: sammccall, ilya-biryukov, nridge.
0x1eaf added projects: clang, clang-tools-extra.
Herald added subscribers: usaxena95, kadircet, arphaman, javed.absar, mgorny.
0x1eaf requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay.
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 `ThreadSignalHandler` 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 `ThreadSignalHandler` 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
`ThreadSignalHandler` 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:
I’m not sure what is the best way to automatically test signal handling and
would welcome suggestions. One way could be to introduce a unit test that would
set up a `ThreadSignalHandler` and just signal itself. Another way might be to
set up a lit test that would spawn clangd, send a message to it, signal it
immediately and check the standard output, although this might be prone to race
conditions.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D109506
Files:
clang-tools-extra/clangd/JSONTransport.cpp
clang-tools-extra/clangd/TUScheduler.cpp
clang-tools-extra/clangd/support/CMakeLists.txt
clang-tools-extra/clangd/support/ThreadSignalHandler.cpp
clang-tools-extra/clangd/support/ThreadSignalHandler.h
Index: clang-tools-extra/clangd/support/ThreadSignalHandler.h
===================================================================
--- /dev/null
+++ clang-tools-extra/clangd/support/ThreadSignalHandler.h
@@ -0,0 +1,31 @@
+//===--- ThreadSignalHandler.h - Thread local signal handling ----*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include <functional>
+
+namespace clang {
+namespace clangd {
+
+using ThreadSignalHandlerCallback = std::function<void(void)>;
+
+class ThreadSignalHandler {
+public:
+ ThreadSignalHandler(const ThreadSignalHandlerCallback &ThreadLocalCallback);
+ ~ThreadSignalHandler();
+
+ ThreadSignalHandler(ThreadSignalHandler &&);
+ ThreadSignalHandler(const ThreadSignalHandler &) = delete;
+ ThreadSignalHandler &operator=(ThreadSignalHandler &&) = delete;
+ ThreadSignalHandler &operator=(const ThreadSignalHandler &) = delete;
+
+private:
+ ThreadSignalHandlerCallback Callback;
+};
+
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/support/ThreadSignalHandler.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/clangd/support/ThreadSignalHandler.cpp
@@ -0,0 +1,50 @@
+//===--- ThreadSignalHandler.cpp - Thread local signal handling --*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "support/ThreadSignalHandler.h"
+#include "llvm/Support/Signals.h"
+#include "llvm/Support/ThreadLocal.h"
+
+using namespace clang::clangd;
+
+static llvm::sys::ThreadLocal<ThreadSignalHandlerCallback> CurrentCallback;
+
+static void registerSignalHandlerIfNeeded() {
+ static llvm::once_flag RegisterOnceFlag;
+ llvm::call_once(RegisterOnceFlag, []() {
+ auto PrintInputs = [](void *Cookie) {
+ ThreadSignalHandlerCallback *Callback = CurrentCallback.get();
+ // TODO: ignore non thread local signals like SIGTERM
+ if (Callback) {
+ (*Callback)();
+ }
+ };
+ llvm::sys::AddSignalHandler(PrintInputs, nullptr);
+ });
+}
+
+ThreadSignalHandler::ThreadSignalHandler(
+ const ThreadSignalHandlerCallback &ThreadLocalCallback)
+ : Callback(ThreadLocalCallback) {
+ assert(CurrentCallback.get() == nullptr);
+ registerSignalHandlerIfNeeded();
+ CurrentCallback.set(&Callback);
+}
+
+ThreadSignalHandler::ThreadSignalHandler(ThreadSignalHandler &&RHS)
+ : Callback(RHS.Callback) {
+ assert(CurrentCallback.get() == &RHS.Callback);
+ RHS.Callback = nullptr;
+ CurrentCallback.set(&Callback);
+}
+
+ThreadSignalHandler::~ThreadSignalHandler() {
+ if (Callback != nullptr) { // haven't been moved
+ CurrentCallback.set(nullptr);
+ }
+}
Index: clang-tools-extra/clangd/support/CMakeLists.txt
===================================================================
--- clang-tools-extra/clangd/support/CMakeLists.txt
+++ clang-tools-extra/clangd/support/CMakeLists.txt
@@ -24,6 +24,7 @@
MemoryTree.cpp
Path.cpp
Shutdown.cpp
+ ThreadSignalHandler.cpp
Threading.cpp
ThreadsafeFS.cpp
Trace.cpp
Index: clang-tools-extra/clangd/TUScheduler.cpp
===================================================================
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -58,6 +58,7 @@
#include "support/Logger.h"
#include "support/MemoryTree.h"
#include "support/Path.h"
+#include "support/ThreadSignalHandler.h"
#include "support/Threading.h"
#include "support/Trace.h"
#include "clang/Frontend/CompilerInvocation.h"
@@ -591,6 +592,8 @@
Deadline scheduleLocked();
/// Should the first task in the queue be skipped instead of run?
bool shouldSkipHeadLocked() const;
+ /// Called by thread local signal handler.
+ void printRequestContextOnSignal() const;
struct Request {
llvm::unique_function<void()> Action;
@@ -1281,6 +1284,8 @@
Status.ASTActivity.Name = CurrentRequest->Name;
});
WithContext WithProvidedContext(ContextProvider(FileName));
+ ThreadSignalHandler ScopedHandler(
+ [this]() { printRequestContextOnSignal(); });
CurrentRequest->Action();
}
@@ -1371,6 +1376,22 @@
llvm_unreachable("Unknown WantDiagnostics");
}
+void ASTWorker::printRequestContextOnSignal() const {
+ auto &OS = llvm::errs();
+ OS << "Signalled during AST action:\n";
+ auto &Command = FileInputs.CompileCommand;
+ OS << "Filename: " << Command.Filename << "\n";
+ OS << "Directory: " << Command.Directory << "\n";
+ OS << "Command Line:";
+ for (auto &Arg : Command.CommandLine) {
+ OS << " " << Arg;
+ }
+ OS << "\n";
+ OS << "Version: " << FileInputs.Version << "\n";
+ OS << "Contents:\n";
+ OS << FileInputs.Contents << "\n";
+}
+
bool ASTWorker::blockUntilIdle(Deadline Timeout) const {
auto WaitUntilASTWorkerIsIdle = [&] {
std::unique_lock<std::mutex> Lock(Mutex);
Index: clang-tools-extra/clangd/JSONTransport.cpp
===================================================================
--- clang-tools-extra/clangd/JSONTransport.cpp
+++ clang-tools-extra/clangd/JSONTransport.cpp
@@ -10,9 +10,11 @@
#include "support/Cancellation.h"
#include "support/Logger.h"
#include "support/Shutdown.h"
+#include "support/ThreadSignalHandler.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/Support/Errno.h"
#include "llvm/Support/Error.h"
+#include "llvm/Support/Threading.h"
#include <system_error>
namespace clang {
@@ -109,6 +111,11 @@
return llvm::errorCodeToError(
std::error_code(errno, std::system_category()));
if (readRawMessage(JSON)) {
+ ThreadSignalHandler ScopedHandler([JSON]() {
+ auto &OS = llvm::errs();
+ OS << "Signalled while processing message:\n";
+ OS << JSON << "\r\n";
+ });
if (auto Doc = llvm::json::parse(JSON)) {
vlog(Pretty ? "<<< {0:2}\n" : "<<< {0}\n", *Doc);
if (!handleMessage(std::move(*Doc), Handler))
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
