[PATCH] D60349: [COFF, ARM64] Fix ABI implementation of struct returns

[Richard Townsend (Arm) via Phabricator via cfe-commits]

richard.townsend.arm added inline comments.


================
Comment at: lib/Sema/SemaDeclCXX.cpp:5908
+    // The checks below ensure that thare no user provided constructors.
+    // For AArch64, we use the C++14 definition of an aggregate, so we also
+    // check for:
----------------
I think these checks are in the wrong place - the aggregate restriction applies to return values, but it doesn't apply to arguments. Placing this logic here breaks argument passing between MSVC and Clang, because Clang (with this logic in place) will pass and expect arguments with an extra layer of indirection: for example, when passing https://cs.chromium.org/chromium/src/v8/include/v8.h?q=v8::Local&sq=package:chromium&g=0&l=183 v8::Local, what you actually see in the register bank is supposed to be its only private member. Moving the logic to MicrosoftCXXABI and applying it only to the return value resolves this problem.


================
Comment at: lib/Sema/SemaDeclCXX.cpp:5916
+    if (isAArch64) {
+      if (D->getAccess() == AS_private || D->getAccess() == AS_protected)
+        return false;
----------------
efriedma wrote:
> D->getAccess() is the access specifier for the class itself (if the class is nested inside another class), not the access specifier for any of its members.
> 
> I think you're looking for HasProtectedFields and HasPrivateFields.  (I think there currently isn't any getter on CXXRecordDecl for them at the moment, but you can add one.)
So the issue that I mentioned in https://bugs.llvm.org/show_bug.cgi?id=41135#c18 is resolved by checking HasProtectedFields/HasPrivateFields.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60349/new/

https://reviews.llvm.org/D60349