Fix codegen of consteval functions returning an empty class, and related issues #93115

efriedma-quic · 2024-05-23T00:34:42Z

Fix codegen of consteval functions returning an empty class, and related issues

If a class is empty, don't store it to memory: the store might overwrite useful data. Similarly, if a class has tail padding that might overlap other fields, don't store the tail padding to memory.

The problem here turned out a bit more general than I initially thought: basically all uses of EmitAggregateStore were broken. Call lowering had a method that did mostly the right thing, though: CreateCoercedStore. Adapt CreateCoercedStore so it always does the conservatively right thing, and use it for both calls and ConstantExpr.

Also, along the way, fix the "overlap" bit in AggValueSlot: the bit was set incorrectly for empty classes in some cases.

Fixes #93040.

llvmbot · 2024-05-23T00:35:15Z

@llvm/pr-subscribers-backend-amdgpu

@llvm/pr-subscribers-clang

Author: Eli Friedman (efriedma-quic)

Changes

If a class is empty, don't store it to memory: the store might overwrite useful data.

(See also d60c3d0.)

Fixes #93040.

Full diff: https://github.com/llvm/llvm-project/pull/93115.diff

2 Files Affected:

(modified) clang/lib/CodeGen/CGExprAgg.cpp (+11)
(modified) clang/test/CodeGenCXX/cxx2a-consteval.cpp (+23-1)

diff --git a/clang/lib/CodeGen/CGExprAgg.cpp b/clang/lib/CodeGen/CGExprAgg.cpp
index bba00257fd4f0..b1638fa318270 100644
--- a/clang/lib/CodeGen/CGExprAgg.cpp
+++ b/clang/lib/CodeGen/CGExprAgg.cpp
@@ -135,6 +135,17 @@ class AggExprEmitter : public StmtVisitor<AggExprEmitter> {
     EnsureDest(E->getType());
 
     if (llvm::Value *Result = ConstantEmitter(CGF).tryEmitConstantExpr(E)) {
+      // An empty record can overlap other data (if declared with
+      // no_unique_address); omit the store for such types - as there is no
+      // actual data to store.
+      if (CGF.getLangOpts().CPlusPlus) {
+        if (const RecordType *RT = E->getType()->getAs<RecordType>()) {
+          CXXRecordDecl *Record = cast<CXXRecordDecl>(RT->getDecl());
+          if (Record->isEmpty())
+            return;
+        }
+      }
+
       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
diff --git a/clang/test/CodeGenCXX/cxx2a-consteval.cpp b/clang/test/CodeGenCXX/cxx2a-consteval.cpp
index 075cab58358ab..5d5a62f9928fe 100644
--- a/clang/test/CodeGenCXX/cxx2a-consteval.cpp
+++ b/clang/test/CodeGenCXX/cxx2a-consteval.cpp
@@ -1,4 +1,3 @@
-// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
 // RUN: %clang_cc1 -emit-llvm %s -std=c++2a -triple x86_64-unknown-linux-gnu -o %t.ll
 // RUN: FileCheck -check-prefix=EVAL -input-file=%t.ll %s
 // RUN: FileCheck -check-prefix=EVAL-STATIC -input-file=%t.ll %s
@@ -275,3 +274,26 @@ void f() {
     // EVAL-FN:     call void @_ZN7GH821542S3C2Ei
 }
 }
+
+namespace GH93040 {
+struct C { char c = 1; };
+struct Empty { consteval Empty() {} };
+struct Test : C, Empty {
+  [[no_unique_address]] Empty e;
+};
+
+void f() {
+  Test test;
+
+// Make sure we don't overwrite the initialization of c.
+
+// EVAL-FN-LABEL: define {{.*}} void @_ZN7GH930404TestC2Ev
+// EVAL-FN: entry:
+// EVAL-FN-NEXT:  [[THIS_ADDR:%.*]] = alloca ptr, align 8
+// EVAL-FN-NEXT:  store ptr {{.*}}, ptr [[THIS_ADDR]], align 8
+// EVAL-FN-NEXT:  [[THIS:%.*]] = load ptr, ptr [[THIS_ADDR]], align 8
+// EVAL-FN-NEXT:  call void @_ZN7GH930401CC2Ev(ptr noundef nonnull align 1 dereferenceable(1) [[THIS]])
+// EVAL-FN-NEXT:  %0 = getelementptr inbounds i8, ptr [[THIS]], i64 1
+// EVAL-FN-NEXT:  ret void
+}
+}

llvmbot · 2024-05-23T00:35:16Z

@llvm/pr-subscribers-clang-codegen

Author: Eli Friedman (efriedma-quic)

Changes

If a class is empty, don't store it to memory: the store might overwrite useful data.

(See also d60c3d0.)

Fixes #93040.

Full diff: https://github.com/llvm/llvm-project/pull/93115.diff

2 Files Affected:

(modified) clang/lib/CodeGen/CGExprAgg.cpp (+11)
(modified) clang/test/CodeGenCXX/cxx2a-consteval.cpp (+23-1)

diff --git a/clang/lib/CodeGen/CGExprAgg.cpp b/clang/lib/CodeGen/CGExprAgg.cpp
index bba00257fd4f0..b1638fa318270 100644
--- a/clang/lib/CodeGen/CGExprAgg.cpp
+++ b/clang/lib/CodeGen/CGExprAgg.cpp
@@ -135,6 +135,17 @@ class AggExprEmitter : public StmtVisitor<AggExprEmitter> {
     EnsureDest(E->getType());
 
     if (llvm::Value *Result = ConstantEmitter(CGF).tryEmitConstantExpr(E)) {
+      // An empty record can overlap other data (if declared with
+      // no_unique_address); omit the store for such types - as there is no
+      // actual data to store.
+      if (CGF.getLangOpts().CPlusPlus) {
+        if (const RecordType *RT = E->getType()->getAs<RecordType>()) {
+          CXXRecordDecl *Record = cast<CXXRecordDecl>(RT->getDecl());
+          if (Record->isEmpty())
+            return;
+        }
+      }
+
       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
diff --git a/clang/test/CodeGenCXX/cxx2a-consteval.cpp b/clang/test/CodeGenCXX/cxx2a-consteval.cpp
index 075cab58358ab..5d5a62f9928fe 100644
--- a/clang/test/CodeGenCXX/cxx2a-consteval.cpp
+++ b/clang/test/CodeGenCXX/cxx2a-consteval.cpp
@@ -1,4 +1,3 @@
-// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
 // RUN: %clang_cc1 -emit-llvm %s -std=c++2a -triple x86_64-unknown-linux-gnu -o %t.ll
 // RUN: FileCheck -check-prefix=EVAL -input-file=%t.ll %s
 // RUN: FileCheck -check-prefix=EVAL-STATIC -input-file=%t.ll %s
@@ -275,3 +274,26 @@ void f() {
     // EVAL-FN:     call void @_ZN7GH821542S3C2Ei
 }
 }
+
+namespace GH93040 {
+struct C { char c = 1; };
+struct Empty { consteval Empty() {} };
+struct Test : C, Empty {
+  [[no_unique_address]] Empty e;
+};
+
+void f() {
+  Test test;
+
+// Make sure we don't overwrite the initialization of c.
+
+// EVAL-FN-LABEL: define {{.*}} void @_ZN7GH930404TestC2Ev
+// EVAL-FN: entry:
+// EVAL-FN-NEXT:  [[THIS_ADDR:%.*]] = alloca ptr, align 8
+// EVAL-FN-NEXT:  store ptr {{.*}}, ptr [[THIS_ADDR]], align 8
+// EVAL-FN-NEXT:  [[THIS:%.*]] = load ptr, ptr [[THIS_ADDR]], align 8
+// EVAL-FN-NEXT:  call void @_ZN7GH930401CC2Ev(ptr noundef nonnull align 1 dereferenceable(1) [[THIS]])
+// EVAL-FN-NEXT:  %0 = getelementptr inbounds i8, ptr [[THIS]], i64 1
+// EVAL-FN-NEXT:  ret void
+}
+}

zygoloid

Looks good. Do we also need to worry about overwriting tail padding here?

efriedma-quic · 2024-05-23T01:29:19Z

I didn't think so at first glance... but yes, we do, in certain obscure cases:

#include <new>
struct A { char c; A(); };
struct __attribute((packed)) S  { char a; int x; __attribute((aligned(2))) char y; consteval S() : x(1), a(3), y(2) {} };
struct S2 { [[no_unique_address]] S s; [[no_unique_address]] A a; };
static_assert(sizeof(S)==8 && sizeof(S2)==8);
void f2(S2 *s) { new (&s->s) S; }

I'll look into reworking this.

serge-sans-paille · 2024-05-23T06:53:20Z

clang/lib/CodeGen/CGExprAgg.cpp

@@ -135,6 +135,17 @@ class AggExprEmitter : public StmtVisitor<AggExprEmitter> {
    EnsureDest(E->getType());

    if (llvm::Value *Result = ConstantEmitter(CGF).tryEmitConstantExpr(E)) {
+      // An empty record can overlap other data (if declared with
+      // no_unique_address); omit the store for such types - as there is no


Candide question: Empty record still need one byte when their address is taken (thus this comment about no_unique_address I guess), why don't we see that in the diff?

See what, exactly? Given the derived class, computing the address of the base class doesn't take any instructions, because it's the same address.

github-actions · 2024-05-24T01:46:49Z

✅ With the latest revision this PR passed the C/C++ code formatter.

efriedma-quic · 2024-05-24T01:49:18Z

clang/test/CodeGenOpenCL/addr-space-struct-arg.cl

@@ -177,7 +179,12 @@ kernel void KernelTwoMember(struct StructTwoMember u) {
 // AMDGCN-LABEL: define{{.*}} amdgpu_kernel void @KernelLargeTwoMember
 // AMDGCN-SAME:  (%struct.LargeStructTwoMember %[[u_coerce:.*]])
 // AMDGCN:  %[[u:.*]] = alloca %struct.LargeStructTwoMember, align 8, addrspace(5)
-// AMDGCN:  store %struct.LargeStructTwoMember %[[u_coerce]], ptr addrspace(5) %[[u]]
+// AMDGCN:  %[[U_PTR0:.*]] = getelementptr inbounds %struct.LargeStructTwoMember, ptr addrspace(5) %[[u]], i32 0, i32 0


Unifying the codepaths makes FCA promotion happen more often.

efriedma-quic · 2024-05-24T01:49:51Z

clang/test/CodeGenCXX/address-space-cast-coerce.cpp

@@ -46,9 +46,9 @@ int mane() {
    char1 f1{1};
    char1 f2{1};

-// CHECK: [[TMP:%.+]] = alloca i16


The revised version of casting integers is a bit more aggressive; it's hard to make it precisely match the old code while still preserving the correct semantics.

efriedma-quic · 2024-05-24T01:50:50Z

clang/test/CodeGen/arm-mve-intrinsics/vld24.c

-// CHECK-NEXT:    [[VALUE_COERCE_FCA_0_1_EXTRACT:%.*]] = extractvalue [[STRUCT_UINT32X4X2_T]] [[VALUE_COERCE]], 0, 1
-// CHECK-NEXT:    call void @llvm.arm.mve.vst2q.p0.v4i32(ptr [[ADDR:%.*]], <4 x i32> [[VALUE_COERCE_FCA_0_0_EXTRACT]], <4 x i32> [[VALUE_COERCE_FCA_0_1_EXTRACT]], i32 0)
-// CHECK-NEXT:    call void @llvm.arm.mve.vst2q.p0.v4i32(ptr [[ADDR]], <4 x i32> [[VALUE_COERCE_FCA_0_0_EXTRACT]], <4 x i32> [[VALUE_COERCE_FCA_0_1_EXTRACT]], i32 1)
+// CHECK-NEXT:    [[TMP0:%.*]] = extractvalue [[STRUCT_UINT32X4X2_T:%.*]] [[VALUE_COERCE:%.*]], 0


Apparently I've stumbled over some limitation of instcombine.

efriedma-quic · 2024-05-24T01:51:12Z

clang/test/CodeGen/arm-vfp16-arguments2.cpp

@@ -44,20 +44,20 @@ struct S1 f1(struct S1 s1) { return s1; }

 // CHECK-SOFT: define{{.*}} void @_Z2f22S2(ptr dead_on_unwind noalias nocapture writable writeonly sret(%struct.S2) align 8 %agg.result, [4 x i32] %s2.coerce)
 // CHECK-HARD: define{{.*}} arm_aapcs_vfpcc [2 x <2 x i32>] @_Z2f22S2([2 x <2 x i32>] returned %s2.coerce)
-// CHECK-FULL: define{{.*}} arm_aapcs_vfpcc %struct.S2 @_Z2f22S2(%struct.S2 returned %s2.coerce)
+// CHECK-FULL: define{{.*}} arm_aapcs_vfpcc %struct.S2 @_Z2f22S2(%struct.S2 %s2.coerce)


This is also the instcombine issue.

efriedma-quic · 2024-06-25T16:26:31Z

(I'd like a re-review of the latest version: I made significant revisions to address the tail-padding issues.)

clang/lib/CodeGen/CGCall.cpp

zygoloid · 2024-06-25T19:49:00Z

clang/lib/CodeGen/CodeGenFunction.h

  /// Build all the stores needed to initialize an aggregate at Dest with the
  /// value Val.


This comment looks out of date.

…ted issues If a class is empty, don't store it to memory: the store might overwrite useful data. Similarly, if a class has tail padding that might overlap other fields, don't store the tail padding to memory. The problem here turned out a bit more general than I initially thought: basically all uses of EmitAggregateStore were broken. Call lowering had a method that did mostly the right thing, though: CreateCoercedStore. Adapt CreateCoercedStore so it always does the conservatively right thing, and use it for both calls and ConstantExpr. Also, along the way, fix the "overlap" bit in AggValueSlot: the bit was set incorrectly for empty classes in some cases. Fixes llvm#93040.

AZero13 · 2024-08-02T19:34:27Z

Is this worth back porting as it is a bugfix over code gen?

AaronBallman · 2024-08-05T13:27:01Z

Is this worth back porting as it is a bugfix over code gen?

IMO, it's worth considering, but if we want to go down this route, I think we need to do so relatively quickly -- we have about two weeks until rc3, and given the size of this change, I'm not certain we should try landing it any later than rc3 just due to risk.

efriedma-quic · 2024-08-05T21:00:23Z

This is maybe slightly risky in terms of possible regressions, but it is a fix for a miscompile, and we're early enough in the release process that it's probably fine.

/cherry-pick 1762e01

llvmbot · 2024-08-05T21:05:10Z

Failed to create pull request for issue93115 https://github.com/llvm/llvm-project/actions/runs/10255950901

efriedma-quic · 2024-08-05T21:52:34Z

/cherry-pick 1762e01

…ted issues (llvm#93115) Fix codegen of consteval functions returning an empty class, and related issues If a class is empty, don't store it to memory: the store might overwrite useful data. Similarly, if a class has tail padding that might overlap other fields, don't store the tail padding to memory. The problem here turned out a bit more general than I initially thought: basically all uses of EmitAggregateStore were broken. Call lowering had a method that did mostly the right thing, though: CreateCoercedStore. Adapt CreateCoercedStore so it always does the conservatively right thing, and use it for both calls and ConstantExpr. Also, along the way, fix the "overlap" bit in AggValueSlot: the bit was set incorrectly for empty classes in some cases. Fixes llvm#93040. (cherry picked from commit 1762e01)

llvmbot · 2024-08-05T21:57:36Z

/pull-request #102070

…ted issues (llvm#93115) Fix codegen of consteval functions returning an empty class, and related issues If a class is empty, don't store it to memory: the store might overwrite useful data. Similarly, if a class has tail padding that might overlap other fields, don't store the tail padding to memory. The problem here turned out a bit more general than I initially thought: basically all uses of EmitAggregateStore were broken. Call lowering had a method that did mostly the right thing, though: CreateCoercedStore. Adapt CreateCoercedStore so it always does the conservatively right thing, and use it for both calls and ConstantExpr. Also, along the way, fix the "overlap" bit in AggValueSlot: the bit was set incorrectly for empty classes in some cases. Fixes llvm#93040. (cherry picked from commit 1762e01)

efriedma-quic requested review from zygoloid, rjmccall, serge-sans-paille, cor3ntin, AaronBallman and pogo59 May 23, 2024 00:34

llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. labels May 23, 2024

efriedma-quic requested a review from mstorsjo May 23, 2024 00:36

zygoloid approved these changes May 23, 2024

View reviewed changes

serge-sans-paille reviewed May 23, 2024

View reviewed changes

cor3ntin requested a review from erichkeane May 23, 2024 19:08

efriedma-quic force-pushed the consteval-empty-struct branch from bdfcc72 to 19f3b67 Compare May 24, 2024 01:43

llvmbot added the backend:AMDGPU label May 24, 2024

efriedma-quic changed the title ~~Fix codegen of consteval functions returning an empty class.~~ Fix codegen of consteval functions returning an empty class, and related issues May 24, 2024

efriedma-quic commented May 24, 2024

View reviewed changes

efriedma-quic force-pushed the consteval-empty-struct branch from 19f3b67 to 816ceb2 Compare June 9, 2024 21:02

zygoloid approved these changes Jun 25, 2024

View reviewed changes

efriedma-quic force-pushed the consteval-empty-struct branch from 816ceb2 to 75a99e3 Compare July 30, 2024 06:31

efriedma-quic merged commit 1762e01 into llvm:main Aug 1, 2024
7 checks passed

efriedma-quic added this to the LLVM 19.X Release milestone Aug 5, 2024

thewtex mentioned this pull request Feb 10, 2025

llvmorg 19.1.5 libcxxabi pthread lib name #126605

Closed

kernigh mentioned this pull request Apr 2, 2025

clang 19 or 20 miscompiles llvm::MergeBasicBlockIntoOnlyPred for PPC32 #133507

Open

		/// Build all the stores needed to initialize an aggregate at Dest with the
		/// value Val.

Fix codegen of consteval functions returning an empty class, and related issues #93115

Fix codegen of consteval functions returning an empty class, and related issues #93115

Uh oh!

Conversation

efriedma-quic commented May 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented May 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented May 23, 2024

Uh oh!

zygoloid left a comment

Choose a reason for hiding this comment

Uh oh!

efriedma-quic commented May 23, 2024

Uh oh!

serge-sans-paille May 23, 2024

Choose a reason for hiding this comment

Uh oh!

efriedma-quic May 23, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented May 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

efriedma-quic May 24, 2024

Choose a reason for hiding this comment

Uh oh!

efriedma-quic May 24, 2024

Choose a reason for hiding this comment

Uh oh!

efriedma-quic May 24, 2024

Choose a reason for hiding this comment

Uh oh!

efriedma-quic May 24, 2024

Choose a reason for hiding this comment

Uh oh!

efriedma-quic commented Jun 25, 2024

Uh oh!

Uh oh!

zygoloid Jun 25, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AZero13 commented Aug 2, 2024

Uh oh!

AaronBallman commented Aug 5, 2024

Uh oh!

efriedma-quic commented Aug 5, 2024

Uh oh!

llvmbot commented Aug 5, 2024

Uh oh!

efriedma-quic commented Aug 5, 2024

Uh oh!

llvmbot commented Aug 5, 2024

Uh oh!

Uh oh!

efriedma-quic commented May 23, 2024 •

edited

Loading

llvmbot commented May 23, 2024 •

edited

Loading

github-actions bot commented May 24, 2024 •

edited

Loading