[clang-tools-extra] f82346f - [clangd] Avoid relations being overwritten in a header shard
Nathan Ridge via cfe-commits
cfe-commits at lists.llvm.org
Sun Oct 11 12:33:32 PDT 2020
Author: Nathan Ridge
Date: 2020-10-11T15:32:54-04:00
New Revision: f82346fd7391fdd15b77355deb6f7c030a89dcb5
URL: https://github.com/llvm/llvm-project/commit/f82346fd7391fdd15b77355deb6f7c030a89dcb5
DIFF: https://github.com/llvm/llvm-project/commit/f82346fd7391fdd15b77355deb6f7c030a89dcb5.diff
LOG: [clangd] Avoid relations being overwritten in a header shard
Fixes https://github.com/clangd/clangd/issues/510
Differential Revision: https://reviews.llvm.org/D87256
Added:
Modified:
clang-tools-extra/clangd/index/FileIndex.cpp
clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
clang-tools-extra/clangd/unittests/FileIndexTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/index/FileIndex.cpp b/clang-tools-extra/clangd/index/FileIndex.cpp
index 155e09ca1e97..ad55b6ad7f5d 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 <algorithm>
#include <memory>
#include <tuple>
#include <utility>
@@ -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
diff erent relations under
diff erent
+ // 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 f9f584e8895f..adf39a915c1a 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<std::string> 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 955ee50be895..c5bfbe132d37 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,
More information about the cfe-commits
mailing list