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

Tue Nov 13 11:07:03 PST 2018

Quuxplusone 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)
----------------
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.

================
Comment at: lib/AST/Type.cpp:2234
+bool QualType::isTriviallyRelocatableType(const ASTContext &Context) const {
+  QualType T = Context.getBaseElementType(*this);
+  if (T->isIncompleteType())
----------------
erichkeane wrote:
> Quuxplusone wrote:
> > erichkeane wrote:
> > > You likely want to canonicalize here.
> > You mean `QualType T = Context.getBaseElementType(getCanonicalType());`?
> > I can do that. For my own edification (and/or a test case), in what way does the current code fail?
> More like: QualType T = Context.getBaseElementType(*this).getCanonicalType();
> 
> This desugars typedefs in some cases, which could make the below fail.  
> 
> Something like this: https://godbolt.org/z/-C-Onh
> 
> It MIGHT still pass here, but something else might canonicalize this along the way.
> 
> ADDITIONALLY, I wonder if this properly handles references?  I don't think getBaseElementType will de-reference. You might want to do that as well.
> ADDITIONALLY, I wonder if this properly handles references? I don't think getBaseElementType will de-reference. You might want to do that as well.

I actually don't want references to be stripped here: `int&` should come out as "not trivially relocatable" because it is not an object type.
https://p1144.godbolt.org/z/TbSsOA

Repository:
  rC Clang

https://reviews.llvm.org/D50119