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

Wed Jan 3 18:39:15 PST 2018

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

> 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>
> 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> wr
>>> ote:
>>>
>>>> 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.
>
>
> Yes, I think they are unavoidable.
>

 Okay, I think we've come up with a reasonable language rule in the other
thread.  Do you need anything else before you iterate on this patch?

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