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

David Blaikie via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Nov 14 12:21:18 PST 2018 

dblaikie 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:
> dblaikie wrote:
> > Quuxplusone wrote:
> > > rjmccall wrote:
> > > > Quuxplusone wrote:
> > > > > rjmccall wrote:
> > > > > > 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.
> > > > > > 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.
> > > > > 
> > > > > But overriding "natural" non-trivial relocatability is precisely the reason for P1144 `[[trivially_relocatable]]`! If you just have a plain old Rule-of-Zero object with trivially relocatable subobjects, and you want your object to be trivially relocatable as a result, the core language takes care of that for you (just like with trivial {con,de}structibility and trivial-abi-ness: a Rule-of-Zero composite of trivial objects is itself trivial). The //only// use-case for the `[[trivially_relocatable]]` attribute is when you are trying to tell the compiler that you know exactly what you're doing. (Which is why normal working programmers won't generally use it.)
> > > > > 
> > > > > > 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.
> > > > > 
> > > > > Can you give an example of how it would work with `deque`, for example? What part of the `deque` implementation would become simpler, in exchange for this added complexity of specification?
> > > > > 
> > > > > You say:
> > > > > > 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.
> > > > > 
> > > > > But I don't understand what you mean by this. Are you saying that you want to be able to write `class [[trivially_relocatable]] deque { ... }` to mean that you want the trivially-relocatable-ness of `deque` to match the trivially-relocatable-ness of its least relocatable subobject? But then for example in the libc++ patch, I'd either have to go out of my way to make sure that `__map` and `__deque_base` were trivially relocatable (which would require metaprogramming similar to what's there now, except one turtle lower down in the stack — or were you thinking of adding `[[trivially_relocatable]]` to //all// the turtles in the stack?), or else I'd have to use `class [[trivially_relocatable!]] deque` to overrule the non-trivial-relocatability of `deque`'s `__map` and `__deque_base` subobjects, in which case I'd still need the four lines you were trying to eliminate.
> > > > > https://github.com/Quuxplusone/libcxx/commit/6524822c009e#diff-38adc80cec663f2f29c22e9ffc0de912R957
> > > > > 
> > > > > My kneejerk reaction is that it is a bad idea to have two similarly-named things, one of which has subtly correct semantics and the other of which has subtly incorrect semantics, especially when it's not obvious which one is correct in any specific situation. (OT: This is basically the reason behind my [[ http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1155r0.html | P1155 ]].) But even beyond that //general// reaction, I //specifically// have not understood how `[[trivially_relocatable!]]` would help the programmer of `deque`.
> > > > > 
> > > > > > All conforming implementations have to do the work to support things like this already because of alignas, noexcept, etc.
> > > > > 
> > > > > Yes, but those aren't attributes, grammatically... Well, I guess there is already a standard grammar for parsing unknown attributes in terms of `balanced-token-seq`, and I can't think of any boolean expression that is not a `balanced-token-seq`, so okay, I'll retract my FUD over the //technical// difficulties of `[[trivially_relocatable(bool)]]`. I am still ambivalent as to whether it'd be a good tradeoff. (Complexity of specification and arcane terseness of code, versus simplicity of specification and boilerplate verbosity of code.)
> > > > > But then for example in the libc++ patch, I'd either have to go out of my way to make sure that __map and __deque_base were trivially relocatable (which would require metaprogramming similar to what's there now, except one turtle lower down in the stack — or were you thinking of adding [[trivially_relocatable]] to all the turtles in the stack?),
> > > > 
> > > > You would need the attribute only at the level(s) that actually defined the special members; all the "rule of zero" levels would of course propagate trivial relocatability.
> > > > 
> > > > > I specifically have not understood how [[trivially_relocatable!]] would help the programmer of deque.
> > > > 
> > > > It wouldn't.  I don't think it's worth adding at all, actually.  I'm just saying it's possible to add it if you really think that "trivially-relocatable type with a non-trivially-relocatable subobject" is a relevant use case.
> > > > 
> > > > > Yes, but those aren't attributes, grammatically...
> > > > 
> > > > True, `alignas` is not spelled as an attribute (which I constantly forget — it was a very odd decision).
> > > > 
> > > > Recent drafts of the standard do include `expects` and `ensures`, which take arbitrary expressions as operands.  The spelling's a bit different from the strawman I suggested, though: it's `[[expects: x > 0]]`.
> > > > 
> > > > > I can't think of any boolean expression that is not a balanced-token-seq
> > > > 
> > > > That's not a coincidence: all expressions are `balanced-token-seq`s, as is every other major production in the C++ grammar.  The grammar allows a `balanced-token-seq` there precisely because the committee was anticipating attributes that take arbitrarily complex expressions as operands, which are quite common in all the vendor extensions they were looking at.
> > > > 
> > > > > I am still ambivalent as to whether it'd be a good tradeoff. (Complexity of specification and arcane terseness of code, versus simplicity of specification and boilerplate verbosity of code.)
> > > > 
> > > > My two pieces of feedback are separable.  Even with your preferred semantics, your proposal would be much better if the attribute followed a `noexcept`-like design where it can optionally take a boolean argument.  In fact, adding that is much more important with your semantics for the attribute since so many use sites will be conditional.
> > > > 
> > > > Also, I don't think your semantics actually give rise to a simpler specification.  The specification has to formalize "naturally trivially relocatable" in either case because it's important for rule-of-zero types to be trivially relocatable if all their subobjects are.  My suggested semantics are basically just that the attribute doesn't override the natural relocatability of the type; it only prevents the presence of non-trivial special members from changing it.  That's a short paragraph in the spec in exchange for an attribute that's much less prone to errors in corner cases (e.g. ignoring the possibility of non-trivial pointer types) and which rarely needs to be explicitly conditionalized.  But it does make the (unsafe) override case impossible, although, again, I think the motivation for that is pretty weak.
> > > > I don't think your semantics actually give rise to a simpler specification. The specification has to formalize "naturally trivially relocatable" in either case
> > > 
> > > I think the spec is already simple: [[ http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1144r0.html#wording-inheritance | P1144R0 section 4.4 ]]. The paper spec doesn't need to formalize "naturally trivially relocatable"; that's an artifact of the Clang implementation.
> > > 
> > > > My suggested semantics are basically just that the attribute doesn't override the natural relocatability of the type; it only prevents the presence of non-trivial special members from changing it.
> > > 
> > > In the Clang patch, I'm //currently// using "naturally trivially relocatable" to mean "has defaulted special members //and// all subobjects are trivially relocatable." (So when I say "Xly Yly Zable," it implies "Yly Zable" as well as "Zable".) IIUC, you've been using it to mean "all subobjects are trivially relocatable but maybe there are user-provided special members"? I.e., if I had a class type with no data members, a user-defined destructor, and no attribute, you would have considered that class type to be "naturally trivially relocatable, but //not// trivially relocatable"? ("Xly Yly Zable" wouldn't imply "Yly Zable"?)
> > > 
> > > > if you really think that "trivially-relocatable type with a non-trivially-relocatable subobject" is a relevant use case
> > > 
> > > Yes, that use-case is absolutely indispensable. For example, we must provide a way for the programmer to express that `struct Widget { boost::shared_ptr<Widget> sp; };` is trivially relocatable. (Without upgrading our Boost distribution. ;)) In P1144R0, we can do that by putting the attribute on `Widget`, or by putting the attribute on an "extremely loud and incredibly narrow" wrapper class: 
> > > https://quuxplusone.github.io/blog/2018/10/04/trivially-relocatable-faq/#what-if-i-use-the-trivially_relo
> > "Yes, that use-case is absolutely indispensable. For example, we must provide a way for the programmer to express that struct Widget { boost::shared_ptr<Widget> sp; }; is trivially relocatable. (Without upgrading our Boost distribution. ;)) "
> > 
> > That's something I'd disagree with on first blush - why is this functionality absolutely indispensable?
> > 
> > I'd be pretty concerned about people effectively annotating types they don't own with a guarantee they probably can't actually make - how's someone going to make the determination that a third party type is safe to (indirectyl) add this attribute to? & what if that type changes in the future (or has a debug mode, or something else the user hasn't anticipated) - and it only solves the cases where the third party type is embedded within a user type - which, for something like shared_ptr misses lots of cases where the shared_ptr itself is passed around.
> > 
> > To me that doesn't seem like a "must have" feature - compared to the ability to annotate a type where I've written some user-defined special members, but I know/can guarantee (until I change the class in such a way that it doesn't have that feature - at which point I can remove the attribute too, because I control both implementation and attribution) that move+destroy is semantically equivalent to memcpy.
> > > Yes, that use-case is absolutely indispensable. For example, we must provide a way for the programmer to express that `struct Widget { boost::shared_ptr<Widget> sp; };` is trivially relocatable. (Without upgrading our Boost distribution. ;))
> 
> > That's something I'd disagree with on first blush - why is this functionality absolutely indispensable?
> 
> Because it's close to functionality that people are trying to use today (e.g. Folly uses `FOLLY_ASSUME_RELOCATABLE` to apply trivial relocatability onto non-annotated types such as `boost::shared_ptr` or `std::string`). Admittedly, this is a source of bugs. But without it, I think people would just ask to have it — because it's the thing that gives them the codegen they want.
I guess one of the reasons they're in this situation is that there isn't a standardized form that library authors can use - so they're stuck working around that on their end? & that wouldn't be such an issue if there was a standard way to do this that the compilers understood/respected, rather than only done on a per-library basis for cases where the library is implementing the relocation (rather than on ABI boundaries, etc).


Repository:
  rC Clang

https://reviews.llvm.org/D50119

More information about the cfe-commits mailing list