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); _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
