[clang][CodeGen] Fix MSVC ABI for classes with a deleted copy assignment operator #90547

MaxEW707 · 2024-04-30T02:38:54Z

https://godbolt.org/z/verKj4cnj for reference.
https://godbolt.org/z/z3W9v7o4n for reference.

For global functions and static methods the MSVC ABI returns structs/classes with a reference type non static data member indirectly.
From local testing this is recursively applied to any inner structs/classes.
From local testing this ABI holds true for all currently support architectures including ARM64EC.

I tested locally against MSVC 1939 and MSVC 1929.

The only reference I could find on MSDN to this ABI constraint is this quote, "and no non-static data members of reference type", here.

Pointer fields do not exhibit this behaviour.

llvmbot · 2024-04-30T02:39:25Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Max Winkler (MaxEW707)

Changes

https://godbolt.org/z/verKj4cnj for reference.
https://godbolt.org/z/z3W9v7o4n for reference.

For global functions and static methods the MSVC ABI returns structs/classes with a reference type non static data member indirectly.
From local testing this is recursively applied to any inner structs/classes.
From local testing this ABI holds true for all currently support architectures including ARM64EC.

I tested locally against MSVC 1939 and MSVC 1929.

The only reference I could find on MSDN to this ABI constraint is this quote, "and no non-static data members of reference type", here.

Pointer fields do not exhibit this behaviour.

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

3 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+3)
(modified) clang/lib/CodeGen/MicrosoftCXXABI.cpp (+31-6)
(added) clang/test/CodeGen/x64-microsoft-arguments.cpp (+64)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 4aedfafcb26aea..8f228bb4d296e0 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -63,6 +63,9 @@ ABI Changes in This Version
   MSVC uses a different mangling for these objects, compatibility is not affected.
   (#GH85423).
 
+- 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 d38a26940a3cb6..b0798550ba9775 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)
@@ -1136,6 +1156,9 @@ static bool isTrivialForMSVC(const CXXRecordDecl *RD, QualType Ty,
       return false;
   if (RD->hasNonTrivialDestructor())
     return false;
+  for (const FieldDecl *Field : RD->fields())
+    if (!fieldIsTrivialForMSVC(Field, Context))
+      return false;
   return true;
 }
 
@@ -1144,11 +1167,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);
+}

MaxEW707 · 2024-04-30T02:40:32Z

clang/lib/CodeGen/MicrosoftCXXABI.cpp

  // MSVC always returns structs indirectly from C++ instance methods.
-  bool isIndirectReturn = !isTrivialForABI || FI.isInstanceMethod();
+  bool isIndirectReturn = FI.isInstanceMethod();


I intentionally moved this around to try to reduce calling isTrivialForMSVC for methods and potentially now hitting the recursive field loop I added above.
Since methods always return indirectly we can avoid all these extra checks up front.

Let me know if you prefer this being a separate PR.

I'd like to keep ABI changes as simple as possible; please separate.

efriedma-quic

Maybe we should just be testing if the copy-assignment operator is deleted? For reference, MSVC also returns the following indirectly:

struct Test
{
    int x;
    Test& operator=(const Test&) = delete;
};
Test foo();
Test test(Test x)
{
  return x;
}

efriedma-quic · 2024-04-30T19:00:51Z

clang/lib/CodeGen/MicrosoftCXXABI.cpp

  // MSVC always returns structs indirectly from C++ instance methods.
-  bool isIndirectReturn = !isTrivialForABI || FI.isInstanceMethod();
+  bool isIndirectReturn = FI.isInstanceMethod();


I'd like to keep ABI changes as simple as possible; please separate.

MaxEW707 · 2024-05-01T00:03:01Z

Maybe we should just be testing if the copy-assignment operator is deleted? For reference, MSVC also returns the following indirectly:
struct Test
{
    int x;
    Test& operator=(const Test&) = delete;
};
Test foo();
Test test(Test x)
{
  return x;
}

Argh, I didn't even consider trying an explicitly deleted copy assignment operator.
That should have been obvious considering the copy assignment operator is implicitly deleted since we have a non-static data member of a reference type.

I'll give this a try.

…of reference type

MaxEW707 · 2024-05-03T01:33:58Z

clang/lib/CodeGen/MicrosoftCXXABI.cpp

@@ -1131,13 +1132,18 @@ static bool isTrivialForMSVC(const CXXRecordDecl *RD, QualType Ty,
    return false;
  if (RD->hasNonTrivialCopyAssignment())
    return false;
+  if (RD->needsImplicitCopyAssignment() && !RD->hasSimpleCopyAssignment())


I didn't find a defaultedCopyAssignmentIsDeleted() method on CXXRecordDecl however hasSimpleCopyAssignment() does what we want here when combined with needsImplicitCopyAssignment().

Let me know if there is a better check to capture implicitly declared but deleted copy assignment operators on the record.

If I'm understanding correctly, this is basically split into two parts:

If we implicitly declared the copy-assignment, just query isDeleted() on it directly.

If we didn't implicitly declare the copy-assignment, check whether it would have been deleted. (We can't use this in cases where the copy-assignment was declared because the information isn't stored in the CXXRecordDecl.)

This is kind of complicated, but I can't think of a simpler way to do it. That said, it probably deserves a comment to explain the logic.

Correct. The two main cases we need to cover are the following.
I'll make a comment to explain the logic.

struct Test { Test& operator=(const Test&) = delete; int x; };

In this case DefaultedCopyAssignmentIsDeleted is false on the CXXRecordDecl. So we need to query isDeleted on the copy assignment operator CXXMethodDecl.

struct Test { int& x; };

In this case we have no CXXMethodDecl for the copy assignment operator so we need to check if DefaultedCopyAssignmentIsDeleted is true on the CXXRecordDecl.

efriedma-quic

LGTM

MaxEW707 requested review from rnk, cor3ntin and efriedma-quic April 30, 2024 02:38

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

MaxEW707 commented Apr 30, 2024

View reviewed changes

efriedma-quic reviewed Apr 30, 2024

View reviewed changes

MaxEW707 added 4 commits April 30, 2024 22:08

[clang][CodeGen] Fix MS ABI for classes with non static data members …

f404db4

…of reference type

revert change

386b4fe

PR Feedback on checking for deleted copy assignment operator

b3db2da

Update comment and release notes

f351044

MaxEW707 force-pushed the mwinkler/msvc-abi-reference-nsdm-return-indirect branch from cafb799 to f351044 Compare May 3, 2024 01:30

MaxEW707 commented May 3, 2024

View reviewed changes

MaxEW707 changed the title ~~[clang][CodeGen] Fix MSVC ABI for classes with non static data members of reference type~~ [clang][CodeGen] Fix MSVC ABI for classes with a deleted copy assignment operator May 3, 2024

PR Feedback on comment describing the logic

588fcb0

efriedma-quic approved these changes May 4, 2024

View reviewed changes

Force rebuild due to msvc out of heap space

54a10e0

MaxEW707 merged commit 3f37397 into llvm:main May 7, 2024

MaxEW707 deleted the mwinkler/msvc-abi-reference-nsdm-return-indirect branch May 7, 2024 23:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang][CodeGen] Fix MSVC ABI for classes with a deleted copy assignment operator #90547

[clang][CodeGen] Fix MSVC ABI for classes with a deleted copy assignment operator #90547

Uh oh!

MaxEW707 commented Apr 30, 2024

Uh oh!

llvmbot commented Apr 30, 2024 •

edited

Loading

Uh oh!

MaxEW707 Apr 30, 2024

Uh oh!

efriedma-quic Apr 30, 2024

Uh oh!

efriedma-quic left a comment

Uh oh!

efriedma-quic Apr 30, 2024

Uh oh!

MaxEW707 commented May 1, 2024 •

edited

Loading

Uh oh!

MaxEW707 May 3, 2024

Uh oh!

efriedma-quic May 3, 2024

Uh oh!

MaxEW707 May 4, 2024

Uh oh!

efriedma-quic left a comment

Uh oh!

Uh oh!

[clang][CodeGen] Fix MSVC ABI for classes with a deleted copy assignment operator #90547

[clang][CodeGen] Fix MSVC ABI for classes with a deleted copy assignment operator #90547

Uh oh!

Conversation

MaxEW707 commented Apr 30, 2024

Uh oh!

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

Uh oh!

Uh oh!

MaxEW707 Apr 30, 2024

Choose a reason for hiding this comment

Uh oh!

efriedma-quic Apr 30, 2024

Choose a reason for hiding this comment

Uh oh!

efriedma-quic left a comment

Choose a reason for hiding this comment

Uh oh!

efriedma-quic Apr 30, 2024

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

MaxEW707 May 3, 2024

Choose a reason for hiding this comment

Uh oh!

efriedma-quic May 3, 2024

Choose a reason for hiding this comment

Uh oh!

MaxEW707 May 4, 2024

Choose a reason for hiding this comment

Uh oh!

efriedma-quic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llvmbot commented Apr 30, 2024 •

edited

Loading

MaxEW707 commented May 1, 2024 •

edited

Loading