diff --git a/clang-tools-extra/clangd/index/FileIndex.cpp b/clang-tools-extra/clangd/index/FileIndex.cpp index 155e09ca1e9768..ad55b6ad7f5db1 100644 --- a/clang-tools-extra/clangd/index/FileIndex.cpp +++ b/clang-tools-extra/clangd/index/FileIndex.cpp @@ -34,6 +34,7 @@ #include "llvm/ADT/StringMap.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/Error.h" +#include #include #include #include @@ -147,13 +148,21 @@ FileShardedIndex::FileShardedIndex(IndexFileIn Input) } } } - // Attribute relations to the file declaraing their Subject as Object might - // not have been indexed, see SymbolCollector::processRelations for details. + // The Subject and/or Object shards might be part of multiple TUs. In + // such cases there will be a race and the last TU to write the shard + // will win and all the other relations will be lost. To avoid this, + // we store relations in both shards. A race might still happen if the + // same translation unit produces different relations under different + // configurations, but that's something clangd doesn't handle in general. if (Index.Relations) { for (const auto &R : *Index.Relations) { // FIXME: RelationSlab shouldn't contain dangling relations. - if (auto *File = SymbolIDToFile.lookup(R.Subject)) - File->Relations.insert(&R); + FileShard *SubjectFile = SymbolIDToFile.lookup(R.Subject); + FileShard *ObjectFile = SymbolIDToFile.lookup(R.Object); + if (SubjectFile) + SubjectFile->Relations.insert(&R); + if (ObjectFile && ObjectFile != SubjectFile) + ObjectFile->Relations.insert(&R); } } // Store only the direct includes of a file in a shard. @@ -343,6 +352,12 @@ FileSymbols::buildIndex(IndexType Type, DuplicateHandling DuplicateHandle, for (const auto &R : *RelationSlab) AllRelations.push_back(R); } + // Sort relations and remove duplicates that could arise due to + // relations being stored in both the shards containing their + // subject and object. + llvm::sort(AllRelations); + AllRelations.erase(std::unique(AllRelations.begin(), AllRelations.end()), + AllRelations.end()); size_t StorageSize = RefsStorage.size() * sizeof(Ref) + SymsStorage.size() * sizeof(Symbol); diff --git a/clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp b/clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp index f9f584e8895f52..adf39a915c1a15 100644 --- a/clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp +++ b/clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp @@ -229,6 +229,49 @@ TEST_F(BackgroundIndexTest, IndexTwoFiles) { FileURI("unittest:///root/B.cc")})); } +TEST_F(BackgroundIndexTest, RelationsMultiFile) { + MockFS FS; + FS.Files[testPath("root/Base.h")] = "class Base {};"; + FS.Files[testPath("root/A.cc")] = R"cpp( + #include "Base.h" + class A : public Base {}; + )cpp"; + FS.Files[testPath("root/B.cc")] = R"cpp( + #include "Base.h" + class B : public Base {}; + )cpp"; + + llvm::StringMap Storage; + size_t CacheHits = 0; + MemoryShardStorage MSS(Storage, CacheHits); + OverlayCDB CDB(/*Base=*/nullptr); + BackgroundIndex Index(FS, CDB, [&](llvm::StringRef) { return &MSS; }, + /*Opts=*/{}); + + tooling::CompileCommand Cmd; + Cmd.Filename = testPath("root/A.cc"); + Cmd.Directory = testPath("root"); + Cmd.CommandLine = {"clang++", Cmd.Filename}; + CDB.setCompileCommand(testPath("root/A.cc"), Cmd); + ASSERT_TRUE(Index.blockUntilIdleForTest()); + + Cmd.Filename = testPath("root/B.cc"); + Cmd.CommandLine = {"clang++", Cmd.Filename}; + CDB.setCompileCommand(testPath("root/B.cc"), Cmd); + ASSERT_TRUE(Index.blockUntilIdleForTest()); + + auto HeaderShard = MSS.loadShard(testPath("root/Base.h")); + EXPECT_NE(HeaderShard, nullptr); + SymbolID Base = findSymbol(*HeaderShard->Symbols, "Base").ID; + + RelationsRequest Req; + Req.Subjects.insert(Base); + Req.Predicate = RelationKind::BaseOf; + uint32_t Results = 0; + Index.relations(Req, [&](const SymbolID &, const Symbol &) { ++Results; }); + EXPECT_EQ(Results, 2u); +} + TEST_F(BackgroundIndexTest, MainFileRefs) { MockFS FS; FS.Files[testPath("root/A.h")] = R"cpp( @@ -345,14 +388,15 @@ TEST_F(BackgroundIndexTest, ShardStorageTest) { EXPECT_THAT(Ref.second, UnorderedElementsAre(FileURI("unittest:///root/A.cc"))); - // The BaseOf relationship between A_CC and B_CC is stored in the file - // containing the definition of the subject (A_CC) + // The BaseOf relationship between A_CC and B_CC is stored in both the file + // containing the definition of the subject (A_CC) and the file containing + // the definition of the object (B_CC). SymbolID A = findSymbol(*ShardHeader->Symbols, "A_CC").ID; SymbolID B = findSymbol(*ShardSource->Symbols, "B_CC").ID; EXPECT_THAT(*ShardHeader->Relations, UnorderedElementsAre(Relation{A, RelationKind::BaseOf, B})); - // (and not in the file containing the definition of the object (B_CC)). - EXPECT_EQ(ShardSource->Relations->size(), 0u); + EXPECT_THAT(*ShardSource->Relations, + UnorderedElementsAre(Relation{A, RelationKind::BaseOf, B})); } TEST_F(BackgroundIndexTest, DirectIncludesTest) { diff --git a/clang-tools-extra/clangd/unittests/FileIndexTests.cpp b/clang-tools-extra/clangd/unittests/FileIndexTests.cpp index 955ee50be89554..c5bfbe132d37f4 100644 --- a/clang-tools-extra/clangd/unittests/FileIndexTests.cpp +++ b/clang-tools-extra/clangd/unittests/FileIndexTests.cpp @@ -547,6 +547,8 @@ TEST(FileShardedIndexTest, Sharding) { Sym2.CanonicalDeclaration.FileURI = BHeaderUri.c_str(); Sym2.Definition.FileURI = BSourceUri.c_str(); + auto Sym3 = symbol("3"); // not stored + IndexFileIn IF; { SymbolSlab::Builder B; @@ -562,12 +564,13 @@ TEST(FileShardedIndexTest, Sharding) { } { RelationSlab::Builder B; - // Should be stored in a.h + // Should be stored in a.h and b.h B.insert(Relation{Sym1.ID, RelationKind::BaseOf, Sym2.ID}); - // Should be stored in b.h + // Should be stored in a.h and b.h B.insert(Relation{Sym2.ID, RelationKind::BaseOf, Sym1.ID}); - // Dangling relation should be dropped. - B.insert(Relation{symbol("3").ID, RelationKind::BaseOf, Sym1.ID}); + // Should be stored in a.h (where Sym1 is stored) even though + // the relation is dangling as Sym3 is unknown. + B.insert(Relation{Sym3.ID, RelationKind::BaseOf, Sym1.ID}); IF.Relations.emplace(std::move(B).build()); } @@ -605,7 +608,9 @@ TEST(FileShardedIndexTest, Sharding) { EXPECT_THAT(*Shard->Refs, IsEmpty()); EXPECT_THAT( *Shard->Relations, - UnorderedElementsAre(Relation{Sym1.ID, RelationKind::BaseOf, Sym2.ID})); + UnorderedElementsAre(Relation{Sym1.ID, RelationKind::BaseOf, Sym2.ID}, + Relation{Sym2.ID, RelationKind::BaseOf, Sym1.ID}, + Relation{Sym3.ID, RelationKind::BaseOf, Sym1.ID})); ASSERT_THAT(Shard->Sources->keys(), UnorderedElementsAre(AHeaderUri)); EXPECT_THAT(Shard->Sources->lookup(AHeaderUri).DirectIncludes, IsEmpty()); EXPECT_TRUE(Shard->Cmd.hasValue()); @@ -617,7 +622,8 @@ TEST(FileShardedIndexTest, Sharding) { EXPECT_THAT(*Shard->Refs, IsEmpty()); EXPECT_THAT( *Shard->Relations, - UnorderedElementsAre(Relation{Sym2.ID, RelationKind::BaseOf, Sym1.ID})); + UnorderedElementsAre(Relation{Sym1.ID, RelationKind::BaseOf, Sym2.ID}, + Relation{Sym2.ID, RelationKind::BaseOf, Sym1.ID})); ASSERT_THAT(Shard->Sources->keys(), UnorderedElementsAre(BHeaderUri, AHeaderUri)); EXPECT_THAT(Shard->Sources->lookup(BHeaderUri).DirectIncludes,