ilya-biryukov created this revision.
ilya-biryukov added reviewers: ioeric, sammccall.
Herald added subscribers: jkorous, MaskRay, javed.absar, klimek.
This is more efficient and avoids data races when reading files that
come from the preamble. The staleness can occur when reading a file
from disk that changed after the preamble was built. This can lead to
crashes, e.g. when parsing comments.
We do not to rely on symbols from the main file anyway, since any info
that those provide can always be taken from the AST.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47272
Files:
clangd/ClangdServer.cpp
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/TUScheduler.cpp
clangd/TUScheduler.h
clangd/index/FileIndex.cpp
clangd/index/FileIndex.h
unittests/clangd/FileIndexTests.cpp
unittests/clangd/TestTU.cpp
Index: unittests/clangd/TestTU.cpp
===================================================================
--- unittests/clangd/TestTU.cpp
+++ unittests/clangd/TestTU.cpp
@@ -43,7 +43,7 @@
SymbolSlab TestTU::headerSymbols() const {
auto AST = build();
- return indexAST(&AST);
+ return indexAST(AST.getASTContext(), AST.getPreprocessorPtr());
}
std::unique_ptr<SymbolIndex> TestTU::index() const {
Index: unittests/clangd/FileIndexTests.cpp
===================================================================
--- unittests/clangd/FileIndexTests.cpp
+++ unittests/clangd/FileIndexTests.cpp
@@ -87,7 +87,7 @@
File.HeaderFilename = (Basename + ".h").str();
File.HeaderCode = Code;
auto AST = File.build();
- M.update(File.Filename, &AST);
+ M.update(File.Filename, &AST.getASTContext(), AST.getPreprocessorPtr());
}
TEST(FileIndexTest, IndexAST) {
@@ -129,13 +129,13 @@
Req.Scopes = {"ns::"};
EXPECT_THAT(match(M, Req), UnorderedElementsAre("ns::f", "ns::X"));
- M.update("f1.cpp", nullptr);
+ M.update("f1.cpp", nullptr, nullptr);
EXPECT_THAT(match(M, Req), UnorderedElementsAre());
}
TEST(FileIndexTest, RemoveNonExisting) {
FileIndex M;
- M.update("no.cpp", nullptr);
+ M.update("no.cpp", nullptr, nullptr);
EXPECT_THAT(match(M, FuzzyFindRequest()), UnorderedElementsAre());
}
Index: clangd/index/FileIndex.h
===================================================================
--- clangd/index/FileIndex.h
+++ clangd/index/FileIndex.h
@@ -19,6 +19,7 @@
#include "../ClangdUnit.h"
#include "Index.h"
#include "MemIndex.h"
+#include "clang/Lex/Preprocessor.h"
namespace clang {
namespace clangd {
@@ -56,8 +57,10 @@
class FileIndex : public SymbolIndex {
public:
/// \brief Update symbols in \p Path with symbols in \p AST. If \p AST is
- /// nullptr, this removes all symbols in the file
- void update(PathRef Path, ParsedAST *AST);
+ /// nullptr, this removes all symbols in the file.
+ /// If \p AST is not null, \p PP cannot be null and it should be the
+ /// preprocessor that was used to build \p AST.
+ void update(PathRef Path, ASTContext *AST, std::shared_ptr<Preprocessor> PP);
bool
fuzzyFind(const FuzzyFindRequest &Req,
@@ -73,7 +76,7 @@
/// Retrieves namespace and class level symbols in \p AST.
/// Exposed to assist in unit tests.
-SymbolSlab indexAST(ParsedAST *AST);
+SymbolSlab indexAST(ASTContext &AST, std::shared_ptr<Preprocessor> PP);
} // namespace clangd
} // namespace clang
Index: clangd/index/FileIndex.cpp
===================================================================
--- clangd/index/FileIndex.cpp
+++ clangd/index/FileIndex.cpp
@@ -10,12 +10,12 @@
#include "FileIndex.h"
#include "SymbolCollector.h"
#include "clang/Index/IndexingAction.h"
+#include "clang/Lex/Preprocessor.h"
namespace clang {
namespace clangd {
-SymbolSlab indexAST(ParsedAST *AST) {
- assert(AST && "AST must not be nullptr!");
+SymbolSlab indexAST(ASTContext &AST, std::shared_ptr<Preprocessor> PP) {
SymbolCollector::Options CollectorOpts;
// FIXME(ioeric): we might also want to collect include headers. We would need
// to make sure all includes are canonicalized (with CanonicalIncludes), which
@@ -26,15 +26,18 @@
CollectorOpts.CountReferences = false;
SymbolCollector Collector(std::move(CollectorOpts));
- Collector.setPreprocessor(AST->getPreprocessorPtr());
+ Collector.setPreprocessor(PP);
index::IndexingOptions IndexOpts;
// We only need declarations, because we don't count references.
IndexOpts.SystemSymbolFilter =
index::IndexingOptions::SystemSymbolFilterKind::DeclarationsOnly;
IndexOpts.IndexFunctionLocals = false;
- index::indexTopLevelDecls(AST->getASTContext(), AST->getTopLevelDecls(),
- Collector, IndexOpts);
+ std::vector<Decl *> TopLevelDecls(
+ AST.getTranslationUnitDecl()->decls().begin(),
+ AST.getTranslationUnitDecl()->decls().end());
+ index::indexTopLevelDecls(AST, TopLevelDecls, Collector, IndexOpts);
+
return Collector.takeSymbols();
}
@@ -69,12 +72,14 @@
return {std::move(Snap), Pointers};
}
-void FileIndex::update(PathRef Path, ParsedAST *AST) {
+void FileIndex::update(PathRef Path, ASTContext *AST,
+ std::shared_ptr<Preprocessor> PP) {
if (!AST) {
FSymbols.update(Path, nullptr);
} else {
+ assert(PP);
auto Slab = llvm::make_unique<SymbolSlab>();
- *Slab = indexAST(AST);
+ *Slab = indexAST(*AST, PP);
FSymbols.update(Path, std::move(Slab));
}
auto Symbols = FSymbols.allSymbols();
Index: clangd/TUScheduler.h
===================================================================
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -52,7 +52,7 @@
class TUScheduler {
public:
TUScheduler(unsigned AsyncThreadsCount, bool StorePreamblesInMemory,
- ASTParsedCallback ASTCallback,
+ PreambleParsedCallback PreambleCallback,
std::chrono::steady_clock::duration UpdateDebounce);
~TUScheduler();
@@ -101,7 +101,7 @@
const bool StorePreamblesInMemory;
const std::shared_ptr<PCHContainerOperations> PCHOps;
- const ASTParsedCallback ASTCallback;
+ const PreambleParsedCallback PreambleCallback;
Semaphore Barrier;
llvm::StringMap<std::unique_ptr<FileData>> Files;
// None when running tasks synchronously and non-None when running tasks
Index: clangd/TUScheduler.cpp
===================================================================
--- clangd/TUScheduler.cpp
+++ clangd/TUScheduler.cpp
@@ -414,11 +414,11 @@
TUScheduler::TUScheduler(unsigned AsyncThreadsCount,
bool StorePreamblesInMemory,
- ASTParsedCallback ASTCallback,
+ PreambleParsedCallback PreambleCallback,
steady_clock::duration UpdateDebounce)
: StorePreamblesInMemory(StorePreamblesInMemory),
PCHOps(std::make_shared<PCHContainerOperations>()),
- ASTCallback(std::move(ASTCallback)), Barrier(AsyncThreadsCount),
+ PreambleCallback(std::move(PreambleCallback)), Barrier(AsyncThreadsCount),
UpdateDebounce(UpdateDebounce) {
if (0 < AsyncThreadsCount) {
PreambleTasks.emplace();
@@ -455,7 +455,7 @@
// Create a new worker to process the AST-related tasks.
ASTWorkerHandle Worker = ASTWorker::Create(
File, WorkerThreads ? WorkerThreads.getPointer() : nullptr, Barrier,
- CppFile(File, StorePreamblesInMemory, PCHOps, ASTCallback),
+ CppFile(File, StorePreamblesInMemory, PCHOps, PreambleCallback),
UpdateDebounce);
FD = std::unique_ptr<FileData>(new FileData{
Inputs.Contents, Inputs.CompileCommand, std::move(Worker)});
Index: clangd/ClangdUnit.h
===================================================================
--- clangd/ClangdUnit.h
+++ clangd/ClangdUnit.h
@@ -17,6 +17,7 @@
#include "Protocol.h"
#include "clang/Frontend/FrontendAction.h"
#include "clang/Frontend/PrecompiledPreamble.h"
+#include "clang/Lex/Preprocessor.h"
#include "clang/Serialization/ASTBitCodes.h"
#include "clang/Tooling/CompilationDatabase.h"
#include "clang/Tooling/Core/Replacement.h"
@@ -126,15 +127,16 @@
std::vector<Inclusion> Inclusions;
};
-using ASTParsedCallback = std::function<void(PathRef Path, ParsedAST *)>;
+using PreambleParsedCallback = std::function<void(
+ PathRef Path, ASTContext &, std::shared_ptr<clang::Preprocessor>)>;
/// Manages resources, required by clangd. Allows to rebuild file with new
/// contents, and provides AST and Preamble for it.
class CppFile {
public:
CppFile(PathRef FileName, bool StorePreamblesInMemory,
std::shared_ptr<PCHContainerOperations> PCHs,
- ASTParsedCallback ASTCallback);
+ PreambleParsedCallback PreambleCallback);
/// Rebuild the AST and the preamble.
/// Returns a list of diagnostics or llvm::None, if an error occured.
@@ -170,7 +172,7 @@
std::shared_ptr<PCHContainerOperations> PCHs;
/// This is called after the file is parsed. This can be nullptr if there is
/// no callback.
- ASTParsedCallback ASTCallback;
+ PreambleParsedCallback PreambleCallback;
};
/// Get the beginning SourceLocation at a specified \p Pos.
Index: clangd/ClangdUnit.cpp
===================================================================
--- clangd/ClangdUnit.cpp
+++ clangd/ClangdUnit.cpp
@@ -13,6 +13,8 @@
#include "Logger.h"
#include "SourceCode.h"
#include "Trace.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/Basic/LangOptions.h"
#include "clang/Frontend/CompilerInstance.h"
#include "clang/Frontend/CompilerInvocation.h"
#include "clang/Frontend/FrontendActions.h"
@@ -25,7 +27,6 @@
#include "clang/Sema/Sema.h"
#include "clang/Serialization/ASTWriter.h"
#include "clang/Tooling/CompilationDatabase.h"
-#include "clang/Basic/LangOptions.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/Support/CrashRecoveryContext.h"
@@ -86,6 +87,9 @@
class CppFilePreambleCallbacks : public PreambleCallbacks {
public:
+ CppFilePreambleCallbacks(PathRef File, PreambleParsedCallback ParsedCallback)
+ : File(File), ParsedCallback(ParsedCallback) {}
+
std::vector<serialization::DeclID> takeTopLevelDeclIDs() {
return std::move(TopLevelDeclIDs);
}
@@ -102,6 +106,13 @@
}
}
+ void AfterExecute(CompilerInstance &CI) override {
+ if (!ParsedCallback)
+ return;
+ trace::Span Tracer("Running PreambleCallback");
+ ParsedCallback(File, CI.getASTContext(), CI.getPreprocessorPtr());
+ }
+
void HandleTopLevelDecl(DeclGroupRef DG) override {
for (Decl *D : DG) {
if (isa<ObjCMethodDecl>(D))
@@ -122,6 +133,8 @@
}
private:
+ PathRef File;
+ PreambleParsedCallback ParsedCallback;
std::vector<Decl *> TopLevelDecls;
std::vector<serialization::DeclID> TopLevelDeclIDs;
std::vector<Inclusion> Inclusions;
@@ -277,9 +290,9 @@
CppFile::CppFile(PathRef FileName, bool StorePreamblesInMemory,
std::shared_ptr<PCHContainerOperations> PCHs,
- ASTParsedCallback ASTCallback)
+ PreambleParsedCallback PreambleCallback)
: FileName(FileName), StorePreamblesInMemory(StorePreamblesInMemory),
- PCHs(std::move(PCHs)), ASTCallback(std::move(ASTCallback)) {
+ PCHs(std::move(PCHs)), PreambleCallback(std::move(PreambleCallback)) {
log("Created CppFile for " + FileName);
}
@@ -346,10 +359,6 @@
Diagnostics.insert(Diagnostics.end(), NewAST->getDiagnostics().begin(),
NewAST->getDiagnostics().end());
}
- if (ASTCallback && NewAST) {
- trace::Span Tracer("Running ASTCallback");
- ASTCallback(FileName, NewAST.getPointer());
- }
// Write the results of rebuild into class fields.
Command = std::move(Inputs.CompileCommand);
@@ -404,7 +413,7 @@
assert(!CI.getFrontendOpts().SkipFunctionBodies);
CI.getFrontendOpts().SkipFunctionBodies = true;
- CppFilePreambleCallbacks SerializedDeclsCollector;
+ CppFilePreambleCallbacks SerializedDeclsCollector(FileName, PreambleCallback);
auto BuiltPreamble = PrecompiledPreamble::Build(
CI, &ContentsBuffer, Bounds, *PreambleDiagsEngine, FS, PCHs,
/*StoreInMemory=*/StorePreamblesInMemory, SerializedDeclsCollector);
Index: clangd/ClangdServer.cpp
===================================================================
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -17,6 +17,7 @@
#include "clang/Format/Format.h"
#include "clang/Frontend/CompilerInstance.h"
#include "clang/Frontend/CompilerInvocation.h"
+#include "clang/Lex/Preprocessor.h"
#include "clang/Tooling/CompilationDatabase.h"
#include "clang/Tooling/Refactoring/RefactoringResultConsumer.h"
#include "clang/Tooling/Refactoring/Rename/RenamingAction.h"
@@ -92,12 +93,14 @@
// is parsed.
// FIXME(ioeric): this can be slow and we may be able to index on less
// critical paths.
- WorkScheduler(Opts.AsyncThreadsCount, Opts.StorePreamblesInMemory,
- FileIdx
- ? [this](PathRef Path,
- ParsedAST *AST) { FileIdx->update(Path, AST); }
- : ASTParsedCallback(),
- Opts.UpdateDebounce) {
+ WorkScheduler(
+ Opts.AsyncThreadsCount, Opts.StorePreamblesInMemory,
+ FileIdx
+ ? [this](PathRef Path, ASTContext &AST,
+ std::shared_ptr<Preprocessor>
+ PP) { FileIdx->update(Path, &AST, std::move(PP)); }
+ : PreambleParsedCallback(),
+ Opts.UpdateDebounce) {
if (FileIdx && Opts.StaticIndex) {
MergedIndex = mergeIndex(FileIdx.get(), Opts.StaticIndex);
Index = MergedIndex.get();
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
