[clang] 2214b90 - [clangd] Make signatureHelp work with stale preambles
Mikael Holmén via cfe-commits
cfe-commits at lists.llvm.org
Thu Apr 23 23:11:51 PDT 2020
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.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
More information about the cfe-commits
mailing list