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 @@ -355,7 +355,8 @@ // 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)); + IndexedSymbols.update(Path, std::move(SS), std::move(RS), + Path == MainFile); } } } @@ -478,6 +479,7 @@ std::string AbsolutePath; std::unique_ptr Shard; FileDigest Digest; + bool IsMainFile; }; std::vector IntermediateSymbols; // Make sure we don't have duplicate elements in the queue. Keys are absolute @@ -538,6 +540,7 @@ SI.AbsolutePath = CurDependency.Path; SI.Shard = std::move(Shard); SI.Digest = I.getValue().Digest; + SI.IsMainFile = I.getValue().IsTU; IntermediateSymbols.push_back(std::move(SI)); // Check if the source needs re-indexing. // Get the digest, skip it if file doesn't exist. @@ -567,7 +570,8 @@ ? llvm::make_unique(std::move(*SI.Shard->Refs)) : nullptr; IndexedFileDigests[SI.AbsolutePath] = SI.Digest; - IndexedSymbols.update(SI.AbsolutePath, std::move(SS), std::move(RS)); + IndexedSymbols.update(SI.AbsolutePath, std::move(SS), std::move(RS), + SI.IsMainFile); } } diff --git a/clang-tools-extra/clangd/index/FileIndex.h b/clang-tools-extra/clangd/index/FileIndex.h --- a/clang-tools-extra/clangd/index/FileIndex.h +++ b/clang-tools-extra/clangd/index/FileIndex.h @@ -56,12 +56,17 @@ /// /// The snapshot semantics keeps critical sections minimal since we only need /// locking when we swap or obtain references to snapshots. +/// +/// Will count Symbol::References based on number of references in the main +/// files, while building the index with DuplicateHandling::Merge option. class FileSymbols { public: /// Updates all symbols and refs in a file. /// If either is nullptr, corresponding data for \p Path will be removed. + /// If IsMainFile is true, Refs will be used for counting References during + /// merging. void update(PathRef Path, std::unique_ptr Slab, - std::unique_ptr Refs); + std::unique_ptr Refs, bool IsMainFile); // The index keeps the symbols alive. std::unique_ptr @@ -69,12 +74,14 @@ DuplicateHandling DuplicateHandle = DuplicateHandling::PickOne); private: + using RefSlabAndIsMainFile = + std::pair, /*IsMainFile=*/bool>; mutable std::mutex Mutex; /// Stores the latest symbol snapshots for all active files. llvm::StringMap> FileToSymbols; /// Stores the latest ref snapshots for all active files. - llvm::StringMap> FileToRefs; + llvm::StringMap FileToRefs; }; /// This manages symbols from files and an in-memory index on all symbols. diff --git a/clang-tools-extra/clangd/index/FileIndex.cpp b/clang-tools-extra/clangd/index/FileIndex.cpp --- a/clang-tools-extra/clangd/index/FileIndex.cpp +++ b/clang-tools-extra/clangd/index/FileIndex.cpp @@ -90,7 +90,7 @@ } void FileSymbols::update(PathRef Path, std::unique_ptr Symbols, - std::unique_ptr Refs) { + std::unique_ptr Refs, bool IsMainFile) { std::lock_guard Lock(Mutex); if (!Symbols) FileToSymbols.erase(Path); @@ -99,46 +99,64 @@ if (!Refs) FileToRefs.erase(Path); else - FileToRefs[Path] = std::move(Refs); + FileToRefs[Path] = {std::move(Refs), IsMainFile}; } std::unique_ptr FileSymbols::buildIndex(IndexType Type, DuplicateHandling DuplicateHandle) { std::vector> SymbolSlabs; std::vector> RefSlabs; + std::vector MainFileRefs; { std::lock_guard Lock(Mutex); for (const auto &FileAndSymbols : FileToSymbols) SymbolSlabs.push_back(FileAndSymbols.second); - for (const auto &FileAndRefs : FileToRefs) - RefSlabs.push_back(FileAndRefs.second); + for (const auto &FileAndRefs : FileToRefs) { + RefSlabs.push_back(FileAndRefs.second.first); + if (FileAndRefs.second.second) + MainFileRefs.push_back(RefSlabs.back().get()); + } } std::vector AllSymbols; std::vector SymsStorage; switch (DuplicateHandle) { case DuplicateHandling::Merge: { - llvm::DenseMap Merged; + // Keeps index to symbol in SymsStorage. + llvm::DenseMap Merged; for (const auto &Slab : SymbolSlabs) { for (const auto &Sym : *Slab) { - auto I = Merged.try_emplace(Sym.ID, Sym); - if (!I.second) - I.first->second = mergeSymbol(I.first->second, Sym); + assert(Sym.References == 0 && + "Symbol with non-zero references sent to FileSymbols"); + auto I = Merged.try_emplace(Sym.ID, SymsStorage.size()); + if (I.second) { + SymsStorage.push_back(Sym); + } else { + Symbol &Existing = SymsStorage[I.first->second]; + Existing = mergeSymbol(Existing, Sym); + } } } - SymsStorage.reserve(Merged.size()); - for (auto &Sym : Merged) { - SymsStorage.push_back(std::move(Sym.second)); - AllSymbols.push_back(&SymsStorage.back()); - } - // FIXME: aggregate symbol reference count based on references. + for (const RefSlab *Refs : MainFileRefs) + for (const auto &Sym : *Refs) { + auto It = Merged.find(Sym.first); + assert(It != Merged.end() && "Reference to unknown symbol"); + Symbol &S = SymsStorage[It->second]; + S.References += Sym.second.size(); + } + AllSymbols.reserve(SymsStorage.size()); + for (auto &Sym : SymsStorage) + AllSymbols.push_back(&Sym); break; } case DuplicateHandling::PickOne: { llvm::DenseSet AddedSymbols; for (const auto &Slab : SymbolSlabs) - for (const auto &Sym : *Slab) + for (const auto &Sym : *Slab) { + assert(Sym.References == 0 && + "Symbol with non-zero references sent to FileSymbols"); if (AddedSymbols.insert(Sym.ID).second) AllSymbols.push_back(&Sym); + } break; } } @@ -203,7 +221,7 @@ auto Symbols = indexHeaderSymbols(AST, std::move(PP), Includes); PreambleSymbols.update(Path, llvm::make_unique(std::move(Symbols)), - llvm::make_unique()); + llvm::make_unique(), /*IsMainFile=*/false); PreambleIndex.reset( PreambleSymbols.buildIndex(UseDex ? IndexType::Heavy : IndexType::Light, DuplicateHandling::PickOne)); @@ -213,7 +231,8 @@ auto Contents = indexMainDecls(AST); MainFileSymbols.update( Path, llvm::make_unique(std::move(Contents.first)), - llvm::make_unique(std::move(Contents.second))); + llvm::make_unique(std::move(Contents.second)), + /*IsMainFile=*/true); MainFileIndex.reset( MainFileSymbols.buildIndex(IndexType::Light, DuplicateHandling::PickOne)); } diff --git a/clang-tools-extra/clangd/index/IndexAction.cpp b/clang-tools-extra/clangd/index/IndexAction.cpp --- a/clang-tools-extra/clangd/index/IndexAction.cpp +++ b/clang-tools-extra/clangd/index/IndexAction.cpp @@ -186,7 +186,6 @@ IndexOpts.SystemSymbolFilter = index::IndexingOptions::SystemSymbolFilterKind::All; Opts.CollectIncludePath = true; - Opts.CountReferences = true; if (Opts.Origin == SymbolOrigin::Unknown) Opts.Origin = SymbolOrigin::Static; Opts.StoreAllDocumentation = false; diff --git a/clang-tools-extra/clangd/indexer/IndexerMain.cpp b/clang-tools-extra/clangd/indexer/IndexerMain.cpp --- a/clang-tools-extra/clangd/indexer/IndexerMain.cpp +++ b/clang-tools-extra/clangd/indexer/IndexerMain.cpp @@ -41,6 +41,7 @@ clang::FrontendAction *create() override { SymbolCollector::Options Opts; + Opts.CountReferences = true; return createStaticIndexingAction( Opts, [&](SymbolSlab S) { 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 @@ -32,6 +32,7 @@ return !arg.IsTU && !arg.URI.empty() && arg.Digest == FileDigest{{0}} && arg.DirectIncludes.empty(); } +MATCHER_P(NumReferences, N, "") { return arg.References == N; } class MemoryShardStorage : public BackgroundIndexStorage { mutable std::mutex StorageMu; @@ -112,6 +113,9 @@ #include "A.h" void f_b() { (void)common; + (void)common; + (void)common; + (void)common; })cpp"; llvm::StringMap Storage; size_t CacheHits = 0; @@ -127,20 +131,25 @@ CDB.setCompileCommand(testPath("root/A.cc"), Cmd); ASSERT_TRUE(Idx.blockUntilIdleForTest()); - EXPECT_THAT( - runFuzzyFind(Idx, ""), - UnorderedElementsAre(Named("common"), Named("A_CC"), Named("g"), - AllOf(Named("f_b"), Declared(), Not(Defined())))); + EXPECT_THAT(runFuzzyFind(Idx, ""), + UnorderedElementsAre(AllOf(Named("common"), NumReferences(1U)), + AllOf(Named("A_CC"), NumReferences(0U)), + AllOf(Named("g"), NumReferences(0U)), + AllOf(Named("f_b"), Declared(), + Not(Defined()), NumReferences(0U)))); Cmd.Filename = testPath("root/B.cc"); Cmd.CommandLine = {"clang++", Cmd.Filename}; - CDB.setCompileCommand(testPath("root/A.cc"), Cmd); + CDB.setCompileCommand(testPath("root/B.cc"), Cmd); ASSERT_TRUE(Idx.blockUntilIdleForTest()); // B_CC is dropped as we don't collect symbols from A.h in this compilation. EXPECT_THAT(runFuzzyFind(Idx, ""), - UnorderedElementsAre(Named("common"), Named("A_CC"), Named("g"), - AllOf(Named("f_b"), Declared(), Defined()))); + UnorderedElementsAre(AllOf(Named("common"), NumReferences(5U)), + AllOf(Named("A_CC"), NumReferences(0U)), + AllOf(Named("g"), NumReferences(0U)), + AllOf(Named("f_b"), Declared(), Defined(), + NumReferences(1U)))); auto Syms = runFuzzyFind(Idx, "common"); EXPECT_THAT(Syms, UnorderedElementsAre(Named("common"))); @@ -148,6 +157,9 @@ EXPECT_THAT(getRefs(Idx, Common.ID), RefsAre({FileURI("unittest:///root/A.h"), FileURI("unittest:///root/A.cc"), + FileURI("unittest:///root/B.cc"), + FileURI("unittest:///root/B.cc"), + FileURI("unittest:///root/B.cc"), FileURI("unittest:///root/B.cc")})); } diff --git a/clang-tools-extra/clangd/unittests/FileIndexTests.cpp b/clang-tools-extra/clangd/unittests/FileIndexTests.cpp --- a/clang-tools-extra/clangd/unittests/FileIndexTests.cpp +++ b/clang-tools-extra/clangd/unittests/FileIndexTests.cpp @@ -45,6 +45,9 @@ return llvm::StringRef(arg.Definition.FileURI) == U; } MATCHER_P(QName, N, "") { return (arg.Scope + arg.Name).str() == N; } +MATCHER_P(NumReferences, N, "") { + return arg.References == static_cast(N); +} namespace clang { namespace clangd { @@ -81,7 +84,7 @@ FileSymbols FS; EXPECT_THAT(runFuzzyFind(*FS.buildIndex(IndexType::Light), ""), IsEmpty()); - FS.update("f1", numSlab(1, 3), refSlab(SymbolID("1"), "f1.cc")); + FS.update("f1", numSlab(1, 3), refSlab(SymbolID("1"), "f1.cc"), false); EXPECT_THAT(runFuzzyFind(*FS.buildIndex(IndexType::Light), ""), UnorderedElementsAre(QName("1"), QName("2"), QName("3"))); EXPECT_THAT(getRefs(*FS.buildIndex(IndexType::Light), SymbolID("1")), @@ -90,8 +93,8 @@ TEST(FileSymbolsTest, Overlap) { FileSymbols FS; - FS.update("f1", numSlab(1, 3), nullptr); - FS.update("f2", numSlab(3, 5), nullptr); + FS.update("f1", numSlab(1, 3), nullptr, false); + FS.update("f2", numSlab(3, 5), nullptr, false); for (auto Type : {IndexType::Light, IndexType::Heavy}) EXPECT_THAT(runFuzzyFind(*FS.buildIndex(Type), ""), UnorderedElementsAre(QName("1"), QName("2"), QName("3"), @@ -110,8 +113,8 @@ auto X2 = symbol("x"); X2.Definition.FileURI = "file:///x2"; - FS.update("f1", OneSymboSlab(X1), nullptr); - FS.update("f2", OneSymboSlab(X2), nullptr); + FS.update("f1", OneSymboSlab(X1), nullptr, false); + FS.update("f2", OneSymboSlab(X2), nullptr, false); for (auto Type : {IndexType::Light, IndexType::Heavy}) EXPECT_THAT( runFuzzyFind(*FS.buildIndex(Type, DuplicateHandling::Merge), "x"), @@ -123,14 +126,14 @@ FileSymbols FS; SymbolID ID("1"); - FS.update("f1", numSlab(1, 3), refSlab(ID, "f1.cc")); + FS.update("f1", numSlab(1, 3), refSlab(ID, "f1.cc"), false); auto Symbols = FS.buildIndex(IndexType::Light); EXPECT_THAT(runFuzzyFind(*Symbols, ""), UnorderedElementsAre(QName("1"), QName("2"), QName("3"))); EXPECT_THAT(getRefs(*Symbols, ID), RefsAre({FileURI("f1.cc")})); - FS.update("f1", nullptr, nullptr); + FS.update("f1", nullptr, nullptr, false); auto Empty = FS.buildIndex(IndexType::Light); EXPECT_THAT(runFuzzyFind(*Empty, ""), IsEmpty()); EXPECT_THAT(getRefs(*Empty, ID), ElementsAre()); @@ -366,6 +369,33 @@ RefsAre({RefRange(Main.range())})); } +TEST(FileSymbolsTest, CountReferencesNoRefSlabs) { + FileSymbols FS; + FS.update("f1", numSlab(1, 3), nullptr, true); + FS.update("f2", numSlab(1, 3), nullptr, false); + EXPECT_THAT( + runFuzzyFind(*FS.buildIndex(IndexType::Light, DuplicateHandling::Merge), + ""), + UnorderedElementsAre(AllOf(QName("1"), NumReferences(0)), + AllOf(QName("2"), NumReferences(0)), + AllOf(QName("3"), NumReferences(0)))); +} + +TEST(FileSymbolsTest, CountReferencesWithRefSlabs) { + FileSymbols FS; + FS.update("f1cpp", numSlab(1, 3), refSlab(SymbolID("1"), "f1.cpp"), true); + FS.update("f1h", numSlab(1, 3), refSlab(SymbolID("1"), "f1.h"), false); + FS.update("f2cpp", numSlab(1, 3), refSlab(SymbolID("2"), "f2.cpp"), true); + FS.update("f2h", numSlab(1, 3), refSlab(SymbolID("2"), "f2.h"), false); + FS.update("f3cpp", numSlab(1, 3), refSlab(SymbolID("3"), "f3.cpp"), true); + FS.update("f3h", numSlab(1, 3), refSlab(SymbolID("3"), "f3.h"), false); + EXPECT_THAT( + runFuzzyFind(*FS.buildIndex(IndexType::Light, DuplicateHandling::Merge), + ""), + UnorderedElementsAre(AllOf(QName("1"), NumReferences(1)), + AllOf(QName("2"), NumReferences(1)), + AllOf(QName("3"), NumReferences(1)))); +} } // namespace } // namespace clangd } // namespace clang