[Clang] Fix a crash when dumping a pack indexing type. #80439
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: cor3ntin (cor3ntin) ChangesFix a crash caused by incorrect assumptions Full diff: https://github.com/llvm/llvm-project/pull/80439.diff 2 Files Affected:
diff --git a/clang/lib/AST/TypePrinter.cpp b/clang/lib/AST/TypePrinter.cpp
index 63e56a8296db3..281f529ee1f75 100644
--- a/clang/lib/AST/TypePrinter.cpp
+++ b/clang/lib/AST/TypePrinter.cpp
@@ -1195,10 +1195,10 @@ void TypePrinter::printDecltypeBefore(const DecltypeType *T, raw_ostream &OS) {
void TypePrinter::printPackIndexingBefore(const PackIndexingType *T,
raw_ostream &OS) {
- if (T->isInstantiationDependentType())
- OS << T->getPattern() << "...[" << T->getIndexExpr() << "]";
- else
+ if (T->hasSelectedType())
OS << T->getSelectedType();
+ else
+ OS << T->getPattern() << "...[" << T->getIndexExpr() << "]";
spaceBeforePlaceHolder(OS);
}
diff --git a/clang/test/AST/ast-dump-pack-indexing-crash.cpp b/clang/test/AST/ast-dump-pack-indexing-crash.cpp
new file mode 100644
index 0000000000000..1e4e38e2f7378
--- /dev/null
+++ b/clang/test/AST/ast-dump-pack-indexing-crash.cpp
@@ -0,0 +1,24 @@
+// RUN: not %clang_cc1 -std=c++2c -ast-dump %s | FileCheck %s
+
+namespace InvalidPacksShouldNotCrash {
+
+struct NotAPack;
+template <typename T, auto V, template<typename> typename Tp>
+void not_pack() {
+ int i = 0;
+ i...[0]; // expected-error {{i does not refer to the name of a parameter pack}}
+ V...[0]; // expected-error {{V does not refer to the name of a parameter pack}}
+ NotAPack...[0] a; // expected-error{{'NotAPack' does not refer to the name of a parameter pack}}
+ T...[0] b; // expected-error{{'T' does not refer to the name of a parameter pack}}
+ Tp...[0] c; // expected-error{{'Tp' does not refer to the name of a parameter pack}}
+}
+
+// CHECK: -FunctionDecl {{.*}} not_pack 'void ()'
+// CHECK: |-DeclStmt {{.*}}
+// CHECK: | `-VarDecl {{.*}} a 'NotAPack...{{.*}}'
+// CHECK: |-DeclStmt {{.*}}
+// CHECK: | `-VarDecl {{.*}} 'T...{{.*}}'
+// CHECK: `-DeclStmt {{.*}}
+// CHECK: `-VarDecl {{.*}} c 'Tp...{{.*}}'
+
+}
|
Fix a crash caused by incorrect assumptions Reported here llvm#72644 (comment)
erichkeane
reviewed
Feb 2, 2024
| } | ||
|
|
||
| // CHECK: -FunctionDecl {{.*}} not_pack 'void ()' | ||
| // CHECK: |-DeclStmt {{.*}} |
There was a problem hiding this comment.
Instead of testing the spaces/- and |- character, just use 'CHECK-NEXT`. Else this can get fraigle due to WS. Else the test looks fine.
There was a problem hiding this comment.
So the fragility is from checking the - and the |- and the `| `` characters. I'd suggest removing those entirely.
erichkeane
approved these changes
Feb 2, 2024
cor3ntin
force-pushed
the
fix_pack_indexing
branch
from
5999b51
to
0bbf2c3
Compare
erichkeane
approved these changes
Feb 2, 2024
|
Seems like this didn't really solve the problem reported by @bevin-hansson (at least not completely). Well. I don't know that much about this. Just noticed that our downstream bots started to fail on this new test case. Maybe we need to look at it on Monday to give more context, I don't have more info right now at least. |
|
@bjope thanks for letting me know. A reproduction would really help, as upstream bots do not exhibit this behavior at all. |
|
@cor3ntin , our downstream code is doing some extra semantic checks using code like this in Sema::CheckVariableDeclarationType: That is hitting the infinite recursion. So we protect the type size calculation by checking if the type is incomplete, dependent, etc. I must say that I don't know that much about this to say what is correct. Maybe out downstream semantic checks should be protected in some more way to avoid this problem. |
bjope
reviewed
Feb 4, 2024
| int i = 0; | ||
| i...[0]; // expected-error {{i does not refer to the name of a parameter pack}} | ||
| V...[0]; // expected-error {{V does not refer to the name of a parameter pack}} | ||
| NotAPack...[0] a; // expected-error{{'NotAPack' does not refer to the name of a parameter pack}} |
There was a problem hiding this comment.
If you change this to an array, such as
NotAPack...[0] a[2];
then I think you hit the same kind of infinite recursion as we see downstream even without the array nodtation.
agozillon
pushed a commit
to agozillon/llvm-project
that referenced
this pull request
Feb 5, 2024
Fix a crash caused by incorrect assumptions Reported here llvm#72644 (comment)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix a crash caused by incorrect assumptions
Reported here #72644 (comment)