[clang] [clang][CodeGen] Fix MSVC ABI for classes with a deleted copy assignment operator (PR #90547)
Max Winkler via cfe-commits
cfe-commits at lists.llvm.org
Fri May 3 19:51:07 PDT 2024
https://github.com/MaxEW707 updated https://github.com/llvm/llvm-project/pull/90547
>From f404db44d3770cdb8ac5123c16c0b04314eda698 Mon Sep 17 00:00:00 2001
From: MaxEW707 <max.enrico.winkler at gmail.com>
Date: Mon, 29 Apr 2024 22:09:52 -0400
Subject: [PATCH 1/5] [clang][CodeGen] Fix MS ABI for classes with non static
data members of reference type
---
clang/docs/ReleaseNotes.rst | 3 +
clang/lib/CodeGen/MicrosoftCXXABI.cpp | 37 +++++++++--
.../test/CodeGen/x64-microsoft-arguments.cpp | 64 +++++++++++++++++++
3 files changed, 98 insertions(+), 6 deletions(-)
create mode 100644 clang/test/CodeGen/x64-microsoft-arguments.cpp
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index c11f117cd6e8b4..1e3b1111bbc454 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -69,6 +69,9 @@ ABI Changes in This Version
returning a class in a register. This affects some uses of std::pair.
(#GH86384).
+- Fixed Microsoft calling convention when returning classes that have a reference type
+ as a field. Such a class should be returned indirectly.
+
AST Dumping Potentially Breaking Changes
----------------------------------------
diff --git a/clang/lib/CodeGen/MicrosoftCXXABI.cpp b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
index d47927745759e1..5564ab05e0866b 100644
--- a/clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -1103,8 +1103,27 @@ bool MicrosoftCXXABI::hasMostDerivedReturn(GlobalDecl GD) const {
return isDeletingDtor(GD);
}
+static bool fieldIsTrivialForMSVC(const FieldDecl *Field,
+ const ASTContext &Context) {
+ if (Field->getType()->isReferenceType())
+ return false;
+
+ const RecordType *RT =
+ Context.getBaseElementType(Field->getType())->getAs<RecordType>();
+ if (!RT)
+ return true;
+
+ CXXRecordDecl *RD = cast<CXXRecordDecl>(RT->getDecl());
+
+ for (const FieldDecl *RDField : RD->fields())
+ if (!fieldIsTrivialForMSVC(RDField, Context))
+ return false;
+
+ return true;
+}
+
static bool isTrivialForMSVC(const CXXRecordDecl *RD, QualType Ty,
- CodeGenModule &CGM) {
+ CodeGenModule &CGM, const ASTContext &Context) {
// On AArch64, HVAs that can be passed in registers can also be returned
// in registers. (Note this is using the MSVC definition of an HVA; see
// isPermittedToBeHomogeneousAggregate().)
@@ -1122,7 +1141,8 @@ static bool isTrivialForMSVC(const CXXRecordDecl *RD, QualType Ty,
// No base classes
// No virtual functions
// Additionally, we need to ensure that there is a trivial copy assignment
- // operator, a trivial destructor and no user-provided constructors.
+ // operator, a trivial destructor, no user-provided constructors and no
+ // non static data members of reference type.
if (RD->hasProtectedFields() || RD->hasPrivateFields())
return false;
if (RD->getNumBases() > 0)
@@ -1142,6 +1162,9 @@ static bool isTrivialForMSVC(const CXXRecordDecl *RD, QualType Ty,
}
if (RD->hasNonTrivialDestructor())
return false;
+ for (const FieldDecl *Field : RD->fields())
+ if (!fieldIsTrivialForMSVC(Field, Context))
+ return false;
return true;
}
@@ -1150,11 +1173,13 @@ bool MicrosoftCXXABI::classifyReturnType(CGFunctionInfo &FI) const {
if (!RD)
return false;
- bool isTrivialForABI = RD->canPassInRegisters() &&
- isTrivialForMSVC(RD, FI.getReturnType(), CGM);
-
// MSVC always returns structs indirectly from C++ instance methods.
- bool isIndirectReturn = !isTrivialForABI || FI.isInstanceMethod();
+ bool isIndirectReturn = FI.isInstanceMethod();
+ if (!isIndirectReturn) {
+ isIndirectReturn =
+ !(RD->canPassInRegisters() &&
+ isTrivialForMSVC(RD, FI.getReturnType(), CGM, getContext()));
+ }
if (isIndirectReturn) {
CharUnits Align = CGM.getContext().getTypeAlignInChars(FI.getReturnType());
diff --git a/clang/test/CodeGen/x64-microsoft-arguments.cpp b/clang/test/CodeGen/x64-microsoft-arguments.cpp
new file mode 100644
index 00000000000000..9e89b59924ae49
--- /dev/null
+++ b/clang/test/CodeGen/x64-microsoft-arguments.cpp
@@ -0,0 +1,64 @@
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -ffreestanding -emit-llvm -O0 \
+// RUN: -x c++ -o - %s | FileCheck %s
+
+int global_i = 0;
+
+// Pass and return object with a reference type (pass directly, return indirectly).
+// CHECK: define dso_local void @"?f1@@YA?AUS1@@XZ"(ptr dead_on_unwind noalias writable sret(%struct.S1) align 8 {{.*}})
+// CHECK: call void @"?func1@@YA?AUS1@@U1@@Z"(ptr dead_on_unwind writable sret(%struct.S1) align 8 {{.*}}, i64 {{.*}})
+struct S1 {
+ int& r;
+};
+
+S1 func1(S1 x);
+S1 f1() {
+ S1 x{ global_i };
+ return func1(x);
+}
+
+// Pass and return object with a reference type within an inner struct (pass directly, return indirectly).
+// CHECK: define dso_local void @"?f2@@YA?AUS2@@XZ"(ptr dead_on_unwind noalias writable sret(%struct.S2) align 8 {{.*}})
+// CHECK: call void @"?func2@@YA?AUS2@@U1@@Z"(ptr dead_on_unwind writable sret(%struct.S2) align 8 {{.*}}, i64 {{.*}})
+struct Inner {
+ int& r;
+};
+
+struct S2 {
+ Inner i;
+};
+
+S2 func2(S2 x);
+S2 f2() {
+ S2 x{ { global_i } };
+ return func2(x);
+}
+
+// Pass and return object with a reference type (pass directly, return indirectly).
+// CHECK: define dso_local void @"?f3@@YA?AUS3@@XZ"(ptr dead_on_unwind noalias writable sret(%struct.S3) align 8 {{.*}})
+// CHECK: call void @"?func3@@YA?AUS3@@U1@@Z"(ptr dead_on_unwind writable sret(%struct.S3) align 8 {{.*}}, i64 {{.*}})
+struct S3 {
+ const int& r;
+};
+
+S3 func3(S3 x);
+S3 f3() {
+ S3 x{ global_i };
+ return func3(x);
+}
+
+// Pass and return object with a reference type within an inner struct (pass directly, return indirectly).
+// CHECK: define dso_local void @"?f4@@YA?AUS4@@XZ"(ptr dead_on_unwind noalias writable sret(%struct.S4) align 8 {{.*}})
+// CHECK: call void @"?func4@@YA?AUS4@@U1@@Z"(ptr dead_on_unwind writable sret(%struct.S4) align 8 {{.*}}, i64 {{.*}})
+struct InnerConst {
+ const int& r;
+};
+
+struct S4 {
+ InnerConst i;
+};
+
+S4 func4(S4 x);
+S4 f4() {
+ S4 x{ { global_i } };
+ return func4(x);
+}
>From 386b4fe18f5228fcbbabc8683651675f0bd615e0 Mon Sep 17 00:00:00 2001
From: MaxEW707 <max.enrico.winkler at gmail.com>
Date: Tue, 30 Apr 2024 22:05:33 -0400
Subject: [PATCH 2/5] revert change
---
clang/lib/CodeGen/MicrosoftCXXABI.cpp | 37 +++++----------------------
1 file changed, 6 insertions(+), 31 deletions(-)
diff --git a/clang/lib/CodeGen/MicrosoftCXXABI.cpp b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
index 5564ab05e0866b..d47927745759e1 100644
--- a/clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -1103,27 +1103,8 @@ bool MicrosoftCXXABI::hasMostDerivedReturn(GlobalDecl GD) const {
return isDeletingDtor(GD);
}
-static bool fieldIsTrivialForMSVC(const FieldDecl *Field,
- const ASTContext &Context) {
- if (Field->getType()->isReferenceType())
- return false;
-
- const RecordType *RT =
- Context.getBaseElementType(Field->getType())->getAs<RecordType>();
- if (!RT)
- return true;
-
- CXXRecordDecl *RD = cast<CXXRecordDecl>(RT->getDecl());
-
- for (const FieldDecl *RDField : RD->fields())
- if (!fieldIsTrivialForMSVC(RDField, Context))
- return false;
-
- return true;
-}
-
static bool isTrivialForMSVC(const CXXRecordDecl *RD, QualType Ty,
- CodeGenModule &CGM, const ASTContext &Context) {
+ CodeGenModule &CGM) {
// On AArch64, HVAs that can be passed in registers can also be returned
// in registers. (Note this is using the MSVC definition of an HVA; see
// isPermittedToBeHomogeneousAggregate().)
@@ -1141,8 +1122,7 @@ static bool isTrivialForMSVC(const CXXRecordDecl *RD, QualType Ty,
// No base classes
// No virtual functions
// Additionally, we need to ensure that there is a trivial copy assignment
- // operator, a trivial destructor, no user-provided constructors and no
- // non static data members of reference type.
+ // operator, a trivial destructor and no user-provided constructors.
if (RD->hasProtectedFields() || RD->hasPrivateFields())
return false;
if (RD->getNumBases() > 0)
@@ -1162,9 +1142,6 @@ static bool isTrivialForMSVC(const CXXRecordDecl *RD, QualType Ty,
}
if (RD->hasNonTrivialDestructor())
return false;
- for (const FieldDecl *Field : RD->fields())
- if (!fieldIsTrivialForMSVC(Field, Context))
- return false;
return true;
}
@@ -1173,13 +1150,11 @@ bool MicrosoftCXXABI::classifyReturnType(CGFunctionInfo &FI) const {
if (!RD)
return false;
+ bool isTrivialForABI = RD->canPassInRegisters() &&
+ isTrivialForMSVC(RD, FI.getReturnType(), CGM);
+
// MSVC always returns structs indirectly from C++ instance methods.
- bool isIndirectReturn = FI.isInstanceMethod();
- if (!isIndirectReturn) {
- isIndirectReturn =
- !(RD->canPassInRegisters() &&
- isTrivialForMSVC(RD, FI.getReturnType(), CGM, getContext()));
- }
+ bool isIndirectReturn = !isTrivialForABI || FI.isInstanceMethod();
if (isIndirectReturn) {
CharUnits Align = CGM.getContext().getTypeAlignInChars(FI.getReturnType());
>From b3db2da6fe429691d6145d40f8b86d4bc7e4415e Mon Sep 17 00:00:00 2001
From: MaxEW707 <max.enrico.winkler at gmail.com>
Date: Wed, 1 May 2024 22:53:37 -0400
Subject: [PATCH 3/5] PR Feedback on checking for deleted copy assignment
operator
---
clang/lib/CodeGen/MicrosoftCXXABI.cpp | 5 ++++
.../test/CodeGen/x64-microsoft-arguments.cpp | 28 +++++++++++++++++++
2 files changed, 33 insertions(+)
diff --git a/clang/lib/CodeGen/MicrosoftCXXABI.cpp b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
index d47927745759e1..6076047a3c19fa 100644
--- a/clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -1131,6 +1131,8 @@ static bool isTrivialForMSVC(const CXXRecordDecl *RD, QualType Ty,
return false;
if (RD->hasNonTrivialCopyAssignment())
return false;
+ if (RD->needsImplicitCopyAssignment() && !RD->hasSimpleCopyAssignment())
+ return false;
for (const Decl *D : RD->decls()) {
if (auto *Ctor = dyn_cast<CXXConstructorDecl>(D)) {
if (Ctor->isUserProvided())
@@ -1138,6 +1140,9 @@ static bool isTrivialForMSVC(const CXXRecordDecl *RD, QualType Ty,
} else if (auto *Template = dyn_cast<FunctionTemplateDecl>(D)) {
if (isa<CXXConstructorDecl>(Template->getTemplatedDecl()))
return false;
+ } else if (auto *MethodDecl = dyn_cast<CXXMethodDecl>(D)) {
+ if (MethodDecl->isCopyAssignmentOperator() && MethodDecl->isDeleted())
+ return false;
}
}
if (RD->hasNonTrivialDestructor())
diff --git a/clang/test/CodeGen/x64-microsoft-arguments.cpp b/clang/test/CodeGen/x64-microsoft-arguments.cpp
index 9e89b59924ae49..c666c92ad2db25 100644
--- a/clang/test/CodeGen/x64-microsoft-arguments.cpp
+++ b/clang/test/CodeGen/x64-microsoft-arguments.cpp
@@ -62,3 +62,31 @@ S4 f4() {
S4 x{ { global_i } };
return func4(x);
}
+
+// Pass and return an object with an explicitly deleted copy assignment operator (pass directly, return indirectly).
+// CHECK: define dso_local void @"?f5@@YA?AUS5@@XZ"(ptr dead_on_unwind noalias writable sret(%struct.S5) align 4 {{.*}})
+// CHECK: call void @"?func5@@YA?AUS5@@U1@@Z"(ptr dead_on_unwind writable sret(%struct.S5) align 4 {{.*}}, i32 {{.*}})
+struct S5 {
+ S5& operator=(const S5&) = delete;
+ int i;
+};
+
+S5 func5(S5 x);
+S5 f5() {
+ S5 x{ 1 };
+ return func5(x);
+}
+
+// Pass and return an object with an explicitly defaulted copy assignment operator that is implicitly deleted (pass directly, return indirectly).
+// CHECK: define dso_local void @"?f6@@YA?AUS6@@XZ"(ptr dead_on_unwind noalias writable sret(%struct.S6) align 8 {{.*}})
+// CHECK: call void @"?func6@@YA?AUS6@@U1@@Z"(ptr dead_on_unwind writable sret(%struct.S6) align 8 {{.*}}, i64 {{.*}})
+struct S6 {
+ S6& operator=(const S6&) = default;
+ int& i;
+};
+
+S6 func6(S6 x);
+S6 f6() {
+ S6 x{ global_i };
+ return func6(x);
+}
>From f3510448c96235f689cd9a174999efa9435d8a27 Mon Sep 17 00:00:00 2001
From: MaxEW707 <max.enrico.winkler at gmail.com>
Date: Thu, 2 May 2024 21:28:48 -0400
Subject: [PATCH 4/5] Update comment and release notes
---
clang/docs/ReleaseNotes.rst | 4 ++--
clang/lib/CodeGen/MicrosoftCXXABI.cpp | 3 ++-
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 1e3b1111bbc454..616115d11e56f7 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -69,8 +69,8 @@ ABI Changes in This Version
returning a class in a register. This affects some uses of std::pair.
(#GH86384).
-- Fixed Microsoft calling convention when returning classes that have a reference type
- as a field. Such a class should be returned indirectly.
+- Fixed Microsoft calling convention when returning classes that have a deleted
+ copy assignment operator. Such a class should be returned indirectly.
AST Dumping Potentially Breaking Changes
----------------------------------------
diff --git a/clang/lib/CodeGen/MicrosoftCXXABI.cpp b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
index 6076047a3c19fa..92e83a893fcea9 100644
--- a/clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -1122,7 +1122,8 @@ static bool isTrivialForMSVC(const CXXRecordDecl *RD, QualType Ty,
// No base classes
// No virtual functions
// Additionally, we need to ensure that there is a trivial copy assignment
- // operator, a trivial destructor and no user-provided constructors.
+ // operator, a trivial destructor, no user-provided constructors and no
+ // deleted copy assignment operator.
if (RD->hasProtectedFields() || RD->hasPrivateFields())
return false;
if (RD->getNumBases() > 0)
>From 588fcb03d0f4652b6af5c80ab39261664172a15b Mon Sep 17 00:00:00 2001
From: MaxEW707 <max.enrico.winkler at gmail.com>
Date: Fri, 3 May 2024 22:50:49 -0400
Subject: [PATCH 5/5] PR Feedback on comment describing the logic
---
clang/lib/CodeGen/MicrosoftCXXABI.cpp | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/clang/lib/CodeGen/MicrosoftCXXABI.cpp b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
index 92e83a893fcea9..e4f798f6a97d97 100644
--- a/clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -1124,6 +1124,20 @@ static bool isTrivialForMSVC(const CXXRecordDecl *RD, QualType Ty,
// Additionally, we need to ensure that there is a trivial copy assignment
// operator, a trivial destructor, no user-provided constructors and no
// deleted copy assignment operator.
+
+ // We need to cover two cases when checking for a deleted copy assignment
+ // operator.
+ //
+ // struct S { int& r; };
+ // The above will have an implicit copy assignment operator that is deleted
+ // and there will not be a `CXXMethodDecl` for the copy assignment operator.
+ // This is handled by the `needsImplicitCopyAssignment()` check below.
+ //
+ // struct S { S& operator=(const S&) = delete; int i; };
+ // The above will not have an implicit copy assignment operator that is
+ // deleted but there is a deleted `CXXMethodDecl` for the declared copy
+ // assignment operator. This is handled by the `isDeleted()` check below.
+
if (RD->hasProtectedFields() || RD->hasPrivateFields())
return false;
if (RD->getNumBases() > 0)
More information about the cfe-commits
mailing list