[PATCH] D77392: [WIP][clangd] Make signatureHelp work with stale preambles
kadircet updated this revision to Diff 258915.
kadircet marked 2 inline comments as done.
kadircet added a comment.
- Make scanPreambleIncludes return an Expected>
- Bail out in case of errors, document the rational.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77392/new/
https://reviews.llvm.org/D77392
Files:
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/PreambleTests.cpp
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
clang/include/clang/Basic/TokenKinds.h
clang/include/clang/Frontend/PrecompiledPreamble.h
Index: clang/include/clang/Frontend/PrecompiledPreamble.h
===
--- clang/include/clang/Frontend/PrecompiledPreamble.h
+++ 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
@@ -94,6 +95,11 @@
/// 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,
Index: clang/include/clang/Basic/TokenKinds.h
===
--- clang/include/clang/Basic/TokenKinds.h
+++ 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 @@
/// 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 {
+ 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(Val);
+ }
+ static bool isEqual(const clang::tok::PPKeywordKind &LHS,
+ const clang::tok::PPKeywordKind &RHS) {
+return LHS == RHS;
+ }
+};
+} // namespace llvm
#endif
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -246,66 +246,6 @@
EXPECT_EQ(2, CallbackCount);
}
-static std::vector includes(const PreambleData *Preamble) {
- std::vector Result;
- if (Preamble)
-for (const auto &Inclusion : Preamble->Includes.MainFileIncludes)
- Result.push_back(Inclusion.Written);
- return Result;
-}
-
-TEST_F(TUSchedulerTests, PreambleConsistency) {
- std::atomic 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 ");
-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 ";
-Inputs.Version = "B";
-S.update(Path, Inputs, WantDiagnostics::Yes);
-
-S.runWithPreamble("StaleRead", Path, TUScheduler::Stale,
- [&](Ex
[PATCH] D77392: [WIP][clangd] Make signatureHelp work with stale preambles
kadircet marked an inline comment as done. kadircet added inline comments. Comment at: clang-tools-extra/clangd/Preamble.cpp:281 + // We are only interested in newly added includes. + llvm::StringSet<> ExistingIncludes; + for (const auto &Inc : Preamble.LexedIncludes) sammccall wrote: > kadircet wrote: > > sammccall wrote: > > > Why not a DenseSet>? > > > (The copies probably don't matter, but I think it'd be a bit clearer and > > > more typesafe) > > PPKeywordKind didn't have a DenseMapInfo, adding one. It already has two > > invalid enum values. > Ugh, I thought enums had those implicitly. The need for two invalid values is > really annoying, it's probably why we don't have more implicit ones. > > I wonder whether it's feasible (technically and legally) to replace > DenseHashMap with a fork of absl::flat_hash_map. I'm pretty sure it's faster, > and it doesn't have these weird API requirements. IANAL, but they both seemed to have apache v2 :D as for technicalities, how hard can it be to replace one of the most basic data structures in a code base :P Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77392/new/ https://reviews.llvm.org/D77392 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D77392: [WIP][clangd] Make signatureHelp work with stale preambles
sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Keep missing the "ship it" button... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77392/new/ https://reviews.llvm.org/D77392 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D77392: [WIP][clangd] Make signatureHelp work with stale preambles
sammccall marked an inline comment as done.
sammccall added a comment.
Great stuff!
Comment at: clang-tools-extra/clangd/Preamble.cpp:281
+ // We are only interested in newly added includes.
+ llvm::StringSet<> ExistingIncludes;
+ for (const auto &Inc : Preamble.LexedIncludes)
kadircet wrote:
> sammccall wrote:
> > Why not a DenseSet>?
> > (The copies probably don't matter, but I think it'd be a bit clearer and
> > more typesafe)
> PPKeywordKind didn't have a DenseMapInfo, adding one. It already has two
> invalid enum values.
Ugh, I thought enums had those implicitly. The need for two invalid values is
really annoying, it's probably why we don't have more implicit ones.
I wonder whether it's feasible (technically and legally) to replace
DenseHashMap with a fork of absl::flat_hash_map. I'm pretty sure it's faster,
and it doesn't have these weird API requirements.
Comment at: clang-tools-extra/clangd/Preamble.cpp:128
+std::vector
+scanPreambleIncludes(llvm::StringRef Contents,
+ llvm::IntrusiveRefCntPtr VFS,
The error-handling paths here just return {}, same as an empty preamble.
Results:
if both baseline & current fail, then we'll generate no patch --> fine
if baseline is ok and current fails, we'll consider all headers, we'll consider
all headers removed and generate no new includes --> fine for now
(added-includes-only model)
if baseline fails and current is OK, we'll create a patch that adds all the
headers --> really slow, we'd be better off creating an empty patch
clarity and debuggability:
behaviors are implicit and silent. I think we should write out these three
cases explicitly in the code, and log them at least at `-log/verbose`. These
should be rare conditions I think. (If they're not, we should be able to track
down & handle the common causes I think)
I think this function should probably return Expected.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77392/new/
https://reviews.llvm.org/D77392
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D77392: [WIP][clangd] Make signatureHelp work with stale preambles
kadircet updated this revision to Diff 258833.
kadircet marked 2 inline comments as done.
kadircet added a comment.
- Address comments
- Don't store LexedInclude in PreambleData, expose the contents in
PrecompiledPreamble instead.
- Introduce DenseMapInfo for PPKeywordKind
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77392/new/
https://reviews.llvm.org/D77392
Files:
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/PreambleTests.cpp
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
clang/include/clang/Basic/TokenKinds.h
clang/include/clang/Frontend/PrecompiledPreamble.h
Index: clang/include/clang/Frontend/PrecompiledPreamble.h
===
--- clang/include/clang/Frontend/PrecompiledPreamble.h
+++ 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
@@ -94,6 +95,11 @@
/// 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,
Index: clang/include/clang/Basic/TokenKinds.h
===
--- clang/include/clang/Basic/TokenKinds.h
+++ 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 @@
/// 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 {
+ 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(Val);
+ }
+ static bool isEqual(const clang::tok::PPKeywordKind &LHS,
+ const clang::tok::PPKeywordKind &RHS) {
+return LHS == RHS;
+ }
+};
+} // namespace llvm
#endif
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -246,66 +246,6 @@
EXPECT_EQ(2, CallbackCount);
}
-static std::vector includes(const PreambleData *Preamble) {
- std::vector Result;
- if (Preamble)
-for (const auto &Inclusion : Preamble->Includes.MainFileIncludes)
- Result.push_back(Inclusion.Written);
- return Result;
-}
-
-TEST_F(TUSchedulerTests, PreambleConsistency) {
- std::atomic 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 ");
-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 ";
-Inputs.Version = "B";
-S.update(Path, Inputs, WantDiagnostics::Yes);
-
-S.runWithPreamble("StaleRead
[PATCH] D77392: [WIP][clangd] Make signatureHelp work with stale preambles
kadircet marked 15 inline comments as done.
kadircet added inline comments.
Comment at: clang-tools-extra/clangd/Preamble.cpp:278
+ // This shouldn't coincide with any real file name.
+ PP.PatchFileName = llvm::formatv("{0}_preamble_patch.h", FileName);
+
sammccall wrote:
> I'm slightly nervous about incorporating the filename itself, not sure why
> but it feels unneccesary.
> WDYT about "dir/__preamble_patch__.h"?
it was done to get rid of path manipulations, but not that important.
Comment at: clang-tools-extra/clangd/Preamble.cpp:281
+ // We are only interested in newly added includes.
+ llvm::StringSet<> ExistingIncludes;
+ for (const auto &Inc : Preamble.LexedIncludes)
sammccall wrote:
> Why not a DenseSet>?
> (The copies probably don't matter, but I think it'd be a bit clearer and more
> typesafe)
PPKeywordKind didn't have a DenseMapInfo, adding one. It already has two
invalid enum values.
Comment at: clang-tools-extra/clangd/Preamble.h:74
+ /// ones in disabled regions.
+ std::vector LexedIncludes;
};
sammccall wrote:
> Since computing these inclusions seems to be cheap (you've disabled all the
> IO), would it be clearer and more flexible to store the preamble text as a
> string and compute both the baseline & new includes at the same time?
> I'm thinking if other preamble constructs (macros?) are added, needing to put
> more data structures in PreambleData, where if you store the text only those
> can be private details.
>
> (In fact the preamble text is already stored in PrecompiledPreamble, you
> could just expose it there)
>
>
since a preamble can be used with multiple preamble patches i wanted to reduce
lexing overhead. I suppose we can address that later if it shows up in the
traces. exposing the contents in PrecompiledPreamble.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77392/new/
https://reviews.llvm.org/D77392
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D77392: [WIP][clangd] Make signatureHelp work with stale preambles
sammccall added a comment.
This looks pretty good! Haven't reviewed the tests or removal of consistent
preamble support yet. (Mostly note-to-self)
Comment at: clang-tools-extra/clangd/Preamble.cpp:115
+do {
+ PP.Lex(Tok);
+} while (Tok.isNot(tok::eof) &&
Given your getDecomposedTok before, you might want to assert Tok is in the main
file inside the loop
Comment at: clang-tools-extra/clangd/Preamble.cpp:126
+std::vector
+getPreambleIncludes(llvm::StringRef Contents,
+llvm::IntrusiveRefCntPtr VFS,
I'd call this scanPreambleIncludes or something just slightly less generic -
again here we don't want it to be accidentally reused for something that needs
to be accurate.
Comment at: clang-tools-extra/clangd/Preamble.cpp:148
+ PreambleOnlyAction Action;
+ if (!Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0]))
+return {};
Something needs to check that there's one input etc.
(This could maybe be moved into buildCompilerInvocation I guess, but currently
the caller does it)
Comment at: clang-tools-extra/clangd/Preamble.cpp:244
std::vector Diags = PreambleDiagnostics.take();
+auto AllIncludes = getPreambleIncludes(Inputs.Contents,
+
StatCache->getConsumingFS(Inputs.FS),
I'd stick to LexedIncludes or ApproxmimateIncludes or BaselineIncludes - it
excludes stuff under ifdefs that *can* be resolved, and we don't want it to be
used for other purposes...
Comment at: clang-tools-extra/clangd/Preamble.cpp:278
+ // This shouldn't coincide with any real file name.
+ PP.PatchFileName = llvm::formatv("{0}_preamble_patch.h", FileName);
+
I'm slightly nervous about incorporating the filename itself, not sure why but
it feels unneccesary.
WDYT about "dir/__preamble_patch__.h"?
Comment at: clang-tools-extra/clangd/Preamble.cpp:281
+ // We are only interested in newly added includes.
+ llvm::StringSet<> ExistingIncludes;
+ for (const auto &Inc : Preamble.LexedIncludes)
Why not a DenseSet>?
(The copies probably don't matter, but I think it'd be a bit clearer and more
typesafe)
Comment at: clang-tools-extra/clangd/Preamble.cpp:313
+ PPOpts.addRemappedFile(PatchFileName, PatchBuffer.release());
+ PPOpts.Includes.push_back(PatchFileName);
+}
worth a comment indicating exactly where this is going to be inserted into the
parsing stream.
Comment at: clang-tools-extra/clangd/Preamble.h:72
CanonicalIncludes CanonIncludes;
+ /// Contains all of the includes spelled in the preamble section, even the
+ /// ones in disabled regions.
Maybe "Used as a baseline for creating a PreamblePatch"?
Since this is a bit unusual.
Comment at: clang-tools-extra/clangd/Preamble.h:74
+ /// ones in disabled regions.
+ std::vector LexedIncludes;
};
Since computing these inclusions seems to be cheap (you've disabled all the
IO), would it be clearer and more flexible to store the preamble text as a
string and compute both the baseline & new includes at the same time?
I'm thinking if other preamble constructs (macros?) are added, needing to put
more data structures in PreambleData, where if you store the text only those
can be private details.
(In fact the preamble text is already stored in PrecompiledPreamble, you could
just expose it there)
Comment at: clang-tools-extra/clangd/Preamble.h:98
+/// Stores information required to use an existing preamble with different file
+/// contents.
+class PreamblePatch {
This is probably a good place for a high-level overview of the concept, and
maybe an example.
In particular, *something* should mention that this is approximate, and what we
use it for/don't use it for.
Comment at: clang-tools-extra/clangd/Preamble.h:101
+public:
+ PreamblePatch() = default;
+ /// Builds a patch that contains new PP directives introduced to the preamble
// With an empty patch, the preamble is used verbatim.
Comment at: clang-tools-extra/clangd/Preamble.h:107
+ static PreamblePatch create(llvm::StringRef FileName, const ParseInputs &PI,
+ const PreambleData &Preamble);
+ /// Inserts an artifical include to the \p CI that contains new directives
nit: it'd be good to give these "version"s names. e.g. PI -> Modified, Preamble
-> Baseline. And maybe mention these in the class description?
Comment at: clang-tools-extra/clangd/Preamble.h:108
+ const PreambleData &Preamble);
+ /// Inserts an artifical include to the \p CI that contains new directives
+ /// calculated with create.
This exp
[PATCH] D77392: [WIP][clangd] Make signatureHelp work with stale preambles
kadircet added a comment. This is ready for another round. To summarize the current state: - `PreamblePatch` can be create from a `ParseInput` and a `PreambleData`, - It will preprocessor preamble section of current file contents using a dummy FS to reduce cost of stat/realpaths - It won't populate `Resolved` path for inclusions, as we don't need them at this stage, and even later on. As we'll run the real preprocessor when building the ParsedAST, and we can collect resolved paths at that stage instead. - Afterwards it will diff the includes found in current contents with includes found in the PreambleData, and create a patch containing all the new includes - `PreamblePatch` can be applied to a `CompilerInvocation`: - It is a no-op if contents are empty, i.e it was default constructed or there were no new includes. - Otherwise it will create an implicit include which contains patch created with new includes. Things that I don't like about current state: - We'll run processor to collect includes even when the preamble section of current file hasn't changed. - `PrecompiledPreamble` contains the contents of the preamble but it is not exposed, I suppose we can either expose that to get rid of extra preprocessor creation overhead. - We create an extra `CompilerInvocation` and `CompilerInstance` just to run preprocessor and throw them away later on. I am not sure what to do about this :/ Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77392/new/ https://reviews.llvm.org/D77392 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D77392: [WIP][clangd] Make signatureHelp work with stale preambles
kadircet updated this revision to Diff 258064.
kadircet added a comment.
- Use preprocessor instead of raw lexer
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77392/new/
https://reviews.llvm.org/D77392
Files:
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/PreambleTests.cpp
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -246,66 +246,6 @@
EXPECT_EQ(2, CallbackCount);
}
-static std::vector includes(const PreambleData *Preamble) {
- std::vector Result;
- if (Preamble)
-for (const auto &Inclusion : Preamble->Includes.MainFileIncludes)
- Result.push_back(Inclusion.Written);
- return Result;
-}
-
-TEST_F(TUSchedulerTests, PreambleConsistency) {
- std::atomic 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 ");
-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 ";
-Inputs.Version = "B";
-S.update(Path, Inputs, WantDiagnostics::Yes);
-
-S.runWithPreamble("StaleRead", Path, TUScheduler::Stale,
- [&](Expected Pre) {
-ASSERT_TRUE(bool(Pre));
-ASSERT_TRUE(Pre->Preamble);
-EXPECT_EQ(Pre->Preamble->Version, "A");
-EXPECT_THAT(includes(Pre->Preamble),
-ElementsAre(""));
-InconsistentReadDone.notify();
-++CallbackCount;
- });
-S.runWithPreamble("ConsistentRead", Path, TUScheduler::Consistent,
- [&](Expected Pre) {
-ASSERT_TRUE(bool(Pre));
-ASSERT_TRUE(Pre->Preamble);
-EXPECT_EQ(Pre->Preamble->Version, "B");
-EXPECT_THAT(includes(Pre->Preamble),
-ElementsAre(""));
-++CallbackCount;
- });
-S.blockUntilIdle(timeoutSeconds(10));
- }
- EXPECT_EQ(2, CallbackCount);
-}
-
TEST_F(TUSchedulerTests, Cancellation) {
// We have the following update/read sequence
// U0
Index: clang-tools-extra/clangd/unittests/PreambleTests.cpp
===
--- /dev/null
+++ 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
+#include
+
+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.getFileSyst
[PATCH] D77392: [WIP][clangd] Make signatureHelp work with stale preambles
kadircet marked 3 inline comments as done.
kadircet added inline comments.
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1076
+ if (PatchAdditionalIncludes) {
+for (const auto &Inc : newIncludes(
+ Input.Preamble.LexedIncludes,
sammccall wrote:
> This is going to be used in other places where we parse too, we'll want some
> abstraction to encapsulate the analysis, the tweaks to CompilerInvocation,
> and (for other uses) mapping of locations from the preamble file and injected
> regions.
>
> As discussed offline, I think injecting by synthesizing a header snippet
> which is mapped into the VFS and then added to PreprocessorOpts.Includes is
> preferable to tweaking the CompilerInvocation directly:
> - it extends to arbitrary directives, e.g. there's no way today to inject an
> `#import` directive via CompilerInvocation, no control over `<>` vs `""`, ...
> - it's easier to debug as the injected region can easily be dumped as text
> - it's easier to for code to recognize locations in our injected region if
> we're not sharing it with e.g. `-D`s coming from the commandline
> - (I think) it lets us use the PresumedLocation mechanism to record the
> association of each injected line with the corresponding line in the mainfile
> (not sure if this is valuable if we also need to translate the other way)
>
> So I'd probably suggest something like
>
> ```
> class PreamblePatch { // ends up stored in ParsedAST
> public:
> PreamblePatch(); // empty, used when no patching is done
> static PreamblePatch compute(const ParseInputs&);
>
> void apply(CompilerInvocation&); // adjusts PPOptions.{RemappedFileBuffers,
> Includes} to include injected header
>
> friend operator<<(raw_ostream&, const PreamblePatch &); // probably just
> dumps InjectedHeaderContent
>
> // later... not sure about signature/names here
> SourceLocation parsedToUserLoc(SourceLocation);
> SourceLocation userToParsedLoc(SourceLocation); // is this also needed?
>
> private:
> std::string InjectedHeaderContent; // with #line directives, or at least
> comments
> // some data structure to map between locations in the stale preamble and
> their user locations
> // some data structure to map between locations in the injected header and
> their user locations
> };
> ```
implemented an initial version of this, no sourcelocation handling yet.
compute also takes the preamble we are patching though.
Comment at: clang-tools-extra/clangd/Headers.cpp:237
+std::vector getPreambleIncludes(llvm::StringRef Contents,
+ const LangOptions &LangOpts) {
sammccall wrote:
> The tradeoff between raw-lexing and running the actual preprocessor in
> SingleFileParseMode seems pretty interesting.
> (I'm not suggesting any particular change here, but I think it's worth
> exploring at some point. If the raw lexing is "for now" let's call that out,
> if it's a decision let's justify it. Also just curious what you think at this
> point - I know we've discussed this in the past and I pushed for raw-lexing)
>
> ---
>
> The SingleFileParseMode is more accurate:
> - it's probably much easier to handle more directives correctly
> - it will handle ifdefs correctly where possible without reading files
> - it will tell us what includes actually resolve to (I'm not sure if this is
> useful though, when you can inject an #include with the right spelling and
> check that later)
> - ??? maybe we can recognize bad directives that we want to avoid injecting
> for better results ???
> - we don't have to do weird stuff like track the raw-lexed includes in
> preambledata
>
> But potentially expensive with IO:
> - it will stat all the files along the search path.
> - it will ::realpath all the files that were found
>
> Possible workaround for stat along the search path: most of the time, the
> #include directives are going to refer to files that were already included in
> the previous preamble (i.e. the preamble we're trying to patch) so we
> (heuristically) know what they resolve to.
> If we used the HeaderSearchInfo's alias map to redirect to
> then `#include ` will just cost a single
> stat. And when our heuristic is wrong and resolves to something else,
> we'll just get an include-not-found, we can still add the #include.
>
> More radical workaround: give it a completely null VFS so it can't do any IO.
> All includes will fail to resolve, but if we don't need their resolved path
> at this point we don't care, right?
the choice of raw lexer was more of a shortcut. i was just trying to get an
initial version, tried to put some ideas in comments.
Will experiment with couple of options, but using alias mapping to map written
includes to resolved absolute paths and also using the statcached vfs sounds
like the best thing we can do for now.
We should also put this behind a "raw string equal
[PATCH] D77392: [WIP][clangd] Make signatureHelp work with stale preambles
kadircet updated this revision to Diff 256359.
kadircet added a comment.
Herald added a subscriber: mgorny.
- Encapsulate logic into `PreamblePatch`
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77392/new/
https://reviews.llvm.org/D77392
Files:
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/PreambleTests.cpp
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -245,66 +245,6 @@
EXPECT_EQ(2, CallbackCount);
}
-static std::vector includes(const PreambleData *Preamble) {
- std::vector Result;
- if (Preamble)
-for (const auto &Inclusion : Preamble->Includes.MainFileIncludes)
- Result.push_back(Inclusion.Written);
- return Result;
-}
-
-TEST_F(TUSchedulerTests, PreambleConsistency) {
- std::atomic 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 ");
-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 ";
-Inputs.Version = "B";
-S.update(Path, Inputs, WantDiagnostics::Yes);
-
-S.runWithPreamble("StaleRead", Path, TUScheduler::Stale,
- [&](Expected Pre) {
-ASSERT_TRUE(bool(Pre));
-ASSERT_TRUE(Pre->Preamble);
-EXPECT_EQ(Pre->Preamble->Version, "A");
-EXPECT_THAT(includes(Pre->Preamble),
-ElementsAre(""));
-InconsistentReadDone.notify();
-++CallbackCount;
- });
-S.runWithPreamble("ConsistentRead", Path, TUScheduler::Consistent,
- [&](Expected Pre) {
-ASSERT_TRUE(bool(Pre));
-ASSERT_TRUE(Pre->Preamble);
-EXPECT_EQ(Pre->Preamble->Version, "B");
-EXPECT_THAT(includes(Pre->Preamble),
-ElementsAre(""));
-++CallbackCount;
- });
-S.blockUntilIdle(timeoutSeconds(10));
- }
- EXPECT_EQ(2, CallbackCount);
-}
-
TEST_F(TUSchedulerTests, Cancellation) {
// We have the following update/read sequence
// U0
Index: clang-tools-extra/clangd/unittests/PreambleTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/PreambleTests.cpp
@@ -0,0 +1,127 @@
+//===--- 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
+#include
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using testing::_;
+using testing::Contains;
+using testing::Pair;
+
+MATCHER_P(HasContents, Contents, "") { return arg->getBuffer() == Contents; }
+
+TEST(PreamblePatchTest, All) {
+ MockFSProvider FS;
+ MockCompilationDatabase CDB;
+ IgnoreDiagnostics Diags;
+ ParseInputs PI;
[PATCH] D77392: [WIP][clangd] Make signatureHelp work with stale preambles
sammccall added a comment.
This is the perfect feature to start with (doesn't actually need any location
transforms, lets us drop TUScheduler features), so well done for finding that.
That said, high-level comments mostly about preamble patching in general rather
than this particular case.
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1076
+ if (PatchAdditionalIncludes) {
+for (const auto &Inc : newIncludes(
+ Input.Preamble.LexedIncludes,
This is going to be used in other places where we parse too, we'll want some
abstraction to encapsulate the analysis, the tweaks to CompilerInvocation, and
(for other uses) mapping of locations from the preamble file and injected
regions.
As discussed offline, I think injecting by synthesizing a header snippet which
is mapped into the VFS and then added to PreprocessorOpts.Includes is
preferable to tweaking the CompilerInvocation directly:
- it extends to arbitrary directives, e.g. there's no way today to inject an
`#import` directive via CompilerInvocation, no control over `<>` vs `""`, ...
- it's easier to debug as the injected region can easily be dumped as text
- it's easier to for code to recognize locations in our injected region if
we're not sharing it with e.g. `-D`s coming from the commandline
- (I think) it lets us use the PresumedLocation mechanism to record the
association of each injected line with the corresponding line in the mainfile
(not sure if this is valuable if we also need to translate the other way)
So I'd probably suggest something like
```
class PreamblePatch { // ends up stored in ParsedAST
public:
PreamblePatch(); // empty, used when no patching is done
static PreamblePatch compute(const ParseInputs&);
void apply(CompilerInvocation&); // adjusts PPOptions.{RemappedFileBuffers,
Includes} to include injected header
friend operator<<(raw_ostream&, const PreamblePatch &); // probably just
dumps InjectedHeaderContent
// later... not sure about signature/names here
SourceLocation parsedToUserLoc(SourceLocation);
SourceLocation userToParsedLoc(SourceLocation); // is this also needed?
private:
std::string InjectedHeaderContent; // with #line directives, or at least
comments
// some data structure to map between locations in the stale preamble and
their user locations
// some data structure to map between locations in the injected header and
their user locations
};
```
Comment at: clang-tools-extra/clangd/Headers.cpp:237
+std::vector getPreambleIncludes(llvm::StringRef Contents,
+ const LangOptions &LangOpts) {
The tradeoff between raw-lexing and running the actual preprocessor in
SingleFileParseMode seems pretty interesting.
(I'm not suggesting any particular change here, but I think it's worth
exploring at some point. If the raw lexing is "for now" let's call that out, if
it's a decision let's justify it. Also just curious what you think at this
point - I know we've discussed this in the past and I pushed for raw-lexing)
---
The SingleFileParseMode is more accurate:
- it's probably much easier to handle more directives correctly
- it will handle ifdefs correctly where possible without reading files
- it will tell us what includes actually resolve to (I'm not sure if this is
useful though, when you can inject an #include with the right spelling and
check that later)
- ??? maybe we can recognize bad directives that we want to avoid injecting
for better results ???
- we don't have to do weird stuff like track the raw-lexed includes in
preambledata
But potentially expensive with IO:
- it will stat all the files along the search path.
- it will ::realpath all the files that were found
Possible workaround for stat along the search path: most of the time, the
#include directives are going to refer to files that were already included in
the previous preamble (i.e. the preamble we're trying to patch) so we
(heuristically) know what they resolve to.
If we used the HeaderSearchInfo's alias map to redirect to
then `#include ` will just cost a single stat.
And when our heuristic is wrong and resolves to something else, we'll
just get an include-not-found, we can still add the #include.
More radical workaround: give it a completely null VFS so it can't do any IO.
All includes will fail to resolve, but if we don't need their resolved path at
this point we don't care, right?
Comment at: clang-tools-extra/clangd/Headers.h:198
+/// Gets the includes in the preamble section of the file by running lexer over
+/// \p Contents. Returned inclusions are sorted according to written filename.
I'm guessing the APIs we end up with probably belong more in Preamble.h rather
than here, both thematically and in terms of layering
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D773
