dblaikie created this revision. dblaikie added reviewers: ayzhao, hansw. Herald added subscribers: mstorsjo, kristof.beyls. Herald added a project: All. dblaikie requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits.
Details posted here: https://reviews.llvm.org/D119051#3747201 3 cases that were inconsistent with the MSABI without this patch applied: https://godbolt.org/z/GY48qxh3G - field with protected member https://godbolt.org/z/Mb1PYhjrP - non-static data member initializer https://godbolt.org/z/sGvxcEPjo - defaulted copy constructor I'm not sure what's suitable/sufficient testing for this - I did verify the three cases above. Though if it helps to add them as explicit tests, I can do that too. Also, I was wondering if the other use of isTrivialForAArch64MSVC in isPermittedToBeHomogenousAggregate could be another source of bugs - I tried changing the function to unconditionally call isTrivialFor(AArch64)MSVC without testing AArch64 first, but no tests fail, so it looks like this is undertested in any case. But I had trouble figuring out how to exercise this functionality properly to add test coverage and then compare that to MSVC itself... - I got very confused/turned around trying to test this, so I've given up enough to send what I have out for review, but happy to look further into this with help. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D133817 Files: clang/lib/CodeGen/MicrosoftCXXABI.cpp clang/test/CodeGenCXX/microsoft-abi-sret-and-byval.cpp Index: clang/test/CodeGenCXX/microsoft-abi-sret-and-byval.cpp =================================================================== --- clang/test/CodeGenCXX/microsoft-abi-sret-and-byval.cpp +++ clang/test/CodeGenCXX/microsoft-abi-sret-and-byval.cpp @@ -74,6 +74,10 @@ int i; }; +struct SmallWithSmallWithPrivate { + SmallWithPrivate p; +}; + // WIN32: declare dso_local void @"{{.*take_bools_and_chars.*}}" // WIN32: (<{ i8, [3 x i8], i8, [3 x i8], %struct.SmallWithDtor, // WIN32: i8, [3 x i8], i8, [3 x i8], i32, i8, [3 x i8] }>* inalloca(<{ i8, [3 x i8], i8, [3 x i8], %struct.SmallWithDtor, i8, [3 x i8], i8, [3 x i8], i32, i8, [3 x i8] }>) @@ -197,6 +201,10 @@ // WOA64: define dso_local void @"?small_arg_with_private_member@@YA?AUSmallWithPrivate@@U1@@Z"(%struct.SmallWithPrivate* inreg noalias sret(%struct.SmallWithPrivate) align 4 %agg.result, i64 %s.coerce) {{.*}} { SmallWithPrivate small_arg_with_private_member(SmallWithPrivate s) { return s; } +// WOA64: define dso_local i32 @"?small_arg_with_small_struct_with_private_member@@YA?AUSmallWithSmallWithPrivate@@U1@@Z"(i64 %s.coerce) {{.*}} { +// WIN64: define dso_local i32 @"?small_arg_with_small_struct_with_private_member@@YA?AUSmallWithSmallWithPrivate@@U1@@Z"(i32 %s.coerce) {{.*}} { +SmallWithSmallWithPrivate small_arg_with_small_struct_with_private_member(SmallWithSmallWithPrivate s) { return s; } + void call_small_arg_with_dtor() { small_arg_with_dtor(SmallWithDtor()); } Index: clang/lib/CodeGen/MicrosoftCXXABI.cpp =================================================================== --- clang/lib/CodeGen/MicrosoftCXXABI.cpp +++ clang/lib/CodeGen/MicrosoftCXXABI.cpp @@ -1086,8 +1086,8 @@ return isDeletingDtor(GD); } -static bool isTrivialForAArch64MSVC(const CXXRecordDecl *RD) { - // For AArch64, we use the C++14 definition of an aggregate, so we also +static bool isTrivialForMSVC(const CXXRecordDecl *RD) { + // We use the C++14 definition of an aggregate, so we also // check for: // No private or protected non static data members. // No base classes @@ -1115,15 +1115,7 @@ if (!RD) return false; - // Normally, the C++ concept of "is trivially copyable" is used to determine - // if a struct can be returned directly. However, as MSVC and the language - // have evolved, the definition of "trivially copyable" has changed, while the - // ABI must remain stable. AArch64 uses the C++14 concept of an "aggregate", - // while other ISAs use the older concept of "plain old data". - bool isTrivialForABI = RD->isPOD(); - bool isAArch64 = CGM.getTarget().getTriple().isAArch64(); - if (isAArch64) - isTrivialForABI = RD->canPassInRegisters() && isTrivialForAArch64MSVC(RD); + bool isTrivialForABI = RD->canPassInRegisters() && isTrivialForMSVC(RD); // MSVC always returns structs indirectly from C++ instance methods. bool isIndirectReturn = !isTrivialForABI || FI.isInstanceMethod(); @@ -1137,7 +1129,7 @@ // On AArch64, use the `inreg` attribute if the object is considered to not // be trivially copyable, or if this is an instance method struct return. - FI.getReturnInfo().setInReg(isAArch64); + FI.getReturnInfo().setInReg(CGM.getTarget().getTriple().isAArch64()); return true; } @@ -4475,5 +4467,5 @@ // aggregate according to the C++14 spec. This is not consistent with the // AAPCS64, but is defacto spec on that platform. return !CGM.getTarget().getTriple().isAArch64() || - isTrivialForAArch64MSVC(CXXRD); + isTrivialForMSVC(CXXRD); }
Index: clang/test/CodeGenCXX/microsoft-abi-sret-and-byval.cpp =================================================================== --- clang/test/CodeGenCXX/microsoft-abi-sret-and-byval.cpp +++ clang/test/CodeGenCXX/microsoft-abi-sret-and-byval.cpp @@ -74,6 +74,10 @@ int i; }; +struct SmallWithSmallWithPrivate { + SmallWithPrivate p; +}; + // WIN32: declare dso_local void @"{{.*take_bools_and_chars.*}}" // WIN32: (<{ i8, [3 x i8], i8, [3 x i8], %struct.SmallWithDtor, // WIN32: i8, [3 x i8], i8, [3 x i8], i32, i8, [3 x i8] }>* inalloca(<{ i8, [3 x i8], i8, [3 x i8], %struct.SmallWithDtor, i8, [3 x i8], i8, [3 x i8], i32, i8, [3 x i8] }>) @@ -197,6 +201,10 @@ // WOA64: define dso_local void @"?small_arg_with_private_member@@YA?AUSmallWithPrivate@@U1@@Z"(%struct.SmallWithPrivate* inreg noalias sret(%struct.SmallWithPrivate) align 4 %agg.result, i64 %s.coerce) {{.*}} { SmallWithPrivate small_arg_with_private_member(SmallWithPrivate s) { return s; } +// WOA64: define dso_local i32 @"?small_arg_with_small_struct_with_private_member@@YA?AUSmallWithSmallWithPrivate@@U1@@Z"(i64 %s.coerce) {{.*}} { +// WIN64: define dso_local i32 @"?small_arg_with_small_struct_with_private_member@@YA?AUSmallWithSmallWithPrivate@@U1@@Z"(i32 %s.coerce) {{.*}} { +SmallWithSmallWithPrivate small_arg_with_small_struct_with_private_member(SmallWithSmallWithPrivate s) { return s; } + void call_small_arg_with_dtor() { small_arg_with_dtor(SmallWithDtor()); } Index: clang/lib/CodeGen/MicrosoftCXXABI.cpp =================================================================== --- clang/lib/CodeGen/MicrosoftCXXABI.cpp +++ clang/lib/CodeGen/MicrosoftCXXABI.cpp @@ -1086,8 +1086,8 @@ return isDeletingDtor(GD); } -static bool isTrivialForAArch64MSVC(const CXXRecordDecl *RD) { - // For AArch64, we use the C++14 definition of an aggregate, so we also +static bool isTrivialForMSVC(const CXXRecordDecl *RD) { + // We use the C++14 definition of an aggregate, so we also // check for: // No private or protected non static data members. // No base classes @@ -1115,15 +1115,7 @@ if (!RD) return false; - // Normally, the C++ concept of "is trivially copyable" is used to determine - // if a struct can be returned directly. However, as MSVC and the language - // have evolved, the definition of "trivially copyable" has changed, while the - // ABI must remain stable. AArch64 uses the C++14 concept of an "aggregate", - // while other ISAs use the older concept of "plain old data". - bool isTrivialForABI = RD->isPOD(); - bool isAArch64 = CGM.getTarget().getTriple().isAArch64(); - if (isAArch64) - isTrivialForABI = RD->canPassInRegisters() && isTrivialForAArch64MSVC(RD); + bool isTrivialForABI = RD->canPassInRegisters() && isTrivialForMSVC(RD); // MSVC always returns structs indirectly from C++ instance methods. bool isIndirectReturn = !isTrivialForABI || FI.isInstanceMethod(); @@ -1137,7 +1129,7 @@ // On AArch64, use the `inreg` attribute if the object is considered to not // be trivially copyable, or if this is an instance method struct return. - FI.getReturnInfo().setInReg(isAArch64); + FI.getReturnInfo().setInReg(CGM.getTarget().getTriple().isAArch64()); return true; } @@ -4475,5 +4467,5 @@ // aggregate according to the C++14 spec. This is not consistent with the // AAPCS64, but is defacto spec on that platform. return !CGM.getTarget().getTriple().isAArch64() || - isTrivialForAArch64MSVC(CXXRD); + isTrivialForMSVC(CXXRD); }
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits