This is an archive of the discontinued LLVM Phabricator instance.

[trivial-abi] Support types without a copy or move constructor.
Needs ReviewPublic

Authored by zoecarver on Nov 30 2020, 8:12 PM.

Details

Summary

The Itanium ABI specifies that...

A type is non-trivial for the purposes of a call if:

  • it has a non-trivial copy constructor, move constructor, or destructor, or
  • all of its copy and move constructors are deleted.

Currently, the trivial_abi attribute only "undoes" the first bullet but not the second. This patch would allow types with a deleted move and copy constructors to also be trivial (so long as the other conditions are met).

I stumbled upon this limitation in D66262, where we ran into a problem:

  1. The "trivial_abi" attribute requires a move constructor or a copy constructor.
  2. The standard requires that unique_ptr's "move constructor" doesn't participate in overload resolution unless a requirement is met. This requires a templated "move constructor" so that the function may be disabled.
  3. A move constructor is not permitted to be templated.

There are ways to solve this in the library without changing the trivial_abi attribute itself, but it revealed a set of use cases that should probably be supported regardless.

Thanks to @ahatanak for helping me understand this issue better. (And implementing the trivial_abi attribute in the first place.)

Refs D57626, r352822, r350920.

Diff Detail

Event Timeline

zoecarver created this revision.Nov 30 2020, 8:12 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: cfe-commits. · View Herald Transcript
zoecarver requested review of this revision.Nov 30 2020, 8:12 PM
zoecarver edited the summary of this revision. (Show Details)Nov 30 2020, 8:16 PM
zoecarver added a subscriber: ahatanak.
zoecarver added inline comments.
clang/include/clang/Basic/AttrDocs.td
2986–2988

I need to add back the back-ticks.

This seems like a good change, but we should make sure we test cases that may previously have been covered by the implicit deletion of all copy/move constructors. That may just be a matter of auditing tests.

clang/include/clang/Basic/AttrDocs.td
2986–2988

This also needs to be turned into a comma-separated list now that it has three clauses.

zoecarver added inline comments.Nov 30 2020, 8:33 PM
clang/include/clang/Basic/AttrDocs.td
2986–2988

The way I read this is as two clauses, so maybe the first "or" should become an "and." Because non-trivial destructors and non-trivial copy/move constructors sort of go together, and the deleted copy and move constructors are the second clause. But then again, maybe not because the "non-trivial destructor" part of it can apply to both. What do you think of:

A class annotated with `trivial_abi` can have non-trivial copy/move constructors or deleted copy and move constructors, and a non-trivial destructor without automatically becoming non-trivial for the purposes of calls.

And then the example clarifies that the attribute may be applied to a type with non-trivial copy/move constructors and a non-trivial destructor.

zoecarver updated this revision to Diff 308523.Nov 30 2020, 8:45 PM
zoecarver edited the summary of this revision. (Show Details)
  • Fix wording in AttrDocs
  • Add more tests
rjmccall added inline comments.Nov 30 2020, 9:02 PM
clang/include/clang/Basic/AttrDocs.td
2986–2988

How about "can have non-trivial (or completely deleted) copy/move constructors or a non-trivial destructor without..."?

zoecarver added inline comments.Nov 30 2020, 9:06 PM
clang/include/clang/Basic/AttrDocs.td
2986–2988

Perfect. I'll do that.

zoecarver updated this revision to Diff 308526.Nov 30 2020, 9:07 PM
  • Update wording in AttrDocs
rjmccall added inline comments.Nov 30 2020, 11:46 PM
clang/lib/Sema/SemaDeclCXX.cpp
6426

I don't think we can just unconditionally check for the attribute. The rule for trivial_abi is that it overrides direct sources of non-triviality, but if there's a subobject which can't be safely passed in registers, we'll still pass aggregates indirectly. This means we can safely add the attribute whenever a class's own special members don't rely on non-triviality without having to recursively check the component types.

zoecarver added inline comments.Dec 1 2020, 9:41 AM
clang/lib/Sema/SemaDeclCXX.cpp
6426

If there's a non-trivial field (or base) then the attribute won't be applied and a warning will be emitted. See the below tests (though, it would probably be good to add some more).

clang/test/SemaCXX/attr-trivial-abi.cpp
30

Here.

rjmccall added inline comments.Dec 1 2020, 9:51 AM
clang/lib/Sema/SemaDeclCXX.cpp
6426

Do we just remove it without a warning in template instantiation? At any rate, if we're relying on the attribute not being present when it isn't authoritative, we should say that here.

zoecarver added inline comments.Dec 1 2020, 9:55 AM
clang/lib/Sema/SemaDeclCXX.cpp
6426

Alright. I'll put a comment.

Do we just remove it without a warning in template instantiation?

That would appear to be what happens.

zoecarver updated this revision to Diff 308699.Dec 1 2020, 10:11 AM
  • Add comment in SemaDeclCXX.
  • Add tests for non-trivial bases/members.

In the following example, should S1 be passed directly or indirectly? The current patch passes it indirectly.

struct __attribute__((trivial_abi)) S0 {
  S0();
  S0(const S0 &) = delete;
  S0(S0 &&) = delete;
  int a;
};

struct S1 {
  S0 s0;
};

void foo1(S1);

void test1() {
  foo1(S1());
}

In contrast, S3 in the following example is passed directly.

struct __attribute__((trivial_abi)) S2 {
  S2();
  S2(const S2 &);
  S2(S2 &&);
  int a;
};

struct S3 {
  S2 s2;
};

void foo3(S3);

void test3() {
  foo3(S3());
}

Both S1 and S3 have a member whose type is annotated with trivial_abi. The only difference is that, in the first case, the copy and move constructors of member type S0 are both deleted.

That's an excellent example to study. The fundamental question we are looking to answer is whether a type representationally depends on its address. Absent the trivial_abi attribute, the criterion for knowing that it does not is whether it is possible to copy or move it trivially. If it is not possible to copy or move the type at all, it is not possible to copy or move it trivially. So S0 may representationally depend on its address, and we should ignore/diagnose trivial_abi on aggregates containing an S0.

Similarly, if a subobject type has only non-trivial copy/move constructors (but a trivial destructor), we should assume that it representationally depends on its address and prevent aggregates containing it from being passed directly, even if the containing type deletes all its copy/move constructors and uses trivial_abi.

If it is not possible to copy or move the type at all, it is not possible to copy or move it trivially.

This was a bit confusing to me because I think this changed between C++14 and 17. In C++14, a class was trivially copyable if it had no non-trivial copy/move constructors. But now the standard requires at least one non-trivial move/copy constructor. It looks like the type traits maybe haven't caught up.

So S0 may representationally depend on its address, and we should ignore/diagnose trivial_abi on aggregates containing an S0.

I guess the question is, does a type with all copy/move constructors deleted and the trivial_abi attribute depend on its address? And the whole point of this patch is to make that answer, "no." But, a type without the attribute, does depend on its address. And therefore a type holding S0 (without the attribute) also depends on its address because it implicitly has no copy/move constructors. I'm not sure I understand why we need to diagnose aggregates containing an S0 or why the trivial_abi attribute couldn't be applied to those types, though.

Similarly, if a subobject type has only non-trivial copy/move constructors (but a trivial destructor), we should assume that it representationally depends on its address and prevent aggregates containing it from being passed directly, even if the containing type deletes all its copy/move constructors and uses trivial_abi.

I concur with the first part of this, at least. That is, S3 should not be passed directly because it is non-trivially copyable (because of its member) and does not have the attribute. Similar to above, I don't understand why we couldn't pass it directly if it had the attribute, though.

If it is not possible to copy or move the type at all, it is not possible to copy or move it trivially.

This was a bit confusing to me because I think this changed between C++14 and 17. In C++14, a class was trivially copyable if it had no non-trivial copy/move constructors. But now the standard requires at least one non-trivial move/copy constructor. It looks like the type traits maybe haven't caught up.

Sorry, I know that the standard defines terms in this area, so what I said was unnecessarily confusing.

I'm describing the normative considerations that have gone into the language design of trivial_abi, not trying to state the rule in the standard. It doesn't especially matter what the standard says because this is not a standard feature. We want to fit in with the standard framework as much as possible because it simplifies the interactions, but ultimately if we don't think the standard permits what we want out of this extension, we'll just diverge.

So S0 may representationally depend on its address, and we should ignore/diagnose trivial_abi on aggregates containing an S0.

I guess the question is, does a type with all copy/move constructors deleted and the trivial_abi attribute depend on its address? And the whole point of this patch is to make that answer, "no." But, a type without the attribute, does depend on its address. And therefore a type holding S0 (without the attribute) also depends on its address because it implicitly has no copy/move constructors. I'm not sure I understand why we need to diagnose aggregates containing an S0 or why the trivial_abi attribute couldn't be applied to those types, though.

Okay, so, this is the problem with reasoning about this the C++ way. I apologize if what follows is a little long.

Types have representational needs, and those needs dictate the properties of correctly-implemented value operations. However, the C++ special members are designed around the C++ language model; we need to talk briefly about value operations on a deeper level. There are four basic value operations at this deeper level: original initialization, copy, destructive move, and destruction. Everything else can be thought of as a combination of these basic operations (like copy-assignment combining destruction and copy) or a twist on one (like C++'s non-destructive moves, which end up being a sort of optimized copy). The basic representational choices of a type dictate the properties of these four basic operations, and they in turn dictate the properties of optimally-implemented special members.

So, for example, types that manage ownership of some resource pretty much necessarily make initialization and destruction non-trivial, and copy must be non-trivial or impossible, but a true destructive move can remain non-trivial. (This is why C++'s moves are more like a copy than a move at this conceptual level.) Types with some sort of address sensitivity, like a relative or interior pointer, must make copy and destructive move non-trivial or impossible, but destruction can remain trivial (as can initialization, if you're willing to ignore a possibly invalid value). Types that register their objects externally must make all the operations either non-trivial or impossible, and so on.

What trivial_abi really does is make a statement about the type's destructive move, which is otherwise something that C++ can't talk about at all. Passing something in registers, or just in general copying its bits from one location to another and abandoning the old location, is a trivial destructive move, and trivial_abi says that that's okay for a type irrespective of the type's own special members as long it's okay for all its subobjects. So we have to look at every subobject and ask if it allows trivial destructive moves, and if it does then we can honor trivial_abi. That query basically turns into asking whether the subobjects are either trivially-copyable or themselves are marked trivial_abi.

Similarly, if a subobject type has only non-trivial copy/move constructors (but a trivial destructor), we should assume that it representationally depends on its address and prevent aggregates containing it from being passed directly, even if the containing type deletes all its copy/move constructors and uses trivial_abi.

I concur with the first part of this, at least. That is, S3 should not be passed directly because it is non-trivially copyable (because of its member) and does not have the attribute. Similar to above, I don't understand why we couldn't pass it directly if it had the attribute, though.

If you have a type that deletes all its copy/move constructors and does not declare itself trivial_abi, it is saying that it cannot be correctly copied/moved at all. If it deletes all its copy/move constructors and declares itself trivial_abi, it is saying that it can only be destructively moved, and furthermore that that operation is trivial. Making a containing type trivial_abi would make it destructively movable, which its subobject doesn't permit.

It seems like you are discussing the case where a class/struct annotated with trivial_abi contains a member that isn't destructively movable. In that case, clang correctly diagnoses it today. For example, if you remove the attribute from S2 in the above example and add it to S3 instead, it warns that trivial_abi cannot be applied to S3 because S2 is a non-trivial class type.

What I wasn't sure was whether S1 (which isn't annotated with trivial_abi in the original code I posted) should be treated as a destructively movable type despite having all its copy/move constructors deleted when its only member (s0) is destructively movable.

Based on the discussion we had a few years ago (http://lists.llvm.org/pipermail/cfe-dev/2017-November/055966.html), I think the answer is yes, but I just wanted to confirm.

It seems like you are discussing the case where a class/struct annotated with trivial_abi contains a member that isn't destructively movable. In that case, clang correctly diagnoses it today. For example, if you remove the attribute from S2 in the above example and add it to S3 instead, it warns that trivial_abi cannot be applied to S3 because S2 is a non-trivial class type.

What I wasn't sure was whether S1 (which isn't annotated with trivial_abi in the original code I posted) should be treated as a destructively movable type despite having all its copy/move constructors deleted when its only member (s0) is destructively movable.

Based on the discussion we had a few years ago (http://lists.llvm.org/pipermail/cfe-dev/2017-November/055966.html), I think the answer is yes, but I just wanted to confirm.

Hmm. My first instinct is to say that a type that "adds new information" about its special members — i.e. that explicitly declares its destructor or copy/move constructors for some reason other than to immediately explicitly default them — should always be treated conservatively in the absence of a trivial_abi attribute. So a type that explicitly deletes all its copy/move constructors should never be treated as destructively movable without the attribute. A type with implicitly-deleted copy/move constructors due solely to subobjects that are still destructively movable should still be destructively movable, I think.

@rjmccall Thanks for all that information. You're right; thinking about it in the context of four value operations is helpful.

Hmm. My first instinct is to say that a type that "adds new information" about its special members — i.e. that explicitly declares its destructor or copy/move constructors for some reason other than to immediately explicitly default them — should always be treated conservatively in the absence of a trivial_abi attribute. So a type that explicitly deletes all its copy/move constructors should never be treated as destructively movable without the attribute. A type with implicitly-deleted copy/move constructors due solely to subobjects that are still destructively movable should still be destructively movable, I think.

Just to be clear, this is the opposite of what we were talking about earlier. Earlier I was suggesting that a type without the trival_abi attribute (such as S1), and without any move/copy constructors because of a subobject (S0) that had the attribute, should not be destructively movable. But, I see merit in both sides of the argument, so I'm happy to do it either way.

So, if we're all in agreement, I'll update this patch so that S1 is passed directly.

You're right; thinking about it in the context of four value operations is helpful.

FWIW, I think dragging "value operations" into the mix is exactly wrong (and referring to "destructive move" is extra wrong, in the specific context of C++). For a C++ object, we do have language-level operations for "original initialization, copy, and destruction" (I'm willing to grant that "move" is just a special form of "copy"); but we don't have "destructive move," neither at the language level nor at the ABI level. [[trivial_abi]] isn't about language-level operations at all. It's about granting the compiler the freedom to make up and insert bitwise-teleportations of the object from point A to point B, even when the language spec says no operation is happening. So for example, in C++, if we have struct T { T() { print(this); } }; void f(T a) { print(&a); }, and we do f(T()), there is no "copy" happening — from C++'s point of view, there is only one T constructor called. However, the pointer value printed inside T() is permitted to be different from the pointer value printed inside f — basically, after the T object is "constructed," it gets bitwise-teleported to a different location in memory, without being copied, moved, or anything else at the language level. https://godbolt.org/z/e6hzPP

You can see that the code still compiles even when T's copy and move operations are deleted. https://godbolt.org/z/hfr14v This is because those operations are not happening, at the C++ language level. As far as C++ is concerned, we've got just one T object. We're not destroying-it-and-moving-it-elsewhere; that'd be the "relocation" operation described in my P1144 and D50119. A [[trivially_relocatable]] type permits the compiler to replace single pairs of "copy" and "destroy" operations with "bitwise-teleports." A [[trivial_abi]] type, on the other hand, permits the compiler to act at the ABI level and insert arbitrarily many "bitwise-teleport" operations even when the language spec calls for the object to remain in one place.

FWIW, (1) I doubt there's any reason to permit immovable objects to be [[trivial_abi]]; (2) I wish this attribute worked more like D50119 [[trivially_relocatable]] and less like D50119 [[maybe_trivially_relocatable]] (but I know @rjmccall feels exactly the opposite way). IMO, if the attribute simply meant "yes this thing is trivial for purposes of ABI and you can bitwise-teleport it whenever you like, full stop," then you wouldn't need to have these intricate discussions on precisely how triviality should be inherited in different situations — it would all "just work," as naturally as [[trivially_relocatable]].

There is no such thing as an object "teleporting" in C++. Objects are always observed in memory with a specific address. When an argument is passed in registers, its address can be observed to be different on both sides, and that is not permitted; there must be some operation that creates the object at the new address and destroys it at the old. That operation is a destructive move (which I certainly did not originate as a term for this), or what your proposal calls a relocation (which may be a more palatable term for the C++ committee).

As I think I've mentioned before, the only conceptual difference between an *accepted* trivial_abi attribute and your proposed attribute is that your attribute intentionally restricts when destructive moves can be performed so that adopting it doesn't constitute an ABI break. Your trait should certainly consider a trivial_abi type to be trivially relocatable.

I do think that understanding how representation choices restrict types and their operations is important to determining the right design here. I do understand that you would prefer a more unconditional, directive-like approach, but that is not the direction we have taken with trivial_abi.

There is no such thing as an object "teleporting" in C++. Objects are always observed in memory with a specific address. When an argument is passed in registers, its address can be observed to be different on both sides, and that is not permitted; there must be some operation that creates the object at the new address and destroys it at the old.

That's where you're wrong (about C++). You might be right about C or Objective-C, I don't know. In C++, that "teleporting" happens without any call to a special member — there is no move happening, and no destroy happening. You can actually observe this: https://godbolt.org/z/zojooc The object is simply "bitwise-teleported" from one place to another. Standard C++17 says that this is a "guaranteed-copy-elision" context; there is indeed only one C++ "object" here. It just happens to blit around in memory beyond what the C++ code is doing to it.

That operation is a destructive move (which I certainly did not originate as a term for this), or what your proposal calls a relocation (which may be a more palatable term for the C++ committee).

D50119 "relocation" is different; it's a way for the programmer to warrant an invariant about the behavior of the C++-language-level operations which you called "copy" and "destroy," and say that these operations can be replaced by the compiler as long as they're replaced in matching pairs. (A move and a destroy always end up "teleporting" the object from one place to another as a side effect; [[trivially_relocatable]] warrants that that pair of operations has no other side-effects worth preserving.) But D50119 "relocation" doesn't permit the compiler to introduce new "teleports" in places where the programmer hasn't written any operations at all. https://godbolt.org/z/zojooc is an example of code where the programmer hasn't written any operations at all — we have just one object — and yet, the compiler teleports it from place to place.

Your trait should certainly consider a trivial_abi type to be trivially relocatable.

The two attributes are essentially orthogonal, from the compiler's POV. One says it's okay to replace [move+destroy] with [teleport] but you must not introduce any unrequested teleports; the other says it's okay to introduce unrequested teleports but you must fully execute all moves and destroys. https://godbolt.org/z/EKbznG (Of course the other major difference in practice is that [[trivially_relocatable]] is all libc++ magic that manually replaces [move+destroy] pairs only in library code. So my use of libc++ std::swap in that demo is deliberate; you wouldn't see the [move+destroy] pairs getting removed by the compiler if you had written out swap's move-assign-assign-destroy dance by hand.)


However, I see there's yet another way to think about it, which is highly relevant to this PR! In P0593 "implicit-lifetime objects" conceptually spring into existence via the implementation secretly calling one of their constructors at an unspecified point; but this call is unobservable because the compiler magically selects a trivial constructor to call. Types with no trivial constructors can't be implicit-lifetime (IIUC). You could apply the same trick here — you could say that when a [[trivial_abi]] object "bitwise-teleports" from one place to another, it's not actually teleporting — the compiler is secretly inserting calls to [move+destroy] and then replacing the pair with a teleport. But this trick works only if the type is in fact movable and/or copyable! If the type has no move or copy operations, then you can't use this trick to explain what's going on.

Here's an example using Clang trunk [[trivial_abi]]: https://godbolt.org/z/jox9Er
U0 is trivial-abi, movable, non-copyable. U1 is trivial-abi, copyable, non-movable. U2 has a U0 data member, so it's movable and non-copyable, and it inherits trivial-abi. U3 has a U1 data member, so it's copyable and non-movable, and it inherits trivial-abi. But U4 has both U0 and U1 members; it wants to inherit trivial-abi, but it's neither movable nor copyable. If we're using the trick of saying that "the compiler is not inserting bitwise-teleports, it's inserting C++-language-level operations and then replacing them with teleports," then we can't use that trick here, because U4 has no language-level operations to secretly insert — in the same way that a type with no constructors can't be implicit-lifetime (IIUC).

So I think Zoe's PR here is basically forcing us to choose a model: is [[trivial_abi]] giving the compiler permission to insert unrequested bitwise-teleports directly at the ABI level (as the name implies), or is it inserting unrequested operations at the language level and then (á là [[trivially_relocatable]]) optimizing them back down to memcpy? If the former, the PR is good; if the latter, the PR is bad.

There is no such thing as an object "teleporting" in C++. Objects are always observed in memory with a specific address. When an argument is passed in registers, its address can be observed to be different on both sides, and that is not permitted; there must be some operation that creates the object at the new address and destroys it at the old.

That's where you're wrong (about C++). You might be right about C or Objective-C, I don't know. In C++, that "teleporting" happens without any call to a special member — there is no move happening, and no destroy happening. You can actually observe this: https://godbolt.org/z/zojooc The object is simply "bitwise-teleported" from one place to another. Standard C++17 says that this is a "guaranteed-copy-elision" context; there is indeed only one C++ "object" here. It just happens to blit around in memory beyond what the C++ code is doing to it.

I think perhaps we are talking past each other and have reached the limits of what this sub-thread can hope to achieve.

John.

I think perhaps we are talking past each other and have reached the limits of what this sub-thread can hope to achieve.

I agree. I am sure that we (or at least you two) could talk about the semantics of "teleportation" and "destructively moving" objects for many many hours. But I'm not sure it would be productive to do so (at least in this review thread). So, Arthur, why don't you open a mailing list thread to continue discussing if you'd like.

And we can all agree, no matter what we decide to call it, that in the following snippet, S0 and S1 will be passed directly:

struct __attribute__((trivial_abi)) S0 {
  S0();
  S0(const S0 &) = delete;
  S0(S0 &&) = delete;
  int a;
};

struct S1 {
  S0 s0;
};

(Which lines up with how this currently works if instead of being deleted, the copy/move constructors of S0 were simply user-provided.)

I think perhaps we are talking past each other and have reached the limits of what this sub-thread can hope to achieve.

I agree. I am sure that we (or at least you two) could talk about the semantics of "teleportation" and "destructively moving" objects for many many hours. But I'm not sure it would be productive to do so (at least in this review thread). So, Arthur, why don't you open a mailing list thread to continue discussing if you'd like.

And we can all agree, no matter what we decide to call it, that in the following snippet, S0 and S1 will be passed directly:

struct __attribute__((trivial_abi)) S0 {
  S0();
  S0(const S0 &) = delete;
  S0(S0 &&) = delete;
  int a;
};

struct S1 {
  S0 s0;
};

(Which lines up with how this currently works if instead of being deleted, the copy/move constructors of S0 were simply user-provided.)

Yes, I agree that these should be passed directly.

  • Add lots of tests for S0.
  • Implement discussed change for handling S0.
  • Update docs to reflect change.

Sorry for the slow update. This patch now implements the suggested change (to "inherit" the trivial-abi attribute if there are no user-defined special members and its copy/move constructor is deleted solely because of a subobject with the attribute). Let me know what you think/if there are any other review comments.

clang/lib/Sema/SemaDeclCXX.cpp
6518

I don't really love this implementation, to be honest. Mainly because it makes this function recursive and adds another special-case.

I tried a few other implementations, but none of those worked very well. Let me know if you have alternative implementation strategies that you prefer and I'm happy to use those instead.

ahatanak added inline comments.Dec 16 2020, 2:35 PM
clang/lib/Sema/SemaDeclCXX.cpp
6502

It looks like this patch changes the way D is passed in the following code:

struct B {
  int i[4];
  B();
  B(const B &) = default;
  B(B &&);
};

struct D : B {
  D();
  D(const D &) = default;
  D(D &&) = delete;
};

void testB(B a);
void testD(D a);

void testCallB() {
  B b;
  testB(b);
}

void testCallD() {
  D d;
  testD(d);
}

B cannot be passed in registers because it has a non-trivial move constructor, whereas D can be passed in registers because the move constructor is deleted and the copy constructor is trivial.

I'm not sure what the best way to handle this is, but I just wanted to point this out.

I think that as long as the class leaves a copy/move constructor defaulted, there's no need for a new trivial_abi attribute.

I think that as long as the class leaves a copy/move constructor defaulted, there's no need for a new trivial_abi attribute.

Sorry, I'm not sure I follow. Could you elaborate a bit or provide an example? What do you mean by "new" trivial_abi attribute?

clang/lib/Sema/SemaDeclCXX.cpp
6502

Hmm. Good catch. One way to fix this would be to simply create a HasPassableSubobject variable and add that to the conditions below (instead of returning false here). But, it seems that D isn't passed by registers (even though, maybe it should be) on ToT: https://godbolt.org/z/4xevW5

Given that, do you think it's OK to return false here, or should I update this patch to use the logic I just described (even though that would be a nfc)?

ahatanak added inline comments.Dec 16 2020, 9:07 PM
clang/lib/Sema/SemaDeclCXX.cpp
6502

The argument is byval, so D is passed directly. If you remove -O3 and add -target aarch64, you'll see that [2 x i64] is being passed

zoecarver added inline comments.Dec 16 2020, 9:11 PM
clang/lib/Sema/SemaDeclCXX.cpp
6502

Ah, I see now. Great. Thanks. I'll update the patch.

I think that as long as the class leaves a copy/move constructor defaulted, there's no need for a new trivial_abi attribute.

Sorry, I'm not sure I follow. Could you elaborate a bit or provide an example? What do you mean by "new" trivial_abi attribute?

Sorry, I mean that I think Akira's example should be passed directly. It shouldn't require its own trivial_abi attribute in order to get the treatment.

clang/lib/Sema/SemaDeclCXX.cpp
6502

Akira's example is legal C++ with no Clang-specific attributes, so its behavior is governed by the appropriate platform's ABI doc — there exists one correct answer.
At least on x86-64 with the Itanium ABI, GCC and ICC and Clang ToT all agree on the answer: B and D have exactly the same passing convention. If your patch breaks that, that's a problem.
Contrariwise, it appears that B and D have different passing conventions on "armv8" according to Clang, and the same passing convention on "ARM64" according to GCC: https://godbolt.org/z/j9jzYG
Of course if the programmer adds [[clang::trivial_abi]] to one of them, then all bets are off, standards-wise, and you're free to figure out a way to pass it in registers if you want to. But otherwise I think you have to follow what the ABI says.
Bear in mind that I don't really know ABIs other than Itanium/x86-64. Maybe the problem here is that other platforms don't have well-defined ABIs and so we get to make one up? Maybe everyone except me is already aware that that's what we're doing? :)

Oh yes, somehow I overlooked that, sorry.

I think that as long as the class leaves a copy/move constructor defaulted, there's no need for a new trivial_abi attribute.

Sorry, I'm not sure I follow. Could you elaborate a bit or provide an example? What do you mean by "new" trivial_abi attribute?

Sorry, I mean that I think Akira's example should be passed directly. It shouldn't require its own trivial_abi attribute in order to get the treatment.

No worries. I understand now. The problem we're discussing actually has nothing to do with the trivial-abi attribute. We just need to make sure that non-trival-abi types are not affected by this change (and it appears they were).

clang/lib/Sema/SemaDeclCXX.cpp
6502

At least on x86-64 with the Itanium ABI, GCC and ICC and Clang ToT all agree on the answer: B and D have exactly the same passing convention. If your patch breaks that, that's a problem.

IIUC (and that's a big "if") being trivial for the purposes of a call, does not mean it will _always_ be passed through registers. (For example, we'd never pass a 512-byte type through registers.) And, I don't think they do have the same calling convention, though they are both passed indirectly. As Akira rightly pointed out, on ToT D has the byval attribute indicating that it _could_ be passed directly (according to the ABI/standard at least). While it might not be an observable change in the assembly, my patch (or at least the current version on phab) removes the byval argument. I have a local version that fixes this, though, and I'll upload that shortly.

byval actually means it'll be passed on the stack, not as a pointer, so yes, that's a real change in conventions from an indirect argument.

zoecarver updated this revision to Diff 312629.Dec 17 2020, 3:46 PM
  • Fix base-derived case.
  • Rebase.

@ahatanak this patch should now be a nfc for types that don't have anything to do with the trivial-abi attribute.

This latest patch changes the way D is passed in the following example, which doesn't use trivial_abi at all:

struct B0 {
  int a;
  B0();
  B0(const B0 &) = default;
  B0(B0 &&) = delete;
};

struct B1 {
  int a;
  B1();
  B1(const B1 &) = delete;
  B1(B1 &&) = default;
};

struct D {
  B0 b0;
  B1 b1;
};

void testB0(B0 a) {
}

void testB1(B1 a) {
}

void testD(D a) {
}

D should be passed indirectly according to the existing rules, but this patch changes that.

I think we should first clarify or decide what the rules should be when trivial_abi is used on a class or one of its subobjects.

We could probably do something like what this patch is doing and determine whether a class can be passed in registers based on whether its subobjects can be passed in registers. If all of the subobjects can be passed in registers, the current class can be passed in registers too unless something declared in the current class forces it to be passed indirectly (e.g., a virtual function is declared).

Alternatively, if we don't want to diverge too much from the existing Itanium ABI rules, a class annotated with trivial_abi should be treated as if its copy/move constructors are trivial and aren't inaccessible nor deleted for the purpose of calls.

zoecarver added a comment.EditedDec 19 2020, 11:27 AM

We could probably do something like what this patch is doing and determine whether a class can be passed in registers based on whether its subobjects can be passed in registers. If all of the subobjects can be passed in registers, the current class can be passed in registers too unless something declared in the current class forces it to be passed indirectly (e.g., a virtual function is declared).

That's where I'm getting confused because on main both B0 and B1 are passed directly, so why isn't D also getting passed directly? There's nothing declared in that class other than the two members.

Edit: jeez, I just saw my mistake. I was just reading it as they both had a default copy ctor and deleted move ctor.

zoecarver updated this revision to Diff 324874.Feb 18 2021, 9:14 PM
  • Fix the case where an object inherits from two objects, one with a deleted copy constructor and the other with a deleted move constructor.

Sorry for the delay in updating. @ahatanak let me know if you've found another way to break this patch :P

zoecarver updated this revision to Diff 324875.Feb 18 2021, 9:25 PM
  • * Format with clang-format.
ldionne resigned from this revision.Sep 7 2023, 9:24 AM
ldionne added a subscriber: philnik.

@philnik you might be interested by this patch

Herald added a project: Restricted Project. · View Herald TranscriptSep 7 2023, 9:24 AM
Herald added a subscriber: wangpc. · View Herald Transcript