-
Notifications
You must be signed in to change notification settings - Fork 15k
[Clang] Fix crash on void{}
#147514
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Clang] Fix crash on void{}
#147514
Conversation
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-clang Author: Corentin Jabot (cor3ntin) ChangesCaused by an incorrect assertion. Fixes #116440 Full diff: https://github.com/llvm/llvm-project/pull/147514.diff 2 Files Affected:
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp
index fc441dd92d1ee..44931d0481e26 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -2114,8 +2114,10 @@ static int getAsInt32(llvm::ConstantInt *C, llvm::Type *I32Ty) {
Value *ScalarExprEmitter::VisitInitListExpr(InitListExpr *E) {
bool Ignore = TestAndClearIgnoreResultAssign();
(void)Ignore;
- assert (Ignore == false && "init list ignored");
unsigned NumInitElements = E->getNumInits();
+ assert(Ignore == false ||
+ (NumInitElements == 0 && E->getType()->isVoidType()) &&
+ "init list ignored");
// HLSL initialization lists in the AST are an expansion which can contain
// side-effecting expressions wrapped in opaque value expressions. To properly
diff --git a/clang/test/CodeGenCXX/cxx0x-initializer-scalars.cpp b/clang/test/CodeGenCXX/cxx0x-initializer-scalars.cpp
index 2f6a6820a7589..478ad40359727 100644
--- a/clang/test/CodeGenCXX/cxx0x-initializer-scalars.cpp
+++ b/clang/test/CodeGenCXX/cxx0x-initializer-scalars.cpp
@@ -5,3 +5,12 @@ void f()
// CHECK: store i32 0
int i{};
}
+
+
+namespace GH116440 {
+void f() {
+ void{};
+ void();
+}
+
+}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add check-lines to this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you want me to check? noting is emitted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just the function header then CHECK-NEXT: ret void confirms that it actually does nothing/makes it through codegen without going sideways.
So:
// CHECK: define{{.*}} void @{{.*}}GH116440{{.*}}()
// CHECK-NEXT: {{.*}}:
// CHECK-NEXT: ret void
That should work on both windows and itanium manglers, though be more specific on the function name if this test uses a triple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no target-triple on this test, so this will fail in windows mode. You can try running it with -triple x86_64-windows-pc to see what I mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oups!
Caused by an incorrect assertion. Fixes llvm#116440
7f8cabb to
6ca3ffa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we missed a release note?
| assert(Ignore == false || | ||
| (NumInitElements == 0 && E->getType()->isVoidType()) && | ||
| "init list ignored"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new assert formulation raises a warning in gcc -Wall: https://godbolt.org/z/G8v1zqE4Y
And the grouping of the comment "init list ignored" to only the second condition seems unintended or misleading. Could we revise this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #158635
As brought up on the review for llvm#147514, the original patch doesn't correctly parenthesize the expression in the assert. This fixes it.
As brought up on the review for #147514, the original patch doesn't correctly parenthesize the expression in the assert. This fixes it.
Caused by an incorrect assertion.
Fixes #116440