[PATCH] D109506: [clangd] Print current request context along with the stack trace

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This is mostly LG with nits, but I think the code-completion handler is wrong, and the tests as written won't build on windows.

In D109506#2992201 <https://reviews.llvm.org/D109506#2992201>, @sammccall wrote:

> Oops, forgot one thing: you probably want to instrument the preamble-building in TUScheduler (or in Preamble.cpp?), the background indexing, and code completion.
> Those all run the clang parser and should be a rich source of clang bugs.

You've added code completion (i.e. runWithPreamble), but not preamble-building or background-indexing.
This is fine, we can add the others later - just wanted to make sure we're on the same page.

In D109506#3000098 <https://reviews.llvm.org/D109506#3000098>, @0x1eaf wrote:

> 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:
> : 'RUN: at line 2';   yes '[' | head -n 50000 | sh -c "clangd --input-style=delimited 2>&1 || true"
> Exit Code: 141

I think this is related to the fact that tests run with `set -o pipefail`, and `yes` fails when `head` closes the pipe.
`not yes` doesn't seem to help because SIGPIPE is actually caught by the shell rather than `yes`, thus the 141 code. (Hey, signals again!)
This seems to work: `(yes '[' || :) | head -n 50000 | clangd --input-style=delimited`

(It is in principle possbile to disable pipefail in lit tests but awkward on a per-test level and nobody does it.)



================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:556
 
+  /// For calling from ThreadCrashReporter callback.
+  void dumpCurrentRequest(llvm::raw_ostream &OS) const;
----------------
Probably worth commenting "May only call from worker thread" or so.

(Unfortunately we have unusual concurrency constraints on the members - some such as FileInputs are locked when written on the worker thread, locked when read from other threads, unlocked when read from the worker thread, and never written from other threads. Because it's confusing it's probably worth being explicit).


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1382
+void ASTWorker::dumpCurrentRequest(llvm::raw_ostream &OS) const {
+  auto &ActionName = CurrentRequest ? CurrentRequest->Name : "";
+  OS << "Signalled during AST action: " << ActionName << "\n";
----------------
nit: I'd prefer an explicit type over `auto&` here. Probably StringRef?

(I'm not actually sure what type we're getting and whether the reference is dangling).


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1639
+    ThreadCrashReporter ScopedReporter(
+        [&Worker]() { Worker->dumpCurrentRequest(llvm::errs()); });
     std::shared_ptr<const PreambleData> Preamble;
----------------
This doesn't seem right.
This code runs on the PreambleThreads threadpool, not on the worker thread. So a) the state of the worker thread isn't really relevant and b) we're accessing it in a non-threadsafe way.

Instead we need to dump Name, Command, Contents etc separately again.
Maybe dumpCurrentRequest should be a free helper function that takes the individual parameters instead.


================
Comment at: clang-tools-extra/clangd/support/ThreadCrashReporter.cpp:28
+
+  // Traverse to the top of the reporter stack.
+  ThreadCrashReporter *Reporter = CurrentReporter;
----------------
I'm not sure the FIFO behavior is much (or at all) better than LIFO[1], and it seems like it adds a bit of complexity (we'd only need the  `Next` pointer, and there'd be less confusion direction of next vs previous).

[1] (insert joke about python having tracebacks instead of backtraces, because they're backwards...)


================
Comment at: clang-tools-extra/clangd/support/ThreadCrashReporter.h:23
+
+  /// Copies the std::function and sets the copy as current thread-local
+  /// callback. Asserts if the current thread's callback is already set.
----------------
Some mention of constraints on the passed function?

e.g. "The callback is likely to be invoked in a signal handler. Most LLVM signal handling is not strictly async-signal-safe. However reporters should avoid accessing data structures likely to be in a bad state on crash."


================
Comment at: clang-tools-extra/clangd/support/ThreadCrashReporter.h:25
+  /// callback. Asserts if the current thread's callback is already set.
+  ThreadCrashReporter(const SignalCallback &ThreadLocalCallback);
+  /// Resets the currrent thread's callback to nullptr.
----------------
doesn't seem to be any need for a copy here, pass by value and move instead?
(And probably use `llvm::unique_function` to avoid the copyable requirement)


================
Comment at: clang-tools-extra/clangd/support/ThreadCrashReporter.h:35
+
+  /// Callback to install via sys::AddSignalHandler(), the argument is ignored.
+  /// Any signal filtering is the responsibility of the caller.
----------------
This should first have a comment saying what it actually does!
"Calls all currently-active ThreadCrashReporters for the current thread".

Probably also something like "This function is intended to be called from signal handlers, but is not strictly async-signal-safe - see constructor comment."


================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:683
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
+  llvm::sys::AddSignalHandler(&ThreadCrashReporter::runCrashHandlers, nullptr);
   llvm::sys::SetInterruptFunction(&requestShutdown);
----------------
nit: seems a little cleaner to handle dropping the cookie arg here instead of in `runCrashHandlers`
pass `+[](void*){ ThreadCrashReporter::runCrashHandlers(); }`?


================
Comment at: clang-tools-extra/clangd/unittests/ThreadCrashReporterTests.cpp:26
+  AsyncTaskRunner Runner;
+  auto SignalCurrentThread = []() { raise(SIGUSR1); };
+  auto SignalAnotherThread = [&]() {
----------------
unfortunately `SIGUSR1` is not required to be supported (or exist) and appears not to be on MSVC per the failing pre-merge checks. And in fact SetInfoSignalFunction is a no-op on windows.

We could `#ifdef` the test, but it seems like a real shame.
`EXPECT_DEATH` is a candidate but it [doesn't interact well with threads](https://chromium.googlesource.com/external/github.com/google/googletest/+/refs/tags/release-1.8.0/googletest/docs/FAQ.md#why-does-assert_death-complain-about-previous-threads-that-were-already-joined).

Best I can think of is some combination/subset of:
 - a test like this with `#ifdef SIGUSR1`
 - a simple EXPECT_DEATH using `SIGINT` that doesn't spawn any extra threads
 - a threaded test that calls `runCrashHandlers()`directly rather than via a signal handler

(I think most of the cases tested here: scope, FIFO etc could be moved to the third option as they really have nothing to do with signals per se. It's nice to have *some* test that it does work with llvm's signal handler though)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109506/new/

https://reviews.llvm.org/D109506