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

Akira Hatanaka via cfe-commits cfe-commits at lists.llvm.org
Tue Dec 19 21:43:24 PST 2017

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> wrote:
>> 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>
>>> wrote:
>>>> 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.

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20171219/f56a63f8/attachment-0001.html>

More information about the cfe-commits mailing list