[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: docs/LanguageExtensions.rst:1096
+  equivalent to copying the underlying bytes and then dropping the source object
+  on the floor.
 * ``__is_destructible`` (MSVC 2013)
----------------
Quuxplusone wrote:
> rjmccall wrote:
> > Quuxplusone wrote:
> > > rjmccall wrote:
> > > > Quuxplusone wrote:
> > > > > @rjmccall wrote:
> > > > > > trivial_abi permits annotated types to be passed and returned in registers, which is ABI-breaking. Skimming the blog post, it looks like trivially_relocatable does not permit this — it merely signifies that destruction is a no-op after a move construction or assignment.
> > > > > 
> > > > > Not necessarily a "no-op"; my canonical example is a CopyOnlyCXX03SharedPtr which increments a refcount on construction and decrements on destruction. But move-construction plus destruction should "balance out" and result in no observable side effects.
> > > > > 
> > > > > > This is usefully different in the design space, since it means you can safely add the attribute retroactively to e.g. std::unique_ptr, and other templates can then detect that std::unique_ptr is trivially-relocatable and optimize themselves to use memcpy or realloc or whatever it is that they want to do. So in that sense trivial_abi is a *stronger* attribute, not a *weaker* one: the property it determines ought to imply trivially_relocatable.
> > > > > 
> > > > > `trivial_abi` is an "orthogonal" attribute: you can have `trivial_abi` types with non-trivial constructors and destructors, which can have observable side effects. For example,
> > > > > ```
> > > > > struct [[clang::trivial_abi]] DestructionAnnouncer {
> > > > >     ~DestructionAnnouncer() { puts("hello!"); }
> > > > > };
> > > > > ```
> > > > > is `trivial_abi` (because of the annotation) yet not trivially relocatable, because its "move plus destroy" operation has observable side effects.
> > > > > 
> > > > > > The only interesting question in the language design that I know of is what happens if you put the attribute on a template that's instantiated to contain a sub-object that is definitely not trivially relocatable / trivial-ABI. For trivial_abi, we decided that the attribute is simply ignored — it implicitly only applies to specializations where the attribute would be legal. I haven't dug into the design enough to know what trivially_relocatable decides in this situation, but the three basic options are:
> > > > > >
> > > > > > - the attribute always has effect and allows trivial relocation regardless of the subobject types; this is obviously unsafe, so it limits the safe applicability of the attribute to templates
> > > > > > - the attribute is ignored, like trivial_abi is
> > > > > > - the attribute is ill-formed, and you'll need to add a [[trivially_relocatable(bool)]] version to support templates
> > > > > 
> > > > > What happens is basically the first thing you said, except that I disagree that it's "obviously unsafe." Right now, conditionally trivial relocation is possible via template metaprogramming; see the libcxx patch at e.g.
> > > > > https://github.com/Quuxplusone/libcxx/commit/6524822c009e#diff-38adc80cec663f2f29c22e9ffc0de912
> > > > > Since the attribute is an opt-in mechanism, it makes perfect sense to me that if you put it on a class (or class template), then it applies to the class, without any further sanity-checking by the compiler. The compiler has no reason to second-guess the programmer here.
> > > > > 
> > > > > However, there's one more interesting case. Suppose the programmer puts the attribute on a class that isn't relocatable at all! (For example, the union case @erichkeane mentioned, or a class type with a deleted destructor.) In that case, this patch *does* give an error... *unless* the class was produced by instantiating a template, in which case we *don't* give an error, because it's not the template-writer's fault.
> > > > > https://p1144.godbolt.org/z/wSZPba
> > > > > trivial_abi is an "orthogonal" attribute: you can have trivial_abi types with non-trivial constructors and destructors, which can have observable side effects. 
> > > > 
> > > > Let me cut this conversation short.  `trivial_abi` is not such an old and widely-established attribute that we are unable to revise its definition.  I am comfortable making the same semantic guarantees for `trivial_abi` that you're making for `trivially_relocatable`, because I think it is in the language's interest for `trivial_abi` to be strictly stronger than `trivially_relocatable`.
> > > > 
> > > > > What happens is basically the first thing you said, except that I disagree that it's "obviously unsafe." 
> > > > 
> > > > Under your semantics, the attribute is an unchecked assertion about all of a class's subobjects.  A class template which fails to correctly apply the template metaprogramming trick to all of its dependently-typed subobjects — which can be quite awkward because it creates an extra dimension of partial specialization, and which breaks ABI by adding extra template parameters — will be silently miscompiled to allow objects to be memcpy'ed when they're potentially not legal to memcpy.  That is a footgun, and it is indeed "obviously unsafe".
> > > > 
> > > > Now, it's fair to say that it's unsafe in a useful way: because the attribute isn't checked, you can wrap a type you don't control in a `trivially_relocatable` struct and thereby get the advantages of triviality on the wrapper.  The model used by `trivial_abi` doesn't allow that.  But I feel pretty strongly that that is not the right default behavior for the language.
> > > > Under your semantics, the attribute is an unchecked assertion about all of a class's subobjects.
> > > 
> > > The attribute is an unchecked assertion about the class's //special member functions//. The attribute doesn't have anything to do with subobjects, period.
> > > Vice versa, the property currently expressed by "IsNaturallyTriviallyRelocatable" is deduced from all of the class's subobjects. The programmer can overrule the "natural" property in an "unnatural" way by annotating their class with the attribute.
> > > 
> > > And we know this is true because it is possible to make a trivially-relocatable class type containing non-trivially-relocatable members (e.g. a class having a member of type boost::interprocess::offset_ptr), and vice versa it is possible to make a non-trivially-relocatable class containing trivially-relocatable members (e.g. boost::interprocess::offset_ptr itself, which has only one member, of integral type).
> > > 
> > > > A class template which fails to correctly apply the template metaprogramming trick to all of its dependently-typed subobjects — which can be quite awkward because it creates an extra dimension of partial specialization
> > > 
> > > Agreed that it's awkward. The libc++ implementation was awkward, but definitely not challenging. The only thing that makes it at all tricky in the STL is that the STL allocator model permits fancy "pointer" types that can make e.g. std::vector non-trivially relocatable. If it weren't for fancy pointers, you wouldn't need the extra dimension.
> > > 
> > > > and which breaks ABI by adding extra template parameters
> > > 
> > > The libc++ implementation does not break ABI. The extra template parameter is concealed in a private base class.
> > > https://github.com/Quuxplusone/libcxx/commit/6524822c009e#diff-38adc80cec663f2f29c22e9ffc0de912
> > > 
> > > > I feel pretty strongly that that is not the right default behavior for the language.
> > > 
> > > Can you elaborate on that feeling (maybe in private email)? My intent with P1144 is that no industry programmer should ever see this attribute; the right default for industry programmers is to use the Rule of Zero. The reason we need the attribute is as an opt-in mechanism for the implementor of `unique_ptr`, `shared_ptr`, `vector`, and so on, //so that// the end-user can just use the Rule of Zero and everything will work fine. End-users shouldn't be messing with attributes.
> > > And we know this is true because it is possible to make a trivially-relocatable class type containing non-trivially-relocatable members (e.g. a class having a member of type boost::interprocess::offset_ptr), and vice versa it is possible to make a non-trivially-relocatable class containing trivially-relocatable members (e.g. boost::interprocess::offset_ptr itself, which has only one member, of integral type).
> > 
> > Why would a class containing a member of type `boost::interprocess::offset_ptr` be trivially-relocatable?  If you actually trivially relocate an object of the class, the pointer will not be rebased and so will be invalidated.  It would have to be an `offset_ptr` where you happen to know that the referent will always be copied simultaneously, e.g. because it's a member of the object itself.  Of course that's possible, but it's also such a corner case that we shouldn't balk at saying that the programmer ought to be more explicit about recognizing it.
> > 
> > > Agreed that it's awkward. The libc++ implementation was awkward, but definitely not challenging. The only thing that makes it at all tricky in the STL is that the STL allocator model permits fancy "pointer" types that can make e.g. std::vector non-trivially relocatable. If it weren't for fancy pointers, you wouldn't need the extra dimension.
> > 
> > Sure.  My point about the awkwardness is quite narrow: making the attribute take a `bool` argument is just a superior way of managing this over requiring a partial specialization.  Several other language attributes have been heading in this same direction.
> > 
> > > The libc++ implementation does not break ABI. The extra template parameter is concealed in a private base class.
> > 
> > Ah, apologies.
> > 
> > >  My intent with P1144 is that no industry programmer should ever see this attribute; the right default for industry programmers is to use the Rule of Zero. ...  End-users shouldn't be messing with attributes.
> > 
> > Neither of these statements matches my experience.  This is an "expert" feature to be sure, but the C++ community is full of experts who write their own rule-of-five types and who will happily use whatever attributes are available to them to make them faster.
> > 
> > Also, I assume you are intending for this attribute to be standardized eventually, which will greatly expand its reach.
> > Why would a class containing a member of type `boost::interprocess::offset_ptr` be trivially-relocatable? If you actually trivially relocate an object of the class, the pointer will not be rebased and so will be invalidated. It would have to be an offset_ptr where you happen to know that the referent will always be copied simultaneously, e.g. because it's a member of the object itself.
> 
> Exactly! (And to preserve the class invariant, you'd have to add a copy-constructor.)
> 
> > Of course that's possible, but it's also such a corner case that we shouldn't balk at saying that the programmer ought to be more explicit about recognizing it.
> 
> Exactly — and the way for the programmer to explicitly recognize (or I say "warrant") that their class has the property is for them to annotate it with `[[trivially_relocatable]]`. So I guess maybe I don't understand what you mean by "more explicit"?
> 
> > making the attribute take a `bool` argument is just a superior way of managing this
> 
> That's possible, but it's also possible that it would increase the complexity of parsing attributes for some implementations. I mean, we're talking about something like the following, right? (Using the libc++ patch as the example, but I've de-uglified some of the names.) So I think it's a tradeoff and I'm ambivalent about it, so far. (This is one of the [[ https://quuxplusone.github.io/blog/2018/11/11/trivially-relocatable-in-san-diego/#if-you-feel-comfortable-respondi | straw poll questions in P1144R0 ]].)
> ```
> template <class T, class A = allocator<T>>
> class [[trivially_relocatable(__deque_base<T, A>::__allow_trivial_relocation::value)]] deque
>     : private __deque_base<T, A>
> ```
> 
> > This is an "expert" feature to be sure, but the C++ community is full of experts who write their own rule-of-five types and who will happily use whatever attributes are available to them to make them faster.
> 
> Agreed. But the C++ community is //also// full of working programmers who just write simple code with strings and vectors. :) I want `[[trivially_relocatable]]` to be approximately as frequently seen in real codebases as `[[no_unique_address]]` — i.e. maybe a couple times in that smart-pointer library the contractor wrote, but nowhere near the user code. If it's seen frequently in user code, then we've failed those users.
> Exactly! (And to preserve the class invariant, you'd have to add a copy-constructor.)

But then it still wouldn't be trivially relocatable, because there's user-defined code that has to run to copy it correctly.  The only way such a type could ever be meaningfully trivially relocatable outside of obviously unknowable external conditions is if it has fields that it never uses after it's been relocated.

> Exactly — and the way for the programmer to explicitly recognize (or I say "warrant") that their class has the property is for them to annotate it with [[trivially_relocatable]]. So I guess maybe I don't understand what you mean by "more explicit"?

I think it is far more likely that some well-intentioned library author will add `[[trivially_relocatable]]` incorrectly than that they'll actually intend to override the trivial relocatability of their subobjects.

By "more explicit", I was suggesting that you add some kind of "force" syntax to the attribute (straw-man suggestion: `[[trivially_relocatable!]]`).  Without the force, the attribute will negate non-triviality from special members in the class but won't override natural non-triviality from subobjects.

> That's possible, but it's also possible that it would increase the complexity of parsing attributes for some implementations.

All conforming implementations have to do the work to support things like this already because of `alignas`, `noexcept`, etc.

> Agreed. But the C++ community is also full of working programmers who just write simple code with strings and vectors. :) I want [[trivially_relocatable]] to be approximately as frequently seen in real codebases as [[no_unique_address]] — i.e. maybe a couple times in that smart-pointer library the contractor wrote, but nowhere near the user code. If it's seen frequently in user code, then we've failed those users.

I think you are underestimating the sophistication of "working programmers" and overestimating the sophistication of library developers.  A `[[trivially_relocatable]]` that doesn't override subobject triviality is far easier for library authors to use correctly and will avoid an endless cascade of oversights.

Case in point, every single subtle thing that you had to anticipate and call out in your patch to `std::deque` would just go away if the language simply propagated non-triviality of subobjects.


Repository:
  rC Clang

https://reviews.llvm.org/D50119