kadircet created this revision.
kadircet added a reviewer: sammccall.
Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay,
ilya-biryukov.
Herald added a project: clang.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D81964
Files:
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
Index: clang-tools-extra/clangd/unittests/PreambleTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/PreambleTests.cpp
+++ clang-tools-extra/clangd/unittests/PreambleTests.cpp
@@ -488,6 +488,51 @@
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.preambleBounds().Size, ExpectedBounds.Size);
+ EXPECT_EQ(PP.preambleBounds().PreambleEndsAtStartOfLine,
+ ExpectedBounds.PreambleEndsAtStartOfLine);
+ }
+}
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
+++ clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
@@ -444,24 +444,76 @@
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) {
Index: clang-tools-extra/clangd/Preamble.h
===================================================================
--- clang-tools-extra/clangd/Preamble.h
+++ 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 @@
/// using the presumed-location mechanism.
std::vector<Inclusion> preambleIncludes() const;
+ /// Returns preamble bounds for the current version of the file.
+ PreambleBounds preambleBounds() const { return PatchBounds; }
+
/// Returns textual patch contents.
llvm::StringRef text() const { return PatchContents; }
@@ -129,6 +133,7 @@
/// Includes that are present in both \p Baseline and \p Modified. Used for
/// patching includes of baseline preamble.
std::vector<Inclusion> PreambleIncludes;
+ PreambleBounds PatchBounds = {0, false};
};
/// Translates locations inside preamble patch to their main-file equivalent
Index: clang-tools-extra/clangd/Preamble.cpp
===================================================================
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -217,6 +217,7 @@
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 @@
IncludeStructure Includes;
PP.addPPCallbacks(collectIncludeStructureCallback(SM, &Includes));
ScannedPreamble SP;
+ SP.Bounds = std::move(Bounds);
PP.addPPCallbacks(
std::make_unique<DirectiveCollector>(PP, SP.TextualDirectives));
if (llvm::Error Err = Action.Execute())
@@ -463,6 +465,7 @@
llvm::sys::path::append(PatchName, llvm::sys::path::parent_path(FileName),
PreamblePatchHeaderName);
PP.PatchFileName = PatchName.str().str();
+ PP.PatchBounds = std::move(ModifiedScan->Bounds);
llvm::raw_string_ostream Patch(PP.PatchContents);
// Set default filename for subsequent #line directives
@@ -548,6 +551,7 @@
PreamblePatch PreamblePatch::unmodified(const PreambleData &Preamble) {
PreamblePatch PP;
PP.PreambleIncludes = Preamble.Includes.MainFileIncludes;
+ PP.PatchBounds = Preamble.Preamble.getBounds();
return PP;
}
Index: clang-tools-extra/clangd/ParsedAST.cpp
===================================================================
--- clang-tools-extra/clangd/ParsedAST.cpp
+++ clang-tools-extra/clangd/ParsedAST.cpp
@@ -383,7 +383,7 @@
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->preambleBounds());
}
// Important: collectIncludeStructure is registered *after* ReplayPreamble!
// Otherwise we would collect the replayed includes again...
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
