Skip to content

Commit 7388c34

Browse files
committed
[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
1 parent b1166e1 commit 7388c34

File tree

2 files changed

+40
-30
lines changed

2 files changed

+40
-30
lines changed

clang-tools-extra/clangd/index/Index.cpp

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,13 @@ void SwapIndex::relations(
7878

7979
llvm::unique_function<bool(llvm::StringRef) const>
8080
SwapIndex::indexedFiles() const {
81-
return snapshot()->indexedFiles();
81+
// The index snapshot should outlive this method return value.
82+
auto SnapShot = snapshot();
83+
auto IndexedFiles = SnapShot->indexedFiles();
84+
return [KeepAlive{std::move(SnapShot)},
85+
IndexContainsFile{std::move(IndexedFiles)}](llvm::StringRef File) {
86+
return IndexContainsFile(File);
87+
};
8288
}
8389

8490
size_t SwapIndex::estimateMemoryUsage() const {

clang-tools-extra/clangd/index/Merge.cpp

Lines changed: 33 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -44,21 +44,23 @@ bool MergedIndex::fuzzyFind(
4444
SymbolSlab Dyn = std::move(DynB).build();
4545

4646
llvm::DenseSet<SymbolID> SeenDynamicSymbols;
47-
auto DynamicContainsFile = Dynamic->indexedFiles();
48-
More |= Static->fuzzyFind(Req, [&](const Symbol &S) {
49-
// We expect the definition to see the canonical declaration, so it seems
50-
// to be enough to check only the definition if it exists.
51-
if (DynamicContainsFile(S.Definition ? S.Definition.FileURI
52-
: S.CanonicalDeclaration.FileURI))
53-
return;
54-
auto DynS = Dyn.find(S.ID);
55-
++StaticCount;
56-
if (DynS == Dyn.end())
57-
return Callback(S);
58-
++MergedCount;
59-
SeenDynamicSymbols.insert(S.ID);
60-
Callback(mergeSymbol(*DynS, S));
61-
});
47+
{
48+
auto DynamicContainsFile = Dynamic->indexedFiles();
49+
More |= Static->fuzzyFind(Req, [&](const Symbol &S) {
50+
// We expect the definition to see the canonical declaration, so it seems
51+
// to be enough to check only the definition if it exists.
52+
if (DynamicContainsFile(S.Definition ? S.Definition.FileURI
53+
: S.CanonicalDeclaration.FileURI))
54+
return;
55+
auto DynS = Dyn.find(S.ID);
56+
++StaticCount;
57+
if (DynS == Dyn.end())
58+
return Callback(S);
59+
++MergedCount;
60+
SeenDynamicSymbols.insert(S.ID);
61+
Callback(mergeSymbol(*DynS, S));
62+
});
63+
}
6264
SPAN_ATTACH(Tracer, "dynamic", DynamicCount);
6365
SPAN_ATTACH(Tracer, "static", StaticCount);
6466
SPAN_ATTACH(Tracer, "merged", MergedCount);
@@ -77,20 +79,22 @@ void MergedIndex::lookup(
7779
Dynamic->lookup(Req, [&](const Symbol &S) { B.insert(S); });
7880

7981
auto RemainingIDs = Req.IDs;
80-
auto DynamicContainsFile = Dynamic->indexedFiles();
81-
Static->lookup(Req, [&](const Symbol &S) {
82-
// We expect the definition to see the canonical declaration, so it seems
83-
// to be enough to check only the definition if it exists.
84-
if (DynamicContainsFile(S.Definition ? S.Definition.FileURI
85-
: S.CanonicalDeclaration.FileURI))
86-
return;
87-
const Symbol *Sym = B.find(S.ID);
88-
RemainingIDs.erase(S.ID);
89-
if (!Sym)
90-
Callback(S);
91-
else
92-
Callback(mergeSymbol(*Sym, S));
93-
});
82+
{
83+
auto DynamicContainsFile = Dynamic->indexedFiles();
84+
Static->lookup(Req, [&](const Symbol &S) {
85+
// We expect the definition to see the canonical declaration, so it seems
86+
// to be enough to check only the definition if it exists.
87+
if (DynamicContainsFile(S.Definition ? S.Definition.FileURI
88+
: S.CanonicalDeclaration.FileURI))
89+
return;
90+
const Symbol *Sym = B.find(S.ID);
91+
RemainingIDs.erase(S.ID);
92+
if (!Sym)
93+
Callback(S);
94+
else
95+
Callback(mergeSymbol(*Sym, S));
96+
});
97+
}
9498
for (const auto &ID : RemainingIDs)
9599
if (const Symbol *Sym = B.find(ID))
96100
Callback(*Sym);

0 commit comments

Comments
 (0)