[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 created this revision.
ArcsinX added reviewers: sammccall, kadircet.
Herald added subscribers: usaxena95, arphaman.
ArcsinX requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang.

Without this patch the old index could be freed, but there still could be tries to access it via the function returned by `SwapIndex::indexedFiles()` call.
This leads to hard to reproduce clangd crashes at code completion.
This patch keeps the old index alive until the function returned by `SwapIndex::indexedFiles()` call is alive.


Repository:
  rG LLVM Github Monorepo

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,10 @@
 
 llvm::unique_function<bool(llvm::StringRef) const>
 SwapIndex::indexedFiles() const {
-  return snapshot()->indexedFiles();
+  return [ KeepAlive = snapshot(),
+           IndexedFiles = snapshot()->indexedFiles() ](llvm::StringRef File) {
+    return IndexedFiles(File);
+  };
 }
 
 size_t SwapIndex::estimateMemoryUsage() const {


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