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
