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

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak marked 3 inline comments as done.
ahatanak added inline comments.


================
Comment at: include/clang/AST/DeclCXX.h:1489-1491
+  bool shouldBeDestructedInCallee() const {
+    return data().CanPassInRegisters && hasNonTrivialDestructor();
+  }
----------------
rsmith wrote:
> 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.
I sank the code to CGCall.cpp and CGDecl.cpp and removed the method.


================
Comment at: lib/CodeGen/CGCall.cpp:3518
+         CGM.getCXXABI().getRecordArgABI(RD) != CGCXXABI::RAA_Default) ||
+        TypeDestructedInCallee;
     if (DestroyedInCallee)
----------------
rsmith wrote:
> 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 :)
OK, I'll leave this for future work.


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


================
Comment at: lib/Sema/SemaDeclCXX.cpp:12108-12114
+  if (!IsTrivialForCall) {
+    if (ClassDecl->needsOverloadResolutionForCopyConstructor())
+      IsTrivialForCall =
+          SpecialMemberIsTrivial(CopyConstructor, CXXCopyConstructor, true);
+    else
+      IsTrivialForCall = ClassDecl->hasTrivialCopyConstructorForCall();
+  }
----------------
rsmith wrote:
> 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?
Done. I clang-formated the code above too.


https://reviews.llvm.org/D41039