[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