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

[John McCall via cfe-commits]

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.

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