[PATCH] D111669: No longer crash when a consteval function returns a structure

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for the review! I've commit in 
c524f1a0764da0c8d1775b2860dfc33901ca9c8f 
.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111669/new/

https://reviews.llvm.org/D111669

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111669: No longer crash when a consteval function returns a structure

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.

Looks to accomplish this exactly the way that @rjmccall  suggested!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111669/new/

https://reviews.llvm.org/D111669

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111669: No longer crash when a consteval function returns a structure

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Ping


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111669/new/

https://reviews.llvm.org/D111669

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111669: No longer crash when a consteval function returns a structure

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D111669#3088970 , @rjmccall wrote:

> Hmm.  Generally these cases are expected to handle the situation where 
> there's no result slot passed in, which currently isn't exclusive to an 
> ignored result (although you could argue it should be).  IIRC we usually 
> don't pass in a slot when evaluating an expression of agg type as anything 
> except an initializer, e.g. when evaluating the `E` in `E.member`.  The 
> general fix is to call `EnsureDest` before accessing `Dest`.  The only reason 
> we wouldn't need to do that is if `ConstantExpr` is restricted in where it 
> can appear such that that's impossible.

Thank you for the suggestion to use `EnsureDest`, that worked like a charm.

> Skipping an unnecessary initialization does sound like a good optimization in 
> general, but that's on top of the bug fix, not an alternative.
>
> Separately, seeing this code makes me worried that it doesn't support the 
> full set of constant initialization.  We do have some kinds of constant 
> initialization that can't be emitted as abstract initializers, e.g. 
> address-diversified pointer auth.  But we can deal with that as a separate 
> patch.

FWIW, there are actually quite a few crashes during codegen involving consteval 
support (https://bugs.llvm.org/buglist.cgi?quicksearch=consteval_id=224889 
has quite a few of them), so I share your worries.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111669/new/

https://reviews.llvm.org/D111669

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111669: No longer crash when a consteval function returns a structure

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman updated this revision to Diff 382696.
aaron.ballman added a comment.

Updated to use `EnsureDest()` based on review feedback.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111669/new/

https://reviews.llvm.org/D111669

Files:
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/test/CodeGenCXX/cxx20-consteval-crash.cpp


Index: clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
===
--- clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
+++ clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
@@ -12,3 +12,15 @@
 // CHECK-NEXT: @_ZN7PR507872x2E = global i32* @_ZN7PR507872x_E, align 4
 }
 
+namespace PR51484 {
+// This code would previously cause a crash.
+struct X { int val; };
+consteval X g() { return {0}; }
+void f() { g(); }
+
+// CHECK: define dso_local void @_ZN7PR514841fEv() #0 {
+// CHECK: entry:
+// CHECK-NOT: call i32 @_ZN7PR514841gEv()
+// CHECK:  ret void
+// CHECK: }
+}
Index: clang/lib/CodeGen/CGExprAgg.cpp
===
--- clang/lib/CodeGen/CGExprAgg.cpp
+++ clang/lib/CodeGen/CGExprAgg.cpp
@@ -127,6 +127,8 @@
   }
 
   void VisitConstantExpr(ConstantExpr *E) {
+EnsureDest(E->getType());
+
 if (llvm::Value *Result = ConstantEmitter(CGF).tryEmitConstantExpr(E)) {
   CGF.EmitAggregateStore(Result, Dest.getAddress(),
  E->getType().isVolatileQualified());


Index: clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
===
--- clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
+++ clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
@@ -12,3 +12,15 @@
 // CHECK-NEXT: @_ZN7PR507872x2E = global i32* @_ZN7PR507872x_E, align 4
 }
 
+namespace PR51484 {
+// This code would previously cause a crash.
+struct X { int val; };
+consteval X g() { return {0}; }
+void f() { g(); }
+
+// CHECK: define dso_local void @_ZN7PR514841fEv() #0 {
+// CHECK: entry:
+// CHECK-NOT: call i32 @_ZN7PR514841gEv()
+// CHECK:  ret void
+// CHECK: }
+}
Index: clang/lib/CodeGen/CGExprAgg.cpp
===
--- clang/lib/CodeGen/CGExprAgg.cpp
+++ clang/lib/CodeGen/CGExprAgg.cpp
@@ -127,6 +127,8 @@
   }
 
   void VisitConstantExpr(ConstantExpr *E) {
+EnsureDest(E->getType());
+
 if (llvm::Value *Result = ConstantEmitter(CGF).tryEmitConstantExpr(E)) {
   CGF.EmitAggregateStore(Result, Dest.getAddress(),
  E->getType().isVolatileQualified());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111669: No longer crash when a consteval function returns a structure

[John McCall via Phabricator via cfe-commits]

rjmccall requested changes to this revision.
rjmccall added a comment.
This revision now requires changes to proceed.

Hmm.  Generally these cases are expected to handle the situation where there's 
no result slot passed in, which currently isn't exclusive to an ignored result 
(although you could argue it should be).  IIRC we usually don't pass in a slot 
when evaluating an expression of agg type as anything except an initializer, 
e.g. when evaluating the `E` in `E.member`.  The general fix is to call 
`EnsureDest` before accessing `Dest`.  The only reason we wouldn't need to do 
that is if `ConstantExpr` is restricted in where it can appear such that that's 
impossible.

Skipping an unnecessary initialization does sound like a good optimization in 
general, but that's on top of the bug fix, not an alternative.

Separately, seeing this code makes me worried that it doesn't support the full 
set of constant initialization.  We do have some kinds of constant 
initialization that can't be emitted as abstract initializers, e.g. 
address-diversified pointer auth.  But we can deal with that as a separate 
patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111669/new/

https://reviews.llvm.org/D111669

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111669: No longer crash when a consteval function returns a structure

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111669/new/

https://reviews.llvm.org/D111669

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111669: No longer crash when a consteval function returns a structure

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman created this revision.
aaron.ballman added reviewers: erichkeane, rsmith, rjmccall.
aaron.ballman requested review of this revision.
Herald added a project: clang.

I don't think the aggregate emitter needs to emit anything when handling a 
constant expression which has an unused result. The unused result means we have 
nowhere to store the value anyway during codegen, so this early returns in that 
case.

This addresses PR51484.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D111669

Files:
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/test/CodeGenCXX/cxx20-consteval-crash.cpp


Index: clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 %s -emit-llvm 
-o - | FileCheck %s
+
+// Test case comes from PR51484, where this code previously caused a crash.
+struct X { int val; };
+consteval X g() { return {0}; }
+void f() { g(); }
+
+// CHECK: define dso_local void @_Z1fv() #0 {
+// CHECK: entry:
+// CHECK-NOT: call i32 @_Z1gv()
+// CHECK:  ret void
+// CHECK: }
Index: clang/lib/CodeGen/CGExprAgg.cpp
===
--- clang/lib/CodeGen/CGExprAgg.cpp
+++ clang/lib/CodeGen/CGExprAgg.cpp
@@ -127,6 +127,10 @@
   }
 
   void VisitConstantExpr(ConstantExpr *E) {
+// If the result is unused, no need to emit anything for it.
+if (IsResultUnused)
+  return;
+
 if (llvm::Value *Result = ConstantEmitter(CGF).tryEmitConstantExpr(E)) {
   CGF.EmitAggregateStore(Result, Dest.getAddress(),
  E->getType().isVolatileQualified());


Index: clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 %s -emit-llvm -o - | FileCheck %s
+
+// Test case comes from PR51484, where this code previously caused a crash.
+struct X { int val; };
+consteval X g() { return {0}; }
+void f() { g(); }
+
+// CHECK: define dso_local void @_Z1fv() #0 {
+// CHECK: entry:
+// CHECK-NOT: call i32 @_Z1gv()
+// CHECK:  ret void
+// CHECK: }
Index: clang/lib/CodeGen/CGExprAgg.cpp
===
--- clang/lib/CodeGen/CGExprAgg.cpp
+++ clang/lib/CodeGen/CGExprAgg.cpp
@@ -127,6 +127,10 @@
   }
 
   void VisitConstantExpr(ConstantExpr *E) {
+// If the result is unused, no need to emit anything for it.
+if (IsResultUnused)
+  return;
+
 if (llvm::Value *Result = ConstantEmitter(CGF).tryEmitConstantExpr(E)) {
   CGF.EmitAggregateStore(Result, Dest.getAddress(),
  E->getType().isVolatileQualified());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits