[clang-tools-extra] 7388c34 - [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 cfe-commits]

Author: Aleksandr Platonov
Date: 2021-01-22T16:26:39+03:00
New Revision: 7388c34685954862e5f1fa4734f42f7087e697a2

URL: https://github.com/llvm/llvm-project/commit/7388c34685954862e5f1fa4734f42f7087e697a2
DIFF: https://github.com/llvm/llvm-project/commit/7388c34685954862e5f1fa4734f42f7087e697a2.diff

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

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.

Reviewed By: sammccall

Differential Revision: https://reviews.llvm.org/D95206

Added: 
    

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

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/index/Index.cpp b/clang-tools-extra/clangd/index/Index.cpp
index 5da06f36ffe4..1b085140b4ff 100644
--- a/clang-tools-extra/clangd/index/Index.cpp
+++ b/clang-tools-extra/clangd/index/Index.cpp
@@ -78,7 +78,13 @@ void SwapIndex::relations(
 
 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 {

diff  --git a/clang-tools-extra/clangd/index/Merge.cpp b/clang-tools-extra/clangd/index/Merge.cpp
index 0dd0d9e01518..6f369ed2edcf 100644
--- a/clang-tools-extra/clangd/index/Merge.cpp
+++ b/clang-tools-extra/clangd/index/Merge.cpp
@@ -44,21 +44,23 @@ bool MergedIndex::fuzzyFind(
   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 @@ void MergedIndex::lookup(
   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);