[clang][codegen] Fix emission of consteval constructor of derived type

[Fznamznon]

Summary

For simple derived type ConstantEmitter returns a struct of the same
size but different type which is then stored field-by-field into memory
via pointer to derived type. In case base type has more fields than derived,
the incorrect GEP is emitted. So, just cast pointer to derived type to
appropriate type with enough fields.

Fixes https://github.com/llvm/llvm-project/issues/60166

[Fznamznon]

Unit tests fail seems unrelated. I'm seeing the same test fail for a commit that is already in main https://github.com/llvm/llvm-project/commit/f1f583347d00aad378eb0128e72d3d2e8be5174b .

[shafik]

Makes sense to me but I am not familiar with this area so I will let someone else who has more knowledge approve.

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

95

nitpick: I have been using GH as a prefix for Github issues.

118

[shafik]

I think having a link to the github issue in the summary allows for the issue be closed automatically when you commit. Is this correct @aaron.ballman I have been doing this for a while and they get closed when I commit but maybe there is another mechanism involved.

[Fznamznon]

In D142534#4079958, @shafik wrote:

I think having a link to the github issue in the summary allows for the issue be closed automatically when you commit. Is this correct @aaron.ballman I have been doing this for a while and they get closed when I commit but maybe there is another mechanism involved.

This works at least for GitHub pull requests, see https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue . We've been using this in a downstream project. I was hoping it works in the same way for commits, so added issue ID to a commit message.

[Fznamznon]

Rebase and apply nit

[Fznamznon]

Ping?

[aaron.ballman]

In D142534#4080007, @Fznamznon wrote:

In D142534#4079958, @shafik wrote:

I think having a link to the github issue in the summary allows for the issue be closed automatically when you commit. Is this correct @aaron.ballman I have been doing this for a while and they get closed when I commit but maybe there is another mechanism involved.

This works at least for GitHub pull requests, see https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue . We've been using this in a downstream project. I was hoping it works in the same way for commits, so added issue ID to a commit message.

Yes, this works the same way for commits here. I learned recently that folks appreciate a full link to the issue (Fixes https://github.com/llvm/llvm-project/issues/NNNN) instead of Fixes #NNNN because it makes it easier to traverse from the commit message to the github issue (and it will still automatically close the issue on commit), so you might consider changing that when you land the patch.

The changes look correct to me, but codegen is not my area of expertise, so adding some other codegen reviewers to double-check.

[Fznamznon]

Ping.

[Fznamznon]

I discovered that this patch also helps to fix crash reported in https://github.com/llvm/llvm-project/issues/59223 .

[aaron.ballman]

LGTM, but please add a release note about the issues this fixes.

[Fznamznon]

Rebase and add a release note

clang/docs/ReleaseNotes.rst

- Fix crash on invalid code when looking up a destructor in a templated class
   `Issue 59446 <https://github.com/llvm/llvm-project/issues/59446>`_.
 - Fix crash when diagnosing incorrect usage of ``_Nullable`` involving alias
   templates. This fixes
   `Issue 60344 <https://github.com/llvm/llvm-project/issues/60344>`_.
 - Fix confusing warning message when ``/clang:-x`` is passed in ``clang-cl``
   driver mode and emit an error which suggests using ``/TC`` or ``/TP``
   ``clang-cl`` options instead. This fixes
   `Issue 59307 <https://github.com/llvm/llvm-project/issues/59307>`_.
+- Fix crash when evaluating consteval constructor of derived class whose base
+  has more than one field. This fixes
+  `Issue 60166 <https://github.com/llvm/llvm-project/issues/60166>`_.
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - We now generate a diagnostic for signed integer overflow due to unary minus
   in a non-constant expression context. This fixes
   `Issue 31643 <https://github.com/llvm/llvm-project/issues/31643>`_
 - Clang now warns by default for C++20 and later about deprecated capture of
   ``this`` with a capture default of ``=``. This warning can be disabled with

clang/lib/CodeGen/CGExprAgg.cpp

public:
   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(),
+      Address StoreDest = Dest.getAddress();
+      // The emitted value is guaranteed to have the same size as the
+      // destination but can have a different type. Just do a bitcast in this
+      // case to avoid incorrect GEPs.
+      if (Result->getType() != StoreDest.getType())
+        StoreDest =
+            CGF.Builder.CreateElementBitCast(StoreDest, Result->getType());
+      CGF.EmitAggregateStore(Result, StoreDest,
                              E->getType().isVolatileQualified());
       return;
     }
     return Visit(E->getSubExpr());
   }
 
   // l-values.
   void VisitDeclRefExpr(DeclRefExpr *E) { EmitAggLoadOfLValue(E); }

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

 int foo() {
   constexpr Derived a;
 
   auto val = a.Get();
   return val;
 }
 } // namespace Issue55065
 
+namespace GH60166 {
+
+struct Base {
+   void* one = nullptr;
+   void* two = nullptr;
+};
+
+struct Derived : Base {
+   void* three = nullptr;
+   consteval Derived() = default;
+};
+
+void method() {
+  // CHECK: %agg.tmp.ensured = alloca %"struct.GH60166::Derived"
+  // CHECK: %0 = getelementptr inbounds { ptr, ptr, ptr }, ptr %agg.tmp.ensured, i32 0, i32 0
+  // CHECK: store ptr null, ptr %0, align 8
+  // CHECK: %1 = getelementptr inbounds { ptr, ptr, ptr }, ptr %agg.tmp.ensured, i32 0, i32 1
+  // CHECK: store ptr null, ptr %1, align 8
+  // CHECK: %2 = getelementptr inbounds { ptr, ptr, ptr }, ptr %agg.tmp.ensured, i32 0, i32 2
+  // CHECK: store ptr null, ptr %2, align 8
+   (void)Derived();
+}
+
+} // namespace GH60166