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

Richard Smith - zygoloid via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Jan 5 12:54:39 PST 2018

rsmith added inline comments.

Comment at: include/clang/AST/DeclCXX.h:1489-1491
+  bool shouldBeDestructedInCallee() const {
+    return data().CanPassInRegisters && hasNonTrivialDestructor();
+  }
This will return incorrect results on MSVC, where every class type should be destroyed in the callee. Since this is only used  by CodeGen, and only used in combination with the MSVC "destroy right-to-left in callee" ABI setting, please sink it to CodeGen.

Comment at: lib/CodeGen/CGCall.cpp:3518
+         CGM.getCXXABI().getRecordArgABI(RD) != CGCXXABI::RAA_Default) ||
+        TypeDestructedInCallee;
     if (DestroyedInCallee)
This looks wrong (but the wrongness is pre-existing): in the weird MSVC x86_64 case where a small object of class type is passed in registers despite having a non-trivial destructor, this will set `DestroyedInCallee` to false, meaning that the parameter will be destroyed twice, in both the caller and callee.

And yep, that's exactly what both Clang and MSVC do: https://godbolt.org/g/zjeQq6

If we fixed that, we could unconditionally `setExternallyDestructed()` here. But let's leave that discussion for a separate patch :)

Comment at: lib/CodeGen/CGDecl.cpp:1822
+  bool IsScalar = hasScalarEvaluationKind(Ty);
+  if ((!IsScalar && !CurFuncIsThunk &&
+       getTarget().getCXXABI().areArgsDestroyedLeftToRightInCallee()) ||
Should these conditions not also apply to the `shouldBeDestructedInCallee` case? We don't want to destroy `trivial_abi` parameters in thunks, only in the eventual target function.

Comment at: lib/Sema/SemaDeclCXX.cpp:5923
+        M->setTrivialForCall(HasTrivialABI);
+        Record->finishedUserProvidedMethod(M);
+      }
This should have a different name if you're only going to call it for user-provided copy ctors, move ctors, and dtors.

Comment at: lib/Sema/SemaDeclCXX.cpp:12108-12114
+  if (!IsTrivialForCall) {
+    if (ClassDecl->needsOverloadResolutionForCopyConstructor())
+      IsTrivialForCall =
+          SpecialMemberIsTrivial(CopyConstructor, CXXCopyConstructor, true);
+    else
+      IsTrivialForCall = ClassDecl->hasTrivialCopyConstructorForCall();
+  }
Nit: can you use the same form of `?:` expression as is used a few lines above to make it obvious to a reader that this computation parallels that one?


More information about the cfe-commits mailing list