[PATCH] D95206: [clangd][SwapIndex] ensure that the old index is alive while we are using it via the function returned by `SwapIndex::indexedFiles()` call

[Aleksandr Platonov via Phabricator via cfe-commits]

ArcsinX updated this revision to Diff 318470.
ArcsinX added a comment.

Call snapshot() only once to avoid possible race.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95206/new/

https://reviews.llvm.org/D95206

Files:
  clang-tools-extra/clangd/index/Index.cpp
  clang-tools-extra/clangd/index/Merge.cpp


Index: clang-tools-extra/clangd/index/Merge.cpp
===================================================================
--- clang-tools-extra/clangd/index/Merge.cpp
+++ clang-tools-extra/clangd/index/Merge.cpp
@@ -44,21 +44,23 @@
   SymbolSlab Dyn = std::move(DynB).build();
 
   llvm::DenseSet<SymbolID> SeenDynamicSymbols;
-  auto DynamicContainsFile = Dynamic->indexedFiles();
-  More |= Static->fuzzyFind(Req, [&](const Symbol &S) {
-    // We expect the definition to see the canonical declaration, so it seems
-    // to be enough to check only the definition if it exists.
-    if (DynamicContainsFile(S.Definition ? S.Definition.FileURI
-                                         : S.CanonicalDeclaration.FileURI))
-      return;
-    auto DynS = Dyn.find(S.ID);
-    ++StaticCount;
-    if (DynS == Dyn.end())
-      return Callback(S);
-    ++MergedCount;
-    SeenDynamicSymbols.insert(S.ID);
-    Callback(mergeSymbol(*DynS, S));
-  });
+  {
+    auto DynamicContainsFile = Dynamic->indexedFiles();
+    More |= Static->fuzzyFind(Req, [&](const Symbol &S) {
+      // We expect the definition to see the canonical declaration, so it seems
+      // to be enough to check only the definition if it exists.
+      if (DynamicContainsFile(S.Definition ? S.Definition.FileURI
+                                           : S.CanonicalDeclaration.FileURI))
+        return;
+      auto DynS = Dyn.find(S.ID);
+      ++StaticCount;
+      if (DynS == Dyn.end())
+        return Callback(S);
+      ++MergedCount;
+      SeenDynamicSymbols.insert(S.ID);
+      Callback(mergeSymbol(*DynS, S));
+    });
+  }
   SPAN_ATTACH(Tracer, "dynamic", DynamicCount);
   SPAN_ATTACH(Tracer, "static", StaticCount);
   SPAN_ATTACH(Tracer, "merged", MergedCount);
@@ -77,20 +79,22 @@
   Dynamic->lookup(Req, [&](const Symbol &S) { B.insert(S); });
 
   auto RemainingIDs = Req.IDs;
-  auto DynamicContainsFile = Dynamic->indexedFiles();
-  Static->lookup(Req, [&](const Symbol &S) {
-    // We expect the definition to see the canonical declaration, so it seems
-    // to be enough to check only the definition if it exists.
-    if (DynamicContainsFile(S.Definition ? S.Definition.FileURI
-                                         : S.CanonicalDeclaration.FileURI))
-      return;
-    const Symbol *Sym = B.find(S.ID);
-    RemainingIDs.erase(S.ID);
-    if (!Sym)
-      Callback(S);
-    else
-      Callback(mergeSymbol(*Sym, S));
-  });
+  {
+    auto DynamicContainsFile = Dynamic->indexedFiles();
+    Static->lookup(Req, [&](const Symbol &S) {
+      // We expect the definition to see the canonical declaration, so it seems
+      // to be enough to check only the definition if it exists.
+      if (DynamicContainsFile(S.Definition ? S.Definition.FileURI
+                                           : S.CanonicalDeclaration.FileURI))
+        return;
+      const Symbol *Sym = B.find(S.ID);
+      RemainingIDs.erase(S.ID);
+      if (!Sym)
+        Callback(S);
+      else
+        Callback(mergeSymbol(*Sym, S));
+    });
+  }
   for (const auto &ID : RemainingIDs)
     if (const Symbol *Sym = B.find(ID))
       Callback(*Sym);
Index: clang-tools-extra/clangd/index/Index.cpp
===================================================================
--- clang-tools-extra/clangd/index/Index.cpp
+++ clang-tools-extra/clangd/index/Index.cpp
@@ -78,7 +78,12 @@
 
 llvm::unique_function<bool(llvm::StringRef) const>
 SwapIndex::indexedFiles() const {
-  return snapshot()->indexedFiles();
+  // The index snapshot should outlive this method return value.
+  auto SnapShot = snapshot();
+  auto IndexedFiles = SnapShot->indexedFiles();
+  return [KeepAlive{std::move(SnapShot)},
+          IndexContainsFile{std::move(IndexedFiles)}](
+             llvm::StringRef File) { return IndexContainsFile(File); };
 }
 
 size_t SwapIndex::estimateMemoryUsage() const {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D95206.318470.patch
Type: text/x-patch
Size: 3857 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20210122/3c98c51a/attachment-0001.bin>