[clang] fix consteval ctor code generation assert

[luken-google]

Summary

Fixes GH#53983.

[luken-google]

See: https://github.com/llvm/llvm-project/issues/53983

I can add a release note and some testing, but wanted feedback that this was the right approach. The problem is when generating code for the constructor for the bar element inside of MyStruct, Dest has an elementType of Base, but Builder.CreateStructGEP assumes the {i8, i8} layout implied by Base. which causes an assert trying to emit code for the second element. So, I've added logic to make sure the layout of STy and Dest are identical, which fixes the assert but doesn't break any of the existing testing around aggregate stores.

AFAICT the AST and lowering all looks correct, and this is an issue in code generation. But this is a new area of clang for me so I'd welcome suggestions about if there is a better fix or if the fix should be in a different place. Thanks!

[ilya-biryukov]

The change seems reasonable. Could you add a test with a crasher repro and some validation of the generated IR?

[ilya-biryukov]

Sorry, didn't see your comment when submitting mine.

I think the offending code is clearly wrong here and adding the isLayoutIdentical seems like the right thing to do.

This looks like a small change and I'm happy to LGTM this myself.
However, I also have very little experience with codegen, you might want to find someone more experienced with it to have a more informed look. @jyknight maybe?

clang/lib/CodeGen/CGCall.cpp

1338

Is it guaranteed that Dest.getElementType() is never something other than StructType?

[luken-google]

Adds test and release note, refactors to avoid possible null pointer deref.

[luken-google]

Add newline at end of test file.

clang/lib/CodeGen/CGCall.cpp

1338

Yikes, no. And checking the cast as another condition broke a bunch of other cases. This works much better.

[ilya-biryukov]

clang/lib/CodeGen/CGCall.cpp

1338

Intuitively, I feel like canLosslesslyBitCastTo should be used in conjuction with something like Builder.CreateBitcast and after a quick skim through existing I can't find any uses with CreateStore in the codebase. This leads me to think that we need to call a different function here. As mentioned earlier, I may be completely wrong as I don't have much experience with codegen.

I will let @jyknight sanity-check my intuition.

[luken-google]

seems to have fixed itself..

clang/docs/ReleaseNotes.rst

- Implemented `P0634r3 <https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0634r3.html>`_,
   which removes the requirement for the ``typename`` keyword in certain contexts.
 - Implemented The Equality Operator You Are Looking For (`P2468 <http://wg21.link/p2468r2>`_).
 - Implemented `P2113R0: Proposed resolution for 2019 comment CA 112 <https://wg21.link/P2113R0>`_
   ([temp.func.order]p6.2.1 is not implemented, matching GCC).
 - Implemented `P0857R0 <https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0857r0.html>`_,
   which specifies constrained lambdas and constrained template *template-parameter*\s.
 - Required parameter pack to be provided at the end of the concept parameter list. This
   fixes `Issue 48182 <https://github.com/llvm/llvm-project/issues/48182>`_.
+- Fixed a assert triggered in some cases of a ``consteval`` implicit cast constructor. Fixes
+  `Issue 53983 <https://github.com/llvm/llvm-project/issues/53983>`_.
 
 - Do not hide templated base members introduced via using-decl in derived class
   (useful specially for constrained members). Fixes `GH50886 <https://github.com/llvm/llvm-project/issues/50886>`_.
 - Implemented CWG2635 as a Defect Report, which prohibits structured bindings from being constrained.
 - Correctly handle access-checks in requires expression. Fixes `GH53364 <https://github.com/llvm/llvm-project/issues/53364>`_,
   `GH53334 <https://github.com/llvm/llvm-project/issues/53334>`_.
 
 - Implemented `P0960R3: <https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p0960r3.html>`_

clang/lib/CodeGen/CGCall.cpp

 
 // Function to store a first-class aggregate into memory.  We prefer to
 // store the elements rather than the aggregate to be more friendly to
 // fast-isel.
 // FIXME: Do we need to recurse here?
 void CodeGenFunction::EmitAggregateStore(llvm::Value *Val, Address Dest,
                                          bool DestIsVolatile) {
   // Prefer scalar stores to first-class aggregate stores.
-  if (llvm::StructType *STy = dyn_cast<llvm::StructType>(Val->getType())) {
+  llvm::StructType *STy = dyn_cast<llvm::StructType>(Val->getType());
+  if (STy && STy->canLosslesslyBitCastTo(Dest.getElementType())) {
     for (unsigned i = 0, e = STy->getNumElements(); i != e; ++i) {
       Address EltPtr = Builder.CreateStructGEP(Dest, i);
       llvm::Value *Elt = Builder.CreateExtractValue(Val, i);
       Builder.CreateStore(Elt, EltPtr, DestIsVolatile);
     }
   } else {
     Builder.CreateStore(Val, Dest, DestIsVolatile);
   }

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
-// RUN: %clang_cc1 -emit-obj -debug-info-kind=constructor -std=c++20 %s -o -
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 -fexceptions -fcxx-exceptions %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -emit-obj -debug-info-kind=constructor -std=c++20 -fexceptions -fcxx-exceptions %s -o -
 
 namespace PR50787 {
 // This code would previously cause a crash.
 extern int x_;
 consteval auto& X() { return x_; }
 constexpr auto& x1 = X();
 auto x2 = X();
 

 int foo() {
   constexpr Derived a;
 
   auto val = a.Get();
   return val;
 }
 } // namespace Issue55065
 
+namespace Issue53983 {
+// This code would previously cause a crash in code generation
+// for the `bar` element in the `MyStruct` constructor.
+struct Base {
+    constexpr Base() : a(), b() {}
+    char a : 1;
+    char b;
+};
+
+struct Foo : private Base {
+    constexpr Foo(bool b) {
+        if (b) throw "test";
+    }
+};
+
+struct Bar : private Base {
+    consteval Bar(bool) {}
+};
+
+struct MyStruct {
+    Foo foo = true;
+    Bar bar = false;
+};
+MyStruct structTest;
+
+// clang should issue a constructor for MyStruct that calls Foo and Bar,
+// and a constructor for Bar that calls Base.
+// CHECK: define{{.*}} void @_ZN10Issue539838MyStructC2Ev(
+// CHECK: entry:
+// CHECK: call void @_ZN10Issue539833FooC2Eb(
+// CHECK: call void @_ZN10Issue539833BarC2Eb(
+// CHECK: ret void
+
+// CHECK: define{{.*}} void @_ZN10Issue539833BarC2Eb(
+// CHECK: entry:
+// CHECK: call void @_ZN10Issue539834BaseC2Ev(
+// CHECK: ret void
+} // namespace Issue53983