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

Wed Jan 3 10:25:22 PST 2018

On Wed, Jan 3, 2018 at 12:24 PM, Akira Hatanaka <ahatanaka at apple.com> wrote:

> On Jan 2, 2018, at 9:42 AM, David Blaikie via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
>
>
>
> On Tue, Dec 19, 2017 at 9:43 PM Akira Hatanaka <ahatanak at gmail.com> wrote:
>
>> On Tue, Dec 12, 2017 at 12:12 PM, John McCall <rjmccall at gmail.com> wrote:
>>
>>> On Tue, Dec 12, 2017 at 1:45 PM, David Blaikie <dblaikie at gmail.com> w
>>> rote:
>>>
>>>> On Mon, Dec 11, 2017 at 5:38 PM John McCall <rjmccall at gmail.com> wrote:
>>>>
>>>>> On Mon, Dec 11, 2017 at 6:19 PM, David Blaikie <dblaikie at gmail.com> w
>>>>> rote:
>>>>>
>>>>>> On Mon, Dec 11, 2017 at 3:16 PM John McCall via Phabricator <
>>>>>> reviews at reviews.llvm.org> wrote:
>>>>>>
>>>>>>> rjmccall added a comment.
>>>>>>>
>>>>>>> In 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
>
> 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.

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
> 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/f75a1242/attachment-0001.html>