sammccall created this revision.
sammccall added a reviewer: hokein.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.
Clang's error correction can invent extra tokens, including at the "eof"
position of various FileIDs.
(I thought this only happened at the AST level - I was mistaken).
Currently this can lead to Selection looping forever unable to make progress
because the location is not in range.
Unfortunately after fixing this the selection we get may be garbage: it can
also invent tokens whose locations coincide with real ones. Nevertheless
this is better than asserting/hanging.
Fixes https://github.com/llvm/llvm-project/issues/58482
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D136567
Files:
clang-tools-extra/clangd/Selection.cpp
clang-tools-extra/clangd/unittests/SelectionTests.cpp
Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -697,6 +697,24 @@
EXPECT_EQ("WhileStmt", T.commonAncestor()->Parent->kind());
}
+TEST(SelectionTest, MacroInitListCorrection) {
+ // This macro use is broken: the , is unparenthesized and so forms two args.
+ // clang can try to repair it, inserting extra parentheses.
+ // However these parens have dubious locations, we used to infloop here.
+ // https://github.com/llvm/llvm-project/issues/58482
+ const char *Case = R"cpp(
+ struct SS{int x,y;};
+ #define ID(X) X
+ auto s = ID(S^S{1,2}); // error-ok
+ )cpp";
+ auto AST = TestTU::withCode(Annotations(Case).code()).build();
+ auto T = makeSelectionTree(Case, AST);
+ // Ideally we would select RecordLoc, but the token sequence emitted by the
+ // preprocessor is confusing (there's an l_paren with the same loc as SS).
+ // Let's just be happy with selecting something and not crashing.
+ EXPECT_NE(nullptr, T.commonAncestor());
+}
+
TEST(SelectionTest, IncludedFile) {
const char *Case = R"cpp(
void test() {
Index: clang-tools-extra/clangd/Selection.cpp
===================================================================
--- clang-tools-extra/clangd/Selection.cpp
+++ clang-tools-extra/clangd/Selection.cpp
@@ -324,7 +324,11 @@
// Comparing SourceLocations against bounds is cheaper than getFileID().
SourceLocation Limit = SM.getComposedLoc(FID, SM.getFileIDSize(FID));
auto Batch = ExpandedTokens.take_while([&](const syntax::Token &T) {
- return T.location() >= Start && T.location() < Limit;
+ // [Start, Limit) rather than [Start, Limit] seems correct.
+ // However PP error-correction can produce one-past-the-end locations.
+ // (The selections we produce may be dubious, but we won't hang!)
+ // See https://github.com/llvm/llvm-project/issues/58482
+ return T.location() >= Start && T.location() <= Limit;
});
assert(!Batch.empty());
ExpandedTokens = ExpandedTokens.drop_front(Batch.size());
Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -697,6 +697,24 @@
EXPECT_EQ("WhileStmt", T.commonAncestor()->Parent->kind());
}
+TEST(SelectionTest, MacroInitListCorrection) {
+ // This macro use is broken: the , is unparenthesized and so forms two args.
+ // clang can try to repair it, inserting extra parentheses.
+ // However these parens have dubious locations, we used to infloop here.
+ // https://github.com/llvm/llvm-project/issues/58482
+ const char *Case = R"cpp(
+ struct SS{int x,y;};
+ #define ID(X) X
+ auto s = ID(S^S{1,2}); // error-ok
+ )cpp";
+ auto AST = TestTU::withCode(Annotations(Case).code()).build();
+ auto T = makeSelectionTree(Case, AST);
+ // Ideally we would select RecordLoc, but the token sequence emitted by the
+ // preprocessor is confusing (there's an l_paren with the same loc as SS).
+ // Let's just be happy with selecting something and not crashing.
+ EXPECT_NE(nullptr, T.commonAncestor());
+}
+
TEST(SelectionTest, IncludedFile) {
const char *Case = R"cpp(
void test() {
Index: clang-tools-extra/clangd/Selection.cpp
===================================================================
--- clang-tools-extra/clangd/Selection.cpp
+++ clang-tools-extra/clangd/Selection.cpp
@@ -324,7 +324,11 @@
// Comparing SourceLocations against bounds is cheaper than getFileID().
SourceLocation Limit = SM.getComposedLoc(FID, SM.getFileIDSize(FID));
auto Batch = ExpandedTokens.take_while([&](const syntax::Token &T) {
- return T.location() >= Start && T.location() < Limit;
+ // [Start, Limit) rather than [Start, Limit] seems correct.
+ // However PP error-correction can produce one-past-the-end locations.
+ // (The selections we produce may be dubious, but we won't hang!)
+ // See https://github.com/llvm/llvm-project/issues/58482
+ return T.location() >= Start && T.location() <= Limit;
});
assert(!Batch.empty());
ExpandedTokens = ExpandedTokens.drop_front(Batch.size());
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
