[PATCH] D93402: [clang-tidy] prevent readability-identifier-naming from triggering on implicit VarDecls in coroutines
0x1eaf created this revision.
0x1eaf added reviewers: alexfh, njames93, aaron.ballman.
0x1eaf added projects: clang, clang-tools-extra.
Herald added subscribers: lxfind, modocache, xazax.hun.
0x1eaf requested review of this revision.
Herald added a subscriber: cfe-commits.
We have an internal bug report about readability-identifier-naming
warnings on implicit `__coro_gro` and `__promise` variables
in coroutines.
Turns out they haven't been marked as implicit in SemaCoroutine.
This diff resolves that and adds a couroutine test for the check.
Tested with:
> cd build/Debug/bin
> ./llvm-lit -sv --param build_mode=Debug \
../../tools/clang/tools/extra/test \
--filter readability-identifier-naming.cpp
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D93402
Files:
clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-identifier-naming/system/coroutines.h
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
clang/lib/Sema/SemaCoroutine.cpp
Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -544,6 +544,7 @@
auto *VD = VarDecl::Create(Context, FD, FD->getLocation(), FD->getLocation(),
&PP.getIdentifierTable().get("__promise"), T,
Context.getTrivialTypeSourceInfo(T, Loc), SC_None);
+ VD->setImplicit(true);
CheckVariableDeclarationType(VD);
if (VD->isInvalidDecl())
return nullptr;
@@ -1577,6 +1578,7 @@
S.Context, &FD, FD.getLocation(), FD.getLocation(),
&S.PP.getIdentifierTable().get("__coro_gro"), GroType,
S.Context.getTrivialTypeSourceInfo(GroType, Loc), SC_None);
+ GroDecl->setImplicit(true);
S.CheckVariableDeclarationType(GroDecl);
if (GroDecl->isInvalidDecl())
Index: clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
@@ -81,13 +81,14 @@
// RUN: {key: readability-identifier-naming.LocalPointerPrefix, value: 'l_'}, \
// RUN: {key: readability-identifier-naming.LocalConstantPointerCase, value: CamelCase}, \
// RUN: {key: readability-identifier-naming.LocalConstantPointerPrefix, value: 'lc_'}, \
-// RUN: ]}' -- -fno-delayed-template-parsing -Dbad_macro \
+// RUN: ]}' -- -fno-delayed-template-parsing -Dbad_macro -std=c++17 -fcoroutines-ts \
// RUN: -I%S/Inputs/readability-identifier-naming \
// RUN: -isystem %S/Inputs/readability-identifier-naming/system
// clang-format off
#include
+#include
#include "user-header.h"
// NO warnings or fixes expected from declarations within header files without
// the -header-filter= option
@@ -287,7 +288,7 @@
// Overriding a badly-named base isn't a new violation.
void BadBaseMethod() override {}
// CHECK-FIXES: {{^}} void v_Bad_Base_Method() override {}
-
+
void foo() {
BadBaseMethod();
// CHECK-FIXES: {{^}}v_Bad_Base_Method();
@@ -614,3 +615,14 @@
auto GetRes(type_t& Param) -> decltype(Param.res());
// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: invalid case style for parameter 'Param'
// CHECK-FIXES: auto GetRes(type_t& a_param) -> decltype(a_param.res());
+
+#pragma mark - Check implicit declarations in coroutines
+
+struct async_obj {
+public:
+ never_suspend operator co_await() const noexcept;
+};
+
+task ImplicitDeclTest(async_obj &a_object) {
+ co_await a_object; // CHECK-MESSAGES-NOT: warning: invalid case style for local variable
+}
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-identifier-naming/system/coroutines.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-identifier-naming/system/coroutines.h
@@ -0,0 +1,34 @@
+#pragma once
+
+namespace std {
+namespace experimental {
+
+template
+struct coroutine_traits {
+ using promise_type = typename ret_t::promise_type;
+};
+
+template
+struct coroutine_handle {
+ static constexpr coroutine_handle from_address(void *addr) noexcept { return {}; };
+};
+
+} // namespace experimental
+} // namespace std
+
+struct never_suspend {
+ bool await_ready() noexcept { return false; }
+ template
+ void await_suspend(coro_t handle) noexcept {}
+ void await_resume() noexcept {}
+};
+
+struct task {
+ struct promise_type {
+task get_return_object() noexcept { return {}; }
+never_suspend initial_suspend() noexcept { return {}; }
+never_suspend final_suspend() noexcept { return {}; }
+void return_void() {}
+void unhandled_exception() {}
+ };
+};
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/ma
[PATCH] D93402: [clang-tidy] prevent readability-identifier-naming from triggering on implicit VarDecls in coroutines
0x1eaf updated this revision to Diff 312272.
0x1eaf added a comment.
Updated to address review comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93402/new/
https://reviews.llvm.org/D93402
Files:
clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-identifier-naming/system/coroutines.h
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
clang/lib/Sema/SemaCoroutine.cpp
Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -544,6 +544,7 @@
auto *VD = VarDecl::Create(Context, FD, FD->getLocation(), FD->getLocation(),
&PP.getIdentifierTable().get("__promise"), T,
Context.getTrivialTypeSourceInfo(T, Loc), SC_None);
+ VD->setImplicit();
CheckVariableDeclarationType(VD);
if (VD->isInvalidDecl())
return nullptr;
@@ -1577,6 +1578,7 @@
S.Context, &FD, FD.getLocation(), FD.getLocation(),
&S.PP.getIdentifierTable().get("__coro_gro"), GroType,
S.Context.getTrivialTypeSourceInfo(GroType, Loc), SC_None);
+ GroDecl->setImplicit();
S.CheckVariableDeclarationType(GroDecl);
if (GroDecl->isInvalidDecl())
Index: clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
@@ -81,13 +81,14 @@
// RUN: {key: readability-identifier-naming.LocalPointerPrefix, value: 'l_'}, \
// RUN: {key: readability-identifier-naming.LocalConstantPointerCase, value: CamelCase}, \
// RUN: {key: readability-identifier-naming.LocalConstantPointerPrefix, value: 'lc_'}, \
-// RUN: ]}' -- -fno-delayed-template-parsing -Dbad_macro \
+// RUN: ]}' -- -fno-delayed-template-parsing -Dbad_macro -std=c++17 -fcoroutines-ts \
// RUN: -I%S/Inputs/readability-identifier-naming \
// RUN: -isystem %S/Inputs/readability-identifier-naming/system
// clang-format off
#include
+#include
#include "user-header.h"
// NO warnings or fixes expected from declarations within header files without
// the -header-filter= option
@@ -287,7 +288,7 @@
// Overriding a badly-named base isn't a new violation.
void BadBaseMethod() override {}
// CHECK-FIXES: {{^}} void v_Bad_Base_Method() override {}
-
+
void foo() {
BadBaseMethod();
// CHECK-FIXES: {{^}}v_Bad_Base_Method();
@@ -614,3 +615,14 @@
auto GetRes(type_t& Param) -> decltype(Param.res());
// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: invalid case style for parameter 'Param'
// CHECK-FIXES: auto GetRes(type_t& a_param) -> decltype(a_param.res());
+
+// Check implicit declarations in coroutines
+
+struct async_obj {
+public:
+ never_suspend operator co_await() const noexcept;
+};
+
+task ImplicitDeclTest(async_obj &a_object) {
+ co_await a_object; // CHECK-MESSAGES-NOT: warning: invalid case style for local variable
+}
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-identifier-naming/system/coroutines.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-identifier-naming/system/coroutines.h
@@ -0,0 +1,34 @@
+#pragma once
+
+namespace std {
+namespace experimental {
+
+template
+struct coroutine_traits {
+ using promise_type = typename ret_t::promise_type;
+};
+
+template
+struct coroutine_handle {
+ static constexpr coroutine_handle from_address(void *addr) noexcept { return {}; };
+};
+
+} // namespace experimental
+} // namespace std
+
+struct never_suspend {
+ bool await_ready() noexcept { return false; }
+ template
+ void await_suspend(coro_t handle) noexcept {}
+ void await_resume() noexcept {}
+};
+
+struct task {
+ struct promise_type {
+task get_return_object() noexcept { return {}; }
+never_suspend initial_suspend() noexcept { return {}; }
+never_suspend final_suspend() noexcept { return {}; }
+void return_void() {}
+void unhandled_exception() {}
+ };
+};
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D93402: [clang-tidy] prevent readability-identifier-naming from triggering on implicit VarDecls in coroutines
0x1eaf added a comment. > Do you need me to commit on your behalf? Yes, I don't have commit access. The attribution line looks good to me. Thank you! 🙂 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93402/new/ https://reviews.llvm.org/D93402 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D109506: [clangd] Print current request context along with the stack trace
0x1eaf added a comment. I think this should work-around end-to-end test failure on Windows: diff --git a/clang-tools-extra/clangd/test/crash.test b/clang-tools-extra/clangd/test/crash.test index 1197e1ab07c3..68ef54808f09 100644 --- a/clang-tools-extra/clangd/test/crash.test +++ b/clang-tools-extra/clangd/test/crash.test @@ -1,3 +1,4 @@ +# REQUIRES: shell # Overflow the recursive json parser, prevent `yes` error due to broken pipe and `clangd` SIGSEGV from being treated as a failure. # RUN: (yes '[' || :) | head -n 5 | (clangd --input-style=delimited 2>&1 || :) | FileCheck %s # CHECK: Signalled while processing message: Alternatively, the test can be completely disabled for the time being. I don't have commit access, should I create a new revision for review with the test change? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109506/new/ https://reviews.llvm.org/D109506 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D109506: [clangd] Print current request context along with the stack trace
0x1eaf updated this revision to Diff 378257.
0x1eaf added a comment.
Addressed feedback from the last round of comments. Sorry for the delay!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D109506/new/
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/ThreadCrashReporter.cpp
clang-tools-extra/clangd/support/ThreadCrashReporter.h
clang-tools-extra/clangd/test/crash.test
clang-tools-extra/clangd/tool/ClangdMain.cpp
clang-tools-extra/clangd/unittests/CMakeLists.txt
clang-tools-extra/clangd/unittests/ThreadCrashReporterTests.cpp
Index: clang-tools-extra/clangd/unittests/ThreadCrashReporterTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/ThreadCrashReporterTests.cpp
@@ -0,0 +1,78 @@
+///===- ThreadCrashReporterTests.cpp - Thread local signal handling tests -===//
+//
+// 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/ThreadCrashReporter.h"
+#include "support/Threading.h"
+#include "llvm/Support/Signals.h"
+#include "gtest/gtest.h"
+#include
+#include
+
+namespace clang {
+namespace clangd {
+
+namespace {
+
+static void infoSignalHandler() { ThreadCrashReporter::runCrashHandlers(); }
+
+TEST(ThreadCrashReporterTest, All) {
+#if defined(_WIN32)
+ // Simulate signals on Windows for unit testing purposes.
+ // The `crash.test` lit test checks the end-to-end integration.
+ auto SignalCurrentThread = []() { infoSignalHandler(); };
+#else
+ llvm::sys::SetInfoSignalFunction(&infoSignalHandler);
+ auto SignalCurrentThread = []() { raise(SIGUSR1); };
+#endif
+
+ AsyncTaskRunner Runner;
+ auto SignalAnotherThread = [&]() {
+Runner.runAsync("signal another thread", SignalCurrentThread);
+Runner.wait();
+ };
+
+ bool Called;
+ {
+ThreadCrashReporter ScopedReporter([&Called]() { Called = true; });
+// Check handler gets called when a signal gets delivered to the current
+// thread.
+Called = false;
+SignalCurrentThread();
+EXPECT_TRUE(Called);
+
+// Check handler does not get called when another thread gets signalled.
+Called = false;
+SignalAnotherThread();
+EXPECT_FALSE(Called);
+ }
+ // Check handler does not get called when the reporter object goes out of
+ // scope.
+ Called = false;
+ SignalCurrentThread();
+ EXPECT_FALSE(Called);
+
+ std::string Order = "";
+ {
+ThreadCrashReporter ScopedReporter([&Order] { Order.push_back('a'); });
+{
+ ThreadCrashReporter ScopedReporter([&Order] { Order.push_back('b'); });
+ SignalCurrentThread();
+}
+// Check that handlers are called in LIFO order.
+EXPECT_EQ(Order, "ba");
+
+// Check that current handler is the only one after the nested scope is
+// over.
+SignalCurrentThread();
+EXPECT_EQ(Order, "baa");
+ }
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/unittests/CMakeLists.txt
===
--- clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -88,6 +88,7 @@
TestIndex.cpp
TestTU.cpp
TestWorkspace.cpp
+ ThreadCrashReporterTests.cpp
TidyProviderTests.cpp
TypeHierarchyTests.cpp
URITests.cpp
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -24,6 +24,7 @@
#include "refactor/Rename.h"
#include "support/Path.h"
#include "support/Shutdown.h"
+#include "support/ThreadCrashReporter.h"
#include "support/ThreadsafeFS.h"
#include "support/Trace.h"
#include "clang/Format/Format.h"
@@ -679,6 +680,8 @@
llvm::InitializeAllTargetInfos();
llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
+ llvm::sys::AddSignalHandler(
+ [](void *) { ThreadCrashReporter::runCrashHandlers(); }, nullptr);
llvm::sys::SetInterruptFunction(&requestShutdown);
llvm::cl::SetVersionPrinter([](llvm::raw_ostream &OS) {
OS << versionString() << "\n"
Index: clang-tools-extra/clangd/test/crash.test
===
--- /dev/null
+++ clang-tools-extra/clangd/test/crash.test
@@ -0,0 +1,5 @@
+# Overflow the recursive json parser, prevent `yes` error due to broken pipe and `clangd` SIGSEGV from being treated as a failure.
+# RUN: (yes '[' || :) | head -n 5 | (clangd --input-style=delimited 2>&1 || :) | FileCheck %s
+# CHECK: Sig
[PATCH] D109506: [clangd] Print current request context along with the stack trace
0x1eaf marked 9 inline comments as done. 0x1eaf added a comment. I've opted for simulating signals in the unit test on Windows by manually calling the signal handler, and added a list test to test the crash handling integration end-to-end. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109506/new/ https://reviews.llvm.org/D109506 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D109506: [clangd] Print current request context along with the stack trace
0x1eaf updated this revision to Diff 379124.
0x1eaf added a comment.
Updated the AST worker crash handler to avoid `std::string` copy: used a char
pointer instead.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D109506/new/
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/ThreadCrashReporter.cpp
clang-tools-extra/clangd/support/ThreadCrashReporter.h
clang-tools-extra/clangd/test/crash.test
clang-tools-extra/clangd/tool/ClangdMain.cpp
clang-tools-extra/clangd/unittests/CMakeLists.txt
clang-tools-extra/clangd/unittests/ThreadCrashReporterTests.cpp
Index: clang-tools-extra/clangd/unittests/ThreadCrashReporterTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/ThreadCrashReporterTests.cpp
@@ -0,0 +1,78 @@
+///===- ThreadCrashReporterTests.cpp - Thread local signal handling tests -===//
+//
+// 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/ThreadCrashReporter.h"
+#include "support/Threading.h"
+#include "llvm/Support/Signals.h"
+#include "gtest/gtest.h"
+#include
+#include
+
+namespace clang {
+namespace clangd {
+
+namespace {
+
+static void infoSignalHandler() { ThreadCrashReporter::runCrashHandlers(); }
+
+TEST(ThreadCrashReporterTest, All) {
+#if defined(_WIN32)
+ // Simulate signals on Windows for unit testing purposes.
+ // The `crash.test` lit test checks the end-to-end integration.
+ auto SignalCurrentThread = []() { infoSignalHandler(); };
+#else
+ llvm::sys::SetInfoSignalFunction(&infoSignalHandler);
+ auto SignalCurrentThread = []() { raise(SIGUSR1); };
+#endif
+
+ AsyncTaskRunner Runner;
+ auto SignalAnotherThread = [&]() {
+Runner.runAsync("signal another thread", SignalCurrentThread);
+Runner.wait();
+ };
+
+ bool Called;
+ {
+ThreadCrashReporter ScopedReporter([&Called]() { Called = true; });
+// Check handler gets called when a signal gets delivered to the current
+// thread.
+Called = false;
+SignalCurrentThread();
+EXPECT_TRUE(Called);
+
+// Check handler does not get called when another thread gets signalled.
+Called = false;
+SignalAnotherThread();
+EXPECT_FALSE(Called);
+ }
+ // Check handler does not get called when the reporter object goes out of
+ // scope.
+ Called = false;
+ SignalCurrentThread();
+ EXPECT_FALSE(Called);
+
+ std::string Order = "";
+ {
+ThreadCrashReporter ScopedReporter([&Order] { Order.push_back('a'); });
+{
+ ThreadCrashReporter ScopedReporter([&Order] { Order.push_back('b'); });
+ SignalCurrentThread();
+}
+// Check that handlers are called in LIFO order.
+EXPECT_EQ(Order, "ba");
+
+// Check that current handler is the only one after the nested scope is
+// over.
+SignalCurrentThread();
+EXPECT_EQ(Order, "baa");
+ }
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/unittests/CMakeLists.txt
===
--- clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -88,6 +88,7 @@
TestIndex.cpp
TestTU.cpp
TestWorkspace.cpp
+ ThreadCrashReporterTests.cpp
TidyProviderTests.cpp
TypeHierarchyTests.cpp
URITests.cpp
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -24,6 +24,7 @@
#include "refactor/Rename.h"
#include "support/Path.h"
#include "support/Shutdown.h"
+#include "support/ThreadCrashReporter.h"
#include "support/ThreadsafeFS.h"
#include "support/Trace.h"
#include "clang/Format/Format.h"
@@ -679,6 +680,8 @@
llvm::InitializeAllTargetInfos();
llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
+ llvm::sys::AddSignalHandler(
+ [](void *) { ThreadCrashReporter::runCrashHandlers(); }, nullptr);
llvm::sys::SetInterruptFunction(&requestShutdown);
llvm::cl::SetVersionPrinter([](llvm::raw_ostream &OS) {
OS << versionString() << "\n"
Index: clang-tools-extra/clangd/test/crash.test
===
--- /dev/null
+++ clang-tools-extra/clangd/test/crash.test
@@ -0,0 +1,5 @@
+# Overflow the recursive json parser, prevent `yes` error due to broken pipe and `clangd` SIGSEGV from being treated as a failure.
+# RUN: (yes '[' || :) | head -n 5 | (clangd --input-style=delimited 2>&1 || :) | FileChec
[PATCH] D109506: [clangd] Print current request context along with the stack trace
0x1eaf marked an inline comment as done. 0x1eaf added a comment. I don't have commit access, so would appreciate if you commit it, thanks! Forgot to mention on Friday: I've instrumented preamble building, but haven't instrumented background indexing which is outside of `TUScheduler.cpp` and would require exposing `crashDump*` helpers and preferably moving them out of `TUScheduler.cpp`. And I think it might be easier to review it separately. Speaking of the helpers, where would you prefer those to be: a new file+header under clangd/support, or in some existing place? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109506/new/ https://reviews.llvm.org/D109506 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D109506: [clangd] Print current request context along with the stack trace
0x1eaf added a comment. Are there any blockers to landing this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109506/new/ https://reviews.llvm.org/D109506 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D109506: [RFC] Print current request context along with the stack trance in clangd
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 0x7f7ddc819493 __libc_start_main (/lib64/libc.so.6+0x23493)
#16 0x0249775e _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 \nclass Bar {\n Bar
*variables_to_modify;\n foo() {\nfor (auto *c : *variables_to_modify)\n
delete c;\n }\n};\n"}}}
Example output for AST worker crashes:
...
#41 0x7fb18304c14a start_thread pthread_create.c:0:0
#42 0x7fb181bfcdc3 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
class Bar {
Bar *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
+
+namespace clang {
+namespace clangd {
+
+using ThreadSignalHandlerCallback = std::function;
+
+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
[PATCH] D109506: [RFC] Print current request context along with the stack trance in clangd
0x1eaf added a comment.
Thank you for such a prompt and thorough review!
Please consider all the comments I haven't replied to acknowledged and I'm
going to resolve the issues with the next update.
Comment at: clang-tools-extra/clangd/JSONTransport.cpp:117
+ OS << "Signalled while processing message:\n";
+ OS << JSON << "\r\n";
+});
sammccall wrote:
> why \r?
the message itself is terminated with `\r\n` in the protocol and I just
mimicked it for no good reason — will correct
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1381
+ auto &OS = llvm::errs();
+ OS << "Signalled during AST action:\n";
+ auto &Command = FileInputs.CompileCommand;
sammccall wrote:
> the name of the action is available as CurrentRequest->Action, which I think
> is safe to either pass into this function or read directly here.
I've noticed the the action name is always hardcoded as “Build AST ({version})”
and just logged the version directly below, but I guess it's not forward
compatible, so I'm going to log both…
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1392
+ OS << "Contents:\n";
+ OS << FileInputs.Contents << "\n";
+}
sammccall wrote:
> sammccall wrote:
> > hmm, this is potentially really useful (could gather full reproducers) but
> > also is going to dominate the output and make it annoying to see the stack
> > traces/other details. I'm worried this may make this feature net-negative
> > for a majority of users, who won't find the right parts of the output to
> > report...
> >
> > How critical do you think this is for debugging crashes in practice? (I
> > suspect fairly important!)
> > Since we're taking liberties with async-signal-safety anyway, it might be
> > possible to get this sent to a tempfile, which might be more useful. In any
> > case, we might want full file dumps to be off by default outside of
> > environments where anyone is likely to actually try to get the reproducers.
> >
> > I'd probably suggest leaving this out of the initial patch entirely so we
> > can focus on getting the rest of the mechanism right.
> another thought along the lines of reproducers (but nothing for this patch)...
>
> If we're working on a file and have a preamble, then the headers that make up
> the preamble are relevant. In practice it's hard to reproduce bug reports we
> get because we need all their headers. If we're using a preamble then just
> dumping all the filenames it includes would make it possible for some tool to
> take the crash report and zip up a reproducer.
>
> (This probably needs the ability to run all of the handlers for the current
> thread, not just the top of the stack, since we acquire the relevant preamble
> later)
> How critical do you think this is for debugging crashes in practice?
For us it would likely be of utmost importance as our VSCode extension routes
requests to [Glean](https://glean.software) until local clangd finishes the
compilation, so clangd ends up mostly being used for modified files, and the
crashes might be unreproducible on the original source…
> I'd probably suggest leaving this out of the initial patch entirely so we can
> focus on getting the rest of the mechanism right.
Alright, will remove the file contents with the next update.
> In any case, we might want full file dumps to be off by default outside of
> environments where anyone is likely to actually try to get the reproducers.
What would be the best way to re-add it in a subsequent revision: make it
configurable via a command line option, or via an environment variable? And if
it's just an env var, maybe we could include it conditionally in this revision?
> If we're using a preamble then just dumping all the filenames it includes
> would make it possible for some tool to take the crash report and zip up a
> reproducer.
We had discussions within the team about potentially implementing a similar
tool, but the rough idea was to rely on VCS to collect the changes as the delta
would normally be smaller than bundling all the headers that the TU includes.
But if the VCS-agnostic approach would be simpler and more portable — I'm all
for it :)
Comment at: clang-tools-extra/clangd/support/ThreadSignalHandler.cpp:15
+
+static llvm::sys::ThreadLocal CurrentCallback;
+
sammccall wrote:
> sammccall wrote:
> > sammccall wrote:
> > > we assume `thread_local` in clangd (see support/Context.ccp)
> > >
> > > It's possible that llvm::sys::ThreadLocal happens to play nicer with
> > > signal handlers in practice (none of these are completely safe!), but
> > > unless we've seen some concrete evidence I'd rather just have `static
> > > thread_local ThreadSignalHandlerCallback*` here
> > we can quite easily support a stack of handlers, such that we dump *all*
> > living handlers on the crashing thread.
> >
> > just give e
[PATCH] D109506: [RFC] Print current request context along with the stack trance in clangd
0x1eaf added inline comments.
Comment at: clang-tools-extra/clangd/support/ThreadSignalHandler.cpp:22
+ ThreadSignalHandlerCallback *Callback = CurrentCallback.get();
+ // TODO: ignore non thread local signals like SIGTERM
+ if (Callback) {
sammccall wrote:
> I think we should do this already, probably just starting with an allowlist
> of `SIGBUS, SIGFPE, SIGILL, SIGSEGV` (these are signals that are delivered to
> the thread that triggered them per the linux docs).
>
> I'm also wondering how portable this assumption/mechanism is, e.g. whether we
> should enable it for certain platforms only. I guess it's probably true for
> unixy things, and apparently mostly true for windows too[1]? I'm a little
> concerned *something* may go wrong on windows, though.
>
> [1]
> https://stackoverflow.com/questions/15879342/which-thread-are-signal-handlers-e-g-signalsigint-crtl-c-called-on
I've just looked into adding filtering by signal number and there doesn't seem
to be a way to do this portably in LLVM. But it looks like it might be safe
enough to skip the filtering at all:
* From a cursory look at Windows `Signals.inc` it looks like the “signal”
handling is implemented via
[SetUnhandledExceptionFilter()](https://docs.microsoft.com/en-us/windows/win32/api/errhandlingapi/nf-errhandlingapi-setunhandledexceptionfilter)
which is thread-directed, and the `SetConsoleCtrlHandler()` callback [does not
call](https://reviews.llvm.org/source/llvm-github/browse/main/llvm/lib/Support/Windows/Signals.inc$843-846)
the registered handlers.
* And on Unix platforms the `AddSignalHandler()` callbacks are called
[only](https://reviews.llvm.org/source/llvm-github/browse/main/llvm/include/llvm/Support/Signals.h$62-64)
for
[KillSigs](https://reviews.llvm.org/source/llvm-github/browse/main/llvm/lib/Support/Unix/Signals.inc$217)
out of which only `SIGQUIT` seem to be process-directed and would be delivered
to [any thread](https://man7.org/linux/man-pages/man7/signal.7.html) that is
not blocking it, but if the thread gets delivered to has a
`ThreadCrashReporter` installed during the interrupt — it seems to make sense
to run the handler and let it print the thread-local context information (at
least there seems to be no harm in doing so).
What do you think?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D109506/new/
https://reviews.llvm.org/D109506
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D109772: [clangd] Print current request context along with the stack trace
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 `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 0x7f7ddc819493 __libc_start_main (/lib64/libc.so.6+0x23493)
#16 0x0249775e _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 \nclass Bar {\n Bar
*variables_to_modify;\n foo() {\nfor (auto *c : *variables_to_modify)\n
delete c;\n }\n};\n"}}}
Example output for AST worker crashes:
...
#41 0x7fb18304c14a start_thread pthread_create.c:0:0
#42 0x7fb181bfcdc3 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
class Bar {
Bar *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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D109772
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/ThreadCrashReporter.cpp
clang-tools-extra/clangd/support/ThreadCrashReporter.h
clang-tools-extra/clangd/tool/ClangdMain.cpp
clang-tools-extra/clangd/unittests/CMakeLists.txt
clang-tools-extra/clangd/unittests/ThreadCrashReporterTests.cpp
Index: clang-tools-extra/clangd/unittests/ThreadCrashReporterTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/ThreadCrashReporterTests.cpp
@@ -0,0 +1,71 @@
+///===- ThreadCrashReporterTests.cpp - Thread local signal handling tests -===//
+//
+// 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/ThreadCrashReporter.h"
+#include "support/Threading.h"
+#include "llvm/Support/Signals.h"
+#include "gtest/gtest.h"
+#include
+#include
+
+namespace clang {
+namespace clangd {
+
+namespace {
+
+static void infoHandler() { ThreadCrashReporter::runCrashHandlers(nullptr); }
+
+TEST(ThreadCrashReporterTest, All) {
+ llvm::sys::SetInfoSignalFunction(&infoHandler);
+ AsyncTaskRunner Runner;
+ auto SignalCurrentThread = []() { raise(SIGUSR1); };
+ auto SignalAnotherThread = [&]() {
+Runner.runAsync("signal another thread", SignalCurrentThread);
+Runner.wait();
+ };
+
+ bool Called;
+ {
+ThreadCrashReporter ScopedReporter([&Called]() { Called = true; });
+// Check handler gets called when a signal gets delivered to the current
+// thread.
+Called = false;
+SignalCurrentThread();
+EXPECT_TRUE(Called);
+
+// Check handler does not get call
[PATCH] D109506: [RFC] Print current request context along with the stack trance in clangd
0x1eaf updated this revision to Diff 372510.
0x1eaf added a comment.
addressed review comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D109506/new/
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/ThreadCrashReporter.cpp
clang-tools-extra/clangd/support/ThreadCrashReporter.h
clang-tools-extra/clangd/tool/ClangdMain.cpp
clang-tools-extra/clangd/unittests/CMakeLists.txt
clang-tools-extra/clangd/unittests/ThreadCrashReporterTests.cpp
Index: clang-tools-extra/clangd/unittests/ThreadCrashReporterTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/ThreadCrashReporterTests.cpp
@@ -0,0 +1,71 @@
+///===- ThreadCrashReporterTests.cpp - Thread local signal handling tests -===//
+//
+// 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/ThreadCrashReporter.h"
+#include "support/Threading.h"
+#include "llvm/Support/Signals.h"
+#include "gtest/gtest.h"
+#include
+#include
+
+namespace clang {
+namespace clangd {
+
+namespace {
+
+static void infoHandler() { ThreadCrashReporter::runCrashHandlers(nullptr); }
+
+TEST(ThreadCrashReporterTest, All) {
+ llvm::sys::SetInfoSignalFunction(&infoHandler);
+ AsyncTaskRunner Runner;
+ auto SignalCurrentThread = []() { raise(SIGUSR1); };
+ auto SignalAnotherThread = [&]() {
+Runner.runAsync("signal another thread", SignalCurrentThread);
+Runner.wait();
+ };
+
+ bool Called;
+ {
+ThreadCrashReporter ScopedReporter([&Called]() { Called = true; });
+// Check handler gets called when a signal gets delivered to the current
+// thread.
+Called = false;
+SignalCurrentThread();
+EXPECT_TRUE(Called);
+
+// Check handler does not get called when another thread gets signalled.
+Called = false;
+SignalAnotherThread();
+EXPECT_FALSE(Called);
+ }
+ // Check handler does not get called when the reporter object goes out of
+ // scope.
+ Called = false;
+ SignalCurrentThread();
+ EXPECT_FALSE(Called);
+
+ std::string Order = "";
+ {
+ThreadCrashReporter ScopedReporter([&Order] { Order.push_back('a'); });
+{
+ ThreadCrashReporter ScopedReporter([&Order] { Order.push_back('b'); });
+ SignalCurrentThread();
+}
+// Check that handlers are called in FIFO order.
+EXPECT_EQ(Order, "ab");
+
+// Check that current handler is the only one after the nested scope is
+// over.
+SignalCurrentThread();
+EXPECT_EQ(Order, "aba");
+ }
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/unittests/CMakeLists.txt
===
--- clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -88,6 +88,7 @@
TestIndex.cpp
TestTU.cpp
TestWorkspace.cpp
+ ThreadCrashReporterTests.cpp
TidyProviderTests.cpp
TypeHierarchyTests.cpp
URITests.cpp
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -24,6 +24,7 @@
#include "refactor/Rename.h"
#include "support/Path.h"
#include "support/Shutdown.h"
+#include "support/ThreadCrashReporter.h"
#include "support/ThreadsafeFS.h"
#include "support/Trace.h"
#include "clang/Format/Format.h"
@@ -679,6 +680,7 @@
llvm::InitializeAllTargetInfos();
llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
+ llvm::sys::AddSignalHandler(&ThreadCrashReporter::runCrashHandlers, nullptr);
llvm::sys::SetInterruptFunction(&requestShutdown);
llvm::cl::SetVersionPrinter([](llvm::raw_ostream &OS) {
OS << versionString() << "\n"
Index: clang-tools-extra/clangd/support/ThreadCrashReporter.h
===
--- /dev/null
+++ clang-tools-extra/clangd/support/ThreadCrashReporter.h
@@ -0,0 +1,48 @@
+//===--- ThreadCrashReporter.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
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_SUPPORT_THREAD_CRASH_REPORTER_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_SUPPORT_THREAD_CRASH_REPORTER_H
+
+#include
+
+namespace clang {
+namespace cl
[PATCH] D109506: [clangd] Print current request context along with the stack trace
0x1eaf updated this revision to Diff 372515.
0x1eaf retitled this revision from "[RFC] Print current request context along
with the stack trance in clangd" to "[clangd] Print current request context
along with the stack trace".
0x1eaf edited the summary of this revision.
0x1eaf added a comment.
updated revision description for final review after renaming the class
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D109506/new/
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/ThreadCrashReporter.cpp
clang-tools-extra/clangd/support/ThreadCrashReporter.h
clang-tools-extra/clangd/tool/ClangdMain.cpp
clang-tools-extra/clangd/unittests/CMakeLists.txt
clang-tools-extra/clangd/unittests/ThreadCrashReporterTests.cpp
Index: clang-tools-extra/clangd/unittests/ThreadCrashReporterTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/ThreadCrashReporterTests.cpp
@@ -0,0 +1,71 @@
+///===- ThreadCrashReporterTests.cpp - Thread local signal handling tests -===//
+//
+// 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/ThreadCrashReporter.h"
+#include "support/Threading.h"
+#include "llvm/Support/Signals.h"
+#include "gtest/gtest.h"
+#include
+#include
+
+namespace clang {
+namespace clangd {
+
+namespace {
+
+static void infoHandler() { ThreadCrashReporter::runCrashHandlers(nullptr); }
+
+TEST(ThreadCrashReporterTest, All) {
+ llvm::sys::SetInfoSignalFunction(&infoHandler);
+ AsyncTaskRunner Runner;
+ auto SignalCurrentThread = []() { raise(SIGUSR1); };
+ auto SignalAnotherThread = [&]() {
+Runner.runAsync("signal another thread", SignalCurrentThread);
+Runner.wait();
+ };
+
+ bool Called;
+ {
+ThreadCrashReporter ScopedReporter([&Called]() { Called = true; });
+// Check handler gets called when a signal gets delivered to the current
+// thread.
+Called = false;
+SignalCurrentThread();
+EXPECT_TRUE(Called);
+
+// Check handler does not get called when another thread gets signalled.
+Called = false;
+SignalAnotherThread();
+EXPECT_FALSE(Called);
+ }
+ // Check handler does not get called when the reporter object goes out of
+ // scope.
+ Called = false;
+ SignalCurrentThread();
+ EXPECT_FALSE(Called);
+
+ std::string Order = "";
+ {
+ThreadCrashReporter ScopedReporter([&Order] { Order.push_back('a'); });
+{
+ ThreadCrashReporter ScopedReporter([&Order] { Order.push_back('b'); });
+ SignalCurrentThread();
+}
+// Check that handlers are called in FIFO order.
+EXPECT_EQ(Order, "ab");
+
+// Check that current handler is the only one after the nested scope is
+// over.
+SignalCurrentThread();
+EXPECT_EQ(Order, "aba");
+ }
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/unittests/CMakeLists.txt
===
--- clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -88,6 +88,7 @@
TestIndex.cpp
TestTU.cpp
TestWorkspace.cpp
+ ThreadCrashReporterTests.cpp
TidyProviderTests.cpp
TypeHierarchyTests.cpp
URITests.cpp
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -24,6 +24,7 @@
#include "refactor/Rename.h"
#include "support/Path.h"
#include "support/Shutdown.h"
+#include "support/ThreadCrashReporter.h"
#include "support/ThreadsafeFS.h"
#include "support/Trace.h"
#include "clang/Format/Format.h"
@@ -679,6 +680,7 @@
llvm::InitializeAllTargetInfos();
llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
+ llvm::sys::AddSignalHandler(&ThreadCrashReporter::runCrashHandlers, nullptr);
llvm::sys::SetInterruptFunction(&requestShutdown);
llvm::cl::SetVersionPrinter([](llvm::raw_ostream &OS) {
OS << versionString() << "\n"
Index: clang-tools-extra/clangd/support/ThreadCrashReporter.h
===
--- /dev/null
+++ clang-tools-extra/clangd/support/ThreadCrashReporter.h
@@ -0,0 +1,48 @@
+//===--- ThreadCrashReporter.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
+//
[PATCH] D109506: [clangd] Print current request context along with the stack trace
0x1eaf added a comment. I've tried making an integration test in addition to the unit test, but I couldn't find a way to make lit ignore the crashed process exit status: FAIL: Clangd :: crash.test (1049 of 1049) TEST 'Clangd :: crash.test' FAILED Script: -- : 'RUN: at line 2'; yes '[' | head -n 5 | sh -c "clangd --input-style=delimited 2>&1 || true" -- Exit Code: 141 I've also tried using `not` to the same effect: # RUN: yes '[' | head -n 5 | not clangd --input-style=delimited Is there some trick that I'm missing? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109506/new/ https://reviews.llvm.org/D109506 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
