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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
