[clang-tools-extra] r351170 - [clangd] Fix updated file detection logic in indexing
Kadir Cetinkaya via cfe-commits
cfe-commits at lists.llvm.org
Tue Jan 15 01:03:34 PST 2019
Author: kadircet
Date: Tue Jan 15 01:03:33 2019
New Revision: 351170
URL: http://llvm.org/viewvc/llvm-project?rev=351170&view=rev
Log:
[clangd] Fix updated file detection logic in indexing
Summary:
Files without any symbols were never marked as updated during indexing, which resulted in failure while writing shards for these files.
This patch fixes the logic to mark files that are seen for the first time but don't contain any symbols as updated.
Reviewers: ilya-biryukov
Reviewed By: ilya-biryukov
Subscribers: ioeric, MaskRay, jkorous, arphaman, cfe-commits
Differential Revision: https://reviews.llvm.org/D56592
Modified:
clang-tools-extra/trunk/clangd/index/Background.cpp
clang-tools-extra/trunk/clangd/index/Background.h
clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp
Modified: clang-tools-extra/trunk/clangd/index/Background.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Background.cpp?rev=351170&r1=351169&r2=351170&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/index/Background.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/Background.cpp Tue Jan 15 01:03:33 2019
@@ -91,12 +91,10 @@ IncludeGraph getSubGraph(const URI &U, c
// Creates a filter to not collect index results from files with unchanged
// digests.
-// \p FileDigests contains file digests for the current indexed files, and all
-// changed files will be added to \p FilesToUpdate.
+// \p FileDigests contains file digests for the current indexed files.
decltype(SymbolCollector::Options::FileFilter)
-createFileFilter(const llvm::StringMap<FileDigest> &FileDigests,
- llvm::StringMap<FileDigest> &FilesToUpdate) {
- return [&FileDigests, &FilesToUpdate](const SourceManager &SM, FileID FID) {
+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.
@@ -109,8 +107,6 @@ createFileFilter(const llvm::StringMap<F
auto D = FileDigests.find(*AbsPath);
if (D != FileDigests.end() && D->second == Digest)
return false; // Skip files that haven't changed.
-
- FilesToUpdate[*AbsPath] = *Digest;
return true;
};
}
@@ -264,22 +260,34 @@ void BackgroundIndex::enqueueTask(Task T
QueueCV.notify_all();
}
-/// Given index results from a TU, only update files in \p FilesToUpdate.
+/// 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> &FilesToUpdate,
+ const llvm::StringMap<FileDigest> &DigestsSnapshot,
BackgroundIndexStorage *IndexStorage) {
// Partition symbols/references into files.
struct File {
llvm::DenseSet<const Symbol *> Symbols;
llvm::DenseSet<const Ref *> Refs;
+ FileDigest Digest;
};
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;
+ }
for (const auto &Sym : *Index.Symbols) {
if (Sym.CanonicalDeclaration) {
auto DeclPath = URICache.resolve(Sym.CanonicalDeclaration.FileURI);
- if (FilesToUpdate.count(DeclPath) != 0)
- Files[DeclPath].Symbols.insert(&Sym);
+ const auto FileIt = Files.find(DeclPath);
+ if (FileIt != Files.end())
+ FileIt->second.Symbols.insert(&Sym);
}
// For symbols with different declaration and definition locations, we store
// the full symbol in both the header file and the implementation file, so
@@ -288,16 +296,18 @@ void BackgroundIndex::update(llvm::Strin
if (Sym.Definition &&
Sym.Definition.FileURI != Sym.CanonicalDeclaration.FileURI) {
auto DefPath = URICache.resolve(Sym.Definition.FileURI);
- if (FilesToUpdate.count(DefPath) != 0)
- Files[DefPath].Symbols.insert(&Sym);
+ const auto FileIt = Files.find(DefPath);
+ if (FileIt != Files.end())
+ FileIt->second.Symbols.insert(&Sym);
}
}
llvm::DenseMap<const Ref *, SymbolID> RefToIDs;
for (const auto &SymRefs : *Index.Refs) {
for (const auto &R : SymRefs.second) {
auto Path = URICache.resolve(R.Location.FileURI);
- if (FilesToUpdate.count(Path) != 0) {
- auto &F = Files[Path];
+ const auto FileIt = Files.find(Path);
+ if (FileIt != Files.end()) {
+ auto &F = FileIt->getValue();
RefToIDs[&R] = SymRefs.first;
F.Refs.insert(&R);
}
@@ -305,18 +315,14 @@ void BackgroundIndex::update(llvm::Strin
}
// Build and store new slabs for each updated file.
- for (const auto &I : *Index.Sources) {
- std::string Path = URICache.resolve(I.first());
+ for (const auto &FileIt : Files) {
+ llvm::StringRef Path = FileIt.getKey();
SymbolSlab::Builder Syms;
RefSlab::Builder Refs;
- auto FileIt = Files.find(Path);
- if (FileIt != Files.end()) {
- auto &F = *FileIt;
- for (const auto *S : F.second.Symbols)
- Syms.insert(*S);
- for (const auto *R : F.second.Refs)
- Refs.insert(RefToIDs[R], *R);
- }
+ for (const auto *S : FileIt.second.Symbols)
+ Syms.insert(*S);
+ for (const auto *R : FileIt.second.Refs)
+ Refs.insert(RefToIDs[R], *R);
auto SS = llvm::make_unique<SymbolSlab>(std::move(Syms).build());
auto RS = llvm::make_unique<RefSlab>(std::move(Refs).build());
auto IG = llvm::make_unique<IncludeGraph>(
@@ -335,7 +341,7 @@ void BackgroundIndex::update(llvm::Strin
}
{
std::lock_guard<std::mutex> Lock(DigestsMu);
- auto Hash = I.second.Digest;
+ 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)
@@ -410,8 +416,7 @@ llvm::Error BackgroundIndex::index(tooli
"Couldn't build compiler instance");
SymbolCollector::Options IndexOpts;
- llvm::StringMap<FileDigest> FilesToUpdate;
- IndexOpts.FileFilter = createFileFilter(DigestsSnapshot, FilesToUpdate);
+ IndexOpts.FileFilter = createFileFilter(DigestsSnapshot);
IndexFileIn Index;
auto Action = createStaticIndexingAction(
IndexOpts, [&](SymbolSlab S) { Index.Symbols = std::move(S); },
@@ -448,7 +453,7 @@ llvm::Error BackgroundIndex::index(tooli
SPAN_ATTACH(Tracer, "refs", int(Index.Refs->numRefs()));
SPAN_ATTACH(Tracer, "sources", int(Index.Sources->size()));
- update(AbsolutePath, std::move(Index), FilesToUpdate, IndexStorage);
+ update(AbsolutePath, std::move(Index), DigestsSnapshot, IndexStorage);
if (BuildIndexPeriodMs > 0)
SymbolsUpdatedSinceLastIndex = true;
Modified: clang-tools-extra/trunk/clangd/index/Background.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Background.h?rev=351170&r1=351169&r2=351170&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/index/Background.h (original)
+++ clang-tools-extra/trunk/clangd/index/Background.h Tue Jan 15 01:03:33 2019
@@ -91,10 +91,11 @@ public:
blockUntilIdleForTest(llvm::Optional<double> TimeoutSeconds = 10);
private:
- /// Given index results from a TU, only update files in \p FilesToUpdate.
- /// Also stores new index information on IndexStorage.
+ /// 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> &FilesToUpdate,
+ const llvm::StringMap<FileDigest> &DigestsSnapshot,
BackgroundIndexStorage *IndexStorage);
// configuration
Modified: clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp?rev=351170&r1=351169&r2=351170&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp Tue Jan 15 01:03:33 2019
@@ -351,11 +351,82 @@ TEST_F(BackgroundIndexTest, ShardStorage
EXPECT_EQ(CacheHits, 2U); // Check both A.cc and A.h loaded from cache.
// Check if the new symbol has arrived.
- auto ShardSource = MSS.loadShard(testPath("root/A.cc"));
+ ShardHeader = MSS.loadShard(testPath("root/A.h"));
EXPECT_NE(ShardHeader, nullptr);
+ EXPECT_THAT(*ShardHeader->Symbols, Contains(Named("A_CCnew")));
+ auto ShardSource = MSS.loadShard(testPath("root/A.cc"));
+ EXPECT_NE(ShardSource, nullptr);
EXPECT_THAT(*ShardSource->Symbols,
Contains(AllOf(Named("f_b"), Declared(), Defined())));
}
+TEST_F(BackgroundIndexTest, ShardStorageEmptyFile) {
+ MockFSProvider FS;
+ FS.Files[testPath("root/A.h")] = R"cpp(
+ void common();
+ void f_b();
+ class A_CC {};
+ )cpp";
+ FS.Files[testPath("root/B.h")] = R"cpp(
+ #include "A.h"
+ )cpp";
+ FS.Files[testPath("root/A.cc")] =
+ "#include \"B.h\"\nvoid g() { (void)common; }";
+
+ llvm::StringMap<std::string> Storage;
+ size_t CacheHits = 0;
+ MemoryShardStorage MSS(Storage, CacheHits);
+
+ tooling::CompileCommand Cmd;
+ Cmd.Filename = testPath("root/A.cc");
+ Cmd.Directory = testPath("root");
+ Cmd.CommandLine = {"clang++", testPath("root/A.cc")};
+ // Check that A.cc, A.h and B.h has been stored.
+ {
+ OverlayCDB CDB(/*Base=*/nullptr);
+ BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+ [&](llvm::StringRef) { return &MSS; });
+ CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
+ ASSERT_TRUE(Idx.blockUntilIdleForTest());
+ }
+ EXPECT_THAT(Storage.keys(),
+ UnorderedElementsAre(testPath("root/A.cc"), testPath("root/A.h"),
+ testPath("root/B.h")));
+ auto ShardHeader = MSS.loadShard(testPath("root/B.h"));
+ EXPECT_NE(ShardHeader, nullptr);
+ EXPECT_TRUE(ShardHeader->Symbols->empty());
+
+ // Check that A.cc, A.h and B.h has been loaded.
+ {
+ CacheHits = 0;
+ OverlayCDB CDB(/*Base=*/nullptr);
+ BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+ [&](llvm::StringRef) { return &MSS; });
+ CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
+ ASSERT_TRUE(Idx.blockUntilIdleForTest());
+ }
+ EXPECT_EQ(CacheHits, 3U);
+
+ // Update B.h to contain some symbols.
+ FS.Files[testPath("root/B.h")] = R"cpp(
+ #include "A.h"
+ void new_func();
+ )cpp";
+ // Check that B.h has been stored with new contents.
+ {
+ CacheHits = 0;
+ OverlayCDB CDB(/*Base=*/nullptr);
+ BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+ [&](llvm::StringRef) { return &MSS; });
+ CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
+ ASSERT_TRUE(Idx.blockUntilIdleForTest());
+ }
+ EXPECT_EQ(CacheHits, 3U);
+ ShardHeader = MSS.loadShard(testPath("root/B.h"));
+ EXPECT_NE(ShardHeader, nullptr);
+ EXPECT_THAT(*ShardHeader->Symbols,
+ Contains(AllOf(Named("new_func"), Declared(), Not(Defined()))));
+}
+
} // namespace clangd
} // namespace clang
More information about the cfe-commits
mailing list