[clang] Fix codegen of consteval functions returning an empty class. (PR #93115)
https://github.com/efriedma-quic updated
https://github.com/llvm/llvm-project/pull/93115
>From 19f3b677d92ed88b825b455d738055da05f91e0d Mon Sep 17 00:00:00 2001
From: Eli Friedman
Date: Thu, 23 May 2024 18:38:04 -0700
Subject: [PATCH] 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.
---
clang/lib/CodeGen/CGCall.cpp | 138 --
clang/lib/CodeGen/CGExprAgg.cpp | 23 +--
clang/lib/CodeGen/CodeGenFunction.h | 3 +-
clang/test/CodeGen/arm-mve-intrinsics/vld24.c | 43 --
clang/test/CodeGen/arm-vfp16-arguments2.cpp | 10 +-
.../amdgpu-kernel-arg-pointer-type.cu | 11 +-
clang/test/CodeGenCUDA/builtins-amdgcn.cu | 121 +++
.../CodeGenCXX/address-space-cast-coerce.cpp | 6 +-
clang/test/CodeGenCXX/cxx2a-consteval.cpp | 24 ++-
clang/test/CodeGenCXX/trivial_abi.cpp | 20 +++
clang/test/CodeGenHIP/dpp-const-fold.hip | 8 +-
.../CodeGenOpenCL/addr-space-struct-arg.cl| 11 +-
.../amdgpu-abi-struct-arg-byref.cl| 42 --
13 files changed, 253 insertions(+), 207 deletions(-)
diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp
index 97449a5e51e73..db6feb4bf1d79 100644
--- a/clang/lib/CodeGen/CGCall.cpp
+++ b/clang/lib/CodeGen/CGCall.cpp
@@ -1336,75 +1336,54 @@ static llvm::Value *CreateCoercedLoad(Address Src,
llvm::Type *Ty,
return CGF.Builder.CreateLoad(Tmp);
}
-// 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(Val->getType())) {
-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);
- }
-}
-
/// CreateCoercedStore - Create a store to \arg DstPtr from \arg Src,
/// where the source and destination may have different types. The
/// destination is known to be aligned to \arg DstAlign bytes.
///
/// This safely handles the case when the src type is larger than the
/// destination type; the upper bits of the src will be lost.
-static void CreateCoercedStore(llvm::Value *Src,
- Address Dst,
- bool DstIsVolatile,
- CodeGenFunction ) {
- llvm::Type *SrcTy = Src->getType();
- llvm::Type *DstTy = Dst.getElementType();
- if (SrcTy == DstTy) {
-CGF.Builder.CreateStore(Src, Dst, DstIsVolatile);
-return;
- }
-
- llvm::TypeSize SrcSize = CGF.CGM.getDataLayout().getTypeAllocSize(SrcTy);
-
- if (llvm::StructType *DstSTy = dyn_cast(DstTy)) {
-Dst = EnterStructPointerForCoercedAccess(Dst, DstSTy,
- SrcSize.getFixedValue(), CGF);
-DstTy = Dst.getElementType();
- }
-
- llvm::PointerType *SrcPtrTy = llvm::dyn_cast(SrcTy);
- llvm::PointerType *DstPtrTy = llvm::dyn_cast(DstTy);
- if (SrcPtrTy && DstPtrTy &&
- SrcPtrTy->getAddressSpace() != DstPtrTy->getAddressSpace()) {
-Src = CGF.Builder.CreateAddrSpaceCast(Src, DstTy);
-CGF.Builder.CreateStore(Src, Dst, DstIsVolatile);
+void CodeGenFunction::CreateCoercedStore(llvm::Value *Src, Address Dst,
+ llvm::TypeSize DstSize,
+ bool DstIsVolatile) {
+ if (!DstSize)
return;
- }
- // If the source and destination are integer or pointer types, just do an
- // extension or truncation to the desired type.
- if ((isa(SrcTy) || isa(SrcTy)) &&
- (isa(DstTy) || isa(DstTy))) {
-Src = CoerceIntOrPtrToIntOrPtr(Src, DstTy, CGF);
-CGF.Builder.CreateStore(Src, Dst, DstIsVolatile);
-return;
+ llvm::Type *SrcTy = Src->getType();
+ llvm::TypeSize SrcSize = CGM.getDataLayout().getTypeAllocSize(SrcTy);
+
+ // GEP into structs to try to
[clang] Fix codegen of consteval functions returning an empty class. (PR #93115)
@@ -135,6 +135,17 @@ class AggExprEmitter : public StmtVisitor {
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
efriedma-quic wrote:
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.
https://github.com/llvm/llvm-project/pull/93115
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix codegen of consteval functions returning an empty class. (PR #93115)
@@ -135,6 +135,17 @@ class AggExprEmitter : public StmtVisitor {
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
serge-sans-paille wrote:
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?
https://github.com/llvm/llvm-project/pull/93115
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix codegen of consteval functions returning an empty class. (PR #93115)
efriedma-quic wrote:
I didn't think so at first glance... but yes, we do, in certain obscure cases:
```
#include
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; }
```
I'll look into reworking this.
https://github.com/llvm/llvm-project/pull/93115
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix codegen of consteval functions returning an empty class. (PR #93115)
https://github.com/zygoloid approved this pull request. Looks good. Do we also need to worry about overwriting tail padding here? https://github.com/llvm/llvm-project/pull/93115 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix codegen of consteval functions returning an empty class. (PR #93115)
llvmbot wrote:
@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 d60c3d08.)
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
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 {
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()) {
+ CXXRecordDecl *Record = cast(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
+}
+}
``
https://github.com/llvm/llvm-project/pull/93115
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix codegen of consteval functions returning an empty class. (PR #93115)
llvmbot wrote:
@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 d60c3d08.)
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
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 {
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()) {
+ CXXRecordDecl *Record = cast(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
+}
+}
``
https://github.com/llvm/llvm-project/pull/93115
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix codegen of consteval functions returning an empty class. (PR #93115)
https://github.com/efriedma-quic created
https://github.com/llvm/llvm-project/pull/93115
If a class is empty, don't store it to memory: the store might overwrite useful
data.
(See also d60c3d08.)
Fixes #93040.
>From bdfcc729c309fc5b1092b67d7c3c803c852ae251 Mon Sep 17 00:00:00 2001
From: Eli Friedman
Date: Wed, 22 May 2024 17:30:41 -0700
Subject: [PATCH] Fix codegen of consteval functions returning an empty class.
If a class is empty, don't store it to memory: the store might overwrite
useful data.
(See also d60c3d08.)
Fixes #93040.
---
clang/lib/CodeGen/CGExprAgg.cpp | 11 +++
clang/test/CodeGenCXX/cxx2a-consteval.cpp | 24 ++-
2 files changed, 34 insertions(+), 1 deletion(-)
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 {
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()) {
+ CXXRecordDecl *Record = cast(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
+}
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
