[clangd] Add integration test for crash handling

[sammccall]

Summary

This replaces the test removed in 51be7061d025139ba66869d5d99c7157a3ae9edd
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.

[kadircet]

thanks, lgtm!

clang-tools-extra/clangd/Compiler.h

 /// Returns null on errors. When non-null value is returned, it is expected to
 /// be consumed by FrontendAction::BeginSourceFile to properly destroy \p
 /// MainFile.
 std::unique_ptr<CompilerInstance> prepareCompilerInstance(
     std::unique_ptr<clang::CompilerInvocation>, const PrecompiledPreamble *,
     std::unique_ptr<llvm::MemoryBuffer> MainFile,
     IntrusiveRefCntPtr<llvm::vfs::FileSystem>, DiagnosticConsumer &);
 
+/// Respect `#pragma clang __debug crash` etc, which are usually disabled.
+/// This may only be called before threads are spawned.
+void allowCrashPragmasForTest();
+
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_COMPILER_H

clang-tools-extra/clangd/Compiler.cpp

void IgnoreDiagnostics::log(DiagnosticsEngine::Level DiagLevel,
   clangd::vlog("Ignored diagnostic. {0}{1}", Location, Message);
 }
 
 void IgnoreDiagnostics::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
                                          const clang::Diagnostic &Info) {
   IgnoreDiagnostics::log(DiagLevel, Info);
 }
 
+static bool AllowCrashPragmasForTest = false;
+void allowCrashPragmasForTest() { AllowCrashPragmasForTest = true; }
+
 void disableUnsupportedOptions(CompilerInvocation &CI) {
   // Disable "clang -verify" diagnostics, they are rarely useful in clangd, and
   // our compiler invocation set-up doesn't seem to work with it (leading
   // assertions in VerifyDiagnosticConsumer).
   CI.getDiagnosticOpts().VerifyDiagnostics = false;
   CI.getDiagnosticOpts().ShowColors = false;
 
   // Disable any dependency outputting, we don't want to generate files or write
   // to stdout/stderr.
   CI.getDependencyOutputOpts().ShowIncludesDest = ShowIncludesDestination::None;
   CI.getDependencyOutputOpts().OutputFile.clear();
   CI.getDependencyOutputOpts().HeaderIncludeOutputFile.clear();
   CI.getDependencyOutputOpts().DOTOutputFile.clear();
   CI.getDependencyOutputOpts().ModuleDependencyOutputDir.clear();
 
   // Disable any pch generation/usage operations. Since serialized preamble
   // format is unstable, using an incompatible one might result in unexpected
   // behaviours, including crashes.
   CI.getPreprocessorOpts().ImplicitPCHInclude.clear();
   CI.getPreprocessorOpts().PrecompiledPreambleBytes = {0, false};
   CI.getPreprocessorOpts().PCHThroughHeader.clear();
   CI.getPreprocessorOpts().PCHWithHdrStop = false;
   CI.getPreprocessorOpts().PCHWithHdrStopCreate = false;
   // Don't crash on `#pragma clang __debug parser_crash`
+  if (!AllowCrashPragmasForTest)
     CI.getPreprocessorOpts().DisablePragmaDebugCrash = true;
 
   // Always default to raw container format as clangd doesn't registry any other
   // and clang dies when faced with unknown formats.
   CI.getHeaderSearchOpts().ModuleFormat =
       PCHContainerOperations().getRawReader().getFormat().str();
 
   CI.getFrontendOpts().Plugins.clear();
   CI.getFrontendOpts().AddPluginActions.clear();

clang-tools-extra/clangd/TUScheduler.cpp

private:
   void run();
   /// Signal that run() should finish processing pending requests and exit.
   void stop();
 
   /// Adds a new task to the end of the request queue.
   void startTask(llvm::StringRef Name, llvm::unique_function<void()> Task,
                  llvm::Optional<UpdateType> Update,
                  TUScheduler::ASTActionInvalidation);
+  /// Runs a task synchronously.
+  void runTask(llvm::StringRef Name, llvm::function_ref<void()> Task);
 
   /// Determines the next action to perform.
   /// All actions that should never run are discarded.
   /// Returns a deadline for the next action. If it's expired, run now.
   /// scheduleLocked() is called again at the deadline, or if requests arrive.
   Deadline scheduleLocked();
   /// Should the first task in the queue be skipped instead of run?
   bool shouldSkipHeadLocked() const;
auto Task = [this, Preamble = std::move(Preamble), CI = std::move(CI),
     if (WantDiags == WantDiagnostics::No)
       return;
     // Report diagnostics with the new preamble to ensure progress. Otherwise
     // diagnostics might get stale indefinitely if user keeps invalidating the
     // preamble.
     generateDiagnostics(std::move(CI), std::move(PI), std::move(CIDiags));
   };
   if (RunSync) {
-    Task();
+    runTask(TaskName, Task);
     return;
   }
   {
     std::lock_guard<std::mutex> Lock(Mutex);
     PreambleRequests.push_back({std::move(Task), std::move(TaskName),
                                 steady_clock::now(), Context::current().clone(),
                                 llvm::None, llvm::None,
                                 TUScheduler::NoInvalidation, nullptr});
void ASTWorker::stop() {
   }
   PreamblePeer.stop();
   // We are no longer going to build any preambles, let the waiters know that.
   PreambleCV.notify_all();
   Status.stop();
   RequestsCV.notify_all();
 }
 
+void ASTWorker::runTask(llvm::StringRef Name, llvm::function_ref<void()> Task) {
+  ThreadCrashReporter ScopedReporter([this, Name]() {
+    llvm::errs() << "Signalled during AST worker action: " << Name << "\n";
+    crashDumpParseInputs(llvm::errs(), FileInputs);
+  });
+  trace::Span Tracer(Name);
+  WithContext WithProvidedContext(ContextProvider(FileName));
+  Task();
+}
+
 void ASTWorker::startTask(llvm::StringRef Name,
                           llvm::unique_function<void()> Task,
                           llvm::Optional<UpdateType> Update,
                           TUScheduler::ASTActionInvalidation Invalidation) {
   if (RunSync) {
     assert(!Done && "running a task after stop()");
-    trace::Span Tracer(Name + ":" + llvm::sys::path::filename(FileName));
-    WithContext WithProvidedContext(ContextProvider(FileName));
-    Task();
+    runTask(Name, Task);
     return;
   }
 
   {
     std::lock_guard<std::mutex> Lock(Mutex);
     assert(!Done && "running a task after stop()");
     // Cancel any requests invalidated by this request.
     if (Update && Update->ContentChanged) {
// only writer is also this thread.
       if (!Lock.owns_lock()) {
         Status.update([&](TUStatus &Status) {
           Status.ASTActivity.K = ASTAction::Queued;
           Status.ASTActivity.Name = CurrentRequest->Name;
         });
         Lock.lock();
       }
       WithContext Guard(std::move(CurrentRequest->Ctx));
-      trace::Span Tracer(CurrentRequest->Name);
       Status.update([&](TUStatus &Status) {
         Status.ASTActivity.K = ASTAction::RunningAction;
         Status.ASTActivity.Name = CurrentRequest->Name;
       });
-      WithContext WithProvidedContext(ContextProvider(FileName));
-      ThreadCrashReporter ScopedReporter([this]() {
-        const char *Name = CurrentRequest ? CurrentRequest->Name.c_str() : "";
-        llvm::errs() << "Signalled during AST worker action: " << Name << "\n";
-        crashDumpParseInputs(llvm::errs(), FileInputs);
-      });
-      CurrentRequest->Action();
+      runTask(CurrentRequest->Name, CurrentRequest->Action);
     }
 
     bool IsEmpty = false;
     {
       std::lock_guard<std::mutex> Lock(Mutex);
       CurrentRequest.reset();
       IsEmpty = Requests.empty() && PreambleRequests.empty();
     }

clang-tools-extra/clangd/test/crash-parse.test

+# RUN: not --crash clangd -lit-test < %s 2> %t.err
+# RUN: FileCheck %s < %t.err --check-prefixes=CHECK,CHECK-SYNC
+# RUN: not --crash clangd -lit-test -sync=0 < %s 2> %t.async.err
+# RUN: FileCheck %s < %t.async.err
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{
+  "uri":"test:///foo.cc",
+  "languageId":"cpp",
+  "text":"int x;\n#pragma clang __debug llvm_fatal_error"
+}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"sync","params":{}}
+#      CHECK: Signalled during AST worker action: Build AST
+# CHECK-NEXT:   Filename: foo.cc
+# CHECK-SYNC: Signalled during AST worker action: Update
+# CHECK-SYNC:   Filename: foo.cc
+# CHECK-SYNC: Signalled while processing message:
+# CHECK-SYNC:   "languageId":"cpp"

clang-tools-extra/clangd/test/crash-preamble.test

+# RUN: not --crash clangd -lit-test < %s 2> %t.err
+# RUN: FileCheck %s < %t.err --check-prefixes=CHECK,CHECK-SYNC
+# RUN: not --crash clangd -lit-test -sync=0 < %s 2> %t.async.err
+# RUN: FileCheck %s < %t.async.err
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{
+  "uri":"test:///foo.cc",
+  "languageId":"cpp",
+  "text":"#pragma clang __debug llvm_fatal_error"
+}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"sync","params":{}}
+#      CHECK:  Signalled while building preamble
+# CHECK-NEXT:  Filename: foo.cc
+# CHECK-SYNC: Signalled during AST worker action: Update
+# CHECK-SYNC:   Filename: foo.cc
+# CHECK-SYNC: Signalled while processing message:
+# CHECK-SYNC:   "languageId":"cpp"

clang-tools-extra/clangd/tool/ClangdMain.cpp

 //===--- ClangdMain.cpp - clangd server loop ------------------------------===//
 //
 // 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 "ClangdLSPServer.h"
 #include "CodeComplete.h"
+#include "Compiler.h"
 #include "Config.h"
 #include "ConfigProvider.h"
 #include "Feature.h"
 #include "PathMapping.h"
 #include "Protocol.h"
 #include "TidyProvider.h"
 #include "Transport.h"
 #include "index/Background.h"
opt<bool> Test{
     cat(Misc),
     desc("Abbreviation for -input-style=delimited -pretty -sync "
          "-enable-test-scheme -enable-config=0 -log=verbose. "
          "Intended to simplify lit tests"),
     init(false),
     Hidden,
 };
 
+opt<bool> CrashPragmas{
+    "crash-pragmas",
+    cat(Misc),
+    desc("Respect `#pragma clang __debug crash` and friends."),
+    init(false),
+    Hidden,
+};
+
 opt<Path> CheckFile{
     "check",
     cat(Misc),
     desc("Parse one file in isolation instead of acting as a language server. "
          "Useful to investigate/reproduce crashes or configuration problems. "
          "With --check=<filename>, attempts to parse a particular file."),
     init(""),
     ValueOptional,
It should be used via an editor plugin rather than invoked directly. For more information, see:
   https://microsoft.github.io/language-server-protocol/
 
 clangd accepts flags on the commandline, and in the CLANGD_FLAGS environment variable.
 )";
   llvm::cl::HideUnrelatedOptions(ClangdCategories);
   llvm::cl::ParseCommandLineOptions(argc, argv, Overview,
                                     /*Errs=*/nullptr, FlagsEnvVar);
   if (Test) {
+    if (!Sync.getNumOccurrences())
       Sync = true;
+    if (!CrashPragmas.getNumOccurrences())
+      CrashPragmas = true;
     InputStyle = JSONStreamStyle::Delimited;
     LogLevel = Logger::Verbose;
     PrettyPrint = true;
     // Disable config system by default to avoid external reads.
     if (!EnableConfig.getNumOccurrences())
       EnableConfig = false;
     // Disable background index on lit tests by default to prevent disk writes.
     if (!EnableBackgroundIndex.getNumOccurrences())
       EnableBackgroundIndex = false;
     // Ensure background index makes progress.
     else if (EnableBackgroundIndex)
       BackgroundQueue::preventThreadStarvationInTests();
   }
   if (Test || EnableTestScheme) {
     static URISchemeRegistry::Add<TestScheme> X(
         "test", "Test scheme for clangd lit tests.");
   }
+  if (CrashPragmas)
+    allowCrashPragmasForTest();
 
   if (!Sync && WorkerThreadsCount == 0) {
     llvm::errs() << "A number of worker threads cannot be 0. Did you mean to "
                     "specify -sync?";
     return 1;
   }
 
   if (Sync) {