On Thu, 2020-04-23 at 17:46 +0300, Kadir Çetinkaya wrote: > Thanks Mikael, sent out an ugly fix > at 89cb5d558895706e053bc3af972aa5b15aa82863 to get sanitizer build > bots running. >
Nice, thanks! Now our buildbots went silent again. /Mikael > 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
