Thanks Mikael, sent out an ugly fix at 89cb5d558895706e053bc3af972aa5b15aa82863 to get sanitizer build bots running.
Will change the testing scheme for a proper fix. On Thu, Apr 23, 2020 at 4:53 PM Mikael Holmén <mikael.hol...@ericsson.com> wrote: > Hi Kadir, > > I start seeing some sanitizer complaints with this commit for the > following two testcases: > ./ClangdTests/PreamblePatchTest.ContainsNewIncludes > ./ClangdTests/PreamblePatchTest.IncludeParsing > > The complaints look like this: > > ================================================================= > ==75126==ERROR: LeakSanitizer: detected memory leaks > > Direct leak of 369 byte(s) in 4 object(s) allocated from: > #0 0x5bbe40 in operator new(unsigned long, std::nothrow_t const&) > /repo/uabkaka/tmp/llvm/projects/compiler- > rt/lib/asan/asan_new_delete.cc:112 > #1 0x149a3e9 in > llvm::WritableMemoryBuffer::getNewUninitMemBuffer(unsigned long, > llvm::Twine const&) /repo/bbiswjenk/fem023- > eiffel003/workspace/llvm/llvm-master-sanitize-asan/llvm/build-all- > asan/../lib/Support/MemoryBuffer.cpp:298:34 > #2 0x1498db3 in getMemBufferCopyImpl /repo/bbiswjenk/fem023- > eiffel003/workspace/llvm/llvm-master-sanitize-asan/llvm/build-all- > asan/../lib/Support/MemoryBuffer.cpp:127:14 > #3 0x1498db3 in > llvm::MemoryBuffer::getMemBufferCopy(llvm::StringRef, llvm::Twine > const&) /repo/bbiswjenk/fem023-eiffel003/workspace/llvm/llvm-master- > sanitize-asan/llvm/build-all-asan/../lib/Support/MemoryBuffer.cpp:136 > #4 0x506012e in > clang::clangd::PreamblePatch::apply(clang::CompilerInvocation&) const > /repo/bbiswjenk/fem023-eiffel003/workspace/llvm/llvm-master-sanitize- > asan/llvm/build-all-asan/../../clang-tools- > extra/clangd/Preamble.cpp:337:7 > #5 0xf910e6 in clang::clangd::(anonymous > namespace)::PreamblePatchTest_IncludeParsing_Test::TestBody() > /repo/bbiswjenk/fem023-eiffel003/workspace/llvm/llvm-master-sanitize- > asan/llvm/build-all-asan/../../clang-tools- > extra/clangd/unittests/PreambleTests.cpp:91:57 > #6 0x164e8e0 in HandleExceptionsInMethodIfSupported<testing::Test, > void> /repo/bbiswjenk/fem023-eiffel003/workspace/llvm/llvm-master- > sanitize-asan/llvm/build-all- > asan/../utils/unittest/googletest/src/gtest.cc > #7 0x164e8e0 in testing::Test::Run() /repo/bbiswjenk/fem023- > eiffel003/workspace/llvm/llvm-master-sanitize-asan/llvm/build-all- > asan/../utils/unittest/googletest/src/gtest.cc:2474 > #8 0x1651fc5 in testing::TestInfo::Run() /repo/bbiswjenk/fem023- > eiffel003/workspace/llvm/llvm-master-sanitize-asan/llvm/build-all- > asan/../utils/unittest/googletest/src/gtest.cc:2656:11 > #9 0x1653440 in testing::TestCase::Run() /repo/bbiswjenk/fem023- > eiffel003/workspace/llvm/llvm-master-sanitize-asan/llvm/build-all- > asan/../utils/unittest/googletest/src/gtest.cc:2774:28 > #10 0x1671e64 in testing::internal::UnitTestImpl::RunAllTests() > /repo/bbiswjenk/fem023-eiffel003/workspace/llvm/llvm-master-sanitize- > asan/llvm/build-all- > asan/../utils/unittest/googletest/src/gtest.cc:4649:43 > #11 0x1671010 in > HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, > bool> /repo/bbiswjenk/fem023-eiffel003/workspace/llvm/llvm-master- > sanitize-asan/llvm/build-all- > asan/../utils/unittest/googletest/src/gtest.cc > #12 0x1671010 in testing::UnitTest::Run() /repo/bbiswjenk/fem023- > eiffel003/workspace/llvm/llvm-master-sanitize-asan/llvm/build-all- > asan/../utils/unittest/googletest/src/gtest.cc:4257 > #13 0x1633040 in RUN_ALL_TESTS /repo/bbiswjenk/fem023- > eiffel003/workspace/llvm/llvm-master-sanitize-asan/llvm/build-all- > asan/../utils/unittest/googletest/include/gtest/gtest.h:2233:46 > #14 0x1633040 in main /repo/bbiswjenk/fem023- > eiffel003/workspace/llvm/llvm-master-sanitize-asan/llvm/build-all- > asan/../utils/unittest/UnitTestMain/TestMain.cpp:50 > #15 0x7f7dfdfe6544 in __libc_start_main (/lib64/libc.so.6+0x22544) > > SUMMARY: AddressSanitizer: 369 byte(s) leaked in 4 allocation(s). > > Regards, > Mikael > > On Tue, 2020-04-21 at 01:34 -0700, Kadir Cetinkaya via cfe-commits > wrote: > > Author: Kadir Cetinkaya > > Date: 2020-04-21T10:27:26+02:00 > > New Revision: 2214b9076f1d3a4784820c4479e2417685e5c980 > > > > URL: > > > https://github.com/llvm/llvm-project/commit/2214b9076f1d3a4784820c4479e2417685e5c980 > > DIFF: > > > https://github.com/llvm/llvm-project/commit/2214b9076f1d3a4784820c4479e2417685e5c980.diff > > > > LOG: [clangd] Make signatureHelp work with stale preambles > > > > Summary: > > This is achieved by calculating newly added includes and implicitly > > parsing them as if they were part of the main file. > > > > This also gets rid of the need for consistent preamble reads. > > > > Reviewers: sammccall > > > > Subscribers: ilya-biryukov, javed.absar, MaskRay, jkorous, mgrang, > > arphaman, jfb, usaxena95, cfe-commits > > > > Tags: #clang > > > > Differential Revision: https://reviews.llvm.org/D77392 > > > > Added: > > clang-tools-extra/clangd/unittests/PreambleTests.cpp > > > > Modified: > > clang-tools-extra/clangd/ClangdServer.cpp > > clang-tools-extra/clangd/CodeComplete.cpp > > clang-tools-extra/clangd/Preamble.cpp > > clang-tools-extra/clangd/Preamble.h > > clang-tools-extra/clangd/TUScheduler.cpp > > clang-tools-extra/clangd/TUScheduler.h > > clang-tools-extra/clangd/unittests/CMakeLists.txt > > clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp > > clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp > > clang/include/clang/Basic/TokenKinds.h > > clang/include/clang/Frontend/PrecompiledPreamble.h > > > > Removed: > > > > > > > > ##################################################################### > > ########### > > diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang- > > tools-extra/clangd/ClangdServer.cpp > > index f82bfa4f2682..c5148f81f3df 100644 > > --- a/clang-tools-extra/clangd/ClangdServer.cpp > > +++ b/clang-tools-extra/clangd/ClangdServer.cpp > > @@ -280,11 +280,8 @@ void ClangdServer::signatureHelp(PathRef File, > > Position Pos, > > Pos, FS, Index)); > > }; > > > > - // Unlike code completion, we wait for an up-to-date preamble > > here. > > - // Signature help is often triggered after code completion. If the > > code > > - // completion inserted a header to make the symbol available, then > > using > > - // the old preamble would yield useless results. > > - WorkScheduler.runWithPreamble("SignatureHelp", File, > > TUScheduler::Consistent, > > + // Unlike code completion, we wait for a preamble here. > > + WorkScheduler.runWithPreamble("SignatureHelp", File, > > TUScheduler::Stale, > > std::move(Action)); > > } > > > > > > diff --git a/clang-tools-extra/clangd/CodeComplete.cpp b/clang- > > tools-extra/clangd/CodeComplete.cpp > > index 7dbb4f5b78a3..3fa5b53ebaad 100644 > > --- a/clang-tools-extra/clangd/CodeComplete.cpp > > +++ b/clang-tools-extra/clangd/CodeComplete.cpp > > @@ -1023,6 +1023,7 @@ struct SemaCompleteInput { > > PathRef FileName; > > const tooling::CompileCommand &Command; > > const PreambleData &Preamble; > > + const PreamblePatch &PreamblePatch; > > llvm::StringRef Contents; > > size_t Offset; > > llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS; > > @@ -1060,7 +1061,6 @@ bool > > semaCodeComplete(std::unique_ptr<CodeCompleteConsumer> Consumer, > > ParseInput.CompileCommand = Input.Command; > > ParseInput.FS = VFS; > > ParseInput.Contents = std::string(Input.Contents); > > - ParseInput.Opts = ParseOptions(); > > > > IgnoreDiagnostics IgnoreDiags; > > auto CI = buildCompilerInvocation(ParseInput, IgnoreDiags); > > @@ -1096,6 +1096,7 @@ bool > > semaCodeComplete(std::unique_ptr<CodeCompleteConsumer> Consumer, > > PreambleBounds PreambleRegion = > > ComputePreambleBounds(*CI->getLangOpts(), > > ContentsBuffer.get(), 0); > > bool CompletingInPreamble = PreambleRegion.Size > Input.Offset; > > + Input.PreamblePatch.apply(*CI); > > // NOTE: we must call BeginSourceFile after > > prepareCompilerInstance. Otherwise > > // the remapped buffers do not get freed. > > auto Clang = prepareCompilerInstance( > > @@ -1754,8 +1755,10 @@ codeComplete(PathRef FileName, const > > tooling::CompileCommand &Command, > > SpecFuzzyFind, Opts); > > return (!Preamble || Opts.RunParser == > > CodeCompleteOptions::NeverParse) > > ? std::move(Flow).runWithoutSema(Contents, *Offset, > > VFS) > > - : std::move(Flow).run( > > - {FileName, Command, *Preamble, Contents, *Offset, > > VFS}); > > + : std::move(Flow).run({FileName, Command, *Preamble, > > + // We want to serve code > > completions with > > + // low latency, so don't bother > > patching. > > + PreamblePatch(), Contents, > > *Offset, VFS}); > > } > > > > SignatureHelp signatureHelp(PathRef FileName, > > @@ -1775,10 +1778,15 @@ SignatureHelp signatureHelp(PathRef FileName, > > Options.IncludeMacros = false; > > Options.IncludeCodePatterns = false; > > Options.IncludeBriefComments = false; > > - IncludeStructure PreambleInclusions; // Unused for signatureHelp > > + > > + ParseInputs PI; > > + PI.CompileCommand = Command; > > + PI.Contents = Contents.str(); > > + PI.FS = std::move(VFS); > > + auto PP = PreamblePatch::create(FileName, PI, Preamble); > > semaCodeComplete( > > std::make_unique<SignatureHelpCollector>(Options, Index, > > Result), Options, > > - {FileName, Command, Preamble, Contents, *Offset, > > std::move(VFS)}); > > + {FileName, Command, Preamble, PP, Contents, *Offset, > > std::move(PI.FS)}); > > return Result; > > } > > > > > > diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools- > > extra/clangd/Preamble.cpp > > index 97cd22a5d1fc..8392748227b4 100644 > > --- a/clang-tools-extra/clangd/Preamble.cpp > > +++ b/clang-tools-extra/clangd/Preamble.cpp > > @@ -8,11 +8,39 @@ > > > > #include "Preamble.h" > > #include "Compiler.h" > > +#include "Headers.h" > > #include "Logger.h" > > #include "Trace.h" > > +#include "clang/Basic/Diagnostic.h" > > +#include "clang/Basic/LangOptions.h" > > #include "clang/Basic/SourceLocation.h" > > +#include "clang/Basic/TokenKinds.h" > > +#include "clang/Frontend/CompilerInvocation.h" > > +#include "clang/Frontend/FrontendActions.h" > > +#include "clang/Lex/Lexer.h" > > #include "clang/Lex/PPCallbacks.h" > > +#include "clang/Lex/Preprocessor.h" > > #include "clang/Lex/PreprocessorOptions.h" > > +#include "clang/Tooling/CompilationDatabase.h" > > +#include "llvm/ADT/ArrayRef.h" > > +#include "llvm/ADT/IntrusiveRefCntPtr.h" > > +#include "llvm/ADT/STLExtras.h" > > +#include "llvm/ADT/SmallString.h" > > +#include "llvm/ADT/StringRef.h" > > +#include "llvm/ADT/StringSet.h" > > +#include "llvm/Support/Error.h" > > +#include "llvm/Support/ErrorHandling.h" > > +#include "llvm/Support/FormatVariadic.h" > > +#include "llvm/Support/MemoryBuffer.h" > > +#include "llvm/Support/Path.h" > > +#include "llvm/Support/VirtualFileSystem.h" > > +#include "llvm/Support/raw_ostream.h" > > +#include <iterator> > > +#include <memory> > > +#include <string> > > +#include <system_error> > > +#include <utility> > > +#include <vector> > > > > namespace clang { > > namespace clangd { > > @@ -74,6 +102,81 @@ class CppFilePreambleCallbacks : public > > PreambleCallbacks { > > const SourceManager *SourceMgr = nullptr; > > }; > > > > +// Runs preprocessor over preamble section. > > +class PreambleOnlyAction : public PreprocessorFrontendAction { > > +protected: > > + void ExecuteAction() override { > > + Preprocessor &PP = getCompilerInstance().getPreprocessor(); > > + auto &SM = PP.getSourceManager(); > > + PP.EnterMainSourceFile(); > > + auto Bounds = > > ComputePreambleBounds(getCompilerInstance().getLangOpts(), > > + SM.getBuffer(SM.getMainFileI > > D()), 0); > > + Token Tok; > > + do { > > + PP.Lex(Tok); > > + assert(SM.isInMainFile(Tok.getLocation())); > > + } while (Tok.isNot(tok::eof) && > > + SM.getDecomposedLoc(Tok.getLocation()).second < > > Bounds.Size); > > + } > > +}; > > + > > +/// Gets the includes in the preamble section of the file by running > > +/// preprocessor over \p Contents. Returned includes do not contain > > resolved > > +/// paths. \p VFS and \p Cmd is used to build the compiler > > invocation, which > > +/// might stat/read files. > > +llvm::Expected<std::vector<Inclusion>> > > +scanPreambleIncludes(llvm::StringRef Contents, > > + llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> > > VFS, > > + const tooling::CompileCommand &Cmd) { > > + // Build and run Preprocessor over the preamble. > > + ParseInputs PI; > > + PI.Contents = Contents.str(); > > + PI.FS = std::move(VFS); > > + PI.CompileCommand = Cmd; > > + IgnoringDiagConsumer IgnoreDiags; > > + auto CI = buildCompilerInvocation(PI, IgnoreDiags); > > + if (!CI) > > + return llvm::createStringError(llvm::inconvertibleErrorCode(), > > + "failed to create compiler > > invocation"); > > + CI->getDiagnosticOpts().IgnoreWarnings = true; > > + auto ContentsBuffer = llvm::MemoryBuffer::getMemBuffer(Contents); > > + auto Clang = prepareCompilerInstance( > > + std::move(CI), nullptr, std::move(ContentsBuffer), > > + // Provide an empty FS to prevent preprocessor from performing > > IO. This > > + // also implies missing resolved paths for includes. > > + new llvm::vfs::InMemoryFileSystem, IgnoreDiags); > > + if (Clang->getFrontendOpts().Inputs.empty()) > > + return llvm::createStringError(llvm::inconvertibleErrorCode(), > > + "compiler instance had no > > inputs"); > > + // We are only interested in main file includes. > > + Clang->getPreprocessorOpts().SingleFileParseMode = true; > > + PreambleOnlyAction Action; > > + if (!Action.BeginSourceFile(*Clang, Clang- > > >getFrontendOpts().Inputs[0])) > > + return llvm::createStringError(llvm::inconvertibleErrorCode(), > > + "failed BeginSourceFile"); > > + Preprocessor &PP = Clang->getPreprocessor(); > > + IncludeStructure Includes; > > + PP.addPPCallbacks( > > + collectIncludeStructureCallback(Clang->getSourceManager(), > > &Includes)); > > + if (llvm::Error Err = Action.Execute()) > > + return std::move(Err); > > + Action.EndSourceFile(); > > + return Includes.MainFileIncludes; > > +} > > + > > +const char *spellingForIncDirective(tok::PPKeywordKind > > IncludeDirective) { > > + switch (IncludeDirective) { > > + case tok::pp_include: > > + return "include"; > > + case tok::pp_import: > > + return "import"; > > + case tok::pp_include_next: > > + return "include_next"; > > + default: > > + break; > > + } > > + llvm_unreachable("not an include directive"); > > +} > > } // namespace > > > > PreambleData::PreambleData(const ParseInputs &Inputs, > > @@ -166,5 +269,78 @@ bool isPreambleCompatible(const PreambleData > > &Preamble, > > Preamble.Preamble.CanReuse(CI, ContentsBuffer.get(), > > Bounds, > > Inputs.FS.get()); > > } > > + > > +PreamblePatch PreamblePatch::create(llvm::StringRef FileName, > > + const ParseInputs &Modified, > > + const PreambleData &Baseline) { > > + // First scan the include directives in Baseline and Modified. > > These will be > > + // used to figure out newly added directives in Modified. Scanning > > can fail, > > + // the code just bails out and creates an empty patch in such > > cases, as: > > + // - If scanning for Baseline fails, no knowledge of existing > > includes hence > > + // patch will contain all the includes in Modified. Leading to > > rebuild of > > + // whole preamble, which is terribly slow. > > + // - If scanning for Modified fails, cannot figure out newly added > > ones so > > + // there's nothing to do but generate an empty patch. > > + auto BaselineIncludes = scanPreambleIncludes( > > + // Contents needs to be null-terminated. > > + Baseline.Preamble.getContents().str(), > > + Baseline.StatCache->getConsumingFS(Modified.FS), > > Modified.CompileCommand); > > + if (!BaselineIncludes) { > > + elog("Failed to scan includes for baseline of {0}: {1}", > > FileName, > > + BaselineIncludes.takeError()); > > + return {}; > > + } > > + auto ModifiedIncludes = scanPreambleIncludes( > > + Modified.Contents, Baseline.StatCache- > > >getConsumingFS(Modified.FS), > > + Modified.CompileCommand); > > + if (!ModifiedIncludes) { > > + elog("Failed to scan includes for modified contents of {0}: > > {1}", FileName, > > + ModifiedIncludes.takeError()); > > + return {}; > > + } > > + > > + PreamblePatch PP; > > + // This shouldn't coincide with any real file name. > > + llvm::SmallString<128> PatchName; > > + llvm::sys::path::append(PatchName, > > llvm::sys::path::parent_path(FileName), > > + "__preamble_patch__.h"); > > + PP.PatchFileName = PatchName.str().str(); > > + > > + // We are only interested in newly added includes, record the ones > > in Baseline > > + // for exclusion. > > + llvm::DenseSet<std::pair<tok::PPKeywordKind, llvm::StringRef>> > > + ExistingIncludes; > > + for (const auto &Inc : *BaselineIncludes) > > + ExistingIncludes.insert({Inc.Directive, Inc.Written}); > > + // Calculate extra includes that needs to be inserted. > > + llvm::raw_string_ostream Patch(PP.PatchContents); > > + for (const auto &Inc : *ModifiedIncludes) { > > + if (ExistingIncludes.count({Inc.Directive, Inc.Written})) > > + continue; > > + Patch << llvm::formatv("#{0} {1}\n", > > spellingForIncDirective(Inc.Directive), > > + Inc.Written); > > + } > > + Patch.flush(); > > + > > + // FIXME: Handle more directives, e.g. define/undef. > > + return PP; > > +} > > + > > +void PreamblePatch::apply(CompilerInvocation &CI) const { > > + // No need to map an empty file. > > + if (PatchContents.empty()) > > + return; > > + auto &PPOpts = CI.getPreprocessorOpts(); > > + auto PatchBuffer = > > + // we copy here to ensure contents are still valid if CI > > outlives the > > + // PreamblePatch. > > + llvm::MemoryBuffer::getMemBufferCopy(PatchContents, > > PatchFileName); > > + // CI will take care of the lifetime of the buffer. > > + PPOpts.addRemappedFile(PatchFileName, PatchBuffer.release()); > > + // The patch will be parsed after loading the preamble ast and > > before parsing > > + // the main file. > > + PPOpts.Includes.push_back(PatchFileName); > > +} > > + > > } // namespace clangd > > } // namespace clang > > > > diff --git a/clang-tools-extra/clangd/Preamble.h b/clang-tools- > > extra/clangd/Preamble.h > > index bd67610e0ad7..10c292a71f38 100644 > > --- a/clang-tools-extra/clangd/Preamble.h > > +++ b/clang-tools-extra/clangd/Preamble.h > > @@ -32,6 +32,7 @@ > > #include "clang/Frontend/CompilerInvocation.h" > > #include "clang/Frontend/PrecompiledPreamble.h" > > #include "clang/Tooling/CompilationDatabase.h" > > +#include "llvm/ADT/StringRef.h" > > > > #include <memory> > > #include <string> > > @@ -88,6 +89,31 @@ buildPreamble(PathRef FileName, CompilerInvocation > > CI, > > bool isPreambleCompatible(const PreambleData &Preamble, > > const ParseInputs &Inputs, PathRef > > FileName, > > const CompilerInvocation &CI); > > + > > +/// Stores information required to parse a TU using a (possibly > > stale) Baseline > > +/// preamble. Updates compiler invocation to approximately reflect > > additions to > > +/// the preamble section of Modified contents, e.g. new include > > directives. > > +class PreamblePatch { > > +public: > > + // With an empty patch, the preamble is used verbatim. > > + PreamblePatch() = default; > > + /// Builds a patch that contains new PP directives introduced to > > the preamble > > + /// section of \p Modified compared to \p Baseline. > > + /// FIXME: This only handles include directives, we should at > > least handle > > + /// define/undef. > > + static PreamblePatch create(llvm::StringRef FileName, > > + const ParseInputs &Modified, > > + const PreambleData &Baseline); > > + /// Adjusts CI (which compiles the modified inputs) to be used > > with the > > + /// baseline preamble. This is done by inserting an artifical > > include to the > > + /// \p CI that contains new directives calculated in create. > > + void apply(CompilerInvocation &CI) const; > > + > > +private: > > + std::string PatchContents; > > + std::string PatchFileName; > > +}; > > + > > } // namespace clangd > > } // namespace clang > > > > > > diff --git a/clang-tools-extra/clangd/TUScheduler.cpp b/clang-tools- > > extra/clangd/TUScheduler.cpp > > index 26adcfd2b8f2..2f2abb59ab3c 100644 > > --- a/clang-tools-extra/clangd/TUScheduler.cpp > > +++ b/clang-tools-extra/clangd/TUScheduler.cpp > > @@ -866,36 +866,6 @@ ASTWorker::getPossiblyStalePreamble() const { > > return LatestPreamble; > > } > > > > -void ASTWorker::getCurrentPreamble( > > - llvm::unique_function<void(std::shared_ptr<const PreambleData>)> > > Callback) { > > - // We could just call startTask() to throw the read on the queue, > > knowing > > - // it will run after any updates. But we know this task is cheap, > > so to > > - // improve latency we cheat: insert it on the queue after the last > > update. > > - std::unique_lock<std::mutex> Lock(Mutex); > > - auto LastUpdate = > > - std::find_if(Requests.rbegin(), Requests.rend(), > > - [](const Request &R) { return > > R.UpdateType.hasValue(); }); > > - // If there were no writes in the queue, and CurrentRequest is not > > a write, > > - // the preamble is ready now. > > - if (LastUpdate == Requests.rend() && > > - (!CurrentRequest || CurrentRequest->UpdateType.hasValue())) { > > - Lock.unlock(); > > - return Callback(getPossiblyStalePreamble()); > > - } > > - assert(!RunSync && "Running synchronously, but queue is non- > > empty!"); > > - Requests.insert(LastUpdate.base(), > > - Request{[Callback = std::move(Callback), this]() > > mutable { > > - Callback(getPossiblyStalePreamble()); > > - }, > > - "GetPreamble", steady_clock::now(), > > - Context::current().clone(), > > - /*UpdateType=*/None, > > - /*InvalidationPolicy=*/TUScheduler::NoInva > > lidation, > > - /*Invalidate=*/nullptr}); > > - Lock.unlock(); > > - RequestsCV.notify_all(); > > -} > > - > > void ASTWorker::waitForFirstPreamble() const { > > BuiltFirstPreamble.wait(); } > > > > tooling::CompileCommand ASTWorker::getCurrentCompileCommand() const > > { > > @@ -1307,41 +1277,21 @@ void > > TUScheduler::runWithPreamble(llvm::StringRef Name, PathRef File, > > return; > > } > > > > - // Future is populated if the task needs a specific preamble. > > - std::future<std::shared_ptr<const PreambleData>> > > ConsistentPreamble; > > - // FIXME: Currently this only holds because ASTWorker blocks after > > issuing a > > - // preamble build. Get rid of consistent reads or make them build > > on the > > - // calling thread instead. > > - if (Consistency == Consistent) { > > - std::promise<std::shared_ptr<const PreambleData>> Promise; > > - ConsistentPreamble = Promise.get_future(); > > - It->second->Worker->getCurrentPreamble( > > - [Promise = std::move(Promise)]( > > - std::shared_ptr<const PreambleData> Preamble) mutable { > > - Promise.set_value(std::move(Preamble)); > > - }); > > - } > > - > > std::shared_ptr<const ASTWorker> Worker = It->second- > > >Worker.lock(); > > auto Task = > > [Worker, Consistency, Name = Name.str(), File = File.str(), > > Contents = It->second->Contents, > > Command = Worker->getCurrentCompileCommand(), > > Ctx = Context::current().derive(kFileBeingProcessed, > > std::string(File)), > > - ConsistentPreamble = std::move(ConsistentPreamble), > > Action = std::move(Action), this]() mutable { > > std::shared_ptr<const PreambleData> Preamble; > > - if (ConsistentPreamble.valid()) { > > - Preamble = ConsistentPreamble.get(); > > - } else { > > - if (Consistency != PreambleConsistency::StaleOrAbsent) { > > - // Wait until the preamble is built for the first time, > > if preamble > > - // is required. This avoids extra work of processing the > > preamble > > - // headers in parallel multiple times. > > - Worker->waitForFirstPreamble(); > > - } > > - Preamble = Worker->getPossiblyStalePreamble(); > > + if (Consistency == PreambleConsistency::Stale) { > > + // Wait until the preamble is built for the first time, if > > preamble > > + // is required. This avoids extra work of processing the > > preamble > > + // headers in parallel multiple times. > > + Worker->waitForFirstPreamble(); > > } > > + Preamble = Worker->getPossiblyStalePreamble(); > > > > std::lock_guard<Semaphore> BarrierLock(Barrier); > > WithContext Guard(std::move(Ctx)); > > > > diff --git a/clang-tools-extra/clangd/TUScheduler.h b/clang-tools- > > extra/clangd/TUScheduler.h > > index a7f5d2f493fb..48ed2c76f546 100644 > > --- a/clang-tools-extra/clangd/TUScheduler.h > > +++ b/clang-tools-extra/clangd/TUScheduler.h > > @@ -256,11 +256,6 @@ class TUScheduler { > > > > /// Controls whether preamble reads wait for the preamble to be > > up-to-date. > > enum PreambleConsistency { > > - /// The preamble is generated from the current version of the > > file. > > - /// If the content was recently updated, we will wait until we > > have a > > - /// preamble that reflects that update. > > - /// This is the slowest option, and may be delayed by other > > tasks. > > - Consistent, > > /// The preamble may be generated from an older version of the > > file. > > /// Reading from locations in the preamble may cause files to be > > re-read. > > /// This gives callers two options: > > > > diff --git a/clang-tools-extra/clangd/unittests/CMakeLists.txt > > b/clang-tools-extra/clangd/unittests/CMakeLists.txt > > index 541367a1d96a..4119445b85a0 100644 > > --- a/clang-tools-extra/clangd/unittests/CMakeLists.txt > > +++ b/clang-tools-extra/clangd/unittests/CMakeLists.txt > > @@ -59,6 +59,7 @@ add_unittest(ClangdUnitTests ClangdTests > > LSPClient.cpp > > ParsedASTTests.cpp > > PathMappingTests.cpp > > + PreambleTests.cpp > > PrintASTTests.cpp > > QualityTests.cpp > > RenameTests.cpp > > > > diff --git a/clang-tools- > > extra/clangd/unittests/CodeCompleteTests.cpp b/clang-tools- > > extra/clangd/unittests/CodeCompleteTests.cpp > > index 794b21ee61a5..47637024ab91 100644 > > --- a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp > > +++ b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp > > @@ -1186,6 +1186,31 @@ TEST(SignatureHelpTest, OpeningParen) { > > } > > } > > > > +TEST(SignatureHelpTest, StalePreamble) { > > + TestTU TU; > > + TU.Code = ""; > > + IgnoreDiagnostics Diags; > > + auto Inputs = TU.inputs(); > > + auto CI = buildCompilerInvocation(Inputs, Diags); > > + ASSERT_TRUE(CI); > > + auto EmptyPreamble = buildPreamble(testPath(TU.Filename), *CI, > > Inputs, > > + /*InMemory=*/true, > > /*Callback=*/nullptr); > > + ASSERT_TRUE(EmptyPreamble); > > + > > + TU.AdditionalFiles["a.h"] = "int foo(int x);"; > > + const Annotations Test(R"cpp( > > + #include "a.h" > > + void bar() { foo(^2); })cpp"); > > + TU.Code = Test.code().str(); > > + Inputs = TU.inputs(); > > + auto Results = > > + signatureHelp(testPath(TU.Filename), Inputs.CompileCommand, > > + *EmptyPreamble, TU.Code, Test.point(), > > Inputs.FS, nullptr); > > + EXPECT_THAT(Results.signatures, ElementsAre(Sig("foo([[int x]]) -> > > int"))); > > + EXPECT_EQ(0, Results.activeSignature); > > + EXPECT_EQ(0, Results.activeParameter); > > +} > > + > > class IndexRequestCollector : public SymbolIndex { > > public: > > bool > > > > diff --git a/clang-tools-extra/clangd/unittests/PreambleTests.cpp > > b/clang-tools-extra/clangd/unittests/PreambleTests.cpp > > new file mode 100644 > > index 000000000000..2815ca0a46f1 > > --- /dev/null > > +++ b/clang-tools-extra/clangd/unittests/PreambleTests.cpp > > @@ -0,0 +1,123 @@ > > +//===--- PreambleTests.cpp --------------------------------------*- > > C++ -*-===// > > +// > > +// Part of the LLVM Project, under the Apache License v2.0 with LLVM > > Exceptions. > > +// See https://llvm.org/LICENSE.txt for license information. > > +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception > > +// > > +//===--------------------------------------------------------------- > > -------===// > > + > > +#include "Annotations.h" > > +#include "Compiler.h" > > +#include "Preamble.h" > > +#include "TestFS.h" > > +#include "clang/Lex/PreprocessorOptions.h" > > +#include "llvm/ADT/STLExtras.h" > > +#include "llvm/ADT/StringRef.h" > > +#include "gmock/gmock.h" > > +#include "gtest/gtest.h" > > +#include <string> > > +#include <vector> > > + > > +namespace clang { > > +namespace clangd { > > +namespace { > > + > > +using testing::_; > > +using testing::Contains; > > +using testing::Pair; > > + > > +MATCHER_P(HasContents, Contents, "") { return arg->getBuffer() == > > Contents; } > > + > > +TEST(PreamblePatchTest, IncludeParsing) { > > + MockFSProvider FS; > > + MockCompilationDatabase CDB; > > + IgnoreDiagnostics Diags; > > + ParseInputs PI; > > + PI.FS = FS.getFileSystem(); > > + > > + // We expect any line with a point to show up in the patch. > > + llvm::StringRef Cases[] = { > > + // Only preamble > > + R"cpp(^#include "a.h")cpp", > > + // Both preamble and mainfile > > + R"cpp( > > + ^#include "a.h" > > + garbage, finishes preamble > > + #include "a.h")cpp", > > + // Mixed directives > > + R"cpp( > > + ^#include "a.h" > > + #pragma directive > > + // some comments > > + ^#include_next <a.h> > > + #ifdef skipped > > + ^#import "a.h" > > + #endif)cpp", > > + // Broken directives > > + R"cpp( > > + #include "a > > + ^#include "a.h" > > + #include <b > > + ^#include <b.h>)cpp", > > + }; > > + > > + const auto FileName = testPath("foo.cc"); > > + for (const auto Case : Cases) { > > + Annotations Test(Case); > > + const auto Code = Test.code(); > > + PI.CompileCommand = *CDB.getCompileCommand(FileName); > > + > > + SCOPED_TRACE(Code); > > + // Check preamble lexing logic by building an empty preamble and > > patching it > > + // with all the contents. > > + PI.Contents = ""; > > + const auto CI = buildCompilerInvocation(PI, Diags); > > + const auto EmptyPreamble = buildPreamble(FileName, *CI, PI, > > true, nullptr); > > + PI.Contents = Code.str(); > > + > > + std::string ExpectedBuffer; > > + const auto Points = Test.points(); > > + for (const auto &P : Points) { > > + // Copy the whole line. > > + auto StartOffset = llvm::cantFail(positionToOffset(Code, P)); > > + ExpectedBuffer.append(Code.substr(StartOffset) > > + .take_until([](char C) { return C == > > '\n'; }) > > + .str()); > > + ExpectedBuffer += '\n'; > > + } > > + > > + PreamblePatch::create(FileName, PI, *EmptyPreamble).apply(*CI); > > + EXPECT_THAT(CI->getPreprocessorOpts().RemappedFileBuffers, > > + Contains(Pair(_, HasContents(ExpectedBuffer)))); > > + } > > +} > > + > > +TEST(PreamblePatchTest, ContainsNewIncludes) { > > + MockFSProvider FS; > > + MockCompilationDatabase CDB; > > + IgnoreDiagnostics Diags; > > + > > + const auto FileName = testPath("foo.cc"); > > + ParseInputs PI; > > + PI.FS = FS.getFileSystem(); > > + PI.CompileCommand = *CDB.getCompileCommand(FileName); > > + PI.Contents = "#include <existing.h>\n"; > > + > > + // Check > > diff ing logic by adding a new header to the preamble and ensuring > > + // only it is patched. > > + const auto CI = buildCompilerInvocation(PI, Diags); > > + const auto FullPreamble = buildPreamble(FileName, *CI, PI, true, > > nullptr); > > + > > + constexpr llvm::StringLiteral Patch = > > + "#include <test>\n#import <existing.h>\n"; > > + // We provide the same includes twice, they shouldn't be included > > in the > > + // patch. > > + PI.Contents = (Patch + PI.Contents + PI.Contents).str(); > > + PreamblePatch::create(FileName, PI, *FullPreamble).apply(*CI); > > + EXPECT_THAT(CI->getPreprocessorOpts().RemappedFileBuffers, > > + Contains(Pair(_, HasContents(Patch)))); > > +} > > + > > +} // namespace > > +} // namespace clangd > > +} // namespace clang > > > > diff --git a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp > > b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp > > index 7106e01a10e4..6f50a5acd4e3 100644 > > --- a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp > > +++ b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp > > @@ -246,66 +246,6 @@ TEST_F(TUSchedulerTests, Debounce) { > > EXPECT_EQ(2, CallbackCount); > > } > > > > -static std::vector<std::string> includes(const PreambleData > > *Preamble) { > > - std::vector<std::string> Result; > > - if (Preamble) > > - for (const auto &Inclusion : Preamble- > > >Includes.MainFileIncludes) > > - Result.push_back(Inclusion.Written); > > - return Result; > > -} > > - > > -TEST_F(TUSchedulerTests, PreambleConsistency) { > > - std::atomic<int> CallbackCount(0); > > - { > > - Notification InconsistentReadDone; // Must live longest. > > - TUScheduler S(CDB, optsForTest()); > > - auto Path = testPath("foo.cpp"); > > - // Schedule two updates (A, B) and two preamble reads (stale, > > consistent). > > - // The stale read should see A, and the consistent read should > > see B. > > - // (We recognize the preambles by their included files). > > - auto Inputs = getInputs(Path, "#include <A>"); > > - Inputs.Version = "A"; > > - updateWithCallback(S, Path, Inputs, WantDiagnostics::Yes, [&]() > > { > > - // This callback runs in between the two preamble updates. > > - > > - // This blocks update B, preventing it from winning the race > > - // against the stale read. > > - // If the first read was instead consistent, this would > > deadlock. > > - InconsistentReadDone.wait(); > > - // This delays update B, preventing it from winning a race > > - // against the consistent read. The consistent read sees B > > - // only because it waits for it. > > - // If the second read was stale, it would usually see A. > > - std::this_thread::sleep_for(std::chrono::milliseconds(100)); > > - }); > > - Inputs.Contents = "#include <B>"; > > - Inputs.Version = "B"; > > - S.update(Path, Inputs, WantDiagnostics::Yes); > > - > > - S.runWithPreamble("StaleRead", Path, TUScheduler::Stale, > > - [&](Expected<InputsAndPreamble> Pre) { > > - ASSERT_TRUE(bool(Pre)); > > - ASSERT_TRUE(Pre->Preamble); > > - EXPECT_EQ(Pre->Preamble->Version, "A"); > > - EXPECT_THAT(includes(Pre->Preamble), > > - ElementsAre("<A>")); > > - InconsistentReadDone.notify(); > > - ++CallbackCount; > > - }); > > - S.runWithPreamble("ConsistentRead", Path, > > TUScheduler::Consistent, > > - [&](Expected<InputsAndPreamble> Pre) { > > - ASSERT_TRUE(bool(Pre)); > > - ASSERT_TRUE(Pre->Preamble); > > - EXPECT_EQ(Pre->Preamble->Version, "B"); > > - EXPECT_THAT(includes(Pre->Preamble), > > - ElementsAre("<B>")); > > - ++CallbackCount; > > - }); > > - S.blockUntilIdle(timeoutSeconds(10)); > > - } > > - EXPECT_EQ(2, CallbackCount); > > -} > > - > > TEST_F(TUSchedulerTests, Cancellation) { > > // We have the following update/read sequence > > // U0 > > > > diff --git a/clang/include/clang/Basic/TokenKinds.h > > b/clang/include/clang/Basic/TokenKinds.h > > index c25181e6827c..4e66aa1c8c2d 100644 > > --- a/clang/include/clang/Basic/TokenKinds.h > > +++ b/clang/include/clang/Basic/TokenKinds.h > > @@ -14,6 +14,7 @@ > > #ifndef LLVM_CLANG_BASIC_TOKENKINDS_H > > #define LLVM_CLANG_BASIC_TOKENKINDS_H > > > > +#include "llvm/ADT/DenseMapInfo.h" > > #include "llvm/Support/Compiler.h" > > > > namespace clang { > > @@ -95,7 +96,25 @@ bool isAnnotation(TokenKind K); > > /// Return true if this is an annotation token representing a > > pragma. > > bool isPragmaAnnotation(TokenKind K); > > > > -} // end namespace tok > > -} // end namespace clang > > +} // end namespace tok > > +} // end namespace clang > > + > > +namespace llvm { > > +template <> struct DenseMapInfo<clang::tok::PPKeywordKind> { > > + static inline clang::tok::PPKeywordKind getEmptyKey() { > > + return clang::tok::PPKeywordKind::pp_not_keyword; > > + } > > + static inline clang::tok::PPKeywordKind getTombstoneKey() { > > + return clang::tok::PPKeywordKind::NUM_PP_KEYWORDS; > > + } > > + static unsigned getHashValue(const clang::tok::PPKeywordKind &Val) > > { > > + return static_cast<unsigned>(Val); > > + } > > + static bool isEqual(const clang::tok::PPKeywordKind &LHS, > > + const clang::tok::PPKeywordKind &RHS) { > > + return LHS == RHS; > > + } > > +}; > > +} // namespace llvm > > > > #endif > > > > diff --git a/clang/include/clang/Frontend/PrecompiledPreamble.h > > b/clang/include/clang/Frontend/PrecompiledPreamble.h > > index 0d95ee683eee..538f6c92ad55 100644 > > --- a/clang/include/clang/Frontend/PrecompiledPreamble.h > > +++ b/clang/include/clang/Frontend/PrecompiledPreamble.h > > @@ -16,6 +16,7 @@ > > #include "clang/Lex/Lexer.h" > > #include "clang/Lex/Preprocessor.h" > > #include "llvm/ADT/IntrusiveRefCntPtr.h" > > +#include "llvm/ADT/StringRef.h" > > #include "llvm/Support/AlignOf.h" > > #include "llvm/Support/MD5.h" > > #include <cstddef> > > @@ -94,6 +95,11 @@ class PrecompiledPreamble { > > /// be used for logging and debugging purposes only. > > std::size_t getSize() const; > > > > + /// Returned string is not null-terminated. > > + llvm::StringRef getContents() const { > > + return {PreambleBytes.data(), PreambleBytes.size()}; > > + } > > + > > /// Check whether PrecompiledPreamble can be reused for the new > > contents(\p > > /// MainFileBuffer) of the main file. > > bool CanReuse(const CompilerInvocation &Invocation, > > > > > > > > _______________________________________________ > > cfe-commits mailing list > > cfe-commits@lists.llvm.org > > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits