This is an archive of the discontinued LLVM Phabricator instance.

P1144 "Trivially relocatable" (0/3): Compiler support for `__is_trivially_relocatable(T)`
Needs ReviewPublic

Authored by Quuxplusone on Jul 31 2018, 5:20 PM.

Details

Summary

Part 0/3 is D50119: it adds __has_extension(__is_trivially_relocatable) to the compiler.
Part 1/3 is D61761: it implements the library traits and algorithms mandated by P1144.
Part 2/3 is D63620: it implements Quality-of-Implementation features to warrant certain std library types as "trivially relocatable."
Part 3/3 is D67524: it implements Quality-of-Implementation features to optimize certain std library functions for better performance on types that have been warranted "trivially relocatable."

This is the compiler half of C++ proposal 1144 "Object relocation in terms of move plus destroy," as seen on https://godbolt.org/g/zUUAVW and https://quuxplusone.github.io/blog/2018/07/18/announcing-trivially-relocatable/ .

There are two parts to this compiler support:

  • the type trait __is_trivially_relocatable(T), which is similar in spirit to __is_trivially_destructible(T), in that it lets the programmer access information that the compiler itself already knows; and
  • the warranting attribute [[clang::trivially_relocatable]], which is similar in spirit to [[clang::trivial_abi]], in that it lets the programmer communicate back to the compiler that a certain user-defined type should be assumed to have this property even though it would not naturally have the property all else being equal.

As proposed in P1144R3, [[clang::trivially_relocatable(some-bool-expr)]] can be used to make a type conditionally trivially relocatable. I used this version of the attribute very heavily in my libc++ patch D63620.

Furthermore, I've implemented @rjmccall's idea for an attribute [[clang::maybe_trivially_relocatable]] that says "My member functions don't interfere with my own trivial relocatability, but my data members' types might." Personally I have not used this version of the attribute, but I am certainly biased. See my C++Now 2019 session ( https://www.youtube.com/watch?v=SGdfPextuAU&t=40m ) for details on the different approaches to conditionally trivial relocatability.

The official home of this branch thus far has been https://github.com/Quuxplusone/clang/tree/trivially-relocatable , but I figure that a Clang review would be a good way to get some more eyeballs on it, plus, if we can get it into Clang trunk then I wouldn't have to keep rebasing it.

Diff Detail

Repository
rC Clang

Event Timeline

Rakete1111 added a subscriber: Rakete1111.EditedAug 1 2018, 10:04 AM
  • There's a bug in your implementation:
struct X {
  X &operator=(X &&);
};
static_assert(__is_trivially_relocatable(X)); // oops, fires!

X has a move constructor and a destructor, so it is trivially relocatable.

  • Please run your code through clang-format.
  • Might be useful to add a note explaining why the type isn't trivially relocatable instead of the general "because it is not destructible".
include/clang/Sema/Sema.h
4304

Any reason why this is a free function? Should be a member function of QualType.

lib/AST/DeclCXX.cpp
283

Lingering debug message? :) There are many of them.

For a single expression, drop the braces of the if statement.

lib/Sema/SemaDeclCXX.cpp
6066

This should be a // TODO: .... Is this comment really appropriate? The intended audience isn't compiler writers I think.

6157

This really just checks whether the type has defaulted copy constructor. If there was a move constructor, it would have been handled above. If the copy/move constructor is implicitly deleted, it would have been handled also above. Please simplify this accordingly.

6158

Why do you need to check whether the attribute is present or not? This is supposed to be whether the type is naturally trivially relocatable, so the presence of the attribute is not important.

6656

You don't actually need those three lines. This is already handled in CheckCompletedCXXClass.

test/SemaCXX/trivially-relocatable.cpp
43

Why does this restriction exist? None of the existing attributes have it and I don't see why it would make sense to disallow this.

472

This is the right place for regression tests. Add them to test/Parser/cxx-class.cpp.

Address feedback from @Rakete1111. Fix wrong ordering of 'class|struct|...' in the diagnostic. Add union test cases. Correctly drop the attribute whenever it is diagnosed as inapplicable.

include/clang/Basic/DiagnosticSemaKinds.td
2942

Might be useful to add a note explaining why the type isn't trivially relocatable instead of the general "because it is not destructible".

You mean, like, a series of notes pointing at "because its base class B is not destructible... because B's destructor is defined as deleted here"? I agree that might be helpful, but since it only happens when the programmer is messing around with the attribute anyway, I wouldn't want to do anything too innovative or LoC-consuming. I'd cut and paste ~10 lines of code from somewhere else that already does something like that if you point me at it, but otherwise I think it's not worth the number of extra codepaths that would need to be tested.

include/clang/Sema/Sema.h
4304

class QualType is defined in AST/, whereas all the C++-specific TriviallyRelocatable stuff is currently confined to Sema/. My impression was that I should try to preserve that separation, even if it meant being ugly right here. I agree that this is a stupid hack, and would love to get rid of it, but I think I need some guidance as to how much mixing of AST and Sema is permitted.

lib/Sema/SemaDeclCXX.cpp
6066

Similar to the //puts comments, this comment is appropriate for me but not for Clang. :) Will replace with a comment containing the actual proposed wording.

6157

Can you elaborate on this one? I assume you mean that some of lines 6157 through 6171 are superfluous, but I can't figure out which ones or how to simplify it.

6158

I just thought that checking the presence of the attribute would be cheaper than doing these lookups, so I was trying to short-circuit it. However, you are correct for two reasons:

  • The check passes 99.99% of the time anyway, so it's *everyone* paying a small cost just so that the 0.01% can be a bit faster. This is a bad tradeoff.
  • Skipping the check when the attribute is present is actually incorrect, in the case that the type is not relocatable and the attribute is going to be dropped. (It was not dropped in this revision. It will be dropped in the next revision.) In that case, even with the attribute, we still want to execute this codepath.
6656

Confirmed. Nice!

test/SemaCXX/trivially-relocatable.cpp
43

[[noreturn]] has it, and I am pretty sure that [[trivial_abi]] *ought* to have it, even though it currently doesn't. The intent is to make it harder for people to create ODR violations by declaring a type, using it in some way, and then saying "oh by the way this type was xxxxx all along."

struct S { S(S&&); ~S(); };
std::vector<S> vec;
struct [[trivially_relocatable]] S;  // ha ha, now you have to re-do all of vector's codegen!

Does that make sense as a reason it would be (mildly) beneficial to have this diagnostic?
You could still reasonably object to my assumptions that (A) the diagnostic is worth implementing in Clang and/or (B) the diagnostic is worth mandating in the Standard wording; but FWIW I do think it's very cheap both to implement and to specify.

(I will try to adjust the patch so that after the error is given, we remove the attribute from the class so as to make consistent any subsequent cascading errors. https://godbolt.org/g/nVRiow )

430

There's a bug in your implementation:

struct X {
  X &operator=(X &&);
};
static_assert(__is_trivially_relocatable(X)); // oops, fires!

X has a move constructor and a destructor, so it is trivially relocatable.

Actually, this X does *not* have a move constructor! And its copy constructor is implicitly deleted. So it is not relocatable, let alone trivially relocatable.
https://godbolt.org/g/ZsCqvd

<source>:15:7: error: call to implicitly-deleted copy constructor of 'X'
    X y(std::move(x));
      ^ ~~~~~~~~~~~~
<source>:10:6: note: copy constructor is implicitly deleted because 'X' has a user-declared move assignment operator
  X &operator=(X &&);
     ^
472

I believe both of these cases are already covered by other tests in the suite (in fact that's how I found they were bad); I just wanted to have the known trouble spots tested in this single file for my own convenience. I'll remove these two "regression tests."

Quuxplusone marked 9 inline comments as done.Aug 1 2018, 1:13 PM

clang-format.

Rakete1111 added inline comments.Aug 1 2018, 10:14 PM
compiler-explorer-llvm-commit.sh
1

What's this file? A mistake?

include/clang/AST/DeclCXX.h
471

That comment misses a "or is a reference".

include/clang/Basic/DiagnosticSemaKinds.td
2942

Fair enough.

include/clang/Sema/Sema.h
4304

Nah, it's fine. There are lots of C++ specific things in AST/, because the AST nodes represent C++-y stuff. Trivially copyable is also part of QualType, even though it's C++ specific.

lib/Sema/SemaDecl.cpp
15823

You don't need this. This is already handled by CheckCompleteCXXClass.

lib/Sema/SemaDeclAttr.cpp
6481

Why is this attribute under "Microsoft Attributes"? ;)

lib/Sema/SemaDeclCXX.cpp
6157

Actually, nvm. I was thinking of calling a few functions instead of call LookupSpecialMember, but I forgot that those functions can only work if there was previously a call to LookupSpecialMember :/.

6166

Extra braces.

6187

This is dead code. Record never needs an implicit move constructor at this point, because either 1) it never did or 2) it was defined above by LookupSpecialMember.

12631

Same, already handled by CheckCompleteCXXClass.

test/SemaCXX/trivially-relocatable.cpp
43

Didn't know [[noreturn]] had it. Ah I see. Sold :)

430

Oops, you're right ofc. Sorry

Quuxplusone marked 20 inline comments as done.

Further removal of dead code based on @Rakete1111's feedback.

compiler-explorer-llvm-commit.sh
1

Yeah, it's pipework for the Compiler Explorer integration. I manually deleted it from the previously uploaded diff, but forgot to delete it this time. You can ignore it.

include/clang/Sema/Sema.h
4304

Okay, moved!

lib/Sema/SemaDeclAttr.cpp
6481

I dunno, the random //comments seem pretty silly to me. There's nothing "Microsoft" about TrivialABI either (and no reason anyone should care, in this context, that EmptyBases or LayoutVersion are "Microsoft"). I just put it here because it's somewhat related to TrivialABI — and/or, to reduce code churn if anyone ever decides to Do The Right Thing and alphabetize these switch cases.

lib/Sema/SemaDeclCXX.cpp
6187

Confirmed that this code is never hit; and removed. Just for my own information: you're saying that the call to LookupSpecialMember on line 6179, even though it's looking up the destructor, will actually cause all the needsImplicitFootor flags to get resolved? And so also I guess I should never have been looking at those flags directly; I should have handled this case by calling LookupSpecialMember like I do on line 6196. Is that basically correct?

Quuxplusone marked 2 inline comments as done.Aug 2 2018, 12:59 PM
Rakete1111 added inline comments.Aug 2 2018, 1:04 PM
lib/Sema/SemaDeclCXX.cpp
6187

No, not the 6179 one, but the one before it 6163. Yeah you could have :)

Quuxplusone marked an inline comment as done.Aug 2 2018, 2:31 PM
Rakete1111 added inline comments.Aug 2 2018, 2:36 PM
lib/Sema/SemaDeclCXX.cpp
6091

Can you move this in ActOnFields? That way we avoid two iterations of the fields.

6187

Sorry for the noise, that isn't it because of the if statement right before 6163 :/. I was wrong...

Every implicit constructor is already defined before the call to CheckCompletedCXXClass (except if it's a template), so needsImplicitFootor is always false. This means that you can drop the if statement right before 6163, because it's always true.

I'm 99% sure of the previous paragraph. :)

Rakete1111 added inline comments.Aug 2 2018, 4:06 PM
lib/Sema/SemaDeclCXX.cpp
6174

The call to isTemplateInstantiation is wrong. Consider:

template<class T>
struct [[trivially_relocatable]] A {
  T t;
};

struct X {
  X() = default;
  X(X &&) = delete;
};

A<X> d;
static_assert(!__is_trivially_relocatable(decltype(d))); // oops, fires

There is also no diagnostic saying that A<X> cannot be marked [[trivially_relocatable]].

6176

Record is never being defined at this point, even for templates. It also always has a definition AFAIK.

Quuxplusone marked an inline comment as done.Aug 2 2018, 4:32 PM
Quuxplusone added inline comments.
lib/Sema/SemaDeclCXX.cpp
6091

Done. Btw, I notice that ActOnFields spends a lot of time doing dyn_cast<CXXRecordDecl>(Record) over and over. If I had commit privs, I'd refactor it to compute CXXRecordDecl *CXXRecord = dyn_cast_or_null<CXXRecordDecl>(Record); once at the very top of the function, and then use CXXRecord throughout.

6174

The absence of any diagnostics is intentional. We're saying that A is trivially relocatable except-of-course-when-it's-not-relocatable-at-all, similar to how templates currently work with constexpr.

However, the fact that __is_trivially_relocatable(A<X>) when !__is_constructible(A<X>, A<X>&&) does seem like a bug. I should probably move the isTemplateInstantiation check down to control just the diagnostic, eh?

6176

Yes, that matches my observations so far (but I'm running the tests again to confirm). I'd originally copied this formula from somewhere else, I forget where.

Rakete1111 added inline comments.
lib/Sema/SemaDeclCXX.cpp
6091

Done. :) Thanks

6174

Yes, good idea.

Quuxplusone marked 8 inline comments as done.

Correctly drop the attribute from non-relocatable instantiations of class templates (even though they don't print the diagnostic).

Quuxplusone added inline comments.Aug 3 2018, 1:04 PM
lib/Sema/SemaDeclCXX.cpp
6187

I notice that test/SemaCXX/implicit-member-functions.cpp has started failing in my branch, although there's a *possibility* that that's due to a change in master. I'm going to investigate a little bit.

Quuxplusone marked an inline comment as done.

Rebased, and ping!

It would be nice to be able to diagnose X:

template <typename T>
struct Foo {
  struct [[trivially_relocatable]] X { // no warning
    X(X &&) = delete;
  };
};

Foo<int> f; // no warning
static_assert(!__is_trivially_relocatable(Foo<void>::X)); // ok

But otherwise, you might want to put the attribute in the clang namespace. LGTM then, but you'll want to wait for someone more senior than me to review (and possibly accept) it.

A type with no move-constructor, and some non-defaulted copy-constructors (even if it *also* has some defaulted copy-constructors) should not be considered naturally trivially relocatable. The paper already said this, but the Clang patch was misimplemented.

Thanks to @Rakete1111 for either finding or inspiring this fix. :)

I suspect there may be some way of doing this without the extra bitflag, but I didn't immediately see how.

Update to match the wording in D1144R0 draft revision 13. There's no longer any need to fail when we see a mutable member; on the other hand, we *always* need to perform the special member lookup to find out if our class is move-constructible and destructible.

You'll not be able to get the C++ spelling without it getting into the standard. Additionally, it seems easy enough to calculate upon need, so storing it in the AST is a waste. @aaron.ballman is likely another person who can review this.

docs/LanguageExtensions.rst
1093

How would this behave with unions? I don't see any exclusions happening on union-ness. A CXXRecordDecl can be a union as well as a class.

include/clang/AST/DeclCXX.h
474

Typically we'd have this calculated when requested rather than stored. I suspect using a bit for something like this isn't going to be terribly acceptable.

include/clang/Basic/Attr.td
2096

This spelling is almost definitely not acceptable until it is in the standard. Also, why specify that it was added in 2008?

include/clang/Basic/DiagnosticSemaKinds.td
8207

I'm shocked that there isn't a different diagnostic to do this same thing. @aaron.ballman likely knows better... I haven't seen the usage yet, but I presume you don't necessarily want a behavior that doesn't allow forward declarations.

lib/AST/Type.cpp
2234 ↗(On Diff #161866)

You likely want to canonicalize here.

lib/Parse/ParseDeclCXX.cpp
3811

A note for future reviewers, after the C++11 spelling is removed, this likely needs to go as well.

Quuxplusone marked an inline comment as done.Nov 13 2018, 8:19 AM
Quuxplusone added inline comments.
docs/LanguageExtensions.rst
1093

My intent is to make Clang's behavior match the wording in P1144R0; so, it should work for unions as well as for structs/classes. Any union which is trivially move-constructible and trivially destructible will be __is_trivially_relocatable. Any union which is non-trivially destructible *must* have a user-provided destructor, and therefore will *not* be __is_trivially_relocatable unless the user has annotated it with the attribute.
https://p1144.godbolt.org/z/F06TTQ

include/clang/AST/DeclCXX.h
474

You know better than I do; but I'm also not sure how to calculate it on request.

include/clang/Basic/Attr.td
2096

Agreed, it should be [[clang::trivially_relocatable]] for Clang's purposes. This spelling was because this patch came from the Godbolt Compiler Explorer patch where I wanted the shorter/future spelling for public relations reasons. :)
IIUC, the appropriate fix here is to change these two lines from

let Spellings = [CXX11<"", "trivially_relocatable", "200809">,
              CXX11<"clang", "trivially_relocatable">];

to

let Spellings = [Clang<"trivially_relocatable">,

I believe the "200809" was because I wanted it to be available in C++11 mode on Godbolt.

include/clang/Basic/DiagnosticSemaKinds.td
8207

I would be very happy to see this diagnostic get into trunk separately and earlier than D50119. There are some other diagnostics that could be merged with this one, e.g. [[noreturn]] needs a version of this diagnostic, and I believe [[clang::trivial_abi]] should have it added.

I don't know how to link to comments on Phabricator, but Ctrl+F downward for this example:

struct S { S(S&&); ~S(); };
std::vector<S> vec;
struct [[trivially_relocatable]] S;  // ha ha, now you have to re-do all of vector's codegen!

This is why it is important to diagnose and disallow "backward declarations." I don't particularly care about "forward declarations" (either to allow or disallow them). The attribute would end up getting used only on library types where IMHO nobody should ever be forward-declaring them anyway. E.g. it is not a good idea for a regular C++ programmer to forward-declare unique_ptr. But if there's a way to allow forward-declarations (when the type remains incomplete) while disallowing backward-declarations (adding the attribute to an already-complete type), then I will be happy to do it.

lib/AST/Type.cpp
2234 ↗(On Diff #161866)

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?

Hey @rjmccall - I'm trying to remember, but can't get it quite clear in my head. I seem to recall some discussion about trivial_abi not implying/being strong enough for all the cases that trivial_relocatable sounds like it covers? Do you happen to remember the distinction there (the summary in this patch ("the warranting attribute [[trivially_relocatable]], which is similar in spirit to [[trivial_abi]], in that it lets the programmer communicate back to the compiler that a certain user-defined type should be assumed to have this property even though it would not naturally have the property all else being equal.") doesn't sound like what I remember - but my memory is hazy)?

erichkeane added inline comments.Nov 13 2018, 8:36 AM
docs/LanguageExtensions.rst
1093

Makes sense.

include/clang/AST/DeclCXX.h
474

You'll end up just recursively walking the CXXRecordDecl upon application of the type trait, and need to check the things that make it NOT trivially relocatable.

I think it also extensively simplifies this patch, since none of the CXXRecordDecl functions are required (just the type-trait emission).

include/clang/Basic/Attr.td
2096

Yep, thats the suggested fix.

include/clang/Basic/DiagnosticSemaKinds.td
8207

Sure, I get that... I'm just surprised/amazed it doesn't already exist. Hopefully Aaron takes a look.

lib/AST/Type.cpp
2234 ↗(On Diff #161866)

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.

Hey @rjmccall - I'm trying to remember, but can't get it quite clear in my head. I seem to recall some discussion about trivial_abi not implying/being strong enough for all the cases that trivial_relocatable sounds like it covers? Do you happen to remember the distinction there (the summary in this patch ("the warranting attribute [[trivially_relocatable]], which is similar in spirit to [[trivial_abi]], in that it lets the programmer communicate back to the compiler that a certain user-defined type should be assumed to have this property even though it would not naturally have the property all else being equal.") doesn't sound like what I remember - but my memory is hazy)?

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

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

If there are corner-case differences beyond that, I have no particular objection to unifying the semantics so that trivial_abi is strictly stronger, although we should talk about it case-by-case.

test/SemaCXX/trivially-relocatable.cpp
43

I don't see any good reason to allow either this or [[trivial_abi]] on an arbitrary declaration of the type; it should have to be written on the definition. This also gives sensible semantics for class template specializations.

The intrinsic obviously has to require its operand type to be complete, and therefore there is a unique declaration which can have the attribute.

Quuxplusone marked 3 inline comments as done.Nov 13 2018, 10:16 AM
Quuxplusone added inline comments.
docs/LanguageExtensions.rst
1096

@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

rjmccall added inline comments.Nov 13 2018, 10:50 AM
docs/LanguageExtensions.rst
1096

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.

docs/LanguageExtensions.rst
1096

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.

lib/AST/Type.cpp
2234 ↗(On Diff #161866)

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

erichkeane added inline comments.Nov 13 2018, 11:09 AM
lib/AST/Type.cpp
2234 ↗(On Diff #161866)

Ah, I see. For some reason I had it in my mind that you wanted to follow references.

rjmccall added inline comments.Nov 13 2018, 11:34 AM
docs/LanguageExtensions.rst
1096

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.

docs/LanguageExtensions.rst
1096

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

rjmccall added inline comments.Nov 13 2018, 3:41 PM
docs/LanguageExtensions.rst
1096

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.

docs/LanguageExtensions.rst
1096

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

rjmccall added inline comments.Nov 14 2018, 12:41 AM
docs/LanguageExtensions.rst
1096

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-seqs, 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.

aaron.ballman added inline comments.Nov 14 2018, 8:43 AM
include/clang/Basic/Attr.td
2096

to
let Spellings = [Clang<"trivially_relocatable">,

Close, but since this attribute has no meaning in C mode, it should be let Spellings = [Clang<"trivially_relocatable", 0>,

include/clang/Basic/DiagnosticSemaKinds.td
8207

Yeah, I'm also surprised this doesn't already exist, but... it doesn't seem to. We have err_noreturn_missing_on_first_decl, err_carries_dependency_missing_on_first_decl, and err_internal_linkage_redeclaration to all do the same thing.

docs/LanguageExtensions.rst
1096

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: 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

dblaikie added inline comments.Nov 14 2018, 11:55 AM
docs/LanguageExtensions.rst
1096

"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.

docs/LanguageExtensions.rst
1096

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.

dblaikie added inline comments.Nov 14 2018, 12:21 PM
docs/LanguageExtensions.rst
1096

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

rjmccall added inline comments.Nov 14 2018, 12:24 PM
docs/LanguageExtensions.rst
1096

I think the spec is already simple: P1144R0 section 4.4. The paper spec doesn't need to formalize "naturally trivially relocatable"; that's an artifact of the Clang implementation.

I misunderstood how you were using "naturally trivially relocatable", sorry. What I mean is that the specification has to formalize rules for deciding and propagating trivial relocatability for arbitrary class types, which is the main source of complexity in the specification.

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"

Correct. And "naturally trivially relocatable" in this sense is not a generally-interesting property — it's only used as part of deciding trivial relocability in the presence of the attribute.

I completely agree with Dave's points. The short-term needs of programmers who want specific optimizations but are reluctant to make trivial changes to their local copies of Boost do not seem like they should dictate the long-term language design.

jfb added a subscriber: jfb.Nov 16 2018, 1:17 PM
Quuxplusone marked 14 inline comments as done.
Quuxplusone edited the summary of this revision. (Show Details)

Use [[clang::trivially_relocatable]] instead of [[trivially_relocatable]].
Canonicalize in QualType::isTriviallyRelocatableType.
Slight wording tweaks to the documentation.

include/clang/AST/DeclCXX.h
474

@erichkeane: I tried to follow this advice tonight, but I can't find any other type trait in Clang that is implemented the way you described. Every other type trait seems to be implemented in exactly the way I did it: isPolymorphic, isAbstract, isStandardLayoutType, isTrivialType, isAggregateType, hasTrivialDefaultConstructor... they all seem to occupy a bit in CXXRecordDecl. None of them seem to be computed by walking the fields() of a CXXRecordDecl on the fly — and that makes sense to me, because that would be an arbitrarily slow operation, whereas keeping the bit is O(1) no matter how many times the result is asked for.
If calculating traits on request is "typical," can you point me to somewhere in Clang where it happens, so that I can try to copy how they do it?

rjmccall added inline comments.
include/clang/AST/DeclCXX.h
474

Most of the other unary traits actually have to be computed just for at least some aspect of normal compiler operation, but that's not true of this, so I can understand the desire to avoid burdening the logic. In this case, though, I think it's fine because it's pretty trivial to collect this when we're already collecting almost all of the relevant information as part of the trivial-ABI calculation. In fact, it meshes well with some things we've thought about doing for some non-standard sources of non-triviality; CC'ing Akira Hatanaka for his thoughts.

lib/Parse/ParseDeclCXX.cpp
3811

With this line:

struct [[clang::trivially_relocatable()]] A {};
// expected-error@-1 {{attribute 'trivially_relocatable' cannot have an argument list}}
struct [[clang::trivially_relocatable(42)]] B {};
// expected-error@-1 {{attribute 'trivially_relocatable' cannot have an argument list}}

Without this line:

struct [[clang::trivially_relocatable()]] A {};
struct [[clang::trivially_relocatable(42)]] B {};
// expected-error@-1 {{'trivially_relocatable' attribute takes no arguments}}

IMO the former behavior (with this line) is much preferable to the latter behavior. I think I wish this switch statement included all attributes!

Implement [[clang::maybe_trivially_relocatable]] along the lines suggested by @rjmccall. The idea is that there are two levels of "opt-in-ness."

The first level, [[clang::maybe_trivially_relocatable]], means "I warrant that even though I may have user-provided, non-defaulted, special member functions, I have designed them so that my relocation operation will not do anything substantially different from memberwise relocation." So if all of my member+base subobjects are trivially relocatable (and not mutable and not volatile), then I myself will be trivially relocatable.

The second level, [[clang::trivially_relocatable]], means "I warrant that even though I may have user-provided, non-defaulted, special member functions, and even though I may have non-trivially relocatable (or mutable or volatile) subobjects, I have designed them so that my relocation operation will not do anything substantially different from memcpy." So I myself will be trivially relocatable no matter what my subobjects claim about themselves. Significantly, this means that I can write a class that overrules a decision made by one of its members.

  • I can make a TriviallyRelocatableWidget that encloses a heap and a bunch of offset_ptrs into that heap, even though a single offset_ptr in isolation is not trivially relocatable.
  • I can make a TriviallyRelocatableWidget that encloses a boost::shared_ptr<int>, even though the maintainer of boost::shared_ptr may not have marked boost::shared_ptr as trivially relocatable.
  • (Of dubious usefulness) I can make a BaseClass that is not marked trivially relocatable, and then make a TriviallyRelocatableDerivedClass that strengthens the guarantees of its BaseClass while remaining Liskov-substitutable for it.

Now that [[clang::maybe_trivially_relocatable]] is implemented, we can see if it does actually simplify the libc++ patch. I will try to get to that tonight. The current libc++ patch is here:
https://github.com/Quuxplusone/libcxx/tree/trivially-relocatable

Now that [[clang::maybe_trivially_relocatable]] is implemented, we can see if it does actually simplify the libc++ patch. I will try to get to that tonight.

Here is deque implemented with [[trivially_relocatable]]: https://github.com/Quuxplusone/libcxx/compare/0533994b3fcb115fdd89a38f80f737de8e76d023...1df78ab704b8b3d5a5e225a7624eb5fe10e3f401
And deque implemented with [[maybe_trivially_relocatable]]:
https://github.com/Quuxplusone/libcxx/compare/0533994b3fcb115fdd89a38f80f737de8e76d023...5945ccee2f7bb30fbb4e3a7cf9308ee8145c758b

Here is unordered_set implemented with [[trivially_relocatable]]:
https://github.com/Quuxplusone/libcxx/compare/0533994b3fcb115fdd89a38f80f737de8e76d023...8ddd963c738cef0c3ad5b314746ac5ddc2415751
And unordered_set implemented with [[maybe_trivially_relocatable]]:
https://github.com/Quuxplusone/libcxx/compare/0533994b3fcb115fdd89a38f80f737de8e76d023...0e8ddfe99145fe69a18a3fd8929674d937f22b99

The [[maybe_trivially_relocatable]] patches are much shorter (18 and 30 lines) than the [[trivially_relocatable]] patches (78 and 145 lines). However, it's worth noting that 56 of the lines in the unordered_set [[trivially_relocatable]] patch are there only to deal with C++03's lack of using-constructor declarations, and would disappear if we were working on a C++11-only codebase.

In the unordered_set [[trivially_relocatable]] patch, I use the "triviality-forcing wrapper" pattern which would not be possible with [[maybe_trivially_relocatable]]. This saves me from having to pipe the attribute down through unique_ptr and __bucket_list_deallocator.

In the unordered_set [[maybe_trivially_relocatable]] patch, I must wrap the attribute in a macro _LIBCPP_MAYBE_TRIVIALLY_RELOCATABLE_UNLESS_DEBUG. Without this caveat, I would have ended up with unsafe behavior in debug mode. The unordered_set [[trivially_relocatable]] patch does not have this danger; the fact that we break the Rule of Zero in debug mode is sufficient to disable trivial relocatability.

In the unordered_set [[maybe_trivially_relocatable]] patch, I must either pipe the attribute down through unique_ptr, or else give up using unique_ptr in __hash_table. I chose to modify unique_ptr; but note that many working programmers wouldn't have this luxury. In the unordered_set [[trivially_relocatable]] patch, I don't need to modify unique_ptr; I just use the "triviality-forcing wrapper" pattern.

In the unordered_set [[trivially_relocatable]] patch, I chose to "cheat" and write is_trivially_relocatable<__next_pointer>::value && is_trivially_relocatable<__pointer_allocator>::value instead of piping trivial relocatability down through __bucket_list_deallocator. If I'd done that, the patch would have gotten even longer. (And for no benefit, since __bucket_list_deallocator is a private helper class never directly touched by the user.)

So, both ways have their "cheats" and both ways have their dangers-of-unsafe-behavior. The [[maybe_trivially_relocatable]] patches are significantly shorter. The [[trivially_relocatable]] patches touch fewer classes. I'm still more comfortable with [[trivially_relocatable]], but I can see the appeal of terser code.

I still believe it is impossible to implement std::optional with only [[maybe_trivially_relocatable]].

In the unordered_set [[maybe_trivially_relocatable]] patch, I must wrap the attribute in a macro _LIBCPP_MAYBE_TRIVIALLY_RELOCATABLE_UNLESS_DEBUG. Without this caveat, I would have ended up with unsafe behavior in debug mode. The unordered_set [[trivially_relocatable]] patch does not have this danger; the fact that we break the Rule of Zero in debug mode is sufficient to disable trivial relocatability.

Can you elaborate? Providing a non-defaulted copy/move constructor or destructor should make an unannotated class non-trivially-relocatable under both rules.

In the unordered_set [[maybe_trivially_relocatable]] patch, I must either pipe the attribute down through unique_ptr, or else give up using unique_ptr in __hash_table. I chose to modify unique_ptr; but note that many working programmers wouldn't have this luxury. In the unordered_set [[trivially_relocatable]] patch, I don't need to modify unique_ptr; I just use the "triviality-forcing wrapper" pattern.

Sure, this is an acknowledged downside of [[maybe_trivially_relocatable]] for third-party programmers.

I still believe it is impossible to implement std::optional with only [[maybe_trivially_relocatable]].

This might also need elaboration.

Quuxplusone added a comment.EditedNov 19 2018, 4:21 PM

In the unordered_set [[maybe_trivially_relocatable]] patch, I must wrap the attribute in a macro _LIBCPP_MAYBE_TRIVIALLY_RELOCATABLE_UNLESS_DEBUG. Without this caveat, I would have ended up with unsafe behavior in debug mode. The unordered_set [[trivially_relocatable]] patch does not have this danger; the fact that we break the Rule of Zero in debug mode is sufficient to disable trivial relocatability.

Can you elaborate? Providing a non-defaulted copy/move constructor or destructor should make an unannotated class non-trivially-relocatable under both rules.

In the patch I was describing, I had annotated unordered_set. However, your comment made me realize that after annotating __hash_table, I could completely drop the [[maybe_trivially_relocatable]] annotation on unordered_set, and hide its move-constructor behind #if _LIBCPP_DEBUG_LEVEL >= 2 the same way I do in the [[trivially_relocatable]] patch. So I retract that complaint.

Here is unordered_set implemented with [[trivially_relocatable]]:
https://github.com/Quuxplusone/libcxx/compare/0533994b3fcb115fdd89a38f80f737de8e76d023...8ddd963c738cef0c3ad5b314746ac5ddc2415751
And here's the new and improved version of unordered_set implemented with [[maybe_trivially_relocatable]]:
https://github.com/Quuxplusone/libcxx/compare/0533994b3fcb115fdd89a38f80f737de8e76d023...0e8ddfe99145fe69a18a3fd8929674d937f22b99

I still believe it is impossible to implement std::optional with only [[maybe_trivially_relocatable]].

This might also need elaboration.

Because __optional_destruct_base contains an anonymous union member, and that union type is not destructible; therefore that union type is not trivially relocatable; therefore __optional_destruct_base contains a member of non-trivially-destructible [OOPS: I mean non-trivially-relocatable] type. However, I'm working on changing my patch to make anonymous unions "see-through" in this respect, so that that union's non-destructibility doesn't interfere with the [[maybe_trivially_relocatable]]ity of its enclosing class type, as long as all the members of the union are trivially relocatable. This might fix optional. I'm not sure yet.

In the unordered_set [[maybe_trivially_relocatable]] patch, I must wrap the attribute in a macro _LIBCPP_MAYBE_TRIVIALLY_RELOCATABLE_UNLESS_DEBUG. Without this caveat, I would have ended up with unsafe behavior in debug mode. The unordered_set [[trivially_relocatable]] patch does not have this danger; the fact that we break the Rule of Zero in debug mode is sufficient to disable trivial relocatability.

Can you elaborate? Providing a non-defaulted copy/move constructor or destructor should make an unannotated class non-trivially-relocatable under both rules.

In the patch I was describing, I had annotated unordered_set. However, your comment made me realize that after annotating __hash_table, I could completely drop the [[maybe_trivially_relocatable]] annotation on unordered_set, and hide its move-constructor behind #if _LIBCPP_DEBUG_LEVEL >= 2 the same way I do in the [[trivially_relocatable]] patch. So I retract that complaint.

Here is unordered_set implemented with [[trivially_relocatable]]:
https://github.com/Quuxplusone/libcxx/compare/0533994b3fcb115fdd89a38f80f737de8e76d023...8ddd963c738cef0c3ad5b314746ac5ddc2415751
And here's the new and improved version of unordered_set implemented with [[maybe_trivially_relocatable]]:
https://github.com/Quuxplusone/libcxx/compare/0533994b3fcb115fdd89a38f80f737de8e76d023...0e8ddfe99145fe69a18a3fd8929674d937f22b99

Okay, thank you.

I still believe it is impossible to implement std::optional with only [[maybe_trivially_relocatable]].

This might also need elaboration.

Because __optional_destruct_base contains an anonymous union member, and that union type is not destructible; therefore that union type is not trivially relocatable; therefore __optional_destruct_base contains a member of non-trivially-destructible type. However, I'm working on changing my patch to make anonymous unions "see-through" in this respect, so that that union's non-destructibility doesn't interfere with the [[maybe_trivially_relocatable]]ity of its enclosing class type, as long as all the members of the union are trivially relocatable. This might fix optional. I'm not sure yet.

Ah, that makes sense. And yeah, that seems like the right rule for unions. I really appreciate you putting the effort into exploring this part of the design space.

I still believe it is impossible to implement std::optional with only [[maybe_trivially_relocatable]].

This might also need elaboration.

Because __optional_destruct_base contains an anonymous union member, and that union type is not destructible; therefore that union type is not trivially relocatable; therefore __optional_destruct_base contains a member of non-trivially-destructible type. However, I'm working on changing my patch to make anonymous unions "see-through" in this respect, so that that union's non-destructibility doesn't interfere with the [[maybe_trivially_relocatable]]ity of its enclosing class type, as long as all the members of the union are trivially relocatable. This might fix optional. I'm not sure yet.

Ah, that makes sense. And yeah, that seems like the right rule for unions. I really appreciate you putting the effort into exploring this part of the design space.

I think it's part of the right rule, but it's not enough, either. Consider:

struct [[clang::maybe_trivially_relocatable]] DestroyBase {
    int i;
    DestroyBase(DestroyBase&&) = delete;
    ~DestroyBase();
};
struct [[clang::maybe_trivially_relocatable]] MoveBase : DestroyBase {
    MoveBase(MoveBase&&);
};

static_assert(not std::is_move_constructible_v<DestroyBase>); //, therefore
static_assert(not std::is_relocatable_v<DestroyBase>); //, therefore
static_assert(not std::is_trivially_relocatable_v<DestroyBase>); //; and since MoveBase now has a base class of non-trivially-relocatable type,
static_assert(not std::is_trivially_relocatable_v<MoveBase>);

So maybe you need a notion like "a class is BlahBlah if all of its direct members are BlahBlah, and either it is annotated [[maybe_trivially_relocatable]] or else it has no non-defaulted move or destroy operations." And then "a move-constructible, destructible class is trivially relocatable if it is BlahBlah, or if it is annotated [[trivially_relocatable]]." And then a class like DestroyBase can be "BlahBlah but not move-constructible," and if it's placed inside MoveBase, and MoveBase is both annotated and move-constructible, then MoveBase becomes trivially relocatable.

This does feel like it would be twice as hard to specify in the Standard, because I think I'd have to specify both the rules for BlahBlah and the rules for "trivially relocatable."

I'm also extremely concerned about the philosophical correctness of separating BlahBlahness from actual physical relocatability. What does it mean, abstractly, to say that "DestroyBase is not even moveable at all, but if it were, I'm confident that its relocation operation would be trivial"? That's some weird counterfactual logic that forces the writer of the class (DestroyBase in this case, but also e.g. lock_guard) to think about how their class might get used downstream, and what the class's hypothetical invariants might be, under an operation (move+destroy) that it itself does not claim to support.

The benefit of my original "second-level" [[trivially_relocatable]] attribute is that it puts all the responsibility in a single very high-level place. optional is the thing that knows exactly when it's trivially relocatable (or at least can make a conservative guess, which might boil down to "I'm never trivially relocatable"), so optional is the thing that should get the attribute.

If I get time today I'll try adding the boolean argument to the attribute, and do the libc++ patches that way for comparison.

I still believe it is impossible to implement std::optional with only [[maybe_trivially_relocatable]].

This might also need elaboration.

Because __optional_destruct_base contains an anonymous union member, and that union type is not destructible; therefore that union type is not trivially relocatable; therefore __optional_destruct_base contains a member of non-trivially-destructible type. However, I'm working on changing my patch to make anonymous unions "see-through" in this respect, so that that union's non-destructibility doesn't interfere with the [[maybe_trivially_relocatable]]ity of its enclosing class type, as long as all the members of the union are trivially relocatable. This might fix optional. I'm not sure yet.

Ah, that makes sense. And yeah, that seems like the right rule for unions. I really appreciate you putting the effort into exploring this part of the design space.

I think it's part of the right rule, but it's not enough, either. Consider:

struct [[clang::maybe_trivially_relocatable]] DestroyBase {
    int i;
    DestroyBase(DestroyBase&&) = delete;
    ~DestroyBase();
};
struct [[clang::maybe_trivially_relocatable]] MoveBase : DestroyBase {
    MoveBase(MoveBase&&);
};

static_assert(not std::is_move_constructible_v<DestroyBase>); //, therefore
static_assert(not std::is_relocatable_v<DestroyBase>); //, therefore
static_assert(not std::is_trivially_relocatable_v<DestroyBase>); //; and since MoveBase now has a base class of non-trivially-relocatable type,
static_assert(not std::is_trivially_relocatable_v<MoveBase>);

Hmm. I don't remember what we do in this case for trivial_abi (which does need to address it because it's possible to return types like DestroyBase). It looks like we currently make it non-trivial. But trivial-ABI needs to consider copy constructors as well as move constructors, which would undermine my case that it should be a subset of trivially-relocatability, at least in the fantasy world where there are correct types which default their move constructors but not their copy constructors or vice-versa.

So maybe you need a notion like "a class is BlahBlah if all of its direct members are BlahBlah, and either it is annotated [[maybe_trivially_relocatable]] or else it has no non-defaulted move or destroy operations." And then "a move-constructible, destructible class is trivially relocatable if it is BlahBlah, or if it is annotated [[trivially_relocatable]]." And then a class like DestroyBase can be "BlahBlah but not move-constructible," and if it's placed inside MoveBase, and MoveBase is both annotated and move-constructible, then MoveBase becomes trivially relocatable.

I'm completely comfortable with saying that MoveBase just isn't trivially-relocatable under maybe_trivially_relocatable attribute, and if you've somehow arranged for it to be, you need to use the stronger attribute. In practice types like this either won't exist or won't actually satisfy the requirements. That should allow you to avoid distinguishing between BlahBlah and trivially-relocatable. As long as the analysis jumps directly down to variant members instead of treating anonymous unions as formal subobjects, I don't think this will cause serious limitations or usability problems. Variant members are important as a special case only because anonymous unions can't really be fixed otherwise, and it's consistent with basically every other language rule around construction and destruction.

The benefit of my original "second-level" [[trivially_relocatable]] attribute is that it puts all the responsibility in a single very high-level place. optional is the thing that knows exactly when it's trivially relocatable (or at least can make a conservative guess, which might boil down to "I'm never trivially relocatable"), so optional is the thing that should get the attribute.

Yes, it's definitely unambiguous what happens with your stronger attribute.

If I get time today I'll try adding the boolean argument to the attribute, and do the libc++ patches that way for comparison.

Okay. Sorry for the delayed response, I had last week off.

Hmm. I don't remember what we do in this case for trivial_abi (which does need to address it because it's possible to return types like DestroyBase). It looks like we currently make it non-trivial. But trivial-ABI needs to consider copy constructors as well as move constructors, which would undermine my case that it should be a subset of trivially-relocatability, at least in the fantasy world where there are correct types which default their move constructors but not their copy constructors or vice-versa.

Submitted for your consideration:

template<class T>
class HeapResource {
    std::unique_ptr<T> ptr_;
    HeapResource(const HeapResource& rhs) : ptr_(rhs.ptr_->clone()) {}  // not defaulted
    HeapResource(HeapResource&&) = default;  // defaulted
    ~HeapResource() = default;
};

This type is isomorphic to a real-world implementation of std::function. When you copy a HeapResource, you copy a T (which is observable); when you move a HeapResource, you do not touch T at all. Therefore copy-and-destroy is observably different from move-and-destroy. But (A) as a library vendor, I don't think I would care about clients who pathologically try to observe the difference; and (B) I don't think I grasp exactly how [[trivial_abi]] would care about this case either. (But remember: I still think of [[trivial_abi]] as completely orthogonal. You cut that debate short. ;) I would see no intrinsic difficulty with marking HeapResource as [[trivial_abi]], for example.)

So maybe you need a notion like "a class is BlahBlah if all of its direct members are BlahBlah, and either it is annotated [[maybe_trivially_relocatable]] or else it has no non-defaulted move or destroy operations." And then "a move-constructible, destructible class is trivially relocatable if it is BlahBlah, or if it is annotated [[trivially_relocatable]]." And then a class like DestroyBase can be "BlahBlah but not move-constructible," and if it's placed inside MoveBase, and MoveBase is both annotated and move-constructible, then MoveBase becomes trivially relocatable.

I'm completely comfortable with saying that MoveBase just isn't trivially-relocatable under maybe_trivially_relocatable attribute, and if you've somehow arranged for it to be, you need to use the stronger attribute.

To clarify: you'd be completely comfortable with the user's having the ability to "use the stronger attribute"? That is, there seems to be a reasonable argument in favor of the existence of the stronger attribute, even in the presence of the weaker attribute?

In practice types like this either won't exist or won't actually satisfy the requirements.

I think we probably agree on the nature of reality here, but I don't understand the way you phrased this sentence. I mean, DestroyBase and MoveBase absolutely do exist, in libc++'s implementation of optional. If you just mean that "in practice all such types will be private implementation details, and we'll never have to worry about the implications of these types' advertising themselves as trivially relocatable because they'll only ever be used in highly controlled circumstances where we can trust the programmer not to fall into any pits of their own making," then I agree, I think.

If I get time today I'll try adding the boolean argument to the attribute, and do the libc++ patches that way for comparison.

Okay. Sorry for the delayed response, I had last week off.

No worries. I didn't have last week off, and I still didn't actually get around to adding the boolean argument yet.

Speaking of the boolean argument... The usual usage will be something like [[trivially_relocatable(is_trivially_relocatable_v<allocator_type> && is_trivially_relocatable_v<pointer> && is_trivially_relocatable_v<size_type>)]], basically listing out the types of all the class's non-static data members. This has suddenly suggested to me that maybe the compiler could generate this kind of thing automatically... which we'd naturally spell [[trivially_relocatable(auto)]]! That is, maybe the right spelling for [[maybe_trivially_relocatable]] is [[trivially_relocatable(auto)]]!
...Or, on second thought, maybe not. That seems super prone to overuse and thus abuse, like, if someone blindly applied it to their codebase's equivalent of std::list. Yeah, I keep coming back to: the best way to avoid accidental knife injuries is to keep the knife sharp.

Quuxplusone retitled this revision from Compiler support for P1144R0 "__is_trivially_relocatable(T)" to P1144 "Trivially relocatable" (0/3): Compiler support for `__is_trivially_relocatable(T)`.Sep 12 2019, 3:00 PM
Quuxplusone edited the summary of this revision. (Show Details)

Migrate this patch to the monorepo.
Apply @Mordante's improvements and test-suite fixes.
This patch now passes all tests (although it had to change the expected output of some of them).

martong resigned from this revision.Jan 7 2020, 7:35 AM

@rjmccall ping? any further thoughts on [[maybe_trivially_relocatable]], in light of the followup libc++ patches? (Those patches do not use [[maybe_trivially_relocatable]], although that may just be because I wrote them.)

How could one go about getting this patch into Clang?

Also, for those who haven't seen it, I gave a full-length talk on "trivially relocatable" at C++Now 2019: https://www.youtube.com/watch?v=SGdfPextuAU The talk compares my fully implemented Clang/libc++ solution https://wg21.link/p1144 to other practical solutions such as Folly, BSL, and EASTL, and to other paper solutions such as https://wg21.link/n4158 and https://wg21.link/p1029.

If the committee is actively looking into this problem and considering multiple alternatives, I don't see a particular need to accept your proposal into Clang in advance of the committee's decision. Arguably that would even be somewhat inappropriate, since it might be seen as an endorsement of your proposal over the alternatives, which I don't believe anyone from Clang has looked into. Letting the committee make a decision also addresses our disagreements about the language design and avoids introducing unnecessary divergence if the eventually-accepted design doesn't match your proposal.

Having a workable patch that you've taken advantage of in various projects should count as implementation experience for the committee's purposes.

If the committee *isn't* taking this up, they absolutely should, though.

If the committee is actively looking into this problem and considering multiple alternatives, I don't see a particular need to accept your proposal into Clang in advance of the committee's decision. Arguably that would even be somewhat inappropriate, since it might be seen as an endorsement of your proposal over the alternatives, which I don't believe anyone from Clang has looked into. Letting the committee make a decision also addresses our disagreements about the language design and avoids introducing unnecessary divergence if the eventually-accepted design doesn't match your proposal.

Having a workable patch that you've taken advantage of in various projects should count as implementation experience for the committee's purposes.

If the committee *isn't* taking this up, they absolutely should, though.

I see the situation as exactly the opposite of what you just said. We cannot get implementation experience without an implementation. I would like Clang to be that implementation. But if Clang refuses to implement anything that is not Standard C++, then we have a chicken-and-egg problem: nobody can experiment with TR on real codebases until a compiler supports it. I would like Clang to be that compiler.

I don't plan to pursue P1144 any more in the ISO C++ Committee until someone has succeeded in implementing it in a compiler and letting people like Mark Elendt actually use it. You can say "ISO ought to take this up" all you want, but if no vendor is willing to implement the feature even with a pull request in hand, it's hardly fair to ask ISO to save you by standardizing it _before_ the implementation experience is in. They should be spending their time standardizing existing practice. That WG21 has been really really bad about standardizing vaporware lately is *not* a good trend, and I do not plan to encourage the trend.

Can you explain what is the distinction you draw between a new vendor-specific attribute like D70469 [[clang::acquire_handle]] or D70520 [[clang::export_name]] or D60455 [[clang::sycl_kernel]], and a new vendor-specific attribute like D50119 [[clang::trivially_relocatable]]? What's the secret sauce that permitted those extensions even as this one has stalled for years?

If the committee is actively looking into this problem and considering multiple alternatives, I don't see a particular need to accept your proposal into Clang in advance of the committee's decision. Arguably that would even be somewhat inappropriate, since it might be seen as an endorsement of your proposal over the alternatives, which I don't believe anyone from Clang has looked into. Letting the committee make a decision also addresses our disagreements about the language design and avoids introducing unnecessary divergence if the eventually-accepted design doesn't match your proposal.

Having a workable patch that you've taken advantage of in various projects should count as implementation experience for the committee's purposes.

If the committee *isn't* taking this up, they absolutely should, though.

I see the situation as exactly the opposite of what you just said. We cannot get implementation experience without an implementation. I would like Clang to be that implementation. But if Clang refuses to implement anything that is not Standard C++, then we have a chicken-and-egg problem: nobody can experiment with TR on real codebases until a compiler supports it. I would like Clang to be that compiler.

We have implementation experience: it's been implemented, and you have some experience using it. And LLVM/clang is a github project now, so you can easily make a fork somewhere with your patch applied, and people can build and use that compiler to get additional user experience. What you're asking now is for us to officially declare this to be a supported feature by putting it in our upcoming releases. I personally am not very fond of the proposed language design, so I'm not inclined to make that declaration. However, I don't speak for the entire clang project, and if consensus among other contributors is to take it, I won't block that. And of course we'll take it if the committee standardizes it.

Can you explain what is the distinction you draw between a new vendor-specific attribute like D70469 [[clang::acquire_handle]] or D70520 [[clang::export_name]] or D60455 [[clang::sycl_kernel]], and a new vendor-specific attribute like D50119 [[clang::trivially_relocatable]]? What's the secret sauce that permitted those extensions even as this one has stalled for years?

We are generally more conservative about taking features that are squarely about core language semantics and thus of direct interest to the language committee. The acquire/release attributes enable an interesting static analysis that it's hard to imagine would ever be standardized. export_name is a target-specific attribute. sycl_kernel is a standard part of the independently-standardized SYCL language extension. In contrast, this proposal describes a new core property of types, with complex interactions with the core language rules for implicit special members, and which clearly ought to be adopted throughout the standard library. And, as mentioned, you haven't sold us on the design.

azat added a subscriber: azat.Nov 16 2022, 12:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 16 2022, 12:03 AM