[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
Sat May 4 12:43:37 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/6] [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/6] 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/6] 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/6] 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/6] 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)

>From 54a10e011ed587fdee1fe27f28884b8c20750e1d Mon Sep 17 00:00:00 2001
From: MaxEW707 <max.enrico.winkler at gmail.com>
Date: Sat, 4 May 2024 15:43:19 -0400
Subject: [PATCH 6/6] Force rebuild due to msvc out of heap space




More information about the cfe-commits mailing list