MSVC ABI: Looks like even non-aarch64 uses the MSVC/14 definition for pod/aggregate passing

[dblaikie]

Summary

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.

[rnk]

Thanks!

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.

I think I would take the three cases you found (protected, nsdmi, defaulted copy ctor), add each to this file, and check for the IR prototypes here in this test file. Either sret is used, or it is not.

Regarding HFAs, I believe that logic is only used on x86/x64 for the __vectorcall calling convention. I believe it is well-tested with respect to C-like structs, but the C++ aspects that you are changing are not well tested. I think I managed to construct a case using NSDMIs (Richard used to prefer the terminology "default member initializers" which is simpler): https://godbolt.org/z/daPzKxj3h It looks like we don't match arm64 MSVC's behavior exactly here, but your change to remove the "!isAArch64" test would probably cause us to change behavior in this case, right?

clang/test/CodeGenCXX/microsoft-abi-sret-and-byval.cpp

77

I'm amused that this class is passed directly, but if you pass the field by itself, it is passed indirectly. :shrug:

[dblaikie]

Add the three test cases. Verified they failed without the patch and pass with it. Testing them across all the Windows ABI variants, since they seem to apply equally.

[dblaikie]

In D133817#3790057, @rnk wrote:

Thanks!

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.

I think I would take the three cases you found (protected, nsdmi, defaulted copy ctor), add each to this file, and check for the IR prototypes here in this test file. Either sret is used, or it is not.

OK, done that. Removed the other attempt I'd made at the protected-field-in-field test case in favor of the one at the end with the other two cases, since that seemed clearer.

Regarding HFAs, I believe that logic is only used on x86/x64 for the __vectorcall calling convention.

Oooh.. hmm, that should help. I'll look around.

I believe it is well-tested with respect to C-like structs, but the C++ aspects that you are changing are not well tested. I think I managed to construct a case using NSDMIs (Richard used to prefer the terminology "default member initializers" which is simpler): https://godbolt.org/z/daPzKxj3h It looks like we don't match arm64 MSVC's behavior exactly here, but your change to remove the "!isAArch64" test would probably cause us to change behavior in this case, right?

It would change the behavior, but I think in the wrong direction... making non-AArch64 behave like the buggy AArch64... hrm.

So I think https://godbolt.org/z/Md91eE8KM demonstrates that the homogenous aggregate rules are different for AArch64 from x86_64 - so we aren't going to/shouldn't lose the AArch64 check in isPermittedToBeHomogeneousAggregate.

But it's also true that the rules are different again from the isTrivialForMSVC - as per your test with "default member initializers", this doesn't seem to change the return ABI, but it does make the (only the AArch64, not the x86_64) consider the type non-homogenous-aggregate.

(even though the ways that homogenous aggregate rules are inconsistent with isTrivialForMSVC, isTrivialForMSVC does agree between x86 and AArch64... so it suggests that generalizing that function across x86 and AArch64 is correct, but if someone wants to fix the AArch64 vectorcall ABI, they will need to do so by introducing a separate function, isHomogenousAggregateForAArch64MSVC or the like)

So, I could leave a test case and some comments, or follow-up with a separate fix for the homogenous aggregate issue? I guess it comes down to writing that separate function to do the tests. (would be great to throw some of this at the ABI fuzzer @majnemer had back in the day, at least I think he had an ABI fuzzer of some sort, yeah?)

clang/test/CodeGenCXX/microsoft-abi-sret-and-byval.cpp

77

Yeah, isn't that... neat?

[dblaikie]

Ping on this.

(also sent out D134688 for the HFA stuff, which separates that from the "isTrivialFor" functionality entirely making it easier to make the different choices here)

[dblaikie]

Ping

[ayzhao]

Removing myself as a reviewer as I'm not really familiar with this area.

[rnk]

lgtm

In D133817#3794429, @dblaikie wrote:

So, I could leave a test case and some comments, or follow-up with a separate fix for the homogenous aggregate issue? I guess it comes down to writing that separate function to do the tests. (would be great to throw some of this at the ABI fuzzer @majnemer had back in the day, at least I think he had an ABI fuzzer of some sort, yeah?)

Right, we did have a record layout fuzzer which worked pretty well. I think it only ever ran on a workstation, it relied on custom tools and the like. I think it's lost to the sands of time.

[hans]

Right, we did have a record layout fuzzer which worked pretty well. I think it only ever ran on a workstation, it relied on custom tools and the like. I think it's lost to the sands of time.

(The code still exists: https://github.com/dberlin/superfuzz I've never run it though, so I don't know how much work that would take.)

clang/lib/CodeGen/MicrosoftCXXABI.cpp

static bool isDeletingDtor(GlobalDecl GD) {
   return isa<CXXDestructorDecl>(GD.getDecl()) &&
          GD.getDtorType() == Dtor_Deleting;
 }
 
 bool MicrosoftCXXABI::hasMostDerivedReturn(GlobalDecl GD) const {
   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
   //   No virtual functions
   // Additionally, we need to ensure that there is a trivial copy assignment
   // operator, a trivial destructor and no user-provided constructors.
   if (RD->hasProtectedFields() || RD->hasPrivateFields())
     return false;
static bool isTrivialForMSVC(const CXXRecordDecl *RD) {
   return true;
 }
 
 bool MicrosoftCXXABI::classifyReturnType(CGFunctionInfo &FI) const {
   const CXXRecordDecl *RD = FI.getReturnType()->getAsCXXRecordDecl();
   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();
 
   if (isIndirectReturn) {
     CharUnits Align = CGM.getContext().getTypeAlignInChars(FI.getReturnType());
     FI.getReturnInfo() = ABIArgInfo::getIndirect(Align, /*ByVal=*/false);
 
     // MSVC always passes `this` before the `sret` parameter.
     FI.getReturnInfo().setSRetAfterThis(FI.isInstanceMethod());
 
     // 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;
   }
 
   // Otherwise, use the C ABI rules.
   return false;
 }
 

clang/test/CodeGenCXX/microsoft-abi-sret-and-byval.cpp

 // RUN: %clang_cc1 -no-opaque-pointers -std=c++11 -emit-llvm %s -o - -triple=i386-pc-linux | FileCheck -check-prefix LINUX %s
-// RUN: %clang_cc1 -no-opaque-pointers -std=c++11 -emit-llvm %s -o - -triple=i386-pc-win32 -mconstructor-aliases -fno-rtti | FileCheck -check-prefix WIN32 %s
-// RUN: %clang_cc1 -no-opaque-pointers -std=c++11 -emit-llvm %s -o - -triple=thumb-pc-win32 -mconstructor-aliases -fno-rtti | FileCheck -check-prefix WOA %s
-// RUN: %clang_cc1 -no-opaque-pointers -std=c++11 -emit-llvm %s -o - -triple=x86_64-pc-win32 -mconstructor-aliases -fno-rtti | FileCheck -check-prefix WIN64 %s
-// RUN: %clang_cc1 -no-opaque-pointers -std=c++11 -emit-llvm %s -o - -triple=aarch64-windows-msvc -mconstructor-aliases -fno-rtti | FileCheck -check-prefix WOA64 %s
+// RUN: %clang_cc1 -no-opaque-pointers -std=c++11 -emit-llvm %s -o - -triple=i386-pc-win32 -mconstructor-aliases -fno-rtti | FileCheck -check-prefix WIN32 --check-prefix WIN %s
+// RUN: %clang_cc1 -no-opaque-pointers -std=c++11 -emit-llvm %s -o - -triple=thumb-pc-win32 -mconstructor-aliases -fno-rtti | FileCheck -check-prefix WOA --check-prefix WIN %s
+// RUN: %clang_cc1 -no-opaque-pointers -std=c++11 -emit-llvm %s -o - -triple=x86_64-pc-win32 -mconstructor-aliases -fno-rtti | FileCheck -check-prefix WIN64 --check-prefix WIN %s
+// RUN: %clang_cc1 -no-opaque-pointers -std=c++11 -emit-llvm %s -o - -triple=aarch64-windows-msvc -mconstructor-aliases -fno-rtti | FileCheck -check-prefix WOA64 --check-prefix WIN %s
 
 struct Empty {};
 
 struct EmptyWithCtor {
   EmptyWithCtor() {}
 };
 
 struct Small {
struct BaseNoByval : Small {
   int bb;
 };
 
 struct SmallWithPrivate {
 private:
  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] }>)
 void take_bools_and_chars(char a, char b, SmallWithDtor c, char d, bool e, int f, bool g);
 void call_bools_and_chars() {
   take_bools_and_chars('A', 'B', SmallWithDtor(), 'D', true, 13, false);
 }
 

 // WOA: }
 
 
 // Test that the eligible non-aggregate is passed directly, but returned
 // indirectly on ARM64 Windows.
 // 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());
 }
 // WIN64-LABEL: define dso_local void @"?call_small_arg_with_dtor@@YAXXZ"()
 // WIN64:   call noundef %struct.SmallWithDtor* @"??0SmallWithDtor@@QEAA@XZ"
 // WIN64:   call void @"?small_arg_with_dtor@@YAXUSmallWithDtor@@@Z"(i32 %{{.*}})
 // WIN64:   ret void
 

 // WIN32-LABEL: define dso_local x86_thiscallcc void @"?g@C@pr30293@@QAEXXZ"(%"struct.pr30293::C"* {{[^,]*}} %this)
 // WIN32: call x86_thiscallcc noundef %struct.SmallWithDtor* @"??0SmallWithDtor@@QAE@XZ"
 // WIN32: call void @"?h@C@pr30293@@UAAXUSmallWithDtor@@@Z"(<{ i8*, %struct.SmallWithDtor }>* inalloca(<{ i8*, %struct.SmallWithDtor }>) %{{[^,)]*}})
 // WIN32: declare dso_local void @"?h@C@pr30293@@UAAXUSmallWithDtor@@@Z"(<{ i8*, %struct.SmallWithDtor }>* inalloca(<{ i8*, %struct.SmallWithDtor }>))
 
 // WIN64-LABEL: define dso_local void @"?g@C@pr30293@@QEAAXXZ"(%"struct.pr30293::C"* {{[^,]*}} %this)
 // WIN64: declare dso_local void @"?h@C@pr30293@@UEAAXUSmallWithDtor@@@Z"(i8* noundef, i32)
 }
+
+namespace protected_member_of_member {
+struct field { protected: int i; };
+struct t1 {
+  field f;
+};
+extern const t1& v1;
+t1 f1() { return v1; }
+// WIN: define dso_local {{.*}}i32 @"?f1@protected_member_of_member@@YA?AUt1@1@XZ"()
+}
+
+namespace default_member_initializer {
+struct t1 {
+  int i = 3;
+};
+extern const t1& v1;
+t1 f1() { return v1; }
+// WIN: define dso_local {{.*}}i32 @"?f1@default_member_initializer@@YA?AUt1@1@XZ"()
+}
+
+namespace defaulted_copy_ctor {
+struct t1 {
+  int i;
+  t1(const t1&) = default;
+};
+extern const t1& v1;
+t1 f1() { return v1; }
+// WIN: define dso_local {{.*}}i32 @"?f1@defaulted_copy_ctor@@YA?AUt1@1@XZ"()
+}