Author: ibiryukov Date: Wed Mar 14 10:46:52 2018 New Revision: 327537 URL: http://llvm.org/viewvc/llvm-project?rev=327537&view=rev Log: [clangd] Don't expose vfs in TUScheduler::runWithPreamble.
Summary: It was previously an easy way to concurrently access a mutable vfs, which is a recipe for disaster. Reviewers: sammccall Reviewed By: sammccall Subscribers: klimek, jkorous-apple, cfe-commits, ioeric Differential Revision: https://reviews.llvm.org/D44463 Modified: clang-tools-extra/trunk/clangd/ClangdServer.cpp clang-tools-extra/trunk/clangd/TUScheduler.cpp clang-tools-extra/trunk/clangd/TUScheduler.h clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp Modified: clang-tools-extra/trunk/clangd/ClangdServer.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.cpp?rev=327537&r1=327536&r2=327537&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/ClangdServer.cpp (original) +++ clang-tools-extra/trunk/clangd/ClangdServer.cpp Wed Mar 14 10:46:52 2018 @@ -159,12 +159,11 @@ void ClangdServer::codeComplete(PathRef llvm::Expected<InputsAndPreamble> IP) { assert(IP && "error when trying to read preamble for codeComplete"); auto PreambleData = IP->Preamble; - auto &Command = IP->Inputs.CompileCommand; // FIXME(ibiryukov): even if Preamble is non-null, we may want to check // both the old and the new version in case only one of them matches. CompletionList Result = clangd::codeComplete( - File, Command, PreambleData ? &PreambleData->Preamble : nullptr, + File, IP->Command, PreambleData ? &PreambleData->Preamble : nullptr, Contents, Pos, FS, PCHs, CodeCompleteOpts); CB(std::move(Result)); }; @@ -191,8 +190,7 @@ void ClangdServer::signatureHelp(PathRef return CB(IP.takeError()); auto PreambleData = IP->Preamble; - auto &Command = IP->Inputs.CompileCommand; - CB(clangd::signatureHelp(File, Command, + CB(clangd::signatureHelp(File, IP->Command, PreambleData ? &PreambleData->Preamble : nullptr, Contents, Pos, FS, PCHs)); }; Modified: clang-tools-extra/trunk/clangd/TUScheduler.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/TUScheduler.cpp?rev=327537&r1=327536&r2=327537&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/TUScheduler.cpp (original) +++ clang-tools-extra/trunk/clangd/TUScheduler.cpp Wed Mar 14 10:46:52 2018 @@ -407,7 +407,8 @@ unsigned getDefaultAsyncThreadsCount() { struct TUScheduler::FileData { /// Latest inputs, passed to TUScheduler::update(). - ParseInputs Inputs; + std::string Contents; + tooling::CompileCommand Command; ASTWorkerHandle Worker; }; @@ -456,9 +457,11 @@ void TUScheduler::update(PathRef File, P File, WorkerThreads ? WorkerThreads.getPointer() : nullptr, Barrier, CppFile(File, StorePreamblesInMemory, PCHOps, ASTCallback), UpdateDebounce); - FD = std::unique_ptr<FileData>(new FileData{Inputs, std::move(Worker)}); + FD = std::unique_ptr<FileData>(new FileData{ + Inputs.Contents, Inputs.CompileCommand, std::move(Worker)}); } else { - FD->Inputs = Inputs; + FD->Contents = Inputs.Contents; + FD->Command = Inputs.CompileCommand; } FD->Worker->update(std::move(Inputs), WantDiags, std::move(OnUpdated)); } @@ -500,26 +503,28 @@ void TUScheduler::runWithPreamble( SPAN_ATTACH(Tracer, "file", File); std::shared_ptr<const PreambleData> Preamble = It->second->Worker->getPossiblyStalePreamble(); - Action(InputsAndPreamble{It->second->Inputs, Preamble.get()}); + Action(InputsAndPreamble{It->second->Contents, It->second->Command, + Preamble.get()}); return; } - ParseInputs InputsCopy = It->second->Inputs; std::shared_ptr<const ASTWorker> Worker = It->second->Worker.lock(); - auto Task = [InputsCopy, Worker, this](std::string Name, std::string File, - Context Ctx, - decltype(Action) Action) mutable { + auto Task = [Worker, this](std::string Name, std::string File, + std::string Contents, + tooling::CompileCommand Command, Context Ctx, + decltype(Action) Action) mutable { std::lock_guard<Semaphore> BarrierLock(Barrier); WithContext Guard(std::move(Ctx)); trace::Span Tracer(Name); SPAN_ATTACH(Tracer, "file", File); std::shared_ptr<const PreambleData> Preamble = Worker->getPossiblyStalePreamble(); - Action(InputsAndPreamble{InputsCopy, Preamble.get()}); + Action(InputsAndPreamble{Contents, Command, Preamble.get()}); }; PreambleTasks->runAsync("task:" + llvm::sys::path::filename(File), Bind(Task, std::string(Name), std::string(File), + It->second->Contents, It->second->Command, Context::current().clone(), std::move(Action))); } Modified: clang-tools-extra/trunk/clangd/TUScheduler.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/TUScheduler.h?rev=327537&r1=327536&r2=327537&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/TUScheduler.h (original) +++ clang-tools-extra/trunk/clangd/TUScheduler.h Wed Mar 14 10:46:52 2018 @@ -29,7 +29,8 @@ struct InputsAndAST { }; struct InputsAndPreamble { - const ParseInputs &Inputs; + llvm::StringRef Contents; + const tooling::CompileCommand &Command; const PreambleData *Preamble; }; @@ -78,11 +79,14 @@ public: void runWithAST(llvm::StringRef Name, PathRef File, Callback<InputsAndAST> Action); - /// Schedule an async read of the Preamble. Preamble passed to \p Action may - /// be built for any version of the file, callers must not rely on it being - /// consistent with the current version of the file. - /// If an error occurs during processing, it is forwarded to the \p Action - /// callback. + /// Schedule an async read of the Preamble. + /// The preamble may be stale, generated from an older version of the file. + /// Reading from locations in the preamble may cause the files to be re-read. + /// This gives callers two options: + /// - validate that the preamble is still valid, and only use it in this case + /// - accept that preamble contents may be outdated, and try to avoid reading + /// source code from headers. If an error occurs during processing, it is + /// forwarded to the \p Action callback. void runWithPreamble(llvm::StringRef Name, PathRef File, Callback<InputsAndPreamble> Action); Modified: clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp?rev=327537&r1=327536&r2=327537&view=diff ============================================================================== --- clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp (original) +++ clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp Wed Mar 14 10:46:52 2018 @@ -226,8 +226,7 @@ TEST_F(TUSchedulerTests, ManyUpdates) { EXPECT_THAT(Context::current().get(NonceKey), Pointee(Nonce)); ASSERT_TRUE((bool)Preamble); - EXPECT_EQ(Preamble->Inputs.FS, Inputs.FS); - EXPECT_EQ(Preamble->Inputs.Contents, Inputs.Contents); + EXPECT_EQ(Preamble->Contents, Inputs.Contents); std::lock_guard<std::mutex> Lock(Mut); ++TotalPreambleReads; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
