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.wink...@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.wink...@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.wink...@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.wink...@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.wink...@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) _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits