No longer crash when a consteval function returns a structure

[aaron.ballman]

Summary

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.

[aaron.ballman]

Ping

[rjmccall]

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.

[aaron.ballman]

Updated to use EnsureDest() based on review feedback.

[aaron.ballman]

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&list_id=224889 has quite a few of them), so I share your worries.

[aaron.ballman]

Ping

[erichkeane]

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

[aaron.ballman]

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

clang/lib/CodeGen/CGExprAgg.cpp

public:
   }
   void VisitUnaryCoawait(UnaryOperator *E) { Visit(E->getSubExpr()); }
   void VisitUnaryExtension(UnaryOperator *E) { Visit(E->getSubExpr()); }
   void VisitSubstNonTypeTemplateParmExpr(SubstNonTypeTemplateParmExpr *E) {
     return Visit(E->getReplacement());
   }
 
   void VisitConstantExpr(ConstantExpr *E) {
+    EnsureDest(E->getType());
+
     if (llvm::Value *Result = ConstantEmitter(CGF).tryEmitConstantExpr(E)) {
       CGF.EmitAggregateStore(Result, Dest.getAddress(),
                              E->getType().isVolatileQualified());
       return;
     }
     return Visit(E->getSubExpr());
   }
 

clang/test/CodeGenCXX/cxx20-consteval-crash.cpp

 // RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 %s -emit-llvm -o - | FileCheck %s
 
 namespace PR50787 {
 // This code would previously cause a crash.
 extern int x_;
 consteval auto& X() { return x_; }
 constexpr auto& x1 = X();
 auto x2 = X();
 
 // CHECK: @_ZN7PR507872x_E = external global i32, align 4
 // CHECK-NEXT: @_ZN7PR507872x1E = constant i32* @_ZN7PR507872x_E, align 8
 // 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: }
+}