Page MenuHomePhabricator

Add support for attribute "trivial_abi"
ClosedPublic

Authored by ahatanak on Dec 8 2017, 3:36 PM.

Details

Summary

This patch adds support for a new attribute "trivial_abi", which will be used to instruct clang to pass and return non-trivial C++ structs directly when it's possible to do so. The original RFC I sent to cfe-dev last month is here:

http://lists.llvm.org/pipermail/cfe-dev/2017-November/055955.html

A couple of questions:

  1. The attribute is tentatively named "trivial_abi". Is there a better name that conveys what the attribute is supposed to do? How about c_abi or pass_by_value?
  1. Propagating the property of a "trivial_abi" class to the containing classes turned out to be more complicated than I initially expected. Do we really need or want clang to do that rather than asking users to annotate the containing classes with "trivial_abi"? If we do, is there a simpler way to accomplish that than what I did in this patch?

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
rjmccall added inline comments.Jan 4 2018, 7:40 PM
include/clang/AST/DeclCXX.h
1502 ↗(On Diff #128694)

These method names read as if they're actions, not predicates. Please use "is..." or "can..." or "should..." or something that makes it clear that they're predicates.

include/clang/Basic/AttrDocs.td
2234 ↗(On Diff #128694)

You should say "when the type would otherwise be considered..." here, because this attribute actually changes the definition of non-trivial for the purposes of calls.

I would suggest rephrasing the bit about non-trivial special members like so: "A class annotated with `trivial_abi` can have non-trivial destructors or copy/move constructors without automatically becoming non-trivial for the purposes of calls."

2251 ↗(On Diff #128694)

You should state explicitly in this example that A is itself trivial for the purposes of calls.

2255 ↗(On Diff #128694)

"If a type is trivial for the purposes of calls, has a non-trivial destructor, and is passed as an argument by value, the convention is that the callee will destroy the object before returning."

2257 ↗(On Diff #128694)

Don't phrase it as being "dropped", phrase it as having no effect.

2261 ↗(On Diff #128694)

I think the right list of exceptions is:

  • The class directly declares a virtual base or virtual methods.
  • The class has a base class that is non-trivial for the purposes of calls.
  • The class has a non-static data member whose type is non-trivial for the purposes of calls, which includes:
    • classes that are non-trivial for the purposes of calls
    • __weak-qualified types in Objective-C++
    • arrays of any of the above

I don't see why strong types would be affected. We've talked about changing the C++ ABI for structs containing strong members as part of the __strong-pointers-in-structs feature, but that's not even implicated here because there's an attribute which did not previously exist, so there's no established ABI.

lib/CodeGen/CGCall.cpp
3509 ↗(On Diff #128694)

You do have to push a cleanup here in case there's an exception thrown before the call is made. Fortunately, you should be able to just use the logic immediately below, since it's exactly the same treatment that the Microsoft C++ ABI uses.

lib/CodeGen/CodeGenFunction.cpp
1150 ↗(On Diff #128694)

We must already have a place that does this for the Microsoft ABI.

ahatanak updated this revision to Diff 128767.Jan 5 2018, 9:54 AM

Address review comments.

I also made changes so that FunctionDecl::IsTrivialForCall is always set to true for special functions of "trivial_abi" classes.

There is still one microsoft IRGen test failing because I haven't implemented serialization of the "ForCall" bits I introduced in FunctionDecl and CXXRecordDecl.

ahatanak marked 10 inline comments as done.Jan 5 2018, 10:12 AM
ahatanak added inline comments.
include/clang/AST/DeclCXX.h
443 ↗(On Diff #128694)

Yes, only three bits are needed here. I'll fix this later.

478 ↗(On Diff #128694)

I think we need to check whether a struct annotated with "trivial_abi" is ill-formed before ActOnFields and CheckCompletedCXXClass set the triviality flags in Sema::ActOnFinishCXXMemberSpecification and drop the attribute if it is ill-formed. If we don't want to drop the attribute, we will have to find out whether "trivial_abi" has no effect before calls to FunctionDecl::setTrivialForCall are made.

I added this bit to CXXRecordDecl in the previous patch because I felt hesitant to iterate over the base classes and members of the class again, but I think that is better than adding a bit just to check whether the class is ill-formed. I've removed the bit in the new patch.

ahatanak marked an inline comment as done.Jan 5 2018, 10:21 AM
ahatanak added inline comments.
include/clang/Basic/AttrDocs.td
2261 ↗(On Diff #128694)

I realized I hadn't taken care of arrays of non-trivial structs. I fixed it in checkIllFormedTrivialABIStruct and added a test case to test/SemaObjCXX/attr-trivial-abi.mm.

ahatanak updated this revision to Diff 128781.Jan 5 2018, 12:21 PM

Sorry, the patch I've just uploaded was missing some changes I made.

ahatanak updated this revision to Diff 128784.Jan 5 2018, 12:35 PM
  • Serialize/deserialize the bits I added to FunctionDecl and CXXRecordDecl.
  • Enable passing non-trivial structs when clang abi-compat version is 4.0 or lower.
  • Fix a bug in Sema::CheckCompletedCXXClass where the CXXRecordDecl flags were not being set when the class had a defaulted special function. Fixed it by renaming finishedUserProvidedMethod to setTrivialForCallFlags and calling it in Sema::CheckCompletedCXXClass.
rsmith added inline comments.Jan 5 2018, 12:54 PM
include/clang/AST/DeclCXX.h
1489–1491 ↗(On Diff #128767)

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.

lib/CodeGen/CGCall.cpp
3518 ↗(On Diff #128767)

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 :)

lib/CodeGen/CGDecl.cpp
1822 ↗(On Diff #128767)

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.

lib/Sema/SemaDeclCXX.cpp
5923 ↗(On Diff #128767)

This should have a different name if you're only going to call it for user-provided copy ctors, move ctors, and dtors.

12108–12114 ↗(On Diff #128767)

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?

ahatanak updated this revision to Diff 128798.Jan 5 2018, 2:21 PM
ahatanak marked an inline comment as done.
ahatanak marked 3 inline comments as done.Jan 5 2018, 2:26 PM
ahatanak added inline comments.
include/clang/AST/DeclCXX.h
1489–1491 ↗(On Diff #128767)

I sank the code to CGCall.cpp and CGDecl.cpp and removed the method.

lib/CodeGen/CGCall.cpp
3518 ↗(On Diff #128767)

OK, I'll leave this for future work.

lib/Sema/SemaDeclCXX.cpp
5923 ↗(On Diff #128767)

I renamed the function to setTrivialForCallFlags.

12108–12114 ↗(On Diff #128767)

Done. I clang-formated the code above too.

ahatanak marked 2 inline comments as done.Jan 5 2018, 3:04 PM
ahatanak added inline comments.
lib/CodeGen/CGDecl.cpp
1827 ↗(On Diff #128798)

This is not correct since it isn't checking Arg.isIndirect(). I'll fix it and upload a new patch.

rsmith added inline comments.Jan 5 2018, 3:20 PM
lib/CodeGen/CGCall.cpp
3498 ↗(On Diff #128798)

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.

lib/CodeGen/CGDecl.cpp
1822 ↗(On Diff #128798)

Same comment regarding this name.

lib/CodeGen/MicrosoftCXXABI.cpp
833 ↗(On Diff #128798)

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.
ahatanak updated this revision to Diff 128815.Jan 5 2018, 3:21 PM
ahatanak marked an inline comment as done.

Check whether the argument is passed indirectly before pushing the cleanup.

ahatanak updated this revision to Diff 128838.Jan 5 2018, 7:21 PM
ahatanak marked an inline comment as done.Jan 5 2018, 7:25 PM
ahatanak added inline comments.
lib/CodeGen/MicrosoftCXXABI.cpp
833 ↗(On Diff #128798)

I'm not very familiar with microsoft's ABI, but I think that makes sense.

ahatanak updated this revision to Diff 128839.Jan 5 2018, 7:28 PM
ahatanak marked 3 inline comments as done.

Rename variable to HasTrivialABIOverride.

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.

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.

lib/CodeGen/CGDecl.cpp
1825 ↗(On Diff #128839)

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.

aaron.ballman added inline comments.Jan 6 2018, 8:27 AM
include/clang/Basic/Attr.td
1159 ↗(On Diff #128839)

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>

lib/Sema/SemaDeclCXX.cpp
7594 ↗(On Diff #128839)

Can use const auto * here.

7640 ↗(On Diff #128839)

This should use dyn_cast instead of dyn_cast_or_null.

ahatanak updated this revision to Diff 128895.Jan 8 2018, 12:55 AM

Address review comments.

Also, emit the declaration of the destructor of a trivial-abi override class in Sema::ActOnParamDeclarator and mark it as referenced. This is necessary because a trivial-abi type that is passed by value needs to be destructed in the callee and the destructor has to be declared in the AST when IRGen emits the call to the destructor in the callee.

ahatanak marked 4 inline comments as done.Jan 8 2018, 1:05 AM

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.

include/clang/Basic/Attr.td
1159 ↗(On Diff #128839)

I don't think this attribute makes sense in C, so I've set the LangOpts to be CPlusPlus.

lib/CodeGen/CGDecl.cpp
1825 ↗(On Diff #128839)

It turns out Sema needs this function too. I defined function hasTrivialABIOverride in CXXRecordDecl and moved the code there.

rjmccall added inline comments.Jan 8 2018, 8:20 AM
include/clang/AST/Type.h
811 ↗(On Diff #128895)

This should get a comment, even if it's just to refer to the CXXRecordDecl method.

lib/CodeGen/CGCall.cpp
3498 ↗(On Diff #128895)

Please sink this call to the points where you use it; it should be possible to avoid computing it in most cases.

3505 ↗(On Diff #128895)

e.g. here you can conditionalize it on HasAggregateEvalKind, which is both slightly faster and clearer to the reader.

3518 ↗(On Diff #128895)

And here you can guard it by RD && RD->hasNonTrivialDestructor(), and you can just call hasNonTrivialABIOverride() directly on RD.

lib/Sema/SemaDecl.cpp
11700 ↗(On Diff #128895)

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.

ahatanak updated this revision to Diff 128960.Jan 8 2018, 11:40 AM
ahatanak marked 7 inline comments as done.

Address review comments.

rjmccall accepted this revision.Jan 8 2018, 1:02 PM

Thanks, looks good to me.

This revision is now accepted and ready to land.Jan 8 2018, 1:02 PM
rsmith added a comment.Jan 8 2018, 1:39 PM

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.

include/clang/Sema/Sema.h
2239 ↗(On Diff #128895)

Please use an enum here; two bool parameters in a row is too error-prone, especially if both have default arguments.

lib/CodeGen/MicrosoftCXXABI.cpp
863–864 ↗(On Diff #128895)

Please retain the two pre-existing "Note"s pointing out how the ABI rule here is intentionally non-conforming.

lib/Sema/SemaDecl.cpp
11700 ↗(On Diff #128895)

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).

lib/Sema/SemaDeclCXX.cpp
7580 ↗(On Diff #128895)

Either "ill-formed" is the wrong word here, or this needs to produce ext_ diagnostics that -pedantic-errors turns into hard errors.

7582–7583 ↗(On Diff #128895)

Suppress the diagnostic in the case where this happens during template instantiation.

7595 ↗(On Diff #128895)

This can be simplified to B.getType()->getAsCXXRecordDecl().

7598 ↗(On Diff #128895)

It isn't correct to check canPassInRegisters on a dependent type here; you need to skip this check in that case.

ahatanak updated this revision to Diff 129038.Jan 9 2018, 12:04 AM

Add more tests for template instantiation of "trivial_abi" classes.

ahatanak marked 7 inline comments as done.Jan 9 2018, 12:12 AM
ahatanak added inline comments.
lib/Sema/SemaDecl.cpp
11700 ↗(On Diff #128895)

I defined function ASTContext::isParamDestroyedInCallee and used it in two places. I didn't use it in CodeGenFunction::EmitParmDecl because the destructor cleanup is pushed only when the argument is passed indirectly in MSVC's case, whereas it is always pushed when the class is HasTrivialABIOverride.

lib/Sema/SemaDeclCXX.cpp
7580 ↗(On Diff #128895)

I made the warning derive from ExtWarn.

ahatanak updated this revision to Diff 129345.Jan 10 2018, 2:34 PM
ahatanak marked 2 inline comments as done.

Partially revert the changes I made to CodeGenFunction::EmitParmDecl add IRGen test case for exception handling.

ahatanak added inline comments.Jan 10 2018, 2:45 PM
lib/Sema/SemaDecl.cpp
11700 ↗(On Diff #128895)

I was misunderstanding the code in CodeGenFunction::EmitParmDecl. When the argument is a class that is passed by value, Arg.isIndirect() returns true even when the argument is passed directly. The code that pushes the destruction cleanup should remain in the if part of the if statement and isParamDestroyedInCallee can be used to determine whether the parameter is destructed in the callee.

ahatanak updated this revision to Diff 129397.Jan 10 2018, 11:17 PM

In CXXRecordDecl::addedMember, set HasTrivialSpecialMembersForCall if Method->isTrivialForCall() returns true. This fixes a bug where CXXRecordDecl::hasNonTrivialDestructorForCall would return false for the implicit destructor even when the parent class had attribute trivial_abi.

Run test/CodeGenObjCXX/trivial_abi.mm with -fclang-abi-compat=4.0 to confirm the bug has been fixed.

I think I've addressed all the review comments. Any other comments from anyone?

rjmccall accepted this revision.Jan 22 2018, 7:51 AM

Looks great to me! Thanks for taking this on, it's a pretty major improvement for users.

Would you like to create an issue with itanium-cxx-abi to document this, or do you want me to handle that?

Yes, please document this in itanium-cxx-abi. Thanks!

rsmith accepted this revision.Feb 2 2018, 3:13 PM
rsmith added inline comments.
include/clang/Sema/Sema.h
2240–2241 ↗(On Diff #129397)

These seem a bit opaque at call sites. How about changing this to enum class TrivialABIHandling { Ignore, Consider }; or something like that?

lib/CodeGen/MicrosoftCXXABI.cpp
863–864 ↗(On Diff #128895)

This comment appears to apply to the code above it rather than the code below it, as does the note below.

We've still lost the *other* "non-conforming" comment, which appertains to the "size <= 64" case below.

Closed by commit rL324269: Add support for attribute 'trivial_abi'. (authored by ahatanak, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.
ahatanak marked 2 inline comments as done.

Yes, please document this in itanium-cxx-abi. Thanks!

Looks like this hasn't happened yet.