[clangd] Make signatureHelp work with stale preambles

Summary:
This is achieved by calculating newly added includes and implicitly
parsing them as if they were part of the main file.

This also gets rid of the need for consistent preamble reads.

Reviewers: sammccall

Subscribers: ilya-biryukov, javed.absar, MaskRay, jkorous, mgrang, arphaman, jfb, usaxena95, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D77392

clang-tools-extra/clangd/ClangdServer.cpp

@@ -280,11 +280,8 @@ void ClangdServer::signatureHelp(PathRef File, Position Pos,
                              Pos, FS, Index));
   };
 
-  // Unlike code completion, we wait for an up-to-date preamble here.
-  // Signature help is often triggered after code completion. If the code
-  // completion inserted a header to make the symbol available, then using
-  // the old preamble would yield useless results.
-  WorkScheduler.runWithPreamble("SignatureHelp", File, TUScheduler::Consistent,
+  // Unlike code completion, we wait for a preamble here.
+  WorkScheduler.runWithPreamble("SignatureHelp", File, TUScheduler::Stale,
                                 std::move(Action));
 }
 

clang-tools-extra/clangd/CodeComplete.cpp

@@ -1023,6 +1023,7 @@ struct SemaCompleteInput {
   PathRef FileName;
   const tooling::CompileCommand &Command;
   const PreambleData &Preamble;
+  const PreamblePatch &PreamblePatch;
   llvm::StringRef Contents;
   size_t Offset;
   llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS;
@@ -1060,7 +1061,6 @@ bool semaCodeComplete(std::unique_ptr<CodeCompleteConsumer> Consumer,
   ParseInput.CompileCommand = Input.Command;
   ParseInput.FS = VFS;
   ParseInput.Contents = std::string(Input.Contents);
-  ParseInput.Opts = ParseOptions();
 
   IgnoreDiagnostics IgnoreDiags;
   auto CI = buildCompilerInvocation(ParseInput, IgnoreDiags);
@@ -1096,6 +1096,7 @@ bool semaCodeComplete(std::unique_ptr<CodeCompleteConsumer> Consumer,
   PreambleBounds PreambleRegion =
       ComputePreambleBounds(*CI->getLangOpts(), ContentsBuffer.get(), 0);
   bool CompletingInPreamble = PreambleRegion.Size > Input.Offset;
+  Input.PreamblePatch.apply(*CI);
   // NOTE: we must call BeginSourceFile after prepareCompilerInstance. Otherwise
   // the remapped buffers do not get freed.
   auto Clang = prepareCompilerInstance(
@@ -1754,8 +1755,10 @@ codeComplete(PathRef FileName, const tooling::CompileCommand &Command,
       SpecFuzzyFind, Opts);
   return (!Preamble || Opts.RunParser == CodeCompleteOptions::NeverParse)
              ? std::move(Flow).runWithoutSema(Contents, *Offset, VFS)
-             : std::move(Flow).run(
-                   {FileName, Command, *Preamble, Contents, *Offset, VFS});
+             : std::move(Flow).run({FileName, Command, *Preamble,
+                                    // We want to serve code completions with
+                                    // low latency, so don't bother patching.
+                                    PreamblePatch(), Contents, *Offset, VFS});
 }
 
 SignatureHelp signatureHelp(PathRef FileName,
@@ -1775,10 +1778,15 @@ SignatureHelp signatureHelp(PathRef FileName,
   Options.IncludeMacros = false;
   Options.IncludeCodePatterns = false;
   Options.IncludeBriefComments = false;
-  IncludeStructure PreambleInclusions; // Unused for signatureHelp
+
+  ParseInputs PI;
+  PI.CompileCommand = Command;
+  PI.Contents = Contents.str();
+  PI.FS = std::move(VFS);
+  auto PP = PreamblePatch::create(FileName, PI, Preamble);
   semaCodeComplete(
       std::make_unique<SignatureHelpCollector>(Options, Index, Result), Options,
-      {FileName, Command, Preamble, Contents, *Offset, std::move(VFS)});
+      {FileName, Command, Preamble, PP, Contents, *Offset, std::move(PI.FS)});
   return Result;
 }
 

clang-tools-extra/clangd/Preamble.cpp

@@ -8,11 +8,39 @@
 
 #include "Preamble.h"
 #include "Compiler.h"
+#include "Headers.h"
 #include "Logger.h"
 #include "Trace.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/TokenKinds.h"
+#include "clang/Frontend/CompilerInvocation.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Lex/Lexer.h"
 #include "clang/Lex/PPCallbacks.h"
+#include "clang/Lex/Preprocessor.h"
 #include "clang/Lex/PreprocessorOptions.h"
+#include "clang/Tooling/CompilationDatabase.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSet.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/VirtualFileSystem.h"
+#include "llvm/Support/raw_ostream.h"
+#include <iterator>
+#include <memory>
+#include <string>
+#include <system_error>
+#include <utility>
+#include <vector>
 
 namespace clang {
 namespace clangd {
@@ -74,6 +102,81 @@ class CppFilePreambleCallbacks : public PreambleCallbacks {
   const SourceManager *SourceMgr = nullptr;
 };
 
+// Runs preprocessor over preamble section.
+class PreambleOnlyAction : public PreprocessorFrontendAction {
+protected:
+  void ExecuteAction() override {
+    Preprocessor &PP = getCompilerInstance().getPreprocessor();
+    auto &SM = PP.getSourceManager();
+    PP.EnterMainSourceFile();
+    auto Bounds = ComputePreambleBounds(getCompilerInstance().getLangOpts(),
+                                        SM.getBuffer(SM.getMainFileID()), 0);
+    Token Tok;
+    do {
+      PP.Lex(Tok);
+      assert(SM.isInMainFile(Tok.getLocation()));
+    } while (Tok.isNot(tok::eof) &&
+             SM.getDecomposedLoc(Tok.getLocation()).second < Bounds.Size);
+  }
+};
+
+/// Gets the includes in the preamble section of the file by running
+/// preprocessor over \p Contents. Returned includes do not contain resolved
+/// paths. \p VFS and \p Cmd is used to build the compiler invocation, which
+/// might stat/read files.
+llvm::Expected<std::vector<Inclusion>>
+scanPreambleIncludes(llvm::StringRef Contents,
+                     llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS,
+                     const tooling::CompileCommand &Cmd) {
+  // Build and run Preprocessor over the preamble.
+  ParseInputs PI;
+  PI.Contents = Contents.str();
+  PI.FS = std::move(VFS);
+  PI.CompileCommand = Cmd;
+  IgnoringDiagConsumer IgnoreDiags;
+  auto CI = buildCompilerInvocation(PI, IgnoreDiags);
+  if (!CI)
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "failed to create compiler invocation");
+  CI->getDiagnosticOpts().IgnoreWarnings = true;
+  auto ContentsBuffer = llvm::MemoryBuffer::getMemBuffer(Contents);
+  auto Clang = prepareCompilerInstance(
+      std::move(CI), nullptr, std::move(ContentsBuffer),
+      // Provide an empty FS to prevent preprocessor from performing IO. This
+      // also implies missing resolved paths for includes.
+      new llvm::vfs::InMemoryFileSystem, IgnoreDiags);
+  if (Clang->getFrontendOpts().Inputs.empty())
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "compiler instance had no inputs");
+  // We are only interested in main file includes.
+  Clang->getPreprocessorOpts().SingleFileParseMode = true;
+  PreambleOnlyAction Action;
+  if (!Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0]))
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "failed BeginSourceFile");
+  Preprocessor &PP = Clang->getPreprocessor();
+  IncludeStructure Includes;
+  PP.addPPCallbacks(
+      collectIncludeStructureCallback(Clang->getSourceManager(), &Includes));
+  if (llvm::Error Err = Action.Execute())
+    return std::move(Err);
+  Action.EndSourceFile();
+  return Includes.MainFileIncludes;
+}
+
+const char *spellingForIncDirective(tok::PPKeywordKind IncludeDirective) {
+  switch (IncludeDirective) {
+  case tok::pp_include:
+    return "include";
+  case tok::pp_import:
+    return "import";
+  case tok::pp_include_next:
+    return "include_next";
+  default:
+    break;
+  }
+  llvm_unreachable("not an include directive");
+}
 } // namespace
 
 PreambleData::PreambleData(const ParseInputs &Inputs,
@@ -166,5 +269,78 @@ bool isPreambleCompatible(const PreambleData &Preamble,
          Preamble.Preamble.CanReuse(CI, ContentsBuffer.get(), Bounds,
                                     Inputs.FS.get());
 }
+
+PreamblePatch PreamblePatch::create(llvm::StringRef FileName,
+                                    const ParseInputs &Modified,
+                                    const PreambleData &Baseline) {
+  // First scan the include directives in Baseline and Modified. These will be
+  // used to figure out newly added directives in Modified. Scanning can fail,
+  // the code just bails out and creates an empty patch in such cases, as:
+  // - If scanning for Baseline fails, no knowledge of existing includes hence
+  //   patch will contain all the includes in Modified. Leading to rebuild of
+  //   whole preamble, which is terribly slow.
+  // - If scanning for Modified fails, cannot figure out newly added ones so
+  //   there's nothing to do but generate an empty patch.
+  auto BaselineIncludes = scanPreambleIncludes(
+      // Contents needs to be null-terminated.
+      Baseline.Preamble.getContents().str(),
+      Baseline.StatCache->getConsumingFS(Modified.FS), Modified.CompileCommand);
+  if (!BaselineIncludes) {
+    elog("Failed to scan includes for baseline of {0}: {1}", FileName,
+         BaselineIncludes.takeError());
+    return {};
+  }
+  auto ModifiedIncludes = scanPreambleIncludes(
+      Modified.Contents, Baseline.StatCache->getConsumingFS(Modified.FS),
+      Modified.CompileCommand);
+  if (!ModifiedIncludes) {
+    elog("Failed to scan includes for modified contents of {0}: {1}", FileName,
+         ModifiedIncludes.takeError());
+    return {};
+  }
+
+  PreamblePatch PP;
+  // This shouldn't coincide with any real file name.
+  llvm::SmallString<128> PatchName;
+  llvm::sys::path::append(PatchName, llvm::sys::path::parent_path(FileName),
+                          "__preamble_patch__.h");
+  PP.PatchFileName = PatchName.str().str();
+
+  // We are only interested in newly added includes, record the ones in Baseline
+  // for exclusion.
+  llvm::DenseSet<std::pair<tok::PPKeywordKind, llvm::StringRef>>
+      ExistingIncludes;
+  for (const auto &Inc : *BaselineIncludes)
+    ExistingIncludes.insert({Inc.Directive, Inc.Written});
+  // Calculate extra includes that needs to be inserted.
+  llvm::raw_string_ostream Patch(PP.PatchContents);
+  for (const auto &Inc : *ModifiedIncludes) {
+    if (ExistingIncludes.count({Inc.Directive, Inc.Written}))
+      continue;
+    Patch << llvm::formatv("#{0} {1}\n", spellingForIncDirective(Inc.Directive),
+                           Inc.Written);
+  }
+  Patch.flush();
+
+  // FIXME: Handle more directives, e.g. define/undef.
+  return PP;
+}
+
+void PreamblePatch::apply(CompilerInvocation &CI) const {
+  // No need to map an empty file.
+  if (PatchContents.empty())
+    return;
+  auto &PPOpts = CI.getPreprocessorOpts();
+  auto PatchBuffer =
+      // we copy here to ensure contents are still valid if CI outlives the
+      // PreamblePatch.
+      llvm::MemoryBuffer::getMemBufferCopy(PatchContents, PatchFileName);
+  // CI will take care of the lifetime of the buffer.
+  PPOpts.addRemappedFile(PatchFileName, PatchBuffer.release());
+  // The patch will be parsed after loading the preamble ast and before parsing
+  // the main file.
+  PPOpts.Includes.push_back(PatchFileName);
+}
+
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/Preamble.h

@@ -32,6 +32,7 @@
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/PrecompiledPreamble.h"
 #include "clang/Tooling/CompilationDatabase.h"
+#include "llvm/ADT/StringRef.h"
 
 #include <memory>
 #include <string>
@@ -88,6 +89,31 @@ buildPreamble(PathRef FileName, CompilerInvocation CI,
 bool isPreambleCompatible(const PreambleData &Preamble,
                           const ParseInputs &Inputs, PathRef FileName,
                           const CompilerInvocation &CI);
+
+/// Stores information required to parse a TU using a (possibly stale) Baseline
+/// preamble. Updates compiler invocation to approximately reflect additions to
+/// the preamble section of Modified contents, e.g. new include directives.
+class PreamblePatch {
+public:
+  // With an empty patch, the preamble is used verbatim.
+  PreamblePatch() = default;
+  /// Builds a patch that contains new PP directives introduced to the preamble
+  /// section of \p Modified compared to \p Baseline.
+  /// FIXME: This only handles include directives, we should at least handle
+  /// define/undef.
+  static PreamblePatch create(llvm::StringRef FileName,
+                              const ParseInputs &Modified,
+                              const PreambleData &Baseline);
+  /// Adjusts CI (which compiles the modified inputs) to be used with the
+  /// baseline preamble. This is done by inserting an artifical include to the
+  /// \p CI that contains new directives calculated in create.
+  void apply(CompilerInvocation &CI) const;
+
+private:
+  std::string PatchContents;
+  std::string PatchFileName;
+};
+
 } // namespace clangd
 } // namespace clang
 

clang-tools-extra/clangd/TUScheduler.cpp

@@ -866,36 +866,6 @@ ASTWorker::getPossiblyStalePreamble() const {
   return LatestPreamble;
 }
 
-void ASTWorker::getCurrentPreamble(
-    llvm::unique_function<void(std::shared_ptr<const PreambleData>)> Callback) {
-  // We could just call startTask() to throw the read on the queue, knowing
-  // it will run after any updates. But we know this task is cheap, so to
-  // improve latency we cheat: insert it on the queue after the last update.
-  std::unique_lock<std::mutex> Lock(Mutex);
-  auto LastUpdate =
-      std::find_if(Requests.rbegin(), Requests.rend(),
-                   [](const Request &R) { return R.UpdateType.hasValue(); });
-  // If there were no writes in the queue, and CurrentRequest is not a write,
-  // the preamble is ready now.
-  if (LastUpdate == Requests.rend() &&
-      (!CurrentRequest || CurrentRequest->UpdateType.hasValue())) {
-    Lock.unlock();
-    return Callback(getPossiblyStalePreamble());
-  }
-  assert(!RunSync && "Running synchronously, but queue is non-empty!");
-  Requests.insert(LastUpdate.base(),
-                  Request{[Callback = std::move(Callback), this]() mutable {
-                            Callback(getPossiblyStalePreamble());
-                          },
-                          "GetPreamble", steady_clock::now(),
-                          Context::current().clone(),
-                          /*UpdateType=*/None,
-                          /*InvalidationPolicy=*/TUScheduler::NoInvalidation,
-                          /*Invalidate=*/nullptr});
-  Lock.unlock();
-  RequestsCV.notify_all();
-}
-
 void ASTWorker::waitForFirstPreamble() const { BuiltFirstPreamble.wait(); }
 
 tooling::CompileCommand ASTWorker::getCurrentCompileCommand() const {
@@ -1307,41 +1277,21 @@ void TUScheduler::runWithPreamble(llvm::StringRef Name, PathRef File,
     return;
   }
 
-  // Future is populated if the task needs a specific preamble.
-  std::future<std::shared_ptr<const PreambleData>> ConsistentPreamble;
-  // FIXME: Currently this only holds because ASTWorker blocks after issuing a
-  // preamble build. Get rid of consistent reads or make them build on the
-  // calling thread instead.
-  if (Consistency == Consistent) {
-    std::promise<std::shared_ptr<const PreambleData>> Promise;
-    ConsistentPreamble = Promise.get_future();
-    It->second->Worker->getCurrentPreamble(
-        [Promise = std::move(Promise)](
-            std::shared_ptr<const PreambleData> Preamble) mutable {
-          Promise.set_value(std::move(Preamble));
-        });
-  }
-
   std::shared_ptr<const ASTWorker> Worker = It->second->Worker.lock();
   auto Task =
       [Worker, Consistency, Name = Name.str(), File = File.str(),
        Contents = It->second->Contents,
        Command = Worker->getCurrentCompileCommand(),
        Ctx = Context::current().derive(kFileBeingProcessed, std::string(File)),
-       ConsistentPreamble = std::move(ConsistentPreamble),
        Action = std::move(Action), this]() mutable {
         std::shared_ptr<const PreambleData> Preamble;
-        if (ConsistentPreamble.valid()) {
-          Preamble = ConsistentPreamble.get();
-        } else {
-          if (Consistency != PreambleConsistency::StaleOrAbsent) {
-            // Wait until the preamble is built for the first time, if preamble
-            // is required. This avoids extra work of processing the preamble
-            // headers in parallel multiple times.
-            Worker->waitForFirstPreamble();
-          }
-          Preamble = Worker->getPossiblyStalePreamble();
+        if (Consistency == PreambleConsistency::Stale) {
+          // Wait until the preamble is built for the first time, if preamble
+          // is required. This avoids extra work of processing the preamble
+          // headers in parallel multiple times.
+          Worker->waitForFirstPreamble();
         }
+        Preamble = Worker->getPossiblyStalePreamble();
 
         std::lock_guard<Semaphore> BarrierLock(Barrier);
         WithContext Guard(std::move(Ctx));