[clangd] Also cache failures while indexing

[kadircet]

Summary

Clangd currently doesn't cache any indexing failures, which results in
retrying those failed files even if their contents haven't changed.

[sammccall]

Change looks sensible, just wondering if we should keep the main-file symbols/refs, and whether we can simplify/unify a bit.

clang-tools-extra/clangd/index/Background.cpp

359 ↗

(On Diff #207260)

couldn't we have a much more unified handling of error/non-error case, by simply dropping all non-main-file shards at this point? (in case of error)

In theory we're processing them a bit, but it practice it seems like they're empty.

clang-tools-extra/clangd/index/IndexAction.cpp

161 ↗

(On Diff #207260)

I'm pretty sure the include graph is basically good in this case (up to the possibly-missing files). So we should at least send that, so invalidation works (or can work in the future).

Also, we agreed not to overwrite the symbols for the included headers' shards, but what about the main file itself? Surely better to have something than nothing. (The "something" is roughly clang's recovery, which is what AST-based completions rely on anyway)

More generally, do we need filtering logic at both ends?
It seems like this file could just pass through the data it has, along with the info on whether there was an error (somehow)

163 ↗

(On Diff #207260)

comment belongs in the if, not the else

[kadircet]

• Address comments

[kadircet]

clang-tools-extra/clangd/index/IndexAction.cpp

161 ↗

(On Diff #207260)

Left the error detection behavior same(checking for uncompilable errors)

[sammccall]

Thanks!

clang-tools-extra/clangd/index/Background.cpp

140 ↗

(On Diff #207296)

(I'd suggest inlining this again as it's fairly clear straight-line code, but up to you)

313 ↗

(On Diff #207296)

Maybe add a FIXME that we should store other contents too, but only if the current shard contents are missing or also had errors.

500 ↗

(On Diff #207296)

why do we want to skip (maybe) rebuilding the index in this case?

500 ↗

(On Diff #207296)

I think this is now a "warning" rather than an error, maybe log something like "failed to compile {Cmd.Filename}, index may be incomplete"

[kadircet]

• Address comments

[kadircet]

clang-tools-extra/clangd/index/Background.cpp

313 ↗

(On Diff #207296)

I agree with storing contents if they are missing, but does it make sense to replace one broken shard information with other?

500 ↗

(On Diff #207296)

actually, we don't want to skip. I had written that part with the old assumption; that is, "we don't get any symbol info when compilation fails".

[sammccall]

Still LG

clang-tools-extra/clangd/index/Background.cpp

313 ↗

(On Diff #207296)

I'd say it makes sense if the content has changed.

i.e:

bool replace() {
  if old == null
    return true
  if !new.isTU && new.errors && !old.errors
    return false
  return new.digest != old.digest
}

(don't let this block you landing it, up to you)

[kadircet]

• Update fixme to mention error case in addition to the missing files.

clang-tools-extra/trunk/clangd/index/Background.h

public:
   static void preventThreadStarvationInTests();
 
 private:
   /// Given index results from a TU, only update symbols coming from files with
   /// different digests than \p DigestsSnapshot. Also stores new index
   /// information on IndexStorage.
   void update(llvm::StringRef MainFile, IndexFileIn Index,
               const llvm::StringMap<FileDigest> &DigestsSnapshot,
-              BackgroundIndexStorage *IndexStorage);
+              BackgroundIndexStorage *IndexStorage, bool HadErrors);
 
   // configuration
   const FileSystemProvider &FSProvider;
   const GlobalCompilationDatabase &CDB;
   Context BackgroundContext;
 
   // index state
   llvm::Error index(tooling::CompileCommand,

clang-tools-extra/trunk/clangd/index/Background.cpp

 //===-- Background.cpp - Build an index in a background thread ------------===//
 //
 // 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 "index/Background.h"
 #include "ClangdUnit.h"
 #include "Compiler.h"
+#include "Headers.h"
 #include "Logger.h"
+#include "Path.h"
 #include "SourceCode.h"
 #include "Symbol.h"
 #include "Threading.h"
 #include "Trace.h"
 #include "URI.h"
 #include "index/IndexAction.h"
 #include "index/MemIndex.h"
+#include "index/Ref.h"
+#include "index/Relation.h"
 #include "index/Serialization.h"
 #include "index/SymbolCollector.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSet.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/SHA1.h"
 #include "llvm/Support/Threading.h"
 
 #include <atomic>
 #include <chrono>
 #include <memory>
 #include <numeric>
 #include <queue>
void BackgroundIndex::enqueueTask(Task T, llvm::ThreadPriority Priority) {
   QueueCV.notify_all();
 }
 
 /// Given index results from a TU, only update symbols coming from files that
 /// are different or missing from than \p DigestsSnapshot. Also stores new index
 /// information on IndexStorage.
 void BackgroundIndex::update(llvm::StringRef MainFile, IndexFileIn Index,
                              const llvm::StringMap<FileDigest> &DigestsSnapshot,
-                             BackgroundIndexStorage *IndexStorage) {
+                             BackgroundIndexStorage *IndexStorage,
+                             bool HadErrors) {
   // Partition symbols/references into files.
   struct File {
     llvm::DenseSet<const Symbol *> Symbols;
     llvm::DenseSet<const Ref *> Refs;
     llvm::DenseSet<const Relation *> Relations;
     FileDigest Digest;
   };
   llvm::StringMap<File> Files;
   URIToFileCache URICache(MainFile);
   for (const auto &IndexIt : *Index.Sources) {
     const auto &IGN = IndexIt.getValue();
+    // In case of failures, we only store main file of TU. That way we can still
+    // get symbols from headers if some other TU can compile them. Note that
+    // sources do not contain any information regarding missing headers, since
+    // we don't even know what absolute path they should fall in.
+    // FIXME: Also store contents from other files whenever the current contents
+    // for those files are missing or if they had errors before.
+    if (HadErrors && !IGN.IsTU)
+      continue;
     const auto AbsPath = URICache.resolve(IGN.URI);
     const auto DigestIt = DigestsSnapshot.find(AbsPath);
     // File has different contents.
     if (DigestIt == DigestsSnapshot.end() || DigestIt->getValue() != IGN.Digest)
       Files.try_emplace(AbsPath).first->getValue().Digest = IGN.Digest;
   }
   // This map is used to figure out where to store relations.
   llvm::DenseMap<SymbolID, File *> SymbolIDToFile;
for (const auto *R : FileIt.second.Refs)
       Refs.insert(RefToIDs[R], *R);
     for (const auto *Rel : FileIt.second.Relations)
       Relations.insert(*Rel);
     auto SS = llvm::make_unique<SymbolSlab>(std::move(Syms).build());
     auto RS = llvm::make_unique<RefSlab>(std::move(Refs).build());
     auto RelS = llvm::make_unique<RelationSlab>(std::move(Relations).build());
     auto IG = llvm::make_unique<IncludeGraph>(
         getSubGraph(URI::create(Path), Index.Sources.getValue()));
+
     // We need to store shards before updating the index, since the latter
     // consumes slabs.
+    // FIXME: Also skip serializing the shard if it is already up-to-date.
     if (IndexStorage) {
       IndexFileOut Shard;
       Shard.Symbols = SS.get();
       Shard.Refs = RS.get();
       Shard.Relations = RelS.get();
       Shard.Sources = IG.get();
 
       if (auto Error = IndexStorage->storeShard(Path, Shard))
         elog("Failed to write background-index shard for file {0}: {1}", Path,
              std::move(Error));
     }
+
     {
       std::lock_guard<std::mutex> Lock(DigestsMu);
       auto Hash = FileIt.second.Digest;
       // Skip if file is already up to date.
       auto DigestIt = IndexedFileDigests.try_emplace(Path);
       if (!DigestIt.second && DigestIt.first->second == Hash)
         continue;
       DigestIt.first->second = Hash;
llvm::Error BackgroundIndex::index(tooling::CompileCommand Cmd,
   const FrontendInputFile &Input = Clang->getFrontendOpts().Inputs.front();
   if (!Action->BeginSourceFile(*Clang, Input))
     return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                    "BeginSourceFile() failed");
   if (llvm::Error Err = Action->Execute())
     return Err;
 
   Action->EndSourceFile();
-  if (Clang->hasDiagnostics() &&
-      Clang->getDiagnostics().hasUncompilableErrorOccurred()) {
-    return llvm::createStringError(
-        llvm::inconvertibleErrorCode(),
-        "IndexingAction failed: has uncompilable errors");
-  }
 
   assert(Index.Symbols && Index.Refs && Index.Sources &&
          "Symbols, Refs and Sources must be set.");
 
   log("Indexed {0} ({1} symbols, {2} refs, {3} files)",
       Inputs.CompileCommand.Filename, Index.Symbols->size(),
       Index.Refs->numRefs(), Index.Sources->size());
   SPAN_ATTACH(Tracer, "symbols", int(Index.Symbols->size()));
   SPAN_ATTACH(Tracer, "refs", int(Index.Refs->numRefs()));
   SPAN_ATTACH(Tracer, "sources", int(Index.Sources->size()));
 
-  update(AbsolutePath, std::move(Index), DigestsSnapshot, IndexStorage);
+  bool HadErrors = Clang->hasDiagnostics() &&
+                   Clang->getDiagnostics().hasUncompilableErrorOccurred();
+  update(AbsolutePath, std::move(Index), DigestsSnapshot, IndexStorage,
+         HadErrors);
+  if (HadErrors)
+    log("Failed to compile {0}, index may be incomplete", AbsolutePath);
 
   if (BuildIndexPeriodMs > 0)
     SymbolsUpdatedSinceLastIndex = true;
   else
     reset(
         IndexedSymbols.buildIndex(IndexType::Light, DuplicateHandling::Merge));
 
   return llvm::Error::success();
for (const auto &I : *Shard->Sources) {
       // Get the digest, skip it if file doesn't exist.
       auto Buf = FS->getBufferForFile(CurDependency.Path);
       if (!Buf) {
         elog("Couldn't get buffer for file: {0}: {1}", CurDependency.Path,
              Buf.getError().message());
         continue;
       }
       // If digests match then dependency doesn't need re-indexing.
+      // FIXME: Also check for dependencies(sources) of this shard and compile
+      // commands for cache invalidation.
       CurDependency.NeedsReIndexing =
           digest(Buf->get()->getBuffer()) != I.getValue().Digest;
     }
   }
   // Load shard information into background-index.
   {
     std::lock_guard<std::mutex> Lock(DigestsMu);
     // This can override a newer version that is added in another thread,

clang-tools-extra/trunk/clangd/index/IndexAction.cpp

 //===--- IndexAction.cpp -----------------------------------------*- 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 "IndexAction.h"
+#include "Logger.h"
+#include "index/Relation.h"
 #include "index/SymbolOrigin.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Index/IndexingAction.h"
 #include "clang/Tooling/Tooling.h"
+#include <utility>
 
 namespace clang {
 namespace clangd {
 namespace {
 
 llvm::Optional<std::string> toURI(const FileEntry *File) {
   if (!File)
     return llvm::None;
bool BeginInvocation(CompilerInstance &CI) override {
     CI.getDiagnostics().setIgnoreAllWarnings(true);
 
     return WrapperFrontendAction::BeginInvocation(CI);
   }
 
   void EndSourceFileAction() override {
     WrapperFrontendAction::EndSourceFileAction();
 
-    const auto &CI = getCompilerInstance();
-    if (CI.hasDiagnostics() &&
-        CI.getDiagnostics().hasUncompilableErrorOccurred()) {
-      llvm::errs() << "Skipping TU due to uncompilable errors\n";
-      return;
-    }
     SymbolsCallback(Collector->takeSymbols());
     if (RefsCallback != nullptr)
       RefsCallback(Collector->takeRefs());
     if (RelationsCallback != nullptr)
       RelationsCallback(Collector->takeRelations());
     if (IncludeGraphCallback != nullptr) {
 #ifndef NDEBUG
       // This checks if all nodes are initialized.

clang-tools-extra/trunk/clangd/index/Serialization.h

struct IndexFileOut {
   const IncludeGraph *Sources = nullptr;
   // TODO: Support serializing Dex posting lists.
   IndexFileFormat Format = IndexFileFormat::RIFF;
 
   IndexFileOut() = default;
   IndexFileOut(const IndexFileIn &I)
       : Symbols(I.Symbols ? I.Symbols.getPointer() : nullptr),
         Refs(I.Refs ? I.Refs.getPointer() : nullptr),
-        Relations(I.Relations ? I.Relations.getPointer() : nullptr) {}
+        Relations(I.Relations ? I.Relations.getPointer() : nullptr),
+        Sources(I.Sources ? I.Sources.getPointer() : nullptr) {}
 };
 // Serializes an index file.
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const IndexFileOut &O);
 
 // Convert a single symbol to YAML, a nice debug representation.
 std::string toYAML(const Symbol &);
 std::string toYAML(const std::pair<SymbolID, ArrayRef<Ref>> &);
 std::string toYAML(const Relation &);

clang-tools-extra/trunk/clangd/unittests/BackgroundIndexTests.cpp

TEST_F(BackgroundIndexTest, NoDotsInAbsPath) {
 
   ASSERT_TRUE(Idx.blockUntilIdleForTest());
   for (llvm::StringRef AbsPath : MSS.AccessedPaths.keys()) {
     EXPECT_FALSE(AbsPath.contains("./")) << AbsPath;
     EXPECT_FALSE(AbsPath.contains("../")) << AbsPath;
   }
 }
 
+TEST_F(BackgroundIndexTest, UncompilableFiles) {
+  MockFSProvider FS;
+  llvm::StringMap<std::string> Storage;
+  size_t CacheHits = 0;
+  MemoryShardStorage MSS(Storage, CacheHits);
+  OverlayCDB CDB(/*Base=*/nullptr);
+  BackgroundIndex Idx(Context::empty(), FS, CDB,
+                      [&](llvm::StringRef) { return &MSS; });
+
+  tooling::CompileCommand Cmd;
+  FS.Files[testPath("A.h")] = "void foo();";
+  FS.Files[testPath("B.h")] = "#include \"C.h\"\nasdf;";
+  FS.Files[testPath("C.h")] = "";
+  FS.Files[testPath("A.cc")] = R"cpp(
+  #include "A.h"
+  #include "B.h"
+  #include "not_found_header.h"
+
+  void foo() {}
+  )cpp";
+  Cmd.Filename = "../A.cc";
+  Cmd.Directory = testPath("build");
+  Cmd.CommandLine = {"clang++", "../A.cc"};
+  CDB.setCompileCommand(testPath("build/../A.cc"), Cmd);
+  ASSERT_TRUE(Idx.blockUntilIdleForTest());
+
+  // Make sure we only store the shard for main file.
+  EXPECT_THAT(Storage.keys(), ElementsAre(testPath("A.cc")));
+  auto Shard = MSS.loadShard(testPath("A.cc"));
+  EXPECT_THAT(*Shard->Symbols, UnorderedElementsAre(Named("foo")));
+  EXPECT_THAT(Shard->Sources->keys(),
+              UnorderedElementsAre("unittest:///A.cc", "unittest:///A.h",
+                                   "unittest:///B.h"));
+}
+
 } // namespace clangd
 } // namespace clang