[clang-tools-extra] 4317ee2 - [clangd] Make use of preamble bounds from the patch inside ReplayPreamble
Kadir Cetinkaya via cfe-commits
cfe-commits at lists.llvm.org
Wed Jun 17 09:34:26 PDT 2020
Author: Kadir Cetinkaya
Date: 2020-06-17T18:32:59+02:00
New Revision: 4317ee27bd64759b922438659f5b6883c0fbe404
URL: https://github.com/llvm/llvm-project/commit/4317ee27bd64759b922438659f5b6883c0fbe404
DIFF: https://github.com/llvm/llvm-project/commit/4317ee27bd64759b922438659f5b6883c0fbe404.diff
LOG: [clangd] Make use of preamble bounds from the patch inside ReplayPreamble
Summary:
Clangd was using bounds from the stale preamble, which might result in
crashes. For example:
```
#include "a.h"
#include "b.h" // this line is newly inserted
#include "c.h"
```
PreambleBounds for the baseline only contains first two lines, but
ReplayPreamble logic contains an include from the third line. This would
result in a crash as we only lex preamble part of the current file
during ReplayPreamble.
This patch adds a `preambleBounds` method to PreamblePatch, which can be
used to figure out preamble bounds for the current version of the file.
Then uses it when attaching ReplayPreamble, so that it can lex the
up-to-date preamble region.
Reviewers: sammccall
Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D81964
Added:
Modified:
clang-tools-extra/clangd/ParsedAST.cpp
clang-tools-extra/clangd/Preamble.cpp
clang-tools-extra/clangd/Preamble.h
clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
clang-tools-extra/clangd/unittests/PreambleTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp
index e85f1b30cd3a..f622ab298196 100644
--- a/clang-tools-extra/clangd/ParsedAST.cpp
+++ b/clang-tools-extra/clangd/ParsedAST.cpp
@@ -383,7 +383,7 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
Includes.MainFileIncludes = Patch->preambleIncludes();
// Replay the preamble includes so that clang-tidy checks can see them.
ReplayPreamble::attach(Patch->preambleIncludes(), *Clang,
- Preamble->Preamble.getBounds());
+ Patch->modifiedBounds());
}
// Important: collectIncludeStructure is registered *after* ReplayPreamble!
// Otherwise we would collect the replayed includes again...
diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp
index c32dc8c66b2f..d7a15ff0a101 100644
--- a/clang-tools-extra/clangd/Preamble.cpp
+++ b/clang-tools-extra/clangd/Preamble.cpp
@@ -217,6 +217,7 @@ struct DirectiveCollector : public PPCallbacks {
struct ScannedPreamble {
std::vector<Inclusion> Includes;
std::vector<TextualPPDirective> TextualDirectives;
+ PreambleBounds Bounds = {0, false};
};
/// Scans the preprocessor directives in the preamble section of the file by
@@ -284,6 +285,7 @@ scanPreamble(llvm::StringRef Contents,
IncludeStructure Includes;
PP.addPPCallbacks(collectIncludeStructureCallback(SM, &Includes));
ScannedPreamble SP;
+ SP.Bounds = Bounds;
PP.addPPCallbacks(
std::make_unique<DirectiveCollector>(PP, SP.TextualDirectives));
if (llvm::Error Err = Action.Execute())
@@ -463,6 +465,7 @@ PreamblePatch PreamblePatch::create(llvm::StringRef FileName,
llvm::sys::path::append(PatchName, llvm::sys::path::parent_path(FileName),
PreamblePatchHeaderName);
PP.PatchFileName = PatchName.str().str();
+ PP.ModifiedBounds = ModifiedScan->Bounds;
llvm::raw_string_ostream Patch(PP.PatchContents);
// Set default filename for subsequent #line directives
@@ -548,6 +551,7 @@ std::vector<Inclusion> PreamblePatch::preambleIncludes() const {
PreamblePatch PreamblePatch::unmodified(const PreambleData &Preamble) {
PreamblePatch PP;
PP.PreambleIncludes = Preamble.Includes.MainFileIncludes;
+ PP.ModifiedBounds = Preamble.Preamble.getBounds();
return PP;
}
diff --git a/clang-tools-extra/clangd/Preamble.h b/clang-tools-extra/clangd/Preamble.h
index 35c168573b90..1de1d6cfe5fa 100644
--- a/clang-tools-extra/clangd/Preamble.h
+++ b/clang-tools-extra/clangd/Preamble.h
@@ -31,6 +31,7 @@
#include "support/Path.h"
#include "clang/Frontend/CompilerInvocation.h"
#include "clang/Frontend/PrecompiledPreamble.h"
+#include "clang/Lex/Lexer.h"
#include "clang/Tooling/CompilationDatabase.h"
#include "llvm/ADT/StringRef.h"
@@ -119,6 +120,9 @@ class PreamblePatch {
/// using the presumed-location mechanism.
std::vector<Inclusion> preambleIncludes() const;
+ /// Returns preamble bounds for the Modified.
+ PreambleBounds modifiedBounds() const { return ModifiedBounds; }
+
/// Returns textual patch contents.
llvm::StringRef text() const { return PatchContents; }
@@ -129,6 +133,7 @@ class PreamblePatch {
/// Includes that are present in both \p Baseline and \p Modified. Used for
/// patching includes of baseline preamble.
std::vector<Inclusion> PreambleIncludes;
+ PreambleBounds ModifiedBounds = {0, false};
};
/// Translates locations inside preamble patch to their main-file equivalent
diff --git a/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp b/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
index 810bb28a7d13..05396739addd 100644
--- a/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
@@ -444,24 +444,76 @@ TEST(ParsedASTTest, ReplayPreambleForTidyCheckers) {
EXPECT_EQ(SkippedFiles[I].kind(), tok::header_name);
}
+ TU.AdditionalFiles["a.h"] = "";
+ TU.AdditionalFiles["b.h"] = "";
+ TU.AdditionalFiles["c.h"] = "";
// Make sure replay logic works with patched preambles.
- TU.Code = "";
- StoreDiags Diags;
+ llvm::StringLiteral Baseline = R"cpp(
+ #include "a.h"
+ #include "c.h")cpp";
MockFSProvider FS;
+ TU.Code = Baseline.str();
auto Inputs = TU.inputs(FS);
- auto CI = buildCompilerInvocation(Inputs, Diags);
- auto EmptyPreamble =
- buildPreamble(testPath(TU.Filename), *CI, Inputs, true, nullptr);
- ASSERT_TRUE(EmptyPreamble);
- TU.Code = "#include <a.h>";
+ auto BaselinePreamble = TU.preamble();
+ ASSERT_TRUE(BaselinePreamble);
+
+ // First make sure we don't crash on various modifications to the preamble.
+ llvm::StringLiteral Cases[] = {
+ // clang-format off
+ // New include in middle.
+ R"cpp(
+ #include "a.h"
+ #include "b.h"
+ #include "c.h")cpp",
+ // New include at top.
+ R"cpp(
+ #include "b.h"
+ #include "a.h"
+ #include "c.h")cpp",
+ // New include at bottom.
+ R"cpp(
+ #include "a.h"
+ #include "c.h"
+ #include "b.h")cpp",
+ // Same size with a missing include.
+ R"cpp(
+ #include "a.h"
+ #include "b.h")cpp",
+ // Smaller with no new includes.
+ R"cpp(
+ #include "a.h")cpp",
+ // Smaller with a new includes.
+ R"cpp(
+ #include "b.h")cpp",
+ // clang-format on
+ };
+ for (llvm::StringLiteral Case : Cases) {
+ TU.Code = Case.str();
+
+ IgnoreDiagnostics Diags;
+ auto CI = buildCompilerInvocation(TU.inputs(FS), Diags);
+ auto PatchedAST = ParsedAST::build(testPath(TU.Filename), TU.inputs(FS),
+ std::move(CI), {}, BaselinePreamble);
+ ASSERT_TRUE(PatchedAST);
+ EXPECT_TRUE(PatchedAST->getDiagnostics().empty());
+ }
+
+ // Then ensure correctness by making sure includes were seen only once.
+ // Note that we first see the includes from the patch, as preamble includes
+ // are replayed after exiting the built-in file.
Includes.clear();
+ TU.Code = R"cpp(
+ #include "a.h"
+ #include "b.h")cpp";
+ IgnoreDiagnostics Diags;
+ auto CI = buildCompilerInvocation(TU.inputs(FS), Diags);
auto PatchedAST = ParsedAST::build(testPath(TU.Filename), TU.inputs(FS),
- std::move(CI), {}, EmptyPreamble);
+ std::move(CI), {}, BaselinePreamble);
ASSERT_TRUE(PatchedAST);
- // Make sure includes were seen only once.
+ EXPECT_TRUE(PatchedAST->getDiagnostics().empty());
EXPECT_THAT(Includes,
ElementsAre(WithFileName(testPath("__preamble_patch__.h")),
- WithFileName("a.h")));
+ WithFileName("b.h"), WithFileName("a.h")));
}
TEST(ParsedASTTest, PatchesAdditionalIncludes) {
diff --git a/clang-tools-extra/clangd/unittests/PreambleTests.cpp b/clang-tools-extra/clangd/unittests/PreambleTests.cpp
index 8d00fa0b2ee7..0b662d705b10 100644
--- a/clang-tools-extra/clangd/unittests/PreambleTests.cpp
+++ b/clang-tools-extra/clangd/unittests/PreambleTests.cpp
@@ -488,6 +488,51 @@ TEST(TranslatePreamblePatchLocation, Simple) {
EXPECT_EQ(DecompLoc.first, SM.getMainFileID());
EXPECT_EQ(SM.getLineNumber(DecompLoc.first, DecompLoc.second), 3U);
}
+
+TEST(PreamblePatch, ModifiedBounds) {
+ struct {
+ llvm::StringLiteral Baseline;
+ llvm::StringLiteral Modified;
+ } Cases[] = {
+ // Size increased
+ {
+ "",
+ R"cpp(
+ #define FOO
+ FOO)cpp",
+ },
+ // Stayed same
+ {"#define FOO", "#define BAR"},
+ // Got smaller
+ {
+ R"cpp(
+ #define FOO
+ #undef FOO)cpp",
+ "#define FOO"},
+ };
+
+ for (const auto &Case : Cases) {
+ auto TU = TestTU::withCode(Case.Baseline);
+ auto BaselinePreamble = TU.preamble();
+ ASSERT_TRUE(BaselinePreamble);
+
+ Annotations Modified(Case.Modified);
+ TU.Code = Modified.code().str();
+ MockFSProvider FSProvider;
+ auto PP = PreamblePatch::create(testPath(TU.Filename),
+ TU.inputs(FSProvider), *BaselinePreamble);
+
+ IgnoreDiagnostics Diags;
+ auto CI = buildCompilerInvocation(TU.inputs(FSProvider), Diags);
+ ASSERT_TRUE(CI);
+
+ const auto ExpectedBounds =
+ Lexer::ComputePreamble(Case.Modified, *CI->getLangOpts());
+ EXPECT_EQ(PP.modifiedBounds().Size, ExpectedBounds.Size);
+ EXPECT_EQ(PP.modifiedBounds().PreambleEndsAtStartOfLine,
+ ExpectedBounds.PreambleEndsAtStartOfLine);
+ }
+}
} // namespace
} // namespace clangd
} // namespace clang
More information about the cfe-commits
mailing list