[clang-tools-extra] 9c4a168 - [clangd] Fix clangd crash when including a header
Kadir Cetinkaya via cfe-commits
cfe-commits at lists.llvm.org
Thu Sep 16 01:27:46 PDT 2021
Author: Queen Dela Cruz
Date: 2021-09-16T10:27:15+02:00
New Revision: 9c4a1686d7c487fff8e63fa67e64623eea8986d5
URL: https://github.com/llvm/llvm-project/commit/9c4a1686d7c487fff8e63fa67e64623eea8986d5
DIFF: https://github.com/llvm/llvm-project/commit/9c4a1686d7c487fff8e63fa67e64623eea8986d5.diff
LOG: [clangd] Fix clangd crash when including a header
Fixes https://github.com/clangd/clangd/issues/819
SourceLocation of macros change when a header file is included above it. This is not checked when creating a PreamblePatch, resulting in reusing previously built preamble with an incorrect source location for the macro in the example test case.
This patch stores the SourceLocation in the struct TextualPPDirective so that it gets checked when comparing old vs new preambles.
Also creates a preamble patch for code completion parsing so that clangd does not crash when following the example test case with a large file.
Reviewed By: kadircet
Differential Revision: https://reviews.llvm.org/D108045
Added:
Modified:
clang-tools-extra/clangd/CodeComplete.cpp
clang-tools-extra/clangd/ParsedAST.cpp
clang-tools-extra/clangd/Preamble.cpp
clang-tools-extra/clangd/Preamble.h
clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
clang-tools-extra/clangd/unittests/PreambleTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/CodeComplete.cpp b/clang-tools-extra/clangd/CodeComplete.cpp
index 54d0e69c4cf45..69338814ebdd2 100644
--- a/clang-tools-extra/clangd/CodeComplete.cpp
+++ b/clang-tools-extra/clangd/CodeComplete.cpp
@@ -1874,9 +1874,10 @@ CodeCompleteResult codeComplete(PathRef FileName, Position Pos,
? std::move(Flow).runWithoutSema(ParseInput.Contents, *Offset,
*ParseInput.TFS)
: std::move(Flow).run({FileName, *Offset, *Preamble,
- // We want to serve code completions with
- // low latency, so don't bother patching.
- /*PreamblePatch=*/llvm::None, ParseInput});
+ /*PreamblePatch=*/
+ PreamblePatch::createMacroPatch(
+ FileName, ParseInput, *Preamble),
+ ParseInput});
}
SignatureHelp signatureHelp(PathRef FileName, Position Pos,
@@ -1898,7 +1899,8 @@ SignatureHelp signatureHelp(PathRef FileName, Position Pos,
Result),
Options,
{FileName, *Offset, Preamble,
- PreamblePatch::create(FileName, ParseInput, Preamble), ParseInput});
+ PreamblePatch::createFullPatch(FileName, ParseInput, Preamble),
+ ParseInput});
return Result;
}
diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp
index e3fd08afdaa7e..d8351199d100f 100644
--- a/clang-tools-extra/clangd/ParsedAST.cpp
+++ b/clang-tools-extra/clangd/ParsedAST.cpp
@@ -284,7 +284,7 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
llvm::Optional<PreamblePatch> Patch;
bool PreserveDiags = true;
if (Preamble) {
- Patch = PreamblePatch::create(Filename, Inputs, *Preamble);
+ Patch = PreamblePatch::createFullPatch(Filename, Inputs, *Preamble);
Patch->apply(*CI);
PreserveDiags = Patch->preserveDiagnostics();
}
diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp
index b41bd915ea22b..72a53b2fdac57 100644
--- a/clang-tools-extra/clangd/Preamble.cpp
+++ b/clang-tools-extra/clangd/Preamble.cpp
@@ -142,10 +142,11 @@ struct TextualPPDirective {
unsigned DirectiveLine;
// Full text that's representing the directive, including the `#`.
std::string Text;
+ unsigned Offset;
bool operator==(const TextualPPDirective &RHS) const {
- return std::tie(DirectiveLine, Text) ==
- std::tie(RHS.DirectiveLine, RHS.Text);
+ return std::tie(DirectiveLine, Offset, Text) ==
+ std::tie(RHS.DirectiveLine, RHS.Offset, RHS.Text);
}
};
@@ -155,7 +156,7 @@ struct TextualPPDirective {
std::string spellDirective(llvm::StringRef Prefix,
CharSourceRange DirectiveRange,
const LangOptions &LangOpts, const SourceManager &SM,
- unsigned &DirectiveLine) {
+ unsigned &DirectiveLine, unsigned &Offset) {
std::string SpelledDirective;
llvm::raw_string_ostream OS(SpelledDirective);
OS << Prefix;
@@ -169,6 +170,7 @@ std::string spellDirective(llvm::StringRef Prefix,
auto DecompLoc = SM.getDecomposedLoc(DirectiveRange.getBegin());
DirectiveLine = SM.getLineNumber(DecompLoc.first, DecompLoc.second);
+ Offset = DecompLoc.second;
auto TargetColumn = SM.getColumnNumber(DecompLoc.first, DecompLoc.second) - 1;
// Pad with spaces before DirectiveRange to make sure it will be on right
@@ -217,7 +219,7 @@ struct DirectiveCollector : public PPCallbacks {
spellDirective("#define ",
CharSourceRange::getTokenRange(
MI->getDefinitionLoc(), MI->getDefinitionEndLoc()),
- LangOpts, SM, TD.DirectiveLine);
+ LangOpts, SM, TD.DirectiveLine, TD.Offset);
}
private:
@@ -426,7 +428,8 @@ void escapeBackslashAndQuotes(llvm::StringRef Text, llvm::raw_ostream &OS) {
PreamblePatch PreamblePatch::create(llvm::StringRef FileName,
const ParseInputs &Modified,
- const PreambleData &Baseline) {
+ const PreambleData &Baseline,
+ PatchType PatchType) {
trace::Span Tracer("CreatePreamblePatch");
SPAN_ATTACH(Tracer, "File", FileName);
assert(llvm::sys::path::is_absolute(FileName) && "relative FileName!");
@@ -475,7 +478,7 @@ PreamblePatch PreamblePatch::create(llvm::StringRef FileName,
escapeBackslashAndQuotes(FileName, Patch);
Patch << "\"\n";
- if (IncludesChanged) {
+ if (IncludesChanged && PatchType == PatchType::All) {
// We are only interested in newly added includes, record the ones in
// Baseline for exclusion.
llvm::DenseMap<std::pair<tok::PPKeywordKind, llvm::StringRef>,
@@ -528,6 +531,18 @@ PreamblePatch PreamblePatch::create(llvm::StringRef FileName,
return PP;
}
+PreamblePatch PreamblePatch::createFullPatch(llvm::StringRef FileName,
+ const ParseInputs &Modified,
+ const PreambleData &Baseline) {
+ return create(FileName, Modified, Baseline, PatchType::All);
+}
+
+PreamblePatch PreamblePatch::createMacroPatch(llvm::StringRef FileName,
+ const ParseInputs &Modified,
+ const PreambleData &Baseline) {
+ return create(FileName, Modified, Baseline, PatchType::MacroDirectives);
+}
+
void PreamblePatch::apply(CompilerInvocation &CI) const {
// No need to map an empty file.
if (PatchContents.empty())
diff --git a/clang-tools-extra/clangd/Preamble.h b/clang-tools-extra/clangd/Preamble.h
index ecf10880ba7dc..a1b7a0f52b029 100644
--- a/clang-tools-extra/clangd/Preamble.h
+++ b/clang-tools-extra/clangd/Preamble.h
@@ -97,15 +97,20 @@ bool isPreambleCompatible(const PreambleData &Preamble,
/// new include directives.
class PreamblePatch {
public:
+ enum class PatchType { MacroDirectives, All };
/// \p Preamble is used verbatim.
static PreamblePatch unmodified(const PreambleData &Preamble);
/// 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);
+ static PreamblePatch createFullPatch(llvm::StringRef FileName,
+ const ParseInputs &Modified,
+ const PreambleData &Baseline);
+ static PreamblePatch createMacroPatch(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.
@@ -129,6 +134,11 @@ class PreamblePatch {
bool preserveDiagnostics() const { return PatchContents.empty(); }
private:
+ static PreamblePatch create(llvm::StringRef FileName,
+ const ParseInputs &Modified,
+ const PreambleData &Baseline,
+ PatchType PatchType);
+
PreamblePatch() = default;
std::string PatchContents;
std::string PatchFileName;
diff --git a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
index e37b65a9d4b81..8c0d3d34c75db 100644
--- a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -3262,6 +3262,23 @@ TEST(CompletionTest, ObjCCategoryDecls) {
EXPECT_THAT(Results.Completions, UnorderedElementsAre(Labeled("BarExt")));
}
}
+
+TEST(CompletionTest, PreambleCodeComplete) {
+ llvm::StringLiteral Baseline = "\n#define MACRO 12\nint num = MACRO;";
+ llvm::StringLiteral ModifiedCC =
+ "#include \"header.h\"\n#define MACRO 12\nint num = MACRO; int num2 = M^";
+
+ Annotations Test(ModifiedCC);
+ auto BaselineTU = TestTU::withCode(Baseline);
+ auto ModifiedTU = TestTU::withCode(Test.code());
+
+ MockFS FS;
+ auto Inputs = ModifiedTU.inputs(FS);
+ auto Result = codeComplete(testPath(ModifiedTU.Filename), Test.point(),
+ BaselineTU.preamble().get(), Inputs, {});
+ EXPECT_THAT(Result.Completions, Not(testing::IsEmpty()));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/clangd/unittests/PreambleTests.cpp b/clang-tools-extra/clangd/unittests/PreambleTests.cpp
index 70a14241a8ac4..bd9f9893adf96 100644
--- a/clang-tools-extra/clangd/unittests/PreambleTests.cpp
+++ b/clang-tools-extra/clangd/unittests/PreambleTests.cpp
@@ -59,7 +59,8 @@ collectPatchedIncludes(llvm::StringRef ModifiedContents,
// Create the patch.
TU.Code = ModifiedContents.str();
auto PI = TU.inputs(FS);
- auto PP = PreamblePatch::create(testPath(TU.Filename), PI, *BaselinePreamble);
+ auto PP = PreamblePatch::createFullPatch(testPath(TU.Filename), PI,
+ *BaselinePreamble);
// Collect patch contents.
IgnoreDiagnostics Diags;
auto CI = buildCompilerInvocation(PI, Diags);
@@ -183,8 +184,8 @@ TEST(PreamblePatchTest, PatchesPreambleIncludes) {
#include "a.h"
#include "b.h"
)cpp";
- auto PP = PreamblePatch::create(testPath(TU.Filename), TU.inputs(FS),
- *BaselinePreamble);
+ auto PP = PreamblePatch::createFullPatch(testPath(TU.Filename), TU.inputs(FS),
+ *BaselinePreamble);
// Only a.h should exists in the preamble, as c.h has been dropped and b.h was
// newly introduced.
EXPECT_THAT(PP.preambleIncludes(),
@@ -221,8 +222,8 @@ std::string getPreamblePatch(llvm::StringRef Baseline,
}
MockFS FS;
auto TU = TestTU::withCode(Modified);
- return PreamblePatch::create(testPath("main.cpp"), TU.inputs(FS),
- *BaselinePreamble)
+ return PreamblePatch::createFullPatch(testPath("main.cpp"), TU.inputs(FS),
+ *BaselinePreamble)
.text()
.str();
}
@@ -523,8 +524,8 @@ TEST(PreamblePatch, ModifiedBounds) {
Annotations Modified(Case.Modified);
TU.Code = Modified.code().str();
MockFS FS;
- auto PP = PreamblePatch::create(testPath(TU.Filename), TU.inputs(FS),
- *BaselinePreamble);
+ auto PP = PreamblePatch::createFullPatch(testPath(TU.Filename),
+ TU.inputs(FS), *BaselinePreamble);
IgnoreDiagnostics Diags;
auto CI = buildCompilerInvocation(TU.inputs(FS), Diags);
@@ -546,6 +547,13 @@ TEST(PreamblePatch, DropsDiagnostics) {
// Ensure they are dropeed when a patched preamble is used.
EXPECT_FALSE(createPatchedAST("", Code)->getDiagnostics());
}
+
+TEST(PreamblePatch, MacroLoc) {
+ llvm::StringLiteral Baseline = "\n#define MACRO 12\nint num = MACRO;";
+ llvm::StringLiteral Modified = " \n#define MACRO 12\nint num = MACRO;";
+ auto AST = createPatchedAST(Baseline, Modified);
+ ASSERT_TRUE(AST);
+}
} // namespace
} // namespace clangd
} // namespace clang
More information about the cfe-commits
mailing list