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

Akira Hatanaka via cfe-commits cfe-commits at lists.llvm.org
Wed Jan 3 11:07:13 PST 2018



> On Jan 3, 2018, at 10:25 AM, John McCall <rjmccall at gmail.com> wrote:
> 
> On Wed, Jan 3, 2018 at 12:24 PM, Akira Hatanaka <ahatanaka at apple.com <mailto:ahatanaka at apple.com>> wrote:
>> On Jan 2, 2018, at 9:42 AM, David Blaikie via cfe-commits <cfe-commits at lists.llvm.org <mailto:cfe-commits at lists.llvm.org>> wrote:
>> 
>> 
>> 
>> On Tue, Dec 19, 2017 at 9:43 PM Akira Hatanaka <ahatanak at gmail.com <mailto:ahatanak at gmail.com>> wrote:
>> On Tue, Dec 12, 2017 at 12:12 PM, John McCall <rjmccall at gmail.com <mailto:rjmccall at gmail.com>> wrote:
>> On Tue, Dec 12, 2017 at 1:45 PM, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
>> On Mon, Dec 11, 2017 at 5:38 PM John McCall <rjmccall at gmail.com <mailto:rjmccall at gmail.com>> wrote:
>> On Mon, Dec 11, 2017 at 6:19 PM, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
>> On Mon, Dec 11, 2017 at 3:16 PM John McCall via Phabricator <reviews at reviews.llvm.org <mailto:reviews at reviews.llvm.org>> wrote:
>> rjmccall added a comment.
>> 
>> In https://reviews.llvm.org/D41039#951648 <https://reviews.llvm.org/D41039#951648>, @ahatanak wrote:
>> 
>> > I had a discussion with Duncan today and he pointed out that perhaps we shouldn't allow users to annotate a struct with "trivial_abi" if one of its subobjects is non-trivial and is not annotated with "trivial_abi" since that gives users too much power.
>> >
>> > Should we error out or drop "trivial_abi" from struct Outer when the following code is compiled?
>> >
>> >   struct Inner1 {
>> >     ~Inner1(); // non-trivial
>> >     int x;
>> >   };
>> >
>> >   struct __attribute__((trivial_abi)) Outer {
>> >     ~Outer();
>> >     Inner1 x;
>> >   };
>> >
>> >
>> > The current patch doesn't error out or drop the attribute, but the patch would probably be much simpler if we didn't allow it.
>> 
>> 
>> I think it makes sense to emit an error if there is provably a non-trivial-ABI component.  However, for class temploids I think that diagnostic should only fire on the definition, not on instantiations; for example:
>> 
>>   template <class T> struct __attribute__((trivial_abi)) holder {
>>      T value;
>>      ~holder() {}
>>   };
>>   holder<std::string> hs; // this instantiation should be legal despite the fact that holder<std::string> cannot be trivial-ABI.
>> 
>> But we should still be able to emit the diagnostic in template definitions, e.g.:
>> 
>>   template <class T> struct __attribute__((trivial_abi)) named_holder {
>>      std::string name; // there are no instantiations of this template that could ever be trivial-ABI
>>      T value;
>>      ~named_holder() {}
>>   };
>> 
>> The wording should be something akin to the standard template rule that a template is ill-formed if it has no valid instantiations, no diagnostic required.
>> 
>> I would definitely like to open the conversation about the name of the attribute.  I don't think we've used "abi" in an existing attribute name; usually it's more descriptive.  And "trivial" is a weighty word in the standard.  I'm not sure I have a great counter-proposal off the top of my head, though.
>> 
>> Agreed on both counts (would love a better name, don't have any stand-out candidates off the top of my head).
>> 
>> I feel like a more descriptive term about the property of the object would make me happier - something like "address_independent_identity" (s/identity/value/?) though, yeah, that's not spectacular by any stretch.
>> 
>> Incidentally, your comments are not showing up on Phabricator for some reason.
>> 
>> Yeah, Phab doesn't do a great job looking on the mailing list for interesting replies - I forget, maybe it only catches bottom post or top post but not infix replies or something... 
>> 
>> Well, fortunately the mailing list is also archived. :)
>> 
>> 
>> The term "trivially movable" suggests itself, with two caveats:
>>   - What we're talking about is trivial *destructive* movability, i.e. that the combination of moving the value to a new object and not destroying the old object can be done trivially, which is not quite the same as trivial movability in the normal C++ sense, which I guess could be a property that someone theoretically might care about (if the type is trivially destructed, but it isn't copyable for semantic reasons?).
>>   - Trivial destructive movability is a really common property, and it's something that a compiler would really like to optimize based on even in cases where trivial_abi can't be adopted for binary-compatibility reasons.  Stealing the term for the stronger property that the type is trivially destructively movable *and its ABI should reflect that in a specific way* would be unfortunate.
>> 
>> Fair point.
>>  
>> "trivially_movable" is a long attribute anyway, and "trivially_destructively_movable" is even worse.
>> 
>> Maybe that second point is telling us that this isn't purely descriptive — it's inherently talking about the ABI, not just the semantics of the type.  I might be talking myself into accepting trivial_abi if we don't end up with a better suggestion.
>> 
>> *nod* I think if you want to slice this that way (ensuring there's space to support describing a similar property for use only in non-ABI-breaking contexts as well) it seems like ABI is the term to use somewhere in the name.
>> 
>> Yeah.  We just had a little internal conversation about it, and the idea of "address_invariant_abi" came up, but I'm not sure it has enough to recommend it over "trivial_abi" to justify the longer name.
>> 
>> 
>> Random thing that occurred to me: is it actually reasonable to enforce trivial_abi correctness in a non-template context?  Templates aren't the only case where a user could reasonably want to add trivial_abi and just have it be suppressed if it's wrong.  Imagine if some stdlib made std::string trivial_abi; someone might reasonably want to make my named_holder example above trivial_abi as well, with the expectation that it would only have an effect on targets where std::string was trivial_abi.  At the very least, I'm concerned that we might be opening ourselves up to a need to add supporting features, like a way to be conditionally trivial_abi based on context.
>> 
>> Fair point, much like the quirky but useful behavior of "= default". Good point about non-dependent contexts still being relevant to this subjective behavior... 
>> 
>> I was already leaning towards this being a warning rather than an error - this situation leans me moreso that way & possibly suppressing the warning when the types are split between system and non-system headers (if the attribute's on a type in a non-system header, but the type that's blocking it from being active is in a system header, don't warn).
>> 
>> That's an interesting idea.
>> 
>> 
>> OK, so if a trivial_abi class has a component that is not trivial, we drop the trivial_abi from the containing class. Also, we may decide not to error out or warn if the attribute is on a template or the non-trivial class comes from a system header.
>> 
>> What would the rules be for propagating the trivial_abi property outwards? I tried to extend the existing standard rules for determining whether a special function is trivial to trivial_abi or whether a type should be forced to be passed indirectly in my first patch, but it looks like that approach is too complicated.
>> 
>> What makes it too complicated? That would seem unfortunate to diverge here, I would think..
>>  
> 
> Based on my understanding of the discussion yesterday, I think we’ve decided not to diverge from the standard:
> 
> http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20180101/thread.html#214043 <http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20180101/thread.html#214043>
> 
> My patch keeps track of the triviality of special functions using two bitfields in CXXRecordDecl (HasTrivialSpecialMembers and HasTrivialSpecialMembersForCall) and two flags in FunctionDecl (IsTrivial and IsTrivialForCall) because it needs to distinguish between triviality according to the current standard’s definition and triviality for the purpose of calls. Those flags have to be updated in various places, which I thought might be too complicated.
> 
> Well, we're not changing the definition of triviality, and the intent is that we're not changing the definition of triviality-for-calls (as standardized by Itanium) in the absence of the trivial_abi attribute.
> 
> Under the definition we developed in that thread, I think your new tracking bits are unavoidable.
> 

Yes, I think they are unavoidable.

> John.
>  
> 
>> I'm thinking about using the following rules instead. What do you think?
>> 
>> trivial_abi=true for a class if it meets the following conditions:
>> 
>> 1. it has attribute "trivial_abi" or
>> 2. if it doesn't have attribute "trivial_abi", it must meet all of the following conditions:
>>  2-1. it doesn't have any virtual functions or virtual bases and
>>  2-2. it doesn't have any members that would cause the type to be non-trivial (e.g., __weak objc pointers) and
>>  2-3. it doesn't have any user-provided copy constructors, move constructors, or destructors (explicitly defaulted methods are OK) and
>>  2-4. trivial_abi=true for all of its base classes and members
>> 
>> These rules apply only when the class or the type of one of its members or base classes has attribute "trivial_abi". The existing rules in the standard will be used to determine whether clang should force a type to be passed indirectly if none of the classes in the class hierarchy have attribute "trivial_abi".
>>  
>> John.
>> 
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org <mailto:cfe-commits at lists.llvm.org>
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits>
> 
> 
> 
> -- 
> I suppose you'd like my real thoughts as well.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180103/9d9e8231/attachment-0001.html>


More information about the cfe-commits mailing list