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

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak marked 4 inline comments as done.
ahatanak added a comment.

In https://reviews.llvm.org/D41039#969171, @rjmccall wrote:

> I'll trust Richard on the tricky Sema/AST bits.  The functionality of the patch looks basically acceptable to me, although I'm still not thrilled about the idea of actually removing the attribute from the AST rather than just letting it not have effect.  But we could clean that up later if it's significantly simpler to do it this way.


Sure, we can clean up this later.

> Please add a CodeGenObjCXX test case that `__weak` fields in ARC++ do actually override the `trivial_abi` attribute but that `__strong` fields do not.  Also, your test case does not seem to actually test arrays of `__weak` references.

Done. When I was writing a test that tests `__strong` fields, I found out that the destructor of the struct wasn't being declared in the AST, which caused an assertion to fail in CodeGenFunction::destroyCXXObject when compiling a function that takes a trivial_abi type. I fixed it by forcing the declaration of the destructor in Sema::ActOnParamDeclarator.



================
Comment at: include/clang/Basic/Attr.td:1159
+def TrivialABI : InheritableAttr {
+  let Spellings = [Clang<"trivial_abi">];
+  let Subjects = SubjectList<[CXXRecord]>;
----------------
aaron.ballman wrote:
> Would this attribute make sense in C, or does it really only make sense in C++? If it doesn't make sense in C++, then you should also set `LangOpts` to be `CPlusPlus`. If it does make sense in C, then the Clang spelling should be `Clang<"trivial_abi", 1>`
I don't think this attribute makes sense in C, so I've set the LangOpts to be CPlusPlus.


================
Comment at: lib/CodeGen/CGDecl.cpp:1825
+    HasTrivialABIOverride =
+        RD->canPassInRegisters() && RD->hasNonTrivialDestructor();
+
----------------
rjmccall wrote:
> You could make a helper function to do this computation and then declare it in some internal-to-IRGen header so that you don't write it out multiple times.
It turns out Sema needs this function too. I defined function hasTrivialABIOverride in CXXRecordDecl and moved the code there.


https://reviews.llvm.org/D41039