diff --git a/clang-tools-extra/clangd/index/Background.h b/clang-tools-extra/clangd/index/Background.h --- a/clang-tools-extra/clangd/index/Background.h +++ b/clang-tools-extra/clangd/index/Background.h @@ -12,6 +12,7 @@ #include "Context.h" #include "FSProvider.h" #include "GlobalCompilationDatabase.h" +#include "SourceCode.h" #include "Threading.h" #include "index/FileIndex.h" #include "index/Index.h" @@ -93,11 +94,17 @@ 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 &DigestsSnapshot, + const llvm::StringMap &IndexedFilesSnapshot, BackgroundIndexStorage *IndexStorage, bool HadErrors); // configuration @@ -115,7 +122,10 @@ std::condition_variable IndexCV; FileSymbols IndexedSymbols; - llvm::StringMap 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 IndexedFiles; // Key is absolute file path. std::mutex DigestsMu; BackgroundIndexStorage::Factory IndexStorageFactory; diff --git a/clang-tools-extra/clangd/index/Background.cpp b/clang-tools-extra/clangd/index/Background.cpp --- a/clang-tools-extra/clangd/index/Background.cpp +++ b/clang-tools-extra/clangd/index/Background.cpp @@ -101,28 +101,6 @@ 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 &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. @@ -274,12 +252,12 @@ } /// 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 &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 &IndexedFilesSnapshot, + BackgroundIndexStorage *IndexStorage, bool HadErrors) { // Partition symbols/references into files. struct File { llvm::DenseSet Symbols; @@ -291,18 +269,14 @@ 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. @@ -387,11 +361,15 @@ { std::lock_guard 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. @@ -440,10 +418,10 @@ auto Hash = digest(Buf->get()->getBuffer()); // Take a snapshot of the digests to avoid locking for each file in the TU. - llvm::StringMap DigestsSnapshot; + llvm::StringMap IndexedFilesSnapshot; { std::lock_guard Lock(DigestsMu); - DigestsSnapshot = IndexedFileDigests; + IndexedFilesSnapshot = IndexedFiles; } vlog("Indexing {0} (digest:={1})", Cmd.Filename, llvm::toHex(Hash)); @@ -463,7 +441,27 @@ "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); }, @@ -503,7 +501,7 @@ 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) @@ -524,6 +522,7 @@ std::unique_ptr Shard; FileDigest Digest = {}; bool CountReferences = false; + bool HadErrors = false; }; std::vector IntermediateSymbols; // Make sure we don't have duplicate elements in the queue. Keys are absolute @@ -586,6 +585,8 @@ 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. @@ -620,7 +621,10 @@ SI.Shard->Relations ? llvm::make_unique(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); } diff --git a/clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp b/clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp --- a/clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp +++ b/clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp @@ -524,18 +524,41 @@ 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")); - - 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())); + 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()); + } + + { + 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) {