This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by dblaikie on Sep 13 2022, 4:41 PM.

Details

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.

Diff Detail

Event Timeline

dblaikie created this revision.Sep 13 2022, 4:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 13 2022, 4:41 PM
dblaikie requested review of this revision.Sep 13 2022, 4:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 13 2022, 4:42 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
rnk added a comment.Sep 14 2022, 10:43 AM

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 updated this revision to Diff 460637.Sep 15 2022, 10:38 PM
dblaikie marked an inline comment as done.

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.

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?

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)

ayzhao resigned from this revision.Oct 3 2022, 2:45 PM

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

rnk added a comment.Oct 3 2022, 3:16 PM

lgtm

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 added a subscriber: hans.Oct 4 2022, 12:52 AM

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.)

This revision was not accepted when it landed; it landed in state Needs Review.Oct 4 2022, 1:27 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.