[PATCH] D87256: [clangd] Avoid relations being overwritten in a header shard
This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGf82346fd7391: [clangd] Avoid relations being overwritten in a header shard (authored by nridge). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87256/new/ https://reviews.llvm.org/D87256 Files: clang-tools-extra/clangd/index/FileIndex.cpp clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp clang-tools-extra/clangd/unittests/FileIndexTests.cpp Index: clang-tools-extra/clangd/unittests/FileIndexTests.cpp === --- clang-tools-extra/clangd/unittests/FileIndexTests.cpp +++ clang-tools-extra/clangd/unittests/FileIndexTests.cpp @@ -547,6 +547,8 @@ 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 @@ } { 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 @@ 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 @@ 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, Index: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp === --- clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp +++ clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp @@ -229,6 +229,49 @@ 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 @@ 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 defi
[PATCH] D87256: [clangd] Avoid relations being overwritten in a header shard
nridge marked an inline comment as done. nridge added inline comments. Comment at: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp:232 +TEST_F(BackgroundIndexTest, RelationsMultiFile) { + MockFS FS; kadircet wrote: > kadircet wrote: > > nridge wrote: > > > kadircet wrote: > > > > do we still need this test? > > > I think it's still useful, yes. If someone makes a change to the way > > > relations are stored in the future, they could regress this use case > > > without a test specifically for it. > > this one was marked as resolved but i still don't see the reasoning. can we > > test this in fileindextests instead? > > > > we already test the sharding logic, we need to test the merging logic now. > > can we rather test it at FileSymbols layer directly? or is there something > > extra that i am misisng? > okay, i still think it is better to test on FileSymbols layer but not that > important. The reason I'd like to have a test at this layer is so that we have a test that closely approximates the steps to reproduce: create files with certain contents and inclusion relationships between them, index them, and perform a particular query. I feel like internal abstractions like `FileSymbols` are liable to change over time through refactorings, and so tests written against them are less robust. Note, I don't think using `BackgroundIndex` is important for the kind of test I'd like; `FileIndex` would be fine too. However, I could not see how to use `FileIndex` with files that contain `#include` statements (didn't see a way to provide a `MockFS` or another way to get the includes to be resolved). That said, I did try to write an additional test using `FileSymbols`, but I found that: * it only exercises the merging logic, not the sharding logic * I couldn't get the test to fail even if I omit the merging logic of this patch, because both `MemIndex` and `Dex` build a `DenseMap` for relations and therefore end up implicitly deduplicating anyways Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87256/new/ https://reviews.llvm.org/D87256 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87256: [clangd] Avoid relations being overwritten in a header shard
nridge updated this revision to Diff 297482. nridge added a comment. Slightly simplified test (base class does not need to be a template to trigger the issue) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87256/new/ https://reviews.llvm.org/D87256 Files: clang-tools-extra/clangd/index/FileIndex.cpp clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp clang-tools-extra/clangd/unittests/FileIndexTests.cpp Index: clang-tools-extra/clangd/unittests/FileIndexTests.cpp === --- clang-tools-extra/clangd/unittests/FileIndexTests.cpp +++ clang-tools-extra/clangd/unittests/FileIndexTests.cpp @@ -547,6 +547,8 @@ 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 @@ } { 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 @@ 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 @@ 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, Index: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp === --- clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp +++ clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp @@ -229,6 +229,49 @@ 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 @@ 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 s
[PATCH] D87256: [clangd] Avoid relations being overwritten in a header shard
kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. thanks! (and sorry for taking too long on this one.) Comment at: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp:232 +TEST_F(BackgroundIndexTest, RelationsMultiFile) { + MockFS FS; kadircet wrote: > nridge wrote: > > kadircet wrote: > > > do we still need this test? > > I think it's still useful, yes. If someone makes a change to the way > > relations are stored in the future, they could regress this use case > > without a test specifically for it. > this one was marked as resolved but i still don't see the reasoning. can we > test this in fileindextests instead? > > we already test the sharding logic, we need to test the merging logic now. > can we rather test it at FileSymbols layer directly? or is there something > extra that i am misisng? okay, i still think it is better to test on FileSymbols layer but not that important. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87256/new/ https://reviews.llvm.org/D87256 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87256: [clangd] Avoid relations being overwritten in a header shard
nridge added a comment. Sorry, I had a response to that comment but accidentally left it as unsubmitted... Comment at: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp:232 +TEST_F(BackgroundIndexTest, RelationsMultiFile) { + MockFS FS; kadircet wrote: > do we still need this test? I think it's still useful, yes. If someone makes a change to the way relations are stored in the future, they could regress this use case without a test specifically for it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87256/new/ https://reviews.llvm.org/D87256 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87256: [clangd] Avoid relations being overwritten in a header shard
kadircet added inline comments. Comment at: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp:232 +TEST_F(BackgroundIndexTest, RelationsMultiFile) { + MockFS FS; kadircet wrote: > do we still need this test? this one was marked as resolved but i still don't see the reasoning. can we test this in fileindextests instead? we already test the sharding logic, we need to test the merging logic now. can we rather test it at FileSymbols layer directly? or is there something extra that i am misisng? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87256/new/ https://reviews.llvm.org/D87256 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87256: [clangd] Avoid relations being overwritten in a header shard
nridge updated this revision to Diff 294577. nridge marked 5 inline comments as done. nridge added a comment. Address review comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87256/new/ https://reviews.llvm.org/D87256 Files: clang-tools-extra/clangd/index/FileIndex.cpp clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp clang-tools-extra/clangd/unittests/FileIndexTests.cpp Index: clang-tools-extra/clangd/unittests/FileIndexTests.cpp === --- clang-tools-extra/clangd/unittests/FileIndexTests.cpp +++ clang-tools-extra/clangd/unittests/FileIndexTests.cpp @@ -547,6 +547,8 @@ 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 @@ } { 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 @@ 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 @@ 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, Index: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp === --- clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp +++ clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp @@ -229,6 +229,49 @@ FileURI("unittest:///root/B.cc")})); } +TEST_F(BackgroundIndexTest, RelationsMultiFile) { + MockFS FS; + FS.Files[testPath("root/RAV.h")] = "template class RAV {};"; + FS.Files[testPath("root/A.cc")] = R"cpp( +#include "RAV.h" +class A : public RAV {}; + )cpp"; + FS.Files[testPath("root/B.cc")] = R"cpp( +#include "RAV.h" +class B : public RAV {}; + )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/RAV.h")); + EXPECT_NE(HeaderShard, nullptr); + SymbolID RAV = findSymbol(*HeaderShard->Symbols, "RAV").ID; + + RelationsRequest Req; + Req.Subjects.insert(RAV); + 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 @@ 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 +
[PATCH] D87256: [clangd] Avoid relations being overwritten in a header shard
kadircet added inline comments. Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:152 + // Attribute relations to both their subject's and object's locations. + // See https://github.com/clangd/clangd/issues/510 for discussion of why. if (Index.Relations) { instead of (or in addition to) providing the link, can we give a short summary? e.g. ``` Subject and/or Object files might be part of multiple TUs. In such cases there will be a race and last TU to write the shard will win and all the other relations will be lost. We are storing relations in both places, as we do for symbols, to reduce such races. Note that they might still happen if same translation unit produces different relations under different configurations but that's something clangd doesn't handle in general. ``` Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:350 AllRelations.push_back(R); +// Sort relations and remove duplicates that could arise due to +// relations being stored in both the shards containing their can you move this outside of the for loop, so that we do it only once. Comment at: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp:232 +TEST_F(BackgroundIndexTest, RelationsMultiFile) { + MockFS FS; do we still need this test? Comment at: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp:396 SymbolID B = findSymbol(*ShardSource->Symbols, "B_CC").ID; - EXPECT_THAT(*ShardHeader->Relations, + EXPECT_THAT(*ShardSource->Relations, + UnorderedElementsAre(Relation{A, RelationKind::BaseOf, B})); s/ShardSource/ShardHeader Comment at: clang-tools-extra/clangd/unittests/FileIndexTests.cpp:571 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 even though it's dangling +B.insert(Relation{Sym3.ID, RelationKind::BaseOf, Sym1.ID}); maybe mention that `Sym1` belongs to `a.h` in the comment. `stored in a.h (where Sym1 is stored) even tho the reference is dangling as Sym3 is unknown ` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87256/new/ https://reviews.llvm.org/D87256 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87256: [clangd] Avoid relations being overwritten in a header shard
nridge updated this revision to Diff 293045. nridge added a comment. Herald added a subscriber: mgrang. Address review comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87256/new/ https://reviews.llvm.org/D87256 Files: clang-tools-extra/clangd/index/FileIndex.cpp clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp clang-tools-extra/clangd/unittests/FileIndexTests.cpp Index: clang-tools-extra/clangd/unittests/FileIndexTests.cpp === --- clang-tools-extra/clangd/unittests/FileIndexTests.cpp +++ clang-tools-extra/clangd/unittests/FileIndexTests.cpp @@ -547,6 +547,8 @@ 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,12 @@ } { 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 even though it's dangling +B.insert(Relation{Sym3.ID, RelationKind::BaseOf, Sym1.ID}); IF.Relations.emplace(std::move(B).build()); } @@ -605,7 +607,9 @@ 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 +621,8 @@ 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, Index: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp === --- clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp +++ clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp @@ -229,6 +229,49 @@ FileURI("unittest:///root/B.cc")})); } +TEST_F(BackgroundIndexTest, RelationsMultiFile) { + MockFS FS; + FS.Files[testPath("root/RAV.h")] = "template class RAV {};"; + FS.Files[testPath("root/A.cc")] = R"cpp( +#include "RAV.h" +class A : public RAV {}; + )cpp"; + FS.Files[testPath("root/B.cc")] = R"cpp( +#include "RAV.h" +class B : public RAV {}; + )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/RAV.h")); + EXPECT_NE(HeaderShard, nullptr); + SymbolID RAV = findSymbol(*HeaderShard->Symbols, "RAV").ID; + + RelationsRequest Req; + Req.Subjects.insert(RAV); + 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 @@ 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 cont
[PATCH] D87256: [clangd] Avoid relations being overwritten in a header shard
hokein added a comment. +@kadircet, he is tracking on this -- we had some discussion internally last week, but we don't reply on that thread yet (sorry). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87256/new/ https://reviews.llvm.org/D87256 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87256: [clangd] Avoid relations being overwritten in a header shard
nridge created this revision. nridge added a reviewer: hokein. Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, jkorous. Herald added a project: clang. nridge requested review of this revision. Herald added subscribers: MaskRay, ilya-biryukov. Fixes https://github.com/clangd/clangd/issues/510 Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D87256 Files: clang-tools-extra/clangd/index/FileIndex.cpp clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp clang-tools-extra/clangd/unittests/FileIndexTests.cpp Index: clang-tools-extra/clangd/unittests/FileIndexTests.cpp === --- clang-tools-extra/clangd/unittests/FileIndexTests.cpp +++ clang-tools-extra/clangd/unittests/FileIndexTests.cpp @@ -547,6 +547,8 @@ 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,12 @@ } { RelationSlab::Builder B; -// Should be stored in a.h -B.insert(Relation{Sym1.ID, RelationKind::BaseOf, Sym2.ID}); // Should be stored in b.h +B.insert(Relation{Sym1.ID, RelationKind::BaseOf, Sym2.ID}); +// Should be stored in a.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 even though it's dangling +B.insert(Relation{Sym3.ID, RelationKind::BaseOf, Sym1.ID}); IF.Relations.emplace(std::move(B).build()); } @@ -605,7 +607,8 @@ EXPECT_THAT(*Shard->Refs, IsEmpty()); EXPECT_THAT( *Shard->Relations, -UnorderedElementsAre(Relation{Sym1.ID, RelationKind::BaseOf, Sym2.ID})); +UnorderedElementsAre(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 +620,7 @@ 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})); ASSERT_THAT(Shard->Sources->keys(), UnorderedElementsAre(BHeaderUri, AHeaderUri)); EXPECT_THAT(Shard->Sources->lookup(BHeaderUri).DirectIncludes, Index: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp === --- clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp +++ clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp @@ -229,6 +229,49 @@ FileURI("unittest:///root/B.cc")})); } +TEST_F(BackgroundIndexTest, RelationsMultiFile) { + MockFS FS; + FS.Files[testPath("root/RAV.h")] = "template class RAV {};"; + FS.Files[testPath("root/A.cc")] = R"cpp( +#include "RAV.h" +class A : public RAV {}; + )cpp"; + FS.Files[testPath("root/B.cc")] = R"cpp( +#include "RAV.h" +class B : public RAV {}; + )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/RAV.h")); + EXPECT_NE(HeaderShard, nullptr); + SymbolID RAV = findSymbol(*HeaderShard->Symbols, "RAV").ID; + + RelationsRequest Req; + Req.Subjects.insert(RAV); + 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( @@ -346,13 +389,13 @@ 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) + // 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(*ShardHea