[PATCH] D41039: Add support for attribute "trivial_abi"

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

I'd like to see more testing for the template instantiation case. I don't see any test coverage for the "attribute only affects instantiations whose members are trivial-for-calls" part.



================
Comment at: include/clang/Sema/Sema.h:2239
   bool SpecialMemberIsTrivial(CXXMethodDecl *MD, CXXSpecialMember CSM,
-                              bool Diagnose = false);
+                              bool ForCall = false, bool Diagnose = false);
   CXXSpecialMember getSpecialMember(const CXXMethodDecl *MD);
----------------
Please use an enum here; two bool parameters in a row is too error-prone, especially if both have default arguments.


================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:863-864
+      return RAA_Default;
+    // Otherwise, if the copy ctor is trivial and the object is small, pass
+    // direct.
+    if (CopyCtorIsTrivial &&
----------------
Please retain the two pre-existing "Note"s pointing out how the ABI rule here is intentionally non-conforming.


================
Comment at: lib/Sema/SemaDecl.cpp:11700
+    }
+  }
+
----------------
rjmccall wrote:
> I think it's correct not to call CheckDestructorAccess and DiagnoseUseOfDecl here, since according to the standard destructor access is always supposed to be checked at the call-site, but please leave a comment explaining that.
The corresponding code for `areArgsDestroyedLeftToRightInCallee` is in `SemaChecking`. This should be done in the same place.

More generally, we have at least three different places where we check `CXXABI::areArgsDestroyedLeftToRightInCallee() || Type->hasTrivialABIOverride()`. It would make sense to factor that out into a `isParamDestroyedInCallee` function (probably on `ASTContext`, since we don't have anywhere better for ABI-specific checks at a layering level that can talk about types).


================
Comment at: lib/Sema/SemaDeclCXX.cpp:7580
 
+static void checkIllFormedTrivialABIStruct(CXXRecordDecl &RD, Sema &S) {
+  auto PrintDiagAndRemoveAttr = [&]() {
----------------
Either "ill-formed" is the wrong word here, or this needs to produce `ext_` diagnostics that `-pedantic-errors` turns into hard errors.


================
Comment at: lib/Sema/SemaDeclCXX.cpp:7582-7583
+  auto PrintDiagAndRemoveAttr = [&]() {
+    S.Diag(RD.getAttr<TrivialABIAttr>()->getLocation(),
+           diag::warn_cannot_use_trivial_abi) << &RD;
+    RD.dropAttr<TrivialABIAttr>();
----------------
Suppress the diagnostic in the case where this happens during template instantiation.


================
Comment at: lib/Sema/SemaDeclCXX.cpp:7595
+    const auto *BaseClassDecl
+        = cast<CXXRecordDecl>(B.getType()->getAs<RecordType>()->getDecl());
+    // Ill-formed if the base class is non-trivial for the purpose of calls or a
----------------
This can be simplified to `B.getType()->getAsCXXRecordDecl()`.


================
Comment at: lib/Sema/SemaDeclCXX.cpp:7598
+    // virtual base.
+    if (!BaseClassDecl->canPassInRegisters() || B.isVirtual()) {
+      PrintDiagAndRemoveAttr();
----------------
It isn't correct to check `canPassInRegisters` on a dependent type here; you need to skip this check in that case.


https://reviews.llvm.org/D41039