[clang] 2214b90 - [clangd] Make signatureHelp work with stale preambles
Kadir Çetinkaya via cfe-commits
cfe-commits at lists.llvm.org
Thu Apr 23 07:46:52 PDT 2020
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.holmen at 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 at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200423/c6b72c33/attachment-0001.html>
More information about the cfe-commits
mailing list