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

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

rsmith added inline comments.


================
Comment at: lib/CodeGen/CGCall.cpp:3498
   bool HasAggregateEvalKind = hasAggregateEvaluationKind(type);
+  bool TypeDestructedInCallee = false;
+  if (const auto *RD = type->getAsCXXRecordDecl())
----------------
This is a confusing name for this flag, because there are other reasons why we can choose to destroy in the callee (specifically the MS ABI reason). Rather, I think this is capturing that "this parameter's ABI was affected by a `trivial_abi` attribute", which implies that it should be destroyed (only) in the callee. Maybe calling this something like `HasTrivialABIOverride` would help.


================
Comment at: lib/CodeGen/CGDecl.cpp:1822
+  bool IsScalar = hasScalarEvaluationKind(Ty);
+  bool TypeDestructedInCallee = false;
+  if (const auto *RD = Ty->getAsCXXRecordDecl())
----------------
Same comment regarding this name.


================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:833
     // passed in registers, which is non-conforming.
     if (RD->hasNonTrivialDestructor() &&
         getContext().getTypeSize(RD->getTypeForDecl()) > 64)
----------------
Should we be checking `hasNonTrivialDestructorForCalls` here? What should happen if we have a small class whose copy constructor is trivial for calls but whose destructor is not? The existing machinations of this ABI only work because they can make an additional trivial copy of the function parameter when passing it direct, *even if* it has a non-trivial destructor. I don't think that's correct in general if the copy constructor is only trival-for-calls. Consider:

```
struct X {
  shared_ptr<T> sp; // shared_ptr<T> is trivial-for-calls
  ~X();
};
```

In the absence of the destructor, X should be passed direct.
In the presence of the destructor, though, passing direct would result in the X destructor running in both caller and callee, and X being passed through registers without running the copy constructor. Which results in a double-delete.

So I think what we want here is this:

 * If the copy ctor and dtor are both trivial-for-calls, pass direct.
 * Otherwise, if the copy ctor is trivial (not just trivial-for-calls) and the object is small, pass direct (regardless of the triviality of the dtor -- we'll make a copy, notionally using the trivial copy ctor, and destroy both the original and the copy).
 * Otherwise, pass indirect.


https://reviews.llvm.org/D41039