[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

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

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

comment belongs in the if, not the else

[kadircet]

• Address comments

[kadircet]

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

161

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

[sammccall]

Thanks!

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

140

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

317

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

511

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

511

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

317

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

511

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

317

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/clangd/index/Background.h

 #include "FSProvider.h"
 #include "GlobalCompilationDatabase.h"
 #include "Threading.h"
 #include "index/FileIndex.h"
 #include "index/Index.h"
 #include "index/Serialization.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/Support/SHA1.h"
 #include "llvm/Support/Threading.h"
 #include <atomic>
 #include <condition_variable>
 #include <deque>
 #include <mutex>
 #include <string>
 #include <thread>

clang-tools-extra/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>
if (llvm::sys::path::is_absolute(Cmd.Filename)) {
     AbsolutePath = Cmd.Filename;
   } else {
     AbsolutePath = Cmd.Directory;
     llvm::sys::path::append(AbsolutePath, Cmd.Filename);
     llvm::sys::path::remove_dots(AbsolutePath, true);
   }
   return AbsolutePath;
 }
+
+void storeSlabs(BackgroundIndexStorage *IndexStorage, PathRef Identifier,
+                IndexFileOut Out) {
+  if (!IndexStorage)
+    return;
+  if (auto Error = IndexStorage->storeShard(Identifier, Out))
+    elog("Failed to write background-index shard for file {0}: {1}", Identifier,
+         std::move(Error));
+}
+
+void storeSlabs(BackgroundIndexStorage *IndexStorage, PathRef Identifier,
+                SymbolSlab *Syms, RefSlab *Refs, RelationSlab *Rels,
+                IncludeGraph *Sources) {
+  IndexFileOut Out;
+  Out.Symbols = Syms;
+  Out.Refs = Refs;
+  Out.Relations = Rels;
+  Out.Sources = Sources;
+  storeSlabs(IndexStorage, Identifier, Out);
+}
+
 } // namespace
 
 BackgroundIndex::BackgroundIndex(
     Context BackgroundContext, const FileSystemProvider &FSProvider,
     const GlobalCompilationDatabase &CDB,
     BackgroundIndexStorage::Factory IndexStorageFactory,
     size_t BuildIndexPeriodMs, size_t ThreadPoolSize)
     : SwapIndex(llvm::make_unique<MemIndex>()), FSProvider(FSProvider),
void BackgroundIndex::update(llvm::StringRef MainFile, IndexFileIn Index,
   llvm::StringMap<File> Files;
   URIToFileCache URICache(MainFile);
   for (const auto &IndexIt : *Index.Sources) {
     const auto &IGN = IndexIt.getValue();
     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 &Sym : *Index.Symbols) {
     if (Sym.CanonicalDeclaration) {
       auto DeclPath = URICache.resolve(Sym.CanonicalDeclaration.FileURI);
       const auto FileIt = Files.find(DeclPath);
       if (FileIt != Files.end()) {
for (const auto &R : SymRefs.second) {
       }
     }
   }
   for (const auto &Rel : *Index.Relations) {
     const auto FileIt = SymbolIDToFile.find(Rel.Subject);
     if (FileIt != SymbolIDToFile.end())
       FileIt->second->Relations.insert(&Rel);
   }
 
   // Build and store new slabs for each updated file.
   for (const auto &FileIt : Files) {
     llvm::StringRef Path = FileIt.getKey();
     SymbolSlab::Builder Syms;
     RefSlab::Builder Refs;
     RelationSlab::Builder Relations;
     for (const auto *S : FileIt.second.Symbols)
       Syms.insert(*S);
     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.
-    if (IndexStorage) {
-      IndexFileOut Shard;
-      Shard.Symbols = SS.get();
-      Shard.Refs = RS.get();
-      Shard.Relations = RelS.get();
-      Shard.Sources = IG.get();
+    storeSlabs(IndexStorage, Path, SS.get(), RS.get(), RelS.get(), 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()));
 
+  if (Clang->hasDiagnostics() &&
+      Clang->getDiagnostics().hasUncompilableErrorOccurred()) {
+    // In case of failures, we don't shard index into files but rather save
+    // everything for the 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.
+    storeSlabs(IndexStorage, AbsolutePath, std::move(Index));
+    return llvm::createStringError(
+        llvm::inconvertibleErrorCode(),
+        "IndexingAction failed: has uncompilable errors");
+  }
+
   update(AbsolutePath, std::move(Index), DigestsSnapshot, IndexStorage);
 
   if (BuildIndexPeriodMs > 0)
     SymbolsUpdatedSinceLastIndex = true;
   else
     reset(
         IndexedSymbols.buildIndex(IndexType::Light, DuplicateHandling::Merge));
 
   return llvm::Error::success();
 }
 
 std::vector<BackgroundIndex::Source>
 BackgroundIndex::loadShard(const tooling::CompileCommand &Cmd,
                            BackgroundIndexStorage *IndexStorage,
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/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 "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();
 
+    SymbolSlab Syms;
+    RefSlab Refs;
+    RelationSlab Rels;
+
     const auto &CI = getCompilerInstance();
     if (CI.hasDiagnostics() &&
         CI.getDiagnostics().hasUncompilableErrorOccurred()) {
       llvm::errs() << "Skipping TU due to uncompilable errors\n";
-      return;
+    } else {
+      // Return empty information for non-compilable TUs.
+      Syms = Collector->takeSymbols();
+      Refs = Collector->takeRefs();
+      Rels = Collector->takeRelations();
     }
-    SymbolsCallback(Collector->takeSymbols());
+
+    SymbolsCallback(std::move(Syms));
     if (RefsCallback != nullptr)
-      RefsCallback(Collector->takeRefs());
+      RefsCallback(std::move(Refs));
     if (RelationsCallback != nullptr)
-      RelationsCallback(Collector->takeRelations());
+      RelationsCallback(std::move(Rels));
     if (IncludeGraphCallback != nullptr) {
 #ifndef NDEBUG
       // This checks if all nodes are initialized.
       for (const auto &Node : IG)
         assert(Node.getKeyData() == Node.getValue().URI.data());
 #endif
       IncludeGraphCallback(std::move(IG));
     }

clang-tools-extra/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/clangd/unittests/BackgroundIndexTests.cpp

 #include "SyncAPI.h"
 #include "TestFS.h"
 #include "TestTU.h"
 #include "index/Background.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "llvm/Support/Threading.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <thread>
 
 using ::testing::_;
 using ::testing::AllOf;
 using ::testing::Contains;
 using ::testing::ElementsAre;
+using ::testing::IsEmpty;
 using ::testing::Not;
 using ::testing::UnorderedElementsAre;
 
 namespace clang {
 namespace clangd {
 
 MATCHER_P(Named, N, "") { return arg.Name == N; }
 MATCHER(Declared, "") {
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());
+
+  EXPECT_THAT(Storage.keys(), ElementsAre(testPath("A.cc")));
+
+  auto Shard = MSS.loadShard(testPath("A.cc"));
+  EXPECT_THAT(*Shard->Symbols, IsEmpty());
+  EXPECT_THAT(Shard->Sources->keys(),
+              UnorderedElementsAre("unittest:///A.cc", "unittest:///A.h",
+                                   "unittest:///B.h", "unittest:///C.h"));
+}
+
 } // namespace clangd
 } // namespace clang