On Wed, Jan 3, 2018 at 2:07 PM, Akira Hatanaka <ahatan...@apple.com> wrote:
> On Jan 3, 2018, at 10:25 AM, John McCall <rjmcc...@gmail.com> wrote: > > On Wed, Jan 3, 2018 at 12:24 PM, Akira Hatanaka <ahatan...@apple.com> > wrote: > >> On Jan 2, 2018, at 9:42 AM, David Blaikie via cfe-commits < >> cfe-commits@lists.llvm.org> wrote: >> >> >> >> On Tue, Dec 19, 2017 at 9:43 PM Akira Hatanaka <ahata...@gmail.com> >> wrote: >> >>> On Tue, Dec 12, 2017 at 12:12 PM, John McCall <rjmcc...@gmail.com> wr >>> ote: >>> >>>> On Tue, Dec 12, 2017 at 1:45 PM, David Blaikie <dblai...@gmail.com> w >>>> rote: >>>> >>>>> On Mon, Dec 11, 2017 at 5:38 PM John McCall <rjmcc...@gmail.com> >>>>> wrote: >>>>> >>>>>> On Mon, Dec 11, 2017 at 6:19 PM, David Blaikie <dblai...@gmail.com> w >>>>>> rote: >>>>>> >>>>>>> On Mon, Dec 11, 2017 at 3:16 PM John McCall via Phabricator < >>>>>>> revi...@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.
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
