[clangd] clangd --check: standalone diagnosis of common problems

[sammccall]

Summary

This is a tool to simply parse a file as clangd would, and run some
common features (code actions, go-to-definition, hover) in an attempt to
trigger or reproduce crashes, error diagnostics, etc.

This is easier and more predictable than loading the file in clangd, because:

• there's no editor/plugin variation to worry about
• there's no accidental variation of user behavior or other extraneous requests
• we trigger features at every token, rather than guessing
• everything is synchronoous, logs are easier to reason about
• it's easier to (get users to) capture logs when running on the command-line

This is a fairly lightweight variant of this idea.
We could do a lot more with it, and maybe we should.
But I can't in the near future, and experience will tell us if we made
the right tradeoffs and if it's worth investing further.

[kadircet]

thanks! this mostly looks good, as discussed offline I believe having an infra that we can improve over time is better than not having anything until we've got the "perfect" solution.

i just got a couple of questions about some pieces, and some nits.

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

58

put into anon namespace?

88

i agree, this would help identifying the case of "clangd binary has been copied to some place without the necessary lib directory".

but i think we should check for the final -resource-dir in the compile command below. as user's compile flags can override whatever default clangd comes up with.

96

we don't have any custom fallback commands, what's the point of printing this always?

126

IIRC, option providers don't really log anything about success/failure of parsing the config file. what's the point of having this exactly?

127

it is clear that we've crashed while parsing compile commands if we don't see cc1 args are in the output. so maybe drop this one?

132

maybe we should also note that this doesn't reflect the final result. as we turn off pchs or dependency file outputting related flags afterwards.

138

this seems ... surprising :D but I also don't have suggestion for a better place.

203

maybe we could print errors if the following has no results for "identifiers" ?

215

nit: maybe make this a free function and return ErrCount ?

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

52

why not have this in Check.h ?

852

nit: static_cast<int>(Err..)

maybe do the same for line 846 while here. (line 872 already does that)

[sammccall]

address (some of) comments

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

88

Moved FIXME to the right place.

126

This is needed in order to run the correct clang-tidy checks.

127

I don't think that's clear - you can find it out by reading the code, but I expect people to be running this who aren't reading the code. (They won't be able to work out all the details, but they tend to be interested in the broad strokes of what's going on).

So generally I've tried to include log statements before each major step.

132

Sure, and we also run clang-tidy, and fiddle with the preamble, and skip function bodies...

I think a disclaimer that "running clangd is not equivalent to running clang <cc1-args>" would be somewhat confusing, as I'm not sure that's a thing that anyone would expect to be true.

I expect the cc1 args to be useless to anyone not familiar with clangd internals, but they're kind of important to include. Reworded the log message a bit, is this enough?

203

There are lots of ways go-to-def can yield no results (e.g. macros, #ifdef-'d sections, templated code we can't resolve)

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

52

A header seems a bit out of place in tool/ and it doesn't seem necessary, being one function with a simple signature and no unit tests.

We'll get a linker error if we ever get this wrong, right?

[kadircet]

thanks LGTM!

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

108

already done in the else statement.

126

ah sorry, i forgot Inputs was a member

132

SG, and actually thinking again, we are only making changes in the positive direction for clangd. i.e. there shouldn't be an instance in which this cc1 works, but clangd fails.

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

52

yeah, it just felt uncommon, i don't think we can mess this up.

[thakis]

Might break tests: http://45.33.8.238/linux/29238/step_9.txt

[sammccall]

Thanks, I think this fails whenever compile_commands.json doesn't exist in the tree or under build.
(Which of course it does in my local checkout...). Reverted, probably need to rename/copy the test file.

[rsmith]

clang-tools-extra/clangd/test/check.test

1

This test implicitly parses a source file that #includes standard library headers, and fails if those headers aren't available; this causes the test to fail in some build environments. Would it be possible to make this test work in freestanding environments?

[hoy]

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

243

Hi, I'm seeing an error with this "tweak: ExpandAutoType ==> FAIL: Could not deduce type for 'auto' type". It's probably due to that the std string header was not available when running check.test, thus the test failed. Is it expected that the header is available when test is run? Thanks.

[sammccall]

Sorry @rsmith @hoy, I've replaced the test with one without such dependencies in bc18d8d9b705d31a94c51900c8b18e4feaf9c7fb.

[hoy]

In D88338#2307976, @sammccall wrote:

Sorry @rsmith @hoy, I've replaced the test with one without such dependencies in bc18d8d9b705d31a94c51900c8b18e4feaf9c7fb.

Thanks for the quick turnaround!

clang-tools-extra/clangd/test/check-fail.test

+// RUN: not clangd -check=%s 2>&1 | FileCheck -strict-whitespace %s
+
+// CHECK: Testing on source file {{.*}}check-fail.test
+// CHECK: internal (cc1) args are: -cc1
+// CHECK: Building preamble...
+// CHECK: [pp_file_not_found] Line {{.*}}: 'missing.h' file not found
+// CHECK: Building AST...
+// CHECK: Testing features at each token
+// CHECK: tweak: ExpandAutoType ==> FAIL
+// CHECK: All checks completed, 2 errors
+
+#include "missing.h"
+auto x = []{};

clang-tools-extra/clangd/test/check.test

+# RUN: clangd -log=verbose -check 2>&1 | FileCheck -strict-whitespace %s
+
+CHECK: Testing on source file {{.*}}test.cc
+CHECK: internal (cc1) args are: -cc1
+CHECK: Building preamble...
+CHECK: Built preamble
+CHECK: Building AST...
+CHECK: Testing features at each token
+CHECK-DAG: hover: false
+CHECK-DAG: hover: true
+CHECK-DAG: tweak: AddUsing
+CHECK: All checks completed, 0 errors
+

clang-tools-extra/clangd/tool/CMakeLists.txt

 include_directories(${CMAKE_CURRENT_SOURCE_DIR}/..)
 include_directories(${CMAKE_CURRENT_BINARY_DIR}/..)
 
 add_clang_tool(clangd
   ClangdMain.cpp
+  Check.cpp
   $<TARGET_OBJECTS:obj.clangDaemonTweaks>
   )
 
 set(LLVM_LINK_COMPONENTS
   support
   )
 
 set(CLANGD_XPC_LIBS "")

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

+//===--- Check.cpp - clangd self-diagnostics ------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+//
+// Many basic problems can occur processing a file in clangd, e.g.:
+//  - system includes are not found
+//  - crash when indexing its AST
+// clangd --check provides a simplified, isolated way to reproduce these,
+// with no editor, LSP, threads, background indexing etc to contend with.
+//
+// One important use case is gathering information for bug reports.
+// Another is reproducing crashes, and checking which setting prevent them.
+//
+// It simulates opening a file (determining compile command, parsing, indexing)
+// and then running features at many locations.
+//
+// Currently it adds some basic logging of progress and results.
+// We should consider extending it to also recognize common symptoms and
+// recommend solutions (e.g. standard library installation issues).
+//
+//===----------------------------------------------------------------------===//
+
+#include "ClangdLSPServer.h"
+#include "CodeComplete.h"
+#include "GlobalCompilationDatabase.h"
+#include "Hover.h"
+#include "ParsedAST.h"
+#include "Preamble.h"
+#include "SourceCode.h"
+#include "XRefs.h"
+#include "index/CanonicalIncludes.h"
+#include "index/FileIndex.h"
+#include "refactor/Tweak.h"
+#include "support/ThreadsafeFS.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/Basic/DiagnosticIDs.h"
+#include "clang/Format/Format.h"
+#include "clang/Frontend/CompilerInvocation.h"
+#include "clang/Tooling/CompilationDatabase.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/Support/Path.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+// Print (and count) the error-level diagnostics (warnings are ignored).
+unsigned showErrors(llvm::ArrayRef<Diag> Diags) {
+  unsigned ErrCount = 0;
+  for (const auto &D : Diags) {
+    if (D.Severity >= DiagnosticsEngine::Error) {
+      elog("[{0}] Line {1}: {2}", D.Name, D.Range.start.line + 1, D.Message);
+      ++ErrCount;
+    }
+  }
+  return ErrCount;
+}
+
+// This class is just a linear pipeline whose functions get called in sequence.
+// Each exercises part of clangd's logic on our test file and logs results.
+// Later steps depend on state built in earlier ones (such as the AST).
+// Many steps can fatally fail (return false), then subsequent ones cannot run.
+// Nonfatal failures are logged and tracked in ErrCount.
+class Checker {
+  // from constructor
+  std::string File;
+  ClangdLSPServer::Options Opts;
+  // from buildCommand
+  tooling::CompileCommand Cmd;
+  // from buildInvocation
+  ParseInputs Inputs;
+  std::unique_ptr<CompilerInvocation> Invocation;
+  format::FormatStyle Style;
+  // from buildAST
+  std::shared_ptr<const PreambleData> Preamble;
+  llvm::Optional<ParsedAST> AST;
+  FileIndex Index;
+
+public:
+  // Number of non-fatal errors seen.
+  unsigned ErrCount = 0;
+
+  Checker(llvm::StringRef File, const ClangdLSPServer::Options &Opts)
+      : File(File), Opts(Opts) {}
+
+  // Read compilation database and choose a compile command for the file.
+  bool buildCommand() {
+    log("Loading compilation database...");
+    std::unique_ptr<GlobalCompilationDatabase> BaseCDB =
+        std::make_unique<DirectoryBasedGlobalCompilationDatabase>(
+            Opts.CompileCommandsDir);
+    BaseCDB = getQueryDriverDatabase(llvm::makeArrayRef(Opts.QueryDriverGlobs),
+                                     std::move(BaseCDB));
+    auto Mangler = CommandMangler::detect();
+    if (Opts.ResourceDir)
+      Mangler.ResourceDir = *Opts.ResourceDir;
+    auto CDB = std::make_unique<OverlayCDB>(
+        BaseCDB.get(), std::vector<std::string>{},
+        tooling::ArgumentsAdjuster(std::move(Mangler)));
+
+    if (auto TrueCmd = CDB->getCompileCommand(File)) {
+      Cmd = std::move(*TrueCmd);
+      log("Compile command from CDB is: {0}", llvm::join(Cmd.CommandLine, " "));
+    } else {
+      Cmd = CDB->getFallbackCommand(File);
+      log("Generic fallback command is: {0}", llvm::join(Cmd.CommandLine, " "));
+    }
+
+    return true;
+  }
+
+  // Prepare inputs and build CompilerInvocation (parsed compile command).
+  bool buildInvocation(const ThreadsafeFS &TFS,
+                       llvm::Optional<std::string> Contents) {
+    StoreDiags CaptureInvocationDiags;
+    std::vector<std::string> CC1Args;
+    Inputs.CompileCommand = Cmd;
+    Inputs.TFS = &TFS;
+    if (Contents.hasValue()) {
+      Inputs.Contents = *Contents;
+      log("Imaginary source file contents:\n{0}", Inputs.Contents);
+    } else {
+      if (auto Contents = TFS.view(llvm::None)->getBufferForFile(File)) {
+        Inputs.Contents = Contents->get()->getBuffer().str();
+      } else {
+        elog("Couldn't read {0}: {1}", File, Contents.getError().message());
+        return false;
+      }
+    }
+    Inputs.Opts.ClangTidyOpts =
+        Opts.GetClangTidyOptions(*TFS.view(llvm::None), File);
+    log("Parsing command...");
+    Invocation =
+        buildCompilerInvocation(Inputs, CaptureInvocationDiags, &CC1Args);
+    auto InvocationDiags = CaptureInvocationDiags.take();
+    ErrCount += showErrors(InvocationDiags);
+    log("internal (cc1) args are: {0}", llvm::join(CC1Args, " "));
+    if (!Invocation) {
+      elog("Failed to parse command line");
+      return false;
+    }
+
+    // FIXME: Check that resource-dir/built-in-headers exist?
+
+    Style = getFormatStyleForFile(File, Inputs.Contents, TFS);
+
+    return true;
+  }
+
+  // Build preamble and AST, and index them.
+  bool buildAST() {
+    log("Building preamble...");
+    Preamble =
+        buildPreamble(File, *Invocation, Inputs, /*StoreInMemory=*/true,
+                      [&](ASTContext &Ctx, std::shared_ptr<Preprocessor> PP,
+                          const CanonicalIncludes &Includes) {
+                        if (!Opts.BuildDynamicSymbolIndex)
+                          return;
+                        log("Indexing headers...");
+                        Index.updatePreamble(File, /*Version=*/"null", Ctx,
+                                             std::move(PP), Includes);
+                      });
+    if (!Preamble) {
+      elog("Failed to build preamble");
+      return false;
+    }
+    ErrCount += showErrors(Preamble->Diags);
+
+    log("Building AST...");
+    AST = ParsedAST::build(File, Inputs, std::move(Invocation),
+                           /*InvocationDiags=*/std::vector<Diag>{}, Preamble);
+    if (!AST) {
+      elog("Failed to build AST");
+      return false;
+    }
+    ErrCount += showErrors(llvm::makeArrayRef(AST->getDiagnostics())
+                               .drop_front(Preamble->Diags.size()));
+
+    if (Opts.BuildDynamicSymbolIndex) {
+      log("Indexing AST...");
+      Index.updateMain(File, *AST);
+    }
+    return true;
+  }
+
+  // Run AST-based features at each token in the file.
+  void testLocationFeatures() {
+    log("Testing features at each token (may be slow in large files)");
+    auto SpelledTokens =
+        AST->getTokens().spelledTokens(AST->getSourceManager().getMainFileID());
+    for (const auto &Tok : SpelledTokens) {
+      unsigned Start = AST->getSourceManager().getFileOffset(Tok.location());
+      unsigned End = Start + Tok.length();
+      Position Pos = offsetToPosition(Inputs.Contents, Start);
+      // FIXME: dumping the tokens may leak sensitive code into bug reports.
+      // Add an option to turn this off, once we decide how options work.
+      vlog("  {0} {1}", Pos, Tok.text(AST->getSourceManager()));
+      auto Tree = SelectionTree::createRight(AST->getASTContext(),
+                                             AST->getTokens(), Start, End);
+      Tweak::Selection Selection(&Index, *AST, Start, End, std::move(Tree));
+      for (const auto &T : prepareTweaks(Selection, Opts.TweakFilter)) {
+        auto Result = T->apply(Selection);
+        if (!Result) {
+          elog("    tweak: {0} ==> FAIL: {1}", T->id(), Result.takeError());
+          ++ErrCount;
+        } else {
+          vlog("    tweak: {0}", T->id());
+        }
+      }
+      unsigned Definitions = locateSymbolAt(*AST, Pos, &Index).size();
+      vlog("    definition: {0}", Definitions);
+
+      auto Hover = getHover(*AST, Pos, Style, &Index);
+      vlog("    hover: {0}", Hover.hasValue());
+
+      // FIXME: it'd be nice to include code completion, but it's too slow.
+      // Maybe in combination with a line restriction?
+    }
+  }
+};
+
+} // namespace
+
+bool check(llvm::StringRef File, const ThreadsafeFS &TFS,
+           const ClangdLSPServer::Options &Opts) {
+  llvm::SmallString<0> FakeFile;
+  llvm::Optional<std::string> Contents;
+  if (File.empty()) {
+    llvm::sys::path::system_temp_directory(false, FakeFile);
+    llvm::sys::path::append(FakeFile, "test.cc");
+    File = FakeFile;
+    Contents = R"cpp(
+      #include <stddef.h>
+      #include <string>
+
+      size_t N = 50;
+      auto xxx = std::string(N, 'x');
+    )cpp";
+  }
+  log("Testing on source file {0}", File);
+
+  Checker C(File, Opts);
+  if (!C.buildCommand() || !C.buildInvocation(TFS, Contents) || !C.buildAST())
+    return false;
+  C.testLocationFeatures();
+
+  log("All checks completed, {0} errors", C.ErrCount);
+  return C.ErrCount == 0;
+}
+
+} // namespace clangd
+} // namespace clang

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

 #include <thread>
 
 #ifndef _WIN32
 #include <unistd.h>
 #endif
 
 namespace clang {
 namespace clangd {
+
+// Implemented in Check.cpp.
+bool check(const llvm::StringRef File, const ThreadsafeFS &TFS,
+           const ClangdLSPServer::Options &Opts);
+
 namespace {
 
 using llvm::cl::cat;
 using llvm::cl::CommaSeparated;
 using llvm::cl::desc;
 using llvm::cl::Hidden;
 using llvm::cl::init;
 using llvm::cl::list;
 using llvm::cl::opt;
 using llvm::cl::OptionCategory;
+using llvm::cl::ValueOptional;
 using llvm::cl::values;
 
 // All flags must be placed in a category, or they will be shown neither in
 // --help, nor --help-hidden!
 OptionCategory CompileCommands("clangd compilation flags options");
 OptionCategory Features("clangd feature options");
 OptionCategory Misc("clangd miscellaneous options");
 OptionCategory Protocol("clangd protocol and logging options");
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<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,
+};
+
 enum PCHStorageFlag { Disk, Memory };
 opt<PCHStorageFlag> PCHStorage{
     "pch-storage",
     cat(Misc),
     desc("Storing PCHs in memory increases memory usages, but may "
          "improve performance"),
     values(
         clEnumValN(PCHStorageFlag::Disk, "disk", "store PCHs on disk"),

 #endif
 
 } // namespace
 } // namespace clangd
 } // namespace clang
 
 enum class ErrorResultCode : int {
   NoShutdownRequest = 1,
-  CantRunAsXPCService = 2
+  CantRunAsXPCService = 2,
+  CheckFailed = 3
 };
 
 int main(int argc, char *argv[]) {
   using namespace clang;
   using namespace clang::clangd;
 
   llvm::InitializeAllTargetInfos();
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
)";
 
   llvm::Optional<trace::Session> TracingSession;
   if (Tracer)
     TracingSession.emplace(*Tracer);
 
   // If a user ran `clangd` in a terminal without redirecting anything,
   // it's somewhat likely they're confused about how to use clangd.
   // Show them the help overview, which explains.
-  if (llvm::outs().is_displayed() && llvm::errs().is_displayed())
+  if (llvm::outs().is_displayed() && llvm::errs().is_displayed() &&
+      !CheckFile.getNumOccurrences())
     llvm::errs() << Overview << "\n";
   // Use buffered stream to stderr (we still flush each log message). Unbuffered
   // stream can cause significant (non-deterministic) latency for the logger.
   llvm::errs().SetBuffered();
   // Don't flush stdout when logging, this would be both slow and racy!
   llvm::errs().tie(nullptr);
   StreamLogger Logger(llvm::errs(), LogLevel);
   LoggingSession LoggingSession(Logger);
Opts.TweakFilter = [&](const Tweak &T) {
     return true;
   };
   if (ForceOffsetEncoding != OffsetEncoding::UnsupportedEncoding)
     Opts.Encoding = ForceOffsetEncoding;
 
   // Shall we allow to customize the file limit?
   Opts.Rename.AllowCrossFile = CrossFileRename;
 
+  if (CheckFile.getNumOccurrences()) {
+    llvm::SmallString<256> Path;
+    llvm::sys::fs::real_path(CheckFile, Path, /*expand_tilde=*/true);
+    log("Entering check mode (no LSP server)");
+    return check(Path, TFS, Opts)
+               ? 0
+               : static_cast<int>(ErrorResultCode::CheckFailed);
+  }
+
   // Initialize and run ClangdLSPServer.
   // Change stdin to binary to not lose \r\n on windows.
   llvm::sys::ChangeStdinToBinary();
   std::unique_ptr<Transport> TransportLayer;
   if (getenv("CLANGD_AS_XPC_SERVICE")) {
 #if CLANGD_BUILD_XPC
     log("Starting LSP over XPC service");
     TransportLayer = newXPCTransport();
 #else
     llvm::errs() << "This clangd binary wasn't built with XPC support.\n";
-    return (int)ErrorResultCode::CantRunAsXPCService;
+    return static_cast<int>(ErrorResultCode::CantRunAsXPCService);
 #endif
   } else {
     log("Starting LSP over stdin/stdout");
     TransportLayer = newJSONTransport(
         stdin, llvm::outs(),
         InputMirrorStream ? InputMirrorStream.getPointer() : nullptr,
         PrettyPrint, InputStyle);
   }