[clangd] Make HadErrors part of background index's internal state

[sammccall]

Logic looks great, just naming/comment nits

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

276

, or we indexed successfully this time.

444

I'm happy with this inline or out-of-line, but if it's inline here it shouldn't have a function-style comment with \p :-)

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

98

I wonder whether a more "flavorful" name like "ShardVersion" might help explain the purpose here?

125

It's not obvious to me why we're apologising for this not being the include graph. The data happens to come from there (kind of) but I wouldn't really mention it.

128

Again, I think "Versions" would make it more obvious what this is for.

129

rename mutex to match guarded variable

[kadircet]

• Address comments

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

 //===--- Background.h - Build an index in a background thread ----*- 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_BACKGROUND_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_BACKGROUND_H
 
 #include "Context.h"
 #include "FSProvider.h"
 #include "GlobalCompilationDatabase.h"
+#include "SourceCode.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/Support/SHA1.h"
 #include "llvm/Support/Threading.h"
public:
   LLVM_NODISCARD bool
   blockUntilIdleForTest(llvm::Optional<double> TimeoutSeconds = 10);
 
   // Disables thread priority lowering in background index to make sure it can
   // progress on loaded systems. Only affects tasks that run after the call.
   static void preventThreadStarvationInTests();
 
 private:
+  /// Represents the state of a single file when indexing was performed.
+  struct IndexedFile {
+    FileDigest Digest{0};
+    bool HadErrors = false;
+  };
+
   /// Given index results from a TU, only update symbols coming from files with
-  /// different digests than \p DigestsSnapshot. Also stores new index
+  /// different digests than \p IndexedFilesSnapshot. Also stores new index
   /// information on IndexStorage.
   void update(llvm::StringRef MainFile, IndexFileIn Index,
-              const llvm::StringMap<FileDigest> &DigestsSnapshot,
+              const llvm::StringMap<IndexedFile> &IndexedFilesSnapshot,
               BackgroundIndexStorage *IndexStorage, bool HadErrors);
 
   // configuration
   const FileSystemProvider &FSProvider;
   const GlobalCompilationDatabase &CDB;
   Context BackgroundContext;
 
   // index state
   llvm::Error index(tooling::CompileCommand,
                     BackgroundIndexStorage *IndexStorage);
   void buildIndex(); // Rebuild index periodically every BuildIndexPeriodMs.
   const size_t BuildIndexPeriodMs;
   std::atomic<bool> SymbolsUpdatedSinceLastIndex;
   std::mutex IndexMu;
   std::condition_variable IndexCV;
 
   FileSymbols IndexedSymbols;
-  llvm::StringMap<FileDigest> IndexedFileDigests; // Key is absolute file path.
+  // We do not store the whole IncludeGraph since we only care about Digest and
+  // Errorness of each source file. We might start storing IncludeGraph directly
+  // once there are use cases for additional information in include graph.
+  llvm::StringMap<IndexedFile> IndexedFiles; // Key is absolute file path.
   std::mutex DigestsMu;
 
   BackgroundIndexStorage::Factory IndexStorageFactory;
   struct Source {
     std::string Path;
     bool NeedsReIndexing;
     Source(llvm::StringRef Path, bool NeedsReIndexing)
         : Path(Path), NeedsReIndexing(NeedsReIndexing) {}
   };

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

for (auto &Include : Node.DirectIncludes) {
     auto I = IG.try_emplace(Include).first;
     I->getValue().URI = I->getKey();
     Include = I->getKey();
   }
 
   return IG;
 }
 
-// Creates a filter to not collect index results from files with unchanged
-// digests.
-// \p FileDigests contains file digests for the current indexed files.
-decltype(SymbolCollector::Options::FileFilter)
-createFileFilter(const llvm::StringMap<FileDigest> &FileDigests) {
-  return [&FileDigests](const SourceManager &SM, FileID FID) {
-    const auto *F = SM.getFileEntryForID(FID);
-    if (!F)
-      return false; // Skip invalid files.
-    auto AbsPath = getCanonicalPath(F, SM);
-    if (!AbsPath)
-      return false; // Skip files without absolute path.
-    auto Digest = digestFile(SM, FID);
-    if (!Digest)
-      return false;
-    auto D = FileDigests.find(*AbsPath);
-    if (D != FileDigests.end() && D->second == Digest)
-      return false; // Skip files that haven't changed.
-    return true;
-  };
-}
-
 // We cannot use vfs->makeAbsolute because Cmd.FileName is either absolute or
 // relative to Cmd.Directory, which might not be the same as current working
 // directory.
 llvm::SmallString<128> getAbsolutePath(const tooling::CompileCommand &Cmd) {
   llvm::SmallString<128> AbsolutePath;
   if (llvm::sys::path::is_absolute(Cmd.Filename)) {
     AbsolutePath = Cmd.Filename;
   } else {
if (Priority == llvm::ThreadPriority::Default) {
           });
     }
     Queue.insert(I, {std::move(T), 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,
-                             bool HadErrors) {
+/// are different or missing from than \p IndexedFilesSnapshot. Also stores new
+/// index information on IndexStorage.
+void BackgroundIndex::update(
+    llvm::StringRef MainFile, IndexFileIn Index,
+    const llvm::StringMap<IndexedFile> &IndexedFilesSnapshot,
+    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.Flags & IncludeGraphNode::SourceFlag::IsTU))
-      continue;
+    // Note that sources do not contain any information regarding missing
+    // headers, since we don't even know what absolute path they should fall in.
     const auto AbsPath = URICache.resolve(IGN.URI);
-    const auto DigestIt = DigestsSnapshot.find(AbsPath);
+    const auto DigestIt = IndexedFilesSnapshot.find(AbsPath);
     // File has different contents.
-    if (DigestIt == DigestsSnapshot.end() || DigestIt->getValue() != IGN.Digest)
+    if (DigestIt == IndexedFilesSnapshot.end() ||
+        DigestIt->getValue().Digest != IGN.Digest ||
+        (DigestIt->getValue().HadErrors && !HadErrors))
       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 (IndexStorage) {
       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)
+      auto DigestIt = IndexedFiles.try_emplace(Path);
+      IndexedFile &IF = DigestIt.first->second;
+      // Skip if file is already up to date, unless previous index was broken
+      // and this one is not.
+      if (!DigestIt.second && IF.Digest == Hash && IF.HadErrors && !HadErrors)
         continue;
-      DigestIt.first->second = Hash;
+      IF.Digest = Hash;
+      IF.HadErrors = HadErrors;
+
       // This can override a newer version that is added in another thread, if
       // this thread sees the older version but finishes later. This should be
       // rare in practice.
       IndexedSymbols.update(Path, std::move(SS), std::move(RS), std::move(RelS),
                             Path == MainFile);
     }
   }
 }
llvm::Error BackgroundIndex::index(tooling::CompileCommand Cmd,
 
   auto FS = FSProvider.getFileSystem();
   auto Buf = FS->getBufferForFile(AbsolutePath);
   if (!Buf)
     return llvm::errorCodeToError(Buf.getError());
   auto Hash = digest(Buf->get()->getBuffer());
 
   // Take a snapshot of the digests to avoid locking for each file in the TU.
-  llvm::StringMap<FileDigest> DigestsSnapshot;
+  llvm::StringMap<IndexedFile> IndexedFilesSnapshot;
   {
     std::lock_guard<std::mutex> Lock(DigestsMu);
-    DigestsSnapshot = IndexedFileDigests;
+    IndexedFilesSnapshot = IndexedFiles;
   }
 
   vlog("Indexing {0} (digest:={1})", Cmd.Filename, llvm::toHex(Hash));
   ParseInputs Inputs;
   Inputs.FS = std::move(FS);
   Inputs.FS->setCurrentWorkingDirectory(Cmd.Directory);
   Inputs.CompileCommand = std::move(Cmd);
   auto CI = buildCompilerInvocation(Inputs);
   if (!CI)
     return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                    "Couldn't build compiler invocation");
   IgnoreDiagnostics IgnoreDiags;
   auto Clang = prepareCompilerInstance(std::move(CI), /*Preamble=*/nullptr,
                                        std::move(*Buf), Inputs.FS, IgnoreDiags);
   if (!Clang)
     return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                    "Couldn't build compiler instance");
 
   SymbolCollector::Options IndexOpts;
-  IndexOpts.FileFilter = createFileFilter(DigestsSnapshot);
+  // Creates a filter to not collect index results from files with unchanged
+  // digests.
+  // \p FileDigestsSnapshot contains in memory state.
+  IndexOpts.FileFilter = [&IndexedFilesSnapshot](const SourceManager &SM,
+                                                 FileID FID) {
+    const auto *F = SM.getFileEntryForID(FID);
+    if (!F)
+      return false; // Skip invalid files.
+    auto AbsPath = getCanonicalPath(F, SM);
+    if (!AbsPath)
+      return false; // Skip files without absolute path.
+    auto Digest = digestFile(SM, FID);
+    if (!Digest)
+      return false;
+    auto D = IndexedFilesSnapshot.find(*AbsPath);
+    if (D != IndexedFilesSnapshot.end() && D->second.Digest == Digest &&
+        !D->second.HadErrors)
+      return false; // Skip files that haven't changed, without errors.
+    return true;
+  };
+
   IndexFileIn Index;
   auto Action = createStaticIndexingAction(
       IndexOpts, [&](SymbolSlab S) { Index.Symbols = std::move(S); },
       [&](RefSlab R) { Index.Refs = std::move(R); },
       [&](RelationSlab R) { Index.Relations = std::move(R); },
       [&](IncludeGraph IG) { Index.Sources = std::move(IG); });
 
   // We're going to run clang here, and it could potentially crash.
llvm::Error BackgroundIndex::index(tooling::CompileCommand Cmd,
 
   bool HadErrors = Clang->hasDiagnostics() &&
                    Clang->getDiagnostics().hasUncompilableErrorOccurred();
   if (HadErrors) {
     log("Failed to compile {0}, index may be incomplete", AbsolutePath);
     for (auto &It : *Index.Sources)
       It.second.Flags |= IncludeGraphNode::SourceFlag::HadErrors;
   }
-  update(AbsolutePath, std::move(Index), DigestsSnapshot, IndexStorage,
+  update(AbsolutePath, std::move(Index), IndexedFilesSnapshot, IndexStorage,
          HadErrors);
 
   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,
                            llvm::StringSet<> &LoadedShards) {
   struct ShardInfo {
     std::string AbsolutePath;
     std::unique_ptr<IndexFileIn> Shard;
     FileDigest Digest = {};
     bool CountReferences = false;
+    bool HadErrors = false;
   };
   std::vector<ShardInfo> IntermediateSymbols;
   // Make sure we don't have duplicate elements in the queue. Keys are absolute
   // paths.
   llvm::StringSet<> InQueue;
   auto FS = FSProvider.getFileSystem();
   // Dependencies of this TU, paired with the information about whether they
   // need to be re-indexed or not.
for (const auto &I : *Shard->Sources) {
       // We found source file info for current dependency.
       assert(I.getValue().Digest != FileDigest{{0}} && "Digest is empty?");
       ShardInfo SI;
       SI.AbsolutePath = CurDependency.Path;
       SI.Shard = std::move(Shard);
       SI.Digest = I.getValue().Digest;
       SI.CountReferences =
           I.getValue().Flags & IncludeGraphNode::SourceFlag::IsTU;
+      SI.HadErrors =
+          I.getValue().Flags & IncludeGraphNode::SourceFlag::HadErrors;
       IntermediateSymbols.push_back(std::move(SI));
       // Check if the source needs re-indexing.
       // 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;
for (const ShardInfo &SI : IntermediateSymbols) {
               : nullptr;
       auto RS = SI.Shard->Refs
                     ? llvm::make_unique<RefSlab>(std::move(*SI.Shard->Refs))
                     : nullptr;
       auto RelS =
           SI.Shard->Relations
               ? llvm::make_unique<RelationSlab>(std::move(*SI.Shard->Relations))
               : nullptr;
-      IndexedFileDigests[SI.AbsolutePath] = SI.Digest;
+      IndexedFile &IF = IndexedFiles[SI.AbsolutePath];
+      IF.Digest = SI.Digest;
+      IF.HadErrors = SI.HadErrors;
+
       IndexedSymbols.update(SI.AbsolutePath, std::move(SS), std::move(RS),
                             std::move(RelS), SI.CountReferences);
     }
   }
 
   return Dependencies;
 }
 

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

TEST_F(BackgroundIndexTest, UncompilableFiles) {
   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")));
+  EXPECT_THAT(Storage.keys(), ElementsAre(testPath("A.cc"), testPath("A.h"),
+                                          testPath("B.h"), testPath("C.h")));
+
+  {
     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"));
-
     EXPECT_THAT(Shard->Sources->lookup("unittest:///A.cc"), HadErrors());
-  // FIXME: We should also persist headers while marking them with errors.
-  EXPECT_THAT(Shard->Sources->lookup("unittest:///A.h"), Not(HadErrors()));
-  EXPECT_THAT(Shard->Sources->lookup("unittest:///B.h"), Not(HadErrors()));
+  }
+
+  {
+    auto Shard = MSS.loadShard(testPath("A.h"));
+    EXPECT_THAT(*Shard->Symbols, UnorderedElementsAre(Named("foo")));
+    EXPECT_THAT(Shard->Sources->keys(),
+                UnorderedElementsAre("unittest:///A.h"));
+    EXPECT_THAT(Shard->Sources->lookup("unittest:///A.h"), HadErrors());
+  }
+
+  {
+    auto Shard = MSS.loadShard(testPath("B.h"));
+    EXPECT_THAT(*Shard->Symbols, UnorderedElementsAre(Named("asdf")));
+    EXPECT_THAT(Shard->Sources->keys(),
+                UnorderedElementsAre("unittest:///B.h", "unittest:///C.h"));
+    EXPECT_THAT(Shard->Sources->lookup("unittest:///B.h"), HadErrors());
+  }
+
+  {
+    auto Shard = MSS.loadShard(testPath("C.h"));
+    EXPECT_THAT(*Shard->Symbols, UnorderedElementsAre());
+    EXPECT_THAT(Shard->Sources->keys(),
+                UnorderedElementsAre("unittest:///C.h"));
+    EXPECT_THAT(Shard->Sources->lookup("unittest:///C.h"), HadErrors());
+  }
 }
 
 TEST_F(BackgroundIndexTest, CmdLineHash) {
   MockFSProvider FS;
   llvm::StringMap<std::string> Storage;
   size_t CacheHits = 0;
   MemoryShardStorage MSS(Storage, CacheHits);
   OverlayCDB CDB(/*Base=*/nullptr, /*FallbackFlags=*/{},